feat: enhance background folder watcher and improve UX/stability#22
Merged
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
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 52 minutes and 52 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough設定の永続化（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 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
… 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app.go (1)
382-401:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winエラー時は空文字列を返すべきです。
runtime.OpenDirectoryDialogがエラーを返した場合でもpathをそのまま返しています。Wailsのドキュメントによると、キャンセル時は空文字列でエラーなし、実際のエラー時はpathの値が未定義の可能性があります。エラー時は明示的に空文字列を返してください。🐛 修正案
func (a *App) SelectWatchFolder() string { path, err := runtime.OpenDirectoryDialog(a.ctx, runtime.OpenDialogOptions{ Title: "Select Watch Folder", }) if err != nil { log.Println("Error opening directory dialog:", err) + return "" } return path } func (a *App) SelectExportFolder() string { path, err := runtime.OpenDirectoryDialog(a.ctx, runtime.OpenDialogOptions{ Title: "Select Export Folder", }) if err != nil { log.Println("Error opening directory dialog:", err) + return "" } return path }🤖 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 382 - 401, The SelectWatchFolder and SelectExportFolder functions currently return path even when runtime.OpenDirectoryDialog returns an error; update both functions (SelectWatchFolder and SelectExportFolder) so that after calling OpenDirectoryDialog you check if err != nil, log the error as currently done, and explicitly return an empty string ("") on error instead of returning path, ensuring callers never receive an undefined/non-deterministic path value when an error occurs.
♻️ Duplicate comments (1)
settings.go (1)
132-138: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winルートディレクトリ判定ロジックに欠陥があります。
watchReal == sep && strings.HasPrefix(exportReal, sep)の条件は、Unix系OSではすべての絶対パスが/で始まるため、WatchFolderがルート/の場合に任意のExportFolderを誤って拒否します（意図通り）。しかし、実際にはこの条件は「watchRealがルートならexportRealは必ず子」という判定として正しく機能しています。問題は、通常のケース（どちらもルートでない場合）で
strings.HasPrefix(exportReal, watchReal+sep)だけに依存している点です。これは過去のレビューで指摘されたfilepath.Relベースの判定を採用していないため、エッジケースで脆弱です。♻️ filepath.Rel を使った堅牢な実装
+ isSubdir := func(base, target string) bool { + rel, err := filepath.Rel(base, target) + return err == nil && + rel != "." && + !strings.HasPrefix(rel, "..") + } + if watchReal == exportReal { return "Error: Export folder cannot be the same as the Watch folder." } - sep := string(filepath.Separator) - if (watchReal == sep && strings.HasPrefix(exportReal, sep)) || strings.HasPrefix(exportReal, watchReal+sep) { + if isSubdir(watchReal, exportReal) { return "Error: Export folder cannot be a subdirectory of the Watch folder." } - if (exportReal == sep && strings.HasPrefix(watchReal, sep)) || strings.HasPrefix(watchReal, exportReal+sep) { + 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 - 138, The current HasPrefix-based checks around watchReal and exportReal are fragile; replace them with filepath.Rel-based checks to robustly detect when one path is a subdirectory of the other: use filepath.Rel(watchReal, exportReal) and filepath.Rel(exportReal, watchReal), handle returned errors, treat a relation of "." or a non-empty rel that does not start with ".."+string(os.PathSeparator) as "subdir" (and also treat rel == "." as same directory), and then return the existing error messages from the branches that detect exportReal inside watchReal or vice versa; update the logic that references watchReal, exportReal, sep and strings.HasPrefix to use these filepath.Rel checks instead.
🤖 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 `@settings.go`:
- Around line 155-165: The rollback path currently ignores possible errors from
saveSettings() and a.updateWatcher(oldSettings.WatchFolder); modify the
error-recovery block in the if oldSettings.WatchFolder != s.WatchFolder branch
so that you capture the returned errors from saveSettings() and from
a.updateWatcher(oldSettings.WatchFolder) and log them (using the package logger
or log.Printf) with clear messages including the operation and the error; keep
the existing non-panicking behavior but ensure any failures in saveSettings() or
restoring the watcher are logged for debugging, referencing settingsMu,
currentSettings, oldSettings, saveSettings(), and a.updateWatcher().
---
Outside diff comments:
In `@app.go`:
- Around line 382-401: The SelectWatchFolder and SelectExportFolder functions
currently return path even when runtime.OpenDirectoryDialog returns an error;
update both functions (SelectWatchFolder and SelectExportFolder) so that after
calling OpenDirectoryDialog you check if err != nil, log the error as currently
done, and explicitly return an empty string ("") on error instead of returning
path, ensuring callers never receive an undefined/non-deterministic path value
when an error occurs.
---
Duplicate comments:
In `@settings.go`:
- Around line 132-138: The current HasPrefix-based checks around watchReal and
exportReal are fragile; replace them with filepath.Rel-based checks to robustly
detect when one path is a subdirectory of the other: use filepath.Rel(watchReal,
exportReal) and filepath.Rel(exportReal, watchReal), handle returned errors,
treat a relation of "." or a non-empty rel that does not start with
".."+string(os.PathSeparator) as "subdir" (and also treat rel == "." as same
directory), and then return the existing error messages from the branches that
detect exportReal inside watchReal or vice versa; update the logic that
references watchReal, exportReal, sep and strings.HasPrefix to use these
filepath.Rel checks instead.
🪄 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: d9f7a096-46d7-4422-a045-fb605ae2586e
📒 Files selected for processing (3)
app.gosettings.gowatcher.go
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai resume |
Contributor
✅ Actions performedReviews resumed. |
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 )