From 9c11d24887a4d001a5fcf46911b269ea3abf93d7 Mon Sep 17 00:00:00 2001 From: Agus Zubiaga Date: Sat, 3 May 2025 17:43:46 -0300 Subject: [PATCH] Fix hiding editor toolbar and add `agent_review` setting (#29854) Closes #29836 The agent diff toolbar item was causing the editor toolbar to show even when all the other elements were disabled via settings. This PR fixes this by setting the location to `ToolbarItemLocation::Hidden` in the states where it shouldn't show. It also adds a new a `toolbar.agent_review` setting to hide the agent review buttons altogether. However, if the other toolbar elements are hidden and the file isn't under review, the editor toolbar will still be hidden. So you only need to set this to `false` if you don't want them to show up even under agent review. Release Notes: - N/A --- assets/settings/default.json | 8 +- crates/agent/src/agent_diff.rs | 160 +++++++++++++++--- .../src/assistant_settings.rs | 27 +-- crates/editor/src/editor_settings.rs | 9 +- crates/zed/src/zed.rs | 2 +- docs/src/configuring-zed.md | 6 +- 6 files changed, 159 insertions(+), 53 deletions(-) diff --git a/assets/settings/default.json b/assets/settings/default.json index 47b65a7f6b..8e75cb18db 100644 --- a/assets/settings/default.json +++ b/assets/settings/default.json @@ -300,8 +300,10 @@ "breadcrumbs": true, // Whether to show quick action buttons. "quick_actions": true, - // Whether to show the Selections menu in the editor toolbar - "selections_menu": true + // Whether to show the Selections menu in the editor toolbar. + "selections_menu": true, + // Whether to show agent review buttons in the editor toolbar. + "agent_review": true }, // Scrollbar related settings "scrollbar": { @@ -659,6 +661,8 @@ "always_allow_tool_actions": false, // When enabled, the agent will stream edits. "stream_edits": false, + // When enabled, agent edits will be displayed in single-file editors for review + "single_file_review": true, "default_profile": "write", "profiles": { "ask": { diff --git a/crates/agent/src/agent_diff.rs b/crates/agent/src/agent_diff.rs index e7f16221c2..79dc31232b 100644 --- a/crates/agent/src/agent_diff.rs +++ b/crates/agent/src/agent_diff.rs @@ -6,7 +6,7 @@ use assistant_settings::AssistantSettings; use buffer_diff::DiffHunkStatus; use collections::{HashMap, HashSet}; use editor::{ - Direction, Editor, EditorEvent, MultiBuffer, MultiBufferSnapshot, ToPoint, + Direction, Editor, EditorEvent, EditorSettings, MultiBuffer, MultiBufferSnapshot, ToPoint, actions::{GoToHunk, GoToPreviousHunk}, scroll::Autoscroll, }; @@ -867,19 +867,24 @@ impl editor::Addon for AgentDiffAddon { pub struct AgentDiffToolbar { active_item: Option, + _settings_subscription: Subscription, } pub enum AgentDiffToolbarItem { Pane(WeakEntity), Editor { editor: WeakEntity, + state: EditorState, _diff_subscription: Subscription, }, } impl AgentDiffToolbar { - pub fn new() -> Self { - Self { active_item: None } + pub fn new(cx: &mut Context) -> Self { + Self { + active_item: None, + _settings_subscription: cx.observe_global::(Self::update_location), + } } fn dispatch_action(&self, action: &dyn Action, window: &mut Window, cx: &mut Context) { @@ -905,6 +910,39 @@ impl AgentDiffToolbar { cx.dispatch_action(action.as_ref()); }) } + + fn handle_diff_notify(&mut self, agent_diff: Entity, cx: &mut Context) { + let Some(AgentDiffToolbarItem::Editor { editor, state, .. }) = self.active_item.as_mut() + else { + return; + }; + + *state = agent_diff.read(cx).editor_state(&editor); + self.update_location(cx); + cx.notify(); + } + + fn update_location(&mut self, cx: &mut Context) { + let location = self.location(cx); + cx.emit(ToolbarItemEvent::ChangeLocation(location)); + } + + fn location(&self, cx: &App) -> ToolbarItemLocation { + if !EditorSettings::get_global(cx).toolbar.agent_review { + return ToolbarItemLocation::Hidden; + } + + match &self.active_item { + None => ToolbarItemLocation::Hidden, + Some(AgentDiffToolbarItem::Pane(_)) => ToolbarItemLocation::PrimaryRight, + Some(AgentDiffToolbarItem::Editor { state, .. }) => match state { + EditorState::Generating | EditorState::Reviewing => { + ToolbarItemLocation::PrimaryLeft + } + EditorState::Idle => ToolbarItemLocation::Hidden, + }, + } + } } impl EventEmitter for AgentDiffToolbar {} @@ -919,7 +957,7 @@ impl ToolbarItemView for AgentDiffToolbar { if let Some(item) = active_pane_item { if let Some(pane) = item.act_as::(cx) { self.active_item = Some(AgentDiffToolbarItem::Pane(pane.downgrade())); - return ToolbarItemLocation::PrimaryRight; + return self.location(cx); } if let Some(editor) = item.act_as::(cx) { @@ -928,18 +966,17 @@ impl ToolbarItemView for AgentDiffToolbar { self.active_item = Some(AgentDiffToolbarItem::Editor { editor: editor.downgrade(), - _diff_subscription: cx.observe(&agent_diff, |_, _, cx| { - cx.notify(); - }), + state: agent_diff.read(cx).editor_state(&editor.downgrade()), + _diff_subscription: cx.observe(&agent_diff, Self::handle_diff_notify), }); - return ToolbarItemLocation::PrimaryLeft; + return self.location(cx); } } } self.active_item = None; - ToolbarItemLocation::Hidden + return self.location(cx); } fn pane_focus_update( @@ -958,15 +995,14 @@ impl Render for AgentDiffToolbar { }; match active_item { - AgentDiffToolbarItem::Editor { editor, .. } => { + AgentDiffToolbarItem::Editor { editor, state, .. } => { let Some(editor) = editor.upgrade() else { return Empty.into_any(); }; - let agent_diff = AgentDiff::global(cx); let editor_focus_handle = editor.read(cx).focus_handle(cx); - let content = match agent_diff.read(cx).editor_state(&editor) { + let content = match state { EditorState::Idle => return Empty.into_any(), EditorState::Generating => vec![ h_flex() @@ -1063,10 +1099,9 @@ impl Render for AgentDiffToolbar { .child(vertical_divider()) .track_focus(&editor_focus_handle) .on_action({ - let agent_diff = agent_diff.clone(); let editor = editor.clone(); move |_action: &OpenAgentDiff, window, cx| { - agent_diff.update(cx, |agent_diff, cx| { + AgentDiff::global(cx).update(cx, |agent_diff, cx| { agent_diff.deploy_pane_from_editor(&editor, window, cx); }); } @@ -1160,7 +1195,7 @@ pub struct AgentDiff { } #[derive(Clone, Debug, PartialEq, Eq)] -enum EditorState { +pub enum EditorState { Idle, Reviewing, Generating, @@ -1549,10 +1584,11 @@ impl AgentDiff { cx.notify(); } - fn editor_state(&self, editor: &Entity) -> &EditorState { + fn editor_state(&self, editor: &WeakEntity) -> EditorState { self.reviewing_editors - .get(&editor.downgrade()) - .unwrap_or(&EditorState::Idle) + .get(&editor) + .cloned() + .unwrap_or(EditorState::Idle) } fn deploy_pane_from_editor(&self, editor: &Entity, window: &mut Window, cx: &mut App) { @@ -1658,7 +1694,10 @@ impl AgentDiff { let active_item = workspace.active_item(cx)?; let editor = active_item.act_as::(cx)?; - if !matches!(self.editor_state(&editor), EditorState::Reviewing) { + if !matches!( + self.editor_state(&editor.downgrade()), + EditorState::Reviewing + ) { return None; } @@ -1716,7 +1755,7 @@ mod tests { use assistant_tool::ToolWorkingSet; use context_server::ContextServerSettings; use editor::EditorSettings; - use gpui::TestAppContext; + use gpui::{TestAppContext, UpdateGlobal, VisualTestContext}; use project::{FakeFs, Project}; use prompt_store::PromptBuilder; use serde_json::json; @@ -1940,6 +1979,21 @@ mod tests { let (workspace, cx) = cx.add_window_view(|window, cx| Workspace::test_new(project.clone(), window, cx)); + // Add the diff toolbar to the active pane + let diff_toolbar = cx.new_window_entity(|_, cx| AgentDiffToolbar::new(cx)); + + workspace.update_in(cx, { + let diff_toolbar = diff_toolbar.clone(); + + move |workspace, window, cx| { + workspace.active_pane().update(cx, |pane, cx| { + pane.toolbar().update(cx, |toolbar, cx| { + toolbar.add_item(diff_toolbar, window, cx); + }); + }) + } + }); + // Set the active thread cx.update(|window, cx| { AgentDiff::set_active_thread(&workspace.downgrade(), &thread, window, cx) @@ -1968,6 +2022,19 @@ mod tests { }); cx.run_until_parked(); + // Toolbar knows about the current editor, but it's hidden since there are no changes yet + assert!(diff_toolbar.read_with(cx, |toolbar, _cx| matches!( + toolbar.active_item, + Some(AgentDiffToolbarItem::Editor { + state: EditorState::Idle, + .. + }) + ))); + assert_eq!( + diff_toolbar.read_with(cx, |toolbar, cx| toolbar.location(cx)), + ToolbarItemLocation::Hidden + ); + // Make changes cx.update(|_, cx| { action_log.update(cx, |log, cx| log.track_buffer(buffer1.clone(), cx)); @@ -2016,6 +2083,31 @@ mod tests { Point::new(1, 0)..Point::new(1, 0) ); + // The toolbar is displayed in the right state + assert_eq!( + diff_toolbar.read_with(cx, |toolbar, cx| toolbar.location(cx)), + ToolbarItemLocation::PrimaryLeft + ); + assert!(diff_toolbar.read_with(cx, |toolbar, _cx| matches!( + toolbar.active_item, + Some(AgentDiffToolbarItem::Editor { + state: EditorState::Reviewing, + .. + }) + ))); + + // The toolbar respects its setting + override_toolbar_agent_review_setting(false, cx); + assert_eq!( + diff_toolbar.read_with(cx, |toolbar, cx| toolbar.location(cx)), + ToolbarItemLocation::Hidden + ); + override_toolbar_agent_review_setting(true, cx); + assert_eq!( + diff_toolbar.read_with(cx, |toolbar, cx| toolbar.location(cx)), + ToolbarItemLocation::PrimaryLeft + ); + // After keeping a hunk, the cursor should be positioned on the second hunk. workspace.update(cx, |_, cx| { cx.dispatch_action(&Keep); @@ -2111,5 +2203,33 @@ mod tests { .range(), Point::new(0, 0)..Point::new(0, 0) ); + + // Editor 1 toolbar is hidden since all changes have been reviewed + workspace.update_in(cx, |workspace, window, cx| { + workspace.activate_item(&editor1, true, true, window, cx) + }); + + assert!(diff_toolbar.read_with(cx, |toolbar, _cx| matches!( + toolbar.active_item, + Some(AgentDiffToolbarItem::Editor { + state: EditorState::Idle, + .. + }) + ))); + assert_eq!( + diff_toolbar.read_with(cx, |toolbar, cx| toolbar.location(cx)), + ToolbarItemLocation::Hidden + ); + } + + fn override_toolbar_agent_review_setting(active: bool, cx: &mut VisualTestContext) { + cx.update(|_window, cx| { + SettingsStore::update_global(cx, |store, _cx| { + let mut editor_settings = store.get::(None).clone(); + editor_settings.toolbar.agent_review = active; + store.override_global(editor_settings); + }) + }); + cx.run_until_parked(); } } diff --git a/crates/assistant_settings/src/assistant_settings.rs b/crates/assistant_settings/src/assistant_settings.rs index 5d805aaa5a..8fb18c5fb3 100644 --- a/crates/assistant_settings/src/assistant_settings.rs +++ b/crates/assistant_settings/src/assistant_settings.rs @@ -69,7 +69,7 @@ pub enum AssistantProviderContentV1 { }, } -#[derive(Clone, Debug)] +#[derive(Default, Clone, Debug)] pub struct AssistantSettings { pub enabled: bool, pub button: bool, @@ -91,31 +91,6 @@ pub struct AssistantSettings { pub single_file_review: bool, } -impl Default for AssistantSettings { - fn default() -> Self { - Self { - enabled: Default::default(), - button: Default::default(), - dock: Default::default(), - default_width: Default::default(), - default_height: Default::default(), - default_model: Default::default(), - inline_assistant_model: Default::default(), - commit_message_model: Default::default(), - thread_summary_model: Default::default(), - inline_alternatives: Default::default(), - using_outdated_settings_version: Default::default(), - enable_experimental_live_diffs: Default::default(), - default_profile: Default::default(), - profiles: Default::default(), - always_allow_tool_actions: Default::default(), - notify_when_agent_waiting: Default::default(), - stream_edits: Default::default(), - single_file_review: true, - } - } -} - impl AssistantSettings { pub fn stream_edits(&self, cx: &App) -> bool { cx.has_flag::() || self.stream_edits diff --git a/crates/editor/src/editor_settings.rs b/crates/editor/src/editor_settings.rs index 1706951dc4..072f517bec 100644 --- a/crates/editor/src/editor_settings.rs +++ b/crates/editor/src/editor_settings.rs @@ -101,6 +101,7 @@ pub struct Toolbar { pub breadcrumbs: bool, pub quick_actions: bool, pub selections_menu: bool, + pub agent_review: bool, } #[derive(Copy, Clone, Debug, Serialize, Deserialize, JsonSchema, PartialEq, Eq)] @@ -400,11 +401,15 @@ pub struct ToolbarContent { /// /// Default: true pub quick_actions: Option, - - /// Whether to show the selections menu in the editor toolbar + /// Whether to show the selections menu in the editor toolbar. /// /// Default: true pub selections_menu: Option, + /// Whether to display Agent review buttons in the editor toolbar. + /// Only applicable while reviewing a file edited by the Agent. + /// + /// Default: true + pub agent_review: Option, } /// Scrollbar related settings diff --git a/crates/zed/src/zed.rs b/crates/zed/src/zed.rs index 2b675ed9eb..fac6890c96 100644 --- a/crates/zed/src/zed.rs +++ b/crates/zed/src/zed.rs @@ -915,7 +915,7 @@ fn initialize_pane( toolbar.add_item(migration_banner, window, cx); let project_diff_toolbar = cx.new(|cx| ProjectDiffToolbar::new(workspace, cx)); toolbar.add_item(project_diff_toolbar, window, cx); - let agent_diff_toolbar = cx.new(|_cx| AgentDiffToolbar::new()); + let agent_diff_toolbar = cx.new(AgentDiffToolbar::new); toolbar.add_item(agent_diff_toolbar, window, cx); }) }); diff --git a/docs/src/configuring-zed.md b/docs/src/configuring-zed.md index 38895915e4..b5fd910c43 100644 --- a/docs/src/configuring-zed.md +++ b/docs/src/configuring-zed.md @@ -1034,7 +1034,8 @@ List of `string` values "toolbar": { "breadcrumbs": true, "quick_actions": true, - "selections_menu": true + "selections_menu": true, + "agent_review": true }, ``` @@ -3090,7 +3091,8 @@ Run the `theme selector: toggle` action in the command palette to see a current "editor_model": { "provider": "zed.dev", "model": "claude-3-7-sonnet-latest" - } + }, + "single_file_review": true, } ```