From 49d046e4cb5b26d612b12029b1307063b87912bf Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Tue, 5 May 2026 17:44:09 -0300 Subject: [PATCH] fix(tui): repair streaming markdown table scrollback --- codex-rs/tui/src/app/event_dispatch.rs | 26 ++++++-- codex-rs/tui/src/app/resize_reflow.rs | 11 +++- codex-rs/tui/src/insert_history.rs | 35 +++++----- codex-rs/tui/src/transcript_reflow.rs | 33 ++++++++-- codex-rs/tui/src/tui.rs | 89 +++++++++++++++++++++++--- 5 files changed, 151 insertions(+), 43 deletions(-) diff --git a/codex-rs/tui/src/app/event_dispatch.rs b/codex-rs/tui/src/app/event_dispatch.rs index aefab7a1b3..5d0a06c8aa 100644 --- a/codex-rs/tui/src/app/event_dispatch.rs +++ b/codex-rs/tui/src/app/event_dispatch.rs @@ -218,9 +218,10 @@ impl App { let end = self.transcript_cells.len(); let start = trailing_run_start::(&self.transcript_cells); - if start < end { - let consolidated: Arc = - Arc::new(history_cell::AgentMarkdownCell::new(source, &cwd)); + let replaced_stream_cells = start < end; + let consolidated: Arc = + Arc::new(history_cell::AgentMarkdownCell::new(source, &cwd)); + if replaced_stream_cells { self.transcript_cells .splice(start..end, std::iter::once(consolidated.clone())); @@ -228,11 +229,20 @@ impl App { t.consolidate_cells(start..end, consolidated.clone()); tui.frame_requester().schedule_frame(); } - - self.maybe_finish_stream_reflow(tui)?; } else { - self.maybe_finish_stream_reflow(tui)?; + self.transcript_cells.push(consolidated.clone()); + if let Some(Overlay::Transcript(t)) = &mut self.overlay { + t.insert_cell(consolidated.clone()); + tui.frame_requester().schedule_frame(); + } + self.insert_history_cell_lines( + tui, + consolidated.as_ref(), + tui.terminal.last_known_screen_size.width, + ); } + let force_source_backed_reflow = tui.uses_newline_history_insert(); + self.maybe_finish_stream_reflow(tui, force_source_backed_reflow)?; } AppEvent::ConsolidateProposedPlan(source) => { if !self.terminal_resize_reflow_enabled() { @@ -268,7 +278,9 @@ impl App { tui.terminal.last_known_screen_size.width, ); - self.maybe_finish_stream_reflow(tui)?; + self.maybe_finish_stream_reflow( + tui, /*force_source_backed_reflow*/ false, + )?; } } AppEvent::ApplyThreadRollback { num_turns } => { diff --git a/codex-rs/tui/src/app/resize_reflow.rs b/codex-rs/tui/src/app/resize_reflow.rs index 58b2e21dff..be94ccec0b 100644 --- a/codex-rs/tui/src/app/resize_reflow.rs +++ b/codex-rs/tui/src/app/resize_reflow.rs @@ -239,13 +239,20 @@ impl App { /// was active or while its transient cells were still present, this method runs an immediate /// source-backed reflow so terminal scrollback reflects the finalized cell instead of the /// transient stream rows. - pub(super) fn maybe_finish_stream_reflow(&mut self, tui: &mut tui::Tui) -> Result<()> { + pub(super) fn maybe_finish_stream_reflow( + &mut self, + tui: &mut tui::Tui, + force_source_backed_reflow: bool, + ) -> Result<()> { if !self.terminal_resize_reflow_enabled() { self.transcript_reflow.clear(); return Ok(()); } - if self.transcript_reflow.take_stream_finish_reflow_needed() { + if self + .transcript_reflow + .take_stream_finish_reflow_needed(force_source_backed_reflow) + { self.schedule_immediate_resize_reflow(tui); self.maybe_run_resize_reflow(tui)?; } else if self.transcript_reflow.pending_is_due(Instant::now()) { diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index 4f3ea981bd..76a7fe7cc8 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -38,19 +38,20 @@ use ratatui::text::Span; /// Selects the terminal escape strategy for inserting history lines above the viewport. /// /// Standard terminals support `DECSTBM` scroll regions and Reverse Index (`ESC M`), -/// which let us slide existing content down without redrawing it. Zellij silently -/// drops or mishandles those sequences, so `Zellij` mode falls back to emitting -/// newlines at the bottom of the screen and writing lines at absolute positions. +/// which let us slide existing content down without redrawing it. Some terminals +/// or terminal-like surfaces mishandle those sequences for normal scrollback, so +/// `Newline` mode falls back to emitting newlines at the bottom of the screen +/// and writing lines at absolute positions. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub enum InsertHistoryMode { Standard, - Zellij, + Newline, } impl InsertHistoryMode { - pub fn new(is_zellij: bool) -> Self { - if is_zellij { - Self::Zellij + pub fn new(use_newline_insert: bool) -> Self { + if use_newline_insert { + Self::Newline } else { Self::Standard } @@ -72,9 +73,9 @@ where /// Insert `lines` above the viewport, using the escape strategy selected by `mode`. /// /// In `Standard` mode this manipulates DECSTBM scroll regions to slide existing -/// scrollback down and writes new lines into the freed space. In `Zellij` mode it -/// emits newlines at the screen bottom to create space (since Zellij ignores scroll -/// region escapes) and writes lines at computed absolute positions. Both modes +/// scrollback down and writes new lines into the freed space. In `Newline` mode it +/// emits newlines at the screen bottom to create space and writes lines at computed +/// absolute positions. Both modes /// update `terminal.viewport_area` so subsequent draw passes know where the /// viewport moved to. Resize reflow uses the same viewport-aware path after /// clearing old scrollback. @@ -124,7 +125,7 @@ where let wrapped_lines = wrapped_rows as u16; match mode { - InsertHistoryMode::Zellij => { + InsertHistoryMode::Newline => { let space_below = screen_size.height.saturating_sub(area.bottom()); let shift_down = wrapped_lines.min(space_below); let scroll_up_amount = wrapped_lines.saturating_sub(shift_down); @@ -814,7 +815,7 @@ mod tests { } #[test] - fn vt100_zellij_mode_inserts_history_and_updates_viewport() { + fn vt100_newline_mode_inserts_history_and_updates_viewport() { let width: u16 = 32; let height: u16 = 8; let backend = VT100Backend::new(width, height); @@ -822,9 +823,9 @@ mod tests { let viewport = Rect::new(/*x*/ 0, /*y*/ 4, width, /*height*/ 2); term.set_viewport_area(viewport); - let line: Line<'static> = Line::from("zellij history"); - insert_history_lines_with_mode(&mut term, vec![line], InsertHistoryMode::Zellij) - .expect("insert zellij history"); + let line: Line<'static> = Line::from("newline history"); + insert_history_lines_with_mode(&mut term, vec![line], InsertHistoryMode::Newline) + .expect("insert newline history"); let start_row = 0; let rows: Vec = term @@ -834,8 +835,8 @@ mod tests { .rows(start_row, width) .collect(); assert!( - rows.iter().any(|row| row.contains("zellij history")), - "expected zellij history row in screen output, rows: {rows:?}" + rows.iter().any(|row| row.contains("newline history")), + "expected newline history row in screen output, rows: {rows:?}" ); assert_eq!(term.viewport_area, Rect::new(0, 5, width, 2)); assert_eq!(term.visible_history_rows(), 1); diff --git a/codex-rs/tui/src/transcript_reflow.rs b/codex-rs/tui/src/transcript_reflow.rs index 33318a4d0f..c761588e4d 100644 --- a/codex-rs/tui/src/transcript_reflow.rs +++ b/codex-rs/tui/src/transcript_reflow.rs @@ -148,8 +148,13 @@ impl TranscriptReflowState { /// This is a draining read because each resize-during-stream episode should force at most one /// post-consolidation repair. Calling it before consolidation would drop the repair request and /// leave finalized scrollback shaped by transient stream rows. - pub(crate) fn take_stream_finish_reflow_needed(&mut self) -> bool { - let needed = self.ran_during_stream || self.resize_requested_during_stream; + pub(crate) fn take_stream_finish_reflow_needed( + &mut self, + force_source_backed_reflow: bool, + ) -> bool { + let needed = self.ran_during_stream + || self.resize_requested_during_stream + || force_source_backed_reflow; self.ran_during_stream = false; self.resize_requested_during_stream = false; needed @@ -276,8 +281,8 @@ mod tests { let mut state = TranscriptReflowState::default(); state.mark_resize_requested_during_stream(); - assert!(state.take_stream_finish_reflow_needed()); - assert!(!state.take_stream_finish_reflow_needed()); + assert!(state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ false)); + assert!(!state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ false)); } #[test] @@ -285,8 +290,22 @@ mod tests { let mut state = TranscriptReflowState::default(); state.mark_ran_during_stream(); - assert!(state.take_stream_finish_reflow_needed()); - assert!(!state.take_stream_finish_reflow_needed()); + assert!(state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ false)); + assert!(!state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ false)); + } + + #[test] + fn take_stream_finish_reflow_needed_reflows_when_forced() { + let mut state = TranscriptReflowState::default(); + + assert!(state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ true)); + } + + #[test] + fn take_stream_finish_reflow_needed_ignores_unforced_finalization() { + let mut state = TranscriptReflowState::default(); + + assert!(!state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ false)); } #[test] @@ -297,6 +316,6 @@ mod tests { state.clear(); - assert!(!state.take_stream_finish_reflow_needed()); + assert!(!state.take_stream_finish_reflow_needed(/*force_source_backed_reflow*/ false)); } } diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index d7f14c8a3a..ec69c74407 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -48,6 +48,9 @@ use crate::tui::event_stream::TuiEventStream; use crate::tui::job_control::SuspendContext; use codex_config::types::NotificationCondition; use codex_config::types::NotificationMethod; +use codex_terminal_detection::Multiplexer; +use codex_terminal_detection::TerminalInfo; +use codex_terminal_detection::TerminalName; mod event_stream; mod frame_rate_limiter; @@ -73,10 +76,38 @@ fn should_emit_notification(condition: NotificationCondition, terminal_focused: } } +fn insert_history_mode_for_terminal( + terminal_info: &TerminalInfo, +) -> crate::insert_history::InsertHistoryMode { + let use_newline_insert = matches!(terminal_info.multiplexer, Some(Multiplexer::Zellij {})) + || matches!( + terminal_info.name, + TerminalName::GnomeTerminal + | TerminalName::Vte + | TerminalName::VsCode + | TerminalName::WindowsTerminal + ); + crate::insert_history::InsertHistoryMode::new(use_newline_insert) +} + #[cfg(test)] mod tests { + use super::insert_history_mode_for_terminal; use super::should_emit_notification; use codex_config::types::NotificationCondition; + use codex_terminal_detection::Multiplexer; + use codex_terminal_detection::TerminalInfo; + use codex_terminal_detection::TerminalName; + + fn terminal_info(name: TerminalName, multiplexer: Option) -> TerminalInfo { + TerminalInfo { + name, + term_program: None, + version: None, + term: None, + multiplexer, + } + } #[test] fn unfocused_notification_condition_is_suppressed_when_focused() { @@ -101,6 +132,39 @@ mod tests { /*terminal_focused*/ false )); } + + #[test] + fn newline_history_terminals_use_newline_history_insert() { + for name in [ + TerminalName::GnomeTerminal, + TerminalName::Vte, + TerminalName::VsCode, + TerminalName::WindowsTerminal, + ] { + assert_eq!( + insert_history_mode_for_terminal(&terminal_info(name, /*multiplexer*/ None)), + crate::insert_history::InsertHistoryMode::Newline + ); + } + assert_eq!( + insert_history_mode_for_terminal(&terminal_info( + TerminalName::Unknown, + Some(Multiplexer::Zellij {}) + )), + crate::insert_history::InsertHistoryMode::Newline + ); + } + + #[test] + fn regular_terminals_keep_standard_history_insert() { + assert_eq!( + insert_history_mode_for_terminal(&terminal_info( + TerminalName::Iterm2, + /*multiplexer*/ None + )), + crate::insert_history::InsertHistoryMode::Standard + ); + } } pub fn set_modes() -> Result<()> { @@ -381,6 +445,7 @@ pub struct Tui { notification_backend: Option, notification_condition: NotificationCondition, is_zellij: bool, + history_insert_mode: crate::insert_history::InsertHistoryMode, // When false, enter_alt_screen() becomes a no-op (for Zellij scrollback support) alt_screen_enabled: bool, } @@ -397,10 +462,9 @@ impl Tui { // Cache this to avoid contention with the event reader. supports_color::on_cached(supports_color::Stream::Stdout); let _ = crate::terminal_palette::default_colors(); - let is_zellij = matches!( - codex_terminal_detection::terminal_info().multiplexer, - Some(codex_terminal_detection::Multiplexer::Zellij {}) - ); + let terminal_info = codex_terminal_detection::terminal_info(); + let is_zellij = matches!(terminal_info.multiplexer, Some(Multiplexer::Zellij {})); + let history_insert_mode = insert_history_mode_for_terminal(&terminal_info); Self { frame_requester, @@ -417,10 +481,15 @@ impl Tui { notification_backend: Some(detect_backend(NotificationMethod::default())), notification_condition: NotificationCondition::default(), is_zellij, + history_insert_mode, alt_screen_enabled: true, } } + pub(crate) fn uses_newline_history_insert(&self) -> bool { + self.history_insert_mode == crate::insert_history::InsertHistoryMode::Newline + } + /// Set whether alternate screen is enabled. When false, enter_alt_screen() becomes a no-op. pub fn set_alt_screen_enabled(&mut self, enabled: bool) { self.alt_screen_enabled = enabled; @@ -694,12 +763,12 @@ impl Tui { } /// Write any buffered history lines above the viewport and clear the buffer. - /// Returns `true` when Zellij mode was used, signaling that the caller must + /// Returns `true` when newline mode was used, signaling that the caller must /// invalidate the diff buffer for a full repaint. fn flush_pending_history_lines( terminal: &mut Terminal, pending_history_lines: &mut Vec>, - is_zellij: bool, + history_insert_mode: crate::insert_history::InsertHistoryMode, ) -> Result { if pending_history_lines.is_empty() { return Ok(false); @@ -708,10 +777,10 @@ impl Tui { crate::insert_history::insert_history_lines_with_mode( terminal, pending_history_lines.clone(), - crate::insert_history::InsertHistoryMode::new(is_zellij), + history_insert_mode, )?; pending_history_lines.clear(); - Ok(is_zellij) + Ok(history_insert_mode == crate::insert_history::InsertHistoryMode::Newline) } pub fn draw( @@ -747,7 +816,7 @@ impl Tui { needs_full_repaint |= Self::flush_pending_history_lines( terminal, &mut self.pending_history_lines, - self.is_zellij, + self.history_insert_mode, )?; if needs_full_repaint { @@ -803,7 +872,7 @@ impl Tui { let flushed_history = Self::flush_pending_history_lines( terminal, &mut self.pending_history_lines, - self.is_zellij, + self.history_insert_mode, )?; needs_full_repaint |= flushed_history;