From 962b02424809948860d907dd286bda08732263a1 Mon Sep 17 00:00:00 2001 From: Bennet Bo Fenner Date: Tue, 22 Apr 2025 18:08:35 +0200 Subject: [PATCH] agent: Improve the review changes UX (#29221) Release Notes: - agent: Improved the AI-generated changes review UX by clearly exposing the generating state in the multibuffer tab. --------- Co-authored-by: Danilo Leal --- crates/agent/src/active_thread.rs | 99 ++-------------------- crates/agent/src/agent_diff.rs | 27 +++++- crates/agent/src/message_editor.rs | 4 +- crates/agent/src/ui.rs | 2 + crates/agent/src/ui/animated_label.rs | 116 ++++++++++++++++++++++++++ 5 files changed, 151 insertions(+), 97 deletions(-) create mode 100644 crates/agent/src/ui/animated_label.rs diff --git a/crates/agent/src/active_thread.rs b/crates/agent/src/active_thread.rs index 802826ed1456699583743dc650af398bd7df22af..51181a0d57760ffa4070aff3f55b9f6ec01ef04c 100644 --- a/crates/agent/src/active_thread.rs +++ b/crates/agent/src/active_thread.rs @@ -6,7 +6,9 @@ use crate::thread::{ }; use crate::thread_store::{RulesLoadingError, ThreadStore}; use crate::tool_use::{PendingToolUseStatus, ToolUse}; -use crate::ui::{AddedContext, AgentNotification, AgentNotificationEvent, ContextPill}; +use crate::ui::{ + AddedContext, AgentNotification, AgentNotificationEvent, AnimatedLabel, ContextPill, +}; use crate::{AssistantPanel, OpenActiveThreadAsMarkdown}; use anyhow::Context as _; use assistant_settings::{AssistantSettings, NotifyWhenAgentWaiting}; @@ -1504,45 +1506,8 @@ impl ActiveThread { let needs_confirmation = tool_uses.iter().any(|tool_use| tool_use.needs_confirmation); - let generating_label = (is_generating && is_last_message).then(|| { - Label::new("Generating") - .color(Color::Muted) - .size(LabelSize::Small) - .with_animations( - "generating-label", - vec![ - Animation::new(Duration::from_secs(1)), - Animation::new(Duration::from_secs(1)).repeat(), - ], - |mut label, animation_ix, delta| { - match animation_ix { - 0 => { - let chars_to_show = (delta * 10.).ceil() as usize; - let text = &"Generating"[0..chars_to_show]; - label.set_text(text); - } - 1 => { - let text = match delta { - d if d < 0.25 => "Generating", - d if d < 0.5 => "Generating.", - d if d < 0.75 => "Generating..", - _ => "Generating...", - }; - label.set_text(text); - } - _ => {} - } - label - }, - ) - .with_animation( - "pulsating-label", - Animation::new(Duration::from_secs(2)) - .repeat() - .with_easing(pulsating_between(0.6, 1.)), - |label, delta| label.map_element(|label| label.alpha(delta)), - ) - }); + let generating_label = (is_generating && is_last_message) + .then(|| AnimatedLabel::new("Generating").size(LabelSize::Small)); // Don't render user messages that are just there for returning tool results. if message.role == Role::User && thread.message_has_tool_results(message_id) { @@ -2303,34 +2268,7 @@ impl ActiveThread { .size(IconSize::XSmall) .color(Color::Muted), ) - .child({ - Label::new("Thinking") - .color(Color::Muted) - .size(LabelSize::Small) - .with_animation( - "generating-label", - Animation::new(Duration::from_secs(1)).repeat(), - |mut label, delta| { - let text = match delta { - d if d < 0.25 => "Thinking", - d if d < 0.5 => "Thinking.", - d if d < 0.75 => "Thinking..", - _ => "Thinking...", - }; - label.set_text(text); - label - }, - ) - .with_animation( - "pulsating-label", - Animation::new(Duration::from_secs(2)) - .repeat() - .with_easing(pulsating_between(0.6, 1.)), - |label, delta| { - label.map_element(|label| label.alpha(delta)) - }, - ) - }), + .child(AnimatedLabel::new("Thinking").size(LabelSize::Small)), ) .child( h_flex() @@ -2895,30 +2833,7 @@ impl ActiveThread { .border_color(self.tool_card_border_color(cx)) .rounded_b_lg() .child( - Label::new("Waiting for Confirmation…") - .color(Color::Muted) - .size(LabelSize::Small) - .with_animation( - "generating-label", - Animation::new(Duration::from_secs(1)).repeat(), - |mut label, delta| { - let text = match delta { - d if d < 0.25 => "Waiting for Confirmation", - d if d < 0.5 => "Waiting for Confirmation.", - d if d < 0.75 => "Waiting for Confirmation..", - _ => "Waiting for Confirmation...", - }; - label.set_text(text); - label - }, - ) - .with_animation( - "pulsating-label", - Animation::new(Duration::from_secs(2)) - .repeat() - .with_easing(pulsating_between(0.6, 1.)), - |label, delta| label.map_element(|label| label.alpha(delta)), - ), + AnimatedLabel::new("Waiting for Confirmation").size(LabelSize::Small) ) .child( h_flex() diff --git a/crates/agent/src/agent_diff.rs b/crates/agent/src/agent_diff.rs index 05f003d03a39c6413973f40ba7354a03a2d2da32..7a7b289fa6410abf6263c0e15f1ff6f8fe1894e2 100644 --- a/crates/agent/src/agent_diff.rs +++ b/crates/agent/src/agent_diff.rs @@ -1,4 +1,4 @@ -use crate::{Keep, KeepAll, Reject, RejectAll, Thread, ThreadEvent}; +use crate::{Keep, KeepAll, Reject, RejectAll, Thread, ThreadEvent, ui::AnimatedLabel}; use anyhow::Result; use buffer_diff::DiffHunkStatus; use collections::{HashMap, HashSet}; @@ -8,8 +8,8 @@ use editor::{ scroll::Autoscroll, }; use gpui::{ - Action, AnyElement, AnyView, App, Entity, EventEmitter, FocusHandle, Focusable, SharedString, - Subscription, Task, WeakEntity, Window, prelude::*, + Action, AnyElement, AnyView, App, Empty, Entity, EventEmitter, FocusHandle, Focusable, + SharedString, Subscription, Task, WeakEntity, Window, prelude::*, }; use language::{Capability, DiskState, OffsetRangeExt, Point}; use multi_buffer::PathKey; @@ -307,6 +307,10 @@ impl AgentDiff { window: &mut Window, cx: &mut Context, ) { + if self.thread.read(cx).is_generating() { + return; + } + let snapshot = self.multibuffer.read(cx).snapshot(cx); let diff_hunks_in_ranges = self .editor @@ -339,6 +343,10 @@ impl AgentDiff { window: &mut Window, cx: &mut Context, ) { + if self.thread.read(cx).is_generating() { + return; + } + let snapshot = self.multibuffer.read(cx).snapshot(cx); let diff_hunks_in_ranges = self .editor @@ -650,6 +658,11 @@ fn render_diff_hunk_controls( cx: &mut App, ) -> AnyElement { let editor = editor.clone(); + + if agent_diff.read(cx).thread.read(cx).is_generating() { + return Empty.into_any(); + } + h_flex() .h(line_height) .mr_0p5() @@ -857,8 +870,14 @@ impl Render for AgentDiffToolbar { None => return div(), }; - let is_empty = agent_diff.read(cx).multibuffer.read(cx).is_empty(); + let is_generating = agent_diff.read(cx).thread.read(cx).is_generating(); + if is_generating { + return div() + .w(rems(6.5625)) // Arbitrary 105px size—so the label doesn't dance around + .child(AnimatedLabel::new("Generating")); + } + let is_empty = agent_diff.read(cx).multibuffer.read(cx).is_empty(); if is_empty { return div(); } diff --git a/crates/agent/src/message_editor.rs b/crates/agent/src/message_editor.rs index 633ad2ee5407c5080f6538cb7a8920398e75cd22..14f7e6de40696c508778c0df0a1772d1cf218a9f 100644 --- a/crates/agent/src/message_editor.rs +++ b/crates/agent/src/message_editor.rs @@ -405,8 +405,10 @@ impl MessageEditor { }); } - fn handle_review_click(&self, window: &mut Window, cx: &mut Context) { + fn handle_review_click(&mut self, window: &mut Window, cx: &mut Context) { + self.edits_expanded = true; AgentDiff::deploy(self.thread.clone(), self.workspace.clone(), window, cx).log_err(); + cx.notify(); } fn handle_file_click( diff --git a/crates/agent/src/ui.rs b/crates/agent/src/ui.rs index e31733eccaa270b74238d03a4370e6ab86a88484..d0ef9deafe750fccbde2ce819f9c92465bf58917 100644 --- a/crates/agent/src/ui.rs +++ b/crates/agent/src/ui.rs @@ -1,7 +1,9 @@ mod agent_notification; +mod animated_label; mod context_pill; mod usage_banner; pub use agent_notification::*; +pub use animated_label::*; pub use context_pill::*; pub use usage_banner::*; diff --git a/crates/agent/src/ui/animated_label.rs b/crates/agent/src/ui/animated_label.rs new file mode 100644 index 0000000000000000000000000000000000000000..c08d11e99318d003621c2e4ad7f7eeaf5b440b5c --- /dev/null +++ b/crates/agent/src/ui/animated_label.rs @@ -0,0 +1,116 @@ +use gpui::{Animation, AnimationExt, FontWeight, pulsating_between}; +use std::time::Duration; +use ui::prelude::*; + +#[derive(IntoElement)] +pub struct AnimatedLabel { + base: Label, + text: SharedString, +} + +impl AnimatedLabel { + pub fn new(text: impl Into) -> Self { + let text = text.into(); + AnimatedLabel { + base: Label::new(text.clone()), + text, + } + } +} + +impl LabelCommon for AnimatedLabel { + fn size(mut self, size: LabelSize) -> Self { + self.base = self.base.size(size); + self + } + + fn weight(mut self, weight: FontWeight) -> Self { + self.base = self.base.weight(weight); + self + } + + fn line_height_style(mut self, line_height_style: LineHeightStyle) -> Self { + self.base = self.base.line_height_style(line_height_style); + self + } + + fn color(mut self, color: Color) -> Self { + self.base = self.base.color(color); + self + } + + fn strikethrough(mut self) -> Self { + self.base = self.base.strikethrough(); + self + } + + fn italic(mut self) -> Self { + self.base = self.base.italic(); + self + } + + fn alpha(mut self, alpha: f32) -> Self { + self.base = self.base.alpha(alpha); + self + } + + fn underline(mut self) -> Self { + self.base = self.base.underline(); + self + } + + fn truncate(mut self) -> Self { + self.base = self.base.truncate(); + self + } + + fn single_line(mut self) -> Self { + self.base = self.base.single_line(); + self + } + + fn buffer_font(mut self, cx: &App) -> Self { + self.base = self.base.buffer_font(cx); + self + } +} + +impl RenderOnce for AnimatedLabel { + fn render(self, _window: &mut Window, _cx: &mut App) -> impl IntoElement { + let text = self.text.clone(); + + self.base + .color(Color::Muted) + .with_animations( + "animated-label", + vec![ + Animation::new(Duration::from_secs(1)), + Animation::new(Duration::from_secs(1)).repeat(), + ], + move |mut label, animation_ix, delta| { + match animation_ix { + 0 => { + let chars_to_show = (delta * text.len() as f32).ceil() as usize; + let text = SharedString::from(text[0..chars_to_show].to_string()); + label.set_text(text); + } + 1 => match delta { + d if d < 0.25 => label.set_text(text.clone()), + d if d < 0.5 => label.set_text(format!("{}.", text)), + d if d < 0.75 => label.set_text(format!("{}..", text)), + _ => label.set_text(format!("{}...", text)), + }, + _ => {} + } + label + }, + ) + .with_animation( + "pulsating-label", + Animation::new(Duration::from_secs(2)) + .repeat() + .with_easing(pulsating_between(0.6, 1.)), + |label, delta| label.map_element(|label| label.alpha(delta)), + ) + } +}