feat: Avatar on push notifications #6853
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.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
WalkthroughStandardizes notification title/message and avatar handling across Android, iOS, and JS; adds an Android VideoConf broadcast receiver; enhances iOS NotificationService with avatar fetching, intent donation and videoconf handling; hardens reply background tasks; updates entitlements, Info.plists, Xcode/CocoaPods references, and request User-Agent handling. (50 words) Changes
Sequence Diagram(s)%%{init: {"theme":"default","themeVariables":{"actorBorder":"#2b6cb0","noteBorder":"#718096","noteBkg":"#f7fafc"}}}%%
sequenceDiagram
autonumber
participant OS as Android OS
participant Broadcast as VideoConfBroadcast
participant Native as VideoConfModule (native storage)
participant App as MainActivity
participant JS as JavaScript layer
Note over OS,Broadcast: User taps call action (accept/decline)
OS->>Broadcast: deliver(action, extras)
activate Broadcast
Broadcast->>Broadcast: validate action & extract extras
alt valid action
Broadcast->>Native: storePendingAction(payload JSON)
Broadcast->>OS: cancel notification (if id)
Broadcast->>App: startActivity(intent with event + extras)
else invalid
Broadcast->>Broadcast: log warning and ignore
end
deactivate Broadcast
App->>JS: deliver intent extras on launch/resume
JS->>Native: readPendingAction()
JS->>JS: handle videoconf event (accept/decline)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_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: 6
🧹 Nitpick comments (11)
ios/Podfile (1)
37-40: LGTM! Optional: Add trailing comma for better diffs.The removal of
react-native-notificationsfrom static frameworks correctly aligns with the notification system migration.Consider adding a trailing comma after
'simdjson'per Ruby convention:$static_framework = [ 'WatermelonDB', - 'simdjson' + 'simdjson', ]This makes future diffs cleaner when adding items to the array. Based on static analysis hints (RuboCop).
android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (1)
11-11: Unused import detected.The
Configurationimport appears to be unused in this file.import android.content.Intent -import android.content.res.Configuration import chat.rocket.reactnative.notification.VideoConfModuleandroid/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt (2)
36-50: Potential race condition in read-then-clear pattern.The
getPendingActionmethod reads and then clears the value in separate operations. If called concurrently (unlikely but possible), both calls could retrieve the same action before either clears it.For a single-consumer scenario this is likely acceptable, but consider using
commit()instead ofapply()for synchronous clearing if atomicity becomes important.
56-65: Log swallowed exception for debuggability.Per static analysis, the caught exception is silently swallowed. While failures in
clearPendingActionmay be acceptable, logging helps with debugging.@ReactMethod override fun clearPendingAction() { try { reactApplicationContext.getSharedPreferences(PREFS_NAME, Context.MODE_PRIVATE) .edit() .remove(KEY_VIDEO_CONF_ACTION) .apply() } catch (e: Exception) { - // Ignore errors + // Log but don't propagate - cleanup failures are non-critical + android.util.Log.w("VideoConfModule", "Failed to clear pending action", e) } }ios/NotificationService/NotificationService.swift (1)
48-51: Sanitize username for filesystem safety.The username is used directly in the filename without sanitization. Usernames with characters like
/or..could potentially cause issues or path traversal in the temp directory.// Save to temp file (UNNotificationAttachment requires file URL) let tempDir = FileManager.default.temporaryDirectory - let fileName = "\(username)_avatar.png" + let sanitizedUsername = username.replacingOccurrences(of: "[^A-Za-z0-9_-]", with: "_", options: .regularExpression) + let fileName = "\(sanitizedUsername)_avatar.png" let fileURL = tempDir.appendingPathComponent(fileName)android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
81-81: Minor: Inconsistent indentation.Line 81 uses a tab character while the rest of the file uses spaces. This is a minor formatting inconsistency.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
103-105: Extract duplicate notification ID generation logic.The notification ID generation logic is duplicated in
showIncomingCallandcancelCall. Consider extracting to a private helper function for maintainability.+ /** + * Generates a unique notification ID from room ID and caller ID. + */ + private fun generateNotificationId(rid: String?, callerId: String): Int { + val notificationIdStr = ((rid ?: "") + callerId).replace(Regex("[^A-Za-z0-9]"), "") + return notificationIdStr.hashCode() + } + fun showIncomingCall(bundle: Bundle, ejson: Ejson) { val rid = ejson.rid // ... - val notificationIdStr = (rid + callerId).replace(Regex("[^A-Za-z0-9]"), "") - val notificationId = notificationIdStr.hashCode() + val notificationId = generateNotificationId(rid, callerId)Also applies to: 203-205
app/lib/notifications/index.ts (1)
72-75: Consider if both awaits are necessary.
removeAllNotificationsreturns a promise that should be awaited, butsetNotificationsBadgeCountmay not require awaiting if it's fire-and-forget. If they're independent operations, they could be parallelized withPromise.all.export const removeNotificationsAndBadge = async (): Promise<void> => { - await removeAllNotifications(); - await setNotificationsBadgeCount(); + await Promise.all([removeAllNotifications(), setNotificationsBadgeCount()]); };android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (3)
115-149: Polling loop is functional but consider alternative patterns.The thread-based polling with 100ms sleep intervals works but is not ideal. Consider using a
CountDownLatchorCompletableFuturefor cleaner async coordination. However, given this is notification handling code that needs to be reliable, the current approach is acceptable.
519-522: Potential StringIndexOutOfBoundsException.If
posis-1(no ":" found),startbecomes0, andmessage.substring(0)is safe. However, if the message is empty after the ":", this could produce unexpected results. Consider adding a bounds check.private String extractMessage(String message, Ejson ejson) { if (message == null) { return ""; } if (ejson != null && ejson.type != null && !ejson.type.equals("d")) { int pos = message.indexOf(":"); - int start = pos == -1 ? 0 : pos + 2; - return message.substring(start); + if (pos == -1) { + return message; + } + int start = Math.min(pos + 2, message.length()); + return message.substring(start); } return message; }
362-373: Channel creation is correct but could include more configuration.The channel is created with
IMPORTANCE_HIGHwhich is appropriate. Consider adding description, vibration pattern, or sound configuration similar to theVideoConfNotificationchannel for consistency.
📜 Review details
Configuration used: CodeRabbit 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 ignored due to path filters (2)
ios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (44)
android/app/build.gradle(0 hunks)android/app/src/main/AndroidManifest.xml(2 hunks)android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt(2 hunks)android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java(15 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java(2 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/NativeVideoConfSpec.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java(2 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java(2 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfTurboPackage.kt(1 hunks)app/index.tsx(1 hunks)app/lib/native/NativeVideoConfAndroid.ts(1 hunks)app/lib/notifications/index.ts(2 hunks)app/lib/notifications/push.ts(2 hunks)app/lib/notifications/videoConf/backgroundNotificationHandler.ts(0 hunks)app/lib/notifications/videoConf/getInitialNotification.ts(1 hunks)app/lib/store/appStateMiddleware.ts(1 hunks)app/sagas/deepLinking.js(2 hunks)app/sagas/troubleshootingNotification.ts(2 hunks)app/views/JitsiMeetView/index.tsx(2 hunks)app/views/PushTroubleshootView/components/DeviceNotificationSettings.tsx(1 hunks)index.js(1 hunks)ios/AppDelegate.swift(0 hunks)ios/NotificationService/NotificationService.swift(3 hunks)ios/Podfile(1 hunks)ios/ReplyNotification.swift(1 hunks)ios/RocketChatRN-Bridging-Header.h(0 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(36 hunks)ios/Shared/Extensions/Bundle+Extensions.swift(1 hunks)ios/Shared/Models/NotificationType.swift(1 hunks)ios/Shared/Models/Payload.swift(1 hunks)ios/Shared/RocketChat/API/Request.swift(1 hunks)ios/fastlane/Fastfile(1 hunks)jest.setup.js(1 hunks)package.json(1 hunks)patches/@notifee+react-native+7.8.2.patch(0 hunks)patches/@react-native-firebase+messaging+21.12.2.patch(0 hunks)patches/react-native-notifications+5.1.0.patch(0 hunks)react-native.config.js(1 hunks)
💤 Files with no reviewable changes (7)
- android/app/build.gradle
- app/lib/notifications/videoConf/backgroundNotificationHandler.ts
- ios/RocketChatRN-Bridging-Header.h
- ios/AppDelegate.swift
- patches/@react-native-firebase+messaging+21.12.2.patch
- patches/@notifee+react-native+7.8.2.patch
- patches/react-native-notifications+5.1.0.patch
🧰 Additional context used
🧬 Code graph analysis (8)
app/lib/store/appStateMiddleware.ts (1)
app/lib/notifications/index.ts (1)
removeNotificationsAndBadge(72-75)
app/lib/notifications/push.ts (3)
app/definitions/INotification.ts (1)
INotification(1-16)app/lib/notifications/index.ts (1)
onNotification(18-66)app/lib/services/restApi.ts (1)
registerPushToken(989-1007)
app/lib/notifications/videoConf/getInitialNotification.ts (2)
app/lib/store/auxStore.ts (1)
store(6-6)app/actions/deepLinking.ts (1)
deepLinkingClickCallPush(26-31)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (2)
createNotificationChannel(54-77)showIncomingCall(85-183)android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java (1)
Notification(24-57)
app/index.tsx (1)
app/lib/notifications/videoConf/getInitialNotification.ts (1)
getInitialNotification(13-62)
android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
NotificationHelper(10-40)
ios/Shared/Extensions/Bundle+Extensions.swift (1)
app/lib/constants/userAgent.ts (1)
userAgent(3-5)
ios/Shared/RocketChat/API/Request.swift (1)
app/lib/constants/userAgent.ts (1)
userAgent(3-5)
🪛 detekt (1.23.8)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt
[warning] 62-62: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🪛 RuboCop (1.81.7)
ios/Podfile
[convention] 39-39: Put a comma after the last item of a multiline array.
(Style/TrailingCommaInArrayLiteral)
⏰ 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 (59)
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1-3439: Auto-generated CocoaPods project file changes look consistent.This file is auto-generated by CocoaPods when running
pod install. The changes correctly update:
- Framework references for the NotificationService, RocketChatRN, and Rocket.Chat pod targets
- xcconfig file references for debug/release configurations
- Shell script build phases for embedding frameworks and copying resources
- Resource bundles including new Expo-related privacy bundles (ExpoNotifications_privacy.bundle, etc.)
The structure is consistent across all targets and configurations.
ios/fastlane/Fastfile (1)
125-125: Verify if arm64 architecture constraint is intentional for E2E testing.The
build_experimental_simulatorlane restricts builds toARCHS=arm64, which targets Apple Silicon architectures only. This could prevent E2E test builds on Intel-based Mac systems. However, the CI workflow already runs onmacos-15(Apple Silicon), so this primarily affects local development workflows on Intel Macs.Confirm whether this architectural constraint is intentional as part of the project's migration to Apple Silicon, or clarify how this relates to the PR's objective of adding avatar images to push notifications.
jest.setup.js (1)
154-156: Appropriate minimal mock for expo-device.Setting
isDevice: trueprovides a reasonable default for tests simulating physical device behavior.ios/ReplyNotification.swift (5)
12-18: LGTM - Delegation pattern is well-structured.The weak reference for
originalDelegatecorrectly avoids retain cycles, and the staticsharedinstance ensures the delegate stays alive for the lifetime of the app.
20-29: LGTM - Proper delegate chain setup.The configure method correctly captures the original delegate before setting itself, ensuring the delegation chain is preserved.
33-50: LGTM - Clean action routing.The method correctly intercepts
REPLY_ACTIONwhile ensuring all other notification responses are forwarded to expo-notifications.
52-63: LGTM!
65-75: LGTM!The
#availablecheck appropriately guards the call to the original delegate.react-native.config.js (1)
1-3: LGTM! Configuration correctly updated for the migration.Removing the Firebase messaging platform override is appropriate since
@react-native-firebase/messaginghas been removed from the project dependencies as part of the expo-notifications migration.package.json (1)
64-64: Update expo-notifications to 0.32.15.expo-notifications has been updated to 0.32.15, but the package.json specifies 0.32.11. expo-device at ^8.0.10 is current, so no update needed there. While no known critical security vulnerabilities were found in these specific versions, staying current with patch releases ensures you have the latest fixes.
app/views/PushTroubleshootView/components/DeviceNotificationSettings.tsx (1)
21-21: Linking.openSettings() is the correct approach for Android notification settings.The migration from
notifee.openNotificationSettings()toLinking.openSettings()is appropriate. On Android, this standard React Native API correctly opens the app's notification settings. The platform-specific implementation is proper, with distinct handling for iOS (app-settings:deep link) and Android (Linking.openSettings()).ios/Shared/Models/Payload.swift (3)
11-14: LGTM!The
Callerstruct is properly defined withCodableconformance for JSON serialization, and optional fields provide appropriate flexibility for video conference notifications.
29-33: LGTM!The video conference fields are properly structured with optional types, maintaining backward compatibility while supporting the new video conference notification feature.
21-21: Verify messageId usage before making it optional.Changing
messageIdfromStringtoString?requires confirming that all consumers handle the optional safely. Review the codebase for any force unwrapping (messageId!) or direct property access that assumes a non-nil value, as these patterns would break with the optional type change.app/lib/notifications/push.ts (6)
13-19: LGTM!The migration to
Notifications.setBadgeCountAsyncis correctly implemented with proper async/await and error handling.
21-27: LGTM!The migration to
Notifications.dismissAllNotificationsAsyncis correctly implemented with proper async/await and error handling.
34-80: LGTM!The notification transformation logic is well-structured with proper platform-specific handling for Android and iOS, appropriate fallbacks, and comprehensive field mapping to the
INotificationinterface.
85-126: LGTM!The notification categories setup is well-implemented with proper action configurations, localized strings, and appropriate foreground behavior settings for each action type.
131-159: LGTM!The push notification registration flow is correctly implemented with proper permission handling, device checks, and error handling.
161-223: LGTM!The notification configuration is well-structured with proper setup of handlers, listeners, and initial notification retrieval. The platform-specific handling for iOS background state is preserved appropriately.
android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java (1)
129-129: LGTM!The User-Agent header addition aligns with the cross-platform consistency effort and follows the same pattern used in iOS.
ios/Shared/Models/NotificationType.swift (1)
14-14: LGTM!The new
videoconfnotification type case is properly added, following the existing enum pattern and maintainingCodableconformance.app/lib/store/appStateMiddleware.ts (1)
20-20: LGTM!Adding error handling for the async
removeNotificationsAndBadge()call is appropriate and prevents unhandled promise rejections. Usingconsole.warnis suitable for this non-critical failure scenario.app/sagas/troubleshootingNotification.ts (1)
3-3: LGTM!The migration from Notifee to Expo Notifications for permission checking is correctly implemented and aligns with the broader notification system migration.
Also applies to: 22-23
app/index.tsx (1)
141-144: LGTM!The video conference initial notification handling is correctly integrated into the app initialization flow with an appropriate early return to prevent duplicate navigation or deep linking.
ios/Shared/RocketChat/API/Request.swift (1)
58-58: Bundle.userAgent extension is properly defined and correctly implemented.The
Bundle.userAgentstatic var atios/Shared/Extensions/Bundle+Extensions.swift:17returns a well-formatted User-Agent string following the pattern "RC Mobile; ios {version}; v{appVersion} ({build})" using ProcessInfo and Bundle metadata.android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java (2)
44-51: LGTM! Robust data extraction with proper fallback.The new extraction pattern with fallback logic and null checks prevents downstream errors and handles missing data gracefully.
87-87: LGTM! Consistent User-Agent header addition.The User-Agent header is correctly set using the standardized helper method.
app/sagas/deepLinking.js (2)
226-226: LGTM! Safe property access.Optional chaining prevents runtime errors when
calleris undefined or null in video conference notifications.
249-252: LGTM! Proper guard clause.The early return when
hostis falsy prevents unnecessary processing and potential errors in subsequent host normalization logic.ios/Shared/Extensions/Bundle+Extensions.swift (1)
15-23: LGTM! Consistent User-Agent implementation.The User-Agent format matches the Android implementation in NotificationHelper.java, ensuring consistency across platforms for native API requests. The fallback to "unknown" handles missing bundle info gracefully.
index.js (1)
25-26: LGTM! Clear documentation of architectural change.The comment clearly documents that video conference notifications are now handled natively in the Android FCM service stack, replacing the previous JavaScript-based approach.
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
3-3: LGTM! Well-documented User-Agent helper.The implementation is clean and consistent with the iOS counterpart. The format string clearly documents the expected output, and the method uses appropriate Android system APIs.
Also applies to: 28-39
android/app/src/main/AndroidManifest.xml (2)
78-85: LGTM! Secure FCM service configuration.The service is properly configured with
android:exported="false"for security, and the priority of 100 ensures this custom handler takes precedence for processing FCM messages.
95-103: LGTM! Secure broadcast receiver configuration.The receiver is properly configured with
android:exported="false"and specific intent filters for video conference actions, following Android security best practices.android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (2)
20-43: LGTM! Robust FCM message handling.The implementation includes proper validation, error handling, and logging. The data-to-Bundle conversion is correct, and the try-catch ensures that FCM processing errors don't crash the service.
45-50: LGTM! Proper token handling delegation.The comment clearly documents that token management is handled by the expo-notifications JS layer, and the delegation to the superclass is appropriate.
app/lib/native/NativeVideoConfAndroid.ts (1)
1-9: LGTM! Clean TurboModule interface.The interface definition follows React Native TurboModule patterns correctly. The native Android module is properly registered with the name
'VideoConfModule'(as defined inNativeVideoConfSpec.kt:13), matching the module reference on line 9.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfTurboPackage.kt (1)
12-36: LGTM!Standard TurboModule package implementation. The module info configuration is correct with
isTurboModule = trueand appropriate flags for a non-C++ module.android/app/src/main/java/chat/rocket/reactnative/notification/NativeVideoConfSpec.kt (1)
9-23: LGTM!Clean TurboModule spec definition following React Native conventions. The abstract methods provide a clear contract for the concrete
VideoConfModuleimplementation.android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (2)
37-45: LGTM!New video conference fields align with the payload structure used across the native notification handling code. The inline comments documenting the
statusvalues (0=incoming, 4=cancelled) are helpful.
194-197: LGTM!The
Callerclass follows the same pattern as the existingSenderclass, maintaining consistency in the codebase.android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (1)
45-84: LGTM!The video conference intent handling is well-structured:
- Validates the
videoConfActionflag before processing- Properly cancels the notification when
notificationIdis valid- Builds a consistent JSON payload matching
VideoConfBroadcast- Clears the flag after processing to prevent re-processing on configuration changes
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt (1)
65-71: LGTM!The launch intent configuration with
FLAG_ACTIVITY_NEW_TASK | FLAG_ACTIVITY_CLEAR_TOPis appropriate for launching the activity from a broadcast receiver context.app/lib/notifications/videoConf/getInitialNotification.ts (2)
13-28: LGTM!The Android path correctly:
- Guards against missing
NativeVideoConfModule- Uses
JSON.parse(appropriate since the native side serializes with Gson)- Validates
notificationTypebefore dispatching- Handles errors gracefully without crashing
30-59: 'DECLINE_ACTION' identifier is correctly configured in iOS notification categories.The hardcoded
'DECLINE_ACTION'identifier on line 48 matches the action identifier registered in the iOS notification category setup inapp/lib/notifications/push.ts(line 116). The 'VIDEOCONF' category is properly configured viaNotifications.setNotificationCategoryAsync()with both 'ACCEPT_ACTION' and 'DECLINE_ACTION' identifiers, ensuring consistency across the notification handling pipeline.ios/NotificationService/NotificationService.swift (3)
9-66: Well-implemented avatar fetching with proper timeout and error handling.The 3-second timeout prevents blocking notification delivery, URL encoding handles special characters correctly, and all failure paths gracefully return
nilto allow the notification to proceed without an avatar. Good defensive coding.
134-162: Video conference handling looks correct.The status-based routing (status 4 for cancellation, status 0/nil for incoming) and notification cleanup logic are well-implemented. The iOS 15+ interruption level for time-sensitive notifications is appropriate for incoming calls.
188-199: Good integration of avatar fetching with weak self capture.The
[weak self]capture prevents potential retain cycles, and the optional binding ensures safe access tobestAttemptContentbefore delivery.android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
27-49: Clean migration to new notification architecture.The documentation clearly explains the notification flow, and removing
INotificationsApplicationin favor of the newVideoConfTurboPackageis a good architectural decision. Theopenmodifier on the class allows for flexibility in testing or subclassing.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (3)
54-77: Good notification channel configuration.The channel setup correctly uses high importance for incoming calls, enables lights and vibration, sets public visibility for lock screen display, and configures appropriate audio attributes for ringtone usage.
159-172: Solid notification builder configuration for incoming calls.The notification correctly uses
CATEGORY_CALL, sets ongoing to prevent accidental dismissal, and properly configures full-screen intent for heads-up display. The action ordering (Decline, Accept) follows platform conventions where the positive action is on the right.
188-195: Correct PendingIntent flag handling for Android 12+.The version check for
FLAG_IMMUTABLEon Android S and above is correct and required for targeting API 31+.app/lib/notifications/index.ts (2)
22-28: LGTM - Video conference action handling is clear.The ACCEPT_ACTION/DECLINE_ACTION handling correctly parses the ejson and dispatches the appropriate action with the event type mapped from the identifier.
42-51: Good type-to-path mapping for room navigation.The mapping correctly handles the different subscription types (channel, direct, group, omnichannel) and the special case for omnichannel using sender name.
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
475-489: Good timeout handling for avatar fetching.The 3-second Glide timeout with
TimeoutExceptionhandling prevents blocking the FCM service. This matches the iOS implementation timeout, maintaining consistency across platforms.
325-353: Video conference notification handling is well-structured.The status-based routing correctly delegates to
VideoConfNotificationfor incoming calls (status 0/null) and cancellation (status 4). The fallback for caller ID from bothcallerandsenderfields handles different payload structures gracefully.app/views/JitsiMeetView/index.tsx (2)
5-5: Unused import removed.The removal of
BackHandleris correct since it's no longer used after removing the hardware back button listener.
55-56: Clarify scope of JitsiMeetView changes in notification avatar PR.This appears to be a large refactoring PR affecting hundreds of files. While the comment correctly notes that WebView handles back navigation natively, the extensive rewrite of JitsiMeetView seems disconnected from avatar fetching for notifications. Confirm whether these structural changes are necessary for the notification feature, and ensure the WebView fallback (when Jitsi app is not installed) is properly tested to verify back button behavior remains safe during active video conferences.
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt
Show resolved
Hide resolved
ios/ReplyNotification.swift
Outdated
| let backgroundTask = UIApplication.shared.beginBackgroundTask(expirationHandler: nil) | ||
|
|
||
| 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 { | ||
| UIApplication.shared.endBackgroundTask(backgroundTask) | ||
| completionHandler() | ||
| } |
There was a problem hiding this comment.
Provide an expiration handler for the background task.
Passing nil to expirationHandler means if the system needs to reclaim resources before the network request completes, the task will be terminated without cleanup. This could leave the completion handler uncalled.
- let backgroundTask = UIApplication.shared.beginBackgroundTask(expirationHandler: nil)
+ var backgroundTask: UIBackgroundTaskIdentifier = .invalid
+ backgroundTask = UIApplication.shared.beginBackgroundTask {
+ UIApplication.shared.endBackgroundTask(backgroundTask)
+ backgroundTask = .invalid
+ }
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 {
- UIApplication.shared.endBackgroundTask(backgroundTask)
+ if backgroundTask != .invalid {
+ UIApplication.shared.endBackgroundTask(backgroundTask)
+ backgroundTask = .invalid
+ }
completionHandler()
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let backgroundTask = UIApplication.shared.beginBackgroundTask(expirationHandler: nil) | |
| 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 { | |
| UIApplication.shared.endBackgroundTask(backgroundTask) | |
| completionHandler() | |
| } | |
| var backgroundTask: UIBackgroundTaskIdentifier = .invalid | |
| backgroundTask = UIApplication.shared.beginBackgroundTask { | |
| UIApplication.shared.endBackgroundTask(backgroundTask) | |
| backgroundTask = .invalid | |
| } | |
| 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 { | |
| if backgroundTask != .invalid { | |
| UIApplication.shared.endBackgroundTask(backgroundTask) | |
| backgroundTask = .invalid | |
| } | |
| completionHandler() | |
| } |
🤖 Prompt for AI Agents
In ios/ReplyNotification.swift around lines 97 to 105, the background task is
started with a nil expirationHandler which can allow the system to terminate the
task without cleanup; change beginBackgroundTask to supply an expirationHandler
that calls UIApplication.shared.endBackgroundTask(backgroundTask) and invokes
the completionHandler (on the main thread) to ensure cleanup if the system
expires the task, and guard/end the backgroundTask only if it's not
UIBackgroundTaskIdentifier.invalid.
| jest.mock('expo-notifications', () => ({ | ||
| getDevicePushTokenAsync: jest.fn(() => Promise.resolve({ data: 'mock-token' })), | ||
| getPermissionsAsync: jest.fn(() => Promise.resolve({ status: 'granted' })), | ||
| requestPermissionsAsync: jest.fn(() => Promise.resolve({ status: 'granted' })), | ||
| setBadgeCountAsync: jest.fn(() => Promise.resolve(true)), | ||
| dismissAllNotificationsAsync: jest.fn(() => Promise.resolve()), | ||
| setNotificationHandler: jest.fn(), | ||
| setNotificationCategoryAsync: jest.fn(() => Promise.resolve()), | ||
| addNotificationReceivedListener: jest.fn(() => ({ remove: jest.fn() })), | ||
| addNotificationResponseReceivedListener: jest.fn(() => ({ remove: jest.fn() })), | ||
| addPushTokenListener: jest.fn(() => ({ remove: jest.fn() })), | ||
| getLastNotificationResponse: jest.fn(() => null), | ||
| DEFAULT_ACTION_IDENTIFIER: 'expo.modules.notifications.actions.DEFAULT' | ||
| })); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all expo-notifications imports and method calls to verify mock coverage
echo "=== Searching for expo-notifications imports ==="
rg -n "from 'expo-notifications'" -g '!jest.setup.js' -g '!node_modules' --type=ts --type=tsx --type=js --type=jsx
echo -e "\n=== Searching for Notifications. method calls ==="
rg -nP "Notifications\." -g '!jest.setup.js' -g '!node_modules' --type=ts --type=tsx --type=js --type=jsx | head -50
echo -e "\n=== Searching for specific expo-notifications API calls ==="
rg -n "getDevicePushTokenAsync|getPermissionsAsync|requestPermissionsAsync|setBadgeCountAsync|dismissAllNotificationsAsync|setNotificationHandler|setNotificationCategoryAsync|addNotificationReceivedListener|addNotificationResponseReceivedListener|addPushTokenListener|getLastNotificationResponse" -g '!jest.setup.js' -g '!node_modules' --type=ts --type=tsx --type=js --type=jsxRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Retry without problematic file type flags
echo "=== Searching for expo-notifications imports ==="
rg -n "from 'expo-notifications'" -g '!jest.setup.js' -g '!node_modules'
echo -e "\n=== Searching for Notifications. method calls ==="
rg -n "Notifications\." -g '!jest.setup.js' -g '!node_modules' | head -50
echo -e "\n=== Searching for specific expo-notifications API calls ==="
rg -n "getDevicePushTokenAsync|getPermissionsAsync|requestPermissionsAsync|setBadgeCountAsync|dismissAllNotificationsAsync|setNotificationHandler|setNotificationCategoryAsync|addNotificationReceivedListener|addNotificationResponseReceivedListener|addPushTokenListener|getLastNotificationResponse" -g '!jest.setup.js' -g '!node_modules'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 3923
🏁 Script executed:
#!/bin/bash
# Check the actual method call in getInitialNotification.ts
cat -n app/lib/notifications/videoConf/getInitialNotification.ts | sed -n '30,36p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 449
🏁 Script executed:
#!/bin/bash
# Verify that the synchronous version is also used
cat -n app/lib/notifications/push.ts | sed -n '215,220p'Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 354
Add missing getLastNotificationResponseAsync method to expo-notifications mock.
The mock provides getLastNotificationResponse() (synchronous), but the codebase also uses getLastNotificationResponseAsync() (asynchronous) at app/lib/notifications/videoConf/getInitialNotification.ts:33. Both methods need to be mocked:
getLastNotificationResponse: jest.fn(() => null),
getLastNotificationResponseAsync: jest.fn(() => Promise.resolve(null)),Without the async version, tests exercising the video conference notification path will fail.
🤖 Prompt for AI Agents
In jest.setup.js around lines 139 to 152, the expo-notifications mock is missing
getLastNotificationResponseAsync which the codebase calls; add an entry to the
mocked object named getLastNotificationResponseAsync that is a jest.fn returning
Promise.resolve(null) (keep the existing synchronous getLastNotificationResponse
as-is) so tests using the async API for initial notification handling will not
fail.
|
Android Build Available Rocket.Chat Experimental 4.68.0.107859 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSiAJ5ygEz8GR1Rrnnn3bC77pDWG9tjeQViLkWBVyO1nepTv_aX5udjWLz4BhmduivlomUXEO6HyB6FRhF_ |
|
iOS Build Available Rocket.Chat Experimental 4.68.0.107860 |
- 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.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
ios/NotificationService/NotificationService.swift (1)
134-134: Force unwrap could crash on malformed UTF-8 strings.The force unwrap on
.data(using: .utf8)!could cause a crash if the ejson string contains invalid UTF-8 sequences. This issue was previously flagged and remains unresolved.Apply this diff to use safe optional binding:
- let ejson = (bestAttemptContent.userInfo["ejson"] as? String ?? "").data(using: .utf8)! - guard let data = try? (JSONDecoder().decode(Payload.self, from: ejson)) else { + guard let ejson = (bestAttemptContent.userInfo["ejson"] as? String ?? "").data(using: .utf8), + let data = try? JSONDecoder().decode(Payload.self, from: ejson) else { contentHandler(bestAttemptContent) return }
🧹 Nitpick comments (4)
ios/RocketChatRN/RocketChatRN.entitlements (1)
15-16: LGTM! Communication notification entitlement added correctly.The entitlement enables Communication Notifications API support for displaying sender avatars, which aligns with the PR objectives.
Operational requirements:
- Ensure this entitlement is enabled in your Apple Developer Portal for both development and production provisioning profiles
- App Store review may require explanation of why the app uses communication notification capabilities
ios/NotificationService/NotificationService.swift (3)
40-44: Consider using URLComponents for safer URL construction.String concatenation for URL construction works but could fail silently with malformed server URLs. URLComponents would provide more robust URL validation.
Apply this diff to use URLComponents:
- let fullPath = "\(avatarPath)?format=png&size=100&rc_token=\(credentials.userToken)&rc_uid=\(credentials.userId)" - guard let avatarURL = URL(string: server + fullPath) else { + guard var components = URLComponents(string: server) else { + completion(nil) + return + } + components.path = avatarPath + components.queryItems = [ + URLQueryItem(name: "format", value: "png"), + URLQueryItem(name: "size", value: "100"), + URLQueryItem(name: "rc_token", value: credentials.userToken), + URLQueryItem(name: "rc_uid", value: credentials.userId) + ] + guard let avatarURL = components.url else { completion(nil) return }
75-75: Remove redundant nil initialization.Initializing an optional variable with
nilis redundant in Swift.Apply this diff:
- var senderImage: INImage? = nil + var senderImage: INImage?
121-126: Consider logging Communication Notification update failures.While the silent fallback to regular notifications is appropriate, logging the error would help diagnose issues during development and debugging.
Apply this diff:
do { let updatedContent = try bestAttemptContent.updating(from: intent) self.bestAttemptContent = updatedContent } catch { - // Failed to update with intent, will fall back to regular notification + NSLog("Failed to update notification with intent: \(error.localizedDescription)") }
📜 Review details
Configuration used: CodeRabbit 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 (4)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java(3 hunks)ios/NotificationService/NotificationService.entitlements(1 hunks)ios/NotificationService/NotificationService.swift(3 hunks)ios/RocketChatRN/RocketChatRN.entitlements(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ios/NotificationService/NotificationService.swift (4)
ios/Shared/Extensions/String+Extensions.swift (1)
removeTrailingSlash(25-31)ios/Shared/RocketChat/Storage.swift (1)
getCredentials(11-19)ios/Shared/Extensions/Bundle+Extensions.swift (1)
string(8-14)ios/Shared/RocketChat/RocketChat.swift (1)
getPushWithId(25-37)
🪛 SwiftLint (0.57.0)
ios/NotificationService/NotificationService.swift
[Warning] 75-75: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
🔇 Additional comments (5)
ios/NotificationService/NotificationService.entitlements (1)
5-6: LGTM! Entitlement correctly added to NotificationService extension.The communication notification entitlement is properly configured for the notification service extension, consistent with the main app target. This allows the extension to modify notification content with sender information and avatars.
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (2)
37-44: LGTM! Video conference fields added appropriately.The new fields
caller,callId, andstatusare well-documented and use appropriate types for video conference notification support.
208-211: LGTM! Caller class follows existing patterns.The
Callerclass structure is consistent with the existingSenderclass and appropriately models video conference caller information.ios/NotificationService/NotificationService.swift (2)
1-2: LGTM!The Intents framework import is necessary for the Communication Notifications API functionality introduced in this PR.
247-256: LGTM!The avatar fetching integration is well-implemented with proper memory management using
[weak self]to prevent retain cycles. The asynchronous flow ensures the notification is delivered after avatar fetch completes or times out.
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
Outdated
Show resolved
Hide resolved
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
deployed
to
experimental_android_build
GitHub Actions
Active
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
373-387: Consider extracting duplicated title logic.The notification title derivation based on
ejson.typeis duplicated betweenbuildNotification(lines 373-387) andnotificationStyle(lines 546-562). Consider extracting this into a helper method to reduce duplication and ensure consistency.🔎 Proposed refactor
+ private String deriveNotificationTitle(Ejson ejson, String fallbackTitle) { + if (ejson == null || ejson.type == null) { + return fallbackTitle; + } + if ("p".equals(ejson.type) || "c".equals(ejson.type)) { + return (ejson.name != null && !ejson.name.isEmpty()) ? ejson.name : fallbackTitle; + } else if ("d".equals(ejson.type)) { + return fallbackTitle; + } else if ("l".equals(ejson.type)) { + return (ejson.sender != null && ejson.sender.name != null && !ejson.sender.name.isEmpty()) + ? ejson.sender.name : fallbackTitle; + } + return fallbackTitle; + }Then use it in both locations:
String notificationTitle = deriveNotificationTitle(ejson, title);ios/NotificationService/NotificationService.swift (1)
86-104: Consider extracting the prefix-stripping logic.The nested conditionals for stripping sender prefixes from group/channel message bodies (lines 89-99) are complex and could benefit from extraction into a helper method for improved readability and testability.
📜 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 (7)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.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/VideoConfNotification.ktios/NotificationService/NotificationService.swiftios/Shared/Models/Payload.swift
🧰 Additional context used
🧬 Code graph analysis (3)
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)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
getAvatar(215-217)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
app/sagas/login.js (8)
server(86-86)server(230-230)server(299-299)server(375-375)token(322-322)token(389-389)userId(134-134)userId(341-341)
🪛 SwiftLint (0.57.0)
ios/NotificationService/NotificationService.swift
[Warning] 139-139: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 154-154: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 155-155: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
⏰ 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: Build iOS Experimental / Build
- GitHub Check: Build Android Experimental / Build
🔇 Additional comments (17)
ios/Shared/Models/Payload.swift (1)
11-15: LGTM!The addition of the optional
usernamefield to theCallerstruct aligns with the Android-side changes inEjson.javaand enables avatar generation for video conference callers using the/avatar/{username}endpoint.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (2)
159-166: LGTM!The avatar fetching logic correctly handles the nullable
avatarUrifromgetCallerAvatarUri()and gracefully falls back to no avatar when the URI is null or the fetch fails.
211-217: LGTM!The
getAvatarhelper correctly delegates toNotificationHelper.fetchAvatarBitmapwith anullfallback, allowing the notification to proceed without an avatar if fetching fails or times out.android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
62-85: LGTM!The
fetchAvatarBitmapimplementation is well-structured:
- 3-second timeout is appropriate given FCM's 10-second limit
- Proper null/empty URI guard returning fallback
- Correctly restores thread interrupt status on
InterruptedException- Graceful fallback on all error paths
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
465-467: LGTM!Good refactor delegating avatar fetching to
NotificationHelper.fetchAvatarBitmapwithlargeIcon()as the fallback. This centralizes the timeout and error handling logic.
501-505: LGTM!The bounds check fix properly handles edge cases like
":"(colon only) wherepos=0andstart=2would exceed the message length. The early return when no colon is found is also a good improvement.android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (3)
191-201: LGTM!Good defensive programming. Validating column indices before accessing prevents crashes if the schema changes or the column is missing. The explicit logging helps with debugging.
248-272: LGTM!The double-encoding handling is a good defensive measure—JSON values can sometimes be double-encoded depending on how they were stored. Validating the required RSA key fields (
n,e,d) before proceeding prevents cryptic failures downstream.
288-356: LGTM!Significantly improved error handling in
decryptRoomKey:
- Early return on null/empty
e2eKey- Granular try/catch blocks with specific logging
- Null checks after each fallible operation
- Proper algorithm state management
This makes debugging E2E decryption issues much easier.
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (3)
63-83: LGTM!The
buildAvatarUrihelper centralizes server URL and credential validation, reducing code duplication. TheerrorContextparameter provides useful context in log messages for debugging different avatar types.
85-116: LGTM!The refactored
getAvatarUriproperly:
- URL-encodes
sender.usernameandridto handle special characters (addressing past review feedback)- Uses the API 24-compatible
URLEncoder.encode(String, String)overload- Distinguishes between DM (sender avatar) and group/channel (room avatar)
118-136: LGTM!The
getCallerAvatarUrimethod follows the same robust pattern asgetAvatarUri:
- Validates caller and username before proceeding
- URL-encodes the username
- Delegates to
buildAvatarUrifor consistent URI constructionios/NotificationService/NotificationService.swift (5)
1-10: LGTM!The addition of the Intents framework and
finalContentproperty appropriately supports the new communication intent functionality for richer notifications with avatars.
13-34: LGTM!The refactored guard-based payload extraction is much safer than the previous force-unwrap approach and properly handles malformed data with graceful fallback.
38-74: LGTM!The video conference handling correctly manages call cancellations, sets appropriate notification priority with
timeSensitive, and properly uses weak self to prevent retain cycles in the avatar fetch closure.
201-241: LGTM!The message content fetching logic properly handles placeholder removal, server fallbacks, and uses weak self to prevent retain cycles.
243-250: LGTM!The decryption helper appropriately handles different E2E encryption algorithm versions with safe optional returns.
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
iOS Build Available Rocket.Chat Experimental 4.68.0.107949 |
|
Android Build Available Rocket.Chat Experimental 4.68.0.107948 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTMSv1I2bYQ2c16qHto-fIin-txKTcqrTUTvb26CmqsruD5K3maAnkNLQZNSYYlgEuGPjqvOqhp7qIes3cv |
- Updated type casting for body content to ensure proper string handling. - Simplified variable declarations for senderImage, recipients, and speakableGroupName. - Configured URLSession with specific timeouts for better resource management during avatar fetching.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
ios/NotificationService/NotificationService.swift (3)
89-100: Simplify: Redundant optional cast.
bestAttemptContent.bodyis already of typeString(non-optional perUNMutableNotificationContentAPI), so casting it toString?is unnecessary. You can access it directly.🔎 Proposed simplification
- if let body = bestAttemptContent.body as? String { + let body = bestAttemptContent.body + if !body.isEmpty { let prefix = "\(senderUsername): " if body.hasPrefix(prefix) { bestAttemptContent.body = String(body.dropFirst(prefix.count)) } else { // Try with sender name (display name) as fallback let senderNamePrefix = "\(senderName): " if body.hasPrefix(senderNamePrefix) { bestAttemptContent.body = String(body.dropFirst(senderNamePrefix.count)) } } } - }
158-159: Use optional binding instead of force unwrap.While the force unwrap is safe here (checked by the ternary condition), idiomatic Swift prefers optional binding.
🔎 Proposed simplification
- speakableGroupName = (groupName != nil) ? INSpeakableString(spokenPhrase: groupName!) : nil + if let groupName = groupName { + speakableGroupName = INSpeakableString(spokenPhrase: groupName) + }
314-320: URL-encode room ID in avatar path.Line 319: The room ID (
rid) is used directly in the URL path without encoding. While room IDs are typically alphanumeric, encoding ensures correctness if they contain special characters.🔎 Proposed fix
} else { - guard let rid = payload.rid else { + guard let rid = payload.rid, + let encoded = rid.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) else { completion(nil) return } - avatarPath = "/avatar/room/\(rid)" + avatarPath = "/avatar/room/\(encoded)" }
📜 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 (1)
ios/NotificationService/NotificationService.swift
🧰 Additional context used
🧬 Code graph analysis (1)
ios/NotificationService/NotificationService.swift (2)
ios/Shared/RocketChat/RocketChat.swift (1)
decryptContent(77-82)ios/Shared/RocketChat/Storage.swift (1)
getCredentials(11-19)
⏰ 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 (5)
ios/NotificationService/NotificationService.swift (5)
1-10: LGTM: Imports and properties are well-structured.The addition of the Intents framework and
finalContentproperty appropriately supports the new avatar and intent-based notification features.
13-34: LGTM: Clean refactoring with safe parsing and routing.The guard-based flow safely parses the ejson payload and routes to appropriate processors with proper error handling.
38-74: LGTM: Video conference handling is correct.Proper handling of cancelled calls, appropriate use of
weak selfto prevent retain cycles, and sensible fallback tobestAttemptContentwhen intent update fails.
201-250: LGTM: Helper methods handle edge cases well.Both
fetchMessageContentanddecryptMessagehave appropriate error handling and fallback paths to ensure notifications are always delivered.
183-185: This API usage is correct according to Apple documentation.The
setImage(_:forParameterNamed: \.speakableGroupName)API is the documented and supported way to attach an image to the speakableGroupName parameter in INSendMessageIntent (available since iOS 12). This matches the official Apple Developer documentation and is used correctly in this code.Note: Some developers report that the group image and name appear in the system UI only when there are at least 2 recipients in the conversation, but this is a display behavior detail rather than an API usage issue.
Likely an incorrect or invalid review comment.
- Implemented URL encoding for user credentials to prevent malformed URLs during avatar fetching. - Updated the full avatar path construction to use encoded credentials for improved reliability.
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_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
ios/NotificationService/NotificationService.swift (4)
89-89: Simplify:bodyis alreadyString, notAny.
UNMutableNotificationContent.bodyis declared asString(not optional, notAny), so the optional castas? Stringis unnecessary. You can bind it directly:let body = bestAttemptContent.body.🔎 Proposed simplification
- if let body = bestAttemptContent.body as? String { + let body = bestAttemptContent.body + if !body.isEmpty {
157-159: Prefer optional map over force unwrap.While the force unwrap on Line 158 is safe due to the nil-check on Line 157, Swift's optional map is more idiomatic and eliminates the force unwrap.
🔎 Proposed refactor
- if isGroup { - speakableGroupName = (groupName != nil) ? INSpeakableString(spokenPhrase: groupName!) : nil + if isGroup { + speakableGroupName = groupName.map { INSpeakableString(spokenPhrase: $0) }
263-264: Prefer URLComponents for URL construction.String concatenation on Line 264 is fragile. Use
URLComponentsto safely build the URL with base, path, and query items, which handles edge cases like missing slashes and special characters more robustly.🔎 Proposed refactor
- let fullPath = "\(avatarPath)?format=png&size=100&rc_token=\(encodedToken)&rc_uid=\(encodedUserId)" - guard let avatarURL = URL(string: server + fullPath) else { + guard var components = URLComponents(string: server) else { completion(nil) return } + components.path = avatarPath + components.queryItems = [ + URLQueryItem(name: "format", value: "png"), + URLQueryItem(name: "size", value: "100"), + URLQueryItem(name: "rc_token", value: credentials.userToken), + URLQueryItem(name: "rc_uid", value: credentials.userId) + ] + guard let avatarURL = components.url else { + completion(nil) + return + }Note:
URLQueryItemhandles encoding automatically, so you can pass the raw credentials without manual encoding.
322-327: Consider URL-encoding the room ID.Line 326 interpolates
riddirectly into the path without encoding. While Rocket.Chat room IDs are typically alphanumeric, encoding ensures special characters don't cause malformed URLs or unexpected behavior.🔎 Proposed fix
} else { - guard let rid = payload.rid else { + guard let rid = payload.rid, + let encoded = rid.addingPercentEncoding(withAllowedCharacters: .urlPathAllowed) else { completion(nil) return } - avatarPath = "/avatar/room/\(rid)" + avatarPath = "/avatar/room/\(encoded)" }
📜 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 (1)
ios/NotificationService/NotificationService.swift
🧰 Additional context used
🧬 Code graph analysis (1)
ios/NotificationService/NotificationService.swift (5)
ios/Shared/RocketChat/API/Request.swift (2)
request(45-72)body(41-43)ios/RocketChat Watch App/Database/Database.swift (1)
removeTrailingSlash(125-131)ios/Shared/RocketChat/RocketChat.swift (1)
decryptContent(77-82)ios/Shared/RocketChat/Encryption.swift (1)
decryptContent(175-210)ios/Shared/RocketChat/Storage.swift (1)
getCredentials(11-19)
⏰ 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 (6)
ios/NotificationService/NotificationService.swift (6)
1-10: LGTM: Clean property setup.The new
Intentsimport andfinalContentproperty properly support the avatar-aware communication intent flow.
13-34: LGTM: Robust error handling.The guard-based pattern safely handles JSON decoding failures and routes notifications correctly by type.
38-74: LGTM: Video conference handling is sound.Properly handles call cancellations, sets time-sensitive priority, and gracefully falls back when avatar fetching fails or intent updating produces nil.
199-241: LGTM: Robust message content fetching.Properly removes placeholders, fetches from server with appropriate fallbacks, and updates ejson userInfo for correct in-app navigation.
243-250: LGTM: Safe E2E decryption.Correctly checks encryption algorithms and safely delegates to the encryption module with optional chaining.
293-303: LGTM: Caller avatar fetching is correct.Properly encodes the username with
.urlPathAllowedand safely retrieves credentials before constructing the avatar path.
diegolmello
changed the title
I'm seeing this on Android Studio. |
Proposed changes
This PR enhances push notification avatar handling across iOS and Android platforms. The changes improve avatar fetching, display, and error handling for both regular messages and video conference notifications. Key improvements include:
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-1581
How to test or reproduce
iOS/Android Testing:
Edge Cases:
Screenshots
Types of changes
Checklist
Further comments
This PR focuses on improving the reliability and user experience of push notification avatars. The changes include:
The implementation ensures that avatars are fetched efficiently without blocking notification delivery, with proper fallback mechanisms in place for edge cases.