-
Notifications
You must be signed in to change notification settings - Fork 46
Implement SurfaceController #195
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
base: main
Are you sure you want to change the base?
Conversation
/gemini summarize |
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 significant refactoring by implementing SurfaceController
to manage UI surfaces, which simplifies GenUiSurface
into a stateless widget. This change also involves removing the SurfaceManager
and the abstract AiClient
, consolidating logic into GenUiManager
and GeminiAiClient
respectively. While the overall design is improved, I've found a few critical issues, including syntax errors and incorrect variable names in the example apps that will break them. Additionally, the removal of the retry logic for AI client network calls is a concern for application resilience, and one of the spikes is broken due to API changes.
Summary of ChangesThis pull request significantly refactors the UI management and AI client interaction within the Highlights
Changelog
Activity
|
SurfaceController({required this.definitionNotifier, required this.catalog}); | ||
|
||
final ValueNotifier<UiDefinition?> definitionNotifier; | ||
final Catalog catalog; |
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.
Is the catalog meant to be per-surface? That might be hard to communicate to the generating LLM: I don't think we want to have to communicate the entire catalog with each request. Or is this just meant to be a reference to a common central catalog? I wonder if there's a way we could enforce that there's only one catalog per GenUIManager
? Maybe have it owned by the GenUIManager
instead, and pass one of those to the SurfaceController
constructor?
try { | ||
final response = await _aiClient.generateText( | ||
final response = await _aiClient.generateText(conversation: _conversation); | ||
List.of(_chatController.conversation.value), |
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 looks like a syntax error?
No description provided.