fix(Android): Push notification on killed apps #6863
Conversation
…andling and migrate notification logic - Added expo-notifications for push notification management, replacing react-native-notifications. - Implemented device token registration and notification response handling. - Enhanced badge count management and notification dismissal methods. - Set up notification categories for iOS to support actions like reply and video conference responses. - Updated MainApplication to reflect new notification architecture.
…ent custom FCM handling - Removed react-native-notifications dependency and related mock configurations. - Introduced RCFirebaseMessagingService for handling FCM messages and routing to CustomPushNotification. - Updated CustomPushNotification to manage notifications without react-native-notifications, enhancing E2E decryption and MessagingStyle support. - Adjusted MainApplication and notification classes to reflect the new architecture and improve notification processing. - Cleaned up unused imports and code related to the previous notification system.
…e reply handling - Removed react-native-notifications dependency and related code from the project. - Implemented a custom reply notification handler in ReplyNotification to manage direct replies from iOS notifications. - Updated AppDelegate to configure the new reply notification handler. - Adjusted Podfile and Podfile.lock to reflect the removal of react-native-notifications and added necessary Expo modules. - Cleaned up imports and ensured compatibility with the new notification architecture.
- Introduced a new video conference notification system, replacing the previous handling with a custom implementation. - Added VideoConfNotification class to manage incoming call notifications with accept/decline actions. - Implemented VideoConfBroadcast receiver to handle notification actions and store them for the JS layer. - Updated MainActivity to process video conference intents and integrate with the new notification system. - Enhanced getInitialNotification to check for pending video conference actions. - Updated AndroidManifest.xml to register the new broadcast receiver. - Cleaned up related code and ensured compatibility with the new notification architecture.
…or iOS - Added support for video conference notifications in the iOS NotificationService. - Implemented logic to process incoming video call notifications, including handling call status and displaying appropriate alerts. - Updated Payload and NotificationType models to accommodate video conference data. - Enhanced getInitialNotification to check for video conference actions on iOS using expo-notifications. - Improved error handling for notification responses.
- Enhanced the onNotification function to streamline processing of video conference notifications, including accept and decline actions. - Updated getInitialNotification to handle video conference actions more effectively, ensuring proper event dispatching based on user interaction. - Improved error handling and code readability by reducing nested conditions and clarifying logic flow.
…encies - Removed @notifee/react-native and @react-native-firebase/messaging from the project, along with related code and configurations. - Updated notification handling to utilize expo-notifications instead, ensuring a more streamlined approach to push notifications. - Cleaned up package.json, yarn.lock, and Podfile.lock to reflect the removal of obsolete dependencies. - Deleted background notification handler and adjusted notification settings management accordingly.
…ng to TurboModules - Replaced the existing VideoConfModule and related classes with a TurboModule implementation for improved performance and integration. - Updated MainApplication to use VideoConfTurboPackage instead of the legacy VideoConfPackage. - Enhanced notification handling by introducing a new User-Agent header in relevant requests. - Removed obsolete Java classes and streamlined the notification architecture to utilize Kotlin for better maintainability. - Improved the handling of video conference actions and ensured compatibility with the new TurboModule system.
- Added a 3-second timeout for fetching avatars in CustomPushNotification to prevent blocking the FCM service. - Enhanced error handling in ReplyBroadcast to return early if the bundle is null. - Updated push notification configuration to re-register the push token with the server if the user is authenticated. - Modified appStateMiddleware to handle potential errors when clearing notifications. - Improved notification delivery logic in iOS NotificationService to handle cases with no messageId or notification content.
- Added functionality to encode and store the full payload from the server in the userInfo dictionary under the key "ejson" for improved navigation.
- Added a method to fetch user avatars from the server and attach them to notifications in NotificationService. - Introduced a userAgent property in Bundle for consistent User-Agent string usage across API requests.
- Updated avatar URI generation to differentiate between direct messages and group/channel notifications, ensuring the correct avatar is fetched based on the message type. - Refactored avatar fetching logic in NotificationService to improve clarity and maintainability. - Added support for Communication Notifications API to update notification content with sender avatars.
- Updated CustomPushNotification to use a volatile Bundle for thread safety. - Enhanced VideoConfBroadcast to include host and callId in the notification payload. - Improved notification validation in the onNotification function to ensure required fields are present before dispatching app initialization. - Refactored iOS NotificationService to manage background tasks more effectively and ensure proper cleanup. - Added NSUserActivityTypes for message intents in Info.plist files for both iOS targets.
- Enhanced message parsing in CustomPushNotification to handle cases where the message format may not include a colon. - Updated Ejson to ensure proper URL encoding for avatar paths, improving reliability in generating user and room avatars. - Added error handling in onNotification to catch parsing errors for video conference notifications, logging warnings for failed attempts. - Improved room name assignment logic to handle cases where sender information may be missing. - Updated iOS ReplyNotification to provide user feedback for invalid notification data and failed replies.
- Enhanced CustomPushNotification to dynamically set notification titles for groups, direct messages, and omnichannel based on the Ejson payload. - Updated Ejson class to include a room name field for better context in notifications. - Improved iOS NotificationService to determine notification titles based on payload type, ensuring accurate representation of sender or room names.
…hat.ReactNative into feat.push-avatar-ios
…ding errors - Updated avatar URI generation to specify "UTF-8" encoding instead of StandardCharsets.UTF_8. - Changed exception handling to catch UnsupportedEncodingException specifically for better clarity in error logging.
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
177-220: Address TODO: Database lock issues with concurrent access.The TODO comment on line 182 flags known lock issues when accessing the WatermelonDB SQLite database concurrently. Direct SQLite access from notification code while WatermelonDB may be active can cause
SQLITE_LOCKEDorSQLITE_BUSYerrors, resulting in notification processing failures.Additionally, there's no timeout for database operations, which could cause notification handling to hang indefinitely if a lock is held.
Consider these options:
- Implement a retry mechanism with exponential backoff for locked database scenarios
- Add a timeout to database operations (e.g., using
SQLiteDatabase.OpenParamswith busy timeout on API 27+)- Use WatermelonDB's own query mechanisms if available from native code
- Cache room encryption status in a separate, notification-safe storage
#!/bin/bash # Check if WatermelonDB provides native query APIs that could be used instead rg -nP 'WatermelonDB|SQLiteAdapter' --type=java --type=kotlin -C3 # Search for other direct SQLite access patterns in the codebase rg -nP 'SQLiteDatabase\.open' --type=java -C3
🧹 Nitpick comments (8)
app/lib/notifications/index.ts (2)
24-33: Missingreturnin catch block causes fallthrough to regular notification handling.When EJSON parsing fails for video conf actions (ACCEPT/DECLINE), execution continues past the catch block and attempts to re-parse the same payload at line 38. If the second parse also fails, the notification is handled as if no ejson was present, triggering
appInit(). If parsing succeeds the second time (unlikely but theoretically possible with race conditions), the video conf action could be mishandled as a regular notification.Consider adding a
returnafter the warning to exit early on parse failure:🔎 Proposed fix
} catch (e) { console.warn('Failed to parse video conf notification:', e); + return; } } }
58-63: LGTM! Clean room name resolution with null-safe fallbacks.The refactored logic is more readable and handles edge cases well with nullish coalescing. The optional chaining on
senderis appropriate defensive coding since the EJSON payload structure isn't strictly validated.Note: The
IEjsoninterface (line 12) declaressenderas a required object with required string fields, but the runtime optional chaining suggestssendercan be undefined. Consider updating the interface to match reality:sender?: { username?: string; name?: string };ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1-3349: Note: Xcode project files are machine-generatedThis
.pbxprojfile is automatically generated and maintained by Xcode and CocoaPods. The changes appear to reflect updates to Pod dependencies (renaming/reorganizing Pod frameworks fromNotificationServicetoRocket.Chatnaming scheme) and associated build configurations.If there are any build issues after this change:
- Ensure
Podfile.lockis committed alongside this change- Team members should run
pod installafter pulling- Consider regenerating with
pod install --repo-updateif issues persistandroid/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
211-217: Consider adding context about blocking behavior in the docstring.The
getAvatarfunction delegates toNotificationHelper.fetchAvatarBitmapwhich is a synchronous blocking call with a 3-second timeout. While this is acceptable for notification handling, consider noting this blocking behavior in the docstring for future maintainers.Suggested docstring enhancement
/** * Fetches avatar bitmap from URI using Glide. * Returns null if fetch fails or times out, in which case notification will display without avatar. + * Note: This is a blocking call with a 3-second timeout. */ private fun getAvatar(uri: String): Bitmap? { return NotificationHelper.fetchAvatarBitmap(context, uri, null) }android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
62-85: Well-implemented avatar fetching with proper timeout and error handling.The 3-second timeout is appropriate given FCM's constraints, and the interrupt status is correctly restored on
InterruptedException. The null/empty check at the start prevents unnecessary work.One consideration:
Glide.with(context)should ideally use an Application context to avoid potential memory leaks if an Activity context is passed. In the current usage (notification handling), the context is typically the application context, but explicitly usingcontext.getApplicationContext()would be safer.Suggested defensive improvement
try { // Use a 3-second timeout to avoid blocking the FCM service for too long // FCM has a 10-second limit, so we need to fail fast and use fallback icon - Bitmap avatar = Glide.with(context) + Bitmap avatar = Glide.with(context.getApplicationContext()) .asBitmap() .apply(RequestOptions.bitmapTransform(new RoundedCorners(10))) .load(uri)ios/NotificationService/NotificationService.swift (1)
86-100: Sender prefix stripping logic handles both username and display name.The logic to strip the sender prefix from the message body is thorough, trying both the username and the display name as fallbacks. This ensures consistent behavior across different message formats.
Minor observation: the cast
bestAttemptContent.body as? Stringon line 89 is unusual sincebodyis already aStringproperty onUNMutableNotificationContent. This cast will always succeed.Simplify the body access
- if let body = bestAttemptContent.body as? String { + let body = bestAttemptContent.body + if true {Or simply remove the
if letand accessbestAttemptContent.bodydirectly.android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
303-317: Duplicate title determination logic should be extracted.The logic for determining
notificationTitle(lines 303-317) andconversationTitle(lines 465-481) is nearly identical. This duplication increases maintenance burden and risk of inconsistency.Extract common title logic to a helper method
+ /** + * Determines the display title based on notification type. + */ + private String determineTitle(Ejson ejson, String defaultTitle) { + if (ejson == null || ejson.type == null) { + return defaultTitle; + } + + if ("p".equals(ejson.type) || "c".equals(ejson.type)) { + // For groups/channels, use room name if available + return (ejson.name != null && !ejson.name.isEmpty()) ? ejson.name : defaultTitle; + } else if ("d".equals(ejson.type)) { + // For direct messages, use default title (sender name) + return defaultTitle; + } else if ("l".equals(ejson.type)) { + // For omnichannel, use sender name if available + return (ejson.sender != null && ejson.sender.name != null && !ejson.sender.name.isEmpty()) + ? ejson.sender.name : defaultTitle; + } + + return defaultTitle; + }Then use
determineTitle(ejson, title)in bothbuildNotificationandnotificationStyle.Also applies to: 465-481
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
267-291: Improved parsing and validation logic.The double-parsing fallback (lines 269-280) and required field validation (lines 288-290) improve robustness when handling potentially inconsistent private key data.
The need for double-parsing suggests inconsistent JSON encoding upstream. Consider standardizing the private key storage format to eliminate the fallback path and reduce complexity.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/Ejson.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/Encryption.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/storage/MMKVKeyManager.javaapp/lib/notifications/index.tsios/NotificationService/Info.plistios/NotificationService/NotificationService.entitlementsios/NotificationService/NotificationService.swiftios/ReplyNotification.swiftios/RocketChatRN.xcodeproj/project.pbxprojios/RocketChatRN/Info.plistios/RocketChatRN/RocketChatRN.entitlementsios/Shared/Extensions/Bundle+Extensions.swiftios/Shared/Models/Payload.swiftios/Shared/RocketChat/API/Request.swift
💤 Files with no reviewable changes (1)
- android/app/src/main/java/chat/rocket/reactnative/storage/MMKVKeyManager.java
🧰 Additional context used
🧬 Code graph analysis (8)
ios/Shared/Extensions/Bundle+Extensions.swift (1)
app/lib/constants/userAgent.ts (1)
userAgent(3-5)
android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(107-520)
android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(107-520)
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
context(22-232)
ios/ReplyNotification.swift (1)
ios/Shared/RocketChat/RocketChat.swift (1)
sendMessage(39-75)
ios/Shared/RocketChat/API/Request.swift (1)
app/lib/constants/userAgent.ts (1)
userAgent(3-5)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
app/lib/encryption/utils.ts (1)
decodePrefixedBase64(200-219)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (3)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (2)
context(22-232)getAvatar(215-217)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
Encryption(107-520)android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
NotificationHelper(22-86)
⏰ 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). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🔇 Additional comments (28)
app/lib/notifications/index.ts (1)
47-50: Good defensive guard for incomplete notification data.The early return when
rid,type, orhostare missing prevents navigation failures with incomplete data. Falling back toappInit()is appropriate as it initializes the app without attempting invalid deep linking.ios/ReplyNotification.swift (2)
91-98: Good improvement: User-facing error feedback.The addition of a failure notification for invalid notification data is a solid improvement over silent failure. Users now receive clear feedback when reply processing cannot proceed.
127-127: Improved error message clarity.The updated message "Failed to send reply." is more concise and consistent with the error message at line 93.
ios/Shared/Models/Payload.swift (1)
11-15: LGTM! Username field enriches caller information.The addition of the optional
usernamefield to theCallerstruct aligns with the Android changes and enables richer notification displays.ios/RocketChatRN/RocketChatRN.entitlements (1)
15-16: LGTM! Communication entitlement properly configured.The
com.apple.developer.usernotifications.communicationentitlement enables communication notification features on iOS, which supports the INSendMessageIntent integration.ios/NotificationService/NotificationService.entitlements (1)
5-6: LGTM! Notification service extension entitlement properly configured.The communication entitlement is correctly added to the notification service extension, enabling it to handle communication notifications.
ios/NotificationService/Info.plist (1)
36-46: LGTM! INSendMessageIntent support properly configured.The addition of both
NSUserActivityTypesandNSExtensionAttributescorrectly enables the notification service extension to handle communication intents for improved notification threading and display.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt (1)
46-57: LGTM! Video conference payload enriched with host and callId.The additions properly extract the
hostandcallIdfrom extras with safe defaults, enabling the JS layer to handle video conference events with complete context.ios/RocketChatRN/Info.plist (1)
111-114: LGTM! App-level INSendMessageIntent support configured.The
NSUserActivityTypesdeclaration at the app level complements the notification service extension configuration, completing the communication notification integration.ios/Shared/Extensions/Bundle+Extensions.swift (1)
15-23: LGTM! User-Agent generation is well-implemented.The static
Bundle.userAgentproperty provides centralized UA string generation with good defensive coding (?? "unknown"fallbacks for missing values). The format "RC Mobile; ios {systemVersion}; v{appVersion} ({build})" clearly identifies the native app.ios/Shared/RocketChat/API/Request.swift (1)
58-58: No verification needed—the iOS implementation is correct.The
Bundle.userAgentreturns"RC Mobile; ios {version}; v{appVersion} ({build})", which matches the exact format used inapp/lib/methods/helpers/fetch.tsfor API requests. The backend already expects and requires this format, as documented in the fetch.ts comment referencingapp/statistics/server/lib/UAParserCustom.js.The confusion in the original comment stems from
userAgent.ts, which provides browser-style UA strings for WebView components—not for API requests, which is what the iOS change addresses. Backend compatibility is not a concern.Likely an incorrect or invalid review comment.
android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java (2)
27-28: 15-second timeout for cold start is reasonable but close to FCM limits.The increased timeout from 3s to 15s accommodates cold start scenarios where React Native initialization takes longer. However, FCM background message handling has platform-specific time limits (typically 10-20 seconds on Android). If React context isn't available within this window, the notification may still fail silently.
The current implementation handles timeout gracefully via
onTimeoutcallback, which is good.
110-133: Background thread callback invocation is intentional and documented.The comment on line 117 clarifies that the callback is called on the background thread to support notification image loading. This is a reasonable design choice, but callers must be aware they're not on the main thread.
The decryption call signature change (
decryptMessage(ejson)without Context) aligns with theEncryption.javarefactor that now usesMainApplication.instanceinternally.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
159-166: Avatar fetching with graceful fallback is well implemented.The avatar fetching logic safely handles the case where the avatar URI is null or the fetch fails/times out. The conditional
setLargeIcononly applies when a bitmap is successfully retrieved, ensuring the notification displays correctly even without an avatar.Also applies to: 181-185
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (2)
63-116: Centralized avatar URI building with proper URL encoding.The
buildAvatarUrihelper method consolidates the common logic for constructing avatar URIs, which improves maintainability. The differentiation between DM (sender avatar) and group/channel (room avatar) paths is correct.URL encoding with UTF-8 is the right approach for handling special characters in usernames. Note that
UnsupportedEncodingExceptionis extremely unlikely since UTF-8 is guaranteed to be available on all Java implementations, but catching it is still correct practice.Security note: The auth tokens are passed as query parameters (
rc_token,rc_uid), which is acceptable assuming all server communication uses HTTPS. These tokens would be visible in server access logs.
118-136: Caller avatar URI generation follows the same pattern.The
getCallerAvatarUrimethod correctly reusesbuildAvatarUriand properly handles the case where caller or username is null.android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java (1)
129-129: API call updated to match Encryption signature change.The call to
Encryption.shared.encryptMessageContentcorrectly reflects the removal of the Context parameter, aligning with the broader refactor whereEncryptionnow usesMainApplication.instanceinternally for context-dependent operations.ios/NotificationService/NotificationService.swift (3)
17-33: Clean refactoring with guard-based routing.The restructured
didReceivemethod with guard statements improves readability and provides clear routing based on notification type. Early exit for invalid payloads prevents unnecessary processing.
269-274: Proper timeout configuration for notification service extension.Both
timeoutIntervalForRequestandtimeoutIntervalForResourceare set to 3 seconds, which is critical for notification service extensions that have strict time limits (~30 seconds total). The comment correctly explains the distinction between these timeout types.
157-168: Dummy recipient for iOS group conversation handling is documented.The comment on line 159 explains why a placeholder recipient is needed for iOS to properly treat the notification as a group conversation. This is a known iOS quirk for
INSendMessageIntent.android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (4)
50-63: Thread-safe pending notification queue implementation.Using
CopyOnWriteArrayListensures thread safety when notifications arrive concurrently before React initialization. ThePendingNotificationwrapper correctly captures both context and bundle for deferred processing.
90-109: Queued notification processing on React context initialization.The
setReactContextmethod now correctly processes any notifications that were queued while waiting for React to initialize. The exception handling around individual notification processing prevents one failed notification from blocking others.
201-213: E2E decryption now happens synchronously without React dependency.This is a significant improvement for killed-app scenarios. The decryption uses
MainApplication.instanceinternally (viaEncryption.shared), eliminating the need to wait for React context for E2E notifications.One concern: if decryption fails (line 211), the notification is silently dropped. Consider whether a fallback (e.g., showing encrypted indicator or generic message) would improve user experience.
Is silently dropping E2E notifications on decryption failure the intended behavior? Users might not realize they received a message if decryption fails.
143-152: Message-id-only notifications correctly queue when React is unavailable.These notifications require MMKV access (for auth tokens) which depends on React context. Queueing them for later processing is the correct approach. The warning log helps with debugging.
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
40-45: Singleton pattern for Application instance is correct.The companion object with
@JvmStaticonlateinit var instanceand a private setter is a clean approach for exposing the Application instance to Java code. The@JvmStaticannotation auto-generates a static getter method (getInstance()) for Java interoperability. Settinginstance = thisinonCreate()is the correct lifecycle point.While
MainApplication.instancewould throwUninitializedPropertyAccessExceptionif accessed beforeonCreate()completes, the actual usage inEncryption.java:230occurs during FCM notification handling (viagetDatabasePath()), which always happens after application initialization. This timing ensures safe access.android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)
306-376: Enhanced error handling and logging.The granular try-catch blocks (lines 313-319, 329-337, 341-349, 357-362) with specific error messages significantly improve debuggability for E2E decryption failures in production.
406-444: Method signature changes remove Context dependency.The removal of
Contextparameters fromdecryptMessage(line 406) andencryptMessageContent(line 446) simplifies the API. These methods now rely onMainApplication.getInstance()accessed viagetDatabasePath().These changes depend on fixing the critical issues identified in
getDatabasePath(null safety and database path calculation). Verify that the earlier issues are resolved before merging.Also applies to: 446-502
228-230: Remove null check suggestion—MainApplication.getInstance() initialization is guaranteed before notification processing.
MainApplication.instanceis a Kotlinlateinitvariable initialized inonCreate()at the start of application lifecycle. Android guaranteesApplication.onCreate()executes before any service or broadcast receiver (including FCM notification handlers) can process messages. Unlike a nullable type,lateinitthrowsUninitializedPropertyAccessException, not returnnull. The context at line 232 is safe to use without defensive checks.Likely an incorrect or invalid review comment.
| var backgroundTask: UIBackgroundTaskIdentifier = .invalid | ||
| backgroundTask = UIApplication.shared.beginBackgroundTask { | ||
| // Expiration handler - called if system needs to reclaim resources | ||
| if backgroundTask != .invalid { | ||
| UIApplication.shared.endBackgroundTask(backgroundTask) | ||
| backgroundTask = .invalid | ||
| } | ||
| completionHandler() | ||
| } |
There was a problem hiding this comment.
Critical: Double completion handler invocation and thread-safety issue.
The current implementation has two critical problems:
-
Double completion handler call: The expiration handler calls
completionHandler()at line 110, and the defer block also callscompletionHandler()at line 121. If the expiration handler fires beforesendMessagecompletes, both will execute, callingcompletionHandler()twice. iOS requires completion handlers to be called exactly once. -
Race condition: The
backgroundTaskvariable is accessed and modified from multiple threads without synchronization—the expiration handler can run on any thread, while the defer block runs on the main thread. This creates a race condition.
🔎 Proposed fix using a completion flag and synchronization
+ private var completionLock = NSLock()
+ private var hasCalledCompletion = false
+
private func handleReplyAction(response: UNNotificationResponse, completionHandler: @escaping () -> Void) {
guard let textResponse = response as? UNTextInputNotificationResponse else {
completionHandler()
return
}
let userInfo = response.notification.request.content.userInfo
guard let ejsonString = userInfo["ejson"] as? String,
let ejsonData = ejsonString.data(using: .utf8),
let payload = try? JSONDecoder().decode(Payload.self, from: ejsonData),
let rid = payload.rid else {
// Show failure notification to user
let content = UNMutableNotificationContent()
content.body = "Failed to send reply. Invalid notification data."
let request = UNNotificationRequest(identifier: "replyPayloadFailure", content: content, trigger: nil)
UNUserNotificationCenter.current().add(request, withCompletionHandler: nil)
completionHandler()
return
}
let message = textResponse.userText
let rocketchat = RocketChat(server: payload.host.removeTrailingSlash())
+ let completionLock = NSLock()
+ var hasCalledCompletion = false
+
+ let safeComplete = {
+ completionLock.lock()
+ defer { completionLock.unlock() }
+ if !hasCalledCompletion {
+ hasCalledCompletion = true
+ completionHandler()
+ }
+ }
+
var backgroundTask: UIBackgroundTaskIdentifier = .invalid
backgroundTask = UIApplication.shared.beginBackgroundTask {
// Expiration handler - called if system needs to reclaim resources
+ completionLock.lock()
+ let taskToEnd = backgroundTask
+ backgroundTask = .invalid
+ completionLock.unlock()
+
- if backgroundTask != .invalid {
- UIApplication.shared.endBackgroundTask(backgroundTask)
- backgroundTask = .invalid
- }
- completionHandler()
+ if taskToEnd != .invalid {
+ UIApplication.shared.endBackgroundTask(taskToEnd)
+ }
+ safeComplete()
}
rocketchat.sendMessage(rid: rid, message: message, threadIdentifier: payload.tmid) { response in
// Ensure we're on the main thread for UI operations
DispatchQueue.main.async {
defer {
+ completionLock.lock()
+ let taskToEnd = backgroundTask
+ backgroundTask = .invalid
+ completionLock.unlock()
+
- if backgroundTask != .invalid {
- UIApplication.shared.endBackgroundTask(backgroundTask)
- backgroundTask = .invalid
- }
- completionHandler()
+ if taskToEnd != .invalid {
+ UIApplication.shared.endBackgroundTask(taskToEnd)
+ }
+ safeComplete()
}
guard let response = response, response.success else {
// Show failure notification
let content = UNMutableNotificationContent()
content.body = "Failed to send reply."
let request = UNNotificationRequest(identifier: "replyFailure", content: content, trigger: nil)
UNUserNotificationCenter.current().add(request, withCompletionHandler: nil)
return
}
}
}
}This fix ensures:
completionHandler()is called exactly once using a synchronized flag- Thread-safe access to
backgroundTaskusing a lock - Background task is properly ended before calling completion
Committable suggestion skipped: line range outside the PR's diff.
| 407D3EDE3DABEE15D27BD87D /* ShellScript */ = { | ||
| isa = PBXShellScriptBuildPhase; | ||
| alwaysOutOfDate = 1; | ||
| buildActionMask = 2147483647; | ||
| files = ( | ||
| ); | ||
| inputFileListPaths = ( | ||
| ); | ||
| inputPaths = ( | ||
| ); | ||
| name = "[Expo] Configure project"; | ||
| outputFileListPaths = ( | ||
| "${PODS_ROOT}/Target Support Files/Pods-defaults-RocketChatRN/Pods-defaults-RocketChatRN-frameworks.sh", | ||
| "${PODS_XCFRAMEWORKS_BUILD_DIR}/OpenSSL-Universal/OpenSSL.framework/OpenSSL", | ||
| "${PODS_XCFRAMEWORKS_BUILD_DIR}/hermes-engine/Pre-built/hermes.framework/hermes", | ||
| ); | ||
| outputPaths = ( | ||
| "$(DERIVED_FILE_DIR)/Pods-defaults-NotificationService-checkManifestLockResult.txt", | ||
| ); | ||
| runOnlyForDeploymentPostprocessing = 0; | ||
| shellPath = /bin/sh; | ||
| shellScript = "# This script configures Expo modules and generates the modules provider file.\nbash -l -c \"./Pods/Target\\ Support\\ Files/Pods-defaults-RocketChatRN/expo-configure-project.sh\"\n"; | ||
| shellScript = "\"${PODS_ROOT}/Target Support Files/Pods-defaults-RocketChatRN/Pods-defaults-RocketChatRN-frameworks.sh\"\n"; | ||
| showEnvVarsInLog = 0; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the exact lines mentioned to confirm the content
sed -n '1670,1687p' ios/RocketChatRN.xcodeproj/project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 853
🏁 Script executed:
# Let's also check the overall structure of the file to understand the context
wc -l ios/RocketChatRN.xcodeproj/project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 125
🏁 Script executed:
# Search for other shell script phases to understand the pattern
rg "shellScript = " ios/RocketChatRN.xcodeproj/project.pbxproj -A 1 -B 10 | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 5971
🏁 Script executed:
# Search for NotificationService target in the project file
rg "NotificationService" ios/RocketChatRN.xcodeproj/project.pbxproj | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3537
🏁 Script executed:
# Check if there are named vs unnamed build phases and their purpose
rg "PBXShellScriptBuildPhase" ios/RocketChatRN.xcodeproj/project.pbxproj -A 5 | rg "name = |shellScript = " | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Check for duplicate output paths to see if this is a duplicate/orphaned script
rg "Pods-defaults-NotificationService-checkManifestLockResult.txt" ios/RocketChatRN.xcodeproj/project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 255
🏁 Script executed:
# Search for the build phases that contain these output paths and identify which targets they belong to
rg -B 20 "Pods-defaults-NotificationService-checkManifestLockResult.txt" ios/RocketChatRN.xcodeproj/project.pbxproj | rg "isa = PBXNativeTarget|isa = PBXAggregateTarget|buildPhases = \(|shellScript|name = "Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1603
🏁 Script executed:
# More specifically, find the section at 1670-1687 and identify its parent target
sed -n '1600,1700p' ios/RocketChatRN.xcodeproj/project.pbxproj | rg -B 30 "407D3EDE3DABEE15D27BD87D" | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2573
🏁 Script executed:
# Find which target this build phase is assigned to
rg "407D3EDE3DABEE15D27BD87D" ios/RocketChatRN.xcodeproj/project.pbxproj -B 50 | rg "isa = PBXNativeTarget|name = " | head -10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 221
🏁 Script executed:
# Get the surrounding context to see which target contains this build phase
rg -n "407D3EDE3DABEE15D27BD87D" ios/RocketChatRN.xcodeproj/project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 184
🏁 Script executed:
# Check which target's buildPhases includes line 1282
sed -n '1200,1350p' ios/RocketChatRN.xcodeproj/project.pbxproj | head -200Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 5664
🏁 Script executed:
# More targeted - look for the buildPhases array that includes this phase ID
sed -n '1270,1290p' ios/RocketChatRN.xcodeproj/project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1118
🏁 Script executed:
# Now check the NotificationService target's build phases
sed -n '3150,3250p' ios/RocketChatRN.xcodeproj/project.pbxproj | grep -A 30 "NotificationService"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
# Find NotificationService target definition and its buildPhases
rg -A 30 "1EFEB5942493B6640072EDC0 /\* NotificationService \*/" ios/RocketChatRN.xcodeproj/project.pbxproj | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2231
Build script configuration has mismatched output path
The unnamed shell script at this location is assigned to the RocketChatRN target but outputs to the NotificationService manifest lock file:
- Executes:
Pods-defaults-RocketChatRN-frameworks.sh(correct for RocketChatRN) - Output path:
Pods-defaults-NotificationService-checkManifestLockResult.txt(belongs to NotificationService target)
This breaks build phase dependency tracking. Regenerate the Xcode project configuration by running pod install --repo-update from the ios directory.
🤖 Prompt for AI Agents
In ios/RocketChatRN.xcodeproj/project.pbxproj around lines 1670 to 1687, the
ShellScript build phase for the RocketChatRN target runs the
Pods-defaults-RocketChatRN-frameworks.sh script but its outputPaths points to
Pods-defaults-NotificationService-checkManifestLockResult.txt; regenerate the
Xcode project by running pod install --repo-update from the ios directory to fix
target-specific entries, or manually update the build phase outputPaths to the
correct file (e.g. Pods-defaults-RocketChatRN-checkManifestLockResult.txt) so
the output matches the RocketChatRN target and rebuild to verify dependency
tracking is restored.
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)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
177-234: Critical: Remove duplicate code block in readRoom.Lines 195-207 are duplicated at lines 209-221. This appears to be a copy-paste error that results in:
- Cursor operations executed twice unnecessarily
- The first cursor close (line 207) making subsequent cursor operations (line 209) fail or behave unexpectedly
Remove one of these duplicate blocks.
🔎 Proposed fix
cursor.moveToFirst(); int e2eKeyColumnIndex = cursor.getColumnIndex("e2e_key"); int encryptedColumnIndex = cursor.getColumnIndex("encrypted"); if (e2eKeyColumnIndex == -1) { Log.e(TAG, "e2e_key column not found in subscriptions table"); cursor.close(); return null; } String e2eKey = cursor.getString(e2eKeyColumnIndex); Boolean encrypted = encryptedColumnIndex != -1 && cursor.getInt(encryptedColumnIndex) > 0; cursor.close(); - - cursor.moveToFirst(); - int e2eKeyColumnIndex = cursor.getColumnIndex("e2e_key"); - int encryptedColumnIndex = cursor.getColumnIndex("encrypted"); - - if (e2eKeyColumnIndex == -1) { - Log.e(TAG, "e2e_key column not found in subscriptions table"); - cursor.close(); - return null; - } - - String e2eKey = cursor.getString(e2eKeyColumnIndex); - Boolean encrypted = encryptedColumnIndex != -1 && cursor.getInt(encryptedColumnIndex) > 0; - cursor.close(); return new Room(e2eKey, encrypted);
🤖 Fix all issues with AI Agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java:
- Around line 265-271: The comment/code mismatch in Encryption.java: the code
uses context.getFilesDir().getParentFile() but the comments say "files
directory"; update the implementation to use context.getFilesDir() (assign
internalDir = context.getFilesDir()), keep fullDbName = name + ".db", and update
the inline comments to state "Use files directory (where WatermelonDB stores
files when appGroupPath is empty)" so comments and variables (fullDbName,
internalDir) consistently reflect the files directory path.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
💤 Files with no reviewable changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/E2ENotificationProcessor.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (1)
ios/Shared/RocketChat/Database.swift (2)
getDatabasePath(32-36)openDatabase(38-44)
⏰ 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). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (7)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (4)
50-63: LGTM! Thread-safe pending notification holder.The
PendingNotificationholder class andCopyOnWriteArrayListprovide a clean, thread-safe mechanism for queueing notifications that arrive before React Native initialization.
143-152: LGTM! Correct queueing for message-id-only notifications.Message-id-only notifications require MMKV (which needs React context) for fetching tokens to make API calls. Queueing them until React is ready is the correct approach.
198-213: LGTM! Simplified E2E decryption flow.E2E decryption now happens immediately using direct SQLiteDatabase access via
Encryption.shared.decryptMessage(ejson), eliminating the previous async processor dependency. The error handling is appropriate.
88-109: MainApplication.getInstance() initialization is properly sequenced and safe.The static
instanceis initialized at the very start ofMainActivity.onCreate()(line 70), which executes before Firebase services can receive any messages. Android guarantees thatApplication.onCreate()completes before any service components are started, so early notifications will never encounter an uninitializedgetInstance(). The queuing mechanism insetReactContext()(lines 88-109) correctly handles cases where notifications arrive before React is initialized, not before the singleton is set.android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)
420-458: LGTM! Simplified decryptMessage signature.Removing the Context parameter and using
readRoom(ejson)directly simplifies the API and aligns with the new direct SQLiteDatabase access pattern. The fallback logic for V1/V2 formats is preserved.
460-516: LGTM! Consistent encryptMessageContent signature change.The Context parameter removal is consistent with
decryptMessageand the overall refactoring. The encryption logic for V1/V2 formats is preserved correctly.
242-244: No action required — initialization order is safe.The switch to
MainApplication.getInstance()is sound. Android's framework guarantees thatApplication.onCreate()executes before anyServiceor broadcast receiver can deliver callbacks, and the singleton instance is set at line 70 inonCreate()— very early, before any other initialization. Additionally,CustomPushNotificationalready includes a pending notifications queue mechanism that handles the edge case of notifications arriving before React Native is fully initialized (seependingNotificationsandsetReactContext()at lines 63–116).
| // TODO: It's getting lock issues | ||
| // Open database in read-only mode (safer for concurrent access with WatermelonDB) | ||
| db = SQLiteDatabase.openDatabase(dbPath, null, SQLiteDatabase.OPEN_READONLY); |
There was a problem hiding this comment.
Resolve TODO: Address database lock issues before merging.
The TODO indicates known lock issues when accessing the SQLiteDatabase concurrently with WatermelonDB. This is a production code path for push notifications and must be resolved. Consider:
- Using
SQLiteDatabase.OPEN_READONLY | SQLiteDatabase.ENABLE_WRITE_AHEAD_LOGGINGto reduce lock contention - Implementing retry logic with exponential backoff
- Adding timeout handling to prevent indefinite blocking
Do not merge production code with unresolved concurrency TODOs that could cause notification failures.
Would you like me to generate a more robust implementation with retry logic and proper lock handling?
| // WatermelonDB on Android: when appGroupPath is empty (Android), SQLiteAdapter stores files | ||
| // in the app's files directory, not the databases directory | ||
| // It appends ".db" internally, so the actual file is "{name}.db.db" | ||
| String fullDbName = name + ".db"; | ||
| // Use files directory (where WatermelonDB stores files when appGroupPath is empty) | ||
| File internalDir = context.getFilesDir().getParentFile(); | ||
| return new File(internalDir, fullDbName).getAbsolutePath(); |
There was a problem hiding this comment.
Resolve comment-code inconsistency from previous review.
This issue was flagged in a previous review but remains unresolved. The comments claim WatermelonDB uses the "files directory" (lines 266, 269), but the code uses context.getFilesDir().getParentFile() — the parent of the files directory.
Clarify and fix:
- If databases are in
/data/data/<package>/files/, usecontext.getFilesDir()and update code - If databases are in
/data/data/<package>/(parent), update all comments to state "parent app data directory" not "files directory"
Based on learnings from past review comments.
🤖 Recommended approach
Verify the actual WatermelonDB path by checking what appGroupPath is set to in your WatermelonDB initialization, then align code and comments to match reality. The iOS snippet shows databases in the group directory; Android should similarly document its actual storage location accurately.
🤖 Prompt for AI Agents
In
@android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java
around lines 265 - 271, The comment/code mismatch in Encryption.java: the code
uses context.getFilesDir().getParentFile() but the comments say "files
directory"; update the implementation to use context.getFilesDir() (assign
internalDir = context.getFilesDir()), keep fullDbName = name + ".db", and update
the inline comments to state "Use files directory (where WatermelonDB stores
files when appGroupPath is empty)" so comments and variables (fullDbName,
internalDir) consistently reflect the files directory path.
…yption class - Added a static method to retrieve the MainApplication instance for easier access. - Removed redundant code in the Encryption class related to cursor handling for e2e_key and encrypted columns.
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
Proposed changes
This PR fixes a critical issue where push notifications (especially E2E encrypted notifications) failed to display when the Android app was killed. The root cause was that the notification processing code was waiting for React Native to initialize before decrypting E2E messages, which could cause timeouts or failures when the app was completely killed.
Key improvements:
Issue(s)
Fixes push notification handling when Android app is killed (especially E2E encrypted notifications)
How to test or reproduce
Test E2E notifications when app is killed:
Test message-id-only notifications:
Test regular notifications:
Test app startup scenarios:
Screenshots
N/A - Backend/notification handling changes
Types of changes
Checklist
Further comments
Technical Details
Problem:
When the Android app was killed, push notifications (especially E2E encrypted ones) would fail to display because:
Solution:
Encryption.readRoom()to useSQLiteDatabase.openDatabase()directly instead of WatermelonDB wrapper, eliminating the React Native dependency for E2E decryptioninstanceproperty toMainApplicationso Java code can access the application context without React NativeE2ENotificationProcessor.javaas it's no longer neededFiles Changed:
MainApplication.kt: Added static instance and getInstance() methodCustomPushNotification.java: Simplified notification handling, removed async waiting logic, added queueing for message-id-only notificationsEncryption.java: Refactored to use SQLiteDatabase directly, removed Context parameter dependencyReplyBroadcast.java: Updated to use new Encryption API (no Context parameter)MMKVKeyManager.java: Removed unused appContext fieldE2ENotificationProcessor.java: DELETED - no longer neededNote: There's a TODO comment in
Encryption.readRoom()about potential lock issues with concurrent database access. This should be monitored in production.