feat: add DynamicDataProvider and TemplateBuilder#454
Open
Potatomonsta wants to merge 4 commits intodevfrom
Open
feat: add DynamicDataProvider and TemplateBuilder#454Potatomonsta wants to merge 4 commits intodevfrom
Potatomonsta wants to merge 4 commits intodevfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR introduces StacDynamicDataProvider and StacTemplateBuilder widgets to enable declarative network data fetching and template-driven list rendering. New parsers and utilities support dynamic data resolution through provider scopes. The framework adds a second-pass JSON resolution step. An example app is refactored to use the new pattern. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DDP as StacDynamicDataProvider<br/>Widget
participant Network as StacNetworkService
participant Scope as DynamicDataScope
participant TB as StacTemplateBuilder<br/>Parser
Client->>DDP: Build with network request
DDP->>Network: Fetch data
Network-->>DDP: Return JSON response
alt Has targetPath
DDP->>DDP: Extract nested data
end
DDP->>Scope: Wrap with provider data
Scope-->>Client: Provide scope to subtree
Client->>TB: Render template builder
TB->>Scope: Resolve list data via provider
Scope-->>TB: Return data from scope
loop For each item in list
TB->>TB: Apply itemTemplate
TB->>TB: Replace {{placeholders}}
end
TB->>TB: Inject children into layout widget
TB-->>Client: Render final widget tree
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
examples/movie_app/stac/home_screen.dart (1)
84-90: Prefer stable provider IDs over UI titles.Using display strings as
id/providerIdcouples data binding to copy/i18n changes; stable constants are safer.🔧 Proposed refactor
-StacWidget _buildMovieSection({ - required String title, - required StacNetworkRequest request, -}) { +StacWidget _buildMovieSection({ + required String title, + required String providerId, + required StacNetworkRequest request, +}) { return StacDynamicDataProvider( - id: title, + id: providerId, @@ child: StacTemplateBuilder( - providerId: title, + providerId: providerId, itemTemplate: _buildMoviePosterItem(),Also applies to: 114-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/movie_app/stac/home_screen.dart` around lines 84 - 90, The StacDynamicDataProvider is using the UI title string as its id (in _buildMovieSection) which couples data binding to UI copy/i18n; change to use a stable constant or enum value for the provider id (e.g., define a const String or enum like MovieSectionId.NEW_RELEASES / POPULAR and pass that into StacDynamicDataProvider's id/providerId instead of title) and update the call sites (including the other occurrences flagged around lines 114-115) so the display title remains only for UI and the provider id is a stable, non-localized identifier.packages/stac/lib/src/utils/template_utils.dart (1)
118-136:isEmptyListis too broad for rendering decisions.Current recursion returns
trueif any nested list is empty, which can incorrectly route non-empty payloads to empty-state templates.🔧 Proposed fix
bool isEmptyList(dynamic data) { - if (data is List && data.isEmpty) { - return true; - } - - if (data is Map) { - for (final value in data.values) { - if (value is List && value.isEmpty) { - return true; - } - if (value is Map && isEmptyList(value)) { - return true; - } - } - } - - return false; + return data is List && data.isEmpty; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/utils/template_utils.dart` around lines 118 - 136, isEmptyList currently returns true if any nested List is empty, which misroutes non-empty payloads; change its semantics so it returns true only when the inspected value is truly empty: if data is a List return data.isEmpty; if data is a Map return true only if every value is empty (use recursion: for each value, if value is a List return value.isNotEmpty -> then return false; if value is a Map call isEmptyList(value) and if any recursive call returns false then overall Map is not empty); for any other types return false. Update the function isEmptyList to implement this "all children empty -> true" logic instead of the current "any child empty -> true".packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart (2)
45-58: Consider handling widget model updates.The
_fetchDatais only called ininitState, which means if the widget's model changes (e.g., different request URL), the data won't be re-fetched. If the parent can rebuild with a differentStacDynamicDataProvidermodel, consider implementingdidUpdateWidgetto re-fetch when the request changes.♻️ Suggested implementation
`@override` void initState() { super.initState(); _future = _fetchData(); } + + `@override` + void didUpdateWidget(covariant _DynamicDataProviderWidget oldWidget) { + super.didUpdateWidget(oldWidget); + if (widget.model.request != oldWidget.model.request || + widget.model.id != oldWidget.model.id) { + _future = _fetchData(); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart` around lines 45 - 58, The widget currently calls _fetchData only in initState so updates to widget.model.request won't trigger a re-fetch; implement didUpdateWidget in the same StatefulWidget class to compare oldWidget.model.request (or the relevant identifier) with widget.model.request and if changed assign _future = _fetchData() (and call setState if needed) so the new request is fetched; reference initState, _fetchData, didUpdateWidget, widget.model.request and the _future field when making the change.
74-82: Consider handling List response data.The response data parsing handles
StringandMaptypes explicitly, but if the API returns a JSON array directly (i.e.,response.data is List), it falls through to the genericresponseData = response.databranch. While this works, adding explicit handling would improve clarity and ensure proper type safety.♻️ Suggested improvement
dynamic responseData; if (response.data is String) { responseData = jsonDecode(response.data); } else if (response.data is Map) { responseData = response.data; + } else if (response.data is List) { + responseData = response.data; } else { responseData = response.data; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart` around lines 74 - 82, The response parsing currently checks response.data for String and Map but doesn't explicitly handle List results; update the parsing around the responseData assignment so that if response.data is a List you set responseData = response.data as List<dynamic>, and also handle the case where jsonDecode(response.data) returns a List by checking the decoded value and assigning it to responseData accordingly (use jsonDecodeResult is List to cast to List<dynamic>); refer to the responseData variable, response.data checks, and the jsonDecode call to locate and update the logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart`:
- Around line 47-58: The child JSON injection currently unconditionally adds a
'children' key to the child's serialized map (see model.child.toJson(),
childJson and processedChildren in stac_template_builder_parser) which will be
dropped for widgets that do not support children; add an explicit validation
step before mutating childJson: detect the child's widget type (from the
serialized map/type discriminator), verify it is a layout widget that supports
'children' (e.g., only allow known container/layout widget types), and
throw/return a clear error with context if it does not; update the code around
model.child, childJson and the children injection to perform this check and
prevent silent loss of injected children.
In `@packages/stac/lib/src/utils/template_utils.dart`:
- Around line 55-59: The function currently returns the literal string "null"
when the variable current is null; change it to return a real null value instead
(i.e., return null) and update the function's return type to a nullable type
(e.g., String?) if needed so the compiler accepts a null return; then audit
callers that consume this function (those that perform placeholder substitution)
to handle nulls correctly (e.g., treat null as an empty string or skip
substitution) so rendered output no longer contains the literal "null".
---
Nitpick comments:
In `@examples/movie_app/stac/home_screen.dart`:
- Around line 84-90: The StacDynamicDataProvider is using the UI title string as
its id (in _buildMovieSection) which couples data binding to UI copy/i18n;
change to use a stable constant or enum value for the provider id (e.g., define
a const String or enum like MovieSectionId.NEW_RELEASES / POPULAR and pass that
into StacDynamicDataProvider's id/providerId instead of title) and update the
call sites (including the other occurrences flagged around lines 114-115) so the
display title remains only for UI and the provider id is a stable, non-localized
identifier.
In
`@packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart`:
- Around line 45-58: The widget currently calls _fetchData only in initState so
updates to widget.model.request won't trigger a re-fetch; implement
didUpdateWidget in the same StatefulWidget class to compare
oldWidget.model.request (or the relevant identifier) with widget.model.request
and if changed assign _future = _fetchData() (and call setState if needed) so
the new request is fetched; reference initState, _fetchData, didUpdateWidget,
widget.model.request and the _future field when making the change.
- Around line 74-82: The response parsing currently checks response.data for
String and Map but doesn't explicitly handle List results; update the parsing
around the responseData assignment so that if response.data is a List you set
responseData = response.data as List<dynamic>, and also handle the case where
jsonDecode(response.data) returns a List by checking the decoded value and
assigning it to responseData accordingly (use jsonDecodeResult is List to cast
to List<dynamic>); refer to the responseData variable, response.data checks, and
the jsonDecode call to locate and update the logic.
In `@packages/stac/lib/src/utils/template_utils.dart`:
- Around line 118-136: isEmptyList currently returns true if any nested List is
empty, which misroutes non-empty payloads; change its semantics so it returns
true only when the inspected value is truly empty: if data is a List return
data.isEmpty; if data is a Map return true only if every value is empty (use
recursion: for each value, if value is a List return value.isNotEmpty -> then
return false; if value is a Map call isEmptyList(value) and if any recursive
call returns false then overall Map is not empty); for any other types return
false. Update the function isEmptyList to implement this "all children empty ->
true" logic instead of the current "any child empty -> true".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/stac_gallery/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
examples/movie_app/stac/home_screen.dartpackages/stac/lib/src/framework/stac_service.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/dynamic_data_scope.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dartpackages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dartpackages/stac/lib/src/parsers/widgets/widgets.dartpackages/stac/lib/src/utils/template_utils.dartpackages/stac_core/lib/foundation/specifications/widget_type.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dartpackages/stac_core/lib/widgets/widgets.dart
packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart
Show resolved
Hide resolved
Potatomonsta
force-pushed
the
mn/dsl-templatable
branch
from
0b3a616 to
7268b62
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart (1)
47-58:⚠️ Potential issue | 🟠 MajorValidate that
model.childcan hostchildrenbefore injection.Line 51 unconditionally injects/merges a
childrenfield. Ifmodel.childis not a multi-child host, the generated template output is effectively lost. This should fail fast with a clear error.🛠️ Suggested guard
final childJson = jsonDecode(jsonEncode(model.child.toJson())) as Map<String, dynamic>; +final childType = childJson['type'] as String?; +const multiChildTypes = <String>{ + 'column', + 'row', + 'wrap', + 'stack', + 'listView', + 'gridView', +}; +if (childType == null || !multiChildTypes.contains(childType)) { + throw FormatException( + 'TemplateBuilder child type "$childType" does not support "children".', + ); +} if (!childJson.containsKey('children')) { childJson['children'] = []; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart` around lines 47 - 58, Before mutating childJson and adding processedChildren, validate that model.child can actually host multiple children: detect this by checking the widget type (e.g., model.child is MultiChildWidget / Row / Column / Stack as appropriate) or by confirming the original toJson contains a 'children' key that is intended to be a list; if neither check passes, throw a clear exception (or assert) naming model.child and processedChildren to fail fast rather than silently dropping the generated children. Apply this guard around the existing childJson manipulation (referencing model.child, childJson and processedChildren) so injection only proceeds when the target supports 'children'.
🧹 Nitpick comments (2)
packages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dart (1)
164-164: Update map type toMap<String, dynamic>for explicit placeholder lookup.Line 161 explicitly converts to
Map<dynamic, dynamic>, which weakens type guarantees. Since template placeholders use string keys, preferMap<String, dynamic>instead. This requires updating both the conversion at line 161 and theprocessTemplateRecursivelyfunction signature inpackages/stac/lib/src/utils/template_utils.dart(line 66) to accept the more specific type.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dart` at line 164, The map conversion currently casts resolvedTemplateJson to Map<dynamic, dynamic>; change the local variable mapDataContext to be typed Map<String, dynamic> when converting resolvedTemplateJson, and update the processTemplateRecursively signature in packages/stac/lib/src/utils/template_utils.dart to accept Map<String, dynamic> (and any internal uses to index by String). Locate the call using resolvedTemplateJson and mapDataContext and ensure all lookups use string keys, then adjust processTemplateRecursively (and any related helper functions) to use Map<String, dynamic> to preserve stronger typing for placeholder lookup.examples/movie_app/stac/home_screen.dart (1)
84-90: Use a stable provider ID instead of display title.Line 89 and Line 114 currently key scope data by
title. Prefer a stable non-UI identifier to avoid key collisions/regressions when labels change.♻️ Proposed refactor
-StacWidget _buildMovieSection({ - required String title, - required StacNetworkRequest request, -}) { +StacWidget _buildMovieSection({ + required String providerId, + required String title, + required StacNetworkRequest request, +}) { return StacDynamicDataProvider( - id: title, + id: providerId, request: request, targetPath: 'results', @@ child: StacTemplateBuilder( - providerId: title, + providerId: providerId, itemTemplate: _buildMoviePosterItem(),_buildMovieSection( + providerId: 'now_playing', title: AppStrings.nowPlaying, request: StacNetworkRequest(Also applies to: 114-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/movie_app/stac/home_screen.dart` around lines 84 - 90, The provider is currently keyed by the UI title (id: title) which can change and cause collisions; update the StacWidget _buildMovieSection signature to accept a required stable identifier (e.g., required String id) and use that for StacDynamicDataProvider's id instead of the display title, then update all callers (the other usage around lines 114-115) to pass stable constant IDs (like "popular_movies", "top_rated", etc.) while still passing the display title for the title parameter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart`:
- Around line 43-49: The state only initializes _future in initState, so when
widget.request, widget.id or widget.targetPath change the old Future remains;
implement didUpdateWidget(StatefulWidget oldWidget) in the same State class,
call super.didUpdateWidget(oldWidget), compare the new widget.request, widget.id
and widget.targetPath to oldWidget.request, oldWidget.id and
oldWidget.targetPath, and when any differ call setState(() => _future =
_fetchData()) to refetch; keep using the existing _fetchData function and
_future field.
---
Duplicate comments:
In
`@packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart`:
- Around line 47-58: Before mutating childJson and adding processedChildren,
validate that model.child can actually host multiple children: detect this by
checking the widget type (e.g., model.child is MultiChildWidget / Row / Column /
Stack as appropriate) or by confirming the original toJson contains a 'children'
key that is intended to be a list; if neither check passes, throw a clear
exception (or assert) naming model.child and processedChildren to fail fast
rather than silently dropping the generated children. Apply this guard around
the existing childJson manipulation (referencing model.child, childJson and
processedChildren) so injection only proceeds when the target supports
'children'.
---
Nitpick comments:
In `@examples/movie_app/stac/home_screen.dart`:
- Around line 84-90: The provider is currently keyed by the UI title (id: title)
which can change and cause collisions; update the StacWidget _buildMovieSection
signature to accept a required stable identifier (e.g., required String id) and
use that for StacDynamicDataProvider's id instead of the display title, then
update all callers (the other usage around lines 114-115) to pass stable
constant IDs (like "popular_movies", "top_rated", etc.) while still passing the
display title for the title parameter.
In
`@packages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dart`:
- Line 164: The map conversion currently casts resolvedTemplateJson to
Map<dynamic, dynamic>; change the local variable mapDataContext to be typed
Map<String, dynamic> when converting resolvedTemplateJson, and update the
processTemplateRecursively signature in
packages/stac/lib/src/utils/template_utils.dart to accept Map<String, dynamic>
(and any internal uses to index by String). Locate the call using
resolvedTemplateJson and mapDataContext and ensure all lookups use string keys,
then adjust processTemplateRecursively (and any related helper functions) to use
Map<String, dynamic> to preserve stronger typing for placeholder lookup.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/stac_gallery/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
examples/movie_app/stac/home_screen.dartexamples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swiftpackages/stac/lib/src/framework/stac_service.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/dynamic_data_scope.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dartpackages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dartpackages/stac/lib/src/parsers/widgets/widgets.dartpackages/stac/lib/src/utils/template_utils.dartpackages/stac_core/lib/foundation/specifications/widget_type.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dartpackages/stac_core/lib/widgets/widgets.dart
💤 Files with no reviewable changes (1)
- examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dart
- packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/dynamic_data_scope.dart
- packages/stac/lib/src/utils/template_utils.dart
- packages/stac_core/lib/foundation/specifications/widget_type.dart
- packages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dart
- packages/stac_core/lib/widgets/template_builder/stac_template_builder.dart
- packages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.dart
...ac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart
Show resolved
Hide resolved
Potatomonsta
force-pushed
the
mn/dsl-templatable
branch
from
7268b62 to
a4a1c82
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart (1)
45-49:⚠️ Potential issue | 🟠 MajorRefetch is still not triggered when provider config changes.
Line 48 initializes
_futureonce ininitState; reused state keeps stale data ifrequest,id, ortargetPathchanges.🔧 Proposed fix
class _DynamicDataProviderWidgetState extends State<_DynamicDataProviderWidget> { late Future<Response?> _future; @@ void initState() { super.initState(); _future = _fetchData(); } + + `@override` + void didUpdateWidget(covariant _DynamicDataProviderWidget oldWidget) { + super.didUpdateWidget(oldWidget); + final requestChanged = jsonEncode(oldWidget.model.request.toJson()) != + jsonEncode(widget.model.request.toJson()); + final idChanged = oldWidget.model.id != widget.model.id; + final pathChanged = oldWidget.model.targetPath != widget.model.targetPath; + + if (requestChanged || idChanged || pathChanged) { + setState(() { + _future = _fetchData(); + }); + } + }#!/bin/bash # Verify whether _DynamicDataProviderWidgetState handles model updates. # Expected: no didUpdateWidget => issue confirmed. rg -n -C3 'class _DynamicDataProviderWidgetState|initState|didUpdateWidget|_future\s*=' packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart` around lines 45 - 49, The state initializes _future once in initState (in _DynamicDataProviderWidgetState) so updates to widget.request, widget.id, or widget.targetPath don't trigger a refetch; implement didUpdateWidget to compare the incoming widget's request/id/targetPath to the oldWidget and when any differ call setState and reassign _future = _fetchData() so the UI will refetch, ensuring you reference the existing _fetchData method and the _future field in the new logic.
🧹 Nitpick comments (1)
examples/movie_app/stac/home_screen.dart (1)
101-106: Consider simplifying single-child Row or documenting intent.
StacRowwithspaceBetweenalignment containing only one child has no practical effect—the single child will be positioned at the start regardless. If this is a placeholder for a future "See All" button, consider adding a comment. Otherwise, theStacRowwrapper could be removed.♻️ Suggested simplification (if no future additions planned)
StacPadding( padding: StacEdgeInsets.only( left: 16, right: 16, top: 24, bottom: 10, ), - child: StacRow( - mainAxisAlignment: StacMainAxisAlignment.spaceBetween, - children: [ - StacText(data: title, style: StacThemeData.textTheme.labelLarge), - ], - ), + child: StacText(data: title, style: StacThemeData.textTheme.labelLarge), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/movie_app/stac/home_screen.dart` around lines 101 - 106, This StacRow uses StacMainAxisAlignment.spaceBetween but only contains a single child (StacText with title and StacThemeData.textTheme.labelLarge), so the spaceBetween alignment has no effect; either remove the StacRow wrapper and render the StacText directly, or if you intend to add a trailing "See All" button later, add a brief comment above the StacRow explaining it’s a deliberate placeholder for a future second child (e.g., a "See All" action) so reviewers understand the intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stac_cli/lib/src/models/project/subscription.g.dart`:
- Around line 10-33: The generated file subscription.g.dart has formatting
differences causing CI failure; run the repository formatter and regenerate the
generated file instead of hand-editing: run the formatting task (e.g., melos run
format) and, if this file is produced by codegen, re-run the generation step
that emits subscription.g.dart so the FirestoreDateTimeNullable/fromJson calls
and enum decode lines (e.g., _$SubscriptionStatusEnumMap,
_$SubscriptionEnvironmentEnumMap) are properly formatted; commit the reformatted
and regenerated subscription.g.dart to clear the CI formatting error.
In
`@packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart`:
- Line 70: CI is failing due to formatting; run the project's formatter (e.g.,
execute "melos run format") and reformat the file containing the if (childType
== null || !_layoutWidgetTypesWithChildren.contains(childType)) check so the
Dart formatter adjusts spacing/line breaks to match project style; ensure you
stage the reformatted changes and re-run the formatter before committing.
In `@packages/stac/lib/src/utils/template_utils.dart`:
- Around line 72-93: The current substitution in the template replacement loop
(inside the block using regex r'\{\{([^}]+)\}\}', processing variables via
extractNestedData) always stringifies results into processedValue and assigns
template[key] = processedValue, which loses native types when the entire field
is a single placeholder; change the logic so that if the original value exactly
equals the single-placeholder match (e.g., value.trim() == placeholder) and the
resolved dataValue is non-null, assign template[key] = dataValue (preserve its
native type) instead of replacing as a string, otherwise continue with the
existing string replacement flow; apply the same change to the analogous code at
the other occurrence (lines flagged 176-192) to keep behavior consistent.
---
Duplicate comments:
In
`@packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dart`:
- Around line 45-49: The state initializes _future once in initState (in
_DynamicDataProviderWidgetState) so updates to widget.request, widget.id, or
widget.targetPath don't trigger a refetch; implement didUpdateWidget to compare
the incoming widget's request/id/targetPath to the oldWidget and when any differ
call setState and reassign _future = _fetchData() so the UI will refetch,
ensuring you reference the existing _fetchData method and the _future field in
the new logic.
---
Nitpick comments:
In `@examples/movie_app/stac/home_screen.dart`:
- Around line 101-106: This StacRow uses StacMainAxisAlignment.spaceBetween but
only contains a single child (StacText with title and
StacThemeData.textTheme.labelLarge), so the spaceBetween alignment has no
effect; either remove the StacRow wrapper and render the StacText directly, or
if you intend to add a trailing "See All" button later, add a brief comment
above the StacRow explaining it’s a deliberate placeholder for a future second
child (e.g., a "See All" action) so reviewers understand the intent.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/stac_gallery/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
examples/movie_app/stac/home_screen.dartexamples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swiftpackages/stac/lib/src/framework/stac_service.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/dynamic_data_scope.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dartpackages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dartpackages/stac/lib/src/parsers/widgets/widgets.dartpackages/stac/lib/src/utils/template_utils.dartpackages/stac_cli/lib/src/commands/init_command.dartpackages/stac_cli/lib/src/exceptions/auth_exception.dartpackages/stac_cli/lib/src/exceptions/build_exception.dartpackages/stac_cli/lib/src/models/project/project.g.dartpackages/stac_cli/lib/src/models/project/subscription.g.dartpackages/stac_cli/lib/src/models/project/ui_loads.dartpackages/stac_cli/lib/src/models/project/ui_loads.g.dartpackages/stac_cli/lib/src/services/auth_service.dartpackages/stac_cli/lib/src/services/build_service.dartpackages/stac_cli/lib/src/services/upgrade_service.dartpackages/stac_core/lib/foundation/specifications/widget_type.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dartpackages/stac_core/lib/widgets/widgets.dart
💤 Files with no reviewable changes (1)
- examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift
✅ Files skipped from review due to trivial changes (9)
- packages/stac_cli/lib/src/models/project/ui_loads.g.dart
- packages/stac_cli/lib/src/models/project/ui_loads.dart
- packages/stac_cli/lib/src/services/build_service.dart
- packages/stac_cli/lib/src/services/auth_service.dart
- packages/stac_cli/lib/src/exceptions/build_exception.dart
- packages/stac_cli/lib/src/commands/init_command.dart
- packages/stac_cli/lib/src/models/project/project.g.dart
- packages/stac_cli/lib/src/exceptions/auth_exception.dart
- packages/stac_cli/lib/src/services/upgrade_service.dart
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/stac_core/lib/widgets/widgets.dart
- packages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dart
- packages/stac_core/lib/widgets/template_builder/stac_template_builder.dart
- packages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dart
- packages/stac_core/lib/foundation/specifications/widget_type.dart
- packages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dart
- packages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.dart
packages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dart
Outdated
Show resolved
Hide resolved
Potatomonsta
force-pushed
the
mn/dsl-templatable
branch
from
a4a1c82 to
0fb0142
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/stac/lib/src/utils/template_utils.dart (1)
118-136:isEmptyListmay produce unexpected results for nested structures.The function returns
trueif any nested list within a map is empty, not just the top-level data. This could cause false positives where a structure like{"items": [1,2,3], "metadata": []}would be considered "empty" due to the emptymetadataarray.Consider whether this semantic is intentional. If checking only the top-level or a specific path is intended, the function name and behavior could be clarified.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stac/lib/src/utils/template_utils.dart` around lines 118 - 136, isEmptyList currently returns true if any nested list in a Map is empty, causing false positives (e.g., {"items":[1,2,3],"metadata":[]}); update the isEmptyList function so it only treats data as "empty" when the top-level value is an empty List, or when a Map is considered empty only if all of its values are empty according to the same recursive rule (i.e., change the Map iteration logic in isEmptyList to require every value to be empty rather than any single empty value), and add/adjust unit tests to cover {"items":[1,2,3],"metadata":[]} and other nested cases; keep the function name isEmptyList or rename it consistently if you decide to change semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/stac/lib/src/utils/template_utils.dart`:
- Around line 118-136: isEmptyList currently returns true if any nested list in
a Map is empty, causing false positives (e.g., {"items":[1,2,3],"metadata":[]});
update the isEmptyList function so it only treats data as "empty" when the
top-level value is an empty List, or when a Map is considered empty only if all
of its values are empty according to the same recursive rule (i.e., change the
Map iteration logic in isEmptyList to require every value to be empty rather
than any single empty value), and add/adjust unit tests to cover
{"items":[1,2,3],"metadata":[]} and other nested cases; keep the function name
isEmptyList or rename it consistently if you decide to change semantics.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/stac_gallery/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (25)
examples/movie_app/stac/home_screen.dartexamples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swiftpackages/stac/lib/src/framework/stac_service.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/dynamic_data_scope.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/stac_dynamic_data_provider_parser.dartpackages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dartpackages/stac/lib/src/parsers/widgets/stac_template_builder/stac_template_builder_parser.dartpackages/stac/lib/src/parsers/widgets/widgets.dartpackages/stac/lib/src/utils/template_utils.dartpackages/stac_cli/lib/src/commands/init_command.dartpackages/stac_cli/lib/src/exceptions/auth_exception.dartpackages/stac_cli/lib/src/exceptions/build_exception.dartpackages/stac_cli/lib/src/models/project/project.g.dartpackages/stac_cli/lib/src/models/project/subscription.g.dartpackages/stac_cli/lib/src/models/project/ui_loads.dartpackages/stac_cli/lib/src/models/project/ui_loads.g.dartpackages/stac_cli/lib/src/services/auth_service.dartpackages/stac_cli/lib/src/services/build_service.dartpackages/stac_cli/lib/src/services/upgrade_service.dartpackages/stac_core/lib/foundation/specifications/widget_type.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.dartpackages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.dartpackages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dartpackages/stac_core/lib/widgets/widgets.dart
💤 Files with no reviewable changes (1)
- examples/stac_gallery/macos/Flutter/GeneratedPluginRegistrant.swift
🚧 Files skipped from review as they are similar to previous changes (16)
- packages/stac_cli/lib/src/models/project/project.g.dart
- packages/stac_cli/lib/src/models/project/ui_loads.dart
- packages/stac/lib/src/parsers/widgets/stac_dynamic_view/stac_dynamic_view_parser.dart
- packages/stac_core/lib/foundation/specifications/widget_type.dart
- packages/stac_cli/lib/src/services/upgrade_service.dart
- packages/stac_core/lib/widgets/template_builder/stac_template_builder.g.dart
- packages/stac/lib/src/parsers/widgets/stac_dynamic_data_provider/dynamic_data_scope.dart
- packages/stac_cli/lib/src/commands/init_command.dart
- packages/stac_cli/lib/src/models/project/ui_loads.g.dart
- packages/stac_cli/lib/src/models/project/subscription.g.dart
- packages/stac_cli/lib/src/exceptions/auth_exception.dart
- packages/stac_cli/lib/src/services/build_service.dart
- packages/stac_cli/lib/src/services/auth_service.dart
- packages/stac_core/lib/widgets/template_builder/stac_template_builder.dart
- packages/stac_core/lib/widgets/dynamic_data_provider/stac_dynamic_data_provider.g.dart
- packages/stac_cli/lib/src/exceptions/build_exception.dart
- Add shared template utilities for nested data extraction and placeholder processing - Introduce DynamicDataProvider model/parser and DynamicDataScope for scoped fetched data - Introduce TemplateBuilder model/parser for data-driven child generation from lists or direct JSON - Wire DynamicDataProvider/TemplateBuilder into StacService resolution and parser registry - Refactor movie_app home_screen to use DynamicDataProvider + TemplateBuilder instead of raw JSON itemTemplate
- Adjusted formatting for better readability in `stac_template_builder_parser.dart`, `init_command.dart`, and various exception classes. - Enhanced JSON serialization/deserialization structure in `project.g.dart`, `subscription.g.dart`, and `ui_loads.g.dart`. - Refactored `auth_service.dart` and `build_service.dart` for consistent line breaks and improved clarity. - Updated `analysis_options.yaml` to switch linting from `flutter_lints` to `lints/recommended`.
- Updated `processTemplateRecursively` and `resolveDynamicDataInJson` to handle cases where only a single placeholder is present. - Improved nested data extraction and resolution logic for better accuracy in template processing.
Potatomonsta
force-pushed
the
mn/dsl-templatable
branch
from
10d8ee5 to
e9281f3
Compare
- Bumped versions of `stac`, `stac_core`, `stac_framework`, and `stac_cli` to 1.4.0 across multiple example applications and the CLI package.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Summary by CodeRabbit