-
Notifications
You must be signed in to change notification settings - Fork 46
Support incremental surface updates #284
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?
Support incremental surface updates #284
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 an excellent feature for incremental surface updates by adding update
and replace
actions. The implementation is well-structured and includes corresponding updates to documentation, examples, and tests. My review includes a few suggestions to enhance robustness and maintainability, fix a potential bug related to newline character escaping, and acknowledges a TODO
regarding orphaned widgets which is important for long-term performance.
final stateValue = valueStore.forSurface(event.surfaceId); | ||
final eventString = | ||
'Action: ${jsonEncode(event.value)}\n' | ||
'Action: ${jsonEncode(event.value)}\\n' |
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 change from \n
to \\n
appears to be a mistake. The double backslash escapes the newline character, which means a literal \n
string will be sent to the model instead of a newline. This is unlikely to be interpreted as a line break by the AI. The original single backslash \n
was correct for creating a newline character within the string.
'Action: ${jsonEncode(event.value)}\\n' | |
'Action: ${jsonEncode(event.value)}\n' |
final newWidgetsList = definition['widgets'] as List<Object?>; | ||
final newWidgetsMap = <String, JsonMap>{}; | ||
for (final widget in newWidgetsList) { | ||
final typedWidget = widget as JsonMap; | ||
newWidgetsMap[typedWidget['id'] as String] = typedWidget; | ||
} |
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 in this block uses several direct casts (as
) on the definition
map, which is populated from AI-generated content. This could lead to runtime exceptions if the AI model provides a malformed response that doesn't adhere to the expected schema. To make the code more robust, consider using type checks (is
) and conditional casts (as?
) to gracefully handle potential data inconsistencies.
For example:
final newWidgetsList = definition['widgets'];
if (newWidgetsList is! List) {
// Handle error, e.g., throw an ArgumentError or log a warning.
return;
}
for (final widget in newWidgetsList) {
if (widget is! JsonMap) {
// Handle error
continue;
}
final id = widget['id'];
if (id is! String) {
// Handle error
continue;
}
newWidgetsMap[id] = widget;
}
...newWidgetsMap, | ||
}; | ||
|
||
// TODO(andrewkolb): Prune orphaned widgets. |
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 TODO
comment highlights a significant issue. The current implementation for the update
action merges new widgets but does not remove widgets that are no longer referenced in the widget tree. This will cause an accumulation of "orphaned" widgets in the UiDefinition
, leading to memory bloat and potential performance degradation over time. This should be addressed to ensure the long-term stability of the application.
final surfaceId = args['surfaceId'] as String; | ||
final definition = args['definition'] as JsonMap; | ||
final action = args['action'] as String; | ||
definition['action'] = action; |
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 line mutates the definition
map, which is part of the input args
. While it may work here, modifying input parameters is generally considered bad practice as it can lead to unexpected side effects and make the code harder to reason about. A cleaner approach would be to create a new map that includes the action
property, leaving the original definition
map unchanged.
For example:
final newDefinition = {
...definition,
'action': action,
};
onAddOrUpdate(surfaceId, newDefinition);
This PR implements incremental surface updates by introducing
update
andreplace
actions for theaddOrUpdateSurface
tool.Changes:
SurfaceUpdated
toSurfaceChanged
.update
action inGenUiManager
to upsert widgets.update
action toreplace
.AddOrUpdateSurfaceTool
schema and descriptions.Fixes #283