Migrate gulf_client to use the latest protocol schema
#328
Conversation
Deletes the old `DataModelNode` and replaces the existing component and stream message models with new class structures that align with the official `gulf_schema.json`. This is the first phase of a larger refactoring effort. The new classes are currently empty shells without serialization logic, which will be implemented in subsequent phases. This change intentionally breaks the build, as the rest of the package has not yet been updated to use these new models.
Implements the `fromJson` factory constructors for all new data model classes, enabling the client to parse a JSONL stream conforming to the new schema. This includes the deserialization logic for: - The `GulfStreamMessage` sealed class and its variants. - The `Component` class. - The `ComponentProperties` discriminated union and all of its specific subclasses. - All helper classes (e.g., `BoundValue`, `Children`, `Action`).
Refactors the `GulfInterpreter` to align with the new schema-driven data models. Key changes include: - Replaces the `DataModelNode` map with a single `Map<String, dynamic>` to store the application's data model. - Updates the `processMessage` method to handle the new `GulfStreamMessage` types (`BeginRendering`, `DataModelUpdate`, etc.). - Implements a new path-based `_updateDataModel` method to insert or replace content in the data model. - Rewrites the `resolveDataBinding` method to traverse the new map-based data model. - Removes the obsolete `getDataNode` method.
Refactors the `_LayoutEngine` in `gulf_view.dart` to be compatible with the new schema-driven data models. Key changes include: - Replaces the property resolution logic with a `switch` expression on the `component.componentProperties` discriminated union. - Introduces a new `_resolveValue` helper method to handle the `BoundValue` type for data binding. - Removes the old, generic `_resolveProperties` method. - Updates the logic for building templated lists to align with the new component structure. This change brings the rendering logic in line with the new data models, making it more type-safe and robust.
Updates the `IMPLEMENTATION.md` file to reflect the significant architectural changes made during the refactoring. The documentation now accurately describes: - The new type-safe schema using discriminated unions for components. - The path-based, JSON-like data model. - The updated protocol message types. - The refactored `GulfInterpreter` and its new state management logic.
![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 is a significant and well-executed refactoring to align the gulf_client with the latest protocol schema. The move to a discriminated union pattern for components using ComponentProperties is a major improvement for type safety and maintainability. The introduction of a ComponentPropertiesVisitor and the addition of comprehensive tests are also excellent. I've identified a critical issue in the data model update logic and a few medium-severity suggestions to further improve type safety, consistency, and robustness. Overall, this is a high-quality contribution.
gspencergoog
changed the title
gspencergoog
changed the title
gulf_client to use the latest protocol schema
gspencergoog
force-pushed
the
gulf_refactor
branch
from
ffe128c to
6a36bc5
Compare
gspencergoog
force-pushed
the
gulf_refactor
branch
from
73308fe to
ac92e64
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This is a substantial and well-executed pull request that migrates the gulf_client to a new protocol schema. The architectural changes, such as introducing a sealed class hierarchy for component properties and refining the data model handling, are excellent improvements that significantly enhance type safety and maintainability. The updates to documentation and tests are thorough and align well with the code changes. My review comments focus on further hardening the implementation by addressing a security concern in the example app, improving schema consistency, and adding more robust error handling for invalid data and pathing.
| } else { | ||
| // Path is invalid for the current data model structure. | ||
| return; | ||
| } |
There was a problem hiding this comment.
When an invalid path is encountered during a data model update, the _updateDataModel method currently returns silently. This can make debugging difficult, as an invalid dataModelUpdate message will fail without any indication. It would be better to report this as an error, so the UI layer can display a meaningful message to the developer.
} else {
// Path is invalid for the current data model structure.
_error = 'Invalid path for data model update: $path';
notifyListeners();
return;
}|
|
||
| final builder = registry.getBuilder(component.type); | ||
| final properties = component.componentProperties; | ||
| final builder = registry.getBuilder(properties.runtimeType.toString()); |
There was a problem hiding this comment.
Looking up widget builders using properties.runtimeType.toString() is functional but can be fragile. It relies on developers registering the exact string name of the properties class, which is prone to typos and less discoverable. A more type-safe approach would be to use the Type object itself as the key in the WidgetRegistry.
This would involve changes in widget_registry.dart:
class WidgetRegistry {
final Map<Type, CatalogWidgetBuilder> _builders = {};
void register<T extends ComponentProperties>(CatalogWidgetBuilder builder) {
_builders[T] = builder;
}
CatalogWidgetBuilder? getBuilder(Type type) {
return _builders[type];
}
}Then, this line would become:
final builder = registry.getBuilder(properties.runtimeType);
And registration in example/lib/main.dart would become type-safe:
registry.register<ColumnProperties>((context, component, properties, children) {
// ...
});This would prevent a whole class of runtime errors and improve the developer experience.
| final resolvedProperties = visitor.visit( | ||
| templateComponent.componentProperties, | ||
| itemData as Map<String, Object?>, | ||
| ); |
There was a problem hiding this comment.
The cast itemData as Map<String, Object?> is unsafe. If the data list resolved from the data binding contains elements that are not maps, this will cause a runtime crash. It would be more robust to handle this case gracefully by performing a type check.
| final resolvedProperties = visitor.visit( | |
| templateComponent.componentProperties, | |
| itemData as Map<String, Object?>, | |
| ); | |
| final resolvedProperties = visitor.visit( | |
| templateComponent.componentProperties, | |
| itemData is Map<String, Object?> ? itemData : const <String, Object?>{}, | |
| ); |
Description
This pull request represents a substantial update to the
gulf_clientspike, aligning it with a new, more robust GULF protocol schema. The core purpose of these changes is to enhance the client's ability to render dynamic user interfaces from a streaming JSONL format by improving type safety, streamlining data management, and providing a clearer, more extensible framework for UI component definition and rendering. The modifications touch upon the fundamental data structures, message processing, and widget construction logic, ensuring a more reliable and maintainable client implementation.Highlights
gulf_clientspike has been migrated to use the latest GULF protocol schema, introducing a more structured and type-safe approach to defining UI components and data models.DataModelNodestructure to a directMap<String, dynamic>for data, and introducing a sealed class hierarchy forComponentPropertiesto enhance type safety.Map<String, dynamic>within theGulfInterpreter, with updates and data binding resolution performed using dot-separated paths, including support for array indexing.ComponentPropertiesVisitorhas been introduced to provide type-safe resolution of component properties and data bindings, simplifying the widget building process.IMPLEMENTATION.mddocument has been thoroughly updated to reflect the new protocol and architectural changes, and the example application (main.dart) has been revised to demonstrate the new schema and component handling.Changelog
ComponentPropertiestypes.src/models/data_node.dart.DataModelNodeto aMap<String, dynamic>.processMessageto handle new message types (BeginRenderinginstead ofUiRoot).resolveDataBindingto support dot-separated paths and array indexing.ComponentPropertiesas a sealed class with numerous subclasses for specific component types (e.g.,HeadingProperties,TextProperties).ValuewithBoundValuefor data binding.toJsonmethods andoperator==/hashCodeoverrides from model classes.GulfInterpreter's data model handling.GulfStreamMessagefactory constructor to use top-level keys for message type identification.BeginRenderingmessage type.DataModelUpdateto usepathandcontentsfields.toJsonmethods andoperator==/hashCodeoverrides from stream message classes.parseDoublehelper function for safe number parsing.ComponentPropertiesVisitorto resolve component properties and data bindings._GulfViewStateto display interpreter errors._LayoutEngineto utilizeComponentPropertiesVisitorfor property resolution._LayoutEnginefor consistency across different component types.mockitoto dev dependencies.ComponentPropertiesVisitor.FakeGulfInterpreterfor testing purposes.interpreter.dartand new message types.JsonUtils.DataModelNode.packages/spikes/gulf_client/test/widgets/gulf_view_test.dart.GulfViewto match the new component property handling and message types.