Fix jitter in TUI apps/connectors picker

codex-rs/tui/src/bottom_pane/list_selection_view.rs

@@ -23,14 +23,25 @@ use super::CancellationEvent;
 use super::bottom_pane_view::BottomPaneView;
 use super::popup_consts::MAX_POPUP_ROWS;
 use super::scroll_state::ScrollState;
+pub(crate) use super::selection_popup_common::ColumnWidthMode;
 use super::selection_popup_common::GenericDisplayRow;
 use super::selection_popup_common::measure_rows_height;
+use super::selection_popup_common::measure_rows_height_stable_col_widths;
+use super::selection_popup_common::measure_rows_height_with_col_width_mode;
 use super::selection_popup_common::render_rows;
+use super::selection_popup_common::render_rows_stable_col_widths;
+use super::selection_popup_common::render_rows_with_col_width_mode;
 use unicode_width::UnicodeWidthStr;
 
 /// One selectable item in the generic selection list.
 pub(crate) type SelectionAction = Box<dyn Fn(&AppEventSender) + Send + Sync>;
 
+/// One row in a [`ListSelectionView`] selection list.
+///
+/// This is the source-of-truth model for row state before filtering and
+/// formatting into render rows. A row is treated as disabled when either
+/// `is_disabled` is true or `disabled_reason` is present; disabled rows cannot
+/// be accepted and are skipped by keyboard navigation.
 #[derive(Default)]
 pub(crate) struct SelectionItem {
     pub name: String,
@@ -46,6 +57,16 @@ pub(crate) struct SelectionItem {
     pub disabled_reason: Option<String>,
 }
 
+/// Construction-time configuration for [`ListSelectionView`].
+///
+/// This config is consumed once by [`ListSelectionView::new`]. After
+/// construction, mutable interaction state (filtering, scrolling, and selected
+/// row) lives on the view itself.
+///
+/// `col_width_mode` controls column width mode in selection lists:
+/// `AutoVisible` (default) measures only rows visible in the viewport
+/// `AutoAllRows` measures all rows to ensure stable column widths as the user scrolls
+/// `Fixed` used a fixed 30/70  split between columns
 pub(crate) struct SelectionViewParams {
     pub title: Option<String>,
     pub subtitle: Option<String>,
@@ -54,6 +75,7 @@ pub(crate) struct SelectionViewParams {
     pub items: Vec<SelectionItem>,
     pub is_searchable: bool,
     pub search_placeholder: Option<String>,
+    pub col_width_mode: ColumnWidthMode,
     pub header: Box<dyn Renderable>,
     pub initial_selected_idx: Option<usize>,
 }
@@ -68,12 +90,18 @@ impl Default for SelectionViewParams {
             items: Vec::new(),
             is_searchable: false,
             search_placeholder: None,
+            col_width_mode: ColumnWidthMode::AutoVisible,
             header: Box::new(()),
             initial_selected_idx: None,
         }
     }
 }
 
