From 7dfbe0b9088def57e7b7e42be99b4cf8c531efad Mon Sep 17 00:00:00 2001 From: Danilo Leal <67129314+danilo-leal@users.noreply.github.com> Date: Mon, 5 May 2025 15:50:53 -0300 Subject: [PATCH] agent: Improve terminal tool card design (#29712) To-dos: - [x] Expose the command to defend against cases where that's just super long - [x] Tackle the vertical scroll conflict with panel scroll - [x] Reduce default font-size Release Notes: - N/A --------- Co-authored-by: Ben Brandt Co-authored-by: Mikayla Maki Co-authored-by: Agus Zubiaga --- crates/assistant_tools/src/edit_file_tool.rs | 1 - crates/assistant_tools/src/terminal_tool.rs | 244 +++++++++---------- crates/debugger_ui/src/session/running.rs | 10 +- crates/terminal/src/terminal.rs | 14 ++ crates/terminal_view/src/persistence.rs | 1 + crates/terminal_view/src/terminal_element.rs | 39 ++- crates/terminal_view/src/terminal_panel.rs | 3 + crates/terminal_view/src/terminal_view.rs | 7 +- 8 files changed, 174 insertions(+), 145 deletions(-) diff --git a/crates/assistant_tools/src/edit_file_tool.rs b/crates/assistant_tools/src/edit_file_tool.rs index 900062936a..814f2948ce 100644 --- a/crates/assistant_tools/src/edit_file_tool.rs +++ b/crates/assistant_tools/src/edit_file_tool.rs @@ -708,7 +708,6 @@ impl ToolCard for EditFileToolCard { .cursor_pointer() .h_5() .justify_center() - .rounded_b_md() .border_t_1() .border_color(border_color) .bg(cx.theme().colors().editor_background) diff --git a/crates/assistant_tools/src/terminal_tool.rs b/crates/assistant_tools/src/terminal_tool.rs index a9002597e1..f2f43ac5b9 100644 --- a/crates/assistant_tools/src/terminal_tool.rs +++ b/crates/assistant_tools/src/terminal_tool.rs @@ -1,10 +1,7 @@ use crate::schema::json_schema_for; use anyhow::{Context as _, Result, anyhow, bail}; use assistant_tool::{ActionLog, Tool, ToolCard, ToolResult, ToolUseStatus}; -use gpui::{ - Animation, AnimationExt, AnyWindowHandle, App, AppContext, Empty, Entity, EntityId, Task, - Transformation, WeakEntity, Window, percentage, -}; +use gpui::{AnyWindowHandle, App, AppContext, Empty, Entity, EntityId, Task, WeakEntity, Window}; use language::LineEnding; use language_model::{LanguageModelRequestMessage, LanguageModelToolSchemaFormat}; use portable_pty::{CommandBuilder, PtySize, native_pty_system}; @@ -19,7 +16,7 @@ use std::{ time::{Duration, Instant}, }; use terminal_view::TerminalView; -use ui::{Disclosure, IconName, Tooltip, prelude::*}; +use ui::{Disclosure, Tooltip, prelude::*}; use util::{ get_system_shell, markdown::MarkdownInlineCode, size::format_file_size, time::duration_alt_display, @@ -175,11 +172,13 @@ impl Tool for TerminalTool { workspace.downgrade(), None, project.downgrade(), + true, window, cx, ) }) })?; + let _ = card.update(cx, |card, _| { card.terminal = Some(terminal_view.clone()); card.start_instant = Instant::now(); @@ -378,155 +377,118 @@ impl ToolCard for TerminalToolCard { }; let tool_failed = matches!(status, ToolUseStatus::Error(_)); + let command_failed = self.command_finished && self.exit_status.is_none_or(|code| !code.success()); + if (tool_failed || command_failed) && self.elapsed_time.is_none() { self.elapsed_time = Some(self.start_instant.elapsed()); } let time_elapsed = self .elapsed_time .unwrap_or_else(|| self.start_instant.elapsed()); - let should_hide_terminal = - tool_failed || self.finished_with_empty_output || !self.preview_expanded; + let should_hide_terminal = tool_failed || self.finished_with_empty_output; - let border_color = cx.theme().colors().border.opacity(0.6); let header_bg = cx .theme() .colors() .element_background .blend(cx.theme().colors().editor_foreground.opacity(0.025)); - let header_label = h_flex() - .w_full() - .max_w_full() - .px_1() - .gap_0p5() - .opacity(0.8) - .child( - h_flex() - .child( - Icon::new(IconName::Terminal) - .size(IconSize::XSmall) - .color(Color::Muted), - ) - .child( - div() - .id(("terminal-tool-header-input-command", self.entity_id)) - .text_size(rems(0.8125)) - .font_buffer(cx) - .child(self.input_command.clone()) - .ml_1p5() - .mr_0p5() - .tooltip({ - let path = self - .working_dir - .as_ref() - .cloned() - .or_else(|| env::current_dir().ok()) - .map(|path| format!("\"{}\"", path.display())) - .unwrap_or_else(|| "current directory".to_string()); - Tooltip::text(if self.command_finished { - format!("Ran in {path}") - } else { - format!("Running in {path}") - }) - }), - ), - ) - .into_any_element(); + let border_color = cx.theme().colors().border.opacity(0.6); + + let path = self + .working_dir + .as_ref() + .cloned() + .or_else(|| env::current_dir().ok()) + .map(|path| format!("{}", path.display())) + .unwrap_or_else(|| "current directory".to_string()); let header = h_flex() .flex_none() - .p_1() .gap_1() .justify_between() .rounded_t_md() - .bg(header_bg) - .child(header_label) - .map(|header| { - let header = header - .when(self.was_content_truncated, |header| { - let tooltip = - if self.content_line_count + 10 > terminal::MAX_SCROLL_HISTORY_LINES { - "Output exceeded terminal max lines and was \ - truncated, the model received the first 16 KB." - .to_string() - } else { - format!( - "Output is {} long, to avoid unexpected token usage, \ - only 16 KB was sent back to the model.", - format_file_size(self.original_content_len as u64, true), - ) - }; - header.child( - div() - .id(("terminal-tool-truncated-label", self.entity_id)) - .tooltip(Tooltip::text(tooltip)) - .child( - Label::new("(truncated)") - .color(Color::Disabled) - .size(LabelSize::Small), - ), - ) - }) - .when(time_elapsed > Duration::from_secs(10), |header| { - header.child( - Label::new(format!("({})", duration_alt_display(time_elapsed))) - .buffer_font(cx) - .color(Color::Disabled) - .size(LabelSize::Small), - ) - }); - - if tool_failed || command_failed { - header.child( - div() - .id(("terminal-tool-error-code-indicator", self.entity_id)) - .child( - Icon::new(IconName::Close) - .size(IconSize::Small) - .color(Color::Error), - ) - .when(command_failed && self.exit_status.is_some(), |this| { - this.tooltip(Tooltip::text(format!( - "Exited with code {}", - self.exit_status - .and_then(|status| status.code()) - .unwrap_or(-1), - ))) - }) - .when( - !command_failed && tool_failed && status.error().is_some(), - |this| { - this.tooltip(Tooltip::text(format!( - "Error: {}", - status.error().unwrap(), - ))) - }, - ), - ) - } else if self.command_finished { - header.child( - Icon::new(IconName::Check) - .size(IconSize::Small) - .color(Color::Success), - ) + .child( + div() + .id(("command-target-path", self.entity_id)) + .w_full() + .max_w_full() + .overflow_x_scroll() + .child( + Label::new(path) + .buffer_font(cx) + .size(LabelSize::XSmall) + .color(Color::Muted), + ), + ) + .when(self.was_content_truncated, |header| { + let tooltip = if self.content_line_count + 10 > terminal::MAX_SCROLL_HISTORY_LINES { + "Output exceeded terminal max lines and was \ + truncated, the model received the first 16 KB." + .to_string() } else { - header.child( - Icon::new(IconName::ArrowCircle) - .size(IconSize::Small) - .color(Color::Info) - .with_animation( - "arrow-circle", - Animation::new(Duration::from_secs(2)).repeat(), - |icon, delta| { - icon.transform(Transformation::rotate(percentage(delta))) - }, - ), + format!( + "Output is {} long, to avoid unexpected token usage, \ + only 16 KB was sent back to the model.", + format_file_size(self.original_content_len as u64, true), ) - } + }; + header.child( + h_flex() + .id(("terminal-tool-truncated-label", self.entity_id)) + .tooltip(Tooltip::text(tooltip)) + .gap_1() + .child( + Icon::new(IconName::Info) + .size(IconSize::XSmall) + .color(Color::Ignored), + ) + .child( + Label::new("Truncated") + .color(Color::Muted) + .size(LabelSize::Small), + ), + ) }) - .when(!tool_failed && !self.finished_with_empty_output, |header| { + .when(time_elapsed > Duration::from_secs(10), |header| { + header.child( + Label::new(format!("({})", duration_alt_display(time_elapsed))) + .buffer_font(cx) + .color(Color::Muted) + .size(LabelSize::Small), + ) + }) + .when(tool_failed || command_failed, |header| { + header.child( + div() + .id(("terminal-tool-error-code-indicator", self.entity_id)) + .child( + Icon::new(IconName::Close) + .size(IconSize::Small) + .color(Color::Error), + ) + .when(command_failed && self.exit_status.is_some(), |this| { + this.tooltip(Tooltip::text(format!( + "Exited with code {}", + self.exit_status + .and_then(|status| status.code()) + .unwrap_or(-1), + ))) + }) + .when( + !command_failed && tool_failed && status.error().is_some(), + |this| { + this.tooltip(Tooltip::text(format!( + "Error: {}", + status.error().unwrap(), + ))) + }, + ), + ) + }) + .when(!should_hide_terminal, |header| { header.child( Disclosure::new( ("terminal-tool-disclosure", self.entity_id), @@ -549,9 +511,25 @@ impl ToolCard for TerminalToolCard { .border_color(border_color) .rounded_lg() .overflow_hidden() - .child(header) - .when(!should_hide_terminal, |this| { - this.child(div().child(terminal.clone()).min_h(px(250.0))) + .child( + v_flex().p_2().gap_0p5().bg(header_bg).child(header).child( + Label::new(self.input_command.clone()) + .buffer_font(cx) + .size(LabelSize::Small), + ), + ) + .when(self.preview_expanded && !should_hide_terminal, |this| { + this.child( + div() + .pt_2() + .min_h_72() + .border_t_1() + .border_color(border_color) + .bg(cx.theme().colors().editor_background) + .rounded_b_md() + .text_ui_sm(cx) + .child(terminal.clone()), + ) }) .into_any() } diff --git a/crates/debugger_ui/src/session/running.rs b/crates/debugger_ui/src/session/running.rs index e908c7c119..5654609a92 100644 --- a/crates/debugger_ui/src/session/running.rs +++ b/crates/debugger_ui/src/session/running.rs @@ -749,7 +749,15 @@ impl RunningState { let terminal = terminal_task.await?; let terminal_view = cx.new_window_entity(|window, cx| { - TerminalView::new(terminal.clone(), workspace, None, weak_project, window, cx) + TerminalView::new( + terminal.clone(), + workspace, + None, + weak_project, + false, + window, + cx, + ) })?; running.update_in(cx, |running, window, cx| { diff --git a/crates/terminal/src/terminal.rs b/crates/terminal/src/terminal.rs index b905eea50d..9cd11311f2 100644 --- a/crates/terminal/src/terminal.rs +++ b/crates/terminal/src/terminal.rs @@ -601,6 +601,8 @@ pub struct TerminalContent { pub cursor_char: char, pub terminal_bounds: TerminalBounds, pub last_hovered_word: Option, + pub scrolled_to_top: bool, + pub scrolled_to_bottom: bool, } #[derive(Clone)] @@ -625,6 +627,8 @@ impl Default for TerminalContent { cursor_char: Default::default(), terminal_bounds: Default::default(), last_hovered_word: None, + scrolled_to_top: false, + scrolled_to_bottom: false, } } } @@ -1208,6 +1212,14 @@ impl Terminal { .push_back(InternalEvent::Scroll(AlacScroll::Bottom)); } + pub fn scrolled_to_top(&self) -> bool { + self.last_content.scrolled_to_top + } + + pub fn scrolled_to_bottom(&self) -> bool { + self.last_content.scrolled_to_bottom + } + ///Resize the terminal and the PTY. pub fn set_size(&mut self, new_bounds: TerminalBounds) { if self.last_content.terminal_bounds != new_bounds { @@ -1405,6 +1417,8 @@ impl Terminal { cursor_char: term.grid()[content.cursor.point].c, terminal_bounds: last_content.terminal_bounds, last_hovered_word: last_content.last_hovered_word.clone(), + scrolled_to_top: content.display_offset == term.history_size(), + scrolled_to_bottom: content.display_offset == 0, } } diff --git a/crates/terminal_view/src/persistence.rs b/crates/terminal_view/src/persistence.rs index 72937f2e4c..5186a08b50 100644 --- a/crates/terminal_view/src/persistence.rs +++ b/crates/terminal_view/src/persistence.rs @@ -261,6 +261,7 @@ async fn deserialize_pane_group( workspace.clone(), Some(workspace_id), project.downgrade(), + false, window, cx, ) diff --git a/crates/terminal_view/src/terminal_element.rs b/crates/terminal_view/src/terminal_element.rs index 71e89286dc..7b1304159f 100644 --- a/crates/terminal_view/src/terminal_element.rs +++ b/crates/terminal_view/src/terminal_element.rs @@ -1,9 +1,9 @@ use editor::{CursorLayout, HighlightedRange, HighlightedRangeLine}; use gpui::{ AnyElement, App, AvailableSpace, Bounds, ContentMask, Context, DispatchPhase, Element, - ElementId, Entity, FocusHandle, Font, FontStyle, FontWeight, GlobalElementId, HighlightStyle, - Hitbox, Hsla, InputHandler, InteractiveElement, Interactivity, IntoElement, LayoutId, - ModifiersChangedEvent, MouseButton, MouseMoveEvent, Pixels, Point, ShapedLine, + ElementId, Entity, FocusHandle, Focusable, Font, FontStyle, FontWeight, GlobalElementId, + HighlightStyle, Hitbox, Hsla, InputHandler, InteractiveElement, Interactivity, IntoElement, + LayoutId, ModifiersChangedEvent, MouseButton, MouseMoveEvent, Pixels, Point, ShapedLine, StatefulInteractiveElement, StrikethroughStyle, Styled, TextRun, TextStyle, UTF16Selection, UnderlineStyle, WeakEntity, WhiteSpace, Window, WindowTextSystem, div, fill, point, px, relative, size, @@ -158,6 +158,7 @@ pub struct TerminalElement { focused: bool, cursor_visible: bool, interactivity: Interactivity, + embedded: bool, block_below_cursor: Option>, } @@ -178,6 +179,7 @@ impl TerminalElement { focused: bool, cursor_visible: bool, block_below_cursor: Option>, + embedded: bool, ) -> TerminalElement { TerminalElement { terminal, @@ -187,6 +189,7 @@ impl TerminalElement { focus: focus.clone(), cursor_visible, block_below_cursor, + embedded, interactivity: Default::default(), } .track_focus(&focus) @@ -503,11 +506,15 @@ impl TerminalElement { ); self.interactivity.on_scroll_wheel({ let terminal_view = self.terminal_view.downgrade(); - move |e, _, cx| { + move |e, window, cx| { terminal_view .update(cx, |terminal_view, cx| { - terminal_view.scroll_wheel(e, cx); - cx.notify(); + if !terminal_view.embedded + || terminal_view.focus_handle(cx).is_focused(window) + { + terminal_view.scroll_wheel(e, cx); + cx.notify(); + } }) .ok(); } @@ -580,6 +587,16 @@ impl Element for TerminalElement { window: &mut Window, cx: &mut App, ) -> (LayoutId, Self::RequestLayoutState) { + if self.embedded { + let scrollable = { + let term = self.terminal.read(cx); + !term.scrolled_to_top() && !term.scrolled_to_bottom() && self.focused + }; + if scrollable { + self.interactivity.occlude_mouse(); + } + } + let layout_id = self.interactivity .request_layout(global_id, window, cx, |mut style, window, cx| { @@ -636,10 +653,14 @@ impl Element for TerminalElement { let font_weight = terminal_settings.font_weight.unwrap_or_default(); let line_height = terminal_settings.line_height.value(); - let font_size = terminal_settings.font_size; - let font_size = - font_size.map_or(buffer_font_size, |size| theme::adjusted_font_size(size, cx)); + let font_size = if self.embedded { + window.text_style().font_size.to_pixels(window.rem_size()) + } else { + terminal_settings + .font_size + .map_or(buffer_font_size, |size| theme::adjusted_font_size(size, cx)) + }; let theme = cx.theme().clone(); diff --git a/crates/terminal_view/src/terminal_panel.rs b/crates/terminal_view/src/terminal_panel.rs index e2675fb11c..f6964abf05 100644 --- a/crates/terminal_view/src/terminal_panel.rs +++ b/crates/terminal_view/src/terminal_panel.rs @@ -437,6 +437,7 @@ impl TerminalPanel { weak_workspace.clone(), database_id, project.downgrade(), + false, window, cx, ) @@ -674,6 +675,7 @@ impl TerminalPanel { workspace.weak_handle(), workspace.database_id(), workspace.project().downgrade(), + false, window, cx, ) @@ -714,6 +716,7 @@ impl TerminalPanel { workspace.weak_handle(), workspace.database_id(), workspace.project().downgrade(), + false, window, cx, ) diff --git a/crates/terminal_view/src/terminal_view.rs b/crates/terminal_view/src/terminal_view.rs index ba1f02caaa..3f392da438 100644 --- a/crates/terminal_view/src/terminal_view.rs +++ b/crates/terminal_view/src/terminal_view.rs @@ -111,6 +111,7 @@ pub struct TerminalView { context_menu: Option<(Entity, gpui::Point, Subscription)>, cursor_shape: CursorShape, blink_state: bool, + embedded: bool, blinking_terminal_enabled: bool, cwd_serialized: bool, blinking_paused: bool, @@ -162,6 +163,7 @@ impl TerminalView { workspace: WeakEntity, workspace_id: Option, project: WeakEntity, + embedded: bool, window: &mut Window, cx: &mut Context, ) -> Self { @@ -200,6 +202,7 @@ impl TerminalView { blink_epoch: 0, hover_target_tooltip: None, hover_tooltip_update: Task::ready(()), + embedded, workspace_id, show_breadcrumbs: TerminalSettings::get_global(cx).toolbar.breadcrumbs, block_below_cursor: None, @@ -381,7 +384,6 @@ impl TerminalView { return; } } - self.terminal.update(cx, |term, _| term.scroll_wheel(event)); } @@ -1377,6 +1379,7 @@ impl Render for TerminalView { focused, self.should_show_cursor(focused, cx), self.block_below_cursor.clone(), + self.embedded, )) .when_some(self.render_scrollbar(cx), |div, scrollbar| { div.child(scrollbar) @@ -1502,6 +1505,7 @@ impl Item for TerminalView { self.workspace.clone(), workspace_id, self.project.clone(), + false, window, cx, ) @@ -1659,6 +1663,7 @@ impl SerializableItem for TerminalView { workspace, Some(workspace_id), project.downgrade(), + false, window, cx, )