chore: API deprecation #53
Conversation
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe pull request systematically adds Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/action_api/event_resolver.dart (1)
49-52: Inverted null-check causes incorrect behavior.The condition
if (driver.externalEventHandler != null)is inverted. Currently, this throws an exception when the handler is set, and silently skips execution when it's not set—the opposite of the intended behavior.The same issue exists in the
OpenUrlEventcase at lines 60-63.🐛 Proposed fix for both cases
case NavigationEvent(): assert( driver.externalEventHandler != null, "ExternalEventHandler instance is not set", ); - if (driver.externalEventHandler != null) { + if (driver.externalEventHandler == null) { driver.logError("ExternalEventHandler instance is not set"); throw StateError("ExternalEventHandler instance is not set"); } await driver.externalEventHandler?.handleNavigation( context, event.path, event.extra, ); break; case OpenUrlEvent(): - if (driver.externalEventHandler != null) { + if (driver.externalEventHandler == null) { driver.logError("ExternalEventHandler instance is not set"); throw StateError("ExternalEventHandler instance is not set"); } await driver.externalEventHandler?.handleOpenUrl(event.url); break;
🤖 Fix all issues with AI agents
In @lib/src/action_api/action_executor.dart:
- Around line 11-12: The deprecation message on the ActionExecutor class is
vague; update the @Deprecated string on ActionExecutor to include migration
guidance by suggesting the replacement API (e.g., reference the new class or
interface like [SomeNewApi] or the preferred delegate) and keep the existing
timeline text (e.g., "Use [SomeNewApi] instead. Will be removed in the next
major release."). Ensure the message references the ActionExecutor symbol so
callers see the recommended alternative when they get deprecation warnings.
In @lib/src/driver_api/ui_driver.dart:
- Around line 33-37: The doc comment above the deprecated field eventResolver is
describing a script runner (copy/paste error); update the comment to accurately
describe eventResolver (e.g., that it resolves events for the UI driver) or move
the script-runner comment onto the correct symbol if a scriptRunner exists;
ensure the summary and description text reference the abstract EventResolver
type and retain the @Deprecated annotation on eventResolver.
In @lib/src/registry_api/registry.dart:
- Around line 16-17: The deprecation message for the constructor parameter
logger is too vague; update the @Deprecated annotation on DebugLogger? logger in
registry.dart to include migration guidance pointing users to the replacement
APIs (for example mention LoggingCapabilityDelegate and logManager) so callers
know what to use instead; locate the parameter named logger (DebugLogger?
logger) and change the deprecation string to a brief actionable guidance like
"Will be removed in the next major release; use LoggingCapabilityDelegate and
logManager instead."
In @lib/src/transport_api/streamer.dart:
- Around line 6-7: The deprecation notice on the Streamer class is fine but
should include migration guidance; update the @Deprecated message on class
Streamer to mention the suggested replacement and migration path (e.g., indicate
which new API or class to use and a brief note like "use X instead" or link to
docs), so consumers know what to migrate to before the next major release.
In @lib/src/transport_api/transport.dart:
- Around line 17-18: Update the @Deprecated message on the Transport class to
include migration guidance and the replacement API, e.g., reference the new
Streamer (or whichever replacement) and a brief instruction or link for
migration; modify the annotation on Transport to a more informative string like
"Will be removed in the next major release. Migrate to Streamer: see <migration
guide or method> for how to port usage." so users get clear transition steps.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.mdlib/src/action_api/action_executor.dartlib/src/action_api/event_resolver.dartlib/src/action_api/external_event_handler.dartlib/src/driver_api/display_state_notifier.dartlib/src/driver_api/ui_driver.dartlib/src/misc/logger/io.dartlib/src/misc/logger/logger.dartlib/src/misc/logger/web.dartlib/src/misc/script_runner.dartlib/src/registry_api/registry.dartlib/src/transport_api/streamer.dartlib/src/transport_api/transport.dartlib/src/transport_api/transport_options.dartpubspec.yaml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-28T19:09:54.482Z
Learnt from: lesleysin
Repo: Duit-Foundation/duit_kernel PR: 43
File: lib/src/action_api/action_parser.dart:12-18
Timestamp: 2025-10-28T19:09:54.482Z
Learning: DuitDataSource is an extension type on Map<String, dynamic> in the duit_kernel project. Since extension types are compile-time abstractions, the runtime type remains Map<String, dynamic>, making it fully compatible with methods expecting Map<String, dynamic> parameters. No signature changes are needed when passing DuitDataSource to such methods.
Applied to files:
pubspec.yaml
🪛 markdownlint-cli2 (0.18.1)
CHANGELOG.md
1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
⏰ 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 (14)
CHANGELOG.md (1)
1-3: Changelog entry looks good.The entry accurately documents the public API deprecation for version 4.2.0-rc.3.
Regarding the markdownlint hint (MD041): the file starts with a level-2 heading rather than a top-level H1. This is a common convention for changelogs, so it's fine to leave as-is. If you want to silence the lint, you could add a
# Changelogheading at the top.pubspec.yaml (1)
3-3: Version bump is consistent with the changelog.The version update to
4.2.0-rc.3aligns with the CHANGELOG entry and appropriately reflects the API deprecation work in this release candidate.lib/src/driver_api/display_state_notifier.dart (1)
15-16: Deprecation annotation applied correctly.The
@Deprecatedannotation is properly placed on theWidgetDisplayStateNotifierinterface. The annotation follows the same pattern used consistently across this PR.lib/src/misc/logger/io.dart (1)
9-10: Deprecation annotation looks good.The
DefaultLoggerclass is properly marked for removal in the next major release. The annotation placement and message are clear.lib/src/misc/logger/web.dart (1)
9-10: Consistent deprecation across platform implementations.The web implementation of
DefaultLoggeris deprecated with the same message as the io variant, maintaining consistency across the codebase.lib/src/misc/script_runner.dart (1)
4-7: Good practice including migration guidance.The deprecation message helpfully points users to
ScriptingCapabilityDelegateas the replacement. This provides a clear migration path for consumers of this API.lib/src/action_api/event_resolver.dart (2)
6-7: Deprecation annotation applied correctly.The
EventResolverabstract class is properly marked for removal.
20-21: Deprecation annotation applied correctly.The
DefaultEventResolverclass is properly marked for removal alongside its parent.lib/src/misc/logger/logger.dart (1)
4-5: Interface deprecation aligns with implementations.The
DebugLoggerinterface is properly deprecated alongside itsDefaultLoggerimplementations, ensuring a consistent deprecation story for users of the logging API.lib/src/action_api/action_executor.dart (1)
39-114: Deprecation applied consistently.The
DefaultActionExecutorclass is correctly marked as deprecated with the same message as its parent. The implementation logic remains unchanged, which is appropriate for a deprecation-only change. The same suggestion about adding migration guidance applies here.lib/src/transport_api/transport_options.dart (1)
2-3: Deprecation correctly applied toTransportOptions.The annotation is properly placed on the abstract class. As with other deprecations in this PR, consider adding a migration hint if there's a replacement API (e.g., a new transport configuration approach via capability delegates).
lib/src/action_api/external_event_handler.dart (1)
7-8: Deprecation correctly applied toExternalEventHandler.The interface is properly marked as deprecated. No changes to the method signatures or behavior. Same recommendation applies regarding migration guidance if a replacement exists.
lib/src/driver_api/ui_driver.dart (2)
17-50: Deprecation annotations consistently applied to driver properties.All relevant properties (
source,transportOptions,transport,scriptRunner,eventResolver,actionExecutor,externalEventHandler,driverChannel) are properly marked for deprecation. The class already mixes in capability delegates (e.g.,TransportCapabilityDelegate,ScriptingCapabilityDelegate,LoggingCapabilityDelegate) which appear to be the migration path. Consider whether referencing these in the deprecation messages would aid users.
75-76:build()method deprecation is appropriate.The method is correctly marked as deprecated with the standard message. This aligns with the broader pattern of moving away from direct widget building in favor of the capability delegate pattern.
| @Deprecated("Will be removed in the next major release.") | ||
| abstract class ActionExecutor { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding migration guidance to the deprecation message.
The deprecation annotation is correctly applied, but unlike the logger parameter on line 14 which directs users to [LoggingCapabilityDelegate], this message doesn't indicate what developers should migrate to. Providing a replacement hint (e.g., "Use [SomeNewApi] instead. Will be removed in the next major release.") would help users prepare for the transition.
🤖 Prompt for AI Agents
In @lib/src/action_api/action_executor.dart around lines 11 - 12, The
deprecation message on the ActionExecutor class is vague; update the @Deprecated
string on ActionExecutor to include migration guidance by suggesting the
replacement API (e.g., reference the new class or interface like [SomeNewApi] or
the preferred delegate) and keep the existing timeline text (e.g., "Use
[SomeNewApi] instead. Will be removed in the next major release."). Ensure the
message references the ActionExecutor symbol so callers see the recommended
alternative when they get deprecation warnings.
| /// The script runner used by the UI driver. | ||
| /// | ||
| /// The script runner is used to execute scripts defined in the layout. | ||
| @Deprecated("Will be removed in the next major release.") | ||
| abstract EventResolver eventResolver; |
There was a problem hiding this comment.
Pre-existing doc comment appears misplaced.
The doc comment on lines 33-35 states "The script runner used by the UI driver" but it documents eventResolver, not scriptRunner. This appears to be a copy-paste issue that predates this PR. Consider fixing while you're updating this file.
Suggested fix
- /// The script runner used by the UI driver.
- ///
- /// The script runner is used to execute scripts defined in the layout.
+ /// The event resolver used by the UI driver.
+ ///
+ /// The event resolver is used to resolve events from server actions.
@Deprecated("Will be removed in the next major release.")
abstract EventResolver eventResolver;📝 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.
| /// The script runner used by the UI driver. | |
| /// | |
| /// The script runner is used to execute scripts defined in the layout. | |
| @Deprecated("Will be removed in the next major release.") | |
| abstract EventResolver eventResolver; | |
| /// The event resolver used by the UI driver. | |
| /// | |
| /// The event resolver is used to resolve events from server actions. | |
| @Deprecated("Will be removed in the next major release.") | |
| abstract EventResolver eventResolver; |
🤖 Prompt for AI Agents
In @lib/src/driver_api/ui_driver.dart around lines 33 - 37, The doc comment
above the deprecated field eventResolver is describing a script runner
(copy/paste error); update the comment to accurately describe eventResolver
(e.g., that it resolves events for the UI driver) or move the script-runner
comment onto the correct symbol if a scriptRunner exists; ensure the summary and
description text reference the abstract EventResolver type and retain the
@Deprecated annotation on eventResolver.
| @Deprecated("Will be removed in the next major release.") | ||
| DebugLogger? logger, |
There was a problem hiding this comment.
Deprecation message loses migration guidance.
The previous deprecation message likely indicated what to use instead (e.g., LoggingCapabilityDelegate and logManager). The new message only states that the parameter will be removed without directing users to the replacement. Consider preserving the migration hint:
Suggested improvement
- @Deprecated("Will be removed in the next major release.")
+ @Deprecated("Use [LoggingCapabilityDelegate] and [logManager] instead. Will be removed in the next major release.")📝 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.
| @Deprecated("Will be removed in the next major release.") | |
| DebugLogger? logger, | |
| @Deprecated("Use [LoggingCapabilityDelegate] and [logManager] instead. Will be removed in the next major release.") | |
| DebugLogger? logger, |
🤖 Prompt for AI Agents
In @lib/src/registry_api/registry.dart around lines 16 - 17, The deprecation
message for the constructor parameter logger is too vague; update the
@Deprecated annotation on DebugLogger? logger in registry.dart to include
migration guidance pointing users to the replacement APIs (for example mention
LoggingCapabilityDelegate and logManager) so callers know what to use instead;
locate the parameter named logger (DebugLogger? logger) and change the
deprecation string to a brief actionable guidance like "Will be removed in the
next major release; use LoggingCapabilityDelegate and logManager instead."
| @Deprecated("Will be removed in the next major release.") | ||
| abstract class Streamer { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deprecation annotation applied correctly.
The @Deprecated annotation is properly placed on the class. One suggestion: consider adding migration guidance to the deprecation message (e.g., what users should migrate to), similar to how ScriptRunner points users to ScriptingCapabilityDelegate. This helps downstream consumers prepare for the next major release.
🤖 Prompt for AI Agents
In @lib/src/transport_api/streamer.dart around lines 6 - 7, The deprecation
notice on the Streamer class is fine but should include migration guidance;
update the @Deprecated message on class Streamer to mention the suggested
replacement and migration path (e.g., indicate which new API or class to use and
a brief note like "use X instead" or link to docs), so consumers know what to
migrate to before the next major release.
| @Deprecated("Will be removed in the next major release.") | ||
| abstract class Transport { |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Deprecation annotation applied correctly.
The @Deprecated annotation is properly placed on the Transport class. As with Streamer, consider adding migration guidance in the deprecation message to help users transition to the replacement API before the next major release.
🤖 Prompt for AI Agents
In @lib/src/transport_api/transport.dart around lines 17 - 18, Update the
@Deprecated message on the Transport class to include migration guidance and the
replacement API, e.g., reference the new Streamer (or whichever replacement) and
a brief instruction or link for migration; modify the annotation on Transport to
a more informative string like "Will be removed in the next major release.
Migrate to Streamer: see <migration guide or method> for how to port usage." so
users get clear transition steps.
Summary by CodeRabbit
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.