feat(voip): call waiting, busy detection, and videoconf blocking#7077
Open
diegolmello wants to merge 25 commits intofeat.voip-lib-newfrom
Open
feat(voip): call waiting, busy detection, and videoconf blocking#7077diegolmello wants to merge 25 commits intofeat.voip-lib-newfrom
diegolmello wants to merge 25 commits intofeat.voip-lib-newfrom
Conversation
Check permission before TelecomManager.isInCall on API 26+, catch SecurityException, and use AudioManager MODE_IN_COMMUNICATION fallback on all API levels when Telecom is unavailable or denied. Made-with: Cursor
Add requestPhoneStatePermission() with session-scoped prompt, i18n rationale, and Jest coverage. English strings only per issue scope. Made-with: Cursor
Call requestPhoneStatePermission() at the start of MediaSessionInstance.startCall() so Android can use TelecomManager for busy detection. Videoconf is unchanged. Tests: extend MediaSessionInstance.test with startCall permission assertion. Made-with: Cursor
…VoIP Align native busy detection with iOS (non-ended calls): reject a second incoming push while the first is still ringing or dialing, not only active/hold. Made-with: Cursor
When the user is already in a call, still satisfy PushKit by reporting to CallKit then rejecting, but do not store the payload for getInitialEvents. Keeps DDP listener/timeout so the reject signal can be sent. Clear matching initial events in rejectBusyCall as a safeguard. Made-with: Cursor
Added a new function to determine if incoming videoconference calls should be blocked based on active VoIP calls or pending native accept states. Updated the videoConf saga to utilize this new logic. Additionally, created unit tests for the new functionality to ensure correct behavior in various scenarios.
Track incoming calls by UUID in a dictionary so CXCallObserver can handle simultaneous calls without overwriting state. Do not clear that map when stopping the native DDP client. Set supportsHolding to false on reportNewIncomingCall so CallKit shows End & Accept / Decline without Hold & Accept. Made-with: Cursor
Added new tests to validate the behavior of incoming and outgoing calls in the MediaSessionInstance. Implemented mock functions for RNCallKeep and introduced utility functions to build client media calls. The tests cover scenarios for handling new calls based on the current call state and native accepted call ID.
Export handleVoipAcceptSucceededFromNative and dispatchVoipAcceptFailureFromNative for spec coverage. Add tests for VoipAcceptSucceeded (incoming deep link + nativeAcceptedCallId, dedupe), accept failure dispatch, and getInitialMediaCallEvents cold paths (voipAcceptFailed stash and Android answered bootstrap). Made-with: Cursor
Test VoipAcceptSucceeded/VoipAcceptFailed through setupMediaCallEvents and DeviceEventEmitter instead of exporting internal dispatch helpers. Made-with: Cursor
- Share setupMediaCallEvents hooks for VoipAcceptSucceeded/Failed tests - Document per-callId dedupe semantics for call-waiting in VoipService Made-with: Cursor
…ter rebase - AppDelegate: always prepareIncomingCall with storeEventsForJs true; no hasActiveCall/rejectBusyCall - MediaSessionInstance: remove single-call JS busy guard; keep requestPhoneStatePermission in startCall Made-with: Cursor
…tests Document why hasActiveCall() and rejectBusyCall() are not used from AppDelegate during CallKit call-waiting, and type voipBlocksIncomingVideoconf tests with IClientMediaCall instead of any. Made-with: Cursor
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:
WalkthroughAdds call-waiting and busy-detection: Android and iOS now detect active calls to reject incoming VoIP calls, Android prompts READ_PHONE_STATE once per session, videoconf direct-call handling is blocked during active VoIP calls, and tests plus i18n entries were added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client App
participant AndroidNotif as VoipNotification<br/>(Android)
participant AudioMgr as AudioManager / Telecom
participant DDP as DDP Connection
participant VoipSvc as VoipService<br/>(iOS)
rect rgba(0, 100, 200, 0.5)
note over Client,AndroidNotif: Incoming VoIP call (Android)
AndroidNotif->>AndroidNotif: onMessageReceived(payload)
AndroidNotif->>AndroidNotif: hasActiveCall(context)
AndroidNotif->>AudioMgr: check MODE_IN_COMMUNICATION
AndroidNotif->>AudioMgr: query Telecom (API26+, if permission)
alt Active call detected
AndroidNotif->>AndroidNotif: rejectBusyCall(payload)
AndroidNotif->>AndroidNotif: cancel timeout, start DDP listener
alt DDP logged in
AndroidNotif->>DDP: send reject signal
else
AndroidNotif->>AndroidNotif: queue reject signal
end
else
AndroidNotif->>Client: showIncomingCall(payload)
end
end
rect rgba(0, 100, 200, 0.5)
note over VoipSvc,DDP: Incoming VoIP call (iOS)
VoipSvc->>VoipSvc: hasActiveCall() via CXCallObserver
alt Active call exists
VoipSvc->>VoipSvc: rejectBusyCall(payload)
VoipSvc->>Client: end CallKit call immediately
VoipSvc->>DDP: send/queue native reject
else
VoipSvc->>VoipSvc: track incoming call by UUID
end
end
rect rgba(0, 100, 200, 0.5)
note over Client: VideoConf blocking check
Client->>Client: onDirectCall saga
Client->>Client: voipBlocksIncomingVideoconf()
alt VoIP active
Client->>Client: skip videoConf handling
else
Client->>Client: process videoConf normally
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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. 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 |
Add Phone_state_permission_title and Phone_state_permission_message translations for all languages registered in app/i18n/index.ts. Made-with: Cursor
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 684-689: The stale-push check must run before the busy-path so
delayed pushes are ignored rather than marked busy: in the VoipNotification
handling where voipPayload.isVoipIncomingCall() is matched, call the
stale-lifetime guard (the logic that uses
voipPayload.getRemainingLifetimeMs()/isExpired()) first and return/ignore if
expired, and only after that check call hasActiveCall(context) to decide between
rejectBusyCall(context, voipPayload) and showIncomingCall(voipPayload); update
the order of those calls accordingly.
- Around line 537-545: rejectBusyCall currently calls startListeningForCallEnd
first, but startListeningForCallEnd recreates the ddpClient and resets
isDdpLoggedIn which breaks cleanup for the older ringing call; to fix, ensure
the reject signal is sent or queued before mutating the DDP client state—move
the sendRejectSignal / queueRejectSignal logic to run prior to calling
startListeningForCallEnd in rejectBusyCall, or alternatively change
startListeningForCallEnd so it does not recreate ddpClient/reset isDdpLoggedIn
(preserve existing ddpClient and login flag) when handling a new incoming call;
reference functions: rejectBusyCall, startListeningForCallEnd, sendRejectSignal,
queueRejectSignal, and symbols ddpClient/isDdpLoggedIn.
- Around line 510-529: The hasSystemLevelActiveCallIndicators function currently
falls back to checking only AudioManager.MODE_IN_COMMUNICATION when
READ_PHONE_STATE is unavailable or on Android < O; update the fallback to also
consider AudioManager.MODE_IN_CALL so regular cellular calls are detected. In
practice, modify the AudioManager check in hasSystemLevelActiveCallIndicators to
return true when audio?.mode == AudioManager.MODE_IN_COMMUNICATION ||
audio?.mode == AudioManager.MODE_IN_CALL, keeping the existing
TelecomManager.isInCall branch unchanged.
In `@app/lib/methods/voipPhoneStatePermission.test.ts`:
- Around line 10-12: Replace the forbidden import() type annotations used in the
jest.requireActual generic (e.g. jest.requireActual<typeof
import('./helpers')>('./helpers')) by removing the import() generic and letting
TypeScript infer the type, or by using an explicit type alias via an import type
statement (e.g. import type { ... } from './helpers') and applying that named
type instead; update the mocks in the jest.doMock calls (the instances wrapping
'./helpers' at the shown locations and the other occurrences around lines 28-30
and 50-52) to stop using typeof import('./helpers') and use either plain
jest.requireActual('./helpers') or
jest.requireActual<YourImportedType>('./helpers') after adding an import type.
In `@app/lib/methods/voipPhoneStatePermission.ts`:
- Around line 17-20: Replace the hardcoded English positive button label in the
PermissionsAndroid.request call by using the i18n translation function instead
of 'Ok' (update the buttonPositive option in voipPhoneStatePermission.ts / the
PermissionsAndroid.request invocation); add or reuse an appropriate i18n key
such as 'Phone_state_permission_ok' (or reuse an existing generic 'ok' key) so
the positive button label is localized consistently with message/title.
In `@app/lib/services/voip/MediaCallEvents.test.ts`:
- Around line 51-54: The three imports (RNCallKeep, NativeVoipModule, and the
named imports getInitialMediaCallEvents and setupMediaCallEvents) must be moved
to the top of the module before any jest.mock(...) calls to satisfy the
import/first ESLint rule; relocate the lines "import RNCallKeep from
'react-native-callkeep';", "import NativeVoipModule from
'../../native/NativeVoip';" and "import { getInitialMediaCallEvents,
setupMediaCallEvents } from './MediaCallEvents';" to the very top of
MediaCallEvents.test.ts so they appear before the jest.mock blocks.
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 1-4: The imports are mis-grouped causing import/order lint
failures: move external module imports (e.g., react-native-callkeep ->
RNCallKeep and '@rocket.chat/media-signaling' -> IClientMediaCall) together at
the top, followed by third-party relative imports (if any), and then local
project imports (e.g., Navigation and IDDPMessage). Update the import order in
MediaSessionInstance.test.ts so RNCallKeep is not separated by local imports and
ensure the external imports appear before the local '../../' imports.
In `@ios/Libraries/VoipService.swift`:
- Around line 43-59: The code currently uses a single global ddpClient /
isDdpLoggedIn for all incoming calls causing cross-call/socket reuse; change to
per-call (or per-host) DDP ownership: add a mapping (e.g., callIdToDdpClient or
hostToDdpClient) and store the DDP client and login state for each incoming call
when it is created, update handleNativeAccept, handleNativeReject, the DDP
stream listener registration/cleanup, and any call-end handlers to use the
per-call/per-host client instead of the global ddpClient/isDdpLoggedIn, and
ensure you create/login that client before sending accept/reject signals and
remove/close the client and its mapping when the call finishes or on timeout
(also update nativeAcceptHandledCallIds and observedIncomingCalls cleanup to use
the per-call client lifecycle).
🪄 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: db5d6ba2-7849-49d7-80c1-c9237b9d2b56
📒 Files selected for processing (12)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.ktapp/i18n/locales/en.jsonapp/lib/methods/voipPhoneStatePermission.test.tsapp/lib/methods/voipPhoneStatePermission.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/voipBlocksIncomingVideoconf.test.tsapp/lib/services/voip/voipBlocksIncomingVideoconf.tsapp/sagas/videoConf.tsios/Libraries/AppDelegate+Voip.swiftios/Libraries/VoipService.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-30T15:49:30.957Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: app/containers/RoomItem/Actions.tsx:12-12
Timestamp: 2026-03-30T15:49:30.957Z
Learning: In RocketChat/Rocket.Chat.ReactNative, `react-native-worklets` version 0.6.1 does NOT export a built-in Jest mock (e.g., no `react-native-worklets/lib/module/mock`). The correct Jest mock approach for this version is to add a manual mock in `jest.setup.js`: `jest.mock('react-native-worklets', () => ({ scheduleOnRN: jest.fn((fn, ...args) => fn(...args)) }))`.
Applied to files:
app/lib/services/voip/MediaSessionInstance.test.ts
🪛 ESLint
app/lib/methods/voipPhoneStatePermission.test.ts
[error] 11-11: import() type annotations are forbidden.
(@typescript-eslint/consistent-type-imports)
[error] 29-29: import() type annotations are forbidden.
(@typescript-eslint/consistent-type-imports)
[error] 51-51: import() type annotations are forbidden.
(@typescript-eslint/consistent-type-imports)
app/lib/services/voip/MediaSessionInstance.test.ts
[error] 1-1: There should be at least one empty line between import groups
(import/order)
[error] 3-3: There should be at least one empty line between import groups
(import/order)
[error] 4-4: There should be at least one empty line between import groups
(import/order)
[error] 4-4: react-native-callkeep import should occur before type import of ../../../definitions/IDDPMessage
(import/order)
app/lib/services/voip/MediaCallEvents.test.ts
[error] 51-51: Import in body of module; reorder to top.
(import/first)
[error] 51-51: react-native-callkeep import should occur before import of ../../../actions/actionsTypes
(import/order)
[error] 53-53: Import in body of module; reorder to top.
(import/first)
[error] 54-54: Import in body of module; reorder to top.
(import/first)
🪛 GitHub Actions: Format Code with Prettier
app/lib/methods/voipPhoneStatePermission.test.ts
[error] 11-11: @typescript-eslint/consistent-type-imports: import() type annotations are forbidden
🪛 GitHub Check: format
app/lib/methods/voipPhoneStatePermission.test.ts
[failure] 51-51:
import() type annotations are forbidden
[failure] 29-29:
import() type annotations are forbidden
[failure] 11-11:
import() type annotations are forbidden
🔇 Additional comments (4)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
244-311: Nice coverage for the new waiting-call and start-call paths.These cases pin the two regressions most likely here: not JS-rejecting waiting incoming calls and keeping the phone-state permission request fire-and-forget.
Also applies to: 431-439
app/lib/services/voip/MediaCallEvents.test.ts (1)
69-223: Great coverage for duplicate native delivery and cold-start recovery.The new cases lock down the most failure-prone paths here: idempotent native events and hydrating the correct deep link from initial VoIP state.
ios/Libraries/AppDelegate+Voip.swift (1)
43-57: This CallKit capability change looks right.Making the default
prepareIncomingCall(..., storeEventsForJs: true)path explicit and disabling hold keeps iOS from advertising an action the app still doesn't support.ios/Libraries/VoipService.swift (1)
151-160: ThestoreEventsForJsswitch is a good safeguard.Skipping
storeInitialEventsfor calls that will be auto-rejected is the right way to avoid feeding JS a fake cold-start incoming ring.
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
Show resolved
Hide resolved
android/app/src/main/java/chat/rocket/reactnative/voip/VoipNotification.kt
Show resolved
Hide resolved
- Drop typeof import() from jest.requireActual in voipPhoneStatePermission tests - Move imports to top of MediaCallEvents.test.ts for import/first and import/order - Reorder imports in MediaSessionInstance.test.ts for import/order Made-with: Cursor
10 tasks
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
…ction) Listen for didToggleHoldCallAction outside the iOS-only block so Android Telecom and iOS CallKit both drive hold/unhold. Use a closure flag to auto-resume only after OS-initiated hold, not manual hold. Reset bookkeeping when there is no active media call or callUUID does not match callId/nativeAcceptedCallId so workspace or call changes cannot leave stale wasAutoHeld state. Tests mock RNCallKeep and assert toggleHold behavior and cleanup. Made-with: Cursor
diegolmello
force-pushed
the
feat.call-waiting
branch
from
4e3dc62 to
ab85a8f
Compare
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 (3)
app/lib/services/voip/MediaCallEvents.test.ts (2)
257-262: Minor: redundantmockClear()call.
jest.clearAllMocks()on line 258 already clearstoggleHold, so line 259 is redundant.♻️ Suggested cleanup
beforeEach(() => { jest.clearAllMocks(); - toggleHold.mockClear(); mockAddEventListener.mockImplementation(() => ({ remove: jest.fn() })); getState.mockReturnValue({ ...activeCallBase, isOnHold: false, toggleHold }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaCallEvents.test.ts` around lines 257 - 262, The beforeEach setup redundantly calls toggleHold.mockClear() after jest.clearAllMocks(); remove the explicit toggleHold.mockClear() call so the beforeEach only calls jest.clearAllMocks(), mockAddEventListener.mockImplementation(...), and getState.mockReturnValue(...); this keeps toggleHold cleared (via jest.clearAllMocks()) and eliminates the duplicate mockClear invocation.
283-299: Missing test for manual-resume-between-OS-events scenario.There's no test covering the case where:
- OS puts call on hold (
hold: true)- User manually resumes via UI (state changes but
wasAutoHeldstaystrue)- OS sends
hold: false- Expected: no toggle (call is already resumed)
This would help catch the issue flagged in
MediaCallEvents.tswheretoggleHold()is called without checkingisOnHoldon the resume path.🧪 Suggested test case
it('hold: false after OS-initiated hold but user already resumed does not toggle again', () => { setupMediaCallEvents(); const handler = getToggleHoldHandler(); // OS puts on hold handler({ hold: true, callUUID: 'uuid-1' }); expect(toggleHold).toHaveBeenCalledTimes(1); // User manually resumes (isOnHold becomes false, but wasAutoHeld is still true in closure) getState.mockReturnValue({ ...activeCallBase, isOnHold: false, toggleHold }); // OS sends hold: false handler({ hold: false, callUUID: 'uuid-1' }); // Should NOT toggle again since call is already active expect(toggleHold).toHaveBeenCalledTimes(1); expect(mockSetCurrentCallActive).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaCallEvents.test.ts` around lines 283 - 299, Add a unit test in MediaCallEvents.test.ts to cover the manual-resume-between-OS-events scenario: use setupMediaCallEvents() and getToggleHoldHandler() to simulate an OS-initiated hold (handler({ hold: true, callUUID: 'uuid-1' })) asserting toggleHold was called once, then mock getState to return { ...activeCallBase, isOnHold: false, toggleHold } to simulate the user manually resuming the call (wasAutoHeld remains true in closure) and finally call the handler with { hold: false, callUUID: 'uuid-1' } and assert toggleHold was not called again and mockSetCurrentCallActive was not called; this ensures the toggleHold/resume path in MediaCallEvents (the toggleHold handler logic) checks current isOnHold before toggling.ios/Libraries/VoipService.swift (1)
538-565: Document the implicit dependency on priorprepareIncomingCallcall.
rejectBusyCallrelies onddpClientbeing set up by a priorprepareIncomingCallcall. If called without that setup, the reject signal is silently dropped (lines 568-572, 588-592 guard onddpClient). This differs from Android'srejectBusyCallwhich is self-contained by callingstartListeningForCallEnd()first.Consider adding a precondition note to the docstring:
📝 Suggested docstring clarification
/// Rejects an incoming call because the user is already on another call. /// Must be called **after** `reportNewIncomingCall` (PushKit requirement). + /// Requires a prior `prepareIncomingCall(_:storeEventsForJs:)` call to set up the DDP client; + /// otherwise the reject signal will be silently dropped. /// /// **Call-waiting:** `AppDelegate+Voip` does **not** invoke this; second rings are shown in CallKit🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/VoipService.swift` around lines 538 - 565, Update the rejectBusyCall docstring to explicitly state it requires prior prepareIncomingCall (which sets up ddpClient) so that sendRejectSignal won’t be silently dropped; mention the difference from Android's self-contained startListeningForCallEnd() flow and note that callers should either call prepareIncomingCall first or ensure ddpClient is initialized (or handle queued rejects via queueRejectSignal). Reference the relevant symbols: rejectBusyCall, prepareIncomingCall, ddpClient, sendRejectSignal, queueRejectSignal, and startListeningForCallEnd() in the note.
🤖 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/MediaCallEvents.ts`:
- Around line 120-124: The resume path should avoid flipping a user-resumed call
back to hold: update the block that checks wasAutoHeld to first verify isOnHold
is true before calling toggleHold(); if isOnHold is already false, skip
toggleHold() but still clear wasAutoHeld and call
RNCallKeep.setCurrentCallActive(callUUID) as needed. Specifically modify the
wasAutoHeld handling (references: wasAutoHeld, isOnHold, toggleHold(),
RNCallKeep.setCurrentCallActive(callUUID)) to guard toggleHold() with an
isOnHold check and always reset wasAutoHeld.
---
Nitpick comments:
In `@app/lib/services/voip/MediaCallEvents.test.ts`:
- Around line 257-262: The beforeEach setup redundantly calls
toggleHold.mockClear() after jest.clearAllMocks(); remove the explicit
toggleHold.mockClear() call so the beforeEach only calls jest.clearAllMocks(),
mockAddEventListener.mockImplementation(...), and getState.mockReturnValue(...);
this keeps toggleHold cleared (via jest.clearAllMocks()) and eliminates the
duplicate mockClear invocation.
- Around line 283-299: Add a unit test in MediaCallEvents.test.ts to cover the
manual-resume-between-OS-events scenario: use setupMediaCallEvents() and
getToggleHoldHandler() to simulate an OS-initiated hold (handler({ hold: true,
callUUID: 'uuid-1' })) asserting toggleHold was called once, then mock getState
to return { ...activeCallBase, isOnHold: false, toggleHold } to simulate the
user manually resuming the call (wasAutoHeld remains true in closure) and
finally call the handler with { hold: false, callUUID: 'uuid-1' } and assert
toggleHold was not called again and mockSetCurrentCallActive was not called;
this ensures the toggleHold/resume path in MediaCallEvents (the toggleHold
handler logic) checks current isOnHold before toggling.
In `@ios/Libraries/VoipService.swift`:
- Around line 538-565: Update the rejectBusyCall docstring to explicitly state
it requires prior prepareIncomingCall (which sets up ddpClient) so that
sendRejectSignal won’t be silently dropped; mention the difference from
Android's self-contained startListeningForCallEnd() flow and note that callers
should either call prepareIncomingCall first or ensure ddpClient is initialized
(or handle queued rejects via queueRejectSignal). Reference the relevant
symbols: rejectBusyCall, prepareIncomingCall, ddpClient, sendRejectSignal,
queueRejectSignal, and startListeningForCallEnd() in the note.
🪄 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: 3860bccc-8a58-4ea2-a1ee-3fd01d1940c4
📒 Files selected for processing (4)
app/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsios/Libraries/AppDelegate+Voip.swiftios/Libraries/VoipService.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- ios/Libraries/AppDelegate+Voip.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 (8)
app/lib/services/voip/MediaCallEvents.ts (1)
98-118: Guard logic and hold=true path look correct.The stale-event guard (lines 106-111) properly resets
wasAutoHeldon callUUID mismatch, and the hold path (lines 113-118) correctly checks!isOnHoldbefore invokingtoggleHold()to avoid redundant toggles.app/lib/services/voip/MediaCallEvents.test.ts (2)
90-251: Test coverage for VoIP accept/reject flows is thorough.The suite properly validates:
VoipAcceptSucceededdeep-linking dispatch with correct payload shape- Deduplication for same
callId- Non-incoming-call type filtering
VoipAcceptFaileddispatch withvoipAcceptFailed: true- Cold-start initial-event handling for both failure and success paths
264-355: Hold-action tests comprehensively cover the listener registration and state transitions.Good coverage for:
- Listener registration
- Guard against no active call
- Guard against mismatched callUUID
- Deduplication of consecutive hold=true events
- Cleanup via
remove()ios/Libraries/VoipService.swift (5)
43-59: Per-call tracking structure looks correct.The migration from a single optional slot to a UUID-keyed dictionary properly enables concurrent call tracking. The deduplication guard per
callIdand lifecycle documentation are clear.
140-149:hasActiveCall()implementation is correct; intentional non-use is well-documented.The method properly configures the observer and checks for any non-ended call. The docstring clearly explains why it exists despite not being called from the PushKit path (Android parity, future use).
154-160: Conditional event stashing is correct.The
storeEventsForJsparameter properly prevents auto-rejected calls from appearing as fresh incoming sessions in JS. The timeout and DDP listener are always set up, which is necessary sincerejectBusyCalldepends on the DDP client being initialized.
625-648: Per-call tracking helpers are correctly implemented.Both methods properly dispatch to main thread when needed and use the UUID key consistently with the map structure.
650-674:handleObservedCallChangedcorrectly uses per-call lookup.The method properly looks up the tracked call by UUID and handles both connection (triggering native accept) and end (triggering reject signal) cases. Removing from the map before calling
handleNativeAcceptis safe since the payload is passed directly.
Slice 1: Add Ok i18n key and use i18n.t for READ_PHONE_STATE rationale button. Slice 2: Evaluate incoming push lifetime/expiry before hasActiveCall so stale pushes do not call rejectBusyCall or showIncomingCall; extract pure dispatch helper with JUnit tests; add junit test dependency. Made-with: Cursor
Replace singleton DDP client with VoipPerCallDdpRegistry keyed by callId so busy reject and second incoming call do not disconnect the first call's listener. Add isLiveClient guards for stale callbacks; stopClient(callId) and stopAllClients for scoped and full teardown. Add unit tests for registry isolation and logged-in state.
diegolmello
force-pushed
the
feat.call-waiting
branch
from
e7c1d5c to
6bebf15
Compare
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
Include MODE_IN_CALL alongside MODE_IN_COMMUNICATION in the hasSystemLevelActiveCallIndicators fallback so that active cellular calls are also detected when READ_PHONE_STATE is unavailable or the device runs Android < O.
If the user manually resumes a call between an OS-initiated hold and the subsequent hold:false event, wasAutoHeld remains true but isOnHold is already false. Without the guard, toggleHold() would flip the call back to held. Now toggleHold/setCurrentCallActive are only called when the call is still on hold; wasAutoHeld is always reset.
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_ios_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
experimental_android_build
GitHub Actions
Waiting
diegolmello
requested a deployment
to
official_ios_build
GitHub Actions
Waiting
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 support call waiting and busy behavior for VoIP, and to avoid conflicting direct videoconf “calls” while VoIP is active.iOS (CallKit / VoipService)
CXCallObserverso multiple incoming VoIP calls can be handled (call-waiting style), with hold disabled on reported calls (supportsHolding: falseinAppDelegate+Voip).callIddeduplication for native accept DDP signaling so repeated observer callbacks do not double-accept the same call.Android
READ_PHONE_STATE: runtime request helper and call fromMediaSessionInstance.startCallso Telecom “in call” checks can run when the OS allows.isInCallwhen permission is missing; reject busy incoming VoIP pushes with logging aligned to existing behavior.JS / product behavior
voipBlocksIncomingVideoconf: if a VoIP call is active or a native-accepted call id is set, incoming direct videoconf call handling invideoConfsaga no-ops, avoiding duplicate/conflicting call UIs.MediaCallEvents: keep native handler wiring internal; expanded tests for the accept pipeline and related flows;MediaSessionInstancetests extended for new call handling; videoconf blocking unit tests.Issue(s)
https://rocketchat.atlassian.net/browse/VMUX-64
How to test or reproduce
Call waiting (iOS)
With VoIP enabled, place or answer a VoIP call, then receive a second incoming VoIP push. Confirm CallKit shows the second call, hold is not offered as supported, and answering/rejecting behaves correctly without duplicate native accept signals.
Busy incoming (Android)
Start or simulate an active/ringing/dialing VoIP connection, then trigger a second incoming VoIP push. Confirm the busy path rejects the second call as intended and logs clearly.
READ_PHONE_STATEStart a VoIP call from the app; confirm the permission prompt flow (when applicable) and that busy detection improves once granted (API 26+ Telecom path).
Videoconf guard
While a VoIP session is active (or after native accept before JS fully binds), trigger an incoming direct videoconf call; confirm the app does not stack conflicting videoconf call handling.
Regression
Run
TZ=UTC yarn test— includes new/updated tests forMediaCallEvents,MediaSessionInstance,voipPhoneStatePermission, andvoipBlocksIncomingVideoconf/videoConf.Screenshots
Types of changes
Checklist
Further comments
Base: targets improvements relative to
feat.voip-lib-new(notdevelop). If the merge target branch differs, adjust the PR base accordingly.Files touched (high level):
VoipService.swift,AppDelegate+Voip.swift,VoipNotification.kt,MediaSessionInstance.ts,videoConf.ts, newvoipBlocksIncomingVideoconf.ts+voipPhoneStatePermission.ts, i18n strings for permission-related copy, and VoIP/videoconf tests.Summary by CodeRabbit
New Features
Tests