From 07c1db351a27d3f3c07935f50d57f08576c5d26a Mon Sep 17 00:00:00 2001 From: Jeremy Rose <172423086+nornagon-openai@users.noreply.github.com> Date: Wed, 1 Oct 2025 14:29:05 -0700 Subject: [PATCH] rework patch/exec approval UI (#4573) | Scenario | Screenshot | | ---------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | | short patch | short patch | | short command | short command | | long patch | long patch | | long command | long command | | viewing complete patch | viewing complete patch | --- codex-rs/tui/src/app.rs | 37 +- codex-rs/tui/src/app_event.rs | 4 + .../tui/src/bottom_pane/approval_overlay.rs | 218 ++++---- .../tui/src/bottom_pane/bottom_pane_view.rs | 10 +- codex-rs/tui/src/bottom_pane/command_popup.rs | 5 +- .../tui/src/bottom_pane/custom_prompt_view.rs | 60 +-- .../tui/src/bottom_pane/file_search_popup.rs | 6 +- .../src/bottom_pane/list_selection_view.rs | 296 +++++------ .../src/bottom_pane/selection_popup_common.rs | 97 +--- ..._list_selection_spacing_with_subtitle.snap | 13 +- ...st_selection_spacing_without_subtitle.snap | 11 +- codex-rs/tui/src/chatwidget.rs | 26 +- ...ly_patch_manual_flow_history_approved.snap | 3 +- ...ly_patch_manual_flow_history_proposed.snap | 6 - ...hatwidget__tests__approval_modal_exec.snap | 28 +- ..._tests__approval_modal_exec_no_reason.snap | 19 +- ...atwidget__tests__approval_modal_patch.snap | 21 +- ...c_approval_history_proposed_multiline.snap | 7 - ..._exec_approval_history_proposed_short.snap | 6 - ...dget__tests__exec_approval_modal_exec.snap | 42 ++ ...sts__status_widget_and_approval_modal.snap | 23 +- codex-rs/tui/src/chatwidget/tests.rs | 232 ++++----- codex-rs/tui/src/diff_render.rs | 471 ++++++------------ codex-rs/tui/src/history_cell.rs | 38 +- codex-rs/tui/src/pager_overlay.rs | 420 +++++++++------- codex-rs/tui/src/render/mod.rs | 45 ++ codex-rs/tui/src/render/renderable.rs | 102 ++++ ...der__tests__apply_update_block_manual.snap | 10 +- ...ript_overlay_apply_patch_scroll_vt100.snap | 9 +- codex-rs/tui/src/status_indicator_widget.rs | 3 +- 30 files changed, 1127 insertions(+), 1141 deletions(-) delete mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_proposed.snap delete mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap delete mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap create mode 100644 codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap create mode 100644 codex-rs/tui/src/render/renderable.rs diff --git a/codex-rs/tui/src/app.rs b/codex-rs/tui/src/app.rs index 2d840a8c..383e5cae 100644 --- a/codex-rs/tui/src/app.rs +++ b/codex-rs/tui/src/app.rs @@ -1,10 +1,14 @@ use crate::app_backtrack::BacktrackState; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; +use crate::bottom_pane::ApprovalRequest; use crate::chatwidget::ChatWidget; +use crate::diff_render::DiffSummary; +use crate::exec_command::strip_bash_lc_and_escape; use crate::file_search::FileSearchManager; use crate::history_cell::HistoryCell; use crate::pager_overlay::Overlay; +use crate::render::highlight::highlight_bash_to_lines; use crate::resume_picker::ResumeSelection; use crate::tui; use crate::tui::TuiEvent; @@ -292,7 +296,7 @@ impl App { } else { text.lines().map(ansi_escape_line).collect() }; - self.overlay = Some(Overlay::new_static_with_title( + self.overlay = Some(Overlay::new_static_with_lines( pager_lines, "D I F F".to_string(), )); @@ -324,12 +328,18 @@ impl App { Ok(()) => { if let Some(profile) = profile { self.chat_widget.add_info_message( - format!("Model changed to {model} for {profile} profile"), + format!("Model changed to {model}{reasoning_effort} for {profile} profile", reasoning_effort = effort.map(|e| format!(" {e}")).unwrap_or_default()), None, ); } else { - self.chat_widget - .add_info_message(format!("Model changed to {model}"), None); + self.chat_widget.add_info_message( + format!( + "Model changed to {model}{reasoning_effort}", + reasoning_effort = + effort.map(|e| format!(" {e}")).unwrap_or_default() + ), + None, + ); } } Err(err) => { @@ -363,6 +373,25 @@ impl App { AppEvent::OpenReviewCustomPrompt => { self.chat_widget.show_review_custom_prompt(); } + AppEvent::FullScreenApprovalRequest(request) => match request { + ApprovalRequest::ApplyPatch { cwd, changes, .. } => { + let _ = tui.enter_alt_screen(); + let diff_summary = DiffSummary::new(changes, cwd); + self.overlay = Some(Overlay::new_static_with_renderables( + vec![diff_summary.into()], + "P A T C H".to_string(), + )); + } + ApprovalRequest::Exec { command, .. } => { + let _ = tui.enter_alt_screen(); + let full_cmd = strip_bash_lc_and_escape(&command); + let full_cmd_lines = highlight_bash_to_lines(&full_cmd); + self.overlay = Some(Overlay::new_static_with_lines( + full_cmd_lines, + "E X E C".to_string(), + )); + } + }, } Ok(true) } diff --git a/codex-rs/tui/src/app_event.rs b/codex-rs/tui/src/app_event.rs index 56c66379..9c8a71de 100644 --- a/codex-rs/tui/src/app_event.rs +++ b/codex-rs/tui/src/app_event.rs @@ -4,6 +4,7 @@ use codex_core::protocol::ConversationPathResponseEvent; use codex_core::protocol::Event; use codex_file_search::FileMatch; +use crate::bottom_pane::ApprovalRequest; use crate::history_cell::HistoryCell; use codex_core::protocol::AskForApproval; @@ -76,4 +77,7 @@ pub(crate) enum AppEvent { /// Open the custom prompt option from the review popup. OpenReviewCustomPrompt, + + /// Open the approval popup. + FullScreenApprovalRequest(ApprovalRequest), } diff --git a/codex-rs/tui/src/bottom_pane/approval_overlay.rs b/codex-rs/tui/src/bottom_pane/approval_overlay.rs index 7ab2b79a..511039df 100644 --- a/codex-rs/tui/src/bottom_pane/approval_overlay.rs +++ b/codex-rs/tui/src/bottom_pane/approval_overlay.rs @@ -1,16 +1,21 @@ +use std::collections::HashMap; use std::path::PathBuf; use crate::app_event::AppEvent; use crate::app_event_sender::AppEventSender; use crate::bottom_pane::BottomPaneView; use crate::bottom_pane::CancellationEvent; -use crate::bottom_pane::list_selection_view::HeaderLine; use crate::bottom_pane::list_selection_view::ListSelectionView; use crate::bottom_pane::list_selection_view::SelectionItem; use crate::bottom_pane::list_selection_view::SelectionViewParams; +use crate::diff_render::DiffSummary; use crate::exec_command::strip_bash_lc_and_escape; use crate::history_cell; +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; use crossterm::event::KeyCode; @@ -22,8 +27,11 @@ use ratatui::layout::Rect; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; +use ratatui::widgets::Paragraph; +use ratatui::widgets::Wrap; /// Request coming from the agent that needs user approval. +#[derive(Clone, Debug)] pub(crate) enum ApprovalRequest { Exec { id: String, @@ -33,13 +41,15 @@ pub(crate) enum ApprovalRequest { ApplyPatch { id: String, reason: Option, - grant_root: Option, + cwd: PathBuf, + changes: HashMap, }, } /// Modal overlay asking the user to approve or deny one or more requests. pub(crate) struct ApprovalOverlay { - current: Option, + current_request: Option, + current_variant: Option, queue: Vec, app_event_tx: AppEventSender, list: ListSelectionView, @@ -51,23 +61,16 @@ pub(crate) struct ApprovalOverlay { impl ApprovalOverlay { pub fn new(request: ApprovalRequest, app_event_tx: AppEventSender) -> Self { let mut view = Self { - current: Some(ApprovalRequestState::from(request)), + current_request: None, + current_variant: None, queue: Vec::new(), app_event_tx: app_event_tx.clone(), - list: ListSelectionView::new( - SelectionViewParams { - title: String::new(), - ..Default::default() - }, - app_event_tx, - ), + list: ListSelectionView::new(Default::default(), app_event_tx), options: Vec::new(), current_complete: false, done: false, }; - let (options, params) = view.build_options(); - view.options = options; - view.list = ListSelectionView::new(params, view.app_event_tx.clone()); + view.set_current(request); view } @@ -76,28 +79,30 @@ impl ApprovalOverlay { } fn set_current(&mut self, request: ApprovalRequest) { - self.current = Some(ApprovalRequestState::from(request)); + self.current_request = Some(request.clone()); + let ApprovalRequestState { variant, header } = ApprovalRequestState::from(request); + self.current_variant = Some(variant.clone()); self.current_complete = false; - let (options, params) = self.build_options(); + let (options, params) = Self::build_options(variant, header); self.options = options; self.list = ListSelectionView::new(params, self.app_event_tx.clone()); } - fn build_options(&self) -> (Vec, SelectionViewParams) { - let Some(state) = self.current.as_ref() else { - return ( - Vec::new(), - SelectionViewParams { - title: String::new(), - ..Default::default() - }, - ); - }; - let (options, title) = match &state.variant { + fn build_options( + variant: ApprovalVariant, + header: Box, + ) -> (Vec, SelectionViewParams) { + let (options, title) = match &variant { ApprovalVariant::Exec { .. } => (exec_options(), "Allow command?".to_string()), ApprovalVariant::ApplyPatch { .. } => (patch_options(), "Apply changes?".to_string()), }; + let header = Box::new(ColumnRenderable::new([ + Box::new(Line::from(title.bold())), + Box::new(Line::from("")), + header, + ])); + let items = options .iter() .map(|opt| SelectionItem { @@ -111,10 +116,9 @@ impl ApprovalOverlay { .collect(); let params = SelectionViewParams { - title, footer_hint: Some("Press Enter to confirm or Esc to cancel".to_string()), items, - header: state.header.clone(), + header, ..Default::default() }; @@ -128,8 +132,8 @@ impl ApprovalOverlay { let Some(option) = self.options.get(actual_idx) else { return; }; - if let Some(state) = self.current.as_ref() { - match (&state.variant, option.decision) { + if let Some(variant) = self.current_variant.as_ref() { + match (&variant, option.decision) { (ApprovalVariant::Exec { id, command }, decision) => { self.handle_exec_decision(id, command, decision); } @@ -171,30 +175,43 @@ impl ApprovalOverlay { } fn try_handle_shortcut(&mut self, key_event: &KeyEvent) -> bool { - if key_event.kind != KeyEventKind::Press { - return false; - } - let KeyEvent { - code: KeyCode::Char(c), - modifiers, - .. - } = key_event - else { - return false; - }; - if modifiers.contains(KeyModifiers::CONTROL) || modifiers.contains(KeyModifiers::ALT) { - return false; - } - let lower = c.to_ascii_lowercase(); - if let Some(idx) = self - .options - .iter() - .position(|opt| opt.shortcut.map(|s| s == lower).unwrap_or(false)) - { - self.apply_selection(idx); - true - } else { - false + match key_event { + KeyEvent { + kind: KeyEventKind::Press, + code: KeyCode::Char('a'), + modifiers, + .. + } if modifiers.contains(KeyModifiers::CONTROL) => { + if let Some(request) = self.current_request.as_ref() { + self.app_event_tx + .send(AppEvent::FullScreenApprovalRequest(request.clone())); + true + } else { + false + } + } + KeyEvent { + kind: KeyEventKind::Press, + code: KeyCode::Char(c), + modifiers, + .. + } if !modifiers.contains(KeyModifiers::CONTROL) + && !modifiers.contains(KeyModifiers::ALT) => + { + let lower = c.to_ascii_lowercase(); + match self + .options + .iter() + .position(|opt| opt.shortcut.map(|s| s == lower).unwrap_or(false)) + { + Some(idx) => { + self.apply_selection(idx); + true + } + None => false, + } + } + _ => false, } } } @@ -215,9 +232,9 @@ impl BottomPaneView for ApprovalOverlay { return CancellationEvent::Handled; } if !self.current_complete - && let Some(state) = self.current.as_ref() + && let Some(variant) = self.current_variant.as_ref() { - match &state.variant { + match &variant { ApprovalVariant::Exec { id, command } => { self.handle_exec_decision(id, command, ReviewDecision::Abort); } @@ -235,14 +252,6 @@ impl BottomPaneView for ApprovalOverlay { self.done } - fn desired_height(&self, width: u16) -> u16 { - self.list.desired_height(width) - } - - fn render(&self, area: Rect, buf: &mut Buffer) { - self.list.render(area, buf); - } - fn try_consume_approval_request( &mut self, request: ApprovalRequest, @@ -256,9 +265,19 @@ impl BottomPaneView for ApprovalOverlay { } } +impl Renderable for ApprovalOverlay { + fn desired_height(&self, width: u16) -> u16 { + self.list.desired_height(width) + } + + fn render(&self, area: Rect, buf: &mut Buffer) { + self.list.render(area, buf); + } +} + struct ApprovalRequestState { variant: ApprovalVariant, - header: Vec, + header: Box, } impl From for ApprovalRequestState { @@ -269,63 +288,50 @@ impl From for ApprovalRequestState { command, reason, } => { - let mut header = Vec::new(); + let mut header: Vec> = Vec::new(); if let Some(reason) = reason && !reason.is_empty() { - header.push(HeaderLine::Text { - text: reason, - italic: true, - }); - header.push(HeaderLine::Spacer); + header.push(reason.italic().into()); + header.push(Line::from("")); } - let command_snippet = exec_snippet(&command); - if !command_snippet.is_empty() { - header.push(HeaderLine::Text { - text: format!("Command: {command_snippet}"), - italic: false, - }); - header.push(HeaderLine::Spacer); + let full_cmd = strip_bash_lc_and_escape(&command); + let mut full_cmd_lines = highlight_bash_to_lines(&full_cmd); + if let Some(first) = full_cmd_lines.first_mut() { + first.spans.insert(0, Span::from("$ ")); } + header.extend(full_cmd_lines); Self { variant: ApprovalVariant::Exec { id, command }, - header, + header: Box::new(Paragraph::new(header).wrap(Wrap { trim: false })), } } ApprovalRequest::ApplyPatch { id, reason, - grant_root, + cwd, + changes, } => { - let mut header = Vec::new(); + let mut header: Vec> = Vec::new(); + header.push(DiffSummary::new(changes, cwd).into()); if let Some(reason) = reason && !reason.is_empty() { - header.push(HeaderLine::Text { - text: reason, - italic: true, - }); - header.push(HeaderLine::Spacer); - } - if let Some(root) = grant_root { - header.push(HeaderLine::Text { - text: format!( - "Grant write access to {} for the remainder of this session.", - root.display() - ), - italic: false, - }); - header.push(HeaderLine::Spacer); + header.push(Box::new(Line::from(""))); + header.push(Box::new( + Paragraph::new(reason.italic()).wrap(Wrap { trim: false }), + )); } Self { variant: ApprovalVariant::ApplyPatch { id }, - header, + header: Box::new(ColumnRenderable::new(header)), } } } } } +#[derive(Clone)] enum ApprovalVariant { Exec { id: String, command: Vec }, ApplyPatch { id: String }, @@ -343,20 +349,20 @@ fn exec_options() -> Vec { vec![ ApprovalOption { label: "Approve and run now".to_string(), - description: "(Y) Run this command one time".to_string(), + description: "Run this command one time".to_string(), decision: ReviewDecision::Approved, shortcut: Some('y'), }, ApprovalOption { label: "Always approve this session".to_string(), - description: "(A) Automatically approve this command for the rest of the session" + description: "Automatically approve this command for the rest of the session" .to_string(), decision: ReviewDecision::ApprovedForSession, shortcut: Some('a'), }, ApprovalOption { label: "Cancel".to_string(), - description: "(N) Do not run the command".to_string(), + description: "Do not run the command".to_string(), decision: ReviewDecision::Abort, shortcut: Some('n'), }, @@ -367,13 +373,13 @@ fn patch_options() -> Vec { vec![ ApprovalOption { label: "Approve".to_string(), - description: "(Y) Apply the proposed changes".to_string(), + description: "Apply the proposed changes".to_string(), decision: ReviewDecision::Approved, shortcut: Some('y'), }, ApprovalOption { label: "Cancel".to_string(), - description: "(N) Do not apply the changes".to_string(), + description: "Do not apply the changes".to_string(), decision: ReviewDecision::Abort, shortcut: Some('n'), }, @@ -516,8 +522,8 @@ mod tests { }; let view = ApprovalOverlay::new(exec_request, tx); - let mut buf = Buffer::empty(Rect::new(0, 0, 80, 6)); - view.render(Rect::new(0, 0, 80, 6), &mut buf); + let mut buf = Buffer::empty(Rect::new(0, 0, 80, view.desired_height(80))); + view.render(Rect::new(0, 0, 80, view.desired_height(80)), &mut buf); let rendered: Vec = (0..buf.area.height) .map(|row| { @@ -529,7 +535,7 @@ mod tests { assert!( rendered .iter() - .any(|line| line.contains("Command: echo hello world")), + .any(|line| line.contains("echo hello world")), "expected header to include command snippet, got {rendered:?}" ); } diff --git a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs index 8f569ec8..0271a7e1 100644 --- a/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs +++ b/codex-rs/tui/src/bottom_pane/bottom_pane_view.rs @@ -1,12 +1,12 @@ use crate::bottom_pane::ApprovalRequest; +use crate::render::renderable::Renderable; use crossterm::event::KeyEvent; -use ratatui::buffer::Buffer; use ratatui::layout::Rect; use super::CancellationEvent; /// Trait implemented by every view that can be shown in the bottom pane. -pub(crate) trait BottomPaneView { +pub(crate) trait BottomPaneView: Renderable { /// Handle a key event while the view is active. A redraw is always /// scheduled after this call. fn handle_key_event(&mut self, _key_event: KeyEvent) {} @@ -21,12 +21,6 @@ pub(crate) trait BottomPaneView { CancellationEvent::NotHandled } - /// Return the desired height of the view. - fn desired_height(&self, width: u16) -> u16; - - /// Render the view: this will be displayed in place of the composer. - fn render(&self, area: Rect, buf: &mut Buffer); - /// Optional paste handler. Return true if the view modified its state and /// needs a redraw. fn handle_paste(&mut self, _pasted: String) -> bool { diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index c4939582..b597f9ec 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -6,6 +6,8 @@ use super::popup_consts::MAX_POPUP_ROWS; use super::scroll_state::ScrollState; use super::selection_popup_common::GenericDisplayRow; use super::selection_popup_common::render_rows; +use crate::render::Insets; +use crate::render::RectExt; use crate::slash_command::SlashCommand; use crate::slash_command::built_in_slash_commands; use codex_common::fuzzy_match::fuzzy_match; @@ -205,13 +207,12 @@ impl WidgetRef for CommandPopup { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let rows = self.rows_from_matches(self.filtered()); render_rows( - area, + area.inset(Insets::tlbr(0, 2, 0, 0)), buf, &rows, &self.state, MAX_POPUP_ROWS, "no matches", - false, ); } } diff --git a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs index a8084d0d..07bd9203 100644 --- a/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs +++ b/codex-rs/tui/src/bottom_pane/custom_prompt_view.rs @@ -12,6 +12,8 @@ use ratatui::widgets::StatefulWidgetRef; use ratatui::widgets::Widget; use std::cell::RefCell; +use crate::render::renderable::Renderable; + use super::popup_consts::STANDARD_POPUP_HINT_LINE; use super::CancellationEvent; @@ -94,6 +96,36 @@ impl BottomPaneView for CustomPromptView { self.complete } + fn handle_paste(&mut self, pasted: String) -> bool { + if pasted.is_empty() { + return false; + } + self.textarea.insert_str(&pasted); + true + } + + fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { + if area.height < 2 || area.width <= 2 { + return None; + } + let text_area_height = self.input_height(area.width).saturating_sub(1); + if text_area_height == 0 { + return None; + } + let extra_offset: u16 = if self.context_label.is_some() { 1 } else { 0 }; + let top_line_count = 1u16 + extra_offset; + let textarea_rect = Rect { + x: area.x.saturating_add(2), + y: area.y.saturating_add(top_line_count).saturating_add(1), + width: area.width.saturating_sub(2), + height: text_area_height, + }; + let state = *self.textarea_state.borrow(); + self.textarea.cursor_pos_with_state(textarea_rect, state) + } +} + +impl Renderable for CustomPromptView { fn desired_height(&self, width: u16) -> u16 { let extra_top: u16 = if self.context_label.is_some() { 1 } else { 0 }; 1u16 + extra_top + self.input_height(width) + 3u16 @@ -200,34 +232,6 @@ impl BottomPaneView for CustomPromptView { ); } } - - fn handle_paste(&mut self, pasted: String) -> bool { - if pasted.is_empty() { - return false; - } - self.textarea.insert_str(&pasted); - true - } - - fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { - if area.height < 2 || area.width <= 2 { - return None; - } - let text_area_height = self.input_height(area.width).saturating_sub(1); - if text_area_height == 0 { - return None; - } - let extra_offset: u16 = if self.context_label.is_some() { 1 } else { 0 }; - let top_line_count = 1u16 + extra_offset; - let textarea_rect = Rect { - x: area.x.saturating_add(2), - y: area.y.saturating_add(top_line_count).saturating_add(1), - width: area.width.saturating_sub(2), - height: text_area_height, - }; - let state = *self.textarea_state.borrow(); - self.textarea.cursor_pos_with_state(textarea_rect, state) - } } impl CustomPromptView { diff --git a/codex-rs/tui/src/bottom_pane/file_search_popup.rs b/codex-rs/tui/src/bottom_pane/file_search_popup.rs index e017b504..6ba8599d 100644 --- a/codex-rs/tui/src/bottom_pane/file_search_popup.rs +++ b/codex-rs/tui/src/bottom_pane/file_search_popup.rs @@ -3,6 +3,9 @@ use ratatui::buffer::Buffer; use ratatui::layout::Rect; use ratatui::widgets::WidgetRef; +use crate::render::Insets; +use crate::render::RectExt; + use super::popup_consts::MAX_POPUP_ROWS; use super::scroll_state::ScrollState; use super::selection_popup_common::GenericDisplayRow; @@ -139,13 +142,12 @@ impl WidgetRef for &FileSearchPopup { }; render_rows( - area, + area.inset(Insets::tlbr(0, 2, 0, 0)), buf, &rows_all, &self.state, MAX_POPUP_ROWS, empty_message, - false, ); } } diff --git a/codex-rs/tui/src/bottom_pane/list_selection_view.rs b/codex-rs/tui/src/bottom_pane/list_selection_view.rs index b20d7476..e2614885 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -2,15 +2,23 @@ use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyModifiers; use ratatui::buffer::Buffer; +use ratatui::layout::Constraint; +use ratatui::layout::Layout; use ratatui::layout::Rect; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; +use ratatui::widgets::Block; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; -use textwrap::wrap; use crate::app_event_sender::AppEventSender; +use crate::render::Insets; +use crate::render::RectExt as _; +use crate::render::renderable::ColumnRenderable; +use crate::render::renderable::Renderable; +use crate::style::user_message_style; +use crate::terminal_palette; use super::CancellationEvent; use super::bottom_pane_view::BottomPaneView; @@ -23,12 +31,6 @@ use super::selection_popup_common::render_rows; /// One selectable item in the generic selection list. pub(crate) type SelectionAction = Box; -#[derive(Clone, Debug, PartialEq, Eq)] -pub(crate) enum HeaderLine { - Text { text: String, italic: bool }, - Spacer, -} - pub(crate) struct SelectionItem { pub name: String, pub description: Option, @@ -38,20 +40,31 @@ pub(crate) struct SelectionItem { pub search_value: Option, } -#[derive(Default)] pub(crate) struct SelectionViewParams { - pub title: String, + pub title: Option, pub subtitle: Option, pub footer_hint: Option, pub items: Vec, pub is_searchable: bool, pub search_placeholder: Option, - pub header: Vec, + pub header: Box, +} + +impl Default for SelectionViewParams { + fn default() -> Self { + Self { + title: None, + subtitle: None, + footer_hint: None, + items: Vec::new(), + is_searchable: false, + search_placeholder: None, + header: Box::new(()), + } + } } pub(crate) struct ListSelectionView { - title: String, - subtitle: Option, footer_hint: Option, items: Vec, state: ScrollState, @@ -62,23 +75,22 @@ pub(crate) struct ListSelectionView { search_placeholder: Option, filtered_indices: Vec, last_selected_actual_idx: Option, - header: Vec, + header: Box, } impl ListSelectionView { - fn dim_prefix_span() -> Span<'static> { - "▌ ".dim() - } - - fn render_dim_prefix_line(area: Rect, buf: &mut Buffer) { - let para = Paragraph::new(Line::from(Self::dim_prefix_span())); - para.render(area, buf); - } - pub fn new(params: SelectionViewParams, app_event_tx: AppEventSender) -> Self { + let mut header = params.header; + if params.title.is_some() || params.subtitle.is_some() { + let title = params.title.map(|title| Line::from(title.bold())); + let subtitle = params.subtitle.map(|subtitle| Line::from(subtitle.dim())); + header = Box::new(ColumnRenderable::new([ + header, + Box::new(title), + Box::new(subtitle), + ])); + } let mut s = Self { - title: params.title, - subtitle: params.subtitle, footer_hint: params.footer_hint, items: params.items, state: ScrollState::new(), @@ -93,7 +105,7 @@ impl ListSelectionView { }, filtered_indices: Vec::new(), last_selected_actual_idx: None, - header: params.header, + header, }; s.apply_filter(); s @@ -171,7 +183,7 @@ impl ListSelectionView { .filter_map(|(visible_idx, actual_idx)| { self.items.get(*actual_idx).map(|item| { let is_selected = self.state.selected_idx == Some(visible_idx); - let prefix = if is_selected { '>' } else { ' ' }; + let prefix = if is_selected { '›' } else { ' ' }; let name = item.name.as_str(); let name_with_marker = if item.is_current { format!("{name} (current)") @@ -179,7 +191,13 @@ impl ListSelectionView { item.name.clone() }; let n = visible_idx + 1; - let display_name = format!("{prefix} {n}. {name_with_marker}"); + let display_name = if self.is_searchable { + // The number keys don't work when search is enabled (since we let the + // numbers be used for the search query). + format!("{prefix} {name_with_marker}") + } else { + format!("{prefix} {n}. {name_with_marker}") + }; GenericDisplayRow { name: display_name, match_indices: None, @@ -231,39 +249,6 @@ impl ListSelectionView { pub(crate) fn take_last_selected_index(&mut self) -> Option { self.last_selected_actual_idx.take() } - - fn header_spans_for_width(&self, width: u16) -> Vec>> { - if self.header.is_empty() || width == 0 { - return Vec::new(); - } - let prefix_width = Self::dim_prefix_span().width() as u16; - let available = width.saturating_sub(prefix_width).max(1) as usize; - let mut lines = Vec::new(); - for entry in &self.header { - match entry { - HeaderLine::Spacer => lines.push(Vec::new()), - HeaderLine::Text { text, italic } => { - if text.is_empty() { - lines.push(Vec::new()); - continue; - } - for part in wrap(text, available) { - let span = if *italic { - Span::from(part.into_owned()).italic() - } else { - Span::from(part.into_owned()) - }; - lines.push(vec![span]); - } - } - } - } - lines - } - - fn header_height(&self, width: u16) -> u16 { - self.header_spans_for_width(width).len() as u16 - } } impl BottomPaneView for ListSelectionView { @@ -299,6 +284,24 @@ impl BottomPaneView for ListSelectionView { self.search_query.push(c); self.apply_filter(); } + KeyEvent { + code: KeyCode::Char(c), + modifiers, + .. + } if !self.is_searchable + && !modifiers.contains(KeyModifiers::CONTROL) + && !modifiers.contains(KeyModifiers::ALT) => + { + if let Some(idx) = c + .to_digit(10) + .map(|d| d as usize) + .and_then(|d| d.checked_sub(1)) + && idx < self.items.len() + { + self.state.selected_idx = Some(idx); + self.accept(); + } + } KeyEvent { code: KeyCode::Enter, modifiers: KeyModifiers::NONE, @@ -316,7 +319,9 @@ impl BottomPaneView for ListSelectionView { self.complete = true; CancellationEvent::Handled } +} +impl Renderable for ListSelectionView { fn desired_height(&self, width: u16) -> u16 { // Measure wrapped height for up to MAX_POPUP_ROWS items at the given width. // Build the same display rows used by the renderer so wrapping math matches. @@ -324,19 +329,13 @@ impl BottomPaneView for ListSelectionView { let rows_height = measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, width); - // +1 for the title row, +1 for a spacer line beneath the header, - // +1 for optional subtitle, +1 for optional footer (2 lines incl. spacing) - let mut height = self.header_height(width); - height = height.saturating_add(rows_height + 2); + let mut height = self.header.desired_height(width); + height = height.saturating_add(rows_height + 3); if self.is_searchable { height = height.saturating_add(1); } - if self.subtitle.is_some() { - // +1 for subtitle (the spacer is accounted for above) - height = height.saturating_add(1); - } if self.footer_hint.is_some() { - height = height.saturating_add(2); + height = height.saturating_add(1); } height } @@ -346,52 +345,42 @@ impl BottomPaneView for ListSelectionView { return; } - let mut next_y = area.y; - let header_spans = self.header_spans_for_width(area.width); - for spans in header_spans.into_iter() { - if next_y >= area.y + area.height { - return; - } - let row = Rect { - x: area.x, - y: next_y, - width: area.width, - height: 1, - }; - let mut prefixed: Vec> = vec![Self::dim_prefix_span()]; - if spans.is_empty() { - prefixed.push(String::new().into()); - } else { - prefixed.extend(spans); - } - Paragraph::new(Line::from(prefixed)).render(row, buf); - next_y = next_y.saturating_add(1); + let [content_area, footer_area] = Layout::vertical([ + Constraint::Fill(1), + Constraint::Length(if self.footer_hint.is_some() { 1 } else { 0 }), + ]) + .areas(area); + + Block::default() + .style(user_message_style(terminal_palette::default_bg())) + .render(content_area, buf); + + let header_height = self.header.desired_height(content_area.width); + let rows = self.build_rows(); + let rows_height = + measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, content_area.width); + let [header_area, _, search_area, list_area] = Layout::vertical([ + Constraint::Max(header_height), + Constraint::Max(1), + Constraint::Length(if self.is_searchable { 1 } else { 0 }), + Constraint::Length(rows_height), + ]) + .areas(content_area.inset(Insets::vh(1, 2))); + + if header_area.height < header_height { + let [header_area, elision_area] = + Layout::vertical([Constraint::Fill(1), Constraint::Length(1)]).areas(header_area); + self.header.render(header_area, buf); + Paragraph::new(vec![ + Line::from(format!("[… {header_height} lines] ctrl + a view all")).dim(), + ]) + .render(elision_area, buf); + } else { + self.header.render(header_area, buf); } - if next_y >= area.y + area.height { - return; - } - - let title_area = Rect { - x: area.x, - y: next_y, - width: area.width, - height: 1, - }; - Paragraph::new(Line::from(vec![ - Self::dim_prefix_span(), - self.title.clone().bold(), - ])) - .render(title_area, buf); - next_y = next_y.saturating_add(1); - - if self.is_searchable && next_y < area.y + area.height { - let search_area = Rect { - x: area.x, - y: next_y, - width: area.width, - height: 1, - }; + if self.is_searchable { + Line::from(self.search_query.clone()).render(search_area, buf); let query_span: Span<'static> = if self.search_query.is_empty() { self.search_placeholder .as_ref() @@ -400,80 +389,40 @@ impl BottomPaneView for ListSelectionView { } else { self.search_query.clone().into() }; - Paragraph::new(Line::from(vec![Self::dim_prefix_span(), query_span])) - .render(search_area, buf); - next_y = next_y.saturating_add(1); + Line::from(query_span).render(search_area, buf); } - if let Some(sub) = &self.subtitle { - if next_y >= area.y + area.height { - return; - } - let subtitle_area = Rect { - x: area.x, - y: next_y, - width: area.width, - height: 1, + if list_area.height > 0 { + let list_area = Rect { + x: list_area.x - 2, + y: list_area.y, + width: list_area.width + 2, + height: list_area.height, }; - Paragraph::new(Line::from(vec![Self::dim_prefix_span(), sub.clone().dim()])) - .render(subtitle_area, buf); - next_y = next_y.saturating_add(1); - } - - if next_y >= area.y + area.height { - return; - } - let spacer_area = Rect { - x: area.x, - y: next_y, - width: area.width, - height: 1, - }; - Self::render_dim_prefix_line(spacer_area, buf); - next_y = next_y.saturating_add(1); - - let footer_reserved = if self.footer_hint.is_some() { 2 } else { 0 }; - if next_y >= area.y + area.height { - return; - } - let rows_area = Rect { - x: area.x, - y: next_y, - width: area.width, - height: area - .height - .saturating_sub(next_y.saturating_sub(area.y)) - .saturating_sub(footer_reserved), - }; - - let rows = self.build_rows(); - if rows_area.height > 0 { render_rows( - rows_area, + list_area, buf, &rows, &self.state, - MAX_POPUP_ROWS, + list_area.height as usize, "no matches", - true, ); } if let Some(hint) = &self.footer_hint { - let footer_area = Rect { - x: area.x, - y: area.y + area.height - 1, - width: area.width, - height: 1, + let hint_area = Rect { + x: footer_area.x + 2, + y: footer_area.y, + width: footer_area.width.saturating_sub(2), + height: footer_area.height, }; - Paragraph::new(hint.clone().dim()).render(footer_area, buf); + Line::from(hint.clone().dim()).render(hint_area, buf); } } } #[cfg(test)] mod tests { - use super::BottomPaneView; use super::*; use crate::app_event::AppEvent; use crate::bottom_pane::popup_consts::STANDARD_POPUP_HINT_LINE; @@ -504,7 +453,7 @@ mod tests { ]; ListSelectionView::new( SelectionViewParams { - title: "Select Approval Mode".to_string(), + title: Some("Select Approval Mode".to_string()), subtitle: subtitle.map(str::to_string), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, @@ -516,7 +465,7 @@ mod tests { fn render_lines(view: &ListSelectionView) -> String { let width = 48; - let height = BottomPaneView::desired_height(view, width); + let height = view.desired_height(width); let area = Rect::new(0, 0, width, height); let mut buf = Buffer::empty(area); view.render(area, &mut buf); @@ -567,7 +516,7 @@ mod tests { }]; let mut view = ListSelectionView::new( SelectionViewParams { - title: "Select Approval Mode".to_string(), + title: Some("Select Approval Mode".to_string()), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, is_searchable: true, @@ -579,6 +528,9 @@ mod tests { view.set_search_query("filters".to_string()); let lines = render_lines(&view); - assert!(lines.contains("▌ filters")); + assert!( + lines.contains("filters"), + "expected search query line to include rendered query, got {lines:?}" + ); } } diff --git a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs index f3b23801..fd523532 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -3,17 +3,13 @@ use ratatui::layout::Rect; // Note: Table-based layout previously used Constraint; the manual renderer // below no longer requires it. use ratatui::style::Color; -use ratatui::style::Modifier; -use ratatui::style::Style; use ratatui::style::Stylize; use ratatui::text::Line; use ratatui::text::Span; -use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use unicode_width::UnicodeWidthChar; use super::scroll_state::ScrollState; -use crate::ui_consts::LIVE_PREFIX_COLS; /// A generic representation of a display row for selection popups. pub(crate) struct GenericDisplayRow { @@ -23,8 +19,6 @@ pub(crate) struct GenericDisplayRow { pub description: Option, // optional grey text after the name } -impl GenericDisplayRow {} - /// Compute a shared description-column start based on the widest visible name /// plus two spaces of padding. Ensures at least one column is left for the /// description. @@ -117,71 +111,19 @@ pub(crate) fn render_rows( state: &ScrollState, max_results: usize, empty_message: &str, - include_border: bool, ) { - if include_border { - use ratatui::widgets::Block; - use ratatui::widgets::BorderType; - use ratatui::widgets::Borders; - - // Always draw a dim left border to match other popups. - let block = Block::default() - .borders(Borders::LEFT) - .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().add_modifier(Modifier::DIM)); - block.render(area, buf); - } - - // Content renders to the right of the border with the same live prefix - // padding used by the composer so the popup aligns with the input text. - let prefix_cols = LIVE_PREFIX_COLS; - let content_area = Rect { - x: area.x.saturating_add(prefix_cols), - y: area.y, - width: area.width.saturating_sub(prefix_cols), - height: area.height, - }; - - // Clear the padding column(s) so stale characters never peek between the - // border and the popup contents. - let padding_cols = prefix_cols.saturating_sub(1); - if padding_cols > 0 { - let pad_start = area.x.saturating_add(1); - let pad_end = pad_start - .saturating_add(padding_cols) - .min(area.x.saturating_add(area.width)); - let pad_bottom = area.y.saturating_add(area.height); - for x in pad_start..pad_end { - for y in area.y..pad_bottom { - if let Some(cell) = buf.cell_mut((x, y)) { - cell.set_symbol(" "); - } - } - } - } - if rows_all.is_empty() { - if content_area.height > 0 { - let para = Paragraph::new(Line::from(empty_message.dim().italic())); - para.render( - Rect { - x: content_area.x, - y: content_area.y, - width: content_area.width, - height: 1, - }, - buf, - ); + if area.height > 0 { + Line::from(empty_message.dim().italic()).render(area, buf); } return; } // Determine which logical rows (items) are visible given the selection and // the max_results clamp. Scrolling is still item-based for simplicity. - let max_rows_from_area = content_area.height as usize; let visible_items = max_results .min(rows_all.len()) - .min(max_rows_from_area.max(1)); + .min(area.height.max(1) as usize); let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); if let Some(sel) = state.selected_idx { @@ -195,18 +137,18 @@ pub(crate) fn render_rows( } } - let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_area.width); + let desc_col = compute_desc_col(rows_all, start_idx, visible_items, area.width); // Render items, wrapping descriptions and aligning wrapped lines under the // shared description column. Stop when we run out of vertical space. - let mut cur_y = content_area.y; + let mut cur_y = area.y; for (i, row) in rows_all .iter() .enumerate() .skip(start_idx) .take(visible_items) { - if cur_y >= content_area.y + content_area.height { + if cur_y >= area.y + area.height { break; } @@ -217,7 +159,7 @@ pub(crate) fn render_rows( description, } = row; - let full_line = build_full_line( + let mut full_line = build_full_line( &GenericDisplayRow { name: name.clone(), match_indices: match_indices.clone(), @@ -226,32 +168,31 @@ pub(crate) fn render_rows( }, desc_col, ); + if Some(i) == state.selected_idx { + // Match previous behavior: cyan + bold for the selected row. + full_line.spans.iter_mut().for_each(|span| { + span.style = span.style.fg(Color::Cyan).bold(); + }); + } // Wrap with subsequent indent aligned to the description column. use crate::wrapping::RtOptions; use crate::wrapping::word_wrap_line; - let options = RtOptions::new(content_area.width as usize) + let options = RtOptions::new(area.width as usize) .initial_indent(Line::from("")) .subsequent_indent(Line::from(" ".repeat(desc_col))); let wrapped = word_wrap_line(&full_line, options); // Render the wrapped lines. - for mut line in wrapped { - if cur_y >= content_area.y + content_area.height { + for line in wrapped { + if cur_y >= area.y + area.height { break; } - if Some(i) == state.selected_idx { - // Match previous behavior: cyan + bold for the selected row. - line.style = Style::default() - .fg(Color::Cyan) - .add_modifier(Modifier::BOLD); - } - let para = Paragraph::new(line); - para.render( + line.render( Rect { - x: content_area.x, + x: area.x, y: cur_y, - width: content_area.width, + width: area.width, height: 1, }, buf, diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap index ced53b7c..9cbfe88f 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_with_subtitle.snap @@ -2,10 +2,11 @@ source: tui/src/bottom_pane/list_selection_view.rs expression: render_lines(&view) --- -▌ Select Approval Mode -▌ Switch between Codex approval presets -▌ -▌ > 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files -Press Enter to confirm or Esc to go back + Select Approval Mode + Switch between Codex approval presets + +› 1. Read Only (current) Codex can read files + 2. Full Access Codex can edit files + + Press Enter to confirm or Esc to go back diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap index b9858a43..5e3cf2c6 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__list_selection_view__tests__list_selection_spacing_without_subtitle.snap @@ -2,9 +2,10 @@ source: tui/src/bottom_pane/list_selection_view.rs expression: render_lines(&view) --- -▌ Select Approval Mode -▌ -▌ > 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files -Press Enter to confirm or Esc to go back + Select Approval Mode + +› 1. Read Only (current) Codex can read files + 2. Full Access Codex can edit files + + Press Enter to confirm or Esc to go back diff --git a/codex-rs/tui/src/chatwidget.rs b/codex-rs/tui/src/chatwidget.rs index 5973f619..9940c8f7 100644 --- a/codex-rs/tui/src/chatwidget.rs +++ b/codex-rs/tui/src/chatwidget.rs @@ -79,7 +79,6 @@ use crate::history_cell; use crate::history_cell::AgentMessageCell; use crate::history_cell::HistoryCell; use crate::history_cell::McpToolCallCell; -use crate::history_cell::PatchEventType; use crate::markdown::append_markdown; use crate::slash_command::SlashCommand; use crate::status::RateLimitSnapshotDisplay; @@ -534,9 +533,6 @@ impl ChatWidget { fn on_patch_apply_begin(&mut self, event: PatchApplyBeginEvent) { self.add_to_history(history_cell::new_patch_event( - PatchEventType::ApplyBegin { - auto_approved: event.auto_approved, - }, event.changes, &self.config.cwd, )); @@ -736,8 +732,6 @@ impl ChatWidget { pub(crate) fn handle_exec_approval_now(&mut self, id: String, ev: ExecApprovalRequestEvent) { self.flush_answer_stream_with_separator(); - // Emit the proposed command into history (like proposed patches) - self.add_to_history(history_cell::new_proposed_command(&ev.command)); let command = shlex::try_join(ev.command.iter().map(String::as_str)) .unwrap_or_else(|_| ev.command.join(" ")); self.notify(Notification::ExecApprovalRequested { command }); @@ -757,16 +751,12 @@ impl ChatWidget { ev: ApplyPatchApprovalRequestEvent, ) { self.flush_answer_stream_with_separator(); - self.add_to_history(history_cell::new_patch_event( - PatchEventType::ApprovalRequest, - ev.changes.clone(), - &self.config.cwd, - )); let request = ApprovalRequest::ApplyPatch { id, reason: ev.reason, - grant_root: ev.grant_root, + changes: ev.changes.clone(), + cwd: self.config.cwd.clone(), }; self.bottom_pane.push_approval_request(request); self.request_redraw(); @@ -1631,7 +1621,7 @@ impl ChatWidget { } self.bottom_pane.show_selection_view(SelectionViewParams { - title: "Select model and reasoning level".to_string(), + title: Some("Select model and reasoning level".to_string()), subtitle: Some( "Switch between OpenAI models for this and future Codex CLI session".to_string(), ), @@ -1677,7 +1667,7 @@ impl ChatWidget { } self.bottom_pane.show_selection_view(SelectionViewParams { - title: "Select Approval Mode".to_string(), + title: Some("Select Approval Mode".to_string()), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, ..Default::default() @@ -1852,7 +1842,7 @@ impl ChatWidget { }); self.bottom_pane.show_selection_view(SelectionViewParams { - title: "Select a review preset".into(), + title: Some("Select a review preset".into()), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, ..Default::default() @@ -1888,7 +1878,7 @@ impl ChatWidget { } self.bottom_pane.show_selection_view(SelectionViewParams { - title: "Select a base branch".to_string(), + title: Some("Select a base branch".to_string()), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, is_searchable: true, @@ -1929,7 +1919,7 @@ impl ChatWidget { } self.bottom_pane.show_selection_view(SelectionViewParams { - title: "Select a commit to review".to_string(), + title: Some("Select a commit to review".to_string()), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, is_searchable: true, @@ -2154,7 +2144,7 @@ pub(crate) fn show_review_commit_picker_with_entries( } chat.bottom_pane.show_selection_view(SelectionViewParams { - title: "Select a commit to review".to_string(), + title: Some("Select a commit to review".to_string()), footer_hint: Some(STANDARD_POPUP_HINT_LINE.to_string()), items, is_searchable: true, diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap index 3ed1e075..6e22bceb 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_approved.snap @@ -2,4 +2,5 @@ source: tui/src/chatwidget/tests.rs expression: lines_to_single_string(&approved_lines) --- -• Change Approved foo.txt (+1 -0) +• Added foo.txt (+1 -0) + 1 +hello diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_proposed.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_proposed.snap deleted file mode 100644 index 2d0c73ca..00000000 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__apply_patch_manual_flow_history_proposed.snap +++ /dev/null @@ -1,6 +0,0 @@ ---- -source: tui/src/chatwidget/tests.rs -expression: lines_to_single_string(&proposed_lines) ---- -• Proposed Change foo.txt (+1 -0) - 1 +hello diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap index 304adf54..10ded3d3 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec.snap @@ -1,18 +1,16 @@ --- source: tui/src/chatwidget/tests.rs -expression: terminal.backend() +expression: terminal.backend().vt100().screen().contents() --- -" " -"▌ this is a test reason such as one that would be produced by the model " -"▌ " -"▌ Command: echo hello world " -"▌ " -"▌ Allow command? " -"▌ " -"▌ > 1. Approve and run now (Y) Run this command one time " -"▌ 2. Always approve this session (A) Automatically approve this command for " -"▌ the rest of the session " -"▌ 3. Cancel (N) Do not run the command " -" " -"Press Enter to confirm or Esc to cancel " -" " + Allow command? + + this is a test reason such as one that would be produced by the model + + $ echo hello world + +› 1. Approve and run now Run this command one time + 2. Always approve this session Automatically approve this command for the + rest of the session + 3. Cancel Do not run the command + + Press Enter to confirm or Esc to cancel diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap index 239681ac..c3e04bd1 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_exec_no_reason.snap @@ -3,14 +3,15 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- " " -"▌ Command: echo hello world " -"▌ " -"▌ Allow command? " -"▌ " -"▌ > 1. Approve and run now (Y) Run this command one time " -"▌ 2. Always approve this session (A) Automatically approve this command for " -"▌ the rest of the session " -"▌ 3. Cancel (N) Do not run the command " " " -"Press Enter to confirm or Esc to cancel " +" Allow command? " +" " +" $ echo hello world " +" " +"› 1. Approve and run now Run this command one time " +" 2. Always approve this session Automatically approve this command for the " +" rest of the session " +" 3. Cancel Do not run the command " +" " +" Press Enter to confirm or Esc to cancel " " " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap index 2ee14a57..fd1860ca 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__approval_modal_patch.snap @@ -3,14 +3,17 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- " " -"▌ The model wants to apply changes " -"▌ " -"▌ Grant write access to /tmp for the remainder of this session. " -"▌ " -"▌ Apply changes? " -"▌ " -"▌ > 1. Approve (Y) Apply the proposed changes " -"▌ 2. Cancel (N) Do not apply the changes " " " -"Press Enter to confirm or Esc to cancel " +" Apply changes? " +" " +" README.md (+2 -0) " +" 1 +hello " +" 2 +world " +" " +" The model wants to apply changes " +" " +"› 1. Approve Apply the proposed changes " +" 2. Cancel Do not apply the changes " +" " +" Press Enter to confirm or Esc to cancel " " " diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap deleted file mode 100644 index 119965df..00000000 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_multiline.snap +++ /dev/null @@ -1,7 +0,0 @@ ---- -source: tui/src/chatwidget/tests.rs -expression: lines_to_single_string(&proposed_multi) ---- -• Proposed Command - └ echo line1 - echo line2 diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap deleted file mode 100644 index dfc52c01..00000000 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_history_proposed_short.snap +++ /dev/null @@ -1,6 +0,0 @@ ---- -source: tui/src/chatwidget/tests.rs -expression: lines_to_single_string(&proposed) ---- -• Proposed Command - └ echo hello world diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap new file mode 100644 index 00000000..5beb5323 --- /dev/null +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__exec_approval_modal_exec.snap @@ -0,0 +1,42 @@ +--- +source: tui/src/chatwidget/tests.rs +expression: "format!(\"{buf:?}\")" +--- +Buffer { + area: Rect { x: 0, y: 0, width: 80, height: 15 }, + content: [ + " ", + " ", + " Allow command? ", + " ", + " this is a test reason such as one that would be produced by the model ", + " ", + " $ echo hello world ", + " ", + "› 1. Approve and run now Run this command one time ", + " 2. Always approve this session Automatically approve this command for the ", + " rest of the session ", + " 3. Cancel Do not run the command ", + " ", + " Press Enter to confirm or Esc to cancel ", + " ", + ], + styles: [ + x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: BOLD, + x: 16, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: ITALIC, + x: 71, y: 4, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 0, y: 8, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD, + x: 34, y: 8, fg: Cyan, bg: Reset, underline: Reset, modifier: BOLD | DIM, + x: 59, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 34, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 76, y: 9, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 34, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 53, y: 10, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 34, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 56, y: 11, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + x: 2, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: DIM, + x: 41, y: 13, fg: Reset, bg: Reset, underline: Reset, modifier: NONE, + ] +} diff --git a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap index e762896f..86d89b80 100644 --- a/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap +++ b/codex-rs/tui/src/chatwidget/snapshots/codex_tui__chatwidget__tests__status_widget_and_approval_modal.snap @@ -3,16 +3,17 @@ source: tui/src/chatwidget/tests.rs expression: terminal.backend() --- " " -"▌ this is a test reason such as one that would be produced by the model " -"▌ " -"▌ Command: echo 'hello world' " -"▌ " -"▌ Allow command? " -"▌ " -"▌ > 1. Approve and run now (Y) Run this command one time " -"▌ 2. Always approve this session (A) Automatically approve this command for " -"▌ the rest of the session " -"▌ 3. Cancel (N) Do not run the command " " " -"Press Enter to confirm or Esc to cancel " +" Allow command? " +" " +" this is a test reason such as one that would be produced by the model " +" " +" $ echo 'hello world' " +" " +"› 1. Approve and run now Run this command one time " +" 2. Always approve this session Automatically approve this command for the " +" rest of the session " +" 3. Cancel Do not run the command " +" " +" Press Enter to confirm or Esc to cancel " " " diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index ed43172d..77cde195 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -83,66 +83,6 @@ fn upgrade_event_payload_for_tests(mut payload: serde_json::Value) -> serde_json payload } -/* -#[test] -fn final_answer_without_newline_is_flushed_immediately() { - let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); - - // Set up a VT100 test terminal to capture ANSI visual output - let width: u16 = 80; - // Increased height to keep the initial banner/help lines in view even if - // the session renders an extra header line or minor layout changes occur. - let height: u16 = 2500; - let viewport = Rect::new(0, height - 1, width, 1); - let backend = ratatui::backend::TestBackend::new(width, height); - let mut terminal = crate::custom_terminal::Terminal::with_options(backend) - .expect("failed to construct terminal"); - terminal.set_viewport_area(viewport); - - // Simulate a streaming answer without any newline characters. - chat.handle_codex_event(Event { - id: "sub-a".into(), - msg: EventMsg::AgentMessageDelta(AgentMessageDeltaEvent { - delta: "Hi! How can I help with codex-rs or anything else today?".into(), - }), - }); - - // Now simulate the final AgentMessage which should flush the pending line immediately. - chat.handle_codex_event(Event { - id: "sub-a".into(), - msg: EventMsg::AgentMessage(AgentMessageEvent { - message: "Hi! How can I help with codex-rs or anything else today?".into(), - }), - }); - - // Drain history insertions and verify the final line is present. - let cells = drain_insert_history(&mut rx); - assert!( - cells.iter().any(|lines| { - let s = lines - .iter() - .flat_map(|l| l.spans.iter()) - .map(|sp| sp.content.clone()) - .collect::(); - s.contains("codex") - }), - "expected 'codex' header to be emitted", - ); - let found_final = cells.iter().any(|lines| { - let s = lines - .iter() - .flat_map(|l| l.spans.iter()) - .map(|sp| sp.content.clone()) - .collect::(); - s.contains("Hi! How can I help with codex-rs or anything else today?") - }); - assert!( - found_final, - "expected final answer text to be flushed to history" - ); -} -*/ - #[test] fn resumed_initial_messages_render_history() { let (mut chat, mut rx, _ops) = make_chatwidget_manual(); @@ -452,15 +392,18 @@ fn exec_approval_emits_proposed_command_and_decision_history() { msg: EventMsg::ExecApprovalRequest(ev), }); - // Snapshot the Proposed Command cell emitted into history - let proposed = drain_insert_history(&mut rx) - .pop() - .expect("expected proposed command cell"); - assert_snapshot!( - "exec_approval_history_proposed_short", - lines_to_single_string(&proposed) + let proposed_cells = drain_insert_history(&mut rx); + assert!( + proposed_cells.is_empty(), + "expected approval request to render via modal without emitting history cells" ); + // The approval modal should display the command snippet for user confirmation. + let area = Rect::new(0, 0, 80, chat.desired_height(80)); + let mut buf = ratatui::buffer::Buffer::empty(area); + (&chat).render_ref(area, &mut buf); + assert_snapshot!("exec_approval_modal_exec", format!("{buf:?}")); + // Approve via keyboard and verify a concise decision history line is added chat.handle_key_event(KeyEvent::new(KeyCode::Char('y'), KeyModifiers::NONE)); let decision = drain_insert_history(&mut rx) @@ -476,7 +419,7 @@ fn exec_approval_emits_proposed_command_and_decision_history() { fn exec_approval_decision_truncates_multiline_and_long_commands() { let (mut chat, mut rx, _op_rx) = make_chatwidget_manual(); - // Multiline command: should render proposed command fully in history with prefixes + // Multiline command: modal should show full command, history records decision only let ev_multi = ExecApprovalRequestEvent { call_id: "call-multi".into(), command: vec!["bash".into(), "-lc".into(), "echo line1\necho line2".into()], @@ -489,12 +432,29 @@ fn exec_approval_decision_truncates_multiline_and_long_commands() { id: "sub-multi".into(), msg: EventMsg::ExecApprovalRequest(ev_multi), }); - let proposed_multi = drain_insert_history(&mut rx) - .pop() - .expect("expected proposed multiline command cell"); - assert_snapshot!( - "exec_approval_history_proposed_multiline", - lines_to_single_string(&proposed_multi) + let proposed_multi = drain_insert_history(&mut rx); + assert!( + proposed_multi.is_empty(), + "expected multiline approval request to render via modal without emitting history cells" + ); + + let area = Rect::new(0, 0, 80, chat.desired_height(80)); + let mut buf = ratatui::buffer::Buffer::empty(area); + (&chat).render_ref(area, &mut buf); + let mut saw_first_line = false; + for y in 0..area.height { + let mut row = String::new(); + for x in 0..area.width { + row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); + } + if row.contains("echo line1") { + saw_first_line = true; + break; + } + } + assert!( + saw_first_line, + "expected modal to show first line of multiline snippet" ); // Deny via keyboard; decision snippet should be single-line and elided with " ..." @@ -519,7 +479,11 @@ fn exec_approval_decision_truncates_multiline_and_long_commands() { id: "sub-long".into(), msg: EventMsg::ExecApprovalRequest(ev_long), }); - drain_insert_history(&mut rx); // proposed cell not needed for this assertion + let proposed_long = drain_insert_history(&mut rx); + assert!( + proposed_long.is_empty(), + "expected long approval request to avoid emitting history cells before decision" + ); chat.handle_key_event(KeyEvent::new(KeyCode::Char('n'), KeyModifiers::NONE)); let aborted_long = drain_insert_history(&mut rx) .pop() @@ -935,18 +899,21 @@ fn render_bottom_first_row(chat: &ChatWidget, width: u16) -> String { let area = Rect::new(0, 0, width, height); let mut buf = Buffer::empty(area); (chat).render_ref(area, &mut buf); - let mut row = String::new(); - // Row 0 is the top spacer for the bottom pane; row 1 contains the header line - let y = 1u16.min(height.saturating_sub(1)); - for x in 0..area.width { - let s = buf[(x, y)].symbol(); - if s.is_empty() { - row.push(' '); - } else { - row.push_str(s); + for y in 0..area.height { + let mut row = String::new(); + for x in 0..area.width { + let s = buf[(x, y)].symbol(); + if s.is_empty() { + row.push(' '); + } else { + row.push_str(s); + } + } + if !row.trim().is_empty() { + return row; } } - row + String::new() } #[test] @@ -1181,12 +1148,19 @@ fn approval_modal_exec_snapshot() { // Render to a fixed-size test terminal and snapshot. // Call desired_height first and use that exact height for rendering. let height = chat.desired_height(80); - let mut terminal = ratatui::Terminal::new(ratatui::backend::TestBackend::new(80, height)) - .expect("create terminal"); + let mut terminal = + crate::custom_terminal::Terminal::with_options(VT100Backend::new(80, height)) + .expect("create terminal"); + let viewport = Rect::new(0, 0, 80, height); + terminal.set_viewport_area(viewport); + terminal .draw(|f| f.render_widget_ref(&chat, f.area())) .expect("draw approval modal"); - assert_snapshot!("approval_modal_exec", terminal.backend()); + assert_snapshot!( + "approval_modal_exec", + terminal.backend().vt100().screen().contents() + ); } // Snapshot test: command approval modal without a reason @@ -1470,13 +1444,27 @@ fn apply_patch_events_emit_history_cells() { msg: EventMsg::ApplyPatchApprovalRequest(ev), }); let cells = drain_insert_history(&mut rx); - assert!(!cells.is_empty(), "expected pending patch cell to be sent"); - let blob = lines_to_single_string(cells.last().unwrap()); assert!( - blob.contains("Proposed Change"), - "missing proposed change header: {blob:?}" + cells.is_empty(), + "expected approval request to surface via modal without emitting history cells" ); + let area = Rect::new(0, 0, 80, chat.desired_height(80)); + let mut buf = ratatui::buffer::Buffer::empty(area); + (&chat).render_ref(area, &mut buf); + let mut saw_summary = false; + for y in 0..area.height { + let mut row = String::new(); + for x in 0..area.width { + row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); + } + if row.contains("foo.txt (+1 -0)") { + saw_summary = true; + break; + } + } + assert!(saw_summary, "expected approval modal to show diff summary"); + // 2) Begin apply -> per-file apply block cell (no global header) let mut changes2 = HashMap::new(); changes2.insert( @@ -1562,8 +1550,8 @@ fn apply_patch_manual_approval_adjusts_header() { assert!(!cells.is_empty(), "expected apply block cell to be sent"); let blob = lines_to_single_string(cells.last().unwrap()); assert!( - blob.contains("Change Approved foo.txt"), - "expected change approved summary: {blob:?}" + blob.contains("Added foo.txt") || blob.contains("Edited foo.txt"), + "expected apply summary header for foo.txt: {blob:?}" ); } @@ -1587,9 +1575,11 @@ fn apply_patch_manual_flow_snapshot() { grant_root: None, }), }); - let proposed_lines = drain_insert_history(&mut rx) - .pop() - .expect("proposed patch cell"); + let history_before_apply = drain_insert_history(&mut rx); + assert!( + history_before_apply.is_empty(), + "expected approval modal to defer history emission" + ); let mut apply_changes = HashMap::new(); apply_changes.insert( @@ -1610,10 +1600,6 @@ fn apply_patch_manual_flow_snapshot() { .pop() .expect("approved patch cell"); - assert_snapshot!( - "apply_patch_manual_flow_history_proposed", - lines_to_single_string(&proposed_lines) - ); assert_snapshot!( "apply_patch_manual_flow_history_approved", lines_to_single_string(&approved_lines) @@ -1803,24 +1789,42 @@ fn apply_patch_request_shows_diff_summary() { }), }); - // Drain history insertions and verify the diff summary is present + // No history entries yet; the modal should contain the diff summary let cells = drain_insert_history(&mut rx); assert!( - !cells.is_empty(), - "expected a history cell with the proposed patch summary" - ); - let blob = lines_to_single_string(cells.last().unwrap()); - - // Header should summarize totals - assert!( - blob.contains("Proposed Change README.md (+2 -0)"), - "missing or incorrect diff header: {blob:?}" + cells.is_empty(), + "expected approval request to render via modal instead of history" ); - // Per-file summary line should include the file path and counts + let area = Rect::new(0, 0, 80, chat.desired_height(80)); + let mut buf = ratatui::buffer::Buffer::empty(area); + (&chat).render_ref(area, &mut buf); + + let mut saw_header = false; + let mut saw_line1 = false; + let mut saw_line2 = false; + for y in 0..area.height { + let mut row = String::new(); + for x in 0..area.width { + row.push(buf[(x, y)].symbol().chars().next().unwrap_or(' ')); + } + if row.contains("README.md (+2 -0)") { + saw_header = true; + } + if row.contains("+line one") { + saw_line1 = true; + } + if row.contains("+line two") { + saw_line2 = true; + } + if saw_header && saw_line1 && saw_line2 { + break; + } + } + assert!(saw_header, "expected modal to show diff header with totals"); assert!( - blob.contains("README.md"), - "missing per-file diff summary: {blob:?}" + saw_line1 && saw_line2, + "expected modal to show per-line diff summary" ); } diff --git a/codex-rs/tui/src/diff_render.rs b/codex-rs/tui/src/diff_render.rs index 1e0c222b..033fd092 100644 --- a/codex-rs/tui/src/diff_render.rs +++ b/codex-rs/tui/src/diff_render.rs @@ -1,16 +1,20 @@ use diffy::Hunk; +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; use ratatui::style::Color; use ratatui::style::Modifier; use ratatui::style::Style; use ratatui::style::Stylize; use ratatui::text::Line as RtLine; use ratatui::text::Span as RtSpan; +use ratatui::widgets::Paragraph; use std::collections::HashMap; use std::path::Path; use std::path::PathBuf; use crate::exec_command::relativize_to_home; -use crate::history_cell::PatchEventType; +use crate::render::renderable::ColumnRenderable; +use crate::render::renderable::Renderable; use codex_core::git_info::get_git_repo_root; use codex_core::protocol::FileChange; @@ -23,24 +27,57 @@ enum DiffLineType { Context, } +pub struct DiffSummary { + changes: HashMap, + cwd: PathBuf, +} + +impl DiffSummary { + pub fn new(changes: HashMap, cwd: PathBuf) -> Self { + Self { changes, cwd } + } +} + +impl Renderable for FileChange { + fn render(&self, area: Rect, buf: &mut Buffer) { + let mut lines = vec![]; + render_change(self, &mut lines, area.width as usize); + Paragraph::new(lines).render(area, buf); + } + + fn desired_height(&self, width: u16) -> u16 { + let mut lines = vec![]; + render_change(self, &mut lines, width as usize); + lines.len() as u16 + } +} + +impl From for Box { + fn from(val: DiffSummary) -> Self { + let mut rows: Vec> = vec![]; + + for (i, row) in collect_rows(&val.changes).into_iter().enumerate() { + if i > 0 { + rows.push(Box::new(RtLine::from(""))); + } + let mut path = RtLine::from(display_path_for(&row.path, &val.cwd)); + path.push_span(" "); + path.extend(render_line_count_summary(row.added, row.removed)); + rows.push(Box::new(path)); + rows.push(Box::new(row.change)); + } + + Box::new(ColumnRenderable::new(rows)) + } +} + pub(crate) fn create_diff_summary( changes: &HashMap, - event_type: PatchEventType, cwd: &Path, wrap_cols: usize, ) -> Vec> { let rows = collect_rows(changes); - let header_kind = match event_type { - PatchEventType::ApplyBegin { auto_approved } => { - if auto_approved { - HeaderKind::Edited - } else { - HeaderKind::ChangeApproved - } - } - PatchEventType::ApprovalRequest => HeaderKind::ProposedChange, - }; - render_changes_block(rows, wrap_cols, header_kind, cwd) + render_changes_block(rows, wrap_cols, cwd) } // Shared row for per-file presentation @@ -81,30 +118,18 @@ fn collect_rows(changes: &HashMap) -> Vec { rows } -enum HeaderKind { - ProposedChange, - Edited, - ChangeApproved, +fn render_line_count_summary(added: usize, removed: usize) -> Vec> { + let mut spans = Vec::new(); + spans.push("(".into()); + spans.push(format!("+{added}").green()); + spans.push(" ".into()); + spans.push(format!("-{removed}").red()); + spans.push(")".into()); + spans } -fn render_changes_block( - rows: Vec, - wrap_cols: usize, - header_kind: HeaderKind, - cwd: &Path, -) -> Vec> { +fn render_changes_block(rows: Vec, wrap_cols: usize, cwd: &Path) -> Vec> { let mut out: Vec> = Vec::new(); - let term_cols = wrap_cols; - - fn render_line_count_summary(added: usize, removed: usize) -> Vec> { - let mut spans = Vec::new(); - spans.push("(".into()); - spans.push(format!("+{added}").green()); - spans.push(" ".into()); - spans.push(format!("-{removed}").red()); - spans.push(")".into()); - spans - } let render_path = |row: &Row| -> Vec> { let mut spans = Vec::new(); @@ -121,66 +146,31 @@ fn render_changes_block( let file_count = rows.len(); let noun = if file_count == 1 { "file" } else { "files" }; let mut header_spans: Vec> = vec!["• ".into()]; - match header_kind { - HeaderKind::ProposedChange => { - header_spans.push("Proposed Change".bold()); - if let [row] = &rows[..] { - header_spans.push(" ".into()); - header_spans.extend(render_path(row)); - header_spans.push(" ".into()); - header_spans.extend(render_line_count_summary(row.added, row.removed)); - } else { - header_spans.push(format!(" to {file_count} {noun} ").into()); - header_spans.extend(render_line_count_summary(total_added, total_removed)); - } - } - HeaderKind::Edited => { - if let [row] = &rows[..] { - let verb = match &row.change { - FileChange::Add { .. } => "Added", - FileChange::Delete { .. } => "Deleted", - _ => "Edited", - }; - header_spans.push(verb.bold()); - header_spans.push(" ".into()); - header_spans.extend(render_path(row)); - header_spans.push(" ".into()); - header_spans.extend(render_line_count_summary(row.added, row.removed)); - } else { - header_spans.push("Edited".bold()); - header_spans.push(format!(" {file_count} {noun} ").into()); - header_spans.extend(render_line_count_summary(total_added, total_removed)); - } - } - HeaderKind::ChangeApproved => { - header_spans.push("Change Approved".bold()); - if let [row] = &rows[..] { - header_spans.push(" ".into()); - header_spans.extend(render_path(row)); - header_spans.push(" ".into()); - header_spans.extend(render_line_count_summary(row.added, row.removed)); - } else { - header_spans.push(format!(" {file_count} {noun} ").into()); - header_spans.extend(render_line_count_summary(total_added, total_removed)); - } - } + if let [row] = &rows[..] { + let verb = match &row.change { + FileChange::Add { .. } => "Added", + FileChange::Delete { .. } => "Deleted", + _ => "Edited", + }; + header_spans.push(verb.bold()); + header_spans.push(" ".into()); + header_spans.extend(render_path(row)); + header_spans.push(" ".into()); + header_spans.extend(render_line_count_summary(row.added, row.removed)); + } else { + header_spans.push("Edited".bold()); + header_spans.push(format!(" {file_count} {noun} ").into()); + header_spans.extend(render_line_count_summary(total_added, total_removed)); } out.push(RtLine::from(header_spans)); - // For Change Approved, we only show the header summary and no per-file/diff details. - if matches!(header_kind, HeaderKind::ChangeApproved) { - return out; - } - for (idx, r) in rows.into_iter().enumerate() { // Insert a blank separator between file chunks (except before the first) if idx > 0 { out.push("".into()); } // File header line (skip when single-file header already shows the name) - let skip_file_header = - matches!(header_kind, HeaderKind::ProposedChange | HeaderKind::Edited) - && file_count == 1; + let skip_file_header = file_count == 1; if !skip_file_header { let mut header: Vec> = Vec::new(); header.push(" └ ".dim()); @@ -190,71 +180,77 @@ fn render_changes_block( out.push(RtLine::from(header)); } - match r.change { - FileChange::Add { content } => { - for (i, raw) in content.lines().enumerate() { - out.extend(push_wrapped_diff_line( - i + 1, - DiffLineType::Insert, - raw, - term_cols, - )); - } - } - FileChange::Delete { content } => { - for (i, raw) in content.lines().enumerate() { - out.extend(push_wrapped_diff_line( - i + 1, - DiffLineType::Delete, - raw, - term_cols, - )); - } - } - FileChange::Update { unified_diff, .. } => { - if let Ok(patch) = diffy::Patch::from_str(&unified_diff) { - let mut is_first_hunk = true; - for h in patch.hunks() { - if !is_first_hunk { - out.push(RtLine::from(vec![" ".into(), "⋮".dim()])); - } - is_first_hunk = false; + render_change(&r.change, &mut out, wrap_cols); + } - let mut old_ln = h.old_range().start(); - let mut new_ln = h.new_range().start(); - for l in h.lines() { - match l { - diffy::Line::Insert(text) => { - let s = text.trim_end_matches('\n'); - out.extend(push_wrapped_diff_line( - new_ln, - DiffLineType::Insert, - s, - term_cols, - )); - new_ln += 1; - } - diffy::Line::Delete(text) => { - let s = text.trim_end_matches('\n'); - out.extend(push_wrapped_diff_line( - old_ln, - DiffLineType::Delete, - s, - term_cols, - )); - old_ln += 1; - } - diffy::Line::Context(text) => { - let s = text.trim_end_matches('\n'); - out.extend(push_wrapped_diff_line( - new_ln, - DiffLineType::Context, - s, - term_cols, - )); - old_ln += 1; - new_ln += 1; - } + out +} + +fn render_change(change: &FileChange, out: &mut Vec>, width: usize) { + match change { + FileChange::Add { content } => { + for (i, raw) in content.lines().enumerate() { + out.extend(push_wrapped_diff_line( + i + 1, + DiffLineType::Insert, + raw, + width, + )); + } + } + FileChange::Delete { content } => { + for (i, raw) in content.lines().enumerate() { + out.extend(push_wrapped_diff_line( + i + 1, + DiffLineType::Delete, + raw, + width, + )); + } + } + FileChange::Update { unified_diff, .. } => { + if let Ok(patch) = diffy::Patch::from_str(unified_diff) { + let mut is_first_hunk = true; + for h in patch.hunks() { + if !is_first_hunk { + out.push(RtLine::from(vec![" ".into(), "⋮".dim()])); + } + is_first_hunk = false; + + let mut old_ln = h.old_range().start(); + let mut new_ln = h.new_range().start(); + for l in h.lines() { + match l { + diffy::Line::Insert(text) => { + let s = text.trim_end_matches('\n'); + out.extend(push_wrapped_diff_line( + new_ln, + DiffLineType::Insert, + s, + width, + )); + new_ln += 1; + } + diffy::Line::Delete(text) => { + let s = text.trim_end_matches('\n'); + out.extend(push_wrapped_diff_line( + old_ln, + DiffLineType::Delete, + s, + width, + )); + old_ln += 1; + } + diffy::Line::Context(text) => { + let s = text.trim_end_matches('\n'); + out.extend(push_wrapped_diff_line( + new_ln, + DiffLineType::Context, + s, + width, + )); + old_ln += 1; + new_ln += 1; } } } @@ -262,8 +258,6 @@ fn render_changes_block( } } } - - out } pub(crate) fn display_path_for(path: &Path, cwd: &Path) -> String { @@ -300,7 +294,7 @@ fn push_wrapped_diff_line( line_number: usize, kind: DiffLineType, text: &str, - term_cols: usize, + width: usize, ) -> Vec> { let indent = " "; let ln_str = line_number.to_string(); @@ -325,7 +319,7 @@ fn push_wrapped_diff_line( // Fit the content for the current terminal row: // compute how many columns are available after the prefix, then split // at a UTF-8 character boundary so this row's chunk fits exactly. - let available_content_cols = term_cols.saturating_sub(prefix_cols + 1).max(1); + let available_content_cols = width.saturating_sub(prefix_cols + 1).max(1); let split_at_byte_index = remaining_text .char_indices() .nth(available_content_cols) @@ -385,11 +379,8 @@ mod tests { use ratatui::widgets::Paragraph; use ratatui::widgets::WidgetRef; use ratatui::widgets::Wrap; - fn diff_summary_for_tests( - changes: &HashMap, - event_type: PatchEventType, - ) -> Vec> { - create_diff_summary(changes, event_type, &PathBuf::from("/"), 80) + fn diff_summary_for_tests(changes: &HashMap) -> Vec> { + create_diff_summary(changes, &PathBuf::from("/"), 80) } fn snapshot_lines(name: &str, lines: Vec>, width: u16, height: u16) { @@ -421,42 +412,6 @@ mod tests { assert_snapshot!(name, text); } - #[test] - fn ui_snapshot_add_details() { - let mut changes: HashMap = HashMap::new(); - changes.insert( - PathBuf::from("README.md"), - FileChange::Add { - content: "first line\nsecond line\n".to_string(), - }, - ); - - let lines = diff_summary_for_tests(&changes, PatchEventType::ApprovalRequest); - - snapshot_lines("add_details", lines, 80, 10); - } - - #[test] - fn ui_snapshot_update_details_with_rename() { - let mut changes: HashMap = HashMap::new(); - - let original = "line one\nline two\nline three\n"; - let modified = "line one\nline two changed\nline three\n"; - let patch = diffy::create_patch(original, modified).to_string(); - - changes.insert( - PathBuf::from("src/lib.rs"), - FileChange::Update { - unified_diff: patch, - move_path: Some(PathBuf::from("src/lib_new.rs")), - }, - ); - - let lines = diff_summary_for_tests(&changes, PatchEventType::ApprovalRequest); - - snapshot_lines("update_details_with_rename", lines, 80, 12); - } - #[test] fn ui_snapshot_wrap_behavior_insert() { // Narrow width to force wrapping within our diff line rendering @@ -469,71 +424,6 @@ mod tests { snapshot_lines("wrap_behavior_insert", lines, 90, 8); } - #[test] - fn ui_snapshot_single_line_replacement_counts() { - // Reproduce: one deleted line replaced by one inserted line, no extra context - let original = "# Codex CLI (Rust Implementation)\n"; - let modified = "# Codex CLI (Rust Implementation) banana\n"; - let patch = diffy::create_patch(original, modified).to_string(); - - let mut changes: HashMap = HashMap::new(); - changes.insert( - PathBuf::from("README.md"), - FileChange::Update { - unified_diff: patch, - move_path: None, - }, - ); - - let lines = diff_summary_for_tests(&changes, PatchEventType::ApprovalRequest); - - snapshot_lines("single_line_replacement_counts", lines, 80, 8); - } - - #[test] - fn ui_snapshot_blank_context_line() { - // Ensure a hunk that includes a blank context line at the beginning is rendered visibly - let original = "\nY\n"; - let modified = "\nY changed\n"; - let patch = diffy::create_patch(original, modified).to_string(); - - let mut changes: HashMap = HashMap::new(); - changes.insert( - PathBuf::from("example.txt"), - FileChange::Update { - unified_diff: patch, - move_path: None, - }, - ); - - let lines = diff_summary_for_tests(&changes, PatchEventType::ApprovalRequest); - - snapshot_lines("blank_context_line", lines, 80, 10); - } - - #[test] - fn ui_snapshot_vertical_ellipsis_between_hunks() { - // Create a patch with two separate hunks to ensure we render the vertical ellipsis (⋮) - let original = - "line 1\nline 2\nline 3\nline 4\nline 5\nline 6\nline 7\nline 8\nline 9\nline 10\n"; - let modified = "line 1\nline two changed\nline 3\nline 4\nline 5\nline 6\nline 7\nline 8\nline nine changed\nline 10\n"; - let patch = diffy::create_patch(original, modified).to_string(); - - let mut changes: HashMap = HashMap::new(); - changes.insert( - PathBuf::from("example.txt"), - FileChange::Update { - unified_diff: patch, - move_path: None, - }, - ); - - let lines = diff_summary_for_tests(&changes, PatchEventType::ApprovalRequest); - - // Height is large enough to show both hunks and the separator - snapshot_lines("vertical_ellipsis_between_hunks", lines, 80, 16); - } - #[test] fn ui_snapshot_apply_update_block() { let mut changes: HashMap = HashMap::new(); @@ -549,12 +439,8 @@ mod tests { }, ); - for (name, auto_approved) in [ - ("apply_update_block", true), - ("apply_update_block_manual", false), - ] { - let lines = - diff_summary_for_tests(&changes, PatchEventType::ApplyBegin { auto_approved }); + for name in ["apply_update_block", "apply_update_block_manual"] { + let lines = diff_summary_for_tests(&changes); snapshot_lines(name, lines, 80, 12); } @@ -575,12 +461,7 @@ mod tests { }, ); - let lines = diff_summary_for_tests( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - ); + let lines = diff_summary_for_tests(&changes); snapshot_lines("apply_update_with_rename_block", lines, 80, 12); } @@ -608,12 +489,7 @@ mod tests { }, ); - let lines = diff_summary_for_tests( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - ); + let lines = diff_summary_for_tests(&changes); snapshot_lines("apply_multiple_files_block", lines, 80, 14); } @@ -628,12 +504,7 @@ mod tests { }, ); - let lines = diff_summary_for_tests( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - ); + let lines = diff_summary_for_tests(&changes); snapshot_lines("apply_add_block", lines, 80, 10); } @@ -652,12 +523,7 @@ mod tests { }, ); - let lines = diff_summary_for_tests( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - ); + let lines = diff_summary_for_tests(&changes); // Cleanup best-effort; rendering has already read the file let _ = std::fs::remove_file(&tmp_path); @@ -681,14 +547,7 @@ mod tests { }, ); - let lines = create_diff_summary( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - &PathBuf::from("/"), - 72, - ); + let lines = create_diff_summary(&changes, &PathBuf::from("/"), 72); // Render with backend width wider than wrap width to avoid Paragraph auto-wrap. snapshot_lines("apply_update_block_wraps_long_lines", lines, 80, 12); @@ -711,14 +570,7 @@ mod tests { }, ); - let mut lines = create_diff_summary( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - &PathBuf::from("/"), - 28, - ); + let mut lines = create_diff_summary(&changes, &PathBuf::from("/"), 28); // Drop the combined header for this text-only snapshot if !lines.is_empty() { lines.remove(0); @@ -745,14 +597,7 @@ mod tests { }, ); - let lines = create_diff_summary( - &changes, - PatchEventType::ApplyBegin { - auto_approved: true, - }, - &cwd, - 80, - ); + let lines = create_diff_summary(&changes, &cwd, 80); snapshot_lines("apply_update_block_relativizes_path", lines, 80, 10); } diff --git a/codex-rs/tui/src/history_cell.rs b/codex-rs/tui/src/history_cell.rs index 6a1ac37a..4121a983 100644 --- a/codex-rs/tui/src/history_cell.rs +++ b/codex-rs/tui/src/history_cell.rs @@ -5,7 +5,6 @@ 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; @@ -50,12 +49,6 @@ use std::time::Instant; use tracing::error; use unicode_width::UnicodeWidthStr; -#[derive(Clone, Debug)] -pub(crate) enum PatchEventType { - ApprovalRequest, - ApplyBegin { auto_approved: bool }, -} - /// Represents an event to display in the conversation history. Returns its /// `Vec>` representation to make it easier to display in a /// scrollable list. @@ -277,19 +270,13 @@ pub(crate) fn new_review_status_line(message: String) -> PlainHistoryCell { #[derive(Debug)] pub(crate) struct PatchHistoryCell { - event_type: PatchEventType, changes: HashMap, cwd: PathBuf, } impl HistoryCell for PatchHistoryCell { fn display_lines(&self, width: u16) -> Vec> { - create_diff_summary( - &self.changes, - self.event_type.clone(), - &self.cwd, - width as usize, - ) + create_diff_summary(&self.changes, &self.cwd, width as usize) } } @@ -1016,12 +1003,10 @@ impl HistoryCell for PlanUpdateCell { /// a proposed patch. The summary lines should already be formatted (e.g. /// "A path/to/file.rs"). pub(crate) fn new_patch_event( - event_type: PatchEventType, changes: HashMap, cwd: &Path, ) -> PatchHistoryCell { PatchHistoryCell { - event_type, changes, cwd: cwd.to_path_buf(), } @@ -1052,27 +1037,6 @@ pub(crate) fn new_patch_apply_failure(stderr: String) -> PlainHistoryCell { PlainHistoryCell { lines } } -/// Create a new history cell for a proposed command approval. -/// Renders a header and the command preview similar to how proposed patches -/// show a header and summary. -pub(crate) fn new_proposed_command(command: &[String]) -> PlainHistoryCell { - let cmd = strip_bash_lc_and_escape(command); - - let mut lines: Vec> = Vec::new(); - lines.push(Line::from(vec!["• ".dim(), "Proposed Command".bold()])); - - let highlighted_lines = crate::render::highlight::highlight_bash_to_lines(&cmd); - let initial_prefix: Span<'static> = " └ ".dim(); - let subsequent_prefix: Span<'static> = " ".into(); - lines.extend(prefix_lines( - highlighted_lines, - initial_prefix, - subsequent_prefix, - )); - - PlainHistoryCell { lines } -} - pub(crate) fn new_reasoning_block( full_reasoning_buffer: String, config: &Config, diff --git a/codex-rs/tui/src/pager_overlay.rs b/codex-rs/tui/src/pager_overlay.rs index ec9652ce..6a0562fe 100644 --- a/codex-rs/tui/src/pager_overlay.rs +++ b/codex-rs/tui/src/pager_overlay.rs @@ -3,13 +3,14 @@ use std::sync::Arc; use std::time::Duration; use crate::history_cell::HistoryCell; -use crate::render::line_utils::push_owned_lines; +use crate::render::renderable::Renderable; use crate::tui; use crate::tui::TuiEvent; use crossterm::event::KeyCode; use crossterm::event::KeyEvent; use crossterm::event::KeyEventKind; use ratatui::buffer::Buffer; +use ratatui::buffer::Cell; use ratatui::layout::Rect; use ratatui::style::Color; use ratatui::style::Style; @@ -22,6 +23,7 @@ use ratatui::widgets::Clear; use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use ratatui::widgets::WidgetRef; +use ratatui::widgets::Wrap; pub(crate) enum Overlay { Transcript(TranscriptOverlay), @@ -33,10 +35,17 @@ impl Overlay { Self::Transcript(TranscriptOverlay::new(cells)) } - pub(crate) fn new_static_with_title(lines: Vec>, title: String) -> Self { + pub(crate) fn new_static_with_lines(lines: Vec>, title: String) -> Self { Self::Static(StaticOverlay::with_title(lines, title)) } + pub(crate) fn new_static_with_renderables( + renderables: Vec>, + title: String, + ) -> Self { + Self::Static(StaticOverlay::with_renderables(renderables, title)) + } + pub(crate) fn handle_event(&mut self, tui: &mut tui::Tui, event: TuiEvent) -> Result<()> { match self { Overlay::Transcript(o) => o.handle_event(tui, event), @@ -78,57 +87,53 @@ fn render_key_hints(area: Rect, buf: &mut Buffer, pairs: &[(&str, &str)]) { /// Generic widget for rendering a pager view. struct PagerView { - texts: Vec>, + renderables: Vec>, scroll_offset: usize, title: String, - wrap_cache: Option, last_content_height: Option, + last_rendered_height: Option, /// If set, on next render ensure this chunk is visible. pending_scroll_chunk: Option, } impl PagerView { - fn new(texts: Vec>, title: String, scroll_offset: usize) -> Self { + fn new(renderables: Vec>, title: String, scroll_offset: usize) -> Self { Self { - texts, + renderables, scroll_offset, title, - wrap_cache: None, last_content_height: None, + last_rendered_height: None, pending_scroll_chunk: None, } } + fn content_height(&self, width: u16) -> usize { + self.renderables + .iter() + .map(|c| c.desired_height(width) as usize) + .sum() + } + fn render(&mut self, area: Rect, buf: &mut Buffer) { Clear.render(area, buf); self.render_header(area, buf); - let content_area = self.scroll_area(area); + let content_area = self.content_area(area); self.update_last_content_height(content_area.height); - self.ensure_wrapped(content_area.width); + let content_height = self.content_height(content_area.width); + self.last_rendered_height = Some(content_height); // If there is a pending request to scroll a specific chunk into view, // satisfy it now that wrapping is up to date for this width. - if let (Some(idx), Some(cache)) = - (self.pending_scroll_chunk.take(), self.wrap_cache.as_ref()) - && let Some(range) = cache.chunk_ranges.get(idx).cloned() - { - self.ensure_range_visible(range, content_area.height as usize, cache.wrapped.len()); + if let Some(idx) = self.pending_scroll_chunk.take() { + self.ensure_chunk_visible(idx, content_area); } - // Compute page bounds without holding an immutable borrow on cache while mutating self - let wrapped_len = self - .wrap_cache - .as_ref() - .map(|c| c.wrapped.len()) - .unwrap_or(0); self.scroll_offset = self .scroll_offset - .min(wrapped_len.saturating_sub(content_area.height as usize)); - let start = self.scroll_offset; - let end = (start + content_area.height as usize).min(wrapped_len); + .min(content_height.saturating_sub(content_area.height as usize)); - let wrapped = self.cached(); - let page = &wrapped[start..end]; - self.render_content_page_prepared(content_area, buf, page); - self.render_bottom_bar(area, content_area, buf, wrapped); + self.render_content(content_area, buf); + + self.render_bottom_bar(area, content_area, buf, content_height); } fn render_header(&self, area: Rect, buf: &mut Buffer) { @@ -139,20 +144,38 @@ impl PagerView { header.dim().render_ref(area, buf); } - // Removed unused render_content_page (replaced by render_content_page_prepared) + fn render_content(&self, area: Rect, buf: &mut Buffer) { + let mut y = -(self.scroll_offset as isize); + let mut drawn_bottom = area.y; + for renderable in &self.renderables { + let top = y; + let height = renderable.desired_height(area.width) as isize; + y += height; + let bottom = y; + if bottom < area.y as isize { + continue; + } + if top > area.y as isize + area.height as isize { + break; + } + if top < 0 { + let drawn = render_offset_content(area, buf, &**renderable, (-top) as u16); + drawn_bottom = drawn_bottom.max(area.y + drawn); + } else { + let draw_height = (height as u16).min(area.height.saturating_sub(top as u16)); + let draw_area = Rect::new(area.x, area.y + top as u16, area.width, draw_height); + renderable.render(draw_area, buf); + drawn_bottom = drawn_bottom.max(draw_area.y.saturating_add(draw_area.height)); + } + } - fn render_content_page_prepared(&self, area: Rect, buf: &mut Buffer, page: &[Line<'static>]) { - Clear.render(area, buf); - Paragraph::new(page.to_vec()).render_ref(area, buf); - - let visible = page.len(); - if visible < area.height as usize { - for i in 0..(area.height as usize - visible) { - let add = ((visible + i).min(u16::MAX as usize)) as u16; - let y = area.y.saturating_add(add); - Span::from("~") - .dim() - .render_ref(Rect::new(area.x, y, 1, 1), buf); + for y in drawn_bottom..area.bottom() { + if area.width == 0 { + break; + } + buf[(area.x, y)] = Cell::from('~'); + for x in area.x + 1..area.right() { + buf[(x, y)] = Cell::from(' '); } } } @@ -162,7 +185,7 @@ impl PagerView { full_area: Rect, content_area: Rect, buf: &mut Buffer, - wrapped: &[Line<'static>], + total_len: usize, ) { let sep_y = content_area.bottom(); let sep_rect = Rect::new(full_area.x, sep_y, full_area.width, 1); @@ -170,10 +193,10 @@ impl PagerView { Span::from("─".repeat(sep_rect.width as usize)) .dim() .render_ref(sep_rect, buf); - let percent = if wrapped.is_empty() { + let percent = if total_len == 0 { 100 } else { - let max_scroll = wrapped.len().saturating_sub(content_area.height as usize); + let max_scroll = total_len.saturating_sub(content_area.height as usize); if max_scroll == 0 { 100 } else { @@ -210,7 +233,7 @@ impl PagerView { kind: KeyEventKind::Press | KeyEventKind::Repeat, .. } => { - let area = self.scroll_area(tui.terminal.viewport_area); + let area = self.content_area(tui.terminal.viewport_area); self.scroll_offset = self.scroll_offset.saturating_sub(area.height as usize); } KeyEvent { @@ -218,7 +241,7 @@ impl PagerView { kind: KeyEventKind::Press | KeyEventKind::Repeat, .. } => { - let area = self.scroll_area(tui.terminal.viewport_area); + let area = self.content_area(tui.terminal.viewport_area); self.scroll_offset = self.scroll_offset.saturating_add(area.height as usize); } KeyEvent { @@ -248,7 +271,7 @@ impl PagerView { self.last_content_height = Some(height as usize); } - fn scroll_area(&self, area: Rect) -> Rect { + fn content_area(&self, area: Rect) -> Rect { let mut area = area; area.y = area.y.saturating_add(1); area.height = area.height.saturating_sub(2); @@ -256,67 +279,24 @@ impl PagerView { } } -#[derive(Debug, Clone)] -struct WrapCache { - width: u16, - wrapped: Vec>, - /// For each input Text chunk, the inclusive-excluded range of wrapped lines produced. - chunk_ranges: Vec>, - base_len: usize, -} - impl PagerView { - fn ensure_wrapped(&mut self, width: u16) { - let width = width.max(1); - let needs = match self.wrap_cache { - Some(ref c) => c.width != width || c.base_len != self.texts.len(), - None => true, - }; - if !needs { - return; - } - let mut wrapped: Vec> = Vec::new(); - let mut chunk_ranges: Vec> = Vec::with_capacity(self.texts.len()); - for text in &self.texts { - let start = wrapped.len(); - for line in &text.lines { - let ws = crate::wrapping::word_wrap_line(line, width as usize); - push_owned_lines(&ws, &mut wrapped); - } - let end = wrapped.len(); - chunk_ranges.push(start..end); - } - self.wrap_cache = Some(WrapCache { - width, - wrapped, - chunk_ranges, - base_len: self.texts.len(), - }); - } - - fn cached(&self) -> &[Line<'static>] { - if let Some(cache) = self.wrap_cache.as_ref() { - &cache.wrapped - } else { - &[] - } - } - fn is_scrolled_to_bottom(&self) -> bool { if self.scroll_offset == usize::MAX { return true; } - let Some(cache) = &self.wrap_cache else { - return false; - }; let Some(height) = self.last_content_height else { return false; }; - if cache.wrapped.is_empty() { + if self.renderables.is_empty() { return true; } - let visible = height.min(cache.wrapped.len()); - let max_scroll = cache.wrapped.len().saturating_sub(visible); + let Some(total_height) = self.last_rendered_height else { + return false; + }; + if total_height <= height { + return true; + } + let max_scroll = total_height.saturating_sub(height); self.scroll_offset >= max_scroll } @@ -325,32 +305,57 @@ impl PagerView { self.pending_scroll_chunk = Some(chunk_index); } - fn ensure_range_visible( - &mut self, - range: std::ops::Range, - viewport_height: usize, - total_wrapped: usize, - ) { - if viewport_height == 0 || total_wrapped == 0 { + fn ensure_chunk_visible(&mut self, idx: usize, area: Rect) { + if area.height == 0 || idx >= self.renderables.len() { return; } - let first = range.start.min(total_wrapped.saturating_sub(1)); - let last = range - .end - .saturating_sub(1) - .min(total_wrapped.saturating_sub(1)); - let current_top = self.scroll_offset.min(total_wrapped.saturating_sub(1)); - let current_bottom = current_top.saturating_add(viewport_height.saturating_sub(1)); - + let first = self + .renderables + .iter() + .take(idx) + .map(|r| r.desired_height(area.width) as usize) + .sum(); + let last = first + self.renderables[idx].desired_height(area.width) as usize; + let current_top = self.scroll_offset; + let current_bottom = current_top.saturating_add(area.height.saturating_sub(1) as usize); if first < current_top { self.scroll_offset = first; } else if last > current_bottom { - // Scroll just enough so that 'last' is visible at the bottom - self.scroll_offset = last.saturating_sub(viewport_height.saturating_sub(1)); + self.scroll_offset = last.saturating_sub(area.height.saturating_sub(1) as usize); } } } +struct CachedParagraph { + paragraph: Paragraph<'static>, + height: std::cell::Cell>, + last_width: std::cell::Cell>, +} + +impl CachedParagraph { + fn new(paragraph: Paragraph<'static>) -> Self { + Self { + paragraph, + height: std::cell::Cell::new(None), + last_width: std::cell::Cell::new(None), + } + } +} + +impl Renderable for CachedParagraph { + fn render(&self, area: Rect, buf: &mut Buffer) { + self.paragraph.render_ref(area, buf); + } + fn desired_height(&self, width: u16) -> u16 { + if self.last_width.get() != Some(width) { + let height = self.paragraph.line_count(width) as u16; + self.height.set(Some(height)); + self.last_width.set(Some(width)); + } + self.height.get().unwrap_or(0) + } +} + pub(crate) struct TranscriptOverlay { view: PagerView, cells: Vec>, @@ -375,8 +380,8 @@ impl TranscriptOverlay { fn render_cells_to_texts( cells: &[Arc], highlight_cell: Option, - ) -> Vec> { - let mut texts: Vec> = Vec::new(); + ) -> Vec> { + let mut texts: Vec> = Vec::new(); let mut first = true; for (idx, cell) in cells.iter().enumerate() { let mut lines: Vec> = Vec::new(); @@ -392,7 +397,9 @@ impl TranscriptOverlay { cell.transcript_lines() }; lines.extend(cell_lines); - texts.push(Text::from(lines)); + texts.push(Box::new(CachedParagraph::new( + Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }), + ))); first = false; } texts @@ -406,9 +413,10 @@ impl TranscriptOverlay { lines.push(Line::from("")); } lines.extend(cell.transcript_lines()); - self.view.texts.push(Text::from(lines)); + self.view.renderables.push(Box::new(CachedParagraph::new( + Paragraph::new(Text::from(lines)).wrap(Wrap { trim: false }), + ))); self.cells.push(cell); - self.view.wrap_cache = None; if follow_bottom { self.view.scroll_offset = usize::MAX; } @@ -416,8 +424,7 @@ impl TranscriptOverlay { pub(crate) fn set_highlight_cell(&mut self, cell: Option) { self.highlight_cell = cell; - self.view.wrap_cache = None; - self.view.texts = Self::render_cells_to_texts(&self.cells, self.highlight_cell); + self.view.renderables = Self::render_cells_to_texts(&self.cells, self.highlight_cell); if let Some(idx) = self.highlight_cell { self.view.scroll_chunk_into_view(idx); } @@ -490,8 +497,17 @@ pub(crate) struct StaticOverlay { impl StaticOverlay { pub(crate) fn with_title(lines: Vec>, title: String) -> Self { + Self::with_renderables( + vec![Box::new(CachedParagraph::new(Paragraph::new(Text::from( + lines, + ))))], + title, + ) + } + + pub(crate) fn with_renderables(renderables: Vec>, title: String) -> Self { Self { - view: PagerView::new(vec![Text::from(lines)], title, 0), + view: PagerView::new(renderables, title, 0), is_done: false, } } @@ -547,6 +563,33 @@ impl StaticOverlay { } } +fn render_offset_content( + area: Rect, + buf: &mut Buffer, + renderable: &dyn Renderable, + scroll_offset: u16, +) -> u16 { + let height = renderable.desired_height(area.width); + let mut tall_buf = Buffer::empty(Rect::new( + 0, + 0, + area.width, + height.min(area.height + scroll_offset), + )); + renderable.render(*tall_buf.area(), &mut tall_buf); + let copy_height = area + .height + .min(tall_buf.area().height.saturating_sub(scroll_offset)); + for y in 0..copy_height { + let src_y = y + scroll_offset; + for x in 0..area.width { + buf[(area.x + x, area.y + y)] = tall_buf[(x, src_y)].clone(); + } + } + + copy_height +} + #[cfg(test)] mod tests { use super::*; @@ -558,12 +601,12 @@ mod tests { use crate::exec_cell::CommandOutput; use crate::history_cell::HistoryCell; - use crate::history_cell::PatchEventType; use crate::history_cell::new_patch_event; use codex_core::protocol::FileChange; use codex_protocol::parse_command::ParsedCommand; use ratatui::Terminal; use ratatui::backend::TestBackend; + use ratatui::text::Text; #[derive(Debug)] struct TestCell { @@ -580,6 +623,15 @@ mod tests { } } + fn paragraph_block(label: &str, lines: usize) -> Box { + let text = Text::from( + (0..lines) + .map(|i| Line::from(format!("{label}{i}"))) + .collect::>(), + ); + Box::new(Paragraph::new(text)) as Box + } + #[test] fn edit_prev_hint_is_visible() { let mut overlay = TranscriptOverlay::new(vec![Arc::new(TestCell { @@ -657,11 +709,7 @@ mod tests { content: "hello\nworld\n".to_string(), }, ); - let approval_cell: Arc = Arc::new(new_patch_event( - PatchEventType::ApprovalRequest, - approval_changes, - &cwd, - )); + let approval_cell: Arc = Arc::new(new_patch_event(approval_changes, &cwd)); cells.push(approval_cell); let mut apply_changes = HashMap::new(); @@ -671,13 +719,7 @@ mod tests { content: "hello\nworld\n".to_string(), }, ); - let apply_begin_cell: Arc = Arc::new(new_patch_event( - PatchEventType::ApplyBegin { - auto_approved: false, - }, - apply_changes, - &cwd, - )); + let apply_begin_cell: Arc = Arc::new(new_patch_event(apply_changes, &cwd)); cells.push(apply_begin_cell); let apply_end_cell: Arc = @@ -711,7 +753,6 @@ mod tests { overlay.render(area, &mut buf); overlay.view.scroll_offset = 0; - overlay.view.wrap_cache = None; overlay.render(area, &mut buf); let snapshot = buffer_to_text(&buf, area); @@ -783,54 +824,89 @@ mod tests { } #[test] - fn pager_wrap_cache_reuses_for_same_width_and_rebuilds_on_change() { - let long = "This is a long line that should wrap multiple times to ensure non-empty wrapped output."; - let mut pv = PagerView::new( - vec![Text::from(vec![long.into()]), Text::from(vec![long.into()])], + fn pager_view_content_height_counts_renderables() { + let pv = PagerView::new( + vec![paragraph_block("a", 2), paragraph_block("b", 3)], "T".to_string(), 0, ); - // Build cache at width 24 - pv.ensure_wrapped(24); - let w1 = pv.cached(); - assert!(!w1.is_empty(), "expected wrapped output to be non-empty"); - let ptr1 = w1.as_ptr(); + assert_eq!(pv.content_height(80), 5); + } - // Re-run with same width: cache should be reused (pointer stability heuristic) - pv.ensure_wrapped(24); - let w2 = pv.cached(); - let ptr2 = w2.as_ptr(); - assert_eq!(ptr1, ptr2, "cache should not rebuild for unchanged width"); + #[test] + fn pager_view_ensure_chunk_visible_scrolls_down_when_needed() { + let mut pv = PagerView::new( + vec![ + paragraph_block("a", 1), + paragraph_block("b", 3), + paragraph_block("c", 3), + ], + "T".to_string(), + 0, + ); + let area = Rect::new(0, 0, 20, 8); - // Change width: cache should rebuild and likely produce different length - // Drop immutable borrow before mutating - let prev_len = w2.len(); - pv.ensure_wrapped(36); - let w3 = pv.cached(); - assert_ne!( - prev_len, - w3.len(), - "wrapped length should change on width change" + pv.scroll_offset = 0; + let content_area = pv.content_area(area); + pv.ensure_chunk_visible(2, content_area); + + let mut buf = Buffer::empty(area); + pv.render(area, &mut buf); + let rendered = buffer_to_text(&buf, area); + + assert!( + rendered.contains("c0"), + "expected chunk top in view: {rendered:?}" + ); + assert!( + rendered.contains("c1"), + "expected chunk middle in view: {rendered:?}" + ); + assert!( + rendered.contains("c2"), + "expected chunk bottom in view: {rendered:?}" ); } #[test] - fn pager_wrap_cache_invalidates_on_append() { - let long = "Another long line for wrapping behavior verification."; - let mut pv = PagerView::new(vec![Text::from(vec![long.into()])], "T".to_string(), 0); - pv.ensure_wrapped(28); - let w1 = pv.cached(); - let len1 = w1.len(); + fn pager_view_ensure_chunk_visible_scrolls_up_when_needed() { + let mut pv = PagerView::new( + vec![ + paragraph_block("a", 2), + paragraph_block("b", 3), + paragraph_block("c", 3), + ], + "T".to_string(), + 0, + ); + let area = Rect::new(0, 0, 20, 3); + + pv.scroll_offset = 6; + pv.ensure_chunk_visible(0, area); + + assert_eq!(pv.scroll_offset, 0); + } + + #[test] + fn pager_view_is_scrolled_to_bottom_accounts_for_wrapped_height() { + let mut pv = PagerView::new(vec![paragraph_block("a", 10)], "T".to_string(), 0); + let area = Rect::new(0, 0, 20, 8); + let mut buf = Buffer::empty(area); + + pv.render(area, &mut buf); - // Append new lines should cause ensure_wrapped to rebuild due to len change - pv.texts.push(Text::from(vec![long.into()])); - pv.texts.push(Text::from(vec![long.into()])); - pv.ensure_wrapped(28); - let w2 = pv.cached(); assert!( - w2.len() >= len1, - "wrapped length should grow or stay same after append" + !pv.is_scrolled_to_bottom(), + "expected view to report not at bottom when offset < max" + ); + + pv.scroll_offset = usize::MAX; + pv.render(area, &mut buf); + + assert!( + pv.is_scrolled_to_bottom(), + "expected view to report at bottom after scrolling to end" ); } } diff --git a/codex-rs/tui/src/render/mod.rs b/codex-rs/tui/src/render/mod.rs index e457423f..441c1d6b 100644 --- a/codex-rs/tui/src/render/mod.rs +++ b/codex-rs/tui/src/render/mod.rs @@ -1,2 +1,47 @@ +use ratatui::layout::Rect; + pub mod highlight; pub mod line_utils; +pub mod renderable; + +pub struct Insets { + pub left: u16, + pub top: u16, + pub right: u16, + pub bottom: u16, +} + +impl Insets { + pub fn tlbr(top: u16, left: u16, bottom: u16, right: u16) -> Self { + Self { + top, + left, + bottom, + right, + } + } + + pub fn vh(v: u16, h: u16) -> Self { + Self { + top: v, + left: h, + bottom: v, + right: h, + } + } +} + +pub trait RectExt { + fn inset(&self, insets: Insets) -> Rect; +} + +impl RectExt for Rect { + fn inset(&self, insets: Insets) -> Rect { + Rect { + x: self.x + insets.left, + y: self.y + insets.top, + width: self.width - insets.left - insets.right, + height: self.height - insets.top - insets.bottom, + } + } +} diff --git a/codex-rs/tui/src/render/renderable.rs b/codex-rs/tui/src/render/renderable.rs new file mode 100644 index 00000000..868d0726 --- /dev/null +++ b/codex-rs/tui/src/render/renderable.rs @@ -0,0 +1,102 @@ +use ratatui::buffer::Buffer; +use ratatui::layout::Rect; +use ratatui::text::Line; +use ratatui::widgets::Paragraph; +use ratatui::widgets::WidgetRef; + +pub trait Renderable { + fn render(&self, area: Rect, buf: &mut Buffer); + fn desired_height(&self, width: u16) -> u16; +} + +impl Renderable for () { + fn render(&self, _area: Rect, _buf: &mut Buffer) {} + fn desired_height(&self, _width: u16) -> u16 { + 0 + } +} + +impl Renderable for &str { + fn render(&self, area: Rect, buf: &mut Buffer) { + self.render_ref(area, buf); + } + fn desired_height(&self, _width: u16) -> u16 { + 1 + } +} + +impl Renderable for String { + fn render(&self, area: Rect, buf: &mut Buffer) { + self.render_ref(area, buf); + } + fn desired_height(&self, _width: u16) -> u16 { + 1 + } +} + +impl<'a> Renderable for Line<'a> { + fn render(&self, area: Rect, buf: &mut Buffer) { + WidgetRef::render_ref(self, area, buf); + } + fn desired_height(&self, _width: u16) -> u16 { + 1 + } +} + +impl<'a> Renderable for Paragraph<'a> { + fn render(&self, area: Rect, buf: &mut Buffer) { + self.render_ref(area, buf); + } + fn desired_height(&self, width: u16) -> u16 { + self.line_count(width) as u16 + } +} + +impl Renderable for Option { + fn render(&self, area: Rect, buf: &mut Buffer) { + if let Some(renderable) = self { + renderable.render(area, buf); + } + } + + fn desired_height(&self, width: u16) -> u16 { + if let Some(renderable) = self { + renderable.desired_height(width) + } else { + 0 + } + } +} + +pub struct ColumnRenderable { + children: Vec>, +} + +impl Renderable for ColumnRenderable { + fn render(&self, area: Rect, buf: &mut Buffer) { + let mut y = area.y; + for child in &self.children { + let child_area = Rect::new(area.x, y, area.width, child.desired_height(area.width)) + .intersection(area); + if !child_area.is_empty() { + child.render(child_area, buf); + } + y += child_area.height; + } + } + + fn desired_height(&self, width: u16) -> u16 { + self.children + .iter() + .map(|child| child.desired_height(width)) + .sum() + } +} + +impl ColumnRenderable { + pub fn new(children: impl IntoIterator>) -> Self { + Self { + children: children.into_iter().collect(), + } + } +} diff --git a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap index 12c10ca4..d188b2fd 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__diff_render__tests__apply_update_block_manual.snap @@ -2,11 +2,11 @@ source: tui/src/diff_render.rs expression: terminal.backend() --- -"• Change Approved example.txt (+1 -1) " -" " -" " -" " -" " +"• Edited example.txt (+1 -1) " +" 1 line one " +" 2 -line two " +" 2 +line two changed " +" 3 line three " " " " " " " diff --git a/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap b/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap index 725f210c..df5f17ad 100644 --- a/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap +++ b/codex-rs/tui/src/snapshots/codex_tui__pager_overlay__tests__transcript_overlay_apply_patch_scroll_vt100.snap @@ -1,16 +1,15 @@ --- source: tui/src/pager_overlay.rs -assertion_line: 721 expression: snapshot --- / T R A N S C R I P T / / / / / / / / / / / / / / / / / / / / / / / / / / / / / -• Proposed Change foo.txt (+2 -0) +• Added foo.txt (+2 -0) 1 +hello 2 +world -• Change Approved foo.txt (+2 -0) - -✓ Patch applied +• Added foo.txt (+2 -0) + 1 +hello + 2 +world ─────────────────────────────────────────────────────────────────────────── 0% ─ ↑/↓ scroll PgUp/PgDn page Home/End jump q quit Esc edit prev diff --git a/codex-rs/tui/src/status_indicator_widget.rs b/codex-rs/tui/src/status_indicator_widget.rs index b6fa2fd5..afb79520 100644 --- a/codex-rs/tui/src/status_indicator_widget.rs +++ b/codex-rs/tui/src/status_indicator_widget.rs @@ -17,7 +17,6 @@ use crate::app_event_sender::AppEventSender; use crate::key_hint; use crate::shimmer::shimmer_spans; use crate::tui::FrameRequester; -use crate::ui_consts::LIVE_PREFIX_COLS; pub(crate) struct StatusIndicatorWidget { /// Animated header text (defaults to "Working"). @@ -160,7 +159,7 @@ impl WidgetRef for StatusIndicatorWidget { let pretty_elapsed = fmt_elapsed_compact(elapsed); // Plain rendering: no borders or padding so the live cell is visually indistinguishable from terminal scrollback. - let mut spans = vec![" ".repeat(LIVE_PREFIX_COLS as usize).into()]; + let mut spans = vec![" ".into()]; spans.extend(shimmer_spans(&self.header)); spans.extend(vec![ " ".into(),