feat: New capabilities #52
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Introduced ScriptingCapabilityDelegate with methods for script evaluation and execution. * Added TransportCapabilityDelegate for server communication and resource management. * Updated index.dart to export new capabilities. * Enhanced UIDriver to include new capability delegates.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughDeprecates the DebugLogger system and introduces a LoggingCapabilityDelegate mixin with IO and web LoggingManager implementations, adds Transport, Scripting, and NativeModule capability delegates, integrates logging/capabilities into UIDriver, and migrates internal components (executor, resolver, registry) to use the new logging API. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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: 11
🤖 Fix all issues with AI agents
In @lib/src/action_api/event_resolver.dart:
- Around line 58-61: The condition is inverted: the error/throw runs when
driver.externalEventHandler exists instead of when it's missing; change the
check in the block that references driver.externalEventHandler from != null to
== null so the driver.error("ExternalEventHandler instance is not set") and
throw StateError(...) only execute when externalEventHandler is null; keep the
same error message and StateError but invert the condition around
driver.externalEventHandler to correctly detect the missing handler.
- Around line 47-50: The condition in the event resolver is inverted: it
currently checks if driver.externalEventHandler != null and then logs an error
and throws, but you should log and throw when the handler IS null. Change the
conditional to check driver.externalEventHandler == null before calling
driver.error(...) and throwing StateError("ExternalEventHandler instance is not
set") so the error path runs only when externalEventHandler is missing.
In @lib/src/capabilities/logging/io.dart:
- Line 9: The timestamp in the StringBuffer initialization (variable "buff")
uses DateTime.now().toUtc() and should be converted to an ISO 8601 string for
consistency with the web implementation; update the expression to call
toIso8601String() on the UTC time (e.g.,
DateTime.now().toUtc().toIso8601String()) so the log line uses a parseable ISO
8601 timestamp.
- Around line 4-77: The LoggingManager has duplicated message formatting across
its level methods (critical, debug, error, info, verbose, warning); extract the
common logic into a private helper (e.g. String _formatMessage(String level,
message, Object? exception, StackTrace? stackTrace)) that builds the
StringBuffer and returns the formatted string, then update each override to call
debugPrint(_formatMessage("LEVEL", message, exception, stackTrace)); ensure
helper is used by all logging methods to remove duplication and keep behavior
identical.
In @lib/src/capabilities/logging/logging_capability.dart:
- Around line 10-98: Update the six logging method signatures (info, debug,
warning, error, critical, verbose) to explicitly type the first parameter as
dynamic (e.g., dynamic message) instead of leaving it implicit, so adjust the
declarations in LoggingCapabilityDelegate / LoggingCapability (the methods named
info, debug, warning, error, critical, verbose) while keeping the optional
parameters (Object? exception, StackTrace? stackTrace) and existing behavior
unchanged.
In @lib/src/capabilities/logging/web.dart:
- Around line 6-79: The six logging methods in LoggingManager (critical, debug,
error, info, verbose, warning) duplicate the same message-building logic;
extract a private helper (e.g., String _formatMessage(String level, message,
Object? exception, StackTrace? stackTrace)) that constructs the StringBuffer,
appends exception and stackTrace when non-null, and returns the final string,
then replace each method to call console.error/debug/info/log/warn with
_formatMessage("LEVEL", message, exception, stackTrace).toJS so only the level
and console method differ.
- Line 11: The timestamp in the StringBuffer assigned to buff uses local time
via DateTime.now().toIso8601String(), causing inconsistency with the IO logger
which uses UTC; update the StringBuffer construction in web.dart (the buff
variable) to use UTC ISO timestamp by replacing DateTime.now().toIso8601String()
with DateTime.now().toUtc().toIso8601String() so both loggers produce consistent
UTC-formatted timestamps.
In @lib/src/capabilities/transport_capability.dart:
- Around line 16-29: The doc comment for executeRemoteAction incorrectly says it
returns a ServerEvent; update the comment above the TransportCapabilityDelegate
method executeRemoteAction to describe the actual return type Future<Map<String,
dynamic>?> (e.g., "Returns a Future resolving to a Map<String, dynamic>?
representing the server response") and remove or replace any mention of
ServerEvent so the text matches the method signature.
- Around line 48-63: The doc comment for TransportCapabilityDelegate.connect is
misleading and incorrect: rephrase the Stream description to state it "emits
server events over time" (not "completes when the connection is established")
and remove the incorrect "Returns a [ServerEvent] object" line, replacing it
with that the method returns a Stream<Map<String, dynamic>> emitting server
events (or static/initial payloads) and mention parameters initialRequestData
and staticContent; leave the method signature and the
MissingCapabilityMethodImplementation throw in place.
- Around line 31-46: The doc comment above the request method is incorrect: it
says "Returns a [ServerEvent] object" but the signature returns
Future<Map<String, dynamic>?>; update the documentation for request (in
TransportCapabilityDelegate) to describe the actual return type (e.g., "Returns
a Future containing a Map<String, dynamic>? representing the server response" or
equivalent) and ensure the parameter descriptions still match the method
signature; do not change the method signature or the
MissingCapabilityMethodImplementation throw.
In @lib/src/registry_api/registry.dart:
- Around line 75-77: Fix the typos: rename the misspelled symbol "desctiption"
to "description" (update its declaration and all usages, including the variable
referenced near line 71 and the _logManager.warning call that currently says
"Not found desctiption for specified tag - $tag"), and correct the log message
that reads "successfull" to "successful" (update the _logManager.* call near
line 90). Ensure all references and import/exports (if any) are updated to the
corrected identifier.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (15)
lib/src/action_api/action_executor.dartlib/src/action_api/event_resolver.dartlib/src/capabilities/index.dartlib/src/capabilities/logging/index.dartlib/src/capabilities/logging/io.dartlib/src/capabilities/logging/logging_capability.dartlib/src/capabilities/logging/web.dartlib/src/capabilities/scripting_capability.dartlib/src/capabilities/transport_capability.dartlib/src/driver_api/http_meta.dartlib/src/driver_api/ui_driver.dartlib/src/misc/logger/io.dartlib/src/misc/logger/logger.dartlib/src/misc/logger/web.dartlib/src/registry_api/registry.dart
🔇 Additional comments (14)
lib/src/driver_api/http_meta.dart (1)
5-5: LGTM: Improved immutability.Making the
methodfieldfinalis a good practice for data classes, preventing accidental mutations and making the intent clearer.lib/src/misc/logger/logger.dart (1)
4-5: LGTM: Clear deprecation path.The deprecation annotation provides a clear migration path to
LoggingCapabilityDelegate. This is a clean approach to guiding developers toward the new capability system.lib/src/misc/logger/io.dart (1)
9-10: LGTM: Consistent deprecation strategy.Deprecating
DefaultLoggeralongsideDebugLoggermaintains consistency in the migration path towardLoggingCapabilityDelegate.lib/src/capabilities/logging/index.dart (1)
1-2: LGTM: Proper platform-specific export structure.The conditional export correctly selects the platform-specific logging implementation (io.dart for native, web.dart for JS environments) while always exposing the common interface from logging_capability.dart.
lib/src/capabilities/index.dart (1)
5-7: LGTM: Clear capability API expansion.The new exports properly expose the transport, scripting, and logging capability delegates, expanding the public API surface as intended by the PR objectives.
lib/src/misc/logger/web.dart (1)
9-10: LGTM - Deprecation annotation is appropriate.The deprecation annotation correctly signals users to migrate to the new
LoggingCapabilityDelegatepattern. The class remains functional for backward compatibility during the transition period.lib/src/action_api/action_executor.dart (2)
13-14: LGTM - Deprecation properly signals migration path.The deprecated logger field is retained for backward compatibility while the implementation migrates to
driver.error().
62-66: LGTM - Logging migration looks correct.The migration from
logger?.error(...)todriver.error(...)with positional parameters aligns with the newLoggingCapabilityDelegateinterface signature.lib/src/action_api/event_resolver.dart (1)
108-112: LGTM - Catch block logging migration is correct.The error logging in the catch block correctly uses the new
driver.error()approach with proper positional parameters.lib/src/driver_api/ui_driver.dart (2)
7-15: LGTM - Capability delegate composition is well-structured.The UIDriver now correctly incorporates
TransportCapabilityDelegate,ScriptingCapabilityDelegate, andLoggingCapabilityDelegate. This enables thedriver.error()calls used in ActionExecutor and EventResolver to work via theLoggingCapabilityDelegatemixin.
45-46: LGTM - Deprecation consistent with migration strategy.The deprecated
loggerproperty maintains backward compatibility while signaling migration to the capability delegate pattern.lib/src/capabilities/scripting_capability.dart (1)
5-43: LGTM - Capability delegate pattern well implemented.The mixin provides a clear contract with
@mustBeOverriddenannotations and sensible defaults that throwMissingCapabilityMethodImplementation. This pattern catches incomplete implementations at runtime while providing clear error messages.lib/src/registry_api/registry.dart (2)
38-46: Logging calls correctly updated to new API.All logging statements have been properly migrated to use
LoggingCapabilityDelegate:
info()calls use single message parametererror()calls correctly pass exception and stack trace as separate parameters- The signatures align with the new delegate interface
Also applies to: 56-64
11-26: Well-structured migration with clear deprecation path.The migration from
DebugLoggertoLoggingCapabilityDelegateis well-executed:
- Provides a sensible default (
const LoggingManager())- Maintains backward compatibility with the deprecated
loggerparameter- Clear deprecation message directing users to the new API
LoggingManageris properly exported through the capabilities module and accessible to users across both IO and web platforms
Comment on lines
6
to
79
| final class LoggingManager with LoggingCapabilityDelegate { | ||
| const LoggingManager(); | ||
|
|
||
| @override | ||
| void critical(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] CRITICAL: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.error(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void debug(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] DEBUG: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.debug(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void error(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] ERROR: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.error(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void info(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] INFO: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.info(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void verbose(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] VERBOSE: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.log(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void warning(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] WARNING: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.warn(buff.toString().toJS); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting common log formatting logic.
All six methods share identical formatting logic with only the level prefix and console method differing. Extracting a private helper would improve maintainability.
♻️ Suggested refactor
final class LoggingManager with LoggingCapabilityDelegate {
const LoggingManager();
String _formatMessage(String level, message, Object? exception, StackTrace? stackTrace) {
final buff = StringBuffer("[DUIT FRAMEWORK] $level: $message \nTime: ${DateTime.now().toIso8601String()}");
if (exception != null) {
buff.write("\nException: ${exception.toString()}");
}
if (stackTrace != null) {
buff.write("\nStackTrace: ${stackTrace.toString()}");
}
return buff.toString();
}
@override
void critical(message, [Object? exception, StackTrace? stackTrace]) {
console.error(_formatMessage("CRITICAL", message, exception, stackTrace).toJS);
}
@override
void debug(message, [Object? exception, StackTrace? stackTrace]) {
console.debug(_formatMessage("DEBUG", message, exception, stackTrace).toJS);
}
// ... similar for other methods
}🤖 Prompt for AI Agents
In @lib/src/capabilities/logging/web.dart around lines 6 - 79, The six logging
methods in LoggingManager (critical, debug, error, info, verbose, warning)
duplicate the same message-building logic; extract a private helper (e.g.,
String _formatMessage(String level, message, Object? exception, StackTrace?
stackTrace)) that constructs the StringBuffer, appends exception and stackTrace
when non-null, and returns the final string, then replace each method to call
console.error/debug/info/log/warn with _formatMessage("LEVEL", message,
exception, stackTrace).toJS so only the level and console method differ.
* Replaced instances of `driver.error` with `driver.logError` for improved logging consistency. * Updated logging method names in `LoggingManager` and `LoggingCapabilityDelegate` to include the `log` prefix. * Adjusted logging calls in `DuitRegistry` to reflect the new method names.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Fix all issues with AI agents
In @lib/src/action_api/event_resolver.dart:
- Around line 47-50: The null check is inverted: currently it checks if
driver.externalEventHandler != null and then logs and throws about it being
unset; change the condition to if (driver.externalEventHandler == null) so the
error path only runs when the handler is missing, update the log via
driver.logError(...) and the thrown StateError(...) remain but will now execute
correctly when driver.externalEventHandler is null; locate this logic around the
externalEventHandler check in event_resolver.dart to apply the fix.
- Around line 58-61: The null-check for the external event handler is inverted:
in event_resolver.dart the block currently checks if
(driver.externalEventHandler != null) before calling driver.logError and
throwing StateError; change the condition to if (driver.externalEventHandler ==
null) so you only log and throw when the handler is missing (mirror the fix
applied for the NavigationEvent case); update the condition around
driver.externalEventHandler, keeping the driver.logError(...) and throw
StateError(...) calls intact.
In @lib/src/capabilities/logging/io.dart:
- Around line 7-77: All six methods (logCritical, logDebug, logError, logInfo,
logVerbose, logWarning) duplicate the same message-building and debugPrint
logic; extract that into a single private helper (e.g. _log(String level,
message, [Object? exception, StackTrace? stackTrace])) that builds the
StringBuffer with the "[DUIT FRAMEWORK] <LEVEL>: $message", timestamp, optional
exception and stackTrace, and calls debugPrint, then replace each public logX
method to simply call that helper with the appropriate level label.
In @lib/src/capabilities/logging/logging_capability.dart:
- Around line 9-99: The current LoggingCapabilityDelegate mixin defines concrete
methods (logInfo, logDebug, logWarning, logError, logCritical, logVerbose) that
throw MissingCapabilityMethodImplementation at runtime; change each to an
abstract method declaration (e.g., void logInfo(Object? message, [Object?
exception, StackTrace? stackTrace]);) by removing the implementation body, the
throw, and the @mustBeOverridden annotation so the compiler enforces
implementations for logInfo, logDebug, logWarning, logError, logCritical, and
logVerbose at compile time.
- Around line 15-23: The parameter "message" in logInfo is untyped (dynamic);
change its declaration to a typed parameter (e.g., Object? or String) in the
logInfo method to make the contract explicit and then apply the same type
annotation to the other logging methods (logDebug, logWarning, logError,
logCritical, logVerbose) in the LoggingCapabilityDelegate so all signatures are
consistent while keeping the thrown MissingCapabilityMethodImplementation
unchanged.
In @lib/src/capabilities/logging/web.dart:
- Around line 6-8: Remove the stray blank line after the const constructor in
the LoggingManager class declaration; locate the final class declaration "final
class LoggingManager with LoggingCapabilityDelegate" and its "const
LoggingManager();" constructor and delete the extra empty line that follows so
the constructor and subsequent code are consistently spaced.
- Around line 10-79: All six logging methods (logCritical, logDebug, logError,
logInfo, logVerbose, logWarning) duplicate the same string-building and console
call pattern; extract the shared logic into a private helper (e.g., _log) that
accepts the level label, the console function
(console.error/console.debug/console.info/console.log/console.warn), the
message, optional exception and stackTrace, builds the StringBuffer once,
appends exception/stackTrace when non-null (no explicit .toString needed) and
invokes consoleFn(buff.toString().toJS); then replace each public method to call
_log with the appropriate level and console function.
In @lib/src/driver_api/ui_driver.dart:
- Around line 46-47: The deprecated abstract field DebugLogger? logger forces
implementers to override it; change it to a non-abstract, nullable deprecated
getter that returns null by default (e.g., replace the abstract field
declaration with a getter for logger that returns null) so existing UIDriver
implementations compile without changes and callers can migrate to
LoggingCapabilityDelegate; update any references to the field name logger and
keep the @Deprecated annotation on the getter.
In @lib/src/registry_api/registry.dart:
- Around line 15-26: The initialize method currently ignores the deprecated
logger parameter causing silent breakage; modify initialize (method name:
initialize) to preserve backward compatibility by using the provided DebugLogger
logger when LoggingCapabilityDelegate logManager is null: if logManager is null
but logger is non-null, wrap or convert logger into a LoggingCapabilityDelegate
(or assign a simple adapter) and assign it to _logManager before proceeding, so
that callers passing the old logger still get logging configured; update places
referencing _logManager to use the adapter and ensure _componentRegistry.init()
runs after this assignment.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (9)
lib/src/action_api/action_executor.dartlib/src/action_api/event_resolver.dartlib/src/capabilities/index.dartlib/src/capabilities/logging/io.dartlib/src/capabilities/logging/logging_capability.dartlib/src/capabilities/logging/web.dartlib/src/capabilities/native_module_capability.dartlib/src/driver_api/ui_driver.dartlib/src/registry_api/registry.dart
🔇 Additional comments (6)
lib/src/capabilities/index.dart (1)
5-8: LGTM! Clean API surface extension.The new capability exports follow the existing pattern and properly expose the Transport, Scripting, Logging, and NativeModule capability delegates.
lib/src/action_api/action_executor.dart (2)
13-14: LGTM! Proper deprecation with clear migration path.The deprecation annotation correctly guides users to the new
LoggingCapabilityDelegatewhile maintaining backward compatibility.
62-66: LGTM! Consistent logging migration.All error logging calls have been properly migrated from the deprecated
logger?.errortodriver.logErrorwith correct positional parameter passing.Also applies to: 75-79, 101-105
lib/src/action_api/event_resolver.dart (2)
8-9: LGTM! Consistent deprecation pattern.The logger field deprecation matches the approach in ActionExecutor and provides clear migration guidance.
108-112: LGTM! Proper logging migration.The error logging has been correctly migrated to use
driver.logErrorwith positional parameters.lib/src/capabilities/native_module_capability.dart (1)
5-17: LGTM! Well-structured capability delegate.The
NativeModuleCapabilityDelegatemixin follows the established capability pattern with proper use of@mustBeOverriddenannotations and clear error messages for unimplemented methods.
Comment on lines
47
to
50
| if (driver.externalEventHandler != null) { | ||
| logger?.error("ExternalEventHandler instance is not set"); | ||
| driver.logError("ExternalEventHandler instance is not set"); | ||
| throw StateError("ExternalEventHandler instance is not set"); | ||
| } |
There was a problem hiding this comment.
Critical logic error: Inverted null check.
The condition checks if (driver.externalEventHandler != null) but then logs and throws an error stating the handler is NOT set. This logic is inverted—the error should only be thrown when the handler IS null.
🐛 Proposed fix
- if (driver.externalEventHandler != null) {
+ if (driver.externalEventHandler == null) {
driver.logError("ExternalEventHandler instance is not set");
throw StateError("ExternalEventHandler instance is not set");
}📝 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.
Suggested change
| if (driver.externalEventHandler != null) { | |
| logger?.error("ExternalEventHandler instance is not set"); | |
| driver.logError("ExternalEventHandler instance is not set"); | |
| throw StateError("ExternalEventHandler instance is not set"); | |
| } | |
| if (driver.externalEventHandler == null) { | |
| driver.logError("ExternalEventHandler instance is not set"); | |
| throw StateError("ExternalEventHandler instance is not set"); | |
| } |
🤖 Prompt for AI Agents
In @lib/src/action_api/event_resolver.dart around lines 47 - 50, The null check
is inverted: currently it checks if driver.externalEventHandler != null and then
logs and throws about it being unset; change the condition to if
(driver.externalEventHandler == null) so the error path only runs when the
handler is missing, update the log via driver.logError(...) and the thrown
StateError(...) remain but will now execute correctly when
driver.externalEventHandler is null; locate this logic around the
externalEventHandler check in event_resolver.dart to apply the fix.
Comment on lines
58
to
61
| if (driver.externalEventHandler != null) { | ||
| logger?.error("ExternalEventHandler instance is not set"); | ||
| driver.logError("ExternalEventHandler instance is not set"); | ||
| throw StateError("ExternalEventHandler instance is not set"); | ||
| } |
There was a problem hiding this comment.
Critical logic error: Inverted null check (duplicate issue).
Same inverted logic as in the NavigationEvent case. The condition should check if the handler IS null before logging and throwing an error.
🐛 Proposed fix
- if (driver.externalEventHandler != null) {
+ if (driver.externalEventHandler == null) {
driver.logError("ExternalEventHandler instance is not set");
throw StateError("ExternalEventHandler instance is not set");
}📝 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.
Suggested change
| if (driver.externalEventHandler != null) { | |
| logger?.error("ExternalEventHandler instance is not set"); | |
| driver.logError("ExternalEventHandler instance is not set"); | |
| throw StateError("ExternalEventHandler instance is not set"); | |
| } | |
| if (driver.externalEventHandler == null) { | |
| driver.logError("ExternalEventHandler instance is not set"); | |
| throw StateError("ExternalEventHandler instance is not set"); | |
| } |
🤖 Prompt for AI Agents
In @lib/src/action_api/event_resolver.dart around lines 58 - 61, The null-check
for the external event handler is inverted: in event_resolver.dart the block
currently checks if (driver.externalEventHandler != null) before calling
driver.logError and throwing StateError; change the condition to if
(driver.externalEventHandler == null) so you only log and throw when the handler
is missing (mirror the fix applied for the NavigationEvent case); update the
condition around driver.externalEventHandler, keeping the driver.logError(...)
and throw StateError(...) calls intact.
Comment on lines
+7
to
+77
| @override | ||
| void logCritical(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] CRITICAL: $message \nTime: ${DateTime.now().toUtc()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| debugPrint(buff.toString()); | ||
| } | ||
|
|
||
| @override | ||
| void logDebug(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] DEBUG: $message \nTime: ${DateTime.now().toUtc()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| debugPrint(buff.toString()); | ||
| } | ||
|
|
||
| @override | ||
| void logError(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] ERROR: $message \nTime: ${DateTime.now().toUtc()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| debugPrint(buff.toString()); | ||
| } | ||
|
|
||
| @override | ||
| void logInfo(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] INFO: $message \nTime: ${DateTime.now().toUtc()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| debugPrint(buff.toString()); | ||
| } | ||
|
|
||
| @override | ||
| void logVerbose(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] VERBOSE: $message \nTime: ${DateTime.now().toUtc()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| debugPrint(buff.toString()); | ||
| } | ||
|
|
||
| @override | ||
| void logWarning(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] WARNING: $message \nTime: ${DateTime.now().toUtc()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| debugPrint(buff.toString()); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract common logging logic to reduce duplication.
All six logging methods follow an identical pattern, differing only in the log level label. This creates significant code duplication (~66 lines) that could be reduced to ~20 lines by extracting a common helper method.
♻️ Proposed refactor
final class LoggingManager with LoggingCapabilityDelegate {
const LoggingManager();
+ void _log(String level, message, [Object? exception, StackTrace? stackTrace]) {
+ final buff = StringBuffer("[DUIT FRAMEWORK] $level: $message \nTime: ${DateTime.now().toUtc()}");
+ if (exception != null) {
+ buff.write("\nException: ${exception.toString()}");
+ }
+ if (stackTrace != null) {
+ buff.write("\nStackTrace: ${stackTrace.toString()}");
+ }
+ debugPrint(buff.toString());
+ }
+
@override
- void logCritical(message, [Object? exception, StackTrace? stackTrace]) {
- final buff = StringBuffer("[DUIT FRAMEWORK] CRITICAL: $message \nTime: ${DateTime.now().toUtc()}");
- if (exception != null) {
- buff.write("\nException: ${exception.toString()}");
- }
- if (stackTrace != null) {
- buff.write("\nStackTrace: ${stackTrace.toString()}");
- }
- debugPrint(buff.toString());
- }
+ void logCritical(message, [Object? exception, StackTrace? stackTrace]) =>
+ _log("CRITICAL", message, exception, stackTrace);
@override
- void logDebug(message, [Object? exception, StackTrace? stackTrace]) {
- final buff = StringBuffer("[DUIT FRAMEWORK] DEBUG: $message \nTime: ${DateTime.now().toUtc()}");
- if (exception != null) {
- buff.write("\nException: ${exception.toString()}");
- }
- if (stackTrace != null) {
- buff.write("\nStackTrace: ${stackTrace.toString()}");
- }
- debugPrint(buff.toString());
- }
+ void logDebug(message, [Object? exception, StackTrace? stackTrace]) =>
+ _log("DEBUG", message, exception, stackTrace);
@override
- void logError(message, [Object? exception, StackTrace? stackTrace]) {
- final buff = StringBuffer("[DUIT FRAMEWORK] ERROR: $message \nTime: ${DateTime.now().toUtc()}");
- if (exception != null) {
- buff.write("\nException: ${exception.toString()}");
- }
- if (stackTrace != null) {
- buff.write("\nStackTrace: ${stackTrace.toString()}");
- }
- debugPrint(buff.toString());
- }
+ void logError(message, [Object? exception, StackTrace? stackTrace]) =>
+ _log("ERROR", message, exception, stackTrace);
@override
- void logInfo(message, [Object? exception, StackTrace? stackTrace]) {
- final buff = StringBuffer("[DUIT FRAMEWORK] INFO: $message \nTime: ${DateTime.now().toUtc()}");
- if (exception != null) {
- buff.write("\nException: ${exception.toString()}");
- }
- if (stackTrace != null) {
- buff.write("\nStackTrace: ${stackTrace.toString()}");
- }
- debugPrint(buff.toString());
- }
+ void logInfo(message, [Object? exception, StackTrace? stackTrace]) =>
+ _log("INFO", message, exception, stackTrace);
@override
- void logVerbose(message, [Object? exception, StackTrace? stackTrace]) {
- final buff = StringBuffer("[DUIT FRAMEWORK] VERBOSE: $message \nTime: ${DateTime.now().toUtc()}");
- if (exception != null) {
- buff.write("\nException: ${exception.toString()}");
- }
- if (stackTrace != null) {
- buff.write("\nStackTrace: ${stackTrace.toString()}");
- }
- debugPrint(buff.toString());
- }
+ void logVerbose(message, [Object? exception, StackTrace? stackTrace]) =>
+ _log("VERBOSE", message, exception, stackTrace);
@override
- void logWarning(message, [Object? exception, StackTrace? stackTrace]) {
- final buff = StringBuffer("[DUIT FRAMEWORK] WARNING: $message \nTime: ${DateTime.now().toUtc()}");
- if (exception != null) {
- buff.write("\nException: ${exception.toString()}");
- }
- if (stackTrace != null) {
- buff.write("\nStackTrace: ${stackTrace.toString()}");
- }
- debugPrint(buff.toString());
- }
+ void logWarning(message, [Object? exception, StackTrace? stackTrace]) =>
+ _log("WARNING", message, exception, stackTrace);
}📝 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.
Suggested change
| @override | |
| void logCritical(message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] CRITICAL: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| @override | |
| void logDebug(message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] DEBUG: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| @override | |
| void logError(message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] ERROR: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| @override | |
| void logInfo(message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] INFO: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| @override | |
| void logVerbose(message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] VERBOSE: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| @override | |
| void logWarning(message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] WARNING: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| final class LoggingManager with LoggingCapabilityDelegate { | |
| const LoggingManager(); | |
| void _log(String level, message, [Object? exception, StackTrace? stackTrace]) { | |
| final buff = StringBuffer("[DUIT FRAMEWORK] $level: $message \nTime: ${DateTime.now().toUtc()}"); | |
| if (exception != null) { | |
| buff.write("\nException: ${exception.toString()}"); | |
| } | |
| if (stackTrace != null) { | |
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | |
| } | |
| debugPrint(buff.toString()); | |
| } | |
| @override | |
| void logCritical(message, [Object? exception, StackTrace? stackTrace]) => | |
| _log("CRITICAL", message, exception, stackTrace); | |
| @override | |
| void logDebug(message, [Object? exception, StackTrace? stackTrace]) => | |
| _log("DEBUG", message, exception, stackTrace); | |
| @override | |
| void logError(message, [Object? exception, StackTrace? stackTrace]) => | |
| _log("ERROR", message, exception, stackTrace); | |
| @override | |
| void logInfo(message, [Object? exception, StackTrace? stackTrace]) => | |
| _log("INFO", message, exception, stackTrace); | |
| @override | |
| void logVerbose(message, [Object? exception, StackTrace? stackTrace]) => | |
| _log("VERBOSE", message, exception, stackTrace); | |
| @override | |
| void logWarning(message, [Object? exception, StackTrace? stackTrace]) => | |
| _log("WARNING", message, exception, stackTrace); | |
| } |
🤖 Prompt for AI Agents
In @lib/src/capabilities/logging/io.dart around lines 7 - 77, All six methods
(logCritical, logDebug, logError, logInfo, logVerbose, logWarning) duplicate the
same message-building and debugPrint logic; extract that into a single private
helper (e.g. _log(String level, message, [Object? exception, StackTrace?
stackTrace])) that builds the StringBuffer with the "[DUIT FRAMEWORK] <LEVEL>:
$message", timestamp, optional exception and stackTrace, and calls debugPrint,
then replace each public logX method to simply call that helper with the
appropriate level label.
Comment on lines
+9
to
+99
| mixin LoggingCapabilityDelegate { | ||
| /// Logs an informational message. | ||
| /// | ||
| /// Use this method to log general informational messages about the | ||
| /// application's state or flow. | ||
| @mustBeOverridden | ||
| void logInfo( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "info", | ||
| "LoggingCapabilityDelegate", | ||
| ); | ||
|
|
||
| /// Logs a debug message. | ||
| /// | ||
| /// Use this method to log detailed debugging information that is typically | ||
| /// only useful during development. | ||
| @mustBeOverridden | ||
| void logDebug( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "debug", | ||
| "LoggingCapabilityDelegate", | ||
| ); | ||
|
|
||
| /// Logs a warning message. | ||
| /// | ||
| /// Use this method to log warnings about potential issues or unexpected | ||
| /// conditions that don't prevent the application from functioning. | ||
| @mustBeOverridden | ||
| void logWarning( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "warning", | ||
| "LoggingCapabilityDelegate", | ||
| ); | ||
|
|
||
| /// Logs an error message. | ||
| /// | ||
| /// Use this method to log error messages that indicate problems or failures | ||
| /// in the application. | ||
| @mustBeOverridden | ||
| void logError( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "error", | ||
| "LoggingCapabilityDelegate", | ||
| ); | ||
|
|
||
| /// Logs a critical error message. | ||
| /// | ||
| /// Use this method to log critical errors that require immediate attention | ||
| /// and may indicate serious problems in the application. | ||
| @mustBeOverridden | ||
| void logCritical( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "critical", | ||
| "LoggingCapabilityDelegate", | ||
| ); | ||
|
|
||
| /// Logs a verbose message. | ||
| /// | ||
| /// Use this method to log very detailed information that is typically only | ||
| /// useful for deep debugging or troubleshooting. | ||
| @mustBeOverridden | ||
| void logVerbose( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "verbose", | ||
| "LoggingCapabilityDelegate", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using abstract methods instead of throwing implementations.
The mixin uses concrete methods that throw MissingCapabilityMethodImplementation. While this provides runtime checking, Dart allows abstract methods in mixins (since 2.14). Abstract methods would provide compile-time checking and clearer intent. However, if runtime flexibility is needed (e.g., partially implementing the mixin), the current approach is acceptable.
Alternative approach using abstract methods
mixin LoggingCapabilityDelegate {
/// Logs an informational message.
void logInfo(Object? message, [Object? exception, StackTrace? stackTrace]);
/// Logs a debug message.
void logDebug(Object? message, [Object? exception, StackTrace? stackTrace]);
// ... etc for all methods
}This would enforce implementation at compile-time rather than runtime.
🤖 Prompt for AI Agents
In @lib/src/capabilities/logging/logging_capability.dart around lines 9 - 99,
The current LoggingCapabilityDelegate mixin defines concrete methods (logInfo,
logDebug, logWarning, logError, logCritical, logVerbose) that throw
MissingCapabilityMethodImplementation at runtime; change each to an abstract
method declaration (e.g., void logInfo(Object? message, [Object? exception,
StackTrace? stackTrace]);) by removing the implementation body, the throw, and
the @mustBeOverridden annotation so the compiler enforces implementations for
logInfo, logDebug, logWarning, logError, logCritical, and logVerbose at compile
time.
Comment on lines
+15
to
+23
| void logInfo( | ||
| message, [ | ||
| Object? exception, | ||
| StackTrace? stackTrace, | ||
| ]) => | ||
| throw const MissingCapabilityMethodImplementation( | ||
| "info", | ||
| "LoggingCapabilityDelegate", | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add type annotation to the message parameter.
The message parameter lacks a type annotation, making it implicitly dynamic. This reduces type safety and could lead to runtime issues if non-stringifiable objects are passed. Consider using Object? or String to make the contract explicit.
♻️ Suggested fix
@mustBeOverridden
void logInfo(
- message, [
+ Object? message, [
Object? exception,
StackTrace? stackTrace,
]) =>Apply the same change to all logging methods (logDebug, logWarning, logError, logCritical, logVerbose).
🤖 Prompt for AI Agents
In @lib/src/capabilities/logging/logging_capability.dart around lines 15 - 23,
The parameter "message" in logInfo is untyped (dynamic); change its declaration
to a typed parameter (e.g., Object? or String) in the logInfo method to make the
contract explicit and then apply the same type annotation to the other logging
methods (logDebug, logWarning, logError, logCritical, logVerbose) in the
LoggingCapabilityDelegate so all signatures are consistent while keeping the
thrown MissingCapabilityMethodImplementation unchanged.
Comment on lines
+6
to
+8
| final class LoggingManager with LoggingCapabilityDelegate { | ||
| const LoggingManager(); | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Inconsistent spacing after class declaration.
There's an extra blank line after the const constructor at line 8. While minor, consistent formatting improves readability.
🤖 Prompt for AI Agents
In @lib/src/capabilities/logging/web.dart around lines 6 - 8, Remove the stray
blank line after the const constructor in the LoggingManager class declaration;
locate the final class declaration "final class LoggingManager with
LoggingCapabilityDelegate" and its "const LoggingManager();" constructor and
delete the extra empty line that follows so the constructor and subsequent code
are consistently spaced.
Comment on lines
+10
to
+19
| void logCritical(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] CRITICAL: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.error(buff.toString().toJS); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
logCritical and logError both use console.error.
Both logCritical and logError use console.error(), making them functionally identical in the browser console. Consider using a more prominent output method for critical errors, such as also triggering a visual alert or using a distinct prefix that stands out more clearly.
Alternatively, if browser console limitations mean they must use the same method, document this behavior to set expectations for users debugging in the browser console.
Also applies to: 34-43
Comment on lines
+10
to
+79
| void logCritical(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] CRITICAL: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.error(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void logDebug(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] DEBUG: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.debug(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void logError(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] ERROR: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.error(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void logInfo(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] INFO: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.info(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void logVerbose(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] VERBOSE: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.log(buff.toString().toJS); | ||
| } | ||
|
|
||
| @override | ||
| void logWarning(message, [Object? exception, StackTrace? stackTrace]) { | ||
| final buff = StringBuffer("[DUIT FRAMEWORK] WARNING: $message \nTime: ${DateTime.now().toIso8601String()}"); | ||
| if (exception != null) { | ||
| buff.write("\nException: ${exception.toString()}"); | ||
| } | ||
| if (stackTrace != null) { | ||
| buff.write("\nStackTrace: ${stackTrace.toString()}"); | ||
| } | ||
| console.warn(buff.toString().toJS); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
High code duplication across logging methods.
All six logging methods follow an identical pattern, differing only in the log level prefix and console method. Consider extracting the common logic to a helper method to improve maintainability.
♻️ Refactored approach
void _log(
String level,
void Function(JSString) consoleFn,
Object? message, [
Object? exception,
StackTrace? stackTrace,
]) {
final buff = StringBuffer("[DUIT FRAMEWORK] $level: $message \nTime: ${DateTime.now().toIso8601String()}");
if (exception != null) {
buff.write("\nException: $exception");
}
if (stackTrace != null) {
buff.write("\nStackTrace: $stackTrace");
}
consoleFn(buff.toString().toJS);
}
@override
void logInfo(message, [Object? exception, StackTrace? stackTrace]) {
_log("INFO", console.info, message, exception, stackTrace);
}
@override
void logError(message, [Object? exception, StackTrace? stackTrace]) {
_log("ERROR", console.error, message, exception, stackTrace);
}
// ... etcNote: The .toString() calls on exception and stackTrace are also redundant since string interpolation handles this automatically.
🤖 Prompt for AI Agents
In @lib/src/capabilities/logging/web.dart around lines 10 - 79, All six logging
methods (logCritical, logDebug, logError, logInfo, logVerbose, logWarning)
duplicate the same string-building and console call pattern; extract the shared
logic into a private helper (e.g., _log) that accepts the level label, the
console function
(console.error/console.debug/console.info/console.log/console.warn), the
message, optional exception and stackTrace, builds the StringBuffer once,
appends exception/stackTrace when non-null (no explicit .toString needed) and
invokes consoleFn(buff.toString().toJS); then replace each public method to call
_log with the appropriate level and console function.
Comment on lines
+46
to
47
| @Deprecated("Use [LoggingCapabilityDelegate] instead") | ||
| abstract DebugLogger? logger; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making the deprecated logger field nullable and provide a default implementation.
The abstract deprecated field forces all UIDriver implementations to provide a logger even though it's deprecated. This creates unnecessary migration friction. Consider making it non-abstract with a default null value, or provide a getter that returns null by default, allowing implementers to opt out of the deprecated API immediately.
♻️ Suggested approach
@Deprecated("Use [LoggingCapabilityDelegate] instead")
DebugLogger? get logger => null;This allows existing implementations to compile without changes while new implementations can safely ignore the deprecated member.
🤖 Prompt for AI Agents
In @lib/src/driver_api/ui_driver.dart around lines 46 - 47, The deprecated
abstract field DebugLogger? logger forces implementers to override it; change it
to a non-abstract, nullable deprecated getter that returns null by default
(e.g., replace the abstract field declaration with a getter for logger that
returns null) so existing UIDriver implementations compile without changes and
callers can migrate to LoggingCapabilityDelegate; update any references to the
field name logger and keep the @Deprecated annotation on the getter.
Comment on lines
15
to
26
| static FutureOr<void> initialize({ | ||
| @Deprecated("Use [LoggingCapabilityDelegate] and [logManager] instead") | ||
| DebugLogger? logger, | ||
| DuitTheme? theme, | ||
| ComponentRegistry? componentRegistry, | ||
| LoggingCapabilityDelegate? logManager, | ||
| }) async { | ||
| _logger = logger ?? _logger; | ||
| _componentRegistry = componentRegistry ?? _componentRegistry; | ||
| _theme = theme ?? _theme; | ||
| _logManager = logManager ?? _logManager; | ||
| await _componentRegistry.init(); | ||
| } |
There was a problem hiding this comment.
Breaking change: Deprecated logger parameter is ignored.
The deprecated logger parameter in initialize() is completely ignored. Existing code passing a logger will silently fail to configure logging, which is a breaking change disguised as a deprecation.
To maintain backward compatibility during the deprecation period, you should fall back to the old logger if logManager is not provided:
🔧 Suggested fix for backward compatibility
static FutureOr<void> initialize({
@Deprecated("Use [LoggingCapabilityDelegate] and [logManager] instead")
DebugLogger? logger,
DuitTheme? theme,
ComponentRegistry? componentRegistry,
LoggingCapabilityDelegate? logManager,
}) async {
_componentRegistry = componentRegistry ?? _componentRegistry;
_theme = theme ?? _theme;
- _logManager = logManager ?? _logManager;
+ _logManager = logManager ?? (logger != null ? _adaptLogger(logger) : _logManager);
await _componentRegistry.init();
}
+
+ static LoggingCapabilityDelegate _adaptLogger(DebugLogger logger) {
+ // Create an adapter that wraps the old logger
+ // Or document that users must provide logManager explicitly
+ }Alternatively, if true backward compatibility is not required, document this as a breaking change in the PR description.
🤖 Prompt for AI Agents
In @lib/src/registry_api/registry.dart around lines 15 - 26, The initialize
method currently ignores the deprecated logger parameter causing silent
breakage; modify initialize (method name: initialize) to preserve backward
compatibility by using the provided DebugLogger logger when
LoggingCapabilityDelegate logManager is null: if logManager is null but logger
is non-null, wrap or convert logger into a LoggingCapabilityDelegate (or assign
a simple adapter) and assign it to _logManager before proceeding, so that
callers passing the old logger still get logging configured; update places
referencing _logManager to use the adapter and ensure _componentRegistry.init()
runs after this assignment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.