+/// Runtime state for rendering and interacting with a list-based selection popup.
+///
+/// This type is the single authority for filtered index mapping between
+/// visible rows and source items and for preserving selection while filters
+/// change.
 pub(crate) struct ListSelectionView {
     footer_note: Option<Line<'static>>,
     footer_hint: Option<Line<'static>>,
@@ -84,13 +112,21 @@ pub(crate) struct ListSelectionView {
     is_searchable: bool,
     search_query: String,
     search_placeholder: Option<String>,
+    col_width_mode: ColumnWidthMode,
     filtered_indices: Vec<usize>,
     last_selected_actual_idx: Option<usize>,
     header: Box<dyn Renderable>,
     initial_selected_idx: Option<usize>,
 }
 
 impl ListSelectionView {
+    /// Create a selection popup view with filtering, scrolling, and callbacks wired.
+    ///
+    /// The constructor normalizes header/title composition and immediately
+    /// applies filtering so `ScrollState` starts in a valid visible range.
+    /// When search is enabled, rows without `search_value` will disappear as
+    /// soon as the query is non-empty, which can look like dropped data unless
+    /// callers intentionally populate that field.
     pub fn new(params: SelectionViewParams, app_event_tx: AppEventSender) -> Self {
         let mut header = params.header;
         if params.title.is_some() || params.subtitle.is_some() {
@@ -116,6 +152,7 @@ impl ListSelectionView {
             } else {
                 None
             },
+            col_width_mode: params.col_width_mode,
             filtered_indices: Vec::new(),
             last_selected_actual_idx: None,
             header,
@@ -434,12 +471,27 @@ impl Renderable for ListSelectionView {
         // Build the same display rows used by the renderer so wrapping math matches.
         let rows = self.build_rows();
         let rows_width = Self::rows_width(width);
-        let rows_height = measure_rows_height(
-            &rows,
-            &self.state,
-            MAX_POPUP_ROWS,
-            rows_width.saturating_add(1),
-        );
+        let rows_height = match self.col_width_mode {
+            ColumnWidthMode::AutoVisible => measure_rows_height(
+                &rows,
+                &self.state,
+                MAX_POPUP_ROWS,
+                rows_width.saturating_add(1),
+            ),
+            ColumnWidthMode::AutoAllRows => measure_rows_height_stable_col_widths(
+                &rows,
+                &self.state,
+                MAX_POPUP_ROWS,
+                rows_width.saturating_add(1),
+            ),
+            ColumnWidthMode::Fixed => measure_rows_height_with_col_width_mode(
+                &rows,
+                &self.state,
+                MAX_POPUP_ROWS,
+                rows_width.saturating_add(1),
+                ColumnWidthMode::Fixed,
+            ),
+        };
 
         // Subtract 4 for the padding on the left and right of the header.
         let mut height = self.header.desired_height(width.saturating_sub(4));
@@ -483,12 +535,27 @@ impl Renderable for ListSelectionView {
             .desired_height(outer_content_area.width.saturating_sub(4));
         let rows = self.build_rows();
         let rows_width = Self::rows_width(outer_content_area.width);
-        let rows_height = measure_rows_height(
-            &rows,
-            &self.state,
-            MAX_POPUP_ROWS,
-            rows_width.saturating_add(1),
-        );
+        let rows_height = match self.col_width_mode {
+            ColumnWidthMode::AutoVisible => measure_rows_height(
+                &rows,
+                &self.state,
+                MAX_POPUP_ROWS,
+                rows_width.saturating_add(1),
+            ),
+            ColumnWidthMode::AutoAllRows => measure_rows_height_stable_col_widths(
+                &rows,
+                &self.state,
+                MAX_POPUP_ROWS,
+                rows_width.saturating_add(1),
+            ),
+            ColumnWidthMode::Fixed => measure_rows_height_with_col_width_mode(
+                &rows,
+                &self.state,
+                MAX_POPUP_ROWS,
+                rows_width.saturating_add(1),
+                ColumnWidthMode::Fixed,
+            ),
+        };
         let [header_area, _, search_area, list_area] = Layout::vertical([
             Constraint::Max(header_height),
             Constraint::Max(1),
@@ -529,14 +596,33 @@ impl Renderable for ListSelectionView {
                 width: rows_width.max(1),
                 height: list_area.height,
             };
-            render_rows(
-                render_area,
-                buf,
-                &rows,
-                &self.state,
-                render_area.height as usize,
-                "no matches",
-            );
+            match self.col_width_mode {
+                ColumnWidthMode::AutoVisible => render_rows(
+                    render_area,
+                    buf,
+                    &rows,
+                    &self.state,
+                    render_area.height as usize,
+                    "no matches",
+                ),
+                ColumnWidthMode::AutoAllRows => render_rows_stable_col_widths(
+                    render_area,
+                    buf,
+                    &rows,
+                    &self.state,
+                    render_area.height as usize,
+                    "no matches",
+                ),
+                ColumnWidthMode::Fixed => render_rows_with_col_width_mode(
+                    render_area,
+                    buf,
+                    &rows,
+                    &self.state,
+                    render_area.height as usize,
+                    "no matches",
+                    ColumnWidthMode::Fixed,
+                ),
+            }
         }
 
         if footer_area.height > 0 {
@@ -585,7 +671,9 @@ mod tests {
     use super::*;
     use crate::app_event::AppEvent;
     use crate::bottom_pane::popup_consts::standard_popup_hint_line;
+    use crossterm::event::KeyCode;
     use insta::assert_snapshot;
+    use pretty_assertions::assert_eq;
     use ratatui::layout::Rect;
     use tokio::sync::mpsc::unbounded_channel;
 
@@ -647,6 +735,55 @@ mod tests {
         lines.join("\n")
     }
 
+    fn description_col(rendered: &str, item_marker: &str, description: &str) -> usize {
+        let line = rendered
+            .lines()
+            .find(|line| line.contains(item_marker) && line.contains(description))
+            .expect("expected rendered line to contain row marker and description");
+        line.find(description)
+            .expect("expected rendered line to contain description")
+    }
+
+    fn make_scrolling_width_items() -> Vec<SelectionItem> {
+        let mut items: Vec<SelectionItem> = (1..=8)
+            .map(|idx| SelectionItem {
+                name: format!("Item {idx}"),
+                description: Some(format!("desc {idx}")),
+                dismiss_on_select: true,
+                ..Default::default()
+            })
+            .collect();
+        items.push(SelectionItem {
+            name: "Item 9 with an intentionally much longer name".to_string(),
+            description: Some("desc 9".to_string()),
+            dismiss_on_select: true,
+            ..Default::default()
+        });
+        items
+    }
+
+    fn render_before_after_scroll_snapshot(col_width_mode: ColumnWidthMode, width: u16) -> String {
+        let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
+        let tx = AppEventSender::new(tx_raw);
+        let mut view = ListSelectionView::new(
+            SelectionViewParams {
+                title: Some("Debug".to_string()),
+                items: make_scrolling_width_items(),
+                col_width_mode,
+                ..Default::default()
+            },
+            tx,
+        );
+
+        let before_scroll = render_lines_with_width(&view, width);
+        for _ in 0..8 {
+            view.handle_key_event(KeyEvent::from(KeyCode::Down));
+        }
+        let after_scroll = render_lines_with_width(&view, width);
+
+        format!("before scroll:\n{before_scroll}\n\nafter scroll:\n{after_scroll}")
+    }
+
     #[test]
     fn renders_blank_line_between_title_and_items_without_subtitle() {
         let view = make_selection_view(None);
@@ -921,4 +1058,96 @@ mod tests {
             render_lines_with_width(&view, 24)
         );
     }
+
+    #[test]
+    fn snapshot_auto_visible_col_width_mode_scroll_behavior() {
+        assert_snapshot!(
+            "list_selection_col_width_mode_auto_visible_scroll",
+            render_before_after_scroll_snapshot(ColumnWidthMode::AutoVisible, 96)
+        );
+    }
+
+    #[test]
+    fn snapshot_auto_all_rows_col_width_mode_scroll_behavior() {
+        assert_snapshot!(
+            "list_selection_col_width_mode_auto_all_rows_scroll",
+            render_before_after_scroll_snapshot(ColumnWidthMode::AutoAllRows, 96)
+        );
+    }
+
+    #[test]
+    fn snapshot_fixed_col_width_mode_scroll_behavior() {
+        assert_snapshot!(
+            "list_selection_col_width_mode_fixed_scroll",
+            render_before_after_scroll_snapshot(ColumnWidthMode::Fixed, 96)
+        );
+    }
+
+    #[test]
+    fn auto_all_rows_col_width_does_not_shift_when_scrolling() {
+        let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
+        let tx = AppEventSender::new(tx_raw);
+
+        let mut view = ListSelectionView::new(
+            SelectionViewParams {
+                title: Some("Debug".to_string()),
+                items: make_scrolling_width_items(),
+                col_width_mode: ColumnWidthMode::AutoAllRows,
+                ..Default::default()
+            },
+            tx,
+        );
+
+        let before_scroll = render_lines_with_width(&view, 96);
+        for _ in 0..8 {
+            view.handle_key_event(KeyEvent::from(KeyCode::Down));
+        }
+        let after_scroll = render_lines_with_width(&view, 96);
+
+        assert!(
+            after_scroll.contains("9. Item 9 with an intentionally much longer name"),
+            "expected the scrolled view to include the longer row:\n{after_scroll}"
+        );
+
+        let before_col = description_col(&before_scroll, "8. Item 8", "desc 8");
+        let after_col = description_col(&after_scroll, "8. Item 8", "desc 8");
+        assert_eq!(
+            before_col, after_col,
+            "description column changed across scroll:\nbefore:\n{before_scroll}\nafter:\n{after_scroll}"
+        );
+    }
+
+    #[test]
+    fn fixed_col_width_is_30_70_and_does_not_shift_when_scrolling() {
+        let (tx_raw, _rx) = unbounded_channel::<AppEvent>();
+        let tx = AppEventSender::new(tx_raw);
+        let width = 96;
+        let mut view = ListSelectionView::new(
+            SelectionViewParams {
+                title: Some("Debug".to_string()),
+                items: make_scrolling_width_items(),
+                col_width_mode: ColumnWidthMode::Fixed,
+                ..Default::default()
+            },
+            tx,
+        );
+
+        let before_scroll = render_lines_with_width(&view, width);
+        let before_col = description_col(&before_scroll, "8. Item 8", "desc 8");
+        let expected_desc_col = ((width.saturating_sub(2) as usize) * 3) / 10;
+        assert_eq!(
+            before_col, expected_desc_col,
+            "fixed mode should place description column at a 30/70 split:\n{before_scroll}"
+        );
+
+        for _ in 0..8 {
+            view.handle_key_event(KeyEvent::from(KeyCode::Down));
+        }
+        let after_scroll = render_lines_with_width(&view, width);
+        let after_col = description_col(&after_scroll, "8. Item 8", "desc 8");
+        assert_eq!(
+            before_col, after_col,
+            "fixed description column changed across scroll:\nbefore:\n{before_scroll}\nafter:\n{after_scroll}"
+        );
+    }
 }

codex-rs/tui/src/bottom_pane/mod.rs

@@ -65,6 +65,7 @@ mod skill_popup;
 mod skills_toggle_view;
 mod slash_commands;
 pub(crate) use footer::CollaborationModeIndicator;
+pub(crate) use list_selection_view::ColumnWidthMode;
 pub(crate) use list_selection_view::SelectionViewParams;
 mod feedback_view;
 pub(crate) use feedback_view::FeedbackAudience;