From 367370b25eec2df598f7e0141e4eaf7624d23c07 Mon Sep 17 00:00:00 2001 From: Felipe Coury Date: Mon, 6 Apr 2026 15:08:24 -0300 Subject: [PATCH] fix(tui): prevent duplicate table scrollback Defer live table tails during stream consolidation so their provisional render is not inserted before the canonical markdown cell is rebuilt. Use the stronger combined scrollback and visible-screen clear path during transcript reflow so tmux does not preserve stale viewport table output. --- codex-rs/tui/src/app.rs | 77 +++++++++++-- codex-rs/tui/src/app_event.rs | 13 ++- codex-rs/tui/src/chatwidget.rs | 27 ++++- .../src/chatwidget/tests/status_and_layout.rs | 106 ++++++++++++++++++ 4 files changed, 208 insertions(+), 15 deletions(-) diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 056f089407..2ed7fab612 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -2,6 +2,7 @@ use crate::app_backtrack::BacktrackState; use crate::app_command::AppCommand; use crate::app_command::AppCommandView; use crate::app_event::AppEvent; +use crate::app_event::ConsolidationScrollbackReflow; use crate::app_event::ExitMode; use crate::app_event::FeedbackCategory; use crate::app_event::RealtimeAudioDeviceKind; @@ -3350,6 +3351,11 @@ impl App { self.reflow_ran_during_stream = false; } + fn schedule_immediate_resize_reflow(&mut self, tui: &mut tui::Tui) { + self.resize_reflow_pending_until = Some(Instant::now()); + tui.frame_requester().schedule_frame(); + } + fn handle_draw_size_change( &mut self, size: ratatui::layout::Size, @@ -3417,25 +3423,38 @@ impl App { } self.resize_reflow_pending_until = None; - // Drop any queued pre-resize inserts before rebuilding at the new width. + + // Track that a reflow happened during an active stream or while trailing + // unconsolidated AgentMessageCells are still pending consolidation so + // ConsolidateAgentMessage can schedule a follow-up reflow. + let reflow_ran_during_stream = + !self.transcript_cells.is_empty() && self.should_mark_reflow_as_stream_time(); + + self.reflow_transcript_now(tui)?; + + if reflow_ran_during_stream { + self.reflow_ran_during_stream = true; + } + + Ok(()) + } + + fn reflow_transcript_now(&mut self, tui: &mut tui::Tui) -> Result<()> { + // Drop any queued pre-resize/pre-consolidation inserts before rebuilding from cells. tui.clear_pending_history_lines(); if self.transcript_cells.is_empty() { self.reset_history_emission_state(); return Ok(()); } - tui.terminal.clear_scrollback()?; - tui.terminal.clear()?; + if tui.is_alt_screen_active() { + tui.terminal.clear_visible_screen()?; + } else { + tui.terminal.clear_scrollback_and_visible_screen_ansi()?; + } self.reset_history_emission_state(); - // Track that a reflow happened during an active stream or while trailing - // unconsolidated AgentMessageCells are still pending consolidation so - // ConsolidateAgentMessage can schedule a follow-up reflow. - if self.should_mark_reflow_as_stream_time() { - self.reflow_ran_during_stream = true; - } - let width = tui.terminal.size()?.width; // Iterate by index to avoid cloning the Vec and bumping Arc refcounts. for i in 0..self.transcript_cells.len() { @@ -3446,6 +3465,27 @@ impl App { Ok(()) } + fn finish_agent_message_consolidation( + &mut self, + tui: &mut tui::Tui, + scrollback_reflow: ConsolidationScrollbackReflow, + ) -> Result<()> { + match scrollback_reflow { + ConsolidationScrollbackReflow::IfResizeReflowRan => { + self.maybe_finish_stream_reflow(tui); + } + ConsolidationScrollbackReflow::Required => { + self.schedule_immediate_resize_reflow(tui); + self.maybe_run_resize_reflow(tui)?; + if self.resize_reflow_pending_until.is_none() { + self.reflow_ran_during_stream = false; + } + } + } + + Ok(()) + } + fn should_mark_reflow_as_stream_time(&self) -> bool { self.chat_widget.has_active_agent_stream() || self.chat_widget.has_active_plan_stream() @@ -4425,7 +4465,20 @@ impl App { tui.terminal.last_known_screen_size.width, ); } - AppEvent::ConsolidateAgentMessage { source, cwd } => { + AppEvent::ConsolidateAgentMessage { + source, + cwd, + scrollback_reflow, + deferred_history_cell, + } => { + if let Some(cell) = deferred_history_cell { + let cell: Arc = cell.into(); + if let Some(Overlay::Transcript(t)) = &mut self.overlay { + t.insert_cell(cell.clone()); + } + self.transcript_cells.push(cell); + } + // Walk backward to find the contiguous run of streaming AgentMessageCells that // belong to the just-finalized stream let end = self.transcript_cells.len(); @@ -4450,7 +4503,7 @@ impl App { tui.frame_requester().schedule_frame(); } - self.maybe_finish_stream_reflow(tui); + self.finish_agent_message_consolidation(tui, scrollback_reflow)?; } else { tracing::debug!( "ConsolidateAgentMessage: no cells to consolidate(start={start}, end={end})", diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 22573f0770..873a6bec5d 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -62,6 +62,12 @@ impl RealtimeAudioDeviceKind { } } +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum ConsolidationScrollbackReflow { + IfResizeReflowRan, + Required, +} + #[derive(Debug, Clone, Copy, PartialEq, Eq)] #[cfg_attr(not(target_os = "windows"), allow(dead_code))] pub(crate) enum WindowsSandboxEnableMode { @@ -274,10 +280,15 @@ pub(crate) enum AppEvent { /// finalization. The `App` handler walks backward through `transcript_cells` /// to find the `AgentMessageCell` run and splices in the consolidated cell. /// The `cwd` keeps local file-link display stable across the final re-render. - /// If `reflow_ran_during_stream` is set, a follow-up reflow is scheduled. + /// `scrollback_reflow` lets table-tail finalization force the already-emitted + /// terminal scrollback to be rebuilt from the consolidated source-backed cell. + /// `deferred_history_cell` lets callers add the final stream tail to the + /// transcript without first writing its provisional render to scrollback. ConsolidateAgentMessage { source: String, cwd: PathBuf, + scrollback_reflow: ConsolidationScrollbackReflow, + deferred_history_cell: Option>, }, /// Replace the contiguous run of streaming `ProposedPlanStreamCell`s at the diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index dca66badb7..49c65c4adb 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -1737,10 +1737,29 @@ impl ChatWidget { fn flush_answer_stream_with_separator(&mut self) { let had_stream_controller = self.stream_controller.is_some(); if let Some(mut controller) = self.stream_controller.take() { + let scrollback_reflow = if controller.has_live_tail() { + crate::app_event::ConsolidationScrollbackReflow::Required + } else { + crate::app_event::ConsolidationScrollbackReflow::IfResizeReflowRan + }; self.clear_active_stream_tail(); let (cell, source) = controller.finalize(); - if let Some(cell) = cell { - self.add_boxed_history(cell); + let deferred_history_cell = + if scrollback_reflow == crate::app_event::ConsolidationScrollbackReflow::Required { + cell + } else { + if let Some(cell) = cell { + self.add_boxed_history(cell); + } + None + }; + if let Some(cell) = deferred_history_cell.as_ref() { + debug_assert!( + cell.as_any() + .downcast_ref::() + .is_some(), + "only agent message stream tails should be deferred for consolidation", + ); } // Consolidate the run of streaming AgentMessageCells into a single AgentMarkdownCell // that can re-render from source on resize. @@ -1748,7 +1767,11 @@ impl ChatWidget { self.app_event_tx.send(AppEvent::ConsolidateAgentMessage { source, cwd: self.config.cwd.to_path_buf(), + scrollback_reflow, + deferred_history_cell, }); + } else if let Some(cell) = deferred_history_cell { + self.add_boxed_history(cell); } } self.adaptive_chunking.reset(); diff --git a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs index 9b7b45898d..8da8114c1e 100644 --- a/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs +++ b/codex-rs/tui/src/chatwidget/tests/status_and_layout.rs @@ -265,6 +265,112 @@ async fn flush_answer_stream_does_not_stop_animation_while_plan_stream_is_active ); } +#[tokio::test] +async fn flush_answer_stream_requests_scrollback_reflow_for_live_table_tail() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + let cwd = chat.config.cwd.to_path_buf(); + + let mut controller = + crate::streaming::controller::StreamController::new(Some(80), cwd.as_path(), false); + controller.push("| Name | Notes |\n"); + controller.push("| --- | --- |\n"); + controller.push("| alpha | FINAL_DUPLICATE_SENTINEL |\n"); + assert!( + controller.has_live_tail(), + "expected table holdback to leave a live tail for this regression", + ); + chat.stream_controller = Some(controller); + + while rx.try_recv().is_ok() {} + + chat.flush_answer_stream_with_separator(); + + let mut saw_consolidate = false; + let mut saw_insert_history = false; + while let Ok(event) = rx.try_recv() { + match event { + AppEvent::InsertHistoryCell(_) => saw_insert_history = true, + AppEvent::ConsolidateAgentMessage { + scrollback_reflow, + deferred_history_cell, + .. + } => { + saw_consolidate = true; + assert_eq!( + scrollback_reflow, + crate::app_event::ConsolidationScrollbackReflow::Required + ); + assert!( + deferred_history_cell.is_some(), + "live table tail should be staged for consolidation without provisional insert", + ); + } + _ => {} + } + } + + assert!( + saw_consolidate, + "expected stream finalization to consolidate" + ); + assert!( + !saw_insert_history, + "live table tail should not be inserted before canonical reflow" + ); +} + +#[tokio::test] +async fn flush_answer_stream_keeps_default_reflow_for_plain_text_tail() { + let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await; + let cwd = chat.config.cwd.to_path_buf(); + + let mut controller = + crate::streaming::controller::StreamController::new(Some(80), cwd.as_path(), false); + assert!(controller.push("plain response line\n")); + assert!( + !controller.has_live_tail(), + "plain completed text should not force scrollback reflow", + ); + chat.stream_controller = Some(controller); + + while rx.try_recv().is_ok() {} + + chat.flush_answer_stream_with_separator(); + + let mut saw_consolidate = false; + let mut saw_insert_history = false; + while let Ok(event) = rx.try_recv() { + match event { + AppEvent::InsertHistoryCell(_) => saw_insert_history = true, + AppEvent::ConsolidateAgentMessage { + scrollback_reflow, + deferred_history_cell, + .. + } => { + saw_consolidate = true; + assert_eq!( + scrollback_reflow, + crate::app_event::ConsolidationScrollbackReflow::IfResizeReflowRan + ); + assert!( + deferred_history_cell.is_none(), + "plain text should keep the normal provisional insert path", + ); + } + _ => {} + } + } + + assert!( + saw_consolidate, + "expected stream finalization to consolidate" + ); + assert!( + saw_insert_history, + "plain text should still insert history before consolidation" + ); +} + #[tokio::test] async fn flush_answer_stream_does_not_stop_animation_while_plan_table_stream_is_active() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(None).await;