tests, scroll to bottom from top

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<String>,
-    dismissed_command_popup_token: Option<String>,
+    dismissed_slash_token: Option<String>,
     current_file_query: Option<String>,
     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<String> = 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;

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<usize>,
+    // 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<Row> = 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::<Vec<(usize, &SlashCommand)>>();
-
-        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));
+    }
+}