fix(provider): keep picker selection visible#2241
Conversation
Make the /provider modal size itself to the provider list when space allows, and scroll the rendered list when the selected provider moves past the visible rows. Also make the selected row use a continuous, subtle highlight so the current selection remains visible without looking disconnected. Adds regression coverage for bottom providers, initial bottom selection, full height rendering, and selected-row highlighting.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces scrolling support and improved row highlighting for the provider picker in the TUI. It calculates a visible range of providers based on the viewport height, styles the selected row with a continuous background, dynamically adjusts the popup height, and adds unit tests. The review feedback suggests improving the UX by centering the selected item in the viewport during scrolling, rather than pinning it to the bottom.
| fn visible_start(&self, visible_rows: usize) -> usize { | ||
| if visible_rows == 0 { | ||
| return 0; | ||
| } | ||
| let max_start = self.providers.len().saturating_sub(visible_rows); | ||
| self.selected_idx | ||
| .saturating_add(1) | ||
| .saturating_sub(visible_rows) | ||
| .min(max_start) | ||
| } |
There was a problem hiding this comment.
Usability / UX Improvement: Center the Selected Item in the Viewport
Currently, the visible_start calculation pins the selected item to the very bottom of the viewport once the selection index exceeds the viewport height:
self.selected_idx
.saturating_add(1)
.saturating_sub(visible_rows)This means that when navigating both up and down, the selected item is always at the bottom row of the list. This prevents the user from seeing any of the upcoming items below the selection, which is counter-intuitive and degrades the navigation experience.
By centering the selected item (or keeping it as close to the center as possible), the user can see context both above and below the current selection. This is completely stateless and fits perfectly within the existing function.
Here is the suggested change to implement centered scrolling:
fn visible_start(&self, visible_rows: usize) -> usize {
if visible_rows == 0 {
return 0;
}
let max_start = self.providers.len().saturating_sub(visible_rows);
let half_viewport = visible_rows / 2;
self.selected_idx
.saturating_sub(half_viewport)
.min(max_start)
}![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let visible_rows = usize::from(inner.height); | ||
| let visible_start = self.visible_start(visible_rows); | ||
| let mut lines: Vec<Line> = Vec::with_capacity(visible_rows); | ||
| for (idx, (provider, has_key)) in self | ||
| .providers | ||
| .iter() | ||
| .enumerate() | ||
| .skip(visible_start) | ||
| .take(visible_rows) | ||
| { | ||
| let is_selected = idx == self.selected_idx; | ||
| let is_active = *provider == self.active_provider; | ||
| let arrow = if is_selected { "▸" } else { " " }; | ||
| let active_dot = if is_active { " *" } else { " " }; | ||
| let label_style = if is_selected { | ||
| let spacer_style = if is_selected { | ||
| Self::selected_row_bg_style() | ||
| } else { | ||
| Style::default() | ||
| .fg(palette::SELECTION_TEXT) | ||
| .bg(palette::SELECTION_BG) | ||
| .add_modifier(Modifier::BOLD) | ||
| }; | ||
| let label_style = if is_selected { | ||
| Self::selected_row_style(palette::TEXT_PRIMARY) | ||
| } else { | ||
| Style::default().fg(palette::TEXT_PRIMARY) | ||
| }; | ||
| let hint_style = if is_selected { | ||
| Style::default() | ||
| .fg(palette::SELECTION_TEXT) | ||
| .bg(palette::SELECTION_BG) | ||
| let hint_fg = if *has_key { | ||
| palette::TEXT_MUTED | ||
| } else { | ||
| palette::STATUS_WARNING | ||
| }; | ||
| Self::selected_row_style(hint_fg) | ||
| } else if *has_key { | ||
| Style::default().fg(palette::TEXT_MUTED) | ||
| } else { | ||
| Style::default().fg(palette::STATUS_WARNING) | ||
| }; | ||
| let hint = Self::provider_hint(*provider, *has_key); | ||
| lines.push(Line::from(vec![ | ||
| Span::raw(" "), | ||
| let mut line = Line::from(vec![ | ||
| Span::styled(" ", spacer_style), | ||
| Span::styled(arrow, label_style), | ||
| Span::raw(" "), | ||
| Span::styled(" ", spacer_style), | ||
| Span::styled(provider.display_name().to_string(), label_style), | ||
| Span::styled(active_dot, label_style), | ||
| Span::raw(" "), | ||
| Span::styled(" ", spacer_style), | ||
| Span::styled(hint, hint_style), | ||
| ])); | ||
| ]); | ||
| if is_selected { | ||
| line.style = Self::selected_row_bg_style(); | ||
| let target_width = usize::from(inner.width); | ||
| let line_width = line.width(); | ||
| if line_width < target_width { | ||
| line.spans.push(Span::styled( | ||
| " ".repeat(target_width - line_width), | ||
| Self::selected_row_bg_style(), | ||
| )); | ||
| } | ||
| } | ||
| lines.push(line); | ||
| } | ||
| Paragraph::new(lines).render(inner, buf); |
There was a problem hiding this comment.
When the terminal is small enough that popup_height < providers.len() + 2, the list silently clips to a subset of providers. There is no "↑" / "↓" glyph (or similar) on the border to signal that more items exist above or below the visible window. A user whose terminal is too short to show all 12 providers would have no visual cue that providers are hidden outside the viewport — they might not know to press the arrow keys to reveal them.
…ht, and wrap Up/Down navigation between the first and last providers.
|
hi @Hmbown please take a look when you have time. thanks. |
From @reidliu41 — the /provider modal now sizes dynamically, scrolls to keep the selected row visible, wraps Up/Down between first and last providers, and renders the selected row with a full-row highlight.
CHANGELOG: [Unreleased] promoted to [0.8.47] with 16 Added, 5 Changed, 11 Fixed entries covering closed-loop verification, all-tools-loaded, runtime goal tools, DDG default, Xiaomi MiMo, global AGENTS.md fallback, DeepSeek V4 Pro pricing, and PR #2241 merge. READMEs: 15 new contributors added across en/zh-CN/ja-JP: @Fire-dtx, @imkingjh999, @harvey2011888, @victorcheng2333, @IIzzaya, @PurplePulse, @cyq1017, @knqiufan, @Colorful-glassblock, @hongqitai, @EmiyaKiritsugu3, @HUQIANTAO, @mvanhorn. Updated @reidliu41 and @aboimpinto entries with new PRs.
2 participants
Summary
Fixes #2238.
The
/providermodal had more provider rows than its fixed-height list could show. Arrow-key navigation changed the selected provider internally, but rendering always started from the first provider, so later providers such as SGLang, vLLM, and Ollama could be selected without becoming visible.This updates the picker to:
from: cannot move forward to next
to:
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR fixes the
/providermodal so the selected provider is always visible during keyboard navigation. Previously the list was fixed at height 12, causing providers like SGLang, vLLM, and Ollama to be navigated to but never displayed.providers.len() + 2(clamped to terminal height), showing all 12 providers when the terminal has enough room.visible_starthelper computes the minimum scroll offset to keep the selection visible, and the render loop uses.skip(visible_start).take(visible_rows)to emit only the visible slice.move_up/move_downnow wrap from first↔last instead of clamping, and the corresponding test comment is corrected from "twice" to "once".Confidence Score: 5/5
Safe to merge — the change is self-contained to a single TUI modal and carries no state-mutation risk outside the picker.
The visible_start formula correctly computes the minimum scroll offset that keeps the selection on-screen in all navigation scenarios (forward, backward, and wrap-around), and is covered by four focused render tests. The popup height calculation clamps properly to both the terminal size and the 8-row minimum. No data path, persistence, or API surface is affected.
No files require special attention.
Important Files Changed
Reviews (2): Last reviewed commit: "Make the selected row easier to see with..." | Re-trigger Greptile