fix(voip): Phone account creation#7170
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughVoIP Android now always constructs and registers a self‑managed PhoneAccountHandle (resolving app label) and tightens input validation/logging; RNCallKeep Android hardens PhoneAccount access and always marks connections self‑managed; iOS initializes delayed events; manifest foreground service types and a dev dependency updated. Changes
Sequence DiagramsequenceDiagram
participant RN as React Native JS
participant NM as Native Module
participant TM as TelecomManager
participant PA as PhoneAccountRegistry
participant VC as VoiceConnectionService
RN->>NM: displayIncomingCall / startCall (uuid, number, caller)
NM->>NM: validate inputs (non-empty callId/caller)
NM->>TM: ensureSelfManagedPhoneAccountRegistered(appLabel)
TM->>PA: registerPhoneAccount(PhoneAccount with CAPABILITY_SELF_MANAGED)
PA-->>TM: registration result / SecurityException handled
NM->>VC: create Connection (set PROPERTY_SELF_MANAGED)
VC-->>NM: connection lifecycle events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
666-669:⚠️ Potential issue | 🟡 MinorRedundant null checks on non-nullable
Stringparameters.
callIdandcallerare declaredString(non-nullable), soisNullOrEmpty()will issue a compiler warning and the log message is misleading ("callId is null or empty" also fires for emptycaller). PreferisEmpty()and disambiguate the message.🔧 Proposed fix
- if (callId.isNullOrEmpty() || caller.isNullOrEmpty()) { - Log.e(TAG, "Cannot register call with TelecomManager: callId is null or empty") + if (callId.isEmpty() || caller.isEmpty()) { + Log.e(TAG, "Cannot register call with TelecomManager: callId or caller is 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 666 - 669, The null-checks on callId and caller are redundant because they are non-nullable Strings; replace isNullOrEmpty() with isEmpty() and provide distinct log messages so the logged reason identifies which parameter is empty. Locate the check in VoipNotification (the block using TAG, callId and caller) and change the condition to test callId.isEmpty() || caller.isEmpty(), but log separate errors (e.g., "Cannot register call: callId is empty" or "Cannot register call: caller is empty") or check each individually and return after the specific log to make the message unambiguous.
🧹 Nitpick comments (1)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
717-733: Consider registering once per process rather than on every incoming call.
addNewIncomingCallgets invoked for every push, which meansregisterPhoneAccountruns each time. While the Telecom API is safe to call repeatedly, on some OEMs repeated registration can reset user-facing account state (e.g., enabled toggle) or add noticeable latency in the FCM hot path. A process-scopedAtomicBoolean/lazy guard is cheap insurance:🔧 Proposed fix sketch
companion object { ... + private val phoneAccountRegistered = AtomicBoolean(false) } ... - private fun ensureSelfManagedPhoneAccountRegistered( + private fun ensureSelfManagedPhoneAccountRegistered( telecomManager: TelecomManager, handle: PhoneAccountHandle, label: String ) { if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return + if (!phoneAccountRegistered.compareAndSet(false, true)) return try { val account = PhoneAccount.builder(handle, label) .setCapabilities(PhoneAccount.CAPABILITY_SELF_MANAGED) .build() telecomManager.registerPhoneAccount(account) } catch (e: SecurityException) { + phoneAccountRegistered.set(false) // allow retry on next push Log.e(TAG, "SecurityException registering PhoneAccount. MANAGE_OWN_CALLS may be denied.", e) } catch (e: Exception) { + phoneAccountRegistered.set(false) Log.e(TAG, "Failed to register self-managed PhoneAccount", e) } }🤖 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 717 - 733, The ensureSelfManagedPhoneAccountRegistered call is executed for every incoming push (via addNewIncomingCall) and should be guarded so registerPhoneAccount runs only once per process; add a process-scoped guard (e.g., a private companion object AtomicBoolean or a lazily-initialized flag) and check it at the top of ensureSelfManagedPhoneAccountRegistered (or from addNewIncomingCall) to short-circuit repeated registration, ensuring thread-safety (compare-and-set) so the first thread registers and others skip; preserve existing exception handling and optionally reset the flag only on fatal unrecoverable conditions if needed.
🤖 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 680-686: The PhoneAccountHandle ID is currently built using
getApplicationLabel(), which is locale-dependent and causes orphaned
PhoneAccount registrations when the app locale/label changes; change the handle
ID construction to use a stable identifier such as context.packageName (keep the
localized getApplicationLabel() only for display/labeling), i.e. when creating
PhoneAccountHandle use PhoneAccountHandle(componentName, context.packageName) or
otherwise pass packageName as the persistent ID to ensure matching native/JS
handles, and update the getApplicationLabel() implementation to remain for UI
label use only and not for the PhoneAccountHandle ID (also ensure
ensureSelfManagedPhoneAccountRegistered and any other places constructing
PhoneAccountHandle use the stable packageName ID).
In `@patches/react-native-callkeep`+4.3.16.patch:
- Around line 33-55: The change unconditionally sets
Connection.PROPERTY_SELF_MANAGED in VoiceConnectionService which assumes the
PhoneAccount was always registered with CAPABILITY_SELF_MANAGED (per
VoipNotification.registerCallWithTelecomManager); add a brief comment in
VoiceConnectionService explaining this assumption and the risk if JS ever calls
RNCallKeep.setup with selfManaged:false, or alternatively add an assertion/check
to guard against mismatched capability vs property; also mention in the patch
header that removing telecomManager.getPhoneAccount(request.getAccountHandle())
avoids a potential NPE when the handle is unregistered, and update the
patch-package context/footer so the patch hash matches.
---
Outside diff comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 666-669: The null-checks on callId and caller are redundant
because they are non-nullable Strings; replace isNullOrEmpty() with isEmpty()
and provide distinct log messages so the logged reason identifies which
parameter is empty. Locate the check in VoipNotification (the block using TAG,
callId and caller) and change the condition to test callId.isEmpty() ||
caller.isEmpty(), but log separate errors (e.g., "Cannot register call: callId
is empty" or "Cannot register call: caller is empty") or check each individually
and return after the specific log to make the message unambiguous.
---
Nitpick comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt`:
- Around line 717-733: The ensureSelfManagedPhoneAccountRegistered call is
executed for every incoming push (via addNewIncomingCall) and should be guarded
so registerPhoneAccount runs only once per process; add a process-scoped guard
(e.g., a private companion object AtomicBoolean or a lazily-initialized flag)
and check it at the top of ensureSelfManagedPhoneAccountRegistered (or from
addNewIncomingCall) to short-circuit repeated registration, ensuring
thread-safety (compare-and-set) so the first thread registers and others skip;
preserve existing exception handling and optionally reset the flag only on fatal
unrecoverable conditions if needed.
🪄 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: c43b215f-56b5-45bc-bea0-e418710a4079
📒 Files selected for processing (2)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktpatches/react-native-callkeep+4.3.16.patch
📜 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: format
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktpatches/react-native-callkeep+4.3.16.patch
🔇 Additional comments (2)
patches/react-native-callkeep+4.3.16.patch (2)
60-67: LGTM — iOS lazy init avoids silent event drop.
[NSMutableArray addObject:]on a nil receiver is a no-op in Objective-C, so events posted before_delayedEventswas initialized were being silently dropped. This matches the upstream workaround referenced in the comment.
5-28: The patch is safe; no 3-arg calls todisplayIncomingCallorstartCallexist in the codebase.All VoIP service calls use 2 arguments (userId/actor, or peer/type), which will match the remaining 4-arg and 5-arg overloads with the
hasVideoparameter. TheMediaSessionInstancewrapper properly abstracts the native module, and no code calls the removed 3-arg forms.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
patches/react-native-callkeep+4.3.16.patch (1)
38-53: Defensive try/catch aroundhasPhoneAccountis reasonable; consider caching the lookup.Wrapping the capability check in
try/catch(SecurityException)is a sensible hardening (self-managed reads can still throw on some OEMs / permission states). Minor nit:telecomManager.getPhoneAccount(handle)is invoked twice — once for the null check and again forisEnabled(). Caching into a local avoids a second IPC and a theoretical TOCTOU where the second call returnsnull.♻️ Optional refactor
- try { - return isConnectionServiceAvailable() && telecomManager != null && - hasPermissions() && telecomManager.getPhoneAccount(handle) != null && - telecomManager.getPhoneAccount(handle).isEnabled(); - } catch (SecurityException e) { + try { + if (!isConnectionServiceAvailable() || telecomManager == null || !hasPermissions()) { + return false; + } + PhoneAccount account = telecomManager.getPhoneAccount(handle); + return account != null && account.isEnabled(); + } catch (SecurityException e) { Log.w(TAG, "[RNCallKeepModule] hasPhoneAccount: SecurityException querying phone account", e); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-callkeep`+4.3.16.patch around lines 38 - 53, The try/catch around the phone-account check is fine but avoid calling telecomManager.getPhoneAccount(handle) twice; in the method inside RNCallKeepModule (the block that currently calls telecomManager.getPhoneAccount(handle) for null and then .isEnabled()), cache the result into a local (e.g., PhoneAccount phoneAccount = telecomManager.getPhoneAccount(handle)), then check phoneAccount != null && phoneAccount.isEnabled() inside the existing try, and keep the SecurityException catch that logs and returns false.
🤖 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 677-680: Update the stale comment above the PhoneAccountHandle
construction (around ComponentName, CALLKEEP_CONNECTION_SERVICE_CLASS,
PhoneAccountHandle) to state that the handle ID is the app's package name
(context.packageName) not the application label, fix the wording from "collide"
to "coincide", and add a short note warning future readers not to revert to
using getApplicationLabel() for the handle ID.
---
Nitpick comments:
In `@patches/react-native-callkeep`+4.3.16.patch:
- Around line 38-53: The try/catch around the phone-account check is fine but
avoid calling telecomManager.getPhoneAccount(handle) twice; in the method inside
RNCallKeepModule (the block that currently calls
telecomManager.getPhoneAccount(handle) for null and then .isEnabled()), cache
the result into a local (e.g., PhoneAccount phoneAccount =
telecomManager.getPhoneAccount(handle)), then check phoneAccount != null &&
phoneAccount.isEnabled() inside the existing try, and keep the SecurityException
catch that logs and returns false.
🪄 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: 1744987e-7dd3-4a22-b154-247b71c6a0e7
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (4)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktpackage.jsonpatches/react-native-callkeep+4.3.16.patch
✅ Files skipped from review due to trivial changes (1)
- package.json
📜 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)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
patches/react-native-callkeep+4.3.16.patchandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
🔇 Additional comments (4)
android/app/src/main/AndroidManifest.xml (1)
141-141: LGTM — foreground service types match declared permissions.Broadening
foregroundServiceTypetomicrophone|phoneCallis consistent with theFOREGROUND_SERVICE_PHONE_CALLpermission declared at line 18 and the move to a self-managed TelecomConnectionServicein this PR.android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (1)
716-732: Self-managed PhoneAccount registration from native looks correct.Good call registering from native in addition to JS — this closes the gap where an FCM push arrives before
RNCallKeep.setuphas run on JS. The API 26+ guard,CAPABILITY_SELF_MANAGEDcapability, and splitSecurityException/ generic catch are all appropriate, andregisterPhoneAccountis documented as idempotent for the same handle, so repeated invocations on each incoming call are safe.patches/react-native-callkeep+4.3.16.patch (2)
5-12: Handle ID aligned with native registration — nice fix.Switching the RNCallKeep
PhoneAccountHandleID tocontext.getPackageName()matches the native-side construction inVoipNotification.registerCallWithTelecomManager(line 680), so both paths register/lookup the same PhoneAccount regardless of device locale or a translatedapp_name. This is the right pairing for the orphaned-account fix.
87-94: iOS_delayedEventsnil-guard looks correct.This matches the upstream workaround for the referenced issue (events enqueued before
_delayedEventsis initialized bystartObserving/module init would previously be silently dropped). Lazy-allocating on first append is safe here sincesendEventWithNameWrapperruns on the RN JS/main queue.
|
Android Build Available Rocket.Chat Experimental 4.72.0.108569 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNSRYW-2TFrOlHk24qg7j_ZmKTAlNXVqZrEzWaZucppIt6VCyp5_QpGKTvNjZHVJvAAMSGVQYb1OzdZtrLfR |
|
Android Build Available Rocket.Chat 4.72.0.108570 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNQYwjBaLVyN1WC8TPQDMpkyVQVcRadBPowZxvCALz-B5I1RUN-9GrN0m64M1uE8dCndGe5QDqdHMHyaAevv |
|
Android Build Available Rocket.Chat Experimental 4.72.0.108571 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTY1nXdHE8lOll-W2sb3tsLQ9dJ92nAY11bezqbwCITOtfTcRvcyUdR8IHiZZ1eyUeqPa-mWooTNV45w3TM |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
patches/react-native-callkeep+4.3.16.patch (1)
45-52: Minor:getPhoneAccount(handle)is still invoked twice — residual NPE window.The try/catch correctly handles
SecurityException, but the expression callstelecomManager.getPhoneAccount(handle)twice. If the first call returns non-null and the second returns null (unlikely but possible if the account is deregistered between calls, or on certain OEM stubs),.isEnabled()NPEs despite the guard. Cache the result once:🛡️ Proposed refactor
- try { - return isConnectionServiceAvailable() && telecomManager != null && - hasPermissions() && telecomManager.getPhoneAccount(handle) != null && - telecomManager.getPhoneAccount(handle).isEnabled(); - } catch (SecurityException e) { + try { + if (!isConnectionServiceAvailable() || telecomManager == null || !hasPermissions()) { + return false; + } + PhoneAccount account = telecomManager.getPhoneAccount(handle); + return account != null && account.isEnabled(); + } catch (SecurityException e) { Log.w(TAG, "[RNCallKeepModule] hasPhoneAccount: SecurityException querying phone account", e); return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patches/react-native-callkeep`+4.3.16.patch around lines 45 - 52, The expression calls telecomManager.getPhoneAccount(handle) twice causing a residual NPE window; update the logic in the hasPhoneAccount (or equivalent) block to call telecomManager.getPhoneAccount(handle) once, store the result in a local variable (e.g., PhoneAccount account), then evaluate account != null && account.isEnabled() combined with the existing isConnectionServiceAvailable(), telecomManager != null, and hasPermissions() checks inside the try/catch so the null-check and isEnabled() use the same cached account instance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@patches/react-native-callkeep`+4.3.16.patch:
- Around line 45-52: The expression calls telecomManager.getPhoneAccount(handle)
twice causing a residual NPE window; update the logic in the hasPhoneAccount (or
equivalent) block to call telecomManager.getPhoneAccount(handle) once, store the
result in a local variable (e.g., PhoneAccount account), then evaluate account
!= null && account.isEnabled() combined with the existing
isConnectionServiceAvailable(), telecomManager != null, and hasPermissions()
checks inside the try/catch so the null-check and isEnabled() use the same
cached account instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 198750cd-a8a3-420c-9969-083d36ddf66d
📒 Files selected for processing (1)
patches/react-native-callkeep+4.3.16.patch
📜 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 (5)
patches/react-native-callkeep+4.3.16.patch (5)
69-89: UnconditionalPROPERTY_SELF_MANAGED— see prior note on coupling to the native registration path.Consistent with
VoipNotification.ensureSelfManagedPhoneAccountRegisteredalways usingCAPABILITY_SELF_MANAGED. The added comment clearly documents the Android 11+READ_PHONE_NUMBERSmotivation. Still worth the caveat from the earlier review: any futureRNCallKeep.setup({ selfManaged: false })path would silently advertise a mismatched PROPERTY vs CAPABILITY and be rejected by Telecom — consider an assert/guard if that flow ever becomes reachable.
9-10: PhoneAccountHandle id correctly aligned with native registration.Switching to
context.getPackageName()matches the handle id used byVoipNotification.registerCallWithTelecomManager(PhoneAccountHandle(componentName, context.packageName)), so both JS-side and native-side resolve to the same account. Good fix.
67-67: Null-check onRNCallKeepModule.instance— the targeted PR fix looks correct.This avoids the NPE in
startForegroundServicewhenVoiceConnectionServiceis started before (or outside of) the React module lifecycle (e.g., FCM background wake). The downstreamif (currentActivity != null)already handles the null path gracefully.
100-101: iOS_delayedEventslazy-init is correct.Appending to a
nilNSMutableArrayis a silent no-op in Objective-C, so delayed events were being dropped until the array was first allocated elsewhere. The lazy-init here (backed by the linked upstream issue) fixes that cleanly.
18-37: No risk to app code—this codebase does not call these overloads from JavaScript.The app never invokes
RNCallKeep.displayIncomingCall()orRNCallKeep.startCall()from JS code. It instead uses the@rocket.chat/media-signalingSDK for call management (viamediaSessionInstance.startCall()). ThedisplayIncomingCallmethod appears only in the test mock, never in production code. The 3-argument overloads being removed are not a breaking change for this application.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 666-667: The current error log in the validation branch prints raw
identifiers (callId and caller) which may expose sensitive data; update the
Log.e call in the same block (reference TAG, callId, caller) to avoid outputting
the actual values and instead log only that fields are empty or present (e.g.,
indicate which field is empty or true/false presence flags) and keep the message
descriptive about the TelecomManager registration failure without including the
caller or callId contents.
🪄 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: 011c4077-2f18-4e81-8f65-49efcfc60da0
📒 Files selected for processing (1)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
📜 Review details
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
🔇 Additional comments (3)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt (3)
677-687: LGTM — PhoneAccountHandle now uses a stable ID.The
context.packageNamehandle ID matches the patched RNCallKeep handle construction, while the localized app label is kept for display only.
711-715: LGTM — label lookup is scoped to display text.This helper now supports the PhoneAccount label without feeding localized text into the persistent handle ID.
718-734: LGTM — native self-managed registration is guarded and contained.The Android O+ guard,
CAPABILITY_SELF_MANAGED, and exception handling fit the FCM-before-JS setup path.
1e6b760 to
e74ba05
Compare
…/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>
Proposed changes
Fixes Android VoIP incoming calls that failed from a cold state because the
PhoneAccountwas not yet registered withTelecomManagerwhen the FCM push arrived. The app now registers a self-managedPhoneAccountnatively (idempotently) before callingaddNewIncomingCall, so calls work even before JS boots and runsRNCallKeep.setup.Additional Android hardening via
react-native-callkeeppatches:packageName(locale-stable) instead of the localized app label as thePhoneAccountHandleID, so the JS-side and native-side registrations share one handle.PROPERTY_SELF_MANAGEDinstead of queryingtelecomManager.getPhoneAccount(...), which requiresREAD_PHONE_NUMBERSon Android 11+ and isn't grantable from an FCM background context.hasPhoneAccountintry/catchforSecurityException.RNCallKeepModule.instanceinstartForegroundServiceto avoid NPE when the service starts before the JS module initialises.Android manifest: add
phoneCallto theVoiceConnectionServiceforegroundServiceTypeso the foreground service is legal for a self-managed telecom connection.iOS: initialise event storage eagerly inside
RCT_EXPORT_MODULE()so events emitted beforesetupisn't called (cold-start CallKit) aren't dropped.Issue(s)
Related to the VoIP lib base branch (
feat.voip-lib-new).How to test or reproduce
Android:
iOS:
Screenshots
N/A — native-only behavioural fix.
Types of changes
Checklist
Further comments
The
react-native-callkeepchanges live inpatches/react-native-callkeep+4.3.16.patch(applied viapatch-packageatpostinstall). The callkeep version is unchanged (4.3.16); only the patch grew. The@types/jestbump inpackage.jsonis an unrelated stray pin — harmless.