From 310f2114ae8fc519ba71e49c3e77e3f965f5c727 Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Mon, 22 Dec 2025 19:15:23 -0800 Subject: [PATCH] fix(tui2): fix screen corruption (#8463) Summary Fixes intermittent screen corruption in tui2 (random stale characters) by addressing two terminal state desyncs: nested alt-screen transitions and the first-draw viewport clear. - Make alt-screen enter/leave re-entrant via a small nesting guard so closing - Ensure the first viewport draw clears after the viewport is sized, preventing old terminal contents from leaking through when diff-based rendering skips space cells. - Add docs + a small unit test for the alt-screen nesting behavior. Testing - cargo test -p codex-tui2 - cargo clippy -p codex-tui2 --all-features --tests - Manual: - Opened the transcript overlay and dismissed it repeatedly; verified the normal view redraws cleanly with no leftover characters. - Ran tui2 in a new folder with no trust settings (and also cleared the trust setting from config to re-trigger the prompt); verified the initial trust/onboarding screen renders without artifacts. --- codex-rs/tui2/src/tui.rs | 25 ++++++- codex-rs/tui2/src/tui/alt_screen_nesting.rs | 81 +++++++++++++++++++++ 2 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 codex-rs/tui2/src/tui/alt_screen_nesting.rs diff --git a/codex-rs/tui2/src/tui.rs b/codex-rs/tui2/src/tui.rs index 712c5cf55..2b79b1231 100644 --- a/codex-rs/tui2/src/tui.rs +++ b/codex-rs/tui2/src/tui.rs @@ -46,6 +46,7 @@ use crate::tui::job_control::SUSPEND_KEY; #[cfg(unix)] use crate::tui::job_control::SuspendContext; +mod alt_screen_nesting; mod frame_requester; #[cfg(unix)] mod job_control; @@ -131,6 +132,7 @@ pub struct Tui { draw_tx: broadcast::Sender<()>, pub(crate) terminal: Terminal, pending_history_lines: Vec>, + alt_screen_nesting: alt_screen_nesting::AltScreenNesting, alt_saved_viewport: Option, #[cfg(unix)] suspend_context: SuspendContext, @@ -159,6 +161,7 @@ impl Tui { draw_tx, terminal, pending_history_lines: vec![], + alt_screen_nesting: alt_screen_nesting::AltScreenNesting::default(), alt_saved_viewport: None, #[cfg(unix)] suspend_context: SuspendContext::new(), @@ -305,6 +308,10 @@ impl Tui { /// Enter alternate screen and expand the viewport to full terminal size, saving the current /// inline viewport for restoration when leaving. pub fn enter_alt_screen(&mut self) -> Result<()> { + if !self.alt_screen_nesting.enter() { + self.alt_screen_active.store(true, Ordering::Relaxed); + return Ok(()); + } let _ = execute!(self.terminal.backend_mut(), EnterAlternateScreen); if let Ok(size) = self.terminal.size() { self.alt_saved_viewport = Some(self.terminal.viewport_area); @@ -322,6 +329,11 @@ impl Tui { /// Leave alternate screen and restore the previously saved inline viewport, if any. pub fn leave_alt_screen(&mut self) -> Result<()> { + if !self.alt_screen_nesting.leave() { + self.alt_screen_active + .store(self.alt_screen_nesting.is_active(), Ordering::Relaxed); + return Ok(()); + } let _ = execute!(self.terminal.backend_mut(), LeaveAlternateScreen); if let Some(saved) = self.alt_saved_viewport.take() { self.terminal.set_viewport_area(saved); @@ -368,8 +380,17 @@ impl Tui { let area = Rect::new(0, 0, size.width, height.min(size.height)); if area != terminal.viewport_area { // TODO(nornagon): probably this could be collapsed with the clear + set_viewport_area above. - terminal.clear()?; - terminal.set_viewport_area(area); + if terminal.viewport_area.is_empty() { + // On the first draw the viewport is empty, so `Terminal::clear()` is a no-op. + // If we don't clear after sizing the viewport, diff-based rendering may skip + // writing spaces (because "space" == "space" in the buffers) and stale terminal + // contents can leak through as random characters between words. + terminal.set_viewport_area(area); + terminal.clear()?; + } else { + terminal.clear()?; + terminal.set_viewport_area(area); + } } // Update the y position for suspending so Ctrl-Z can place the cursor correctly. diff --git a/codex-rs/tui2/src/tui/alt_screen_nesting.rs b/codex-rs/tui2/src/tui/alt_screen_nesting.rs new file mode 100644 index 000000000..d384bd270 --- /dev/null +++ b/codex-rs/tui2/src/tui/alt_screen_nesting.rs @@ -0,0 +1,81 @@ +//! Alternate-screen nesting guard. +//! +//! The main `codex-tui2` UI typically runs inside the terminal’s alternate screen buffer so the +//! full viewport can be used without polluting normal scrollback. Some sub-flows (e.g. pager-style +//! overlays) also call `enter_alt_screen()`/`leave_alt_screen()` for historical reasons. +//! +//! Those calls are conceptually “idempotent” (the UI is already on the alt screen), but the +//! underlying terminal commands are *not*: issuing a real `LeaveAlternateScreen` while the rest of +//! the app still thinks it is drawing on the alternate buffer desynchronizes rendering and can +//! leave stale characters behind when returning to the normal view. +//! +//! `AltScreenNesting` tracks a small nesting depth so only the outermost enter/leave actually +//! toggles the terminal mode. + +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub(crate) struct AltScreenNesting { + depth: u16, +} + +impl AltScreenNesting { + pub(crate) fn is_active(self) -> bool { + self.depth > 0 + } + + /// Record an enter-alt-screen request. + /// + /// Returns `true` when the caller should actually enter the alternate screen. + pub(crate) fn enter(&mut self) -> bool { + if self.depth == 0 { + self.depth = 1; + true + } else { + self.depth = self.depth.saturating_add(1); + false + } + } + + /// Record a leave-alt-screen request. + /// + /// Returns `true` when the caller should actually leave the alternate screen. + pub(crate) fn leave(&mut self) -> bool { + match self.depth { + 0 => false, + 1 => { + self.depth = 0; + true + } + _ => { + self.depth = self.depth.saturating_sub(1); + false + } + } + } +} + +#[cfg(test)] +mod tests { + use super::AltScreenNesting; + use pretty_assertions::assert_eq; + + #[test] + fn alt_screen_nesting_tracks_outermost_transitions() { + let mut nesting = AltScreenNesting::default(); + assert_eq!(false, nesting.is_active()); + + assert_eq!(true, nesting.enter()); + assert_eq!(true, nesting.is_active()); + + assert_eq!(false, nesting.enter()); + assert_eq!(true, nesting.is_active()); + + assert_eq!(false, nesting.leave()); + assert_eq!(true, nesting.is_active()); + + assert_eq!(true, nesting.leave()); + assert_eq!(false, nesting.is_active()); + + assert_eq!(false, nesting.leave()); + assert_eq!(false, nesting.is_active()); + } +}