diff --git a/codex-rs/tui/src/bottom_pane/chat_composer.rs b/codex-rs/tui/src/bottom_pane/chat_composer.rs index 1e9faeec70..136436d4f3 100644 --- a/codex-rs/tui/src/bottom_pane/chat_composer.rs +++ b/codex-rs/tui/src/bottom_pane/chat_composer.rs @@ -28,6 +28,8 @@ const BASE_PLACEHOLDER_TEXT: &str = "..."; /// If the pasted content exceeds this number of characters, replace it with a /// placeholder in the UI. const LARGE_PASTE_CHAR_THRESHOLD: usize = 1000; +/// Keep at least this many rows for the composer when popups are visible. +const MIN_TEXTAREA_HEIGHT: u16 = 3; /// Result returned when the user interacts with the text area. pub enum InputResult { @@ -43,7 +45,7 @@ pub(crate) struct ChatComposer<'a> { ctrl_c_quit_hint: bool, use_shift_enter_hint: bool, dismissed_file_popup_token: Option, - dismissed_command_popup_token: Option, + dismissed_slash_token: Option, current_file_query: Option, pending_pastes: Vec<(String, String)>, } @@ -75,7 +77,7 @@ impl ChatComposer<'_> { ctrl_c_quit_hint: false, use_shift_enter_hint, dismissed_file_popup_token: None, - dismissed_command_popup_token: None, + dismissed_slash_token: None, current_file_query: None, pending_pastes: Vec::new(), }; @@ -158,7 +160,11 @@ impl ChatComposer<'_> { self.textarea.insert_str(&pasted); } self.sync_command_popup(); - self.sync_file_search_popup(); + if matches!(self.active_popup, ActivePopup::Command(_)) { + self.dismissed_file_popup_token = None; + } else { + self.sync_file_search_popup(); + } true } @@ -228,7 +234,7 @@ impl ChatComposer<'_> { if let Some(stripped) = first_line.strip_prefix('/') { let token = stripped.trim_start(); let cmd_token = token.split_whitespace().next().unwrap_or(""); - self.dismissed_command_popup_token = Some(cmd_token.to_string()); + self.dismissed_slash_token = Some(cmd_token.to_string()); } self.active_popup = ActivePopup::None; (InputResult::None, true) @@ -607,12 +613,12 @@ impl ChatComposer<'_> { .unwrap_or(""); let input_starts_with_slash = first_line.starts_with('/'); - let current_cmd_token: Option = if input_starts_with_slash { + let current_cmd_token: Option<&str> = if input_starts_with_slash { if let Some(stripped) = first_line.strip_prefix('/') { let token = stripped.trim_start(); - Some(token.split_whitespace().next().unwrap_or("").to_string()) + Some(token.split_whitespace().next().unwrap_or("")) } else { - Some(String::new()) + Some("") } } else { None @@ -623,25 +629,19 @@ impl ChatComposer<'_> { popup.on_composer_text_change(first_line.to_string()); } else { self.active_popup = ActivePopup::None; - self.dismissed_command_popup_token = None; + self.dismissed_slash_token = None; } } _ => { if input_starts_with_slash { - // Avoid reopening immediately after user pressed Esc until - // the '/token' changes. - if self - .dismissed_command_popup_token - .as_ref() - .is_some_and(|t| Some(t) == current_cmd_token.as_ref()) - { + if self.dismissed_slash_token.as_deref() == current_cmd_token { return; } let mut command_popup = CommandPopup::new(); command_popup.on_composer_text_change(first_line.to_string()); self.active_popup = ActivePopup::Command(command_popup); } - } + } } } @@ -701,11 +701,14 @@ impl WidgetRef for &ChatComposer<'_> { fn render_ref(&self, area: Rect, buf: &mut Buffer) { match &self.active_popup { ActivePopup::Command(popup) => { - let popup_height = popup.calculate_required_height(); + let desired_popup = popup.calculate_required_height(); + // Reserve exactly the number of text lines for the textarea so we + // don't create visual blank rows when the composer is short. + let text_lines = self.textarea.lines().len().max(1) as u16; + let popup_height = desired_popup.min(area.height.saturating_sub(text_lines)); // Split the provided rect so that the popup is rendered at the - // **bottom** and the textarea occupies the remaining space above. - let popup_height = popup_height.min(area.height); + // bottom and the textarea occupies the remaining space above. let textarea_rect = Rect { x: area.x, y: area.y, @@ -723,9 +726,9 @@ impl WidgetRef for &ChatComposer<'_> { self.textarea.render(textarea_rect, buf); } ActivePopup::File(popup) => { - let popup_height = popup.calculate_required_height(); - - let popup_height = popup_height.min(area.height); + let desired_popup = popup.calculate_required_height(); + let text_lines = self.textarea.lines().len().max(1) as u16; + let popup_height = desired_popup.min(area.height.saturating_sub(text_lines)); let textarea_rect = Rect { x: area.x, y: area.y, @@ -1057,6 +1060,30 @@ mod tests { } } + #[test] + fn esc_dismiss_slash_popup_reopen_on_token_change() { + use crate::bottom_pane::chat_composer::ActivePopup; + use crossterm::event::KeyCode; + use crossterm::event::KeyEvent; + use crossterm::event::KeyModifiers; + + let (tx, _rx) = std::sync::mpsc::channel(); + let sender = AppEventSender::new(tx); + let mut composer = ChatComposer::new(true, sender, false); + + // Type a slash command prefix. Popup should appear. + composer.handle_paste("/".to_string()); + assert!(matches!(composer.active_popup, ActivePopup::Command(_))); + + // Press Esc: popup should close. + let _ = composer.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE)); + assert!(matches!(composer.active_popup, ActivePopup::None)); + + // Append to token (change '/' -> '/c'): popup should reopen. + composer.handle_paste("c".to_string()); + assert!(matches!(composer.active_popup, ActivePopup::Command(_))); + } + #[test] fn test_multiple_pastes_submission() { use crossterm::event::KeyCode; diff --git a/codex-rs/tui/src/bottom_pane/command_popup.rs b/codex-rs/tui/src/bottom_pane/command_popup.rs index d50f60a306..0556cbbfd4 100644 --- a/codex-rs/tui/src/bottom_pane/command_popup.rs +++ b/codex-rs/tui/src/bottom_pane/command_popup.rs @@ -25,6 +25,8 @@ pub(crate) struct CommandPopup { command_filter: String, all_commands: Vec<(&'static str, SlashCommand)>, selected_idx: Option, + // Index of the first visible row in the filtered list. + scroll_top: usize, } impl CommandPopup { @@ -33,6 +35,7 @@ impl CommandPopup { command_filter: String::new(), all_commands: built_in_slash_commands(), selected_idx: None, + scroll_top: 0, } } @@ -66,26 +69,28 @@ impl CommandPopup { 0 => None, _ => Some(self.selected_idx.unwrap_or(0).min(matches_len - 1)), }; + + self.adjust_scroll(matches_len); } /// Determine the preferred height of the popup. This is the number of - /// rows required to show **at most** `MAX_POPUP_ROWS` commands plus the - /// table/border overhead (one line at the top and one at the bottom). + /// rows required to show at most MAX_POPUP_ROWS commands. pub(crate) fn calculate_required_height(&self) -> u16 { self.filtered_commands().len().clamp(1, MAX_POPUP_ROWS) as u16 } /// Return the list of commands that match the current filter. Matching is - /// performed using a *prefix* comparison on the command name. + /// performed using a case-insensitive prefix comparison on the command name. fn filtered_commands(&self) -> Vec<&SlashCommand> { + let filter = self.command_filter.as_str(); self.all_commands .iter() .filter_map(|(_name, cmd)| { - if self.command_filter.is_empty() - || cmd - .command() - .starts_with(&self.command_filter.to_ascii_lowercase()) - { + if filter.is_empty() { + return Some(cmd); + } + let name = cmd.command(); + if name.len() >= filter.len() && name[..filter.len()].eq_ignore_ascii_case(filter) { Some(cmd) } else { None @@ -96,26 +101,30 @@ impl CommandPopup { /// Move the selection cursor one step up. pub(crate) fn move_up(&mut self) { - if let Some(len) = self.filtered_commands().len().checked_sub(1) { - if len == usize::MAX { - return; - } + let matches = self.filtered_commands(); + let len = matches.len(); + if len == 0 { + self.selected_idx = None; + self.scroll_top = 0; + return; } - if let Some(idx) = self.selected_idx { - if idx > 0 { - self.selected_idx = Some(idx - 1); - } - } else if !self.filtered_commands().is_empty() { - self.selected_idx = Some(0); + match self.selected_idx { + Some(idx) if idx > 0 => self.selected_idx = Some(idx - 1), + Some(_) => self.selected_idx = Some(len - 1), // wrap to last + None => self.selected_idx = Some(0), } + + self.adjust_scroll(len); } /// Move the selection cursor one step down. pub(crate) fn move_down(&mut self) { - let matches_len = self.filtered_commands().len(); + let matches = self.filtered_commands(); + let matches_len = matches.len(); if matches_len == 0 { self.selected_idx = None; + self.scroll_top = 0; return; } @@ -130,6 +139,8 @@ impl CommandPopup { self.selected_idx = Some(0); } } + + self.adjust_scroll(matches_len); } /// Return currently selected command, if any. @@ -137,6 +148,26 @@ impl CommandPopup { let matches = self.filtered_commands(); self.selected_idx.and_then(|idx| matches.get(idx).copied()) } + + fn adjust_scroll(&mut self, matches_len: usize) { + if matches_len == 0 { + self.scroll_top = 0; + return; + } + let visible_rows = MAX_POPUP_ROWS.min(matches_len); + if let Some(sel) = self.selected_idx { + if sel < self.scroll_top { + self.scroll_top = sel; + } else { + let bottom = self.scroll_top + visible_rows - 1; + if sel > bottom { + self.scroll_top = sel + 1 - visible_rows; + } + } + } else { + self.scroll_top = 0; + } + } } impl WidgetRef for CommandPopup { @@ -145,20 +176,7 @@ impl WidgetRef for CommandPopup { let mut rows: Vec = Vec::new(); - let start_idx = match (self.selected_idx, matches.len()) { - (Some(sel), len) if len > 0 => sel.saturating_sub(MAX_POPUP_ROWS.saturating_sub(1)), - _ => 0, - }; - - let enumerated_visible = matches - .iter() - .enumerate() - .skip(start_idx) - .take(MAX_POPUP_ROWS) - .map(|(i, cmd)| (i, *cmd)) - .collect::>(); - - if enumerated_visible.is_empty() { + if matches.is_empty() { rows.push(Row::new(vec![ Cell::from(""), Cell::from("No matching commands").add_modifier(Modifier::ITALIC), @@ -166,7 +184,14 @@ impl WidgetRef for CommandPopup { } else { let default_style = Style::default(); let command_style = Style::default().fg(Color::LightBlue); - for (global_idx, cmd) in enumerated_visible.into_iter() { + let visible_rows = MAX_POPUP_ROWS.min(matches.len()); + let start_idx = self.scroll_top.min(matches.len().saturating_sub(1)); + for (global_idx, cmd) in matches + .iter() + .enumerate() + .skip(start_idx) + .take(visible_rows) + { rows.push(Row::new(vec![ Cell::from(Line::from(vec![ if Some(global_idx) == self.selected_idx { @@ -201,3 +226,37 @@ impl WidgetRef for CommandPopup { table.render(area, buf); } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn move_down_wraps_to_top() { + let mut popup = CommandPopup::new(); + // Show all commands by simulating composer input starting with '/'. + popup.on_composer_text_change("/".to_string()); + let len = popup.filtered_commands().len(); + assert!(len > 0); + + // Move to last item. + for _ in 0..len.saturating_sub(1) { + popup.move_down(); + } + // Next move_down should wrap to index 0. + popup.move_down(); + assert_eq!(popup.selected_idx, Some(0)); + } + + #[test] + fn move_up_wraps_to_bottom() { + let mut popup = CommandPopup::new(); + popup.on_composer_text_change("/".to_string()); + let len = popup.filtered_commands().len(); + assert!(len > 0); + + // Initial selection is 0; moving up should wrap to last. + popup.move_up(); + assert_eq!(popup.selected_idx, Some(len - 1)); + } +}