From 9a638dbf4e5ce769b4a4ce0864d17a5f15e99d2b Mon Sep 17 00:00:00 2001 From: Huaiwu Li Date: Thu, 30 Oct 2025 18:07:51 -0700 Subject: [PATCH] fix(tui): propagate errors in insert_history_lines_to_writer (#4266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 Co-authored-by: Eric Traut --- codex-rs/tui/src/chatwidget/tests.rs | 15 ++++--- codex-rs/tui/src/insert_history.rs | 43 +++++++++++-------- codex-rs/tui/src/tui.rs | 2 +- codex-rs/tui/tests/suite/vt100_history.rs | 3 +- codex-rs/tui/tests/suite/vt100_live_commit.rs | 3 +- 5 files changed, 40 insertions(+), 26 deletions(-) diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 6c9b78e3..a6a991bb 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -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()); diff --git a/codex-rs/tui/src/insert_history.rs b/codex-rs/tui/src/insert_history.rs index 8f4bfa10..36ef47da 100644 --- a/codex-rs/tui/src/insert_history.rs +++ b/codex-rs/tui/src/insert_history.rs @@ -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(terminal: &mut crate::custom_terminal::Terminal, lines: Vec) +pub fn insert_history_lines( + terminal: &mut crate::custom_terminal::Terminal, + lines: Vec, +) -> 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 = 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(); diff --git a/codex-rs/tui/src/tui.rs b/codex-rs/tui/src/tui.rs index f6e1c2c5..ca672355 100644 --- a/codex-rs/tui/src/tui.rs +++ b/codex-rs/tui/src/tui.rs @@ -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. diff --git a/codex-rs/tui/tests/suite/vt100_history.rs b/codex-rs/tui/tests/suite/vt100_history.rs index a4c0e387..6df9dedf 100644 --- a/codex-rs/tui/tests/suite/vt100_history.rs +++ b/codex-rs/tui/tests/suite/vt100_history.rs @@ -36,7 +36,8 @@ impl TestScenario { } fn run_insert(&mut self, lines: Vec>) { - 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"); } } diff --git a/codex-rs/tui/tests/suite/vt100_live_commit.rs b/codex-rs/tui/tests/suite/vt100_live_commit.rs index e9689cf5..2be9658b 100644 --- a/codex-rs/tui/tests/suite/vt100_live_commit.rs +++ b/codex-rs/tui/tests/suite/vt100_live_commit.rs @@ -26,7 +26,8 @@ fn live_001_commit_on_overflow() { let commit_rows = rb.drain_commit_ready(3); let lines: Vec> = 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();