Stac Framework Enhancements: Navigation, Borders, Colors & Architecture Improvements #361
Conversation
WalkthroughRefactors bottom navigation to a controller-centric parser with an InheritedWidget scope, migrates its model to stac_core, and adjusts parsers to use typed parsing APIs. Updates set-value action to use StacAction instead of Map and invokes parse() for execution. Adds border constructors and a color opacity extension. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Widget as SetValue Parser
participant Model as StacSetValueAction
participant Action as StacAction
participant Ctx as Context
User->>Widget: onCall()
Widget->>Model: set value in context/state
alt action present
Widget->>Action: Model.action.parse(Ctx)
Action-->>Widget: completes (success/error)
else no action
Widget-->>User: return
end
sequenceDiagram
autonumber
participant App
participant Core as StacDefaultBottomNavigationController (core model)
participant Parser as Controller Parser
participant Scope as BottomNavigationScope
participant Ctrl as BottomNavigationController
participant Bar as BottomNavigationBar (parser widget)
participant View as BottomNavigationView (parser widget)
participant Child as Parsed Child
App->>Parser: parse(Core model)
Parser->>Ctrl: create with length/initialIndex
Parser->>Scope: provide {length, controller}
Scope-->>Bar: inherited controller
Scope-->>View: inherited controller
Bar->>Ctrl: set index on tap
View->>Ctrl: read index
View->>Child: model.children[index].parse(context)
Child-->>View: widget
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (16)
💤 Files with no reviewable changes (3)
🚧 Files skipped from review as they are similar to previous changes (8)
🔇 Additional comments (3)
Comment |
Potatomonsta
force-pushed
the
mn/DSL-bugfixes
branch
2 times, most recently
from
030f125 to
1641cb0
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
packages/stac_core/lib/foundation/borders/stac_border_radius/stac_border_radius.dart (1)
246-261: Fix error messages: incorrect type name in thrown ArgumentErrorMessages say “StacEdgeInsets” but this is StacBorderRadius.
- throw ArgumentError('Invalid input format for StacEdgeInsets'); + throw ArgumentError('Invalid input format for StacBorderRadius'); ... - throw ArgumentError('Invalid input format for StacEdgeInsets'); + throw ArgumentError('Invalid input format for StacBorderRadius');packages/stac_core/lib/actions/set_value/stac_set_value_action.dart (1)
14-23: Update documentation examples to reflect typedaction.The Dart example still shows a Map, which no longer matches the public API.
-/// const StacSetValueAction( -/// values: [ -/// {"key": "isLoggedIn", "value": true}, -/// {"key": "token", "value": "abc123"}, -/// ], -/// action: {"type": "navigate", "routeName": "/home"}, -/// ) +/// final action = StacAction.fromJson({ +/// "type": "navigate", +/// "routeName": "/home", +/// }); +/// StacSetValueAction( +/// values: [ +/// {"key": "isLoggedIn", "value": true}, +/// {"key": "token", "value": "abc123"}, +/// ], +/// action: action, +/// );JSON example can stay as-is since wire format remains a map.
Also applies to: 26-37
packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart (4)
41-51: Dispose controller and remove listener to avoid leaks and setState-after-dispose._state adds a listener but never removes/disposing the controller.
@override void initState() { super.initState(); _controller = BottomNavigationController( length: widget.model.length, initialIndex: widget.model.initialIndex ?? 0, ); _controller.addListener(_onIndexChange); } + + @override + void dispose() { + _controller.removeListener(_onIndexChange); + _controller.dispose(); + super.dispose(); + }
57-65: Handle model updates (length/initialIndex) in didUpdateWidget.Without reconciliation, controller can hold stale length/index when the model changes, breaking bar/view coordination.
@override Widget build(BuildContext context) { return BottomNavigationScope( length: widget.model.length, controller: _controller, child: widget.model.child.parse(context) ?? const SizedBox(), ); } + + @override + void didUpdateWidget( + covariant _DefaultBottomNavigationControllerWidget oldWidget, + ) { + super.didUpdateWidget(oldWidget); + final int newLength = widget.model.length; + if (newLength != oldWidget.model.length) { + final int proposed = + widget.model.initialIndex ?? _controller.index; + _controller.removeListener(_onIndexChange); + _controller.dispose(); + final int clampedInitial = + (newLength > 0 && proposed >= 0 && proposed < newLength) + ? proposed + : 0; + _controller = BottomNavigationController( + length: newLength, + initialIndex: clampedInitial, + ); + _controller.addListener(_onIndexChange); + setState(() {}); + } else if (widget.model.initialIndex != null && + widget.model.initialIndex != _controller.index) { + final idx = widget.model.initialIndex!; + if (idx >= 0 && idx < newLength) { + _controller.index = idx; + } + } + }
104-107: updateShouldNotify should depend on length/controller, not child identity.Comparing child causes unnecessary rebuilds and may miss changes in controller/length.
- @override - bool updateShouldNotify(covariant InheritedWidget oldWidget) { - return oldWidget.child != child; - } + @override + bool updateShouldNotify(covariant BottomNavigationScope oldWidget) { + return oldWidget.length != length || oldWidget.controller != controller; + }
134-143: Tighten index bounds and early-return logic.Ensure illegal indices never pass asserts; keep early return first to avoid asserting on degenerate lengths.
- void _changeIndex(int value) { - assert(value >= 0 && (value < length || length == 0)); - - if (value == _index || length < 2) { - return; - } - - _index = value; - notifyListeners(); - } + void _changeIndex(int value) { + if (value == _index || length < 2) return; + assert(value >= 0 && value < length); + _index = value; + notifyListeners(); + }
🧹 Nitpick comments (10)
packages/stac_core/lib/foundation/borders/stac_border/stac_border.dart (1)
86-124: Border.all: add basic validation and doc parity for strokeAlign
- Guard against negative width; optionally document expected strokeAlign range and include it in the JSON snippet for consistency.
factory StacBorder.all({ StacColor? color, StacBorderStyle? borderStyle, double? width, double? strokeAlign, }) { + assert(width == null || width >= 0, 'width cannot be negative'); return StacBorder( color: color, borderStyle: borderStyle, width: width, strokeAlign: strokeAlign, ); }packages/stac_core/lib/foundation/borders/stac_border_radius/stac_border_radius.dart (3)
108-141: horizontal(): add simple non-negative radius assertsSmall guardrails improve early failure and parity with typical radius expectations.
-const StacBorderRadius.horizontal({double? left, double? right}) - : this( +const StacBorderRadius.horizontal({double? left, double? right}) + : assert(left == null || left >= 0, 'left radius cannot be negative'), + assert(right == null || right >= 0, 'right radius cannot be negative'), + this( topLeft: left, topRight: right, bottomLeft: left, bottomRight: right, );
142-175: vertical(): add non-negative radius assertsSame rationale as horizontal().
-const StacBorderRadius.vertical({double? top, double? bottom}) - : this( +const StacBorderRadius.vertical({double? top, double? bottom}) + : assert(top == null || top >= 0, 'top radius cannot be negative'), + assert(bottom == null || bottom >= 0, 'bottom radius cannot be negative'), + this( topLeft: top, topRight: top, bottomLeft: bottom, bottomRight: bottom, );
176-207: circular(): add non-negative radius assert and cross-reference all()
- Add an assert for clarity.
- Optional: mention it’s equivalent to all(radius) in the docs.
-const StacBorderRadius.circular(double radius) - : this( +const StacBorderRadius.circular(double radius) + : assert(radius >= 0, 'radius cannot be negative'), + this( topLeft: radius, topRight: radius, bottomLeft: radius, bottomRight: radius, );packages/stac/lib/src/parsers/actions/stac_set_value/stac_set_value_action_parser.dart (2)
26-26: Guard nullable follow-up action when parsing/executing.If
actionis optional, prefer a null-aware call to avoid surprising NPEs and make the intent explicit. Also lets the method be a no-op when no follow-up action is provided.- return model.action.parse(context); + return model.action?.parse(context);Optionally remove
async(no awaits remain) to avoid an extra microtask hop:- FutureOr<dynamic> onCall( + FutureOr<dynamic> onCall( BuildContext context, StacSetValueAction model, - ) async { + ) {Please confirm
stac_action_parser.dartdefinesparseforStacAction?or that callers never rely onaction == null.
23-25: Stricter input validation forvalueskeys.Casting
value['key'] as Stringthrows a type error with little context if the payload is malformed. Add a clear check to fail fast with a helpful message.- for (final value in model.values ?? []) { - StacRegistry.instance.setValue(value['key'] as String, value['value']); - } + for (final value in model.values ?? const []) { + final key = value['key']; + if (key is! String || key.isEmpty) { + throw FormatException('StacSetValueAction.values[]: "key" must be a non-empty String.'); + } + StacRegistry.instance.setValue(key, value['value']); + }packages/stac_core/lib/actions/set_value/stac_set_value_action.dart (2)
50-50: API change:actionnow typed asStacAction?— verify downstream usage or add a legacy shim.This is a source-level breaking change for callers passing a
Map. Consider a deprecated convenience factory to ease migration.class StacSetValueAction extends StacAction { /// Creates a [StacSetValueAction] that writes [values] and optionally runs [action]. const StacSetValueAction({this.values, this.action}); + + @Deprecated('Use the typed `action: StacAction` instead.') + factory StacSetValueAction.legacy({ + List<Map<String, dynamic>>? values, + Map<String, dynamic>? action, + }) => + StacSetValueAction( + values: values, + action: action == null ? null : StacAction.fromJson(action), + );Run the verification script below to spot legacy call sites passing a map to
action.
39-39: Consider omitting nulls in JSON output.To avoid emitting
"action": nulland trim payloads, setincludeIfNull: false.Based on learnings
-@JsonSerializable() +@JsonSerializable(includeIfNull: false) class StacSetValueAction extends StacAction {packages/stac_core/lib/widgets/list_view/stac_list_view.dart (1)
122-123: Enhance itemTemplate documentation.The documentation for
itemTemplateis minimal. Consider adding details about:
- How it differs from the
childrenproperty- When to use
itemTemplatevschildren- Example JSON structure showing itemTemplate usage
- How data binding works with itemTemplate
packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart (1)
97-101: Demote log severity or provide maybeOf().Logging an error for missing scope can be noisy for optional composition. Prefer a warning, or add a nullable maybeOf() and reserve of() for non-null (assert).
- Log.e( + Log.w( "BottomNavigationScope.of() called with a context that does not contain a BottomNavigationScope.", );Optionally add:
- static BottomNavigationScope? maybeOf(BuildContext context) => context.dependOnInheritedWidgetOfExactType();
- Have of() call maybeOf() and assert non-null in debug.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
packages/stac/lib/src/parsers/actions/stac_set_value/stac_set_value_action_parser.dart(2 hunks)packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_bar/stac_bottom_navigation_bar_parser.dart(2 hunks)packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_view/stac_bottom_navigation_view_parser.dart(2 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.freezed.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart(6 hunks)packages/stac/lib/src/parsers/widgets/stac_list_view/stac_list_view_parser.dart(1 hunks)packages/stac/lib/src/parsers/widgets/widgets.dart(1 hunks)packages/stac_core/lib/actions/set_value/stac_set_value_action.dart(1 hunks)packages/stac_core/lib/actions/set_value/stac_set_value_action.g.dart(1 hunks)packages/stac_core/lib/foundation/borders/stac_border/stac_border.dart(1 hunks)packages/stac_core/lib/foundation/borders/stac_border_radius/stac_border_radius.dart(1 hunks)packages/stac_core/lib/foundation/colors/stac_color/stac_colors.dart(1 hunks)packages/stac_core/lib/widgets/align/stac_align.g.dart(2 hunks)packages/stac_core/lib/widgets/column/stac_column.dart(2 hunks)packages/stac_core/lib/widgets/column/stac_column.g.dart(2 hunks)packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart(1 hunks)packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart(1 hunks)packages/stac_core/lib/widgets/list_view/stac_list_view.dart(2 hunks)packages/stac_core/lib/widgets/list_view/stac_list_view.g.dart(2 hunks)packages/stac_core/lib/widgets/widgets.dart(1 hunks)
💤 Files with no reviewable changes (3)
- packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart
- packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart
- packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.freezed.dart
🔇 Additional comments (17)
packages/stac_core/lib/widgets/align/stac_align.g.dart (1)
9-13: Looks good — nullable alignment decode matches the widget contract.Allowing JSON decode to propagate
nullkeeps parity with the optionalalignmentfield and lets higher layers decide on defaults. No action needed.packages/stac_core/lib/foundation/borders/stac_border_radius/stac_border_radius.dart (1)
68-107: only(): API and implementation look goodConstructor mirrors common patterns and keeps const/redirecting form. No issues.
packages/stac_core/lib/actions/set_value/stac_set_value_action.g.dart (1)
14-16: Serialization aligns with typedStacAction— looks good.
fromJsonroutesactionthroughStacAction.fromJson, andtoJsoncallsaction?.toJson(). This matches the new field type and preserves wire format.Please confirm
StacAction.fromJsoncorrectly discriminates all supported action subtypes (incl. "navigate") and that generated code was rebuilt across packages.Also applies to: 22-22
packages/stac_core/lib/widgets/widgets.dart (1)
82-82: LGTM!The new export correctly exposes the default bottom navigation controller from stac_core, aligning with the architecture improvements mentioned in the PR objectives.
packages/stac/lib/src/parsers/widgets/stac_list_view/stac_list_view_parser.dart (1)
10-10: LGTM!Centralizing the
WidgetTypeimport fromstac_coreimproves consistency across the parser layer.packages/stac_core/lib/widgets/list_view/stac_list_view.g.dart (2)
42-44: LGTM!The generated JSON deserialization for
itemTemplatecorrectly follows the same pattern as other optionalStacWidgetfields likeseparator.
69-69: LGTM!The generated JSON serialization for
itemTemplatecorrectly handles the optional field.packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_bar/stac_bottom_navigation_bar_parser.dart (3)
6-6: LGTM!The import change to the parser variant aligns with the architecture improvements for bottom navigation mentioned in the PR objectives.
23-26: LGTM!Delegating to a private widget
_BottomNavigationBarWidgetis a clean encapsulation pattern that separates parser logic from rendering logic.
28-66: LGTM! Correctly fixes the timing issue.The private widget correctly defers
BottomNavigationScope.of(context)access to build time (line 35), which addresses the timing issue mentioned in the PR objectives. This ensures the InheritedWidget is accessed at the appropriate lifecycle phase.packages/stac_core/lib/widgets/list_view/stac_list_view.dart (1)
65-65: No action needed:itemTemplateis fully supported by the parser.The
stac_dynamic_view_parser.dartincludes logic to detect theitemTemplateJSON key, iterate over the data list, and generate child items accordingly—so the new field is already integrated.packages/stac_core/lib/widgets/column/stac_column.dart (1)
54-104: Item template integration looks solid.Constructor wiring and inline docs clearly align this widget with the new data-driven pattern used in
StacListView, with no backwards-compatibility risk. Nicely scoped addition.packages/stac/lib/src/parsers/widgets/widgets.dart (1)
24-24: Export swap matches the parser-based controller shift.Routing consumers through the parser implementation keeps the public surface consistent with the new architecture—no issues spotted.
packages/stac_core/lib/widgets/column/stac_column.g.dart (1)
38-57: Generated serialization stays in sync.
itemTemplatenow round-trips cleanly through JSON alongsidechildren; generation matches the manual field addition.packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_view/stac_bottom_navigation_view_parser.dart (1)
1-37: Deferred build solves the scope timing bug.Handing parsing off to
_BottomNavigationViewWidgetso the controller lookup occurs during build neatly avoids the prematureInheritedWidgetaccess, while the clamp keeps indices safe. Looks good.packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart (1)
9-24: New controller JSON wiring is consistent.
length/initialIndexcoercion and nestedchildserialization mirror the rest of the widget models—no gaps found.packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart (1)
123-126: LGTM on type wiring.type maps cleanly to WidgetType.defaultBottomNavigationController; consistent with Stac conventions.
.../widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart
Show resolved
Hide resolved
.../widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart
Show resolved
Hide resolved
| BottomNavigationController({this.initialIndex = 0, required this.length}) | ||
| : _index = initialIndex; | ||
|
|
||
| /// The initial index when the controller is created. | ||
| final int initialIndex; |
There was a problem hiding this comment.
Validate controller constructor inputs.
Guard against negative length and out-of-range initialIndex.
- BottomNavigationController({this.initialIndex = 0, required this.length})
- : _index = initialIndex;
+ BottomNavigationController({this.initialIndex = 0, required this.length})
+ : assert(length >= 0, 'length must be >= 0'),
+ assert(initialIndex >= 0, 'initialIndex must be >= 0'),
+ assert(length == 0 || initialIndex < length,
+ 'initialIndex must be < length when length > 0'),
+ _index = initialIndex;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BottomNavigationController({this.initialIndex = 0, required this.length}) | |
| : _index = initialIndex; | |
| /// The initial index when the controller is created. | |
| final int initialIndex; | |
| BottomNavigationController({this.initialIndex = 0, required this.length}) | |
| : assert(length >= 0, 'length must be >= 0'), | |
| assert(initialIndex >= 0, 'initialIndex must be >= 0'), | |
| assert(length == 0 || initialIndex < length, | |
| 'initialIndex must be < length when length > 0'), | |
| _index = initialIndex; | |
| /// The initial index when the controller is created. | |
| final int initialIndex; |
🤖 Prompt for AI Agents
packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart
around lines 117-121: the constructor currently does not validate inputs; add
guards to ensure length is positive (length > 0) and initialIndex is within
range (0 <= initialIndex < length). Implement these checks in the constructor
(use assert(...) for debug builds and throw ArgumentError or RangeError for
production) — e.g. assert(length > 0), assert(initialIndex >= 0 && initialIndex
< length) and throw a descriptive ArgumentError if validation fails — then keep
assigning _index = initialIndex.
.../lib/src/parsers/widgets/stac_bottom_navigation_view/stac_bottom_navigation_view_parser.dart
Show resolved
Hide resolved
| BuildContext context, | ||
| StacDefaultBottomNavigationController model, | ||
| ) { | ||
| BuildContext context, StacDefaultBottomNavigationController model) { |
| /// works in StacListView. | ||
| /// | ||
| /// Type: [StacWidget]? | ||
| final StacWidget? itemTemplate; |
There was a problem hiding this comment.
Is it possible to find a way so that we don't have to explicitly define item template in each class? Otherwise, we then add itemTemplate to each class, like gridview and all?
There was a problem hiding this comment.
@divyanshub024 perhaps can discuss. We can also add it to Row and GridView for now, maybe a parent abstract but thats going to add similar number of lines, but perhaps cleaner approach, Id say we can take up as an improvement to DynamicView in general later along with other improvements we have discussed in the past
Potatomonsta
force-pushed
the
mn/DSL-bugfixes
branch
from
1641cb0 to
1b5b61f
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart (2)
106-109: FixupdateShouldNotifyto detect controller/length changes.The current implementation only compares
child, which means descendant widgets won't rebuild whencontrollerorlengthchanges. This can lead to stale state inStacBottomNavigationBarandStacBottomNavigationView.Apply this diff:
@override bool updateShouldNotify(covariant InheritedWidget oldWidget) { - return oldWidget.child != child; + return oldWidget is! BottomNavigationScope || + oldWidget.controller != controller || + oldWidget.length != length; }
106-109: FixupdateShouldNotifyto compare data fields.The current implementation compares
childreferences, butBottomNavigationScopeexists to propagatecontrollerandlength. Dependent widgets won't rebuild when these data fields change if only the child reference is compared.Apply this diff:
@override bool updateShouldNotify(covariant InheritedWidget oldWidget) { - return oldWidget.child != child; + return oldWidget is BottomNavigationScope && + (oldWidget.controller != controller || oldWidget.length != length); }
♻️ Duplicate comments (4)
packages/stac_core/lib/foundation/borders/stac_border/stac_border.dart (1)
126-170: The horizontal/vertical semantics now match Flutter conventions.The current implementation correctly assigns:
vertical→ top/bottom (lines 165-166)horizontal→ left/right (lines 167-168)This aligns with Flutter's convention where "vertical" refers to the vertical axis (top/bottom) and "horizontal" refers to the horizontal axis (left/right). The documentation at lines 128-129 also correctly describes this mapping.
packages/stac_core/lib/foundation/colors/stac_color/stac_colors.dart (1)
35-47: Opacity suffix handling is already correct.The past review raised a concern about double opacity suffixes (e.g.,
"primary@80@50"). The current implementation already addresses this by usingsplit('@').firston line 43 to extract the base color before appending the new suffix, preventing any accumulation of@<percentage>strings.packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart (2)
119-120: Add constructor input validation.The constructor should validate that
lengthis non-negative andinitialIndexis within valid range before initializing_index. Without these checks, the controller can be constructed in an invalid state.This issue was previously flagged and remains unaddressed. Based on the past review suggestion:
BottomNavigationController({this.initialIndex = 0, required this.length}) - : _index = initialIndex; + : assert(length >= 0, 'length must be >= 0'), + assert(initialIndex >= 0, 'initialIndex must be >= 0'), + assert(length == 0 || initialIndex < length, + 'initialIndex must be < length when length > 0'), + _index = initialIndex;
119-120: Add constructor parameter validation.The constructor accepts
initialIndexandlengthwithout validation, which can lead to invalid initial state. For example,BottomNavigationController(length: 3, initialIndex: 5)will create a controller with_index = 5, violating the bounds that_changeIndexenforces.Apply this diff to add validation:
BottomNavigationController({this.initialIndex = 0, required this.length}) - : _index = initialIndex; + : assert(length >= 0, 'length must be >= 0'), + assert(initialIndex >= 0, 'initialIndex must be >= 0'), + assert(length == 0 || initialIndex < length, + 'initialIndex must be < length when length > 0'), + _index = initialIndex;
🧹 Nitpick comments (3)
packages/stac_core/lib/actions/set_value/stac_set_value_action.g.dart (1)
14-16: LGTM! Consider verifying JSON structure in upstream validation.The generated
fromJsonlogic correctly deserializes theactionfield as aStacActionwhen non-null. The cast toMap<String, dynamic>assumes well-formed JSON input.If there's a risk of malformed JSON in production, consider adding validation in the upstream code before calling
fromJsonto ensure theactionfield is properly structured.packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart (2)
136-145: Consider clarifying length requirements.The
_changeIndexmethod allowslength == 0in the assert (line 137) but prevents navigation whenlength < 2(line 139). A length of 0 doesn't make practical sense for a navigation controller.Consider adding a constructor assertion to require
length >= 2for a functional navigation controller:BottomNavigationController({this.initialIndex = 0, required this.length}) : assert(length >= 0, 'length must be >= 0'), + assert(length >= 2, 'Navigation requires at least 2 items'), assert(initialIndex >= 0, 'initialIndex must be >= 0'), assert(initialIndex < length, 'initialIndex must be < length'), _index = initialIndex;
28-30: Consider adding trailing comma for Dart style consistency.Adding a trailing comma after constructor parameters enables better formatting and cleaner diffs when parameters are added.
Apply this diff:
const _DefaultBottomNavigationControllerWidget({ required this.model, -}); +});Based on past review comments.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
packages/stac/lib/src/parsers/actions/stac_set_value/stac_set_value_action_parser.dart(2 hunks)packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_bar/stac_bottom_navigation_bar_parser.dart(2 hunks)packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_view/stac_bottom_navigation_view_parser.dart(2 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.freezed.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart(0 hunks)packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller_parser.dart(5 hunks)packages/stac/lib/src/parsers/widgets/stac_list_view/stac_list_view_parser.dart(1 hunks)packages/stac/lib/src/parsers/widgets/widgets.dart(1 hunks)packages/stac_core/lib/actions/set_value/stac_set_value_action.dart(1 hunks)packages/stac_core/lib/actions/set_value/stac_set_value_action.g.dart(1 hunks)packages/stac_core/lib/foundation/borders/stac_border/stac_border.dart(1 hunks)packages/stac_core/lib/foundation/borders/stac_border_radius/stac_border_radius.dart(1 hunks)packages/stac_core/lib/foundation/colors/stac_color/stac_colors.dart(1 hunks)packages/stac_core/lib/widgets/column/stac_column.dart(2 hunks)packages/stac_core/lib/widgets/column/stac_column.g.dart(2 hunks)packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart(1 hunks)packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart(1 hunks)packages/stac_core/lib/widgets/list_view/stac_list_view.dart(2 hunks)packages/stac_core/lib/widgets/list_view/stac_list_view.g.dart(2 hunks)packages/stac_core/lib/widgets/widgets.dart(1 hunks)
💤 Files with no reviewable changes (3)
- packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart
- packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart
- packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/stac_default_bottom_navigation_controller.freezed.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/stac_core/lib/widgets/widgets.dart
- packages/stac_core/lib/widgets/column/stac_column.g.dart
- packages/stac_core/lib/actions/set_value/stac_set_value_action.dart
- packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart
- packages/stac_core/lib/widgets/column/stac_column.dart
🔇 Additional comments (19)
packages/stac_core/lib/foundation/borders/stac_border/stac_border.dart (1)
86-124: LGTM!The
allfactory constructor correctly applies uniform styling to all sides by passing the parameters to the main constructor's global properties.packages/stac_core/lib/foundation/borders/stac_border_radius/stac_border_radius.dart (4)
68-106: LGTM!The
onlyconstructor correctly delegates all parameters to the primary constructor, providing an explicit API for specifying individual corner radii.
108-140: LGTM!The
horizontalconstructor correctly applies theleftparameter to left-side corners (topLeft, bottomLeft) and therightparameter to right-side corners (topRight, bottomRight).
142-174: LGTM!The
verticalconstructor correctly applies thetopparameter to top corners (topLeft, topRight) and thebottomparameter to bottom corners (bottomLeft, bottomRight).
176-206: LGTM!The
circularconstructor correctly applies the same radius value to all four corners, creating a uniform circular border radius.packages/stac/lib/src/parsers/actions/stac_set_value/stac_set_value_action_parser.dart (1)
4-4: LGTM!The import addition is necessary to support the new
parsemethod call on line 26.packages/stac_core/lib/actions/set_value/stac_set_value_action.g.dart (1)
22-22: LGTM!The
toJsonlogic correctly uses the null-safe operator to serialize theactionfield, handling the nullable type appropriately.packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.g.dart (2)
9-24: LGTM!The generated JSON serialization code correctly handles type conversions, optional fields, and nested widget serialization with appropriate null safety.
1-24: LGTM! Generated JSON serialization code.The generated serialization correctly handles all fields including the optional
initialIndexand nestedchildwidget. Standard json_serializable output with proper type conversions.packages/stac/lib/src/parsers/widgets/stac_list_view/stac_list_view_parser.dart (2)
10-10: LGTM!The import refactor to use a more specific path (
widget_type.dart) instead of the broadstac_core.dartimproves maintainability and reduces unnecessary dependencies.
10-10: LGTM! Import specificity improved.Importing only the required
WidgetTypefrom the specific path improves clarity and reduces dependencies.packages/stac_core/lib/widgets/list_view/stac_list_view.g.dart (2)
42-44: LGTM!The generated serialization code for the new
itemTemplatefield correctly handles null safety and uses standardStacWidgetserialization patterns consistent with other optional widget fields in the codebase.Also applies to: 69-69
42-44: LGTM! Generated serialization for itemTemplate.The generated code correctly handles the optional
itemTemplatefield with proper null checks and nestedStacWidgetserialization.Also applies to: 69-69
packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_bar/stac_bottom_navigation_bar_parser.dart (3)
23-66: LGTM!The refactor to use an internal
_BottomNavigationBarWidgetproperly encapsulates rendering logic and correctly accesses theBottomNavigationScopeat build time (notinitState), which addresses timing concerns mentioned in the PR objectives. Null-safe controller access provides graceful degradation when the scope is absent.
23-26: LGTM! Parser delegates to internal widget.The refactor correctly separates parsing from rendering by delegating to
_BottomNavigationBarWidget, consistent with the broader parser-based architecture changes in this PR.
28-66: LGTM! Internal widget properly wired to BottomNavigationScope.The
_BottomNavigationBarWidgetcorrectly retrieves the controller fromBottomNavigationScopewith null-safe access patterns. The onTap handler updates the controller index, and currentIndex reads from it with a safe default.packages/stac/lib/src/parsers/widgets/stac_bottom_navigation_view/stac_bottom_navigation_view_parser.dart (2)
19-37: LGTM! Build-time deferral fixes timing issues.The refactor correctly addresses the BottomNavigationScope timing issue by deferring the
BottomNavigationScope.of(context)call to the widget's build method rather than during parsing. The implementation includes proper safeguards:
- Empty children check (line 32)
- Null-safe controller access with fallback (line 33)
- Index clamping to prevent out-of-bounds access (line 34)
- Graceful fallback to SizedBox when parsing fails (line 35)
This aligns with the PR's objective of fixing DSL post-order widget creation issues.
3-3: Confirm import:BottomNavigationScopeis defined instac_default_bottom_navigation_controller_parser.dart; the import is correct.packages/stac_core/lib/foundation/colors/stac_color/stac_colors.dart (1)
35-47: LGTM! The opacity suffix issue has been resolved.The implementation now correctly handles repeated
withOpacitycalls by stripping any existing opacity suffix (line 43:split('@').first) before appending the new percentage. This ensures stable results like"primary@80".withOpacity(0.5)→"primary@50"instead of the previous"primary@80@50".
| StacRegistry.instance.setValue(value['key'] as String, value['value']); | ||
| } | ||
| return Stac.onCallFromJson(model.action, context); | ||
| return model.action.parse(context); |
There was a problem hiding this comment.
🧩 Analysis chain
Verify null safety for the action invocation.
The change from Stac.onCallFromJson(model.action, context) to model.action.parse(context) introduces a potential null pointer exception. Since model.action is typed as StacAction? (nullable), calling .parse(context) directly will throw if action is null.
Consider applying this diff to handle the null case safely:
- return model.action.parse(context);
+ return model.action?.parse(context);Alternatively, verify whether model.action is guaranteed to be non-null in practice by running the following script:
🏁 Script executed:
#!/bin/bash
# Description: Search for StacSetValueAction usage to verify if action field can be null.
# Search for JSON examples or tests where StacSetValueAction is instantiated
rg -n -C3 --type=dart 'StacSetValueAction\('
# Search for fromJson calls that might pass null action
rg -n -C3 --type=dart "action.*:.*null" | rg -C3 "StacSetValueAction"Length of output: 4220
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return model.action.parse(context); | |
| return model.action?.parse(context); |
🤖 Prompt for AI Agents
packages/stac/lib/src/parsers/actions/stac_set_value/stac_set_value_action_parser.dart
around line 26: model.action is nullable so calling .parse(context)
unconditionally can cause an NPE; update this line to guard against null by
using a null-aware call that returns null when action is missing, and if the
method's return type is non-nullable instead throw a clear exception when
model.action is null so callers get an explicit error.
- Migrate StacDefaultBottomNavigationController from legacy Freezed model to new stac_models system - Move model definition from packages/stac to packages/stac_core/lib/widgets/ - Replace Freezed-generated code with JsonSerializable-based implementation - Update parser to use new StacWidget base class and parse method - Add comprehensive documentation with Dart and JSON examples - Remove legacy Freezed files (.freezed.dart) and update imports - Update widget exports in both packages This migration follows the established pattern of moving widget models to the stac_models package while maintaining backward compatibility. The new implementation provides better type safety and follows the current architecture standards. Files changed: - Deleted: packages/stac/lib/src/parsers/widgets/stac_default_bottom_navigation_controller/*.dart - Added: packages/stac_core/lib/widgets/default_bottom_navigation_controller/stac_default_bottom_navigation_controller.dart - Updated: parser implementation and widget exports
- Change action field type from Map<String, dynamic>? to StacAction? - Update JSON serialization to properly handle StacAction objects - Regenerate model files to reflect the type change - Remove trailing whitespace in actionType getter
- Add StacBorder.all() factory method for uniform borders on all sides - Add StacBorder.symmetric() factory method for horizontal/vertical symmetric borders - Include comprehensive documentation with Dart and JSON examples for both methods - Provide convenient constructors for common border patterns
- Add StacBorderRadius.only() factory for individual side styling - Add StacBorderRadius.horizontal() factory for top/bottom borders only - Add StacBorderRadius.vertical() factory for left/right borders only - Add StacBorderRadius.circular() factory for uniform circular borders - Include comprehensive documentation with Dart and JSON examples
…arsers - Wrap StacBottomNavigationBar in _BottomNavigationBarWidget to defer controller access - Wrap StacBottomNavigationView in _BottomNavigationViewWidget to defer controller access - Move BottomNavigationScope.of(context) calls from parse() to build() methods - Fix timing issue where InheritedWidget was accessed before being created The issue was that both navigation parsers were calling BottomNavigationScope.of(context) during the parsing phase, before the BottomNavigationScope was actually created in the widget tree by StacDefaultBottomNavigationController. This caused the controller to be null and prevented proper communication between the navigation bar and view. By wrapping the widgets and deferring the BottomNavigationScope access to build time, the InheritedWidget is guaranteed to be available when needed, ensuring proper bottom navigation functionality.
- Add StacColorExtension with withOpacity() method - Support opacity values from 0.0 to 1.0 with automatic clamping - Convert opacity to percentage format (@<opacity>) for consistency - Include comprehensive documentation with Dart and JSON examples - Enable easy opacity manipulation for both hex colors and theme colors
Potatomonsta
force-pushed
the
mn/DSL-bugfixes
branch
from
1b5b61f to
158e011
Compare
2 participants
Stac Framework Enhancements: Navigation, Borders, Colors & Architecture Improvements
Overview
This PR introduces significant enhancements to the Stac framework, focusing on improved navigation functionality, enhanced border and color systems, and architectural improvements. The changes span across multiple packages and include both new features and critical bug fixes.
New Features
Added
withOpacity()extension method toStacColorStacColors.primary.withOpacity(0.8)→"primary@80"Added factory constructors to
StacBorderStacBorder.all()- uniform borders on all sidesStacBorder.symmetric()- horizontal/vertical symmetric bordersAdded factory constructors to
StacBorderRadiusStacBorderRadius.only()- individual corner stylingStacBorderRadius.horizontal()- left/right symmetric cornersStacBorderRadius.vertical()- top/bottom symmetric cornersStacBorderRadius.circular()- uniform circular bordersMigrated
StacDefaultBottomNavigationControllerto stac_coreBug Fixes
Fixed
BottomNavigationScopetiming issuesInheritedWidgetaccess to build timeFixed
StacSetValueActiontype safetyactionfield fromMap<String, dynamic>?toStacAction?Added
itemTemplatesupport toStacColumnStacListViewAdded
itemTemplatesupport toStacListViewExamples
Color System Extension
Border Factory Methods
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes