fix(cli): limit provider selector height - #10420
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d71b3bea4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28e3ed5af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f28e3ed5af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
thanks... taking a look |
|
Nice fix — verified locally that it caps the list at 10 rows with a working search fallback. 👍 One small suggestion on the Suggested change (tested locally: // replace the two sentinel consts with:
#[derive(Clone, PartialEq, Eq)]
enum ProviderChoice {
/// A real provider, identified by its provider name (e.g. "anthropic").
Provider(String),
/// The "Search all providers..." entry in the paginated list.
Search,
/// The "Search again..." entry in the fuzzy-search results list.
SearchAgain,
}
fn provider_choice_items(items: &[ProviderItem]) -> Vec<(ProviderChoice, String, String)> {
items
.iter()
.map(|(name, label, hint)| {
(ProviderChoice::Provider(name.clone()), label.clone(), hint.clone())
})
.collect()
}
let mut items = provider_choice_items(&filtered_items);
items.push((
ProviderChoice::SearchAgain,
"Search again...".to_string(),
"Enter a different search term".to_string(),
));
match cliclack::select("Which model provider should we use?")
.items(&items)
.max_rows(MAX_PROVIDER_ROWS)
.interact()?
{
ProviderChoice::SearchAgain => continue,
ProviderChoice::Provider(name) => return Ok(name),
ProviderChoice::Search => unreachable!("Search entry is not added to the results list"),
}and the paginated select in let mut paginated_items = provider_choice_items(&provider_items);
paginated_items.insert(
MAX_PROVIDER_ROWS - 1,
(
ProviderChoice::Search,
"Search all providers...".to_string(),
"Filter the complete provider list".to_string(),
),
);
match cliclack::select("Which model provider should we use?")
.initial_value(ProviderChoice::Provider(default_provider.clone()))
.items(&paginated_items)
.max_rows(MAX_PROVIDER_ROWS)
.interact()?
{
ProviderChoice::Search => search_provider_dialog(&provider_items)?,
ProviderChoice::Provider(name) => name,
ProviderChoice::SearchAgain => {
unreachable!("SearchAgain entry is not added to the paginated list")
}
}Fully optional — the current version works fine. The only slight wart in the enum version is the two |
|
@495696116 do you want to apply those changes - and then can approve it and merge? (or I can comit to your branch if you would like me to) LMK - but nice work! |
|
Thanks! Applied the type-safe Validation on the updated branch:
I also confirmed the updated branch merges cleanly with the current |
Summary
Why
goose configurecurrently renders every provider at once, which can overflow smaller terminals and make the prompt hard to scan.cliclack0.5.5 does not reset its private list offset when filtering a paginated selector, so long lists use a separate fuzzy-search step before rendering paginated results.Validation
cargo fmt --checkcargo test -p goose-cli --no-default-features --features rustls-tls commands::configure::tests --locked(3 passed)cargo check -p goose-cli --no-default-features --features rustls-tls --lockedcargo clippy -p goose-cli --all-targets --no-default-features --features rustls-tls --locked -- -D warningsGOOSE_PATH_ROOTCloses #10415