fix(voip): recover answer-call and start-call failure paths#7281
Conversation
Wrap answerCall's accept() in a try/catch so signalling rejections terminate the native call, reset nativeAcceptedCallId, and surface a localised alert instead of leaving CallKit stuck. Re-evaluate isInActiveVoipCall() after the microphone permission await in startCall so a call accepted from a system notification during the prompt cannot start a second concurrent session.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds error handling for VoIP answer failures (terminating native calls, clearing native call state, and showing an error alert) and revalidates active-call state after permission prompts for outgoing calls; also adds one new i18n key. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MediaSessionInstance
participant MainCall
participant NativeLayer
participant AlertSystem
User->>MediaSessionInstance: answerCall()
MediaSessionInstance->>MainCall: accept()
MainCall-->>MediaSessionInstance: reject/error
MediaSessionInstance->>NativeLayer: terminateNativeCall(callId)
NativeLayer-->>MediaSessionInstance: terminated
MediaSessionInstance->>MediaSessionInstance: reset nativeAcceptedCallId (if matching)
MediaSessionInstance->>AlertSystem: showErrorAlert("VoIP_Answer_Failed")
AlertSystem-->>User: error displayed
sequenceDiagram
participant User
participant MediaSessionInstance
participant PermissionSystem
participant VoIPSession
User->>MediaSessionInstance: startCall(...)
MediaSessionInstance->>PermissionSystem: requestVoipCallPermissions()
PermissionSystem-->>MediaSessionInstance: permissions result
MediaSessionInstance->>MediaSessionInstance: re-check isInActiveVoipCall
alt another call became active
MediaSessionInstance-->>User: reject with VoIP_Already_In_Call
else no active call
MediaSessionInstance->>VoIPSession: instance.startCall(...)
VoIPSession-->>MediaSessionInstance: call started
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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. Review rate limit: 3/8 reviews remaining, refill in 31 minutes and 7 seconds.Comment |
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/MediaSessionInstance.ts`:
- Around line 218-221: When the guard in MediaSessionInstance that checks
isInActiveVoipCall() aborts, clear the optimistic room context set by
startCallByRoom() to avoid leaving a stale roomId; either defer assigning roomId
until after startCall() succeeds or explicitly reset the provisional roomId on
this error path (e.g., undo the assignment made by startCallByRoom() before
throwing). Locate references to startCallByRoom(), startCall(),
isInActiveVoipCall(), and the roomId field in MediaSessionInstance and ensure
the failure branch clears or unsets roomId so later DM-room resolution is not
suppressed.
🪄 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: aa6a1e12-1337-49a9-9c43-c88fc461e7eb
📒 Files selected for processing (3)
app/i18n/locales/en.jsonapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.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)
app/i18n/**/*.{ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement i18n translations in app/i18n/ directory with i18n-js supporting 40+ locales and RTL support
Files:
app/i18n/locales/en.json
**/*.{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/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.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
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
🧠 Learnings (5)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
📚 Learning: 2026-04-22T22:57:58.545Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Applied to files:
app/i18n/locales/en.jsonapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
📚 Learning: 2026-04-30T16:41:37.607Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7279
File: app/lib/services/voip/playCallEndedSound.ts:39-46
Timestamp: 2026-04-30T16:41:37.607Z
Learning: In `app/lib/services/voip/playCallEndedSound.ts`, the `isPlaying` lock is intentionally released only via `didJustFinish` (natural completion) plus a 5-second watchdog timer as a stuck-lock safety net. Treating `status.isPlaying === false && status.isLoaded === true` as a terminal state is explicitly avoided because it would falsely trigger on transient audio pauses (focus loss, iOS session interruption, Bluetooth handoff), prematurely releasing the lock and risking doubled playback when audio resumes.
Applied to files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
📚 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
📚 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/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
🔇 Additional comments (3)
app/i18n/locales/en.json (1)
999-999: Looks good.This new key matches the recovery path in
answerCalland keeps the localized message centralized with the rest of the VoIP strings.app/lib/services/voip/MediaSessionInstance.ts (1)
156-166: Good recovery path.Catching
accept()here, terminating the native call, and short-circuiting before navigation/setCall is the right failure handling for this race.app/lib/services/voip/MediaSessionInstance.test.ts (1)
800-884: Good coverage for both recovery paths.These tests exercise the native-call termination on
accept()rejection and the post-permission active-call recheck, which should make the regression much harder to reintroduce.
When startCallByRoom() optimistically sets roomId before calling startCall(), and the new B6 post-permission active-call re-evaluation throws (because an incoming call was accepted during the permission prompt), the catch in startCallByRoom() previously only logged the error. The stale roomId would remain in the call store and could suppress DM resolution for the just-accepted incoming call. Reset roomId to null on the catch path. Add a regression test that asserts setRoomId is called with the optimistic rid first and with null after the post-permission guard rejects.
Proposed changes
Two coupled fixes to the JS media-session lifecycle that prevent stuck call states.
Answer-call error recovery (B5): The
accept()await inanswerCallis now wrapped in a try/catch. When it rejects (signalling timeout, server reject, ICE failure, peer abort), the native call is terminated viaterminateNativeCall,nativeAcceptedCallIdis reset so the active-call guard releases immediately, and the user sees a localised "Could not answer call." alert. Previously a rejection left CallKit/Telecom stuck in "answering" state for the full 60-second timeout and blocked any new call attempt.Start-call post-permission guard (B6):
startCallre-evaluatesisInActiveVoipCall()after awaitingrequestVoipCallPermissions. On Android the microphone permission prompt can take several seconds; if the user accepts an incoming call from the heads-up notification while it is on screen, the guard transitions fromfalsetotrueduring that window. The re-evaluation throws the sameVoIP_Already_In_Callerror used in the original synchronous check, preventing a second concurrent call session from starting.Optimistic roomId cleanup:
startCallByRoomsetsroomIdoptimistically before callingstartCall. When the new B6 guard rejects after the permission await, the catch instartCallByRoomnow resetsroomIdtonullso a concurrent incoming call can resolve its own DM context instead of being suppressed by the stale value.Issue(s)
Part of PR #6918 fix series.
How to test or reproduce
B5: With a real device, simulate a signalling failure (kill WebSocket while tapping Accept). Within ~1 second an alert "Could not answer call." appears; the call guard releases and a new outgoing call can be placed immediately.
B6: On Android, start an outgoing call; while the microphone permission prompt is on screen, accept an incoming call from the system notification. After granting (or denying) the permission, no second call session is started, and the incoming call's DM context resolves correctly (no stale
roomIdfrom the aborted outgoing attempt).Screenshots
N/A
Types of changes
Checklist
Further comments
New i18n key
VoIP_Answer_Failedadded toen.json; other locales follow the standard translation flow.Summary by CodeRabbit
New Features
Bug Fixes