From 7d54d9f45ed31c21877c93b52a977816ba7be117 Mon Sep 17 00:00:00 2001 From: Umesh Yadav <23421535+imumesh18@users.noreply.github.com> Date: Wed, 4 Jun 2025 21:50:56 +0530 Subject: [PATCH] agent: Show warning for image context pill if model doesn't support images (#31848) Closes #31781 Currently we don't any warning or error if the image is not supported by the current model in selected in the agent panel which leads for users to think it's supported as there is no visual feedback provided by zed. This PR adds a warning on image context pill to show warning when the model doesn't support it. | Before | After | |--------|-------| | image | image | Release Notes: - Show warning for image context pill in agent panel when selected model doesn't support images. --------- Signed-off-by: Umesh Yadav Co-authored-by: Bennet Bo Fenner --- crates/agent/src/active_thread.rs | 6 +- crates/agent/src/agent.rs | 7 +- crates/agent/src/context.rs | 11 +- crates/agent/src/context_strip.rs | 16 ++- crates/agent/src/inline_prompt_editor.rs | 2 + crates/agent/src/message_editor.rs | 1 + crates/agent/src/ui/context_pill.rs | 140 ++++++++++++++++++----- 7 files changed, 149 insertions(+), 34 deletions(-) diff --git a/crates/agent/src/active_thread.rs b/crates/agent/src/active_thread.rs index 1d15ee6ccb..d04d0cbcb9 100644 --- a/crates/agent/src/active_thread.rs +++ b/crates/agent/src/active_thread.rs @@ -1,4 +1,3 @@ -use crate::AgentPanel; use crate::context::{AgentContextHandle, RULES_ICON}; use crate::context_picker::{ContextPicker, MentionLink}; use crate::context_store::ContextStore; @@ -13,6 +12,7 @@ use crate::tool_use::{PendingToolUseStatus, ToolUse}; use crate::ui::{ AddedContext, AgentNotification, AgentNotificationEvent, AnimatedLabel, ContextPill, }; +use crate::{AgentPanel, ModelUsageContext}; use agent_settings::{AgentSettings, NotifyWhenAgentWaiting}; use anyhow::Context as _; use assistant_tool::ToolUseStatus; @@ -1348,6 +1348,7 @@ impl ActiveThread { Some(self.text_thread_store.downgrade()), context_picker_menu_handle.clone(), SuggestContextKind::File, + ModelUsageContext::Thread(self.thread.clone()), window, cx, ) @@ -1826,9 +1827,10 @@ impl ActiveThread { // Get all the data we need from thread before we start using it in closures let checkpoint = thread.checkpoint_for_message(message_id); + let configured_model = thread.configured_model().map(|m| m.model); let added_context = thread .context_for_message(message_id) - .map(|context| AddedContext::new_attached(context, cx)) + .map(|context| AddedContext::new_attached(context, configured_model.as_ref(), cx)) .collect::>(); let tool_uses = thread.tool_uses_for_message(message_id, cx); diff --git a/crates/agent/src/agent.rs b/crates/agent/src/agent.rs index c847477b18..db458b771e 100644 --- a/crates/agent/src/agent.rs +++ b/crates/agent/src/agent.rs @@ -36,7 +36,7 @@ use fs::Fs; use gpui::{App, Entity, actions, impl_actions}; use language::LanguageRegistry; use language_model::{ - ConfiguredModel, LanguageModelId, LanguageModelProviderId, LanguageModelRegistry, + ConfiguredModel, LanguageModel, LanguageModelId, LanguageModelProviderId, LanguageModelRegistry, }; use prompt_store::PromptBuilder; use schemars::JsonSchema; @@ -132,6 +132,11 @@ impl ModelUsageContext { } } } + + pub fn language_model(&self, cx: &App) -> Option> { + self.configured_model(cx) + .map(|configured_model| configured_model.model) + } } /// Initializes the `agent` crate. diff --git a/crates/agent/src/context.rs b/crates/agent/src/context.rs index 62106e1968..aaf613ea5f 100644 --- a/crates/agent/src/context.rs +++ b/crates/agent/src/context.rs @@ -745,6 +745,7 @@ pub struct ImageContext { pub enum ImageStatus { Loading, Error, + Warning, Ready, } @@ -761,11 +762,17 @@ impl ImageContext { self.image_task.clone().now_or_never().flatten() } - pub fn status(&self) -> ImageStatus { + pub fn status(&self, model: Option<&Arc>) -> ImageStatus { match self.image_task.clone().now_or_never() { None => ImageStatus::Loading, Some(None) => ImageStatus::Error, - Some(Some(_)) => ImageStatus::Ready, + Some(Some(_)) => { + if model.is_some_and(|model| !model.supports_images()) { + ImageStatus::Warning + } else { + ImageStatus::Ready + } + } } } diff --git a/crates/agent/src/context_strip.rs b/crates/agent/src/context_strip.rs index f28e61aa82..2de2dcd024 100644 --- a/crates/agent/src/context_strip.rs +++ b/crates/agent/src/context_strip.rs @@ -23,7 +23,7 @@ use crate::thread_store::{TextThreadStore, ThreadStore}; use crate::ui::{AddedContext, ContextPill}; use crate::{ AcceptSuggestedContext, AgentPanel, FocusDown, FocusLeft, FocusRight, FocusUp, - RemoveAllContext, RemoveFocusedContext, ToggleContextPicker, + ModelUsageContext, RemoveAllContext, RemoveFocusedContext, ToggleContextPicker, }; pub struct ContextStrip { @@ -37,6 +37,7 @@ pub struct ContextStrip { _subscriptions: Vec, focused_index: Option, children_bounds: Option>>, + model_usage_context: ModelUsageContext, } impl ContextStrip { @@ -47,6 +48,7 @@ impl ContextStrip { text_thread_store: Option>, context_picker_menu_handle: PopoverMenuHandle, suggest_context_kind: SuggestContextKind, + model_usage_context: ModelUsageContext, window: &mut Window, cx: &mut Context, ) -> Self { @@ -81,6 +83,7 @@ impl ContextStrip { _subscriptions: subscriptions, focused_index: None, children_bounds: None, + model_usage_context, } } @@ -98,11 +101,20 @@ impl ContextStrip { .as_ref() .and_then(|thread_store| thread_store.upgrade()) .and_then(|thread_store| thread_store.read(cx).prompt_store().as_ref()); + + let current_model = self.model_usage_context.language_model(cx); + self.context_store .read(cx) .context() .flat_map(|context| { - AddedContext::new_pending(context.clone(), prompt_store, project, cx) + AddedContext::new_pending( + context.clone(), + prompt_store, + project, + current_model.as_ref(), + cx, + ) }) .collect::>() } else { diff --git a/crates/agent/src/inline_prompt_editor.rs b/crates/agent/src/inline_prompt_editor.rs index 624db3c19e..6aca18ceb8 100644 --- a/crates/agent/src/inline_prompt_editor.rs +++ b/crates/agent/src/inline_prompt_editor.rs @@ -912,6 +912,7 @@ impl PromptEditor { text_thread_store.clone(), context_picker_menu_handle.clone(), SuggestContextKind::Thread, + ModelUsageContext::InlineAssistant, window, cx, ) @@ -1083,6 +1084,7 @@ impl PromptEditor { text_thread_store.clone(), context_picker_menu_handle.clone(), SuggestContextKind::Thread, + ModelUsageContext::InlineAssistant, window, cx, ) diff --git a/crates/agent/src/message_editor.rs b/crates/agent/src/message_editor.rs index 8c620413a1..b9037e3e74 100644 --- a/crates/agent/src/message_editor.rs +++ b/crates/agent/src/message_editor.rs @@ -169,6 +169,7 @@ impl MessageEditor { Some(text_thread_store.clone()), context_picker_menu_handle.clone(), SuggestContextKind::File, + ModelUsageContext::Thread(thread.clone()), window, cx, ) diff --git a/crates/agent/src/ui/context_pill.rs b/crates/agent/src/ui/context_pill.rs index 605a142980..1abdd8fb8d 100644 --- a/crates/agent/src/ui/context_pill.rs +++ b/crates/agent/src/ui/context_pill.rs @@ -93,20 +93,9 @@ impl ContextPill { Self::Suggested { icon_path: Some(icon_path), .. - } - | Self::Added { - context: - AddedContext { - icon_path: Some(icon_path), - .. - }, - .. } => Icon::from_path(icon_path), - Self::Suggested { kind, .. } - | Self::Added { - context: AddedContext { kind, .. }, - .. - } => Icon::new(kind.icon()), + Self::Suggested { kind, .. } => Icon::new(kind.icon()), + Self::Added { context, .. } => context.icon(), } } } @@ -133,6 +122,7 @@ impl RenderOnce for ContextPill { on_click, } => { let status_is_error = matches!(context.status, ContextStatus::Error { .. }); + let status_is_warning = matches!(context.status, ContextStatus::Warning { .. }); base_pill .pr(if on_remove.is_some() { px(2.) } else { px(4.) }) @@ -140,6 +130,9 @@ impl RenderOnce for ContextPill { if status_is_error { pill.bg(cx.theme().status().error_background) .border_color(cx.theme().status().error_border) + } else if status_is_warning { + pill.bg(cx.theme().status().warning_background) + .border_color(cx.theme().status().warning_border) } else if *focused { pill.bg(color.element_background) .border_color(color.border_focused) @@ -195,7 +188,8 @@ impl RenderOnce for ContextPill { |label, delta| label.opacity(delta), ) .into_any_element(), - ContextStatus::Error { message } => element + ContextStatus::Warning { message } + | ContextStatus::Error { message } => element .tooltip(ui::Tooltip::text(message.clone())) .into_any_element(), }), @@ -270,6 +264,7 @@ pub enum ContextStatus { Ready, Loading { message: SharedString }, Error { message: SharedString }, + Warning { message: SharedString }, } #[derive(RegisterComponent)] @@ -285,6 +280,19 @@ pub struct AddedContext { } impl AddedContext { + pub fn icon(&self) -> Icon { + match &self.status { + ContextStatus::Warning { .. } => Icon::new(IconName::Warning).color(Color::Warning), + ContextStatus::Error { .. } => Icon::new(IconName::XCircle).color(Color::Error), + _ => { + if let Some(icon_path) = &self.icon_path { + Icon::from_path(icon_path) + } else { + Icon::new(self.kind.icon()) + } + } + } + } /// Creates an `AddedContext` by retrieving relevant details of `AgentContext`. This returns a /// `None` if `DirectoryContext` or `RulesContext` no longer exist. /// @@ -293,6 +301,7 @@ impl AddedContext { handle: AgentContextHandle, prompt_store: Option<&Entity>, project: &Project, + model: Option<&Arc>, cx: &App, ) -> Option { match handle { @@ -304,11 +313,15 @@ impl AddedContext { AgentContextHandle::Thread(handle) => Some(Self::pending_thread(handle, cx)), AgentContextHandle::TextThread(handle) => Some(Self::pending_text_thread(handle, cx)), AgentContextHandle::Rules(handle) => Self::pending_rules(handle, prompt_store, cx), - AgentContextHandle::Image(handle) => Some(Self::image(handle, cx)), + AgentContextHandle::Image(handle) => Some(Self::image(handle, model, cx)), } } - pub fn new_attached(context: &AgentContext, cx: &App) -> AddedContext { + pub fn new_attached( + context: &AgentContext, + model: Option<&Arc>, + cx: &App, + ) -> AddedContext { match context { AgentContext::File(context) => Self::attached_file(context, cx), AgentContext::Directory(context) => Self::attached_directory(context), @@ -318,7 +331,7 @@ impl AddedContext { AgentContext::Thread(context) => Self::attached_thread(context), AgentContext::TextThread(context) => Self::attached_text_thread(context), AgentContext::Rules(context) => Self::attached_rules(context), - AgentContext::Image(context) => Self::image(context.clone(), cx), + AgentContext::Image(context) => Self::image(context.clone(), model, cx), } } @@ -593,7 +606,11 @@ impl AddedContext { } } - fn image(context: ImageContext, cx: &App) -> AddedContext { + fn image( + context: ImageContext, + model: Option<&Arc>, + cx: &App, + ) -> AddedContext { let (name, parent, icon_path) = if let Some(full_path) = context.full_path.as_ref() { let full_path_string: SharedString = full_path.to_string_lossy().into_owned().into(); let (name, parent) = @@ -604,21 +621,30 @@ impl AddedContext { ("Image".into(), None, None) }; + let status = match context.status(model) { + ImageStatus::Loading => ContextStatus::Loading { + message: "Loading…".into(), + }, + ImageStatus::Error => ContextStatus::Error { + message: "Failed to load Image".into(), + }, + ImageStatus::Warning => ContextStatus::Warning { + message: format!( + "{} doesn't support attaching Images as Context", + model.map(|m| m.name().0).unwrap_or_else(|| "Model".into()) + ) + .into(), + }, + ImageStatus::Ready => ContextStatus::Ready, + }; + AddedContext { kind: ContextKind::Image, name, parent, tooltip: None, icon_path, - status: match context.status() { - ImageStatus::Loading => ContextStatus::Loading { - message: "Loading…".into(), - }, - ImageStatus::Error => ContextStatus::Error { - message: "Failed to load image".into(), - }, - ImageStatus::Ready => ContextStatus::Ready, - }, + status, render_hover: Some(Rc::new({ let image = context.original_image.clone(); move |_, cx| { @@ -787,6 +813,7 @@ impl Component for AddedContext { original_image: Arc::new(Image::empty()), image_task: Task::ready(Some(LanguageModelImage::empty())).shared(), }, + None, cx, ), ); @@ -806,6 +833,7 @@ impl Component for AddedContext { }) .shared(), }, + None, cx, ), ); @@ -820,6 +848,7 @@ impl Component for AddedContext { original_image: Arc::new(Image::empty()), image_task: Task::ready(None).shared(), }, + None, cx, ), ); @@ -841,3 +870,60 @@ impl Component for AddedContext { ) } } + +#[cfg(test)] +mod tests { + use super::*; + use gpui::App; + use language_model::{LanguageModel, fake_provider::FakeLanguageModel}; + use std::sync::Arc; + + #[gpui::test] + fn test_image_context_warning_for_unsupported_model(cx: &mut App) { + let model: Arc = Arc::new(FakeLanguageModel::default()); + assert!(!model.supports_images()); + + let image_context = ImageContext { + context_id: ContextId::zero(), + project_path: None, + original_image: Arc::new(Image::empty()), + image_task: Task::ready(Some(LanguageModelImage::empty())).shared(), + full_path: None, + }; + + let added_context = AddedContext::image(image_context, Some(&model), cx); + + assert!(matches!( + added_context.status, + ContextStatus::Warning { .. } + )); + + assert!(matches!(added_context.kind, ContextKind::Image)); + assert_eq!(added_context.name.as_ref(), "Image"); + assert!(added_context.parent.is_none()); + assert!(added_context.icon_path.is_none()); + } + + #[gpui::test] + fn test_image_context_ready_for_no_model(cx: &mut App) { + let image_context = ImageContext { + context_id: ContextId::zero(), + project_path: None, + original_image: Arc::new(Image::empty()), + image_task: Task::ready(Some(LanguageModelImage::empty())).shared(), + full_path: None, + }; + + let added_context = AddedContext::image(image_context, None, cx); + + assert!( + matches!(added_context.status, ContextStatus::Ready), + "Expected ready status when no model provided" + ); + + assert!(matches!(added_context.kind, ContextKind::Image)); + assert_eq!(added_context.name.as_ref(), "Image"); + assert!(added_context.parent.is_none()); + assert!(added_context.icon_path.is_none()); + } +}