fix(locations): register watcher on location add, canonicalize paths#3043
Merged
jamiepine merged 4 commits intospacedriveapp:mainfrom Mar 26, 2026
Merged
fix(locations): register watcher on location add, canonicalize paths#3043jamiepine merged 4 commits intospacedriveapp:mainfrom
jamiepine merged 4 commits intospacedriveapp:mainfrom
Conversation
Two upstream bugs fixed: 1. LocationManager::add_location() stored paths as-is without canonicalization. Relative paths (e.g. from cwd-dependent contexts) broke the watcher, volume manager, and indexer pipelines. Now calls tokio::fs::canonicalize() on local physical paths before storing, with UNC prefix stripping on Windows. 2. LocationAddAction::execute() never registered new locations with the FsWatcherService. The watcher only discovered locations at startup via load_library_locations(). Any location added at runtime had no filesystem monitoring — creates, deletes, and renames went undetected. Now calls fs_watcher.watch_location() after successful creation.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCanonicalizes local physical SD paths asynchronously when adding locations (Windows extended-prefix stripped) and registers newly added locations with the filesystem watcher; watcher (un)watch errors are logged and do not change action results. Changes
Sequence DiagramsequenceDiagram
actor User
participant Action as LocationAddAction
participant Manager as LocationManager
participant FS as Filesystem
participant Watcher as FileSystemWatcher
User->>Action: execute(add location)
Action->>Manager: add_location(sd_path)
alt sd_path is local physical
Manager->>FS: tokio::fs::canonicalize(path)
FS-->>Manager: canonical_path / error
opt canonical_path (Windows)
Manager->>Manager: strip_windows_extended_prefix(canonical_path)
end
end
Manager-->>Action: location_id
opt action has local path
Action->>FS: tokio::fs::canonicalize(local_path)
FS-->>Action: canonical_path / error
opt canonical_path (Windows)
Action->>Action: strip_windows_extended_prefix(canonical_path)
end
Action->>Watcher: watch_location(LocationMeta{id, library_id, root_path, rule_toggles})
alt watch success
Watcher-->>Action: ok
else watch error
Watcher-->>Action: error (logged, non-fatal)
end
end
Action-->>User: LocationAddOutput
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
7 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/location/manager.rs`:
- Around line 61-71: The Windows UNC handling in the cfg(windows) block around
the canonical variable must recognize the UNC form "\\?\UNC\server\share\..."
and reconstruct it as a valid UNC path "\\server\share\..." before falling back
to stripping a local-drive "\\?\" prefix; update the logic in the canonical
normalization (the block that currently uses canonical.to_string_lossy() and
strip_prefix(r"\\?\")) to first check strip_prefix(r"\\?\UNC\") and if present
create a PathBuf from "\\" + stripped, otherwise strip r"\\?\" for local drives,
else return canonical; mirror the approach used in core/src/volume/fs/refs.rs to
ensure starts_with("\\\\") checks in classification.rs work correctly.
In `@core/src/ops/locations/add/action.rs`:
- Around line 108-126: The watcher is being registered with a canonicalized path
that still may contain the Windows extended prefix, causing mismatches with the
persisted path; update the block that builds root_path (inside the if let
Some(local_path) ... and before creating LocationMeta and calling
fs_watcher.watch_location) to apply the same Windows path normalization used by
location_manager.add_location() — i.e., strip the Windows "\\?\" extended prefix
(or otherwise normalize to the exact form saved to the DB) after
tokio::fs::canonicalize returns and before constructing LocationMeta, so that
LocationMeta.root_path matches the normalized DB path used by the persistent
handler.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 386339bf-614c-4c9f-8672-fbf9d51dbaa0
📒 Files selected for processing (2)
core/src/location/manager.rscore/src/ops/locations/add/action.rs
canonicalize() on Windows can produce \?\UNC\server\share\... for network paths. The previous strip_prefix(r"\?\") would produce UNC\server\share\... which is invalid. Now handles both forms: - \?\UNC\server\share\... → \server\share\... (network UNC) - \?\C:\... → C:\... (local drive) Applied in both manager.rs (add_location) and add/action.rs (watcher registration) to ensure consistency. Same normalization as volume/fs/refs.rs:contains_path(). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract duplicated Windows extended path normalization into shared `common::utils::strip_windows_extended_prefix()` helper (was copy-pasted in location/manager.rs, locations/add/action.rs, volume/fs/refs.rs) - Add `unwatch_location()` call in LocationRemoveAction to stop the filesystem watcher when a location is deleted (symmetric with the `watch_location()` added in LocationAddAction) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/src/ops/locations/remove/action.rs (1)
46-46: Prefer rationale-focused inline comments (or remove them).Line 46 and Line 53 comments restate behavior; consider rewriting them to capture intent (why DB removal is authoritative and watcher cleanup is best-effort).
Suggested wording
- // Remove the location from DB + // Keep deletion semantics authoritative in the DB; watcher cleanup is best-effort. ... - // Unwatch the location from the filesystem watcher + // Avoid turning watcher teardown failures into user-visible delete failures.As per coding guidelines, "Inline comments should explain WHY decisions were made, not WHAT the code does, and should be kept to one sentence when possible."
Also applies to: 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/ops/locations/remove/action.rs` at line 46, Replace the inline "what" comments in the remove location flow with one-sentence rationale comments explaining why DB removal is authoritative and watcher cleanup is best-effort; specifically update the comment on the DB removal near the remove_location/remove action (the "Remove the location from DB" comment) and the watcher cleanup comment (around watcher cleanup logic) to briefly state the intent (e.g., that persistent state is the source of truth and watcher shutdowns are attempted best-effort to avoid impacting correctness) in one sentence each.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/src/common/utils.rs`:
- Around line 21-23: The branch that unconditionally strips the verbatim prefix
using s.strip_prefix(r"\\?\") should only apply to drive-letter paths; update
the condition around the existing s.strip_prefix usage so it additionally checks
the remainder begins with a drive-letter form (e.g. matches r"^[A-Za-z]:\\", or
equivalently check stripped.get(0..2) ends with ':' and stripped.get(2..3) ==
"\\"), and only then construct PathBuf::from(stripped); leave other verbatim
forms (like volume GUIDs) untouched so they fall through to the existing else
branch.
---
Nitpick comments:
In `@core/src/ops/locations/remove/action.rs`:
- Line 46: Replace the inline "what" comments in the remove location flow with
one-sentence rationale comments explaining why DB removal is authoritative and
watcher cleanup is best-effort; specifically update the comment on the DB
removal near the remove_location/remove action (the "Remove the location from
DB" comment) and the watcher cleanup comment (around watcher cleanup logic) to
briefly state the intent (e.g., that persistent state is the source of truth and
watcher shutdowns are attempted best-effort to avoid impacting correctness) in
one sentence each.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 16c71f8b-178a-4cb0-b59c-57d4afc45ec4
📒 Files selected for processing (5)
core/src/common/utils.rscore/src/location/manager.rscore/src/ops/locations/add/action.rscore/src/ops/locations/remove/action.rscore/src/volume/fs/refs.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/location/manager.rs
- core/src/ops/locations/add/action.rs
Comment on lines
+21
to
+23
| } else if let Some(stripped) = s.strip_prefix(r"\\?\") { | ||
| std::path::PathBuf::from(stripped) | ||
| } else { |
There was a problem hiding this comment.
Restrict \\?\ stripping to canonical drive-letter paths only.
On Line 21, the generic strip_prefix(r"\\?\") branch also rewrites non-drive verbatim paths (for example \\?\Volume{...}), which can produce invalid relative-like paths. Please gate this branch to drive-letter forms (C:\...) and leave other verbatim forms unchanged.
Proposed fix
- } else if let Some(stripped) = s.strip_prefix(r"\\?\") {
- std::path::PathBuf::from(stripped)
+ } else if let Some(stripped) = s.strip_prefix(r"\\?\") {
+ // Only normalize canonical local-drive form (e.g. C:\...)
+ if stripped.len() >= 3
+ && stripped.as_bytes()[1] == b':'
+ && stripped.as_bytes()[2] == b'\\'
+ {
+ std::path::PathBuf::from(stripped)
+ } else {
+ path
+ }
} else {
path
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } else if let Some(stripped) = s.strip_prefix(r"\\?\") { | |
| std::path::PathBuf::from(stripped) | |
| } else { | |
| } else if let Some(stripped) = s.strip_prefix(r"\\?\") { | |
| // Only normalize canonical local-drive form (e.g. C:\...) | |
| if stripped.len() >= 3 | |
| && stripped.as_bytes()[1] == b':' | |
| && stripped.as_bytes()[2] == b'\\' | |
| { | |
| std::path::PathBuf::from(stripped) | |
| } else { | |
| path | |
| } | |
| } else { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/src/common/utils.rs` around lines 21 - 23, The branch that
unconditionally strips the verbatim prefix using s.strip_prefix(r"\\?\") should
only apply to drive-letter paths; update the condition around the existing
s.strip_prefix usage so it additionally checks the remainder begins with a
drive-letter form (e.g. matches r"^[A-Za-z]:\\", or equivalently check
stripped.get(0..2) ends with ':' and stripped.get(2..3) == "\\"), and only then
construct PathBuf::from(stripped); leave other verbatim forms (like volume
GUIDs) untouched so they fall through to the existing else branch.
Volume GUIDs (\?\Volume{...}\) and other verbatim forms are invalid
without the prefix. Only strip when followed by a drive letter (X:\).
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
2 participants
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.
Summary
Locations added at runtime are never registered with the
FsWatcherService. The watcher only discovers locations at startup viaload_library_locations(). This means no filesystem events (creates, deletes, renames) are detected for any location added through the UI until the app is restarted.Additionally,
LocationManager::add_location()stores paths without canonicalization, so relative paths (from cwd-dependent contexts) break the watcher, volume manager, and indexer.Changes
1. Register watcher on location add (
locations/add/action.rs)After
LocationManager::add_location()succeeds, callfs_watcher.watch_location(meta)to start OS-level filesystem monitoring immediately. This mirrors whatload_library_locations()does at startup.2. Canonicalize paths before storing (
location/manager.rs)Call
tokio::fs::canonicalize()on local physical paths before storing in DB...components\?\UNC prefix on Windows (same pattern asvolume/manager.rs)Test plan
Watching location <id> at <path>after addingRelated