valknar/llmx
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(tui): propagate errors in insert_history_lines_to_writer (#4266)

Browse Source 
## What?
Fixed error handling in `insert_history_lines_to_writer` where all
terminal operations were silently ignoring errors via `.ok()`.

  ## Why?
Silent I/O failures could leave the terminal in an inconsistent state
(e.g., scroll region not reset) with no way to debug. This violates Rust
error handling best practices.

  ## How?
  - Changed function signature to return `io::Result<()>`
  - Replaced all `.ok()` calls with `?` operator to propagate errors
- Added `tracing::warn!` in wrapper function for backward compatibility
  - Updated 15 test call sites to handle Result  with `.expect()`

  ## Testing
  - ✅ Pass all tests

  ## Type of Change
  - [x] Bug fix (non-breaking change)

---------

Signed-off-by: Huaiwu Li <lhwzds@gmail.com>
Co-authored-by: Eric Traut <etraut@openai.com>
This commit is contained in:
Huaiwu Li
2025-10-30 18:07:51 -07:00
committed by GitHub
parent dc2aeac21f
commit 9a638dbf4e
5 changed files with 40 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

15
codex-rs/tui/src/chatwidget/tests.rs
View File

@@ -1582,7 +1582,8 @@ async fn binary_size_transcript_snapshot() {
}
has_emitted_history = true;
transcript.push_str(&lines_to_single_string(&lines));
crate::insert_history::insert_history_lines(&mut terminal, lines);
crate::insert_history::insert_history_lines(&mut terminal, lines)
.expect("Failed to insert history lines in test");
}
}
}
@@ -1603,7 +1604,8 @@ async fn binary_size_transcript_snapshot() {
}
has_emitted_history = true;
transcript.push_str(&lines_to_single_string(&lines));
crate::insert_history::insert_history_lines(&mut terminal, lines);
crate::insert_history::insert_history_lines(&mut terminal, lines)
.expect("Failed to insert history lines in test");
}
}
}
@@ -2654,7 +2656,8 @@ fn chatwidget_exec_and_status_layout_vt100_snapshot() {
term.set_viewport_area(viewport);
for lines in drain_insert_history(&mut rx) {
crate::insert_history::insert_history_lines(&mut term, lines);
crate::insert_history::insert_history_lines(&mut term, lines)
.expect("Failed to insert history lines in test");
}
term.draw(|f| {
@@ -2731,7 +2734,8 @@ printf 'fenced within fenced\n'
while let Ok(app_ev) = rx.try_recv() {
if let AppEvent::InsertHistoryCell(cell) = app_ev {
let lines = cell.display_lines(width);
crate::insert_history::insert_history_lines(&mut term, lines);
crate::insert_history::insert_history_lines(&mut term, lines)
.expect("Failed to insert history lines in test");
inserted_any = true;
}
}
@@ -2749,7 +2753,8 @@ printf 'fenced within fenced\n'
}),
});
for lines in drain_insert_history(&mut rx) {
crate::insert_history::insert_history_lines(&mut term, lines);
crate::insert_history::insert_history_lines(&mut term, lines)
.expect("Failed to insert history lines in test");
}
assert_snapshot!(term.backend().vt100().screen().contents());

43
codex-rs/tui/src/insert_history.rs
View File

