From 589e2c0fe4ae4327d415be604b9337132b9c40c2 Mon Sep 17 00:00:00 2001 From: Danilo Leal <67129314+danilo-leal@users.noreply.github.com> Date: Thu, 18 Sep 2025 11:48:36 -0300 Subject: [PATCH] agent: Make settings view more consistent across different sections (#38419) Closes https://github.com/zed-industries/zed/issues/37660 This PR makes sections in the AI settings UI more consistent with each other and also just overall simpler. One of the main changes here is adding the tools from a given MCP server in a modal (as opposed to in a disclosure within the settings view). That's mostly an artifact of wanting to make all of the items within sections look more of the same. Then, in the process of doing so, also changed the logic that we were using to display MCP servers; previously, in the case of extension-based servers, we were only showing those that were _configured_, which felt wrong because you should be able to see everything you have _installed_, despite of its status (configured or not). However, there's still a bit of a bug (to be solved in a follow-up PR), which already existed but it was just not visible given we'd only display configured servers: an MCP server installed through an extension stays as a "custom server" until it is configured. If you don't configure it, you can't also uninstall it from the settings view (though it is possible to do so via the extensions UI). Release Notes: - agent: Improve settings view UI and solve issue where MCP servers would get unsorted upon turning them on and off (they're all alphabetically sorted now). --- assets/keymaps/default-linux.json | 7 + assets/keymaps/default-macos.json | 7 + assets/keymaps/default-windows.json | 7 + crates/agent_ui/src/agent_configuration.rs | 342 ++++++++---------- .../configure_context_server_tools_modal.rs | 176 +++++++++ crates/project/src/context_server_store.rs | 9 + crates/ui/src/components/divider.rs | 2 + 7 files changed, 367 insertions(+), 183 deletions(-) create mode 100644 crates/agent_ui/src/agent_configuration/configure_context_server_tools_modal.rs diff --git a/assets/keymaps/default-linux.json b/assets/keymaps/default-linux.json index 8ca0a5d42094db8b4b37c7e6919da0f7a6bd41db..ceb25c1835172068a809c6e5492d000cde7da5fe 100644 --- a/assets/keymaps/default-linux.json +++ b/assets/keymaps/default-linux.json @@ -1140,6 +1140,13 @@ "ctrl-enter": "menu::Confirm" } }, + { + "context": "ContextServerToolsModal", + "use_key_equivalents": true, + "bindings": { + "escape": "menu::Cancel" + } + }, { "context": "OnboardingAiConfigurationModal", "use_key_equivalents": true, diff --git a/assets/keymaps/default-macos.json b/assets/keymaps/default-macos.json index 0ef4757fc523c9ae145175da07a52ced322efa0c..3d5887de75bf13218985c33ab37b8b54ca9ea0a1 100644 --- a/assets/keymaps/default-macos.json +++ b/assets/keymaps/default-macos.json @@ -1244,6 +1244,13 @@ "cmd-enter": "menu::Confirm" } }, + { + "context": "ContextServerToolsModal", + "use_key_equivalents": true, + "bindings": { + "escape": "menu::Cancel" + } + }, { "context": "OnboardingAiConfigurationModal", "use_key_equivalents": true, diff --git a/assets/keymaps/default-windows.json b/assets/keymaps/default-windows.json index 78d5e4e698daefee5a57b04d6a8548fb948233b1..9165840d695af73a41aedded9b8037ffbce8ccbf 100644 --- a/assets/keymaps/default-windows.json +++ b/assets/keymaps/default-windows.json @@ -1160,6 +1160,13 @@ "ctrl-enter": "menu::Confirm" } }, + { + "context": "ContextServerToolsModal", + "use_key_equivalents": true, + "bindings": { + "escape": "menu::Cancel" + } + }, { "context": "OnboardingAiConfigurationModal", "use_key_equivalents": true, diff --git a/crates/agent_ui/src/agent_configuration.rs b/crates/agent_ui/src/agent_configuration.rs index aacfb423539496e6c5cb93ad8c12f1ed8ede346a..69044d9c663e6b42b0a73fb9bc29738a3c3fc737 100644 --- a/crates/agent_ui/src/agent_configuration.rs +++ b/crates/agent_ui/src/agent_configuration.rs @@ -1,5 +1,6 @@ mod add_llm_provider_modal; mod configure_context_server_modal; +mod configure_context_server_tools_modal; mod manage_profiles_modal; mod tool_picker; @@ -42,12 +43,12 @@ use workspace::{Workspace, create_and_open_local_file}; use zed_actions::ExtensionCategoryFilter; pub(crate) use configure_context_server_modal::ConfigureContextServerModal; +pub(crate) use configure_context_server_tools_modal::ConfigureContextServerToolsModal; pub(crate) use manage_profiles_modal::ManageProfilesModal; use crate::{ - AddContextServer, ExternalAgent, NewExternalAgentThread, + AddContextServer, agent_configuration::add_llm_provider_modal::{AddLlmProviderModal, LlmCompatibleProvider}, - placeholder_command, }; pub struct AgentConfiguration { @@ -200,9 +201,8 @@ impl AgentConfiguration { .when(is_expanded, |this| this.mb_2()) .child( div() - .opacity(0.6) .px_2() - .child(Divider::horizontal().color(DividerColor::Border)), + .child(Divider::horizontal().color(DividerColor::BorderFaded)), ) .child( h_flex() @@ -227,7 +227,7 @@ impl AgentConfiguration { .child( h_flex() .w_full() - .gap_2() + .gap_1p5() .child( Icon::new(provider.icon()) .size(IconSize::Small) @@ -345,6 +345,8 @@ impl AgentConfiguration { PopoverMenu::new("add-provider-popover") .trigger( Button::new("add-provider", "Add Provider") + .style(ButtonStyle::Filled) + .layer(ElevationIndex::ModalSurface) .icon_position(IconPosition::Start) .icon(IconName::Plus) .icon_size(IconSize::Small) @@ -533,10 +535,6 @@ impl AgentConfiguration { } } - fn card_item_bg_color(&self, cx: &mut Context) -> Hsla { - cx.theme().colors().background.opacity(0.25) - } - fn card_item_border_color(&self, cx: &mut Context) -> Hsla { cx.theme().colors().border.opacity(0.6) } @@ -546,7 +544,73 @@ impl AgentConfiguration { window: &mut Window, cx: &mut Context, ) -> impl IntoElement { - let context_server_ids = self.context_server_store.read(cx).configured_server_ids(); + let mut registry_descriptors = self + .context_server_store + .read(cx) + .all_registry_descriptor_ids(cx); + let server_count = registry_descriptors.len(); + + // Sort context servers: non-mcp-server ones first, then mcp-server ones + registry_descriptors.sort_by(|a, b| { + let has_mcp_prefix_a = a.0.starts_with("mcp-server-"); + let has_mcp_prefix_b = b.0.starts_with("mcp-server-"); + + match (has_mcp_prefix_a, has_mcp_prefix_b) { + // If one has mcp-server- prefix and other doesn't, non-mcp comes first + (true, false) => std::cmp::Ordering::Greater, + (false, true) => std::cmp::Ordering::Less, + // If both have same prefix status, sort by appropriate key + _ => { + let get_sort_key = |server_id: &str| -> String { + if let Some(suffix) = server_id.strip_prefix("mcp-server-") { + suffix.to_string() + } else { + server_id.to_string() + } + }; + + let key_a = get_sort_key(&a.0); + let key_b = get_sort_key(&b.0); + key_a.cmp(&key_b) + } + } + }); + + let add_server_popover = PopoverMenu::new("add-server-popover") + .trigger( + Button::new("add-server", "Add Server") + .style(ButtonStyle::Filled) + .layer(ElevationIndex::ModalSurface) + .icon_position(IconPosition::Start) + .icon(IconName::Plus) + .icon_size(IconSize::Small) + .icon_color(Color::Muted) + .label_size(LabelSize::Small), + ) + .anchor(gpui::Corner::TopRight) + .menu({ + move |window, cx| { + Some(ContextMenu::build(window, cx, |menu, _window, _cx| { + menu.entry("Add Custom Server", None, { + |window, cx| window.dispatch_action(AddContextServer.boxed_clone(), cx) + }) + .entry("Install from Extensions", None, { + |window, cx| { + window.dispatch_action( + zed_actions::Extensions { + category_filter: Some( + ExtensionCategoryFilter::ContextServers, + ), + id: None, + } + .boxed_clone(), + cx, + ) + } + }) + })) + } + }); v_flex() .p(DynamicSpacing::Base16.rems(cx)) @@ -555,18 +619,26 @@ impl AgentConfiguration { .border_b_1() .border_color(cx.theme().colors().border) .child( - v_flex() - .gap_0p5() - .child(Headline::new("Model Context Protocol (MCP) Servers")) + h_flex() + .w_full() + .items_start() + .justify_between() + .gap_1() .child( - Label::new( - "All context servers connected through the Model Context Protocol.", - ) - .color(Color::Muted), - ), + v_flex() + .gap_0p5() + .child(Headline::new("Model Context Protocol (MCP) Servers")) + .child( + Label::new( + "All MCP servers connected directly or via a Zed extension.", + ) + .color(Color::Muted), + ), + ) + .child(add_server_popover), ) - .map(|parent| { - if context_server_ids.is_empty() { + .child(v_flex().w_full().gap_1().map(|parent| { + if registry_descriptors.is_empty() { parent.child( h_flex() .p_4() @@ -582,56 +654,28 @@ impl AgentConfiguration { ), ) } else { - parent.children(context_server_ids.into_iter().map(|context_server_id| { - self.render_context_server(context_server_id, window, cx) - })) + { + parent.children(registry_descriptors.into_iter().enumerate().flat_map( + |(index, context_server_id)| { + let mut elements: Vec = vec![ + self.render_context_server(context_server_id, window, cx) + .into_any_element(), + ]; + + if index < server_count - 1 { + elements.push( + Divider::horizontal() + .color(DividerColor::BorderFaded) + .into_any_element(), + ); + } + + elements + }, + )) + } } - }) - .child( - h_flex() - .justify_between() - .gap_1p5() - .child( - h_flex().w_full().child( - Button::new("add-context-server", "Add Custom Server") - .style(ButtonStyle::Filled) - .layer(ElevationIndex::ModalSurface) - .full_width() - .icon(IconName::Plus) - .icon_size(IconSize::Small) - .icon_position(IconPosition::Start) - .on_click(|_event, window, cx| { - window.dispatch_action(AddContextServer.boxed_clone(), cx) - }), - ), - ) - .child( - h_flex().w_full().child( - Button::new( - "install-context-server-extensions", - "Install MCP Extensions", - ) - .style(ButtonStyle::Filled) - .layer(ElevationIndex::ModalSurface) - .full_width() - .icon(IconName::ToolHammer) - .icon_size(IconSize::Small) - .icon_position(IconPosition::Start) - .on_click(|_event, window, cx| { - window.dispatch_action( - zed_actions::Extensions { - category_filter: Some( - ExtensionCategoryFilter::ContextServers, - ), - id: None, - } - .boxed_clone(), - cx, - ) - }), - ), - ), - ) + })) } fn render_context_server( @@ -724,7 +768,7 @@ impl AgentConfiguration { IconButton::new("context-server-config-menu", IconName::Settings) .icon_color(Color::Muted) .icon_size(IconSize::Small), - Tooltip::text("Open MCP server options"), + Tooltip::text("Configure MCP Server"), ) .anchor(Corner::TopRight) .menu({ @@ -733,6 +777,8 @@ impl AgentConfiguration { let language_registry = self.language_registry.clone(); let context_server_store = self.context_server_store.clone(); let workspace = self.workspace.clone(); + let tools = self.tools.clone(); + move |window, cx| { Some(ContextMenu::build(window, cx, |menu, _window, _cx| { menu.entry("Configure Server", None, { @@ -749,7 +795,28 @@ impl AgentConfiguration { ) .detach_and_log_err(cx); } - }) + }).when(tool_count >= 1, |this| this.entry("View Tools", None, { + let context_server_id = context_server_id.clone(); + let tools = tools.clone(); + let workspace = workspace.clone(); + + move |window, cx| { + let context_server_id = context_server_id.clone(); + let tools = tools.clone(); + let workspace = workspace.clone(); + + workspace.update(cx, |workspace, cx| { + ConfigureContextServerToolsModal::toggle( + context_server_id, + tools, + workspace, + window, + cx, + ); + }) + .ok(); + } + })) .separator() .entry("Uninstall", None, { let fs = fs.clone(); @@ -820,17 +887,11 @@ impl AgentConfiguration { v_flex() .id(item_id.clone()) - .border_1() - .rounded_md() - .border_color(self.card_item_border_color(cx)) - .bg(self.card_item_bg_color(cx)) - .overflow_hidden() .child( h_flex() - .p_1() .justify_between() .when( - error.is_some() || are_tools_expanded && tool_count >= 1, + error.is_none() && are_tools_expanded && tool_count >= 1, |element| { element .border_b_1() @@ -841,31 +902,12 @@ impl AgentConfiguration { h_flex() .flex_1() .min_w_0() - .child( - Disclosure::new( - "tool-list-disclosure", - are_tools_expanded || error.is_some(), - ) - .disabled(tool_count == 0) - .on_click(cx.listener({ - let context_server_id = context_server_id.clone(); - move |this, _event, _window, _cx| { - let is_open = this - .expanded_context_server_tools - .entry(context_server_id.clone()) - .or_insert(false); - - *is_open = !*is_open; - } - })), - ) .child( h_flex() .id(SharedString::from(format!("tooltip-{}", item_id))) .h_full() .w_3() - .ml_1() - .mr_1p5() + .mr_2() .justify_center() .tooltip(Tooltip::text(tooltip_text)) .child(status_indicator), @@ -969,8 +1011,8 @@ impl AgentConfiguration { if let Some(error) = error { return parent.child( h_flex() - .p_2() .gap_2() + .pr_4() .items_start() .child( h_flex() @@ -998,37 +1040,11 @@ impl AgentConfiguration { return parent; } - parent.child(v_flex().py_1p5().px_1().gap_1().children( - tools.iter().enumerate().map(|(ix, tool)| { - h_flex() - .id(("tool-item", ix)) - .px_1() - .gap_2() - .justify_between() - .hover(|style| style.bg(cx.theme().colors().element_hover)) - .rounded_sm() - .child( - Label::new(tool.name()) - .buffer_font(cx) - .size(LabelSize::Small), - ) - .child( - Icon::new(IconName::Info) - .size(IconSize::Small) - .color(Color::Ignored), - ) - .tooltip(Tooltip::text(tool.description())) - }), - )) + parent }) } fn render_agent_servers_section(&mut self, cx: &mut Context) -> impl IntoElement { - let custom_settings = cx - .global::() - .get::(None) - .custom - .clone(); let user_defined_agents = self .agent_server_store .read(cx) @@ -1036,22 +1052,12 @@ impl AgentConfiguration { .filter(|name| name.0 != GEMINI_NAME && name.0 != CLAUDE_CODE_NAME) .cloned() .collect::>(); + let user_defined_agents = user_defined_agents .into_iter() .map(|name| { - self.render_agent_server( - IconName::Ai, - name.clone(), - ExternalAgent::Custom { - name: name.clone().into(), - command: custom_settings - .get(&name.0) - .map(|settings| settings.command.clone()) - .unwrap_or(placeholder_command()), - }, - cx, - ) - .into_any_element() + self.render_agent_server(IconName::Ai, name) + .into_any_element() }) .collect::>(); @@ -1075,6 +1081,8 @@ impl AgentConfiguration { .child(Headline::new("External Agents")) .child( Button::new("add-agent", "Add Agent") + .style(ButtonStyle::Filled) + .layer(ElevationIndex::ModalSurface) .icon_position(IconPosition::Start) .icon(IconName::Plus) .icon_size(IconSize::Small) @@ -1107,14 +1115,11 @@ impl AgentConfiguration { .child(self.render_agent_server( IconName::AiGemini, "Gemini CLI", - ExternalAgent::Gemini, - cx, )) + .child(Divider::horizontal().color(DividerColor::BorderFaded)) .child(self.render_agent_server( IconName::AiClaude, "Claude Code", - ExternalAgent::ClaudeCode, - cx, )) .children(user_defined_agents), ) @@ -1124,47 +1129,18 @@ impl AgentConfiguration { &self, icon: IconName, name: impl Into, - agent: ExternalAgent, - cx: &mut Context, ) -> impl IntoElement { - let name = name.into(); - h_flex() - .p_1() - .pl_2() - .gap_1p5() - .justify_between() - .border_1() - .rounded_md() - .border_color(self.card_item_border_color(cx)) - .bg(self.card_item_bg_color(cx)) - .overflow_hidden() - .child( - h_flex() - .gap_1p5() - .child(Icon::new(icon).size(IconSize::Small).color(Color::Muted)) - .child(Label::new(name.clone())), - ) - .child( - Button::new( - SharedString::from(format!("start_acp_thread-{name}")), - "Start New Thread", - ) - .layer(ElevationIndex::ModalSurface) - .label_size(LabelSize::Small) - .icon(IconName::Thread) - .icon_position(IconPosition::Start) - .icon_size(IconSize::XSmall) - .icon_color(Color::Muted) - .on_click(move |_, window, cx| { - window.dispatch_action( - NewExternalAgentThread { - agent: Some(agent.clone()), - } - .boxed_clone(), - cx, - ); - }), - ) + h_flex().gap_1p5().justify_between().child( + h_flex() + .gap_1p5() + .child(Icon::new(icon).size(IconSize::Small).color(Color::Muted)) + .child(Label::new(name.into())) + .child( + Icon::new(IconName::Check) + .size(IconSize::Small) + .color(Color::Success), + ), + ) } } diff --git a/crates/agent_ui/src/agent_configuration/configure_context_server_tools_modal.rs b/crates/agent_ui/src/agent_configuration/configure_context_server_tools_modal.rs new file mode 100644 index 0000000000000000000000000000000000000000..5a59806972ecf1b6cbc0702809c98acf1a86b387 --- /dev/null +++ b/crates/agent_ui/src/agent_configuration/configure_context_server_tools_modal.rs @@ -0,0 +1,176 @@ +use assistant_tool::{ToolSource, ToolWorkingSet}; +use context_server::ContextServerId; +use gpui::{ + DismissEvent, Entity, EventEmitter, FocusHandle, Focusable, ScrollHandle, Window, prelude::*, +}; +use ui::{Divider, DividerColor, Modal, ModalHeader, WithScrollbar, prelude::*}; +use workspace::{ModalView, Workspace}; + +pub struct ConfigureContextServerToolsModal { + context_server_id: ContextServerId, + tools: Entity, + focus_handle: FocusHandle, + expanded_tools: std::collections::HashMap, + scroll_handle: ScrollHandle, +} + +impl ConfigureContextServerToolsModal { + fn new( + context_server_id: ContextServerId, + tools: Entity, + _window: &mut Window, + cx: &mut Context, + ) -> Self { + Self { + context_server_id, + tools, + focus_handle: cx.focus_handle(), + expanded_tools: std::collections::HashMap::new(), + scroll_handle: ScrollHandle::new(), + } + } + + pub fn toggle( + context_server_id: ContextServerId, + tools: Entity, + workspace: &mut Workspace, + window: &mut Window, + cx: &mut Context, + ) { + workspace.toggle_modal(window, cx, |window, cx| { + Self::new(context_server_id, tools, window, cx) + }); + } + + fn cancel(&mut self, _: &menu::Cancel, _: &mut Window, cx: &mut Context) { + cx.emit(DismissEvent) + } + + fn render_modal_content( + &self, + window: &mut Window, + cx: &mut Context, + ) -> impl IntoElement { + let tools_by_source = self.tools.read(cx).tools_by_source(cx); + let server_tools = tools_by_source + .get(&ToolSource::ContextServer { + id: self.context_server_id.0.clone().into(), + }) + .map(|tools| tools.as_slice()) + .unwrap_or(&[]); + + div() + .size_full() + .pb_2() + .child( + v_flex() + .id("modal_content") + .px_2() + .gap_1() + .max_h_128() + .overflow_y_scroll() + .track_scroll(&self.scroll_handle) + .children(server_tools.iter().enumerate().flat_map(|(index, tool)| { + let tool_name = tool.name(); + let is_expanded = self + .expanded_tools + .get(&tool_name) + .copied() + .unwrap_or(false); + + let icon = if is_expanded { + IconName::ChevronUp + } else { + IconName::ChevronDown + }; + + let mut items = vec![ + v_flex() + .child( + h_flex() + .id(SharedString::from(format!("tool-header-{}", index))) + .py_1() + .pl_1() + .pr_2() + .w_full() + .justify_between() + .rounded_sm() + .hover(|s| s.bg(cx.theme().colors().element_hover)) + .child( + Label::new(tool_name.clone()) + .buffer_font(cx) + .size(LabelSize::Small), + ) + .child( + Icon::new(icon) + .size(IconSize::Small) + .color(Color::Muted), + ) + .on_click(cx.listener({ + move |this, _event, _window, _cx| { + let current = this + .expanded_tools + .get(&tool_name) + .copied() + .unwrap_or(false); + this.expanded_tools + .insert(tool_name.clone(), !current); + _cx.notify(); + } + })), + ) + .when(is_expanded, |this| { + this.child( + Label::new(tool.description()).color(Color::Muted).mx_1(), + ) + }) + .into_any_element(), + ]; + + if index < server_tools.len() - 1 { + items.push( + h_flex() + .w_full() + .child(Divider::horizontal().color(DividerColor::BorderVariant)) + .into_any_element(), + ); + } + + items + })), + ) + .vertical_scrollbar_for(self.scroll_handle.clone(), window, cx) + .into_any_element() + } +} + +impl ModalView for ConfigureContextServerToolsModal {} + +impl Focusable for ConfigureContextServerToolsModal { + fn focus_handle(&self, _cx: &App) -> FocusHandle { + self.focus_handle.clone() + } +} + +impl EventEmitter for ConfigureContextServerToolsModal {} + +impl Render for ConfigureContextServerToolsModal { + fn render(&mut self, window: &mut Window, cx: &mut Context) -> impl IntoElement { + div() + .key_context("ContextServerToolsModal") + .occlude() + .elevation_3(cx) + .w(rems(34.)) + .on_action(cx.listener(Self::cancel)) + .track_focus(&self.focus_handle) + .child( + Modal::new("configure-context-server-tools", None::) + .header( + ModalHeader::new() + .headline(format!("Tools from {}", self.context_server_id.0)) + .show_dismiss_button(true), + ) + .child(self.render_modal_content(window, cx)), + ) + } +} diff --git a/crates/project/src/context_server_store.rs b/crates/project/src/context_server_store.rs index 20188df5c4ae38b2ae305daee5b3eecc25319951..7abd9d85fa6395d104a87e6f585a6c8934a84684 100644 --- a/crates/project/src/context_server_store.rs +++ b/crates/project/src/context_server_store.rs @@ -286,6 +286,15 @@ impl ContextServerStore { self.servers.keys().cloned().collect() } + pub fn all_registry_descriptor_ids(&self, cx: &App) -> Vec { + self.registry + .read(cx) + .context_server_descriptors() + .into_iter() + .map(|(id, _)| ContextServerId(id)) + .collect() + } + pub fn running_servers(&self) -> Vec> { self.servers .values() diff --git a/crates/ui/src/components/divider.rs b/crates/ui/src/components/divider.rs index e2bb2341192cd44708f851fc0e64055ba8a25523..98eb45fd1dc1845284d63952eac684790d73bec4 100644 --- a/crates/ui/src/components/divider.rs +++ b/crates/ui/src/components/divider.rs @@ -36,6 +36,7 @@ enum DividerDirection { #[derive(Default)] pub enum DividerColor { Border, + BorderFaded, #[default] BorderVariant, } @@ -44,6 +45,7 @@ impl DividerColor { pub fn hsla(self, cx: &mut App) -> Hsla { match self { DividerColor::Border => cx.theme().colors().border, + DividerColor::BorderFaded => cx.theme().colors().border.opacity(0.6), DividerColor::BorderVariant => cx.theme().colors().border_variant, } }