refactor(voip): split MediaSessionInstance into Controller + Orchestrator#7112
refactor(voip): split MediaSessionInstance into Controller + Orchestrator#7112diegolmello wants to merge 13 commits intofeat.voip-lib-newfrom
Conversation
- Extract MediaSessionController class for pure session lifecycle - WebRTC processor factory configuration with ICE servers - MediaSignalingSession creation and disposal - Add unit tests for controller Refs: #1
- Rename class to CallOrchestrator - Import and use MediaSessionController for session operations - Remove duplicated ICE/session logic - Keep singleton export for backward compatibility
- Add optional onCallStarted/onCallEnded callbacks - Default callbacks call Navigation.navigate for backward compat - Export CallOrchestrator and CallOrchestratorConfig types
- Change answerCall return type to Promise<CallResult> - Change startCall return type to CallResult - Export CallResult type
- Add @rocket.chat/media-signaling to transformIgnorePatterns - Add MediaSessionStore global mock in jest.setup.js
- Update CreateCall.test.tsx to spy on mediaSessionInstance.startCall - Update MediaCallEvents.test.ts to mock startCall returning CallResult - Tests now work with new CallResult return type
- Rewrite MediaSessionInstance.test.ts to test at boundaries - Mock Controller interface instead of internal session - Test call flow orchestration without native WebRTC - Verify callback invocations and CallResult return types
- Remove unused setupSendSignal method - Fix test imports and mock structure - All tests passing
- Move newCall handler out of onChange callback so it registers once
instead of re-attaching on every store change (listener leak).
- Drop dead mediaSessionStoreChangeUnsubscribe field + reset hook now
that onChange is no longer used.
- Make CallResult.callId optional and return { success: true } from
startCall instead of a fake empty-string callId.
- Clean up dangling block after removed else in answerCall.
- Remove reset() from MediaSessionController.configure() so it no longer disposes sendSignalFn mid-setup; makeInstance() would throw "send signal function must be set". - Reorder CallOrchestrator.init(): reset controller, set send fn, then configure. Send fn now survives until getInstance(). - Re-subscribe to mediaSessionStore.onChange to refresh the session and re-attach the newCall listener when the store replaces the instance.
init() now stops the previous stream-notify-user listener and unsubscribes from mediaSessionStore.onChange before replacing the controller. attachNewCallListener tracks the attached session so onChange re-emits with the same instance don't register duplicate newCall handlers.
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
WalkthroughIntroduces a new MediaSessionController to manage per-user MediaSignalingSession and WebRTC processor configuration, replaces MediaSessionInstance with CallOrchestrator (updated call APIs), updates Jest setup/mocks and config, and adds/adjusts tests for controller, instance/orchestration, and call flows. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App/Test
participant Controller as MediaSessionController
participant Store as mediaSessionStore
participant Session as MediaSignalingSession
participant Processor as WebRTC Processor
participant DDP as DDP/Stream
App->>Controller: new Controller(userId)
Controller->>Store: register processorFactory(...) rgba(100,150,240,0.5)
Store->>Processor: create processor using factory (uses iceServers, timeout)
Controller->>Store: setSendSignalFn(fn)
DDP->>Controller: media-signal received -> Controller.getSession()
Controller->>Session: processSignal(signal)
Session->>Processor: handle media / signaling
Session->>App: emit newCall
App->>Controller: attachNewCallListener -> Controller.refreshSession()
Controller->>Session: subscribe to call.end -> cleanup (RNCallKeep.endCall)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
🚥 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 |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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/MediaSessionInstance.ts (2)
167-173:⚠️ Potential issue | 🟡 MinorPropagate
startCallByRoom()results instead of dropping them.This helper currently returns
undefinedon both the success path and theotherUserId-missing path, so callers cannot observe the same failure modes thatstartCall()now surfaces.🛠️ Suggested fix
- public startCallByRoom = (room: TSubscriptionModel | ISubscription) => { + public startCallByRoom = (room: TSubscriptionModel | ISubscription): CallResult => { useCallStore.getState().setRoomId(room.rid ?? null); const otherUserId = getUidDirectMessage(room); - if (otherUserId) { - this.startCall(otherUserId, 'user'); + if (!otherUserId) { + return { success: false, error: 'Unable to resolve direct-message peer' }; } + return this.startCall(otherUserId, 'user'); };🤖 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 167 - 173, The helper startCallByRoom currently drops the return value from startCall and always returns undefined; change it to propagate startCall's result by returning the value of this.startCall(...) when otherUserId is present (and still set the room id via useCallStore.getState().setRoomId). Ensure the function returns the same type as startCall so callers can observe its success/failure; keep the getUidDirectMessage(room) check and return (or forward) the appropriate result when otherUserId is missing.
146-164:⚠️ Potential issue | 🟠 MajorReturn a
CallResultwhenaccept()fails.
answerCall()is typed asPromise<CallResult>, but a rejection frommainCall.accept()still escapes this method. That bypasses the native-call cleanup below and forces callers back into.catch(...)instead of the new result contract.🛠️ Suggested fix
if (mainCall && mainCall.callId === callId) { console.log('[VoIP] Accepting call:', callId); - await mainCall.accept(); - console.log('[VoIP] Setting current call active:', callId); - RNCallKeep.setCurrentCallActive(callId); - useCallStore.getState().setCall(mainCall); - this.onCallStarted(); - this.resolveRoomIdFromContact(mainCall.contact).catch(error => { - console.error('[VoIP] Error resolving room id from contact (answerCall):', error); - }); - return { success: true, callId }; + try { + await mainCall.accept(); + console.log('[VoIP] Setting current call active:', callId); + RNCallKeep.setCurrentCallActive(callId); + useCallStore.getState().setCall(mainCall); + this.onCallStarted(); + this.resolveRoomIdFromContact(mainCall.contact).catch(error => { + console.error('[VoIP] Error resolving room id from contact (answerCall):', error); + }); + return { success: true, callId }; + } catch (error) { + RNCallKeep.endCall(callId); + const st = useCallStore.getState(); + if (st.nativeAcceptedCallId === callId) { + st.resetNativeCallId(); + } + console.error('[VoIP] Error accepting call:', error); + return { success: false, error: 'Failed to accept call' }; + } }As per coding guidelines, "Use explicit error handling with try/catch blocks for async operations".
🤖 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 146 - 164, The code currently awaits mainCall.accept() without a try/catch so a rejection escapes answerCall and skips the native-call cleanup; wrap the await mainCall.accept() in a try/catch inside the block where mainCall.callId === callId, and on catch: log the error, call RNCallKeep.endCall(callId), check useCallStore.getState().nativeAcceptedCallId === callId and call st.resetNativeCallId() if so, and then return a CallResult like { success: false, error: /* error message or object */ } so callers receive the expected result contract; keep the successful-path calls to RNCallKeep.setCurrentCallActive, useCallStore.getState().setCall(mainCall), this.onCallStarted(), and the resolveRoomIdFromContact(...) call unchanged.
🧹 Nitpick comments (2)
app/lib/services/voip/MediaSessionController.ts (1)
25-39: Avoid stacking aux-store subscriptions on repeatedconfigure()calls.A second
configure()on the same controller registers two more aux-store listeners and overwrites the first unsubscribe handles, soreset()can only detach the latest pair. Make this method idempotent, or tear the existing subscriptions down before re-subscribing.♻️ Possible hardening
public configure(): void { + this.storeTimeoutUnsubscribe?.(); + this.storeIceServersUnsubscribe?.(); + this.storeTimeoutUnsubscribe = null; + this.storeIceServersUnsubscribe = null; registerGlobals(); this.configureIceServers(); mediaSessionStore.setWebRTCProcessorFactory(Also applies to: 68-85
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionController.ts` around lines 25 - 39, The configure() method currently re-registers aux-store listeners on each call (via mediaSessionStore.setWebRTCProcessorFactory and mediaSessionStore.getInstance(this.userId)) and overwrites previous unsubscribe handles so reset() can only clear the latest; make configure() idempotent by checking for and calling any existing unsubscribe/cleanup functions (stored on the controller instance) before re-subscribing or by early-returning if already configured, and ensure those unsubscribe handles are set when creating the subscriptions and invoked from reset() (and from configure() prior to re-subscribing) so multiple configure() calls do not stack listeners.app/lib/services/voip/MediaSessionInstance.test.ts (1)
26-27: Wire these reusable mock handles to the mocked modules.
mockGetDMSubscriptionByUsernameandmockGetUidDirectMessageare separatejest.fns from the ones returned in thejest.mock(...)factories above, so mutating them inbeforeEach()does not change whatCallOrchestratoractually calls.♻️ Suggested cleanup
-const mockGetDMSubscriptionByUsername = jest.fn(); -const mockGetUidDirectMessage = jest.fn(() => 'other-user-id'); +const { getDMSubscriptionByUsername: mockGetDMSubscriptionByUsername } = jest.requireMock( + '../../database/services/Subscription' +); +const { getUidDirectMessage: mockGetUidDirectMessage } = jest.requireMock('../../methods/helpers/helpers');Also applies to: 116-117
🤖 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 26 - 27, The test defines separate jest.fn()s (mockGetDMSubscriptionByUsername and mockGetUidDirectMessage) but the jest.mock(...) factories return different functions, so mutations in beforeEach() don't affect CallOrchestrator; update the mock factories to return these shared variables (i.e., have the jest.mock(...) factories reference and return mockGetDMSubscriptionByUsername and mockGetUidDirectMessage) or rewire the module mocks to export these specific jest.fn handles, and apply the same fix for the other duplicate mocks referenced later (the ones around lines 116-117) so CallOrchestrator uses the mutable mock handles.
🤖 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.test.ts`:
- Around line 52-56: The test mock uses a plain function with an unused rest
parameter for mediaSessionInstance.startCall which trips ESLint; replace that
implementation with a Jest mock so it is inspectable and lint-clean, i.e.,
change the startCall value in the jest.mock for MediaSessionInstance to
jest.fn(() => ({ success: true, callId: '' })) and keep endCall as jest.fn() so
tests can assert calls on mediaSessionInstance.startCall.
In `@app/lib/services/voip/MediaSessionController.test.ts`:
- Line 1: The test file imports an unused type IClientMediaCall which triggers
`@typescript-eslint/no-unused-vars`; remove the import declaration referencing
IClientMediaCall from MediaSessionController.test.ts (the import line: import
type { IClientMediaCall } from '@rocket.chat/media-signaling';) or, if the type
is intended to be used, replace its usage accordingly—otherwise simply delete
that named type from the import so only used symbols remain.
In `@jest.setup.js`:
- Around line 214-219: The shared test mock mockSession is missing
setIceGatheringTimeout which causes tests exercising
MediaSessionController.configureIceServers() to throw; update the mockSession
object (the mock returned in jest.setup.js) to include a setIceGatheringTimeout
property implemented as a jest.fn() (and any related methods invoked by
configureIceServers, e.g., if configureIceServers also calls
setIceGatheringState or similar, add those as jest.fn() too) so calls to
session.setIceGatheringTimeout(...) succeed during tests.
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 167-173: The helper startCallByRoom currently drops the return
value from startCall and always returns undefined; change it to propagate
startCall's result by returning the value of this.startCall(...) when
otherUserId is present (and still set the room id via
useCallStore.getState().setRoomId). Ensure the function returns the same type as
startCall so callers can observe its success/failure; keep the
getUidDirectMessage(room) check and return (or forward) the appropriate result
when otherUserId is missing.
- Around line 146-164: The code currently awaits mainCall.accept() without a
try/catch so a rejection escapes answerCall and skips the native-call cleanup;
wrap the await mainCall.accept() in a try/catch inside the block where
mainCall.callId === callId, and on catch: log the error, call
RNCallKeep.endCall(callId), check useCallStore.getState().nativeAcceptedCallId
=== callId and call st.resetNativeCallId() if so, and then return a CallResult
like { success: false, error: /* error message or object */ } so callers receive
the expected result contract; keep the successful-path calls to
RNCallKeep.setCurrentCallActive, useCallStore.getState().setCall(mainCall),
this.onCallStarted(), and the resolveRoomIdFromContact(...) call unchanged.
---
Nitpick comments:
In `@app/lib/services/voip/MediaSessionController.ts`:
- Around line 25-39: The configure() method currently re-registers aux-store
listeners on each call (via mediaSessionStore.setWebRTCProcessorFactory and
mediaSessionStore.getInstance(this.userId)) and overwrites previous unsubscribe
handles so reset() can only clear the latest; make configure() idempotent by
checking for and calling any existing unsubscribe/cleanup functions (stored on
the controller instance) before re-subscribing or by early-returning if already
configured, and ensure those unsubscribe handles are set when creating the
subscriptions and invoked from reset() (and from configure() prior to
re-subscribing) so multiple configure() calls do not stack listeners.
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 26-27: The test defines separate jest.fn()s
(mockGetDMSubscriptionByUsername and mockGetUidDirectMessage) but the
jest.mock(...) factories return different functions, so mutations in
beforeEach() don't affect CallOrchestrator; update the mock factories to return
these shared variables (i.e., have the jest.mock(...) factories reference and
return mockGetDMSubscriptionByUsername and mockGetUidDirectMessage) or rewire
the module mocks to export these specific jest.fn handles, and apply the same
fix for the other duplicate mocks referenced later (the ones around lines
116-117) so CallOrchestrator uses the mutable mock handles.
🪄 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: 45e0ada3-935f-40fb-a6b6-3a478d9cef92
📒 Files selected for processing (9)
.gitignoreapp/containers/NewMediaCall/CreateCall.test.tsxapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsjest.config.jsjest.setup.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
jest.config.jsapp/lib/services/voip/MediaCallEvents.test.tsapp/containers/NewMediaCall/CreateCall.test.tsxjest.setup.jsapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
jest.config.jsapp/lib/services/voip/MediaCallEvents.test.tsapp/containers/NewMediaCall/CreateCall.test.tsxjest.setup.jsapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{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:
jest.config.jsapp/lib/services/voip/MediaCallEvents.test.tsapp/containers/NewMediaCall/CreateCall.test.tsxjest.setup.jsapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{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.test.tsapp/containers/NewMediaCall/CreateCall.test.tsxapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Files:
app/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Place reusable UI components in app/containers/ directory
Files:
app/containers/NewMediaCall/CreateCall.test.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to @(app/sagas/videoConf.ts|app/lib/methods/videoConf.ts) : Manage video conferencing via Redux actions/reducers/sagas in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using server-managed Jitsi integration; do not conflate with VoIP
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to **/*.{js,jsx,ts,tsx} : Use ESLint with rocket.chat/eslint-config base configuration including React, React Native, TypeScript, and Jest plugins
Applied to files:
jest.config.js
📚 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:
jest.config.jsapp/lib/services/voip/MediaCallEvents.test.tsapp/containers/NewMediaCall/CreateCall.test.tsxapp/lib/services/voip/MediaSessionInstance.test.ts
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to **/*.{js,jsx,ts,tsx,json} : Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Applied to files:
jest.config.js
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
app/lib/services/voip/MediaCallEvents.test.tsapp/containers/NewMediaCall/CreateCall.test.tsxjest.setup.jsapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to @(app/sagas/videoConf.ts|app/lib/methods/videoConf.ts) : Manage video conferencing via Redux actions/reducers/sagas in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using server-managed Jitsi integration; do not conflate with VoIP
Applied to files:
app/containers/NewMediaCall/CreateCall.test.tsxapp/lib/services/voip/MediaSessionController.test.tsapp/lib/services/voip/MediaSessionController.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
📚 Learning: 2026-03-30T15:49:26.708Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6875
File: app/containers/RoomItem/Actions.tsx:12-12
Timestamp: 2026-03-30T15:49:26.708Z
Learning: In Rocket.Chat.ReactNative, do not rely on `react-native-worklets` v0.6.1 exporting a built-in Jest mock (e.g., `react-native-worklets/lib/module/mock` does not exist for this version). Instead, add the Jest manual mock in your repo’s `jest.setup.js`/`jest.setup.ts`, mocking `react-native-worklets` to provide `scheduleOnRN: jest.fn((fn, ...args) => fn(...args))`. This ensures Jest can import the module and that `scheduleOnRN` executes the passed function during tests.
Applied to files:
jest.setup.js
🪛 ESLint
app/lib/services/voip/MediaCallEvents.test.ts
[error] 54-54: 'args' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
app/lib/services/voip/MediaSessionController.test.ts
[error] 1-1: 'IClientMediaCall' is defined but never used.
(@typescript-eslint/no-unused-vars)
🔇 Additional comments (1)
.gitignore (1)
86-90: LGTM!The ignore patterns are appropriate:
*.p8files contain sensitive Apple credentials and should never be committed, and the local tooling directories (.claude/,.superset/) are correctly excluded from version control.
| jest.mock('./MediaSessionInstance', () => ({ | ||
| mediaSessionInstance: { | ||
| startCall: (...args: unknown[]) => ({ success: true, callId: '' }), | ||
| endCall: jest.fn() | ||
| } |
There was a problem hiding this comment.
Use a mock function for startCall.
The rest parameter here is unused, and ESLint is already flagging it. Switching this to jest.fn(() => ({ success: true, callId: '' })) removes the lint failure and keeps the mock inspectable.
🧰 Tools
🪛 ESLint
[error] 54-54: 'args' is defined but never used. Allowed unused args must match /^_/u.
(@typescript-eslint/no-unused-vars)
🤖 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 52 - 56, The test
mock uses a plain function with an unused rest parameter for
mediaSessionInstance.startCall which trips ESLint; replace that implementation
with a Jest mock so it is inspectable and lint-clean, i.e., change the startCall
value in the jest.mock for MediaSessionInstance to jest.fn(() => ({ success:
true, callId: '' })) and keep endCall as jest.fn() so tests can assert calls on
mediaSessionInstance.startCall.
| @@ -0,0 +1,127 @@ | |||
| import type { IClientMediaCall } from '@rocket.chat/media-signaling'; | |||
There was a problem hiding this comment.
Remove the unused type import.
IClientMediaCall is not referenced anywhere in this file, so this currently fails @typescript-eslint/no-unused-vars.
🧰 Tools
🪛 ESLint
[error] 1-1: 'IClientMediaCall' is defined but never used.
(@typescript-eslint/no-unused-vars)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/lib/services/voip/MediaSessionController.test.ts` at line 1, The test
file imports an unused type IClientMediaCall which triggers
`@typescript-eslint/no-unused-vars`; remove the import declaration referencing
IClientMediaCall from MediaSessionController.test.ts (the import line: import
type { IClientMediaCall } from '@rocket.chat/media-signaling';) or, if the type
is intended to be used, replace its usage accordingly—otherwise simply delete
that named type from the import so only used symbols remain.
| const mockSession = { | ||
| on: jest.fn(), | ||
| processSignal: jest.fn(), | ||
| startCall: jest.fn(), | ||
| getMainCall: jest.fn() | ||
| }; |
There was a problem hiding this comment.
Complete the shared session mock.
MediaSessionController.configureIceServers() can call session.setIceGatheringTimeout(...) after subscription updates. The global mockSession returned here does not provide that method, so any test that exercises that path will throw because of the mock rather than the code under test.
🧪 Suggested mock update
const mockSession = {
on: jest.fn(),
processSignal: jest.fn(),
startCall: jest.fn(),
- getMainCall: jest.fn()
+ getMainCall: jest.fn(),
+ setIceGatheringTimeout: jest.fn()
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const mockSession = { | |
| on: jest.fn(), | |
| processSignal: jest.fn(), | |
| startCall: jest.fn(), | |
| getMainCall: jest.fn() | |
| }; | |
| const mockSession = { | |
| on: jest.fn(), | |
| processSignal: jest.fn(), | |
| startCall: jest.fn(), | |
| getMainCall: jest.fn(), | |
| setIceGatheringTimeout: jest.fn() | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@jest.setup.js` around lines 214 - 219, The shared test mock mockSession is
missing setIceGatheringTimeout which causes tests exercising
MediaSessionController.configureIceServers() to throw; update the mockSession
object (the mock returned in jest.setup.js) to include a setIceGatheringTimeout
property implemented as a jest.fn() (and any related methods invoked by
configureIceServers, e.g., if configureIceServers also calls
setIceGatheringState or similar, add those as jest.fn() too) so calls to
session.setIceGatheringTimeout(...) succeed during tests.
- Guard reject() with role check to avoid throws on outbound ringing calls - Wrap reject()/hangup() in try/catch so CallKeep teardown always runs - Detect synchronous state transition to avoid double-firing onCallEnded - Fall back to invoking onCallEnded() when hangup early-returns or no mainCall match - Add 8 tests covering all new edge-case paths
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
26-27: Disconnected mock variables have no effect on tests.These mock variables are declared separately from the
jest.mock()implementations above (lines 18-24). The module under test uses the mocks fromjest.requireMock()or the inlinejest.mock()factories, not these standalone variables.Specifically:
mockGetDMSubscriptionByUsername(line 26) is not the same function as the mock at line 19- Line 117's
mockGetDMSubscriptionByUsername.mockResolvedValue(null)modifies an unused variableEither remove these declarations or properly connect them by capturing from
jest.requireMock:♻️ Suggested fix
-const mockGetDMSubscriptionByUsername = jest.fn(); -const mockGetUidDirectMessage = jest.fn(() => 'other-user-id'); +const { getDMSubscriptionByUsername: mockGetDMSubscriptionByUsername } = jest.requireMock( + '../../database/services/Subscription' +); +const { getUidDirectMessage: mockGetUidDirectMessage } = jest.requireMock('../../methods/helpers/helpers');🤖 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 26 - 27, The two standalone mocks mockGetDMSubscriptionByUsername and mockGetUidDirectMessage are disconnected from the module mocks declared via jest.mock and thus changing them has no effect; replace or bind them by retrieving the actual mocks from jest.requireMock for the module that exports getDMSubscriptionByUsername/getUidDirectMessage (or update the jest.mock factory to return these variables), e.g., obtain the mocked functions from jest.requireMock('<module>') and then call mockResolvedValue/mockImplementation on those retrieved references so the tests modify the real mocked implementations used by MediaSessionInstance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 26-27: The two standalone mocks mockGetDMSubscriptionByUsername
and mockGetUidDirectMessage are disconnected from the module mocks declared via
jest.mock and thus changing them has no effect; replace or bind them by
retrieving the actual mocks from jest.requireMock for the module that exports
getDMSubscriptionByUsername/getUidDirectMessage (or update the jest.mock factory
to return these variables), e.g., obtain the mocked functions from
jest.requireMock('<module>') and then call mockResolvedValue/mockImplementation
on those retrieved references so the tests modify the real mocked
implementations used by MediaSessionInstance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f407b96f-f269-4f0d-8f18-f69d6c742951
📒 Files selected for processing (2)
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,jsx,ts,tsx,json}
📄 CodeRabbit inference engine (CLAUDE.md)
Configure Prettier with tabs, single quotes, 130 character width, no trailing commas, arrow parens avoid, and bracket same line
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use ESLint with
@rocket.chat/eslint-configbase configuration including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Use TypeScript with strict mode enabled and configure baseUrl to app/ for import resolution
**/*.{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/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{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
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to @(app/sagas/videoConf.ts|app/lib/methods/videoConf.ts) : Manage video conferencing via Redux actions/reducers/sagas in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using server-managed Jitsi integration; do not conflate with VoIP
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP with WebRTC peer-to-peer audio calls in app/lib/services/voip/ using Zustand stores instead of Redux, with native CallKit (iOS) and Telecom (Android) integration; keep VoIP and VideoConf separate
Applied to files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
📚 Learning: 2026-04-07T17:49:17.538Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-07T17:49:17.538Z
Learning: Applies to @(app/sagas/videoConf.ts|app/lib/methods/videoConf.ts) : Manage video conferencing via Redux actions/reducers/sagas in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using server-managed Jitsi integration; do not conflate with VoIP
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
🔇 Additional comments (9)
app/lib/services/voip/MediaSessionInstance.ts (5)
21-28: LGTM! Well-defined type exports for call orchestration API.The
CallOrchestratorConfigandCallResulttypes provide clear contracts for the orchestrator's callback configuration and method return values. The discriminated union forCallResultenables type-safe error handling.
30-42: LGTM! Clean dependency injection for navigation callbacks.The constructor properly accepts optional callbacks with sensible defaults, allowing callers to customize navigation behavior while maintaining backward compatibility with the default
Navigation.navigate('CallView')behavior.
44-63: LGTM! Proper cleanup on re-initialization.The
init()method correctly cleans up previous listeners and state before setting up new ones, preventing listener leaks on re-init (e.g., logout/login cycle). The sequence ofcontroller.reset()→ new controller →setSendSignalFn()→configure()ensures the store is properly configured before session creation.
65-94: LGTM! Effective deduplication for newCall listener attachment.The
attachedSessiontracking prevents duplicate listener attachment whenonChangefires multiple times with the same session instance. Theendedevent listener properly wires upRNCallKeep.endCallandonCallEnded()cleanup.
186-212: LGTM! Robust endCall implementation with proper edge-case handling.The implementation correctly:
- Guards
reject()to only callee in ringing state (avoiding throws on outbound ringing calls)- Wraps termination in try/catch so CallKeep teardown always runs
- Detects synchronous state transitions to avoid double-firing
onCallEnded(theendedevent listener handles it whenhangup()succeeds)- Falls back to invoking
onCallEnded()when hangup early-returns or throwsapp/lib/services/voip/MediaSessionInstance.test.ts (4)
113-129: LGTM! Well-structured test setup with proper cleanup.The
beforeEachandafterEachhooks correctly reset mocks and the orchestrator instance, ensuring test isolation. The mock return value setup formockUseCallStoreGetStateprovides a clean baseline for each test.
141-167: LGTM! Thorough test for session change listener re-attachment.This test properly validates that when
mediaSessionStore.onChangefires with a new session, thenewCalllistener is attached to the new session. The mock setup correctly simulates two distinct sessions.
169-185: LGTM! Good invariant test for initialization ordering.Testing the call order between
dispose,setSendSignalFn, andgetInstancevalidates a critical invariant: the send signal function must be set beforegetInstanceis called, otherwisemakeInstancewould throw.
311-435: LGTM! Comprehensive endCall test coverage.The test suite thoroughly covers edge cases:
- Synchronous state transitions (hangup sets state to 'hangup')
- Role-based termination (reject for callee-ringing, hangup otherwise)
- Fallback
onCallEndedinvocation when mainCall is missing/mismatched- Error resilience (CallKeep teardown runs even when hangup throws)
- Early-return handling (hangup doesn't transition state)
1 participant
Summary
Refactors the monolithic
MediaSessionInstancesingleton into two focused modules following the Single Responsibility Principle:MediaSessionController— pure session lifecycle (WebRTC processor factory, ICE config,MediaSignalingSessioncreate/dispose). Exposesinit(userId),reset(),getSession().CallOrchestrator(the refactoredMediaSessionInstance) — call lifecycle (answerCall,startCall,startCallByRoom,endCall), RNCallKeep coordination, room resolution. Navigation is now injected via optionalonCallStarted/onCallEndedcallbacks instead of callingNavigation.navigatedirectly, and call methods returnPromise<CallResult>instead ofPromise<void>.Both remain singletons exported from the existing paths, so callers (
login.js,CreateCall.tsx,MediaCallEvents.ts) need no changes.Motivation
The previous
MediaSessionInstancewas a shallow 230-line module juggling session lifecycle, call orchestration, navigation, store writes, and DB lookups. Tests required extensive cross-cutting mocks and reached into implementation details. Splitting the module lets each half be tested at its boundary.Changes
app/lib/services/voip/MediaSessionController.ts+ unit testsapp/lib/services/voip/MediaSessionInstance.tsto delegate session management to the controllerMediaSessionInstance.test.ts) rewritten to mock the controller interface rather than inspecting internal statenewCallhandler (see commits 84d4b60, c2258a6, 11b3792)media-signalingmodule transformTest plan
yarn test app/lib/services/voip/MediaSessionController.test.tsyarn test app/lib/services/voip/MediaSessionInstance.test.tsyarn test app/containers/NewMediaCall/CreateCall.test.tsxyarn lintSummary by CodeRabbit
Refactor
Tests
Chores