-
Notifications
You must be signed in to change notification settings - Fork 9.3k
feat(cli): Add /model command for interactive model selection #8940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @abhipatel12, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the CLI experience by providing users with an interactive way to choose their Gemini model. It integrates a new slash command, a dedicated UI dialog, and robust state management, all while ensuring the feature can be toggled via configuration. The changes aim to improve user control over the AI model used in their sessions. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Size Change: +5.05 kB (+0.03%) Total Size: 17.4 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new /model slash command for interactive model selection, gated by a useModelRouter setting. The implementation is well-structured, adding a new reusable DescriptiveRadioButtonSelect component, a useModelCommand hook, and integrating them into the existing application architecture. My review identifies a critical bug and a high-severity issue within the new DescriptiveRadioButtonSelect component related to state management and props synchronization. Addressing these will significantly improve the robustness and reliability of this new feature.
packages/cli/src/ui/components/shared/DescriptiveRadioButtonSelect.tsx
Outdated
Show resolved
Hide resolved
packages/cli/src/ui/components/shared/DescriptiveRadioButtonSelect.tsx
Outdated
Show resolved
Hide resolved
af1641e to
926d52f
Compare
miguelsolorio
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Hello! This is a great addition to the CLI. The new I have a few suggestions that could make the code even better, mostly related to our React best practices as outlined in 1. Refactor In We can refactor the component to derive the initial model directly from the context during rendering. This makes the data flow clearer and more efficient. Suggestion: 2. Improve Numeric Input Logic in The current implementation for selecting items with number keys has a small bug. When the number of items is a multiple of 10 (e.g., 10, 20), it's not possible to select items like '2' or '3' directly, as the component incorrectly waits for a second digit. Suggestion: Prompt Engineering Reflection It looks like this change was likely generated from a prompt similar to this:
To get even better results next time, we could enhance the prompt by reminding the model of our specific coding standards:
|
| disabled?: boolean; | ||
| } | ||
|
|
||
| export interface DescriptiveRadioButtonSelectProps<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please refactor this to use composition and a common base class with the existing RadioButton class. There is a lot of duplicate logic and we will make future enhancements that should be adopted by both. Generally this is just a radio button class with nicer rendering of the inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been refactored after we made the changes to extract the logic using the BaseSelectionList. Lmk what you think
| setIsModelDialogOpen(false); | ||
| }, []); | ||
|
|
||
| const handleModelSelect = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason to not just move this directly within modelDialog? it already knows about the config. Might as well not separate stuff that can just be in one place. will simplify appContainer rather than tangling it with impl details. as a follow up we should regularize all the openDialog, closeDialog spew in AppContainer with generic functionality for that for all dialogs rather than creating so much boilerplate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
| { isActive: true }, | ||
| ); | ||
|
|
||
| const handleSelect = (model: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why create this helper. just pass onSelect directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left this b/c it combines two things (setting the model + closing dialog) and provides the useCallback optimization
| config?.getModel() || DEFAULT_GEMINI_MODEL_AUTO, | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this useEffect is a no-op. you are already passing in
config?.getModel in the useState.
Are you concerned about the case of the config changing while the dialog is open? Config should be fixed today so no need to check for that... probably have bigger problems if the config changes while the model dialog is open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, removed this
926d52f to
fdb907a
Compare
abhipatel12
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made the changes! Let me know how this looks!
| config?.getModel() || DEFAULT_GEMINI_MODEL_AUTO, | ||
| ); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, removed this
| { isActive: true }, | ||
| ); | ||
|
|
||
| const handleSelect = (model: string) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left this b/c it combines two things (setting the model + closing dialog) and provides the useCallback optimization
| disabled?: boolean; | ||
| } | ||
|
|
||
| export interface DescriptiveRadioButtonSelectProps<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been refactored after we made the changes to extract the logic using the BaseSelectionList. Lmk what you think
| setIsModelDialogOpen(false); | ||
| }, []); | ||
|
|
||
| const handleModelSelect = useCallback( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
1146334 to
6af89af
Compare
packages/cli/src/ui/components/shared/DescriptiveRadioButtonSelect.test.tsx
Outdated
Show resolved
Hide resolved
jacob314
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit introduces a new `/model` slash command to the Gemini CLI. When executed, this command opens a full-screen dialog that presents a numbered list of available models, each with a title and a description. This provides a user-friendly way to view and select different models within the CLI. Key changes include: - A new `modelCommand.ts` to define the `/model` command. - A `ModelDialog.tsx` component to render the selection UI. - A reusable `DescriptiveRadioButtonSelect.tsx` component for creating lists with titles and descriptions. - Integration into the CLI's state management and command processing systems to handle dialog visibility.
6af89af to
3655f66
Compare
…-gemini#8940) Co-authored-by: Miguel Solorio <miguel.solorio07@gmail.com>
…-gemini#8940) Co-authored-by: Miguel Solorio <miguel.solorio07@gmail.com>
TLDR
This pull request introduces a new
/modelslash command that allows users to interactively select the Gemini model they want to use for their session. Executing the command opens a full-screen dialog where the user can choose between Auto (default), Pro, Flash, and Flash-Lite models.This feature is conditionally enabled and will only be available when the
useModelRoutersetting is set totruein the user's configuration.UI:
Co-authored by @miguelsolorio
Dive Deeper
This PR implements the
/modelcommand and its corresponding UI through several new components and hooks:/modelCommand: A newSlashCommand(modelCommand.ts) is defined. Its action returns adialogtype, triggering theDialogManagerto open the newModelDialog.ModelDialog.tsx: A new dialog component that displays the available models with descriptions. It utilizes a new generic, reusable component for the selection UI.DescriptiveRadioButtonSelect.tsx: A reusable and keyboard-navigable component for selecting an option from a list where each item has a title and a description. It supports navigation withj/k, arrow keys, and number selection.useModelCommand.ts: A new hook that encapsulates the state management for the model dialog (isModelDialogOpen,openModelDialog,closeModelDialog,handleModelSelect). It interacts with theConfigservice to persist the user's selection.BuiltinCommandLoaderhas been updated to only load the/modelcommand ifconfig.getUseModelRouter()returnstrue, effectively feature-gating the command.Reviewer Test Plan
Enable the Feature: In your
settings.json, setuseModelRouter: true:{ "experimental": { "useModelRouter": true } }Launch the CLI: Run
npm start.Open the Dialog: Type
/modeland press Enter.Verify UI: Confirm that the "Select Model" dialog appears, showing the list of available models.
Test Keyboard Navigation:
2) to select the corresponding model.Confirm Selection: After selecting a model with Enter or a number key, the dialog should close. You can re-open it to verify your selection is highlighted.
Testing Matrix
Linked issues / bugs
This PR makes progress on #6079