fix(voip): reconnect SDK on lock-screen CallKit native accept#7299
fix(voip): reconnect SDK on lock-screen CallKit native accept#7299diegolmello wants to merge 3 commits into
Conversation
The foreground-reconnect path (PR #7297) doesn't fire when CallKit accepts a call from the lock screen, since the app stays backgrounded and APP_STATE.FOREGROUND never transitions. Trigger checkAndReopen inline from VoipAcceptSucceeded so the zombie DDP socket is torn down before SDP/ICE replays kick off.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📜 Recent 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)
WalkthroughInvoke SDK reconnection when an incoming VoIP accept matches the active workspace: call ChangesVoIP SDK Reconnection on Call Accept
Sequence DiagramsequenceDiagram
participant Native as Native layer (event)
participant Handler as MediaCallEvents handler
participant Connect as SDK (checkAndReopen)
participant Media as mediaSessionInstance
Native->>Handler: VoipAcceptSucceeded(host, callId,...)
Handler->>Handler: verify host matches active workspace
Handler->>Connect: call checkAndReopen()
Connect-->>Handler: resolves (or rejects, logged)
Handler->>Media: applyRestStateSignals(...)
Handler-->>Native: complete handling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Error
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Error
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/lib/services/voip/MediaCallEvents.test.ts (1)
166-225: 💤 Low valueNew reconnect tests are comprehensive and well-structured
All four assertions — triggering on host match, skipping on cross-workspace, deduplication, and invocation order — correctly exercise the added code path via the real
DeviceEventEmitteremitter rather than unit-testing the handler in isolation. The ordering test (lines 205-225) correctly validates synchronous invocation ordering usingmockImplementationOnce, which matches the current fire-and-forget design.One note: the dedupe test at line 191-203 uses
mockServerSelector.mockReturnValue(...)(persistent) rather thanmockReturnValueOnce. Jest'sclearAllMocks()clears usage data (calls, instances, results) but not implementation. Since the value set is identical to the declared default ('https://workspace-a.example.com'), there is no observable cross-test leakage — but prefermockReturnValueOncefor consistency with the other tests in the suite.♻️ Consistency fix for the dedupe test mock
- mockServerSelector.mockReturnValue('https://workspace-a.example.com'); + mockServerSelector.mockReturnValueOnce('https://workspace-a.example.com'); + mockServerSelector.mockReturnValueOnce('https://workspace-a.example.com');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/services/voip/MediaCallEvents.test.ts` around lines 166 - 225, The dedupe test uses mockServerSelector.mockReturnValue(...) which sets a persistent implementation; change it to mockServerSelector.mockReturnValueOnce('https://workspace-a.example.com') in the 'triggers SDK reconnect only once for duplicate VoipAcceptSucceeded with the same callId' test (the one that builds payload with callId 'reopen-dedupe' and emits via DeviceEventEmitter) so the mock matches the pattern used by other tests and avoids persistent test state while still ensuring checkAndReopen is called only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Around line 99-102: checkAndReopen() returns a Promise and currently lacks
error handling and sequencing with applyRestStateSignals; update the block
around mediaSessionInstance so you await checkAndReopen() (or at minimum append
.catch(...) like you do for applyRestStateSignals) and only call
mediaSessionInstance.applyRestStateSignals() after checkAndReopen completes,
handling errors for both with mediaCallLogger.error and TAG; target the
checkAndReopen() call and the mediaSessionInstance.applyRestStateSignals()
invocation to implement the .catch handlers (or make the function async and use
await) so there are no unhandled rejections and the reconnect completes before
signals are replayed.
---
Nitpick comments:
In `@app/lib/services/voip/MediaCallEvents.test.ts`:
- Around line 166-225: The dedupe test uses
mockServerSelector.mockReturnValue(...) which sets a persistent implementation;
change it to
mockServerSelector.mockReturnValueOnce('https://workspace-a.example.com') in the
'triggers SDK reconnect only once for duplicate VoipAcceptSucceeded with the
same callId' test (the one that builds payload with callId 'reopen-dedupe' and
emits via DeviceEventEmitter) so the mock matches the pattern used by other
tests and avoids persistent test state while still ensuring checkAndReopen is
called only once.
🪄 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: 2d666c73-1874-4d94-8983-dfe2839ab3a7
📒 Files selected for processing (3)
app/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ESLint and Test / run-eslint-and-test
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP WebRTC peer-to-peer audio calls with Zustand stores (not Redux) in 'app/lib/services/voip/' with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.ts
🔇 Additional comments (2)
app/lib/services/voip/MediaCallEvents.ios.test.ts (1)
91-93: LGTM — minimal stub correctly prevents module-graph breakageThe
checkAndReopen: jest.fn()stub is all that's needed here; the reconnect-path assertions belong inMediaCallEvents.test.ts(where they live).app/lib/services/voip/MediaCallEvents.test.ts (1)
72-74: LGTM — correct mirror of the iOS test file's connect stub
|
iOS Build Available Rocket.Chat Experimental 4.72.0.108761 |
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat Experimental 4.72.0.108760 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNTgQysT27xV5Qg-Jxf4z__YC67cy2QkaPXFijbiX688QlPj_kIPo9kuHXozpkmbeUO7yG8z2btF5xsNZUMW |
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Around line 99-101: The catch handlers for checkAndReopen() use an implicitly
typed parameter; change the arrow parameter to an explicitly typed catch
parameter (error: unknown) in the MediaCallEvents.ts handlers (the
checkAndReopen().catch(...) callback and the other catch on the adjacent line),
and before passing it to mediaCallLogger.error ensure it's narrowed/serialized
(e.g., inspect or String(error) / JSON.stringify when safe) so the logger
receives a well-typed value rather than an implicit any.
🪄 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: 7e8e5156-d10d-417e-9e08-e19298302c4e
📒 Files selected for processing (3)
app/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
✅ Files skipped from review due to trivial changes (1)
- app/lib/services/voip/MediaCallEvents.ios.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/lib/services/voip/MediaCallEvents.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: format
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/voip/MediaCallEvents.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
Files:
app/lib/services/voip/MediaCallEvents.ts
**/*.{ts,tsx,js,jsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Use Prettier formatting with tabs, single quotes, 130 character width, no trailing commas, avoid arrow parens, and bracket same line
Files:
app/lib/services/voip/MediaCallEvents.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP WebRTC peer-to-peer audio calls with Zustand stores (not Redux) in 'app/lib/services/voip/' with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/MediaCallEvents.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/voip/MediaCallEvents.ts
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/lib/services/voip/MediaCallEvents.ts
[failure] 99-99:
Parameter 'error' implicitly has an 'any' type.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
diegolmello
temporarily deployed
to
experimental_ios_build
GitHub Actions
Inactive
diegolmello
temporarily deployed
to
experimental_android_build
GitHub Actions
Inactive
diegolmello
had a problem deploying
to
official_ios_build
GitHub Actions
Failure
diegolmello
had a problem deploying
to
official_android_build
GitHub Actions
Failure
|
iOS Build Available Rocket.Chat Experimental 4.72.0.108763 |
diegolmello
had a problem deploying
to
upload_experimental_android
GitHub Actions
Failure
|
Android Build Available Rocket.Chat Experimental 4.72.0.108762 Internal App Sharing: https://play.google.com/apps/test/RQVpXLytHNc/ahAO29uNQRS8i_wVfhUJwYwfZN99TzgG4gSBUcwVzW_e5rkEZ1CQHfsNGsQgrzplP-f6imInCfqKgFOXnioNTkb16P |
1 participant
Proposed changes
Stacks on #7297. When CallKit accepts a VoIP call from the lock screen, the app stays backgrounded —
APP_STATE.FOREGROUNDnever transitions, so the foreground-reconnect saga added in #7297 never fires. The DDP socket can be a zombie (kept-alivereadyState=OPEN, but TCP dead after a long iOS suspend), and any outgoing SDP/ICE sends triggered byapplyRestStateSignals()are silently swallowed.This adds a parallel reconnect trigger inside
handleVoipAcceptSucceededFromNative: when the accepted call's host matches the active workspace, callcheckAndReopen()(fire-and-forget) before replaying REST state signals. Same primitive as the saga, just driven from the VOIP accept path instead of the app-state listener.The behavior depends on #7297's unconditional teardown — without that patch
checkAndReopenno-ops against the zombie socket (the pre-#7297 implementation only opens when!this.connected).Issue(s)
Follow-up to #7297 — covers the lock-screen accept scenario that PR did not.
How to test or reproduce
Screenshots
N/A — logging only on the JS side.
Types of changes
Checklist
Further comments
Cross-platform (
MediaCallEvents.test.ts) covers four cases: same-workspace firescheckAndReopenonce, cross-workspace does not fire, duplicatecallIddeliveries dedupe, andcheckAndReopenis invoked beforeapplyRestStateSignals. The iOS test file (MediaCallEvents.ios.test.ts) gets the matching'../connect'mock so the new import does not break its module graph.Considered (and rejected) alternatives:
APP_STATE.FOREGROUNDredux dispatch from the accept path — would re-runlocalAuthenticate(biometric prompt) andsetUserPresenceOnline, both wrong for a backgrounded lock-screen accept.checkAndReopenlives in the JS SDK, so the trigger has to cross the bridge anyway; doing it from JS keeps the concern in one place.No native code changed.
Summary by CodeRabbit
Tests
Bug Fixes