fix(logger): Reduce Sentry noise with targeted error logging (ADR-0076)#4467
fix(logger): Reduce Sentry noise with targeted error logging (ADR-0076)#4467
Conversation
…teExceptionThrower (ADR-0076)
…dEmailExceptionThrower (ADR-0076)
|
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:
WalkthroughSentry initializer now imports dart:io and configures the Sentry SDK to ignore SocketException types; its 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/lib/utils/sentry/sentry_initializer.dart (1)
84-103:⚠️ Potential issue | 🟠 MajorDocstring describes event-filtering behavior that the implementation doesn't perform.
The doc comment claims
_beforeSendHandlerwill "drop events unless tagged as auth-critical" and provides anextras: {'auth_critical': true}usage example. However, the actual implementation only sanitizes headers and deminifies exceptions—there is no check forevent.tags['auth_critical']orevent.extra['auth_critical'], and the method returns all events unconditionally.No call sites in the codebase currently use this pattern, so the promise hasn't been relied upon yet. This mismatch still risks confusion for developers reading the docs expecting event filtering that won't occur.
Since this PR references ADR-0078 for event filtering logic, either:
- Move the auth-critical filtering description to ADR-0078's implementation, or
- Defer the allowlist documentation and simplify the comment to describe only what currently runs: header sanitization and exception deminification.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/lib/utils/sentry/sentry_initializer.dart` around lines 84 - 103, The docstring for _beforeSendHandler claims event filtering/allowlisting for auth-critical events but the implementation only calls _sanitizeRequest and _deminifyExceptions and returns the event; update the comment to accurately describe current behavior by removing the allowlist/filtering text and the extras example (or reference ADR-0078 if you prefer to document filtering there), and keep only a short description that this handler sanitizes request headers via _sanitizeRequest and deminifies exceptions via _deminifyExceptions before returning the event.
🧹 Nitpick comments (1)
lib/main/exceptions/thrower/remote_exception_thrower.dart (1)
70-100: Minor: redundantUnknownRemoteExceptionconstruction and redundantstatusCodeparameter.Two small cleanups:
- In the
defaultbranch the sameUnknownRemoteException(code: statusCode, message: response.statusMessage)is constructed twice — once aslogError'sexception:argument and once as the thrown value. Build it once and reuse._handleDioResponseErrorreceives bothstatusCodeandresponse, butstatusCodeis justresponse.statusCode. Passing onlyresponsekeeps the contract tighter and avoids any future drift between the two.♻️ Proposed refactor
- if (response != null) { - return _handleDioResponseError(statusCode, response); - } + if (response != null) { + return _handleDioResponseError(response); + } @@ - void _handleDioResponseError(int? statusCode, Response response) { + void _handleDioResponseError(Response response) { + final statusCode = response.statusCode; switch (statusCode) { case HttpStatus.unauthorized: // 401 is handled by auth retry flow — no log needed throw const BadCredentialsException(); case HttpStatus.internalServerError: logWarning('RemoteExceptionThrower: HTTP 500'); throw const InternalServerError(); case HttpStatus.badGateway: logWarning('RemoteExceptionThrower: HTTP 502'); throw BadGateway(); default: + final exception = UnknownRemoteException( + code: statusCode, + message: response.statusMessage, + ); if (statusCode != null && statusCode >= 400 && statusCode < 500) { logWarning('RemoteExceptionThrower: HTTP 4xx ($statusCode)'); } else if (statusCode != null && statusCode >= 500) { logWarning('RemoteExceptionThrower: HTTP 5xx ($statusCode)'); } else { - logError( - 'RemoteExceptionThrower: unknown HTTP status $statusCode', - exception: UnknownRemoteException( - code: statusCode, - message: response.statusMessage, - ), - ); + logError( + 'RemoteExceptionThrower: unknown HTTP status $statusCode', + exception: exception, + ); } - throw UnknownRemoteException( - code: statusCode, - message: response.statusMessage, - ); + throw exception; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/main/exceptions/thrower/remote_exception_thrower.dart` around lines 70 - 100, Refactor _handleDioResponseError to accept only the Dio Response (remove the redundant statusCode parameter) and inside the function compute finalStatus = response.statusCode; in the default branch construct a single UnknownRemoteException once (e.g., final ex = UnknownRemoteException(code: finalStatus, message: response.statusMessage)), pass ex to logError(..., exception: ex) and then throw ex; update any callers of _handleDioResponseError to pass the Response object only, and keep existing switch cases (HttpStatus.unauthorized, HttpStatus.internalServerError, HttpStatus.badGateway) unchanged while switching on finalStatus.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/main/exceptions/thrower/remote_exception_thrower.dart`:
- Around line 51-68: The log in _handleDioException currently interpolates the
full DioException.response (which serialises headers and body) — change the
logWarning call in _handleDioException to only include non-sensitive
discriminants needed for routing such as error.type, response?.statusCode, and a
minimal request identifier like response?.requestOptions?.path or method, and
never include response?.data or response.toString(); keep full response/body
logging only inside the branch handlers (_handleDioResponseError or
_handleDioErrorWithoutResponse) if those handlers decide richer context is safe,
and preserve the existing RefreshTokenFailedException short-circuit behavior.
---
Outside diff comments:
In `@core/lib/utils/sentry/sentry_initializer.dart`:
- Around line 84-103: The docstring for _beforeSendHandler claims event
filtering/allowlisting for auth-critical events but the implementation only
calls _sanitizeRequest and _deminifyExceptions and returns the event; update the
comment to accurately describe current behavior by removing the
allowlist/filtering text and the extras example (or reference ADR-0078 if you
prefer to document filtering there), and keep only a short description that this
handler sanitizes request headers via _sanitizeRequest and deminifies exceptions
via _deminifyExceptions before returning the event.
---
Nitpick comments:
In `@lib/main/exceptions/thrower/remote_exception_thrower.dart`:
- Around line 70-100: Refactor _handleDioResponseError to accept only the Dio
Response (remove the redundant statusCode parameter) and inside the function
compute finalStatus = response.statusCode; in the default branch construct a
single UnknownRemoteException once (e.g., final ex =
UnknownRemoteException(code: finalStatus, message: response.statusMessage)),
pass ex to logError(..., exception: ex) and then throw ex; update any callers of
_handleDioResponseError to pass the Response object only, and keep existing
switch cases (HttpStatus.unauthorized, HttpStatus.internalServerError,
HttpStatus.badGateway) unchanged while switching on finalStatus.
🪄 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: 0546413f-4366-4fb0-af9d-57af2f0d04b3
📒 Files selected for processing (3)
core/lib/utils/sentry/sentry_initializer.dartlib/main/exceptions/thrower/remote_exception_thrower.dartlib/main/exceptions/thrower/send_email_exception_thrower.dart
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4467. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…code complexity (ADR-0078) Extract DioResponseErrorHandler and DioNoResponseErrorHandler to separate files, reducing aggregate branch count in RemoteExceptionThrower per CodeScene ADR-0078.
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: 2
🧹 Nitpick comments (1)
lib/main/exceptions/thrower/dio_response_error_handler.dart (1)
9-46: LGTM on the response-status split.Logging levels align with the ADR-0076 intent (401 silent, 4xx/5xx warnings, truly unknown → error). Nullable
statusCodeis safely funneled to_logUnhandledStatusCode, which logs as error only when the code is null or outside 4xx/5xx — appropriate.Minor nit:
BadGateway()at Line 21 isn't markedconstwhileInternalServerError()at Line 18 is — worth making both const ifBadGateway's constructor allows it, for consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/main/exceptions/thrower/dio_response_error_handler.dart` around lines 9 - 46, The BadGateway instantiation in DioResponseErrorHandler.handle should be made const for consistency with InternalServerError (i.e., change BadGateway() to const BadGateway()) if the BadGateway class has a const constructor; update the call in the switch case that currently throws BadGateway() so both 500 and 502 branches use const constructors, leaving other logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/main/exceptions/thrower/dio_no_response_error_handler.dart`:
- Around line 11-24: The switch in RemoteExceptionThrower.handle currently sends
DioExceptionType.sendTimeout and receiveTimeout to the default path which ends
up calling _handleUnknownUnderlying/_throwUnknownRemoteException and logError;
update the switch in dio_no_response_error_handler.dart to treat sendTimeout and
receiveTimeout the same as connectionTimeout by grouping them into the same
case, calling logWarning('RemoteExceptionThrower: connection timeout') and
throwing ConnectionTimeout(message: error.message) so timeouts are handled as
expected and do not trigger logError/Sentry noise.
- Around line 19-20: The switch branch that maps DioExceptionType.badResponse to
BadCredentialsException is incorrect for the null-response path; remove the
explicit case in dio_no_response_error_handler.dart (the "case
DioExceptionType.badResponse: throw const BadCredentialsException();" in the
handler) so the code falls through to the existing _handleUnknownUnderlying
logic (or explicitly call _handleUnknownUnderlying(...) there) instead, ensuring
we do not silently trigger the auth-retry path for an unknown/edge state and
that the condition is logged/handled by the unknown-error flow referenced by
DioResponseErrorHandler/_handleUnknownUnderlying.
---
Nitpick comments:
In `@lib/main/exceptions/thrower/dio_response_error_handler.dart`:
- Around line 9-46: The BadGateway instantiation in
DioResponseErrorHandler.handle should be made const for consistency with
InternalServerError (i.e., change BadGateway() to const BadGateway()) if the
BadGateway class has a const constructor; update the call in the switch case
that currently throws BadGateway() so both 500 and 502 branches use const
constructors, leaving other logic unchanged.
🪄 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: 6be1f6fe-def7-48e6-ba9e-1d473cde0a2d
📒 Files selected for processing (3)
lib/main/exceptions/thrower/dio_no_response_error_handler.dartlib/main/exceptions/thrower/dio_response_error_handler.dartlib/main/exceptions/thrower/remote_exception_thrower.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/main/exceptions/thrower/remote_exception_thrower.dart
…ndler (ADR-0078) - Group sendTimeout/receiveTimeout with connectionTimeout to avoid logError noise for normal network conditions (fixes Sentry regression risk) - Remove badResponse → BadCredentialsException mapping when response is null, which was silently triggering auth-retry for genuinely unknown states
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 `@lib/main/exceptions/thrower/dio_no_response_error_handler.dart`:
- Around line 20-22: The default branch currently forwards
DioExceptionType.cancel into
_handleUnknownUnderlying/_throwUnknownRemoteException which calls logError;
change the switch (or add a case) to explicitly handle DioExceptionType.cancel
by calling logWarning (or suppressing logs) and throwing a dedicated
cancellation exception similar to the timeout path (mirror the behavior used for
timeout handling), i.e., add a case for DioExceptionType.cancel that avoids
logError and invokes the same known exception type or a new
CancelledRequestException so cancellations do not generate Sentry error noise;
update references in _handleUnknownUnderlying/_throwUnknownRemoteException usage
as needed.
🪄 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: 80d24f30-f502-43aa-9f31-b79fc792cded
📒 Files selected for processing (1)
lib/main/exceptions/thrower/dio_no_response_error_handler.dart
| logWarning('RemoteExceptionThrower: connection error'); | ||
| throw ConnectionError(message: error.message); | ||
| default: | ||
| _handleUnknownUnderlying(error.error); |
There was a problem hiding this comment.
should put also error.stackTrace
| _throwUnknownRemoteException(underlyingError); | ||
| } | ||
|
|
||
| void _throwUnknownRemoteException(dynamic underlyingError) { |
There was a problem hiding this comment.
should put also error.stackTrace
| code: statusCode, | ||
| message: response.statusMessage, | ||
| ); | ||
| _logUnhandledStatusCode(statusCode, exception); |
| @@ -37,70 +29,50 @@ class RemoteExceptionThrower extends ExceptionThrower { | |||
|
|
|||
| void handleDioError(dynamic error) { | |||
There was a problem hiding this comment.
| void handleDioError(dynamic error) { | |
| void handleDioError(dynamic error, [StackTrace? stackTrace]) { |
|
|
||
| final response = error.response; | ||
| final statusCode = response?.statusCode; | ||
| logError( |
There was a problem hiding this comment.
try to put stack trace in every logError
Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
(1 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| remote_exception_thrower.dart | 9.01 → 10.00 | Complex Method, Bumpy Road Ahead |
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.
2 participants
Context
Implements the call-site fixes from ADR-0076.
Requires ADR-0078 PR to be merged first.
What changed
RemoteExceptionThrower
logErrorat the top ofthrowExceptionNoNetwork,ConnectionTimeout,ConnectionError,SocketException) →logWarninglogWarninglogErrorSendEmailExceptionThrower
logErrorfor no realtime network →logWarning(network loss is a normal user-facing condition)
SentryInitializer
SocketExceptiontoignoredExceptionsForType(second line of defence against accidental
logErrorregressions at call sites)Blocker
#4466
Summary by CodeRabbit
Bug Fixes
Refactor
Chores