From d045974fab7f0c93e433bd8439d4972fe879f968 Mon Sep 17 00:00:00 2001 From: Daniel Edrisian Date: Wed, 17 Sep 2025 11:36:38 -0700 Subject: [PATCH] nornagon review fixes --- codex-rs/protocol/src/protocol.rs | 3 ++ codex-rs/tui/src/chatwidget.rs | 66 ++++++++++++---------------- codex-rs/tui/src/chatwidget/tests.rs | 4 +- codex-rs/tui/src/history_cell.rs | 9 +--- 4 files changed, 33 insertions(+), 49 deletions(-) diff --git a/codex-rs/protocol/src/protocol.rs b/codex-rs/protocol/src/protocol.rs index c467cba346..8088ff43bc 100644 --- a/codex-rs/protocol/src/protocol.rs +++ b/codex-rs/protocol/src/protocol.rs @@ -984,6 +984,9 @@ pub struct ReviewRequest { pub struct ReviewOutputEvent { pub findings: Vec, pub overall_correctness: String, + + /// This will always have a value, even when there are no findings, or + /// if the review output wasn't valid JSON. pub overall_explanation: String, pub overall_confidence_score: f32, } diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index c1d2351b81..b891518f62 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -281,15 +281,10 @@ impl ChatWidget { self.bottom_pane.set_token_usage(info.clone()); self.token_info = info; } - /// Finalize any active exec as failed, push an error message into history, - /// and stop/clear running UI state. - fn finalize_turn_with_error_message(&mut self, message: Option) { + /// Finalize any active exec as failed and stop/clear running UI state. + fn finalize_turn(&mut self) { // Ensure any spinner is replaced by a red ✗ and flushed into history. self.finalize_active_exec_cell_as_failed(); - // Emit the provided error message/history cell. - if let Some(message) = message { - self.add_to_history(history_cell::new_error_event(message)); - } // Reset running state and clear streaming buffers. self.bottom_pane.set_task_running(false); self.running_commands.clear(); @@ -297,7 +292,8 @@ impl ChatWidget { } fn on_error(&mut self, message: String) { - self.finalize_turn_with_error_message(Some(message)); + self.finalize_turn(); + self.add_to_history(history_cell::new_error_event(message)); self.request_redraw(); // After an error ends the turn, try sending the next queued input. @@ -309,11 +305,13 @@ impl ChatWidget { /// separated by newlines rather than auto‑submitting the next one. fn on_interrupted_turn(&mut self, reason: TurnAbortReason) { // Finalize, log a gentle prompt, and clear running state. - self.finalize_turn_with_error_message(if reason == TurnAbortReason::ReviewEnded { - None - } else { - Some("Conversation interrupted - tell the model what to do differently".to_owned()) - }); + self.finalize_turn(); + + if reason != TurnAbortReason::ReviewEnded { + self.add_to_history(history_cell::new_error_event( + "Conversation interrupted - tell the model what to do differently".to_owned(), + )); + } // If any messages were queued during the task, restore them into the composer. if !self.queued_user_messages.is_empty() { @@ -1177,24 +1175,21 @@ impl ChatWidget { self.flush_active_exec_cell(); if output.findings.is_empty() { - // Show explanation or a fallback when there are no structured findings. - let mut lines: Vec> = - codex_core::review_format::format_review_findings_block(&[], None) - .lines() - .map(|s| ratatui::text::Line::from(s.to_string())) - .collect(); - lines.push("".into()); let explanation = output.overall_explanation.trim().to_string(); if explanation.is_empty() { - lines.push("Review failed -- no response found".into()); + tracing::error!("Reviewer failed to output a response."); + self.add_to_history(history_cell::new_error_event( + "Reviewer failed to output a response.".to_owned(), + )); } else { - for l in explanation.lines() { - lines.push(ratatui::text::Line::from(l.to_string())); - } + // Show explanation when there are no structured findings. + let body_cell = crate::history_cell::AgentMessageCell::new( + vec![format!("\n{explanation}").into()], + false, + ); + self.app_event_tx + .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } - let body_cell = crate::history_cell::AgentMessageCell::new(lines, false); - self.app_event_tx - .send(AppEvent::InsertHistoryCell(Box::new(body_cell))); } else { let message_text = codex_core::review_format::format_review_findings_block(&output.findings, None); @@ -1515,14 +1510,7 @@ impl ChatWidget { if text.is_empty() { return; } - - let user_message: UserMessage = text.into(); - if self.bottom_pane.is_task_running() { - self.queued_user_messages.push_back(user_message); - self.refresh_queued_user_messages(); - } else { - self.submit_user_message(user_message); - } + self.submit_user_message(text.into()); } pub(crate) fn token_usage(&self) -> TokenUsage { @@ -1560,10 +1548,10 @@ impl WidgetRef for &ChatWidget { if !active_cell_area.is_empty() && let Some(cell) = &self.active_exec_cell { - let mut area_to_render = active_cell_area; - area_to_render.y = area_to_render.y.saturating_add(1); - area_to_render.height = area_to_render.height.saturating_sub(1); - cell.render_ref(area_to_render, buf); + let mut active_cell_area = active_cell_area; + active_cell_area.y = active_cell_area.y.saturating_add(1); + active_cell_area.height -= 1; + cell.render_ref(active_cell_area, buf); } } } diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index 96dc14202a..ffe3f3f707 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -206,7 +206,7 @@ fn entered_review_mode_uses_request_hint() { let cells = drain_insert_history(&mut rx); let banner = lines_to_single_string(cells.last().expect("review banner")); - assert_eq!(banner, "\n>> Code review started: feature branch <<\n"); + assert_eq!(banner, ">> Code review started: feature branch <<\n"); assert!(chat.is_review_mode); } @@ -225,7 +225,7 @@ fn entered_review_mode_defaults_to_current_changes_banner() { let cells = drain_insert_history(&mut rx); let banner = lines_to_single_string(cells.last().expect("review banner")); - assert_eq!(banner, "\n>> Code review started: current changes <<\n"); + assert_eq!(banner, ">> Code review started: current changes <<\n"); assert!(chat.is_review_mode); } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index d5b6cd6090..40afaee98f 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -237,14 +237,7 @@ pub(crate) struct ReviewStatusHistoryCell { impl HistoryCell for ReviewStatusHistoryCell { fn display_lines(&self, _width: u16) -> Vec> { - // Add one empty line above the review status banner for visual padding - vec!["".into(), Line::from(self.message.clone().cyan())] - } - - fn is_stream_continuation(&self) -> bool { - // Treat status lines as part of the same history block to avoid an extra - // blank separator when following other content in the same turn. - true + vec![Line::from(self.message.clone().cyan())] } }