Bennet Bo Fenner created
Make sure that we never write thread metadata to the DB for a thread
running on a collab project.
When joining a collab project and starting a native thread we would set
all your threads that had no paths associated with to the path of the
collab project (in `move_thread_paths`).
Self-Review Checklist:
- [x] I've reviewed my own diff for quality, security, and reliability
- [x] Unsafe blocks (if any) have justifying comments
- [x] The content is consistent with the [UI/UX
checklist](https://github.com/zed-industries/zed/blob/main/CONTRIBUTING.md#uiux-checklist)
- [x] Tests cover the new/changed behavior
- [x] Performance impact has been considered and is acceptable
Release Notes:
- N/Acrates/agent_ui/src/agent_panel.rs | 4
crates/agent_ui/src/thread_metadata_store.rs | 142 +++++++++++++++++++++
crates/project/src/project.rs | 12 +
crates/sidebar/src/sidebar.rs | 7 +
crates/sidebar/src/sidebar_tests.rs | 71 +++++++++++
5 files changed, 235 insertions(+), 1 deletion(-)
@@ -2120,6 +2120,10 @@ impl AgentPanel {
});
}
+ if self.project.read(cx).is_via_collab() {
+ return;
+ }
+
// Update metadata store so threads' path lists stay in sync with
// the project's current worktrees. Without this, threads saved
// before a worktree was added would have stale paths and not
@@ -1138,7 +1138,7 @@ impl ThreadMetadataStore {
};
let thread_ref = thread.read(cx);
- if thread_ref.is_draft_thread() {
+ if thread_ref.is_draft_thread() || thread_ref.project().read(cx).is_via_collab() {
return;
}
@@ -3747,4 +3747,144 @@ mod tests {
);
});
}
+
+ #[gpui::test]
+ async fn test_collab_guest_threads_not_saved_to_metadata_store(cx: &mut TestAppContext) {
+ init_test(cx);
+
+ let fs = FakeFs::new(cx.executor());
+ let project = Project::test(fs, [Path::new("/project-a")], cx).await;
+
+ let (panel, mut vcx) = setup_panel_with_project(project.clone(), cx);
+ crate::test_support::open_thread_with_connection(
+ &panel,
+ StubAgentConnection::new(),
+ &mut vcx,
+ );
+ let thread = panel.read_with(&vcx, |panel, cx| panel.active_agent_thread(cx).unwrap());
+ let thread_id = crate::test_support::active_thread_id(&panel, &vcx);
+ thread.update_in(&mut vcx, |thread, _window, cx| {
+ thread.push_user_content_block(None, "hello".into(), cx);
+ thread.set_title("Thread".into(), cx).detach();
+ });
+ vcx.run_until_parked();
+
+ // Confirm the thread is in the store while the project is local.
+ cx.update(|cx| {
+ let store = ThreadMetadataStore::global(cx);
+ assert!(
+ store.read(cx).entry(thread_id).is_some(),
+ "thread must be in the store while the project is local"
+ );
+ });
+
+ cx.update(|cx| {
+ let store = ThreadMetadataStore::global(cx);
+ store.update(cx, |store, cx| {
+ store.delete(thread_id, cx);
+ });
+ });
+ project.update(cx, |project, _cx| {
+ project.mark_as_collab_for_testing();
+ });
+
+ thread.update_in(&mut vcx, |thread, _window, cx| {
+ thread.push_user_content_block(None, "more content".into(), cx);
+ });
+ vcx.run_until_parked();
+
+ cx.update(|cx| {
+ let store = ThreadMetadataStore::global(cx);
+ assert!(
+ store.read(cx).entry(thread_id).is_none(),
+ "threads must not be persisted while the project is a collab guest session"
+ );
+ });
+ }
+
+ // When a worktree is added to a collab project, update_thread_work_dirs
+ // fires with the new worktree paths. Without an is_via_collab() guard it
+ // overwrites the stored paths of any retained or active local threads with
+ // the new (expanded) path set, corrupting metadata that belonged to the
+ // guest's own local project.
+ #[gpui::test]
+ async fn test_collab_guest_retained_thread_paths_not_overwritten_on_worktree_change(
+ cx: &mut TestAppContext,
+ ) {
+ init_test(cx);
+
+ let fs = FakeFs::new(cx.executor());
+ fs.insert_tree("/project-a", serde_json::json!({})).await;
+ fs.insert_tree("/project-b", serde_json::json!({})).await;
+ let project = Project::test(fs, [Path::new("/project-a")], cx).await;
+
+ let (panel, mut vcx) = setup_panel_with_project(project.clone(), cx);
+
+ // Open thread A and give it content so its metadata is saved with /project-a.
+ crate::test_support::open_thread_with_connection(
+ &panel,
+ StubAgentConnection::new(),
+ &mut vcx,
+ );
+ let thread_a_id = crate::test_support::active_thread_id(&panel, &vcx);
+ let thread_a = panel.read_with(&vcx, |panel, cx| panel.active_agent_thread(cx).unwrap());
+ thread_a.update_in(&mut vcx, |thread, _window, cx| {
+ thread.push_user_content_block(None, "hello".into(), cx);
+ thread.set_title("Thread A".into(), cx).detach();
+ });
+ vcx.run_until_parked();
+
+ cx.update(|cx| {
+ let store = ThreadMetadataStore::global(cx);
+ let entry = store.read(cx).entry(thread_a_id).unwrap();
+ assert_eq!(
+ entry.folder_paths().paths(),
+ &[std::path::PathBuf::from("/project-a")],
+ "thread A must be saved with /project-a before collab"
+ );
+ });
+
+ // Open thread B, making thread A a retained thread in the panel.
+ crate::test_support::open_thread_with_connection(
+ &panel,
+ StubAgentConnection::new(),
+ &mut vcx,
+ );
+ vcx.run_until_parked();
+
+ // Transition the project into collab mode (simulates joining as a guest).
+ project.update(cx, |project, _cx| {
+ project.mark_as_collab_for_testing();
+ });
+
+ // Add a second worktree. For a real collab guest this would be one of
+ // the host's worktrees arriving via the collab protocol, but here we
+ // use a local path because the test infrastructure cannot easily produce
+ // a remote worktree with a fully-scanned root entry.
+ //
+ // This fires WorktreeAdded → update_thread_work_dirs. Without an
+ // is_via_collab() guard that call overwrites the stored paths of
+ // retained thread A from {/project-a} to {/project-a, /project-b},
+ // polluting its metadata with a path it never belonged to.
+ project
+ .update(cx, |project, cx| {
+ project.find_or_create_worktree(Path::new("/project-b"), true, cx)
+ })
+ .await
+ .unwrap();
+ vcx.run_until_parked();
+
+ cx.update(|cx| {
+ let store = ThreadMetadataStore::global(cx);
+ let entry = store
+ .read(cx)
+ .entry(thread_a_id)
+ .expect("thread A must still exist in the store");
+ assert_eq!(
+ entry.folder_paths().paths(),
+ &[std::path::PathBuf::from("/project-a")],
+ "retained thread A's stored path must not be updated while the project is via collab"
+ );
+ });
+ }
}
@@ -2075,6 +2075,18 @@ impl Project {
project
}
+ /// Transitions a local test project into the `Collab` client state so that
+ /// `is_via_collab()` returns `true`. Use only in tests.
+ #[cfg(any(test, feature = "test-support"))]
+ pub fn mark_as_collab_for_testing(&mut self) {
+ self.client_state = ProjectClientState::Collab {
+ sharing_has_stopped: false,
+ capability: Capability::ReadWrite,
+ remote_id: 0,
+ replica_id: clock::ReplicaId::new(1),
+ };
+ }
+
#[cfg(any(test, feature = "test-support"))]
pub fn add_test_remote_worktree(
&mut self,
@@ -510,6 +510,9 @@ impl Sidebar {
cx: &mut Context<Self>,
) {
let project = workspace.read(cx).project().clone();
+ if project.read(cx).is_via_collab() {
+ return;
+ }
cx.subscribe_in(
&project,
@@ -576,6 +579,10 @@ impl Sidebar {
old_paths: &WorktreePaths,
cx: &mut Context<Self>,
) {
+ if project.read(cx).is_via_collab() {
+ return;
+ }
+
let new_paths = project.read(cx).worktree_paths(cx);
let old_folder_paths = old_paths.folder_path_list().clone();
@@ -10297,3 +10297,74 @@ fn test_worktree_info_missing_branch_returns_none() {
assert_eq!(infos[0].branch_name, None);
assert_eq!(infos[0].name, SharedString::from("myapp"));
}
+
+#[gpui::test]
+async fn test_collab_guest_move_thread_paths_is_noop(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 = project::Project::test(fs, ["/project-a".as_ref()], cx).await;
+
+ let (multi_workspace, cx) =
+ cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx));
+
+ // Set up the sidebar while the project is local. This registers the
+ // WorktreePathsChanged subscription for the project.
+ let _sidebar = setup_sidebar(&multi_workspace, cx);
+
+ let session_id = acp::SessionId::new(Arc::from("test-thread"));
+ save_named_thread_metadata("test-thread", "My Thread", &project, cx).await;
+
+ let thread_id = cx.update(|_window, cx| {
+ ThreadMetadataStore::global(cx)
+ .read(cx)
+ .entry_by_session(&session_id)
+ .map(|e| e.thread_id)
+ .expect("thread must be in the store")
+ });
+
+ cx.update(|_window, cx| {
+ let store = ThreadMetadataStore::global(cx);
+ let entry = store.read(cx).entry(thread_id).unwrap();
+ assert_eq!(
+ entry.folder_paths().paths(),
+ &[PathBuf::from("/project-a")],
+ "thread must be saved with /project-a before collab"
+ );
+ });
+
+ // Transition the project into collab mode. The sidebar's subscription is
+ // still active from when the project was local.
+ project.update(cx, |project, _cx| {
+ project.mark_as_collab_for_testing();
+ });
+
+ // Adding a worktree fires WorktreePathsChanged with old_paths = {/project-a}.
+ // The sidebar's subscription is still active, so move_thread_paths is called.
+ // Without the is_via_collab() guard inside move_thread_paths, this would
+ // update the stored thread paths from {/project-a} to {/project-a, /project-b}.
+ project
+ .update(cx, |project, cx| {
+ project.find_or_create_worktree("/project-b", true, cx)
+ })
+ .await
+ .expect("should add worktree");
+ cx.run_until_parked();
+
+ cx.update(|_window, cx| {
+ let store = ThreadMetadataStore::global(cx);
+ let entry = store
+ .read(cx)
+ .entry(thread_id)
+ .expect("thread must still exist");
+ assert_eq!(
+ entry.folder_paths().paths(),
+ &[PathBuf::from("/project-a")],
+ "thread path must not change when project is via collab"
+ );
+ });
+}