From 44b9011611e1f4213ef34bd51b33476475803a94 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Sun, 13 Sep 2026 18:07:40 +0000 Subject: [PATCH] Preserve terminal scrollback when growing the TUI viewport (#45271) ## Why Scrolling with `CSI S` can discard history rows in QTermWidget and xterm.js when the viewport grows. ## What changed Use newlines at the bottom of the history scroll region for `ScrollbackStrategy::Standard`, preserving the composer and restoring the cursor and full-screen scroll region afterward. Fall back to full-screen scrolling when only one history row exists, since `DECSTBM` requires two distinct rows. Skip scrolling when there are no history rows or no rows to scroll. ## Testing Add regression tests for composer and cursor preservation, scroll-region restoration, emitted newline sequences, single-row scrollback preservation, and no-op growth. Snapshot visible geometry for growth by one, three, and four rows. GitOrigin-RevId: 1dbfc30bb6997dcc2738ec6b2f42d35c786124de --- codex-rs/tui/src/tui/scrollback.rs | 28 ++- codex-rs/tui/src/tui/scrollback_tests.rs | 160 ++++++++++++++++++ ...ck__tests__standard_viewport_growth_1.snap | 10 ++ ...ck__tests__standard_viewport_growth_3.snap | 10 ++ ...ck__tests__standard_viewport_growth_4.snap | 10 ++ 5 files changed, 216 insertions(+), 2 deletions(-) create mode 100644 codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_1.snap create mode 100644 codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_3.snap create mode 100644 codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_4.snap diff --git a/codex-rs/tui/src/tui/scrollback.rs b/codex-rs/tui/src/tui/scrollback.rs index cd2654870d..b0ee6c5c7a 100644 --- a/codex-rs/tui/src/tui/scrollback.rs +++ b/codex-rs/tui/src/tui/scrollback.rs @@ -3,6 +3,8 @@ use crate::custom_terminal::Terminal; use crate::insert_history::HistoryLineWrapPolicy; use crate::insert_history::InsertHistoryMode; +use crate::insert_history::ResetScrollRegion; +use crate::insert_history::SetScrollRegion; use codex_terminal_detection::TerminalInfo; use codex_terminal_detection::TerminalName; use crossterm::cursor::MoveTo; @@ -57,10 +59,32 @@ impl ScrollbackStrategy { where B: Backend + Write, { + if scroll_by == 0 || viewport_top == 0 { + return Ok(()); + } + match self { - Self::FullScreen => { + Self::Standard if viewport_top > 1 => { + // CSI S can discard departing rows in QTermWidget and xterm.js. Newlines at the + // bottom of the history region preserve native scrollback while protecting the + // composer. Restore the cursor without changing the renderer's cached position. + let cursor = terminal.last_known_cursor_pos; + let writer = terminal.backend_mut(); + queue!( + writer, + SetScrollRegion(1..viewport_top), + MoveTo(/*x*/ 0, viewport_top - 1) + )?; + for _ in 0..scroll_by { + queue!(writer, Print("\r\n"))?; + } + queue!(writer, ResetScrollRegion, MoveTo(cursor.x, cursor.y))?; + Ok(()) + } + Self::FullScreen | Self::Standard => { // Partial DEC scroll regions can discard rows instead of moving them into Windows // Terminal's scrollback. Clear the stale composer, then scroll the entire screen. + // Also use this path for a single history row: DECSTBM requires two distinct rows. terminal.clear_after_position(Position::new(/*x*/ 0, viewport_top))?; let writer = terminal.backend_mut(); queue!( @@ -72,7 +96,7 @@ impl ScrollbackStrategy { } Ok(()) } - Self::Standard | Self::Zellij => terminal + Self::Zellij => terminal .backend_mut() .scroll_region_up(0..viewport_top, scroll_by), } diff --git a/codex-rs/tui/src/tui/scrollback_tests.rs b/codex-rs/tui/src/tui/scrollback_tests.rs index e55bbd7a60..33fdeb7052 100644 --- a/codex-rs/tui/src/tui/scrollback_tests.rs +++ b/codex-rs/tui/src/tui/scrollback_tests.rs @@ -11,6 +11,8 @@ use crossterm::cursor::MoveTo; use crossterm::queue; use crossterm::style::Print; use pretty_assertions::assert_eq; +use ratatui::backend::CrosstermBackend; +use ratatui::layout::Position; use ratatui::layout::Rect; use ratatui::layout::Size; use ratatui::text::Line; @@ -167,3 +169,161 @@ fn full_screen_viewport_growth_preserves_terminal_scrollback() { history-row-4 "); } + +#[test] +fn standard_viewport_growth_preserves_composer_and_cursor() { + for scroll_by in [1, 3, 4] { + let width = 24; + let height = 6; + let backend = VT100Backend::with_scrollback(width, height, /*scrollback_len*/ 32); + let mut terminal = Terminal::with_options(backend).expect("terminal with scrollback"); + let viewport_top = 4; + terminal.set_viewport_area(Rect::new( + /*x*/ 0, + viewport_top, + width, + /*height*/ 2, + )); + for (row, text) in [ + "history-1", + "history-2", + "history-3", + "history-4", + "composer-1", + "composer-2", + ] + .into_iter() + .enumerate() + { + queue!( + terminal.backend_mut(), + MoveTo(/*x*/ 0, row as u16), + Print(text) + ) + .expect("seed terminal row"); + } + let cursor = Position::new(/*x*/ 7, /*y*/ 5); + terminal + .set_cursor_position(cursor) + .expect("position cursor"); + + ScrollbackStrategy::Standard + .grow_viewport( + &mut terminal, + viewport_top, + Size::new(width, height), + scroll_by, + ) + .expect("grow standard viewport"); + + let screen = terminal.backend().vt100().screen(); + assert_eq!( + (screen.cursor_position(), terminal.last_known_cursor_pos), + ((cursor.y, cursor.x), cursor) + ); + // vt100 intentionally omits scrollback for partial regions, even for newlines. Snapshot + // visible geometry here; the wire test below and real-terminal smoke cover scrollback. + insta::assert_snapshot!( + format!("standard_viewport_growth_{scroll_by}"), + screen.contents() + ); + + // A newline at the physical bottom must scroll the full screen after the region reset. + queue!( + terminal.backend_mut(), + MoveTo(/*x*/ 0, height - 1), + Print("\r\nnext-row") + ) + .expect("write after restoring the scroll region"); + assert_eq!( + terminal + .backend() + .vt100() + .screen() + .rows(/*start*/ 0, width) + .collect::>()[4..], + ["composer-2", "next-row"] + ); + } +} + +#[test] +fn standard_viewport_growth_uses_scrollback_preserving_newlines() { + for scroll_by in [1, 3, 4] { + let screen_size = Size::new(/*width*/ 24, /*height*/ 6); + let cursor = Position::new(/*x*/ 7, /*y*/ 5); + let backend = CrosstermBackend::new(Vec::::new()); + let mut terminal = + Terminal::with_screen_size_and_cursor_position_for_test(backend, screen_size, cursor); + ScrollbackStrategy::Standard + .grow_viewport( + &mut terminal, + /*viewport_top*/ 4, + screen_size, + scroll_by, + ) + .expect("grow standard viewport"); + assert_eq!( + terminal.backend().writer(), + format!( + "\x1b[1;4r\x1b[4;1H{}\x1b[r\x1b[6;8H", + "\r\n".repeat(usize::from(scroll_by)) + ) + .as_bytes() + ); + } +} + +#[test] +fn standard_viewport_growth_with_one_history_row_preserves_scrollback() { + let width = 24; + let height = 3; + let backend = VT100Backend::with_scrollback(width, height, /*scrollback_len*/ 32); + let mut terminal = Terminal::with_options(backend).expect("terminal with scrollback"); + queue!( + terminal.backend_mut(), + Print("history\r\nstale-composer\r\nstale-footer") + ) + .expect("seed terminal"); + ScrollbackStrategy::Standard + .grow_viewport( + &mut terminal, + /*viewport_top*/ 1, + Size::new(width, height), + /*scroll_by*/ 1, + ) + .expect("grow past the single history row"); + let screen = terminal.backend().vt100().screen(); + let mut history = screen.clone(); + history.set_scrollback(/*rows*/ usize::MAX); + insta::assert_snapshot!(format!("SCROLLBACK:\n{}\nVISIBLE:\n{}", history.contents(), screen.contents()), @r" + SCROLLBACK: + history + VISIBLE: + "); +} + +#[test] +fn standard_viewport_growth_without_rows_is_cursor_neutral() { + for (viewport_top, scroll_by) in [(4, 0), (0, 1)] { + let backend = VT100Backend::new(/*width*/ 24, /*height*/ 6); + let mut terminal = Terminal::with_options(backend).expect("terminal"); + let cursor = Position::new(/*x*/ 7, /*y*/ 5); + terminal + .set_cursor_position(cursor) + .expect("position cursor"); + let before = terminal.backend().vt100().screen().state_formatted(); + ScrollbackStrategy::Standard + .grow_viewport( + &mut terminal, + viewport_top, + Size::new(/*width*/ 24, /*height*/ 6), + scroll_by, + ) + .expect("no-op viewport growth"); + assert_eq!( + terminal.backend().vt100().screen().state_formatted(), + before + ); + } +} diff --git a/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_1.snap b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_1.snap new file mode 100644 index 0000000000..e2675bf767 --- /dev/null +++ b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_1.snap @@ -0,0 +1,10 @@ +--- +source: tui/src/tui/scrollback_tests.rs +expression: screen.contents() +--- +history-2 +history-3 +history-4 + +composer-1 +composer-2 diff --git a/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_3.snap b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_3.snap new file mode 100644 index 0000000000..2b4a26782c --- /dev/null +++ b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_3.snap @@ -0,0 +1,10 @@ +--- +source: tui/src/tui/scrollback_tests.rs +expression: screen.contents() +--- +history-4 + + + +composer-1 +composer-2 diff --git a/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_4.snap b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_4.snap new file mode 100644 index 0000000000..3f85e02e23 --- /dev/null +++ b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__scrollback__tests__standard_viewport_growth_4.snap @@ -0,0 +1,10 @@ +--- +source: tui/src/tui/scrollback_tests.rs +expression: screen.contents() +--- + + + + +composer-1 +composer-2