Update codegen#417
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughIntroduces a Param model and migrates codegen to use it; refactors the code generator into staged emitters with debug mode; adds a requiresId flag to JsonWidget annotation; migrates theme encoder/decoder access to singleton instances; adds a radio_group builder; exposes InputDecorationThemeSchema; bumps package versions and SDK constraints to ^3.9.0. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Analyzer as Analyzer
participant CodeGen as Code Generator
participant Param as Param Model
participant Decoders as Decoders
Note over Analyzer,CodeGen: Parameter extraction → Param conversion → decode
Analyzer->>CodeGen: FormalParameterElement(s)
CodeGen->>Param: Param.fromFormalParameter(...)
Param-->>CodeGen: Param (displayName, typeName, typeNullable, defaultValueCode)
CodeGen->>Decoders: decode(ClassElement, Param)
Decoders-->>CodeGen: decode snippet / code fragment
CodeGen->>CodeGen: _generateModel / _generateClass / _prepareCode
CodeGen-->>FS: emit generated Dart file
sequenceDiagram
autonumber
participant Widget as Builder/Widget
participant ThemeInst as ThemeEncoder/ThemeDecoder.instance
Note over Widget,ThemeInst: Singleton theme access used throughout
Widget->>ThemeInst: ThemeDecoder.instance.decodeXxx(value)
ThemeInst-->>Widget: decoded value
Note right of Widget: Old static access (ThemeDecoder.decodeXxx) removed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/codegen/lib/src/decoder/decoders.dart (1)
246-252: Fix PreferredSizeWidget default normalization.Converting default value code with
.replaceAll('PreferredSizeWidget', 'JsonWidgetData').replaceAll('Widget', 'JsonWidgetData')corrupts strings such asconst <PreferredSizeWidget>[]: after the first replacement we haveconst <JsonWidgetData>[], and the second replacement then turns that intoconst <JsonJsonWidgetDataData>[], yielding invalid generated code. Please update the normalization logic to avoid reprocessing theWidgetsubstring once it’s already insideJsonWidgetData(e.g., by using a helper that applies regex replacements with a negative lookbehind).One way to fix this is to extract a helper:
+String _normalizeWidgetDefault(String value) { + return value + .replaceAllMapped(RegExp(r'PreferredSizeWidget(\?)?'), + (match) => 'JsonWidgetData${match[1] ?? ''}') + .replaceAllMapped(RegExp(r'(?<![A-Za-z])Widget(\?)?'), + (match) => 'JsonWidgetData${match[1] ?? ''}'); +} + String _widgetDecoder( Param element, String name, String funName, String? defaultValueCode, ) => ''' () { dynamic parsed = $funName(map['$name'], registry: registry,); - ${defaultValueCode == null ? '' : 'parsed ??= ${defaultValueCode.replaceAll('PreferredSizeWidget', 'JsonWidgetData').replaceAll('Widget', 'JsonWidgetData')};'} + ${defaultValueCode == null ? '' : 'parsed ??= ${_normalizeWidgetDefault(defaultValueCode)};'}
🧹 Nitpick comments (4)
packages/json_dynamic_widget/CHANGELOG.md (1)
1-7: Clarify whether widget replacement is a breaking change.This is a major version bump (11.0.0) that replaces Radio with an auto-generated version. Consider adding explicit migration guidance or a note indicating whether this is a breaking change and what impact existing implementations should expect. The PR summary mentions a "new radio group builder," which might warrant additional documentation in the changelog entry.
packages/json_dynamic_widget/lib/src/builders/json_scaffold_builder.dart (1)
34-42: Simplify the opacity calculation and improve variable naming.The variable name
floatingButtonVisibilityValueis misleading—it's actually calculating a scrim opacity adjustment factor, not floating action button visibility. Additionally, the multi-step calculation can be simplified for better clarity and maintainability.The current calculation:
extentRemaining = 0.3 * (1.0 - animation.value)floatingButtonVisibilityValue = extentRemaining * 0.3 * 10 = 0.9 * (1.0 - animation.value)opacity = max(0.1, 0.6 - 0.9 * (1.0 - animation.value))This simplifies to:
opacity = max(0.1, 0.9 * animation.value - 0.3)Apply this diff to simplify:
- final extentRemaining = - _kBottomSheetDominatesPercentage * (1.0 - animation.value); - final floatingButtonVisibilityValue = - extentRemaining * _kBottomSheetDominatesPercentage * 10; - - final double opacity = math.max( + final double opacity = math.max( _kMinBottomSheetScrimOpacity, - _kMaxBottomSheetScrimOpacity - floatingButtonVisibilityValue, + 0.9 * animation.value - 0.3, );Alternatively, if the intermediate calculations are intentional for readability, rename the variable:
final extentRemaining = _kBottomSheetDominatesPercentage * (1.0 - animation.value); - final floatingButtonVisibilityValue = + final opacityReduction = extentRemaining * _kBottomSheetDominatesPercentage * 10; final double opacity = math.max( _kMinBottomSheetScrimOpacity, - _kMaxBottomSheetScrimOpacity - floatingButtonVisibilityValue, + _kMaxBottomSheetScrimOpacity - opacityReduction, );packages/json_dynamic_widget/example/assets/pages/radio.json (1)
37-38: Guard against null when composing the status string.Because
+binds tighter than??, the current expression runs'Selected radio group value: ' + radioGroupValuefirst. IfradioGroupValueis evernull(for example after clearing the selection), theString+operator will throw before?? ''can run. Wrap the nullable operand so you always concatenate a string.Apply this diff:
- "text": "${'Selected radio group value: ' + radioGroupValue ?? ''}" + "text": "${'Selected radio group value: ' + (radioGroupValue ?? '')}"packages/json_dynamic_widget/lib/src/builders/json_radio_group_builder.dart (1)
53-82: Consider performance optimization for registry reads.The
buildmethod reads from the registry on every build (line 56-58), which could be inefficient if the widget rebuilds frequently. Consider caching the value in state and listening to registry changes instead.@override void initState() { super.initState(); _groupValue = widget.data.jsonWidgetRegistry.getValue( widget.data.jsonWidgetId, ) ?? widget.groupValue; // Listen to external changes widget.data.jsonWidgetRegistry.valueStream .where((event) => !event.isSelfTriggered && event.id == widget.data.jsonWidgetId ) .listen((event) { if (mounted) { setState(() { _groupValue = event.value; }); } }); } @override Widget build(BuildContext context) { return RadioGroup<dynamic>( groupValue: _groupValue, // Use cached state value onChanged: (value) { if (widget.onChanged != null) { widget.onChanged!(value); } _groupValue = value; widget.data.jsonWidgetRegistry.setValue( widget.data.jsonWidgetId, value, originator: widget.data.jsonWidgetId, ); if (mounted) { setState(() {}); } }, child: widget.child?.build( childBuilder: widget.childBuilder, context: context, registry: widget.data.jsonWidgetRegistry, ) ?? const SizedBox(), ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/json_dynamic_widget/example/macos/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (56)
packages/annotation/CHANGELOG.md(1 hunks)packages/annotation/lib/dynamic_widget_annotation.dart(5 hunks)packages/annotation/pubspec.yaml(2 hunks)packages/codegen/CHANGELOG.md(2 hunks)packages/codegen/lib/json_dynamic_widget_codegen.dart(1 hunks)packages/codegen/lib/src/builder/json_widget_library_builder.dart(19 hunks)packages/codegen/lib/src/builder/json_widget_registrar_builder.dart(4 hunks)packages/codegen/lib/src/decoder/decoders.dart(5 hunks)packages/codegen/lib/src/decoder/schema_decoders.dart(3 hunks)packages/codegen/lib/src/encoder/encoders.dart(1 hunks)packages/codegen/lib/src/extension/dart_type_extension.dart(0 hunks)packages/codegen/lib/src/model/param.dart(1 hunks)packages/codegen/pubspec.yaml(2 hunks)packages/json_dynamic_widget/CHANGELOG.md(1 hunks)packages/json_dynamic_widget/README.md(1 hunks)packages/json_dynamic_widget/example/.gitignore(1 hunks)packages/json_dynamic_widget/example/assets/pages/card.json(1 hunks)packages/json_dynamic_widget/example/assets/pages/checkbox.json(4 hunks)packages/json_dynamic_widget/example/assets/pages/cupertino_switch.yaml(1 hunks)packages/json_dynamic_widget/example/assets/pages/dynamic.json(2 hunks)packages/json_dynamic_widget/example/assets/pages/form.json(3 hunks)packages/json_dynamic_widget/example/assets/pages/null_value_passing.json(1 hunks)packages/json_dynamic_widget/example/assets/pages/radio.json(1 hunks)packages/json_dynamic_widget/example/assets/pages/switch.json(3 hunks)packages/json_dynamic_widget/example/assets/pages/theme.json(2 hunks)packages/json_dynamic_widget/example/lib/src/custom_schemas/dotted_border_schema.dart(1 hunks)packages/json_dynamic_widget/example/lib/src/custom_schemas/svg_schema.dart(1 hunks)packages/json_dynamic_widget/example/macos/Runner.xcodeproj/project.pbxproj(3 hunks)packages/json_dynamic_widget/example/macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme(1 hunks)packages/json_dynamic_widget/example/pubspec.yaml(2 hunks)packages/json_dynamic_widget/lib/builders.dart(1 hunks)packages/json_dynamic_widget/lib/json_dynamic_widget.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_animated_container_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_animated_theme_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_checkbox_builder.dart(2 hunks)packages/json_dynamic_widget/lib/src/builders/json_circular_progress_indicator_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_container_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_cupertino_switch_builder.dart(2 hunks)packages/json_dynamic_widget/lib/src/builders/json_decorated_box_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_dropdown_button_form_field_builder.dart(12 hunks)packages/json_dynamic_widget/lib/src/builders/json_layout_builder_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_linear_progress_indicator_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_measured_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_radio_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_radio_group_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_rich_text_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_scaffold_builder.dart(2 hunks)packages/json_dynamic_widget/lib/src/builders/json_semantics_builder.dart(1 hunks)packages/json_dynamic_widget/lib/src/builders/json_switch_builder.dart(5 hunks)packages/json_dynamic_widget/lib/src/builders/json_text_form_field_builder.dart(11 hunks)packages/json_dynamic_widget/lib/src/builders/json_widget_builder.dart(2 hunks)packages/json_dynamic_widget/lib/src/decoders/input_decoration_decoder.dart(4 hunks)packages/json_dynamic_widget/lib/src/models/deferred_json_widget_data.dart(2 hunks)packages/json_dynamic_widget/lib/src/models/json_widget_data.dart(4 hunks)packages/json_dynamic_widget/pubspec.yaml(3 hunks)pubspec.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- packages/codegen/lib/src/extension/dart_type_extension.dart
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
packages/json_dynamic_widget/CHANGELOG.md
5-5: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (60)
packages/json_dynamic_widget/example/.gitignore (1)
32-32: LGTM!Adding
Podfile.lockto the gitignore is a sensible addition that aligns with Flutter best practices. The iOS CocoaPods lockfile is auto-generated and environment-specific, so excluding it from version control prevents spurious diffs and merge conflicts.packages/json_dynamic_widget/example/assets/pages/form.json (1)
106-108: Icon size adjustments look good.The reduction of suffix icon sizes from 50 to 20 across all three form fields (first name, last name, email address) is consistent and reasonable for Material Design conventions. The
fontFamilyis properly preserved as "MaterialIcons" in all cases.Also applies to: 155-157, 199-201
packages/json_dynamic_widget/example/macos/Runner.xcodeproj/xcshareddata/xcschemes/Runner.xcscheme (1)
61-61: Verify scope: GPU validation mode seems unrelated to codegen updates.This Xcode scheme change (enabling GPU validation mode for the macOS example app's Debug configuration) appears tangential to the stated PR objectives about codegen updates, the new
Parammodel, and version bumps. While the change is harmless and valid, consider whether it should be in this PR or separated into a distinct, focused change.packages/json_dynamic_widget/example/assets/pages/null_value_passing.json (1)
20-20: ID addition is correct and necessary.The
text_form_fieldwidget builder explicitly requires an ID via@JsonWidget(requiresId: true)(packages/json_dynamic_widget/lib/src/builders/json_text_form_field_builder.dart). The addition of"id": "text_field"is therefore necessary and aligns with the PR's ID-based registration improvements.packages/json_dynamic_widget/example/assets/pages/card.json (1)
75-111: Widget type change approved; overflow_bar is properly supported.The
overflow_barwidget is registered and supported in json_dynamic_widget, with a dedicated builder implementation and test coverage. The JSON structure, styling, and callback configuration are all correct and consistent with framework patterns.packages/json_dynamic_widget/example/assets/pages/cupertino_switch.yaml (2)
62-77: Verify the bareexample_switchwidget is intentional.The
cupertino_switchat lines 62–63 has noargsproperties (label, value, enabled), unlike the other switches in the file (lines 24–27, 37–40, 50–52). The conditional widget then references this switch's state at line 68.Confirm this is intentional (e.g., a minimal example showing id-based referencing) rather than an incomplete definition.
2-77: YAML structure and syntax look good.The indentation is consistent, the widget IDs are unique and well-formed, and the conditional logic is sound. The changes align well with the PR objectives of introducing id-based wiring and conditional rendering for dynamic widgets.
packages/json_dynamic_widget/lib/src/builders/json_rich_text_builder.dart (1)
12-12: LGTM! Clean migration to singleton pattern.The change from static method access to singleton instance access is straightforward and aligns with the broader project-wide migration mentioned in the PR summary.
packages/json_dynamic_widget/lib/src/decoders/input_decoration_decoder.dart (1)
76-194: LGTM! Singleton migration applied consistently.All
ThemeDecodercalls have been successfully migrated from static methods to the singleton instance pattern (ThemeDecoder.instance.decodeX). The changes are mechanical, consistent, and preserve all existing parameters and logic.packages/json_dynamic_widget/example/macos/Runner.xcodeproj/project.pbxproj (1)
410-410: LGTM! Deployment target bump is consistent.The macOS deployment target has been updated to 10.15 across all project-level build configurations (Profile, Debug, Release), which aligns with the broader SDK updates in this PR.
Note: The target-level configurations specify 11.5 (lines 436, 567, 592), which will be the effective deployment target since target settings override project settings.
Also applies to: 494-494, 541-541
packages/json_dynamic_widget/lib/src/builders/json_semantics_builder.dart (2)
18-20: Based on my verification:ThemeDecoder provides functionality to decode theme related objects from JSON maps, and the usage pattern
ThemeDecoder.instance.decodeAppBarThemewithvalidate: falseis established in the json_theme package documentation, which is a dependency of json_dynamic_widget.The change in
_decodeSortKeycorrectly migrates from static access to the singleton pattern usingThemeDecoder.instancewhile preserving thevalidate: falseparameter. The Singleton pattern in Dart ensures a class has only one instance and provides a global point of access to it, helping eliminate redundant code. This method is now consistent with other builder methods in the codebase that already useThemeDecoder.instancepattern.
13-16: Migration to singleton instance is verified and correct.
ThemeEncoder.instanceandThemeDecoder.instanceare external singletons provided by thejson_theme(v10.0.0) package and are already properly initialized by that dependency. The refactoring at lines 15 and 20 injson_semantics_builder.dartfollows the established pattern used consistently throughout the codebase.packages/json_dynamic_widget/example/lib/src/custom_schemas/svg_schema.dart (1)
1-1: No action required—import is correct and already standard in the codebase.The import path
package:json_theme/json_theme.dartis already the established pattern across the codebase. Multiple existing files (input_decoration_decoder.dart, json_dropdown_button_form_field_builder.dart, json_text_form_field_builder.dart, issue_219_page.dart, and others) successfully import from this path and useSchemaHelperandColorSchemawithout issues. The change in svg_schema.dart aligns it with this existing standard, not introducing a new or risky import.packages/json_dynamic_widget/lib/src/builders/json_scaffold_builder.dart (2)
9-11: LGTM!The constants are well-named and the values are reasonable for configuring bottom sheet scrim behavior.
44-47: Verify that non-dismissible scrim is intentional.The
ModalBarrierhasdismissible: false, which prevents users from dismissing the bottom sheet by tapping the scrim. This differs from typical bottom sheet behavior where tapping outside dismisses the sheet. Please confirm this is the intended UX.packages/json_dynamic_widget/example/lib/src/custom_schemas/dotted_border_schema.dart (1)
1-1: The import path change is correct and aligns with the documented json_theme pattern.The documented import pattern for json_theme is
import 'package:json_theme/json_theme.dart', which matches the change made in this file. TheSchemaHelperandColorSchemasymbols used in the file (line 14) are correctly accessed from this import path. The change is approved and requires no further modifications.pubspec.yaml (1)
5-5: SDK constraint updated to ^3.9.0.The Dart SDK constraint has been raised from ^3.7.0 to ^3.9.0, which aligns with the broader toolchain upgrades across the workspace packages.
Verify that all consumers of this workspace can support Dart SDK ^3.9.0 and that the removal of the example package from the workspace is intentional and won't break any existing build/test pipelines.
packages/json_dynamic_widget/lib/src/builders/json_linear_progress_indicator_builder.dart (1)
11-17: LGTM! Migrated to singleton pattern.The update correctly switches from static
ThemeDecoder.decodeColortoThemeDecoder.instance.decodeColor, maintaining the same decode logic while adopting the singleton pattern used throughout this PR.packages/json_dynamic_widget/README.md (1)
131-154: LGTM! Documentation updated to reflect instance method usage.The README correctly updates the pre-7.0.0 example code to use
ThemeDecoder.instancemethods instead of static methods, maintaining consistency with the singleton pattern adopted throughout this PR.packages/json_dynamic_widget/example/pubspec.yaml (2)
7-7: LGTM! Dependency versions updated.The SDK constraint and dependencies have been appropriately updated:
- Dart SDK: ^3.9.0 (consistent with workspace)
- json_theme: ^10.0.0 (major version bump)
- build_runner: ^2.10.1
- json_dynamic_widget_codegen: ^4.0.0 (major version bump aligning with this PR)
Also applies to: 20-20, 25-25, 30-30
33-37: Dependency overrides added for local development.The addition of
dependency_overrideswith path references to local annotation and codegen packages is appropriate for development and testing within the workspace. Note that these overrides should not be present when publishing packages.packages/json_dynamic_widget/lib/src/builders/json_decorated_box_builder.dart (1)
11-17: LGTM! Migrated to singleton pattern.Both decoder methods correctly use
ThemeDecoder.instanceinstead of static calls, maintaining the same decode logic while adopting the consistent singleton pattern used throughout this PR.packages/json_dynamic_widget/lib/json_dynamic_widget.dart (1)
13-13: Verify intentional exposure of InputDecorationThemeSchema.The
InputDecorationThemeSchemais no longer hidden from thejson_themeexport, making it publicly accessible throughjson_dynamic_widget. This is a public API change that expands the library's surface area.Please confirm:
- This exposure is intentional and aligns with the PR objectives
- The change is documented in the changelog
- There are no naming conflicts or unintended side effects from exposing this schema
If this change is part of the codegen refactor mentioned in the AI summary, consider documenting the reason for exposing this schema in the changelog or migration guide.
packages/json_dynamic_widget/example/assets/pages/dynamic.json (1)
126-131: The JSON structure described in this review comment does not match the actual file state.The review claims the
sizeproperty was moved to be a sibling oficonat the same level. However, the actual file showssizeis nested inside theargsobject:"icon": { "type": "icon", "args": { "icon": { "codePoint": 61912, "fontFamily": "MaterialIcons" }, "size": 50 } }This is the Param-based parameter model using the type/args wrapper pattern. The file is already in this state, and the review's "After structure" example with
sizeas a direct sibling toicondoes not reflect the actual implementation.The icon builder code (
json_icon_builder.dart) uses@jsonWidgetcode-generation annotations consistent with this structure, and the CHANGELOG contains no recent entries documenting a specific change to thesizeproperty position.Likely an incorrect or invalid review comment.
packages/json_dynamic_widget/example/assets/pages/switch.json (1)
30-30: LGTM! ID attributes added consistently.The addition of
idattributes to the switch widgets is clean and follows a consistent naming pattern that clearly identifies each switch's purpose.Also applies to: 52-52, 74-74
packages/json_dynamic_widget/example/assets/pages/theme.json (2)
88-89: LGTM! Checkbox ID added.The addition of the
idattribute to the checkbox widget is consistent with the PR's objective to support widget identification.
98-124: Verify the radio button value design.The new
radio_groupstructure uses boolean values (true/false) for the two radio buttons. Typically, radio buttons should have distinct, meaningful values that represent different options rather than boolean states. Using booleans may limit the semantics and make it unclear what each radio represents.Consider whether these values should be more descriptive (e.g., "option1"/"option2" or specific feature flags) to better convey their purpose.
packages/json_dynamic_widget/lib/src/builders/json_container_builder.dart (2)
18-21: LGTM! Singleton pattern migration for encoders.The migration from static method calls to singleton instance access for
ThemeEncoderis consistent with the broader refactoring pattern in this PR.Also applies to: 24-27
30-31: LGTM! Singleton pattern migration for decoders.The migration from static method calls to singleton instance access for
ThemeDecoderaligns with the encoder changes and maintains consistency.Also applies to: 34-35
packages/json_dynamic_widget/pubspec.yaml (3)
4-4: Major version bump to 11.0.0.This major version bump signals breaking changes. Ensure that all breaking changes introduced in this PR are documented in the CHANGELOG and that migration guidance is provided for users upgrading from 10.x.
Do you want me to check if the CHANGELOG has been updated with breaking changes and migration notes?
44-45: Test API override noted.The
test_api: '^0.7.6'dependency override may indicate a compatibility workaround. Ensure this override is still necessary and doesn't mask underlying dependency conflicts.
7-7: Verify dependency requirements for SDK 3.9.0 constraint.Codebase analysis reveals no Dart 3.9.0-specific language features in use (no extension types, sealed classes, augmentation syntax, etc.). The codebase uses only Dart 3.0+ features like super parameters. Dart 3.9.0's notable changes include stricter null safety type promotion, AOT-compiled CLI snapshots, pub workspace handling improvements, and cross-compilation targets—none of which are evident as direct requirements in the code.
The SDK constraint bump to
^3.9.0may be driven by the coordinated major version updates tojson_themeandjson_dynamic_widget_codegen, or by workspace pub changes. Verify that the dependent packages actually require Dart 3.9.0, or if the constraint could be relaxed to^3.8.0or lower to maintain broader compatibility.packages/annotation/pubspec.yaml (2)
4-4: Verify the patch version bump is appropriate.This package is bumped to 3.0.1 (patch), while the main
json_dynamic_widgetpackage bumps to 11.0.0 (major) andjson_dynamic_widget_codegenbumps to 4.0.0 (major). Given the PR introduces a newrequiresIdfield to the annotation according to the AI summary, verify whether this should be a minor version bump (3.1.0) rather than a patch, as it adds new functionality.
7-7: LGTM! SDK and test dependency updates.The SDK constraint increase to
^3.9.0and test version update to^1.26.3maintain consistency with other packages in the workspace.Also applies to: 20-20
packages/json_dynamic_widget/lib/src/builders/json_cupertino_switch_builder.dart (2)
10-10: LGTM! ID requirement added.The change from
@jsonWidgetto@JsonWidget(requiresId: true)enforces that this widget must have an ID, which aligns with the PR's objective to standardize ID usage for stateful form widgets like switches.
89-137: LGTM! Formatting improvements.The indentation and formatting adjustments improve readability without changing the validation or builder logic.
packages/codegen/pubspec.yaml (2)
4-4: LGTM! Major version bump with SDK update.The major version bump to 4.0.0 and SDK constraint increase to
^3.9.0are appropriate for the significant codegen refactoring (Param model, singleton patterns) described in the PR.Also applies to: 7-7
16-17: LGTM! Codegen toolchain updates.The major version updates to
analyzer(^8.4.0),build(^4.0.0),json_theme(^10.0.0), andsource_gen(^4.0.2) provide the necessary tooling support for the enhanced code generation capabilities.Also applies to: 21-23
packages/json_dynamic_widget/lib/src/builders/json_dropdown_button_form_field_builder.dart (5)
65-65: LGTM! API expanded with new fields.The addition of
barrierDismissable,errorBuilder,forceErrorText,initialValue, andpaddingfields enhances the dropdown form field's capabilities, aligning with Flutter'sDropdownButtonFormFieldAPI.Also applies to: 73-73, 76-76, 82-82, 91-91, 102-102, 110-110, 113-113, 119-119, 128-128
280-349: LGTM! Singleton pattern migration with backward compatibility.The migration to
ThemeDecoder.instanceis consistent with the PR's refactoring pattern. Importantly, line 329 provides backward compatibility by falling back tomap['value']ifmap['initialValue']is not present, which helps with migration from the old API.
357-391: LGTM! Encoder updates with new field serialization.The
toJsonmethod correctly usesThemeEncoder.instanceand includes all new fields in the serialization output.
584-657: LGTM! Widget build updated with new properties.The widget build correctly passes all new properties (
barrierDismissible,errorBuilder,forceErrorText,initialValue,padding) to theDropdownButtonFormField, and the state management has been appropriately simplified.
119-119: Breaking change: Verify migration documentation.The
valuefield has been replaced withinitialValue. While line 329 provides backward compatibility by falling back tomap['value'], this is still a breaking change in the public API. Ensure this change is documented in the CHANGELOG with clear migration instructions.Do you want me to check if the CHANGELOG documents this breaking change?
Also applies to: 329-329
packages/json_dynamic_widget/lib/src/builders/json_widget_builder.dart (1)
165-167: Avoids self-originated rebuild loopsThe additional originator check keeps widgets from thrashing when they write back into the registry and immediately observe their own event. Nice catch.
packages/json_dynamic_widget/lib/src/models/json_widget_data.dart (3)
9-20: LGTM! Clean implementation of ID provision tracking.The
hasProvidedIdfield is properly initialized based on whether an ID was explicitly provided or defaulted. The logic correctly distinguishes between user-provided IDs and auto-generated UUIDs.
224-242: Verify that preservinghasProvidedIdincopyWithis intentional.The
copyWithmethod preserves the originalhasProvidedIdvalue even when a differentjsonWidgetIdmight be provided. This means the flag tracks the original provision state rather than the current ID state. If this is intentional (to maintain the original "was an ID provided" semantics), this is correct. However, ifcopyWithwith a new ID should updatehasProvidedId, this would need adjustment.Consider whether
copyWithshould recalculatehasProvidedIdwhenjsonWidgetIdis explicitly provided:JsonWidgetData copyWith({ dynamic jsonWidgetArgs, JsonWidgetBuilder? jsonWidgetBuilder, Set<String>? jsonWidgetListenVariables, String? jsonWidgetId, JsonWidgetRegistry? jsonWidgetRegistry, String? jsonWidgetType, }) => JsonWidgetData( hasProvidedId: jsonWidgetId != null ? true : hasProvidedId, // ... rest of parameters );
100-104: LGTM! Clean timer initialization.The inline timer construction with
.start()is more concise than the previous two-step approach.packages/json_dynamic_widget/lib/src/builders/json_switch_builder.dart (3)
8-8: LGTM! Annotation updated to require ID.The change from
@jsonWidgetto@JsonWidget(requiresId: true)aligns with the PR's goal of requiring IDs for widgets that need state management through the registry.
140-187: LGTM! Widget properties correctly wired to Switch.All the new properties (
activeThumbColor,onFocusChanged,trackOutlineColor,trackOutlineWidth) are correctly passed through to the underlying FlutterSwitchwidget, and the state management logic remains intact.
28-62: ****The review comment incorrectly attributes the
activeColorparameter to the_Switchclass. The Switch builder injson_switch_builder.dartusesactiveThumbColor(notactiveColor). The parameteractiveColorexists in other builders—json_checkbox_builder.dartandjson_cupertino_switch_builder.dart—but those are different widgets with their own API surfaces. No breaking change to the Switch builder regarding this parameter rename has been identified.Likely an incorrect or invalid review comment.
packages/json_dynamic_widget/lib/src/builders/json_radio_group_builder.dart (1)
1-17: LGTM! New radio group builder with required ID.The builder is properly annotated with
@JsonWidget(requiresId: true)and follows the established pattern for JSON widget builders.packages/json_dynamic_widget/lib/src/builders/json_text_form_field_builder.dart (3)
9-9: LGTM! Annotation updated to require ID.The builder now correctly requires an ID, which is essential for text form field state management through the registry.
270-368: LGTM! All new parameters correctly propagated.All the new parameters are properly passed through to the underlying
TextFormFieldwidget, maintaining consistency with Flutter's API.
56-133:groupIdparameter is correctly configured.The groupId parameter is of type Object and its default value is EditableText. The code at line 84 (
groupId = EditableText) correctly matches Flutter's TextFormField API specification. The other new parameters also appropriately extend the widget's capabilities.packages/annotation/lib/dynamic_widget_annotation.dart (2)
5-5: LGTM! Backward-compatible default forrequiresId.The
jsonWidgetconstant is updated to explicitly includerequiresId: false, maintaining backward compatibility for existing widgets.
130-158: LGTM! Well-documented new annotation field.The
requiresIdfield is properly documented and added to theJsonWidgetannotation with a sensible default offalse, ensuring backward compatibility while enabling new functionality for widgets that require IDs.packages/codegen/lib/src/model/param.dart (2)
1-36: LGTM! Clean parameter model implementation.The
Paramclass provides a clean, immutable representation of parameter metadata. ThefromFormalParameterfactory appropriately extracts all necessary information from the analyzer'sFormalParameterElement.
16-27: Verify null-safety ofparam.nameaccess.Line 22 uses
param.name!with a force unwrap. WhileFormalParameterElement.nameshould always be non-null for valid formal parameters, consider adding a null check or assertion for defensive programming.factory Param.fromFormalParameter( FormalParameterElement param, { required TypeChecker builderParamChecker, }) { assert(param.name != null, 'Parameter name cannot be null'); return Param( custom: false, defaultValueCode: param.defaultValueCode, displayName: param.name!, isBuilderParam: builderParamChecker.annotationsOf(param).isNotEmpty, isRequired: param.isRequired, typeName: param.type.getDisplayString(), typeNullable: param.type.nullabilitySuffix == NullabilitySuffix.question, ); }packages/codegen/lib/src/decoder/schema_decoders.dart (1)
1-43: LGTM! Clean migration to Param model.The schema decoders have been successfully migrated from
ParameterElementto the newParammodel. All decoder signatures are consistently updated, and the addition ofList<int>schema decoder (line 23) fills a previous gap in type coverage.packages/codegen/lib/src/encoder/encoders.dart (1)
1-72: LGTM! Successful migration to Param model.The encoders have been cleanly migrated from
ParameterElementto the newParammodel. The signature changes are consistent throughout, and the logic for handling nullability, type names, and default values has been correctly adapted to useParam's properties (displayName,typeNullable,typeName).
1 participant
Summary by CodeRabbit
New Features
Compatibility
Improvements
Other