feat: native accept success event#7068
Merged
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:
WalkthroughReworks native-to-JS VoIP accept signaling: introduces and emits a new VoipAcceptSucceeded event on Android/iOS, records a sticky nativeAcceptedCallId with a 15s stale timer in the call store, and gates auto-answer to require native accept confirmation before answering incoming calls. Changes
Sequence DiagramsequenceDiagram
participant Native as Native Layer (iOS/Android)
participant Events as VoIP Event Bridge
participant Store as useCallStore
participant Session as MediaSessionInstance
participant Deep as deepLinking Saga
Native->>Events: Emit VoipAcceptSucceeded (payload)
Events->>Store: setNativeAcceptedCallId(callId)
activate Store
Store->>Store: store nativeAcceptedCallId\narm 15s stale timer
deactivate Store
Events->>Deep: deepLinkingOpen({callId, host})
activate Deep
Deep->>Store: navigation / room open
deactivate Deep
Session->>Session: Receive stream "accepted" notification
Session->>Store: read nativeAcceptedCallId
alt nativeAcceptedCallId matches && no bound call && signedContractId matches
Session->>Session: answerCall(callId)
activate Session
Session->>Store: setCall(call)
Store->>Store: clear nativeAcceptedCallId\ncancel stale timer
deactivate Session
else mismatch or already-bound
Session->>Session: skip auto-answer
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 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 |
![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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/lib/services/voip/MediaCallEvents.ts (1)
149-175:⚠️ Potential issue | 🟠 MajorOnly set
nativeAcceptedCallIdfor explicit native accept-success.
VoipNotification.prepareMainActivityForIncomingVoip(...storePayloadForJs = true)still stashes plain incoming-call payloads on Android when MainActivity is just opened. This block still treats every Android initial event as “answered”, so Line 174 marks a non-accepted launch asnativeAcceptedCallIdand weakens the new auto-answer gate. Keep the deep link, but reservesetNativeAcceptedCallId()for an explicit accept-success signal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaCallEvents.ts` around lines 149 - 175, The code currently assumes Android initial events mean the call was answered and calls useCallStore.getState().setNativeAcceptedCallId(initialEvents.callId); change this so setNativeAcceptedCallId is only invoked when there's an explicit native accept-success (e.g., when an RNCallKeepPerformAnswerCallAction is present for the call UUID or another explicit accept signal), not by default on Android; update the logic in MediaCallEvents (the block that uses isIOS, RNCallKeep.getInitialEvents(), initialEvents.callId and setNativeAcceptedCallId) to detect a real "answer" event before setting nativeAcceptedCallId and keep the deep link handling separate from marking the call as natively accepted.app/lib/services/voip/MediaSessionInstance.ts (1)
64-80:⚠️ Potential issue | 🟠 MajorAwait
processSignal()before triggering the native-accept auto-answer.
MediaSignalingSession.processSignal()is asynchronous and returns a Promise. The current code callsprocessSignal()without awaiting it, then immediately checks conditions and may callanswerCall()before the session state is ready. If this happens, the new fallback path can end CallKeep and clearnativeAcceptedCallIdfor a valid native accept.Suggested change
- this.instance.processSignal(signal); - - console.log('🤙 [VoIP] Processed signal:', signal); - - // Answer when native already accepted and stream matches device contract + callId. - const storeSlice = useCallStore.getState(); - const { call, nativeAcceptedCallId } = storeSlice; - - if ( - signal.type === 'notification' && - signal.notification === 'accepted' && - signal.signedContractId === getUniqueIdSync() && - nativeAcceptedCallId === signal.callId && - call == null - ) { - this.answerCall(signal.callId).catch(error => { - console.error('[VoIP] Error answering call on notification/accepted:', error); - }); - } + void Promise.resolve(this.instance.processSignal(signal)) + .then(() => { + console.log('🤙 [VoIP] Processed signal:', signal); + + const { call, nativeAcceptedCallId } = useCallStore.getState(); + if ( + signal.type === 'notification' && + signal.notification === 'accepted' && + signal.signedContractId === getUniqueIdSync() && + nativeAcceptedCallId === signal.callId && + call == null + ) { + return this.answerCall(signal.callId); + } + }) + .catch(error => { + console.error('[VoIP] Error processing signal:', error); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 64 - 80, The code calls this.instance.processSignal(signal) without awaiting it, so session state may not be updated before the native-accept auto-answer logic runs; update the handler to await the promise returned by MediaSignalingSession.processSignal (i.e., await this.instance.processSignal(signal)) before reading useCallStore.getState() and potentially calling this.answerCall(signal.callId), ensuring any surrounding function is async or the promise is properly handled so the conditionals that use nativeAcceptedCallId and call see the updated session state.
🤖 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/voip/useCallStore.ts`:
- Around line 36-50: The cleanup timers currently null out callId
unconditionally which can clear an unrelated JS call; update the two timer
cleanup blocks (the one using staleNativeTimer/staleNativeScheduledId and the
other at the second cleanup path) to only reset callId when the stored callId
matches the nativeAcceptedCallId/scheduled id being cleared (use get() to
compare callId === scheduled/nativeAcceptedCallId before setting callId:null),
leaving nativeAcceptedCallId cleared as before; also add a regression test that
does setCallId('js'); setNativeAcceptedCallId('native') and verifies that
expiring the native-accept timer clears nativeAcceptedCallId but does not clear
the unrelated callId.
---
Outside diff comments:
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Around line 149-175: The code currently assumes Android initial events mean
the call was answered and calls
useCallStore.getState().setNativeAcceptedCallId(initialEvents.callId); change
this so setNativeAcceptedCallId is only invoked when there's an explicit native
accept-success (e.g., when an RNCallKeepPerformAnswerCallAction is present for
the call UUID or another explicit accept signal), not by default on Android;
update the logic in MediaCallEvents (the block that uses isIOS,
RNCallKeep.getInitialEvents(), initialEvents.callId and setNativeAcceptedCallId)
to detect a real "answer" event before setting nativeAcceptedCallId and keep the
deep link handling separate from marking the call as natively accepted.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 64-80: The code calls this.instance.processSignal(signal) without
awaiting it, so session state may not be updated before the native-accept
auto-answer logic runs; update the handler to await the promise returned by
MediaSignalingSession.processSignal (i.e., await
this.instance.processSignal(signal)) before reading useCallStore.getState() and
potentially calling this.answerCall(signal.callId), ensuring any surrounding
function is async or the promise is properly handled so the conditionals that
use nativeAcceptedCallId and call see the updated session state.
🪄 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: c61b6815-89b4-4b19-b30e-9b1224ca7bf8
📒 Files selected for processing (12)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktapp/containers/MediaCallHeader/MediaCallHeader.test.tsxapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.tsapp/lib/services/voip/useCallStore.tsapp/sagas/deepLinking.jsapp/sagas/selectServer.tsios/Libraries/VoipModule.mmios/Libraries/VoipService.swift
💤 Files with no reviewable changes (1)
- app/sagas/selectServer.ts
📜 Review details
🧰 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
🔇 Additional comments (3)
app/containers/MediaCallHeader/MediaCallHeader.test.tsx (1)
92-106: LGTM! Test correctly covers the native-accepted intermediate state.The test validates the scenario where a call has been natively accepted (with
callIdset) but the JS-sideanswerCallhasn't completed yet (call: null,callState: 'none'). This aligns well with the PR objectives.Optional: For consistency with the existing "no call" test (lines 77-90), you could add assertions for the other child elements:
expect(getByTestId('media-call-header-empty')).toBeTruthy(); expect(queryByTestId('media-call-header')).toBeNull(); + expect(queryByTestId('media-call-header-collapse')).toBeNull(); + expect(queryByTestId('media-call-header-content')).toBeNull(); + expect(queryByTestId('media-call-header-end')).toBeNull();[approve_code_changes, suggest_optional_refactor]
app/lib/services/voip/useCallStore.test.ts (1)
97-131: Good timer lifecycle coverage.The cancel/rearm cases here pin the new stale-native behavior tightly.
app/lib/services/voip/useCallStore.ts (1)
270-281: Nice reset semantics.Preserving
nativeAcceptedCallIdhere and rearming the timeout makes the cold/warm-start handoff resilient without leaving sticky native state around forever.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
199-350: Add a negative test forsignedContractIdmismatch.The new suite validates
nativeAcceptedCallIdgating well, but it does not assert the device-contract guard failure path (signedContractId !== getUniqueIdSync()), which is part of the production gate.Suggested test case
+ it('does not call answerCall when signedContractId does not match device', async () => { + const answerSpy = jest.spyOn(mediaSessionInstance, 'answerCall').mockResolvedValue(undefined); + mockUseCallStoreGetState.mockReturnValue({ + reset: mockCallStoreReset, + setCall: jest.fn(), + setCallId: jest.fn(), + resetNativeCallId: jest.fn(), + call: null, + callId: null, + nativeAcceptedCallId: 'from-signal' + }); + mediaSessionInstance.init('user-1'); + const streamHandler = getStreamNotifyHandler(); + streamHandler({ + msg: 'changed', + fields: { + eventName: 'uid/media-signal', + args: [ + { + type: 'notification', + notification: 'accepted', + signedContractId: 'other-device-id', + callId: 'from-signal' + } + ] + } + }); + await Promise.resolve(); + expect(answerSpy).not.toHaveBeenCalled(); + answerSpy.mockRestore(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.test.ts` around lines 199 - 350, Add a negative test that ensures mediaSessionInstance.answerCall is NOT called when the incoming notification's signedContractId does not match the local device id: spy on mediaSessionInstance.answerCall, mock getUniqueIdSync to return a different id than the notification.signedContractId, set up call store state (using mockUseCallStoreGetState) with nativeAcceptedCallId matching the signal, init mediaSessionInstance, invoke getStreamNotifyHandler() with an 'accepted' notification whose signedContractId != getUniqueIdSync(), await the microtask, and assert answerCall was not called; restore all spies/mocks afterwards.
🤖 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/voip/MediaSessionInstance.test.ts`:
- Around line 101-109: The current getStreamNotifyHandler helper picks the last
registered onStreamData handler which is brittle; update getStreamNotifyHandler
to scan mockOnStreamData.mock.calls (typed as [string, (m: IDDPMessage) =>
void][]) and find the call whose first element equals the event name
"stream-notify-user", then extract and return its handler (throw if not found or
not a function) so the test resolves the correct handler by event name instead
of the last registration.
---
Nitpick comments:
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 199-350: Add a negative test that ensures
mediaSessionInstance.answerCall is NOT called when the incoming notification's
signedContractId does not match the local device id: spy on
mediaSessionInstance.answerCall, mock getUniqueIdSync to return a different id
than the notification.signedContractId, set up call store state (using
mockUseCallStoreGetState) with nativeAcceptedCallId matching the signal, init
mediaSessionInstance, invoke getStreamNotifyHandler() with an 'accepted'
notification whose signedContractId != getUniqueIdSync(), await the microtask,
and assert answerCall was not called; restore all spies/mocks afterwards.
🪄 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: da339f73-b3f8-4eef-9101-f6ee07c12a35
📒 Files selected for processing (1)
app/lib/services/voip/MediaSessionInstance.test.ts
📜 Review details
🔇 Additional comments (1)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
68-99: Nice hardening ofMediaSignalingSessiontest doubles.Tracking
createdSessionsand makingsessionIdnon-writable improves assertion quality and keeps the mock behavior closer to real session semantics.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Contributor
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 `@app/lib/services/voip/useCallStore.ts`:
- Around line 44-49: The timer callback reads the mutable module variable
staleNativeScheduledId at execution time which can race with re-arming; when
scheduling the timeout, capture the current id into a local const (e.g.
scheduledId) and close over that value inside the setTimeout callback instead of
reading staleNativeScheduledId there, then use that captured scheduledId when
calling clearStaleNativeIfStillUnbound(get, scheduledId) and when nulling the
module variable to avoid acting on a re-armed id; update the code around
staleNativeTimer, staleNativeScheduledId, and clearStaleNativeIfStillUnbound to
use the captured scheduledId in the closure.
🪄 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: 7254e343-b0f3-47a9-bd34-26f6b5b53581
📒 Files selected for processing (3)
app/containers/MediaCallHeader/MediaCallHeader.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/useCallStore.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- app/containers/MediaCallHeader/MediaCallHeader.test.tsx
- app/lib/services/voip/MediaSessionInstance.test.ts
📜 Review details
🔇 Additional comments (1)
app/lib/services/voip/useCallStore.ts (1)
265-276: PreservingnativeAcceptedCallIdacrossreset()looks right.Keeping the sticky native id while clearing the transient call UI, then re-arming the stale timer, matches the new accept-success contract cleanly.
diegolmello
force-pushed
the
feat.answer-registered
branch
from
ff24c4c to
38ab95f
Compare
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_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/lib/services/voip/useCallStore.ts (1)
44-49:⚠️ Potential issue | 🟠 MajorStill capture
scheduledIdinside this timeout.Line 46 still reads the mutable
staleNativeScheduledIdat fire time. If an older timeout runs after a re-arm, it can clear the newer native-accepted id instead of the one it was scheduled for. Close overscheduledIdand only null the module slot when it still matches that captured value; please keep a fake-timer re-arm regression with the fix.Suggested fix
staleNativeScheduledId = scheduledId; staleNativeTimer = setTimeout(() => { staleNativeTimer = null; - const scheduled = staleNativeScheduledId; - staleNativeScheduledId = null; - if (scheduled != null) { - clearStaleNativeIfStillUnbound(get, scheduled); - } + if (staleNativeScheduledId === scheduledId) { + staleNativeScheduledId = null; + } + clearStaleNativeIfStillUnbound(get, scheduledId); }, STALE_NATIVE_MS); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/useCallStore.ts` around lines 44 - 49, The timeout callback reads the mutable module variable staleNativeScheduledId at fire time which can allow an older timer to clear a newer scheduled id; to fix, capture the current scheduled id into a local (e.g., scheduledId) before creating the timeout and close over that captured value inside the setTimeout callback, then only null staleNativeScheduledId if it still equals the captured scheduledId and call clearStaleNativeIfStillUnbound(get, capturedScheduledId) (keeping use of get); ensure the change preserves the fake-timer re-arm regression test behavior.
🧹 Nitpick comments (1)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
200-316: Add explicit mismatch cases for the new gate.This suite covers the happy path,
null, and “call already bound”, but it still won't fail if the implementation stops checkingsignedContractIdornativeAcceptedCallId !== signal.callId. A negative test for each mismatch would lock down the contract this PR is introducing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.test.ts` around lines 200 - 316, Add explicit negative tests to lock the new gating logic: add one test that sets mockUseCallStoreGetState to return nativeAcceptedCallId that does NOT match the incoming signal.callId and assert mediaSessionInstance.answerCall is not called, and another test that sets nativeAcceptedCallId to match but the incoming signal.signedContractId to a different value and assert answerCall is not called; locate and extend the existing "stream-notify-user (notification/accepted gated)" suite using the same pattern as the other specs (use getStreamNotifyHandler(), mediaSessionInstance.init('user-1'), spy on answerCall, set mockUseCallStoreGetState values for nativeAcceptedCallId/call and craft the signal args with mismatched signedContractId or callId) so the tests fail if signedContractId or nativeAcceptedCallId checks are removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@app/lib/services/voip/useCallStore.ts`:
- Around line 44-49: The timeout callback reads the mutable module variable
staleNativeScheduledId at fire time which can allow an older timer to clear a
newer scheduled id; to fix, capture the current scheduled id into a local (e.g.,
scheduledId) before creating the timeout and close over that captured value
inside the setTimeout callback, then only null staleNativeScheduledId if it
still equals the captured scheduledId and call
clearStaleNativeIfStillUnbound(get, capturedScheduledId) (keeping use of get);
ensure the change preserves the fake-timer re-arm regression test behavior.
---
Nitpick comments:
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 200-316: Add explicit negative tests to lock the new gating logic:
add one test that sets mockUseCallStoreGetState to return nativeAcceptedCallId
that does NOT match the incoming signal.callId and assert
mediaSessionInstance.answerCall is not called, and another test that sets
nativeAcceptedCallId to match but the incoming signal.signedContractId to a
different value and assert answerCall is not called; locate and extend the
existing "stream-notify-user (notification/accepted gated)" suite using the same
pattern as the other specs (use getStreamNotifyHandler(),
mediaSessionInstance.init('user-1'), spy on answerCall, set
mockUseCallStoreGetState values for nativeAcceptedCallId/call and craft the
signal args with mismatched signedContractId or callId) so the tests fail if
signedContractId or nativeAcceptedCallId checks are removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 985ef5b8-6fa5-4684-97e8-be591c4abbf3
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
app/containers/MediaCallHeader/MediaCallHeader.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/useCallStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/containers/MediaCallHeader/MediaCallHeader.test.tsx
📜 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
diegolmello
force-pushed
the
feat.answer-registered
branch
from
38ab95f to
9619d5c
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_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
experimental_android_build
GitHub Actions
Failure
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/lib/services/voip/useCallStore.ts (1)
248-262: Extract the native teardown cleanup into one shared path.This store action now owns the
callId ?? nativeAcceptedCallIdresolution, butapp/lib/services/voip/MediaSessionInstance.tsandapp/sagas/deepLinking.jsstill inline their ownRNCallKeep.endCall(...)+ reset sequence. Pulling that common cleanup into a shared helper will keep the UUID fallback and native-id cleanup aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/useCallStore.ts` around lines 248 - 262, The native teardown (resolving callId ?? nativeAcceptedCallId, calling RNCallKeep.endCall, and resetting native call id) should be moved into a single exported helper and used by all callers; create a function (e.g., export function endNativeCallCleanup(callId?: string, nativeAcceptedCallId?: string) or as a method on the call store like endNativeCall) that performs the UUID resolution, calls RNCallKeep.endCall(uuid) if present, and invokes resetNativeCallId(), then update useCallStore.endCall to call that helper and replace the inline RNCallKeep.endCall + resetNativeCallId sequences in MediaSessionInstance.ts and app/sagas/deepLinking.js to call the new shared helper instead.app/lib/services/voip/MediaSessionInstance.test.ts (1)
200-316: Add negative coverage for the other accept guards.These tests cover the happy path,
nativeAcceptedCallId === null, andcall != null, but they never assert that mismatchedsignal.callIdor mismatchedsignedContractIdstay blocked. A regression tonativeAcceptedCallId != nullor to ignoring the device contract would still pass this suite.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.test.ts` around lines 200 - 316, Add two negative tests to cover the remaining guards: create tests using mediaSessionInstance.init('user-1') and getStreamNotifyHandler() that (1) set mockUseCallStoreGetState to return nativeAcceptedCallId = 'X' but send a signal with callId = 'Y' and assert mediaSessionInstance.answerCall is NOT called, and (2) set nativeAcceptedCallId to match but send a signal whose signedContractId != 'test-device-id' and assert answerCall is NOT called; use the same spying/restore pattern on answerCall and the existing mock call store shape so the new tests mirror the existing ones (referencing mediaSessionInstance, getStreamNotifyHandler, mockUseCallStoreGetState, nativeAcceptedCallId, signedContractId, callId).
🤖 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/voip/useCallStore.ts`:
- Around line 114-118: The setter setNativeAcceptedCallId currently persists
empty strings before starting timers, causing a sticky nativeAcceptedCallId that
survives reset; change setNativeAcceptedCallId to normalize the incoming callId
(treat '' as null/undefined or omit setting) before calling set, so that
cancelStaleNativeTimer(), set({ nativeAcceptedCallId: ... }), and
createStaleNativeTimer(get) only operate on valid IDs; reference
setNativeAcceptedCallId, cancelStaleNativeTimer, createStaleNativeTimer, reset,
and nativeAcceptedCallId when locating where to normalize the value.
---
Nitpick comments:
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 200-316: Add two negative tests to cover the remaining guards:
create tests using mediaSessionInstance.init('user-1') and
getStreamNotifyHandler() that (1) set mockUseCallStoreGetState to return
nativeAcceptedCallId = 'X' but send a signal with callId = 'Y' and assert
mediaSessionInstance.answerCall is NOT called, and (2) set nativeAcceptedCallId
to match but send a signal whose signedContractId != 'test-device-id' and assert
answerCall is NOT called; use the same spying/restore pattern on answerCall and
the existing mock call store shape so the new tests mirror the existing ones
(referencing mediaSessionInstance, getStreamNotifyHandler,
mockUseCallStoreGetState, nativeAcceptedCallId, signedContractId, callId).
In `@app/lib/services/voip/useCallStore.ts`:
- Around line 248-262: The native teardown (resolving callId ??
nativeAcceptedCallId, calling RNCallKeep.endCall, and resetting native call id)
should be moved into a single exported helper and used by all callers; create a
function (e.g., export function endNativeCallCleanup(callId?: string,
nativeAcceptedCallId?: string) or as a method on the call store like
endNativeCall) that performs the UUID resolution, calls RNCallKeep.endCall(uuid)
if present, and invokes resetNativeCallId(), then update useCallStore.endCall to
call that helper and replace the inline RNCallKeep.endCall + resetNativeCallId
sequences in MediaSessionInstance.ts and app/sagas/deepLinking.js to call the
new shared helper instead.
🪄 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: 3c736b66-51a8-4236-bacf-27fa930d2cb9
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
app/containers/MediaCallHeader/MediaCallHeader.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/useCallStore.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/containers/MediaCallHeader/MediaCallHeader.test.tsx
📜 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 (2)
app/lib/services/voip/useCallStore.ts (1)
37-51: CapturedscheduledIdin the timeout closure is the right fix.The callback now only clears state for the timer that actually fired, so re-arming cannot accidentally act on a newer native id.
app/lib/services/voip/MediaSessionInstance.test.ts (1)
102-111: Handler lookup is much less brittle now.Resolving
stream-notify-userby event name instead of “last registration wins” will keep these tests stable if other listeners are added later.
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>
3 tasks
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
1 participant
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
This branch builds on
feat.voip-lib-newto align native VoIP “accept succeeded” handling across iOS and Android, and to separate “native already accepted this call” from transient UI /callIdstate in JS.VoipAcceptSucceededinstead ofVoipPushInitialEvents(AndroidVoipModule, iOSVoipModulesupported events + notification observers). JS listens on one path for both platforms.MediaCallEvents: On success, clears initial events, records the call viasetNativeAcceptedCallId, and opens the deep link — with deduplication so warm replays do not double-handle the samecallId.useCallStore: AddsnativeAcceptedCallId(kept acrossreset()),setNativeAcceptedCallId/resetNativeCallId, and a stale-native timer so an orphan native-accepted id does not stick around forever if JS never completes setup.MediaSessionInstance:notification/acceptedonstream-notify-useronly triggersanswerCallwhen the signal matchesnativeAcceptedCallId(and device contract), avoidinganswerCallfrom the old Android “initial events” flow.VoipNotification: Adjusts the accept-timeoutRunnableso it is posted only afterfinishexists and cancellation/removal stays correct.useCallStore,MediaSessionInstancegating, andMediaCallHeaderwhen only a native-accepted id is present beforeanswerCallcompletes.Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-62
How to test or reproduce
Automated
Or run the full suite:
TZ=UTC yarn testManual — Android
VoipAcceptFailedstill surfaces and recovery behaves as before.VoipNotification).Manual — iOS
VoipAcceptSucceededis received once JS is up, deep link opens, and media sessionanswerCallruns only whennotification/acceptedmatchesnativeAcceptedCallId.VoipPushTokenRegistered, etc.).UI sanity
answerCallhas not finished,MediaCallHeadershould show the empty placeholder (see unit test) rather than a broken full header.Screenshots
Add screenshots or screen recordings for cold/warm answer on iOS + Android if helpful.
Types of changes
Checklist
Further comments
feat.voip-lib-new(notdevelop) until that line is merged upstream.VoipPushInitialEventsfor success must useVoipAcceptSucceededinstead.Summary by CodeRabbit
Bug Fixes
New Features
Tests