Improve simple chat and make some cross-repo cleanups.#905
Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the simple_chat example to support multiple application modes—text-only, basic catalog, and a custom climbing location catalog—while introducing a new e2e test suite. It also updates the macOS build configuration to use the FlutterGeneratedPluginSwiftPackage and improves error handling in the audio_player widget. Feedback includes correcting a likely typo in the Gemini model name, removing a redundant system prompt fragment, adjusting the logging severity for surface removal, and refining the text controller reset logic to preserve user input.
ditman
left a comment
There was a problem hiding this comment.
Some small comments. Nothing blocking. My biggest concern is exposing the "no asset" version of the basic catalog through its API (it's hard to remove later, and it's something that apps can do very easily themselves when they need it)
| - [ ] I read the [Contributors Guide]. | ||
| - [ ] I have added sample code updates to the [changelog]. | ||
| - [ ] I updated/added relevant documentation (doc comments with `///`). | ||
| - [ ] If my PR is a fork PR, I've checked that [e2e tests] passed. |
There was a problem hiding this comment.
Can tests be run by end users? We have this same message in google/A2UI, and it's not super relevant unless flutter code is being touched (?)
There was a problem hiding this comment.
In a2ui it is relevant if python code is touched, because service is in python.
We have all kinds of dependencies, and we may add more e2e tests. Added condition for changed code. Thank you!
| String? debugApiKey; | ||
|
|
||
| String getApiKey() { | ||
| String apiKey() { |
There was a problem hiding this comment.
Why this renaming? Shouldn't a function name contain a verb?
There was a problem hiding this comment.
| await tester.pumpAndSettle(); | ||
| // We can't use pumpAndSettle() because some catalog widgets (e.g. Image's | ||
| // loadingBuilder shows a CircularProgressIndicator) have indeterminate | ||
| // animations that never settle. |
There was a problem hiding this comment.
Do they never settle because the network is not available, or because the image that we're attempting to load has a bad URL?
There was a problem hiding this comment.
CircularProgressIndicator messes up somehow with pumpAndSettle. This fix is suggested by agent. Updated comment. I do not know details and i do not feel it is worth to spend time investigation.
|
|
||
| When user asks about climbing locations, never use other components. | ||
| ''', | ||
| ..._basicCatalog.systemPromptFragments, |
There was a problem hiding this comment.
should the default systemprompts be placed before the custom catalog fragments?
There was a problem hiding this comment.
Not sure. The example works this way well, and it may break if i change the prompt. Leaving it as is.
|
|
||
| ## Video | ||
|
|
||
| https://github.com/user-attachments/assets/469fb2cf-09cf-463c-8c9c-b9c0cb39203b |
There was a problem hiding this comment.
This demo looks super good. Kudos!
| /// Creates a basic catalog without items that require assets. | ||
| /// | ||
| /// This is useful for the app, that do not work with images, audio or video. | ||
| static Catalog asNoAssetCatalog({ | ||
| List<String> systemPromptFragments = const [], | ||
| }) => asCatalog( | ||
| systemPromptFragments: systemPromptFragments, | ||
| ).copyWithout(itemsToRemove: [audioPlayer, image, video]); | ||
|
|
||
| /// Creates a catalog containing all basic catalog items. |
There was a problem hiding this comment.
Not a fan of exposing an uglier version of the catalog. I think this should be added by the application itself, rather than our API.
There was a problem hiding this comment.
Asset related catalog items require addisional configuration of assets. They cannot be just plugged in. So, it results in extra troubleshooting for early adopters.
Added more comments to both methods to make it clear. Thank you!
Contributes to #907
Fixes #808
Simple chat
Cross-repo cleanups
Video
Screen.Recording.2026-05-11.at.3.26.19.PM.mov