From e318f2bdacf8a85a6444218a21b9b76892a0f51b Mon Sep 17 00:00:00 2001 From: Richard Feldman Date: Wed, 1 Apr 2026 13:51:37 -0400 Subject: [PATCH] Disambiguate project names in the sidebar Extract the tab disambiguation algorithm from pane.rs into a generic util::disambiguate module, then use it to disambiguate sidebar project group names when multiple projects share the same leaf directory name. The disambiguation works at the individual path level across all groups, so even a multi-worktree group like 'code/zed + bar/zed' gets each of its paths disambiguated independently against paths from other groups. Also adds visual tests covering the two-project and three-project (with multi-worktree) scenarios. --- crates/sidebar/src/project_group_builder.rs | 101 ++++++- crates/sidebar/src/sidebar.rs | 17 +- crates/util/src/disambiguate.rs | 147 ++++++++++ crates/util/src/util.rs | 1 + crates/workspace/src/pane.rs | 33 +-- crates/zed/src/visual_test_runner.rs | 294 ++++++++++++++++++++ 6 files changed, 556 insertions(+), 37 deletions(-) create mode 100644 crates/util/src/disambiguate.rs diff --git a/crates/sidebar/src/project_group_builder.rs b/crates/sidebar/src/project_group_builder.rs index 0b8e56ac99565218dd827048afdee71e896f2667..221a68aea97736133f8563354d41dfc4495f59e8 100644 --- a/crates/sidebar/src/project_group_builder.rs +++ b/crates/sidebar/src/project_group_builder.rs @@ -10,7 +10,7 @@ use collections::{HashMap, HashSet, vecmap::VecMap}; use std::{ - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, sync::Arc, }; @@ -28,16 +28,32 @@ pub struct ProjectGroupName { path_list: PathList, } +pub(crate) fn path_suffix(path: &Path, detail: usize) -> String { + let components: Vec<_> = path + .components() + .filter_map(|c| match c { + Component::Normal(s) => Some(s.to_string_lossy()), + _ => None, + }) + .collect(); + let start = components.len().saturating_sub(detail + 1); + components[start..].join("/") +} + impl ProjectGroupName { - pub fn display_name(&self) -> SharedString { + pub fn display_name_from_suffixes( + &self, + path_detail_map: &HashMap, + ) -> SharedString { let mut names = Vec::with_capacity(self.path_list.paths().len()); for abs_path in self.path_list.paths() { - if let Some(name) = abs_path.file_name() { - names.push(name.to_string_lossy().to_string()); + let detail = path_detail_map.get(abs_path).copied().unwrap_or(0); + let suffix = path_suffix(abs_path, detail); + if !suffix.is_empty() { + names.push(suffix); } } if names.is_empty() { - // TODO: Can we do something better in this case? "Empty Workspace".into() } else { names.join(", ").into() @@ -297,6 +313,81 @@ mod tests { }); } + fn group_name_from_paths(paths: &[&str]) -> ProjectGroupName { + ProjectGroupName { + path_list: PathList::new(paths), + } + } + + #[test] + fn test_path_suffix_detail_zero() { + assert_eq!(path_suffix(Path::new("/a/b/c"), 0), "c"); + } + + #[test] + fn test_path_suffix_detail_one() { + assert_eq!(path_suffix(Path::new("/a/b/c"), 1), "b/c"); + } + + #[test] + fn test_path_suffix_detail_two() { + assert_eq!(path_suffix(Path::new("/a/b/c"), 2), "a/b/c"); + } + + #[test] + fn test_path_suffix_clamped() { + let result = path_suffix(Path::new("/a/b"), 5); + assert_eq!(result, "a/b"); + } + + #[test] + fn test_display_name_from_suffixes_single_path() { + let name = group_name_from_paths(&["/code/zed"]); + let map = HashMap::default(); + assert_eq!(name.display_name_from_suffixes(&map).as_ref(), "zed"); + + let map = HashMap::from_iter([(PathBuf::from("/code/zed"), 1)]); + assert_eq!(name.display_name_from_suffixes(&map).as_ref(), "code/zed"); + } + + #[test] + fn test_display_name_from_suffixes_multiple_paths() { + let name = group_name_from_paths(&["/a/zed", "/b/bar"]); + + let map = HashMap::default(); + assert_eq!( + name.display_name_from_suffixes(&map).as_ref(), + "zed, bar", + "PathList sorts lexicographically, so /a/zed comes before /b/bar" + ); + + let map = HashMap::from_iter([(PathBuf::from("/a/zed"), 1), (PathBuf::from("/b/bar"), 0)]); + assert_eq!(name.display_name_from_suffixes(&map).as_ref(), "a/zed, bar"); + } + + #[test] + fn test_display_name_from_suffixes_empty() { + let name = group_name_from_paths(&[]); + let map = HashMap::default(); + assert_eq!( + name.display_name_from_suffixes(&map).as_ref(), + "Empty Workspace" + ); + } + + #[test] + fn test_display_name_from_suffixes_per_path_detail() { + let name = group_name_from_paths(&["/code/zed", "/code/bar/zed"]); + let map = HashMap::from_iter([ + (PathBuf::from("/code/zed"), 1), + (PathBuf::from("/code/bar/zed"), 1), + ]); + assert_eq!( + name.display_name_from_suffixes(&map).as_ref(), + "bar/zed, code/zed", + ); + } + #[gpui::test] async fn test_worktree_checkout_canonicalizes_to_main_repo(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index e09ee3e8809417924b1b1b43f25cee75834568a1..616535b65a5f7ad6a9e622b0165fb67e93b7e28f 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -779,13 +779,26 @@ impl Sidebar { (icon, icon_from_external_svg) }; - for (group_name, group) in project_groups.groups() { + let groups: Vec<_> = project_groups.groups().collect(); + + let all_paths: Vec = groups + .iter() + .flat_map(|(name, _)| name.path_list().paths().iter().cloned()) + .collect(); + let path_details = + util::disambiguate::compute_disambiguation_details(&all_paths, |path, detail| { + crate::project_group_builder::path_suffix(path, detail) + }); + let path_detail_map: collections::HashMap = + all_paths.into_iter().zip(path_details).collect(); + + for (group_name, group) in &groups { let path_list = group_name.path_list().clone(); if path_list.paths().is_empty() { continue; } - let label = group_name.display_name(); + let label = group_name.display_name_from_suffixes(&path_detail_map); let is_collapsed = self.collapsed_groups.contains(&path_list); let should_load_threads = !is_collapsed || !query.is_empty(); diff --git a/crates/util/src/disambiguate.rs b/crates/util/src/disambiguate.rs new file mode 100644 index 0000000000000000000000000000000000000000..08580b467f67bcd2e22f59935ff867f298413005 --- /dev/null +++ b/crates/util/src/disambiguate.rs @@ -0,0 +1,147 @@ +use std::collections::HashMap; +use std::hash::Hash; + +/// Computes the minimum detail level needed for each item so that no two items +/// share the same description. Items whose descriptions are unique at level 0 +/// stay at 0; items that collide get their detail level incremented until either +/// the collision is resolved or increasing the level no longer changes the +/// description (preventing infinite loops for truly identical items). +pub fn compute_disambiguation_details( + items: &[T], + get_description: impl Fn(&T, usize) -> D, +) -> Vec +where + D: Eq + Hash + Clone, +{ + let mut details = vec![0usize; items.len()]; + let mut descriptions: HashMap> = HashMap::default(); + let mut current_descriptions: Vec = + items.iter().map(|item| get_description(item, 0)).collect(); + + loop { + let mut any_collisions = false; + + for (index, (item, &detail)) in items.iter().zip(&details).enumerate() { + if detail > 0 { + let new_description = get_description(item, detail); + if new_description == current_descriptions[index] { + continue; + } + current_descriptions[index] = new_description; + } + descriptions + .entry(current_descriptions[index].clone()) + .or_insert_with(Vec::new) + .push(index); + } + + for (_, indices) in descriptions.drain() { + if indices.len() > 1 { + any_collisions = true; + for index in indices { + details[index] += 1; + } + } + } + + if !any_collisions { + break; + } + } + + details +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_no_conflicts() { + let items = vec!["alpha", "beta", "gamma"]; + let details = compute_disambiguation_details(&items, |item, _detail| item.to_string()); + assert_eq!(details, vec![0, 0, 0]); + } + + #[test] + fn test_simple_two_way_conflict() { + // Two items with the same base name but different parents. + let items = vec![("src/foo.rs", "foo.rs"), ("lib/foo.rs", "foo.rs")]; + let details = compute_disambiguation_details(&items, |item, detail| match detail { + 0 => item.1.to_string(), + _ => item.0.to_string(), + }); + assert_eq!(details, vec![1, 1]); + } + + #[test] + fn test_three_way_conflict() { + let items = vec![ + ("foo.rs", "a/foo.rs"), + ("foo.rs", "b/foo.rs"), + ("foo.rs", "c/foo.rs"), + ]; + let details = compute_disambiguation_details(&items, |item, detail| match detail { + 0 => item.0.to_string(), + _ => item.1.to_string(), + }); + assert_eq!(details, vec![1, 1, 1]); + } + + #[test] + fn test_deeper_conflict() { + // At detail 0, all three show "file.rs". + // At detail 1, items 0 and 1 both show "src/file.rs", item 2 shows "lib/file.rs". + // At detail 2, item 0 shows "a/src/file.rs", item 1 shows "b/src/file.rs". + let items = vec![ + vec!["file.rs", "src/file.rs", "a/src/file.rs"], + vec!["file.rs", "src/file.rs", "b/src/file.rs"], + vec!["file.rs", "lib/file.rs", "x/lib/file.rs"], + ]; + let details = compute_disambiguation_details(&items, |item, detail| { + let clamped = detail.min(item.len() - 1); + item[clamped].to_string() + }); + assert_eq!(details, vec![2, 2, 1]); + } + + #[test] + fn test_mixed_conflicting_and_unique() { + let items = vec![ + ("src/foo.rs", "foo.rs"), + ("lib/foo.rs", "foo.rs"), + ("src/bar.rs", "bar.rs"), + ]; + let details = compute_disambiguation_details(&items, |item, detail| match detail { + 0 => item.1.to_string(), + _ => item.0.to_string(), + }); + assert_eq!(details, vec![1, 1, 0]); + } + + #[test] + fn test_identical_items_terminates() { + // All items return the same description at every detail level. + // The algorithm must terminate rather than looping forever. + let items = vec!["same", "same", "same"]; + let details = compute_disambiguation_details(&items, |item, _detail| item.to_string()); + // After bumping to 1, the description doesn't change from level 0, + // so the items are skipped and the loop terminates. + assert_eq!(details, vec![1, 1, 1]); + } + + #[test] + fn test_single_item() { + let items = vec!["only"]; + let details = compute_disambiguation_details(&items, |item, _detail| item.to_string()); + assert_eq!(details, vec![0]); + } + + #[test] + fn test_empty_input() { + let items: Vec<&str> = vec![]; + let details = compute_disambiguation_details(&items, |item, _detail| item.to_string()); + let expected: Vec = vec![]; + assert_eq!(details, expected); + } +} diff --git a/crates/util/src/util.rs b/crates/util/src/util.rs index bd8ab4e2d4d99864c5e0dc228410904f3338d7c6..3b704e50a531c5302024e215754cb9a866f0036b 100644 --- a/crates/util/src/util.rs +++ b/crates/util/src/util.rs @@ -1,5 +1,6 @@ pub mod archive; pub mod command; +pub mod disambiguate; pub mod fs; pub mod markdown; pub mod path_list; diff --git a/crates/workspace/src/pane.rs b/crates/workspace/src/pane.rs index deb7e1efef37acff992d8f5be5825741e887b979..356e17e3ce51c8e13f412e90b7cf815a7b3bd260 100644 --- a/crates/workspace/src/pane.rs +++ b/crates/workspace/src/pane.rs @@ -4841,36 +4841,9 @@ fn dirty_message_for(buffer_path: Option, path_style: PathStyle) -> } pub fn tab_details(items: &[Box], _window: &Window, cx: &App) -> Vec { - let mut tab_details = items.iter().map(|_| 0).collect::>(); - let mut tab_descriptions = HashMap::default(); - let mut done = false; - while !done { - done = true; - - // Store item indices by their tab description. - for (ix, (item, detail)) in items.iter().zip(&tab_details).enumerate() { - let description = item.tab_content_text(*detail, cx); - if *detail == 0 || description != item.tab_content_text(detail - 1, cx) { - tab_descriptions - .entry(description) - .or_insert(Vec::new()) - .push(ix); - } - } - - // If two or more items have the same tab description, increase their level - // of detail and try again. - for (_, item_ixs) in tab_descriptions.drain() { - if item_ixs.len() > 1 { - done = false; - for ix in item_ixs { - tab_details[ix] += 1; - } - } - } - } - - tab_details + util::disambiguate::compute_disambiguation_details(items, |item, detail| { + item.tab_content_text(detail, cx) + }) } pub fn render_item_indicator(item: Box, cx: &App) -> Option { diff --git a/crates/zed/src/visual_test_runner.rs b/crates/zed/src/visual_test_runner.rs index 27776eba84a10c5314c5e28fab430ad61e65bbf2..dcd2ec81ee44a091c9b444e79177145cf5cfceb7 100644 --- a/crates/zed/src/visual_test_runner.rs +++ b/crates/zed/src/visual_test_runner.rs @@ -568,6 +568,27 @@ fn run_visual_tests(project_path: PathBuf, update_baseline: bool) -> Result<()> } } + // Run Test: Sidebar with duplicate project names + println!("\n--- Test: sidebar_duplicate_names ---"); + match run_sidebar_duplicate_project_names_visual_tests( + app_state.clone(), + &mut cx, + update_baseline, + ) { + Ok(TestResult::Passed) => { + println!("✓ sidebar_duplicate_names: PASSED"); + passed += 1; + } + Ok(TestResult::BaselineUpdated(_)) => { + println!("✓ sidebar_duplicate_names: Baselines updated"); + updated += 1; + } + Err(e) => { + eprintln!("✗ sidebar_duplicate_names: FAILED - {}", e); + failed += 1; + } + } + // Run Test 9: Tool Permissions Settings UI visual test println!("\n--- Test 9: tool_permissions_settings ---"); match run_tool_permissions_visual_tests(app_state.clone(), &mut cx, update_baseline) { @@ -3069,6 +3090,279 @@ fn run_git_command(args: &[&str], dir: &std::path::Path) -> Result<()> { Ok(()) } +#[cfg(target_os = "macos")] +/// Helper to create a project, add a worktree at the given path, and return the project. +fn create_project_with_worktree( + worktree_dir: &Path, + app_state: &Arc, + cx: &mut VisualTestAppContext, +) -> Result> { + let project = cx.update(|cx| { + project::Project::local( + app_state.client.clone(), + app_state.node_runtime.clone(), + app_state.user_store.clone(), + app_state.languages.clone(), + app_state.fs.clone(), + None, + project::LocalProjectFlags { + init_worktree_trust: false, + ..Default::default() + }, + cx, + ) + }); + + let add_task = cx.update(|cx| { + project.update(cx, |project, cx| { + project.find_or_create_worktree(worktree_dir, true, cx) + }) + }); + + cx.background_executor.allow_parking(); + cx.foreground_executor + .block_test(add_task) + .context("Failed to add worktree")?; + cx.background_executor.forbid_parking(); + + cx.run_until_parked(); + Ok(project) +} + +#[cfg(target_os = "macos")] +fn open_sidebar_test_window( + projects: Vec>, + app_state: &Arc, + cx: &mut VisualTestAppContext, +) -> Result> { + anyhow::ensure!(!projects.is_empty(), "need at least one project"); + + let window_size = size(px(400.0), px(600.0)); + let bounds = Bounds { + origin: point(px(0.0), px(0.0)), + size: window_size, + }; + + let mut projects_iter = projects.into_iter(); + let first_project = projects_iter + .next() + .ok_or_else(|| anyhow::anyhow!("need at least one project"))?; + let remaining: Vec<_> = projects_iter.collect(); + + let multi_workspace_window: WindowHandle = cx + .update(|cx| { + cx.open_window( + WindowOptions { + window_bounds: Some(WindowBounds::Windowed(bounds)), + focus: false, + show: false, + ..Default::default() + }, + |window, cx| { + let first_ws = cx.new(|cx| { + Workspace::new(None, first_project.clone(), app_state.clone(), window, cx) + }); + cx.new(|cx| { + let mut mw = MultiWorkspace::new(first_ws, window, cx); + for project in remaining { + let ws = cx.new(|cx| { + Workspace::new(None, project, app_state.clone(), window, cx) + }); + mw.activate(ws, window, cx); + } + mw + }) + }, + ) + }) + .context("Failed to open MultiWorkspace window")?; + + cx.run_until_parked(); + + // Create the sidebar outside the MultiWorkspace update to avoid a + // re-entrant read panic (Sidebar::new reads the MultiWorkspace). + let sidebar = cx + .update_window(multi_workspace_window.into(), |root_view, window, cx| { + let mw_handle: Entity = root_view + .downcast() + .map_err(|_| anyhow::anyhow!("Failed to downcast root view to MultiWorkspace"))?; + Ok(cx.new(|cx| sidebar::Sidebar::new(mw_handle, window, cx))) + }) + .context("Failed to create sidebar")??; + + multi_workspace_window + .update(cx, |mw, _window, cx| { + mw.register_sidebar(sidebar.clone(), cx); + }) + .context("Failed to register sidebar")?; + + cx.run_until_parked(); + + // Open the sidebar + multi_workspace_window + .update(cx, |mw, window, cx| { + mw.toggle_sidebar(window, cx); + }) + .context("Failed to toggle sidebar")?; + + // Let rendering settle + for _ in 0..10 { + cx.advance_clock(Duration::from_millis(100)); + cx.run_until_parked(); + } + + // Refresh the window + cx.update_window(multi_workspace_window.into(), |_, window, _cx| { + window.refresh(); + })?; + + cx.run_until_parked(); + + Ok(multi_workspace_window) +} + +#[cfg(target_os = "macos")] +fn cleanup_sidebar_test_window( + window: WindowHandle, + cx: &mut VisualTestAppContext, +) -> Result<()> { + window.update(cx, |mw, _window, cx| { + for workspace in mw.workspaces() { + let project = workspace.read(cx).project().clone(); + project.update(cx, |project, cx| { + let ids: Vec<_> = project.worktrees(cx).map(|wt| wt.read(cx).id()).collect(); + for id in ids { + project.remove_worktree(id, cx); + } + }); + } + })?; + + cx.run_until_parked(); + + cx.update_window(window.into(), |_, window, _cx| { + window.remove_window(); + })?; + + cx.run_until_parked(); + + for _ in 0..15 { + cx.advance_clock(Duration::from_millis(100)); + cx.run_until_parked(); + } + + Ok(()) +} + +#[cfg(target_os = "macos")] +fn run_sidebar_duplicate_project_names_visual_tests( + app_state: Arc, + cx: &mut VisualTestAppContext, + update_baseline: bool, +) -> Result { + let temp_dir = tempfile::tempdir()?; + let temp_path = temp_dir.keep(); + let canonical_temp = temp_path.canonicalize()?; + + // Create directory structure where every leaf directory is named "zed" but + // lives at a distinct path. This lets us test that the sidebar correctly + // disambiguates projects whose names would otherwise collide. + // + // code/zed/ — project1 (single worktree) + // code/foo/zed/ — project2 (single worktree) + // code/bar/zed/ — project3, first worktree + // code/baz/zed/ — project3, second worktree + // + // No two projects share a worktree path, so ProjectGroupBuilder will + // place each in its own group. + let code_zed = canonical_temp.join("code").join("zed"); + let foo_zed = canonical_temp.join("code").join("foo").join("zed"); + let bar_zed = canonical_temp.join("code").join("bar").join("zed"); + let baz_zed = canonical_temp.join("code").join("baz").join("zed"); + std::fs::create_dir_all(&code_zed)?; + std::fs::create_dir_all(&foo_zed)?; + std::fs::create_dir_all(&bar_zed)?; + std::fs::create_dir_all(&baz_zed)?; + + cx.update(|cx| { + cx.update_flags(true, vec!["agent-v2".to_string()]); + }); + + let mut has_baseline_update = None; + + // --- Test 1: Two single-worktree projects whose leaf name is "zed" --- + { + let project1 = create_project_with_worktree(&code_zed, &app_state, cx)?; + let project2 = create_project_with_worktree(&foo_zed, &app_state, cx)?; + + let window = open_sidebar_test_window(vec![project1, project2], &app_state, cx)?; + + let result = run_visual_test( + "sidebar_two_projects_same_leaf_name", + window.into(), + cx, + update_baseline, + ); + + cleanup_sidebar_test_window(window, cx)?; + match result? { + TestResult::Passed => {} + TestResult::BaselineUpdated(path) => { + has_baseline_update = Some(path); + } + } + } + + // --- Test 2: Three projects, third has two worktrees (all leaf names "zed") --- + // + // project1: code/zed + // project2: code/foo/zed + // project3: code/bar/zed + code/baz/zed + // + // Each project has a unique set of worktree paths, so they form + // separate groups. The sidebar must disambiguate all three. + { + let project1 = create_project_with_worktree(&code_zed, &app_state, cx)?; + let project2 = create_project_with_worktree(&foo_zed, &app_state, cx)?; + + let project3 = create_project_with_worktree(&bar_zed, &app_state, cx)?; + let add_second_worktree = cx.update(|cx| { + project3.update(cx, |project, cx| { + project.find_or_create_worktree(&baz_zed, true, cx) + }) + }); + cx.background_executor.allow_parking(); + cx.foreground_executor + .block_test(add_second_worktree) + .context("Failed to add second worktree to project 3")?; + cx.background_executor.forbid_parking(); + cx.run_until_parked(); + + let window = open_sidebar_test_window(vec![project1, project2, project3], &app_state, cx)?; + + let result = run_visual_test( + "sidebar_three_projects_with_multi_worktree", + window.into(), + cx, + update_baseline, + ); + + cleanup_sidebar_test_window(window, cx)?; + match result? { + TestResult::Passed => {} + TestResult::BaselineUpdated(path) => { + has_baseline_update = Some(path); + } + } + } + + if let Some(path) = has_baseline_update { + Ok(TestResult::BaselineUpdated(path)) + } else { + Ok(TestResult::Passed) + } +} + #[cfg(all(target_os = "macos", feature = "visual-tests"))] fn run_start_thread_in_selector_visual_tests( app_state: Arc,