mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
feat(tui): remove Zellij TUI workarounds (#22214)
## Why We added Zellij-specific TUI workarounds because older Zellij behavior did not work with Codex's normal terminal model: - #8555 made `tui.alternate_screen = "auto"` disable alternate screen in Zellij so transcript history stayed available. - #16578 avoided scroll-region operations in Zellij by emitting raw newlines and using a separate composer styling path. This PR removes both workarounds because the latest Zellij release tested locally (`zellij 0.44.1`) works correctly with Codex's standard TUI behavior: normal alternate-screen handling, redraw, and history insertion. ## What Changed - Removed the `InsertHistoryMode::Zellij` path and the Zellij-only newline scrollback insertion behavior. - Removed cached `is_zellij` state from the TUI and composer. - Removed Zellij-specific composer styling, the helper snapshot, and the `TerminalInfo::is_zellij()` convenience method that only served this workaround. - Changed `tui.alternate_screen = "auto"` to use alternate screen for Zellij too; `--no-alt-screen` and `tui.alternate_screen = "never"` still preserve the inline mode escape hatch. - Updated the generated config schema description for `tui.alternate_screen`. ## How to Test Manual smoke path used with `zellij 0.44.1`: 1. Build and run this branch inside a Zellij `0.44.1` session with default config. 2. Start Codex normally and produce enough assistant/tool output to create scrollback. 3. Confirm the transcript remains readable, the composer renders normally, and scrolling through terminal history works. 4. Resize the Zellij pane while output exists and confirm the TUI redraws without duplicated, missing, or stale rows. 5. Compare with `--no-alt-screen` or `-c tui.alternate_screen=never` if you want to verify the inline fallback still works. Targeted tests: - `just write-config-schema` - `just fmt` - `just fix -p codex-tui` - `cargo test -p codex-terminal-detection` - `cargo test -p codex-tui alternate_screen_auto_uses_alt_screen` Attempted but did not complete locally: - `cargo test -p codex-tui` built and ran the new test successfully, then failed later on unrelated local failures in `status_permissions_full_disk_managed_*` and a stack overflow in `tests::fork_last_filters_latest_session_by_cwd_unless_show_all`. ## Documentation No developers.openai.com Codex documentation update is needed for this revert.
This commit is contained in:
1 parent
68e045a631
commit
6f77b70ff3
12 files changed
+126
-474
No files matched your search
+29
-97
@@ -33,7 +33,6 @@ use ratatui::crossterm::terminal::enable_raw_mode;
|
||||
use ratatui::layout::Offset;
|
||||
use ratatui::layout::Position;
|
||||
use ratatui::layout::Rect;
|
||||
use ratatui::layout::Size;
|
||||
use ratatui::text::Line;
|
||||
use tokio::sync::broadcast;
|
||||
use tokio_stream::Stream;
|
||||
@@ -440,8 +439,7 @@ pub struct Tui {
|
||||
enhanced_keys_supported: bool,
|
||||
notification_backend: Option<DesktopNotificationBackend>,
|
||||
notification_condition: NotificationCondition,
|
||||
is_zellij: bool,
|
||||
// When false, enter_alt_screen() becomes a no-op (for Zellij scrollback support)
|
||||
// When false, enter_alt_screen() becomes a no-op.
|
||||
alt_screen_enabled: bool,
|
||||
}
|
||||
|
||||
@@ -474,10 +472,6 @@ 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 {})
|
||||
);
|
||||
|
||||
Self {
|
||||
frame_requester,
|
||||
@@ -495,7 +489,6 @@ impl Tui {
|
||||
enhanced_keys_supported,
|
||||
notification_backend: Some(detect_backend(NotificationMethod::default())),
|
||||
notification_condition: NotificationCondition::default(),
|
||||
is_zellij,
|
||||
alt_screen_enabled: true,
|
||||
}
|
||||
}
|
||||
@@ -687,62 +680,6 @@ impl Tui {
|
||||
self.pending_history_lines.clear();
|
||||
}
|
||||
|
||||
/// Resize the inline viewport to `height` rows, scrolling content above it if
|
||||
/// the viewport would extend past the bottom of the screen. Returns `true` when
|
||||
/// the caller must invalidate the diff buffer (Zellij mode), because the scroll
|
||||
/// was performed with raw newlines that ratatui cannot track.
|
||||
fn update_inline_viewport(
|
||||
terminal: &mut Terminal,
|
||||
height: u16,
|
||||
is_zellij: bool,
|
||||
) -> Result<bool> {
|
||||
let size = terminal.size()?;
|
||||
let mut needs_full_repaint = false;
|
||||
|
||||
let mut area = terminal.viewport_area;
|
||||
area.height = height.min(size.height);
|
||||
area.width = size.width;
|
||||
if area.bottom() > size.height {
|
||||
let scroll_by = area.bottom() - size.height;
|
||||
if is_zellij {
|
||||
Self::scroll_zellij_expanded_viewport(terminal, size, scroll_by)?;
|
||||
needs_full_repaint = true;
|
||||
} else {
|
||||
terminal
|
||||
.backend_mut()
|
||||
.scroll_region_up(0..area.top(), scroll_by)?;
|
||||
}
|
||||
area.y = size.height - area.height;
|
||||
}
|
||||
if area != terminal.viewport_area {
|
||||
// On startup, the old viewport can still be empty. Clear from the
|
||||
// new viewport top so stale shell cells do not show through spaces.
|
||||
clear_for_viewport_change(terminal, area)?;
|
||||
terminal.set_viewport_area(area);
|
||||
}
|
||||
|
||||
Ok(needs_full_repaint)
|
||||
}
|
||||
|
||||
/// Push content above the viewport upward by `scroll_by` rows using raw
|
||||
/// newlines at the screen bottom. This is the Zellij-safe alternative to
|
||||
/// `scroll_region_up`, which relies on DECSTBM sequences Zellij does not
|
||||
/// support.
|
||||
fn scroll_zellij_expanded_viewport(
|
||||
terminal: &mut Terminal,
|
||||
size: Size,
|
||||
scroll_by: u16,
|
||||
) -> Result<()> {
|
||||
crossterm::queue!(
|
||||
terminal.backend_mut(),
|
||||
crossterm::cursor::MoveTo(0, size.height.saturating_sub(1))
|
||||
)?;
|
||||
for _ in 0..scroll_by {
|
||||
crossterm::queue!(terminal.backend_mut(), crossterm::style::Print("\n"))?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Resize the inline viewport for the resize-reflow path.
|
||||
///
|
||||
/// Unlike the legacy draw path, this path does not scroll rows above the viewport when the
|
||||
@@ -751,7 +688,6 @@ impl Tui {
|
||||
fn update_inline_viewport_for_resize_reflow(
|
||||
terminal: &mut Terminal,
|
||||
height: u16,
|
||||
is_zellij: bool,
|
||||
) -> Result<bool> {
|
||||
let size = terminal.size()?;
|
||||
let terminal_height_shrank = size.height < terminal.last_known_screen_size.height;
|
||||
@@ -768,13 +704,9 @@ impl Tui {
|
||||
if area.bottom() > size.height {
|
||||
let scroll_by = area.bottom() - size.height;
|
||||
if !terminal_height_shrank {
|
||||
if is_zellij {
|
||||
Self::scroll_zellij_expanded_viewport(terminal, size, scroll_by)?;
|
||||
} else {
|
||||
terminal
|
||||
.backend_mut()
|
||||
.scroll_region_up(0..area.top(), scroll_by)?;
|
||||
}
|
||||
terminal
|
||||
.backend_mut()
|
||||
.scroll_region_up(0..area.top(), scroll_by)?;
|
||||
}
|
||||
area.y = size.height - area.height;
|
||||
} else if terminal_height_grew && viewport_was_bottom_aligned {
|
||||
@@ -792,27 +724,23 @@ 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
|
||||
/// invalidate the diff buffer for a full repaint.
|
||||
fn flush_pending_history_lines(
|
||||
terminal: &mut Terminal,
|
||||
pending_history_lines: &mut Vec<PendingHistoryLines>,
|
||||
is_zellij: bool,
|
||||
) -> Result<bool> {
|
||||
) -> Result<()> {
|
||||
if pending_history_lines.is_empty() {
|
||||
return Ok(false);
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
for batch in pending_history_lines.iter() {
|
||||
crate::insert_history::insert_history_lines_with_mode_and_wrap_policy(
|
||||
crate::insert_history::insert_history_lines_with_wrap_policy(
|
||||
terminal,
|
||||
batch.lines.clone(),
|
||||
crate::insert_history::InsertHistoryMode::new(is_zellij),
|
||||
batch.wrap_policy,
|
||||
)?;
|
||||
}
|
||||
pending_history_lines.clear();
|
||||
Ok(is_zellij)
|
||||
Ok(())
|
||||
}
|
||||
|
||||
pub fn draw(
|
||||
@@ -843,17 +771,26 @@ impl Tui {
|
||||
terminal.clear()?;
|
||||
}
|
||||
|
||||
let mut needs_full_repaint =
|
||||
Self::update_inline_viewport(terminal, height, self.is_zellij)?;
|
||||
needs_full_repaint |= Self::flush_pending_history_lines(
|
||||
terminal,
|
||||
&mut self.pending_history_lines,
|
||||
self.is_zellij,
|
||||
)?;
|
||||
let size = terminal.size()?;
|
||||
|
||||
if needs_full_repaint {
|
||||
terminal.invalidate_viewport();
|
||||
let mut area = terminal.viewport_area;
|
||||
area.height = height.min(size.height);
|
||||
area.width = size.width;
|
||||
// If the viewport has expanded, scroll everything else up to make room.
|
||||
if area.bottom() > size.height {
|
||||
terminal
|
||||
.backend_mut()
|
||||
.scroll_region_up(0..area.top(), area.bottom() - size.height)?;
|
||||
area.y = size.height - area.height;
|
||||
}
|
||||
if area != terminal.viewport_area {
|
||||
// On startup, the old viewport can still be empty. Clear from the
|
||||
// new viewport top so stale shell cells do not show through spaces.
|
||||
clear_for_viewport_change(terminal, area)?;
|
||||
terminal.set_viewport_area(area);
|
||||
}
|
||||
|
||||
Self::flush_pending_history_lines(terminal, &mut self.pending_history_lines)?;
|
||||
|
||||
// Update the y position for suspending so Ctrl-Z can place the cursor correctly.
|
||||
#[cfg(unix)]
|
||||
@@ -943,14 +880,9 @@ impl Tui {
|
||||
}
|
||||
|
||||
let terminal = &mut self.terminal;
|
||||
let mut needs_full_repaint =
|
||||
Self::update_inline_viewport_for_resize_reflow(terminal, height, self.is_zellij)?;
|
||||
let flushed_history = Self::flush_pending_history_lines(
|
||||
terminal,
|
||||
&mut self.pending_history_lines,
|
||||
self.is_zellij,
|
||||
)?;
|
||||
needs_full_repaint |= flushed_history;
|
||||
let needs_full_repaint =
|
||||
Self::update_inline_viewport_for_resize_reflow(terminal, height)?;
|
||||
Self::flush_pending_history_lines(terminal, &mut self.pending_history_lines)?;
|
||||
|
||||
if needs_full_repaint {
|
||||
terminal.invalidate_viewport();
|
||||
|
||||
Reference in new issue
Block a user