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
… avoid run test fail
dab246
force-pushed
the
improvement/tf-4136-add-sentry
branch
from
e41d493 to
bdeb1a7
Compare
![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
🤖 Fix all issues with AI agents
In @core/lib/utils/sentry/sentry_initializer.dart:
- Around line 54-59: The code assumes req.headers is non-null when building
sanitizedHeaders, but SentryRequest.headers is nullable; guard against null by
using a default empty map or checking for null before calling Map.from. Update
the sanitizedHeaders construction (use req.headers ?? {} or an early
null-coalescing) so Map<String, String>.from receives a non-null Map, then apply
the removeWhere filter using _blockedHeaderPatterns as before to produce the
sanitizedHeaders.
In @core/lib/utils/sentry/sentry_manager.dart:
- Around line 22-26: The initialize() method currently returns early when
_isSentryAvailable is true, which prevents the provided appRunner from
executing; modify initialize() so that appRunner() is always invoked regardless
of _isSentryAvailable (call await appRunner() before returning when Sentry is
already initialized), preserving existing try/catch and any Sentry setup path
for the first-time initialization; ensure you reference the initialize()
function, the _isSentryAvailable flag, and the appRunner parameter so the app
still starts on repeated calls.
In @lib/main/bindings/network/network_bindings.dart:
- Around line 105-112: The current duplicate check uses fragile string matching
on interceptor runtimeType; replace it with a proper type check against the
actual Sentry interceptor (e.g., detect SentryDioInterceptor via
interceptors.any((i) => i is SentryDioInterceptor) or
interceptors.whereType<SentryDioInterceptor>().isNotEmpty) to reliably guard
adding the interceptor in the block that calls dio.addSentry(); alternatively,
add a registration flag in SentryManager (e.g., a bool like isSentryRegistered)
and consult/set that flag when calling addSentry() to prevent duplicate
registration.
In @lib/main/bindings/network/network_isolate_binding.dart:
- Around line 65-66: The code calls Get.find<DynamicUrlInterceptors>() inside
NetworkIsolateBindings without registering it in the isolate scope, which can
break if main-scope bindings initialize in a different order; fix by registering
the interceptor in NetworkIsolateBindings (call
Get.put(DynamicUrlInterceptors(), tag: BindingTag.isolateTag)) or explicitly
resolve the isolate-tagged instance (Get.find<DynamicUrlInterceptors>(tag:
BindingTag.isolateTag)) before adding it to dio.interceptors so
DynamicUrlInterceptors is reliably available in the isolate binding.
In @lib/main/main_entry.dart:
- Around line 22-28: The code creates a throwaway Executor with
Executor().warmUp(...) while MainBindings().dependencies() registers an Executor
via Get.put(Executor()), so warm up the actual bound instance instead of a new
one: retrieve the registered Executor (the instance created by MainBindings,
e.g., via Get.find<Executor>() after binding or by binding the Executor before
calling Future.wait) and call warmUp(log: BuildUtils.isDebugMode) on that
instance, or move the binding of Executor() out of MainBindings and into the
setup sequence before warming so the warmed executor is the same one that gets
registered.
🧹 Nitpick comments (12)
core/lib/utils/config/env_loader.dart (1)
21-34: Consider adding a clarifying comment for the callback pattern.The
onCallBackis used to reload the base config when FCM config fails, which restoresdotenv.envto a valid state. This fallback behavior is clever but not immediately obvious.📝 Suggested documentation
static Future<void> loadFcmConfigFileToEnv({ Map<String, String>? currentMapEnvData, Future<void> Function()? onCallBack, }) async { try { await dotenv.load( fileName: appFCMConfigurationPath, mergeWith: currentMapEnvData ?? {}, ); } catch (e) { logWarning('EnvLoader::loadFcmConfigFileToEnv: Exception = $e'); + // FCM config is optional; reload base config to restore dotenv.env state await onCallBack?.call(); } }core/lib/utils/application_manager.dart (1)
27-33: Consider resetting_isMobileUserAgentInitializedinclearCache().The
clearCache()method resets all cached values but doesn't reset_isMobileUserAgentInitialized. This could cause inconsistent state in tests where_cachedMobileUserAgentis cleared but_isMobileUserAgentInitializedremains true.♻️ Suggested fix
@visibleForTesting void clearCache() { _packageInfoCache = null; _versionCache = null; _cachedWebUserAgent = null; _cachedMobileUserAgent = null; + _isMobileUserAgentInitialized = false; }docs/configuration/sentry_configuration.md (1)
1-39: Consider expanding documentation for completeness.The configuration guide covers the essentials well. A few suggestions to improve it:
Mobile platform note: The PR objectives mention that mobile Sentry configuration is hardcoded. Consider adding a note clarifying this is for web/Flutter web deployments, or documenting where mobile config resides.
Privacy considerations: Based on team discussions, Sentry captures user PII (account ID, display name, email) for error tracking. Consider adding a note about this and referencing the privacy policy documentation.
Example environment values: Providing example values for
SENTRY_ENVIRONMENT(e.g.,dev,staging,production) in the code block would help first-time users.core/lib/utils/sentry/sentry_initializer.dart (2)
7-17: Good security practice: sanitizing sensitive headers.The blocked header patterns cover common sensitive headers. Consider adding
x-api-key(hyphenated variant) andsessionfor broader coverage.💡 Suggested additions for broader coverage
static const _blockedHeaderPatterns = [ 'authorization', 'cookie', 'set-cookie', 'x-auth', 'x-token', 'api-key', 'apikey', + 'x-api-key', 'secret', 'bearer', + 'session', + 'password', ];
70-85: Manual event reconstruction may lose fields added in future Sentry versions.The comment notes
copyWithis deprecated. However, manually reconstructingSentryEventmeans any new fields added toSentryEventin future Sentry SDK versions will be silently dropped. Consider periodically reviewing the Sentry SDK changelog or adding a comment to track this.lib/main/bindings/network/network_isolate_binding.dart (1)
54-77: Duplicated Sentry registration logic across network bindings.This interceptor setup mirrors
network_bindings.dartexactly, including the fragileruntimeType.toString().contains('Sentry')check. Consider extracting a shared helper (e.g.,SentryDioHelper.addIfAvailable(dio)) to avoid duplication and centralize the duplicate-guard logic.♻️ Example shared helper
// In core or a shared utility file class SentryDioHelper { static void addIfAvailable(Dio dio) { if (!SentryManager.instance.isSentryAvailable) return; final alreadyHasSentry = dio.interceptors .any((i) => i.runtimeType.toString().contains('Sentry')); if (!alreadyHasSentry) { dio.addSentry(); } } }Then both bindings can call
SentryDioHelper.addIfAvailable(dio);.lib/main/main_entry.dart (1)
22-28: No error handling for parallel initialization failures.If any task in
Future.waitthrows, the entire initialization fails without recovery or clear diagnostics. Consider wrapping with error handling or usingFuture.wait(..., eagerError: false)to let other tasks complete, then aggregate errors.core/lib/utils/sentry/sentry_config.dart (2)
57-63: Using exceptions for expected control flow is non-idiomatic.When
SENTRY_ENABLED=false, throwing an exception forces the caller to use try/catch for normal control flow. Consider returning a nullableSentryConfig?or a result type, or providing a separateisEnabled()check.♻️ Suggested approach
- static Future<SentryConfig> load() async { + static Future<SentryConfig?> load() async { await EnvLoader.loadConfigFromEnv(); final sentryAvailable = dotenv.get('SENTRY_ENABLED', fallback: 'false'); final isAvailable = sentryAvailable == 'true'; final sentryDSN = dotenv.get('SENTRY_DSN', fallback: ''); final sentryEnvironment = dotenv.get('SENTRY_ENVIRONMENT', fallback: ''); - if (!isAvailable) { - throw Exception('Sentry is not available'); - } - - if (sentryDSN.trim().isEmpty || sentryEnvironment.trim().isEmpty) { - throw Exception('Sentry configuration is missing'); + if (!isAvailable || sentryDSN.trim().isEmpty || sentryEnvironment.trim().isEmpty) { + return null; } // ... }
39-40: Consider lower default sample rates for production.Defaults of
1.0(100%) for bothtracesSampleRateandprofilesSampleRatewill capture every transaction and profile. This can be expensive in production and may impact quota. Typical production values are 0.1–0.2. Consider making these configurable via environment variables or using lower defaults.lib/features/composer/data/repository/composer_repository_impl.dart (1)
75-75: InlineApplicationManager()instantiation reduces testability.Creating
ApplicationManager()inline couples this method to the concrete implementation. Tests must now rely onApplicationManager.debugDeviceInfoOverrideto mock behavior, which is less explicit than constructor injection.This is acceptable if the team prefers the static override pattern, but be aware it makes test setup less discoverable.
core/lib/utils/sentry/sentry_manager.dart (1)
47-56: Breadcrumb duplicates the captured event.Adding the exception/message as a breadcrumb within
withScopeduplicates the event information. Breadcrumbs are typically for contextual events leading up to an error, not the error itself. Consider using breadcrumbs only for additional context inextrasor removing this pattern.Also applies to: 70-78
core/test/utils/application_manager_test.dart (1)
80-106: Consider verifying Android platform call count for consistency with web test.The web test (lines 69-76) verifies
webBrowserInfois called only once to confirm caching. The Android test doesn't verify that the method channel handler is called only once. For consistency, consider tracking and asserting invocation count.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
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!**/*.lockscribe/pubspec.lockis excluded by!**/*.lockserver_settings/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (78)
contact/pubspec.yamlcontact/test/contact/autocomplete_tmail_contact_method_test.dartcore/lib/core.dartcore/lib/utils/app_logger.dartcore/lib/utils/application_manager.dartcore/lib/utils/config/env_loader.dartcore/lib/utils/sentry/sentry_config.dartcore/lib/utils/sentry/sentry_initializer.dartcore/lib/utils/sentry/sentry_manager.dartcore/lib/utils/string_convert.dartcore/pubspec.yamlcore/test/utils/application_manager_test.dartdocs/configuration/sentry_configuration.mdemail_recovery/pubspec.yamlemail_recovery/test/method/get_email_recovery_action_method_test.dartemail_recovery/test/method/set_email_recovery_action_method_test.dartenv.filefcm/pubspec.yamlfcm/test/method/firebase_registration_get_method_test.dartfcm/test/method/firebase_registration_set_method_test.dartforward/pubspec.yamlforward/test/forward/get_forward_method_test.dartforward/test/forward/set_forward_method_test.dartintegration_test/base/test_base.dartlib/features/base/base_controller.dartlib/features/base/mixin/ai_scribe_mixin.dartlib/features/base/widget/application_version_widget.dartlib/features/composer/data/repository/composer_repository_impl.dartlib/features/composer/presentation/composer_bindings.dartlib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dartlib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dartlib/features/composer/presentation/widgets/web/web_editor_widget.dartlib/features/home/domain/extensions/session_extensions.dartlib/features/login/data/network/interceptors/authorization_interceptors.dartlib/features/login/data/network/oidc_http_client.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/manage_account/presentation/manage_account_dashboard_controller.dartlib/features/public_asset/presentation/public_asset_bindings.dartlib/features/upload/data/network/file_uploader.dartlib/features/upload/domain/model/upload_attachment.dartlib/main.dartlib/main/app_runner.dartlib/main/bindings/core/core_bindings.dartlib/main/bindings/network/network_bindings.dartlib/main/bindings/network/network_isolate_binding.dartlib/main/exceptions/remote_exception_thrower.dartlib/main/main_entry.dartlib/main/utils/app_config.dartlib/main/utils/app_utils.dartlib/main/utils/toast_manager.dartpubspec.yamlrule_filter/pubspec.yamlrule_filter/test/rule_filter/get_rule_filter_method_test.dartrule_filter/test/rule_filter/set_rule_filter_method_test.dartscribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dartscribe/lib/scribe/ai/presentation/model/ai_capability.dartscribe/pubspec.yamlserver_settings/pubspec.yamlserver_settings/test/server_settings/get/get_server_settings_method_test.dartserver_settings/test/server_settings/set/set_server_settings_method_test.darttest/features/base/base_controller_test.darttest/features/composer/data/repository/composer_repository_impl_test.darttest/features/composer/presentation/composer_controller_test.darttest/features/email/presentation/controller/single_email_controller_test.darttest/features/home/presentation/home_controller_test.darttest/features/identity_creator/presentation/identity_creator_controller_test.darttest/features/interceptor/authorization_interceptor_test.darttest/features/login/data/network/oidc_http_client_test.darttest/features/login/presentation/login_controller_test.darttest/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.darttest/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.darttest/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.darttest/features/manage_account/presentation/profiles/identities/identities_controller_test.darttest/features/rule_filter_creator/presentation/rule_filter_creator_controller_test.darttest/features/search/verify_before_time_in_search_email_filter_test.darttest/features/server_settings/data/network/server_settings_api_test.darttest/features/thread/data/network/thread_api_test.darttest/features/thread/presentation/controller/thread_controller_test.dart
💤 Files with no reviewable changes (26)
- fcm/test/method/firebase_registration_set_method_test.dart
- email_recovery/test/method/set_email_recovery_action_method_test.dart
- fcm/test/method/firebase_registration_get_method_test.dart
- test/features/search/verify_before_time_in_search_email_filter_test.dart
- test/features/thread/presentation/controller/thread_controller_test.dart
- test/features/manage_account/presentation/profiles/identities/identities_controller_test.dart
- rule_filter/test/rule_filter/set_rule_filter_method_test.dart
- test/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller_test.dart
- test/features/email/presentation/controller/single_email_controller_test.dart
- contact/test/contact/autocomplete_tmail_contact_method_test.dart
- lib/main/utils/app_config.dart
- email_recovery/test/method/get_email_recovery_action_method_test.dart
- test/features/login/presentation/login_controller_test.dart
- lib/features/composer/presentation/composer_bindings.dart
- test/features/home/presentation/home_controller_test.dart
- test/features/composer/presentation/composer_controller_test.dart
- test/features/server_settings/data/network/server_settings_api_test.dart
- forward/test/forward/set_forward_method_test.dart
- test/features/rule_filter_creator/presentation/rule_filter_creator_controller_test.dart
- server_settings/test/server_settings/get/get_server_settings_method_test.dart
- test/features/base/base_controller_test.dart
- test/features/mailbox_dashboard/presentation/view/mailbox_dashboard_view_widget_test.dart
- rule_filter/test/rule_filter/get_rule_filter_method_test.dart
- lib/main/bindings/core/core_bindings.dart
- server_settings/test/server_settings/set/set_server_settings_method_test.dart
- forward/test/forward/get_forward_method_test.dart
🚧 Files skipped from review as they are similar to previous changes (17)
- lib/features/composer/presentation/widgets/web/web_editor_widget.dart
- forward/pubspec.yaml
- lib/main/utils/toast_manager.dart
- lib/main/exceptions/remote_exception_thrower.dart
- rule_filter/pubspec.yaml
- server_settings/pubspec.yaml
- lib/main/utils/app_utils.dart
- core/lib/core.dart
- pubspec.yaml
- lib/features/composer/presentation/extensions/ai_scribe/handle_ai_scribe_in_composer_extension.dart
- test/features/login/data/network/oidc_http_client_test.dart
- fcm/pubspec.yaml
- email_recovery/pubspec.yaml
- lib/features/composer/presentation/widgets/mobile/mobile_editor_widget.dart
- lib/features/home/domain/extensions/session_extensions.dart
- core/pubspec.yaml
- scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
📚 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/main_entry.dartlib/main/app_runner.dartcore/lib/utils/application_manager.dartlib/main.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/main/main_entry.dartcore/lib/utils/string_convert.dartlib/main/app_runner.dartlib/features/base/mixin/ai_scribe_mixin.dartscribe/lib/scribe/ai/presentation/model/ai_capability.dartlib/features/upload/domain/model/upload_attachment.dartlib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/base/widget/application_version_widget.darttest/features/interceptor/authorization_interceptor_test.dartlib/features/upload/data/network/file_uploader.dartlib/features/login/data/network/oidc_http_client.dartlib/main/bindings/network/network_isolate_binding.darttest/features/thread/data/network/thread_api_test.dartlib/features/public_asset/presentation/public_asset_bindings.dartintegration_test/base/test_base.dartlib/features/login/data/network/interceptors/authorization_interceptors.dartlib/features/manage_account/presentation/manage_account_dashboard_controller.darttest/features/composer/data/repository/composer_repository_impl_test.dartlib/features/composer/data/repository/composer_repository_impl.dartcore/lib/utils/app_logger.darttest/features/identity_creator/presentation/identity_creator_controller_test.dartcore/lib/utils/application_manager.dartcore/lib/utils/sentry/sentry_config.dartlib/main/bindings/network/network_bindings.dartlib/features/base/base_controller.dartcore/test/utils/application_manager_test.darttest/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dartcore/lib/utils/sentry/sentry_manager.dartcore/lib/utils/sentry/sentry_initializer.dartcore/lib/utils/config/env_loader.dartlib/main.dart
📚 Learning: 2025-12-15T06:24:50.369Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
Applied to files:
docs/configuration/sentry_configuration.mdlib/main/bindings/network/network_isolate_binding.dartlib/features/manage_account/presentation/manage_account_dashboard_controller.dartcore/lib/utils/app_logger.dartcore/lib/utils/sentry/sentry_config.dartlib/main/bindings/network/network_bindings.dartlib/features/base/base_controller.dartcore/lib/utils/sentry/sentry_manager.dartcore/lib/utils/sentry/sentry_initializer.dart
📚 Learning: 2025-12-15T06:24:38.823Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:38.823Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, the code calls SentryManager.instance.setUser() with PII fields (account ID, display name, username, email). This is privacy-sensitive. Review and ensure PII handling complies with policy: avoid sending unnecessary PII to Sentry, redact or hash sensitive fields if possible, or document explicit privacy policy and user consent for such data. If PII must be sent, ensure minimal data, secure handling, and add notes to the privacy policy. Consider adding tests or a lint rule to flag setUser calls that include PII.
Applied to files:
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.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.darttest/features/composer/data/repository/composer_repository_impl_test.darttest/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart
📚 Learning: 2025-12-18T09:19:32.437Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4203
File: scribe/pubspec.yaml:37-37
Timestamp: 2025-12-18T09:19:32.437Z
Learning: In the tmail-flutter repository, json_annotation version 4.8.0 is the standardized version across modules and should not be upgraded without considering the standardization practice.
Applied to files:
contact/pubspec.yamllib/main/bindings/network/network_isolate_binding.dartlib/features/login/data/network/interceptors/authorization_interceptors.darttest/features/composer/data/repository/composer_repository_impl_test.dartcore/lib/utils/app_logger.dartlib/main/bindings/network/network_bindings.dartcore/lib/utils/config/env_loader.dart
📚 Learning: 2025-12-12T07:56:31.877Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4204
File: lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart:579-582
Timestamp: 2025-12-12T07:56:31.877Z
Learning: In lib/features/email/data/datasource_impl/email_hive_cache_datasource_impl.dart, the addLabelToEmail method intentionally throws UnimplementedError because label synchronization with the email cache is handled separately via extension methods (e.g., _autoSyncLabelToSelectedEmailOnMemory in handle_label_for_email_extension.dart). Implementing it in the cache datasource would cause data conflicts. This differs from other keyword operations like markAsRead, markAsStar, markAsAnswered, and markAsForwarded, which are implemented directly in the cache datasource.
Applied to files:
lib/features/public_asset/presentation/public_asset_bindings.dart
📚 Learning: 2025-12-23T05:27:47.287Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4210
File: lib/l10n/intl_messages.arb:5151-5182
Timestamp: 2025-12-23T05:27:47.287Z
Learning: In the tmail-flutter repository, localization (l10n) updates for locale files (intl_fr.arb, intl_vi.arb, intl_ru.arb, etc.) are managed automatically through a third-party website/platform. When new English keys are added to intl_messages.arb, translations to other locales are handled by this automated workflow rather than being manually added in pull requests.
Applied to files:
lib/features/login/data/network/interceptors/authorization_interceptors.dartcore/lib/utils/app_logger.dartcore/lib/utils/config/env_loader.dart
📚 Learning: 2025-12-23T05:27:10.887Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4210
File: model/lib/mailbox/presentation_mailbox.dart:22-34
Timestamp: 2025-12-23T05:27:10.887Z
Learning: In the tmail-flutter codebase, the Action Required folder (actionRequiredFolder) is a virtual folder that is AI-curated and contains emails with the keyword "needs-action". Unlike the Favorites folder, users should NOT be able to drag emails into the Action Required folder. The UI correctly prevents this by not wrapping the Action Required folder item in a DragTarget (see mailbox_item_widget.dart).
Applied to files:
lib/features/login/data/network/interceptors/authorization_interceptors.dartcore/lib/utils/app_logger.darttest/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.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
⏰ 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 (email_recovery)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: Build web version and deploy
🔇 Additional comments (41)
scribe/pubspec.yaml (1)
29-29: LGTM — Consistent with repo-wide Dio upgrade.The bump from 5.0.0 to 5.2.0 aligns with the coordinated dependency update across other packages in the monorepo (core, fcm/server_settings, etc.) as part of the Sentry integration work. This ensures version consistency and avoids potential conflicts from mixed Dio versions.
lib/features/upload/data/network/file_uploader.dart (1)
150-154: LGTM!The migration from
DioErrortoDioExceptionis correct for Dio 5.x. The exception handling properly sanitizes request data before rethrowing, which aligns with the PR objective to avoid sending sensitive data to Sentry.lib/features/upload/domain/model/upload_attachment.dart (1)
65-66: LGTM!The migration from
DioError/DioErrorType.canceltoDioException/DioExceptionType.cancelis correct for Dio 5.x. The cancellation detection logic is preserved.test/features/identity_creator/presentation/identity_creator_controller_test.dart (1)
7-7: LGTM!The import change from
ApplicationManagertoFileUtilsaligns with the PR-wide refactoring. The mock is properly declared at line 79 (MockSpec<FileUtils>()) and registered in the DI container at line 162 (Get.put<FileUtils>(MockFileUtils())).core/lib/utils/string_convert.dart (1)
248-251: LGTM — appropriate log level for a recoverable exception.The change from
logErrortologWarningis correct: this exception has a safe fallback (returning trimmed HTML content), so it doesn't warrant error-level severity. This also aligns with the Sentry integration goal of reducing noise from non-critical, recoverable issues and is consistent with similar patterns elsewhere in this file (lines 54, 139, 150).scribe/lib/scribe/ai/presentation/model/ai_capability.dart (1)
26-37: LGTM! Appropriate log level for non-fatal configuration checks.Downgrading from
logErrortologWarningis correct here. An invalid endpoint format or a validation exception is a configuration issue handled gracefully (returnsfalse), not a critical runtime failure. This aligns with the PR's broader effort to harmonize log levels with Sentry integration—reserving error-level logs for issues that require immediate attention.test/features/thread/data/network/thread_api_test.dart (1)
616-616: LGTM - Appropriate removal of explicit content-length header assertions.Removing content-length from mock adapter matching is a good cleanup. These headers are auto-calculated by the HTTP client and don't need to be explicitly asserted in tests. This makes the tests less brittle and better aligned with the Dio 5.2.0 migration.
Also applies to: 665-665, 717-717, 755-755
contact/pubspec.yaml (1)
28-28: LGTM — Dio 5.2.0 upgrade is consistent across the monorepo.The bump to 5.2.0 aligns with the repo-wide standardization. All modules (contact, core, email_recovery, fcm, forward, rule_filter, scribe, and server_settings) are uniformly on 5.2.0, and the codebase migration from
DioErrortoDioExceptionis underway as part of this Dio 5.x upgrade.integration_test/base/test_base.dart (1)
4-4: LGTM!The import path and entry point updates correctly align with the new centralized startup orchestration in
main_entry.dart. The migration from aliasedapp.runTmail()to directrunTmail()is clean and maintains the same test setup behavior.Also applies to: 36-37
lib/features/base/mixin/ai_scribe_mixin.dart (1)
41-42: LGTM — improved stack trace capture.Capturing the stack trace is a good improvement for debugging injection failures. The downgrade from
logErrortologWarningis acceptable since AIScribe is an optional feature and injection failures are non-critical to core app functionality.test/features/mailbox_dashboard/presentation/controller/advanced_filter_controller_test.dart (1)
14-14: LGTM!The
filter_filter.dartimport is correctly added to provideFilterFieldenum values used throughout theonTextChanged::testgroup. The removal ofApplicationManagermock dependencies aligns with the PR's refactoring to a singleton pattern, simplifying test setup without affecting test coverage.env.file (1)
13-15: LGTM — Sentry configuration variables added with safe defaults.
SENTRY_ENABLED=falseis an appropriate default to prevent accidental data reporting in development. The emptySENTRY_DSNandSENTRY_ENVIRONMENTvalues correctly require explicit configuration for each deployment environment. Documentation is available indocs/configuration/sentry_configuration.md.lib/features/login/data/network/oidc_http_client.dart (1)
8-8: LGTM — migration toDioExceptionaligns with Dio 5.x.The rename from
DioErrortoDioExceptionis correct for Dio 5.2.0. The exception handling logic (checking.errorand.response?.statusCode) remains compatible with the new class API.Also applies to: 48-48
lib/features/base/widget/application_version_widget.dart (1)
29-29: LGTM!The migration from
Get.find<ApplicationManager>()to the singletonApplicationManager().getAppVersion()is correct and aligns with the broader refactoring in this PR.lib/features/public_asset/presentation/public_asset_bindings.dart (1)
18-18: LGTM!The import addition for
CreatePublicAssetInteractoris necessary for the usage inbindingsInteractor()and the removal of theApplicationManagerimport aligns with the singleton migration.core/lib/utils/app_logger.dart (2)
64-92: Well-structured Sentry integration.The conditional logic cleanly separates print vs Sentry paths, and
unawaited()correctly avoids blocking the logging flow. The early return when neither path is needed is a good optimization.
134-136: LGTM!Clean helper function that clearly defines which log levels trigger Sentry reporting.
core/lib/utils/config/env_loader.dart (1)
36-42: LGTM!Appropriate error handling for optional configuration loading - logs a warning but allows the app to continue.
core/lib/utils/application_manager.dart (2)
92-152: Well-structured user agent management.The platform-specific dispatching, caching, and initialization checks are well-implemented. The defensive check for
_isMobileUserAgentInitializedin_getMobileUserAgent()prevents silent failures.
154-159: LGTM!Clean composition of the application user agent with proper trimming to handle empty values.
lib/features/manage_account/presentation/manage_account_dashboard_controller.dart (1)
155-165: Sentry user context integration looks good.The user context is correctly set when a session is available. Based on learnings, the team has intentionally included PII (account ID, display name, username, email) for error tracking and plans to document this in the app's privacy policy.
lib/main.dart (1)
12-19: Clean centralization of startup flow.The refactoring to use
runAppWithMonitoring(runTmail)centralizes initialization, error handling, and Sentry integration. This improves maintainability and aligns with the PR objectives.lib/features/base/base_controller.dart (1)
554-559: LGTM!Clearing the Sentry user context at the start of the logout flow is the correct approach. This ensures user identity is disassociated from Sentry before any subsequent cleanup operations, maintaining proper privacy hygiene. Based on learnings, the team has confirmed PII handling will be documented in the privacy policy.
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart (3)
32-32: LGTM!Import is correctly added for accessing
SentryUserclass.
872-879: LGTM!The Sentry user context is correctly set with account information when the session is established. Based on learnings, this PII handling is intentional and will be documented in the app's privacy policy for transparency and compliance.
3415-3417: Consistent with onInit pattern.The
releaseUserAgent()call mirrors theinitUserAgent()inonInit, ensuring proper cleanup of the user agent when the controller is disposed.test/features/interceptor/authorization_interceptor_test.dart (4)
44-52: LGTM!Correctly migrated from
DioErrortoDioExceptionandDioExceptionType.badResponseper Dio 5.x API changes.
241-244: LGTM!Test expectation correctly updated to check for
DioExceptiontype.
250-272: LGTM!Both
requestOneDioError401andrequestTwoDioError401fixtures correctly useDioExceptionwithDioExceptionType.badResponse.
365-375: LGTM!The error expectation correctly checks for
DioExceptionand validates the nestedAccessTokenInvalidException.lib/main/app_runner.dart (2)
11-49: Well-structured error monitoring setup.The implementation correctly layers error handling:
- Zone-level errors via
runZonedGuarded- Flutter framework errors via
FlutterError.onError- Platform-level errors via
PlatformDispatcher.instance.onError- Sentry integration with proper fallback
One consideration:
PlatformDispatcher.instance.onErrorreturnstrue, indicating the error is "handled." This prevents the error from propagating to other handlers. If you want the default crash reporting behavior to also trigger (e.g., for native crash reports on mobile), consider returningfalseafter logging.
35-41: This concern appears to be based on a misunderstanding of how SentryFlutter.init() handles errors.The
appRunneris passed directly toSentryFlutter.init(), which executes it within Sentry's error capture context. Errors that occur duringappRunnerexecution (including fromrunTmailPreload()orrunApp()) are captured and reported to Sentry before any exception propagates.The
fallBackRunneris only invoked ifSentryFlutter.init()itself throws (e.g., misconfigured DSN, network issues during initialization). In that case, Sentry would not be initialized anyway, so local-only logging of initialization errors is expected and unavoidable—this is not an issue with error handling during app execution.core/lib/utils/sentry/sentry_initializer.dart (2)
61-68: Strippingdataandcookiesunconditionally may hide non-sensitive context.Setting
data: nullandcookies: nullis conservative for privacy, but it removes all request body information even when it might be helpful for debugging non-sensitive requests. This is acceptable if the policy is to never send request bodies; just ensure this aligns with the team's intent.
19-44: Review comment is incorrect — Sentry initialization is already guarded.
SentryConfig.load()throws an exception whenSENTRY_ENABLEDis not 'true' (line 57-58), preventing theinit()method from receiving a config object and proceeding with initialization. The scenario described in the review (isAvailable false with DSN set causing events to be captured) cannot occur because the exception short-circuits execution before returning a config. No additional guards are needed.Likely an incorrect or invalid review comment.
test/features/composer/data/repository/composer_repository_impl_test.dart (1)
62-67: LGTM: Constructor updated to match production code changes.The test correctly reflects the removal of
ApplicationManagerfromComposerRepositoryImpl's dependencies, aligning with the broader refactor to instantiate it inline in production code.lib/main/bindings/network/network_bindings.dart (1)
90-104: Good refactor: localdiovariable improves readability.Extracting the Dio instance into a local variable and passing it to
AuthorizationInterceptorsis cleaner than repeatedGet.find<Dio>()calls.lib/main/main_entry.dart (2)
14-17: LGTM: Clean separation of preload and app launch.The split between
runTmailPreload()andrunTmail()allows integration tests to call only preload when needed, which aligns with the test_base.dart changes mentioned in the summary.
30-31: Sequential awaits after parallel init look intentional.
CozyIntegration.integrateCozy()andHiveCacheConfig.instance.initializeEncryptionKey()are awaited sequentially, suggesting they may depend on the parallel initialization results. This ordering is appropriate if dependencies exist.lib/features/login/data/network/interceptors/authorization_interceptors.dart (1)
85-85: LGTM! Correct migration to DioException for Dio 5.x.The
DioError→DioExceptionrename aligns with the Dio 5.2.0 API. The error handling logic (token refresh, retry) remains intact.Also applies to: 162-162
core/lib/utils/sentry/sentry_manager.dart (1)
82-91: PII logging acknowledged.
setUserlogs the user's email, which is intentional per prior discussions. Ensure this is documented in the app's privacy policy for transparency. Based on learnings, this is the expected behavior.core/test/utils/application_manager_test.dart (1)
20-40: LGTM! Thorough test setup and teardown.The test setup properly initializes mocks via static override, and teardown cleans up all state including platform overrides, method channel handlers, and FkUserAgent. This prevents test pollution.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/pubspec.yaml (1)
44-108: Consider updating to more recent package versions.While the Sentry packages are correctly matched to each other (both 9.8.0), several dependencies are already behind their latest available versions. Most notably,
diois pinned to 5.2.0 while version 5.9.0 is available—a gap of 7 minor versions that may contain important performance, security, or compatibility improvements. Similarly,sentry_flutterandsentry_dioare 1 patch behind (9.8.0 vs 9.9.2), andflutter_dotenvis 1 minor behind (5.0.2 vs 6.0.0). These packages should be evaluated for updates to benefit from recent fixes and improvements.
🤖 Fix all issues with AI agents
In @core/lib/utils/sentry/sentry_initializer.dart:
- Around line 27-49: The beforeSend handler (_beforeSendHandler) sanitizes
headers but misses request.queryString and breadcrumb data; update
SentryFlutter.init options to set a beforeBreadcrumb callback (e.g.,
options.beforeBreadcrumb) that filters or redacts URLs and data in Breadcrumbs
created by enableBreadcrumbTrackingForCurrentPlatform(), and extend the request
sanitization inside _beforeSendHandler to explicitly scrub request.queryString
(redact known keys like token/api_key/authorization or drop the entire query
string) before returning the event.
- Around line 75-89: The current code constructs a new SentryEvent losing many
fields; instead mutate the incoming SentryEvent instance by assigning the
sanitized request to event.request and return event (e.g., replace the
SentryEvent(...) construction with event.request = sanitizedRequest; return
event;), removing any use of copyWith/deprecated construction so all existing
fields (transaction, culprit, serverName, debugMeta, sdk, platform, environment,
release, dist, exceptions, threads, measurements, extra, etc.) are preserved.
🧹 Nitpick comments (5)
core/lib/utils/sentry/sentry_manager.dart (1)
45-57: Consider whether the breadcrumb adds value here.Breadcrumbs typically capture a trail of events before an error occurs. Adding a breadcrumb during
captureExceptionfor the same exception duplicates information Sentry already records. Theextrasmetadata could alternatively be passed viaSentry.captureException'swithScopeusingscope.setExtra()orscope.setContext()for cleaner separation.This works as-is, but if the intent is to attach structured metadata, consider using context/extras directly:
withScope: (scope) { if (extras != null) { scope.setContexts('metadata', extras); } },core/lib/utils/sentry/sentry_dio_helper.dart (1)
9-13: Fragile runtime type check for detecting Sentry interceptor.Using
runtimeType.toString().contains('FailedRequestInterceptor')is brittle:
- The class name may change across
sentry_dioversions- In release/minified builds, type names might be obfuscated (though less common in Dart)
Consider tracking whether Sentry was added via a separate flag, or check if
sentry_dioexports the interceptor type for direct type matching.♻️ Alternative: Track state externally
class SentryDioHelper { static final _sentryAddedToDio = Expando<bool>(); static void addIfAvailable(Dio dio) { if (!SentryManager.instance.isSentryAvailable) return; if (_sentryAddedToDio[dio] == true) return; dio.addSentry(); _sentryAddedToDio[dio] = true; } }core/lib/utils/application_manager.dart (1)
18-19: DeviceInfoPlugin is instantiated on every access.The
_deviceInfogetter creates a newDeviceInfoPlugin()instance each time it's accessed (when no override is set). SinceDeviceInfoPluginis lightweight and stateless, this is functionally correct but slightly wasteful. Consider caching it if accessed frequently.♻️ Optional: Cache the plugin instance
+ DeviceInfoPlugin? _deviceInfoCache; + DeviceInfoPlugin get _deviceInfo => - debugDeviceInfoOverride ?? DeviceInfoPlugin(); + debugDeviceInfoOverride ?? (_deviceInfoCache ??= DeviceInfoPlugin());core/lib/utils/sentry/sentry_initializer.dart (1)
7-20: Header sanitization patterns look comprehensive.The blocked patterns cover common authentication/session headers. The case-insensitive
contains()matching is appropriately broad to catch variations likeX-Authorization-Token.Consider adding
tokenas a standalone pattern to catch generic token headers (e.g.,X-Custom-Token,Access-Token).♻️ Optional: Add generic token pattern
static const _blockedHeaderPatterns = [ 'authorization', 'cookie', 'set-cookie', 'x-auth', 'x-token', 'api-key', 'x-api-key', 'apikey', 'secret', 'bearer', 'session', 'password', + 'token', ];core/lib/core.dart (1)
146-147: Consider documenting the rationale for re-exporting a third-party package.Re-exporting
package_info_plusdirectly from the core library is convenient for consumers but has tradeoffs:
- Pro: Consumers access it without adding their own dependency.
- Con: Hides the transitive dependency, may cause version conflicts if a consumer also depends on it directly.
If this is intentional for centralizing version management, consider adding a brief comment explaining the pattern for maintainability.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
contact/pubspec.lockis excluded by!**/*.lockcore/pubspec.lockis excluded by!**/*.lockmodel/pubspec.lockis excluded by!**/*.lockscribe/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
core/lib/core.dartcore/lib/utils/application_manager.dartcore/lib/utils/config/env_loader.dartcore/lib/utils/sentry/sentry_config.dartcore/lib/utils/sentry/sentry_dio_helper.dartcore/lib/utils/sentry/sentry_initializer.dartcore/lib/utils/sentry/sentry_manager.dartcore/pubspec.yamllib/main/bindings/network/network_bindings.dartlib/main/bindings/network/network_isolate_binding.dartlib/main/main_entry.dart
🚧 Files skipped from review as they are similar to previous changes (5)
- lib/main/bindings/network/network_isolate_binding.dart
- core/lib/utils/sentry/sentry_config.dart
- lib/main/bindings/network/network_bindings.dart
- lib/main/main_entry.dart
- core/lib/utils/config/env_loader.dart
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
📚 Learning: 2025-12-15T06:24:50.369Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
Applied to files:
core/lib/utils/sentry/sentry_initializer.dartcore/lib/core.dartcore/lib/utils/sentry/sentry_manager.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:
core/lib/utils/sentry/sentry_initializer.dartcore/lib/utils/sentry/sentry_dio_helper.dartcore/lib/core.dartcore/lib/utils/application_manager.dartcore/lib/utils/sentry/sentry_manager.dart
📚 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:
core/lib/utils/application_manager.dart
📚 Learning: 2025-12-18T09:19:32.437Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4203
File: scribe/pubspec.yaml:37-37
Timestamp: 2025-12-18T09:19:32.437Z
Learning: In the tmail-flutter repository, json_annotation version 4.8.0 is the standardized version across modules and should not be upgraded without considering the standardization practice.
Applied to files:
core/pubspec.yaml
⏰ 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: Build web version and deploy
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (default)
🔇 Additional comments (6)
core/lib/utils/sentry/sentry_manager.dart (2)
8-34: Solid singleton initialization with graceful fallback.The guard at line 23-27 correctly skips re-initialization while still running
appRunner. The fallback pattern ensures the app runs even if Sentry fails to initialize.
83-107: User management looks correct.The
setUserandclearUsermethods properly guard against unavailable Sentry and handle exceptions. Based on learnings, the team has acknowledged that PII (user email) is intentionally sent to Sentry and will be documented in the privacy policy.core/lib/utils/application_manager.dart (3)
36-63: Caching strategy for package info and version is well-implemented.The caching pattern correctly handles:
- First access loads and caches
- Subsequent accesses return cached value
- Error handling returns safe defaults
The
clearCache()method being@visibleForTestingappropriately limits cache invalidation to test scenarios.
93-108: getUserAgent dispatches correctly with safe fallback.The try/catch wrapper and platform-based dispatch are correct. Returning an empty string for unhandled platforms (e.g., desktop) is a safe fallback that allows
generateApplicationUserAgent()to still produce a valid result.
79-91: User agent lifecycle methods are well-structured.The
initUserAgent()andreleaseUserAgent()correctly:
- Guard against non-mobile platforms
- Manage the initialization flag
- Clear the cache on state changes
FkUserAgent.release()is synchronous (returnsvoid), so it's correctly called withoutawait.core/lib/core.dart (1)
63-65: Sentry utility exports are well-organized and properly located.The new exports for Sentry integration (
sentry_manager.dart,env_loader.dart,sentry_dio_helper.dart) are appropriately placed within the Utils section and follow the existing file organization pattern. All exported files exist at their specified paths.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @core/lib/utils/sentry/sentry_initializer.dart:
- Around line 60-65: The code creates sanitizedHeaders with Map<String,
String>.from(req.headers) but req.headers is SentryRequest.headers which is
nullable and Map.from(null) will throw; update the logic that builds
sanitizedHeaders to handle null safely by using a null-aware fallback (e.g.,
treat req.headers ?? {}), so use req.headers if non-null or an empty map
otherwise before calling Map.from, then apply the existing removeWhere filter
using _blockedHeaderPatterns; ensure references include sanitizedHeaders,
req.headers, SentryRequest.headers, and _blockedHeaderPatterns.
- Around line 76-90: The current reconstruction using a new SentryEvent drops
many fields from the original; instead mutate the incoming event by assigning
the sanitized request to event.request and return the original event (do not
call SentryEvent(...) or rely on the deprecated copyWith); update the code
around SentryEvent construction to set event.request = sanitizedRequest and
return event so fields like message, extra, exceptions, sdk, platform, release,
environment, transaction, etc. are preserved.
🧹 Nitpick comments (1)
core/lib/utils/application_manager.dart (1)
39-49: Consider adding error handling for direct callers.
PackageInfo.fromPlatform()can throw platform exceptions. WhilegetAppVersion()wraps this in try/catch, direct callers ofgetPackageInfo()would receive uncaught exceptions. Consider adding error handling if this method is called directly elsewhere.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/lib/utils/application_manager.dartcore/lib/utils/sentry/sentry_initializer.dart
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
📚 Learning: 2025-12-15T06:24:50.369Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:50.369Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, SentryManager.instance.setUser() intentionally sends PII (account ID, display name, username, email) to Sentry for error tracking. The team plans to document this in the app's privacy policy for transparency and compliance.
Applied to files:
core/lib/utils/sentry/sentry_initializer.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:
core/lib/utils/sentry/sentry_initializer.dartcore/lib/utils/application_manager.dart
📚 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:
core/lib/utils/application_manager.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). (9)
- GitHub Check: analyze-test (fcm)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (forward)
🔇 Additional comments (8)
core/lib/utils/sentry/sentry_initializer.dart (2)
7-21: LGTM! Comprehensive sensitive header filtering.The blocked patterns cover common authentication and session headers. Using
contains()matching ensures variations (e.g.,X-Authorization,auth-token) are also filtered.
23-50: LGTM!The initialization flow is well-structured: loads config, gracefully handles missing config by returning false, and properly configures Sentry options including platform-appropriate breadcrumb tracking.
core/lib/utils/application_manager.dart (6)
9-13: LGTM! Proper singleton implementation.The private constructor with static instance and factory pattern is the idiomatic Dart approach for singletons.
15-21: LGTM!The test override pattern with
@visibleForTestingand lazy initialization is a clean approach for testability.
23-37: LGTM!Cache variables are well-organized and
clearCache()properly resets all state for test isolation.
68-94: LGTM! Lifecycle methods with proper guards.The platform guards and error handling are appropriate. Minor note: if
FkUserAgent.release()throws, the state flags won't be cleared, but the logged warning provides visibility into this edge case.
96-156: LGTM! Platform-aware user agent retrieval with caching.Good design with the initialization check on mobile (Lines 135-140) that logs a warning if called before
initUserAgent(). The caching logic prevents redundant platform calls.
158-163: LGTM!The
trim()call elegantly handles empty user agent cases, preventing trailing whitespace in the result.
7 participants
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
Chores
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.