From b48e851fdf252d01337dd01f3e884eb04d8eccd5 Mon Sep 17 00:00:00 2001 From: xiongyiming Date: Wed, 6 May 2026 16:46:13 +0800 Subject: [PATCH] Support child repositories in code review panel --- app/src/pane_group/working_directories.rs | 72 +++-- .../pane_group/working_directories_tests.rs | 296 ++++++++++++++++++ app/src/terminal/view.rs | 6 +- app/src/workspace/view.rs | 30 ++ app/src/workspace/view/right_panel.rs | 118 +++++-- crates/repo_metadata/src/repositories.rs | 269 +++++++++++----- .../repo_metadata/src/repositories_tests.rs | 130 +++++++- 7 files changed, 792 insertions(+), 129 deletions(-) diff --git a/app/src/pane_group/working_directories.rs b/app/src/pane_group/working_directories.rs index a3beebf908..114b117fa2 100644 --- a/app/src/pane_group/working_directories.rs +++ b/app/src/pane_group/working_directories.rs @@ -180,6 +180,10 @@ impl WorkingDirectoriesModel { Self::default() } + fn root_path_is_available(path: &Path) -> bool { + path.exists() + } + /// Get the unique directories for a specific pane group in insertion order (oldest first). fn least_recent_directories_for_pane_group( &self, @@ -216,7 +220,13 @@ impl WorkingDirectoriesModel { pane_group_id: EntityId, ) -> Option + '_> { self.least_recent_repositories_for_pane_group(pane_group_id) - .map(|repos| repos.iter().rev().cloned()) + .map(|repos| { + repos + .iter() + .rev() + .filter(|repo| Self::root_path_is_available(repo)) + .cloned() + }) } /// Get the terminal view ID associated with a specific root path in a pane group. @@ -535,34 +545,47 @@ impl WorkingDirectoriesModel { .get(&pane_group_id) .into_iter() .flat_map(|dirs| dirs.iter()) - .filter_map(|dir| self.get_repo_root_for_path(dir, ctx)) + .flat_map(|dir| { + self.get_repo_root_for_path(dir, ctx) + .map(|repo_root| vec![repo_root]) + .unwrap_or_else(|| self.get_descendant_repo_roots_for_path(dir, ctx)) + }) .collect(); let mut new_roots: HashSet = HashSet::from_iter(new_repo_roots.iter().cloned()); new_roots.extend(new_root_paths.iter().cloned()); // Build mapping from directories to their terminal IDs - let mut new_root_to_terminal: HashMap = terminal_cwds - .iter() - .filter_map(|(terminal_id, cwd)| root_for_raw_path(cwd).map(|p| (p, *terminal_id))) - .collect(); - new_root_to_terminal.retain(|cwd, _terminal_id| new_roots.contains(cwd)); - - // Second pass: if we have a focused terminal, ensure its repo maps to it - // This ensures the dropdown selects the correct repo when a pane is focused or CD'd - let mut focused_repo: Option = None; - if let Some(focused_id) = focused_terminal_id { - let mut repos_to_insert = Vec::new(); - for (dir, terminal_id) in &new_root_to_terminal { - if *terminal_id == focused_id { - if let Some(repo_root) = self.get_repo_root_for_path(dir, ctx) { - repos_to_insert.push((repo_root.clone(), focused_id)); - focused_repo = Some(repo_root); - } + let mut new_root_to_terminal: HashMap = HashMap::new(); + for (terminal_id, cwd) in &terminal_cwds { + let Some(root_path) = root_for_raw_path(cwd) else { + continue; + }; + new_root_to_terminal.insert(root_path.clone(), *terminal_id); + + if self.get_repo_root_for_path(&root_path, ctx).is_none() { + for repo_root in self.get_descendant_repo_roots_for_path(&root_path, ctx) { + new_root_to_terminal + .entry(repo_root) + .or_insert(*terminal_id); } } - for (repo_root, focused_id) in repos_to_insert { - new_root_to_terminal.insert(repo_root, focused_id); - } + } + new_root_to_terminal.retain(|cwd, _terminal_id| new_roots.contains(cwd)); + + // Derive focus only from the focused terminal's actual cwd. A non-repo + // parent may map multiple descendant repos to the same terminal for Code + // Review fallback routing, but those fallback mappings should not decide + // which repository is focused. + let focused_repo = focused_terminal_id.and_then(|focused_id| { + terminal_cwds + .iter() + .find(|(terminal_id, _cwd)| *terminal_id == focused_id) + .and_then(|(_, cwd)| root_for_raw_path(cwd)) + .and_then(|root_path| self.get_repo_root_for_path(&root_path, ctx)) + }); + if let (Some(focused_id), Some(focused_repo)) = (focused_terminal_id, focused_repo.as_ref()) + { + new_root_to_terminal.insert(focused_repo.clone(), focused_id); } // Get or create the IndexSet for repository roots @@ -619,6 +642,11 @@ impl WorkingDirectoriesModel { .and_then(|r| PathBuf::try_from(r).ok()) } + /// Get detected repository roots below a non-repository workspace path. + fn get_descendant_repo_roots_for_path(&self, path: &Path, ctx: &AppContext) -> Vec { + DetectedRepositories::as_ref(ctx).get_descendant_roots_for_path(path) + } + /// Emit a DirectoriesChanged event with the current state for a specific pane group. /// Directories are returned in most recent first order for use in the UI. fn emit_directories_changed(&mut self, pane_group_id: EntityId, ctx: &mut ModelContext) { diff --git a/app/src/pane_group/working_directories_tests.rs b/app/src/pane_group/working_directories_tests.rs index 9925250ae3..a0e3e2a9ed 100644 --- a/app/src/pane_group/working_directories_tests.rs +++ b/app/src/pane_group/working_directories_tests.rs @@ -235,3 +235,299 @@ fn clear_selected_review_repo_removes_only_the_targeted_pane_group_entry() { }); }); } + +#[test] +fn refresh_working_directories_includes_detected_repos_under_non_repo_parent() { + App::test((), |mut app| async move { + let detected_repos_handle = app.add_singleton_model(|_| DetectedRepositories::default()); + + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let repo_1 = workspace.join("repo-1"); + let repo_2 = workspace.join("repo-2"); + fs::create_dir_all(&repo_1).expect("create repo-1"); + fs::create_dir_all(&repo_2).expect("create repo-2"); + + let canonical_workspace = dunce::canonicalize(&workspace).expect("canonical workspace"); + let canonical_repo_1 = dunce::canonicalize(&repo_1).expect("canonical repo-1"); + let canonical_repo_2 = dunce::canonicalize(&repo_2).expect("canonical repo-2"); + + detected_repos_handle.update(&mut app, |repos, _ctx| { + for repo_path in [&canonical_repo_1, &canonical_repo_2] { + let canonical = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized( + repo_path, + ) + .expect("canonicalized repo path"); + repos.insert_test_repo_root(canonical); + } + }); + + let pane_group_id = EntityId::new(); + let terminal = EntityId::new(); + let working_directories_handle = app.add_model(|_| WorkingDirectoriesModel::new()); + + let repos: HashSet = working_directories_handle.update(&mut app, |model, ctx| { + model.refresh_working_directories_for_pane_group( + pane_group_id, + vec![(terminal, canonical_workspace.to_string_lossy().to_string())], + vec![], + Some(terminal), + ctx, + ); + + model + .most_recent_repositories_for_pane_group(pane_group_id) + .expect("pane group repos exist") + .collect() + }); + + assert_eq!( + repos, + HashSet::from_iter([canonical_repo_1.clone(), canonical_repo_2.clone()]) + ); + + working_directories_handle.read(&app, |model, _ctx| { + assert_eq!( + model.get_terminal_id_for_root_path(pane_group_id, &canonical_repo_1), + Some(terminal) + ); + assert_eq!( + model.get_terminal_id_for_root_path(pane_group_id, &canonical_repo_2), + Some(terminal) + ); + }); + }); +} + +#[test] +fn refresh_working_directories_omits_deleted_repos_under_non_repo_parent() { + App::test((), |mut app| async move { + let detected_repos_handle = app.add_singleton_model(|_| DetectedRepositories::default()); + + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let repo_1 = workspace.join("repo-1"); + let deleted_repo = workspace.join("deleted-repo"); + fs::create_dir_all(&repo_1).expect("create repo-1"); + fs::create_dir_all(&deleted_repo).expect("create deleted-repo"); + + let canonical_workspace = dunce::canonicalize(&workspace).expect("canonical workspace"); + let canonical_repo_1 = dunce::canonicalize(&repo_1).expect("canonical repo-1"); + let canonical_deleted_repo = + dunce::canonicalize(&deleted_repo).expect("canonical deleted-repo"); + + detected_repos_handle.update(&mut app, |repos, _ctx| { + for repo_path in [&canonical_repo_1, &canonical_deleted_repo] { + let canonical = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized( + repo_path, + ) + .expect("canonicalized repo path"); + repos.insert_test_repo_root(canonical); + } + }); + + fs::remove_dir_all(&canonical_deleted_repo).expect("remove deleted repo"); + + let pane_group_id = EntityId::new(); + let terminal = EntityId::new(); + let working_directories_handle = app.add_model(|_| WorkingDirectoriesModel::new()); + + let repos: HashSet = working_directories_handle.update(&mut app, |model, ctx| { + model.refresh_working_directories_for_pane_group( + pane_group_id, + vec![(terminal, canonical_workspace.to_string_lossy().to_string())], + vec![], + Some(terminal), + ctx, + ); + + model + .most_recent_repositories_for_pane_group(pane_group_id) + .expect("pane group repos exist") + .collect() + }); + + assert_eq!(repos, HashSet::from_iter([canonical_repo_1])); + working_directories_handle.read(&app, |model, _ctx| { + assert_eq!( + model.get_terminal_id_for_root_path(pane_group_id, &canonical_deleted_repo), + None + ); + }); + }); +} + +#[test] +fn refresh_working_directories_does_not_focus_child_repos_from_non_repo_parent() { + App::test((), |mut app| async move { + let detected_repos_handle = app.add_singleton_model(|_| DetectedRepositories::default()); + + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let repo_1 = workspace.join("repo-1"); + let repo_2 = workspace.join("repo-2"); + fs::create_dir_all(&repo_1).expect("create repo-1"); + fs::create_dir_all(&repo_2).expect("create repo-2"); + + let canonical_workspace = dunce::canonicalize(&workspace).expect("canonical workspace"); + let canonical_repo_1 = dunce::canonicalize(&repo_1).expect("canonical repo-1"); + let canonical_repo_2 = dunce::canonicalize(&repo_2).expect("canonical repo-2"); + + detected_repos_handle.update(&mut app, |repos, _ctx| { + for repo_path in [&canonical_repo_1, &canonical_repo_2] { + let canonical = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized( + repo_path, + ) + .expect("canonicalized repo path"); + repos.insert_test_repo_root(canonical); + } + }); + + let pane_group_id = EntityId::new(); + let terminal = EntityId::new(); + let working_directories_handle = app.add_model(|_| WorkingDirectoriesModel::new()); + + working_directories_handle.update(&mut app, |model, ctx| { + model.refresh_working_directories_for_pane_group( + pane_group_id, + vec![(terminal, canonical_workspace.to_string_lossy().to_string())], + vec![], + Some(terminal), + ctx, + ); + }); + + working_directories_handle.read(&app, |model, _ctx| { + assert_eq!( + model.focused_repo.get(&pane_group_id).cloned().flatten(), + None, + "a non-repo parent with multiple detected child repos should not pick an arbitrary focused repo" + ); + }); + }); +} + +#[test] +fn refresh_working_directories_keeps_repo_terminal_mapping_over_parent_fallback() { + App::test((), |mut app| async move { + let detected_repos_handle = app.add_singleton_model(|_| DetectedRepositories::default()); + + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let repo_1_src = workspace.join("repo-1/src"); + let repo_2 = workspace.join("repo-2"); + fs::create_dir_all(&repo_1_src).expect("create repo-1/src"); + fs::create_dir_all(&repo_2).expect("create repo-2"); + + let repo_1 = workspace.join("repo-1"); + let canonical_workspace = dunce::canonicalize(&workspace).expect("canonical workspace"); + let canonical_repo_1 = dunce::canonicalize(&repo_1).expect("canonical repo-1"); + let canonical_repo_1_src = dunce::canonicalize(&repo_1_src).expect("canonical repo-1/src"); + let canonical_repo_2 = dunce::canonicalize(&repo_2).expect("canonical repo-2"); + + detected_repos_handle.update(&mut app, |repos, _ctx| { + for repo_path in [&canonical_repo_1, &canonical_repo_2] { + let canonical = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized( + repo_path, + ) + .expect("canonicalized repo path"); + repos.insert_test_repo_root(canonical); + } + }); + + let pane_group_id = EntityId::new(); + let repo_terminal = EntityId::new(); + let parent_terminal = EntityId::new(); + let working_directories_handle = app.add_model(|_| WorkingDirectoriesModel::new()); + + working_directories_handle.update(&mut app, |model, ctx| { + model.refresh_working_directories_for_pane_group( + pane_group_id, + vec![ + ( + repo_terminal, + canonical_repo_1_src.to_string_lossy().to_string(), + ), + ( + parent_terminal, + canonical_workspace.to_string_lossy().to_string(), + ), + ], + vec![], + Some(parent_terminal), + ctx, + ); + }); + + working_directories_handle.read(&app, |model, _ctx| { + assert_eq!( + model.get_terminal_id_for_root_path(pane_group_id, &canonical_repo_1), + Some(repo_terminal), + "fallback mappings from a non-repo parent should not overwrite repo-local terminals" + ); + assert_eq!( + model.get_terminal_id_for_root_path(pane_group_id, &canonical_repo_2), + Some(parent_terminal) + ); + }); + }); +} + +#[test] +fn refresh_working_directories_focuses_repo_from_focused_terminal_cwd() { + App::test((), |mut app| async move { + let detected_repos_handle = app.add_singleton_model(|_| DetectedRepositories::default()); + + let temp_dir = tempfile::TempDir::new().expect("temp dir"); + let workspace = temp_dir.path().join("workspace"); + let repo_1_src = workspace.join("repo-1/src"); + let repo_2 = workspace.join("repo-2"); + fs::create_dir_all(&repo_1_src).expect("create repo-1/src"); + fs::create_dir_all(&repo_2).expect("create repo-2"); + + let repo_1 = workspace.join("repo-1"); + let canonical_repo_1 = dunce::canonicalize(&repo_1).expect("canonical repo-1"); + let canonical_repo_2 = dunce::canonicalize(&repo_2).expect("canonical repo-2"); + let canonical_repo_1_src = dunce::canonicalize(&repo_1_src).expect("canonical repo-1/src"); + + detected_repos_handle.update(&mut app, |repos, _ctx| { + for repo_path in [&canonical_repo_1, &canonical_repo_2] { + let canonical = + warp_util::standardized_path::StandardizedPath::from_local_canonicalized( + repo_path, + ) + .expect("canonicalized repo path"); + repos.insert_test_repo_root(canonical); + } + }); + + let pane_group_id = EntityId::new(); + let focused_terminal = EntityId::new(); + let working_directories_handle = app.add_model(|_| WorkingDirectoriesModel::new()); + + working_directories_handle.update(&mut app, |model, ctx| { + model.refresh_working_directories_for_pane_group( + pane_group_id, + vec![( + focused_terminal, + canonical_repo_1_src.to_string_lossy().to_string(), + )], + vec![], + Some(focused_terminal), + ctx, + ); + }); + + working_directories_handle.read(&app, |model, _ctx| { + assert_eq!( + model.focused_repo.get(&pane_group_id).cloned().flatten(), + Some(canonical_repo_1), + "focused repo should come from the focused terminal's actual cwd" + ); + }); + }); +} diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index ef6f46af00..c801d35cde 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -11707,7 +11707,11 @@ impl TerminalView { let old_repo_path = me.current_repo_path.clone(); me.current_repo_path = repo_path_opt.clone(); - if old_repo_path != me.current_repo_path { + // Notify the pane group when detection changes, or when a local + // non-repo directory finishes scanning. The latter lets Code + // Review initialize from detected child repositories. + if old_repo_path != me.current_repo_path || repo_path_opt.is_none() + { ctx.emit(Event::Pane(PaneEvent::RepoChanged)); } diff --git a/app/src/workspace/view.rs b/app/src/workspace/view.rs index ea085cd7d0..a1ff34ded7 100644 --- a/app/src/workspace/view.rs +++ b/app/src/workspace/view.rs @@ -8106,6 +8106,7 @@ impl Workspace { )) } else { let active_pane_group = self.active_tab_pane_group().clone(); + let pane_group_id = active_pane_group.id(); // Read repo_path and terminal_view from the pane group (immutable context). let read_result = active_pane_group.read(ctx, |pane_group, ctx| { pane_group.active_session_view(ctx).map(|terminal_view| { @@ -8119,6 +8120,20 @@ impl Workspace { // Resolve DiffStateModel outside the read closure (needs mutable context). read_result.and_then( |(repo_path, terminal_view): (Option, WeakViewHandle)| { + let repo_path = repo_path + .or_else(|| { + self.right_panel_view + .as_ref(ctx) + .selected_repo_path() + .cloned() + }) + .or_else(|| { + self.working_directories_model.read(ctx, |model, _| { + model + .most_recent_repositories_for_pane_group(pane_group_id) + .and_then(|mut repos| repos.next()) + }) + }); let diff_state_model = repo_path.as_ref().and_then(|rp: &PathBuf| { self.working_directories_model.update(ctx, |model, ctx| { model.get_or_create_diff_state_model( @@ -8284,9 +8299,24 @@ impl Workspace { (repo_path, terminal_view.downgrade()) }) }); + let pane_group_id = pane_group_handle.id(); // Resolve DiffStateModel outside the read closure (needs mutable context). let context = read_result.and_then( |(repo_path, terminal_view): (Option, WeakViewHandle)| { + let repo_path = repo_path + .or_else(|| { + self.right_panel_view + .as_ref(ctx) + .selected_repo_path() + .cloned() + }) + .or_else(|| { + self.working_directories_model.read(ctx, |model, _| { + model + .most_recent_repositories_for_pane_group(pane_group_id) + .and_then(|mut repos| repos.next()) + }) + }); let diff_state_model = repo_path.as_ref().and_then(|rp: &PathBuf| { self.working_directories_model.update(ctx, |model, ctx| { model.get_or_create_diff_state_model( diff --git a/app/src/workspace/view/right_panel.rs b/app/src/workspace/view/right_panel.rs index 69203a767a..6b4587223e 100644 --- a/app/src/workspace/view/right_panel.rs +++ b/app/src/workspace/view/right_panel.rs @@ -195,6 +195,16 @@ impl CodeReviewState { } } + #[cfg(feature = "local_fs")] + fn repo_path_is_available(repo_path: &Path) -> bool { + repo_path.exists() + } + + #[cfg(not(feature = "local_fs"))] + fn repo_path_is_available(_repo_path: &Path) -> bool { + false + } + #[cfg(not(feature = "local_fs"))] fn set_available_repos( &mut self, @@ -205,6 +215,10 @@ impl CodeReviewState { #[cfg(feature = "local_fs")] fn set_available_repos(&mut self, repos: Vec, ctx: &mut ViewContext) { + let repos = repos + .into_iter() + .filter(|repo| Self::repo_path_is_available(repo)) + .collect::>(); let should_clear = self .selected_repo_path .as_ref() @@ -496,6 +510,7 @@ impl RightPanelView { self.code_review_state .as_ref() .and_then(|s| s.selected_repo_path.as_ref()) + .filter(|repo_path| CodeReviewState::repo_path_is_available(repo_path)) } #[cfg(feature = "local_fs")] @@ -1314,9 +1329,41 @@ impl RightPanelView { .unwrap_or_else(|| "".to_string()) } + fn review_path_unavailable_reasons( + active_session_path: Option<&Path>, + repo_path: Option<&Path>, + allow_mapped_terminal: bool, + ) -> Vec { + let mut unavailable_reasons = Vec::new(); + + match repo_path { + Some(repo_path) => match active_session_path { + // Canonicalize the CWD, note that repo_path has already been canonicalized. + Some(cwd) + if canonicalize(cwd) + .as_deref() + .unwrap_or(cwd) + .starts_with(repo_path) => {} + Some(_) if allow_mapped_terminal => {} + Some(_) => { + unavailable_reasons + .push(ReviewTerminalUnavailableReason::SessionOutsideSelectedRepo); + } + None => { + unavailable_reasons + .push(ReviewTerminalUnavailableReason::SessionPathUnavailable); + } + }, + None => unavailable_reasons.push(ReviewTerminalUnavailableReason::NoSelectedRepo), + } + + unavailable_reasons + } + fn review_terminal_status( tv: &ViewHandle, repo_path: Option<&Path>, + allow_mapped_terminal: bool, ai_enabled: bool, ctx: &AppContext, ) -> ReviewTerminalStatus { @@ -1327,23 +1374,11 @@ impl RightPanelView { let model = t.model.lock(); let is_executing = model.block_list().active_block().is_executing(); let is_input_box_visible = t.is_input_box_visible(&model, ctx); - let mut unavailable_reasons = Vec::new(); - - match repo_path { - Some(repo_path) => match active_session_path.as_ref() { - // Canonicalize the CWD, note that repo_path has already been canonicalized. - Some(cwd) - if canonicalize(cwd) - .as_deref() - .unwrap_or(cwd) - .starts_with(repo_path) => {} - Some(_) => unavailable_reasons - .push(ReviewTerminalUnavailableReason::SessionOutsideSelectedRepo), - None => unavailable_reasons - .push(ReviewTerminalUnavailableReason::SessionPathUnavailable), - }, - None => unavailable_reasons.push(ReviewTerminalUnavailableReason::NoSelectedRepo), - } + let mut unavailable_reasons = Self::review_path_unavailable_reasons( + active_session_path.as_deref(), + repo_path, + allow_mapped_terminal, + ); if active_cli_agent.is_none() { if !ai_enabled { @@ -1459,6 +1494,7 @@ impl RightPanelView { let terminal_status = Self::review_terminal_status( &terminal_view, selected_repo_path.as_deref(), + preferred_terminal_id == Some(terminal_id), ai_enabled, ctx, ); @@ -1501,10 +1537,12 @@ impl RightPanelView { fn is_terminal_available_for_review( tv: &ViewHandle, repo_path: &Path, + allow_mapped_terminal: bool, ai_enabled: bool, ctx: &AppContext, ) -> bool { - Self::review_terminal_status(tv, Some(repo_path), ai_enabled, ctx).is_available() + Self::review_terminal_status(tv, Some(repo_path), allow_mapped_terminal, ai_enabled, ctx) + .is_available() } /// Finds the best terminal to send review comments to. @@ -1519,7 +1557,14 @@ impl RightPanelView { ctx: &AppContext, ) -> Option> { let is_available = |tv: &ViewHandle| { - Self::is_terminal_available_for_review(tv, repo_path, ai_enabled, ctx) + let allow_mapped_terminal = preferred_terminal_id == Some(tv.id()); + Self::is_terminal_available_for_review( + tv, + repo_path, + allow_mapped_terminal, + ai_enabled, + ctx, + ) }; // Try the focused terminal first. @@ -1680,6 +1725,41 @@ impl Entity for RightPanelView { type Event = RightPanelEvent; } +#[cfg(test)] +mod tests { + use std::path::Path; + + use super::{ReviewTerminalUnavailableReason, RightPanelView}; + + #[test] + fn review_path_allows_mapped_terminal_outside_selected_repo() { + let reasons = RightPanelView::review_path_unavailable_reasons( + Some(Path::new("/workspace")), + Some(Path::new("/workspace/repo")), + true, + ); + + assert!( + reasons.is_empty(), + "repo-mapped terminals can be rooted at a non-repo parent" + ); + } + + #[test] + fn review_path_rejects_unmapped_terminal_outside_selected_repo() { + let reasons = RightPanelView::review_path_unavailable_reasons( + Some(Path::new("/workspace")), + Some(Path::new("/workspace/repo")), + false, + ); + + assert_eq!( + reasons, + vec![ReviewTerminalUnavailableReason::SessionOutsideSelectedRepo] + ); + } +} + #[cfg(feature = "local_fs")] impl TypedActionView for RightPanelView { type Action = RightPanelAction; diff --git a/crates/repo_metadata/src/repositories.rs b/crates/repo_metadata/src/repositories.rs index b986802121..4611b95abe 100644 --- a/crates/repo_metadata/src/repositories.rs +++ b/crates/repo_metadata/src/repositories.rs @@ -40,6 +40,7 @@ pub enum DetectedRepositoriesEvent { #[derive(Default)] pub struct DetectedRepositories { repository_roots: HashSet, + child_repo_scan_roots_in_flight: HashSet, #[cfg(test)] /// List of spawned background tasks, for testing. spawned_futures: Vec, @@ -122,61 +123,46 @@ impl DetectedRepositories { } let local_path_for_search = path.to_local_path(); + let child_repo_scan_root = matches!(source, RepoDetectionSource::TerminalNavigation) + .then(|| path.clone()) + .filter(|path| self.child_repo_scan_roots_in_flight.insert(path.clone())); + let should_detect_child_repos = child_repo_scan_root.is_some(); let (tx, rx) = oneshot::channel::>(); let spawned_handle = ctx.spawn( async move { if let Some(local_path) = local_path_for_search { - find_git_repo(&local_path).await - } else { - None - } - }, - move |me, res, ctx| { - if let Some(info) = res { - if let Some(repo_root_path) = info - .working_tree_path - .as_ref() - .and_then(|path| StandardizedPath::from_local_canonicalized(path).ok()) + let containing_repo = find_git_repo(&local_path).await; + let child_repos = if should_detect_child_repos && containing_repo.is_none() { - if let Some(local_path) = repo_root_path.to_local_path() { - me.repository_roots - .insert(LocalOrRemotePath::Local(local_path)); - } - - let external_git_dir = StandardizedPath::from_local_canonicalized( - info.git_dir_path.as_path(), - ) - .ok() - // Only treat as external if it's outside the working tree. - .filter(|p| !p.starts_with(&repo_root_path)); - - if let Some(repository) = - DirectoryWatcher::handle(ctx).update(ctx, |watcher, ctx| { - watcher - .add_directory_with_git_dir( - repo_root_path, - external_git_dir, - ctx, - ) - .ok() - }) - { - let repo_path = repository.as_ref(ctx).root_dir().to_local_path(); - ctx.emit(DetectedRepositoriesEvent::DetectedGitRepo { - repository, - source, - }); - let _ = tx.send(repo_path); - } else { - let _ = tx.send(None); - } + find_child_git_repos(&local_path).await } else { - // No working tree path; do not treat git_dir_path as a repository path. - let _ = tx.send(None); + Vec::new() + }; + GitRepoDetectionResult { + containing_repo, + child_repos, } } else { - let _ = tx.send(None); + GitRepoDetectionResult { + containing_repo: None, + child_repos: Vec::new(), + } + } + }, + move |me, res, ctx| { + if let Some(path) = child_repo_scan_root { + me.child_repo_scan_roots_in_flight.remove(&path); + } + + let containing_repo_path = res + .containing_repo + .and_then(|info| me.register_git_repo_info(info, source, ctx)); + + for child_repo in res.child_repos { + me.register_git_repo_info(child_repo, source, ctx); } + + let _ = tx.send(containing_repo_path); }, ); @@ -225,6 +211,30 @@ impl DetectedRepositories { } } + /// Given a local path, return repository roots already detected below that path. + /// + /// This is used for directory-style workspaces where the active terminal is + /// in a non-git parent directory that directly contains multiple repositories. + /// Detection is still explicit and cached: this method does not scan the + /// filesystem. + pub fn get_descendant_roots_for_path(&self, path: &Path) -> Vec { + let Ok(std_path) = StandardizedPath::from_local_canonicalized(path) else { + return Vec::new(); + }; + + let mut roots = self + .repository_roots + .iter() + .filter_map(LocalOrRemotePath::to_local_path) + .filter_map(|repo| StandardizedPath::from_local_canonicalized(repo).ok()) + .filter(|repo| repo != &std_path && repo.starts_with(&std_path)) + .filter_map(|repo| repo.to_local_path()) + .filter(|repo| repo.exists()) + .collect::>(); + roots.sort(); + roots + } + /// Find the local repository that contains the given path, if any. fn find_local_repository_root(&self, path: &StandardizedPath) -> Option { for ancestor in path.ancestors() { @@ -264,6 +274,40 @@ impl DetectedRepositories { LocalOrRemotePath::Remote(remote) => remote.host_id != *host_id, }); } + + #[cfg(feature = "local_fs")] + fn register_git_repo_info( + &mut self, + info: GitRepoInfo, + source: RepoDetectionSource, + ctx: &mut ModelContext, + ) -> Option { + let repo_root_path = info + .working_tree_path + .as_ref() + .and_then(|path| StandardizedPath::from_local_canonicalized(path).ok())?; + + if let Some(local_path) = repo_root_path.to_local_path() { + self.repository_roots + .insert(LocalOrRemotePath::Local(local_path)); + } + + let external_git_dir = + StandardizedPath::from_local_canonicalized(info.git_dir_path.as_path()) + .ok() + // Only treat as external if it's outside the working tree. + .filter(|p| !p.starts_with(&repo_root_path)); + + let repository = DirectoryWatcher::handle(ctx).update(ctx, |watcher, ctx| { + watcher + .add_directory_with_git_dir(repo_root_path, external_git_dir, ctx) + .ok() + })?; + + let repo_path = repository.as_ref(ctx).root_dir().to_local_path(); + ctx.emit(DetectedRepositoriesEvent::DetectedGitRepo { repository, source }); + repo_path + } } impl Entity for DetectedRepositories { @@ -295,6 +339,12 @@ struct GitRepoInfo { git_dir_path: PathBuf, } +#[cfg(feature = "local_fs")] +struct GitRepoDetectionResult { + containing_repo: Option, + child_repos: Vec, +} + /// Finds the Git repository containing the given path, if any. /// /// Supports: @@ -313,56 +363,103 @@ async fn find_git_repo(path: &Path) -> Option { return None; } - // First, check if the current directory is a bare git repository. - if let Some(dir_name) = current.file_name().and_then(|s| s.to_str()) { - if dir_name.ends_with(".git") && is_valid_git_dir(¤t).await { + if let Some(info) = find_git_repo_at_path(¤t).await { + return Some(info); + } + + if !current.pop() { + return None; + } + } +} + +/// Finds a Git repository whose working tree is exactly `path`. +#[cfg(feature = "local_fs")] +async fn find_git_repo_at_path(path: &Path) -> Option { + // First, check if the current directory is a bare git repository. + if let Some(dir_name) = path.file_name().and_then(|s| s.to_str()) { + if dir_name.ends_with(".git") && is_valid_git_dir(path).await { + return Some(GitRepoInfo { + working_tree_path: None, + git_dir_path: path.to_path_buf(), + }); + } + } + + // Check for a .git directory. + let dot_git_path = path.join(".git"); + if let Ok(dot_git_type) = async_fs::symlink_metadata(&dot_git_path) + .await + .map(|m| m.file_type()) + { + if dot_git_type.is_dir() { + // A standard repository with a .git directory. + if is_valid_git_dir(&dot_git_path).await { return Some(GitRepoInfo { - working_tree_path: None, - git_dir_path: current.clone(), + working_tree_path: Some(path.to_path_buf()), + git_dir_path: dot_git_path, }); } - } - - // Check for a .git directory. - let dot_git_path = current.join(".git"); - if let Ok(dot_git_type) = async_fs::symlink_metadata(&dot_git_path) - .await - .map(|m| m.file_type()) - { - if dot_git_type.is_dir() { - // A standard repository with a .git directory. - if is_valid_git_dir(&dot_git_path).await { - return Some(GitRepoInfo { - working_tree_path: Some(current.clone()), - git_dir_path: dot_git_path, - }); - } - } else if dot_git_type.is_file() { - // A potential gitfile, used by worktrees and submodules. - if let Ok(contents) = async_fs::read_to_string(&dot_git_path).await { - // Typical format: "gitdir: \n" - if let Some(rest) = contents.trim().strip_prefix("gitdir:") { - let gitdir_path = PathBuf::from(rest.trim()); - let resolved_gitdir = if gitdir_path.is_absolute() { - gitdir_path - } else { - current.join(gitdir_path) - }; - if is_valid_git_dir(&resolved_gitdir).await { - return Some(GitRepoInfo { - working_tree_path: Some(current.clone()), - git_dir_path: resolved_gitdir, - }); - } + } else if dot_git_type.is_file() { + // A potential gitfile, used by worktrees and submodules. + if let Ok(contents) = async_fs::read_to_string(&dot_git_path).await { + // Typical format: "gitdir: \n" + if let Some(rest) = contents.trim().strip_prefix("gitdir:") { + let gitdir_path = PathBuf::from(rest.trim()); + let resolved_gitdir = if gitdir_path.is_absolute() { + gitdir_path + } else { + path.join(gitdir_path) + }; + if is_valid_git_dir(&resolved_gitdir).await { + return Some(GitRepoInfo { + working_tree_path: Some(path.to_path_buf()), + git_dir_path: resolved_gitdir, + }); } } } } + } - if !current.pop() { - return None; + None +} + +/// Finds git repositories in the direct children of `path`. +#[cfg(feature = "local_fs")] +async fn find_child_git_repos(path: &Path) -> Vec { + use futures::TryStreamExt; + + let Ok(mut entries) = async_fs::read_dir(path).await else { + return Vec::new(); + }; + + let mut child_dirs = Vec::new(); + while let Ok(Some(entry)) = entries.try_next().await { + let child_path = entry.path(); + let Ok(file_type) = entry.file_type().await else { + continue; + }; + if !file_type.is_dir() { + continue; + } + if child_path.file_name().and_then(|name| name.to_str()) == Some(".git") { + continue; + } + child_dirs.push(child_path); + } + + child_dirs.sort(); + + let mut repos = Vec::new(); + for child_dir in child_dirs { + if let Some(info) = find_git_repo_at_path(&child_dir).await { + if info.working_tree_path.as_deref() == Some(child_dir.as_path()) { + repos.push(info); + } } } + repos } /// Checks whether the given directory is a valid Git directory by verifying it contains a HEAD file. diff --git a/crates/repo_metadata/src/repositories_tests.rs b/crates/repo_metadata/src/repositories_tests.rs index f47a548626..a4215de6fb 100644 --- a/crates/repo_metadata/src/repositories_tests.rs +++ b/crates/repo_metadata/src/repositories_tests.rs @@ -75,7 +75,135 @@ fn test_detect_possible_local_git_repo_not_a_git_repo() { } #[test] -fn test_detect_possible_local_git_repo_nested_repo_created_after_parent_registration() { +fn test_detect_possible_git_repo_registers_direct_child_repos() { + VirtualFS::test("detect_child_repos", |dirs, mut vfs| { + vfs.mkdir("workspace"); + stub_git_repository(&mut vfs, "workspace/repo_1"); + stub_git_repository(&mut vfs, "workspace/repo_2"); + + let workspace = dirs.tests().join("workspace"); + let repo_1 = dirs.tests().join("workspace/repo_1"); + let repo_2 = dirs.tests().join("workspace/repo_2"); + + App::test((), |mut app| async move { + let _watcher = app.add_singleton_model(DirectoryWatcher::new); + let repo_handle = app.add_model(|_| DetectedRepositories::default()); + + repo_handle + .update(&mut app, |repo, ctx| { + std::mem::drop(repo.detect_possible_local_git_repo( + &workspace.to_string_lossy(), + RepoDetectionSource::TerminalNavigation, + ctx, + )); + let future_id = repo.spawned_futures().last().unwrap(); + ctx.await_spawned_future(*future_id) + }) + .await; + + let repo_1_canonical = StandardizedPath::from_local_canonicalized(&repo_1).unwrap(); + let repo_2_canonical = StandardizedPath::from_local_canonicalized(&repo_2).unwrap(); + repo_handle.read(&app, |repo, _ctx| { + assert!(repo.repository_roots.contains(&LocalOrRemotePath::Local( + repo_1_canonical.to_local_path().unwrap() + ))); + assert!(repo.repository_roots.contains(&LocalOrRemotePath::Local( + repo_2_canonical.to_local_path().unwrap() + ))); + }); + }); + }); +} + +#[test] +fn test_detect_possible_git_repo_discovers_child_repo_created_after_empty_scan() { + VirtualFS::test("detect_child_repo_after_empty_scan", |dirs, mut vfs| { + vfs.mkdir("workspace"); + + let workspace = dirs.tests().join("workspace"); + let repo_created_later = dirs.tests().join("workspace/repo_created_later"); + + App::test((), |mut app| async move { + let _watcher = app.add_singleton_model(DirectoryWatcher::new); + let repo_handle = app.add_model(|_| DetectedRepositories::default()); + + repo_handle + .update(&mut app, |repo, ctx| { + std::mem::drop(repo.detect_possible_local_git_repo( + &workspace.to_string_lossy(), + RepoDetectionSource::TerminalNavigation, + ctx, + )); + let future_id = repo.spawned_futures().last().unwrap(); + ctx.await_spawned_future(*future_id) + }) + .await; + + repo_handle.read(&app, |repo, _ctx| { + assert!( + repo.repository_roots.is_empty(), + "empty workspace scan should not register repositories" + ); + }); + + stub_git_repository(&mut vfs, "workspace/repo_created_later"); + + repo_handle + .update(&mut app, |repo, ctx| { + std::mem::drop(repo.detect_possible_local_git_repo( + &workspace.to_string_lossy(), + RepoDetectionSource::TerminalNavigation, + ctx, + )); + let future_id = repo.spawned_futures().last().unwrap(); + ctx.await_spawned_future(*future_id) + }) + .await; + + let repo_canonical = + StandardizedPath::from_local_canonicalized(&repo_created_later).unwrap(); + repo_handle.read(&app, |repo, _ctx| { + assert!( + repo.repository_roots.contains(&LocalOrRemotePath::Local( + repo_canonical.to_local_path().unwrap() + )), + "subsequent parent scan should discover newly-created child repos" + ); + }); + }); + }); +} + +#[test] +fn test_get_descendant_roots_for_path_omits_deleted_repos() { + VirtualFS::test("descendant_roots_omit_deleted_repos", |dirs, mut vfs| { + vfs.mkdir("workspace/repo_1"); + vfs.mkdir("workspace/repo_2"); + + let workspace = dirs.tests().join("workspace"); + let repo_1 = dirs.tests().join("workspace/repo_1"); + let repo_2 = dirs.tests().join("workspace/repo_2"); + let repo_1_canonical = StandardizedPath::from_local_canonicalized(&repo_1).unwrap(); + let repo_2_canonical = StandardizedPath::from_local_canonicalized(&repo_2).unwrap(); + + let mut repos = DetectedRepositories::default(); + repos.repository_roots.insert(LocalOrRemotePath::Local( + repo_1_canonical.to_local_path().unwrap(), + )); + repos.repository_roots.insert(LocalOrRemotePath::Local( + repo_2_canonical.to_local_path().unwrap(), + )); + + fs::remove_dir_all(&repo_2).expect("remove repo_2"); + + let descendant_roots = repos.get_descendant_roots_for_path(&workspace); + + assert_eq!(descendant_roots, vec![repo_1]); + }); +} + +#[test] +fn test_detect_possible_git_repo_nested_repo_created_after_parent_registration() { VirtualFS::test("detect_nested_repo", |dirs, mut vfs| { // Create a parent git repository structure stub_git_repository(&mut vfs, "parent_repo");