diff --git a/.tasks/INDEX-004-nested-locations-support.md b/.tasks/INDEX-004-nested-locations-support.md index 6701e6610..3ae69ba9f 100644 --- a/.tasks/INDEX-004-nested-locations-support.md +++ b/.tasks/INDEX-004-nested-locations-support.md @@ -224,27 +224,75 @@ if entry.indexed_at.is_some() { **Option B: Innermost location wins (efficient)** ```rust -// In the watcher event dispatch: -fn find_deepest_watching_location(path: &Path) -> Option { - // Find all locations that contain this path - let candidates = self.watched_locations - .iter() - .filter(|(_, loc)| path.starts_with(&loc.path)) - .collect::>(); +// In the watcher event dispatch or routing: +async fn find_deepest_watching_location( + &self, + event_path: &Path, + library_id: Uuid, + db: &DatabaseConnection, +) -> Result> { + // NOTE: All locations in watched_locations are already filtered to THIS device + // (INDEX-003 Phase 1 ensures only owned locations are watched) - // Return the one with the longest path (deepest nesting) - candidates + let mut candidates = Vec::new(); + + for (location_id, watched_loc) in self.watched_locations.read().await.iter() { + // Get location's entry record to check tree relationship + let location_record = location::Entity::find() + .filter(location::Column::Uuid.eq(*location_id)) + .one(db) + .await?; + + if let Some(loc) = location_record { + if let Some(root_entry_id) = loc.entry_id { + // Check if event path is under this location's entry tree + // Use entry_closure and directory_paths, not path string matching + if is_path_in_entry_tree(event_path, root_entry_id, db).await? { + // Get depth of location's root in the overall entry tree + let depth = get_entry_depth(root_entry_id, db).await?; + candidates.push((*location_id, depth)); + } + } + } + } + + // Return location with deepest (highest depth value) root entry + // Deeper in tree = more nested = should take precedence + Ok(candidates .into_iter() - .max_by_key(|(_, loc)| loc.path.components().count()) - .map(|(id, _)| *id) + .max_by_key(|(_, depth)| *depth) + .map(|(id, _)| id)) } -// Only dispatch event to the innermost location -if let Some(location_id) = find_deepest_watching_location(&event.path) { - dispatch_to_worker(location_id, event).await; +async fn is_path_in_entry_tree( + path: &Path, + root_entry_id: i32, + db: &DatabaseConnection, +) -> Result { + // Try to resolve the path within this entry tree + let path_str = path.to_string_lossy().to_string(); + + let result = db + .query_one(Statement::from_sql_and_values( + DbBackend::Sqlite, + r#" + SELECT 1 + FROM directory_paths dp + INNER JOIN entry_closure ec ON ec.descendant_id = dp.entry_id + WHERE dp.path = ? + AND ec.ancestor_id = ? + LIMIT 1 + "#, + vec![path_str.into(), root_entry_id.into()], + )) + .await?; + + Ok(result.is_some()) } ``` +**Device filtering note**: Since INDEX-003 Phase 1 ensures only this device's locations are loaded into `watched_locations`, we don't need additional device_id filtering here. All locations in the HashMap are guaranteed to be owned by the current device. + **Recommendation**: Start with Option A (both trigger), optimize to Option B later. ### 4. Location Deletion - Preserve Shared Entries @@ -311,12 +359,12 @@ async fn delete_location(&self, location_id: Uuid, db: &DatabaseConnection) -> R **Current sync** (no nesting support): - Location A syncs → creates entries 1-5 -- Location B syncs → creates duplicate entries 100-102 ❌ +- Location B syncs → creates duplicate entries 100-102 **With nesting support**: -- Location A syncs → creates entries 1-5 ✅ -- Location B syncs → just creates location record pointing to existing entry 2 ✅ -- No entry duplication ✅ +- Location A syncs → creates entries 1-5 +- Location B syncs → just creates location record pointing to existing entry 2 +- No entry duplication **Implementation**: Location sync already uses `entry_id` reference, so this works automatically! Just need to ensure receiving device doesn't re-create entries. @@ -575,8 +623,8 @@ mv /Documents/Work /Documents/Personal/Work **Current behavior**: - Location A's watcher detects rename - Updates entry 2's parent from entry 1 to entry 3 (Personal) -- Location B's `entry_id` still points to entry 2 ✅ -- Location B's path is now wrong ❌ +- Location B's `entry_id` still points to entry 2 +- Location B's path is now wrong **Solution**: Update location path when root entry moves: ```rust @@ -636,7 +684,7 @@ fn get_effective_index_mode(path: &Path, db: &DatabaseConnection) -> IndexMode { **With nesting**: - Location B syncs → `entry_id: 2` - Entry 2 might not exist yet on receiving device! -- Foreign key constraint violation ❌ +- Foreign key constraint violation **Solution**: Defer nested location sync until parent location syncs: ```rust diff --git a/core/src/ops/indexing/responder.rs b/core/src/ops/indexing/responder.rs index 7c409596d..50cfd8b11 100644 --- a/core/src/ops/indexing/responder.rs +++ b/core/src/ops/indexing/responder.rs @@ -45,11 +45,11 @@ pub async fn apply( .await? } FsRawEventKind::Modify { path } => { - handle_modify(&ctx, &path, rule_toggles, location_root).await? + handle_modify(&ctx, location_id, &path, rule_toggles, location_root).await? } - FsRawEventKind::Remove { path } => handle_remove(&ctx, &path).await?, + FsRawEventKind::Remove { path } => handle_remove(&ctx, location_id, &path).await?, FsRawEventKind::Rename { from, to } => { - handle_rename(&ctx, &from, &to, rule_toggles, location_root).await? + handle_rename(&ctx, location_id, &from, &to, rule_toggles, location_root).await? } } Ok(()) @@ -91,14 +91,16 @@ pub async fn apply_batch( // Process removes for path in removes { - if let Err(e) = handle_remove(&ctx, &path).await { + if let Err(e) = handle_remove(&ctx, location_id, &path).await { tracing::error!("Failed to handle remove for {}: {}", path.display(), e); } } // Process renames for (from, to) in renames { - if let Err(e) = handle_rename(&ctx, &from, &to, rule_toggles, location_root).await { + if let Err(e) = + handle_rename(&ctx, location_id, &from, &to, rule_toggles, location_root).await + { tracing::error!( "Failed to handle rename from {} to {}: {}", from.display(), @@ -127,7 +129,7 @@ pub async fn apply_batch( // Process modifies for path in modifies { - if let Err(e) = handle_modify(&ctx, &path, rule_toggles, location_root).await { + if let Err(e) = handle_modify(&ctx, location_id, &path, rule_toggles, location_root).await { tracing::error!("Failed to handle modify for {}: {}", path.display(), e); } } @@ -135,6 +137,19 @@ pub async fn apply_batch( Ok(()) } +/// Get the location's root entry ID for scoping queries +async fn get_location_root_entry_id(ctx: &impl IndexingCtx, location_id: Uuid) -> Result { + let location_record = entities::location::Entity::find() + .filter(entities::location::Column::Uuid.eq(location_id)) + .one(ctx.library_db()) + .await? + .ok_or_else(|| anyhow::anyhow!("Location not found: {}", location_id))?; + + location_record + .entry_id + .ok_or_else(|| anyhow::anyhow!("Location {} has no root entry", location_id)) +} + /// Check if a path should be filtered based on indexing rules async fn should_filter_path( path: &Path, @@ -203,7 +218,7 @@ async fn handle_create( // Minimal state provides parent cache used by EntryProcessor let mut state = IndexerState::new(&crate::domain::addressing::SdPath::local(path)); - // CRITICAL: Seed the location root entry into cache to scope parent lookup + // Seed the location root entry into cache to scope parent lookup // This ensures parents are found within THIS location's tree, not another device's location // with the same path. Without this, create_entry could attach to the wrong location's tree. if let Ok(Some(location_record)) = entities::location::Entity::find() @@ -272,6 +287,7 @@ async fn handle_create( /// Handle modify: resolve entry ID by path, then update async fn handle_modify( ctx: &impl IndexingCtx, + location_id: Uuid, path: &Path, rule_toggles: RuleToggles, location_root: &Path, @@ -284,6 +300,9 @@ async fn handle_modify( return Ok(()); } + // Get location root entry ID for scoped queries + let location_root_entry_id = get_location_root_entry_id(ctx, location_id).await?; + // If inode indicates a move, handle as a move and skip update // Responder uses direct filesystem access (None backend) since it reacts to local FS events let meta = EntryProcessor::extract_metadata(path, None).await?; @@ -291,7 +310,9 @@ async fn handle_modify( return Ok(()); } - if let Some(entry_id) = resolve_entry_id_by_path(ctx, path).await? { + if let Some(entry_id) = + resolve_entry_id_by_path_scoped(ctx, path, location_root_entry_id).await? + { let dir_entry = DirEntry { path: meta.path, kind: meta.kind, @@ -305,9 +326,15 @@ async fn handle_modify( } /// Handle remove: resolve entry ID and delete subtree (closure table + cache) -async fn handle_remove(ctx: &impl IndexingCtx, path: &Path) -> Result<()> { +async fn handle_remove(ctx: &impl IndexingCtx, location_id: Uuid, path: &Path) -> Result<()> { debug!("Remove: {}", path.display()); - if let Some(entry_id) = resolve_entry_id_by_path(ctx, path).await? { + + // Get location root entry ID for scoped queries + let location_root_entry_id = get_location_root_entry_id(ctx, location_id).await?; + + if let Some(entry_id) = + resolve_entry_id_by_path_scoped(ctx, path, location_root_entry_id).await? + { delete_subtree(ctx, entry_id).await?; } Ok(()) @@ -316,6 +343,7 @@ async fn handle_remove(ctx: &impl IndexingCtx, path: &Path) -> Result<()> { /// Handle rename/move: resolve source entry and move via EntryProcessor async fn handle_rename( ctx: &impl IndexingCtx, + location_id: Uuid, from: &Path, to: &Path, rule_toggles: RuleToggles, @@ -323,6 +351,9 @@ async fn handle_rename( ) -> Result<()> { debug!("Rename: {} -> {}", from.display(), to.display()); + // Get location root entry ID for scoped queries + let location_root_entry_id = get_location_root_entry_id(ctx, location_id).await?; + // Check if the destination path should be filtered // If the file is being moved to a filtered location, we should remove it from the database if should_filter_path(to, rule_toggles, location_root).await? { @@ -331,9 +362,12 @@ async fn handle_rename( to.display() ); // Treat this as a removal of the source file - return handle_remove(ctx, from).await; + return handle_remove(ctx, location_id, from).await; } - if let Some(entry_id) = resolve_entry_id_by_path(ctx, from).await? { + + if let Some(entry_id) = + resolve_entry_id_by_path_scoped(ctx, from, location_root_entry_id).await? + { debug!("Found entry {} for old path, moving to new path", entry_id); // Create state and populate entry_id_cache with parent directories @@ -341,7 +375,10 @@ async fn handle_rename( // Populate cache with new parent directory if it exists if let Some(new_parent_path) = to.parent() { - if let Ok(Some(parent_id)) = resolve_directory_entry_id(ctx, new_parent_path).await { + if let Ok(Some(parent_id)) = + resolve_directory_entry_id_scoped(ctx, new_parent_path, location_root_entry_id) + .await + { state .entry_id_cache .insert(new_parent_path.to_path_buf(), parent_id); @@ -385,51 +422,73 @@ async fn build_dir_entry(path: &Path) -> Result { }) } -/// Resolve an entry ID by absolute path (directory first, then file by parent/name/extension) -async fn resolve_entry_id_by_path(ctx: &impl IndexingCtx, abs_path: &Path) -> Result> { - if let Some(id) = resolve_directory_entry_id(ctx, abs_path).await? { - return Ok(Some(id)); - } - resolve_file_entry_id(ctx, abs_path).await -} - -/// Resolve a directory entry by its full path in the directory_paths table -async fn resolve_directory_entry_id( +/// Resolve an entry ID by absolute path, scoped to location's entry tree +async fn resolve_entry_id_by_path_scoped( ctx: &impl IndexingCtx, abs_path: &Path, + location_root_entry_id: i32, ) -> Result> { - let path_str = abs_path.to_string_lossy().to_string(); - - // CRITICAL: Path alone is ambiguous if multiple devices have same paths - // Query could return entries from any device's location - // TODO: Scope by location_root to ensure we only find entries in THIS location's tree - // For now, this returns the first match (usually correct if only one device has the path) - let model = entities::directory_paths::Entity::find() - .filter(entities::directory_paths::Column::Path.eq(path_str)) - .one(ctx.library_db()) - .await?; - Ok(model.map(|m| m.entry_id)) + if let Some(id) = + resolve_directory_entry_id_scoped(ctx, abs_path, location_root_entry_id).await? + { + return Ok(Some(id)); + } + resolve_file_entry_id_scoped(ctx, abs_path, location_root_entry_id).await } -/// Resolve a file entry by parent directory path + file name (+ extension) -async fn resolve_file_entry_id(ctx: &impl IndexingCtx, abs_path: &Path) -> Result> { +/// Resolve a directory entry by path, scoped to location's entry tree using entry_closure +async fn resolve_directory_entry_id_scoped( + ctx: &impl IndexingCtx, + abs_path: &Path, + location_root_entry_id: i32, +) -> Result> { + use sea_orm::FromQueryResult; + + let path_str = abs_path.to_string_lossy().to_string(); + + // Query directory_paths and JOIN with entry_closure to scope by location + // This ensures we only find entries within THIS location's tree + #[derive(Debug, FromQueryResult)] + struct DirectoryEntryId { + entry_id: i32, + } + + let result = DirectoryEntryId::find_by_statement(sea_orm::Statement::from_sql_and_values( + sea_orm::DbBackend::Sqlite, + r#" + SELECT dp.entry_id + FROM directory_paths dp + INNER JOIN entry_closure ec ON ec.descendant_id = dp.entry_id + WHERE dp.path = ? + AND ec.ancestor_id = ? + "#, + vec![path_str.into(), location_root_entry_id.into()], + )) + .one(ctx.library_db()) + .await?; + + Ok(result.map(|r| r.entry_id)) +} + +/// Resolve a file entry by parent directory path + file name, scoped to location's tree +async fn resolve_file_entry_id_scoped( + ctx: &impl IndexingCtx, + abs_path: &Path, + location_root_entry_id: i32, +) -> Result> { let parent = match abs_path.parent() { Some(p) => p, None => return Ok(None), }; - let parent_str = parent.to_string_lossy().to_string(); - // CRITICAL: Same ambiguity issue as resolve_directory_entry_id - // TODO: Scope by location to ensure we find the correct parent - let parent_dir = match entities::directory_paths::Entity::find() - .filter(entities::directory_paths::Column::Path.eq(parent_str)) - .one(ctx.library_db()) - .await? - { - Some(m) => m, - None => return Ok(None), - }; + // First resolve parent directory using scoped lookup + let parent_id = + match resolve_directory_entry_id_scoped(ctx, parent, location_root_entry_id).await? { + Some(id) => id, + None => return Ok(None), + }; + // Now find the file entry by parent + name + extension let name = abs_path .file_stem() .and_then(|s| s.to_str()) @@ -441,7 +500,7 @@ async fn resolve_file_entry_id(ctx: &impl IndexingCtx, abs_path: &Path) -> Resul .map(|s| s.to_lowercase()); let mut q = entities::entry::Entity::find() - .filter(entities::entry::Column::ParentId.eq(parent_dir.entry_id)) + .filter(entities::entry::Column::ParentId.eq(parent_id)) .filter(entities::entry::Column::Name.eq(name)); if let Some(e) = ext { q = q.filter(entities::entry::Column::Extension.eq(e));