From 57669b4908c2ee8f126e044b88be3cf3ed8e69f5 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Sun, 6 Apr 2025 09:35:15 -0500 Subject: [PATCH] agent: Refresh UI when context or thread history changes (#28188) I found a few more cases where the UI wasn't updated immediately after an interaction. Release Notes: - agent: Fixed delay after removing threads from "Past Interactions" - agent: Fixed delay after adding/remove context via keyboard --- crates/agent/src/assistant_panel.rs | 2 + crates/agent/src/context_picker.rs | 31 ++++++- .../src/context_picker/completion_provider.rs | 4 +- .../context_picker/fetch_context_picker.rs | 4 +- crates/agent/src/context_store.rs | 83 ++++++++++++------- crates/agent/src/context_strip.rs | 9 +- 6 files changed, 95 insertions(+), 38 deletions(-) diff --git a/crates/agent/src/assistant_panel.rs b/crates/agent/src/assistant_panel.rs index 1f1f7bd00b..db93188d92 100644 --- a/crates/agent/src/assistant_panel.rs +++ b/crates/agent/src/assistant_panel.rs @@ -253,6 +253,8 @@ impl AssistantPanel { let history_store = cx.new(|cx| HistoryStore::new(thread_store.clone(), context_store.clone(), cx)); + cx.observe(&history_store, |_, _, cx| cx.notify()).detach(); + let active_view = ActiveView::thread(thread.clone(), window, cx); let thread_subscription = cx.subscribe(&thread, |_, _, event, cx| { if let ThreadEvent::MessageAdded(_) = &event { diff --git a/crates/agent/src/context_picker.rs b/crates/agent/src/context_picker.rs index e574226370..36c0dc58be 100644 --- a/crates/agent/src/context_picker.rs +++ b/crates/agent/src/context_picker.rs @@ -13,7 +13,8 @@ use editor::display_map::{Crease, FoldId}; use editor::{Anchor, AnchorRangeExt as _, Editor, ExcerptId, FoldPlaceholder, ToOffset}; use file_context_picker::render_file_context_entry; use gpui::{ - App, DismissEvent, Empty, Entity, EventEmitter, FocusHandle, Focusable, Task, WeakEntity, + App, DismissEvent, Empty, Entity, EventEmitter, FocusHandle, Focusable, Subscription, Task, + WeakEntity, }; use multi_buffer::MultiBufferRow; use project::{Entry, ProjectPath}; @@ -105,6 +106,7 @@ pub(super) struct ContextPicker { context_store: WeakEntity, thread_store: Option>, confirm_behavior: ConfirmBehavior, + _subscriptions: Vec, } impl ContextPicker { @@ -116,6 +118,22 @@ impl ContextPicker { window: &mut Window, cx: &mut Context, ) -> Self { + let subscriptions = context_store + .upgrade() + .map(|context_store| { + cx.observe(&context_store, |this, _, cx| this.notify_current_picker(cx)) + }) + .into_iter() + .chain( + thread_store + .as_ref() + .and_then(|thread_store| thread_store.upgrade()) + .map(|thread_store| { + cx.observe(&thread_store, |this, _, cx| this.notify_current_picker(cx)) + }), + ) + .collect::>(); + ContextPicker { mode: ContextPickerState::Default(ContextMenu::build( window, @@ -126,6 +144,7 @@ impl ContextPicker { context_store, thread_store, confirm_behavior, + _subscriptions: subscriptions, } } @@ -370,6 +389,16 @@ impl ContextPicker { recent_context_picker_entries(context_store, self.thread_store.clone(), workspace, cx) } + + fn notify_current_picker(&mut self, cx: &mut Context) { + match &self.mode { + ContextPickerState::Default(entity) => entity.update(cx, |_, cx| cx.notify()), + ContextPickerState::File(entity) => entity.update(cx, |_, cx| cx.notify()), + ContextPickerState::Symbol(entity) => entity.update(cx, |_, cx| cx.notify()), + ContextPickerState::Fetch(entity) => entity.update(cx, |_, cx| cx.notify()), + ContextPickerState::Thread(entity) => entity.update(cx, |_, cx| cx.notify()), + } + } } impl EventEmitter for ContextPicker {} diff --git a/crates/agent/src/context_picker/completion_provider.rs b/crates/agent/src/context_picker/completion_provider.rs index d5f02a7d84..a62872e62d 100644 --- a/crates/agent/src/context_picker/completion_provider.rs +++ b/crates/agent/src/context_picker/completion_provider.rs @@ -232,8 +232,8 @@ impl ContextPickerCompletionProvider { url_to_fetch.to_string(), )) .await?; - context_store.update(cx, |context_store, _| { - context_store.add_fetched_url(url_to_fetch.to_string(), content) + context_store.update(cx, |context_store, cx| { + context_store.add_fetched_url(url_to_fetch.to_string(), content, cx) }) }) .detach_and_log_err(cx); diff --git a/crates/agent/src/context_picker/fetch_context_picker.rs b/crates/agent/src/context_picker/fetch_context_picker.rs index 9cab02f4ae..c4a9dd1211 100644 --- a/crates/agent/src/context_picker/fetch_context_picker.rs +++ b/crates/agent/src/context_picker/fetch_context_picker.rs @@ -213,8 +213,8 @@ impl PickerDelegate for FetchContextPickerDelegate { this.update_in(cx, |this, window, cx| { this.delegate .context_store - .update(cx, |context_store, _cx| { - context_store.add_fetched_url(url, text); + .update(cx, |context_store, cx| { + context_store.add_fetched_url(url, text, cx) })?; match confirm_behavior { diff --git a/crates/agent/src/context_store.rs b/crates/agent/src/context_store.rs index f152864154..af990e1f81 100644 --- a/crates/agent/src/context_store.rs +++ b/crates/agent/src/context_store.rs @@ -98,11 +98,11 @@ impl ContextStore { let buffer = open_buffer_task.await?; let buffer_id = this.update(cx, |_, cx| buffer.read(cx).remote_id())?; - let already_included = this.update(cx, |this, _cx| { + let already_included = this.update(cx, |this, cx| { match this.will_include_buffer(buffer_id, &project_path.path) { Some(FileInclusion::Direct(context_id)) => { if remove_if_exists { - this.remove_context(context_id); + this.remove_context(context_id, cx); } true } @@ -120,8 +120,8 @@ impl ContextStore { let text = text_task.await; - this.update(cx, |this, _cx| { - this.insert_file(make_context_buffer(buffer_info, text)); + this.update(cx, |this, cx| { + this.insert_file(make_context_buffer(buffer_info, text), cx); })?; anyhow::Ok(()) @@ -139,19 +139,20 @@ impl ContextStore { let text = text_task.await; - this.update(cx, |this, _cx| { - this.insert_file(make_context_buffer(buffer_info, text)) + this.update(cx, |this, cx| { + this.insert_file(make_context_buffer(buffer_info, text), cx) })?; anyhow::Ok(()) }) } - fn insert_file(&mut self, context_buffer: ContextBuffer) { + fn insert_file(&mut self, context_buffer: ContextBuffer, cx: &mut Context) { let id = self.next_context_id.post_inc(); self.files.insert(context_buffer.id, id); self.context .push(AssistantContext::File(FileContext { id, context_buffer })); + cx.notify(); } pub fn add_directory( @@ -171,7 +172,7 @@ impl ContextStore { let already_included = match self.includes_directory(&project_path.path) { Some(FileInclusion::Direct(context_id)) => { if remove_if_exists { - self.remove_context(context_id); + self.remove_context(context_id, cx); } true } @@ -238,15 +239,20 @@ impl ContextStore { )); } - this.update(cx, |this, _| { - this.insert_directory(project_path, context_buffers); + this.update(cx, |this, cx| { + this.insert_directory(project_path, context_buffers, cx); })?; anyhow::Ok(()) }) } - fn insert_directory(&mut self, project_path: ProjectPath, context_buffers: Vec) { + fn insert_directory( + &mut self, + project_path: ProjectPath, + context_buffers: Vec, + cx: &mut Context, + ) { let id = self.next_context_id.post_inc(); self.directories.insert(project_path.path.to_path_buf(), id); @@ -256,6 +262,7 @@ impl ContextStore { project_path, context_buffers, })); + cx.notify(); } pub fn add_symbol( @@ -286,7 +293,7 @@ impl ContextStore { if let Some(id) = matching_symbol_id { if remove_if_exists { - self.remove_context(id); + self.remove_context(id, cx); } return Task::ready(Ok(false)); } @@ -301,21 +308,24 @@ impl ContextStore { cx.spawn(async move |this, cx| { let content = collect_content_task.await; - this.update(cx, |this, _cx| { - this.insert_symbol(make_context_symbol( - buffer_info, - project_path, - symbol_name, - symbol_range, - symbol_enclosing_range, - content, - )) + this.update(cx, |this, cx| { + this.insert_symbol( + make_context_symbol( + buffer_info, + project_path, + symbol_name, + symbol_range, + symbol_enclosing_range, + content, + ), + cx, + ) })?; anyhow::Ok(true) }) } - fn insert_symbol(&mut self, context_symbol: ContextSymbol) { + fn insert_symbol(&mut self, context_symbol: ContextSymbol, cx: &mut Context) { let id = self.next_context_id.post_inc(); self.symbols.insert(context_symbol.id.clone(), id); self.symbols_by_path @@ -328,6 +338,7 @@ impl ContextStore { id, context_symbol, })); + cx.notify(); } pub fn add_thread( @@ -338,7 +349,7 @@ impl ContextStore { ) { if let Some(context_id) = self.includes_thread(&thread.read(cx).id()) { if remove_if_exists { - self.remove_context(context_id); + self.remove_context(context_id, cx); } } else { self.insert_thread(thread, cx); @@ -353,14 +364,14 @@ impl ContextStore { }) } - fn insert_thread(&mut self, thread: Entity, cx: &mut App) { + fn insert_thread(&mut self, thread: Entity, cx: &mut Context) { if let Some(summary_task) = thread.update(cx, |thread, cx| thread.generate_detailed_summary(cx)) { let thread = thread.clone(); let thread_store = self.thread_store.clone(); - self.thread_summary_tasks.push(cx.spawn(async move |cx| { + self.thread_summary_tasks.push(cx.spawn(async move |_, cx| { summary_task.await; if let Some(thread_store) = thread_store { @@ -382,15 +393,26 @@ impl ContextStore { self.threads.insert(thread.read(cx).id().clone(), id); self.context .push(AssistantContext::Thread(ThreadContext { id, thread, text })); + cx.notify(); } - pub fn add_fetched_url(&mut self, url: String, text: impl Into) { + pub fn add_fetched_url( + &mut self, + url: String, + text: impl Into, + cx: &mut Context, + ) { if self.includes_url(&url).is_none() { - self.insert_fetched_url(url, text); + self.insert_fetched_url(url, text, cx); } } - fn insert_fetched_url(&mut self, url: String, text: impl Into) { + fn insert_fetched_url( + &mut self, + url: String, + text: impl Into, + cx: &mut Context, + ) { let id = self.next_context_id.post_inc(); self.fetched_urls.insert(url.clone(), id); @@ -400,6 +422,7 @@ impl ContextStore { url: url.into(), text: text.into(), })); + cx.notify(); } pub fn accept_suggested_context( @@ -426,7 +449,7 @@ impl ContextStore { Task::ready(Ok(())) } - pub fn remove_context(&mut self, id: ContextId) { + pub fn remove_context(&mut self, id: ContextId, cx: &mut Context) { let Some(ix) = self.context.iter().position(|context| context.id() == id) else { return; }; @@ -458,6 +481,8 @@ impl ContextStore { self.threads.retain(|_, context_id| *context_id != id); } } + + cx.notify(); } /// Returns whether the buffer is already included directly in the context, or if it will be diff --git a/crates/agent/src/context_strip.rs b/crates/agent/src/context_strip.rs index a2137ede0e..fd56dcf678 100644 --- a/crates/agent/src/context_strip.rs +++ b/crates/agent/src/context_strip.rs @@ -59,6 +59,7 @@ impl ContextStrip { let focus_handle = cx.focus_handle(); let subscriptions = vec![ + cx.observe(&context_store, |_, _, cx| cx.notify()), cx.subscribe_in(&context_picker, window, Self::handle_context_picker_event), cx.on_focus(&focus_handle, window, Self::handle_focus), cx.on_blur(&focus_handle, window, Self::handle_blur), @@ -290,9 +291,9 @@ impl ContextStrip { if let Some(index) = self.focused_index { let mut is_empty = false; - self.context_store.update(cx, |this, _cx| { + self.context_store.update(cx, |this, cx| { if let Some(item) = this.context().get(index) { - this.remove_context(item.id()); + this.remove_context(item.id(), cx); } is_empty = this.context().is_empty(); @@ -475,8 +476,8 @@ impl Render for ContextStrip { Some({ let context_store = self.context_store.clone(); Rc::new(cx.listener(move |_this, _event, _window, cx| { - context_store.update(cx, |this, _cx| { - this.remove_context(id); + context_store.update(cx, |this, cx| { + this.remove_context(id, cx); }); cx.notify(); }))