sidebar: Load archived threads in the correct window (#51831)

Ben Brandt and cameron created

Make sure we don't create a duplicate workspace if it is already open in
another window.

Release Notes:

- N/A

---------

Co-authored-by: cameron <cameron.studdstreet@gmail.com>

Change summary

crates/sidebar/src/sidebar.rs | 402 ++++++++++++++++++++++++++++++++++--
1 file changed, 373 insertions(+), 29 deletions(-)

Detailed changes

crates/sidebar/src/sidebar.rs 🔗

@@ -10,7 +10,7 @@ use editor::Editor;
 use feature_flags::{AgentV2FeatureFlag, FeatureFlagViewExt as _};
 use gpui::{
     Action as _, AnyElement, App, Context, Entity, FocusHandle, Focusable, ListState, Pixels,
-    Render, SharedString, WeakEntity, Window, actions, list, prelude::*, px,
+    Render, SharedString, WeakEntity, Window, WindowHandle, actions, list, prelude::*, px,
 };
 use menu::{Cancel, Confirm, SelectFirst, SelectLast, SelectNext, SelectPrevious};
 use project::{AgentId, Event as ProjectEvent};
@@ -1656,27 +1656,48 @@ impl Sidebar {
         }
     }
 
-    fn activate_thread(
-        &mut self,
+    fn find_workspace_across_windows(
+        &self,
+        cx: &App,
+        predicate: impl Fn(&Entity<Workspace>, &App) -> bool,
+    ) -> Option<(WindowHandle<MultiWorkspace>, Entity<Workspace>)> {
+        cx.windows()
+            .into_iter()
+            .filter_map(|window| window.downcast::<MultiWorkspace>())
+            .find_map(|window| {
+                let workspace = window.read(cx).ok().and_then(|multi_workspace| {
+                    multi_workspace
+                        .workspaces()
+                        .iter()
+                        .find(|workspace| predicate(workspace, cx))
+                        .cloned()
+                })?;
+                Some((window, workspace))
+            })
+    }
+
+    fn find_workspace_in_current_window(
+        &self,
+        cx: &App,
+        predicate: impl Fn(&Entity<Workspace>, &App) -> bool,
+    ) -> Option<Entity<Workspace>> {
+        self.multi_workspace.upgrade().and_then(|multi_workspace| {
+            multi_workspace
+                .read(cx)
+                .workspaces()
+                .iter()
+                .find(|workspace| predicate(workspace, cx))
+                .cloned()
+        })
+    }
+
+    fn load_agent_thread_in_workspace(
+        workspace: &Entity<Workspace>,
         agent: Agent,
         session_info: acp_thread::AgentSessionInfo,
-        workspace: &Entity<Workspace>,
         window: &mut Window,
-        cx: &mut Context<Self>,
+        cx: &mut App,
     ) {
-        let Some(multi_workspace) = self.multi_workspace.upgrade() else {
-            return;
-        };
-
-        // Set focused_thread eagerly so the sidebar highlight updates
-        // immediately, rather than waiting for a deferred AgentPanel
-        // event which can race with ActiveWorkspaceChanged clearing it.
-        self.focused_thread = Some(session_info.session_id.clone());
-
-        multi_workspace.update(cx, |multi_workspace, cx| {
-            multi_workspace.activate(workspace.clone(), cx);
-        });
-
         workspace.update(cx, |workspace, cx| {
             workspace.open_panel::<AgentPanel>(window, cx);
         });
@@ -1694,10 +1715,95 @@ impl Sidebar {
                 );
             });
         }
+    }
+
+    fn activate_thread_locally(
+        &mut self,
+        agent: Agent,
+        session_info: acp_thread::AgentSessionInfo,
+        workspace: &Entity<Workspace>,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
+        let Some(multi_workspace) = self.multi_workspace.upgrade() else {
+            return;
+        };
+
+        // Set focused_thread eagerly so the sidebar highlight updates
+        // immediately, rather than waiting for a deferred AgentPanel
+        // event which can race with ActiveWorkspaceChanged clearing it.
+        self.focused_thread = Some(session_info.session_id.clone());
+
+        multi_workspace.update(cx, |multi_workspace, cx| {
+            multi_workspace.activate(workspace.clone(), cx);
+        });
+
+        Self::load_agent_thread_in_workspace(workspace, agent, session_info, window, cx);
 
         self.update_entries(false, cx);
     }
 
