diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index fd6bd63c103a625bf3cf6564cb3b8cd48144fd73..a1c32edb188e527cc3ed843249a6e40f9636160b 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -2915,17 +2915,19 @@ impl AgentPanel { } } let remote_connection = project.read(cx).remote_connection_options(cx); - let metadata = ThreadMetadata::new_draft( - thread_id, - resume_session_id.clone(), - agent.id(), - title.clone(), - worktree_paths, - remote_connection, - ); - ThreadMetadataStore::global(cx).update(cx, |store, cx| { - store.save_all(vec![metadata], cx); - }); + + if resume_session_id.is_none() { + let metadata = ThreadMetadata::new_draft( + thread_id, + agent.id(), + title.clone(), + worktree_paths, + remote_connection, + ); + ThreadMetadataStore::global(cx).update(cx, |store, cx| { + store.save(metadata, cx); + }); + } if self.selected_agent != agent { self.selected_agent = agent.clone(); diff --git a/crates/agent_ui/src/thread_metadata_store.rs b/crates/agent_ui/src/thread_metadata_store.rs index cdd2f6c6da0b701d681b41072fb4f0de1cac9478..c0d27e3d69e95081facebf3e9005946cfab28b62 100644 --- a/crates/agent_ui/src/thread_metadata_store.rs +++ b/crates/agent_ui/src/thread_metadata_store.rs @@ -274,7 +274,6 @@ pub struct ThreadMetadata { impl ThreadMetadata { pub fn new_draft( thread_id: ThreadId, - session_id: Option, agent_id: AgentId, title: Option, worktree_paths: WorktreePaths, @@ -283,7 +282,7 @@ impl ThreadMetadata { let now = Utc::now(); Self { thread_id, - session_id, + session_id: None, agent_id, title, updated_at: now, @@ -563,12 +562,7 @@ impl ThreadMetadataStore { cx.notify(); } - #[cfg(any(test, feature = "test-support"))] - pub fn save_manually(&mut self, metadata: ThreadMetadata, cx: &mut Context) { - self.save(metadata, cx) - } - - fn save(&mut self, metadata: ThreadMetadata, cx: &mut Context) { + pub fn save(&mut self, metadata: ThreadMetadata, cx: &mut Context) { self.save_internal(metadata); cx.notify(); } @@ -2888,7 +2882,7 @@ mod tests { let thread_id = meta.thread_id; store.update(cx, |store, cx| { - store.save_manually(meta, cx); + store.save(meta, cx); }); let replacements = vec![ @@ -2926,7 +2920,7 @@ mod tests { let thread_id = meta.thread_id; store.update(cx, |store, cx| { - store.save_manually(meta, cx); + store.save(meta, cx); }); let replacements = vec![ @@ -2967,7 +2961,7 @@ mod tests { let thread_id = meta.thread_id; store.update(cx, |store, cx| { - store.save_manually(meta, cx); + store.save(meta, cx); }); let replacements = vec![ @@ -3005,7 +2999,7 @@ mod tests { let thread_id = meta.thread_id; store.update(cx, |store, cx| { - store.save_manually(meta, cx); + store.save(meta, cx); }); let replacements = vec![ diff --git a/crates/sidebar/src/sidebar_tests.rs b/crates/sidebar/src/sidebar_tests.rs index a9251fad75cf95892be0aa88e6d056d86f1aba67..6bbe9f3a2f64460b804300c38f338be29f49a55e 100644 --- a/crates/sidebar/src/sidebar_tests.rs +++ b/crates/sidebar/src/sidebar_tests.rs @@ -279,7 +279,7 @@ fn save_thread_metadata( archived: false, remote_connection: None, }; - ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save_manually(metadata, cx)); + ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save(metadata, cx)); }); cx.run_until_parked(); } @@ -314,7 +314,7 @@ fn save_thread_metadata_with_main_paths( remote_connection: None, }; cx.update(|cx| { - ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save_manually(metadata, cx)); + ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save(metadata, cx)); }); cx.run_until_parked(); } @@ -2354,6 +2354,101 @@ async fn test_confirm_on_historical_thread_activates_workspace(cx: &mut TestAppC ); } +#[gpui::test] +async fn test_confirm_on_historical_thread_preserves_historical_timestamp_and_order( + cx: &mut TestAppContext, +) { + let project = init_test_project_with_agent_panel("/my-project", cx).await; + let (multi_workspace, cx) = + cx.add_window_view(|window, cx| MultiWorkspace::test_new(project.clone(), window, cx)); + let (sidebar, _panel) = setup_sidebar_with_agent_panel(&multi_workspace, cx); + + let newer_session_id = acp::SessionId::new(Arc::from("newer-historical-thread")); + let newer_timestamp = chrono::TimeZone::with_ymd_and_hms(&Utc, 2024, 6, 2, 0, 0, 0).unwrap(); + save_thread_metadata( + newer_session_id, + Some("Newer Historical Thread".into()), + newer_timestamp, + Some(newer_timestamp), + &project, + cx, + ); + + let older_session_id = acp::SessionId::new(Arc::from("older-historical-thread")); + let older_timestamp = chrono::TimeZone::with_ymd_and_hms(&Utc, 2024, 6, 1, 0, 0, 0).unwrap(); + save_thread_metadata( + older_session_id.clone(), + Some("Older Historical Thread".into()), + older_timestamp, + Some(older_timestamp), + &project, + cx, + ); + + cx.run_until_parked(); + multi_workspace.update_in(cx, |_, _window, cx| cx.notify()); + cx.run_until_parked(); + + let historical_entries_before: Vec<_> = visible_entries_as_strings(&sidebar, cx) + .into_iter() + .filter(|entry| entry.contains("Historical Thread")) + .collect(); + assert_eq!( + historical_entries_before, + vec![ + " Newer Historical Thread".to_string(), + " Older Historical Thread".to_string(), + ], + "expected the sidebar to sort historical threads by their saved timestamp before activation" + ); + + let older_entry_index = sidebar.read_with(cx, |sidebar, _cx| { + sidebar + .contents + .entries + .iter() + .position(|entry| { + matches!(entry, ListEntry::Thread(thread) + if thread.metadata.session_id.as_ref() == Some(&older_session_id)) + }) + .expect("expected Older Historical Thread to appear in the sidebar") + }); + + sidebar.update_in(cx, |sidebar, window, cx| { + sidebar.selection = Some(older_entry_index); + sidebar.confirm(&Confirm, window, cx); + }); + cx.run_until_parked(); + cx.run_until_parked(); + cx.run_until_parked(); + + let older_metadata = cx.update(|_, cx| { + ThreadMetadataStore::global(cx) + .read(cx) + .entry_by_session(&older_session_id) + .cloned() + .expect("expected metadata for Older Historical Thread after activation") + }); + assert_eq!( + older_metadata.created_at, + Some(older_timestamp), + "activating a historical thread should not rewrite its saved created_at timestamp" + ); + + let historical_entries_after: Vec<_> = visible_entries_as_strings(&sidebar, cx) + .into_iter() + .filter(|entry| entry.contains("Historical Thread")) + .collect(); + assert_eq!( + historical_entries_after, + vec![ + " Newer Historical Thread".to_string(), + " Older Historical Thread".to_string(), + ], + "activating an older historical thread should not reorder it ahead of a newer historical thread" + ); +} + #[gpui::test] async fn test_confirm_on_historical_thread_in_new_project_group_opens_real_thread( cx: &mut TestAppContext, @@ -6039,7 +6134,7 @@ async fn test_unarchive_first_thread_in_group_does_not_create_spurious_draft( let thread_id = ThreadId::new(); cx.update(|_, cx| { ThreadMetadataStore::global(cx).update(cx, |store, cx| { - store.save_manually( + store.save( ThreadMetadata { thread_id, session_id: Some(session_id.clone()), @@ -6131,7 +6226,7 @@ async fn test_unarchive_into_new_workspace_does_not_create_duplicate_real_thread let original_thread_id = ThreadId::new(); cx.update(|_, cx| { ThreadMetadataStore::global(cx).update(cx, |store, cx| { - store.save_manually( + store.save( ThreadMetadata { thread_id: original_thread_id, session_id: Some(session_id.clone()), @@ -6446,7 +6541,7 @@ async fn test_unarchive_into_inactive_existing_workspace_does_not_leave_active_d let thread_id = ThreadId::new(); cx.update(|_, cx| { ThreadMetadataStore::global(cx).update(cx, |store, cx| { - store.save_manually( + store.save( ThreadMetadata { thread_id, session_id: Some(session_id.clone()), @@ -7296,7 +7391,7 @@ async fn test_unarchive_linked_worktree_thread_into_project_group_shows_only_res cx.update(|_, cx| { ThreadMetadataStore::global(cx).update(cx, |store, cx| { - store.save_manually( + store.save( ThreadMetadata { thread_id: original_thread_id, session_id: Some(session_id.clone()), @@ -8016,7 +8111,7 @@ async fn test_legacy_thread_with_canonical_path_opens_main_repo_workspace(cx: &m archived: false, remote_connection: None, }; - ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save_manually(metadata, cx)); + ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save(metadata, cx)); }); cx.run_until_parked(); @@ -9044,8 +9139,7 @@ mod property_test { remote_connection: None, }; cx.update(|_, cx| { - ThreadMetadataStore::global(cx) - .update(cx, |store, cx| store.save_manually(metadata, cx)) + ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save(metadata, cx)) }); cx.run_until_parked(); } @@ -9931,7 +10025,7 @@ async fn test_remote_project_integration_does_not_briefly_render_as_separate_pro archived: false, remote_connection: None, }; - ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save_manually(metadata, cx)); + ThreadMetadataStore::global(cx).update(cx, |store, cx| store.save(metadata, cx)); }); cx.run_until_parked();