From 2d581f4c011c5381c8fa68965588031b2634f00d Mon Sep 17 00:00:00 2001 From: Ryan Ward Date: Thu, 10 Sep 2026 00:30:57 -0500 Subject: [PATCH] Fix tool output clipping and live-edge scroll following Preserve full terminal-grid width with horizontal scrolling in tool panes. Keep hidden command control changes from unpinning the viewport, and re-pin scrolling at the live edge in normal and inverted layouts. Validation: formatting, test-layout checks, both presubmit Clippy commands, and 141 relevant tests passed. Ten other terminal-view tests failed identically on the untouched baseline (keyboard, cloud-agent, input status, and footer behavior). --- .../inline_action/requested_command.rs | 28 +++- app/src/terminal/block_list_viewport.rs | 82 ++++++------ app/src/terminal/view.rs | 12 +- app/src/terminal/view_tests.rs | 120 +++++++++++++++++- 4 files changed, 197 insertions(+), 45 deletions(-) diff --git a/app/src/ai/blocklist/inline_action/requested_command.rs b/app/src/ai/blocklist/inline_action/requested_command.rs index 550f6be8..f716f860 100644 --- a/app/src/ai/blocklist/inline_action/requested_command.rs +++ b/app/src/ai/blocklist/inline_action/requested_command.rs @@ -8,7 +8,9 @@ use std::sync::Arc; use galaxy_core::ui::appearance::Appearance; use galaxy_core::ui::Icon; use galaxy_editor::render::element::VerticalExpansionBehavior; -use galaxyui::elements::new_scrollable::SingleAxisConfig; +use galaxyui::elements::new_scrollable::{ + ClippedAxisConfiguration, DualAxisConfig, ScrollableAppearance, +}; use galaxyui::elements::{ Align, Border, ChildView, Clipped, ClippedScrollStateHandle, ConstrainedBox, Container, CornerRadius, CrossAxisAlignment, Expanded, Fill, Flex, MainAxisSize, MouseStateHandle, @@ -336,6 +338,7 @@ pub struct RequestedCommandView { is_user_expanded: bool, header_mouse_state: MouseStateHandle, output_scroll_state: ClippedScrollStateHandle, + output_horizontal_scroll_state: ClippedScrollStateHandle, follow_command_output: Cell, is_editing: bool, @@ -596,6 +599,7 @@ impl RequestedCommandView { is_user_expanded: false, header_mouse_state: Default::default(), output_scroll_state: ClippedScrollStateHandle::new(), + output_horizontal_scroll_state: ClippedScrollStateHandle::new(), follow_command_output: Cell::new(true), copied_from_citation: None, derived_from_citations: Default::default(), @@ -1731,15 +1735,27 @@ impl View for RequestedCommandView { if should_use_ligature_rendering(app) { output_grid = output_grid.with_ligature_rendering(); } - let scrollable = NewScrollable::vertical( - SingleAxisConfig::Clipped { - handle: self.output_scroll_state.clone(), + // The terminal grid can be wider than this padded tool pane. Lay it out + // at its natural width so every column remains reachable by scrolling. + let scrollable = NewScrollable::horizontal_and_vertical( + DualAxisConfig::Clipped { + horizontal: ClippedAxisConfiguration { + handle: self.output_horizontal_scroll_state.clone(), + max_size: None, + stretch_child: false, + }, + vertical: ClippedAxisConfiguration { + handle: self.output_scroll_state.clone(), + max_size: None, + stretch_child: false, + }, child: output_grid.finish(), }, - Fill::None, - Fill::None, + theme.nonactive_ui_detail().into(), + theme.active_ui_detail().into(), Fill::None, ) + .with_horizontal_scrollbar(ScrollableAppearance::new(SCROLLBAR_WIDTH, false)) .with_propagate_mousewheel_if_not_handled(true) .finish(); let output_body = ConstrainedBox::new(scrollable) diff --git a/app/src/terminal/block_list_viewport.rs b/app/src/terminal/block_list_viewport.rs index 3f623a6d..3963c672 100644 --- a/app/src/terminal/block_list_viewport.rs +++ b/app/src/terminal/block_list_viewport.rs @@ -136,6 +136,9 @@ impl ScrollLines { #[derive(Copy, Clone, Debug, PartialEq)] pub enum ScrollPosition { + /// Follow newly inserted content at the top of an inverted block list. + FollowsTopOfMostRecentBlock, + /// The scrolling follows the bottom of the most recently executed block. /// In terms of scroll_top, this implies scrolling stays locked to max_scroll_top. FollowsBottomOfMostRecentBlock, @@ -645,6 +648,7 @@ impl<'a> ViewportState<'a> { /// How far the view is scrolled from the top of all blocks in lines. pub fn scroll_top_in_lines(&self) -> Lines { match (self.input_mode, self.scroll_position) { + (_, ScrollPosition::FollowsTopOfMostRecentBlock) => Lines::zero(), ( InputMode::PinnedToBottom, ScrollPosition::FollowsBottomOfMostRecentBlock @@ -775,7 +779,17 @@ impl<'a> ViewportState<'a> { return self.scroll_position; } - self.scroll_position_after_command_execution(app) + // Keep an established live-edge pin across tool/response insertion. + // Recomputing a waterfall gap offset here can turn it into a fixed offset. + match self.scroll_position { + ScrollPosition::FollowsTopOfMostRecentBlock + | ScrollPosition::FollowsBottomOfMostRecentBlock => self.scroll_position, + ScrollPosition::WaterfallGapFollowsBottomOfMostRecentBlock { .. } + | ScrollPosition::FixedAtPosition { .. } + | ScrollPosition::FixedWithinLongRunningBlock { .. } => { + self.scroll_position_after_command_execution(app) + } + } } ScrollPositionUpdate::AfterKeydownOnTerminal | ScrollPositionUpdate::AfterTypedCharacters @@ -821,34 +835,19 @@ impl<'a> ViewportState<'a> { self.scroll_position } } - ScrollPositionUpdate::AfterPageUp => { - let new_scroll_top = (self.scroll_top_in_lines() - - self.content_element_height_lines() - + 1.0.into_lines()) - .max(Lines::zero()); - ScrollPosition::FixedAtPosition { - scroll_lines: self.scroll_lines_from_scroll_top(new_scroll_top), - } - } - ScrollPositionUpdate::AfterPageDown => { - let total_block_heights = self.block_list.block_heights().summary().height; - let visible_rows = self.content_element_height_lines(); - let current_position = self.scroll_top_in_lines(); - if current_position + visible_rows - 1.0.into_lines() - > (total_block_heights - visible_rows).max(Lines::zero()) - { - ScrollPosition::FollowsBottomOfMostRecentBlock + ScrollPositionUpdate::AfterPageUp => self + .scroll_position_for_delta(self.content_element_height_lines() - 1.0.into_lines()), + ScrollPositionUpdate::AfterPageDown => self + .scroll_position_for_delta(1.0.into_lines() - self.content_element_height_lines()), + ScrollPositionUpdate::AfterHome => self.scroll_position_for_offset(Lines::zero()), + ScrollPositionUpdate::AfterEnterAgentView => { + if self.input_mode.is_inverted_blocklist() { + ScrollPosition::FollowsTopOfMostRecentBlock } else { - let new_scroll_top = current_position + visible_rows - 1.0.into_lines(); - ScrollPosition::FixedAtPosition { - scroll_lines: self.scroll_lines_from_scroll_top(new_scroll_top), - } + ScrollPosition::FollowsBottomOfMostRecentBlock } } - ScrollPositionUpdate::AfterHome => ScrollPosition::FixedAtPosition { - scroll_lines: self.scroll_lines_from_scroll_top(Lines::zero()), - }, - ScrollPositionUpdate::AfterEnd | ScrollPositionUpdate::AfterEnterAgentView => { + ScrollPositionUpdate::AfterEnd => { if matches!( self.input_mode, InputMode::PinnedToBottom | InputMode::Waterfall @@ -904,7 +903,8 @@ impl<'a> ViewportState<'a> { ) -> ScrollPosition { if matches!( self.scroll_position, - ScrollPosition::FollowsBottomOfMostRecentBlock + ScrollPosition::FollowsTopOfMostRecentBlock + | ScrollPosition::FollowsBottomOfMostRecentBlock | ScrollPosition::WaterfallGapFollowsBottomOfMostRecentBlock { .. } ) { return self.scroll_position; @@ -975,7 +975,8 @@ impl<'a> ViewportState<'a> { ) -> ScrollPosition { if matches!( self.scroll_position, - ScrollPosition::FollowsBottomOfMostRecentBlock + ScrollPosition::FollowsTopOfMostRecentBlock + | ScrollPosition::FollowsBottomOfMostRecentBlock | ScrollPosition::WaterfallGapFollowsBottomOfMostRecentBlock { .. } ) { return self.scroll_position; @@ -1203,9 +1204,8 @@ impl<'a> ViewportState<'a> { fn scroll_position_after_clear(&self) -> ScrollPosition { match self.input_mode { - InputMode::PinnedToTop | InputMode::PinnedToBottom => { - ScrollPosition::FollowsBottomOfMostRecentBlock - } + InputMode::PinnedToTop => ScrollPosition::FollowsTopOfMostRecentBlock, + InputMode::PinnedToBottom => ScrollPosition::FollowsBottomOfMostRecentBlock, InputMode::Waterfall => ScrollPosition::WaterfallGapFollowsBottomOfMostRecentBlock { scroll_top_in_lines: self.max_scroll_top_in_lines(), }, @@ -1254,6 +1254,7 @@ impl<'a> ViewportState<'a> { // Returns the scroll position to set after a command has finished executing fn scroll_position_after_command_execution(&self, app: &AppContext) -> ScrollPosition { match (self.input_mode, self.block_list.active_gap()) { + (InputMode::PinnedToTop, _) => ScrollPosition::FollowsTopOfMostRecentBlock, (InputMode::Waterfall, Some(gap)) => { // In gap waterfall mode, the logic is somewhat complex for how to adjust scroll position // after a command is executed. The basic result we want is: @@ -1297,13 +1298,20 @@ impl<'a> ViewportState<'a> { let current_top = self.scroll_top_in_lines(); let new_top = (current_top - delta).max(Lines::zero()).min(max_scroll_top); - let fix_to_bottom = new_top >= max_scroll_top - && matches!( - self.input_mode, - InputMode::PinnedToBottom | InputMode::Waterfall - ); + // Horizontal wheel gestures and no-op events must not change follow state. + if delta == Lines::zero() { + return self.scroll_position; + } + self.scroll_position_for_offset(new_top) + } - if fix_to_bottom { + fn scroll_position_for_offset(&self, new_top: Lines) -> ScrollPosition { + let tolerance = Pixels::new(0.5).to_lines(self.size_info.cell_height_px()); + if self.input_mode.is_inverted_blocklist() && new_top <= tolerance { + ScrollPosition::FollowsTopOfMostRecentBlock + } else if !self.input_mode.is_inverted_blocklist() + && new_top + tolerance >= self.max_scroll_top_in_lines() + { ScrollPosition::FollowsBottomOfMostRecentBlock } else if self.block_list.active_block().is_active_and_long_running() && self.does_block_exceed_viewport(self.block_list.active_block_index(), new_top) diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index d6799f95..e813367c 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -6797,7 +6797,17 @@ impl TerminalView { .. } => { if !*agent_has_control && ctx.is_self_or_child_focused() { - let block_index = self.model.lock().block_list().block_index_for_id(block_id); + // Tool output is rendered in its agent card. Its hidden shell block + // must not steal the viewport or clear the user's follow-scroll state. + let block_index = { + let model = self.model.lock(); + let blocks = model.block_list(); + blocks.block_index_for_id(block_id).filter(|index| { + blocks + .block_at(*index) + .is_some_and(|block| block.is_visible(blocks.agent_view_state())) + }) + }; if let Some(block_index) = block_index { self.update_scroll_position_locking( ScrollPositionUpdate::ScrollToBottomOfBlock { block_index }, diff --git a/app/src/terminal/view_tests.rs b/app/src/terminal/view_tests.rs index 56b69a37..0ea4e78a 100644 --- a/app/src/terminal/view_tests.rs +++ b/app/src/terminal/view_tests.rs @@ -3433,7 +3433,7 @@ fn test_viewport_most_recent_at_top() { }, ctx ), - ScrollPosition::FixedAtPosition { .. } + ScrollPosition::FollowsTopOfMostRecentBlock )); assert_eq!( Lines::zero(), @@ -8309,3 +8309,121 @@ fn child_shell_startup_failure_finishes_the_waiting_conversation() { }); }); } + +#[test] +fn scrolling_to_live_edge_repins_both_blocklist_directions() { + App::test((), |mut app| async move { + initialize_app_for_terminal_view(&mut app); + let terminal = add_window_with_terminal(&mut app, None); + terminal.update(&mut app, |view, ctx| { + for mode in [ + InputMode::PinnedToBottom, + InputMode::Waterfall, + InputMode::PinnedToTop, + ] { + let mut model = view.model.lock(); + for _ in 0..100 { + model.simulate_block("echo", "output"); + } + let pinned = if mode.is_inverted_blocklist() { + ScrollPosition::FollowsTopOfMostRecentBlock + } else { + ScrollPosition::FollowsBottomOfMostRecentBlock + }; + view.scroll_position = ScrollState::new(pinned); + let away = if mode.is_inverted_blocklist() { + -5.0 + } else { + 5.0 + }; + let viewport = view.viewport_state(model.block_list(), mode, ctx); + let unpinned = viewport.next_scroll_position( + ScrollPositionUpdate::AfterScrollEvent { + scroll_delta: away.into_lines(), + }, + ctx, + ); + assert!(matches!(unpinned, ScrollPosition::FixedAtPosition { .. })); + view.scroll_position = ScrollState::new(unpinned); + model.simulate_block("echo", "new output"); + assert_eq!(view.scroll_position(), unpinned); + let viewport = view.viewport_state(model.block_list(), mode, ctx); + let toward = if mode.is_inverted_blocklist() { + 10000.0 + } else { + -10000.0 + }; + let repinned = viewport.next_scroll_position( + ScrollPositionUpdate::AfterScrollEvent { + scroll_delta: toward.into_lines(), + }, + ctx, + ); + assert_eq!(repinned, pinned); + view.scroll_position = ScrollState::new(repinned); + model.simulate_block("echo", "more output"); + let viewport = view.viewport_state(model.block_list(), mode, ctx); + assert_eq!( + viewport + .next_scroll_position(ScrollPositionUpdate::AfterRichBlockInserted, ctx), + pinned + ); + assert_eq!( + viewport.next_scroll_position( + ScrollPositionUpdate::AfterScrollEvent { + scroll_delta: Lines::zero() + }, + ctx + ), + pinned + ); + if mode.is_inverted_blocklist() { + assert_eq!(viewport.scroll_top_in_lines(), Lines::zero()); + } else { + assert_eq!( + viewport.scroll_top_in_lines(), + viewport.max_scroll_top_in_lines() + ); + } + } + }); + }); +} + +#[test] +fn hidden_tool_control_changes_preserve_pinned_and_unpinned_scroll() { + App::test((), |mut app| async move { + initialize_app_for_terminal_view(&mut app); + let terminal = add_window_with_terminal(&mut app, None); + terminal.update(&mut app, |view, ctx| { + view.focus_terminal(ctx); + assert!(ctx.is_self_or_child_focused()); + let block_id = { + let mut model = view.model.lock(); + model.simulate_cmd("echo tool output"); + let block = model.block_list_mut().active_block_mut(); + block.hide(); + block.id().clone() + }; + for position in [ + ScrollPosition::FollowsBottomOfMostRecentBlock, + ScrollPosition::FollowsTopOfMostRecentBlock, + ScrollPosition::FixedAtPosition { + scroll_lines: ScrollLines::ScrollTop(2.0.into_lines()), + }, + ] { + view.scroll_position = ScrollState::new(position); + view.handle_cli_subagent_controller_event( + view.cli_subagent_controller.clone(), + &CLISubagentEvent::UpdatedControl { + block_id: block_id.clone(), + requested_command_action_id: None, + agent_has_control: false, + }, + ctx, + ); + assert_eq!(view.scroll_position(), position); + } + }); + }); +}