mirror of
https://github.com/openai/codex.git
synced 2026-05-28 06:55:01 +00:00
fix(tui): keep raw output above composer in zellij (#24593)
## Why Raw output mode intentionally sends logical source lines to the terminal without Codex-inserted wrapping so copied content retains its original line structure. In Zellij, soft-wrapped continuation rows from those raw lines are not confined by the inline history scroll region. When raw mode replays a long transcript, continuation rows can occupy the composer viewport and are overwritten on the following draw, leaving the transcript visibly truncated underneath the composer. This is specific to the combination of Zellij and raw terminal-wrapped history. Rich output and non-Zellij terminals should continue using the existing insertion behavior. Related context: #20819 introduced raw output mode, and #22214 removed the broad Zellij insertion workaround after the standard rich-output path no longer required it. | Before | After | |---|---| | <img width="1728" height="916" alt="image" src="https://github.com/user-attachments/assets/f85398a5-e930-46d9-bcfd-106a24c41466" /> | <img width="1723" height="912" alt="image" src="https://github.com/user-attachments/assets/5c62e16a-a6e5-4842-bcb2-eab163cda04c" /> | ## What Changed - Cache Zellij detection in `Tui` and select a dedicated insertion mode only for `HistoryLineWrapPolicy::Terminal` batches in Zellij. - For that guarded path, clear the existing viewport, append raw source lines through the terminal so its soft wrapping remains selection-friendly, and reserve empty viewport rows before redrawing the composer. - Add snapshot regressions for both an incremental soft-wrapped raw insert and an overflowing raw transcript replay that starts at the top of the cleared terminal. ## How to Test 1. Start Codex inside Zellij with raw output enabled or toggle raw output after a multiline response is in history. 2. Produce or replay output containing long logical lines, such as a fenced shell command with several wrapped lines. 3. Confirm the wrapped history remains visible above the composer and the composer no longer overwrites the end of the response. 4. Toggle back to rich output or run outside Zellij and confirm standard history rendering still behaves normally. Targeted tests run: - `just test -p codex-tui vt100_zellij_raw -- --nocapture` Additional validation notes: - `just test -p codex-tui` was attempted; the two new Zellij raw insertion tests passed, while two existing `app::tests::update_feature_flags_disabling_guardian_*` tests failed outside this history insertion path. - `just argument-comment-lint` was attempted but local Bazel analysis fails before reaching the changed source because the LLVM `compiler-rt` package is missing `include/sanitizer/*.h`. Modified literal callsites were inspected manually.
This commit is contained in:
@@ -40,6 +40,17 @@ pub enum HistoryLineWrapPolicy {
|
||||
Terminal,
|
||||
}
|
||||
|
||||
/// Selects the terminal escape strategy used when writing history above the viewport.
|
||||
///
|
||||
/// Raw lines intentionally remain unbroken so terminal selection copies their source faithfully.
|
||||
/// Zellij does not constrain soft-wrapped continuation rows to Codex's scroll region, so its raw
|
||||
/// path appends history through the terminal and reserves blank rows for the next viewport draw.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub(crate) enum InsertHistoryMode {
|
||||
Standard,
|
||||
ZellijRaw,
|
||||
}
|
||||
|
||||
/// Insert `lines` above the viewport using the terminal's backend writer
|
||||
/// (avoids direct stdout references).
|
||||
pub fn insert_history_lines<B>(
|
||||
@@ -57,6 +68,23 @@ pub fn insert_history_lines_with_wrap_policy<B>(
|
||||
lines: Vec<Line>,
|
||||
wrap_policy: HistoryLineWrapPolicy,
|
||||
) -> io::Result<()>
|
||||
where
|
||||
B: Backend + Write,
|
||||
{
|
||||
insert_history_lines_with_mode_and_wrap_policy(
|
||||
terminal,
|
||||
lines,
|
||||
InsertHistoryMode::Standard,
|
||||
wrap_policy,
|
||||
)
|
||||
}
|
||||
|
||||
pub(crate) fn insert_history_lines_with_mode_and_wrap_policy<B>(
|
||||
terminal: &mut crate::custom_terminal::Terminal<B>,
|
||||
lines: Vec<Line>,
|
||||
mode: InsertHistoryMode,
|
||||
wrap_policy: HistoryLineWrapPolicy,
|
||||
) -> io::Result<()>
|
||||
where
|
||||
B: Backend + Write,
|
||||
{
|
||||
@@ -65,7 +93,6 @@ where
|
||||
let mut area = terminal.viewport_area;
|
||||
let mut should_update_area = false;
|
||||
let last_cursor_pos = terminal.last_known_cursor_pos;
|
||||
let writer = terminal.backend_mut();
|
||||
|
||||
// Pre-wrap lines for terminal scrollback. Three paths:
|
||||
//
|
||||
@@ -102,60 +129,92 @@ where
|
||||
wrapped.extend(line_wrapped);
|
||||
}
|
||||
let wrapped_lines = wrapped_rows as u16;
|
||||
let cursor_top = if area.bottom() < screen_size.height {
|
||||
// If the viewport is not at the bottom of the screen, scroll it down to make room.
|
||||
// Don't scroll it past the bottom of the screen.
|
||||
let scroll_amount = wrapped_lines.min(screen_size.height - area.bottom());
|
||||
match mode {
|
||||
InsertHistoryMode::ZellijRaw => {
|
||||
// The existing viewport is immediately replaced in the same draw pass. Clear it
|
||||
// before terminal scrolling can move composer contents into scrollback.
|
||||
terminal.clear_after_position(area.as_position())?;
|
||||
let writer = terminal.backend_mut();
|
||||
queue!(writer, MoveTo(/*x*/ 0, area.top()))?;
|
||||
for (index, line) in wrapped.iter().enumerate() {
|
||||
if index > 0 {
|
||||
queue!(writer, Print("\r\n"))?;
|
||||
}
|
||||
write_history_line(writer, line, wrap_width)?;
|
||||
}
|
||||
|
||||
let top_1based = area.top() + 1;
|
||||
queue!(writer, SetScrollRegion(top_1based..screen_size.height))?;
|
||||
queue!(writer, MoveTo(/*x*/ 0, area.top()))?;
|
||||
for _ in 0..scroll_amount {
|
||||
queue!(writer, Print("\x1bM"))?;
|
||||
// Writing raw source text through the terminal preserves its soft-wrap metadata.
|
||||
// Advance through empty rows for the viewport so history ends immediately above the
|
||||
// composer even when a replay batch is taller than the visible history region.
|
||||
for _ in 0..area.height {
|
||||
queue!(writer, Print("\r\n"), Clear(ClearType::UntilNewLine))?;
|
||||
}
|
||||
queue!(writer, MoveTo(last_cursor_pos.x, last_cursor_pos.y))?;
|
||||
|
||||
let viewport_top = area
|
||||
.top()
|
||||
.saturating_add(wrapped_lines)
|
||||
.min(screen_size.height.saturating_sub(area.height));
|
||||
if area.y != viewport_top {
|
||||
area.y = viewport_top;
|
||||
should_update_area = true;
|
||||
}
|
||||
}
|
||||
queue!(writer, ResetScrollRegion)?;
|
||||
InsertHistoryMode::Standard => {
|
||||
let writer = terminal.backend_mut();
|
||||
let cursor_top = if area.bottom() < screen_size.height {
|
||||
// If the viewport is not at the bottom of the screen, scroll it down to make room.
|
||||
// Don't scroll it past the bottom of the screen.
|
||||
let scroll_amount = wrapped_lines.min(screen_size.height - area.bottom());
|
||||
|
||||
let cursor_top = area.top().saturating_sub(1);
|
||||
area.y += scroll_amount;
|
||||
should_update_area = true;
|
||||
cursor_top
|
||||
} else {
|
||||
area.top().saturating_sub(1)
|
||||
};
|
||||
let top_1based = area.top() + 1;
|
||||
queue!(writer, SetScrollRegion(top_1based..screen_size.height))?;
|
||||
queue!(writer, MoveTo(/*x*/ 0, area.top()))?;
|
||||
for _ in 0..scroll_amount {
|
||||
queue!(writer, Print("\x1bM"))?;
|
||||
}
|
||||
queue!(writer, ResetScrollRegion)?;
|
||||
|
||||
// Limit the scroll region to the lines from the top of the screen to the
|
||||
// top of the viewport. With this in place, when we add lines inside this
|
||||
// area, only the lines in this area will be scrolled. We place the cursor
|
||||
// at the end of the scroll region, and add lines starting there.
|
||||
//
|
||||
// ┌─Screen───────────────────────┐
|
||||
// │┌╌Scroll region╌╌╌╌╌╌╌╌╌╌╌╌╌╌┐│
|
||||
// │┆ ┆│
|
||||
// │┆ ┆│
|
||||
// │┆ ┆│
|
||||
// │█╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┘│
|
||||
// │╭─Viewport───────────────────╮│
|
||||
// ││ ││
|
||||
// │╰────────────────────────────╯│
|
||||
// └──────────────────────────────┘
|
||||
queue!(writer, SetScrollRegion(1..area.top()))?;
|
||||
let cursor_top = area.top().saturating_sub(1);
|
||||
area.y += scroll_amount;
|
||||
should_update_area = true;
|
||||
cursor_top
|
||||
} else {
|
||||
area.top().saturating_sub(1)
|
||||
};
|
||||
|
||||
// NB: we are using MoveTo instead of set_cursor_position here to avoid messing with the
|
||||
// terminal's last_known_cursor_position, which hopefully will still be accurate after we
|
||||
// fetch/restore the cursor position. insert_history_lines should be cursor-position-neutral :)
|
||||
queue!(writer, MoveTo(/*x*/ 0, cursor_top))?;
|
||||
// Limit the scroll region to the lines from the top of the screen to the
|
||||
// top of the viewport. With this in place, when we add lines inside this
|
||||
// area, only the lines in this area will be scrolled. We place the cursor
|
||||
// at the end of the scroll region, and add lines starting there.
|
||||
//
|
||||
// ┌─Screen───────────────────────┐
|
||||
// │┌╌Scroll region╌╌╌╌╌╌╌╌╌╌╌╌╌╌┐│
|
||||
// │┆ ┆│
|
||||
// │┆ ┆│
|
||||
// │┆ ┆│
|
||||
// │█╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌╌┘│
|
||||
// │╭─Viewport───────────────────╮│
|
||||
// ││ ││
|
||||
// │╰────────────────────────────╯│
|
||||
// └──────────────────────────────┘
|
||||
queue!(writer, SetScrollRegion(1..area.top()))?;
|
||||
|
||||
for line in &wrapped {
|
||||
queue!(writer, Print("\r\n"))?;
|
||||
write_history_line(writer, line, wrap_width)?;
|
||||
// NB: we are using MoveTo instead of set_cursor_position here to avoid messing with the
|
||||
// terminal's last_known_cursor_position, which hopefully will still be accurate after we
|
||||
// fetch/restore the cursor position. insert_history_lines should be cursor-position-neutral :)
|
||||
queue!(writer, MoveTo(/*x*/ 0, cursor_top))?;
|
||||
|
||||
for line in &wrapped {
|
||||
queue!(writer, Print("\r\n"))?;
|
||||
write_history_line(writer, line, wrap_width)?;
|
||||
}
|
||||
|
||||
queue!(writer, ResetScrollRegion)?;
|
||||
queue!(writer, MoveTo(last_cursor_pos.x, last_cursor_pos.y))?;
|
||||
}
|
||||
}
|
||||
|
||||
queue!(writer, ResetScrollRegion)?;
|
||||
|
||||
// Restore the cursor position to where it was before we started.
|
||||
queue!(writer, MoveTo(last_cursor_pos.x, last_cursor_pos.y))?;
|
||||
|
||||
let _ = writer;
|
||||
if should_update_area {
|
||||
terminal.set_viewport_area(area);
|
||||
}
|
||||
@@ -796,6 +855,85 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_zellij_raw_insert_keeps_soft_wrapped_tail_above_viewport() {
|
||||
let width: u16 = 20;
|
||||
let height: u16 = 8;
|
||||
let backend = VT100Backend::new(width, height);
|
||||
let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal");
|
||||
let viewport = Rect::new(
|
||||
/*x*/ 0,
|
||||
/*y*/ height - 2,
|
||||
/*width*/ width,
|
||||
/*height*/ 2,
|
||||
);
|
||||
term.set_viewport_area(viewport);
|
||||
|
||||
let line = Line::from("raw-start-aaaaaaaaaaaaaaaaaaaaaaaa-tail-must-remain");
|
||||
insert_history_lines_with_mode_and_wrap_policy(
|
||||
&mut term,
|
||||
vec![line],
|
||||
InsertHistoryMode::ZellijRaw,
|
||||
HistoryLineWrapPolicy::Terminal,
|
||||
)
|
||||
.expect("insert Zellij raw history");
|
||||
|
||||
let rows: Vec<String> = term.backend().vt100().screen().rows(0, width).collect();
|
||||
insta::assert_snapshot!("zellij_raw_terminal_wrap_above_viewport", rows.join("\n"));
|
||||
let history_rows = rows[..usize::from(term.viewport_area.y)]
|
||||
.iter()
|
||||
.map(|row| row.trim_end())
|
||||
.collect::<String>();
|
||||
let viewport_rows = rows[usize::from(term.viewport_area.y)..].join("\n");
|
||||
assert!(
|
||||
history_rows.contains("tail-must-remain"),
|
||||
"expected wrapped raw tail above the viewport, rows: {rows:?}"
|
||||
);
|
||||
assert!(
|
||||
!viewport_rows.contains("tail-must-remain"),
|
||||
"raw tail must not be written through the viewport, rows: {rows:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_zellij_raw_replay_keeps_overflowing_soft_wrapped_tail_above_viewport() {
|
||||
let width: u16 = 20;
|
||||
let height: u16 = 8;
|
||||
let backend = VT100Backend::new(width, height);
|
||||
let mut term = crate::custom_terminal::Terminal::with_options(backend).expect("terminal");
|
||||
term.set_viewport_area(Rect::new(
|
||||
/*x*/ 0, /*y*/ 0, /*width*/ width, /*height*/ 2,
|
||||
));
|
||||
|
||||
let line = Line::from(format!("raw-start-{}tail-must-remain", "a".repeat(130)));
|
||||
insert_history_lines_with_mode_and_wrap_policy(
|
||||
&mut term,
|
||||
vec![line],
|
||||
InsertHistoryMode::ZellijRaw,
|
||||
HistoryLineWrapPolicy::Terminal,
|
||||
)
|
||||
.expect("replay Zellij raw history");
|
||||
|
||||
let rows: Vec<String> = term.backend().vt100().screen().rows(0, width).collect();
|
||||
insta::assert_snapshot!(
|
||||
"zellij_raw_terminal_wrap_overflow_above_viewport",
|
||||
rows.join("\n")
|
||||
);
|
||||
let history_rows = rows[..usize::from(term.viewport_area.y)]
|
||||
.iter()
|
||||
.map(|row| row.trim_end())
|
||||
.collect::<String>();
|
||||
let viewport_rows = rows[usize::from(term.viewport_area.y)..].join("\n");
|
||||
assert!(
|
||||
history_rows.contains("tail-must-remain"),
|
||||
"expected overflowing raw tail above the viewport, rows: {rows:?}"
|
||||
);
|
||||
assert!(
|
||||
!viewport_rows.contains("tail-must-remain"),
|
||||
"overflowing raw tail must not be written through the viewport, rows: {rows:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_unwrapped_url_like_clears_continuation_rows() {
|
||||
let width: u16 = 20;
|
||||
|
||||
@@ -0,0 +1,10 @@
|
||||
---
|
||||
source: tui/src/insert_history.rs
|
||||
expression: "rows.join(\"\\n\")"
|
||||
---
|
||||
|
||||
|
||||
|
||||
raw-start-aaaaaaaaaa
|
||||
aaaaaaaaaaaaaa-tail-
|
||||
must-remain
|
||||
@@ -0,0 +1,10 @@
|
||||
---
|
||||
source: tui/src/insert_history.rs
|
||||
expression: "rows.join(\"\\n\")"
|
||||
---
|
||||
aaaaaaaaaaaaaaaaaaaa
|
||||
aaaaaaaaaaaaaaaaaaaa
|
||||
aaaaaaaaaaaaaaaaaaaa
|
||||
aaaaaaaaaaaaaaaaaaaa
|
||||
aaaaaaaaaaaaaaaaaaaa
|
||||
tail-must-remain
|
||||
@@ -41,6 +41,7 @@ pub use self::frame_requester::FrameRequester;
|
||||
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::tui::event_stream::EventBroker;
|
||||
@@ -500,6 +501,8 @@ pub struct Tui {
|
||||
enhanced_keys_supported: bool,
|
||||
notification_backend: Option<DesktopNotificationBackend>,
|
||||
notification_condition: NotificationCondition,
|
||||
// Raw terminal-wrapped history needs a non-scroll-region insertion path in Zellij.
|
||||
is_zellij: bool,
|
||||
// When false, enter_alt_screen() becomes a no-op.
|
||||
alt_screen_enabled: bool,
|
||||
// Keeps unmanaged process stderr writes out of the inline viewport.
|
||||
@@ -535,6 +538,7 @@ 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 = codex_terminal_detection::terminal_info().is_zellij();
|
||||
|
||||
Self {
|
||||
frame_requester,
|
||||
@@ -552,6 +556,7 @@ impl Tui {
|
||||
enhanced_keys_supported,
|
||||
notification_backend: Some(detect_backend(NotificationMethod::default())),
|
||||
notification_condition: NotificationCondition::default(),
|
||||
is_zellij,
|
||||
alt_screen_enabled: true,
|
||||
_stderr_guard: stderr_guard,
|
||||
}
|
||||
@@ -797,15 +802,22 @@ impl Tui {
|
||||
fn flush_pending_history_lines(
|
||||
terminal: &mut Terminal,
|
||||
pending_history_lines: &mut Vec<PendingHistoryLines>,
|
||||
is_zellij: bool,
|
||||
) -> Result<()> {
|
||||
if pending_history_lines.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
for batch in pending_history_lines.iter() {
|
||||
crate::insert_history::insert_history_lines_with_wrap_policy(
|
||||
let mode = if is_zellij && batch.wrap_policy == HistoryLineWrapPolicy::Terminal {
|
||||
InsertHistoryMode::ZellijRaw
|
||||
} else {
|
||||
InsertHistoryMode::Standard
|
||||
};
|
||||
crate::insert_history::insert_history_lines_with_mode_and_wrap_policy(
|
||||
terminal,
|
||||
batch.lines.clone(),
|
||||
mode,
|
||||
batch.wrap_policy,
|
||||
)?;
|
||||
}
|
||||
@@ -862,7 +874,11 @@ impl Tui {
|
||||
terminal.set_viewport_area(area);
|
||||
}
|
||||
|
||||
Self::flush_pending_history_lines(terminal, &mut self.pending_history_lines)?;
|
||||
Self::flush_pending_history_lines(
|
||||
terminal,
|
||||
&mut self.pending_history_lines,
|
||||
self.is_zellij,
|
||||
)?;
|
||||
|
||||
// Update the y position for suspending so Ctrl-Z can place the cursor correctly.
|
||||
#[cfg(unix)]
|
||||
@@ -968,7 +984,11 @@ impl Tui {
|
||||
let terminal = &mut self.terminal;
|
||||
let needs_full_repaint =
|
||||
Self::update_inline_viewport_for_resize_reflow(terminal, height)?;
|
||||
Self::flush_pending_history_lines(terminal, &mut self.pending_history_lines)?;
|
||||
Self::flush_pending_history_lines(
|
||||
terminal,
|
||||
&mut self.pending_history_lines,
|
||||
self.is_zellij,
|
||||
)?;
|
||||
|
||||
if needs_full_repaint {
|
||||
terminal.invalidate_viewport();
|
||||
|
||||
Reference in New Issue
Block a user