feat: Receive voip push notifications from backend#7045
Merged
Conversation
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoves CallIdUUID native modules; migrates callUUID → callId across native and JS; adds VoipPayload parsing, per-call lifetime handling, platform DDP clients, persistent VoIP push-token storage and new native APIs (getLastVoipToken/stopNativeDDPClient); updates push-token registration to include voipToken. Changes
Sequence Diagram(s)sequenceDiagram
participant App as JS App
participant Native as Native Module (iOS/Android)
participant Storage as Persistent Storage
participant Backend as Backend API
participant PushSvc as Push Provider
App->>Native: getLastVoipToken()
Native->>Storage: read persisted voipToken
Storage-->>Native: voipToken (or empty)
Native-->>App: voipToken
PushSvc->>Native: VoIP push credentials updated
Native-->>App: emit VoipPushTokenRegistered(token)
App->>App: registerPushToken() (getDeviceToken + getLastVoipToken)
App->>Backend: POST /push.token {value,type,appName,voipToken}
Backend-->>App: 200 OK
App->>Storage: persist lastToken / lastVoipToken
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
18-18:⚠️ Potential issue | 🔴 CriticalRemove unused import to fix pipeline failure.
CallIdUUIDModuleis imported but never used, causing the pipeline to fail with'CallIdUUIDModule' is defined but never used. (no-unused-vars).🐛 Proposed fix
-import CallIdUUIDModule from '../../native/NativeCallIdUUID';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` at line 18, Remove the unused import CallIdUUIDModule from MediaSessionInstance.ts: locate the import statement "import CallIdUUIDModule from '../../native/NativeCallIdUUID';" and delete it so the unused variable error ('CallIdUUIDModule' is defined but never used) is resolved; ensure no other references to CallIdUUIDModule remain in the file (e.g., within MediaSessionInstance class or its methods) before committing.
🧹 Nitpick comments (4)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
60-76: Minor: Log message formatting whenerrorContextis empty.When
errorContextis an empty string (as called fromgetAvatarUrion line 105), the log message on line 63 produces "Cannot generate avatar URI" with a double space.Consider adjusting the log format to handle the empty context case:
💡 Suggested improvement
private String buildAvatarUri(String avatarPath, String errorContext) { String server = serverURL(); if (server == null || server.isEmpty()) { - Log.w(TAG, "Cannot generate " + errorContext + " avatar URI: serverURL is null"); + String contextPrefix = (errorContext == null || errorContext.isEmpty()) ? "" : errorContext + " "; + Log.w(TAG, "Cannot generate " + contextPrefix + "avatar URI: serverURL is null"); return null; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java` around lines 60 - 76, The log message in buildAvatarUri produces a double space when errorContext is empty; update buildAvatarUri (used by getAvatarUri) to format the message safely by either using a conditional prefix/suffix (e.g., if errorContext is empty log "Cannot generate avatar URI" else "Cannot generate {errorContext} avatar URI") or trimming/adding the space only when errorContext is non-empty so the Log.w(TAG, ...) never contains an extra space.android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
58-69: Remove unusedparseEjsonfunction.This private function is not called anywhere in the codebase and can be safely deleted to reduce dead code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt` around lines 58 - 69, The private function parseEjson in RCFirebaseMessagingService is dead code and should be removed; delete the entire parseEjson(ejsonStr: String?): Ejson? function (including its null/empty check, try/catch, and imports used only by it) so the class no longer contains this unused symbol and to avoid leaving unused gson/logging references.android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
141-147: Redundant null check on non-nullableString.
callIdis declared asString(non-nullable), socallId.isNullOrEmpty()will never be null. UsecallId.isEmpty()instead for clarity. Similarly,caller.isNullOrEmpty()should becaller.isEmpty().♻️ Proposed fix
private fun registerCallWithTelecomManager(callId: String, caller: String) { try { // Validate inputs - if (callId.isNullOrEmpty() || caller.isNullOrEmpty()) { + if (callId.isEmpty() || caller.isEmpty()) { Log.e(TAG, "Cannot register call with TelecomManager: callId is null or empty") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt` around lines 141 - 147, In registerCallWithTelecomManager, remove redundant nullability checks on the non-nullable parameters by replacing callId.isNullOrEmpty() with callId.isEmpty() and caller.isNullOrEmpty() with caller.isEmpty(); update the conditional and the associated Log.e message to reflect empty-only checks so the method logic and intent match the String parameter types (references: registerCallWithTelecomManager, callId, caller).ios/RocketChatRN.xcodeproj/project.pbxproj (1)
1146-1147: Consider colocating these Swift files with the app code they extend.
AppDelegate+Voip.swiftandVoipPayload.swiftread more like app-lifecycle/model code than native bridge code, so keeping them underLibrariesmakes ownership harder to scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/RocketChatRN.xcodeproj/project.pbxproj` around lines 1146 - 1147, The two Swift files AppDelegate+Voip.swift and VoipPayload.swift should be moved out of the Libraries group and colocated with the main app source files: move the physical files into the app source folder, remove their entries from the Libraries group in the Xcode project, and add them to the app's source group so they live alongside other app-lifecycle/model code; while doing this keep their target membership for the main app target intact and update the project.pbxproj file references (file reference and group entries) so imports and build settings continue to resolve.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt`:
- Line 26: Replace the unsafe debug log that prints the entire FCM payload in
RCFirebaseMessagingService (the Log.d call referencing TAG and
remoteMessage.data) with a redacted or minimal-safe log: log only non-sensitive
fields such as remoteMessage.from and payload size or a whitelist of allowed
keys, or mask values before logging; do not include remoteMessage.data contents
verbatim. Update the Log.d usage to reference the sanitized information and
ensure any helper you add for redaction is used consistently where
remoteMessage.data was previously logged.
In `@app/lib/native/NativeVoip.ts`:
- Around line 46-57: The current fallback stubs around TurboModuleRegistry.get
for VoipModule (NativeVoipModule) silently hide native registration failures;
replace the use of TurboModuleRegistry.get with
TurboModuleRegistry.getEnforcing<Spec>('VoipModule') and remove the no-op
fallback object so the module throws if the native bridge isn't registered;
update any references to NativeVoipModule (and its methods like
registerVoipToken, getInitialEvents, clearInitialEvents, getLastVoipToken,
addListener, removeListeners) to assume a present native implementation or
handle the thrown error at a higher level rather than continuing with silent
stubs.
In `@app/lib/services/restApi.ts`:
- Around line 1005-1006: The dedupe cache variables lastToken and lastVoipToken
are not cleared when tokens are unregistered, allowing a subsequent register
call with the same tokens to be incorrectly skipped; update the
unregister/remove path(s) that perform push token removal (e.g.,
removePushToken() and the // TODO: add voip token removal branch) to reset
lastToken = '' and lastVoipToken = '' immediately after a successful backend
removal (and also on any error branch where the local state is cleared) so
future registrations are not deduplicated incorrectly.
- Around line 1048-1055: The current success log prints the full push payload
(data) which leaks push and VoIP tokens; remove sensitive token values from
logs. In the block around sdk.post('push.token', data) replace
console.log('registerPushToken success', data) with a non-sensitive message like
console.log('registerPushToken success') or log a masked result, and ensure you
do not log token, voipToken, data, lastToken or lastVoipToken values anywhere;
keep assignment to lastToken/lastVoipToken but never emit them to logs or error
reports.
- Around line 1024-1046: The guard currently returns early on iOS when voipToken
is missing and can still post an empty Android payload; change the early-return
to only skip when neither token nor voipToken exist (if (isIOS && !token &&
!voipToken) return), build the data object only when at least one token exists
(set type/appName/getUniqueId/getBundleId() only when token is truthy), add
voipToken to data only when present, and ensure you never call
sdk.post('push.token', data) with the empty initializer—only post when
data.value or data.voipToken is set.
- Around line 1015-1058: The registerPushToken function currently wraps an async
executor in new Promise which can leave the outer promise unresolved on errors;
change export const registerPushToken = (): Promise<void> => new Promise(async
resolve => { ... }) to an async function export const registerPushToken = async
(): Promise<void> { ... }, remove the Promise constructor and all resolve()
calls, keep the existing try/catch around sdk.post('push.token', data) to log
errors, and ensure awaits (e.g., await getUniqueId()) and uses like
NativeVoipModule.getLastVoipToken() and getBundleId are preserved inside the new
async function body so the returned promise correctly rejects on uncaught
errors.
In `@ios/Libraries/AppDelegate`+Voip.swift:
- Line 29: The iOS AppDelegate+Voip.swift currently lowercases callId (let
callId = voipPayload.callId.lowercased()), causing inconsistent cross-platform
behavior; remove the lowercasing in AppDelegate+Voip.swift so iOS preserves the
original callId, and instead normalize call IDs in the shared/logic layers and
platform implementations for consistency: update VoipNotification.kt to apply
the same normalization (e.g., toLowerCase()) or, preferably, update
MediaCallEvents.ts (where toLowerCase() matching occurs) and other consumers
like videoConf.ts to perform case-insensitive comparisons (normalize both sides)
so all platforms treat callId consistently across call handling functions and
event matching.
In `@ios/Libraries/VoipPayload.swift`:
- Around line 78-80: The notificationId getter uses callId.hashValue which is
unstable across launches; replace it with a deterministic mapping from callId to
an Int (for example compute a stable hash like CRC32 or SHA256 and truncate to a
signed 32-bit Int, or if callId is a UUID parse its numeric components) and
return that value from the `@objc` var notificationId so notifications can be
consistently referenced across app runs; update the notificationId computed
property (and any code that assumes its type/size) to use the stable
hashing/parsing routine instead of hashValue.
In `@ios/Libraries/VoipService.swift`:
- Around line 92-102: invalidatePushToken() currently only clears local state
(lastVoipToken and storage.removeValue(forKey: voipTokenStorageKey)), leaving
server-side VoIP registrations intact; update this method to also fan out an
unregister call to the backend for every logged-in workspace so the token is
removed server-side. Locate invalidatePushToken() and after clearing local state
invoke your network/unregister API for each active workspace (e.g., iterate your
session/workspace manager’s logged-in workspaces and call the existing
unregister endpoint or a new helper like unregisterVoipToken(workspaceId:)),
handle/report errors but ensure best-effort removal for all workspaces before
returning.
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Line 18: Remove the unused import CallIdUUIDModule from
MediaSessionInstance.ts: locate the import statement "import CallIdUUIDModule
from '../../native/NativeCallIdUUID';" and delete it so the unused variable
error ('CallIdUUIDModule' is defined but never used) is resolved; ensure no
other references to CallIdUUIDModule remain in the file (e.g., within
MediaSessionInstance class or its methods) before committing.
---
Nitpick comments:
In `@android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java`:
- Around line 60-76: The log message in buildAvatarUri produces a double space
when errorContext is empty; update buildAvatarUri (used by getAvatarUri) to
format the message safely by either using a conditional prefix/suffix (e.g., if
errorContext is empty log "Cannot generate avatar URI" else "Cannot generate
{errorContext} avatar URI") or trimming/adding the space only when errorContext
is non-empty so the Log.w(TAG, ...) never contains an extra space.
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt`:
- Around line 58-69: The private function parseEjson in
RCFirebaseMessagingService is dead code and should be removed; delete the entire
parseEjson(ejsonStr: String?): Ejson? function (including its null/empty check,
try/catch, and imports used only by it) so the class no longer contains this
unused symbol and to avoid leaving unused gson/logging references.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 141-147: In registerCallWithTelecomManager, remove redundant
nullability checks on the non-nullable parameters by replacing
callId.isNullOrEmpty() with callId.isEmpty() and caller.isNullOrEmpty() with
caller.isEmpty(); update the conditional and the associated Log.e message to
reflect empty-only checks so the method logic and intent match the String
parameter types (references: registerCallWithTelecomManager, callId, caller).
In `@ios/RocketChatRN.xcodeproj/project.pbxproj`:
- Around line 1146-1147: The two Swift files AppDelegate+Voip.swift and
VoipPayload.swift should be moved out of the Libraries group and colocated with
the main app source files: move the physical files into the app source folder,
remove their entries from the Libraries group in the Xcode project, and add them
to the app's source group so they live alongside other app-lifecycle/model code;
while doing this keep their target membership for the main app target intact and
update the project.pbxproj file references (file reference and group entries) so
imports and build settings continue to resolve.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2c605122-3ba0-4d35-a62d-e0d8c9b0a193
📒 Files selected for processing (40)
android/app/src/main/java/chat/rocket/reactnative/MainApplication.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/Ejson.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.ktandroid/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDModule.ktandroid/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDTurboPackage.ktandroid/app/src/main/java/chat/rocket/reactnative/utils/NativeCallIdUUIDSpec.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktapp/actions/deepLinking.tsapp/containers/MediaCallHeader/MediaCallHeader.stories.tsxapp/containers/MediaCallHeader/MediaCallHeader.test.tsxapp/definitions/Voip.tsapp/definitions/rest/v1/push.tsapp/lib/native/NativeCallIdUUID.tsapp/lib/native/NativeVoip.tsapp/lib/notifications/push.tsapp/lib/services/restApi.test.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/pushTokenAux.tsapp/lib/services/voip/simulateCall.tsapp/lib/services/voip/useCallStore.tsapp/stacks/types.tsapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallerInfo.stories.tsxapp/views/CallView/components/CallerInfo.test.tsxapp/views/CallView/components/Dialpad/Dialpad.stories.tsxapp/views/CallView/components/Dialpad/Dialpad.test.tsxapp/views/CallView/index.test.tsxios/AppDelegate.swiftios/Libraries/AppDelegate+Voip.swiftios/Libraries/CallIdUUID.mios/Libraries/CallIdUUID.swiftios/Libraries/VoipModule.mmios/Libraries/VoipPayload.swiftios/Libraries/VoipService.swiftios/RocketChatRN.xcodeproj/project.pbxproj
💤 Files with no reviewable changes (13)
- ios/Libraries/CallIdUUID.m
- app/containers/MediaCallHeader/MediaCallHeader.test.tsx
- app/containers/MediaCallHeader/MediaCallHeader.stories.tsx
- ios/AppDelegate.swift
- android/app/src/main/java/chat/rocket/reactnative/utils/NativeCallIdUUIDSpec.kt
- app/actions/deepLinking.ts
- app/lib/native/NativeCallIdUUID.ts
- app/lib/services/voip/pushTokenAux.ts
- android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDModule.kt
- android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDTurboPackage.kt
- app/lib/services/voip/simulateCall.ts
- android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt
- ios/Libraries/CallIdUUID.swift
📜 Review details
⏰ 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 context used
🧠 Learnings (1)
📚 Learning: 2026-03-05T06:06:12.277Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:12.277Z
Learning: Do not re-activate or reset the WCSession singleton in iOS Objective-C/Swift bridge modules. Ensure WCSession is activated and its delegate is set in a single, central place (e.g., ios/RocketChat Watch App/Loaders/WatchSession.swift) and avoid duplicating activation or delegate assignment in other iOS bridge files like ios/RCTWatchModule.mm. If WCSession is already activated via the central loader, relying on WCSession.defaultSession is sufficient and maintains a single session lifecycle.
Applied to files:
ios/Libraries/VoipModule.mm
🪛 Biome (2.4.6)
app/lib/services/restApi.ts
[error] 1016-1058: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🪛 GitHub Actions: Format Code with Prettier
app/lib/services/voip/MediaSessionInstance.ts
[error] 18-18: 'CallIdUUIDModule' is defined but never used. (no-unused-vars)
🔇 Additional comments (29)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (2)
105-105: LGTM!The call is correctly updated to match the new
buildAvatarUrisignature.
108-125: LGTM!The method is correctly simplified to use the refactored
buildAvatarUri. The "caller" error context provides meaningful log messages, and input validation remains intact.android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
43-47: LGTM!Good refactor to defer bundle creation until after confirming it's not a VoIP payload. This avoids unnecessary allocation for VoIP notifications and keeps the code cleaner.
app/lib/notifications/push.ts (1)
185-201: The iOS first-run re-registration path is correctly implemented.
VoipPushTokenRegistereddoes trigger backend re-registration. Inapp/lib/services/voip/MediaCallEvents.ts, the listener for this event (line 27) callsregisterPushToken()(line 29), ensuring the backend receives both the APNs token and the VoIP token even when they arrive at different times on a fresh install.app/views/CallView/components/CallerInfo.stories.tsx (1)
18-23: Story fixture is aligned with the renamed call identifier.Using
callIdhere keeps the Storybook state in sync with the updated VoIP store shape.app/views/CallView/components/CallerInfo.test.tsx (1)
14-24: Test setup now matches the store contract.This keeps the fixture consistent with the
callIdmigration and avoids stale test state shape.app/stacks/types.ts (1)
299-299:CallView's route contract is simplified cleanly.Making the screen parameterless is consistent with the move to store-owned call state.
app/views/CallView/index.test.tsx (1)
60-76: Shared test state is aligned with thecallIdrename.Good catch updating the common helper, since it keeps the whole CallView suite consistent.
app/views/CallView/components/Dialpad/Dialpad.stories.tsx (1)
18-34: Dialpad stories now seed the current store shape.Using
callIdhere keeps the story helper consistent with the rest of the CallView fixtures.android/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.kt (1)
77-77: Payload handling is consistent with the new VoIP contract.The
callIdlogging updates and directavatarUrlusage keep this activity aligned with the renamed payload fields.Also applies to: 157-157, 232-246
app/views/CallView/CallView.stories.tsx (1)
22-56: CallView stories are using the current store field name.Updating the shared story helper here keeps all variants aligned with the
callIdmigration.app/lib/services/restApi.test.ts (2)
41-87: LGTM!The test suite covers the key scenarios for
registerPushToken:
- Waiting for both iOS tokens before registration
- Deduplication of successful registrations
- Retry behavior after failures
The mock setup and assertions are well-structured.
24-28: No issue found. The mock correctly definesgetBundleIdas a string value, matching the actual implementation inapp/lib/methods/helpers/deviceInfo.ts, where it's exported as a const assigned to the result ofDeviceInfo.getBundleId()(a string, not a function).> Likely an incorrect or invalid review comment.android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (2)
27-29: LGTM!The
avatarUrlproperty is correctly added as nullable with proper@SerializedNameannotation, and is consistently propagated throughtoBundle()andtoWritableMap().
66-145: LGTM!The Gson-based remote payload parsing is well-structured:
- Private helper classes (
RemoteCaller,RemoteVoipPayload) keep internal details encapsulated- Null-safe parsing with appropriate fallbacks
- Exception handling returns
nullgracefully instead of crashingios/Libraries/AppDelegate+Voip.swift (1)
35-49: LGTM!The PKPushRegistryDelegate implementation correctly:
- Reports incoming calls to CallKeep with appropriate parameters
- Stores initial events for later processing
- Calls the PushKit completion handler after processing
app/lib/services/voip/MediaSessionInstance.ts (3)
64-87: LGTM!The
newCallhandler is correctly updated to usecallIdsemantics:
- Checks for existing
callIdinstead ofcallUUID- Uses
setCall(call)without a separate UUID parameterRNCallKeep.endCallcorrectly usescall.callId
90-106: LGTM!The
answerCallmethod correctly:
- Uses
callIdparameter for comparison withmainCall.callId- Sets the call active via
RNCallKeep.setCurrentCallActive(callId)- Falls back to ending the call if not found
120-135: LGTM!The
endCallmethod correctly:
- Compares
mainCall.callIdwith the parameter- Handles both ringing (reject) and active (hangup) states
- Cleans up RNCallKeep state
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
62-64: LGTM!The
callUUIDtocallIdmigration is consistent across:
- Log messages
- Method parameters
- TelecomManager extras bundle
Also applies to: 119-131, 168-181
app/lib/services/voip/useCallStore.ts (3)
12-12: LGTM!The state and action signatures are correctly updated:
callId: string | nullreplacescallUUIDsetCallIdandsetCallsignatures are simplifiedAlso applies to: 30-31, 45-45
61-69: LGTM!
setCallnow correctly derivescallIdfromcall.callIdrather than requiring a separate parameter. This eliminates potential inconsistencies between the storedcallIdand the actual call's identifier.
142-154: LGTM!Both
toggleSpeakerandendCallcorrectly usecallIdfrom the store state, with appropriate null checks before calling RNCallKeep methods.Also applies to: 175-188
ios/Libraries/VoipPayload.swift (2)
3-65: LGTM!The private helper structs (
RemoteCaller,RemoteVoipPayload) are well-designed with clean null-safe parsing and conversion logic. ThetoVoipPayload()method correctly validates required fields before constructing the payload.
67-135: LGTM!The public
VoipPayloadclass:
- Has correct ObjC interop annotations
- Handles both direct dictionary and nested
ejsonpayload formats- Uses
NSNull()appropriately for nil values in dictionaries for ObjC compatibilityapp/lib/services/voip/MediaCallEvents.ts (3)
27-32: LGTM!The token registration flow is correctly updated to call
registerPushToken()with proper error handling. The callback signature change to destructured{ token }aligns with the native event emission pattern.
36-48: Note:callUUIDfrom RNCallKeep events is expected.The
answerCallandendCallevents from RNCallKeep providecallUUIDas part of their API contract. This is passed directly tomediaSessionInstance.answerCall(callUUID)andendCall(callUUID), which is correct since the native iOS code lowercases thecallIdbefore passing it to CallKeep.
52-71: LGTM!The
callIdmigration is consistently applied:
setCallId(data.callId)for Android eventssetCallId(initialEvents.callId)for initial events- Proper case-insensitive comparison between
initialEvents.callIdand CallKeep'scallUUIDAlso applies to: 104-127
ios/RocketChatRN.xcodeproj/project.pbxproj (1)
2062-2096: Configuration confirmed: VoIP background delivery is properly enabled.The shared app plist already contains
UIBackgroundModeswith'voip'(alongside'audio'and'fetch'). The new VoIP entry points wired into both app targets will correctly deliver VoIP pushes on device.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (2)
android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (1)
26-26:⚠️ Potential issue | 🟡 MinorAvoid logging full FCM payload data.
Logging
remoteMessage.dataexposes potentially sensitive information (user IDs, caller details, message metadata) to device logs. Even atDEBUGlevel, this data can leak in production environments.The past review comment indicated this was addressed, but the current code still logs the full payload. Consider logging only non-sensitive fields.
🛡️ Proposed fix to avoid logging sensitive data
- Log.d(TAG, "FCM message received from: ${remoteMessage.from} data: ${remoteMessage.data}") + Log.d(TAG, "FCM message received from: ${remoteMessage.from}, keys: ${remoteMessage.data.keys}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt` at line 26, The current Log.d call in RCFirebaseMessagingService (inside onMessageReceived) prints the entire remoteMessage.data payload which can expose sensitive info; remove or replace that full payload logging and log only safe, minimal fields (e.g., remoteMessage.from, remoteMessage.messageId or a whitelist of non-sensitive keys) and avoid printing remoteMessage.data or any user/caller IDs; update the Log.d invocation (and any other places in RCFirebaseMessagingService referencing remoteMessage.data) to redact or omit sensitive fields while keeping TAG and contextual text.ios/Libraries/VoipService.swift (1)
95-100:⚠️ Potential issue | 🟠 MajorUnregister invalidated VoIP tokens from each workspace.
This still only clears local state. Any logged-in workspace keeps the old token registered server-side, so backend delivery will continue targeting an invalid endpoint until those registrations are explicitly removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/VoipService.swift` around lines 95 - 100, invalidatePushToken currently only clears local state (lastVoipToken and storage.removeValue for voipTokenStorageKey); update it to also call the server-side unregister endpoint for every logged-in workspace so server registrations are removed. Locate the invalidatePushToken method and, after clearing local state, iterate over the list of active workspaces (use the same workspace/session store used elsewhere), call the workspace-level VoIP token unregister API for each (handle asynchronous calls and failures gracefully), and on success ensure any per-workspace token references are cleared; log errors via the existing logger but do not throw, and ensure storage and lastVoipToken remain cleared even if some unregister calls fail.
🧹 Nitpick comments (3)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (3)
169-175:removeCallbacksAndMessages(null)may remove unrelated callbacks.Using
removeCallbacksAndMessages(null)onmainHandlerremoves all pending messages and callbacks from that handler. If other components post to the same main looper handler, this could inadvertently cancel their callbacks.Consider using a dedicated
Runnablereference for the connect timeout (similar toddpDisconnectRunnableinVoipNotification.kt) that can be cancelled specifically.♻️ Proposed fix using specific timeout cancellation
+ private var connectTimeoutRunnable: Runnable? = null + private fun waitForConnected(timeoutMs: Long, callback: (Boolean) -> Unit) { connectedCallback = callback - mainHandler.postDelayed({ + val runnable = Runnable { val cb = connectedCallback ?: return@postDelayed connectedCallback = null Log.e(TAG, "Connect timeout") cb(false) - }, timeoutMs) + } + connectTimeoutRunnable = runnable + mainHandler.postDelayed(runnable, timeoutMs) } // In handleMessage "connected" case: "connected" -> { isConnected = true - mainHandler.removeCallbacksAndMessages(null) + connectTimeoutRunnable?.let { mainHandler.removeCallbacks(it) } + connectTimeoutRunnable = null val cb = connectedCallback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt` around lines 169 - 175, The current use of mainHandler.removeCallbacksAndMessages(null) in the "connected" branch will clear all pending work on the main handler and may cancel unrelated callbacks; instead create and use a dedicated Runnable property (e.g., ddpConnectTimeoutRunnable) for the connection timeout similar to ddpDisconnectRunnable in VoipNotification.kt, post that Runnable when starting the connect timer, and here call mainHandler.removeCallbacks(ddpConnectTimeoutRunnable) (and null out the ddpConnectTimeoutRunnable) before executing the connectedCallback and setting isConnected; update any connect-timeout setup code that previously posted to mainHandler to use this new Runnable so only the intended timeout is removed.
77-103: Add timeout handling forlogin()andsubscribe()operations.Unlike
connect()which has a 10-second timeout,login()andsubscribe()can hang indefinitely if the server never responds. WhileVoipNotification.kthas a safety timeout (INCOMING_CALL_LIFETIME_MS) that guarantees eventual cleanup, adding explicit timeouts would make the client more robust and provide better debugging feedback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt` around lines 77 - 103, login() (and similarly subscribe()) can hang indefinitely if the server never responds; add explicit timeout handling by starting a delayed timeout task immediately after registering pendingCallbacks[msgId] that will, after the chosen timeout (e.g. 10s), remove the pending callback from pendingCallbacks, log an error including the msgId, and invoke the original callback with failure on the mainHandler; ensure the timeout is cancelled when the actual response arrives (inside the pending callback where you already remove pendingCallbacks) so you don't double-call the callback. Use the same pattern in subscribe(): create a msgId, register the pendingCallbacks entry, start a postDelayed timeout on mainHandler that removes the map entry and posts callback(false) if triggered, and cancel that timeout when the response handler runs.
161-166: Consider logging malformed JSON messages for debugging.The swallowed exception at line 164 silently discards malformed JSON messages. While this won't affect functionality, logging these cases would help diagnose protocol issues during development.
🔧 Proposed fix to log parse failures
private fun handleMessage(text: String) { val json = try { JSONObject(text) } catch (e: Exception) { + Log.w(TAG, "Failed to parse DDP message: ${e.message}") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt` around lines 161 - 166, In handleMessage, don't silently swallow JSON parse failures; catch the Exception (e) thrown by JSONObject(text) and log a clear error including the exception and the raw text payload to aid debugging (e.g., use the project's logging utility or Android Log) before returning; update the try/catch around JSONObject(text) to log the parse failure with both e and text so malformed messages are visible in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 110-119: handleTimeout() and handleDeclineAction() tear down local
call state but never stop the shared ddpClient, allowing later server hangups to
trigger duplicate cleanup; update both functions (handleTimeout and
handleDeclineAction) to stop the DDP listener for this call by calling the
ddpClient teardown/unsubscribe/disconnect routine (use the existing ddpClient
symbol to remove any listeners/subscriptions for payload.callId or close the
client if appropriate) before sending ACTION_TIMEOUT/ACTION_DISMISS and
cancelling notifications so the DDP subscriber cannot emit another dismiss
later.
- Around line 127-131: handleDeclineAction currently only performs local cleanup
(cancelTimeout, rejectIncomingCall, cancelById) but never notifies the
backend/caller; add a call to the signaling/API client to send an explicit
decline for payload.callId (e.g., invoke the existing signaling client method
like sendCallDecline/sendDeclineSignal or hit the REST endpoint that notifies
the server) from inside handleDeclineAction, make it async-safe (fire-and-forget
or await with error handling and logging), and include identifying info from
VoipPayload (callId, caller/user id) so the server can stop ringing the remote
party.
- Around line 208-214: The code currently calls disconnectIncomingCall(callId,
true) which marks the call as missed; instead, when handling a remote dismissal
(DDP hangup that results in ACTION_DISMISS), call disconnectIncomingCall(callId,
false) (or use the disconnect variant that does not mark the call as missed) so
the Telecom stack and call log are not updated as a missed call; update the
branch around cancelTimeout(callId), disconnectIncomingCall(...),
cancelById(appContext, payload.notificationId), and the ACTION_DISMISS broadcast
accordingly.
- Around line 200-205: The current check uses
firstArg.optString("signedContractId") which returns "" when the field is
missing, so the condition signedContractId != null is always true and can
wrongly trigger hangup; update the condition in VoipNotification.kt (around
variables signalType, signalCallId, signedContractId, deviceId and firstArg) to
first verify the field exists and is not null using
firstArg.has("signedContractId") && !firstArg.isNull("signedContractId") (then
read the string) before comparing it to deviceId and callId so missing
signedContractId does not cause a hangup.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt`:
- Around line 92-101: The payloads that omit createdAt should not be treated as
immediately expired; update the expiration logic so a null remaining lifetime
means "unknown/not expired" rather than expired. Concretely, change isExpired()
in VoipPayload to return false when getRemainingLifetimeMs() returns null
(instead of true), and apply the same change to the other duplicate isExpired
implementations referenced (the blocks around lines 152-166 and 175-190); keep
getRemainingLifetimeMs() behavior the same and only invert the null-handling in
isExpired so toVoipPayload()/fromBundle() payloads without createdAt are
preserved for VoipModule.getInitialEvents() and
IncomingCallActivity.scheduleTimeout().
- Around line 206-219: In parseCreatedAtMs, the call to formatter.parse(value)
can throw ParseException which currently aborts the isoDateFormats loop; wrap
the formatter.parse(value) call inside a try/catch (catching
java.text.ParseException or Exception) within the synchronized(formatter) block
in parseCreatedAtMs so that on parse failure you continue to the next formatter
instead of exiting the method, and only return null after all formatters have
been tried.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 92-108: In answerCall (MediaSessionInstance.answerCall) the
comparison against mainCall.callId is currently case-sensitive and the comment
"Compare using deterministic UUID conversion" is misleading; change the
comparison to use a case-insensitive normalized check (e.g., compare
mainCall.callId.toLowerCase() === callId.toLowerCase() with null/undefined
guards) so it matches the normalization used in getInitialMediaCallEvents, and
update the comment to accurately state that the code performs case-insensitive
UUID comparison/normalization rather than a deterministic conversion; ensure
this same normalization approach is used in the RNCallKeep event listener call
sites if they pass callUUID directly to answerCall/endCall.
In `@ios/Libraries/DDPClient.swift`:
- Around line 96-101: The subscription callback stored in pendingCallbacks (the
closure created around msgId that currently always calls completion(true)) must
treat "nosub" messages as failures instead of success; update handleMessage()
(the branch that dispatches "ready" and "nosub" to the same callback) so that
when a nosub payload is received you invoke the pending callback with a failure
path (call completion(false) or pass an error/false result) and still remove
pendingCallbacks[msgId]; ensure "ready" continues to call completion(true). Also
include any available nosub reason in the failure logging/arguments so native
hangup tracking won't treat rejected subscriptions as successful.
- Around line 9-13: Introduce a private serial DispatchQueue (e.g. stateQueue)
and use it to serialize all reads and writes to shared DDP client state:
webSocketTask, isConnected, pendingCallbacks, connectedCallback and sendCounter;
update all places that mutate or read these (notably the receive method,
disconnect, the connect-timeout/error handling path, and any code around
sendCounter incrementing) to perform their state touches inside stateQueue.sync
or async blocks so disconnect/reconnect races cannot corrupt the callback map or
crash the client.
In `@ios/Libraries/VoipService.swift`:
- Around line 267-270: The async closures that call stopDDPClientInternal() must
not unconditionally shut down the global ddpClient because a newer ddpClient can
replace it; capture the current ddpClient (or a unique client token) inside
startListeningForCallEnd and, inside the DispatchQueue.main.async closures (the
ones that currently call RNCallKeep.endCall(...),
cancelIncomingCallTimeout(for:), stopDDPClientInternal()), check that the
captured client/token still matches the live ddpClient before calling
stopDDPClientInternal(); only invoke stopDDPClientInternal() when the captured
and current ddpClient are identical (or the token matches) to avoid older
callbacks disconnecting a new listener.
- Around line 172-173: persistVoipToken currently ignores MMKVBridge.setString
failures so token loss on cold start is silent; update persistVoipToken to check
the boolean return of storage.setString(token, forKey: voipTokenStorageKey) and
handle failure by logging via an appropriate logger (or reporting error) and/or
propagating the failure so callers (e.g., getLastVoipToken) can react instead of
silently returning "". Specifically, in persistVoipToken check the result of
storage.setString, call a clear/logging/reporting method with context including
voipTokenStorageKey and token (or a masked token), and consider returning a Bool
or throwing so callers know persistence failed. Ensure you reference
persistVoipToken, storage.setString, voipTokenStorageKey and update call sites
accordingly.
- Around line 246-247: The onCollectionMessage handler currently prints raw DDP
payloads via the print call in client.onCollectionMessage (using TAG), which
exposes sensitive call metadata in release builds; change this to only log in
debug builds (e.g., wrap in a DEBUG conditional or use a debug-only logger)
and/or redact sensitive fields before logging (only log non-identifying keys or
a redacted summary). Locate the client.onCollectionMessage closure and replace
the unconditional print("[\(TAG)] DDP received collection message: \(message)")
with a debug-only or redacting logging path that preserves TAG but avoids
emitting full raw payloads in production.
---
Duplicate comments:
In
`@android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt`:
- Line 26: The current Log.d call in RCFirebaseMessagingService (inside
onMessageReceived) prints the entire remoteMessage.data payload which can expose
sensitive info; remove or replace that full payload logging and log only safe,
minimal fields (e.g., remoteMessage.from, remoteMessage.messageId or a whitelist
of non-sensitive keys) and avoid printing remoteMessage.data or any user/caller
IDs; update the Log.d invocation (and any other places in
RCFirebaseMessagingService referencing remoteMessage.data) to redact or omit
sensitive fields while keeping TAG and contextual text.
In `@ios/Libraries/VoipService.swift`:
- Around line 95-100: invalidatePushToken currently only clears local state
(lastVoipToken and storage.removeValue for voipTokenStorageKey); update it to
also call the server-side unregister endpoint for every logged-in workspace so
server registrations are removed. Locate the invalidatePushToken method and,
after clearing local state, iterate over the list of active workspaces (use the
same workspace/session store used elsewhere), call the workspace-level VoIP
token unregister API for each (handle asynchronous calls and failures
gracefully), and on success ensure any per-workspace token references are
cleared; log errors via the existing logger but do not throw, and ensure storage
and lastVoipToken remain cleared even if some unregister calls fail.
---
Nitpick comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 169-175: The current use of
mainHandler.removeCallbacksAndMessages(null) in the "connected" branch will
clear all pending work on the main handler and may cancel unrelated callbacks;
instead create and use a dedicated Runnable property (e.g.,
ddpConnectTimeoutRunnable) for the connection timeout similar to
ddpDisconnectRunnable in VoipNotification.kt, post that Runnable when starting
the connect timer, and here call
mainHandler.removeCallbacks(ddpConnectTimeoutRunnable) (and null out the
ddpConnectTimeoutRunnable) before executing the connectedCallback and setting
isConnected; update any connect-timeout setup code that previously posted to
mainHandler to use this new Runnable so only the intended timeout is removed.
- Around line 77-103: login() (and similarly subscribe()) can hang indefinitely
if the server never responds; add explicit timeout handling by starting a
delayed timeout task immediately after registering pendingCallbacks[msgId] that
will, after the chosen timeout (e.g. 10s), remove the pending callback from
pendingCallbacks, log an error including the msgId, and invoke the original
callback with failure on the mainHandler; ensure the timeout is cancelled when
the actual response arrives (inside the pending callback where you already
remove pendingCallbacks) so you don't double-call the callback. Use the same
pattern in subscribe(): create a msgId, register the pendingCallbacks entry,
start a postDelayed timeout on mainHandler that removes the map entry and posts
callback(false) if triggered, and cancel that timeout when the response handler
runs.
- Around line 161-166: In handleMessage, don't silently swallow JSON parse
failures; catch the Exception (e) thrown by JSONObject(text) and log a clear
error including the exception and the raw text payload to aid debugging (e.g.,
use the project's logging utility or Android Log) before returning; update the
try/catch around JSONObject(text) to log the parse failure with both e and text
so malformed messages are visible in logs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ce096ae-ca9d-473b-bb80-fc984ff77b89
⛔ Files ignored due to path filters (1)
ios/Podfile.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.javaandroid/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.ktandroid/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktapp/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.tsios/Libraries/AppDelegate+Voip.swiftios/Libraries/DDPClient.swiftios/Libraries/VoipModule.mmios/Libraries/VoipPayload.swiftios/Libraries/VoipService.swiftios/RocketChatRN-Bridging-Header.hios/RocketChatRN.xcodeproj/project.pbxproj
🚧 Files skipped from review as they are similar to previous changes (1)
- android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt
📜 Review details
⏰ 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 context used
🧠 Learnings (2)
📚 Learning: 2026-03-05T06:06:12.277Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:12.277Z
Learning: Do not re-activate or reset the WCSession singleton in iOS Objective-C/Swift bridge modules. Ensure WCSession is activated and its delegate is set in a single, central place (e.g., ios/RocketChat Watch App/Loaders/WatchSession.swift) and avoid duplicating activation or delegate assignment in other iOS bridge files like ios/RCTWatchModule.mm. If WCSession is already activated via the central loader, relying on WCSession.defaultSession is sufficient and maintains a single session lifecycle.
Applied to files:
ios/Libraries/VoipModule.mm
📚 Learning: 2026-03-05T06:06:19.755Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.
Applied to files:
ios/RocketChatRN-Bridging-Header.hios/RocketChatRN.xcodeproj/project.pbxprojios/Libraries/DDPClient.swift
🪛 detekt (1.23.8)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt
[warning] 164-164: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🔇 Additional comments (13)
ios/RocketChatRN-Bridging-Header.h (1)
10-10: LGTM!The addition of
DeviceUID.hto the bridging header correctly enables Swift code to access device-unique identifiers needed for VoIP token registration.app/definitions/Voip.ts (1)
7-17: LGTM!The
VoipPayloadinterface updates align well with the native implementations on both iOS (VoipPayload.swift) and Android (VoipPayload.kt). The addition ofusername,hostName,notificationId, and the optionalavatarUrl/createdAtfields provides the necessary type safety for the VoIP push notification flow.app/lib/native/NativeVoip.ts (1)
53-65: Avoid silently stubbing a critical native module.Using
TurboModuleRegistry.get()with a no-op fallback masks integration failures. IfVoipModulefails to register due to a packaging or linking error, the app will boot successfully but:
getLastVoipToken()returns''silently —registerPushToken()inrestApi.tswon't detect this and may proceed without a valid VoIP tokenstopNativeDDPClient()becomes a no-op — native DDP client monitoring won't be properly stopped, potentially causing resource leaksNativeEventEmitterwon't function — VoIP push token registration events and CallKeep events won't be receivedFor this critical bridge, failing fast during initialization is safer than silently degrading functionality. Consider using
getEnforcing()to surface native registration failures immediately.app/lib/services/voip/MediaSessionInstance.ts (1)
38-39: LGTM on DDP client cleanup during initialization.Stopping the native DDP client at the start of
init()ensures any lingering WebSocket connections from previous sessions are cleaned up before reinitializing the media session. This prevents potential resource leaks and conflicts.android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
1-75: LGTM on overall DDP client implementation.The minimal DDP client correctly implements the subset needed for call hangup detection:
- Proper WebSocket lifecycle management with OkHttp
- Correct DDP protocol handshake (connect, ping/pong)
- Thread-safe callback handling with synchronized blocks
- Clean disconnection with resource cleanup
The integration with
VoipNotification.kt(context snippets 1-3) shows proper usage patterns with the safety timeout providing guaranteed cleanup.ios/Libraries/VoipPayload.swift (3)
111-113:hashValueis not stable across app launches.Swift's
hashValuecan vary between different runs of the program, which could cause issues with notification management (e.g., canceling notifications by ID). Android'sString.hashCode()is deterministic.Consider using a stable hashing algorithm like djb2, or extract numeric components from the UUID string for a consistent ID.
42-70: LGTM on remote payload parsing with validation.The
toVoipPayload()method correctly:
- Validates
notificationType == "voip"before processing- Requires all essential fields (
callId,caller,host,type,hostName)- Validates UUID format via
UUID(uuidString:)- Falls back to
usernameifcaller?.usernameis nil (line 51)
148-162: LGTM on lifetime/expiration logic.The
remainingLifetime()andisExpired()methods correctly handle the edge case wherecreatedAtis nil (returnsnilfor lifetime,truefor expired), which safely treats calls without timestamps as expired.android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java (1)
28-28: LGTM!Making
hostpublic enables cross-package access needed byIncomingCallActivity.ktfor caller avatar loading viaEjson.forCallerAvatar(payload.host, payload.username).android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt (2)
34-39: LGTM on VoIP payload routing.The VoIP payload handling correctly delegates to
VoipNotification.onMessageReceived(), which handles all responsibilities (notification display, TelecomManager registration, timeout scheduling, call-end detection) as shown in the context snippets.
43-52: LGTM on Bundle construction and error handling.Moving Bundle creation into the try block and adding error logging improves the robustness of regular notification processing.
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (2)
345-352: Nice stale-push guard.The lifetime checks keep expired VoIP pushes from surfacing in either Telecom or the notification drawer.
Also applies to: 431-435
362-364: Please verify that answering the call cancels this timeout.Every incoming call is scheduled for forced timeout here, but this file never clears it on accept. If the answer path does not call
VoipNotification.cancelTimeout(callId), an answered call will still be torn down at the original ring expiry.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/lib/services/restApi.ts (1)
1015-1052: Serialize in-flight token registration.The cache check on Line 1020 is only effective after
sdk.post()finishes, so the concurrent fire-and-forget callers inapp/lib/notifications/push.ts:185-202,app/lib/services/voip/MediaCallEvents.ts:27-33, andapp/sagas/login.js:189-195can still submit the same payload multiple times. A shared in-flight promise/lock would make the dedupe deterministic.♻️ Possible shape
let lastToken = ''; let lastVoipToken = ''; +let registerPushTokenInFlight: Promise<void> | null = null; + export const registerPushToken = async (): Promise<void> => { - const token = getDeviceToken(); - // Always returns an empty string on Android - const voipToken = NativeVoipModule.getLastVoipToken(); - - if (token === lastToken && voipToken === lastVoipToken) { - return; - } - - // TODO: server version - if (isIOS && (!token || !voipToken)) { - return; - } - - let data: TRegisterPushTokenData = { - id: '', - value: '', - type: '', - appName: '' - }; - if (token) { - const type = isIOS ? 'apn' : 'gcm'; - data = { - id: await getUniqueId(), - value: token, - type, - appName: getBundleId - }; - } - if (voipToken) { - data.voipToken = voipToken; - } - - try { - // RC 0.60.0 - await sdk.post('push.token', data); - lastToken = token; - lastVoipToken = voipToken; - } catch (e) { - log(e); - } + if (registerPushTokenInFlight) { + return registerPushTokenInFlight; + } + + registerPushTokenInFlight = (async () => { + const token = getDeviceToken(); + // Always returns an empty string on Android + const voipToken = NativeVoipModule.getLastVoipToken(); + + if (token === lastToken && voipToken === lastVoipToken) { + return; + } + + // TODO: server version + if (isIOS && (!token || !voipToken)) { + return; + } + + let data: TRegisterPushTokenData = { + id: '', + value: '', + type: '', + appName: '' + }; + if (token) { + const type = isIOS ? 'apn' : 'gcm'; + data = { + id: await getUniqueId(), + value: token, + type, + appName: getBundleId + }; + } + if (voipToken) { + data.voipToken = voipToken; + } + + try { + // RC 0.60.0 + await sdk.post('push.token', data); + lastToken = token; + lastVoipToken = voipToken; + } catch (e) { + log(e); + } + })().finally(() => { + registerPushTokenInFlight = null; + }); + + return registerPushTokenInFlight; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 1015 - 1052, The registerPushToken function currently only updates lastToken/lastVoipToken after sdk.post, allowing concurrent callers to all post the same payload; add a module-level in-flight lock (e.g., registerPushTokenInFlight: Promise<void> | null) and use it to serialize work: compute token and voipToken (via getDeviceToken and NativeVoipModule.getLastVoipToken), if they match lastToken/lastVoipToken return; if registerPushTokenInFlight exists await it and then re-check the cache; if none, create and assign a Promise that performs the payload build and await sdk.post('push.token', data), update lastToken/lastVoipToken on success, and in a finally block clear registerPushTokenInFlight so concurrent callers either await or re-check the cache and avoid duplicate posts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/services/restApi.ts`:
- Around line 1058-1065: The removePushToken function currently only sends the
regular token to the backend delete endpoint, leaving VoIP subscriptions
registered; fix removePushToken by retrieving the VoIP token (via
NativeVoipModule.getLastVoipToken() or the existing lastVoipToken) and include
it in the payload passed to sdk.current.del('push.token', { token, voipToken }),
while still clearing lastToken and lastVoipToken as done now; update the call
site inside removePushToken so both token and voipToken are sent to the DELETE
endpoint.
- Around line 1024-1027: The early return for iOS in registerPushToken is not
gated by server version; update registerPushToken to first obtain the server
version (reuse the existing compareServerVersion helper used elsewhere in this
file) and only apply the two-token VoIP-aware flow (the isIOS && (!token ||
!voipToken) branch) when compareServerVersion indicates the backend is at or
above the version that supports the voipToken contract; for older servers skip
that branch and fall back to the single-token registration path so legacy
servers don't lose push registration.
---
Nitpick comments:
In `@app/lib/services/restApi.ts`:
- Around line 1015-1052: The registerPushToken function currently only updates
lastToken/lastVoipToken after sdk.post, allowing concurrent callers to all post
the same payload; add a module-level in-flight lock (e.g.,
registerPushTokenInFlight: Promise<void> | null) and use it to serialize work:
compute token and voipToken (via getDeviceToken and
NativeVoipModule.getLastVoipToken), if they match lastToken/lastVoipToken
return; if registerPushTokenInFlight exists await it and then re-check the
cache; if none, create and assign a Promise that performs the payload build and
await sdk.post('push.token', data), update lastToken/lastVoipToken on success,
and in a finally block clear registerPushTokenInFlight so concurrent callers
either await or re-check the cache and avoid duplicate posts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1d4e0171-b9e3-4800-9167-60d43c834bb5
📒 Files selected for processing (2)
app/lib/services/restApi.tsios/Libraries/VoipPayload.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Libraries/VoipPayload.swift
📜 Review details
⏰ 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 (1)
app/lib/services/restApi.ts (1)
1-32: Imports look consistent with the new VoIP registration path.
getUniqueId,NativeVoipModule, andlogare all used by the added token-registration flow.
…ing payload validation, and adjusting call handling conditions. Unify function calls and ensure proper state management across platforms.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
ios/Libraries/DDPClient.swift (1)
62-66:⚠️ Potential issue | 🔴 CriticalRegister connect callback before sending to avoid false timeouts.
connectedCallbackis assigned after send success (Line 65). If"connected"is handled first (Line 321), the success signal is dropped and timeout fires later (Line 281).💡 Suggested fix
- self.send(connectMsg) { [weak self] success in - guard let self else { return } - if success { - self.waitForConnected(timeout: 10.0, completion: completion) - } else { - completion(false) - } - } + self.waitForConnected(timeout: 10.0, completion: completion) + self.send(connectMsg) { [weak self] success in + guard let self else { return } + if !success { + self.stateQueue.async { + let cb = self.connectedCallback + self.connectedCallback = nil + cb?(false) + } + } + }private func waitForConnected(timeout: TimeInterval, completion: `@escaping` (Bool) -> Void) { + if isConnected { + completion(true) + return + } connectedCallback = completion DispatchQueue.main.asyncAfter(deadline: .now() + timeout) { [weak self] inAlso applies to: 274-276, 321-326
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/DDPClient.swift` around lines 62 - 66, The connect callback (connectedCallback) must be registered before sending the connect message to avoid missing an early "connected" event; move the assignment/registration of connectedCallback so it occurs prior to calling self.send(connectMsg) and before invoking waitForConnected in the send completion handler, and apply the same change to the other occurrences around the methods referenced (the send/connect flow and the "connected" handling code paths at the other locations). Ensure the callback is stored on the DDPClient instance (or weakly referenced as currently used) before send returns so any incoming "connected" message is delivered to the registered handler rather than being dropped and causing a false timeout.ios/Libraries/VoipService.swift (2)
185-187:⚠️ Potential issue | 🟠 MajorHandle MMKV persistence failures instead of ignoring them.
Line 186 ignores the return value from
setString. A failed write silently drops the token after cold start.Proposed fix
private static func persistVoipToken(_ token: String) { - storage.setString(token, forKey: voipTokenStorageKey) + let didPersist = storage.setString(token, forKey: voipTokenStorageKey) + `#if` DEBUG + if !didPersist { + print("[\(TAG)] Failed to persist VoIP token for key \(voipTokenStorageKey)") + } + `#endif` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/VoipService.swift` around lines 185 - 187, persistVoipToken currently ignores the Boolean return from storage.setString(voipTokenStorageKey), so MMKV write failures are dropped silently; update persistVoipToken to check the return value of storage.setString(token, forKey: voipTokenStorageKey) and handle a false result by logging an error (include voipTokenStorageKey and token context as appropriate), retrying the write once (or enqueueing the token for a later retry), and/or falling back to an alternative persistent store so the token is not lost; ensure you reference persistVoipToken, storage.setString and voipTokenStorageKey when implementing the fix.
108-114:⚠️ Potential issue | 🟠 MajorUnregister invalidated VoIP tokens from backend workspaces as part of invalidation.
Line 109 already calls this out, but Line 112-113 still only clears local state. This leaves stale server-side registrations active for logged-in workspaces.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/VoipService.swift` around lines 108 - 114, invalidatePushToken currently only clears local state (lastVoipToken and storage.removeValue(forKey: voipTokenStorageKey)) but doesn't inform the backend; before wiping local token, iterate over all logged-in workspaces and call the backend unregister endpoint (e.g., via your workspace API client or WorkspaceManager) to remove the VoIP registration for that token, handling errors/retries as appropriate, then clear lastVoipToken and storage.removeValue(forKey: voipTokenStorageKey); ensure you read the token value first (from lastVoipToken or storage) so you can pass it to the unregister calls inside invalidatePushToken.
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
228-234:removeCallbacksAndMessages(null)removes all pending callbacks.Using
removeCallbacksAndMessages(null)cancels all pending messages on the handler, not just the connect timeout. While this may work currently, it could cause issues if other callbacks are posted tomainHandlerduring connection.Consider storing the timeout
Runnableand removing only that specific callback:Suggested improvement
+ private var connectTimeoutRunnable: Runnable? = null + private fun waitForConnected(timeoutMs: Long, callback: (Boolean) -> Unit) { connectedCallback = callback - mainHandler.postDelayed({ + connectTimeoutRunnable = Runnable { val cb = connectedCallback ?: return@postDelayed connectedCallback = null Log.e(TAG, "Connect timeout") cb(false) - }, timeoutMs) + } + mainHandler.postDelayed(connectTimeoutRunnable!!, timeoutMs) } // In handleMessage "connected" case: - mainHandler.removeCallbacksAndMessages(null) + connectTimeoutRunnable?.let { mainHandler.removeCallbacks(it) } + connectTimeoutRunnable = null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt` around lines 228 - 234, The handler call mainHandler.removeCallbacksAndMessages(null) clears all pending messages; instead capture the specific timeout Runnable when you post it (e.g., store it in a field like connectTimeoutRunnable) and remove only that callback by calling mainHandler.removeCallbacks(connectTimeoutRunnable) in the "connected" branch; update the code paths that post the timeout to use that stored Runnable and ensure connectedCallback is still invoked and nulled as before (symbols: mainHandler, removeCallbacksAndMessages(null), connectedCallback, isConnected).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 68-71: In onFailure override, mark the client as disconnected and
clean up the failed socket before invoking the callback: set the isConnected
flag to false (and null out or close the stored WebSocket reference if you keep
one) inside onFailure (preferably before posting the callback on mainHandler) so
subsequent calls to send() won't use the failed socket; retain the Log.e and
then post callback(false) as before to notify the caller.
- Around line 259-263: The nosub handler currently invokes the stored callback
with the full JSON (in the "nosub" branch inside the when block) but subscribe()
expects the callback to be called with a boolean success (it registers callbacks
that call callback(true)); update the "nosub" branch to retrieve the pending
callback from pendingCallbacks by id (same access as now) and invoke it to
indicate failure (e.g., call the callback with false or pass an error flag)
instead of invoking it with the raw JSON; alternatively, adjust subscribe()’s
registration to accept and interpret the JSON and treat nosub as failure—ensure
the symbols involved are pendingCallbacks, the "nosub" handler in DDPClient.kt,
and subscribe() so the callback contract is consistent.
In `@ios/Libraries/DDPClient.swift`:
- Around line 336-339: In the "ready" case inside DDPClient.swift, the code only
invokes the callback for subs.first, so other subscription IDs in json["subs"]
are ignored; update the logic to iterate over the subs array (json["subs"] as?
[String]) and for each subscription id lookup pendingCallbacks[id] and invoke
the callback (and clear/remove the entry if that’s intended) so all acknowledged
subscriptions complete; adjust references to pendingCallbacks and the callback
variable (cb) inside that loop.
- Around line 179-187: The send(_ dict: [String: Any], completion: `@escaping`
(Bool) -> Void) method can silently drop the completion when webSocketTask is
nil; change it so you explicitly handle the nil case and invoke
completion(false) immediately when webSocketTask is nil, and continue to call
completion(false/true) inside the webSocketTask?.send closure on error/success;
this ensures callers in connect(), login(), subscribe(), and callMethod() never
hang waiting for a callback from a missing webSocketTask.
In `@ios/Libraries/VoipPayload.swift`:
- Around line 78-80: The comment above INCOMING_CALL_LIFETIME_SEC incorrectly
says "milliseconds" even though the constant name and type represent seconds;
update the comment to state "seconds" (or otherwise match the unit used by
INCOMING_CALL_LIFETIME_SEC) so the comment accurately reflects the unit for the
TimeInterval constant INCOMING_CALL_LIFETIME_SEC.
- Around line 47-50: The guard in VoipPayload parsing currently hard-fails when
UUID(uuidString: payloadCallId) returns nil; change this so a non-UUID callId
doesn't drop the whole payload: instead of requiring payloadCallUUID from
UUID(uuidString:), generate a deterministic UUID from payloadCallId (reuse the
same hashing/deterministic approach used in stableNotificationId) and assign it
to payloadCallUUID, or make the stored callUUID optional and proceed without
failing the initializer; update the guard/initializer that references
payloadCallUUID (and any code expecting callUUID) so parsing continues when
callId is not RFC4122-compliant.
In `@ios/Libraries/VoipService.swift`:
- Around line 191-215: Multiple shared VoIP service state variables
(incomingCallTimeouts, ddpClient, observedIncomingCall, isDdpLoggedIn) are
mutated from different async paths causing races; serialize access by
introducing a single synchronization primitive (e.g., a private serial
DispatchQueue or a dedicated lock) and route all reads/writes through it —
update scheduleIncomingCallTimeout, handleIncomingCallTimeout,
cancelIncomingCallTimeout, stopDDPClientInternal and all DDP callback handlers
to perform their mutations and reads via that queue/lock, ensuring
incomingCallTimeouts additions/removals and
ddpClient/isDdpLoggedIn/observedIncomingCall checks/assignments are executed
synchronously on the same serialized context to prevent races.
---
Duplicate comments:
In `@ios/Libraries/DDPClient.swift`:
- Around line 62-66: The connect callback (connectedCallback) must be registered
before sending the connect message to avoid missing an early "connected" event;
move the assignment/registration of connectedCallback so it occurs prior to
calling self.send(connectMsg) and before invoking waitForConnected in the send
completion handler, and apply the same change to the other occurrences around
the methods referenced (the send/connect flow and the "connected" handling code
paths at the other locations). Ensure the callback is stored on the DDPClient
instance (or weakly referenced as currently used) before send returns so any
incoming "connected" message is delivered to the registered handler rather than
being dropped and causing a false timeout.
In `@ios/Libraries/VoipService.swift`:
- Around line 185-187: persistVoipToken currently ignores the Boolean return
from storage.setString(voipTokenStorageKey), so MMKV write failures are dropped
silently; update persistVoipToken to check the return value of
storage.setString(token, forKey: voipTokenStorageKey) and handle a false result
by logging an error (include voipTokenStorageKey and token context as
appropriate), retrying the write once (or enqueueing the token for a later
retry), and/or falling back to an alternative persistent store so the token is
not lost; ensure you reference persistVoipToken, storage.setString and
voipTokenStorageKey when implementing the fix.
- Around line 108-114: invalidatePushToken currently only clears local state
(lastVoipToken and storage.removeValue(forKey: voipTokenStorageKey)) but doesn't
inform the backend; before wiping local token, iterate over all logged-in
workspaces and call the backend unregister endpoint (e.g., via your workspace
API client or WorkspaceManager) to remove the VoIP registration for that token,
handling errors/retries as appropriate, then clear lastVoipToken and
storage.removeValue(forKey: voipTokenStorageKey); ensure you read the token
value first (from lastVoipToken or storage) so you can pass it to the unregister
calls inside invalidatePushToken.
---
Nitpick comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt`:
- Around line 228-234: The handler call
mainHandler.removeCallbacksAndMessages(null) clears all pending messages;
instead capture the specific timeout Runnable when you post it (e.g., store it
in a field like connectTimeoutRunnable) and remove only that callback by calling
mainHandler.removeCallbacks(connectTimeoutRunnable) in the "connected" branch;
update the code paths that post the timeout to use that stored Runnable and
ensure connectedCallback is still invoked and nulled as before (symbols:
mainHandler, removeCallbacksAndMessages(null), connectedCallback, isConnected).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2728e700-0048-46dc-b18d-a76a3b334ab8
📒 Files selected for processing (10)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktapp/lib/services/restApi.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.tsios/Libraries/DDPClient.swiftios/Libraries/VoipPayload.swiftios/Libraries/VoipService.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/services/restApi.ts
📜 Review details
⏰ 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 context used
🧠 Learnings (1)
📚 Learning: 2026-03-05T06:06:19.755Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:19.755Z
Learning: In the Rocket.Chat React Native iOS app, `WCSession` (WatchConnectivity) is activated with its delegate in `ios/RocketChat Watch App/Loaders/WatchSession.swift`, not in `RCTWatchModule.mm`. Since `WCSession.defaultSession` is a singleton, activating it once in `WatchSession.swift` is sufficient; `RCTWatchModule.mm` does not need to re-activate or re-set the delegate.
Applied to files:
ios/Libraries/DDPClient.swift
🔇 Additional comments (16)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (3)
98-101: Payloads missingcreatedAtare treated as expired, which may drop valid calls.When
getRemainingLifetimeMs()returnsnull(becausecreatedAtis missing or unparseable),isExpired()returnstrue. This causes valid incoming calls without a timestamp to be immediately rejected inshowIncomingCall()andgetInitialEvents().Consider returning
falsewhen the lifetime is unknown to preserve calls with missing timestamps:fun isExpired(): Boolean { val remainingLifetimeMs = getRemainingLifetimeMs() - return remainingLifetimeMs?.let { it <= 0L } ?: true + return remainingLifetimeMs?.let { it <= 0L } ?: false }
212-219: LGTM!The date parsing now correctly handles
ParseExceptionusingrunCatching, allowing fallback to subsequent formatters when one fails.
12-18: LGTM!The
VoipPushTypeenum provides clean type-safe handling of push types, andisVoipIncomingCall()correctly validates required fields.Also applies to: 46-47, 55-60
android/app/src/main/AndroidManifest.xml (1)
131-134: LGTM!The
DeclineReceiveris correctly declared as non-exported, which is appropriate since it's only triggered via explicitPendingIntentwith component targeting from within the app.app/lib/services/voip/MediaSessionInstance.ts (3)
37-38: LGTM!Calling
stopNativeDDPClient()during JS initialization prevents interference between the JS-side DDP connection and the native Android DDP client.
92-108: LGTM!The
answerCallmethod correctly usescallIdfor comparison and properly sets up the call state via the store before navigation.
122-137: LGTM!The
endCallmethod properly handles both ringing and connected call states, and comprehensively cleans up RNCallKeep and the Zustand store.app/lib/services/voip/MediaCallEvents.ts (3)
27-32: LGTM!The token registration now uses the centralized
registerPushToken()function with proper error handling. The destructured parameter{ token }matches the expected event payload format.
52-71: LGTM!The Android initial events handler correctly processes incoming calls, sets the call ID in the store, dispatches the navigation action, and answers the call.
110-120: The comparisoninitialEvents.callId === callUUIDmay not serve the intended purpose.
callIdis a server-provided identifier (String) whilecallUUIDcomes from iOS CallKit. These are distinct values from different sources and comparing them directly may not reliably detect if the call was already answered. Consider verifying the intended logic: should the app map the CallKit UUID back to a server-side identifier, or use a different mechanism to track answered calls?> Likely an incorrect or invalid review comment.android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt (1)
273-293: LGTM!The
buildWebSocketURLfunction correctly handles both HTTP and HTTPS schemes, defaulting to WSS for URLs without an explicit scheme.android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (4)
283-300:signedContractIdcheck is logically contradictory and uses incorrect null handling.Line 287 has two issues:
optString("signedContractId")returns""(empty string) when the key is missing, notnull. The checksignedContractId.isNullOrEmpty()will be true for missing fields.The condition
signedContractId.isNullOrEmpty() && signedContractId != deviceIdis contradictory: ifsignedContractIdis null or empty, it will never equaldeviceId(assumingdeviceIdis non-empty), making the second check redundant. However, the intent appears to be: dismiss the call if another device answered (signedContractId exists and differs from this device).Suggested fix
val signalType = firstArg.optString("type") val signalCallId = firstArg.optString("callId") -val signedContractId = firstArg.optString("signedContractId") +val signedContractId = firstArg.takeIf { + it.has("signedContractId") && !it.isNull("signedContractId") +}?.optString("signedContractId") -if (signalType == "notification" && signalCallId == callId && signedContractId.isNullOrEmpty() && signedContractId != deviceId) { +if (signalType == "notification" && signalCallId == callId && !signedContractId.isNullOrEmpty() && signedContractId != deviceId) {
108-142: LGTM!The timeout and decline handlers now properly clean up the DDP client.
handleDeclineActioncorrectly sends a reject signal via DDP before cleanup, andhandleTimeoutcallsstopDDPClientInternal()after disconnecting the call.
417-440: LGTM!The
showIncomingCallmethod properly validates the payload's lifetime before displaying the call UI, and coordinates the notification, telecom registration, timeout scheduling, and DDP listener setup.
503-573: LGTM!The notification is properly configured with
setTimeoutAfterfor auto-dismissal, full-screen intent for locked devices, and appropriate actions for accept/decline.ios/Libraries/VoipPayload.swift (1)
117-125: Nice improvement: deterministic notification ID hashing.The stable hash implementation avoids cross-launch instability and aligns with Android hash semantics.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
There was a problem hiding this comment.
Pull request overview
Adds end-to-end VoIP push support (token registration + incoming call lifecycle) with native-side timeout/hangup detection and a unified call identifier model across iOS/Android and JS.
Changes:
- iOS: Persist/reuse PushKit token, parse richer VoIP payloads, and add a native DDP listener + CallKit observation for call-end/timeout handling.
- Android: Update VoIP payload parsing (incl. createdAt/lifetime), add timeout + native DDP listener for early hangup detection, and implement native decline flow.
- JS: Switch from
callUUIDtocallId, register VoIP tokens via REST, and stop native DDP when JS media session takes over.
Reviewed changes
Copilot reviewed 45 out of 46 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| ios/RocketChatRN.xcodeproj/project.pbxproj | Adds new iOS VoIP/DDP Swift sources and removes CallIdUUID sources from the Xcode project. |
| ios/RocketChatRN-Bridging-Header.h | Exposes RNDeviceInfo DeviceUID to Swift for device identification. |
| ios/Podfile.lock | Updates ReactCodegen checksum due to native dependency changes. |
| ios/Libraries/VoipService.swift | Persists VoIP token, schedules incoming-call timeout, adds native DDP hangup listener and CallKit call observation. |
| ios/Libraries/VoipPayload.swift | New richer VoIP payload model with createdAt expiry, avatar metadata, and stable notification id. |
| ios/Libraries/VoipModule.mm | Exposes new TurboModule methods (getLastVoipToken, stopNativeDDPClient). |
| ios/Libraries/DDPClient.swift | Adds minimal native iOS DDP WebSocket client for call lifecycle events. |
| ios/Libraries/CallIdUUID.swift | Removes deterministic UUID conversion (replaced by UUID callId approach). |
| ios/Libraries/CallIdUUID.m | Removes the CallIdUUID TurboModule bridge. |
| ios/Libraries/AppDelegate+Voip.swift | Moves PKPushRegistryDelegate handling into a dedicated extension and reports incoming calls via CallKeep. |
| ios/AppDelegate.swift | Removes inline PKPushRegistryDelegate implementation from AppDelegate. |
| app/views/CallView/index.test.tsx | Updates tests to use callId instead of callUUID. |
| app/views/CallView/components/Dialpad/Dialpad.test.tsx | Updates test store state to use callId. |
| app/views/CallView/components/Dialpad/Dialpad.stories.tsx | Updates story store state to use callId. |
| app/views/CallView/components/CallerInfo.test.tsx | Updates test store state to use callId. |
| app/views/CallView/components/CallerInfo.stories.tsx | Updates story store state to use callId. |
| app/views/CallView/CallView.stories.tsx | Updates story store state to use callId. |
| app/stacks/types.ts | Removes CallView navigation param (callUUID) since call identity is now stored centrally. |
| app/lib/services/voip/useCallStore.ts | Renames callUUID → callId and aligns CallKeep operations with the new identifier. |
| app/lib/services/voip/simulateCall.ts | Removes call simulation helper (was UUID-based). |
| app/lib/services/voip/pushTokenAux.ts | Removes in-memory VoIP token cache (token is now retrieved from native storage). |
| app/lib/services/voip/MediaSessionInstance.ts | Uses NativeVoip module, stops native DDP client when JS media session starts, and switches to callId. |
| app/lib/services/voip/MediaCallEvents.ts | Updates call open/answer flows to store callId and triggers push token registration on VoIP token updates. |
| app/lib/services/restApi.ts | Reworks push token registration to include VoIP token and caching of last-sent values. |
| app/lib/notifications/push.ts | Registers push tokens on initial acquisition and refresh (without auth gating). |
| app/lib/native/NativeVoip.ts | Extends native VoIP TurboModule interface and provides a safe fallback implementation. |
| app/lib/native/NativeCallIdUUID.ts | Removes unused CallIdUUID TurboModule JS binding. |
| app/lib/hooks/useSubscription.test.ts | Minor TS/Jest typing and formatting updates. |
| app/definitions/rest/v1/push.ts | Extends push.token POST params to optionally include voipToken. |
| app/definitions/Voip.ts | Updates VoIP payload type to include username/hostName/avatarUrl/createdAt/notificationId and removes callUUID. |
| app/containers/MediaCallHeader/MediaCallHeader.test.tsx | Removes callUUID from mocked call store state. |
| app/containers/MediaCallHeader/MediaCallHeader.stories.tsx | Removes callUUID from mocked call store state. |
| app/actions/deepLinking.ts | Removes callUUID from VoIP deep link action params. |
| android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt | Adds createdAt expiry handling, avatar metadata, and moves payload parsing toward remote/ejson format; removes callUUID. |
| android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt | Adds timeouts, native DDP hangup detection, and implements decline/missed handling. |
| android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt | Uses payload expiry for initial events and adds stopNativeDDPClient/getLastVoipToken behavior. |
| android/app/src/main/java/chat/rocket/reactnative/voip/IncomingCallActivity.kt | Adds timeout handling and lifecycle broadcast listening for incoming call UI. |
| android/app/src/main/java/chat/rocket/reactnative/voip/DDPClient.kt | Adds minimal native Android DDP client for early call-end detection and signaling. |
| android/app/src/main/java/chat/rocket/reactnative/utils/NativeCallIdUUIDSpec.kt | Removes CallIdUUID TurboModule spec. |
| android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDTurboPackage.kt | Removes CallIdUUID TurboPackage registration. |
| android/app/src/main/java/chat/rocket/reactnative/utils/CallIdUUIDModule.kt | Removes Android deterministic UUID generator and module. |
| android/app/src/main/java/chat/rocket/reactnative/notification/RCFirebaseMessagingService.kt | Routes VoIP FCM payloads to VoipNotification handler and logs richer message info. |
| android/app/src/main/java/chat/rocket/reactnative/notification/NotificationIntentHandler.kt | Cancels scheduled VoIP timeouts when handling VoIP intents. |
| android/app/src/main/java/chat/rocket/reactnative/notification/Ejson.java | Makes host public to allow Kotlin callers to set it for credential lookup. |
| android/app/src/main/java/chat/rocket/reactnative/MainApplication.kt | Removes CallIdUUID TurboPackage from the package list. |
| android/app/src/main/AndroidManifest.xml | Registers VoIP decline broadcast receiver. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
force-pushed
the
feat.voip-push
branch
from
5ec6f35 to
7f2e4fb
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_ios_build
GitHub Actions
Failure
diegolmello
added a commit
that referenced
this pull request
Apr 22, 2026
…/Decline (#7215) * merge feat.voip-lib * feat(voip): enhance call handling with UUID mapping and event listeners * Base call UI * feat(voip): integrate Zustand for call state management and enhance CallView UI * feat(voip): add simulateCall function for mock call handling in UI development * refactor(CallView): update button handlers and improve UI responsiveness * Add pause-shape-unfilled icon * Base CallHeader * toggleFocus * collapse buttons * Header components * Hide header when no call * Timer * Add use memo * Add voice call item on sidebar * cleanup * Temp use @rocket.chat/media-signaling from .tgz * cleanup * Check module and permissions to enable voip * Refactor stop method to use optional chaining for media signal listeners * voip push first test * Add VoIP call handling with pending call management - Implemented VoIP push notification handling in index.js, including storing call info for later processing. - Added CallKeep event handlers for answering and ending calls from a cold start. - Introduced a new CallIdUUID module to convert call IDs to deterministic UUIDs for compatibility with CallKit. - Created a pending call store to manage incoming calls when the app is not fully initialized. - Updated deep linking actions to include VoIP call handling. - Enhanced MediaSessionInstance to process pending calls and manage call states effectively. * Remove pending store and create getInitialEvents on app/index * Attempt to make iOS calls work from cold state * lint and format * Patch callkeep ios * Temp send iOS voip push token on gcm * Temp fix require cycle * chore: format code and fix lint issues [skip ci] * CallIDUUID module on android and voip push * Add setCallUUID on useCallStore to persist calls accepted on native Android * remove callkeep from notification * Android Incoming Call UI POC * Refactor VoIP handling: Migrate VoIP-related classes to a new package structure, removing deprecated modules and consolidating functionality. Update imports in MainApplication and NotificationIntentHandler to reflect changes. This cleanup enhances code organization and prepares for future VoIP feature enhancements. * Remove VoipForegroundService * cleanup and use caller instead of callerName * Cleanup and make iOS build again * Refactor VoIP handling: Remove unused event emissions for call answered and declined, switch from SharedPreferences to in-memory storage for pending VoIP call data, and update method signatures for better clarity. This cleanup enhances performance and prepares for future VoIP feature improvements. * Refactor VoIP handling: Introduce a new VoipPayload class to encapsulate call data, streamline notification processing, and enhance method signatures across the VoIP module. This update improves code clarity and prepares for future feature enhancements. * Migrate react-native-voip-push-notifications to VoipModule * Refactor VoIP module: Update package structure by moving VoipTurboPackage to the main package and removing the obsolete NativeVoipSpec class. Adjust imports in MainApplication and VoipModule to reflect these changes, enhancing code organization and maintainability. * Unify emitters * Move CallKeep listeners from MediaSessionInstance to getInitialEvents * Clear callkeep on endcall * Unify getInitialEvents logic * getInitialEvents -> MediaCallEvents * chore: format code and fix lint issues [skip ci] * feat(Android): Add full screen incoming call (#6977) * feat: Update call UI (#6990) * feat: Handle audio routing, e.g., Bluetooth headset vs. internal speaker switching (#6992) * fix: empty space when not on call (#6993) * feat: Dialpad (#7000) * action: organized translations * feat: start call (#7024) * chore: format code and fix lint issues * feat: Pre flight (#7038) * action: organized translations * feat: Receive voip push notifications from backend (#7045) * feat: Refactor media session handling and improve disconnect logic (#7065) * feat: Control incoming call from native (#7066) * feat: Voice message blocks (#7057) * feat: native accept success event (#7068) * feat(voip): call waiting, busy detection, and videoconf blocking (#7077) * action: organized translations * feat(voip): tap-to-hide call controls with animations (#7078) * feat(voip): navigate to call DM from message button and header (#7082) * feat(voip): tablet and landscape layout (#7110) * chore: develop into feat.voip-lib-new (RN 81 + Expo 54 + reanimated 4 + true-sheet + iOS 26) (#7114) * chore: format code and fix lint issues * feat(voip): android landscape layout for IncomingCallActivity (#7116) * Update agents files * feat(voip): Support a11y (#7106) * Fix content cutting on iOS on some edge cases * pods * Ignore .worktrees on jest * chore: Merge develop into feat.voip-lib-new (#7129) * fix(voip): show CallKit UI when call is active in background (#7128) * chore: Update media-signaling to 0.2.0 (#7153) * feat(voip): migrate iOS accept/reject from DDP to REST (#7124) * Fix icons * feat(voip): migrate Android accept/reject from DDP to REST (#7127) * test(voip): integration tests for CallView pipeline (#7161) * feat(voip): display video conf provider as subtitle (#7160) * fix(voip): CallView button grid and correct landscape/dialpad layouts (#7164) * fix(voip): prevent stale MMKV cache on Android first-install accept MMKVKeyManager.initialize ran in MainApplication.onCreate before the JS engine started and opened the default MMKV file via the Tencent 1.2 JAR when it was still empty. Tencent caches instances per-ID in a singleton registry, so that empty-state view was held for the rest of the process. JS later wrote credentials through react-native-mmkv (MMKV Core 2.0), which has its own separate registry. When a VoIP push arrived, Ejson.getMMKV() got the cached empty Tencent instance and reported "No userId found in MMKV for server". Closing and reopening the app cleared the cache, which is why only the very first call after install failed. Drop the open/verify block — the encryption key is already cached from SecureKeystore, so no MMKV handle is needed here. The first Tencent instance is now created inside Ejson.getMMKV() after JS has written, so it scans the file fresh. * fix(voip): prevent duplicate ringtone on Android incoming call (#7158) * fix(voip): set explicit snaps for NewMediaCall bottom sheet (#7165) * Update app/lib/services/voip/MediaSessionStore.ts Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com> * fix: make startVoipFork reactive to permissions-changed (#7151) * fix(android): remove MediaProjectionService from merged manifest (#7190) * fix(voip): Phone account creation (#7170) * feat: add Enable Mobile Ringing toggle in user preferences (#7155) * fix(voip): ship blockers for PushKit, licensing, outbound calls, push tokens (#7167) * fix(android): Play Store mic discoverability, safer FCM logs, avatar auth via headers (#7171) * fix(ios): serialize VoipService bridge statics (#7169) * fix(voip): Android DDP thread safety and VoipPayload bundle parity (#7168) * chore(voip): dead-code and hygiene sweep (#7174) * refactor(voip): decouple navigateToCallRoom from Redux and backfill REST/connect tests (#7176) * test(voip): tighten ringing endCall assertion and add VideoConf VoIP-lock saga coverage (#7177) * fix(ios): harden VoIP DDP WebSocket client on receive failures and TLS (#7173) * refactor(voip): MediaCallEvents Redux adapters and resetVoipState (#7178) * refactor(voip): decouple peer autocomplete from Redux; simplify NewMediaCall (#7175) * fix(ios): add NS_SWIFT_NAME to Challenge.runChallenge for Swift 6.2 compatibility Swift 6.2 (Xcode 26.x / macos-26 runner) auto-renames the Objective-C method runChallenge:didReceiveChallenge:completionHandler: to run(_:didReceive:completionHandler:) when imported into Swift. Add NS_SWIFT_NAME to explicitly pin the Swift import name, preventing the compiler from applying its heuristics. This keeps the existing Swift call site in DDPClient.swift working without changes. * fix(ios): cancel old URLSession/webSocketTask before reconnecting in DDPClient.connect (#7197) * fix(ios): add NSLock to nativeAcceptHandledCallIds and 10s REST timeout to handleNativeAccept (#7198) * feat(android): create VoipCallService with FOREGROUND_SERVICE_MICROPHONE (#7199) * fix(android): start VoipCallService on accept, stop on hangup/timeout, install end-call listener (#7200) * fix(voip): enable DM nav for users with SIP extension (#7203) * fix(android): handle null VoiceConnection in answerIncomingCall, notify JS (#7201) * fix(voip): resolve closure capture ordering in handleNativeAccept (#7209) * fix(android): integrate VoIP modules with SSL-pinned OkHttpClient (#7208) * fix(push): gate id and voipToken behind server version checks, fix VideoConf caller extra (#7210) * fix(voip): remove sensitive data from production logs (#7207) * fix(android): remove isRunning guard + add double-tap guard on Accept/Decline - VoipCallService: remove if (!isRunning) guard, call startForeground unconditionally (idempotent on Android, fixes Android 14+ foreground service requirement) - IncomingCallActivity: add AtomicBoolean guard on handleAccept/handleDecline to prevent double-tap from triggering multiple service starts --------- Co-authored-by: diegolmello <diegolmello@users.noreply.github.com> Co-authored-by: Pierre Lehnen <55164754+pierre-lehnen-rc@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-7
How to test or reproduce
Screenshots
Types of changes
Checklist
Further comments
Summary by CodeRabbit
New Features
Bug Fixes