tui: tweaks to dialog display (#4622)

- prefix command approval reasons with "Reason:"
- show keyboard shortcuts for some ListSelectionItems
- remove "description" lines for approval options, and make the labels
more verbose
- add a spacer line in diff display after the path

and some other minor refactors that go along with the above.

<img width="859" height="508" alt="Screenshot 2025-10-02 at 1 24 50 PM"
src="https://github.com/user-attachments/assets/4fa7ecaf-3d3a-406a-bb4d-23e30ce3e5cf"
/>
This commit is contained in:
Jeremy Rose
2025-10-02 14:41:29 -07:00
committed by GitHub
parent 62cc8a4b8d
commit 25a2e15ec5
13 changed files with 159 additions and 150 deletions

View File

@@ -12,6 +12,7 @@ use crate::diff_render::DiffSummary;
use crate::exec_command::strip_bash_lc_and_escape;
use crate::history_cell;
use crate::key_hint;
use crate::key_hint::KeyBinding;
use crate::render::highlight::highlight_bash_to_lines;
use crate::render::renderable::ColumnRenderable;
use crate::render::renderable::Renderable;
@@ -94,8 +95,14 @@ impl ApprovalOverlay {
header: Box<dyn Renderable>,
) -> (Vec<ApprovalOption>, SelectionViewParams) {
let (options, title) = match &variant {
ApprovalVariant::Exec { .. } => (exec_options(), "Allow command?".to_string()),
ApprovalVariant::ApplyPatch { .. } => (patch_options(), "Apply changes?".to_string()),
ApprovalVariant::Exec { .. } => (
exec_options(),
"Would you like to run the following command?".to_string(),
),
ApprovalVariant::ApplyPatch { .. } => (
patch_options(),
"Would you like to make the following edits?".to_string(),
),
};
let header = Box::new(ColumnRenderable::new([
@@ -108,11 +115,9 @@ impl ApprovalOverlay {
.iter()
.map(|opt| SelectionItem {
name: opt.label.clone(),
description: Some(opt.description.clone()),
is_current: false,
actions: Vec::new(),
display_shortcut: opt.display_shortcut,
dismiss_on_select: false,
search_value: None,
..Default::default()
})
.collect();
@@ -197,28 +202,18 @@ impl ApprovalOverlay {
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
e => {
if let Some(idx) = self
.options
.iter()
.position(|opt| opt.shortcut.map(|s| s == lower).unwrap_or(false))
.position(|opt| opt.shortcuts().any(|s| s.is_press(*e)))
{
Some(idx) => {
self.apply_selection(idx);
true
}
None => false,
self.apply_selection(idx);
true
} else {
false
}
}
_ => false,
}
}
}
@@ -299,7 +294,7 @@ impl From<ApprovalRequest> for ApprovalRequestState {
if let Some(reason) = reason
&& !reason.is_empty()
{
header.push(reason.italic().into());
header.push(Line::from(vec!["Reason: ".into(), reason.italic()]));
header.push(Line::from(""));
}
let full_cmd = strip_bash_lc_and_escape(&command);
@@ -347,31 +342,38 @@ enum ApprovalVariant {
#[derive(Clone)]
struct ApprovalOption {
label: String,
description: String,
decision: ReviewDecision,
shortcut: Option<char>,
display_shortcut: Option<KeyBinding>,
additional_shortcuts: Vec<KeyBinding>,
}
impl ApprovalOption {
fn shortcuts(&self) -> impl Iterator<Item = KeyBinding> + '_ {
self.display_shortcut
.into_iter()
.chain(self.additional_shortcuts.iter().copied())
}
}
fn exec_options() -> Vec<ApprovalOption> {
vec![
ApprovalOption {
label: "Approve and run now".to_string(),
description: "Run this command one time".to_string(),
label: "Yes, proceed".to_string(),
decision: ReviewDecision::Approved,
shortcut: Some('y'),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
},
ApprovalOption {
label: "Always approve this session".to_string(),
description: "Automatically approve this command for the rest of the session"
.to_string(),
label: "Yes, and don't ask again for this command".to_string(),
decision: ReviewDecision::ApprovedForSession,
shortcut: Some('a'),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
},
ApprovalOption {
label: "Cancel".to_string(),
description: "Do not run the command".to_string(),
label: "No, and tell Codex what to do differently".to_string(),
decision: ReviewDecision::Abort,
shortcut: Some('n'),
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
},
]
}
@@ -379,16 +381,16 @@ fn exec_options() -> Vec<ApprovalOption> {
fn patch_options() -> Vec<ApprovalOption> {
vec![
ApprovalOption {
label: "Approve".to_string(),
description: "Apply the proposed changes".to_string(),
label: "Yes, proceed".to_string(),
decision: ReviewDecision::Approved,
shortcut: Some('y'),
display_shortcut: None,
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
},
ApprovalOption {
label: "Cancel".to_string(),
description: "Do not apply the changes".to_string(),
label: "No, and tell Codex what to do differently".to_string(),
decision: ReviewDecision::Abort,
shortcut: Some('n'),
display_shortcut: Some(key_hint::plain(KeyCode::Esc)),
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('n'))],
},
]
}

View File

@@ -173,6 +173,7 @@ impl CommandPopup {
name,
match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()),
is_current: false,
display_shortcut: None,
description: Some(description),
}
})

View File

@@ -130,6 +130,7 @@ impl WidgetRef for &FileSearchPopup {
.as_ref()
.map(|v| v.iter().map(|&i| i as usize).collect()),
is_current: false,
display_shortcut: None,
description: None,
})
.collect()

