agent: Improve the review changes UX (#29221)

Bennet Bo Fenner and Danilo Leal created

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 <daniloleal09@gmail.com>

Change summary

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(-)

Detailed changes

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()

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<Self>,
     ) {
+        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<Self>,
     ) {
+        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();
         }

crates/agent/src/message_editor.rs 🔗

@@ -405,8 +405,10 @@ impl MessageEditor {
         });
     }
 
-    fn handle_review_click(&self, window: &mut Window, cx: &mut Context<Self>) {
+    fn handle_review_click(&mut self, window: &mut Window, cx: &mut Context<Self>) {
+        self.edits_expanded = true;
         AgentDiff::deploy(self.thread.clone(), self.workspace.clone(), window, cx).log_err();
+        cx.notify();
     }
 
     fn handle_file_click(

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::*;

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<SharedString>) -> 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)),
+            )
+    }
+}