TF-4268 Enable sentry for Android#4270
TF-4268 Enable sentry for Android#4270dab246 wants to merge 31 commits intofeatures/feb26_sprintfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR implements a comprehensive Sentry integration refactoring, environment configuration restructuring, and new Linagora ecosystem feature integration. Changes include: refactoring Sentry initialization with new configuration fields and synchronous public APIs, moving environment file loading from configuration stage to explicit app startup calls, creating a new ecosystem feature layer (datasource/repository/interactor/controller), tracking JMAP state changes in push notification workflows, implementing Sentry configuration caching via Hive, and reorganizing app startup logic into platform-specific runners with shared error handling hooks. Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@lib/features/mailbox_dashboard/data/network/linagora_ecosystem_api.dart`:
- Around line 34-45: The current code casts
properties[LinagoraEcosystemIdentifier.paywallURL] with `as
ApiUrlLinagoraEcosystem?`, which can throw on other ecosystem implementations;
change the logic in the getPaywallUrl flow to check the runtime type instead
(e.g., `is ApiUrlLinagoraEcosystem`) before using it: call getLinagoraEcosystem,
read properties[LinagoraEcosystemIdentifier.paywallURL], if it is an
ApiUrlLinagoraEcosystem return new PaywallUrlPattern(paywallProperty.value),
otherwise log and throw NotFoundPaywallUrl so
EmptyLinagoraEcosystem/DefaultLinagoraEcosystem don’t produce a TypeError.
In
`@lib/features/mailbox_dashboard/domain/state/get_linagora_ecosystem_state.dart`:
- Around line 5-17: The class names contain a typo ("Linagra" vs "Linagora");
rename GettingLinagraEcosystem to GettingLinagoraEcosystem,
GetLinagraEcosystemSuccess to GetLinagoraEcosystemSuccess, and
GetLinagraEcosystemFailure to GetLinagoraEcosystemFailure, update the
constructor parameter/type references (LinagoraEcosystem) and the props override
in GetLinagoraEcosystemSuccess, and then search/replace all usages/imports of
the old class names across the codebase to ensure consistency.
In `@test/features/linagora_ecosystem/deserialize_linagora_ecosystem_test.dart`:
- Around line 359-378: The test name and assertion disagree: the test "Should
treat unknown object as DefaultLinagoraEcosystem" calls
LinagoraEcosystem.deserialize and currently asserts the value is an
AppLinagoraEcosystem; either rename the test to reflect AppLinagoraEcosystem, or
change the assertion/fixture so it verifies DefaultLinagoraEcosystem
instead—update the test title or replace the expectation
(expect(parsed.properties!.values.first, isA<DefaultLinagoraEcosystem>())) or
alter the JSON payload so deserialization produces DefaultLinagoraEcosystem;
references: test name string, LinagoraEcosystem.deserialize, parsed,
AppLinagoraEcosystem, DefaultLinagoraEcosystem.
🧹 Nitpick comments (9)
lib/main/runner/app_error_handlers.dart (1)
19-28: Consider differentiating error handling between debug and release modes.Returning
trueunconditionally suppresses all async errors from crashing the app. While this is appropriate for production (errors are logged to Sentry), in debug mode it may hide issues that developers should see immediately. Consider returningfalsein debug mode to surface errors more visibly during development.💡 Optional: Debug-aware error handling
+import 'package:flutter/foundation.dart'; + // Handle Asynchronous Errors (Futures, Streams, Platform Channels) PlatformDispatcher.instance.onError = (error, stack) { logError( 'PlatformDispatcherError: $error', exception: error, stackTrace: stack, ); - // Return true to prevent the app from crashing. - return true; + // Return true in release to prevent crashes; false in debug to surface errors. + return kReleaseMode; };core/lib/utils/sentry/sentry_config.dart (1)
58-81: Consider adding environment variable support for new config fields.The new fields (
sessionSampleRate,onErrorSampleRate,enableFramesTracking) are not loaded from environment variables inload(), so they always use constructor defaults. This is fine if these will primarily come from the ecosystem config, but you may want to support env-based overrides for consistency.test/features/linagora_ecosystem/linagora_ecosystem_api_test.dart (2)
62-73: UseexpectLaterfor async exception assertions.The method
api.getLinagoraEcosystem(baseUrl)is async, but the test uses synchronousexpect. While this might work, it's better practice to useexpectLaterfor async code to ensure proper Future handling.Suggested fix
// Act & Assert - expect( - () => api.getLinagoraEcosystem(baseUrl), + await expectLater( + api.getLinagoraEcosystem(baseUrl), throwsA(isA<NotFoundLinagoraEcosystem>()), );
93-105: Same async pattern issue forgetPaywallUrltest.Apply the same fix here for consistency and correctness.
Suggested fix
// Act & Assert - expect( - () => api.getPaywallUrl(baseUrl), + await expectLater( + api.getPaywallUrl(baseUrl), throwsA(isA<NotFoundPaywallUrl>()), );test/features/linagora_ecosystem/linagora_ecosystem_repository_impl_test.dart (2)
5-5: Remove TODO comment.The path adjustment comment should be removed before merging.
Suggested fix
-import 'package:tmail_ui_user/features/mailbox_dashboard/data/repository/linagora_ecosystem_repository_impl.dart'; // Adjust path if needed +import 'package:tmail_ui_user/features/mailbox_dashboard/data/repository/linagora_ecosystem_repository_impl.dart';
42-58: Consider usingthenAnswerwith async throw for consistency.The datasource test uses
thenAnswer((_) async => throw apiError)for the error case, but this test usesthenThrow(exception). For async methods,thenAnswerwith async throw is more explicit. Also, consider usingexpectLaterfor async assertions.Suggested fix
// Arrange final exception = Exception('Something went wrong'); - when(mockDatasource.getLinagoraEcosystem(any)).thenThrow(exception); + when(mockDatasource.getLinagoraEcosystem(any)) + .thenAnswer((_) async => throw exception); // Act final call = repository.getLinagoraEcosystem; // Assert - expect(() => call(baseUrl), throwsA(equals(exception))); + await expectLater(call(baseUrl), throwsA(equals(exception)));lib/features/push_notification/presentation/listener/email_change_listener.dart (1)
149-156: Clarify which dependency is missing in the log.The guard checks three values, but the log only mentions the interactor, which can mislead debugging. Consider logging which dependency is null (without PII).
🛠️ Suggested log tweak
- } else { - logError( - 'GetStoredEmailStateInteractor is null', - ); - } + } else { + logError( + 'GetStoredEmailStateInteractor/session/accountId missing ' + '(interactorNull=${_getStoredEmailStateInteractor == null}, ' + 'sessionNull=${_session == null}, accountIdNull=${_accountId == null})', + ); + }lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart (2)
14-42: Add error handling to prevent unhandled exceptions from Sentry initialization.The async operations
toSentryConfig()andinitializeWithSentryConfig()could throw exceptions (e.g., network issues, invalid configuration). Without a try-catch, these exceptions would propagate uncaught, potentially crashing the app or leaving Sentry in an inconsistent state.♻️ Suggested improvement
Future<void> setUpSentry( SentryConfigLinagoraEcosystem ecosystemConfig, ) async { if (ecosystemConfig.enabled != true) { logWarning('SentryEcosystemMixin::_setUpSentry: Sentry is disabled'); return; } final dsn = ecosystemConfig.dsn?.trimmed; final env = ecosystemConfig.environment?.trimmed; if (dsn == null || dsn.isEmpty) { logWarning( 'SentryEcosystemMixin::_setUpSentry: Sentry DSN is invalid (null or empty)'); return; } if (env == null || env.isEmpty) { logWarning( 'SentryEcosystemMixin::_setUpSentry: Sentry Environment is invalid (null or empty)'); return; } - final sentryConfig = await ecosystemConfig.toSentryConfig(); - - await SentryManager.instance.initializeWithSentryConfig(sentryConfig); - - _setupSentryUser(); + try { + final sentryConfig = await ecosystemConfig.toSentryConfig(); + await SentryManager.instance.initializeWithSentryConfig(sentryConfig); + _setupSentryUser(); + } catch (e, st) { + logError( + 'SentryEcosystemMixin::setUpSentry: Failed to initialize Sentry', + exception: e, + stackTrace: st, + ); + } }
17-19: Fix inconsistent method name in log messages.The log messages reference
_setUpSentrybut the method is namedsetUpSentry. This could cause confusion when debugging.📝 Suggested fix
if (ecosystemConfig.enabled != true) { - logWarning('SentryEcosystemMixin::_setUpSentry: Sentry is disabled'); + logWarning('SentryEcosystemMixin::setUpSentry: Sentry is disabled'); return; }Apply similar changes to lines 27 and 33.
lib/features/mailbox_dashboard/domain/state/get_linagora_ecosystem_state.dart
Outdated
Show resolved
Hide resolved
test/features/linagora_ecosystem/deserialize_linagora_ecosystem_test.dart
Outdated
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4270. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@lib/features/mailbox_dashboard/presentation/controller/linagora_ecosystem_controller.dart`:
- Around line 35-40: The method _loadLinagoraEcosystem currently uses `_baseUrl
??=` which caches the first URL and causes stale values when sessions/accounts
change; change it to always reassign `_baseUrl` from
`_baseController.dynamicUrlInterceptors.jmapUrl` (i.e. use `_baseUrl =
_baseController.dynamicUrlInterceptors.jmapUrl`) so each call to
_loadLinagoraEcosystem picks up the latest URL set via setJmapUrl(); keep the
existing empty-check and logging logic intact.
🧹 Nitpick comments (2)
lib/features/push_notification/presentation/listener/email_change_listener.dart (1)
217-221: Consider usinglogWarninginstead oflogErrorfor empty list conditions.An empty email list may be a legitimate state (e.g., no new emails to push, or all emails filtered out by mailbox exclusions) rather than an actual error. Using
logErrorcould generate noise in error monitoring/alerting systems.The contextual information being logged (state value on line 246, mailbox IDs on line 255) is helpful for debugging.
💡 Suggested change
- logError( - 'List email available to push notification is empty' - ); + logWarning( + 'List email available to push notification is empty' + );Apply similar changes to lines 245, 254, and 287.
Also applies to: 244-248, 253-257, 286-290
lib/features/mailbox_dashboard/domain/usecases/get_linagora_ecosystem_interactor.dart (1)
12-21: Consider capturing the stack trace for improved Sentry diagnostics.Since this PR enables Sentry integration, capturing the stack trace in the catch block would provide richer error context. However, this requires updates to the failure class hierarchy:
FeatureFailurewould need to accept an optionalstackTraceparameter, andGetLinagoraEcosystemFailurewould need to pass it through.
lib/features/mailbox_dashboard/presentation/controller/linagora_ecosystem_controller.dart
Show resolved
Hide resolved
What does this means? THe sentry conf should be taken for the JMAP endpoint Edit: after a code review I better understand that this is what is implemented. |
|
Cc @Crash-- |
|
tddang-linagora
left a comment
tddang-linagora
left a comment
There was a problem hiding this comment.
This failure is neither handled nor logged. If this failure is ever yield, no notification will be shown. Either:
- Handle/Recover from this failure
- Send this error to Sentry
- Remove the entire
try-catchif no error can ever be thrown.
If HiveCacheConfig.instance.setUp() ever throws exception when app receives notification in background/terminated, all the app bindings will fail, hence no notification will be shown.
- Add
try-catchand consider send the error log to Sentry.
If FcmUtils.instance.convertFirebaseDataMessageToStateChange(payloadData) ever return null, no notification will be shown, and the notification payload will be silently dropped.
- Add log and consider send it to Sentry
GetFirebaseRegistrationByDeviceIdFailure is handled and recovered to _registerNewFirebaseRegistrationToken(failure.newFcmToken!);, but ONLY WHEN failure.newFcmToken != null. If failure.newFcmToken is ever null, _registerNewFirebaseRegistrationToken will never be called, no new token will be cached, and push notification function will break.
- Consider log and send to Sentry if that ever happens.
We've set up a Sentry log to check before registering a new token; if it's null, we'll send a log to Sentry. |
This deletion is a dangerous action that can crash the app; we should only send error logs to Sentry. |
Whether or not a notification is displayed depends on the returned |
|
lib/features/mailbox_dashboard/presentation/controller/linagora_ecosystem_controller.dart
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/linagora_ecosystem_controller.dart
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/linagora_ecosystem_controller.dart
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Show resolved
Hide resolved
lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart
Show resolved
Hide resolved
lib/features/push_notification/presentation/listener/email_change_listener.dart
Show resolved
Hide resolved
lib/features/push_notification/presentation/listener/email_change_listener.dart
Show resolved
Hide resolved
dab246
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
from
952501e to
22fed14
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@core/lib/utils/app_logger.dart`:
- Around line 135-138: The change adds Level.trace to the list of levels
reported to Sentry in the _shouldReportToSentry function, which will cause every
logTrace call to be sent; if you intend to limit Sentry volume, remove
Level.trace from the returned condition or gate it behind a configurable flag.
Update the _shouldReportToSentry method to either omit Level.trace or consult a
runtime config/feature flag (e.g., a boolean like enableTraceReporting) before
returning true for Level.trace; reference the function name
_shouldReportToSentry and the Level.trace symbol and ensure callers like
logTrace remain unchanged so trace logging still works locally but Sentry
reporting is controlled.
In
`@lib/features/mailbox_dashboard/domain/linagora_ecosystem/sentry_config_linagora_ecosystem.dart`:
- Around line 35-43: toSentryConfig currently sets isAvailable based only on
enabled; change it to require both enabled and a non-empty DSN (matching the
validation in SentryConfig.load). In the toSentryConfig method replace the
isAvailable assignment so it evaluates (enabled ?? false) && (dsn?.isNotEmpty ??
false), keep dsn as dsn ?? '' and release as appVersion so the returned
SentryConfig is only available when enabled and the DSN is present.
In
`@lib/features/push_notification/domain/state/get_mailboxes_not_put_notifications_state.dart`:
- Around line 27-37: The GetMailboxesNotPutNotificationsFailure class adds
fields (userName, emails, currentState) but does not override Equatable props,
so those fields won't be part of equality; update the class
GetMailboxesNotPutNotificationsFailure (which extends FeatureFailure) to
override List<Object?> get props and return a list including exception, onRetry,
userName, emails, and currentState so equality correctly accounts for the new
fields.
In `@lib/features/push_notification/presentation/services/fcm_receiver.dart`:
- Around line 51-55: The code logs raw FCM tokens in the onTokenRefresh listener
(FirebaseMessaging.instance.onTokenRefresh.listen) which risks leaking sensitive
tokens; update the log call in the FcmReceiver::_onHandleFcmToken:onTokenRefresh
block to redact or hash the token (e.g., show only last 4 chars or a SHA-256
prefix) instead of printing the full newToken, and apply the same redaction to
the initial token log that compares against token; keep calling
FcmService.instance.handleToken(newToken) with the full token but ensure only
the redacted/hashed value is sent to log statements.
- Around line 51-65: The onTokenRefresh listener in
FirebaseMessaging.instance.onTokenRefresh (registered in
FcmReceiver::_onHandleFcmToken / onInitialFcmListener) can be registered
multiple times; store the returned StreamSubscription as a class member (e.g.,
_tokenRefreshSub), cancel it if non-null and not cancelled before creating a new
subscription, and set it to null on dispose; alternatively guard registration so
you only call FirebaseMessaging.instance.onTokenRefresh.listen once per
FcmReceiver instance (check _tokenRefreshSub != null) and ensure cancellation in
the class dispose/teardown path and in _setUpComponentsFromSession before
re-listening.
In
`@test/features/linagora_ecosystem/get_linagora_ecosystem_interactor_test.dart`:
- Around line 76-94: The test is missing an await on the asynchronous assertion
causing flakiness; add await before the expectLater(...) call that asserts on
stream (the call using predicate<Either> checks for GettingLinagoraEcosystem
then GetLinagoraEcosystemFailure) so the test waits for the emitted events to
complete.
- Around line 38-56: The test is missing awaiting the Future returned by
expectLater, causing flakiness; update the test to await expectLater(stream,
emitsInOrder([predicate<Either>((either)=>...), predicate<Either>((either)=>
success is GetLinagoraEcosystemSuccess && success.linagoraEcosystem ==
ecosystem),])), ensure the enclosing test function is declared async/returns
Future so you can use await, and then run the existing untilCalled/verify calls
after the awaited expectLater completes.
🧹 Nitpick comments (5)
core/lib/utils/app_logger.dart (1)
204-213: Consider addingextrasparameter tologTracefor consistency.The
_internalLogfunction passesextrastoSentryManager.instance.captureMessagewhen level isLevel.trace, but the publiclogTracefunction doesn't expose anextrasparameter. Callers cannot pass contextual data with trace logs.♻️ Suggested enhancement
void logTrace( String? message, { bool webConsoleEnabled = false, + Map<String, dynamic>? extras, }) { _internalLog( message, level: Level.trace, webConsoleEnabled: webConsoleEnabled, + extras: extras, ); }lib/features/push_notification/presentation/controller/fcm_message_controller.dart (1)
89-121: Consider short-circuiting when init fails.Right now, init errors are logged but the flow continues, which can lead to follow-up null-path logs. Returning a boolean from
_initialAppConfigand early-exiting the handler would keep the flow cleaner.♻️ Suggested refactor
- Future<void> _initialAppConfig() async { + Future<bool> _initialAppConfig() async { try { await Future.wait([ MainBindings().dependencies(), HiveCacheConfig.instance.setUp() ]); await Future.sync(() { HomeBindings().dependencies(); MailboxDashBoardBindings().dependencies(); FcmInteractorBindings().dependencies(); }); _getInteractorBindings(); + return true; } catch (e, st) { logError( 'FcmMessageController::_initialAppConfig: throw exception', exception: e, stackTrace: st, ); + return false; } }- await _initialAppConfig(); - _getAuthenticatedAccount(stateChange: stateChange); + if (await _initialAppConfig()) { + _getAuthenticatedAccount(stateChange: stateChange); + }core/lib/utils/sentry/sentry_initializer.dart (1)
24-51: Clarify init() return semantics when appRunner is null.The doc says init returns true when Sentry “handled app execution,” but
appRunnercan be null. Consider tightening the doc or enforcing non-null when you need that guarantee.lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart (1)
10-12: Consider applying the user immediately in initSentryUser.If
initSentryUseris called aftersetUpSentry, the user won’t be pushed unless another call happens. You could set the user immediately when Sentry is already initialized (or document the required call order).Also applies to: 44-48
core/lib/utils/sentry/sentry_config.dart (1)
58-71: Normalize SENTRY_ENABLED to avoid silent misconfiguration.The comparison at line 63 uses
sentryAvailable == 'true', which is case-sensitive and doesn't trim whitespace. If set to"TRUE"or" true ", it silently treats the value as false. This is inconsistent with how SENTRY_DSN and SENTRY_ENVIRONMENT are validated (both use.trim()on lines 68-69). Consider normalizing with.trim().toLowerCase()to be more robust:- final sentryAvailable = dotenv.get('SENTRY_ENABLED', fallback: 'false'); + final sentryAvailable = dotenv + .get('SENTRY_ENABLED', fallback: 'false') + .trim() + .toLowerCase();
lib/features/mailbox_dashboard/domain/linagora_ecosystem/sentry_config_linagora_ecosystem.dart
Show resolved
Hide resolved
lib/features/push_notification/domain/state/get_mailboxes_not_put_notifications_state.dart
Show resolved
Hide resolved
test/features/linagora_ecosystem/get_linagora_ecosystem_interactor_test.dart
Outdated
Show resolved
Hide resolved
test/features/linagora_ecosystem/get_linagora_ecosystem_interactor_test.dart
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@lib/features/caching/caching_manager.dart`:
- Around line 258-264: The log message in the saveSentryUser method incorrectly
uses the copy-pasted tag "CachingManager::saveSentryConfiguration"; update the
success log inside Future<void> saveSentryUser(SentryUser sentryUser) to use the
correct tag "CachingManager::saveSentryUser: Sentry user saved successfully" so
log filtering accurately reflects the method that ran.
- Around line 273-279: In getSentryUser(), stop logging the full SentryUser
object (contains PII); instead log only non-sensitive metadata: indicate whether
a user was retrieved, and safe flags such as presence of email/username and an
anonymized identifier (e.g., truncated or hashed user id) for correlation.
Update the log call in CachingManager::getSentryUser to remove
account/display/email/username values and emit only those non-PII fields.
In `@lib/features/caching/clients/sentry_user_cache_client.dart`:
- Around line 7-8: Add a new constant to CachingConstants named
sentryUserCacheBoxName with value 'sentry_user_cache_box' and update
SentryUserCacheClient.tableName to return
CachingConstants.sentryUserCacheBoxName instead of
CachingConstants.sentryConfigurationCacheBoxName; ensure the new constant is
declared as static const String sentryUserCacheBoxName = 'sentry_user_cache_box'
and only SentryUserCacheClient (not SentryConfigurationCacheClient) references
it so the two caches use separate Hive box names.
In
`@lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart`:
- Around line 17-49: When ecosystemConfig.enabled != true in
SentryEcosystemMixin::setUpSentry, clear any cached Sentry state instead of
returning immediately: reset the in-memory _sentryUser (set to null), ensure
SentryManager is not left initialized (call its shutdown/clear method if
available, e.g. SentryManager.instance.close/clear), and persist the disabled
state by invoking _saveSentryConfiguration with a null/empty sentryConfig and
sentryUser so FcmMessageController.setUpSentryConfiguration won't reinitialize
Sentry from stale cache. Ensure these steps occur before returning from
setUpSentry.
In
`@lib/features/push_notification/domain/state/get_email_changes_to_push_notification_state.dart`:
- Around line 27-35: The GetEmailChangesToPushNotificationFailure class
currently defines a currentState field but does not include it in equality, so
update the class to override the props getter (used by
FeatureFailure/EquatableMixin) to include currentState (in addition to the
existing exception, stackTrace, onRetry values from FeatureFailure); ensure the
overridden props returns a list containing super.props elements or the same
exception/stackTrace/onRetry items plus currentState so equality comparisons
account for currentState.
In
`@lib/features/push_notification/presentation/controller/fcm_message_controller.dart`:
- Around line 157-162: The log message uses the old private method name
`_initialAppConfig`; update the message to reference the current method name by
changing the text passed to logError from
'FcmMessageController::_initialAppConfig: throw exception' to
'FcmMessageController::initialAppConfig: throw exception' (locate the call to
logError inside FcmMessageController where initialAppConfig's catch block
handles exceptions and update the string accordingly).
🧹 Nitpick comments (3)
lib/features/push_notification/presentation/services/fcm_service.dart (1)
43-56: Await controller closes and ensure cleanup runs infinally.
StreamController.close()is async; without awaiting you can miss async failures and race teardown. Consider making the method async and moving null resets tofinally.Proposed refactor
- void closeStream() { + Future<void> closeStream() async { try { - backgroundMessageStreamController?.close(); - fcmTokenStreamController?.close(); - - backgroundMessageStreamController = null; - fcmTokenStreamController = null; + await backgroundMessageStreamController?.close(); + await fcmTokenStreamController?.close(); } catch (e, st) { logError( 'FcmService::closeStream: throw exception', exception: e, stackTrace: st, ); + } finally { + backgroundMessageStreamController = null; + fcmTokenStreamController = null; } }core/lib/presentation/state/failure.dart (1)
13-19: Consider whetherstackTraceshould be part of Equatable props.Including it makes each failure instance unique and can trigger extra UI rebuilds; if it’s only for logging, you may want to exclude it from equality.
Optional tweak
- List<Object?> get props => [exception, stackTrace, onRetry]; + List<Object?> get props => [exception, onRetry];lib/features/push_notification/presentation/controller/fcm_message_controller.dart (1)
134-164: Consider proceeding with Sentry config even if user is null.Currently,
setUpSentryConfigurationreturns early ifsentryUseris null (Line 153). However, Sentry can still function for error tracking without a user context. Consider initializing Sentry with the config regardless of user availability:Suggested approach
await SentryManager.instance.initializeWithSentryConfig(sentryConfig); final sentryUser = await cachingManager.getSentryUser(); if (sentryUser == null) { logTrace('FcmMessageController::setUpSentryConfiguration: Sentry user is null'); - return; + // Sentry initialized without user context - this is acceptable + } else { + SentryManager.instance.setUser(sentryUser); } - - SentryManager.instance.setUser(sentryUser);
lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart
Show resolved
Hide resolved
lib/features/push_notification/domain/state/get_email_changes_to_push_notification_state.dart
Show resolved
Hide resolved
lib/features/push_notification/presentation/controller/fcm_message_controller.dart
Show resolved
Hide resolved
…or `Flutter >= 3.3`
…background re-initialization.
dab246
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
from
ea6e5cf to
9c463ba
Compare
|
dab246
force-pushed
the
features/feb26_sprint
branch
from
391f304 to
e8ad5f4
Compare
|
|
|
||
| void _handleBackgroundMessageAction(Map<String, dynamic> payloadData) async { | ||
| log('FcmMessageController::_handleBackgroundMessageAction():payloadData: $payloadData'); | ||
| logTrace('FcmMessageController::_handleBackgroundMessageAction():payloadData keys: ${payloadData.keys.toList()}'); |
There was a problem hiding this comment.
current log in Sentry
FcmMessageController::_handleBackgroundMessageAction():payloadData keys: [94db052675894262fb8644f9517aaf25e8246c5c2eae08b34da40b748f30ec0b:EmailDelivery, 94db052675894262fb8644f9517aaf25e8246c5c2eae08b34da40b748f30ec0b:Email, 94db052675894262fb8644f9517aaf25e8246c5c2eae08b34da40b748f30ec0b:Mailbox]
Please log each item in key: value format
| logTrace('FcmMessageController::_handleBackgroundMessageAction(): stateChange is null'); | ||
| return; | ||
| } else { | ||
| logTrace('FcmMessageController::_handleBackgroundMessageAction(): stateChange: ${stateChange.toString()}'); |
There was a problem hiding this comment.
StateChange was not well in log
FcmMessageController::_handleBackgroundMessageAction(): stateChange: Sqb
|
|
||
| final sentryUser = await cachingManager.getSentryUser(); | ||
| if (sentryUser == null) { | ||
| logTrace('FcmMessageController::setUpSentryConfiguration: Sentry user is null'); |
There was a problem hiding this comment.
most of the time log have this message. IMO, in background mode, we cannot get sentry user from cache, how about to get at least user email?
4 participants
Issue
#4268
Configuration
In
linagora-ecosystem.properties:The Ecosystem API JSON response:
{ "sentry": { "enabled": true, "dsn": "https://dsn@sentry.io/123", "environment": "dev" } }Resolved
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes