Conversation
|
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:
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughConsolidates Sentry symbol upload CI into a platform-aware GitHub Actions step (iOS and Android), makes release creation idempotent, and treats missing artifacts as warnings. Adds SentryConfig serialization ( Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4271. |
tddang-linagora
left a comment
tddang-linagora
left a comment
There was a problem hiding this comment.
All logs in this file are warning log, but all information in this file that is saved to keychain is essential for NSE to handle incoming notification in background. If one ever fails, it will be invisible to both developers and users, making the push notification breaks silently.
- Consider update the logs to error in necessary places.
dab246
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
from
952501e to
22fed14
Compare
dab246
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
from
1a5695b to
df487c6
Compare
dab246
force-pushed
the
features/tf-4269-enable-sentry-for-ios
branch
from
1ae04ad to
b6d0b6c
Compare
Added |
hoangdat
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
from
8dad25d to
c53d43a
Compare
dab246
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
from
ea6e5cf to
9c463ba
Compare
dab246
force-pushed
the
features/tf-4268-enable-sentry-for-android
branch
3 times, most recently
from
4b00f05 to
8dd43d8
Compare
dab246
changed the base branch from
features/tf-4268-enable-sentry-for-android
to
master
dab246
force-pushed
the
features/tf-4269-enable-sentry-for-ios
branch
from
d035b78 to
8690f8d
Compare
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
ios/Podfile (1)
41-47: Consider adding a safeguard for Sentry/HybridSDK version drift.The hard-coded pin on line 46 (version
8.56.2) is currently correct forsentry_flutter 9.8.0. However, ifsentry_flutteris upgraded to require a different HybridSDK version, this pin can become stale and cause silent conflicts duringpod install. Adding a CI check (e.g., comparing the pin against the resolved version inPodfile.lock) or documenting an upgrade procedure would help prevent this maintenance gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Podfile` around lines 41 - 47, The Podfile currently hard-pins pod 'Sentry/HybridSDK', '8.56.2' inside target 'TwakeMailNSE' which can drift from the version required by sentry_flutter; add a safeguard by implementing a CI validation that reads the pinned version in the Podfile (pod 'Sentry/HybridSDK', '8.56.2') and compares it to the resolved Sentry/HybridSDK entry in Podfile.lock (or the CocoaPods resolver output), failing the build if they differ, and/or add a small README entry next to the target block describing the required upgrade steps for sentry_flutter and how to update the HybridSDK pin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yaml:
- Around line 118-168: Parallel matrix legs both call sentry-cli releases new
"$SENTRY_RELEASE" and sentry-cli releases finalize "$SENTRY_RELEASE", causing
duplicate releases and race conditions; fix by making release
creation/finalization idempotent or moving them out of the matrix: either (A)
before calling sentry-cli releases new use sentry-cli releases info
"$SENTRY_RELEASE" -q and only call releases new if that check fails, and ensure
finalize runs only after all upload steps complete, or (B) move the
create/finalize steps into a dedicated non-matrix job that depends on the
Android/iOS symbol-upload jobs so uploads finish before sentry-cli releases
finalize is called.
In `@core/lib/utils/sentry/sentry_config.dart`:
- Around line 101-115: The SentryConfig.toJson() is missing the dist field so
distribution info isn't sent; update the Dart SentryConfig.toJson() to include
'dist': dist, ensure the SentryConfig class retains the dist property, and add a
matching optional dist property to the iOS NSE model (SentryConfig.swift) and
set options.dist = config.dist when initializing Sentry in the NSE
initialization code so NSE events carry the distribution value.
In `@ios/TwakeMailNSE/NotificationService.swift`:
- Around line 17-19: The NSE can reuse process memory and retain prior Sentry
user context; to prevent stale user attribution, clear the Sentry user at the
start of NotificationService.didReceive(_:withContentHandler:) and also during
cleanup in serviceExtensionTimeWillExpire(); call your Sentry clearing API
(e.g., SentryManager.shared.clearUser() or Sentry.setUser(nil)) before any new
event capture and ensure setSentryUser(...) is only called after a fresh valid
session is resolved.
In `@ios/TwakeMailNSE/Utils/SentryManager.swift`:
- Around line 53-63: The capture wrappers (SentryManager.capture(error:) and
capture(message:)) currently only enqueue events and can be lost if the
Notification Service Extension is suspended; after calling
SentrySDK.capture(error: ...) or SentrySDK.capture(message: ...), call
SentrySDK.flush(timeout:) with a short bounded timeout (e.g., 0.3 seconds) to
force delivery before the extension exits; keep the existing isInitialized
guard, perform the capture then immediately call SentrySDK.flush(timeout: 0.3)
(or a configurable short timeout) and ensure this flush is used in early-return
and serviceExtensionTimeWillExpire() paths to improve NSE delivery reliability.
In `@lib/features/mailbox_dashboard/presentation/sentry_ecosystem.dart`:
- Around line 47-49: The wrapper method _saveSentryConfigToKeychain is calling
IOSSharingManager.saveSentryConfigToKeychain without awaiting its Future, so
setUp may complete before the Keychain write finishes; change
_saveSentryConfigToKeychain to be async and await
IOSSharingManager.saveSentryConfigToKeychain when PlatformInfo.isIOS is true,
and do the same for the other equivalent call used later (the second invocation
around the setUp flow) so any errors are observed and the write completes before
setup returns.
---
Nitpick comments:
In `@ios/Podfile`:
- Around line 41-47: The Podfile currently hard-pins pod 'Sentry/HybridSDK',
'8.56.2' inside target 'TwakeMailNSE' which can drift from the version required
by sentry_flutter; add a safeguard by implementing a CI validation that reads
the pinned version in the Podfile (pod 'Sentry/HybridSDK', '8.56.2') and
compares it to the resolved Sentry/HybridSDK entry in Podfile.lock (or the
CocoaPods resolver output), failing the build if they differ, and/or add a small
README entry next to the target block describing the required upgrade steps for
sentry_flutter and how to update the HybridSDK pin.
🪄 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: 774b68cb-3aca-4e4c-a08e-37d96184208d
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
.github/workflows/release.yamlcore/lib/utils/sentry/sentry_config.dartdocs/adr/0084-ios-sentry-dsym-dart-symbols-automation.mdios/Podfileios/Runner.xcodeproj/project.pbxprojios/TwakeMailNSE/Keychain/KeychainController.swiftios/TwakeMailNSE/Model/KeychainSharingSession.swiftios/TwakeMailNSE/Model/SentryConfig.swiftios/TwakeMailNSE/NotificationService.swiftios/TwakeMailNSE/Utils/SentryManager.swiftios/fastlane/Fastfilelib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dartlib/features/mailbox_dashboard/presentation/sentry_ecosystem.dartlib/features/push_notification/data/keychain/keychain_sharing_manager.dartlib/main/utils/ios_sharing_manager.darttest/features/push_notification/data/keychain/keychain_sharing_manager_test.dart
- Podfile: add upgrade note for Sentry/HybridSDK version pin - release.yaml: make sentry-cli releases new idempotent to avoid duplicate release errors when iOS and Android matrix jobs run in parallel - SentryConfig: add missing dist field to toJson() and Swift model, set options.dist in SentryManager so NSE events match uploaded symbols - NotificationService: clear stale Sentry user at start of didReceive and in cleanUp() to prevent cross-request user misattribution - SentryManager: add optional flushTimeout to capture methods; use 0.3s flush in serviceExtensionTimeWillExpire to ensure event delivery before the extension is suspended - SentryEcosystem: await _saveSentryConfigToKeychain to observe errors and ensure write completes before setUp returns
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: dab246 <tdvu@linagora.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…optional - Fastfile: replace unsupported dsym_output_path with archive_path so Gym pins the xcarchive location; dSYMs are read from the archive by CI - release.yaml: add continue-on-error: true so a Sentry upload failure never blocks the release build; replace exit 1 with ::warning:: messages
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: dab246 <tdvu@linagora.com>
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
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.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/release.yaml (1)
119-120:⚠️ Potential issue | 🟠 MajorMove Sentry release lifecycle out of the parallel matrix.
releases info || releases newstill has a check-then-create race when Android and iOS start together, andfinalizestill runs from each matrix leg. Keep artifact uploads matrixed, but create the release once before the matrix and finalize it once after all platform uploads complete.#!/bin/bash # Verify whether Sentry release lifecycle commands still run inside the matrixed job. # Expected: release creation/finalization should be in non-matrix jobs, or otherwise serialized. rg -n -C6 'strategy:|matrix:|sentry-cli releases (info|new|finalize)' .github/workflows/release.yamlAlso applies to: 164-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release.yaml around lines 119 - 120, The Sentry release lifecycle commands (sentry-cli releases info, sentry-cli releases new, and sentry-cli releases finalize) are currently running inside the parallel matrix causing race conditions; move the release creation (the sentry-cli releases info || sentry-cli releases new block) into a non-matrixed job that runs before the matrixed artifact upload job(s), keep artifact uploads in the existing matrixed job(s), and run sentry-cli releases finalize in a single non-matrixed job that runs after all matrixed uploads complete (use job dependencies/needs to serialize create → matrix uploads → finalize). Ensure the exact commands referenced (sentry-cli releases info, sentry-cli releases new, sentry-cli releases finalize) are removed from the matrixed steps and placed in dedicated pre- and post- matrix steps so creation and finalization run only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/release.yaml:
- Around line 144-148: The workflow uploads ProGuard mapping without the build
UUID, so add the Sentry Android Gradle plugin to android/app/build.gradle to
generate and inject the UUID at build time, then modify the release job to pass
that UUID to the upload command: obtain the injected UUID (e.g., from an
environment variable or file emitted by the Gradle plugin) and call sentry-cli
upload-proguard --uuid "$SENTRY_ANDROID_UUID" "$MAPPING_FILE" (referencing
MAPPING_FILE and the sentry-cli upload-proguard invocation in the diff); ensure
the Gradle plugin is configured to emit the UUID in a location or env var your
workflow can read before running the upload step.
---
Duplicate comments:
In @.github/workflows/release.yaml:
- Around line 119-120: The Sentry release lifecycle commands (sentry-cli
releases info, sentry-cli releases new, and sentry-cli releases finalize) are
currently running inside the parallel matrix causing race conditions; move the
release creation (the sentry-cli releases info || sentry-cli releases new block)
into a non-matrixed job that runs before the matrixed artifact upload job(s),
keep artifact uploads in the existing matrixed job(s), and run sentry-cli
releases finalize in a single non-matrixed job that runs after all matrixed
uploads complete (use job dependencies/needs to serialize create → matrix
uploads → finalize). Ensure the exact commands referenced (sentry-cli releases
info, sentry-cli releases new, sentry-cli releases finalize) are removed from
the matrixed steps and placed in dedicated pre- and post- matrix steps so
creation and finalization run only once.
🪄 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: 2bb04864-08e1-42db-8e70-ad5ca1de364a
📒 Files selected for processing (2)
.github/workflows/release.yamlios/fastlane/Fastfile
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/fastlane/Fastfile
| # Upload ProGuard Mapping (Android native de-obfuscation) | ||
| MAPPING_FILE="build/app/outputs/mapping/release/mapping.txt" | ||
| if [ -f "$MAPPING_FILE" ]; then | ||
| echo "Uploading ProGuard mapping from $MAPPING_FILE..." | ||
| sentry-cli upload-proguard "$MAPPING_FILE" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the Android build injects/associates a Sentry ProGuard UUID.
# Expected: either Sentry Gradle plugin configuration is present, or the workflow/build passes
# the generated UUID to `sentry-cli upload-proguard --uuid ...`.
rg -n -C3 'io\.sentry\.android\.gradle|io\.sentry\.proguard-uuid|io\.sentry\.ProguardUuids|upload-proguard|--uuid|--write-properties' --glob '!build/**'Repository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
# Search for Android build files and Sentry configuration
fd -type f -name "build.gradle*" | head -20Repository: linagora/tmail-flutter
Length of output: 236
🏁 Script executed:
# Check the release workflow context around the ProGuard upload
cat -n .github/workflows/release.yaml | sed -n '130,160p'Repository: linagora/tmail-flutter
Length of output: 1821
🏁 Script executed:
# Search for any Sentry configuration in the repo
rg -i "sentry" --type yaml --type gradle --type properties -n | head -30Repository: linagora/tmail-flutter
Length of output: 100
🏁 Script executed:
# Find Android build files
find . -name "build.gradle*" -type f 2>/dev/null | head -20Repository: linagora/tmail-flutter
Length of output: 115
🏁 Script executed:
# Search for Sentry references in YAML and config files
rg -i "sentry" -n | grep -E "(\.yml|\.yaml|\.gradle|android|proguard)" | head -30Repository: linagora/tmail-flutter
Length of output: 1248
🏁 Script executed:
# Look for Fastfile (mentioned in comments at line 154)
find . -name "Fastfile*" -type f 2>/dev/nullRepository: linagora/tmail-flutter
Length of output: 117
🏁 Script executed:
# Check if Sentry SDK is in pubspec.yaml
grep -n "sentry" pubspec.yaml 2>/dev/null || echo "Not found in pubspec.yaml"Repository: linagora/tmail-flutter
Length of output: 117
🏁 Script executed:
# Read the ADR about Android Sentry configuration
cat docs/adr/0076-android-sentry-source-maps-native-symbols-automation.mdRepository: linagora/tmail-flutter
Length of output: 1762
🏁 Script executed:
# Check Android app build.gradle for Sentry Gradle plugin
cat -n android/app/build.gradleRepository: linagora/tmail-flutter
Length of output: 3823
🏁 Script executed:
# Check Android Fastfile for build steps and UUID handling
cat -n android/fastlane/FastfileRepository: linagora/tmail-flutter
Length of output: 1847
🏁 Script executed:
# Check the full release workflow around Sentry release creation and ProGuard upload
cat -n .github/workflows/release.yaml | sed -n '50,165p'Repository: linagora/tmail-flutter
Length of output: 6036
🏁 Script executed:
# Search for sentry-cli release command in workflow
rg "sentry-cli.*release" .github/workflows/release.yaml -n -C2Repository: linagora/tmail-flutter
Length of output: 435
🌐 Web query:
Sentry CLI upload-proguard --uuid release context documentation
💡 Result:
The Sentry CLI command sentry-cli upload-proguard (also available as the newer alias sentry-cli proguard upload as of sentry-cli 3.3.0 in March 2026) uploads ProGuard/R8/DexGuard mapping files to Sentry for deobfuscating Android stack traces. Key options relevant to the query: - --uuid <UUID>: Specifies a custom UUID for the mapping file (replaces deprecated --write-properties). Generate a random UUID and use the same one in your Android SDK configuration (e.g., AndroidManifest.xml meta-data or options.proguardUuid). Example: sentry-cli upload-proguard --uuid 12345678-1234-1234-1234-123456789abc mapping.txt - --release <release>: Associates the mapping with a specific release (deprecated and no longer displayed in Sentry UI; will be removed in future versions). - No explicit --context flag found in sources; "context" likely refers to Source Context (separate feature using debug-files bundle-jvm and --type jvm), not a direct option here. Full usage requires authentication (--auth-token, env SENTRY_AUTH_TOKEN), org/project (--org, --project), and the mapping file path as the last argument. Examples from sources: sentry-cli --auth-token $TOKEN upload-proguard --uuid A_VALID_UUID mapping.txt sentry-cli upload-proguard --uuid A_VALID_UUID --app-id my.app.id --version 1.0.0 --version-code 1 mapping.txt (older style with app/version for association). Prefer the Sentry Android Gradle Plugin for automatic handling. Official docs recommend it over manual CLI. Run sentry-cli upload-proguard --help locally for full flags.
Citations:
- 1: Supply UUID for Proguard Uploads getsentry/sentry-cli#914
- 2: feat(proguard): Add UUID command for mapping files getsentry/sentry-cli#3176
- 3: feat(proguard):
upload-proguard→proguard uploadgetsentry/sentry-cli#3174 - 4: https://docs.sentry.io/platforms/android/enhance-errors/proguard/
- 5: Map Proguard mapping uploaded through sentry-cli using options.proguardUuid from SentryAndroid getsentry/sentry#82821
- 6: https://docs.sentry.io/platforms/android/configuration/gradle.md
- 7: feat(proguard): Upload ProGuard with chunked uploading getsentry/sentry-cli#2918
- 8: Deprecate options for setting release on
upload-proguardcommand getsentry/sentry-cli#2849
🏁 Script executed:
# Check if Sentry Gradle plugin is referenced anywhere
rg "io\.sentry.*gradle|sentry-android" --type gradleRepository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
# Check if there's any UUID generation in the workflow
rg "uuid|UUID" .github/workflows/release.yaml -nRepository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
# Double-check for Sentry plugin in all gradle files
find . -name "*.gradle*" -type f -exec grep -l "sentry" {} \;Repository: linagora/tmail-flutter
Length of output: 48
🏁 Script executed:
# Check if there are any GitHub issues or TODO comments about Sentry Gradle plugin
rg "sentry.*gradle|TODO.*sentry|FIXME.*sentry" -i -nRepository: linagora/tmail-flutter
Length of output: 48
Add Sentry Android Gradle plugin and pass the UUID to the ProGuard upload.
Line 148 uploads mapping.txt without a UUID. To properly match ProGuard mappings with deobfuscated Android events in Sentry, the app must inject a UUID at build time and the CLI must use the same UUID. The recommended approach is to add the Sentry Android Gradle plugin to android/app/build.gradle, which automatically generates and injects the UUID into the app binary. Then pass the generated UUID to sentry-cli upload-proguard --uuid <UUID> "$MAPPING_FILE". Without this UUID linkage, Sentry may accept the mapping upload but fail to deobfuscate obfuscated Android stack traces. See https://docs.sentry.dev/platforms/android/enhance-errors/proguard/ and https://docs.sentry.dev/cli/dif/.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/release.yaml around lines 144 - 148, The workflow uploads
ProGuard mapping without the build UUID, so add the Sentry Android Gradle plugin
to android/app/build.gradle to generate and inject the UUID at build time, then
modify the release job to pass that UUID to the upload command: obtain the
injected UUID (e.g., from an environment variable or file emitted by the Gradle
plugin) and call sentry-cli upload-proguard --uuid "$SENTRY_ANDROID_UUID"
"$MAPPING_FILE" (referencing MAPPING_FILE and the sentry-cli upload-proguard
invocation in the diff); ensure the Gradle plugin is configured to emit the UUID
in a location or env var your workflow can read before running the upload step.
2 participants
Issue
#4269
Blocker
Resolved
Summary by CodeRabbit
New Features
Chores
Documentation