diff --git a/crates/recent_projects/src/recent_projects.rs b/crates/recent_projects/src/recent_projects.rs index 9a6015ba843b06dfe678fee1b5de2fac38295849..e53e2b67965e900890170c05620d27446dead33a 100644 --- a/crates/recent_projects/src/recent_projects.rs +++ b/crates/recent_projects/src/recent_projects.rs @@ -1133,8 +1133,12 @@ impl PickerDelegate for RecentProjectsDelegate { cx.defer(move |cx| { if let Some(task) = handle .update(cx, |multi_workspace, window, cx| { - multi_workspace - .find_or_create_local_workspace(path_list, window, cx) + multi_workspace.find_or_create_local_workspace( + path_list, + &[], + window, + cx, + ) }) .log_err() { diff --git a/crates/sidebar/src/sidebar.rs b/crates/sidebar/src/sidebar.rs index 57fe5a04ac6e656f790d72b1a99dff3e14fa8ead..8dbf155ebd129acb92159e0d34bba6557faf32f3 100644 --- a/crates/sidebar/src/sidebar.rs +++ b/crates/sidebar/src/sidebar.rs @@ -2776,11 +2776,12 @@ impl Sidebar { .clone() }); + let excluded = [workspace_to_remove.clone()]; let remove_task = multi_workspace.update(cx, |mw, cx| { mw.remove( [workspace_to_remove], move |this, window, cx| { - this.find_or_create_local_workspace(fallback_paths, window, cx) + this.find_or_create_local_workspace(fallback_paths, &excluded, window, cx) }, window, cx, diff --git a/crates/workspace/src/multi_workspace.rs b/crates/workspace/src/multi_workspace.rs index f4e8b47399e1420a4b01d380ad4a6532a0934a2d..ef1fcd3b03aadcbeaca65a7ef9dacf73ea51bc75 100644 --- a/crates/workspace/src/multi_workspace.rs +++ b/crates/workspace/src/multi_workspace.rs @@ -822,12 +822,14 @@ impl MultiWorkspace { // Now remove the key. self.project_group_keys.retain(|k| k != key); + let excluded_workspaces = workspaces.clone(); self.remove( workspaces, move |this, window, cx| { if let Some(neighbor_key) = neighbor_key { return this.find_or_create_local_workspace( neighbor_key.path_list().clone(), + &excluded_workspaces, window, cx, ); @@ -860,10 +862,23 @@ impl MultiWorkspace { path_list: &PathList, host: Option<&RemoteConnectionOptions>, cx: &App, + ) -> Option> { + self.workspace_for_paths_excluding(path_list, host, &[], cx) + } + + fn workspace_for_paths_excluding( + &self, + path_list: &PathList, + host: Option<&RemoteConnectionOptions>, + excluding: &[Entity], + cx: &App, ) -> Option> { self.workspaces .iter() .find(|ws| { + if excluding.contains(ws) { + return false; + } let key = ws.read(cx).project_group_key(cx); key.host().as_ref() == host && PathList::new(&ws.read(cx).root_paths(cx)) == *path_list @@ -904,7 +919,7 @@ impl MultiWorkspace { } let Some(connection_options) = host else { - return self.find_or_create_local_workspace(paths, window, cx); + return self.find_or_create_local_workspace(paths, &[], window, cx); }; let app_state = self.workspace().read(cx).app_state().clone(); @@ -956,19 +971,26 @@ impl MultiWorkspace { /// or creates a new one (deserializing its saved state from the database). /// Never searches other windows or matches workspaces with a superset of /// the requested paths. + /// + /// `excluding` lists workspaces that should be skipped during the search + /// (e.g. workspaces that are about to be removed). pub fn find_or_create_local_workspace( &mut self, path_list: PathList, + excluding: &[Entity], window: &mut Window, cx: &mut Context, ) -> Task>> { - if let Some(workspace) = self.workspace_for_paths(&path_list, None, cx) { + if let Some(workspace) = self.workspace_for_paths_excluding(&path_list, None, excluding, cx) + { self.activate(workspace.clone(), window, cx); return Task::ready(Ok(workspace)); } if let Some(transient) = self.active_workspace.transient_workspace() { - if transient.read(cx).project_group_key(cx).path_list() == &path_list { + if transient.read(cx).project_group_key(cx).path_list() == &path_list + && !excluding.contains(transient) + { return Task::ready(Ok(transient.clone())); } } @@ -1503,7 +1525,7 @@ impl MultiWorkspace { cx: &mut Context, ) -> Task>> { if self.multi_workspace_enabled(cx) { - self.find_or_create_local_workspace(PathList::new(&paths), window, cx) + self.find_or_create_local_workspace(PathList::new(&paths), &[], window, cx) } else { let workspace = self.workspace().clone(); cx.spawn_in(window, async move |_this, cx| { diff --git a/crates/workspace/src/persistence.rs b/crates/workspace/src/persistence.rs index 67383740a8b3287bb237748776b0c7ab2654d7ba..584c53f3aedbc0f71c60133a625a2d60ec21cdf4 100644 --- a/crates/workspace/src/persistence.rs +++ b/crates/workspace/src/persistence.rs @@ -2493,6 +2493,7 @@ pub fn delete_unloaded_items( #[cfg(test)] mod tests { use super::*; + use crate::PathList; use crate::{ multi_workspace::MultiWorkspace, persistence::{ @@ -5014,4 +5015,79 @@ mod tests { "After removing the only remaining group, should have an empty workspace" ); } + + /// Regression test for a crash where `find_or_create_local_workspace` + /// returned a workspace that was about to be removed, hitting an assert + /// in `MultiWorkspace::remove`. + /// + /// The scenario: two workspaces share the same root paths (e.g. due to + /// a provisional key mismatch). When the first is removed and the + /// fallback searches for the same paths, `workspace_for_paths` must + /// skip the doomed workspace so the assert in `remove` is satisfied. + #[gpui::test] + async fn test_remove_fallback_skips_excluded_workspaces(cx: &mut gpui::TestAppContext) { + crate::tests::init_test(cx); + cx.update(|cx| { + cx.set_staff(true); + cx.update_flags(true, vec!["agent-v2".to_string()]); + }); + + let fs = fs::FakeFs::new(cx.executor()); + let dir = unique_test_dir(&fs, "shared").await; + + // Two projects that open the same directory — this creates two + // workspaces whose root_paths are identical. + let project_a = Project::test(fs.clone(), [dir.as_path()], cx).await; + let project_b = Project::test(fs.clone(), [dir.as_path()], cx).await; + + let (multi_workspace, cx) = cx + .add_window_view(|window, cx| MultiWorkspace::test_new(project_a.clone(), window, cx)); + + multi_workspace.update(cx, |mw, cx| mw.open_sidebar(cx)); + + let workspace_b = multi_workspace.update_in(cx, |mw, window, cx| { + mw.test_add_workspace(project_b.clone(), window, cx) + }); + cx.run_until_parked(); + + // workspace_a is first in the workspaces vec. + let workspace_a = + multi_workspace.read_with(cx, |mw, _| mw.workspaces().next().cloned().unwrap()); + assert_ne!(workspace_a, workspace_b); + + // Activate workspace_a so removing it triggers the fallback path. + multi_workspace.update_in(cx, |mw, window, cx| { + mw.activate(workspace_a.clone(), window, cx); + }); + cx.run_until_parked(); + + // Remove workspace_a. The fallback searches for the same paths. + // Without the `excluding` parameter, `workspace_for_paths` would + // return workspace_a (first match) and the assert in `remove` + // would fire. With the fix, workspace_a is skipped and + // workspace_b is found instead. + let path_list = PathList::new(std::slice::from_ref(&dir)); + let excluded = vec![workspace_a.clone()]; + multi_workspace.update_in(cx, |mw, window, cx| { + mw.remove( + vec![workspace_a.clone()], + move |this, window, cx| { + this.find_or_create_local_workspace(path_list, &excluded, window, cx) + }, + window, + cx, + ) + .detach_and_log_err(cx); + }); + cx.run_until_parked(); + + // workspace_b should now be active — workspace_a was removed. + multi_workspace.read_with(cx, |mw, _cx| { + assert_eq!( + mw.workspace(), + &workspace_b, + "fallback should have found workspace_b, not the excluded workspace_a" + ); + }); + } }