chore: Migrate to expo-notifications #6846
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.
WalkthroughReplaces legacy Notifee/Firebase/react-native-notifications JS stack with Expo notifications/device; adds native Android video-conference handling (Firebase service, TurboModule, BroadcastReceiver, notification renderer), updates iOS notification service/delegate for videoconf, and migrates notification plumbing across JS, Android, and iOS. Changes
Sequence Diagram(s)sequenceDiagram
participant FCM as Firebase (FCM)
participant AndroidSvc as RCFirebaseMessagingService
participant Custom as CustomPushNotification
participant VideoNotif as VideoConfNotification
participant Broadcast as VideoConfBroadcast
participant VideoModule as VideoConfModule (TurboModule)
participant Main as MainActivity
participant JS as JavaScript app
FCM ->> AndroidSvc: deliver data message
AndroidSvc ->> Custom: onReceived(data -> Bundle)
alt videoconf payload
Custom ->> VideoNotif: showIncomingCall(bundle, ejson)
VideoNotif ->> User: display incoming call (Accept / Decline)
User ->> Broadcast: click action (ACCEPT/DECLINE)
Broadcast ->> VideoModule: storePendingAction(json) (SharedPreferences + emit if JS active)
Broadcast ->> Main: start MainActivity with extras
Main ->> VideoModule: getPendingAction() -> resolves to action JSON
Main ->> JS: dispatch deepLinkingClickCallPush with parsed action
else regular push
Custom ->> JS: forward push to RN runtime (push handling)
end
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
had a problem deploying
to
approve_e2e_testing
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/notifications/push.ts (1)
161-215: Use the correct async Expo APIgetLastNotificationResponseAsyncand fix the require-await violationTwo issues in
pushNotificationConfigure:
handleNotificationis declaredasyncbut has noawait, triggering therequire-awaitESLint rule.Notifications.getLastNotificationResponse()is treated as a synchronous API, but Expo's actual API isgetLastNotificationResponseAsync(), which is async and returns a Promise. This pattern is already correctly implemented elsewhere in the codebase (e.g.,app/lib/notifications/videoConf/getInitialNotification.ts).Make
pushNotificationConfigureitselfasync, change the handler to return aPromisewithout theasynckeyword, and useawaitwithgetLastNotificationResponseAsync:-export const pushNotificationConfigure = (onNotification: (notification: INotification) => void): Promise<any> => { +export const pushNotificationConfigure = async (onNotification: (notification: INotification) => void): Promise<any> => { if (configured) { - return Promise.resolve({ configured: true }); + return { configured: true }; } configured = true; // Set up how notifications should be handled when the app is in foreground Notifications.setNotificationHandler({ - handleNotification: async () => ({ + handleNotification: () => + Promise.resolve({ shouldShowAlert: false, shouldPlaySound: false, shouldSetBadge: false, shouldShowBanner: false, shouldShowList: false - }) + }) }); // ... (rest of code) - // Get initial notification (app was opened by tapping a notification) - const lastResponse = Notifications.getLastNotificationResponse(); + // Get initial notification (app was opened by tapping a notification) + const lastResponse = await Notifications.getLastNotificationResponseAsync(); if (lastResponse) { - return Promise.resolve(transformNotificationResponse(lastResponse)); + return transformNotificationResponse(lastResponse); } - return Promise.resolve(null); + return null; };Call sites already treat
pushNotificationConfigureas returning aPromise, so this change is compatible.
🧹 Nitpick comments (11)
app/views/PushTroubleshootView/components/DeviceNotificationSettings.tsx (1)
17-22: ConfirmLinking.openSettingssupport and consider a small safety netThe switch to
Linking.openSettings()on non‑iOS looks good and matches the intent of opening app settings, but this API depends on the React Native version and can reject on some devices/ROMs. It would be safer to both (a) confirm the app’s minimum RN version guaranteesLinking.openSettingsavailability, and (b) optionally add a.catchto avoid unhandled rejections and, if you want, fall back toLinking.openURL('app-settings:').- } else { - Linking.openSettings(); - } + } else { + Linking.openSettings().catch(() => { + // Optional: fallback if openSettings isn't supported + Linking.openURL('app-settings:').catch(() => { + // Swallow or log if even this fails + }); + }); + }app/views/JitsiMeetView/index.tsx (1)
49-57: Relying on WebView instead of BackHandler in the failure path is reasonableDropping the BackHandler listener in the
Linking.openURLfailure path avoids globally blocking the hardware back button and lets the WebView / navigator handle navigation, which is safer and more predictable. Consider optionally logging the error or showing a lightweight message ifopenURLfails for reasons other than “app not installed”, but the current behavior is acceptable as-is.ios/Podfile (1)
36-40: Consider adding a trailing comma for consistency.The static analysis tool suggests adding a trailing comma after the last item in the multiline array. This is a Ruby style convention that makes future diffs cleaner when adding new entries.
$static_framework = [ 'WatermelonDB', 'simdjson', - 'react-native-mmkv-storage' + 'react-native-mmkv-storage', ]ios/ReplyNotification.swift (1)
52-63: Consider the fallback presentation behavior.When
originalDelegateis nil,completionHandler([])suppresses foreground notification presentation. While expo-notifications should always be present, if for any reason the delegate becomes nil, users won't see foreground notifications.This is likely acceptable since expo-notifications is a core dependency, but worth being aware of during testing.
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (3)
113-147: Thread lifecycle: consider cleanup mechanism.The background thread used to wait for React initialization has no cancellation mechanism. If the notification is no longer relevant (e.g., user dismisses it before React initializes), the thread continues running and may post a stale notification.
Consider using a
ScheduledExecutorServiceor storing a reference to enable cancellation:+ private volatile boolean shouldCancelWait = false; + // Wait for React to initialize with timeout new Thread(() -> { int attempts = 0; int maxAttempts = 50; - while (!isReactInitialized() && attempts < maxAttempts) { + while (!isReactInitialized() && attempts < maxAttempts && !shouldCancelWait) {
238-244: Inconsistent synchronization onmBundle.At line 190,
mBundleassignment is synchronized, but here and at line 262, it's not. SincemBundlecan be accessed from multiple threads (background polling thread, E2E callback), consider making all assignments consistent:if (decrypted != null) { bundle.putString("message", decrypted); - mBundle = bundle; + synchronized(this) { + mBundle = bundle; + }Alternatively, declare
mBundleasvolatileif only single-assignment visibility is needed without compound operations.
342-350: Consider using named constants for video conf status values.Magic numbers
0and4for call status are unclear. Defining constants would improve readability and maintainability:+ private static final int VIDEO_CONF_STATUS_INCOMING = 0; + private static final int VIDEO_CONF_STATUS_ENDED = 4; + ... - if (status == null || status == 0) { + if (status == null || status == VIDEO_CONF_STATUS_INCOMING) { // Incoming call - show notification videoConf.showIncomingCall(bundle, ejson); - } else if (status == 4) { + } else if (status == VIDEO_CONF_STATUS_ENDED) { // Call cancelled/ended - dismiss notification videoConf.cancelCall(rid, callerId);android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (1)
10-14: Remove unusedConfigurationimport.The
Configurationimport on line 11 is not used 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/VideoConfBroadcast.java (1)
54-68: Consider null safety for caller fields.The
extras.getString()calls forcallerIdandcallerNamedefault to empty strings, but if these fields contain sensitive information, logging them at line 72 might be a compliance concern depending on whetherridcontains user-identifiable data.Consider removing
ridfrom the debug log if it could contain user-identifiable information:- Log.d(TAG, "Stored video conf action: " + event + " for rid: " + extras.getString("rid")); + Log.d(TAG, "Stored video conf action: " + event);android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.java (1)
38-54: Consider usingcommit()instead ofapply()for the clear operation.The
apply()method is asynchronous. If the app terminates before the write completes, the action could be processed again on next launch. Usingcommit()ensures the clear happens synchronously before resolving the promise.// Clear the action after reading if (action != null) { - prefs.edit().remove(KEY_VIDEO_CONF_ACTION).apply(); + prefs.edit().remove(KEY_VIDEO_CONF_ACTION).commit(); }app/lib/notifications/push.ts (1)
12-26: Async badge/dismiss helpers look fine; consider centralizing loggingThe async wrappers with try/catch around
setBadgeCountAsyncanddismissAllNotificationsAsynclook good. If you have a central logging/analytics facility, you might want to use it instead ofconsole.logso failures in production are easier to track.
📜 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 (37)
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(16 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java(3 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java(2 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.java(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfPackage.java(1 hunks)app/index.tsx(1 hunks)app/lib/notifications/index.ts(2 hunks)app/lib/notifications/push.ts(3 hunks)app/lib/notifications/videoConf/backgroundNotificationHandler.ts(0 hunks)app/lib/notifications/videoConf/getInitialNotification.ts(1 hunks)app/sagas/deepLinking.js(3 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(1 hunks)ios/NotificationService/NotificationService.swift(2 hunks)ios/Podfile(1 hunks)ios/ReplyNotification.swift(1 hunks)ios/RocketChatRN-Bridging-Header.h(0 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(6 hunks)ios/Shared/Models/NotificationType.swift(1 hunks)ios/Shared/Models/Payload.swift(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 (6)
- android/app/build.gradle
- ios/RocketChatRN-Bridging-Header.h
- app/lib/notifications/videoConf/backgroundNotificationHandler.ts
- patches/@react-native-firebase+messaging+21.12.2.patch
- patches/react-native-notifications+5.1.0.patch
- patches/@notifee+react-native+7.8.2.patch
🧰 Additional context used
🧬 Code graph analysis (11)
app/lib/notifications/push.ts (3)
app/definitions/INotification.ts (1)
INotification(1-16)app/lib/notifications/index.ts (1)
onNotification(18-66)app/sagas/deepLinking.js (1)
token(36-36)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.java (1)
VideoConfNotification(25-236)
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/Encryption.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
CustomPushNotification(45-659)
ios/NotificationService/NotificationService.swift (2)
ios/Shared/Extensions/String+Extensions.swift (1)
removeTrailingSlash(25-31)ios/Shared/RocketChat/RocketChat.swift (1)
getPushWithId(25-37)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
CustomPushNotification(45-659)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (2)
android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboPackage.java (1)
SSLPinningTurboPackage(15-45)android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfPackage.java (1)
VideoConfPackage(17-32)
app/index.tsx (1)
app/lib/notifications/videoConf/getInitialNotification.ts (1)
getInitialNotification(14-63)
ios/ReplyNotification.swift (4)
ios/NotificationService/NotificationService.swift (1)
didReceive(9-56)ios/Shared/Extensions/String+Extensions.swift (1)
removeTrailingSlash(25-31)ios/Shared/RocketChat/RocketChat.swift (1)
sendMessage(39-75)ios/Shared/RocketChat/API/Requests/SendMessage.swift (1)
body(64-66)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java (1)
Notification(25-58)
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java (1)
Notification(25-58)
🪛 GitHub Actions: Format Code with Prettier
app/lib/notifications/push.ts
[error] 170-170: Async method 'handleNotification' has no 'await' expression. ESLint: 'require-await' rule triggered.
🪛 GitHub Check: format
app/lib/notifications/push.ts
[failure] 170-170:
Async method 'handleNotification' has no 'await' expression
🪛 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 (50)
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1543-1547: LGTM - Standard privacy bundle integration for Expo modulesThe changes correctly add required privacy manifest bundles (
ExpoApplication_privacy.bundle,ExpoNotifications_privacy.bundle,ExpoDevice_privacy.bundle) to all three targets (RocketChatRN, Rocket.Chat, NotificationService). This is a standard CocoaPods-generated update that ensures compliance with Apple's App Privacy Details requirements when using these Expo modules.The pattern is consistent with other privacy bundles already in the project, and having these bundles in all targets ensures proper privacy declarations across the main app and extensions.
Also applies to: 1576-1580, 1636-1640, 1669-1673, 1973-1977, 2006-2010
app/views/JitsiMeetView/index.tsx (1)
5-5: Import cleanup after BackHandler removal looks good
BackHandleris no longer used and everything still imported fromreact-nativehere is referenced in the file, so this simplification is correct and keeps the module tidy.index.js (1)
25-27: LGTM!The comment clearly documents the architectural change, explaining that Android video conference notifications are now handled natively in the
RCFirebaseMessagingService -> CustomPushNotification -> VideoConfNotificationchain. This helps future maintainers understand why the background handler was removed.ios/Shared/Models/Payload.swift (3)
11-14: LGTM!The new
Callerstruct properly mirrors the AndroidEjson.Callerclass, maintaining consistency across platforms. The_idnaming follows the Rocket.Chat/MongoDB convention and will decode correctly from JSON.
29-33: LGTM!The video conference fields are appropriately defined as optionals, ensuring backward compatibility with existing notifications that don't include these fields.
21-21: Callers properly handle the optional messageId.The only usage in NotificationService.swift (line 45) correctly guards access with
if let messageId = data.messageId, ensuring safe handling of the optional type. No unsafe unwrapping patterns detected.app/sagas/deepLinking.js (3)
49-56: LGTM!Minor formatting change with no functional impact.
226-226: Good use of optional chaining.Adding
params.caller?._idprevents potential TypeError whencalleris undefined or null, improving robustness for edge cases in video conference notifications.
249-251: LGTM!The early return when
hostis falsy prevents errors during host normalization and is consistent with the similar guard inhandleOpen()at line 142.android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (4)
25-36: Excellent documentation of the notification architecture.The comment clearly explains the hybrid approach where JS uses expo-notifications for token registration while native code handles FCM message processing, notification display, E2E decryption, and direct reply. This is valuable for future maintainers understanding the system.
37-37: LGTM!Removing
INotificationsApplicationfrom the class declaration completes the migration away fromreact-native-notifications.
45-45: LGTM!
VideoConfPackageis properly added to the packages list, enabling theVideoConfModulenative module for JS-native communication.
68-72: [rewritten review comment]
[classification tag]android/app/src/main/java/chat/rocket/reactnative/notification/Encryption.java (2)
320-327: LGTM - Context fallback pattern is sound.The fallback to
CustomPushNotification.reactApplicationContextis properly guarded with a null check before use. The static volatile field ensures visibility across threads.
369-375: LGTM - Direct context assignment is appropriate here.Unlike
decryptMessagewhich receives aContextparameter,encryptMessageContenthas no incoming context to check, so directly using the static field with a null guard is the correct approach.react-native.config.js (1)
1-3: LGTM - Config cleanup aligns with dependency removal.The platform-specific exclusion for
@react-native-firebase/messagingis no longer needed since the package is being removed from the project. The empty dependencies object is a clean approach.ios/AppDelegate.swift (1)
30-31: LGTM - Notification handler initialization is properly placed.
ReplyNotification.configure()is called after Firebase/MMKV initialization but before React Native starts, ensuring the notification delegate chain is established early. TheExpoAppDelegatebase class handles the standard remote notification lifecycle methods that were removed.ios/ReplyNotification.swift (2)
16-28: LGTM - Delegate chain pattern is well-implemented.The weak reference to
originalDelegateprevents retain cycles, and the configuration properly preserves the expo-notifications delegate while intercepting reply actions. This is the correct pattern for delegate interception.
79-117: LGTM - Reply handling with proper background task management.The
deferblock ensuresendBackgroundTaskandcompletionHandlerare always called, even on success. The pattern correctly handles:
- Payload decoding failures (early return with completionHandler)
- Background task lifecycle (safe to call
endBackgroundTaskeven if invalid)- Failure notification display on error
jest.setup.js (1)
139-156: LGTM! Comprehensive expo-notifications mock setup.The mock provides a complete API surface for testing, including proper subscription cleanup with
{ remove: jest.fn() }returns for all listener methods. This aligns well with the expo-notifications migration.android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (2)
360-371: LGTM! Notification channel creation is correct.Proper Android O+ check with
IMPORTANCE_HIGHfor push notifications. The null check onnotificationManageradds resilience.
393-397: LGTM! Correct PendingIntent flags for Android S+.Using
FLAG_IMMUTABLEfor content intents on Android S+ is correct. The reply intent at line 611 correctly usesFLAG_MUTABLEsinceRemoteInputrequires mutability.ios/Shared/Models/NotificationType.swift (1)
11-15: LGTM! Clean addition of videoconf notification type.The new case aligns with the cross-platform video conference notification support and integrates properly with the existing
Codableenum.app/index.tsx (1)
141-144: LGTM! Correct integration of video conference initial notification handling.The placement is appropriate—after push notification handling but before deep linking—ensuring video conference notifications from cold start are properly dispatched via
deepLinkingClickCallPush. The early return prevents duplicate processing.app/sagas/troubleshootingNotification.ts (1)
22-23: LGTM! Clean migration to expo-notifications permissions API.The permission check correctly uses
status === 'granted'. This is slightly stricter than the previous notifee implementation (which may have consideredundeterminedas enabled), which is the correct behavior for troubleshooting diagnostics.android/app/src/main/AndroidManifest.xml (2)
78-85: LGTM! Firebase messaging service properly configured.The service is correctly set with
exported=falseand a high priority intent-filter to ensure it receives FCM messages before other potential handlers.
95-103: LGTM! VideoConfBroadcast receiver properly secured.Good use of
exported=falsewith explicit intent-filter actions using the app's package namespace. This ensures only the app can trigger these video conference actions.android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.java (2)
22-45: LGTM! Solid FCM message handling implementation.Good defensive checks (empty data validation) and proper error handling. The try-catch ensures a malformed notification doesn't crash the service.
47-53: LGTM! Token refresh handling is appropriate.Delegating token management to the expo-notifications JS layer via
getDevicePushTokenAsync()is the correct approach for this migration.android/app/src/main/java/chat/rocket/reactnative/MainActivity.kt (1)
45-85: LGTM! Video conference intent handling is well-implemented.Good defensive patterns: early return when
eventis missing, clearing the flag to prevent re-processing, and proper notification cancellation. The JSON serialization via Gson for JS interop is appropriate.android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (3)
38-45: LGTM! Video conference fields are well-documented.The inline comments explaining the purpose (
// For video conf notifications) and status values (// For video conf: 0=incoming, 4=cancelled) improve code readability. UsingIntegerinstead ofintforstatuscorrectly allows null-checking for the incoming call case.
232-235: LGTM! Caller class structure is consistent with existing Sender class.The new
Callerinner class mirrors theSenderclass structure, maintaining consistency in how user identity data is modeled.
77-80: No thread safety issue:reactApplicationContextis declaredvolatile. The field in CustomPushNotification is properly declared with thevolatilekeyword (line 50), which provides visibility guarantees for reads and writes across threads. This is a standard and safe pattern for shared mutable state in multi-threaded Java code. No race condition exists here.Likely an incorrect or invalid review comment.
ios/NotificationService/NotificationService.swift (1)
58-86: LGTM! Video conference notification handling is well-structured.Good implementation with proper status-based branching, iOS 15+ availability check for
interruptionLevel, and appropriate use of localized strings. Theringtone.mp3file is present in the project bundle, confirming the sound resource dependency is satisfied.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.java (3)
1-22: LGTM! Well-structured BroadcastReceiver setup.Good use of constants for the TAG and clear class documentation explaining the purpose.
24-46: Proper validation and action mapping.The null checks and action-to-event mapping logic are correct. Early returns on invalid input prevent unnecessary processing.
74-80: Appropriate activity launch flags for BroadcastReceiver context.The flags
FLAG_ACTIVITY_NEW_TASK | FLAG_ACTIVITY_CLEAR_TOPare correct when starting an activity from a non-Activity context. The extras forwarding ensures MainActivity receives all necessary data.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.java (3)
1-32: LGTM! Standard React Native module setup.Proper use of
@ReactModuleannotation and clear constants for SharedPreferences.
59-68: Silent error handling is acceptable for cleanup operations.The
clearPendingActionmethod appropriately ignores errors since failing to clear a pending action is not critical.
74-77: LGTM! Static method allows native-to-native calls.Using a static method for
storePendingActionis correct since it's called fromVideoConfBroadcastbefore the React context may be available.app/lib/notifications/videoConf/getInitialNotification.ts (4)
1-8: LGTM! Proper imports for cross-platform handling.Clean separation of platform-specific dependencies.
14-29: LGTM! Android path with proper error handling.The null checks and try-catch ensure graceful degradation if the native module is unavailable or returns invalid data.
40-42: Type narrowing could be simplified.The trigger type checking is verbose but necessary for TypeScript safety. This is acceptable given expo-notifications' type definitions.
31-60: The VIDEOCONF notification category and action identifiers are properly configured.The code correctly checks for
'DECLINE_ACTION'on line 49. The iOS notification category setup inapp/lib/notifications/push.ts(lines 107–117) properly registers bothACCEPT_ACTIONandDECLINE_ACTIONfor theVIDEOCONFcategory, matching what the TypeScript code expects and what the iOS native code sets as the category identifier inNotificationService.swift.app/lib/notifications/index.ts (2)
18-28: LGTM! Clear separation of button action handling.Correctly distinguishes between explicit Accept/Decline button presses and other notification interactions.
30-64: Good fallback toappInit()when notification cannot be parsed.The try-catch ensures the app initializes even if notification parsing fails. The
typesmapping correctly handles all subscription types.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfPackage.java (1)
1-32: LGTM! Standard ReactPackage implementation.Correctly registers the VideoConfModule for React Native access. VideoConfPackage is properly imported and registered in MainApplication.kt (line 45).
app/lib/notifications/push.ts (1)
131-159: The current implementation correctly pairs device token retrieval with device token updatesThe code properly uses
getDevicePushTokenAsync()to obtain the native device token (APNs/FCM) andaddPushTokenListener()to listen for device token updates.addPushTokenListeneris specifically designed for native device token changes, not Expo tokens, so there is no mismatch here. The token type remains consistent as a native device token throughout.Likely an incorrect or invalid review comment.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.java (2)
47-72: Notification channel configuration follows current Android best practicesThe channel is correctly configured with
IMPORTANCE_HIGHfor urgent head-up behavior, ringtone audio set viachannel.setSound()withAudioAttributes.USAGE_NOTIFICATION_RINGTONEfor proper audio routing, and public lockscreen visibility. This aligns with Android 8.0+ patterns for incoming call notifications. The null-check onnotificationManagerbefore creating the channel is good defensive coding.
214-235: Cancel helpers use hashCode() — note 32-bit collision risk
cancelCallreuses(rid + callerId).hashCode()fromshowIncomingCall, andcancelByIdcorrectly wrapsNotificationManager.cancel. The ID generation is consistent between both methods.However,
String.hashCode()carries practical caveats: it produces stable values across process restarts on Android but is not guaranteed by the hashCode specification, and 32-bit collisions are possible with large uid spaces. For production use, consider either accepting the collision risk or usingNotificationManager.notify(tag, id, ...)with a string tag to avoid the integer hash altogether.
| 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, message.length()); | ||
| return message.substring(start); | ||
| } | ||
| return message; | ||
| } |
There was a problem hiding this comment.
Potential StringIndexOutOfBoundsException in extractMessage.
If the message contains a colon at position pos where pos + 2 > message.length(), substring(start) will throw. For example, message "a:" has pos=1, start=3, but length=2.
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;
}📝 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.
| 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, message.length()); | |
| return message.substring(start); | |
| } | |
| return message; | |
| } | |
| 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(":"); | |
| if (pos == -1) { | |
| return message; | |
| } | |
| int start = Math.min(pos + 2, message.length()); | |
| return message.substring(start); | |
| } | |
| return message; | |
| } |
🤖 Prompt for AI Agents
In
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java
around lines 510 to 520, the code computes start = pos == -1 ? 0 : pos + 2 and
then calls message.substring(start), which can throw
StringIndexOutOfBoundsException when pos+2 > message.length(); change the start
calculation to clamp it within the message bounds (for example start = pos == -1
? 0 : Math.min(pos + 2, message.length())) so substring(start) is safe and
returns an empty string when the colon is at the end.
android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java
Show resolved
Hide resolved
| public void showIncomingCall(Bundle bundle, Ejson ejson) { | ||
| String rid = ejson.rid; | ||
| // Video conf uses 'caller' field, regular messages use 'sender' | ||
| String callerId = ""; | ||
| String callerName = "Unknown"; | ||
|
|
||
| if (ejson.caller != null) { | ||
| callerId = ejson.caller._id != null ? ejson.caller._id : ""; | ||
| callerName = ejson.caller.name != null ? ejson.caller.name : "Unknown"; | ||
| } else if (ejson.sender != null) { | ||
| // Fallback to sender if caller is not present | ||
| callerId = ejson.sender._id != null ? ejson.sender._id : ""; | ||
| callerName = ejson.sender.name != null ? ejson.sender.name : (ejson.senderName != null ? ejson.senderName : "Unknown"); | ||
| } | ||
|
|
||
| // Generate unique notification ID from rid + callerId | ||
| String notificationIdStr = (rid + callerId).replaceAll("[^A-Za-z0-9]", ""); | ||
| int notificationId = notificationIdStr.hashCode(); | ||
|
|
||
| Log.d(TAG, "Showing incoming call notification from: " + callerName); | ||
|
|
||
| // Create intent data for actions - include all required fields for JS | ||
| Bundle intentData = new Bundle(); | ||
| intentData.putString("rid", rid != null ? rid : ""); | ||
| intentData.putString("notificationType", "videoconf"); | ||
| intentData.putString("callerId", callerId); | ||
| intentData.putString("callerName", callerName); | ||
| intentData.putString("host", ejson.host != null ? ejson.host : ""); | ||
| intentData.putString("callId", ejson.callId != null ? ejson.callId : ""); | ||
| intentData.putString("ejson", bundle.getString("ejson", "{}")); | ||
| intentData.putInt("notificationId", notificationId); | ||
|
|
||
| Log.d(TAG, "Intent data - rid: " + rid + ", callerId: " + callerId + ", host: " + ejson.host + ", callId: " + ejson.callId); | ||
|
|
||
| // Full screen intent - opens app when notification is tapped | ||
| Intent fullScreenIntent = new Intent(context, MainActivity.class); | ||
| fullScreenIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TOP); | ||
| fullScreenIntent.putExtras(intentData); | ||
| fullScreenIntent.putExtra("event", "default"); | ||
|
|
||
| PendingIntent fullScreenPendingIntent; | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { | ||
| fullScreenPendingIntent = PendingIntent.getActivity( | ||
| context, notificationId, fullScreenIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE | ||
| ); | ||
| } else { | ||
| fullScreenPendingIntent = PendingIntent.getActivity( | ||
| context, notificationId, fullScreenIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | ||
| ); | ||
| } | ||
|
|
||
| // Accept action - directly opens MainActivity (Android 12+ blocks trampoline pattern) | ||
| Intent acceptIntent = new Intent(context, MainActivity.class); | ||
| acceptIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TOP); | ||
| acceptIntent.putExtras(intentData); | ||
| acceptIntent.putExtra("event", "accept"); | ||
| acceptIntent.putExtra("videoConfAction", true); | ||
| acceptIntent.setAction(ACTION_ACCEPT + "_" + notificationId); // Unique action to differentiate intents | ||
|
|
||
| PendingIntent acceptPendingIntent; | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { | ||
| acceptPendingIntent = PendingIntent.getActivity( | ||
| context, notificationId + 1, acceptIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE | ||
| ); | ||
| } else { | ||
| acceptPendingIntent = PendingIntent.getActivity( | ||
| context, notificationId + 1, acceptIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | ||
| ); | ||
| } | ||
|
|
||
| // Decline action - directly opens MainActivity | ||
| Intent declineIntent = new Intent(context, MainActivity.class); | ||
| declineIntent.setFlags(Intent.FLAG_ACTIVITY_NEW_TASK | Intent.FLAG_ACTIVITY_CLEAR_TOP); | ||
| declineIntent.putExtras(intentData); | ||
| declineIntent.putExtra("event", "decline"); | ||
| declineIntent.putExtra("videoConfAction", true); | ||
| declineIntent.setAction(ACTION_DECLINE + "_" + notificationId); // Unique action to differentiate intents | ||
|
|
||
| PendingIntent declinePendingIntent; | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { | ||
| declinePendingIntent = PendingIntent.getActivity( | ||
| context, notificationId + 2, declineIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | PendingIntent.FLAG_IMMUTABLE | ||
| ); | ||
| } else { | ||
| declinePendingIntent = PendingIntent.getActivity( | ||
| context, notificationId + 2, declineIntent, | ||
| PendingIntent.FLAG_UPDATE_CURRENT | ||
| ); | ||
| } | ||
|
|
||
| // Get icons | ||
| Resources res = context.getResources(); | ||
| String packageName = context.getPackageName(); | ||
| int smallIconResId = res.getIdentifier("ic_notification", "drawable", packageName); | ||
|
|
||
| // Build notification | ||
| NotificationCompat.Builder builder = new NotificationCompat.Builder(context, CHANNEL_ID) | ||
| .setSmallIcon(smallIconResId) | ||
| .setContentTitle("Incoming call") | ||
| .setContentText("Video call from " + callerName) | ||
| .setPriority(NotificationCompat.PRIORITY_HIGH) | ||
| .setCategory(NotificationCompat.CATEGORY_CALL) | ||
| .setVisibility(NotificationCompat.VISIBILITY_PUBLIC) | ||
| .setAutoCancel(false) | ||
| .setOngoing(true) | ||
| .setFullScreenIntent(fullScreenPendingIntent, true) | ||
| .setContentIntent(fullScreenPendingIntent) | ||
| .addAction(0, "Decline", declinePendingIntent) | ||
| .addAction(0, "Accept", acceptPendingIntent); | ||
|
|
||
| // Set sound for pre-O devices | ||
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) { | ||
| Uri ringtoneUri = RingtoneManager.getDefaultUri(RingtoneManager.TYPE_RINGTONE); | ||
| builder.setSound(ringtoneUri); | ||
| } | ||
|
|
||
| // Show notification | ||
| if (notificationManager != null) { | ||
| notificationManager.notify(notificationId, builder.build()); | ||
| Log.d(TAG, "Video call notification displayed with ID: " + notificationId); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Android NotificationCompat.Builder setSmallIcon(0) missing resource crash behavior
💡 Result:
Short answer: Passing 0 to setSmallIcon (i.e. no valid drawable resource) will cause the notification to fail at post time and crash the app — typically with errors like "Invalid notification (no valid small icon)" / "Couldn't create icon: StatusBarIcon" or a RemoteServiceException (Bad notification posted). The platform docs state small icon is required and NotificationCompat expects a valid resource id. [1][2][3][4]
Fix: Always pass a valid drawable resource id (not 0). Provide a default notification icon (manifest meta-data or app drawable) and/or guard your code before calling setSmallIcon(…) so you never call it with 0. [1][4]
Sources:
- Android docs: small icon is required / NotificationCompat.Builder setSmallIcon. [1][4]
- Reports and stack traces showing "Invalid notification (no valid small icon)" / RemoteServiceException when icon/resource invalid. [2][3]
🏁 Script executed:
find . -name "VideoConfNotification.java" -type fRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 169
🏁 Script executed:
wc -l ./android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 173
🏁 Script executed:
cat -n ./android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 12205
Fix critical small icon crash and add null-safety guards to showIncomingCall
The method has several robustness issues that can crash the app:
- Critical: Small icon resource lookup can crash notifications
Line 178–182: Ific_notificationdrawable is missing or renamed,getIdentifier()returns0, andsetSmallIcon(0)will crash the notification post withRemoteServiceException: Bad notification postedor"Invalid notification (no valid small icon)". Always validate the resource ID before passing it tosetSmallIcon():
int smallIconResId = res.getIdentifier("ic_notification", "drawable", packageName);
+ if (smallIconResId == 0) {
+ smallIconResId = android.R.drawable.sym_call_incoming; // or your app's default icon
+ }- Null-safety for
ejsonandbundle
Line 81 dereferencesejson.ridwithout null checks; line 109 callsbundle.getString()without null validation. Either can crash if upstream parsing fails:
+ if (ejson == null) {
+ Log.w(TAG, "showIncomingCall called with null Ejson payload");
+ return;
+ }
+ if (bundle == null) {
+ bundle = new Bundle();
+ }
String rid = ejson.rid;-
Notification ID collision when both
ridandcallerIdare empty
Line 96–97: If both are empty,notificationIdStrbecomes"", andhashCode()returns0, causing multiple such calls to reuse notification ID 0. Consider a fallback:notificationIdStr = (rid + callerId).isEmpty() ? String.valueOf(System.currentTimeMillis()) : (rid + callerId). -
Hard-coded user-facing strings
Lines 183–193:"Incoming call","Video call from ","Accept","Decline"should be moved to string resources for localization and consistency.
🤖 Prompt for AI Agents
In
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.java
around lines 80–206, make showIncomingCall null-safe and robust: check bundle
and ejson for null before dereferencing (guard ejson.rid, ejson.caller/sender,
ejson.host/callId and use empty defaults), validate bundle before calling
bundle.getString("ejson", "{}"), and return/log if required data is missing;
validate the drawable resource id from getIdentifier() and if it's 0 use a safe
fallback (app icon or a bundled default) before calling setSmallIcon(); avoid
notification ID collisions by using a fallback when (rid + callerId) is empty
(e.g., use System.currentTimeMillis() or a secure random value to build
notificationIdStr); and replace hard-coded UI strings ("Incoming call", "Video
call from ", "Accept", "Decline") with string resources referenced via
context.getString(...).
package.json
Outdated
| "expo-device": "^8.0.10", | ||
| "expo-document-picker": "13.1.4", | ||
| "expo-file-system": "18.1.7", | ||
| "expo-haptics": "14.1.3", | ||
| "expo-image": "^2.3.2", | ||
| "expo-keep-awake": "14.1.3", | ||
| "expo-local-authentication": "16.0.3", | ||
| "expo-navigation-bar": "^4.2.4", | ||
| "expo-notifications": "^0.32.14", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
expo-notifications 0.32 Expo SDK 53 compatibility requirements
💡 Result:
Short answer
- expo-notifications 0.32 (0.32.x) is published as the current 0.32 release series and is compatible with Expo SDK 53. The package's package.json lists generic peerDependencies (expo/react/react-native: "*"), and the SDK 53 docs and changelog reference expo-notifications updates for SDK 53. [1][2][3]
Key compatibility/requirements for using expo-notifications 0.32 with Expo SDK 53
- You must install expo-notifications (e.g. npx expo install expo-notifications or npm/yarn). Its package.json shows it as the 0.32.x release and expects to be used inside an Expo project. [2][3]
- Push notifications on Android: from SDK 53 remote push notifications are not available in Expo Go — a development build (or standalone/EAS build) is required to test/send push notifications on Android. Local notifications still work in Expo Go. [1]
- App config / build-time plugin: expo-notifications requires using its config plugin (or the built-in plugin) and proper native build configuration (EAS / npx expo run:android|ios) to enable push token registration and entitlements (especially for iOS). Users have reported that omitting the plugin in app.json/app.config.* prevents iOS push from working. Add the plugin and follow the push-notifications setup guide when building. [1][4]
- Platform / OS build requirements: follow SDK 53 platform requirements (compile/target SDK and Xcode versions) when building native apps. See the Expo SDK 53 platform table for supported OS/Xcode/compileSdk targets. [5]
References
- Expo SDK 53 notifications docs (push-notifications behavior in Expo Go / configuration). [1]
- expo-notifications package (0.32.10) package.json on UNPKG (shows 0.32.x release metadata). [2]
- expo-notifications CHANGELOG (0.32.x release notes). [3]
- GitHub issue noting need to add expo-notifications config plugin for iOS push on SDK 53. [4]
- Expo SDK platform/compile requirements table (SDK 53). [5]
If you want, I can paste the exact install/config steps for SDK 53 (app.json plugin entry, EAS build steps, and the minimal code to request permissions and get the device token).
🌐 Web query:
expo-device 8.0 Expo SDK 53 compatibility
💡 Result:
Short answer: No — [email protected] is not the version bundled with Expo SDK 53. SDK 53 was released in Apr/May 2025 and expects the v7.x family of Expo modules; expo-device 8.0.x was released later (Aug–Oct 2025). Use npx expo install expo-device (or pin to the v7.x matching your SDK) to get the correct module for SDK 53. [1][2]
Sources
- Expo SDK 53 release notes. [1]
- expo-device changelog showing 8.0.x release dates. [2]
Fix expo-device version — 8.0.x is incompatible with Expo SDK 53.
The expo-device version should be from the v7.x family (e.g., ^7.x.x), not 8.0.x. Expo SDK 53 was released in April/May 2025 and expects expo-device v7.x; version 8.0.x was released later and is not compatible with this SDK version. Update the dependency to use the correct version range.
The expo-notifications version (^0.32.14) is compatible with SDK 53 and requires no changes.
🤖 Prompt for AI Agents
In package.json around lines 64 to 72, the expo-device dependency is pinned to
^8.0.10 which is incompatible with Expo SDK 53; change the expo-device entry to
a v7.x range (for example ^7.0.0 or ^7.x.x) so it matches SDK 53 expectations,
then run yarn/npm install and verify metro/build to ensure no version conflicts;
leave expo-notifications (^0.32.14) as-is.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/notifications/push.ts (1)
190-206: Missing listener cleanup could cause memory leaks.The listeners added via
addPushTokenListenerandaddNotificationResponseReceivedListenerare never removed. While theconfiguredflag prevents re-initialization, this could still cause issues during testing or hot-reloading.Consider storing the subscriptions and exposing a cleanup function:
let tokenSubscription: Notifications.Subscription | null = null; let responseSubscription: Notifications.Subscription | null = null; // In pushNotificationConfigure: tokenSubscription = Notifications.addPushTokenListener(tokenData => { deviceToken = tokenData.data; }); responseSubscription = Notifications.addNotificationResponseReceivedListener(response => { // ... }); // Export cleanup function: export const cleanupPushNotifications = () => { tokenSubscription?.remove(); responseSubscription?.remove(); configured = false; };
🧹 Nitpick comments (4)
app/lib/notifications/videoConf/getInitialNotification.ts (3)
8-8: Add TypeScript types for VideoConfModule.The
VideoConfModulelacks TypeScript type definitions, which reduces type safety and IDE support.Consider defining an interface for the module:
interface IVideoConfModule { getPendingAction: () => Promise<string | null>; } const { VideoConfModule } = NativeModules as { VideoConfModule?: IVideoConfModule };
17-24: Consider validating parsed data structure.The code parses JSON but only checks
notificationType. If the parsed data has an unexpected structure, it could cause issues downstream indeepLinkingClickCallPush.Consider adding basic validation:
const data = JSON.parse(pendingAction); -if (data?.notificationType === 'videoconf') { +if (data && typeof data === 'object' && data.notificationType === 'videoconf') { store.dispatch(deepLinkingClickCallPush(data)); return true; }
40-42: Type guard relies on runtime property checking.The condition
'type' in trigger && trigger.type === 'push'uses runtime checks that TypeScript can't fully validate. This is acceptable but worth noting.If expo-notifications exports type guards or discriminated unions for triggers, prefer those for better type safety.
app/lib/notifications/push.ts (1)
33-79: Consider improving type safety for payload transformation.The extensive use of
Record<string, any>and fallback to empty strings could hide missing data issues. While functional, stronger typing would catch transformation errors earlier.Consider defining intermediate types:
interface RemoteMessagePayload { message?: string; style?: string; ejson?: string; collapse_key?: string; notId?: string; msgcnt?: string; title?: string; from?: string; image?: string; soundname?: string; }This would make the transformation more self-documenting and catch missing fields during development.
📜 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 (2)
app/lib/notifications/push.ts(3 hunks)app/lib/notifications/videoConf/getInitialNotification.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/lib/notifications/push.ts (2)
app/definitions/INotification.ts (1)
INotification(1-16)app/lib/notifications/index.ts (1)
onNotification(18-66)
app/lib/notifications/videoConf/getInitialNotification.ts (2)
app/lib/store/auxStore.ts (1)
store(6-6)app/actions/deepLinking.ts (1)
deepLinkingClickCallPush(26-31)
🔇 Additional comments (6)
app/lib/notifications/videoConf/getInitialNotification.ts (1)
10-14: LGTM!The signature change to return
Promise<boolean>is well-documented and provides clear semantics for whether a video conference action was dispatched.app/lib/notifications/push.ts (5)
12-18: LGTM!The migration to
setBadgeCountAsynccorrectly updates the signature to returnPromise<void>and includes appropriate error handling.
20-26: LGTM!Properly migrated to
dismissAllNotificationsAsyncwith consistent error handling.
84-125: LGTM!The notification categories are properly configured with appropriate actions and localized titles. The structure correctly distinguishes between MESSAGE (reply in background) and VIDEOCONF (foreground) use cases.
130-158: LGTM!The permission request flow is correctly implemented with proper device checks and graceful error handling.
209-212: No issues identified. The code correctly callsNotifications.getLastNotificationResponse()withoutawait, which is the appropriate synchronous method in expo-notifications v0.32.14. The library also providesNotifications.getLastNotificationResponseAsync()for async use cases, but the synchronous variant is intentionally used here and is the correct choice.
| }); | ||
|
|
||
| // Set up notification categories for iOS | ||
| setupNotificationCategories(); |
There was a problem hiding this comment.
Missing await for setupNotificationCategories.
The async function setupNotificationCategories() is called without await, which means configuration might continue before categories are set up. While this may work in practice due to iOS's asynchronous nature, it's inconsistent with async patterns.
Apply this diff to await the call:
- setupNotificationCategories();
+ await setupNotificationCategories();Note: This requires making pushNotificationConfigure async or restructuring the promise chain.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/lib/notifications/push.ts around line 180, the async function
setupNotificationCategories() is invoked without awaiting it, risking
continuation before categories are configured; update pushNotificationConfigure
to be async (or return a Promise) and add await setupNotificationCategories();
so the call is awaited, or alternatively chain the promise (return
setupNotificationCategories().then(...)) to ensure configuration waits for
completion; ensure callers of pushNotificationConfigure are updated to handle
the async return.
|
iOS Build Available Rocket.Chat Experimental 4.68.0.107809 |
|
Android Build Available Rocket.Chat Experimental 4.68.0.107808 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNR5VeS_aT_14h-wCQVjHcPgTUTPbsNvxCauJr7aPm-67ifOGf8bQEkD5sD41uX9kFBaHueVG6_KzWKS4c1p |
…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.
diegolmello
force-pushed
the
chore.expo-notifications
branch
from
c6f20e7 to
6620a1b
Compare
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
ios/fastlane/Fastfile (1)
115-125: ForcingARCHS=arm64limits simulator builds to Apple Silicon MacsAdding
ARCHS=arm64toxcargswill prevent this lane from building on Intel Macs (which needx86_64for iOS Simulator). If your CI or contributors still use Intel hardware, this lane will start failing there.If the intent is to fix Apple Silicon / arm64-simulator issues, consider one of:
- Gate this via an env var or separate lane (e.g.,
build_experimental_simulator_apple_silicon).- Let Xcode choose architectures (no explicit
ARCHS) and instead handle problematic pods via their own settings.- Document in the README / iOS build doc that this lane now assumes Apple Silicon-only.
Please confirm this trade-off is acceptable for your build matrix.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt (1)
56-65: Consider logging errors in clearPendingAction for debugging.While ignoring errors in a cleanup method may be intentional, logging the exception would aid debugging without affecting functionality.
Apply this diff to add logging:
@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.w("VideoConfModule", "Failed to clear pending action", e) } }android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (1)
188-195: Consider addingFLAG_IMMUTABLEfor pre-S devices.For security best practices,
FLAG_IMMUTABLEis recommended on Android M+ when the PendingIntent doesn't need to be modified. Currently, pre-S devices don't include this flag.private fun createPendingIntent(requestCode: Int, intent: Intent): PendingIntent { val flags = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S) { PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE + } else if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) { + PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE } else { PendingIntent.FLAG_UPDATE_CURRENT } return PendingIntent.getActivity(context, requestCode, intent, flags) }
📜 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 (14)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt(1 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/lib/native/NativeVideoConfAndroid.ts(1 hunks)app/lib/notifications/videoConf/getInitialNotification.ts(1 hunks)ios/Shared/RocketChat/API/Request.swift(2 hunks)ios/fastlane/Fastfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java
🧰 Additional context used
🧬 Code graph analysis (5)
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)
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/RCFirebaseMessagingService.kt (1)
android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
CustomPushNotification(45-659)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboPackage.java (1)
SSLPinningTurboPackage(15-45)
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)
🔇 Additional comments (19)
ios/Shared/RocketChat/API/Request.swift (2)
58-58: LGTM!The User-Agent header is now set using the new computed property, which improves maintainability and provides proper identification of the client.
74-82: Format inconsistency between platforms and Bundle.main usage in notification service extension warrant verification.The implementation constructs a custom User-Agent string. Two concerns should be addressed:
Format inconsistency: The React Native app uses a browser-style User-Agent (
Mozilla/5.0 (iPhone...)), while this iOS notification service extension uses a custom format (RC Mobile; ios ...). Confirm this difference is intentional and appropriate for the API requirements.Bundle.main in notification extension: Since this code is in the NotificationService extension,
Bundle.mainrefers to the extension's bundle, not the main app's bundle. If version info must match the main app, consider usingBundle(identifier: "com.yourapp.mainapp")or sharing version data via app groups.android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java (1)
130-130: LGTM! User-Agent header added for better request tracking.The addition of the User-Agent header using NotificationHelper.getUserAgent() will help with server-side analytics and debugging by providing Android version, app version, and build number information.
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
29-39: LGTM! Well-documented User-Agent helper method.The getUserAgent() method is implemented correctly with clear formatting and appropriate usage of Android Build and BuildConfig constants.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt (1)
12-50: LGTM! SharedPreferences pattern correctly implemented.The module correctly uses SharedPreferences to pass video conference actions from native to JS. The getPendingAction method properly clears the action after reading to ensure one-time consumption.
app/lib/native/NativeVideoConfAndroid.ts (1)
1-9: LGTM! Clean TurboModule interface definition.The interface correctly defines the video conference module bridge with appropriate return types that match the native Kotlin implementation.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfBroadcast.kt (2)
20-38: LGTM! Action validation and mapping correctly implemented.The broadcast receiver properly validates the action and extras before processing, and correctly maps native action constants to event strings for the JS layer.
47-55: No issues found with caller field key consistency.The Android implementation correctly uses the same field names as iOS. Both platforms receive
caller._idandcaller.namefrom the server payload. Android's intermediate representation in intent extras ("callerId"and"callerName"strings) is necessary sinceIntent.putExtra()only accepts primitive types, but these are correctly reconstructed back to thecaller._idandcaller.namestructure in VideoConfBroadcast and MainActivity, matching the iOS approach. The field naming is consistent throughout the notification flow.android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfTurboPackage.kt (1)
12-37: LGTM! Standard TurboReactPackage implementation.The package correctly implements TurboReactPackage with appropriate module info flags and follows the same pattern as other TurboModule packages in the codebase (e.g., SSLPinningTurboPackage).
app/lib/notifications/videoConf/getInitialNotification.ts (2)
13-28: LGTM! Android path correctly retrieves and dispatches pending video conference actions.The Android implementation properly checks for the native module's existence, parses the pending action JSON, validates the notification type, and dispatches the action to the store.
30-59: No action required. The DECLINE_ACTION constant is consistently defined and used across the codebase. It is properly registered withexpo-notifications.setNotificationCategoryAsync()inapp/lib/notifications/push.tsand all references in the JS/TS layer (including the code under review) use the same string identifier. The native iOS implementation is abstracted by the expo-notifications library, so no separate native code verification is necessary.android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
23-46: LGTM! VideoConfTurboPackage correctly registered and architecture documented.The changes properly integrate the VideoConfTurboPackage into the React Native package list and provide helpful documentation about the notification architecture. The removal of INotificationsApplication interface aligns with the migration to expo-notifications.
android/app/src/main/java/chat/rocket/reactnative/notification/NativeVideoConfSpec.kt (1)
9-23: LGTM!Clean TurboModule spec definition. The abstract class correctly defines the API surface for video conference action handling, with proper module naming and React method annotations that will be inherited by the concrete implementation.
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt (2)
85-183: Well-structured incoming call notification implementation.The notification handling correctly:
- Extracts caller info with appropriate fallbacks
- Generates unique notification IDs from rid + callerId
- Uses proper intent flags for Android 12+ trampoline restrictions
- Configures full-screen intent for call-style UX
- Handles pre-O sound configuration separately
54-77: LGTM!Notification channel creation properly handles Android O+ requirements with appropriate importance level, visibility, and audio attributes for incoming call notifications.
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (4)
1-6: LGTM!Package declaration and imports are appropriate for the Firebase messaging service implementation.
8-13: LGTM!Documentation clearly explains the service's purpose and responsibilities.
14-18: LGTM!Idiomatic Kotlin implementation with proper FirebaseMessagingService inheritance.
23-27: Notification-only FCM payloads are not processed.The code only handles FCM messages with a data payload (
remoteMessage.data). Messages containing only a notification payload (without data) are ignored at line 24-26. Confirm this is intentional for the Rocket.Chat integration, or add a comment explaining why notification payloads are not processed.
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt
Show resolved
Hide resolved
| // Get icons | ||
| val packageName = context.packageName | ||
| val smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName) | ||
|
|
||
| // Build notification | ||
| val builder = NotificationCompat.Builder(context, CHANNEL_ID).apply { | ||
| setSmallIcon(smallIconResId) |
There was a problem hiding this comment.
Handle missing icon resource gracefully.
getIdentifier returns 0 if the resource is not found. Using 0 as the small icon resource ID can cause the notification to fail silently or display incorrectly on some Android versions.
// Get icons
val packageName = context.packageName
val smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName)
+ val validIconResId = if (smallIconResId != 0) smallIconResId else android.R.drawable.ic_dialog_info
// Build notification
val builder = NotificationCompat.Builder(context, CHANNEL_ID).apply {
- setSmallIcon(smallIconResId)
+ setSmallIcon(validIconResId)📝 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.
| // Get icons | |
| val packageName = context.packageName | |
| val smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName) | |
| // Build notification | |
| val builder = NotificationCompat.Builder(context, CHANNEL_ID).apply { | |
| setSmallIcon(smallIconResId) | |
| // Get icons | |
| val packageName = context.packageName | |
| val smallIconResId = context.resources.getIdentifier("ic_notification", "drawable", packageName) | |
| val validIconResId = if (smallIconResId != 0) smallIconResId else android.R.drawable.ic_dialog_info | |
| // Build notification | |
| val builder = NotificationCompat.Builder(context, CHANNEL_ID).apply { | |
| setSmallIcon(validIconResId) |
🤖 Prompt for AI Agents
In
android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfNotification.kt
around lines 154-160, getIdentifier may return 0 for "ic_notification" which
will break setSmallIcon; check if smallIconResId == 0 and handle it by falling
back to a valid icon resource (for example the app icon from
context.applicationInfo.icon or a safe android.R.drawable.* fallback), then pass
that non-zero id to setSmallIcon; also consider logging a warning when the
custom drawable is missing so it can be fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/sagas/deepLinking.js (2)
226-239: Guard against missingcaller._idbefore notifying video-conference eventsUsing optional chaining avoids a hard crash when
params.calleris absent, but if that happens you’ll now send requests tonotifyUserwith a path like'undefined/video-conference'anduidin the payload asundefined. That’s likely not a meaningful or valid request.Consider bailing out (and logging) when
uidis missing so you don’t emit malformed events:- const uid = params.caller?._id; - const { rid, callId, event } = params; + const uid = params.caller?._id; + if (!uid) { + log('Missing caller uid in OPEN_VIDEO_CONF params', params); + return; + } + const { rid, callId, event } = params;
249-252: Early return on missinghostprevents crashes but may leave UX in a no-op stateThe new
!hostguard avoidshost.slicethrowing, which is good, but it now makeshandleClickCallPusha silent no-op whenhostis missing. InhandleOpen, the analogous case callsfallbackNavigation()so the app at least initializes instead of “doing nothing”.You may want to mirror that behavior here:
- if (!host) { - return; - } + if (!host) { + yield fallbackNavigation(); + return; + }This keeps the app from feeling unresponsive when a malformed/partial push payload arrives.
📜 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 (1)
app/sagas/deepLinking.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/sagas/deepLinking.js (2)
app/sagas/login.js (1)
params(423-423)app/sagas/rooms.js (1)
params(145-145)
|
Android Build Available Rocket.Chat Experimental 4.68.0.107811 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSU5-XHlfn0jLRCr9n9Twy6EZIMaa0dyPAZ-72bYqXKvcqnxC2RQtwSdN_Td4cFqoC6mPP1DqqpWhGj8VRH |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
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
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
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/CustomPushNotification.java (1)
93-161: Race condition onmBundlefield.
mBundleis accessed from multiple threads without consistent synchronization. While line 190 usessynchronized(CustomPushNotification.this), other accesses throughout the class (lines 94, 164, 203, 216, 242, 262, 375-378, etc.) do not. This can lead to visibility issues, stale reads, and potential crashes.Apply this diff to add proper synchronization:
public class CustomPushNotification { // Instance fields private final Context mContext; - private Bundle mBundle; + private volatile Bundle mBundle; private final NotificationManager notificationManager;Additionally, wrap all reads and writes to
mBundlewithsynchronized(this)blocks, or consider usingAtomicReference<Bundle>for thread-safe access. For example:private synchronized Bundle getBundle() { return mBundle; } private synchronized void setBundle(Bundle bundle) { this.mBundle = bundle; }Then replace direct
mBundleaccesses with these getters/setters throughout the class.
♻️ Duplicate comments (3)
android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java (1)
44-51: Add null check for bundle to prevent NPE.This issue has already been flagged in a previous review. If both
getBundleExtra("pushNotification")andgetExtras()return null, line 51 will throw a NullPointerException when accessingbundle.getString("notId").package.json (1)
64-64:⚠️ Duplicate concern: expo-device version remains incompatible with Expo SDK 53.This issue was already flagged in a previous review. Expo SDK 53 includes React Native 0.79 and was released in April 2025. The version
^8.0.10is incompatible with SDK 53 and should be downgraded to the v7.x family.Apply this fix:
- "expo-device": "^8.0.10", + "expo-device": "^7.x.x",After updating, run
yarn installand verify the lock file to ensure no version conflicts.android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (1)
510-520: PotentialStringIndexOutOfBoundsExceptioninextractMessage.If the message contains a colon at position
poswherepos + 2 > message.length(),substring(start)will throw. For example, message"a:"haspos=1,start=3, butlength=2.Apply this diff to fix:
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; }
🧹 Nitpick comments (3)
package.json (1)
211-211: Consider updating Node.js minimum version requirement.Node 18 reached End-Of-Life on April 30, 2025, and Node 20 is recommended for Expo SDK 53 projects. Consider updating the engines field to
"node": ">=20"to align with current best practices for this SDK version.ios/AppDelegate.swift (1)
28-32: Consider using the existing helper method and review log level.The code duplicates logic from
FileManager+Extensions.swiftwhich provides agroupDir()method (lines 3-11). Additionally, using.debuglog level in production may produce excessive logging.Apply this diff to use the existing helper:
- // Initialize MMKV with app group - if let appGroup = Bundle.main.object(forInfoDictionaryKey: "AppGroup") as? String, - let groupDir = FileManager.default.containerURL(forSecurityApplicationGroupIdentifier: appGroup)?.path { - MMKV.initialize(rootDir: nil, groupDir: groupDir, logLevel: .debug) - } + // Initialize MMKV with app group + let groupDir = FileManager.default.groupDir() + if !groupDir.isEmpty { + MMKV.initialize(rootDir: nil, groupDir: groupDir, logLevel: .info) + }This reduces duplication and changes the log level to
.info(adjust as appropriate for your needs).Please verify whether silent failure is intentional if the app group is not configured. If MMKV requires the app group directory for proper operation with notification extensions, consider logging a warning or error when initialization is skipped.
ios/Podfile (1)
37-40: Add trailing comma to satisfy RuboCop style hintThe multiline
$static_frameworkarray is missing a trailing comma on the last element, which RuboCop is flagging.You can align with the style hint by updating as:
$static_framework = [ 'WatermelonDB', - 'simdjson' + 'simdjson', ]As per static_analysis_hints.
📜 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 (11)
android/app/build.gradle(0 hunks)android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt(1 hunks)android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java(16 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/ReplyBroadcast.java(2 hunks)ios/AppDelegate.swift(1 hunks)ios/Podfile(1 hunks)ios/RocketChatRN-Bridging-Header.h(1 hunks)ios/RocketChatRN.xcodeproj/project.pbxproj(42 hunks)package.json(1 hunks)
💤 Files with no reviewable changes (1)
- android/app/build.gradle
🚧 Files skipped from review as they are similar to previous changes (3)
- android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java
- ios/RocketChatRN-Bridging-Header.h
- android/app/src/main/java/chat/rocket/reactnative/notification/LoadNotification.java
🧰 Additional context used
🧬 Code graph analysis (4)
android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java (1)
android/app/src/main/java/chat/rocket/reactnative/notification/NotificationHelper.java (1)
NotificationHelper(10-40)
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)
ios/AppDelegate.swift (1)
ios/Shared/Extensions/FileManager+Extensions.swift (1)
groupDir(4-12)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (1)
android/app/src/main/java/chat/rocket/reactnative/networking/SSLPinningTurboPackage.java (1)
SSLPinningTurboPackage(15-45)
🪛 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). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🔇 Additional comments (16)
android/app/src/main/java/chat/rocket/reactnative/notification/ReplyBroadcast.java (1)
84-84: Good addition of User-Agent header.The User-Agent header standardizes API requests across notification flows and aligns with the pattern established in other notification handlers. Based on learnings,
NotificationHelper.getUserAgent()safely returns a formatted string.package.json (1)
72-72: expo-notifications version is correct.The
expo-notificationsversion (^0.32.14) is compatible with Expo SDK 53 and requires no changes.ios/AppDelegate.swift (1)
34-35: LGTM - correct initialization order.The
ReplyNotification.configure()call is correctly positioned after MMKV initialization and before React Native setup, aligning with the migration to expo-notifications as described in the PR objectives.android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt (4)
25-25: LGTM! Import follows established pattern.The import addition is consistent with other TurboPackage imports in the file and supports the package registration at line 47.
27-38: Helpful documentation of the hybrid notification architecture.The documentation clearly explains the division of responsibilities between the JS layer (expo-notifications) and native layer (FCM + CustomPushNotification). This hybrid approach appears intentional to leverage expo-notifications for token management while retaining native FCM capabilities for advanced features like E2E encryption and direct reply.
39-39: Removal of INotificationsApplication interface is safe.The interface removal is clean—no remaining references to
INotificationsApplicationor calls togetPushNotificationexist in the codebase. This change is consistent with the migration to expo-notifications.
47-47: VideoConfTurboPackage is properly implemented. The class correctly extendsTurboReactPackageand implements the required methodsgetModule()andgetReactModuleInfoProvider(). The registration at line 47 follows the established pattern.android/app/src/main/java/chat/rocket/reactnative/notification/CustomPushNotification.java (9)
65-72: LGTM: Simplified constructor.The constructor refactor removes external dependencies and creates the notification channel immediately, which is good practice for ensuring notifications can be displayed.
109-150: LGTM: Robust React initialization waiting.The polling logic with timeout ensures notifications are processed even if React initialization is delayed, which is good for reliability.
224-280: LGTM: Well-structured E2E handling.The fast-path optimization for immediate decryption and the fallback to async processing is well-designed. The separation of concerns with
E2ENotificationProcessoris good architecture.
323-351: LGTM: Video conference handling.The logic correctly handles incoming calls (status 0) and ended calls (status 4), with proper fallback from
callertosenderfor caller ID extraction.
373-430: LGTM: Proper PendingIntent flags.The code correctly uses
FLAG_IMMUTABLEfor Android S+ (line 394) and handles API level differences appropriately.
432-453: LGTM: Fallback notification cleanup.The logic correctly identifies and cancels previous fallback notifications from the same server, with proper error handling.
455-492: LGTM: Avatar loading with fallback.The avatar loading logic properly handles errors and provides a reasonable fallback to the app icon.
522-591: LGTM: Notification styling.The code properly handles different Android API levels and uses
MessagingStylefor modern Android versions, which is appropriate for a chat application.
593-637: LGTM: Reply and dismiss actions.The code correctly uses
FLAG_MUTABLEfor the reply action (which needs to be updated with user input) andFLAG_IMMUTABLEfor the dismiss action, following Android best practices.
| 407D3EDE3DABEE15D27BD87D /* ShellScript */ = { | ||
| isa = PBXShellScriptBuildPhase; | ||
| buildActionMask = 2147483647; | ||
| files = ( | ||
| ); | ||
| inputPaths = ( | ||
| "${PODS_ROOT}/Target Support Files/Pods-defaults-Rocket.Chat/Pods-defaults-Rocket.Chat-frameworks.sh", | ||
| "${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", | ||
| ); | ||
| name = "[CP] Embed Pods Frameworks"; | ||
| outputPaths = ( | ||
| "${TARGET_BUILD_DIR}/${FRAMEWORKS_FOLDER_PATH}/hermes.framework", | ||
| "$(DERIVED_FILE_DIR)/Pods-defaults-NotificationService-checkManifestLockResult.txt", | ||
| ); | ||
| runOnlyForDeploymentPostprocessing = 0; | ||
| shellPath = /bin/sh; | ||
| shellScript = "\"${PODS_ROOT}/Target Support Files/Pods-defaults-Rocket.Chat/Pods-defaults-Rocket.Chat-frameworks.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:
#!/bin/bash
# Extract the shell script phase and surrounding context
cd ios/RocketChatRN.xcodeproj
sed -n '1545,1575p' project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2481
🏁 Script executed:
#!/bin/bash
# Look for other shell script phases to compare patterns
cd ios/RocketChatRN.xcodeproj
grep -n "PBXShellScriptBuildPhase" project.pbxproj | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 887
🏁 Script executed:
#!/bin/bash
# Get more context around the target configuration - examine the inputPaths script
cd ios/RocketChatRN.xcodeproj
sed -n '1552,1568p' project.pbxproj | cat -ARepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 919
Incorrect output path target in shell script build phase.
The shell script phase at lines 1552-1568 references the RocketChatRN frameworks script (Pods-defaults-RocketChatRN-frameworks.sh) as input, but declares an output path for the NotificationService target (Pods-defaults-NotificationService-checkManifestLockResult.txt). The outputPaths should reference the RocketChatRN target instead:
"$(DERIVED_FILE_DIR)/Pods-defaults-RocketChatRN-checkManifestLockResult.txt"
This configuration mismatch will cause incorrect dependency tracking between build phases and targets.
🤖 Prompt for AI Agents
In ios/RocketChatRN.xcodeproj/project.pbxproj around lines 1552 to 1568 the
ShellScript build phase lists the RocketChatRN frameworks script as an input but
the outputPaths entry points to the NotificationService target
checkManifestLockResult file; replace the incorrect output path with the
RocketChatRN variant so the outputPaths becomes the RocketChatRN
checkManifestLockResult file (i.e. update
"$(DERIVED_FILE_DIR)/Pods-defaults-NotificationService-checkManifestLockResult.txt"
to "$(DERIVED_FILE_DIR)/Pods-defaults-RocketChatRN-checkManifestLockResult.txt")
to align the input and output target for proper dependency tracking.
| "$(SRCROOT)/../node_modules/react-native-mmkv-storage/ios/**", | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if MMKV storage is properly installed
if [ -d "node_modules/react-native-mmkv-storage/ios" ]; then
echo "✓ MMKV storage iOS directory exists"
echo "Directory contents:"
ls -la node_modules/react-native-mmkv-storage/ios/
else
echo "✗ MMKV storage iOS directory not found"
echo "Checking if node_modules exists:"
ls -la node_modules/ | head -20
fiRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 227
🏁 Script executed:
# Check if the dependency is listed in package.json
grep -A 5 "mmkv" package.json || echo "mmkv not found in package.json"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 306
🏁 Script executed:
# Check all the referenced lines in the pbxproj file to ensure they all reference MMKV storage
grep -n "react-native-mmkv-storage" ios/RocketChatRN.xcodeproj/project.pbxprojRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 515
Fix package name mismatch in header search paths.
The iOS build configuration references react-native-mmkv-storage/ios/**, but package.json declares the dependency as react-native-mmkv (3.3.3). The path in the build configuration must match the actual package name, otherwise header search will fail during iOS build. Update all six occurrences at lines 2487, 2553, 2632, 2709, 3064, and 3128 to reference the correct package path.
🤖 Prompt for AI Agents
In ios/RocketChatRN.xcodeproj/project.pbxproj around lines 2487, 2553, 2632,
2709, 3064, and 3128, the header search paths reference the incorrect package
folder "react-native-mmkv-storage/ios/**"; update each occurrence to the correct
package path "react-native-mmkv/ios/**" so the header search paths match
package.json (react-native-mmkv v3.3.3) and the iOS build can locate headers.
| OTHER_LDFLAGS = ( | ||
| "$(inherited)", | ||
| " ", | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unnecessary empty space entry in OTHER_LDFLAGS.
The OTHER_LDFLAGS configuration includes an empty space string " " which serves no purpose and could potentially cause unexpected behavior during the linking phase.
Apply this change to remove the empty entry:
OTHER_LDFLAGS = (
"$(inherited)",
- " ",
);Also applies to: 3293-3296
🤖 Prompt for AI Agents
In ios/RocketChatRN.xcodeproj/project.pbxproj around lines 3226-3229 and
3293-3296, the OTHER_LDFLAGS arrays contain an unnecessary empty string entry ("
"); remove that empty entry from both locations so the arrays only include
"$(inherited)" (keeping commas/format consistent with surrounding entries) to
avoid potential linker issues.
|
Android Build Available Rocket.Chat Experimental 4.68.0.107907 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSTcBau19k3HjGPeJbjQWsuDBrCXF59dw38LeTFPmfXBjrL-U71NYfZGw3qx2c3h2T9Q08prtt5akS4-lfp |
|
iOS Build Available Rocket.Chat Experimental 4.68.0.107909 |
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
Incoming call notification is not being displayed on iOS and Android when the app is in background.
version tested:
iOS - Rocket.Chat Experimental 4.68.0.107909
android - Rocket.Chat Experimental 4.68.0.107907
Proposed changes
Migrate push notification handling from
react-native-notifications,@notifee/react-native, and@react-native-firebase/messagingtoexpo-notifications. This consolidates notification handling into a single, well-maintained library while implementing custom native handling for advanced features like video conference calls.Key Changes
Dependencies:
react-native-notifications,@notifee/react-native,@react-native-firebase/messagingexpo-notifications,expo-deviceAndroid:
RCFirebaseMessagingService.ktfor FCM handlingVideoConfNotification.ktfor video conference call notifications with full-screen intent, accept/decline actions, and ringtoneVideoConfModuleto TurboModules architecture (NativeVideoConfSpec.kt,VideoConfModule.kt,VideoConfTurboPackage.kt)VideoConfBroadcast.ktfor handling notification action intentsiOS:
NotificationService.swiftto handle message-ID-only notifications by fetching full content from serverejsonpayload with server response for correct navigation on E2EE and message-ID-only notificationscontentHandleris always called to prevent notification service extension timeoutsJavaScript:
expo-notificationsAPIs for token registration, listeners, and badge managementpush.tswith new notification configuration and FCM token refresh handlingnotifications/index.tsbackgroundNotificationHandler.ts(now handled natively)Issue(s)
https://rocketchat.atlassian.net/browse/SUP-859
How to test or reproduce
Push Notifications
Video Conference Calls
E2EE Rooms
Direct Reply (Android)
Token Refresh
Screenshots
Screen_Recording_20251222_154458_RocketChat.Experimental.mp4
ScreenRecording_12-22-2025.15-39-52_1.MP4
Types of changes
Checklist
Further comments
Why this migration?
expo-notificationsworks seamlessly with the Expo ecosystem we already useFiles Changed Summary
RCFirebaseMessagingService.kt,VideoConfNotification.kt,VideoConfModule.kt,VideoConfBroadcast.kt,CustomPushNotification.java,MainActivity.kt,MainApplication.ktNotificationService.swift,ReplyNotification.swift,AppDelegate.swift,Payload.swiftpush.ts,notifications/index.ts,getInitialNotification.tspackage.json,Podfile,AndroidManifest.xml,build.gradleBreaking Changes
None. The migration maintains the same user-facing behavior while improving the underlying implementation.
Summary by CodeRabbit
New Features
Improvements
Removed
✏️ Tip: You can customize this high-level summary in your review settings.