Fix hiding editor toolbar and add `agent_review` setting (#29854)

Agus Zubiaga created

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

Change summary

assets/settings/default.json                        |   8 
crates/agent/src/agent_diff.rs                      | 160 +++++++++++++-
crates/assistant_settings/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(-)

Detailed changes

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": {

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<AgentDiffToolbarItem>,
+    _settings_subscription: Subscription,
 }
 
 pub enum AgentDiffToolbarItem {
     Pane(WeakEntity<AgentDiffPane>),
     Editor {
         editor: WeakEntity<Editor>,
+        state: EditorState,
         _diff_subscription: Subscription,
     },
 }
 
 impl AgentDiffToolbar {
-    pub fn new() -> Self {
-        Self { active_item: None }
+    pub fn new(cx: &mut Context<Self>) -> Self {
+        Self {
+            active_item: None,
+            _settings_subscription: cx.observe_global::<SettingsStore>(Self::update_location),
+        }
     }
 
     fn dispatch_action(&self, action: &dyn Action, window: &mut Window, cx: &mut Context<Self>) {
@@ -905,6 +910,39 @@ impl AgentDiffToolbar {
             cx.dispatch_action(action.as_ref());
         })
     }
+
+    fn handle_diff_notify(&mut self, agent_diff: Entity<AgentDiff>, cx: &mut Context<Self>) {
+        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<Self>) {
+        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<ToolbarItemEvent> for AgentDiffToolbar {}
@@ -919,7 +957,7 @@ impl ToolbarItemView for AgentDiffToolbar {
         if let Some(item) = active_pane_item {
             if let Some(pane) = item.act_as::<AgentDiffPane>(cx) {
                 self.active_item = Some(AgentDiffToolbarItem::Pane(pane.downgrade()));
-                return ToolbarItemLocation::PrimaryRight;
+                return self.location(cx);
             }
 
             if let Some(editor) = item.act_as::<Editor>(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<Editor>) -> &EditorState {
+    fn editor_state(&self, editor: &WeakEntity<Editor>) -> 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<Editor>, 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::<Editor>(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::<EditorSettings>(None).clone();
+                editor_settings.toolbar.agent_review = active;
+                store.override_global(editor_settings);
+            })
+        });
+        cx.run_until_parked();
     }
 }

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::<AgentStreamEditsFeatureFlag>() || self.stream_edits

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<bool>,
-
-    /// 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<bool>,
+    /// 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<bool>,
 }
 
 /// Scrollbar related settings

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

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,
 }
 ```