diff --git a/crates/agent_ui/src/acp/thread_history.rs b/crates/agent_ui/src/acp/thread_history.rs index 16108e599e31f8d56f1b04e9f2e74bebff9ecee1..76f981b8847a191d66c173df000fdbf619d62239 100644 --- a/crates/agent_ui/src/acp/thread_history.rs +++ b/crates/agent_ui/src/acp/thread_history.rs @@ -38,7 +38,8 @@ pub struct AcpThreadHistory { visible_items: Vec, local_timezone: UtcOffset, confirming_delete_history: bool, - _update_task: Task<()>, + _visible_items_task: Task<()>, + _refresh_task: Task<()>, _watch_task: Option>, _subscriptions: Vec, } @@ -111,7 +112,8 @@ impl AcpThreadHistory { search_query: SharedString::default(), confirming_delete_history: false, _subscriptions: vec![search_editor_subscription], - _update_task: Task::ready(()), + _visible_items_task: Task::ready(()), + _refresh_task: Task::ready(()), _watch_task: None, }; this.set_session_list(session_list, cx); @@ -131,7 +133,7 @@ impl AcpThreadHistory { None }; - self._update_task = cx.spawn(async move |this, cx| { + self._visible_items_task = cx.spawn(async move |this, cx| { let new_visible_items = new_list_items.await; this.update(cx, |this, cx| { let new_selected_index = if let Some(history_entry) = selected_history_entry { @@ -170,6 +172,8 @@ impl AcpThreadHistory { self.sessions.clear(); self.visible_items.clear(); self.selected_index = 0; + self._visible_items_task = Task::ready(()); + self._refresh_task = Task::ready(()); let Some(session_list) = self.session_list.as_ref() else { self._watch_task = None; @@ -179,7 +183,7 @@ impl AcpThreadHistory { let Some(rx) = session_list.watch(cx) else { // No watch support - do a one-time refresh self._watch_task = None; - self.refresh_sessions(false, cx); + self.refresh_sessions(false, false, cx); return; }; session_list.notify_refresh(); @@ -192,14 +196,13 @@ impl AcpThreadHistory { updates.push(update); } - let needs_refresh = updates - .iter() - .any(|u| matches!(u, SessionListUpdate::Refresh)); - this.update(cx, |this, cx| { - // We will refresh the whole list anyway, so no need to apply incremental updates or do several refreshes + let needs_refresh = updates + .iter() + .any(|u| matches!(u, SessionListUpdate::Refresh)); + if needs_refresh { - this.refresh_sessions(true, cx); + this.refresh_sessions(true, false, cx); } else { for update in updates { if let SessionListUpdate::SessionInfo { session_id, update } = update { @@ -213,6 +216,10 @@ impl AcpThreadHistory { })); } + pub(crate) fn refresh_full_history(&mut self, cx: &mut Context) { + self.refresh_sessions(true, true, cx); + } + fn apply_info_update( &mut self, session_id: acp::SessionId, @@ -254,13 +261,21 @@ impl AcpThreadHistory { self.update_visible_items(true, cx); } - fn refresh_sessions(&mut self, preserve_selected_item: bool, cx: &mut Context) { + fn refresh_sessions( + &mut self, + preserve_selected_item: bool, + load_all_pages: bool, + cx: &mut Context, + ) { let Some(session_list) = self.session_list.clone() else { self.update_visible_items(preserve_selected_item, cx); return; }; - self._update_task = cx.spawn(async move |this, cx| { + // If a new refresh arrives while pagination is in progress, the previous + // `_refresh_task` is cancelled. This is intentional (latest refresh wins), + // but means sessions may be in a partial state until the new refresh completes. + self._refresh_task = cx.spawn(async move |this, cx| { let mut cursor: Option = None; let mut is_first_page = true; @@ -295,6 +310,10 @@ impl AcpThreadHistory { .ok(); is_first_page = false; + if !load_all_pages { + break; + } + match next_cursor { Some(next_cursor) => { if cursor.as_ref() == Some(&next_cursor) { @@ -1050,7 +1069,10 @@ mod tests { use acp_thread::AgentSessionListResponse; use chrono::NaiveDate; use gpui::TestAppContext; - use std::any::Any; + use std::{ + any::Any, + sync::{Arc, Mutex}, + }; fn init_test(cx: &mut TestAppContext) { cx.update(|cx| { @@ -1104,6 +1126,307 @@ mod tests { } } + #[derive(Clone)] + struct PaginatedTestSessionList { + first_page_sessions: Vec, + second_page_sessions: Vec, + requested_cursors: Arc>>>, + async_responses: bool, + updates_tx: smol::channel::Sender, + updates_rx: smol::channel::Receiver, + } + + impl PaginatedTestSessionList { + fn new( + first_page_sessions: Vec, + second_page_sessions: Vec, + ) -> Self { + let (tx, rx) = smol::channel::unbounded(); + Self { + first_page_sessions, + second_page_sessions, + requested_cursors: Arc::new(Mutex::new(Vec::new())), + async_responses: false, + updates_tx: tx, + updates_rx: rx, + } + } + + fn with_async_responses(mut self) -> Self { + self.async_responses = true; + self + } + + fn requested_cursors(&self) -> Vec> { + self.requested_cursors.lock().unwrap().clone() + } + + fn clear_requested_cursors(&self) { + self.requested_cursors.lock().unwrap().clear() + } + + fn send_update(&self, update: SessionListUpdate) { + self.updates_tx.try_send(update).ok(); + } + } + + impl AgentSessionList for PaginatedTestSessionList { + fn list_sessions( + &self, + request: AgentSessionListRequest, + cx: &mut App, + ) -> Task> { + let requested_cursors = self.requested_cursors.clone(); + let first_page_sessions = self.first_page_sessions.clone(); + let second_page_sessions = self.second_page_sessions.clone(); + + let respond = move || { + requested_cursors + .lock() + .unwrap() + .push(request.cursor.clone()); + + match request.cursor.as_deref() { + None => AgentSessionListResponse { + sessions: first_page_sessions, + next_cursor: Some("page-2".to_string()), + meta: None, + }, + Some("page-2") => AgentSessionListResponse::new(second_page_sessions), + _ => AgentSessionListResponse::new(Vec::new()), + } + }; + + if self.async_responses { + cx.foreground_executor().spawn(async move { + smol::future::yield_now().await; + Ok(respond()) + }) + } else { + Task::ready(Ok(respond())) + } + } + + fn watch(&self, _cx: &mut App) -> Option> { + Some(self.updates_rx.clone()) + } + + fn notify_refresh(&self) { + self.send_update(SessionListUpdate::Refresh); + } + + fn into_any(self: Rc) -> Rc { + self + } + } + + fn test_session(session_id: &str, title: &str) -> AgentSessionInfo { + AgentSessionInfo { + session_id: acp::SessionId::new(session_id), + cwd: None, + title: Some(title.to_string().into()), + updated_at: None, + meta: None, + } + } + + #[gpui::test] + async fn test_refresh_only_loads_first_page_by_default(cx: &mut TestAppContext) { + init_test(cx); + + let session_list = Rc::new(PaginatedTestSessionList::new( + vec![test_session("session-1", "First")], + vec![test_session("session-2", "Second")], + )); + + let (history, cx) = cx.add_window_view(|window, cx| { + AcpThreadHistory::new(Some(session_list.clone()), window, cx) + }); + cx.run_until_parked(); + + history.update(cx, |history, _cx| { + assert_eq!(history.sessions.len(), 1); + assert_eq!( + history.sessions[0].session_id, + acp::SessionId::new("session-1") + ); + }); + assert_eq!(session_list.requested_cursors(), vec![None]); + } + + #[gpui::test] + async fn test_enabling_full_pagination_loads_all_pages(cx: &mut TestAppContext) { + init_test(cx); + + let session_list = Rc::new(PaginatedTestSessionList::new( + vec![test_session("session-1", "First")], + vec![test_session("session-2", "Second")], + )); + + let (history, cx) = cx.add_window_view(|window, cx| { + AcpThreadHistory::new(Some(session_list.clone()), window, cx) + }); + cx.run_until_parked(); + session_list.clear_requested_cursors(); + + history.update(cx, |history, cx| history.refresh_full_history(cx)); + cx.run_until_parked(); + + history.update(cx, |history, _cx| { + assert_eq!(history.sessions.len(), 2); + assert_eq!( + history.sessions[0].session_id, + acp::SessionId::new("session-1") + ); + assert_eq!( + history.sessions[1].session_id, + acp::SessionId::new("session-2") + ); + }); + assert_eq!( + session_list.requested_cursors(), + vec![None, Some("page-2".to_string())] + ); + } + + #[gpui::test] + async fn test_standard_refresh_replaces_with_first_page_after_full_history_refresh( + cx: &mut TestAppContext, + ) { + init_test(cx); + + let session_list = Rc::new(PaginatedTestSessionList::new( + vec![test_session("session-1", "First")], + vec![test_session("session-2", "Second")], + )); + + let (history, cx) = cx.add_window_view(|window, cx| { + AcpThreadHistory::new(Some(session_list.clone()), window, cx) + }); + cx.run_until_parked(); + + history.update(cx, |history, cx| history.refresh_full_history(cx)); + cx.run_until_parked(); + session_list.clear_requested_cursors(); + + history.update(cx, |history, cx| { + history.refresh(cx); + }); + cx.run_until_parked(); + + history.update(cx, |history, _cx| { + assert_eq!(history.sessions.len(), 1); + assert_eq!( + history.sessions[0].session_id, + acp::SessionId::new("session-1") + ); + }); + assert_eq!(session_list.requested_cursors(), vec![None]); + } + + #[gpui::test] + async fn test_re_entering_full_pagination_reloads_all_pages(cx: &mut TestAppContext) { + init_test(cx); + + let session_list = Rc::new(PaginatedTestSessionList::new( + vec![test_session("session-1", "First")], + vec![test_session("session-2", "Second")], + )); + + let (history, cx) = cx.add_window_view(|window, cx| { + AcpThreadHistory::new(Some(session_list.clone()), window, cx) + }); + cx.run_until_parked(); + + history.update(cx, |history, cx| history.refresh_full_history(cx)); + cx.run_until_parked(); + session_list.clear_requested_cursors(); + + history.update(cx, |history, cx| history.refresh_full_history(cx)); + cx.run_until_parked(); + + history.update(cx, |history, _cx| { + assert_eq!(history.sessions.len(), 2); + }); + assert_eq!( + session_list.requested_cursors(), + vec![None, Some("page-2".to_string())] + ); + } + + #[gpui::test] + async fn test_partial_refresh_batch_drops_non_first_page_sessions(cx: &mut TestAppContext) { + init_test(cx); + + let second_page_session_id = acp::SessionId::new("session-2"); + let session_list = Rc::new(PaginatedTestSessionList::new( + vec![test_session("session-1", "First")], + vec![test_session("session-2", "Second")], + )); + + let (history, cx) = cx.add_window_view(|window, cx| { + AcpThreadHistory::new(Some(session_list.clone()), window, cx) + }); + cx.run_until_parked(); + + history.update(cx, |history, cx| history.refresh_full_history(cx)); + cx.run_until_parked(); + + session_list.clear_requested_cursors(); + + session_list.send_update(SessionListUpdate::SessionInfo { + session_id: second_page_session_id.clone(), + update: acp::SessionInfoUpdate::new().title("Updated Second"), + }); + session_list.send_update(SessionListUpdate::Refresh); + cx.run_until_parked(); + + history.update(cx, |history, _cx| { + assert_eq!(history.sessions.len(), 1); + assert_eq!( + history.sessions[0].session_id, + acp::SessionId::new("session-1") + ); + assert!( + history + .sessions + .iter() + .all(|session| session.session_id != second_page_session_id) + ); + }); + assert_eq!(session_list.requested_cursors(), vec![None]); + } + + #[gpui::test] + async fn test_full_pagination_works_with_async_page_fetches(cx: &mut TestAppContext) { + init_test(cx); + + let session_list = Rc::new( + PaginatedTestSessionList::new( + vec![test_session("session-1", "First")], + vec![test_session("session-2", "Second")], + ) + .with_async_responses(), + ); + + let (history, cx) = cx.add_window_view(|window, cx| { + AcpThreadHistory::new(Some(session_list.clone()), window, cx) + }); + cx.run_until_parked(); + session_list.clear_requested_cursors(); + + history.update(cx, |history, cx| history.refresh_full_history(cx)); + cx.run_until_parked(); + + history.update(cx, |history, _cx| { + assert_eq!(history.sessions.len(), 2); + }); + assert_eq!( + session_list.requested_cursors(), + vec![None, Some("page-2".to_string())] + ); + } + #[gpui::test] async fn test_apply_info_update_title(cx: &mut TestAppContext) { init_test(cx); diff --git a/crates/agent_ui/src/acp/thread_view.rs b/crates/agent_ui/src/acp/thread_view.rs index 6e7ff8486bda34cb017b98374acabc81b1f614fc..e939f053314582a42d6a009c155b243ad235802a 100644 --- a/crates/agent_ui/src/acp/thread_view.rs +++ b/crates/agent_ui/src/acp/thread_view.rs @@ -1106,8 +1106,6 @@ impl AcpServerView { if should_send_queued { self.send_queued_message_at_index(0, false, window, cx); } - - self.history.update(cx, |history, cx| history.refresh(cx)); } AcpThreadEvent::Refusal => { let error = ThreadError::Refusal; @@ -1162,7 +1160,6 @@ impl AcpServerView { } }); } - self.history.update(cx, |history, cx| history.refresh(cx)); } AcpThreadEvent::PromptCapabilitiesUpdated => { if let Some(active) = self.thread_view(&thread_id) { diff --git a/crates/agent_ui/src/agent_panel.rs b/crates/agent_ui/src/agent_panel.rs index 91b8f29638f12aaef4f7da22646b8d84dd84657a..35cc8103acab805b878e5545d7191149a69b7a63 100644 --- a/crates/agent_ui/src/agent_panel.rs +++ b/crates/agent_ui/src/agent_panel.rs @@ -1124,22 +1124,7 @@ impl AgentPanel { match self.active_view { ActiveView::Configuration | ActiveView::History { .. } => { if let Some(previous_view) = self.previous_view.take() { - self.active_view = previous_view; - cx.emit(AgentPanelEvent::ActiveViewChanged); - - match &self.active_view { - ActiveView::AgentThread { thread_view } => { - thread_view.focus_handle(cx).focus(window, cx); - } - ActiveView::TextThread { - text_thread_editor, .. - } => { - text_thread_editor.focus_handle(cx).focus(window, cx); - } - ActiveView::Uninitialized - | ActiveView::History { .. } - | ActiveView::Configuration => {} - } + self.set_active_view(previous_view, true, window, cx); } cx.notify(); } @@ -1561,6 +1546,12 @@ impl AgentPanel { window: &mut Window, cx: &mut Context, ) { + let was_in_agent_history = matches!( + self.active_view, + ActiveView::History { + kind: HistoryKind::AgentThreads + } + ); let current_is_uninitialized = matches!(self.active_view, ActiveView::Uninitialized); let current_is_history = matches!(self.active_view, ActiveView::History { .. }); let new_is_history = matches!(new_view, ActiveView::History { .. }); @@ -1593,6 +1584,18 @@ impl AgentPanel { _ => None, }; + let is_in_agent_history = matches!( + self.active_view, + ActiveView::History { + kind: HistoryKind::AgentThreads + } + ); + + if !was_in_agent_history && is_in_agent_history { + self.acp_history + .update(cx, |history, cx| history.refresh_full_history(cx)); + } + if focus { self.focus_handle(cx).focus(window, cx); }