feat: Migration to capability-based API pt1 #50
Conversation
WalkthroughAdds three new capability mixins (focus, server-action execution, UI controller), updates UIDriver to compose them and trims its old controller/action APIs, marks several action_api types as deprecated, adjusts exports, and adds/updates tests and test utilities. Changes
Sequence Diagram(s)Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Pre-merge checks✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (3)
🔇 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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
lib/src/action_api/event_resolver.dart (3)
41-43: Async callback inforEachwon't be awaited.Using
forEachwith an async callback doesn't guarantee sequential execution or that all updates complete before continuing. Each iteration fires off an unawaited future.🔎 Proposed fix
- event.updates.forEach((key, value) async { - await driver.updateAttributes(key, value); - }); + for (final entry in event.updates.entries) { + await driver.updateAttributes(entry.key, entry.value); + }
46-58: Inverted null-check logic throws when handler IS set.The condition on line 50 throws
StateErrorwhenexternalEventHandler != null, but the intent appears to be the opposite—throw when it's missing. The assert on line 46 also contradicts the subsequent check.🔎 Proposed fix
case NavigationEvent(): - assert( - driver.externalEventHandler != null, - "ExternalEventHandler instance is not set", - ); - if (driver.externalEventHandler != null) { + if (driver.externalEventHandler == null) { logger?.error("ExternalEventHandler instance is not set"); throw StateError("ExternalEventHandler instance is not set"); } - await driver.externalEventHandler?.handleNavigation( + await driver.externalEventHandler!.handleNavigation( context, event.path, event.extra, ); break;
60-65: Same inverted null-check logic for OpenUrlEvent.Line 61 throws when the handler exists rather than when it's missing.
🔎 Proposed fix
case OpenUrlEvent(): - if (driver.externalEventHandler != null) { + if (driver.externalEventHandler == null) { logger?.error("ExternalEventHandler instance is not set"); throw StateError("ExternalEventHandler instance is not set"); } - await driver.externalEventHandler?.handleOpenUrl(event.url); + await driver.externalEventHandler!.handleOpenUrl(event.url); break;lib/src/action_api/action_executor.dart (1)
72-82: Unnecessary try-catch around property access.The
action.eventaccess on line 74 is a simple property getter that won't throw, making this try-catch block dead code. Given this class is deprecated, this is low priority.🔎 Simplified implementation
//local execution case LocalAction(): - try { - return action.event; - } catch (e, s) { - logger?.error( - "[Error while executing local action]", - error: e, - stackTrace: s, - ); - } - break; + return action.event;lib/src/driver_api/ui_driver.dart (1)
26-27: Remove commented-out code.Dead code in comments adds noise and confusion. If
eventStreamControlleris no longer needed, remove these lines entirely. If it's planned for future use, track it via an issue instead.🔎 Proposed fix
/// The build context associated with the UI driver. abstract BuildContext buildContext; - /// The stream controller for the UI driver. - // @protected - // abstract StreamController<UIDriverEvent> eventStreamController; - /// The script runner used by the UI driver.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (10)
lib/src/action_api/action_executor.dartlib/src/action_api/event_parser.dartlib/src/action_api/event_resolver.dartlib/src/action_api/external_event_handler.dartlib/src/driver_api/capabilities/actions_capability.dartlib/src/driver_api/capabilities/focus_capability.dartlib/src/driver_api/capabilities/index.dartlib/src/driver_api/capabilities/ui_controller_capability.dartlib/src/driver_api/index.dartlib/src/driver_api/ui_driver.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: test (3.32.7)
🔇 Additional comments (13)
lib/src/action_api/event_parser.dart (1)
8-8: Documentation reference updated appropriately.The bracket link removal for
DefaultEventResolveraligns with the class's deprecation in this PR. Since it's slated for removal, keeping a non-navigable plain text reference is sensible.lib/src/action_api/external_event_handler.dart (1)
7-8: Deprecation annotation added consistently.The deprecation message matches the pattern used across other deprecated classes in this PR (
ActionExecutor,EventResolver). This provides a clear migration signal.lib/src/action_api/event_resolver.dart (1)
6-7: Deprecation annotations applied consistently.Both the abstract class and concrete implementation are marked with the same deprecation message, providing clear API lifecycle signaling.
Also applies to: 19-20
lib/src/action_api/action_executor.dart (1)
11-12: Deprecation annotations consistently applied.Both the abstract class and concrete implementation are deprecated with matching messages, aligning with the broader API migration strategy in this PR.
Also applies to: 38-39
lib/src/driver_api/capabilities/focus_capability.dart (3)
21-28: ExtendedrequestFocussignature with optionaltargetNodeId.The new parameter provides additional flexibility for focus targeting. The default throwing behavior remains consistent with the mixin's contract.
82-86: NewreleaseResourcesmethod added.This provides an explicit cleanup path for capability implementations, ensuring resources (like focus nodes) can be properly disposed. Consistent with the pattern used in other capability delegates.
17-18: Thelate finalfield design is sound.The
driverfield is correctly declared aslate finaland is only accessed in overridden method implementations, not in the default implementations that throwMissingCapabilityMethodImplementation. Consuming classes that override the mixin's methods should initialize this field during setup, but the current design safely prevents access issues by exiting early in default implementations. The pattern is confirmed by the test suite.lib/src/driver_api/capabilities/actions_capability.dart (2)
7-17: New types for user-defined event handling.The
UserDefinedEventHandlertypedef andUserDefinedHandlerKindenum provide a clean abstraction for external event handling, replacing the deprecatedExternalEventHandlerinterface.
29-67: Capability structure follows established pattern.The
late final driverfield and throwing default implementations are consistent withFocusCapabilityDelegate, providing a uniform capability contract.lib/src/driver_api/index.dart (1)
9-9: Export consolidated through capabilities barrel file.This cleanly centralizes the capability exports. Consumers importing from this index will now have access to all three capability delegates (
actions_capability,focus_capability,ui_controller_capability).lib/src/driver_api/capabilities/index.dart (1)
1-3: Clean barrel file for capability exports.Standard pattern that provides a single import point for all capability delegates. This supports the migration to the capability-based API. All exported files are present and correctly referenced.
lib/src/driver_api/ui_driver.dart (2)
54-63: Well-documentedinit()method.The
init()method has thorough documentation covering its purpose, when to call it, and return value semantics. This is a good addition to the API.
7-12: Ensure concrete UIDriver implementations initialize mixin driver fields.The three capability mixins (
FocusCapabilityDelegate,ServerActionExecutionCapabilityDelegate,UIControllerCapabilityDelegate) each declarelate final UIDriver driver;. When a concrete class implements UIDriver, it must initialize this field (typically by assigningthis) before the field is accessed, either during construction or in the newinit()method, to avoidLateInitializationError.
| /// A mixin that provides an interface for managing [ServerAction] execution | ||
| /// and [ServerEvent] handling | ||
| /// | ||
| /// Classes that use [ServerActionExecutionCapabilityDelegate] must implement focus management | ||
| /// behaviors by overriding the methods. By default, methods throw | ||
| /// [MissingCapabilityMethodImplementation] if not overridden. | ||
| /// | ||
| /// See also: | ||
| /// - https://duit.pro/en/docs/core_concepts/actions_events | ||
| mixin ServerActionExecutionCapabilityDelegate { |
There was a problem hiding this comment.
Documentation copy-paste error: references "focus management".
Line 22-23 states this mixin handles "focus management behaviors," but this capability is for action execution and event handling.
🔎 Proposed fix
/// A mixin that provides an interface for managing [ServerAction] execution
/// and [ServerEvent] handling
///
-/// Classes that use [ServerActionExecutionCapabilityDelegate] must implement focus management
-/// behaviors by overriding the methods. By default, methods throw
+/// Classes that use [ServerActionExecutionCapabilityDelegate] must implement action execution
+/// and event handling behaviors by overriding the methods. By default, methods throw
/// [MissingCapabilityMethodImplementation] if not overridden.📝 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.
| /// A mixin that provides an interface for managing [ServerAction] execution | |
| /// and [ServerEvent] handling | |
| /// | |
| /// Classes that use [ServerActionExecutionCapabilityDelegate] must implement focus management | |
| /// behaviors by overriding the methods. By default, methods throw | |
| /// [MissingCapabilityMethodImplementation] if not overridden. | |
| /// | |
| /// See also: | |
| /// - https://duit.pro/en/docs/core_concepts/actions_events | |
| mixin ServerActionExecutionCapabilityDelegate { | |
| /// A mixin that provides an interface for managing [ServerAction] execution | |
| /// and [ServerEvent] handling | |
| /// | |
| /// Classes that use [ServerActionExecutionCapabilityDelegate] must implement action execution | |
| /// and event handling behaviors by overriding the methods. By default, methods throw | |
| /// [MissingCapabilityMethodImplementation] if not overridden. | |
| /// | |
| /// See also: | |
| /// - https://duit.pro/en/docs/core_concepts/actions_events | |
| mixin ServerActionExecutionCapabilityDelegate { |
🤖 Prompt for AI Agents
In lib/src/driver_api/capabilities/actions_capability.dart around lines 19 to
28, the doc comment incorrectly refers to "focus management behaviors"; update
the documentation to accurately describe that the mixin provides interfaces for
ServerAction execution and ServerEvent handling and that implementing classes
must override the execution/handling methods (or they throw
MissingCapabilityMethodImplementation). Replace the erroneous "focus management"
wording with precise language about action execution and event handling and keep
the note about methods throwing MissingCapabilityMethodImplementation by
default.
| @mustBeOverridden | ||
| void attachExternalHandler( | ||
| UserDefinedHandlerKind type, | ||
| UserDefinedEventHandler handle, | ||
| ) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "dispose", | ||
| "ServerActionExecutionCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
Incorrect method name in error message.
The error message on line 75 says "dispose" but the method is attachExternalHandler. This will produce confusing errors for implementers.
🔎 Proposed fix
@mustBeOverridden
void attachExternalHandler(
UserDefinedHandlerKind type,
UserDefinedEventHandler handle,
) =>
throw const MissingCapabilityMethodImplementation(
- "dispose",
+ "attachExternalHandler",
"ServerActionExecutionCapabilityDelegate",
);📝 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.
| @mustBeOverridden | |
| void attachExternalHandler( | |
| UserDefinedHandlerKind type, | |
| UserDefinedEventHandler handle, | |
| ) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "dispose", | |
| "ServerActionExecutionCapabilityDelegate", | |
| ); | |
| @mustBeOverridden | |
| void attachExternalHandler( | |
| UserDefinedHandlerKind type, | |
| UserDefinedEventHandler handle, | |
| ) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "attachExternalHandler", | |
| "ServerActionExecutionCapabilityDelegate", | |
| ); |
🤖 Prompt for AI Agents
In lib/src/driver_api/capabilities/actions_capability.dart around lines 69 to
77, the thrown MissingCapabilityMethodImplementation currently references the
wrong method name "dispose"; change that message to "attachExternalHandler" so
implementers see the correct missing method name, i.e. update the second
argument to the MissingCapabilityMethodImplementation constructor (or the
message text) to "attachExternalHandler" while keeping the same exception type
and the class name "ServerActionExecutionCapabilityDelegate".
| mixin UIControllerCapabilityDelegate { | ||
| late final UIDriver driver; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting the driver initialization contract.
The late final UIDriver driver field requires initialization by the implementing class. Consider adding a doc comment clarifying when and how this field should be initialized to prevent LateInitializationError at runtime.
🤖 Prompt for AI Agents
In lib/src/driver_api/capabilities/ui_controller_capability.dart around lines 4
to 5, the mixin declares a late final UIDriver driver without documenting the
initialization contract; add a doc comment above the driver field that states
that implementers must initialize this field before any getter or method on the
mixin is used (for example in the implementing class constructor or an
initialization method called before use), describe expected lifecycle (who owns
the instance and when it becomes available), and note that failing to do so will
throw LateInitializationError; keep the comment short and prescriptive so
implementers know exactly where and when to assign the driver.
| /// Updates the attributes of a controller. | ||
| @mustBeOverridden | ||
| Future<void> updateAttributes( | ||
| String controllerId, | ||
| Map<String, dynamic> json, | ||
| ) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "updateAttributes", | ||
| "UIControllerCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
Async method throws synchronously instead of returning a failed Future.
updateAttributes declares Future<void> as its return type but throws a synchronous exception. If a caller uses .then()/.catchError() instead of try-catch, the exception won't be caught as expected.
🔎 Proposed fix to wrap in Future.error
@mustBeOverridden
Future<void> updateAttributes(
String controllerId,
Map<String, dynamic> json,
) =>
- throw const MissingCapabilityMethodImplementation(
+ Future.error(const MissingCapabilityMethodImplementation(
"updateAttributes",
"UIControllerCapabilityDelegate",
- );
+ ));📝 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.
| /// Updates the attributes of a controller. | |
| @mustBeOverridden | |
| Future<void> updateAttributes( | |
| String controllerId, | |
| Map<String, dynamic> json, | |
| ) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "updateAttributes", | |
| "UIControllerCapabilityDelegate", | |
| ); | |
| /// Updates the attributes of a controller. | |
| @mustBeOverridden | |
| Future<void> updateAttributes( | |
| String controllerId, | |
| Map<String, dynamic> json, | |
| ) => | |
| Future.error(const MissingCapabilityMethodImplementation( | |
| "updateAttributes", | |
| "UIControllerCapabilityDelegate", | |
| )); |
🤖 Prompt for AI Agents
In lib/src/driver_api/capabilities/ui_controller_capability.dart around lines 38
to 47 the async method updateAttributes currently throws a
MissingCapabilityMethodImplementation synchronously; change it to return a
failed Future instead (e.g., return
Future<void>.error(MissingCapabilityMethodImplementation("updateAttributes","UIControllerCapabilityDelegate")));
so callers using then()/catchError() receive the error via the Future rather
than as a synchronous throw.
| @mustBeOverridden | ||
| int get controllersCount => throw const MissingCapabilityMethodImplementation( | ||
| "controllersCount", | ||
| "UIControllerCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing documentation for controllersCount getter.
Other members have doc comments explaining their purpose. Adding a brief doc comment here would maintain consistency.
🔎 Proposed documentation
+ /// Returns the number of currently attached controllers.
@mustBeOverridden
int get controllersCount => throw const MissingCapabilityMethodImplementation(
"controllersCount",
"UIControllerCapabilityDelegate",
);🤖 Prompt for AI Agents
In lib/src/driver_api/capabilities/ui_controller_capability.dart around lines 49
to 53, the controllersCount getter is missing a doc comment; add a concise Dart
doc comment above the getter that explains what controllersCount represents
(e.g., the number of available UI controllers exposed by this capability) and
any important semantics (whether it can change at runtime or is immutable),
matching the style and tone of the other members in the file.
| @mustBeOverridden | ||
| void releaseResources() => throw const MissingCapabilityMethodImplementation( | ||
| "releaseResources", | ||
| "UIControllerCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing documentation for releaseResources method.
This method lacks a doc comment. Adding one would clarify what resources are released and when this should be called.
🔎 Proposed documentation
+ /// Releases resources held by the controller capability.
+ ///
+ /// This method should be called when the driver is disposed.
@mustBeOverridden
void releaseResources() => throw const MissingCapabilityMethodImplementation(
"releaseResources",
"UIControllerCapabilityDelegate",
);📝 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.
| @mustBeOverridden | |
| void releaseResources() => throw const MissingCapabilityMethodImplementation( | |
| "releaseResources", | |
| "UIControllerCapabilityDelegate", | |
| ); | |
| /// Releases resources held by the controller capability. | |
| /// | |
| /// This method should be called when the driver is disposed. | |
| @mustBeOverridden | |
| void releaseResources() => throw const MissingCapabilityMethodImplementation( | |
| "releaseResources", | |
| "UIControllerCapabilityDelegate", | |
| ); |
🤖 Prompt for AI Agents
In lib/src/driver_api/capabilities/ui_controller_capability.dart around lines 55
to 59, add a doc comment for the releaseResources method explaining which
resources the implementer should free (e.g., UI listeners, controllers,
controllers' streams, timers, native handles), when callers should invoke it
(lifecycle teardown or before disposal), expected behavior (idempotent if
possible), threading/context expectations, and whether it can throw or should
swallow errors; update the comment to mention that implementations must override
the method (it currently throws MissingCapabilityMethodImplementation) and any
recommended order for releasing resources if relevant.
| @Deprecated("Will be removed in the next major release.") | ||
| abstract EventResolver eventResolver; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deprecation messages could be more actionable.
The deprecation annotations say "Will be removed in the next major release" but don't guide users toward the replacement. Consider pointing to the new capability-based APIs.
🔎 Example of more actionable deprecation message
- @Deprecated("Will be removed in the next major release.")
+ @Deprecated("Will be removed in the next major release. Use ServerActionExecutionCapabilityDelegate methods instead.")
abstract EventResolver eventResolver;Also applies to: 41-42, 45-46
🤖 Prompt for AI Agents
In lib/src/driver_api/ui_driver.dart around lines 35-36 (and also update the
similar annotations at 41-42 and 45-46), the @Deprecated messages are vague;
replace them with actionable guidance that points callers to the new
capability-based APIs — e.g., update the deprecation string to mention the
capability-based replacement and where to find migration docs (for example:
"Deprecated — use the capability-based APIs instead; see <docs or module> for
migration"), so users know which API to use and where to find instructions.
| /// Gets the controller associated with the given ID. | ||
| UIElementController? getController(String id); | ||
|
|
||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trailing whitespace on this line.
Line 53 has trailing whitespace after logger. This is a minor style issue but worth cleaning up.
🤖 Prompt for AI Agents
In lib/src/driver_api/ui_driver.dart around line 53, there is trailing
whitespace after `logger`; remove the trailing spaces at the end of that line so
the file has no extraneous whitespace and save the file.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
lib/src/capabilities/actions_capability.dartlib/src/capabilities/focus_capability.dartlib/src/capabilities/index.dartlib/src/capabilities/ui_controller_capability.dartlib/src/driver_api/index.dartlib/src/index.dart
💤 Files with no reviewable changes (1)
- lib/src/driver_api/index.dart
🔇 Additional comments (7)
lib/src/capabilities/focus_capability.dart (1)
1-18: Well-structured mixin with clear contract.The mixin establishes a solid interface for focus management with proper use of
@mustBeOverriddenannotations and sensible default throwing behavior. The documentation is comprehensive and references relevant Flutter APIs.lib/src/index.dart (1)
9-9: LGTM!The new export follows the established pattern and properly exposes the capabilities module through the public API.
lib/src/capabilities/index.dart (1)
1-3: LGTM!Clean barrel file that consolidates the capability exports under a single entry point, following Dart conventions.
lib/src/capabilities/ui_controller_capability.dart (2)
36-43: LGTM!Well-documented method with clear expectations for implementers.
19-21: This pattern is correct in Dart and requires no changes.Each mixin declaring
late final UIDriver driveris the intended design. When mixins are composed on the same class, Dart's linearization ensures all declarations refer to a single field instance on UIDriver—not separate fields. This allows each mixin to independently declare its shared dependency without creating conflicts or redundancy.Likely an incorrect or invalid review comment.
lib/src/capabilities/actions_capability.dart (2)
7-17: LGTM!Clear typedef with helpful documentation explaining the handler signature and its use cases.
27-36: LGTM!Well-documented enum with clear semantics for each handler kind.
| @mustBeOverridden | ||
| Future<void> execute(ServerAction action) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "execute", | ||
| "ServerActionExecutionCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Synchronous throw from async method.
Same pattern as in UIControllerCapabilityDelegate.updateAttributes — consider using Future.error() for consistency with async semantics, or add a note in the doc that the default throws synchronously.
🤖 Prompt for AI Agents
In lib/src/capabilities/actions_capability.dart around lines 60 to 65, the
method currently throws the MissingCapabilityMethodImplementation synchronously
from an async-returning method; change it to return a Future error instead
(e.g., return Future.error(...) with the same
MissingCapabilityMethodImplementation) so the exception is produced
asynchronously and aligns with async semantics — alternatively, if synchronous
behavior is desired, add a clear doc comment stating the method throws
synchronously by default.
| @mustBeOverridden | ||
| Future<void> resolveEvent(BuildContext context, eventData) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "resolveEvent", | ||
| "ServerActionExecutionCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Missing type annotation for eventData parameter.
The eventData parameter is untyped (implicitly dynamic). Adding an explicit type improves code clarity, IDE support, and catches type errors at compile time. Based on the doc comment mentioning "parsed [ServerEvent] or compatible structure," consider using a more specific type or at least Object?.
🔎 Proposed fix
@mustBeOverridden
- Future<void> resolveEvent(BuildContext context, eventData) =>
+ Future<void> resolveEvent(BuildContext context, Object? eventData) =>
throw const MissingCapabilityMethodImplementation(
"resolveEvent",
"ServerActionExecutionCapabilityDelegate",
);📝 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.
| @mustBeOverridden | |
| Future<void> resolveEvent(BuildContext context, eventData) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "resolveEvent", | |
| "ServerActionExecutionCapabilityDelegate", | |
| ); | |
| @mustBeOverridden | |
| Future<void> resolveEvent(BuildContext context, Object? eventData) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "resolveEvent", | |
| "ServerActionExecutionCapabilityDelegate", | |
| ); |
🤖 Prompt for AI Agents
In lib/src/capabilities/actions_capability.dart around lines 100 to 105, the
parameter eventData is currently untyped (implicit dynamic); change its
parameter type to a concrete nullable type (preferably ServerEvent or, if that
type is not available here, Object?) to improve type safety and IDE tooling,
update the method signature accordingly, and add any necessary import for
ServerEvent if used.
| @mustBeOverridden | ||
| void detachFocusNode(String nodeId) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "detachFocusNode", | ||
| "FocusCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Missing documentation for detachFocusNode.
This method lacks the doc comment that all other methods in the mixin have. Add documentation describing the expected behavior, similar to attachFocusNode.
🔎 Proposed documentation
+ /// Detaches and removes the [FocusNode] associated with the given [nodeId].
+ ///
+ /// [nodeId]: The unique identifier of the focus node to detach.
+ ///
+ /// Implementations should remove the mapping between [nodeId] and its associated
+ /// [FocusNode], allowing the node to be garbage collected if no longer referenced.
+ /// This is typically called when a focusable widget is disposed.
+ ///
+ /// If no node with the given [nodeId] is attached, this method may be a no-op.
+ ///
+ /// Throws [MissingCapabilityMethodImplementation] if not overridden.
+ ///
+ /// See also:
+ /// - [attachFocusNode] to associate a node with an id.
@mustBeOverridden
void detachFocusNode(String nodeId) =>
throw const MissingCapabilityMethodImplementation(
"detachFocusNode",
"FocusCapabilityDelegate",
);📝 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.
| @mustBeOverridden | |
| void detachFocusNode(String nodeId) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "detachFocusNode", | |
| "FocusCapabilityDelegate", | |
| ); | |
| /// Detaches and removes the [FocusNode] associated with the given [nodeId]. | |
| /// | |
| /// [nodeId]: The unique identifier of the focus node to detach. | |
| /// | |
| /// Implementations should remove the mapping between [nodeId] and its associated | |
| /// [FocusNode], allowing the node to be garbage collected if no longer referenced. | |
| /// This is typically called when a focusable widget is disposed. | |
| /// | |
| /// If no node with the given [nodeId] is attached, this method may be a no-op. | |
| /// | |
| /// Throws [MissingCapabilityMethodImplementation] if not overridden. | |
| /// | |
| /// See also: | |
| /// - [attachFocusNode] to associate a node with an id. | |
| @mustBeOverridden | |
| void detachFocusNode(String nodeId) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "detachFocusNode", | |
| "FocusCapabilityDelegate", | |
| ); |
🤖 Prompt for AI Agents
In lib/src/capabilities/focus_capability.dart around lines 165 to 170, add the
missing documentation for detachFocusNode to match the style and content of the
other methods (e.g., attachFocusNode): document the purpose (detach an existing
focus node managed by the delegate), the expected parameter (String nodeId) and
what callers should expect (that the node will be removed and any associated
resources cleaned up), any edge cases (e.g., no-op if nodeId not found), and
note that implementors must override this method; update the doc comment above
the method accordingly so it mirrors attachFocusNode's format and wording.
| Future<void> updateAttributes( | ||
| String controllerId, | ||
| Map<String, dynamic> json, | ||
| ) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "updateAttributes", | ||
| "UIControllerCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Synchronous throw from async method signature.
The updateAttributes method is declared as Future<void> but throws synchronously. While Dart will convert this to a failed Future, consider explicitly returning Future.error(...) for clarity, or document that the default implementation throws synchronously.
🔎 Alternative using Future.error
@mustBeOverridden
Future<void> updateAttributes(
String controllerId,
Map<String, dynamic> json,
) =>
- throw const MissingCapabilityMethodImplementation(
+ Future.error(const MissingCapabilityMethodImplementation(
"updateAttributes",
"UIControllerCapabilityDelegate",
- );
+ ));📝 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.
| Future<void> updateAttributes( | |
| String controllerId, | |
| Map<String, dynamic> json, | |
| ) => | |
| throw const MissingCapabilityMethodImplementation( | |
| "updateAttributes", | |
| "UIControllerCapabilityDelegate", | |
| ); | |
| Future<void> updateAttributes( | |
| String controllerId, | |
| Map<String, dynamic> json, | |
| ) => | |
| Future.error(const MissingCapabilityMethodImplementation( | |
| "updateAttributes", | |
| "UIControllerCapabilityDelegate", | |
| )); |
🤖 Prompt for AI Agents
In lib/src/capabilities/ui_controller_capability.dart around lines 92 to 99, the
async-signature method updateAttributes currently throws synchronously; change
it to return a failed Future instead by returning Future.error(...) with the
same MissingCapabilityMethodImplementation (keep the const if appropriate) so
the failure is represented as an asynchronous Future error consistent with the
method's Future<void> signature.
There was a problem hiding this comment.
Actionable comments posted: 6
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
test/action_capability_test.darttest/controller_capability_test.darttest/focus_capability_test.darttest/testutil.dart
🧰 Additional context used
🪛 GitHub Actions: On PR
test/action_capability_test.dart
[warning] 56-56: Missing a required trailing comma (require_trailing_commas).
test/focus_capability_test.dart
[warning] 32-32: Missing a required trailing comma (require_trailing_commas).
test/testutil.dart
[warning] 34-34: Unclosed instance of 'Sink' (close_sinks).
[warning] 119-119: Missing a required trailing comma (require_trailing_commas).
[warning] 126-126: Missing a required trailing comma (require_trailing_commas).
[warning] 133-133: Missing a required trailing comma (require_trailing_commas).
[warning] 140-140: Missing a required trailing comma (require_trailing_commas).
[warning] 153-153: Missing a required trailing comma (require_trailing_commas).
🔇 Additional comments (5)
test/controller_capability_test.dart (1)
9-77: Well-structured test coverage for UIControllerCapabilityDelegate.The tests comprehensively verify that all unimplemented capability methods throw
MissingCapabilityMethodImplementationwith correct metadata. Good use of the sharedthrowsMissingmatcher from testutil.test/focus_capability_test.dart (1)
89-107: Good additions for expanded test coverage.The new tests for
getNodeandreleaseResourcescomplete the coverage for FocusCapabilityDelegate methods.test/action_capability_test.dart (1)
9-85: Comprehensive test suite for ServerActionExecutionCapabilityDelegate.Tests verify all capability methods throw appropriate exceptions with correct metadata. Consistent pattern with other capability tests.
test/testutil.dart (2)
8-24: Well-designed reusable matcher.The
throwsMissinghelper cleanly encapsulates the exception matching logic with property verification formethodNameandmixinName. Good use ofisAwith chained.having()calls.
112-231: MockBuildCtx is adequate for test scaffolding.The extensive
BuildContextmock withUnimplementedErrorthrows is a common pattern for tests that don't need full context functionality. The TODO comments are fine for a test utility.
| test("controllersCount throws MissingMethodImplementation", () { | ||
| expect( | ||
| () => delegate.attachController("", MockController()), | ||
| throwsMissing( | ||
| "attachController", | ||
| "UIControllerCapabilityDelegate", | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
Copy/paste error: Test name doesn't match what's being tested.
The test name says "controllersCount throws MissingMethodImplementation" but it's actually testing attachController. This will cause confusion in test reports.
🔎 Proposed fix
- test("controllersCount throws MissingMethodImplementation", () {
+ test("attachController throws MissingMethodImplementation", () {
expect(
() => delegate.attachController("", MockController()),
throwsMissing(📝 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.
| test("controllersCount throws MissingMethodImplementation", () { | |
| expect( | |
| () => delegate.attachController("", MockController()), | |
| throwsMissing( | |
| "attachController", | |
| "UIControllerCapabilityDelegate", | |
| ), | |
| ); | |
| }); | |
| test("attachController throws MissingMethodImplementation", () { | |
| expect( | |
| () => delegate.attachController("", MockController()), | |
| throwsMissing( | |
| "attachController", | |
| "UIControllerCapabilityDelegate", | |
| ), | |
| ); | |
| }); |
🤖 Prompt for AI Agents
In test/controller_capability_test.dart around lines 26 to 34, the test name is
incorrect: it reads "controllersCount throws MissingMethodImplementation" but
the assertion is testing attachController; update the test description string to
accurately reflect the behavior being tested (e.g., "attachController throws
MissingMethodImplementation") so test reports match the actual assertion.
| @override | ||
| late ViewAttribute attributes; | ||
|
|
||
| @override | ||
| late StreamController<RemoteCommand> commandChannel; | ||
|
|
||
| @override | ||
| late UIDriver driver; | ||
|
|
||
| @override | ||
| late String id; | ||
|
|
||
| @override | ||
| String? tag; | ||
|
|
||
| @override | ||
| late String type; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Late fields without initialization may cause runtime errors.
Fields like attributes, commandChannel, driver, id, and type are declared late but have no default values. If test code accesses these before assignment, you'll get LateInitializationError. Consider initializing them with sensible defaults or making them nullable where appropriate.
🔎 Suggested approach
@override
- late ViewAttribute attributes;
+ ViewAttribute attributes = const ViewAttribute({});
@override
- late StreamController<RemoteCommand> commandChannel;
+ StreamController<RemoteCommand> commandChannel = StreamController<RemoteCommand>();
@override
- late String id;
+ String id = "mock-id";
@override
- late String type;
+ String type = "mock-type";Note: driver would need a mock or nullable handling depending on how UIDriver is structured.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Actions: On PR
[warning] 34-34: Unclosed instance of 'Sink' (close_sinks).
🤖 Prompt for AI Agents
In test/testutil.dart around lines 30 to 46, several fields are declared as late
(attributes, commandChannel, driver, id, type) without defaults which can throw
LateInitializationError if accessed before assignment; change these to either
provide sensible default initializers (e.g., empty ViewAttribute, a new
StreamController<RemoteCommand>(), default id/type strings) or make them
nullable types and handle null in tests, and for driver supply a mock or make it
UIDriver? to avoid forcing late initialization; update any tests that assume
non-null values to use the new defaults or null-safe checks.
Summary by CodeRabbit
Deprecations
New Features
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.