feat: enhance background folder watcher and improve UX/stability#22
Open
amemya wants to merge 4 commits into
Open
feat: enhance background folder watcher and improve UX/stability#22amemya wants to merge 4 commits into
amemya wants to merge 4 commits into
Conversation
- Refactor OnBeforeClose behavior to restore native macOS window close functionality - Fix fsnotify watcher goroutine leaks by properly syncing Close() and app shutdown hooks - Fix infinite loop in SaveSettings by validating bidirectional Watch/Export folder hierarchy - Optimize file reading in processBackgroundFile and OpenImage to eliminate redundant disk I/O - Debounce background processing effectively by checking for file size stabilization - Improve macOS UX by extracting the Application Menu to buildMenu and supporting Escape to close preferences - Add osascript native macOS notifications for background save success, with AppleScript injection defense - Migrate Watch/Export folder configuration in React to use Wails native directory selection dialogs - Resolve React strict-mode issues with EventsOn/EventsOff and fix stale closures in useEffect dependencies - Add proper error handling for settings JSON marshalling and unmarshalling - Remove unused icon.go and dead React states
Contributor
📝 WalkthroughSummary by CodeRabbit新機能
改善
Walkthrough設定の永続化（Watch/Export 等）、fsnotify によるフォルダ監視、背景処理→自動保存トークン経由の保存パイプ、フロントエンドのキャンバス描画と Preferences モーダル、OS別メニュー/通知を追加します。 Changes設定、ファイルウォッチング、自動保存パイプライン
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant App
participant Watcher
participant Settings
participant Canvas
participant SaveHandler
User->>Frontend: open settings / select folders
Frontend->>App: GetSettings()
App->>Settings: read currentSettings
Settings-->>Frontend: Settings
Frontend->>App: SaveSettings(updated)
App->>Settings: validate & persist
App->>Watcher: updateWatcher(newFolder)
Watcher->>Watcher: detect new image file
Watcher->>App: processBackgroundFile(path)
App->>App: processImageFile -> doOpenImage
App->>Frontend: emit process_file(result, exportFolder)
Frontend->>Canvas: renderImageToCanvas(result.imageURL)
Canvas-->>Frontend: toBlob(ArrayBuffer)
Frontend->>App: SaveAutoImage(isPng, savePath)
App->>SaveHandler: handler.prepareSave -> token
Frontend->>SaveHandler: POST /api/save with ArrayBuffer + token
SaveHandler-->>Frontend: success
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances ExifFrame’s background auto-processing pipeline and preferences UX by adding a folder watcher, persisted settings, automated export saving, and macOS-centric UI integrations (menu + notifications) within the Wails desktop app.
Changes:
- Add fsnotify-based background watch folder processing that emits
process_fileevents to the frontend. - Introduce persisted settings (watch/export folders + rendering preferences) with frontend Preferences modal using native directory pickers.
- Add macOS application menu integration and native save notifications via
osascript, plus refactors to reduce redundant file I/O.
Reviewed changes
Copilot reviewed 10 out of 15 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| watcher.go | Adds/controls fsnotify watcher and emits background processing events to the frontend |
| settings.go | Implements settings persistence + validation and triggers watcher restart on changes |
| app.go | Refactors image open path to reuse processing; adds auto-save and directory selection APIs |
| main.go | Adds cross-platform menu building with macOS Preferences integration and shutdown hook |
| handler.go | Adds macOS notification on save completion |
| go.mod / go.sum | Adds fsnotify dependency |
| frontend/src/App.tsx | Adds Preferences modal, settings load/save, and background auto-save pipeline |
| frontend/src/App.css | Styles the Preferences modal |
| frontend/wailsjs/runtime/runtime.js | Adds runtime JS bindings used by the React app |
| frontend/wailsjs/runtime/runtime.d.ts | Adds runtime TypeScript definitions (note: must match JS exports) |
| frontend/wailsjs/runtime/package.json | Packages runtime bindings metadata |
| frontend/wailsjs/go/models.ts | Adds Settings model in generated bindings |
| frontend/wailsjs/go/main/App.js | Updates generated App bindings (new backend methods) |
| frontend/wailsjs/go/main/App.d.ts | Updates generated App typings (new backend methods) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+295
to
+299
| if (!isEventsRegistered.current) { | ||
| EventsOn("open_settings", () => { | ||
| console.log("open_settings event received"); | ||
| setShowSettings(true); | ||
| }); |
Comment on lines
+3
to
+10
| import ( | ||
| "encoding/json" | ||
| "log" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| "sync" | ||
| ) |
Comment on lines
+90
to
+103
| // Validate folder hierarchy to prevent infinite loop (Task 9) | ||
| if s.WatchFolder != "" && s.ExportFolder != "" { | ||
| watchClean := filepath.Clean(s.WatchFolder) | ||
| exportClean := filepath.Clean(s.ExportFolder) | ||
| if watchClean == exportClean { | ||
| return "Error: Export folder cannot be the same as the Watch folder." | ||
| } | ||
| if strings.HasPrefix(exportClean, watchClean+string(filepath.Separator)) { | ||
| return "Error: Export folder cannot be a subdirectory of the Watch folder." | ||
| } | ||
| if strings.HasPrefix(watchClean, exportClean+string(filepath.Separator)) { | ||
| return "Error: Watch folder cannot be a subdirectory of the Export folder." | ||
| } | ||
| } |
Comment on lines
+251
to
+259
| // Show a native notification on macOS | ||
| if runtime.GOOS == "darwin" { | ||
| fileName := filepath.Base(savePath) | ||
| // Prevent AppleScript injection | ||
| fileName = strings.ReplaceAll(fileName, `\`, `\\`) | ||
| fileName = strings.ReplaceAll(fileName, `"`, `\"`) | ||
| msg := "Saved " + fileName | ||
| exec.Command("osascript", "-e", `display notification "`+msg+`" with title "ExifFrame"`).Run() | ||
| } |
Comment on lines
+29
to
+34
| configDir, err := os.UserConfigDir() | ||
| if err == nil { | ||
| appDir := filepath.Join(configDir, "ExifFrame") | ||
| os.MkdirAll(appDir, 0755) | ||
| settingsFile = filepath.Join(appDir, "settings.json") | ||
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.go`:
- Around line 357-360: SelectWatchFolder and SelectExportFolder currently
discard the error returned by runtime.OpenDirectoryDialog, conflating
user-cancel with real failures; change both functions (the calls to
runtime.OpenDirectoryDialog) to capture the returned error, check if err != nil
and log it (or return it) instead of ignoring it — add a log import to app.go if
necessary and use process-appropriate logging in the error branch so real errors
are preserved and distinguishable from an empty path caused by cancelation.
- Around line 338-342: The current check using filepath.Clean and
strings.HasPrefix can be bypassed by symlinks; update the validation in the
SaveAutoImage/save path logic to resolve symlinks and compare real paths: call
filepath.EvalSymlinks on both savePath and exportFolder (the variables used
where you build cleanSave/cleanExport), then use filepath.Rel to compute the
relative path and ensure it does not begin with ".." (meaning the save is
outside the export folder); return the same SaveResult error when the resolved
relative path escapes the export folder. Ensure you handle and propagate any
EvalSymlinks errors (treat them as validation failures) and reference the
existing SaveResult return path in the function.
In `@frontend/src/App.css`:
- Around line 388-395: .modal-content uses a fixed width (width: 400px) causing
clipping on small viewports; update the .modal-content rule to use a responsive
width and constrained dimensions (e.g., width: 90% with max-width: 400px and
max-height: 80vh) and enable internal scrolling (overflow: auto) so content
doesn’t overflow on narrow or short windows while preserving the 400px cap.
In `@frontend/src/App.tsx`:
- Around line 185-197: The background-save callback handling the
offscreenCanvas.toBlob result treats any fetch response as success; in the
offscreenCanvas.toBlob callback (the async function that calls SaveAutoImage and
fetch with resultSave.saveToken and savePath) check the fetch response status by
awaiting the fetch call into a variable and asserting response.ok; if not ok,
log or throw an error (include response.status/text) and avoid printing
"Background save complete" on failure; ensure this error path is handled
consistently with the surrounding try/catch so failures are surfaced rather than
reported as success.
- Around line 278-293: The effect that calls GetSettings() only clears the
initial-load guard when the promise resolves, so a failed GetSettings() leaves
isInitialLoad.current true and prevents SaveSettings from ever running; update
the useEffect that calls GetSettings() to ensure the setTimeout that sets
isInitialLoad.current = false runs regardless of success or failure by using a
try/catch/finally (or .catch/.finally) around GetSettings() and moving the
setTimeout into the finally block, keeping the existing setXYZ calls
(setWatchFolder, setExportFolder, setAspectRatioPreset, setCustomRatioW,
setCustomRatioH, setOrientation, setAlignment) in the success path and
preserving the 100ms delay and timeout semantics.
In `@handler.go`:
- Around line 251-259: The osascript call in handleSave blocks the /api/save
response because exec.Command(...).Run() is executed synchronously and its error
is ignored; change this to create a context with a short timeout and call
exec.CommandContext (or otherwise move the call to a goroutine/run it after
w.WriteHeader) so the HTTP response is sent immediately, and capture and log any
error from the command instead of ignoring it; specifically update the block
that builds the message and calls exec.Command("osascript", ...).Run() to use
context.WithTimeout + exec.CommandContext (or spawn a goroutine) and process the
returned error via your logger (e.g., processLogger or the request-scoped
logger) rather than blocking the handler.
In `@main.go`:
- Line 16: The build fails because the //go:embed all:frontend/dist directive in
main.go references files that don't exist; either ensure the frontend/dist
artifact is produced before the Go build (add a CI step that runs the frontend
build and outputs frontend/dist), or change the embedding strategy to target
only existing files (e.g., embed specific patterns like //go:embed
frontend/dist/* or remove/embed conditionally and fallback to runtime FS) so
compilation doesn't fail when frontend/dist is absent; update main.go
accordingly to use the chosen approach.
In `@settings.go`:
- Around line 64-79: Change saveSettings() to return an error instead of
swallowing failures: have saveSettings check settingsFile, marshal
currentSettings under settingsMu, attempt os.WriteFile and return any
encountered error (including marshal errors). Update the caller SaveSettings()
to call the new saveSettings() and use its returned error to set the response
string/return value (e.g., return success only when err == nil, otherwise return
an error message). Refer to saveSettings(), SaveSettings(), settingsMu,
currentSettings and settingsFile when making the changes.
- Around line 47-60: loadSettings currently unmarshals into a zero-value temp
which overwrites defaults when loading older settings; instead initialize temp
with the existing defaults/currentSettings (e.g., temp := currentSettings or a
default Settings value) before calling json.Unmarshal so only fields present in
the file replace defaults, then lock settingsMu and assign currentSettings =
temp; keep the error handling and file-read logic the same and still return on
unmarshal error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fecca53a-7fb7-49cd-8796-60ff316f3142
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
app.gofrontend/src/App.cssfrontend/src/App.tsxfrontend/wailsjs/go/main/App.d.tsfrontend/wailsjs/go/main/App.jsfrontend/wailsjs/go/models.tsfrontend/wailsjs/runtime/package.jsonfrontend/wailsjs/runtime/runtime.d.tsfrontend/wailsjs/runtime/runtime.jsgo.modhandler.gomain.gosettings.gowatcher.go
| wailsruntime "github.com/wailsapp/wails/v2/pkg/runtime" | ||
| ) | ||
|
|
||
| //go:embed all:frontend/dist |
Contributor
There was a problem hiding this comment.
go:embed の対象欠落でビルドが失敗します。
Line [16] の all:frontend/dist は対象が存在しないとコンパイルエラーになり、提示された静的解析でも no matching files found が出ています。CI で Go ビルド前に frontend/dist を必ず生成するか、埋め込み戦略を調整してください。
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 16-16: pattern all:frontend/dist: no matching files found
(typecheck)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.go` at line 16, The build fails because the //go:embed all:frontend/dist
directive in main.go references files that don't exist; either ensure the
frontend/dist artifact is produced before the Go build (add a CI step that runs
the frontend build and outputs frontend/dist), or change the embedding strategy
to target only existing files (e.g., embed specific patterns like //go:embed
frontend/dist/* or remove/embed conditionally and fallback to runtime FS) so
compilation doesn't fail when frontend/dist is absent; update main.go
accordingly to use the chosen approach.
… optimize UI responsiveness and notifications
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
settings.go (1)
124-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick win保存失敗時に live settings だけ更新されています。
Line 126 で
currentSettingsを先に差し替えているため、saveSettings()が失敗してもGetSettings()は新しい値を返し、updateWatcher()だけは未実行の中途半端な状態になります。永続化成功後に反映するか、失敗時にロールバックしてください。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@settings.go` around lines 124 - 132, The code updates currentSettings before persisting, causing GetSettings() to return new values even if saveSettings() fails and leaving updateWatcher() unrun; fix by persisting first or rolling back on error: either call saveSettings() using the new settings (s) while still holding settingsMu and only assign currentSettings = s and call updateWatcher() after saveSettings() returns nil, or keep the current assignment but on saveSettings() error restore currentSettings = oldWatch (or original settings snapshot) and ensure updateWatcher() is invoked/rolled back accordingly; use the existing symbols settingsMu, currentSettings, saveSettings(), oldWatch, GetSettings(), and updateWatcher() to locate and implement the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.go`:
- Around line 344-348: The current EvalSymlinks call on saveDir rejects new
subfolders because EvalSymlinks fails if the parent doesn't exist; change the
validation in the save path handling so you locate the nearest existing ancestor
of saveDir (walk up using filepath.Dir until you find an existing directory via
os.Stat) and call filepath.EvalSymlinks only on that existing ancestor (store
result in realSaveDirBase), then reconstruct/validate the intended final path
relative to that resolved base; do not call MkdirAll here—defer actual directory
creation to the save routine—and return SaveResult errors only for genuine
invalid resolved-base errors (keep references to savePath, saveDir,
realSaveDir/realSaveDirBase, filepath.EvalSymlinks, and MkdirAll to find the
affected code).
In `@settings.go`:
- Around line 111-120: The current checks use cleaned string paths which can be
bypassed by symlinks; resolve the actual filesystem paths for s.WatchFolder and
s.ExportFolder before comparing (e.g., call filepath.EvalSymlinks on each, then
filepath.Clean and pass those into normalizePathForCompare) so the comparisons
in the block using watchNorm/exportNorm detect real parent/child relationships
the same way SaveAutoImage does; update the code that sets watchNorm and
exportNorm to use the resolved real paths and handle EvalSymlinks errors
appropriately.
---
Outside diff comments:
In `@settings.go`:
- Around line 124-132: The code updates currentSettings before persisting,
causing GetSettings() to return new values even if saveSettings() fails and
leaving updateWatcher() unrun; fix by persisting first or rolling back on error:
either call saveSettings() using the new settings (s) while still holding
settingsMu and only assign currentSettings = s and call updateWatcher() after
saveSettings() returns nil, or keep the current assignment but on saveSettings()
error restore currentSettings = oldWatch (or original settings snapshot) and
ensure updateWatcher() is invoked/rolled back accordingly; use the existing
symbols settingsMu, currentSettings, saveSettings(), oldWatch, GetSettings(),
and updateWatcher() to locate and implement the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17485f33-e433-4cc5-b6a1-93dd9ad1ee1c
📒 Files selected for processing (6)
ExifFrameapp.gofrontend/src/App.cssfrontend/src/App.tsxhandler.gosettings.go
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app.go`:
- Around line 366-368: 現在の脱出判定が strings.HasPrefix(rel, "..") だと "..archive"
のような合法的な子ディレクトリを誤検知するので、filepath.Rel の結果で拒否する条件を「rel がちょうど \"..\" である」か「rel が
\"..\" + パス区切り文字で始まる」場合に限定して下さい（例: rel == ".." || strings.HasPrefix(rel,
".."+string(filepath.Separator))）。該当箇所は rel を調べているブロック（rel, err :=
filepath.Rel(...) とその直後の if ブロック）を更新し、拒否時は従来どおり SaveResult{Error: ...}
を返すようにしてください。
In `@settings.go`:
- Around line 154-156: SaveSettings currently calls updateWatcher(s.WatchFolder)
after persisting without checking for errors, so failures in watcher.go (e.g.,
fsnotify.NewWatcher or watcher.Add) are ignored and the frontend still shows
"Settings saved"; change updateWatcher to return an error, have SaveSettings
call it and if it fails return that error (and revert s.WatchFolder to
oldSettings.WatchFolder or rollback the persisted WatchFolder), ensuring
failures in watcher initialization (fsnotify.NewWatcher / watcher.Add in
watcher.go) are propagated back to the caller and cause SaveSettings to fail.
- Around line 132-135: The current subdirectory checks using
strings.HasPrefix(exportReal, watchReal+string(filepath.Separator)) miss cases
when watchReal or exportReal is root ("/") because concatenating an extra
separator yields "//"; update the logic in settings.go around the
watchReal/exportReal checks to first ensure both paths are cleaned (use
filepath.Clean if not already) and add explicit root handling: treat watchReal
== string(filepath.Separator) as matching any exportReal that starts with
string(filepath.Separator), and likewise treat exportReal ==
string(filepath.Separator) as matching any watchReal that starts with
string(filepath.Separator); i.e., change the conditions around strings.HasPrefix
so they also check for equality to filepath.Separator before relying on the
HasPrefix(w+sep) pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a3ac8c61-b22d-49d7-b54d-43c60a294b00
📒 Files selected for processing (3)
.gitignoreapp.gosettings.go
Comment on lines
+366
to
+368
| rel, err := filepath.Rel(realExport, realAncestor) | ||
| if err != nil || strings.HasPrefix(rel, "..") { | ||
| return SaveResult{Error: "Save path is outside of the allowed export folder"} |
Contributor
There was a problem hiding this comment.
.. で始まる合法な子ディレクトリも誤検知します。
savePath が <export>/..archive/photo.jpg のようなケースだと、rel は "..archive" になり、export 配下なのにここで拒否されます。脱出判定は .. 自体と ../... だけに絞ってください。
修正案
- if err != nil || strings.HasPrefix(rel, "..") {
+ if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
return SaveResult{Error: "Save path is outside of the allowed export folder"}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app.go` around lines 366 - 368, 現在の脱出判定が strings.HasPrefix(rel, "..") だと
"..archive" のような合法的な子ディレクトリを誤検知するので、filepath.Rel の結果で拒否する条件を「rel がちょうど \"..\"
である」か「rel が \"..\" + パス区切り文字で始まる」場合に限定して下さい（例: rel == ".." ||
strings.HasPrefix(rel, ".."+string(filepath.Separator))）。該当箇所は rel
を調べているブロック（rel, err := filepath.Rel(...) とその直後の if ブロック）を更新し、拒否時は従来どおり
SaveResult{Error: ...} を返すようにしてください。
Comment on lines
+132
to
+135
| if strings.HasPrefix(exportReal, watchReal+string(filepath.Separator)) { | ||
| return "Error: Export folder cannot be a subdirectory of the Watch folder." | ||
| } | ||
| if strings.HasPrefix(watchReal, exportReal+string(filepath.Separator)) { |
Contributor
There was a problem hiding this comment.
ルートディレクトリ配下の判定が抜けます。
watchReal か exportReal が / のとき、ここは "//" との前方一致になってしまうので、/tmp/out のような実際の子パスを検出できません。結果として、ルートを選ばれると今回の無限ループ防止を迂回できます。
修正案
+ isSubdir := func(base, target string) bool {
+ rel, err := filepath.Rel(base, target)
+ return err == nil &&
+ rel != "." &&
+ rel != ".." &&
+ !strings.HasPrefix(rel, ".."+string(filepath.Separator))
+ }
+
if watchReal == exportReal {
return "Error: Export folder cannot be the same as the Watch folder."
}
- if strings.HasPrefix(exportReal, watchReal+string(filepath.Separator)) {
+ if isSubdir(watchReal, exportReal) {
return "Error: Export folder cannot be a subdirectory of the Watch folder."
}
- if strings.HasPrefix(watchReal, exportReal+string(filepath.Separator)) {
+ if isSubdir(exportReal, watchReal) {
return "Error: Watch folder cannot be a subdirectory of the Export folder."
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@settings.go` around lines 132 - 135, The current subdirectory checks using
strings.HasPrefix(exportReal, watchReal+string(filepath.Separator)) miss cases
when watchReal or exportReal is root ("/") because concatenating an extra
separator yields "//"; update the logic in settings.go around the
watchReal/exportReal checks to first ensure both paths are cleaned (use
filepath.Clean if not already) and add explicit root handling: treat watchReal
== string(filepath.Separator) as matching any exportReal that starts with
string(filepath.Separator), and likewise treat exportReal ==
string(filepath.Separator) as matching any watchReal that starts with
string(filepath.Separator); i.e., change the conditions around strings.HasPrefix
so they also check for equality to filepath.Separator before relying on the
HasPrefix(w+sep) pattern.
Comment on lines
+154
to
+156
| if oldSettings.WatchFolder != s.WatchFolder { | ||
| a.updateWatcher(s.WatchFolder) | ||
| } |
Contributor
There was a problem hiding this comment.
ウォッチャ再起動の失敗が保存結果に反映されません。
ここは保存成功後に updateWatcher() を呼ぶだけなので、watcher.go 側で fsnotify.NewWatcher / watcher.Add が失敗しても SaveSettings() は空文字を返し、フロントは「Settings saved」を表示します。監視開始に失敗したらエラーを返せるようにして、必要なら WatchFolder もロールバックしてください。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@settings.go` around lines 154 - 156, SaveSettings currently calls
updateWatcher(s.WatchFolder) after persisting without checking for errors, so
failures in watcher.go (e.g., fsnotify.NewWatcher or watcher.Add) are ignored
and the frontend still shows "Settings saved"; change updateWatcher to return an
error, have SaveSettings call it and if it fails return that error (and revert
s.WatchFolder to oldSettings.WatchFolder or rollback the persisted WatchFolder),
ensuring failures in watcher initialization (fsnotify.NewWatcher / watcher.Add
in watcher.go) are propagated back to the caller and cause SaveSettings to fail.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(Resolved #1 )