-
Notifications
You must be signed in to change notification settings - Fork 112
Add an option to switch to Gemini Pro #35
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
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 model switching capability in the travel app, allowing users to choose between Gemini Flash and Pro. The implementation correctly uses a ValueNotifier for reactive UI updates. My review includes a high-severity fix to ensure the ValueNotifier instance is immutable to prevent listener issues, and a suggestion to refactor the UI code for better maintainability by reducing duplication.
| /// | ||
| /// Defaults to 'gemini-2.5-flash'. | ||
| final String model; | ||
| ValueNotifier<GeminiModel> model; |
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.
The model field should be final. A public, non-final field for a ValueNotifier allows the entire notifier instance to be replaced from outside the class. This is risky because any widgets listening to the original notifier instance would stop receiving updates, leading to a broken UI state. The ValueNotifier instance itself should be immutable after creation; only its internal value should change via model.value = ..., which the switchModel method correctly does.
final ValueNotifier<GeminiModel> model;
examples/travel_app/lib/main.dart
Outdated
| itemBuilder: (BuildContext context) => <PopupMenuEntry<GeminiModel>>[ | ||
| PopupMenuItem<GeminiModel>( | ||
| value: GeminiModel.flash, | ||
| child: Row( | ||
| children: [ | ||
| Text('Gemini Flash'), | ||
| if (currentModel == GeminiModel.flash) | ||
| const Icon(Icons.check), | ||
| ], | ||
| ), | ||
| ), | ||
| PopupMenuItem<GeminiModel>( | ||
| value: GeminiModel.pro, | ||
| child: Row( | ||
| children: [ | ||
| Text('Gemini Pro'), | ||
| if (currentModel == GeminiModel.pro) | ||
| const Icon(Icons.check), | ||
| ], | ||
| ), | ||
| ), | ||
| ], |
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.
The code for creating PopupMenuItems is duplicated for each model. This can be made more concise and maintainable by iterating over GeminiModel.values. This avoids repetition and makes it easier to add new models in the future.
For even cleaner code, you could add a displayName property to the GeminiModel enum in pkgs/flutter_genui/lib/src/ai_client/ai_client.dart and use it here instead of the conditional logic for the text.
itemBuilder: (BuildContext context) => [
for (final model in GeminiModel.values)
PopupMenuItem<GeminiModel>(
value: model,
child: Row(
mainAxisAlignment: MainAxisAlignment.spaceBetween,
children: [
Text(model == GeminiModel.flash ? 'Gemini Flash' : 'Gemini Pro'),
if (currentModel == model) const Icon(Icons.check),
],
),
),
],
Changes:
models.
centralizing model name management.
Fixes: #28