@@ -24,7 +24,10 @@ use ratatui::text::Span;
/// Insert `lines` above the viewport using the terminal's backend writer
/// (avoids direct stdout references).
pub fn insert_history_lines<B>(terminal: &mut crate::custom_terminal::Terminal<B>, lines: Vec<Line>)
pub fn insert_history_lines<B>(
terminal: &mut crate::custom_terminal::Terminal<B>,
lines: Vec<Line>,
) -> io::Result<()>
where
B: Backend + Write,
{
@@ -51,13 +54,13 @@ where
// 3) Emitting Reverse Index (RI, ESC M) `scroll_amount` times
// 4) Resetting the scroll region back to full screen
let top_1based = area.top() + 1; // Convert 0-based row to 1-based for DECSTBM
queue!(writer, SetScrollRegion(top_1based..screen_size.height)).ok();
queue!(writer, MoveTo(0, area.top())).ok();
queue!(writer, SetScrollRegion(top_1based..screen_size.height))?;
queue!(writer, MoveTo(0, area.top()))?;
for _ in 0..scroll_amount {
// Reverse Index (RI): ESC M
queue!(writer, Print("\x1bM")).ok();
queue!(writer, Print("\x1bM"))?;
}
queue!(writer, ResetScrollRegion).ok();
queue!(writer, ResetScrollRegion)?;
let cursor_top = area.top().saturating_sub(1);
area.y += scroll_amount;
@@ -82,15 +85,15 @@ where
// ││ ││
// │╰────────────────────────────╯│
// └──────────────────────────────┘
queue!(writer, SetScrollRegion(1..area.top())).ok();
queue!(writer, SetScrollRegion(1..area.top()))?;
// 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(0, cursor_top)).ok();
queue!(writer, MoveTo(0, cursor_top))?;
for line in wrapped {
queue!(writer, Print("\r\n")).ok();
queue!(writer, Print("\r\n"))?;
queue!(
writer,
SetColors(Colors::new(
@@ -103,9 +106,8 @@ where
.map(std::convert::Into::into)
.unwrap_or(CColor::Reset)
))
)
.ok();
queue!(writer, Clear(ClearType::UntilNewLine)).ok();
)?;
queue!(writer, Clear(ClearType::UntilNewLine))?;
// Merge line-level style into each span so that ANSI colors reflect
// line styles (e.g., blockquotes with green fg).
let merged_spans: Vec<Span> = line
@@ -116,18 +118,20 @@ where
content: s.content.clone(),
})
.collect();
write_spans(writer, merged_spans.iter()).ok();
write_spans(writer, merged_spans.iter())?;
}
queue!(writer, ResetScrollRegion).ok();
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)).ok();
queue!(writer, MoveTo(last_cursor_pos.x, last_cursor_pos.y))?;
let _ = writer;
if should_update_area {
terminal.set_viewport_area(area);
}
Ok(())
}
#[derive(Debug, Clone, PartialEq, Eq)]
@@ -328,7 +332,8 @@ mod tests {
// Build a blockquote-like line: apply line-level green style and prefix "> "
let mut line: Line<'static> = Line::from(vec!["> ".into(), "Hello world".into()]);
line = line.style(Color::Green);
insert_history_lines(&mut term, vec![line]);
insert_history_lines(&mut term, vec![line])
.expect("Failed to insert history lines in test");
let mut saw_colored = false;
'outer: for row in 0..height {
@@ -366,7 +371,8 @@ mod tests {
]);
line = line.style(Color::Green);
insert_history_lines(&mut term, vec![line]);
insert_history_lines(&mut term, vec![line])
.expect("Failed to insert history lines in test");
// Parse and inspect the final screen buffer.
let screen = term.backend().vt100().screen();
@@ -428,7 +434,8 @@ mod tests {
Span::raw("Hello world"),
]);
insert_history_lines(&mut term, vec![line]);
insert_history_lines(&mut term, vec![line])
.expect("Failed to insert history lines in test");
let screen = term.backend().vt100().screen();
@@ -484,7 +491,7 @@ mod tests {
let viewport = ratatui::layout::Rect::new(0, height - 1, width, 1);
term.set_viewport_area(viewport);
insert_history_lines(&mut term, lines);
insert_history_lines(&mut term, lines).expect("Failed to insert history lines in test");
let screen = term.backend().vt100().screen();

2
codex-rs/tui/src/tui.rs
View File

@@ -548,7 +548,7 @@ impl Tui {
crate::insert_history::insert_history_lines(
terminal,
self.pending_history_lines.clone(),
);
)?;
self.pending_history_lines.clear();
}
// Update the y position for suspending so Ctrl-Z can place the cursor correctly.

3
codex-rs/tui/tests/suite/vt100_history.rs
View File

@@ -36,7 +36,8 @@ impl TestScenario {
}
fn run_insert(&mut self, lines: Vec<Line<'static>>) {
codex_tui::insert_history::insert_history_lines(&mut self.term, lines);
codex_tui::insert_history::insert_history_lines(&mut self.term, lines)
.expect("Failed to insert history lines in test");
}
}

3
codex-rs/tui/tests/suite/vt100_live_commit.rs
View File

@@ -26,7 +26,8 @@ fn live_001_commit_on_overflow() {
let commit_rows = rb.drain_commit_ready(3);
let lines: Vec<Line<'static>> = commit_rows.into_iter().map(|r| r.text.into()).collect();
codex_tui::insert_history::insert_history_lines(&mut term, lines);
codex_tui::insert_history::insert_history_lines(&mut term, lines)
.expect("Failed to insert history lines in test");
let screen = term.backend().vt100().screen();