TF-4136 Add sentry for web #4137
Conversation
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
189ed2d to
d7bcfe3
Compare
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4137. |
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
4a26f0b to
f782f91
Compare
|
Please document the expected linagora ecosystem values expected by the front. Cc @Arsnael |
cf linagora/tmail-backend#1988 , thanks |
It seems to be more TMail web configs instead cf Likely in Helm, we need to configure it as we did for the FCM env file. |
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
373121e to
1396d2d
Compare
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
296d882 to
6f63b14
Compare
|
This PR acts as a base from which related changes are split into smaller PRs for easier review and merging: |
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
ea5b1f1 to
47253be
Compare
dab246
changed the base branch from
master
to
improvement/tf-4136-re-standardize-the-logger
|
@coderabbitai please review |
|
@dab246 I'll review the changes in this PR for you. I can see this is the base PR for the Sentry integration, and you've already started splitting it into smaller PRs like #4205 to address the review concerns about the PR size. Let me analyze all the changes related to the Sentry integration for web error tracking. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
fcm/pubspec.yaml (1)
25-25: LGTM - Dio upgrade supports Sentry integration.The dio upgrade to 5.2.0 is necessary for sentry_dio integration and aligns with the repo-wide dependency update.
server_settings/pubspec.yaml (1)
24-24: LGTM - Dio upgrade supports Sentry integration.The dio upgrade to 5.2.0 is necessary for sentry_dio integration and aligns with the repo-wide dependency update.
forward/pubspec.yaml (1)
25-25: LGTM - Dio upgrade supports Sentry integration.The dio upgrade to 5.2.0 is necessary for sentry_dio integration and aligns with the repo-wide dependency update.
contact/pubspec.yaml (1)
28-28: LGTM - Dio upgrade supports Sentry integration.The dio upgrade to 5.2.0 is necessary for sentry_dio integration and aligns with the repo-wide dependency update.
rule_filter/pubspec.yaml (1)
25-25: LGTM - Dio upgrade supports Sentry integration.The dio upgrade to 5.2.0 is necessary for sentry_dio integration and aligns with the repo-wide dependency update.
email_recovery/pubspec.yaml (1)
25-25: LGTM - Dio upgrade supports Sentry integration.The dio upgrade to 5.2.0 is necessary for sentry_dio integration and aligns with the repo-wide dependency update.
🧹 Nitpick comments (9)
lib/features/base/base_controller.dart (1)
554-559: EnsureSentryManager.clearUser()is safe when Sentry is disabled/uninitializedHooking Sentry user clearing into
clearDataAndGoToLoginPage()is the right place. Just double‑check thatSentryManager.instance.clearUser()internally handles the “Sentry not initialized / disabled / misconfigured” case as a no‑op so that logout/data‑clear flows can never fail because of Sentry.docs/configuration/sentry_configuration.md (1)
7-29: Minor clarity tweak forSENTRY_ENABLEDexample (optional)The line
SENTRY_ENABLED=true | falseis correct but could be misread as a literal value. Consider a short note like “set totrueto enable,falseto disable” or two separate examples so it’s obvious onlytrue(lowercase) actually enables Sentry in the current implementation.core/lib/utils/sentry/sentry_initializer.dart (1)
6-32: Sentry initialization wiring looks solid;isAvailablereturn is currently redundantThe integration with
SentryFlutter.init(DSN, environment, release, tracing, breadcrumbs,beforeSend, etc.) is well structured. With the currentSentryConfig.load()behavior, any successful load already guaranteesconfig.isAvailable == true, so theboolreturn is effectively alwaystruefor now—but it’s still useful if you later changeload()to represent a “configured but disabled” state instead of throwing.core/lib/utils/sentry/sentry_config.dart (1)
47-73: Consider representing “Sentry disabled” viaisAvailableinstead of throwingThe env parsing and validation are correct, but treating
SENTRY_ENABLED != 'true'as an exception means callers must use try/catch for the normal “disabled” configuration, and anySentryConfiginstance you return will always haveisAvailable == true. A more flexible pattern would be to haveload()return a config withisAvailable = false(and maybe empty DSN) when Sentry is disabled, and let the initializer skipSentryFlutter.initin that case—while still throwing for truly invalid/missing DSN or environment. This isn’t blocking but would simplify control flow and make theisAvailableflag more meaningful.lib/features/login/data/network/interceptors/authorization_interceptors.dart (1)
85-168: Preserve stack traces when wrapping errors intoDioExceptionThe
onError(DioException err, ...)migration and retry logic look correct, but in the catch block the wrapped errors lose their stack:if (e is ServerError || e is TemporarilyUnavailable) { return super.onError( DioException(requestOptions: err.requestOptions, error: e), handler, ); } else { return super.onError(err.copyWith(error: e), handler); }Consider preserving the stack trace (and, if relevant, type/response) so callers and Sentry get full diagnostics, e.g.:
- if (e is ServerError || e is TemporarilyUnavailable) { - return super.onError( - DioException(requestOptions: err.requestOptions, error: e), - handler, - ); - } else { - return super.onError(err.copyWith(error: e), handler); - } + if (e is ServerError || e is TemporarilyUnavailable) { + return super.onError( + DioException( + requestOptions: err.requestOptions, + error: e, + stackTrace: stackTrace, + type: err.type, + response: err.response, + ), + handler, + ); + } else { + return super.onError( + err.copyWith(error: e, stackTrace: stackTrace), + handler, + ); + }This keeps network consumers and Sentry aligned with the original failure context.
lib/main/app_runner.dart (1)
1-49: Centralized monitored startup looks good; watch preloading + error handler overlapThe new
runAppWithMonitoringnicely centralizes error handling (FlutterError.onError,PlatformDispatcher.onError,runZonedGuarded) and delegates Sentry startup viaSentryManager.instance.initialize, which aligns with the updated logger.Two things to verify:
If
initializefalls back tofallBackRunner,runTmailPreload()will have already run inappRunnerand then run again insiderunTmail(). Make sure all preload steps (Hive, env, Cozy, etc.) are idempotent, or consider a fallback that just mountsTMailAppwithout re‑preloading.Given
logErrornow reports to Sentry, double‑check thatSentryManager.initializedoes not also hook the same framework error channels in a way that leads to duplicate Sentry events for a single failure.lib/main/main_entry.dart (1)
14-34: Bootstrap helpers are clear; consider makingrunTmailPreloadfully self‑contained
runTmail/runTmailPreloadnicely centralize app bootstrap (DI, Hive, executor warm‑up, env, Cozy, URL strategy). Two optional tweaks you may want to consider:
Call
WidgetsFlutterBinding.ensureInitialized()at the start ofrunTmailPreload()so it’s safe to use standalone (e.g. from tests) without relying on external callers to have initialized Flutter bindings.Gate
setPathUrlStrategy()behindPlatformInfo.isWebfor clarity, even ifurl_strategyis effectively a no‑op on non‑web platforms.core/lib/utils/app_logger.dart (1)
3-4: Error/WTF logs now go to Sentry; behavior matches platform logging rulesRouting all
logError/logWTFcalls through_internalLogintoSentryManager.instance.captureException(...)is a good fit with the earlier decision to use Sentry as a side effect of error‑level logs. The platform behavior still matches the documented rules: mobile logs only in debug via_debugPrint+BuildUtils.isDebugMode, and web can opt into production console output viawebConsoleEnabled.Two things to keep in mind:
SentryManager.captureExceptionshould be a no‑op (or otherwise safe) when Sentry is disabled or not yet initialized, so that logging from tests or early startup doesn’t throw.With
runAppWithMonitoringalso feeding framework errors intologError, verify that Sentry’s own integrations (insideSentryManager.initialize) are not additionally capturing the same events, or you may see duplicates in Sentry for a single failure.Also applies to: 50-75, 124-126
core/lib/utils/config/env_loader.dart (1)
22-35: Async callback not awaited due toVoidCallbacktype.The
onCallBackparameter is typed asVoidCallback(syncvoid Function()), but the actual callback passed at line 16-18 is async. WhenonCallBack?.call()executes on line 33, the returnedFutureis silently dropped, soloadConfigFromEnv()may not complete before execution continues.If the callback must complete before proceeding, change the signature:
static Future<void> loadFcmConfigFileToEnv({ Map<String, String>? currentMapEnvData, - VoidCallback? onCallBack, + Future<void> Function()? onCallBack, }) async { try { await dotenv.load( fileName: appFCMConfigurationPath, mergeWith: currentMapEnvData ?? {}, ); } catch (e) { logWarning('EnvLoader::loadFcmConfigFileToEnv: Exception = $e'); - onCallBack?.call(); + await onCallBack?.call(); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (9)
contact/pubspec.lockis excluded by!**/*.lockcore/pubspec.lockis excluded by!**/*.lockemail_recovery/pubspec.lockis excluded by!**/*.lockfcm/pubspec.lockis excluded by!**/*.lockforward/pubspec.lockis excluded by!**/*.lockmodel/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lockrule_filter/pubspec.lockis excluded by!**/*.lockserver_settings/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (51)
contact/pubspec.yaml(1 hunks)contact/test/contact/autocomplete_tmail_contact_method_test.dart(0 hunks)core/lib/core.dart(2 hunks)core/lib/utils/app_logger.dart(4 hunks)core/lib/utils/application_manager.dart(2 hunks)core/lib/utils/config/env_loader.dart(1 hunks)core/lib/utils/sentry/sentry_config.dart(1 hunks)core/lib/utils/sentry/sentry_initializer.dart(1 hunks)core/lib/utils/sentry/sentry_manager.dart(1 hunks)core/pubspec.yaml(2 hunks)core/test/utils/application_manager_test.dart(2 hunks)docs/configuration/sentry_configuration.md(1 hunks)email_recovery/pubspec.yaml(1 hunks)email_recovery/test/method/get_email_recovery_action_method_test.dart(0 hunks)email_recovery/test/method/set_email_recovery_action_method_test.dart(0 hunks)env.file(1 hunks)fcm/pubspec.yaml(1 hunks)fcm/test/method/firebase_registration_get_method_test.dart(0 hunks)fcm/test/method/firebase_registration_set_method_test.dart(0 hunks)forward/pubspec.yaml(1 hunks)forward/test/forward/get_forward_method_test.dart(0 hunks)forward/test/forward/set_forward_method_test.dart(0 hunks)integration_test/base/test_base.dart(2 hunks)lib/features/base/base_controller.dart(1 hunks)lib/features/login/data/network/interceptors/authorization_interceptors.dart(2 hunks)lib/features/login/data/network/oidc_http_client.dart(3 hunks)lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart(4 hunks)lib/features/manage_account/presentation/manage_account_dashboard_controller.dart(2 hunks)lib/features/upload/data/network/file_uploader.dart(1 hunks)lib/features/upload/domain/model/upload_attachment.dart(1 hunks)lib/main.dart(1 hunks)lib/main/app_runner.dart(1 hunks)lib/main/bindings/core/core_bindings.dart(0 hunks)lib/main/bindings/network/network_bindings.dart(2 hunks)lib/main/bindings/network/network_isolate_binding.dart(2 hunks)lib/main/exceptions/remote_exception_thrower.dart(2 hunks)lib/main/main_entry.dart(1 hunks)lib/main/utils/app_config.dart(0 hunks)lib/main/utils/app_utils.dart(1 hunks)lib/main/utils/toast_manager.dart(1 hunks)pubspec.yaml(2 hunks)rule_filter/pubspec.yaml(1 hunks)rule_filter/test/rule_filter/get_rule_filter_method_test.dart(0 hunks)rule_filter/test/rule_filter/set_rule_filter_method_test.dart(0 hunks)server_settings/pubspec.yaml(1 hunks)server_settings/test/server_settings/get/get_server_settings_method_test.dart(0 hunks)server_settings/test/server_settings/set/set_server_settings_method_test.dart(0 hunks)test/features/interceptor/authorization_interceptor_test.dart(4 hunks)test/features/login/data/network/oidc_http_client_test.dart(3 hunks)test/features/server_settings/data/network/server_settings_api_test.dart(0 hunks)test/features/thread/data/network/thread_api_test.dart(4 hunks)
💤 Files with no reviewable changes (14)
- email_recovery/test/method/get_email_recovery_action_method_test.dart
- fcm/test/method/firebase_registration_get_method_test.dart
- contact/test/contact/autocomplete_tmail_contact_method_test.dart
- server_settings/test/server_settings/get/get_server_settings_method_test.dart
- test/features/server_settings/data/network/server_settings_api_test.dart
- forward/test/forward/set_forward_method_test.dart
- lib/main/bindings/core/core_bindings.dart
- fcm/test/method/firebase_registration_set_method_test.dart
- forward/test/forward/get_forward_method_test.dart
- server_settings/test/server_settings/set/set_server_settings_method_test.dart
- lib/main/utils/app_config.dart
- email_recovery/test/method/set_email_recovery_action_method_test.dart
- rule_filter/test/rule_filter/get_rule_filter_method_test.dart
- rule_filter/test/rule_filter/set_rule_filter_method_test.dart
🧰 Additional context used
🧠 Learnings (8)
📓 Common learnings
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:50.713Z
Learning: The tmail-flutter codebase uses `DioError` consistently across all modules, even with Dio 5.0.0. Do not suggest replacing `DioError` with `DioException` unless it's part of a codebase-wide migration.
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
lib/features/login/data/network/interceptors/authorization_interceptors.dartintegration_test/base/test_base.dartlib/features/upload/data/network/file_uploader.dartlib/features/manage_account/presentation/manage_account_dashboard_controller.dartlib/features/upload/domain/model/upload_attachment.dartcore/lib/utils/sentry/sentry_manager.dartlib/main/app_runner.dartlib/main/exceptions/remote_exception_thrower.dartcore/lib/utils/sentry/sentry_config.darttest/features/thread/data/network/thread_api_test.dartcore/lib/utils/app_logger.dartlib/features/login/data/network/oidc_http_client.dartlib/main/bindings/network/network_isolate_binding.dartlib/features/base/base_controller.dartlib/main/main_entry.dartlib/main/bindings/network/network_bindings.dartcore/lib/utils/sentry/sentry_initializer.dartcore/lib/core.dartlib/main/utils/toast_manager.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartcore/lib/utils/config/env_loader.darttest/features/interceptor/authorization_interceptor_test.darttest/features/login/data/network/oidc_http_client_test.dartlib/main.dartcore/test/utils/application_manager_test.dartcore/lib/utils/application_manager.dartlib/main/utils/app_utils.dart
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
lib/features/login/data/network/interceptors/authorization_interceptors.dartintegration_test/base/test_base.dartlib/features/upload/data/network/file_uploader.dartlib/features/manage_account/presentation/manage_account_dashboard_controller.dartlib/features/upload/domain/model/upload_attachment.dartcore/lib/utils/sentry/sentry_manager.dartlib/main/app_runner.dartlib/main/exceptions/remote_exception_thrower.dartcore/lib/utils/sentry/sentry_config.darttest/features/thread/data/network/thread_api_test.dartcore/lib/utils/app_logger.dartlib/features/login/data/network/oidc_http_client.dartlib/main/bindings/network/network_isolate_binding.dartlib/features/base/base_controller.dartlib/main/main_entry.dartlib/main/bindings/network/network_bindings.dartcore/lib/utils/sentry/sentry_initializer.dartcore/lib/core.dartlib/main/utils/toast_manager.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartcore/lib/utils/config/env_loader.darttest/features/interceptor/authorization_interceptor_test.darttest/features/login/data/network/oidc_http_client_test.dartlib/main.dartcore/test/utils/application_manager_test.dartcore/lib/utils/application_manager.dartlib/main/utils/app_utils.dart
📚 Learning: 2025-12-09T12:47:50.713Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:50.713Z
Learning: The tmail-flutter codebase uses `DioError` consistently across all modules, even with Dio 5.0.0. Do not suggest replacing `DioError` with `DioException` unless it's part of a codebase-wide migration.
Applied to files:
server_settings/pubspec.yamlrule_filter/pubspec.yamlfcm/pubspec.yamlemail_recovery/pubspec.yamlpubspec.yamlcore/pubspec.yamlforward/pubspec.yamlcontact/pubspec.yaml
📚 Learning: 2025-12-12T11:00:57.625Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4205
File: core/lib/utils/app_logger.dart:80-86
Timestamp: 2025-12-12T11:00:57.625Z
Learning: In the tmail-flutter codebase, logging philosophy differs by platform: web applications use the `webConsoleEnabled` parameter in `app_logger.dart` to selectively display logs in production console, while mobile applications intentionally suppress all logs in release builds (enforced by `BuildUtils.isDebugMode` check in `_debugPrint`).
Applied to files:
lib/main/app_runner.dartlib/main/main_entry.dartlib/main.dart
📚 Learning: 2025-12-12T11:00:52.914Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4205
File: core/lib/utils/app_logger.dart:80-86
Timestamp: 2025-12-12T11:00:52.914Z
Learning: In core/lib/utils/app_logger.dart, implement platform-specific logging behavior as described: web applications should respect a webConsoleEnabled flag to selectively show logs in production, while mobile platforms must suppress logs in release builds by guarding _debugPrint with BuildUtils.isDebugMode. Ensure the logging calls are conditioned on these checks and verify via builds for web and mobile (debug vs release).
Applied to files:
core/lib/utils/app_logger.dart
📚 Learning: 2025-12-12T09:03:05.962Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/presentation/model/popup_menu_item_email_action.dart:34-35
Timestamp: 2025-12-12T09:03:05.962Z
Learning: In the tmail-flutter codebase, popup menu items intentionally use a consistent hoverIcon (imagePaths.icThumbsUp) across all popup item types (email actions, mailbox actions, profile settings, etc.), while actionIcon varies per action type. This is a deliberate UI/UX design choice.
Applied to files:
core/lib/utils/app_logger.dart
📚 Learning: 2025-12-12T04:54:11.121Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Applied to files:
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: Build web version and deploy
🔇 Additional comments (24)
test/features/thread/data/network/thread_api_test.dart (1)
616-616: LGTM! Verify that tests pass after header cleanup.The removal of explicit
content-lengthheaders from the DioAdapter mock setup aligns with the Dio upgrade to 5.2.0 and is consistent with similar changes across other test files in this PR. Newer versions of Dio and http_mock_adapter typically handle content-length automatically, making explicit additions unnecessary.Please confirm that all tests in this file pass after removing the content-length headers to ensure the mock request matching still works correctly with Dio 5.2.0.
Also applies to: 665-665, 717-717, 755-755
integration_test/base/test_base.dart (1)
4-4: LGTM - Integration test updated for new startup flow.The changes correctly align the integration test setup with the refactored app entry point. The migration from
package:tmail_ui_user/main.darttopackage:tmail_ui_user/main/main_entry.dartand the direct call torunTmail()reflect the centralized startup architecture introduced in this PR.Also applies to: 37-37
lib/features/upload/domain/model/upload_attachment.dart (1)
65-65: The DioException migration is complete and verified.No remaining
DioErrorclass references exist in the codebase. The migration toDioExceptionhas been consistently applied across all modules, and the code at line 65 correctly usesDioExceptionwithDioExceptionType.cancel.lib/features/upload/data/network/file_uploader.dart (1)
150-155: DioException migration here looks consistent and safeCatching
DioExceptionand preserving the existingcopyWithsanitization keeps behavior aligned with the Dio 5.x API and the rest of the PR’s migration; no additional changes needed in this block.lib/main/utils/toast_manager.dart (1)
154-169: Correctly classifying empty-spam-folder failures nowPassing the
FeatureFailureinto_isEmptySpamFolderFailure(instead of the underlying exception) matches the helper’s signature and the patterns of the other branches, so empty‑spam‑folder errors will now show the intended toast.env.file (1)
12-15: Sentry env template aligns with the new config loaderThe added
SENTRY_ENABLED,SENTRY_DSN, andSENTRY_ENVIRONMENTentries match whatSentryConfig.load()expects and keep Sentry safely disabled by default until configured.lib/main/exceptions/remote_exception_thrower.dart (2)
34-60: DioException handling preserves previous semanticsSwitching to
DioExceptionwhile keeping the same status‑code mapping and fallback logic maintains the old behavior with Dio 5.x; callers will still see the same domain exceptions for each HTTP / network case.
77-97: Non-response DioException mapping remains consistentThe
_handleDioErrorWithoutResponsemapping fromDioExceptionType.*to your domain exceptions mirrors the priorDioErrorTypelogic, including theSocketException/OAuthAuthorizationErrorpassthrough, so there’s no regression in error classification here.core/lib/utils/sentry/sentry_initializer.dart (1)
34-44: beforeSend handler filtering AssertionError is straightforwardDropping
AssertionErrorevents via_beforeSendHandlerkeeps Sentry noise low; the rest of the events pass through unchanged, which is a reasonable default for a first integration step.lib/main/utils/app_utils.dart (1)
1-76: LGTM! Clean refactoring of environment loading.The removal of environment loading methods from AppUtils aligns with the new EnvLoader utility introduced in the PR. The remaining utility methods are unaffected and the refactoring improves separation of concerns.
core/lib/utils/application_manager.dart (4)
9-13: LGTM! Standard singleton pattern.The singleton implementation follows Dart best practices with a factory constructor and private internal constructor.
15-20: LGTM! Clean test injection mechanism.The test override pattern with
@visibleForTestingannotation provides a clean way to inject mock dependencies in unit tests without modifying the production code path.
28-36: LGTM! Robust error handling.The try/catch wrapper ensures getVersion never throws, returning an empty string on failure. This defensive approach prevents crashes during app startup or Sentry user agent generation.
38-71: LGTM! Comprehensive user agent handling.The implementation correctly:
- Handles platform-specific user agent retrieval (web vs mobile)
- Logs errors without crashing (returns empty string on failure)
- Provides lifecycle methods for mobile user agent initialization
- Generates proper application user agent string combining version and platform user agent
test/features/login/data/network/oidc_http_client_test.dart (1)
20-66: LGTM! Tests updated for DioException migration.The test updates correctly reflect the codebase-wide migration from DioError to DioException. Test descriptions and mock expectations are properly aligned with the implementation changes in the corresponding source file.
core/lib/core.dart (1)
63-64: LGTM! Core exports extended for Sentry and environment utilities.The new exports properly expose:
SentryManagerfor centralized error reportingEnvLoaderfor environment configuration loadingpackage_info_pluslibrary for app version accessThese additions align with the PR's Sentry integration objectives and environment loading refactoring.
Also applies to: 146-146
lib/features/login/data/network/oidc_http_client.dart (1)
8-8: LGTM! Error handling updated for DioException migration.The changes correctly migrate from DioError to DioException:
- Selective import of DioException (line 8)
- Updated catch clause (line 48)
This aligns with the codebase-wide Dio upgrade to 5.2.0 and is consistent with related test updates.
Also applies to: 48-48
lib/features/manage_account/presentation/manage_account_dashboard_controller.dart (1)
154-163: Sentry initialization order is correct.
SentryManager.instance.initialize()is called early inapp_runner.dartbefore the app is displayed, andManageAccountDashboardControlleris only instantiated when the route is navigated to. The initialization order is already properly handled by the app's startup sequence.test/features/interceptor/authorization_interceptor_test.dart (1)
44-52: Tests correctly migrated toDioExceptionThe construction of
DioExceptioninstances and the updatedthrowsA(predicate<DioException>(...))expectations are consistent with the interceptor’s newonError(DioException ...)signature and keep the token‑refresh / queued‑request scenarios well covered.Also applies to: 241-245, 250-272, 370-372
lib/main/bindings/network/network_isolate_binding.dart (1)
7-7: Isolate Dio + Sentry integration is consistent with main bindingsUsing a tagged
dioinstance, constructingAuthorizationInterceptorswith thatdio, and then conditionally addingLogInterceptor(debug only) anddio.addSentry()behindSentryManager.instance.isSentryAvailablematches the main network bindings and looks correct for isolate traffic as well.Also applies to: 55-72
lib/main.dart (1)
11-18: Clean startup flow refactoring.The delegation of monitoring initialization to
runAppWithMonitoringand app startup torunTmailimproves separation of concerns and centralizes error handling setup. This aligns well with the Sentry integration objective.core/test/utils/application_manager_test.dart (1)
20-39: Well-structured test setup with proper isolation.Good use of
debugDeviceInfoOverridefor dependency injection in tests. The comprehensive teardown ensures test isolation by resetting all global state includingPlatformInfo,debugDefaultTargetPlatformOverride, the mock method channel, andFkUserAgent.core/lib/utils/sentry/sentry_manager.dart (2)
78-87: Verify that sending user email to Sentry aligns with privacy requirements.The
setUsermethod sendsSentryUser(which can include email) to Sentry's servers. The linked issue #4136 specifies "Avoid sending sensitive data to Sentry." Confirm whether user email is acceptable for your privacy/compliance requirements, or consider using a hashed/anonymized identifier instead.
17-29: Robust initialization with graceful fallback.The initialization pattern correctly handles Sentry setup failures by catching exceptions and running the fallback runner. This ensures the app remains functional even if Sentry configuration is missing or invalid.
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Show resolved
Hide resolved
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
f3f5b70 to
37ed579
Compare
|
| logError( | ||
| 'FlutterError: ${details.exception}', | ||
| exception: details.exception, | ||
| stackTrace: details.stack, | ||
| ); |
There was a problem hiding this comment.
what happen here if Sentry was not initialized?
There was a problem hiding this comment.
Then the log will not be sent to Sentry.
core/lib/utils/app_logger.dart
Outdated
| SentryManager.instance.captureException( | ||
| exception ?? rawMessage, | ||
| stackTrace: stackTrace, | ||
| message: rawMessage, | ||
| extras: extras, | ||
| ); |
There was a problem hiding this comment.
should we add try/catch to every sentry instance capturing?
There was a problem hiding this comment.
We don’t use try/catch because Sentry.captureException is designed to be fail-safe and will not throw or impact the application’s execution flow.
| import 'package:sentry_flutter/sentry_flutter.dart'; | ||
|
|
||
| class SentryInitializer { | ||
| static const _blockedHeaderPatterns = [ |
There was a problem hiding this comment.
how about add more
bearer (though usually in Authorization: Bearer ...)
There was a problem hiding this comment.
We already have authorization header
| headers: sanitizedHeaders, | ||
| queryString: req.queryString, | ||
| cookies: null, | ||
| data: null, |
There was a problem hiding this comment.
We will not send data requests to Sentry.
dab246
force-pushed
the
improvement/tf-4136-re-standardize-the-logger
branch
from
ef15f3f to
5a57119
Compare
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
1f5a58f to
81fdf44
Compare
Fixed Web: Screen.Recording.2026-01-02.at.13.24.19.movMobile: Screen_recording_20260102_132949.webm |
… avoid run test fail
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
81fdf44 to
859c833
Compare
dab246
changed the base branch from
improvement/tf-4136-re-standardize-the-logger
to
master
Issue
Fixes #4136
Dependency
This PR depends on:
v5.2.0jmap-dart-client#117Description
This pull request integrates Sentry error reporting to capture, track, and analyze runtime issues across the Flutter app.
It includes detailed information for each reported event such as stack trace, breadcrumbs, contexts, and custom logs — helping developers debug production errors more efficiently.
Resolved
1. Issues Captured
Sentry successfully records exceptions triggered within the application:
2. Issue Details
Detailed event information is available, including device data, environment, and app version:
3. Stack Trace
Captured stack trace for debugging and pinpointing the root cause:
4. Breadcrumbs
Breadcrumbs show user actions and app events leading up to the error:
5. Contexts: Reported Exception
Captured context includes runtime environment, user, and device information:
6. Logs
Sentry integrates with
Sentry.loggerandcaptureMessageto record structured logs:Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.