refactor: Replace StacSetValue implementation with a new structure stac_core #363
Conversation
… stac_core - Remove the old StacSetValue class and its associated files. - Introduce a new StacSetValue widget in stac_core that manages application state through key-value pairs. - Update exports to reflect the new StacSetValue parser and widget structure.
WalkthroughStacSetValue was removed from the parser layer (packages/stac) and added as a JsonSerializable widget implementation in packages/stac_core. Parser code now uses the core widget's JSON representation and exports were updated to point to parser modules and new parsers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App
participant Parser as STAC Parser (packages/stac)
participant Core as stac_core
participant Widget as StacSetValue (core)
App->>Parser: provide widget JSON
Parser->>Core: StacWidget.fromJson(json)
Core->>Widget: construct StacSetValue(values, child?)
Widget-->>Core: instance (serializable)
Core-->>Parser: StacWidget (with child.toJson() available)
Parser->>Parser: resolveVariablesInJson(childJson)
Parser-->>App: built widget tree
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value.freezed.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value.g.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value_parser.dart(0 hunks)packages/stac/lib/src/parsers/widgets/widgets.dart(1 hunks)packages/stac_core/lib/widgets/set_value/stac_set_value.dart(1 hunks)packages/stac_core/lib/widgets/set_value/stac_set_value.g.dart(1 hunks)packages/stac_core/lib/widgets/widgets.dart(1 hunks)
💤 Files with no reviewable changes (4)
- packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value_parser.dart
- packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value.g.dart
- packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value.freezed.dart
- packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: analyze
- GitHub Check: format
- GitHub Check: pub_get_check
🔇 Additional comments (9)
packages/stac_core/lib/widgets/widgets.dart (1)
63-63: LGTM!The export is correctly placed and follows the alphabetical ordering convention.
packages/stac/lib/src/parsers/widgets/widgets.dart (2)
62-62: LGTM!The export correctly references the parser implementation, aligning with the separation of widget (now in
stac_core) from parser logic.
66-67: LGTM!The new parser exports are correctly placed in alphabetical order and follow the established naming convention.
packages/stac_core/lib/widgets/set_value/stac_set_value.dart (6)
1-5: LGTM!All imports are necessary and the part directive is correctly configured for
json_serializablecode generation.
33-34: LGTM!The
@JsonSerializable()annotation is correctly applied, and the class properly extendsStacWidget.
40-40: LGTM!The
constconstructor with sensible defaults (empty list forvalues, optionalchild) is well-designed.
43-46: Fields are correctly declared as final with appropriate nullability.The
finalmodifier ensures immutability, and the nullablechildallows flexible usage.
49-50: LGTM!The type getter correctly overrides the base class and returns the appropriate widget type identifier.
53-58: LGTM!The JSON serialization methods correctly delegate to the generated code and follow the standard
json_serializablepattern.
| /// A Stac widget that sets values in the application state. | ||
| /// | ||
| /// This widget allows you to set multiple key-value pairs in the application's | ||
| /// state and optionally render a child widget. It's useful for managing | ||
| /// application state through JSON configuration. | ||
| /// | ||
| /// ```dart | ||
| /// StacSetValue( | ||
| /// values: [ | ||
| /// {"key": "userName", "value": "John Doe"}, | ||
| /// {"key": "isLoggedIn", "value": true}, | ||
| /// ], | ||
| /// child: StacText(data: 'Welcome!'), | ||
| /// ) | ||
| /// ``` | ||
| /// | ||
| /// ```json | ||
| /// { | ||
| /// "type": "setValue", | ||
| /// "values": [ | ||
| /// {"key": "userName", "value": "John Doe"}, | ||
| /// {"key": "isLoggedIn", "value": true} | ||
| /// ], | ||
| /// "child": {"type": "text", "data": "Welcome!"} | ||
| /// } | ||
| /// ``` |
There was a problem hiding this comment.
Documentation implies stricter structure than type enforces.
The documentation examples show a specific schema with "key" and "value" fields in each map, but the field type List<Map<String, dynamic>> allows any map structure. This could lead users to pass incorrectly shaped data that compiles but fails at runtime.
Consider one of the following:
- Stronger typing (recommended): Define a value class to enforce the structure:
class StateValue {
final String key;
final dynamic value;
const StateValue({required this.key, required this.value});
factory StateValue.fromJson(Map<String, dynamic> json) => StateValue(
key: json['key'] as String,
value: json['value'],
);
Map<String, dynamic> toJson() => {'key': key, 'value': value};
}
// Then change the field to:
final List<StateValue> values;-
Runtime validation: Add validation in a custom
fromJsonconverter to check for required keys. -
Documentation update: Clarify that while the examples show
key/valuestructure, other map structures are technically allowed if that's the intended flexibility.
🤖 Prompt for AI Agents
In packages/stac_core/lib/widgets/set_value/stac_set_value.dart around lines 7
to 32, the doc examples show each entry as a map with "key" and "value" but the
field is typed as List<Map<String, dynamic>>, which permits incorrect shapes;
fix by introducing a strongly-typed value model (e.g. StateValue with required
String key and dynamic value plus fromJson/toJson) and change the widget field
to List<StateValue>, update JSON parsing to construct StateValue instances (or,
if you must keep Map typing, add explicit runtime validation in fromJson to
assert each map contains a String "key" and a "value" entry and throw a
descriptive error), and update documentation to match the chosen approach.
| (json['values'] as List<dynamic>?) | ||
| ?.map((e) => e as Map<String, dynamic>) | ||
| .toList() ?? |
There was a problem hiding this comment.
Unsafe cast may cause runtime exception.
The generated code casts each element to Map<String, dynamic> without validation. If the JSON values array contains non-Map elements (e.g., primitives or incorrectly shaped data), this will throw a runtime exception.
Consider adding a JsonKey annotation with a custom converter to the values field in stac_set_value.dart to validate and safely convert each element, or add a readValue callback to handle type mismatches gracefully.
Example with a custom converter:
@JsonKey(fromJson: _parseValues)
final List<Map<String, dynamic>> values;
static List<Map<String, dynamic>> _parseValues(dynamic json) {
if (json is! List) return const [];
return json
.whereType<Map>()
.map((e) => Map<String, dynamic>.from(e))
.toList();
}🤖 Prompt for AI Agents
In packages/stac_core/lib/widgets/set_value/stac_set_value.g.dart around lines
11 to 13, the generated code unsafely casts elements of json['values'] to
Map<String, dynamic> which can throw at runtime for non-Map items; fix by adding
a safe fromJson converter on the values field in stac_set_value.dart (JsonKey
with fromJson or a custom JsonConverter) that rejects non-list inputs,
filters/validates elements with whereType<Map>(), converts each Map to
Map<String, dynamic> (e.g., Map<String, dynamic>.from), and returns an empty
list or default when the input is invalid, then re-run code generation to update
the .g.dart file.
… variable resolution
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value_parser.dart (1)
53-67: Add JSON round-trip tests for StacSetValueParser Cover thetoJson() → resolveVariablesInJson() → Stac.fromJson()flow inpackages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value_parser.dartto guard against data loss or parsing errors.
🧹 Nitpick comments (1)
packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value_parser.dart (1)
36-50: Add validation for key-value map structure.The code casts
value['key']toStringwithout checking if the key exists or is null, which could cause runtime exceptions if the data structure doesn't match expectations.Consider adding validation:
@override void initState() { super.initState(); for (final value in widget.model.values) { - _stacRegistry.setValue(value['key'] as String, value['value']); + final key = value['key']; + if (key is String && key.isNotEmpty) { + _stacRegistry.setValue(key, value['value']); + } } } @override void dispose() { for (final value in widget.model.values) { - _stacRegistry.removeValue(value['key'] as String); + final key = value['key']; + if (key is String && key.isNotEmpty) { + _stacRegistry.removeValue(key); + } } super.dispose(); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/stac/lib/src/parsers/widgets/stac_set_value/stac_set_value_parser.dart(1 hunks)
Description
Replace StacSetValue implementation with a new structure stac_core
Related Issues
Closes #
Type of Change
Summary by CodeRabbit
New Features
Refactor
Bug Fixes