From bf82353f451dd71a1a9696fc595444fca04cc6d6 Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Thu, 9 Oct 2025 10:37:13 -0700 Subject: [PATCH] tui: fix wrapping in user approval decisions (#5008) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit before: Screenshot 2025-10-09 at 10 20 57 AM after: Screenshot 2025-10-09 at 10 20 35 AM --- .../tui/src/bottom_pane/approval_overlay.rs | 118 ++++---------- ...pproval_history_decision_aborted_long.snap | 4 +- codex-rs/tui/src/history_cell.rs | 150 +++++++++++++++++- codex-rs/tui/src/pager_overlay.rs | 8 +- 4 files changed, 184 insertions(+), 96 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 6585124c..d6e96cf7 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -16,7 +16,6 @@ use crate::key_hint::KeyBinding; use crate::render::highlight::highlight_bash_to_lines; use crate::render::renderable::ColumnRenderable; use crate::render::renderable::Renderable; -use crate::text_formatting::truncate_text; use codex_core::protocol::FileChange; use codex_core::protocol::Op; use codex_core::protocol::ReviewDecision; @@ -160,11 +159,8 @@ impl ApprovalOverlay { } fn handle_exec_decision(&self, id: &str, command: &[String], decision: ReviewDecision) { - if let Some(lines) = build_exec_history_lines(command.to_vec(), decision) { - self.app_event_tx.send(AppEvent::InsertHistoryCell(Box::new( - history_cell::new_user_approval_decision(lines), - ))); - } + let cell = history_cell::new_approval_decision_cell(command.to_vec(), decision); + self.app_event_tx.send(AppEvent::InsertHistoryCell(cell)); self.app_event_tx.send(AppEvent::CodexOp(Op::ExecApproval { id: id.to_string(), decision, @@ -396,91 +392,11 @@ fn patch_options() -> Vec { ] } -fn build_exec_history_lines( - command: Vec, - decision: ReviewDecision, -) -> Option>> { - use ReviewDecision::*; - - let (symbol, summary): (Span<'static>, Vec>) = match decision { - Approved => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( - "✔ ".green(), - vec![ - "You ".into(), - "approved".bold(), - " codex to run ".into(), - snippet, - " this time".bold(), - ], - ) - } - ApprovedForSession => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( - "✔ ".green(), - vec![ - "You ".into(), - "approved".bold(), - " codex to run ".into(), - snippet, - " every time this session".bold(), - ], - ) - } - Denied => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( - "✗ ".red(), - vec![ - "You ".into(), - "did not approve".bold(), - " codex to run ".into(), - snippet, - ], - ) - } - Abort => { - let snippet = Span::from(exec_snippet(&command)).dim(); - ( - "✗ ".red(), - vec![ - "You ".into(), - "canceled".bold(), - " the request to run ".into(), - snippet, - ], - ) - } - }; - - let mut lines = Vec::new(); - let mut spans = Vec::new(); - spans.push(symbol); - spans.extend(summary); - lines.push(Line::from(spans)); - Some(lines) -} - -fn truncate_exec_snippet(full_cmd: &str) -> String { - let mut snippet = match full_cmd.split_once('\n') { - Some((first, _)) => format!("{first} ..."), - None => full_cmd.to_string(), - }; - snippet = truncate_text(&snippet, 80); - snippet -} - -fn exec_snippet(command: &[String]) -> String { - let full_cmd = strip_bash_lc_and_escape(command); - truncate_exec_snippet(&full_cmd) -} - #[cfg(test)] mod tests { use super::*; use crate::app_event::AppEvent; + use pretty_assertions::assert_eq; use tokio::sync::mpsc::unbounded_channel; fn make_exec_request() -> ApprovalRequest { @@ -550,6 +466,34 @@ mod tests { ); } + #[test] + fn exec_history_cell_wraps_with_two_space_indent() { + let command = vec![ + "/bin/zsh".into(), + "-lc".into(), + "git add tui/src/render/mod.rs tui/src/render/renderable.rs".into(), + ]; + let cell = history_cell::new_approval_decision_cell(command, ReviewDecision::Approved); + let lines = cell.display_lines(28); + let rendered: Vec = lines + .iter() + .map(|line| { + line.spans + .iter() + .map(|span| span.content.as_ref()) + .collect::() + }) + .collect(); + let expected = vec![ + "✔ You approved codex to".to_string(), + " run /bin/zsh -lc 'git add".to_string(), + " tui/src/render/mod.rs tui/".to_string(), + " src/render/renderable.rs'".to_string(), + " this time".to_string(), + ]; + assert_eq!(rendered, expected); + } + #[test] fn enter_sets_last_selected_index_without_dismissing() { let (tx_raw, mut rx) = unbounded_channel::(); diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap index 524e0968..f04e1f07 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_decision_aborted_long.snap @@ -1,5 +1,7 @@ --- source: tui/src/chatwidget/tests.rs +assertion_line: 495 expression: lines_to_single_string(&aborted_long) --- -✗ You canceled the request to run echo aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa... +✗ You canceled the request to run echo + aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa... diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 151712b6..18a6c72d 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -6,12 +6,15 @@ use crate::exec_cell::TOOL_CALL_MAX_LINES; use crate::exec_cell::output_lines; use crate::exec_cell::spinner; use crate::exec_command::relativize_to_home; +use crate::exec_command::strip_bash_lc_and_escape; use crate::markdown::MarkdownCitationContext; use crate::markdown::append_markdown; use crate::render::line_utils::line_to_static; use crate::render::line_utils::prefix_lines; +use crate::render::line_utils::push_owned_lines; use crate::style::user_message_style; use crate::text_formatting::format_and_truncate_tool_result; +use crate::text_formatting::truncate_text; use crate::ui_consts::LIVE_PREFIX_COLS; use crate::wrapping::RtOptions; use crate::wrapping::word_wrap_line; @@ -262,6 +265,126 @@ impl HistoryCell for PlainHistoryCell { } } +#[derive(Debug)] +pub(crate) struct PrefixedWrappedHistoryCell { + text: Text<'static>, + initial_prefix: Line<'static>, + subsequent_prefix: Line<'static>, +} + +impl PrefixedWrappedHistoryCell { + pub(crate) fn new( + text: impl Into>, + initial_prefix: impl Into>, + subsequent_prefix: impl Into>, + ) -> Self { + Self { + text: text.into(), + initial_prefix: initial_prefix.into(), + subsequent_prefix: subsequent_prefix.into(), + } + } +} + +impl HistoryCell for PrefixedWrappedHistoryCell { + fn display_lines(&self, width: u16) -> Vec> { + if width == 0 { + return Vec::new(); + } + let opts = RtOptions::new(width.max(1) as usize) + .initial_indent(self.initial_prefix.clone()) + .subsequent_indent(self.subsequent_prefix.clone()); + let wrapped = word_wrap_lines(&self.text, opts); + let mut out = Vec::new(); + push_owned_lines(&wrapped, &mut out); + out + } + + fn desired_height(&self, width: u16) -> u16 { + self.display_lines(width).len() as u16 + } +} + +fn truncate_exec_snippet(full_cmd: &str) -> String { + let mut snippet = match full_cmd.split_once('\n') { + Some((first, _)) => format!("{first} ..."), + None => full_cmd.to_string(), + }; + snippet = truncate_text(&snippet, 80); + snippet +} + +fn exec_snippet(command: &[String]) -> String { + let full_cmd = strip_bash_lc_and_escape(command); + truncate_exec_snippet(&full_cmd) +} + +pub fn new_approval_decision_cell( + command: Vec, + decision: codex_core::protocol::ReviewDecision, +) -> Box { + use codex_core::protocol::ReviewDecision::*; + + let (symbol, summary): (Span<'static>, Vec>) = match decision { + Approved => { + let snippet = Span::from(exec_snippet(&command)).dim(); + ( + "✔ ".green(), + vec![ + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet, + " this time".bold(), + ], + ) + } + ApprovedForSession => { + let snippet = Span::from(exec_snippet(&command)).dim(); + ( + "✔ ".green(), + vec![ + "You ".into(), + "approved".bold(), + " codex to run ".into(), + snippet, + " every time this session".bold(), + ], + ) + } + Denied => { + let snippet = Span::from(exec_snippet(&command)).dim(); + ( + "✗ ".red(), + vec![ + "You ".into(), + "did not approve".bold(), + " codex to run ".into(), + snippet, + ], + ) + } + Abort => { + let snippet = Span::from(exec_snippet(&command)).dim(); + ( + "✗ ".red(), + vec![ + "You ".into(), + "canceled".bold(), + " the request to run ".into(), + snippet, + ], + ) + } + }; + + Box::new(PrefixedWrappedHistoryCell::new( + Line::from(summary), + symbol, + " ", + )) +} + /// Cyan history cell line showing the current review status. pub(crate) fn new_review_status_line(message: String) -> PlainHistoryCell { PlainHistoryCell { @@ -447,10 +570,6 @@ pub(crate) fn new_user_prompt(message: String) -> UserHistoryCell { UserHistoryCell { message } } -pub(crate) fn new_user_approval_decision(lines: Vec>) -> PlainHistoryCell { - PlainHistoryCell { lines } -} - #[derive(Debug)] struct SessionHeaderHistoryCell { version: &'static str, @@ -1203,6 +1322,29 @@ mod tests { assert_eq!(cell.desired_transcript_height(80), 1); } + #[test] + fn prefixed_wrapped_history_cell_indents_wrapped_lines() { + let summary = Line::from(vec![ + "You ".into(), + "approved".bold(), + " codex to run ".into(), + "echo something really long to ensure wrapping happens".dim(), + " this time".bold(), + ]); + let cell = PrefixedWrappedHistoryCell::new(summary, "✔ ".green(), " "); + let rendered = render_lines(&cell.display_lines(24)); + assert_eq!( + rendered, + vec![ + "✔ You approved codex".to_string(), + " to run echo something".to_string(), + " really long to ensure".to_string(), + " wrapping happens this".to_string(), + " time".to_string(), + ] + ); + } + #[test] fn active_mcp_tool_call_snapshot() { let invocation = McpInvocation { diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index 46c6c19a..9395fb71 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -582,6 +582,7 @@ fn render_offset_content( #[cfg(test)] mod tests { use super::*; + use codex_core::protocol::ReviewDecision; use insta::assert_snapshot; use std::collections::HashMap; use std::path::PathBuf; @@ -589,6 +590,7 @@ mod tests { use std::time::Duration; use crate::exec_cell::CommandOutput; + use crate::history_cell; use crate::history_cell::HistoryCell; use crate::history_cell::new_patch_event; use codex_core::protocol::FileChange; @@ -712,10 +714,8 @@ mod tests { cells.push(apply_begin_cell); let apply_end_cell: Arc = - Arc::new(crate::history_cell::new_user_approval_decision(vec![ - "✓ Patch applied".green().bold().into(), - "src/foo.txt".dim().into(), - ])); + history_cell::new_approval_decision_cell(vec!["ls".into()], ReviewDecision::Approved) + .into(); cells.push(apply_end_cell); let mut exec_cell = crate::exec_cell::new_active_exec_command(