+    fn activate_thread_in_other_window(
+        &self,
+        agent: Agent,
+        session_info: acp_thread::AgentSessionInfo,
+        workspace: Entity<Workspace>,
+        target_window: WindowHandle<MultiWorkspace>,
+        cx: &mut Context<Self>,
+    ) {
+        let target_session_id = session_info.session_id.clone();
+
+        let activated = target_window
+            .update(cx, |multi_workspace, window, cx| {
+                window.activate_window();
+                multi_workspace.activate(workspace.clone(), cx);
+                Self::load_agent_thread_in_workspace(&workspace, agent, session_info, window, cx);
+            })
+            .log_err()
+            .is_some();
+
+        if activated {
+            if let Some(target_sidebar) = target_window
+                .read(cx)
+                .ok()
+                .and_then(|multi_workspace| {
+                    multi_workspace.sidebar().map(|sidebar| sidebar.to_any())
+                })
+                .and_then(|sidebar| sidebar.downcast::<Self>().ok())
+            {
+                target_sidebar.update(cx, |sidebar, cx| {
+                    sidebar.focused_thread = Some(target_session_id);
+                    sidebar.update_entries(false, cx);
+                });
+            }
+        }
+    }
+
+    fn activate_thread(
+        &mut self,
+        agent: Agent,
+        session_info: acp_thread::AgentSessionInfo,
+        workspace: &Entity<Workspace>,
+        window: &mut Window,
+        cx: &mut Context<Self>,
+    ) {
+        if self
+            .find_workspace_in_current_window(cx, |candidate, _| candidate == workspace)
+            .is_some()
+        {
+            self.activate_thread_locally(agent, session_info, &workspace, window, cx);
+            return;
+        }
+
+        let Some((target_window, workspace)) =
+            self.find_workspace_across_windows(cx, |candidate, _| candidate == workspace)
+        else {
+            return;
+        };
+
+        self.activate_thread_in_other_window(agent, session_info, workspace, target_window, cx);
+    }
+
     fn open_workspace_and_activate_thread(
         &mut self,
         agent: Agent,
@@ -1725,18 +1831,24 @@ impl Sidebar {
         .detach_and_log_err(cx);
     }
 
-    fn find_open_workspace_for_path_list(
+    fn find_current_workspace_for_path_list(
         &self,
         path_list: &PathList,
         cx: &App,
     ) -> Option<Entity<Workspace>> {
-        let multi_workspace = self.multi_workspace.upgrade()?;
-        multi_workspace
-            .read(cx)
-            .workspaces()
-            .iter()
-            .find(|workspace| workspace_path_list(workspace, cx).paths() == path_list.paths())
-            .cloned()
+        self.find_workspace_in_current_window(cx, |workspace, cx| {
+            workspace_path_list(workspace, cx).paths() == path_list.paths()
+        })
+    }
+
+    fn find_open_workspace_for_path_list(
+        &self,
+        path_list: &PathList,
+        cx: &App,
+    ) -> Option<(WindowHandle<MultiWorkspace>, Entity<Workspace>)> {
+        self.find_workspace_across_windows(cx, |workspace, cx| {
+            workspace_path_list(workspace, cx).paths() == path_list.paths()
+        })
     }
 
     fn activate_archived_thread(
@@ -1747,8 +1859,18 @@ impl Sidebar {
         cx: &mut Context<Self>,
     ) {
         if let Some(path_list) = &session_info.work_dirs {
-            if let Some(workspace) = self.find_open_workspace_for_path_list(&path_list, cx) {
-                self.activate_thread(agent, session_info, &workspace, window, cx);
+            if let Some(workspace) = self.find_current_workspace_for_path_list(path_list, cx) {
+                self.activate_thread_locally(agent, session_info, &workspace, window, cx);
+            } else if let Some((target_window, workspace)) =
+                self.find_open_workspace_for_path_list(path_list, cx)
+            {
+                self.activate_thread_in_other_window(
+                    agent,
+                    session_info,
+                    workspace,
+                    target_window,
+                    cx,
+                );
             } else {
                 let path_list = path_list.clone();
                 self.open_workspace_and_activate_thread(agent, session_info, path_list, window, cx);
@@ -1764,7 +1886,7 @@ impl Sidebar {
         });
 
         if let Some(workspace) = active_workspace {
-            self.activate_thread(agent, session_info, &workspace, window, cx);
+            self.activate_thread_locally(agent, session_info, &workspace, window, cx);
         }
     }
 
@@ -5500,4 +5622,226 @@ mod tests {
             "should have opened a second workspace for the archived thread's saved paths"
         );
     }
+
+    #[gpui::test]
+    async fn test_activate_archived_thread_reuses_workspace_in_another_window(
+        cx: &mut TestAppContext,
+    ) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree("/project-a", serde_json::json!({ "src": {} }))
+            .await;
+        fs.insert_tree("/project-b", serde_json::json!({ "src": {} }))
+            .await;
+        cx.update(|cx| <dyn fs::Fs>::set_global(fs.clone(), cx));
+
+        let project_a = project::Project::test(fs.clone(), ["/project-a".as_ref()], cx).await;
+        let project_b = project::Project::test(fs.clone(), ["/project-b".as_ref()], cx).await;
+
+        let multi_workspace_a =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project_a, window, cx));
+        let multi_workspace_b =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project_b, window, cx));
+
+        let multi_workspace_a_entity = multi_workspace_a.root(cx).unwrap();
+
+        let cx_a = &mut gpui::VisualTestContext::from_window(multi_workspace_a.into(), cx);
+        let sidebar = setup_sidebar(&multi_workspace_a_entity, cx_a);
+
+        let session_id = acp::SessionId::new(Arc::from("archived-cross-window"));
+
+        sidebar.update_in(cx_a, |sidebar, window, cx| {
+            sidebar.activate_archived_thread(
+                Agent::NativeAgent,
+                acp_thread::AgentSessionInfo {
+                    session_id: session_id.clone(),
+                    work_dirs: Some(PathList::new(&[PathBuf::from("/project-b")])),
+                    title: Some("Cross Window Thread".into()),
+                    updated_at: None,
+                    created_at: None,
+                    meta: None,
+                },
+                window,
+                cx,
+            );
+        });
+        cx_a.run_until_parked();
+
+        assert_eq!(
+            multi_workspace_a
+                .read_with(cx_a, |mw, _| mw.workspaces().len())
+                .unwrap(),
+            1,
+            "should not add the other window's workspace into the current window"
+        );
+        assert_eq!(
+            multi_workspace_b
+                .read_with(cx_a, |mw, _| mw.workspaces().len())
+                .unwrap(),
+            1,
+            "should reuse the existing workspace in the other window"
+        );
+        assert!(
+            cx_a.read(|cx| cx.active_window().unwrap()) == *multi_workspace_b,
+            "should activate the window that already owns the matching workspace"
+        );
+        sidebar.read_with(cx_a, |sidebar, _| {
+            assert_eq!(
+                sidebar.focused_thread, None,
+                "source window's sidebar should not eagerly claim focus for a thread opened in another window"
+            );
+        });
+    }
+
+    #[gpui::test]
+    async fn test_activate_archived_thread_reuses_workspace_in_another_window_with_target_sidebar(
+        cx: &mut TestAppContext,
+    ) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree("/project-a", serde_json::json!({ "src": {} }))
+            .await;
+        fs.insert_tree("/project-b", serde_json::json!({ "src": {} }))
+            .await;
+        cx.update(|cx| <dyn fs::Fs>::set_global(fs.clone(), cx));
+
+        let project_a = project::Project::test(fs.clone(), ["/project-a".as_ref()], cx).await;
+        let project_b = project::Project::test(fs.clone(), ["/project-b".as_ref()], cx).await;
+
+        let multi_workspace_a =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project_a, window, cx));
+        let multi_workspace_b =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project_b.clone(), window, cx));
+
+        let multi_workspace_a_entity = multi_workspace_a.root(cx).unwrap();
+        let multi_workspace_b_entity = multi_workspace_b.root(cx).unwrap();
+
+        let cx_a = &mut gpui::VisualTestContext::from_window(multi_workspace_a.into(), cx);
+        let sidebar_a = setup_sidebar(&multi_workspace_a_entity, cx_a);
+
+        let cx_b = &mut gpui::VisualTestContext::from_window(multi_workspace_b.into(), cx);
+        let sidebar_b = setup_sidebar(&multi_workspace_b_entity, cx_b);
+        let workspace_b = multi_workspace_b_entity.read_with(cx_b, |mw, _| mw.workspace().clone());
+        let _panel_b = add_agent_panel(&workspace_b, &project_b, cx_b);
+
+        let session_id = acp::SessionId::new(Arc::from("archived-cross-window-with-sidebar"));
+
+        sidebar_a.update_in(cx_a, |sidebar, window, cx| {
+            sidebar.activate_archived_thread(
+                Agent::NativeAgent,
+                acp_thread::AgentSessionInfo {
+                    session_id: session_id.clone(),
+                    work_dirs: Some(PathList::new(&[PathBuf::from("/project-b")])),
+                    title: Some("Cross Window Thread".into()),
+                    updated_at: None,
+                    created_at: None,
+                    meta: None,
+                },
+                window,
+                cx,
+            );
+        });
+        cx_a.run_until_parked();
+
+        assert_eq!(
+            multi_workspace_a
+                .read_with(cx_a, |mw, _| mw.workspaces().len())
+                .unwrap(),
+            1,
+            "should not add the other window's workspace into the current window"
+        );
+        assert_eq!(
+            multi_workspace_b
+                .read_with(cx_a, |mw, _| mw.workspaces().len())
+                .unwrap(),
+            1,
+            "should reuse the existing workspace in the other window"
+        );
+        assert!(
+            cx_a.read(|cx| cx.active_window().unwrap()) == *multi_workspace_b,
+            "should activate the window that already owns the matching workspace"
+        );
+        sidebar_a.read_with(cx_a, |sidebar, _| {
+            assert_eq!(
+                sidebar.focused_thread, None,
+                "source window's sidebar should not eagerly claim focus for a thread opened in another window"
+            );
+        });
+        sidebar_b.read_with(cx_b, |sidebar, _| {
+            assert_eq!(
+                sidebar.focused_thread.as_ref(),
+                Some(&session_id),
+                "target window's sidebar should eagerly focus the activated archived thread"
+            );
+        });
+    }
+
+    #[gpui::test]
+    async fn test_activate_archived_thread_prefers_current_window_for_matching_paths(
+        cx: &mut TestAppContext,
+    ) {
+        init_test(cx);
+        let fs = FakeFs::new(cx.executor());
+        fs.insert_tree("/project-a", serde_json::json!({ "src": {} }))
+            .await;
+        cx.update(|cx| <dyn fs::Fs>::set_global(fs.clone(), cx));
+
+        let project_b = project::Project::test(fs.clone(), ["/project-a".as_ref()], cx).await;
+        let project_a = project::Project::test(fs.clone(), ["/project-a".as_ref()], cx).await;
+
+        let multi_workspace_b =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project_b, window, cx));
+        let multi_workspace_a =
+            cx.add_window(|window, cx| MultiWorkspace::test_new(project_a, window, cx));
+
+        let multi_workspace_a_entity = multi_workspace_a.root(cx).unwrap();
+
+        let cx_a = &mut gpui::VisualTestContext::from_window(multi_workspace_a.into(), cx);
+        let sidebar_a = setup_sidebar(&multi_workspace_a_entity, cx_a);
+
+        let session_id = acp::SessionId::new(Arc::from("archived-current-window"));
+
+        sidebar_a.update_in(cx_a, |sidebar, window, cx| {
+            sidebar.activate_archived_thread(
+                Agent::NativeAgent,
+                acp_thread::AgentSessionInfo {
+                    session_id: session_id.clone(),
+                    work_dirs: Some(PathList::new(&[PathBuf::from("/project-a")])),
+                    title: Some("Current Window Thread".into()),
+                    updated_at: None,
+                    created_at: None,
+                    meta: None,
+                },
+                window,
+                cx,
+            );
+        });
+        cx_a.run_until_parked();
+
+        assert!(
+            cx_a.read(|cx| cx.active_window().unwrap()) == *multi_workspace_a,
+            "should keep activation in the current window when it already has a matching workspace"
+        );
+        sidebar_a.read_with(cx_a, |sidebar, _| {
+            assert_eq!(
+                sidebar.focused_thread.as_ref(),
+                Some(&session_id),
+                "current window's sidebar should eagerly focus the activated archived thread"
+            );
+        });
+        assert_eq!(
+            multi_workspace_a
+                .read_with(cx_a, |mw, _| mw.workspaces().len())
+                .unwrap(),
+            1,
+            "current window should continue reusing its existing workspace"
+        );
+        assert_eq!(
+            multi_workspace_b
+                .read_with(cx_a, |mw, _| mw.workspaces().len())
+                .unwrap(),
+            1,
+            "other windows should not be activated just because they also match the saved paths"
+        );
+    }
 }