Refactor(logger): Introduce handler pipeline (ADR-0078) #4466
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR replaces the previous in-file logging logic with a handler-based logging pipeline. It adds LogRecord, Level enum, LogHandler interface, AppLoggerRegistry singleton dispatcher, LogFormatter interface with Web/Mobile implementations, Console/Sentry handlers, and a buildLogRecord helper. app_logger.dart now forwards public log functions to AppLoggerRegistry. Sentry integration gains a SentryReporter interface and SentryManager implements it with breadcrumb support. Runners register platform-appropriate formatters and handlers. Tests cover registry, formatters, and Sentry handlers. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
core/lib/utils/sentry/sentry_manager.dart (1)
108-117:⚠️ Potential issue | 🟠 MajorError/critical logs without an exception are silently reported to Sentry as
info.
SentryReporter.captureMessage(core/lib/utils/sentry/sentry_reporter.dart:13-16) does not expose alevel, andSentryEventHandler.handle(core/lib/utils/logging/handlers/sentry_event_handler.dart:33-37) invokes it through that interface without passing one. Because the override here defaultsleveltoSentryLevel.info, everylogError/logCriticalthat lacks an exception becomes aninfoevent on Sentry — losing severity, alerting, and issue-grouping fidelity. This is a behavior regression vs. the previous monolithic logger and contradicts the intent stated in the PR description.Either add
levelto theSentryReportercontract and mapLevel.error/Level.criticalinSentryEventHandler, or have the handler call a dedicated reporter method that encodes the severity.🔧 Proposed direction
In
sentry_reporter.dart:void captureMessage( String message, { + SentryLevel level = SentryLevel.info, Map<String, dynamic>? extras, });In
sentry_event_handler.dart:} else { _sentry.captureMessage( record.rawMessage, + level: record.level == Level.critical + ? SentryLevel.fatal + : SentryLevel.error, extras: record.extras, ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/lib/utils/sentry/sentry_manager.dart` around lines 108 - 117, The override captureMessage in sentry_manager.dart accepts a SentryLevel but SentryReporter.captureMessage (and SentryEventHandler.handle) do not pass a level, causing messages without exceptions to be sent as SentryLevel.info; fix by adding an optional SentryLevel parameter to the SentryReporter.captureMessage contract (and its implementations) and update SentryEventHandler.handle to map the logging Level to the correct SentryLevel (e.g., Level.error -> SentryLevel.error, Level.critical -> SentryLevel.fatal/critical) and pass that into SentryReporter.captureMessage; keep the default fallback to SentryLevel.info where callers don't supply one and update any call sites to match the new signature.core/lib/utils/app_logger.dart (1)
11-76:⚠️ Potential issue | 🟡 Minor
webConsoleEnabledparameter is silently ignored at call sites — confirmed semantic regression.The parameter is accepted on every public function but never forwarded to
_dispatch, meaning it no longer reachesConsoleLogHandler. This is a breaking change: at least threelogErrorcalls inauthorization_interceptors.dart(lines 176, 259, 290) and onelogInfocall insentry_config.dart(line 87) explicitly passwebConsoleEnabled: trueexpecting those logs to appear in the production web console. With the new design, these calls silently have no effect.Choose one approach:
- Forward
webConsoleEnabledthrough_dispatchintoLogRecordand haveConsoleLogHandlerOR it with the global setting, restoring per-call override capability.- Remove the parameter entirely and add
@Deprecatedto the old public functions, with a migration guide for callers to use globalregisterLogHandlersconfiguration only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/lib/utils/app_logger.dart` around lines 11 - 76, The public log helpers (logError, logCritical, logWarning, logInfo, logDebug, logTrace, log) currently accept webConsoleEnabled but never forward it to _dispatch so ConsoleLogHandler never sees per-call overrides; fix by threading webConsoleEnabled through: add a webConsoleEnabled parameter to _dispatch and to the LogRecord payload it creates, update all public helpers (logError, logCritical, logWarning, logInfo, logDebug, logTrace, log) to pass their webConsoleEnabled argument into _dispatch, and update ConsoleLogHandler to OR the incoming LogRecord.webConsoleEnabled with the global web console setting when deciding whether to write to the web console. Ensure signatures reference _dispatch, LogRecord, and ConsoleLogHandler are consistently updated and adjust any callers/tests accordingly.
🧹 Nitpick comments (1)
core/lib/utils/logging/log_record.dart (1)
9-17: Consider defensive copying forextrasif API constraints allow.The concern about mutable map contents is valid—
finalonly prevents reassignment, not mutation of the map itself. However, theconstconstructor is a hard requirement based on active usage in tests. Removingconstto implementMap.unmodifiable()would be a breaking change. The actual mutation risk appears low sinceextrasis only passed through handlers without modification, but if the publicconstconstructor can be removed in a future breaking release, defensive copying would improve true immutability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/lib/utils/logging/log_record.dart` around lines 9 - 17, Keep the existing const LogRecord constructor unchanged for backwards compatibility but add a new non-const factory (e.g., LogRecord.withDefensiveExtras or LogRecord.defensive) that defensively wraps any provided extras with Map.unmodifiable(...) before constructing the LogRecord; implement the factory to delegate to a private (non-const) constructor or to the existing constructor after wrapping, and document that callers who want true immutability should use the new factory while tests and existing call sites can continue to use the const constructor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/lib/utils/logging/handlers/sentry_event_handler.dart`:
- Around line 21-38: Update the Sentry reporting API and handler to propagate
severity: modify the SentryReporter interface so both captureMessage(...) and
captureException(...) accept an additional SentryLevel parameter, update
SentryManager.captureException(...) (and captureMessage(...) if needed) to
accept and forward that level to Sentry, then change
SentryEventHandler.handle(LogRecord) to map Level.critical → SentryLevel.fatal
and Level.error → SentryLevel.error and pass the mapped SentryLevel into the
SentryManager calls (both exception and non-exception branches). Finally, update
the tests in sentry_event_handler_test.dart and
sentry_breadcrumb_handler_test.dart to call the new interface signatures and
assert the forwarded SentryLevel.
In `@core/test/utils/logging/app_logger_registry_test.dart`:
- Around line 45-87: The tests mistakenly only register one handler instance
where they intend to exercise type-based deduplication and multi-handler
dispatch; declare a second named fake subclass (e.g. _SecondFakeHandler or
_FakeHandlerAlt) at file scope and use it in the two failing tests: in the
"registers distinct handler types separately" test register both
registry.registerHandler(_FakeHandler(...)) and
registry.registerHandler(_SecondFakeHandler(...)) and assert
registry.handlerCount == 2, and in the "dispatches to multiple handlers when
both accept the level" test register two different handler types (h1 =
_FakeHandler(), h2 = _SecondFakeHandler()), call registry.dispatch(record) and
assert both h1.received and h2.received have length 1 (and their rawMessage
matches), leaving existing idempotency test unchanged.
In `@core/test/utils/logging/handlers/sentry_breadcrumb_handler_test.dart`:
- Around line 7-19: The fake reporter no-ops for captureException/captureMessage
so "does not call" tests aren't observable; update _FakeSentryReporter to either
record calls or fail fast: implement captureException and captureMessage to
append to new lists (e.g., capturedExceptions, capturedMessages) or throw an
AssertionError when invoked, and keep addBreadcrumb behavior; reference the
_FakeSentryReporter class and the methods captureException, captureMessage,
addBreadcrumb and the existing capturedBreadcrumbs to locate where to add
capturedExceptions/capturedMessages (or the throw) so tests can reliably detect
unwanted calls.
---
Outside diff comments:
In `@core/lib/utils/app_logger.dart`:
- Around line 11-76: The public log helpers (logError, logCritical, logWarning,
logInfo, logDebug, logTrace, log) currently accept webConsoleEnabled but never
forward it to _dispatch so ConsoleLogHandler never sees per-call overrides; fix
by threading webConsoleEnabled through: add a webConsoleEnabled parameter to
_dispatch and to the LogRecord payload it creates, update all public helpers
(logError, logCritical, logWarning, logInfo, logDebug, logTrace, log) to pass
their webConsoleEnabled argument into _dispatch, and update ConsoleLogHandler to
OR the incoming LogRecord.webConsoleEnabled with the global web console setting
when deciding whether to write to the web console. Ensure signatures reference
_dispatch, LogRecord, and ConsoleLogHandler are consistently updated and adjust
any callers/tests accordingly.
In `@core/lib/utils/sentry/sentry_manager.dart`:
- Around line 108-117: The override captureMessage in sentry_manager.dart
accepts a SentryLevel but SentryReporter.captureMessage (and
SentryEventHandler.handle) do not pass a level, causing messages without
exceptions to be sent as SentryLevel.info; fix by adding an optional SentryLevel
parameter to the SentryReporter.captureMessage contract (and its
implementations) and update SentryEventHandler.handle to map the logging Level
to the correct SentryLevel (e.g., Level.error -> SentryLevel.error,
Level.critical -> SentryLevel.fatal/critical) and pass that into
SentryReporter.captureMessage; keep the default fallback to SentryLevel.info
where callers don't supply one and update any call sites to match the new
signature.
---
Nitpick comments:
In `@core/lib/utils/logging/log_record.dart`:
- Around line 9-17: Keep the existing const LogRecord constructor unchanged for
backwards compatibility but add a new non-const factory (e.g.,
LogRecord.withDefensiveExtras or LogRecord.defensive) that defensively wraps any
provided extras with Map.unmodifiable(...) before constructing the LogRecord;
implement the factory to delegate to a private (non-const) constructor or to the
existing constructor after wrapping, and document that callers who want true
immutability should use the new factory while tests and existing call sites can
continue to use the const constructor.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fb5bbc7-d7c2-4300-93ed-370da8aa367e
📒 Files selected for processing (21)
core/lib/utils/app_logger.dartcore/lib/utils/logging/app_logger_registry.dartcore/lib/utils/logging/formatters/log_formatter.dartcore/lib/utils/logging/formatters/mobile_console_formatter.dartcore/lib/utils/logging/formatters/web_console_formatter.dartcore/lib/utils/logging/handlers/console_log_handler.dartcore/lib/utils/logging/handlers/sentry_breadcrumb_handler.dartcore/lib/utils/logging/handlers/sentry_event_handler.dartcore/lib/utils/logging/log_handler.dartcore/lib/utils/logging/log_level.dartcore/lib/utils/logging/log_record.dartcore/lib/utils/sentry/sentry_manager.dartcore/lib/utils/sentry/sentry_reporter.dartcore/test/utils/logging/app_logger_registry_test.dartcore/test/utils/logging/formatters/mobile_console_formatter_test.dartcore/test/utils/logging/formatters/web_console_formatter_test.dartcore/test/utils/logging/handlers/sentry_breadcrumb_handler_test.dartcore/test/utils/logging/handlers/sentry_event_handler_test.dartlib/main/runner/app_runner_base.dartlib/main/runner/app_runner_mobile.dartlib/main/runner/app_runner_web.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4466. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/lib/utils/sentry/sentry_manager.dart (2)
143-164: Consider propagating log level/category on the breadcrumb.
Breadcrumbis created with onlymessageanddata, which means every breadcrumb produced via the handler pipeline will land in Sentry with the default level (info) and no category. SinceSentryBreadcrumbHandleris the trace-level sink per ADR-0078, surfacing that aslevel: SentryLevel.debug(and optionally acategorylike'log'or the record's logger name) would make the breadcrumb trail on error events meaningfully more useful without changing quota characteristics. Happy to defer if you prefer to keep Phase 1–3 minimal.♻️ Suggested change (requires plumbing level/category through the method signature)
- void addBreadcrumb( - String message, { - Map<String, dynamic>? extras, - }) { + void addBreadcrumb( + String message, { + Map<String, dynamic>? extras, + SentryLevel level = SentryLevel.debug, + String? category, + }) { if (!_isSentryAvailable) return; try { Sentry.addBreadcrumb( Breadcrumb( message: message, data: extras, + level: level, + category: category, ), ); } catch (e) { logWarning('[SentryManager] Add breadcrumb failed: $e'); } }Note: this also requires updating the
SentryReporterinterface andSentryBreadcrumbHandlercallsite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/lib/utils/sentry/sentry_manager.dart` around lines 143 - 164, The addBreadcrumb method in SentryManager currently creates Breadcrumbs with only message and data, so they all arrive as default level/info and no category; update addBreadcrumb (and the SentryReporter interface and SentryBreadcrumbHandler callsite) to accept and pass a SentryLevel (e.g., SentryLevel.debug for trace-level logs) and an optional category/logger name, then construct Breadcrumb with level: providedLevel and category: providedCategory before calling Sentry.addBreadcrumb, preserving the try/catch and _isSentryAvailable check.
113-117: AligncaptureMessagenamed-parameter order with theSentryReporterinterface.The interface declares
captureMessage(String, {extras, level})(no default forlevel), while this implementation declares{level = SentryLevel.info, extras}. Dart allows this for named parameters, but the divergence in both order and default makes the contract harder to read and invites future confusion (e.g. when someone compares the two signatures to verify they match). Consider aligning the order (and either dropping the default here or adding a matching one in the interface) so the interface and implementation are obviously congruent.♻️ Proposed alignment
`@override` void captureMessage( String message, { - SentryLevel level = SentryLevel.info, Map<String, dynamic>? extras, + SentryLevel level = SentryLevel.info, }) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/lib/utils/sentry/sentry_manager.dart` around lines 113 - 117, The implementation of captureMessage differs from the SentryReporter interface by swapping the named-parameter order and providing a default for level; update the implementation to match the interface signature exactly by changing captureMessage to accept named parameters in the same order ({Map<String, dynamic>? extras, SentryLevel? level}) and remove the SentryLevel.info default, and then run a quick search for any call sites that relied on the previous default and adjust them if necessary; reference captureMessage and the SentryReporter interface when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@core/lib/utils/sentry/sentry_manager.dart`:
- Around line 143-164: The addBreadcrumb method in SentryManager currently
creates Breadcrumbs with only message and data, so they all arrive as default
level/info and no category; update addBreadcrumb (and the SentryReporter
interface and SentryBreadcrumbHandler callsite) to accept and pass a SentryLevel
(e.g., SentryLevel.debug for trace-level logs) and an optional category/logger
name, then construct Breadcrumb with level: providedLevel and category:
providedCategory before calling Sentry.addBreadcrumb, preserving the try/catch
and _isSentryAvailable check.
- Around line 113-117: The implementation of captureMessage differs from the
SentryReporter interface by swapping the named-parameter order and providing a
default for level; update the implementation to match the interface signature
exactly by changing captureMessage to accept named parameters in the same order
({Map<String, dynamic>? extras, SentryLevel? level}) and remove the
SentryLevel.info default, and then run a quick search for any call sites that
relied on the previous default and adjust them if necessary; reference
captureMessage and the SentryReporter interface when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d1e242ba-2644-44be-8a38-948d0fea09a9
📒 Files selected for processing (7)
core/lib/utils/app_logger.dartcore/lib/utils/logging/handlers/sentry_event_handler.dartcore/lib/utils/sentry/sentry_manager.dartcore/lib/utils/sentry/sentry_reporter.dartcore/test/utils/logging/app_logger_registry_test.dartcore/test/utils/logging/handlers/sentry_breadcrumb_handler_test.dartcore/test/utils/logging/handlers/sentry_event_handler_test.dart
✅ Files skipped from review due to trivial changes (1)
- core/test/utils/logging/handlers/sentry_event_handler_test.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- core/lib/utils/sentry/sentry_reporter.dart
- core/test/utils/logging/handlers/sentry_breadcrumb_handler_test.dart
- core/lib/utils/logging/handlers/sentry_event_handler.dart
- core/test/utils/logging/app_logger_registry_test.dart
- core/lib/utils/app_logger.dart
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/lib/utils/logging/app_logger_registry.dart`:
- Around line 35-38: The loop in app_logger_registry.dart that iterates over
_handlers and calls handler.handle(record) can let one handler's exception abort
other handlers and bubble out of the central logging path; wrap each invocation
of handler.handle(record) in its own try/catch so a failing handler doesn't stop
dispatch or crash callers, and on catch report the failure (including the caught
error and stacktrace) to a safe fallback (e.g., a dedicated internal logger or
Zone.current.handleUncaughtError) but continue to the next handler; update the
for (final handler in _handlers) { if (handler.handles(record))
handler.handle(record); } block accordingly to isolate handler failures while
preserving the original record delivery semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 88c70148-9b21-463b-9516-46aa1727414a
📒 Files selected for processing (13)
core/lib/utils/app_logger.dartcore/lib/utils/logging/app_logger_registry.dartcore/lib/utils/logging/handlers/console_log_handler.dartcore/lib/utils/logging/handlers/sentry_breadcrumb_handler.dartcore/lib/utils/logging/log_record.dartcore/lib/utils/sentry/sentry_config.dartcore/lib/utils/sentry/sentry_manager.dartcore/lib/utils/sentry/sentry_reporter.dartcore/test/utils/logging/app_logger_registry_test.dartcore/test/utils/logging/formatters/web_console_formatter_test.dartcore/test/utils/logging/handlers/sentry_breadcrumb_handler_test.dartlib/main/runner/app_runner_base.dartlib/main/runner/app_runner_web.dart
💤 Files with no reviewable changes (1)
- core/lib/utils/sentry/sentry_config.dart
✅ Files skipped from review due to trivial changes (3)
- core/lib/utils/sentry/sentry_reporter.dart
- core/lib/utils/logging/handlers/console_log_handler.dart
- core/test/utils/logging/app_logger_registry_test.dart
🚧 Files skipped from review as they are similar to previous changes (4)
- lib/main/runner/app_runner_web.dart
- lib/main/runner/app_runner_base.dart
- core/test/utils/logging/formatters/web_console_formatter_test.dart
- core/lib/utils/logging/log_record.dart
…rmatters (ADR-0078)
feat(logger): add SentryEventHandler and SentryBreadcrumbHandler (ADR-0078)
…-0078) refactor(logger): register Sentry handlers at startup for mobile and web (ADR-0078)
SentryReporter.captureException and captureMessage now accept a SentryLevel parameter so handlers can forward the mapped severity. SentryEventHandler maps Level.critical → fatal, Level.error → error. SentryManager forwards the level via scope.level.
Web console output is now configured globally via ConsoleLogHandler at startup. The parameter is retained for source compatibility.
- app_logger_registry: add _OtherFakeHandler subclass; fix 'dispatches to multiple handlers' to register two distinct types and assert both receive the record; use const LogRecord - sentry_breadcrumb_handler: track capturedExceptions/capturedMessages in fake; assert both are empty in 'does not call' test - sentry_event_handler: add SentryLevel to fake captured records; assert mapped severity in captureException/captureMessage tests
webConsoleEnabled was deprecated in favour of a global constructor flag, but that removed the ability to force specific log calls visible on web in release/profile mode — a legitimate need for auth and Sentry init diagnostics. Carry webConsoleEnabled through the full pipeline: LogRecord → buildLogRecord → _dispatch → public log functions. ConsoleLogHandler reads the flag from the record rather than from its constructor, so per-call opt-in works independently of build mode. On web: - handles() always returns true (filter deferred to handle()) - handle() prints only when record.webConsoleEnabled || isDebugMode Existing call sites (authorization_interceptors, sentry_config) that pass webConsoleEnabled: true are preserved and functional again.
…essage signature
- addBreadcrumb gains level (default SentryLevel.debug) and category
params in SentryReporter interface and SentryManager implementation;
SentryBreadcrumbHandler passes level: SentryLevel.debug, category: 'log'
so breadcrumbs on error events are clearly labelled in Sentry UI
- captureMessage param order aligned to {extras, level} in SentryManager
to match the SentryReporter interface declaration
- Test: _FakeSentryReporter updated; new test case asserts level/category
- Wrap each handler.handle() in try/catch so a failing handler does not abort the remaining handlers or bubble exceptions to callers - Fix _FakeSentryReporter.addBreadcrumb signature to match SentryReporter interface (add level and category named parameters)
dab246
force-pushed
the
feat/adr-0078-logger-pipeline-skeleton
branch
from
09c28cf to
c0303f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/lib/utils/sentry/sentry_manager.dart (1)
10-10: LGTM — cleanSentryReporterimplementation.The interface adoption,
levelpropagation intoscope.levelfor both capture paths, and the newaddBreadcrumbwith explicit recursion-safety guidance in the catch block all look correct and match theSentryReportercontract (parameter orderextrasbeforelevel).One thing worth double-checking:
logWarningin theaddBreadcrumbcatch (Line 168) is safe today only because no registered handler subscribes towarningvia breadcrumb/event paths. If a future handler ever mapswarning→Sentry.addBreadcrumborSentry.captureMessage, a throwing Sentry SDK here could recurse. The inline note at Lines 166–167 captures this well; consider also asserting it in a test (e.g., a throwingSentryReporterfake that records no re-entry) to lock the invariant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/lib/utils/logging/app_logger_registry.dart`:
- Around line 13-15: The doc comment incorrectly states that registerHandler is
a no-op for duplicate handler registrations; update the comment in
AppLoggerRegistry to say that registerHandler checks by runtimeType and will
replace any existing handler of the same type (rather than preserving it) to
reflect current behavior and avoid relying on stale instances.
- Around line 44-54: The dispatch method iterates the mutable _handlers list
directly which can throw ConcurrentModificationError if a handler
registers/unregisters during handling; modify dispatch (in
AppLoggerRegistry.dispatch) to first snapshot the handlers (e.g., final handlers
= List.of(_handlers)) and iterate that snapshot so mutations during
handler.handle/handler.handles (or calls to registerHandler/unregisterHandler)
won't cause iteration to fail; keep the existing per-handler try/catch behavior
and only change the iteration source to the copied list.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0cbdd090-075d-410b-bdc5-5e54d7d393dd
📒 Files selected for processing (2)
core/lib/utils/logging/app_logger_registry.dartcore/lib/utils/sentry/sentry_manager.dart
Add handlerKey to LogHandler so that two instances of the same concrete class can coexist in AppLoggerRegistry (e.g. two ConsoleLogHandlers with different formatters, or two Sentry handlers targeting different DSNs). Default key is runtimeType.toString(), preserving existing dedup behaviour for handlers that do not override it. Also add @VisibleForTesting on resetForTesting() and convert LogHandler from abstract interface class to abstract class so the default handlerKey implementation is inherited without requiring each implementor to redeclare it.
…verage - handlerKey: verify two instances of same class with distinct keys coexist, same key replaces existing handler, both handlers receive dispatched records - fault-isolation: verify handles()/handle() throwing does not crash dispatch and remaining handlers still receive the record - critical level: verify Level.critical maps to SentryLevel.fatal in both captureException and captureMessage paths of SentryEventHandler
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…entation via acceptsLevel()
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
3 participants
Context
Implements Phase 1–3 of ADR-0078.
app_logger.dartpreviously handled formatting, console output, Sentry routing, and exception capture in a single_internalLogfunction — untestable and non-extensible. This PR replaces it with an explicit handler pipeline where each log destination is an isolated, injectable unit.What changed
LogRecord/LogHandler/AppLoggerRegistry— core pipeline abstractionsConsoleLogHandler— console output with injected platform formatter (mobile: emoji, web: ANSI)SentryEventHandler—error/critical→captureExceptionorcaptureMessageSentryBreadcrumbHandler—trace→addBreadcrumb(zero quota, attached to next error event)SentryReporter— interface extracted fromSentryManagerfor handler testabilityapp_logger.dartpublic API — unchanged, all 188+ call sites unaffectedregisterLogHandlers()— single entry point for handler registration at startupBehavior changes
logTracecaptureMessage— consumes Sentry quotaaddBreadcrumb— zero quotalogErrorwithout exceptionSummary by CodeRabbit
New Features
Tests