TF-4268 Cache Sentry config for FCM background re-initialization #4443
Open
TF-4268 Cache Sentry config for FCM background re-initialization #4443
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds encrypted Hive-backed caching for Sentry: new cache entry models ( 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4443. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Member
|
@CodeRabbit please review |
Contributor
|
🧠 Learnings used✅ Actions performedReview triggered.
|
dab246
changed the base branch from
features/tf-4268-enable-sentry-for-android
to
master
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/caching/caching_manager.dart`:
- Around line 279-289: The new CachingManager::clearSentryConfiguration() is
never invoked from account- or full-reset flows, so cached SentryUser can
survive logout/account switch; update the account-scoped clear paths to call
CachingManager.clearSentryConfiguration() (specifically add calls in
clearAccountDataCached() and clearAll()/full-reset paths where account-scoped
caches are cleared) so _sentryConfigurationCacheManager is cleared during
logout/account switch/full reset; ensure this is invoked alongside other
account-cache clears and before any SentryManager.instance.setUser() calls that
might rehydrate user context.
In `@lib/features/caching/extensions/sentry_cache_extensions.dart`:
- Around line 6-22: The extension method
SentryConfigExtension.toSentryConfigurationCache currently omits the nullable
dist field; update to include dist when constructing SentryConfigurationCache
(add dist: dist) so the distribution is preserved, and then add a nullable dist
property to the SentryConfigurationCache class and its Hive adapter
implementation (ensure read/write and typeId handling include dist) so
serialization/deserialization round-trips the dist value.
In
`@lib/features/push_notification/presentation/controller/fcm_message_controller.dart`:
- Around line 126-132: The catch in FcmMessageController::initialAppConfig
currently only logs via logError and swallows failures, allowing the background
handler to continue with partially-initialized bindings/Hive; change this to
propagate the failure back to the caller by rethrowing the caught error (or
throwing a new descriptive exception) after logging so the background pipeline
can abort/cleanup, or alternatively return an explicit failure/result from
initialAppConfig that the background handler checks before calling
_getInteractorBindings(); update callers to handle the propagated error/result
accordingly.
- Around line 114-117: MainBindings().dependencies() is synchronous (returns
void) and cannot be passed to Future.wait which expects Futures; change the code
to call MainBindings().dependencies() before or after awaiting the asynchronous
HiveCacheConfig.instance.setUp(), or wrap the sync call in a Future (e.g.,
Future.sync) so only actual Futures are passed to Future.wait; locate the call
using the symbols MainBindings().dependencies(), Future.wait, and
HiveCacheConfig.instance.setUp() and modify the invocation so Future.wait
receives only Futures (or call dependencies() separately).
In `@lib/features/push_notification/presentation/services/fcm_service.dart`:
- Around line 35-48: The closeStream method currently wraps both
backgroundMessageStreamController?.close() and fcmTokenStreamController?.close()
in one try so if the first close throws the second is skipped; change it to
close each controller in its own try/catch (e.g., separate try blocks for
backgroundMessageStreamController?.close() and for
fcmTokenStreamController?.close()), await each close independently, and log
errors for each using logError with a contextual message (retain the existing
finally that sets backgroundMessageStreamController = null and
fcmTokenStreamController = null).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5c9ea54-056a-4ebd-852e-2c70f8253d82
📒 Files selected for processing (15)
lib/features/caching/caching_manager.dartlib/features/caching/clients/sentry_configuration_cache_client.dartlib/features/caching/clients/sentry_user_cache_client.dartlib/features/caching/config/hive_cache_config.dartlib/features/caching/entries/sentry_configuration_cache.dartlib/features/caching/entries/sentry_user_cache.dartlib/features/caching/exceptions/local_storage_exception.dartlib/features/caching/extensions/sentry_cache_extensions.dartlib/features/caching/manager/sentry_configuration_cache_manager.dartlib/features/caching/utils/caching_constants.dartlib/features/push_notification/presentation/controller/fcm_message_controller.dartlib/features/push_notification/presentation/listener/email_change_listener.dartlib/features/push_notification/presentation/services/fcm_receiver.dartlib/features/push_notification/presentation/services/fcm_service.dartlib/main/bindings/local/local_bindings.dart
lib/features/push_notification/presentation/controller/fcm_message_controller.dart
Outdated
Show resolved
Hide resolved
lib/features/push_notification/presentation/controller/fcm_message_controller.dart
Show resolved
Hide resolved
hoangdat
reviewed
Apr 9, 2026
…tion Signed-off-by: dab246 <tdvu@linagora.com>
dab246
force-pushed
the
features/tf-4268-fcm-sentry-cache
branch
from
f596b0c to
1609fcb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart (1)
57-59: Include stack trace in cache-failure logging for diagnosis.Current warning only logs
$e; addingstwould make production investigation much easier when cache writes fail.🔧 Proposed adjustment
- } catch (e) { - logWarning('SentryEcosystemMixin::_cacheSentryData: Cannot cache Sentry data: $e'); + } catch (e, st) { + logError( + 'SentryEcosystemMixin::_cacheSentryData: Cannot cache Sentry data', + exception: e, + stackTrace: st, + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart` around lines 57 - 59, The catch block in SentryEcosystemMixin::_cacheSentryData only logs the error object `e`; change the catch to capture the stack trace (catch (e, st)) and include the stack (`st`) in the call to logWarning so the warning message contains both the error and its stack trace for diagnosis.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/caching/manager/sentry_configuration_cache_manager.dart`:
- Around line 47-50: The clearSentryConfiguration method currently stops if
_configurationCacheClient.clearAllData() throws, leaving _userCacheClient data
uncleared; update clearSentryConfiguration to attempt both clears independently
by wrapping each call to _configurationCacheClient.clearAllData() and
_userCacheClient.clearAllData() in their own try/catch blocks so the second
clear always runs, capture any thrown errors (or error messages) and after both
attempts either log the failures via the existing logger/Sentry utilities or
rethrow an aggregated error so callers know the operation failed while ensuring
no partial cache cleanup occurs.
In `@lib/features/push_notification/presentation/services/fcm_receiver.dart`:
- Around line 9-12: The background message handler currently calls
FcmService.instance.initialStreamController(),
FcmMessageController.instance.initialize(), await
FcmMessageController.instance.initialAppConfig(), and await
FcmMessageController.instance.setUpSentryConfiguration() on every message;
change this to run expensive DI/cache/Sentry setup only once per isolate by
adding a one-time async init gate (e.g., a static Future<void>? or boolean +
Future) inside the FcmMessageController (or the top-level handler) that ensures
initialAppConfig() and setUpSentryConfiguration() are awaited only on the first
invocation while still calling lightweight initialization like
initialStreamController()/initialize() per message as needed; reference
FcmMessageController.instance.initialAppConfig and
FcmMessageController.instance.setUpSentryConfiguration as the methods guarded by
the gate.
- Around line 61-65: The token dedupe check uses a stale `token` variable so
repeated identical refresh events still pass; update the baseline after handling
or re-read the current token before comparing. In the
`FirebaseMessaging.instance.onTokenRefresh.listen` handler (and/or in the
surrounding initializer that defines `token`), after calling
`FcmService.instance.handleToken(newToken)` set the stored `token` to `newToken`
(or fetch the latest token from FirebaseMessaging and compare against that) so
subsequent refresh events are deduplicated correctly.
---
Nitpick comments:
In
`@lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart`:
- Around line 57-59: The catch block in SentryEcosystemMixin::_cacheSentryData
only logs the error object `e`; change the catch to capture the stack trace
(catch (e, st)) and include the stack (`st`) in the call to logWarning so the
warning message contains both the error and its stack trace for diagnosis.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 24e0574d-89a2-47ee-b8e7-4fbc30ecc436
📒 Files selected for processing (18)
core/lib/utils/sentry/sentry_config.dartlib/features/caching/caching_manager.dartlib/features/caching/clients/sentry_configuration_cache_client.dartlib/features/caching/clients/sentry_user_cache_client.dartlib/features/caching/config/hive_cache_config.dartlib/features/caching/entries/sentry_configuration_cache.dartlib/features/caching/entries/sentry_user_cache.dartlib/features/caching/exceptions/local_storage_exception.dartlib/features/caching/extensions/sentry_cache_extensions.dartlib/features/caching/manager/sentry_configuration_cache_manager.dartlib/features/caching/utils/caching_constants.dartlib/features/mailbox_dashboard/presentation/extensions/ai_scribe/setup_scribe_prompt_url_extension.dartlib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dartlib/features/push_notification/presentation/controller/fcm_message_controller.dartlib/features/push_notification/presentation/listener/email_change_listener.dartlib/features/push_notification/presentation/services/fcm_receiver.dartlib/features/push_notification/presentation/services/fcm_service.dartlib/main/bindings/local/local_bindings.dart
✅ Files skipped from review due to trivial changes (6)
- lib/features/caching/config/hive_cache_config.dart
- lib/features/caching/clients/sentry_configuration_cache_client.dart
- lib/features/caching/clients/sentry_user_cache_client.dart
- lib/features/caching/entries/sentry_user_cache.dart
- lib/features/caching/utils/caching_constants.dart
- lib/features/push_notification/presentation/listener/email_change_listener.dart
🚧 Files skipped from review as they are similar to previous changes (3)
- lib/features/caching/caching_manager.dart
- lib/features/caching/entries/sentry_configuration_cache.dart
- lib/features/push_notification/presentation/controller/fcm_message_controller.dart
lib/features/push_notification/presentation/services/fcm_receiver.dart
Outdated
Show resolved
Hide resolved
…tialization Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/features/caching/manager/sentry_configuration_cache_manager.dart (1)
48-59: Add local error logging for user-cache clear failure path.At Line 58, a thrown error currently bubbles up without a local
logError(..., stackTrace: ...), which makes cleanup failures harder to diagnose in production. Mirror the guarded logging pattern used for configuration clear.🔧 Proposed minimal change
Future<void> clearSentryConfiguration() async { try { await _configurationCacheClient.clearAllData(); } catch (e, st) { logError( 'SentryConfigurationCacheManager::clearSentryConfiguration: Failed to clear config cache', exception: e, stackTrace: st, ); } - await _userCacheClient.clearAllData(); + try { + await _userCacheClient.clearAllData(); + } catch (e, st) { + logError( + 'SentryConfigurationCacheManager::clearSentryConfiguration: Failed to clear user cache', + exception: e, + stackTrace: st, + ); + rethrow; + } }Based on learnings,
SentryManager.instance.setUser()intentionally carries account/user/email identifiers, so user-cache clear failures should be explicitly diagnosable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/features/caching/manager/sentry_configuration_cache_manager.dart` around lines 48 - 59, The clearSentryConfiguration method currently catches and logs failures for _configurationCacheClient.clearAllData but not for _userCacheClient.clearAllData, so any exception from _userCacheClient will bubble without local diagnostics; wrap the await _userCacheClient.clearAllData() call in its own try/catch and call logError with a descriptive message (e.g., 'SentryConfigurationCacheManager::clearSentryConfiguration: Failed to clear user cache'), passing exception: e and stackTrace: st to mirror the existing logging pattern used for the configuration clear.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/features/push_notification/presentation/services/fcm_receiver.dart`:
- Around line 13-29: The current _backgroundInitCompleter/Completer pattern
should be replaced with a lazy-initialized Future that wraps the initialization
sequence (FcmService.instance.initialStreamController(),
FcmMessageController.instance.initialize(),
FcmMessageController.instance.initialAppConfig(),
FcmMessageController.instance.setUpSentryConfiguration()) so errors are
propagated to the original caller and consistently logged via the same logError
path; add a configurable timeout around the combined Future (e.g.,
timeoutDuration) so a hung initialAppConfig() or setUpSentryConfiguration() will
throw and allow recovery, and ensure on timeout or error the cached Future is
cleared so subsequent calls retry (i.e., reset the stored future to null on
failure) rather than leaving isolates blocked.
---
Nitpick comments:
In `@lib/features/caching/manager/sentry_configuration_cache_manager.dart`:
- Around line 48-59: The clearSentryConfiguration method currently catches and
logs failures for _configurationCacheClient.clearAllData but not for
_userCacheClient.clearAllData, so any exception from _userCacheClient will
bubble without local diagnostics; wrap the await _userCacheClient.clearAllData()
call in its own try/catch and call logError with a descriptive message (e.g.,
'SentryConfigurationCacheManager::clearSentryConfiguration: Failed to clear user
cache'), passing exception: e and stackTrace: st to mirror the existing logging
pattern used for the configuration clear.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 927dec63-3ae0-427f-8952-8959a2e60d1c
📒 Files selected for processing (3)
lib/features/caching/manager/sentry_configuration_cache_manager.dartlib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dartlib/features/push_notification/presentation/services/fcm_receiver.dart
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/features/mailbox_dashboard/presentation/mixin/sentry_ecosystem_mixin.dart
lib/features/push_notification/presentation/services/fcm_receiver.dart
Outdated
Show resolved
Hide resolved
… re-initialization Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
(3 files improve in Code Health)
Gates Passed
See analysis details in CodeScene
View Improvements
| File | Code Health Impact | Categories Improved |
|---|---|---|
| sentry_config.dart | 9.69 → 10.00 | Complex Conditional |
| fcm_message_controller.dart | 8.82 → 10.00 | Code Duplication, Complex Conditional |
| email_change_listener.dart | 8.21 → 10.00 | Complex Method, Complex Conditional |
Quality Gate Profile: The Bare Minimum
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Persists SentryConfig and SentryUser to Hive so the FCM background isolate (which runs in a separate Dart isolate) can re-initialize Sentry without making a network call.
Summary by CodeRabbit
New Features
Improvements