From ea1b6ad1cf2c39dacf10bf1f2667d9c6f7e7eb3f Mon Sep 17 00:00:00 2001 From: dob-oai Date: Thu, 12 Nov 2026 21:29:40 +0000 Subject: [PATCH] Clear inline image previews before history insertion can scroll them (#61196) ## Why Inline image previews can leave stale pixels in terminal history when streamed output scrolls their rows. ## What changed Clear retained previews before inserting history that could move them, then let the next frame redraw them while preserving the draft text. Keep direct Kitty previews when standard history insertion scrolls only above an unchanged, bottom-aligned viewport. ## Testing Add regression coverage for clearing previews before scrolling, preserving unsent draft text, redrawing file-backed previews, and retaining or repainting direct Kitty previews as appropriate. GitOrigin-RevId: 683d1313349b1ac707499b021092fc390e21530d --- codex-rs/tui/src/tui.rs | 47 +++- codex-rs/tui/src/tui/history_tail.rs | 1 + .../tui/src/tui/inline_image_history_tests.rs | 241 ++++++++++++++++++ ..._before_scrolling_and_preserves_draft.snap | 12 + 4 files changed, 289 insertions(+), 12 deletions(-) create mode 100644 codex-rs/tui/src/tui/inline_image_history_tests.rs create mode 100644 codex-rs/tui/src/tui/snapshots/codex_tui__tui__inline_image_history_tests__inline_history_erases_preview_before_scrolling_and_preserves_draft.snap diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index d5b1ac0b9d..e023ff5f4e 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -48,6 +48,7 @@ use self::input_boundary::terminal_input_is_readable; use crate::custom_terminal; use crate::custom_terminal::Terminal as CustomTerminal; use crate::insert_history::HistoryLineWrapPolicy; +use crate::insert_history::InsertHistoryMode; use crate::notifications::DesktopNotificationBackend; use crate::notifications::detect_backend; use crate::terminal_hyperlinks::HyperlinkLine; @@ -73,6 +74,9 @@ mod job_control; mod keyboard_modes; mod link_pointer; pub(crate) use link_pointer::PointerShape; +#[cfg(test)] +#[path = "tui/inline_image_history_tests.rs"] +mod inline_image_history_tests; mod notification_preview; #[cfg(test)] #[path = "tui/owned_screen_tests.rs"] @@ -1143,6 +1147,7 @@ impl Tui { Self::flush_pending_history_lines( &mut self.terminal, &mut self.pending_history_lines, + &mut self.retained_images, self.scrollback, screen_size, )?; @@ -1325,22 +1330,38 @@ impl Tui { } /// Write any buffered history lines above the viewport and clear the buffer. - fn flush_pending_history_lines( - terminal: &mut Terminal, + fn flush_pending_history_lines( + terminal: &mut CustomTerminal, pending_history_lines: &mut Vec, + retained_images: &mut Vec, scrollback: ScrollbackStrategy, screen_size: Size, - ) -> Result<()> { - if pending_history_lines.is_empty() { - return Ok(()); - } - - // A failed write can already have emitted part of this batch. Never retry that batch - // automatically: replaying it would duplicate the successful prefix. Remaining batches - // stay queued, and the caller reports the I/O error. - while !pending_history_lines.is_empty() { - let batch = pending_history_lines.remove(/*index*/ 0); + ) -> Result<()> + where + B: Backend + Write, + { + while let Some(batch) = pending_history_lines.first() { let mode = scrollback.history_insertion_mode(batch.wrap_policy); + let viewport = terminal.viewport_area; + // Standard insertion scrolls only above a bottom-aligned viewport when there + // are at least two history rows (the minimum for a DEC scroll region). + let preserves_direct_kitty = mode == InsertHistoryMode::Standard + && viewport.top() > 1 + && viewport.bottom() == screen_size.height + && screen_size == terminal.last_known_screen_size + && terminal.tmux_kitty_image_base.is_none(); + if retained_images.iter().any(|placement| { + !preserves_direct_kitty + || placement.image.protocol != crate::pets::ImageProtocol::Kitty + || placement.area.intersection(viewport) != placement.area + }) { + // iTerm2's file-backed images and Sixel pixels can scroll with history. + terminal.clear_frame_images(retained_images)?; + } + + // Once insertion starts, an I/O error can leave a prefix on screen. Dequeue + // this batch first so retrying the remaining batches cannot duplicate that prefix. + let batch = pending_history_lines.remove(/*index*/ 0); crate::insert_history::insert_history_hyperlink_lines_with_mode_and_wrap_policy( terminal, &batch.lines, @@ -1469,6 +1490,7 @@ impl Tui { Self::flush_pending_history_lines( terminal, &mut self.pending_history_lines, + &mut self.retained_images, self.scrollback, screen_size, )?; @@ -1607,6 +1629,7 @@ impl Tui { Self::flush_pending_history_lines( terminal, &mut self.pending_history_lines, + &mut self.retained_images, self.scrollback, screen_size, )?; diff --git a/codex-rs/tui/src/tui/history_tail.rs b/codex-rs/tui/src/tui/history_tail.rs index f9a2c9b862..1ccc6c822d 100644 --- a/codex-rs/tui/src/tui/history_tail.rs +++ b/codex-rs/tui/src/tui/history_tail.rs @@ -32,6 +32,7 @@ impl Tui { Self::flush_pending_history_lines( &mut self.terminal, &mut self.pending_history_lines, + &mut self.retained_images, self.scrollback, screen_size, )?; diff --git a/codex-rs/tui/src/tui/inline_image_history_tests.rs b/codex-rs/tui/src/tui/inline_image_history_tests.rs new file mode 100644 index 0000000000..9caaf5b7ab --- /dev/null +++ b/codex-rs/tui/src/tui/inline_image_history_tests.rs @@ -0,0 +1,241 @@ +//! Inline previews must be erased before their rows can enter terminal history. +#![cfg(test)] + +use super::PendingHistoryLines; +use super::ScrollbackStrategy; +use super::Tui; +use crate::custom_terminal::Terminal; +use crate::insert_history::HistoryLineWrapPolicy; +use crate::pets::ImageProtocol; +use crate::terminal_hyperlinks::plain_hyperlink_lines; +use crate::transcript_image::ImagePlacement; +use crate::transcript_image::TranscriptImage; +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; +use ratatui::widgets::Widget; +use std::sync::Arc; + +fn inline_terminal(viewport: Rect) -> Terminal>> { + let mut terminal = Terminal::with_screen_size_and_cursor_position_for_test( + CrosstermBackend::new(Vec::new()), + Size::new(/*width*/ 32, /*height*/ 8), + Position::new(/*x*/ 0, /*y*/ 0), + ); + terminal.set_viewport_area(viewport); + terminal +} + +fn preview_at(protocol: ImageProtocol, y: u16) -> ImagePlacement { + ImagePlacement { + image: Arc::new(TranscriptImage { + protocol, + columns: 2, + rows: 1, + pixels: image::RgbaImage::from_pixel( + /*width*/ 16, + /*height*/ 16, + image::Rgba([0, 100, 200, 255]), + ), + cell_width: 8, + cell_height: 16, + }), + area: Rect::new(/*x*/ 0, y, /*width*/ 2, /*height*/ 1), + first_row: 0, + file: None, + } +} + +#[test] +fn inline_history_erases_preview_before_scrolling_and_preserves_draft() { + let viewport = Rect::new( + /*x*/ 0, /*y*/ 4, /*width*/ 32, /*height*/ 4, + ); + let mut terminal = inline_terminal(viewport); + let size = terminal.last_known_screen_size; + terminal + .draw_with_size(size, &mut Vec::new(), |frame| { + Line::from("unsent [Image #1]").render( + Rect::new( + /*x*/ 0, /*y*/ 7, /*width*/ 32, /*height*/ 1, + ), + frame.buffer_mut(), + ); + }) + .unwrap(); + // vt100 ignores Sixel. Model its out-of-band pixels with a marker written directly + // to the terminal, without changing the renderer's text buffers. + queue!( + terminal.backend_mut(), + MoveTo(/*x*/ 0, /*y*/ 4), + Print("stale preview pixels") + ) + .unwrap(); + let mut retained = vec![preview_at(ImageProtocol::Sixel, viewport.top())]; + // Unrelated frames must not evict an otherwise unchanged preview. + Tui::flush_pending_history_lines( + &mut terminal, + &mut Vec::new(), + &mut retained, + ScrollbackStrategy::Standard, + size, + ) + .unwrap(); + assert_eq!(retained.len(), 1); + let preview = retained[0].registration(); + let before_flush = terminal.backend().writer().len(); + let mut history = vec![PendingHistoryLines { + lines: plain_hyperlink_lines( + (1..=12) + .map(|index| Line::from(format!("streamed response {index}"))) + .collect(), + ), + wrap_policy: HistoryLineWrapPolicy::PreWrap, + }]; + Tui::flush_pending_history_lines( + &mut terminal, + &mut history, + &mut retained, + ScrollbackStrategy::Standard, + size, + ) + .unwrap(); + assert!(retained.is_empty()); + assert!(history.is_empty()); + let output = terminal.backend().writer(); + let flush = String::from_utf8_lossy(&output[before_flush..]); + let erase = flush.find("\x1b[K").expect("erase preview row"); + let history_write = flush.find("\x1b[1;4r").expect("history scroll region"); + assert!( + erase < history_write, + "preview must be erased before history: {flush:?}" + ); + let mut parser = vt100::Parser::new( + /*rows*/ 8, /*cols*/ 32, /*scrollback_len*/ 32, + ); + parser.process(output); + let contents = parser.screen().contents(); + assert!(!contents.contains("stale preview"), "{contents}"); + assert!(contents.contains("streamed response"), "{contents}"); + assert!(contents.contains("unsent [Image #1]"), "{contents}"); + insta::assert_snapshot!(contents); + terminal + .draw_with_size(size, &mut retained, |frame| { + Line::from("unsent [Image #1]").render( + Rect::new( + /*x*/ 0, /*y*/ 7, /*width*/ 32, /*height*/ 1, + ), + frame.buffer_mut(), + ); + frame.image_placements.push(preview); + }) + .unwrap(); + assert_eq!(retained.len(), 1); + assert!(String::from_utf8_lossy(terminal.backend().writer()).contains("\x1bP")); +} + +/// Capture history output and the following frame separately from the initial Kitty upload. +fn kitty_history_output( + protocol: ImageProtocol, + viewport: Rect, + scrollback: ScrollbackStrategy, +) -> (String, String) { + let mut terminal = inline_terminal(viewport); + let size = terminal.last_known_screen_size; + let preview = preview_at(protocol, viewport.top()); + let draw = |frame: &mut crate::custom_terminal::Frame<'_>| { + let mut placement = preview.registration(); + placement.area.y = frame.area().top(); + frame.image_placements.push(placement); + }; + let mut retained = Vec::new(); + terminal.draw_with_size(size, &mut retained, draw).unwrap(); + let output = terminal.backend().writer(); + assert!(String::from_utf8_lossy(output).contains("_Ga=T,")); + let before_flush = output.len(); + let mut history = vec![PendingHistoryLines { + lines: plain_hyperlink_lines(vec![Line::from("streamed response")]), + wrap_policy: HistoryLineWrapPolicy::PreWrap, + }]; + Tui::flush_pending_history_lines(&mut terminal, &mut history, &mut retained, scrollback, size) + .unwrap(); + let before_draw = terminal.backend().writer().len(); + terminal.draw_with_size(size, &mut retained, draw).unwrap(); + let output = terminal.backend().writer(); + ( + String::from_utf8_lossy(&output[before_flush..before_draw]).into_owned(), + String::from_utf8_lossy(&output[before_draw..]).into_owned(), + ) +} + +#[test] +fn direct_kitty_preview_survives_history_above_the_viewport() { + let viewport = Rect::new( + /*x*/ 0, /*y*/ 4, /*width*/ 32, /*height*/ 4, + ); + let (flush, redraw) = + kitty_history_output(ImageProtocol::Kitty, viewport, ScrollbackStrategy::Standard); + assert!(flush.contains("streamed response")); + assert!(!flush.contains("_G"), "{flush:?}"); + assert!(!redraw.contains("_G"), "{redraw:?}"); +} + +#[test] +fn iterm2_file_preview_is_deleted_before_history_and_uploaded_on_the_next_draw() { + let viewport = Rect::new( + /*x*/ 0, /*y*/ 4, /*width*/ 32, /*height*/ 4, + ); + let (flush, redraw) = kitty_history_output( + ImageProtocol::KittyLocalFile, + viewport, + ScrollbackStrategy::Standard, + ); + let (before_history, _) = flush + .split_once("\x1b[1;4r") + .expect("history scroll region"); + let delete = crate::pets::kitty_delete_image(crate::transcript_image::FIRST_IMAGE_ID); + assert!(before_history.contains(&delete), "{flush:?}"); + assert!(!flush.contains("_Ga=T,"), "{flush:?}"); + assert!(redraw.contains("_Ga=T,t=f,"), "{redraw:?}"); +} + +#[test] +fn direct_kitty_preview_is_repainted_when_history_can_move_the_viewport() { + for (viewport, scrollback) in [ + ( + Rect::new( + /*x*/ 0, /*y*/ 2, /*width*/ 32, /*height*/ 4, + ), + ScrollbackStrategy::Standard, + ), + ( + Rect::new( + /*x*/ 0, /*y*/ 1, /*width*/ 32, /*height*/ 7, + ), + ScrollbackStrategy::Standard, + ), + ( + Rect::new( + /*x*/ 0, /*y*/ 4, /*width*/ 32, /*height*/ 4, + ), + ScrollbackStrategy::FullScreen, + ), + ] { + let (flush, redraw) = kitty_history_output(ImageProtocol::Kitty, viewport, scrollback); + let (before_history, _) = flush.split_once("streamed response").expect("history line"); + assert!( + before_history.contains("_Ga=d,"), + "{viewport:?} {scrollback:?}: {flush:?}" + ); + assert!( + redraw.contains("_Ga=T,t=d,"), + "{viewport:?} {scrollback:?}: {redraw:?}" + ); + } +} diff --git a/codex-rs/tui/src/tui/snapshots/codex_tui__tui__inline_image_history_tests__inline_history_erases_preview_before_scrolling_and_preserves_draft.snap b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__inline_image_history_tests__inline_history_erases_preview_before_scrolling_and_preserves_draft.snap new file mode 100644 index 0000000000..ff03f48760 --- /dev/null +++ b/codex-rs/tui/src/tui/snapshots/codex_tui__tui__inline_image_history_tests__inline_history_erases_preview_before_scrolling_and_preserves_draft.snap @@ -0,0 +1,12 @@ +--- +source: tui/src/tui/inline_image_history_tests.rs +expression: contents +--- +streamed response 9 +streamed response 10 +streamed response 11 +streamed response 12 + + + +unsent [Image #1]