[DG-266] adding the callback support to the create component fn - #486
[DG-266] adding the callback support to the create component fn#486carbonFibreCode wants to merge 3 commits into
Conversation
WalkthroughAdds native Dart callback support via a new Changes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (4)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/framework/component/component.dart (1)
1-1: Address Dart formatting failure.The pipeline indicates formatting issues. Run
dart format .to fix.Based on pipeline failure logs.
🧹 Nitpick comments (1)
lib/src/framework/widgets/navigation_bar_custom.dart (1)
29-42: Consider usinggetActionOrCallbackfor consistency with other widgets.Unlike
button.dartandcalendar.dartwhich usepayload.getActionOrCallback(...)to allow developer callbacks to override actions viacreateComponentargs, this widget passes the action directly from props. If callback support should also be available for navigation item selection, consider aligning with the pattern:final action = payload.getActionOrCallback( 'onPageSelected', onPageSelectedAction);If this is intentional (navigation items define their own actions and shouldn't be overridable), this is fine as-is.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
lib/src/framework/component/component.dartlib/src/framework/render_payload.dartlib/src/framework/widgets/animated_button.dartlib/src/framework/widgets/button.dartlib/src/framework/widgets/calendar.dartlib/src/framework/widgets/icon_button.dartlib/src/framework/widgets/navigation_bar.dartlib/src/framework/widgets/navigation_bar_custom.dartlib/src/framework/widgets/page_view.dartlib/src/framework/widgets/refresh_indicator.dartlib/src/framework/widgets/rich_text.dartlib/src/framework/widgets/text_form_field.dart
🧰 Additional context used
🪛 GitHub Actions: Pull Request Checks
lib/src/framework/component/component.dart
[error] 1-1: Dart format check failed. The command 'dart format --output=none --set-exit-if-changed .' reported changes and exited with code 1; run 'dart format' to fix the formatting issues.
🔇 Additional comments (13)
lib/src/framework/component/component.dart (1)
48-57: Two-stage argument resolution enables callback support.The implementation correctly merges default values from
argDefswith developer-providedargs. This allows callbacks and other runtime arguments to be passed even when not explicitly defined inargDefs, which aligns with the PR objective.The spread order ensures developer-provided args override defined args as intended.
lib/src/framework/widgets/refresh_indicator.dart (1)
29-34: LGTM: Action resolution updated to new pattern.The change from
ActionFlow.fromJsontopayload.getActionOrCallbacksimplifies action resolution and aligns with the framework-wide refactor to support callbacks.lib/src/framework/widgets/page_view.dart (1)
37-38: LGTM: Consistent action resolution pattern.The
onPageChangedhandler now usespayload.getActionOrCallback, consistent with the refactor across other widgets.lib/src/framework/widgets/rich_text.dart (1)
86-93: LGTM: Simplified onClick execution.Passing
p0directly toexecuteActioninstead of wrapping it inActionFlow.fromJsonsimplifies the code and aligns with the new callback support mechanism.lib/src/framework/widgets/icon_button.dart (1)
64-68: LGTM: Action resolution updated consistently.The
onClickhandler now usespayload.getActionOrCallback, consistent with the framework-wide refactor.lib/src/framework/widgets/animated_button.dart (1)
67-74: LGTM: Consistent onClick resolution.The animated button now uses
payload.getActionOrCallbackfor action resolution, maintaining consistency with other interactive widgets.lib/src/framework/widgets/navigation_bar.dart (1)
27-40: LGTM: Simplified action execution in navigation handler.Passing
onPageSelectedActiondirectly toexecuteActioninstead of wrapping it inActionFlow.fromJsonaligns with the new callback support approach.lib/src/framework/widgets/text_form_field.dart (1)
80-93: LGTM: Consistent action resolution for form field events.Both
onChangedandonSubmithandlers now usepayload.getActionOrCallback, maintaining consistency with the framework-wide refactor to support callbacks.lib/src/framework/widgets/button.dart (1)
84-88: LGTM!The button correctly adopts the new
getActionOrCallbackpattern, enabling developers to pass callbacks viacreateComponentargs while maintaining fallback to props-defined actions.lib/src/framework/widgets/calendar.dart (1)
76-79: LGTM!Both
onDateSelectedandonRangeSelectedcorrectly use the newgetActionOrCallbackpattern, allowing developers to inject custom callbacks while preserving fallback to dashboard-defined actions.lib/src/framework/render_payload.dart (3)
72-90: LGTM!The
getActionOrCallbackmethod provides a clean mechanism for prioritizing developer-provided callbacks from scope over props-defined actions. The documentation is clear and helpful.
106-113: Function callbacks are invoked with zero arguments;scopeContextandtriggerTypeare ignored.When
actionOrCallbackis aFunction, it's invoked asactionOrCallback()without any parameters. This means:
- Developer callbacks must be zero-argument functions
- The
scopeContextandtriggerTypeparameters passed toexecuteActionare silently ignored for Function callbacksIf developers need access to context data within their callbacks, this design won't support that. Consider either:
- Documenting this limitation clearly in the method's docstring
- Passing context to callbacks if supported signatures are defined (e.g.,
Function(ScopeContext?))If zero-arg callbacks are the intended design, adding a note to the docstring would help prevent confusion.
115-120: No action needed —ActionFlow.fromJsonalready handles invalid inputs gracefully.
ActionFlow.fromJsonacceptsObject?and explicitly checksif (json is! JsonLike) return null;at line 21, returningnullfor any non-JsonLikeinput (primitives, unexpected types) without throwing exceptions. The implementation already satisfies the safety concern.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/src/framework/component/component.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-format-and-analyse
🔇 Additional comments (3)
lib/src/framework/component/component.dart (3)
63-63: LGTM: Consistent argument propagation.The resolved arguments are correctly passed to the scope context for state initialization.
72-72: LGTM: Proper observability integration.The resolved arguments are correctly provided to the state observer for inspection.
97-97: LGTM: Consistent context creation.The resolved arguments are correctly passed to the expression context, ensuring all parameters are available for expression evaluation.
bb608c6 to
c759d15
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/framework/actions/execute_callback/processor.dart (1)
1-1: Fix Dart formatting issues.The pipeline indicates formatting failures. Run
dart format .to fix formatting issues in this file.
🧹 Nitpick comments (2)
lib/src/framework/actions/execute_callback/processor.dart (1)
189-201: Defensive null check is redundant but acceptable.The
has()check at line 75 already confirms the callback exists before calling_executeNativeCallback. This null check will never trigger in normal execution flow since both checks occur synchronously. Consider simplifying:🔎 Optional simplification
final callback = callbackRegistry.get(callbackName); - - if (callback == null) { - executionContext?.notifyComplete( - id: id, - parentActionId: parentActionId, - descriptor: actionDescriptor, - error: 'Native callback "$callbackName" not found in registry', - stackTrace: StackTrace.current, - observabilityContext: observabilityContext, - ); - return null; - } + // callback is guaranteed non-null since has() was checked before calling this methodlib/src/framework/callback/callback_registry.dart (1)
96-100: Consider throwing a descriptive error instead of assert + force-unwrap.The
assertis stripped in release builds, so ifof()is called without aCallbackProviderancestor, users will see a cryptic null dereference error rather than the helpful message. SincemaybeOfis used in the processor, this is low-risk, but for API robustness:🔎 Optional improvement
static CallbackRegistry of(BuildContext context) { final registry = maybeOf(context); - assert(registry != null, 'No CallbackProvider found in widget tree'); - return registry!; + if (registry == null) { + throw FlutterError('No CallbackProvider found in widget tree'); + } + return registry; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
lib/digia_ui.dartlib/src/framework/actions/execute_callback/processor.dartlib/src/framework/callback/callback_registry.dartlib/src/framework/ui_factory.dart
🧰 Additional context used
🪛 GitHub Actions: Pull Request Checks
lib/src/framework/actions/execute_callback/processor.dart
[error] 1-1: Dart format check failed. The formatter changed the file(s); run 'dart format --output=none --set-exit-if-changed .' to reformat.
lib/src/framework/callback/callback_registry.dart
[error] 1-1: Dart format check failed. The formatter changed the file(s); run 'dart format --output=none --set-exit-if-changed .' to reformat.
🔇 Additional comments (11)
lib/digia_ui.dart (1)
11-11: LGTM!The export is correctly placed in alphabetical order and properly exposes the new callback infrastructure (
DUICallback,CallbackRegistry,CallbackProvider) to library consumers.lib/src/framework/actions/execute_callback/processor.dart (2)
70-87: LGTM!The native callback routing logic is well-structured with proper null safety checks and clean early-return pattern.
203-228: LGTM!The callback execution follows the established pattern with proper error handling, observability notifications, and consistent rethrow behavior matching the existing ActionFlow execution path.
lib/src/framework/ui_factory.dart (5)
11-11: LGTM!Import correctly added for the new callback infrastructure.
572-586: LGTM!Clear documentation with a practical example showing how to pass callbacks and their expected signature.
611-623: LGTM!The callback name injection pattern is elegant—when an expression like
args.onButtonClickis evaluated, it returns the string"onButtonClick", which the processor then matches against the registry.
626-642: LGTM!Component correctly created with merged args that include callback name mappings.
654-663: LGTM!The wrapping order is correct—
CallbackProvideris placed aboveDefaultActionExecutorin the widget tree, ensuring the registry is accessible viacontext.getInheritedWidgetOfExactTypefrom within the component's build context.lib/src/framework/callback/callback_registry.dart (3)
3-11: LGTM!The typedef signature is flexible, allowing callbacks to return
nullsynchronously or an asyncFuture<Object?>, which the processor handles correctly withawait.
31-68: LGTM!Clean API with defensive copy in constructor. The registry provides all necessary operations for callback management.
102-109: LGTM!The
updateShouldNotifyreturningfalseis appropriate since the processor usesgetInheritedWidgetOfExactType(no subscription) and callbacks are accessed during action execution rather than during widget builds. Good documentation of the design decision.
1 participant
No description provided.