Add a simple GSP client prototype #317
Conversation
|
/gemini summary |
gspencergoog
force-pushed
the
simple_gsp_client
branch
from
9381cc7 to
e38ba79
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a prototype for a simple GSP client, including the core library, an example application, and tests. The overall architecture is well-documented in the IMPLEMENTATION.md and the code is a good starting point.
My review focuses on improving maintainability and correctness. Key points include:
- Aligning the data model implementation with the design document by using code generation (
freezed/json_serializable) to reduce boilerplate and prevent errors. - Refactoring parts of the widget building logic to be more robust and less brittle to changes.
- Fixing a potential layout issue in the example application.
- Cleaning up test data to be more accurate.
Overall, this is a solid foundation for the new package. Addressing these points will make it more robust and easier to build upon.
| return Expanded( | ||
| child: Padding( | ||
| padding: const EdgeInsets.symmetric(horizontal: 8.0), | ||
| child: TextField( | ||
| decoration: InputDecoration(hintText: component.description), | ||
| ), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The TextField builder is wrapping the TextField widget with an Expanded widget. This forces the TextField to fill available space within a Flex widget (like Row or Column). While this works for the current sample JSON where it's inside a Row, it makes the builder less reusable. If a TextField component were used in a context without a Flex parent, it would cause a runtime layout error.
Widget builders in the registry should be self-contained and not make assumptions about their parent widgets. The Expanded widget should be part of the layout definition (e.g., as a separate Expanded component type) rather than being hardcoded into the TextField builder.
I suggest removing the Expanded widget from this builder.
| return Expanded( | |
| child: Padding( | |
| padding: const EdgeInsets.symmetric(horizontal: 8.0), | |
| child: TextField( | |
| decoration: InputDecoration(hintText: component.description), | |
| ), | |
| ), | |
| ); | |
| return Padding( | |
| padding: const EdgeInsets.symmetric(horizontal: 8.0), | |
| child: TextField( | |
| decoration: InputDecoration(hintText: component.description), | |
| ), | |
| ); |
|
|
||
| #### **Data Models (`models/*.dart`)** | ||
|
|
||
| These will be simple, immutable data classes created using the `freezed` package to represent the JSON structures from the protocol. This provides value equality, `copyWith`, and exhaustive `when` methods for free. |
There was a problem hiding this comment.
The design document states that data models will use the freezed package for immutability and value equality. However, the current implementation uses manually written classes. While this works, it deviates from the documented design and misses out on the benefits of freezed, such as reduced boilerplate and compile-time safety. It would be good to either update the implementation to use freezed (as the pubspec.yaml suggests was the intent) or update this design document to reflect the current manual approach.
| if (currentNode?.items != null) { | ||
| final resolvedItems = <Map<String, dynamic>>[]; | ||
| for (final itemId in currentNode!.items!) { | ||
| final itemNode = _dataModelNodes[itemId]; | ||
| if (itemNode?.children != null) { | ||
| final resolvedItem = <String, dynamic>{}; | ||
| itemNode!.children!.forEach((key, valueId) { | ||
| resolvedItem[key] = _dataModelNodes[valueId]?.value; | ||
| }); | ||
| resolvedItems.add(resolvedItem); | ||
| } | ||
| } | ||
| return resolvedItems; | ||
| } |
There was a problem hiding this comment.
The logic for resolving a list of items from a data binding path is quite nested and specific to this one location. This reduces the readability of the resolveDataBinding method.
To improve clarity and maintainability, consider extracting this block of code into a separate private helper method, such as _resolveItems(List<String> itemIds). This would make the main resolveDataBinding method shorter and easier to understand at a glance.
| .toList(), | ||
| ); | ||
| } | ||
|
|
||
| final String action; | ||
| final Map<String, dynamic>? staticContext; | ||
| final List<DynamicContextItem>? dynamicContext; | ||
|
|
||
| Map<String, dynamic> toJson() { | ||
| return <String, dynamic>{ | ||
| 'action': action, | ||
| if (staticContext != null) 'staticContext': staticContext, | ||
| if (dynamicContext != null) | ||
| 'dynamicContext': dynamicContext!.map((e) => e.toJson()).toList(), | ||
| }; | ||
| } | ||
|
|
||
| @override | ||
| bool operator ==(Object other) { | ||
| if (identical(this, other)) return true; | ||
|
|
||
| return other is Action && | ||
| other.action == action && | ||
| mapEquals(other.staticContext, staticContext) && | ||
| listEquals(other.dynamicContext, dynamicContext); | ||
| } | ||
|
|
||
| @override | ||
| int get hashCode => Object.hash(action, staticContext, dynamicContext); | ||
| } | ||
|
|
||
| class DynamicContextItem { | ||
| const DynamicContextItem({ | ||
| required this.key, | ||
| required this.value, | ||
| }); | ||
|
|
||
| factory DynamicContextItem.fromJson(Map<String, dynamic> json) { | ||
| return DynamicContextItem( | ||
| key: json['key'] as String, | ||
| value: Value.fromJson(json['value'] as Map<String, dynamic>), | ||
| ); | ||
| } | ||
|
|
||
| final String key; | ||
| final Value value; | ||
|
|
||
| Map<String, dynamic> toJson() { | ||
| return <String, dynamic>{ | ||
| 'key': key, | ||
| 'value': value.toJson(), | ||
| }; | ||
| } | ||
|
|
||
| @override | ||
| bool operator ==(Object other) { | ||
| if (identical(this, other)) return true; | ||
|
|
||
| return other is DynamicContextItem && | ||
| other.key == key && | ||
| other.value == value; | ||
| } | ||
|
|
||
| @override | ||
| int get hashCode => Object.hash(key, value); | ||
| } | ||
|
|
||
| class Option { | ||
| const Option({ | ||
| required this.label, | ||
| required this.value, | ||
| }); | ||
|
|
||
| factory Option.fromJson(Map<String, dynamic> json) { | ||
| return Option( | ||
| label: json['label'] as String, | ||
| value: json['value'] as String, | ||
| ); | ||
| } | ||
|
|
||
| final String label; | ||
| final String value; | ||
|
|
||
| Map<String, dynamic> toJson() { | ||
| return <String, dynamic>{ | ||
| 'label': label, | ||
| 'value': value, | ||
| }; | ||
| } | ||
|
|
||
| @override | ||
| bool operator ==(Object other) { | ||
| if (identical(this, other)) return true; | ||
|
|
||
| return other is Option && other.label == label && other.value == value; | ||
| } | ||
|
|
||
| @override | ||
| int get hashCode => Object.hash(label, value); | ||
| } |
There was a problem hiding this comment.
These data model classes are implemented manually, including fromJson, toJson, operator==, and hashCode. This is a lot of boilerplate code that is error-prone and difficult to maintain.
As mentioned in the design document and suggested by the dependencies in pubspec.yaml, using a code generation library like freezed or json_serializable would be a significant improvement. It would automate the generation of this code, reduce the chance of errors, and make the models easier to update.
For example, with freezed and json_serializable, the Component class could be simplified to a declaration, with all the serialization and equality logic generated automatically.
| Map<String, Object?> _resolveProperties( | ||
| Component component, Map<String, dynamic>? itemData) { | ||
| final properties = <String, Object?>{}; | ||
|
|
||
| void resolveValue(String key, Value? value) { | ||
| if (value == null) { | ||
| return; | ||
| } | ||
| if (value.literalString != null) { | ||
| properties[key] = value.literalString; | ||
| } else if (value.literalNumber != null) { | ||
| properties[key] = value.literalNumber; | ||
| } else if (value.literalBoolean != null) { | ||
| properties[key] = value.literalBoolean; | ||
| } else if (value.literalObject != null) { | ||
| properties[key] = value.literalObject; | ||
| } else if (value.literalArray != null) { | ||
| properties[key] = value.literalArray; | ||
| } else if (value.path != null) { | ||
| if (itemData != null) { | ||
| properties[key] = itemData[value.path!.substring(1)]; | ||
| } else { | ||
| properties[key] = interpreter.resolveDataBinding(value.path!); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| resolveValue('text', component.value); | ||
| properties['level'] = component.level; | ||
| properties['description'] = component.description; | ||
| properties['direction'] = component.direction; | ||
| properties['distribution'] = component.distribution; | ||
| properties['alignment'] = component.alignment; | ||
| properties['child'] = component.child; | ||
| properties['tabItems'] = component.tabItems; | ||
| properties['axis'] = component.axis; | ||
| properties['color'] = component.color; | ||
| properties['thickness'] = component.thickness; | ||
| properties['entryPointChild'] = component.entryPointChild; | ||
| properties['contentChild'] = component.contentChild; | ||
| properties['label'] = component.label; | ||
| properties['action'] = component.action; | ||
| properties['textFieldType'] = component.textFieldType; | ||
| properties['validationRegexp'] = component.validationRegexp; | ||
| properties['enableDate'] = component.enableDate; | ||
| properties['enableTime'] = component.enableTime; | ||
| properties['outputFormat'] = component.outputFormat; | ||
| properties['options'] = component.options; | ||
| properties['maxAllowedSelections'] = component.maxAllowedSelections; | ||
| properties['min_value'] = component.min_value; | ||
| properties['max_value'] = component.max_value; | ||
|
|
||
| return properties; | ||
| } |
There was a problem hiding this comment.
The _resolveProperties method is quite long and manually maps every property from the Component object to the properties map. This approach is brittle and not easily maintainable. If a new property is added to the Component class, it must also be manually added here, which is easy to forget.
A more robust approach would be to make this method more generic. You could convert the Component to a map using component.toJson() and then iterate through its key-value pairs. For any value that represents a data binding (i.e., a Value object), you would resolve it. Otherwise, you'd pass the property through as-is. This would make the code more scalable and less prone to errors when the Component model changes.
| testWidgets('processes ComponentUpdate and buffers components', | ||
| (tester) async { | ||
| streamController.add( | ||
| '{"messageType": "ComponentUpdate", "runtimeType": "componentUpdate", "components": [{"id": "root", "type": "Column"}]}'); |
There was a problem hiding this comment.
The JSON string used in this test (and others in this file) includes a "runtimeType": "componentUpdate" field. This field is not defined in the SimpleGspStreamMessage protocol and is not used by the fromJson factory. While the test passes because the parser ignores it, the test data should accurately reflect the protocol schema. Including extraneous fields can be confusing and may mask issues if the server-side implementation changes. Please remove the runtimeType fields from the JSON in the tests.
'{"messageType": "ComponentUpdate", "components": [{"id": "root", "type": "Column"}]}
Summary of ChangesThis pull request introduces a new Flutter package, Highlights
Changelog
Activity
|
Description
This creates a prototype of a simple parser for JSON streamed UI descriptions.