From 45bccd36b038a28b23663189cc6f7557e49e06d0 Mon Sep 17 00:00:00 2001 From: easong-openai Date: Mon, 15 Sep 2025 17:34:04 -0700 Subject: [PATCH] fix permissions alignment --- codex-rs/tui/src/bottom_pane/chat_composer.rs | 11 +- codex-rs/tui/src/bottom_pane/command_popup.rs | 33 +- .../src/bottom_pane/list_selection_view.rs | 34 ++- .../src/bottom_pane/selection_popup_common.rs | 288 +++++++++++++----- ..._chat_composer__tests__slash_popup_mo.snap | 2 +- ..._list_selection_spacing_with_subtitle.snap | 2 +- ...st_selection_spacing_without_subtitle.snap | 2 +- 7 files changed, 283 insertions(+), 89 deletions(-) diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 2138d540..bf11ff57 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -142,14 +142,16 @@ impl ChatComposer { .desired_height(width.saturating_sub(LIVE_PREFIX_COLS)) + match &self.active_popup { ActivePopup::None => FOOTER_HEIGHT_WITH_HINT, - ActivePopup::Command(c) => c.calculate_required_height(), + ActivePopup::Command(c) => c.calculate_required_height(width), ActivePopup::File(c) => c.calculate_required_height(), } } pub fn cursor_pos(&self, area: Rect) -> Option<(u16, u16)> { let popup_constraint = match &self.active_popup { - ActivePopup::Command(popup) => Constraint::Max(popup.calculate_required_height()), + ActivePopup::Command(popup) => { + Constraint::Max(popup.calculate_required_height(area.width)) + } ActivePopup::File(popup) => Constraint::Max(popup.calculate_required_height()), ActivePopup::None => Constraint::Max(FOOTER_HEIGHT_WITH_HINT), }; @@ -1232,7 +1234,10 @@ impl ChatComposer { impl WidgetRef for ChatComposer { fn render_ref(&self, area: Rect, buf: &mut Buffer) { let (popup_constraint, hint_spacing) = match &self.active_popup { - ActivePopup::Command(popup) => (Constraint::Max(popup.calculate_required_height()), 0), + ActivePopup::Command(popup) => ( + Constraint::Max(popup.calculate_required_height(area.width)), + 0, + ), ActivePopup::File(popup) => (Constraint::Max(popup.calculate_required_height()), 0), ActivePopup::None => ( Constraint::Length(FOOTER_HEIGHT_WITH_HINT), diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index a5961e31..8de266f8 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -92,10 +92,35 @@ impl CommandPopup { .ensure_visible(matches_len, MAX_POPUP_ROWS.min(matches_len)); } - /// Determine the preferred height of the popup. This is the number of - /// rows required to show at most MAX_POPUP_ROWS commands. - pub(crate) fn calculate_required_height(&self) -> u16 { - self.filtered_items().len().clamp(1, MAX_POPUP_ROWS) as u16 + /// Determine the preferred height of the popup for a given width. + /// Accounts for wrapped descriptions so that long tooltips don't overflow. + pub(crate) fn calculate_required_height(&self, width: u16) -> u16 { + use super::selection_popup_common::GenericDisplayRow; + use super::selection_popup_common::measure_rows_height; + let matches = self.filtered(); + let rows_all: Vec = if matches.is_empty() { + Vec::new() + } else { + matches + .into_iter() + .map(|(item, indices, _)| match item { + CommandItem::Builtin(cmd) => GenericDisplayRow { + name: format!("/{}", cmd.command()), + match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), + is_current: false, + description: Some(cmd.description().to_string()), + }, + CommandItem::UserPrompt(i) => GenericDisplayRow { + name: format!("/{}", self.prompts[i].name), + match_indices: indices.map(|v| v.into_iter().map(|i| i + 1).collect()), + is_current: false, + description: Some("send saved prompt".to_string()), + }, + }) + .collect() + }; + + measure_rows_height(&rows_all, &self.state, MAX_POPUP_ROWS, width) } /// Compute fuzzy-filtered matches over built-in commands and user prompts, 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 7be91db2..5d5dbf0f 100644 --- a/codex-rs/tui/src/bottom_pane/list_selection_view.rs +++ b/codex-rs/tui/src/bottom_pane/list_selection_view.rs @@ -17,6 +17,7 @@ use super::bottom_pane_view::BottomPaneView; use super::popup_consts::MAX_POPUP_ROWS; use super::scroll_state::ScrollState; use super::selection_popup_common::GenericDisplayRow; +use super::selection_popup_common::measure_rows_height; use super::selection_popup_common::render_rows; /// One selectable item in the generic selection list. @@ -135,11 +136,36 @@ impl BottomPaneView for ListSelectionView { CancellationEvent::Handled } - fn desired_height(&self, _width: u16) -> u16 { - let rows = (self.items.len()).clamp(1, MAX_POPUP_ROWS); + 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. + let rows: Vec = self + .items + .iter() + .enumerate() + .map(|(i, it)| { + let is_selected = self.state.selected_idx == Some(i); + let prefix = if is_selected { '>' } else { ' ' }; + let name_with_marker = if it.is_current { + format!("{} (current)", it.name) + } else { + it.name.clone() + }; + let display_name = format!("{} {}. {}", prefix, i + 1, name_with_marker); + GenericDisplayRow { + name: display_name, + match_indices: None, + is_current: it.is_current, + description: it.description.clone(), + } + }) + .collect(); + + 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 - let mut height = rows as u16 + 2; + // +1 for optional subtitle, +1 for optional footer (2 lines incl. spacing) + let mut height = rows_height + 2; if self.subtitle.is_some() { // +1 for subtitle (the spacer is accounted for above) height = height.saturating_add(1); 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 3852d10b..61a26f93 100644 --- a/codex-rs/tui/src/bottom_pane/selection_popup_common.rs +++ b/codex-rs/tui/src/bottom_pane/selection_popup_common.rs @@ -1,6 +1,7 @@ use ratatui::buffer::Buffer; use ratatui::layout::Rect; -use ratatui::prelude::Constraint; +// 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; @@ -10,9 +11,7 @@ use ratatui::text::Span; use ratatui::widgets::Block; use ratatui::widgets::BorderType; use ratatui::widgets::Borders; -use ratatui::widgets::Cell; -use ratatui::widgets::Row; -use ratatui::widgets::Table; +use ratatui::widgets::Paragraph; use ratatui::widgets::Widget; use super::scroll_state::ScrollState; @@ -27,6 +26,61 @@ pub(crate) struct GenericDisplayRow { 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. +fn compute_desc_col( + rows_all: &[GenericDisplayRow], + start_idx: usize, + visible_items: usize, + content_width: u16, +) -> usize { + let visible_range = start_idx..(start_idx + visible_items); + let max_name_width = rows_all + .iter() + .enumerate() + .filter(|(i, _)| visible_range.contains(i)) + .map(|(_, r)| Line::from(r.name.clone()).width()) + .max() + .unwrap_or(0); + let mut desc_col = max_name_width.saturating_add(2); + if (desc_col as u16) >= content_width { + desc_col = content_width.saturating_sub(1) as usize; + } + desc_col +} + +/// Build the full display line for a row with the description padded to start +/// at `desc_col`. Applies fuzzy-match bolding when indices are present and +/// dims the description. +fn build_full_line(row: &GenericDisplayRow, desc_col: usize) -> Line<'static> { + let mut name_spans: Vec = Vec::with_capacity(row.name.len()); + if let Some(idxs) = row.match_indices.as_ref() { + let mut idx_iter = idxs.iter().peekable(); + for (char_idx, ch) in row.name.chars().enumerate() { + if idx_iter.peek().is_some_and(|next| **next == char_idx) { + idx_iter.next(); + name_spans.push(ch.to_string().bold()); + } else { + name_spans.push(ch.to_string().into()); + } + } + } else { + name_spans.push(row.name.clone().into()); + } + + let this_name_width = Line::from(name_spans.clone()).width(); + let mut full_spans: Vec = name_spans; + if let Some(desc) = row.description.as_ref() { + let gap = desc_col.saturating_sub(this_name_width); + if gap > 0 { + full_spans.push(" ".repeat(gap).into()); + } + full_spans.push(desc.clone().dim()); + } + Line::from(full_spans) +} + /// Render a list of rows using the provided ScrollState, with shared styling /// and behavior for selection popups. pub(crate) fn render_rows( @@ -38,84 +92,168 @@ pub(crate) fn render_rows( _dim_non_selected: bool, empty_message: &str, ) { - let mut rows: Vec = Vec::new(); + // 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. + let content_area = Rect { + x: area.x.saturating_add(1), + y: area.y, + width: area.width.saturating_sub(1), + height: area.height, + }; + if rows_all.is_empty() { - rows.push(Row::new(vec![Cell::from(Line::from( - empty_message.dim().italic(), - ))])); - } else { - let max_rows_from_area = area.height as usize; - let visible_rows = max_results - .min(rows_all.len()) - .min(max_rows_from_area.max(1)); - - // Compute starting index based on scroll state and selection. - let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); - if let Some(sel) = state.selected_idx { - if sel < start_idx { - start_idx = sel; - } else if visible_rows > 0 { - let bottom = start_idx + visible_rows - 1; - if sel > bottom { - start_idx = sel + 1 - visible_rows; - } - } + 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, + ); } + return; + } - for (i, row) in rows_all - .iter() - .enumerate() - .skip(start_idx) - .take(visible_rows) - { - let GenericDisplayRow { - name, - match_indices, - is_current: _is_current, - description, - } = row; + // 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)); - // Highlight fuzzy indices when present. - let mut spans: Vec = Vec::with_capacity(name.len()); - if let Some(idxs) = match_indices.as_ref() { - let mut idx_iter = idxs.iter().peekable(); - for (char_idx, ch) in name.chars().enumerate() { - if idx_iter.peek().is_some_and(|next| **next == char_idx) { - idx_iter.next(); - spans.push(ch.to_string().bold()); - } else { - spans.push(ch.to_string().into()); - } - } - } else { - spans.push(name.clone().into()); + let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); + if let Some(sel) = state.selected_idx { + if sel < start_idx { + start_idx = sel; + } else if visible_items > 0 { + let bottom = start_idx + visible_items - 1; + if sel > bottom { + start_idx = sel + 1 - visible_items; } - - if let Some(desc) = description.as_ref() { - spans.push(" ".into()); - spans.push(desc.clone().dim()); - } - - let mut cell = Cell::from(Line::from(spans)); - if Some(i) == state.selected_idx { - cell = cell.style( - Style::default() - .fg(Color::Cyan) - .add_modifier(Modifier::BOLD), - ); - } - rows.push(Row::new(vec![cell])); } } - let table = Table::new(rows, vec![Constraint::Percentage(100)]) - .block( - Block::default() - .borders(Borders::LEFT) - .border_type(BorderType::QuadrantOutside) - .border_style(Style::default().add_modifier(Modifier::DIM)), - ) - .widths([Constraint::Percentage(100)]); + let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_area.width); - table.render(area, buf); + // 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; + for (i, row) in rows_all + .iter() + .enumerate() + .skip(start_idx) + .take(visible_items) + { + if cur_y >= content_area.y + content_area.height { + break; + } + + let GenericDisplayRow { + name, + match_indices, + is_current: _is_current, + description, + } = row; + + let full_line = build_full_line( + &GenericDisplayRow { + name: name.clone(), + match_indices: match_indices.clone(), + is_current: *_is_current, + description: description.clone(), + }, + desc_col, + ); + + // 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) + .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 { + 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( + Rect { + x: content_area.x, + y: cur_y, + width: content_area.width, + height: 1, + }, + buf, + ); + cur_y = cur_y.saturating_add(1); + } + } +} + +/// Compute the number of terminal rows required to render up to `max_results` +/// items from `rows_all` given the current scroll/selection state and the +/// available `width`. Accounts for description wrapping and alignment so the +/// caller can allocate sufficient vertical space. +pub(crate) fn measure_rows_height( + rows_all: &[GenericDisplayRow], + state: &ScrollState, + max_results: usize, + width: u16, +) -> u16 { + if rows_all.is_empty() { + return 1; // placeholder "no matches" line + } + + let content_width = width.saturating_sub(1).max(1); + + let visible_items = max_results.min(rows_all.len()); + let mut start_idx = state.scroll_top.min(rows_all.len().saturating_sub(1)); + if let Some(sel) = state.selected_idx { + if sel < start_idx { + start_idx = sel; + } else if visible_items > 0 { + let bottom = start_idx + visible_items - 1; + if sel > bottom { + start_idx = sel + 1 - visible_items; + } + } + } + + let desc_col = compute_desc_col(rows_all, start_idx, visible_items, content_width); + + use crate::wrapping::RtOptions; + use crate::wrapping::word_wrap_line; + let mut total: u16 = 0; + for row in rows_all + .iter() + .enumerate() + .skip(start_idx) + .take(visible_items) + .map(|(_, r)| r) + { + let full_line = build_full_line(row, desc_col); + let opts = RtOptions::new(content_width as usize) + .initial_indent(Line::from("")) + .subsequent_indent(Line::from(" ".repeat(desc_col))); + total = total.saturating_add(word_wrap_line(&full_line, opts).len() as u16); + } + total.max(1) } diff --git a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap index b626f80c..f908fb61 100644 --- a/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap +++ b/codex-rs/tui/src/bottom_pane/snapshots/codex_tui__bottom_pane__chat_composer__tests__slash_popup_mo.snap @@ -4,5 +4,5 @@ expression: terminal.backend() --- "▌ /mo " "▌ " -"▌/model choose what model and reasoning effort to use " +"▌/model choose what model and reasoning effort to use " "▌/mention mention a file " 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 aab7e7c9..65606ed7 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 @@ -6,6 +6,6 @@ expression: render_lines(&view) ▌ Switch between Codex approval presets ▌ ▌> 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit 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 77552f2b..b42a5f8c 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 @@ -5,6 +5,6 @@ expression: render_lines(&view) ▌ Select Approval Mode ▌ ▌> 1. Read Only (current) Codex can read files -▌ 2. Full Access Codex can edit files +▌ 2. Full Access Codex can edit files Press Enter to confirm or Esc to go back