View File

@@ -1,6 +1,7 @@
use crossterm::event::KeyCode;
use crossterm::event::KeyEvent;
use crossterm::event::KeyModifiers;
use itertools::Itertools as _;
use ratatui::buffer::Buffer;
use ratatui::layout::Constraint;
use ratatui::layout::Layout;
@@ -13,6 +14,7 @@ use ratatui::widgets::Paragraph;
use ratatui::widgets::Widget;
use crate::app_event_sender::AppEventSender;
use crate::key_hint::KeyBinding;
use crate::render::Insets;
use crate::render::RectExt as _;
use crate::render::renderable::ColumnRenderable;
@@ -31,8 +33,10 @@ use super::selection_popup_common::render_rows;
/// One selectable item in the generic selection list.
pub(crate) type SelectionAction = Box<dyn Fn(&AppEventSender) + Send + Sync>;
#[derive(Default)]
pub(crate) struct SelectionItem {
pub name: String,
pub display_shortcut: Option<KeyBinding>,
pub description: Option<String>,
pub is_current: bool,
pub actions: Vec<SelectionAction>,
@@ -135,18 +139,10 @@ impl ListSelectionView {
self.filtered_indices = self
.items
.iter()
.enumerate()
.filter_map(|(idx, item)| {
let matches = if let Some(search_value) = &item.search_value {
search_value.to_lowercase().contains(&query_lower)
} else {
let mut matches = item.name.to_lowercase().contains(&query_lower);
if !matches && let Some(desc) = &item.description {
matches = desc.to_lowercase().contains(&query_lower);
}
matches
};
matches.then_some(idx)
.positions(|item| {
item.search_value
.as_ref()
.is_some_and(|v| v.to_lowercase().contains(&query_lower))
})
.collect();
} else {
@@ -200,6 +196,7 @@ impl ListSelectionView {
};
GenericDisplayRow {
name: display_name,
display_shortcut: item.display_shortcut,
match_indices: None,
is_current: item.is_current,
description: item.description.clone(),
@@ -329,7 +326,8 @@ impl Renderable for ListSelectionView {
let rows_height = measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, width);
let mut height = self.header.desired_height(width);
// Subtract 4 for the padding on the left and right of the header.
let mut height = self.header.desired_height(width.saturating_sub(4));
height = height.saturating_add(rows_height + 3);
if self.is_searchable {
height = height.saturating_add(1);
@@ -355,7 +353,10 @@ impl Renderable for ListSelectionView {
.style(user_message_style(terminal_palette::default_bg()))
.render(content_area, buf);
let header_height = self.header.desired_height(content_area.width);
let header_height = self
.header
// Subtract 4 for the padding on the left and right of the header.
.desired_height(content_area.width.saturating_sub(4));
let rows = self.build_rows();
let rows_height =
measure_rows_height(&rows, &self.state, MAX_POPUP_ROWS, content_area.width);
@@ -438,17 +439,15 @@ mod tests {
name: "Read Only".to_string(),
description: Some("Codex can read files".to_string()),
is_current: true,
actions: vec![],
dismiss_on_select: true,
search_value: None,
..Default::default()
},
SelectionItem {
name: "Full Access".to_string(),
description: Some("Codex can edit files".to_string()),
is_current: false,
actions: vec![],
dismiss_on_select: true,
search_value: None,
..Default::default()
},
];
ListSelectionView::new(
@@ -510,9 +509,8 @@ mod tests {
name: "Read Only".to_string(),
description: Some("Codex can read files".to_string()),
is_current: false,
actions: vec![],
dismiss_on_select: true,
search_value: None,
..Default::default()
}];
let mut view = ListSelectionView::new(
SelectionViewParams {

View File

@@ -9,11 +9,14 @@ use ratatui::text::Span;
use ratatui::widgets::Widget;
use unicode_width::UnicodeWidthChar;
use crate::key_hint::KeyBinding;
use super::scroll_state::ScrollState;
/// A generic representation of a display row for selection popups.
pub(crate) struct GenericDisplayRow {
pub name: String,
pub display_shortcut: Option<KeyBinding>,
pub match_indices: Option<Vec<usize>>, // indices to bold (char positions)
pub is_current: bool,
pub description: Option<String>, // optional grey text after the name
@@ -92,6 +95,10 @@ fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> {
let this_name_width = Line::from(name_spans.clone()).width();
let mut full_spans: Vec<Span> = name_spans;
if let Some(display_shortcut) = row.display_shortcut {
full_spans.push(" ".into());
full_spans.push(display_shortcut.into());
}
if let Some(desc) = row.description.as_ref() {
let gap = desc_col.saturating_sub(this_name_width);
if gap > 0 {
@@ -155,6 +162,7 @@ pub(crate) fn render_rows(
let GenericDisplayRow {
name,
match_indices,
display_shortcut,
is_current: _is_current,
description,
} = row;
@@ -163,6 +171,7 @@ pub(crate) fn render_rows(
&GenericDisplayRow {
name: name.clone(),
match_indices: match_indices.clone(),
display_shortcut: *display_shortcut,
is_current: *_is_current,
description: description.clone(),
},