sidebar: Fix ordering issue (#53787)

Bennet Bo Fenner created 8 hours ago

Follow up to #53737, that introduced an ordering issue where we would
override the existing thread metadata in the case of loading a session

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/A

Change summary

crates/agent_ui/src/agent_panel.rs           |  24 ++--
crates/agent_ui/src/thread_metadata_store.rs |  18 +--
crates/sidebar/src/sidebar_tests.rs          | 114 ++++++++++++++++++++-
3 files changed, 123 insertions(+), 33 deletions(-)

Detailed changes

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();

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<acp::SessionId>,
         agent_id: AgentId,
         title: Option<SharedString>,
         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>) {
-        self.save(metadata, cx)
-    }
-
-    fn save(&mut self, metadata: ThreadMetadata, cx: &mut Context<Self>) {
+    pub fn save(&mut self, metadata: ThreadMetadata, cx: &mut Context<Self>) {
         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![

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();