feat(voip): iOS CallCoordinator + ActiveCallRegistry + VoipRESTClient + test target#7154
Closed
diegolmello wants to merge 17 commits into
Closed
feat(voip): iOS CallCoordinator + ActiveCallRegistry + VoipRESTClient + test target#7154diegolmello wants to merge 17 commits into
diegolmello wants to merge 17 commits into
Conversation
…ry fields Iteration 2 fixes: wrap REST block in null guard, replace field accesses with as any casts since role/muted/held/contact/setMuted/setHeld were removed from IClientMediaCall in 0.2.0-rc.0 library.
… target + VoipRESTClient PR 4a/4b/5/6 migration (Wave 1 - VoipService isolation): - Add CallCoordinator: pure Swift state machine (6 states, 6 inputs, 4 outputs) - Add ActiveCallRegistry: thread-safe call tracking replacing observedIncomingCalls dict - Add CallEndListener: DDP listener for call-end events per callId - Add DDPClientProtocol: testable interface for DDPClient - Add VoipRESTClient + VoipRESTClientProtocol: REST accept/reject behind protocol - Add Rocket.ChatTests XCTest target with VoipPayload, CallCoordinator, ActiveCallRegistry, CallEndListener, and VoipRESTClient tests - Add test-ios.yml CI workflow for ios/** PRs - Update VoipService to use CallCoordinator for state transitions - Update VoipPerCallDdpRegistry to delegate call tracking to ActiveCallRegistry - Update VoipService to delegate accept/reject to VoipRESTClient - Rebase onto refactor.ddp-ios (PR 7124) to pick up REST accept/reject Part of stacked PR stack: 1a→1b→2→3→4a→4b→5→6
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
Contributor
WalkthroughThis PR introduces comprehensive VoIP call control enhancements including a CallCoordinator state machine for iOS, active call registry tracking, call end detection via DDP, landscape mode and accessibility improvements for the call view UI, and internationalization for call control UI across 26 languages. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CallKit as CallKit (OS)
participant VoipService
participant CallCoordinator
participant ActiveRegistry as ActiveCallRegistry
participant DDPListener as CallEndListener
participant RestClient as VoipRESTClient
User->>VoipService: reportIncomingCall(payload)
VoipService->>ActiveRegistry: addCall(callId, payload)
VoipService->>CallCoordinator: transition(.idle, .incomingPush)
CallCoordinator-->>VoipService: (.incoming, [.ringOS])
VoipService->>CallKit: ringOS()
CallKit->>User: Ring notification
User->>CallKit: User answers
CallKit->>VoipService: handleUserAnswer(callId)
VoipService->>CallCoordinator: transition(.incoming, .userAnswer)
CallCoordinator-->>VoipService: (.answering, [.startAudio])
VoipService->>RestClient: accept(payload, completion)
RestClient-->>VoipService: completion(true)
VoipService->>CallCoordinator: transition(.answering, .restAck)
CallCoordinator-->>VoipService: (.active, [])
alt Remote Hangup
DDPListener->>VoipService: handleDdpCallEnded(callId)
VoipService->>CallCoordinator: transition(.active, .ddpCallEnded)
CallCoordinator-->>VoipService: (.ended, [.endOS])
else User Declines
User->>CallKit: User declines
CallKit->>VoipService: handleUserDecline(callId)
VoipService->>CallCoordinator: transition(.incoming, .userDecline)
CallCoordinator-->>VoipService: (.ended, [.endOS])
end
VoipService->>CallKit: endOS()
VoipService->>ActiveRegistry: removeCall(callId)
VoipService->>DDPListener: stop()
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
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 |
Member
|
@diegolmello could you pin the hashes for the new actions? We have an internal initiative to make sure all action versions are hashes instead of release tags due to supply chain attacks. More info here https://rocketchat.atlassian.net/browse/SB-958 and here https://www.stepsecurity.io/blog/pinning-github-actions-for-enhanced-security-a-complete-guide. That's why Layne failed on this one |
![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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
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.test.ts (1)
613-644:⚠️ Potential issue | 🟠 MajorThese
answerCalltests never exposemainCallto the system under test.Both cases build a
mainCallwithlocalParticipant.contact, butbeforeEach()still makesuseCallStore.getState()returncall: null, and neither test overrides that. So this block is not actually exercising the branch it was rewritten to cover.💡 One way to exercise the intended path
const mainCall = { callId: 'call-ans', accept: jest.fn().mockResolvedValue(undefined), localParticipant: { contact: { username: 'bob', sipExtension: '' } } }; + mockUseCallStoreGetState.mockReturnValue({ + reset: mockCallStoreReset, + setCall: jest.fn(), + setRoomId: mockSetRoomId, + resetNativeCallId: jest.fn(), + call: mainCall as IClientMediaCall, + callId: mainCall.callId, + nativeAcceptedCallId: null, + roomId: null + }); await mediaSessionInstance.answerCall('call-ans');Apply the same setup in the SIP variant.
🤖 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 613 - 644, The tests build a mainCall object but never expose it to the system because useCallStore.getState() still returns call: null; update both tests to inject the constructed mainCall into the call store before calling mediaSessionInstance.answerCall (e.g., mock useCallStore.getState to return { call: mainCall } or set the store state accordingly) so mediaSessionInstance.answerCall sees the mainCall; do this for both the DM test and the SIP-variant test (the latter should provide localParticipant.contact.sipExtension = 'ext') and keep assertions on mockGetDMSubscriptionByUsername and mockSetRoomId unchanged.app/lib/services/voip/MediaSessionInstance.ts (1)
103-115:⚠️ Potential issue | 🔴 CriticalInbound calls no longer have a local source of truth.
After switching
answerCall()andendCall()touseCallStore.getState().call, this file still only mirrorsnewCallevents into the store whenlocalParticipant.role === 'caller'. Callee-side calls therefore have no local fallback here, so native accept/decline can fall through to theelsepath and end CallKeep instead ofaccept()/reject()/hangup().💡 One possible fix
this.instance?.on('newCall', ({ call }: { call: IClientMediaCall }) => { if (call && !call.hidden) { call.emitter.on('stateChange', () => {}); + useCallStore.getState().setCall(call); if (call.localParticipant.role === 'caller') { - useCallStore.getState().setCall(call); Navigation.navigate('CallView'); if (useCallStore.getState().roomId == null) { this.resolveRoomIdFromContact(call.localParticipant.contact).catch(error => {Also applies to: 124-176
🤖 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 103 - 115, The newCall handler in MediaSessionInstance only sets the call into the store when localParticipant.role === 'caller', leaving inbound (callee) calls without a local source of truth; update the handler used in the 'newCall' listener (and the similar block around lines 124-176) to always call useCallStore.getState().setCall(call) for incoming calls (both caller and callee), attach the call.emitter.on('stateChange', ...) handler for callee calls as well, and ensure resolveRoomIdFromContact(call.localParticipant.contact) is invoked or skipped appropriately regardless of role so native accept/decline will route to answerCall()/endCall() instead of falling through to CallKeep handling.
🟡 Minor comments (10)
app/lib/services/restApi.ts-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorFix import order.
The
@rocket.chat/media-signalingtype import should be placed before the local../../definitionsimports per ESLint import ordering rules.🔧 Proposed fix
Move line 17 to before line 3:
import { getUniqueId } from 'react-native-device-info'; +import type { ServerMediaSignal } from '@rocket.chat/media-signaling'; import { type IAvatarSuggestion, type IMessage, ... } from '../../definitions'; -import type { ServerMediaSignal } from '@rocket.chat/media-signaling';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` at line 17, Move the external type import for ServerMediaSignal so it precedes local imports: place "import type { ServerMediaSignal } from '@rocket.chat/media-signaling'" before the local "../../definitions" import statements in restApi.ts to satisfy ESLint import ordering rules (look for the ServerMediaSignal import and the ../../definitions import to reorder).app/lib/services/restApi.ts-1220-1232 (1)
1220-1232:⚠️ Potential issue | 🟡 MinorFix Prettier formatting issues.
The function has formatting violations flagged by Prettier. Additionally, consider whether the type cast on
sdk.getcould be avoided with proper SDK typing.🔧 Proposed fix for formatting
-export const mediaCallsStateSignals = async ( - contractId: string -): Promise<{ signals: ServerMediaSignal[]; success: boolean }> => { +export const mediaCallsStateSignals = async (contractId: string): Promise<{ signals: ServerMediaSignal[]; success: boolean }> => { try { - const result = await (sdk.get as unknown as (path: string, params?: object) => Promise<{ signals: ServerMediaSignal[]; success: boolean }>)( - 'media-calls.stateSignals', - { contractId } - ); + const result = await ( + sdk.get as unknown as (path: string, params?: object) => Promise<{ signals: ServerMediaSignal[]; success: boolean }> + )('media-calls.stateSignals', { contractId }); return result; } catch { return { signals: [], success: false }; } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/restApi.ts` around lines 1220 - 1232, The mediaCallsStateSignals function has Prettier formatting violations and an unnecessary any-style cast on sdk.get; reformat the function body to match project Prettier rules (consistent spacing, line breaks and parentheses) and remove the manual cast by updating the SDK typing or adding a typed wrapper for get so you can call sdk.get<'media-calls.stateSignals' returnType>('media-calls.stateSignals', { contractId }) directly; keep the same return shape ({ signals: ServerMediaSignal[]; success: boolean }) and preserve the try/catch behavior.package.json-50-50 (1)
50-50:⚠️ Potential issue | 🟡 MinorThe RC version is intentional, but confirm stability before merge.
The
@rocket.chat/media-signalingdependency is pinned to a release candidate version (0.2.0-rc.0) as a local tarball, which is part of coordinated VoIP feature work (CallCoordinator state machine + ActiveCallRegistry). Type compatibility has been verified through active usage inuseCallStore.ts(Zustand store),MediaSessionInstance.ts,MediaSessionStore.ts, and test files that all import from this version successfully.Before merging, confirm that the 0.2.0 release is stable and production-ready, or plan to upgrade to the stable release before shipping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 50, The dependency "@rocket.chat/media-signaling" is pinned to an RC tarball ("0.2.0-rc.0"); before merging confirm stability or replace with the stable release: verify that the 0.2.0 release is production-ready (run integration tests and validate imports in useCallStore.ts, MediaSessionInstance.ts, MediaSessionStore.ts and related tests) and then update package.json to point to the published stable version (or document a migration plan/feature flag if holding the RC), ensuring all imports and type compatibility remain intact.app/i18n/locales/pt-PT.json-499-499 (1)
499-499:⚠️ Potential issue | 🟡 MinorUse pt-PT spelling for “controls”.
Line 499 uses “controles”, which is pt-BR; in pt-PT this should be “controlos” for consistency with the locale file.💡 Suggested fix
- "Toggle_call_controls": "Alternar controles de chamada", + "Toggle_call_controls": "Alternar controlos de chamada",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/i18n/locales/pt-PT.json` at line 499, Update the pt-PT translation for the "Toggle_call_controls" key to use European Portuguese spelling: replace the value "Alternar controles de chamada" with "Alternar controlos de chamada" so the key Toggle_call_controls uses "controlos" (pt-PT) instead of "controles" (pt-BR).app/i18n/locales/fi.json-779-779 (1)
779-779:⚠️ Potential issue | 🟡 MinorFinnish translation wording looks unnatural for “controls”.
Line 779 uses
hallintoja, which is not idiomatic for UI control elements in Finnish. Consider using a clearer UI term such asVaihda puhelun ohjaimet(orVaihda puhelun säätimetdepending on glossary preference).💬 Suggested wording update
- "Toggle_call_controls": "Vaihda puhelun hallintoja", + "Toggle_call_controls": "Vaihda puhelun ohjaimet",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/i18n/locales/fi.json` at line 779, The translation for the key "Toggle_call_controls" uses non-idiomatic Finnish "hallintoja"; update the value for the "Toggle_call_controls" JSON key to a more natural UI term such as "Vaihda puhelun ohjaimet" (or "Vaihda puhelun säätimet" if your glossary prefers "säätimet") so the label reads naturally in the UI.app/lib/services/voip/useCallStore.test.ts-48-51 (1)
48-51:⚠️ Potential issue | 🟡 MinorFix formatting for
remoteParticipantsarray.The array elements need proper indentation to satisfy Prettier rules.
🔧 Proposed fix
remoteParticipants: [{ - muted: false, - held: false - }], + remoteParticipants: [ + { + muted: false, + held: false + } + ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/useCallStore.test.ts` around lines 48 - 51, The remoteParticipants array in useCallStore.test.ts is mis-indented and failing Prettier; update the formatting of the remoteParticipants property (in the test setup where remoteParticipants is defined) so each array element and its properties are properly indented according to project Prettier rules (move the object on its own line and indent its keys), then run the formatter or tests to confirm the lint error is resolved.ios/Rocket.ChatTests/VoipRESTClientTests.swift-62-120 (1)
62-120:⚠️ Potential issue | 🟡 MinorThe concrete client path is still untested.
testVoipRESTClientImplementsProtocol()only proves thatVoipRESTClientcan be instantiated, while the rest of the suite validatesFakeVoipRESTClientagainst itself. A regression in the realaccept/rejectrequest orBoolcompletion mapping would still pass here unless the concrete client gets a mocked API/fetch dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Rocket.ChatTests/VoipRESTClientTests.swift` around lines 62 - 120, testVoipRESTClientImplementsProtocol only instantiates VoipRESTClient but doesn't verify its accept/reject behavior; add tests that exercise the real VoipRESTClient.accept(payload:completion:) and .reject(payload:completion:) paths by injecting a controllable network layer (e.g., a URLSession/HTTPClient stub or the same mock used by FakeVoipRESTClient), configure the mock to return success/failure Bool JSON responses, call VoipRESTClient.accept and .reject with a payload, and assert the completion Bool mapping and that the outgoing request contains the expected callId and endpoint; reference VoipRESTClient, accept(payload:completion:), reject(payload:completion:) and reuse patterns from FakeVoipRESTClient tests when asserting call counts and payloads.ios/Rocket.ChatTests/CallCoordinatorTests.swift-68-179 (1)
68-179:⚠️ Potential issue | 🟡 MinorFive explicit state-machine branches are still uncovered.
This manual matrix currently exercises 31/36 branches from
ios/Libraries/CallCoordinator.swift:55-139; the missing noop cases are.incoming + .incomingPush,.answering + .incomingPush,.active + .incomingPush,.active + .ddpCallEnded, and.ending + .incomingPush. A small table-driven matrix here would make omissions like these much harder to miss.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Rocket.ChatTests/CallCoordinatorTests.swift` around lines 68 - 179, The test suite misses five noop branches in CallCoordinator.transition: (.incoming, .incomingPush), (.answering, .incomingPush), (.active, .incomingPush), (.active, .ddpCallEnded) and (.ending, .incomingPush); update CallCoordinatorTests by adding a small table-driven loop that iterates over these (state, input) pairs and asserts the transition returns the same state and outputs == [.noop] (use the existing coordinator.transition(state:input:) helper and CallInput/.incomingPush and .ddpCallEnded symbols), mirroring the pattern used in test_ended_anyInput_illegal to avoid hardcoding more individual test methods.ios/Rocket.ChatTests/ActiveCallRegistryTests.swift-75-92 (1)
75-92:⚠️ Potential issue | 🟡 MinorBound the concurrent test with a timeout.
group.wait()will hang the whole XCTest run if this registry ever deadlocks or a future edit forgets toleave(). Assertinggroup.wait(timeout:) == .successwill fail cleanly instead of stalling CI.💡 Suggested change
- group.wait() + XCTAssertEqual(group.wait(timeout: .now() + 5), .success) XCTAssertEqual(registry.activeCallCount(), 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Rocket.ChatTests/ActiveCallRegistryTests.swift` around lines 75 - 92, The test_concurrentAccess uses group.wait() which can hang the test suite; change it to a bounded wait by replacing the unconditional group.wait() with a timed wait (e.g. group.wait(timeout: .now() + <reasonable seconds>)) and assert the result equals .success (XCTAssertEqual(result, .success)) so the test fails fast instead of stalling CI; update the assertions around registry.activeCallCount() to run only after confirming the timed wait succeeded.app/lib/services/voip/MediaSessionInstance.ts-18-18 (1)
18-18:⚠️ Potential issue | 🟡 MinorFix the new ESLint errors in this file.
The
restApiimport uses redundant path segments, and the directuseCallStore.getState().callreads tripprefer-destructuring. Those will keep lint red.Also applies to: 130-130, 163-163
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaSessionInstance.ts` at line 18, The import of mediaCallsStateSignals should remove the redundant path segment so it references the module directly (replace the '../../services/restApi' import with the module's correct direct path), and replace direct property reads like useCallStore.getState().call with destructuring to satisfy prefer-destructuring (e.g., const { call } = useCallStore.getState();) and apply that change to all occurrences (the ones calling useCallStore.getState().call and references to mediaCallsStateSignals).
🧹 Nitpick comments (4)
.github/workflows/test-ios.yml (2)
22-23: Consider hash-pinning GitHub Actions for supply chain security.Static analysis flagged the use of tag-based action references. While this is common practice, pinning to full SHA hashes provides stronger supply chain security guarantees.
Example for actions/checkout
- - name: Checkout Repository - uses: actions/checkout@v4 + - name: Checkout Repository + uses: actions/checkout@<full-sha> # v4You can find the current SHA for v4 via:
gh api repos/actions/checkout/git/refs/tags/v4 --jq '.object.sha'Also applies to: 29-29, 37-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-ios.yml around lines 22 - 23, Replace tag-based GitHub Action references with pinned commit SHAs: locate the uses: actions/checkout@v4 entry (and any other uses: entries in this workflow that reference tag versions) and change them to the corresponding full commit SHA for that tag (e.g., actions/checkout@<full-sha>) by fetching the tag's object.sha and updating the workflow file; ensure each action reference (including the one shown as uses: actions/checkout@v4 and the other tag-based uses in this workflow) is updated to its specific SHA to provide supply-chain protection.
50-54: Add-workspaceflag to use the generated CocoaPods workspace.This project uses CocoaPods and has a workspace (
ios/RocketChatRN.xcworkspace) that includes both the main project and dependencies. Explicitly specifying the workspace makes the build more reliable and follows best practices for CocoaPods projects.🔧 Proposed fix
run: | cd ios && xcodebuild test \ + -workspace RocketChatRN.xcworkspace \ -scheme Rocket.ChatTests \ -destination 'platform=iOS Simulator,name=iPhone 16' \ COMPILER_INDEX_STORE_DATA_PATH=${{ github.workspace }}/idx \ 2>&1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/test-ios.yml around lines 50 - 54, The xcodebuild invocation for running tests uses the scheme "Rocket.ChatTests" but omits the CocoaPods workspace; update the xcodebuild command (the line invoking "xcodebuild test" with "-scheme Rocket.ChatTests" and "COMPILER_INDEX_STORE_DATA_PATH") to include the workspace flag pointing to the generated workspace (e.g., add "-workspace RocketChatRN.xcworkspace" before the "-scheme" argument) so the build uses the CocoaPods-managed workspace that contains the project and dependencies.app/lib/services/voip/useCallStore.ts (1)
316-320: Annotate the new hook's return type.
useControlsVisibleis exported API surface, so spelling out(): booleanis worth it here.♻️ Small cleanup
-export const useControlsVisible = () => { +export const useControlsVisible = (): boolean => { const controlsVisible = useCallStore(state => state.controlsVisible); const isScreenReaderEnabled = useIsScreenReaderEnabled(); return controlsVisible || isScreenReaderEnabled; };As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/useCallStore.ts` around lines 316 - 320, The exported hook useControlsVisible lacks an explicit return type; update its signature to include an explicit boolean return type (e.g. change "export const useControlsVisible = () => { ... }" to "export const useControlsVisible = (): boolean => { ... }") so the exported API surface is properly annotated; keep the implementation using useCallStore and useIsScreenReaderEnabled unchanged.app/containers/InAppNotification/NotifierComponent.stories.tsx (1)
36-76: Add explicit JSX return types to the new story exports.
Wrapperand these story factories are exported TSX functions, soReact.JSX.Elementannotations would keep them aligned with the repo's strict TS rule and make accidental non-JSX returns noisier.As per coding guidelines,
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/containers/InAppNotification/NotifierComponent.stories.tsx` around lines 36 - 76, Add explicit React.JSX.Element return types to the exported JSX functions: annotate Wrapper and each story factory (DirectMessage, ChannelMessage, WithDarkTheme, WithBlackTheme) with a return type of React.JSX.Element; keep the existing prop type for Wrapper ({ children, theme = 'light' }: { children: React.ReactNode; theme?: TSupportedThemes }) and just append the return type to its signature, and likewise annotate the arrow functions for DirectMessage, ChannelMessage, WithDarkTheme and WithBlackTheme to return React.JSX.Element so they comply with the repo's strict TS rules and prevent accidental non-JSX returns when rendering NotifierComponent.
🤖 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 57-69: The code awaits mediaCallsStateSignals(...) before live
listeners are attached, so any DDP signals arriving during that network
round-trip are lost; change the order to wire live listeners (attach
sdk.onStreamData(...) and the newCall handler and call instance.register(false))
first, then fetch the REST state—either by moving the await
mediaCallsStateSignals(...) to after instance.register(false) or by kicking off
the fetch as a background Promise and awaiting it after listeners are
registered; when the REST response arrives, continue to feed signals into
instance.processSignal(...) as currently done.
In `@app/lib/services/voip/MediaSessionStore.ts`:
- Around line 70-71: The REST preload error currently only logs and then
unconditionally calls register(false) in MediaSessionStore, which leaves call
state stale until DDP arrives; update the catch block around the REST preload to
perform a recovery: implement a bounded retry with exponential backoff (or a
small fixed retry count) for the preload call, and if retries fail, fall back to
a deterministic local initialization before calling register (e.g., set a safe
default state or call register with requestInitialStateSignals=true to force a
subsequent fetch), so that register(...) is invoked only after either a
successful preload or an explicit fallback state; reference the REST preload
try/catch and the register(false) call in MediaSessionStore to locate where to
add retries/fallback.
In `@ios/Libraries/CallCoordinator.swift`:
- Around line 82-84: The .answering state currently treats .restAck by returning
(.active, []), but it does not handle .userDecline or .remoteHangup so the
machine can get stuck or be incorrectly promoted later; update the state handler
for .answering to explicitly handle (.answering, .userDecline) and (.answering,
.remoteHangup) by returning the terminal/cancelled state (e.g., .ended or
.cancelled) with any necessary side-effects instead of .noop, and adjust the
(.answering, .restAck) branch to only promote to .active when the call is not
already cancelled/ended (or simply ignore .restAck if already ended). Apply the
same fixes to the corresponding .answering branches referenced around lines
111-116.
- Around line 86-96: The CallKit UI never gets told to close because the state
machine cases in CallCoordinator.swift for (.active, .remoteHangup) and
(.ending, .ddpCallEnded) do not emit the endOS output; update those switch
branches to include the endOS output so VoipService.handleRemoteHangup and
handleDdpCallEnded will call endOS. Specifically, change the (.active,
.remoteHangup) case currently returning (.ending, []) to return (.ending,
[.endOS]) (or the equivalent output enum value), and change the (.ending,
.ddpCallEnded) case currently returning (.ended, []) to return (.ended,
[.endOS]) so the OS call is terminated.
In `@ios/Libraries/CallEndListener.swift`:
- Around line 15-16: The current startListening replaces
client.onCollectionMessage with a closure bound to the latest callId so end
events for earlier calls are lost; change this by installing a single persistent
handler (register client.onCollectionMessage once, e.g., in init or a lazy/once
guard) that parses the incoming message to extract callId and then routes to the
existing endedCallIds Set<String> (protected by lock) to mark or ignore ends, or
alternatively make the type explicitly single-call by removing endedCallIds and
keeping the existing per-startListening closure but enforcing only one active
call; update startListening and any teardown logic (stopListening) to avoid
re-registering/overwriting the collection message handler.
- Around line 49-58: The contains/remove sequence on endedCallIds is not atomic:
both checks can pass under two locks and dispatch duplicate callEnded(id:).
Acquire lock once around the check-and-remove: inside a single lock() / unlock()
block check if endedCallIds contains(callId); if it does remove(callId) and
allow dispatch, otherwise return—this ensures the dedupe and removal occur
atomically using the existing lock and the endedCallIds set.
In `@ios/Libraries/VoipService.swift`:
- Around line 137-148: The singleton currentState is left in .ended so later
.incomingPush inputs are no-ops; before calling coordinator.transition in
reportIncomingCall (and the other handlers around the same area, e.g., the
functions handling incoming pushes between lines ~165-207), detect if
currentState == .ended and reset it to .idle (or call a coordinator.reset() if
available) so the state machine can transition and ring again; then proceed to
call coordinator.transition(state: currentState, input: .incomingPush) and
handle outputs (ringOS(payload:)) as before.
- Around line 189-194: The coordinator is moved to .answering by
handleUserAnswer() but the success path never feeds a .restAck, leaving
currentState stuck; update the code path that handles a successful native/REST
accept (e.g., in handleUserAnswer() or the callback that receives JS/REST
success) to call the existing handleRestAck() or directly invoke
coordinator.transition(state: currentState, input: .restAck) and assign
currentState to the returned newState so the CallCoordinator receives the
.restAck input and can leave the .answering state (refer to handleUserAnswer,
handleRestAck, coordinator.transition, currentState, and CallCoordinator).
In `@ios/Rocket.ChatTests/VoipPayloadTests.swift`:
- Around line 130-142: The test asserts that VoipPayload.fromDictionary rejects
payloads with type "call_ended", empty caller.name, and empty host, but the
parser (VoipPayload.fromDictionary) currently allows those and instead exposes
isVoipIncomingCall() to indicate incoming status; update the test to call
VoipPayload.fromDictionary(...) for each case (e.g., wrongTypePayload,
emptyCaller, emptyHost) and assert the returned payload is non-nil then assert
payload.isVoipIncomingCall() == false for non-incoming cases; for the missing
callId case keep XCTAssertNil(VoipPayload.fromDictionary(missingCallId)) since
the parser rejects invalid/missing UUIDs. Ensure references to
VoipPayload.fromDictionary and isVoipIncomingCall are used so tests match the
parser contract.
In `@ios/RocketChatRN.xcodeproj/project.pbxproj`:
- Around line 2355-2363: The PBXSourcesBuildPhase (isa = PBXSourcesBuildPhase,
id AABBCC0512345678900A75B9) currently lists four test files but is missing the
entry for VoipRESTClientTests.swift, so the new test suite never gets built; add
the file reference for VoipRESTClientTests.swift into the files array of that
PBXSourcesBuildPhase (and ensure the corresponding PBXBuildFile /
PBXFileReference for VoipRESTClientTests.swift exists and is assigned to the
test target) so the test file is included in the test target build.
- Around line 2124-2126: The project file is missing
MediaCallsAnswerRequest.swift but VoipRESTClient.swift (which defines methods
accept/reject) constructs the MediaCallsAnswerRequest type; re-add the
MediaCallsAnswerRequest.swift entry to the Sources list in the project.pbxproj
(the same PBX group that contains VoipRESTClient.swift and VoipService.swift)
and ensure its file reference entry appears before VoipRESTClient.swift so the
compiler can resolve the MediaCallsAnswerRequest symbol used in
ios/Libraries/VoipRESTClient.swift (references: MediaCallsAnswerRequest,
VoipRESTClient.swift).
- Around line 318-319: The project file defines two PBXBuildFile entries for
VoipRESTClient.swift (7A1B58502F5F58FF002A6BDE and 7A1B58512F5F58FF002A6BDE) but
only the first is included in a PBXSourcesBuildPhase, leaving the Rocket.Chat
target compiling VoipService.swift without VoipRESTClient; open the
PBXSourcesBuildPhase for the Rocket.Chat app target and add the missing build
file reference 7A1B58512F5F58FF002A6BDE to its "files" array (or remove the
duplicate PBXBuildFile and ensure a single correct PBXBuildFile entry pointing
to the VoipRESTClient.swift fileRef is present and referenced by both targets),
ensuring VoipRESTClient.swift is included for the Rocket.Chat target so
VoipService.swift can instantiate it.
- Around line 1383-1386: The RocketChatTests target references a missing
PBXFrameworksBuildPhase (AABBCC0612345678900A75B9) in its buildPhases alongside
AABBCC0512345678900A75B9, which breaks the target graph; fix by either adding a
PBXFrameworksBuildPhase entry with isa = PBXFrameworksBuildPhase, that exact ID
(AABBCC0612345678900A75B9), an empty or appropriate files array and
runOnlyForDeploymentPostprocessing = 0, placed with the other PBX... objects, or
remove the AABBCC0612345678900A75B9 reference from the RocketChatTests
buildPhases list so only valid build phase IDs remain.
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.test.ts`:
- Around line 613-644: The tests build a mainCall object but never expose it to
the system because useCallStore.getState() still returns call: null; update both
tests to inject the constructed mainCall into the call store before calling
mediaSessionInstance.answerCall (e.g., mock useCallStore.getState to return {
call: mainCall } or set the store state accordingly) so
mediaSessionInstance.answerCall sees the mainCall; do this for both the DM test
and the SIP-variant test (the latter should provide
localParticipant.contact.sipExtension = 'ext') and keep assertions on
mockGetDMSubscriptionByUsername and mockSetRoomId unchanged.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 103-115: The newCall handler in MediaSessionInstance only sets the
call into the store when localParticipant.role === 'caller', leaving inbound
(callee) calls without a local source of truth; update the handler used in the
'newCall' listener (and the similar block around lines 124-176) to always call
useCallStore.getState().setCall(call) for incoming calls (both caller and
callee), attach the call.emitter.on('stateChange', ...) handler for callee calls
as well, and ensure resolveRoomIdFromContact(call.localParticipant.contact) is
invoked or skipped appropriately regardless of role so native accept/decline
will route to answerCall()/endCall() instead of falling through to CallKeep
handling.
---
Minor comments:
In `@app/i18n/locales/fi.json`:
- Line 779: The translation for the key "Toggle_call_controls" uses
non-idiomatic Finnish "hallintoja"; update the value for the
"Toggle_call_controls" JSON key to a more natural UI term such as "Vaihda
puhelun ohjaimet" (or "Vaihda puhelun säätimet" if your glossary prefers
"säätimet") so the label reads naturally in the UI.
In `@app/i18n/locales/pt-PT.json`:
- Line 499: Update the pt-PT translation for the "Toggle_call_controls" key to
use European Portuguese spelling: replace the value "Alternar controles de
chamada" with "Alternar controlos de chamada" so the key Toggle_call_controls
uses "controlos" (pt-PT) instead of "controles" (pt-BR).
In `@app/lib/services/restApi.ts`:
- Line 17: Move the external type import for ServerMediaSignal so it precedes
local imports: place "import type { ServerMediaSignal } from
'@rocket.chat/media-signaling'" before the local "../../definitions" import
statements in restApi.ts to satisfy ESLint import ordering rules (look for the
ServerMediaSignal import and the ../../definitions import to reorder).
- Around line 1220-1232: The mediaCallsStateSignals function has Prettier
formatting violations and an unnecessary any-style cast on sdk.get; reformat the
function body to match project Prettier rules (consistent spacing, line breaks
and parentheses) and remove the manual cast by updating the SDK typing or adding
a typed wrapper for get so you can call sdk.get<'media-calls.stateSignals'
returnType>('media-calls.stateSignals', { contractId }) directly; keep the same
return shape ({ signals: ServerMediaSignal[]; success: boolean }) and preserve
the try/catch behavior.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Line 18: The import of mediaCallsStateSignals should remove the redundant path
segment so it references the module directly (replace the
'../../services/restApi' import with the module's correct direct path), and
replace direct property reads like useCallStore.getState().call with
destructuring to satisfy prefer-destructuring (e.g., const { call } =
useCallStore.getState();) and apply that change to all occurrences (the ones
calling useCallStore.getState().call and references to mediaCallsStateSignals).
In `@app/lib/services/voip/useCallStore.test.ts`:
- Around line 48-51: The remoteParticipants array in useCallStore.test.ts is
mis-indented and failing Prettier; update the formatting of the
remoteParticipants property (in the test setup where remoteParticipants is
defined) so each array element and its properties are properly indented
according to project Prettier rules (move the object on its own line and indent
its keys), then run the formatter or tests to confirm the lint error is
resolved.
In `@ios/Rocket.ChatTests/ActiveCallRegistryTests.swift`:
- Around line 75-92: The test_concurrentAccess uses group.wait() which can hang
the test suite; change it to a bounded wait by replacing the unconditional
group.wait() with a timed wait (e.g. group.wait(timeout: .now() + <reasonable
seconds>)) and assert the result equals .success (XCTAssertEqual(result,
.success)) so the test fails fast instead of stalling CI; update the assertions
around registry.activeCallCount() to run only after confirming the timed wait
succeeded.
In `@ios/Rocket.ChatTests/CallCoordinatorTests.swift`:
- Around line 68-179: The test suite misses five noop branches in
CallCoordinator.transition: (.incoming, .incomingPush), (.answering,
.incomingPush), (.active, .incomingPush), (.active, .ddpCallEnded) and (.ending,
.incomingPush); update CallCoordinatorTests by adding a small table-driven loop
that iterates over these (state, input) pairs and asserts the transition returns
the same state and outputs == [.noop] (use the existing
coordinator.transition(state:input:) helper and CallInput/.incomingPush and
.ddpCallEnded symbols), mirroring the pattern used in
test_ended_anyInput_illegal to avoid hardcoding more individual test methods.
In `@ios/Rocket.ChatTests/VoipRESTClientTests.swift`:
- Around line 62-120: testVoipRESTClientImplementsProtocol only instantiates
VoipRESTClient but doesn't verify its accept/reject behavior; add tests that
exercise the real VoipRESTClient.accept(payload:completion:) and
.reject(payload:completion:) paths by injecting a controllable network layer
(e.g., a URLSession/HTTPClient stub or the same mock used by
FakeVoipRESTClient), configure the mock to return success/failure Bool JSON
responses, call VoipRESTClient.accept and .reject with a payload, and assert the
completion Bool mapping and that the outgoing request contains the expected
callId and endpoint; reference VoipRESTClient, accept(payload:completion:),
reject(payload:completion:) and reuse patterns from FakeVoipRESTClient tests
when asserting call counts and payloads.
In `@package.json`:
- Line 50: The dependency "@rocket.chat/media-signaling" is pinned to an RC
tarball ("0.2.0-rc.0"); before merging confirm stability or replace with the
stable release: verify that the 0.2.0 release is production-ready (run
integration tests and validate imports in useCallStore.ts,
MediaSessionInstance.ts, MediaSessionStore.ts and related tests) and then update
package.json to point to the published stable version (or document a migration
plan/feature flag if holding the RC), ensuring all imports and type
compatibility remain intact.
---
Nitpick comments:
In @.github/workflows/test-ios.yml:
- Around line 22-23: Replace tag-based GitHub Action references with pinned
commit SHAs: locate the uses: actions/checkout@v4 entry (and any other uses:
entries in this workflow that reference tag versions) and change them to the
corresponding full commit SHA for that tag (e.g., actions/checkout@<full-sha>)
by fetching the tag's object.sha and updating the workflow file; ensure each
action reference (including the one shown as uses: actions/checkout@v4 and the
other tag-based uses in this workflow) is updated to its specific SHA to provide
supply-chain protection.
- Around line 50-54: The xcodebuild invocation for running tests uses the scheme
"Rocket.ChatTests" but omits the CocoaPods workspace; update the xcodebuild
command (the line invoking "xcodebuild test" with "-scheme Rocket.ChatTests" and
"COMPILER_INDEX_STORE_DATA_PATH") to include the workspace flag pointing to the
generated workspace (e.g., add "-workspace RocketChatRN.xcworkspace" before the
"-scheme" argument) so the build uses the CocoaPods-managed workspace that
contains the project and dependencies.
In `@app/containers/InAppNotification/NotifierComponent.stories.tsx`:
- Around line 36-76: Add explicit React.JSX.Element return types to the exported
JSX functions: annotate Wrapper and each story factory (DirectMessage,
ChannelMessage, WithDarkTheme, WithBlackTheme) with a return type of
React.JSX.Element; keep the existing prop type for Wrapper ({ children, theme =
'light' }: { children: React.ReactNode; theme?: TSupportedThemes }) and just
append the return type to its signature, and likewise annotate the arrow
functions for DirectMessage, ChannelMessage, WithDarkTheme and WithBlackTheme to
return React.JSX.Element so they comply with the repo's strict TS rules and
prevent accidental non-JSX returns when rendering NotifierComponent.
In `@app/lib/services/voip/useCallStore.ts`:
- Around line 316-320: The exported hook useControlsVisible lacks an explicit
return type; update its signature to include an explicit boolean return type
(e.g. change "export const useControlsVisible = () => { ... }" to "export const
useControlsVisible = (): boolean => { ... }") so the exported API surface is
properly annotated; keep the implementation using useCallStore and
useIsScreenReaderEnabled unchanged.
🪄 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: 858012ec-5fd6-4e3f-a3e4-881d3413b8c9
⛔ Files ignored due to path filters (9)
app/containers/Header/components/HeaderButton/__snapshots__/HeaderButtons.test.tsx.snapis excluded by!**/*.snapapp/containers/InAppNotification/__snapshots__/NotifierComponent.test.tsx.snapis excluded by!**/*.snapapp/containers/NewMediaCall/__snapshots__/PeerItem.test.tsx.snapis excluded by!**/*.snapapp/containers/NewMediaCall/__snapshots__/PeerList.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/__snapshots__/index.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/components/Dialpad/__snapshots__/Dialpad.test.tsx.snapis excluded by!**/*.snapapp/views/CallView/components/__snapshots__/CallerInfo.test.tsx.snapis excluded by!**/*.snapios/Podfile.lockis excluded by!**/*.lockyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (71)
.github/workflows/test-ios.yml.gitignoreCLAUDE.mdapp/containers/InAppNotification/NotifierComponent.stories.tsxapp/containers/InAppNotification/NotifierComponent.test.tsxapp/containers/NewMediaCall/PeerItem.tsxapp/i18n/locales/ar.jsonapp/i18n/locales/bn-IN.jsonapp/i18n/locales/cs.jsonapp/i18n/locales/de.jsonapp/i18n/locales/en.jsonapp/i18n/locales/es.jsonapp/i18n/locales/fi.jsonapp/i18n/locales/fr.jsonapp/i18n/locales/hi-IN.jsonapp/i18n/locales/hu.jsonapp/i18n/locales/it.jsonapp/i18n/locales/ja.jsonapp/i18n/locales/nl.jsonapp/i18n/locales/nn.jsonapp/i18n/locales/no.jsonapp/i18n/locales/pt-BR.jsonapp/i18n/locales/pt-PT.jsonapp/i18n/locales/ru.jsonapp/i18n/locales/sl-SI.jsonapp/i18n/locales/sv.jsonapp/i18n/locales/ta-IN.jsonapp/i18n/locales/te-IN.jsonapp/i18n/locales/tr.jsonapp/i18n/locales/zh-CN.jsonapp/i18n/locales/zh-TW.jsonapp/lib/hooks/useIsScreenReaderEnabled.test.tsapp/lib/hooks/useIsScreenReaderEnabled.tsapp/lib/services/restApi.test.tsapp/lib/services/restApi.tsapp/lib/services/voip/MediaCallEvents.ios.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionStore.tsapp/lib/services/voip/mockCall.tsapp/lib/services/voip/useCallStore.test.tsapp/lib/services/voip/useCallStore.tsapp/views/CallView/components/CallButtons.test.tsxapp/views/CallView/components/CallButtons.tsxapp/views/CallView/components/CallerInfo.tsxapp/views/CallView/components/Dialpad/DialpadButton.tsxapp/views/CallView/index.test.tsxapp/views/CallView/index.tsxapp/views/CallView/styles.tsios/Libraries/ActiveCallRegistry.swiftios/Libraries/CallCoordinator.swiftios/Libraries/CallEndListener.swiftios/Libraries/DDPClient.swiftios/Libraries/DDPClientProtocol.swiftios/Libraries/VoipPerCallDdpRegistry.swiftios/Libraries/VoipRESTClient.swiftios/Libraries/VoipService.swiftios/NotificationService/NotificationService-Bridging-Header.hios/Podfileios/Rocket.ChatTests/ActiveCallRegistryTests.swiftios/Rocket.ChatTests/CallCoordinatorTests.swiftios/Rocket.ChatTests/CallEndListenerTests.swiftios/Rocket.ChatTests/VoipPayloadTests.swiftios/Rocket.ChatTests/VoipRESTClientTests.swiftios/RocketChatRN.xcodeproj/project.pbxprojios/RocketChatRN.xcodeproj/xcshareddata/xcschemes/Rocket.ChatTests.xcschemejest.config.jspackage.jsonpackages/rocket.chat-media-signaling-0.1.3.tgzpackages/rocket.chat-media-signaling-0.2.0-rc.0.tgz
💤 Files with no reviewable changes (2)
- app/views/CallView/index.test.tsx
- app/i18n/locales/te-IN.json
Comment on lines
+57
to
+69
| if (instance) { | ||
| // Fetch initial call state via REST before DDP register fires | ||
| try { | ||
| const { signals } = await mediaCallsStateSignals(getUniqueIdSync()); | ||
| for (const signal of signals) { | ||
| instance.processSignal(signal); | ||
| } | ||
| } catch (error) { | ||
| console.error('[VoIP] Failed to fetch initial state signals:', error); | ||
| } | ||
|
|
||
| instance.register(false); | ||
| } |
Contributor
There was a problem hiding this comment.
Don't wait for REST before wiring live signal listeners.
This await mediaCallsStateSignals(...) runs before sdk.onStreamData(...) and the newCall handler are attached. Any DDP signal that lands during that network round-trip is dropped, which creates a cold-start race exactly while you're trying to recover the current call state.
🤖 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 57 - 69, The code
awaits mediaCallsStateSignals(...) before live listeners are attached, so any
DDP signals arriving during that network round-trip are lost; change the order
to wire live listeners (attach sdk.onStreamData(...) and the newCall handler and
call instance.register(false)) first, then fetch the REST state—either by moving
the await mediaCallsStateSignals(...) to after instance.register(false) or by
kicking off the fetch as a background Promise and awaiting it after listeners
are registered; when the REST response arrives, continue to feed signals into
instance.processSignal(...) as currently done.
Comment on lines
+70
to
+71
| mobileDeviceId, | ||
| requestInitialStateSignals: false |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm requestInitialStateSignals is disabled in MediaSessionStore"
rg -n "requestInitialStateSignals" app/lib/services/voip/MediaSessionStore.ts
echo
echo "2) Inspect init preload flow and post-failure behavior"
rg -n -C4 "mediaCallsStateSignals|register\\(" app/lib/services/voip/MediaSessionInstance.tsRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1340
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Examine register() method to understand false parameter ==="
rg -n -A10 "register\s*\(" app/lib/services/voip/MediaSessionInstance.ts | head -30
echo
echo "=== 2) Check if there's DDP handling for initial state or delta sync ==="
rg -n "processSignal|DDP\|ddp\|subscribe" app/lib/services/voip/MediaSessionInstance.ts | head -20
echo
echo "=== 3) Look for any retry/timeout mechanisms in broader context ==="
rg -n "retry\|timeout\|Timeout\|Retry" app/lib/services/voip/
echo
echo "=== 4) Check MediaSessionStore for any sync/fallback logic ==="
cat -n app/lib/services/voip/MediaSessionStore.ts | head -100Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 722
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Find register() method implementation ==="
rg -n "^\s*register\s*\(" app/lib/services/voip/ -A5
echo
echo "=== 2) Check if MediaSessionInstance extends another class ==="
head -40 app/lib/services/voip/MediaSessionInstance.ts | grep -n "class\|extends\|import"
echo
echo "=== 3) Full context of init method to understand flow ==="
rg -n -B5 -A15 "init\s*\(" app/lib/services/voip/MediaSessionInstance.ts | head -50Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 126
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== 1) Read the full MediaSessionInstance.ts file (check size first) ==="
wc -l app/lib/services/voip/MediaSessionInstance.ts
echo
echo "=== 2) Show full file content ==="
cat -n app/lib/services/voip/MediaSessionInstance.ts
echo
echo "=== 3) Search for register method in entire app directory ==="
rg -n "^\s*register\s*\(" app/ --max-count=5 -A3Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 9758
Add retry logic or explicit fallback when REST preload fails.
At line 68, register(false) is called unconditionally even if the REST preload for initial state signals fails (lines 59–66). The catch block logs the error but takes no recovery action, leaving the call state potentially stale until a DDP signal arrives via the listener at line 75.
While DDP eventually syncs state, this gap introduces unnecessary latency and silent error recovery. Consider adding retry logic in the catch block or documenting the expected behavior and acceptable sync delay.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/lib/services/voip/MediaSessionStore.ts` around lines 70 - 71, The REST
preload error currently only logs and then unconditionally calls register(false)
in MediaSessionStore, which leaves call state stale until DDP arrives; update
the catch block around the REST preload to perform a recovery: implement a
bounded retry with exponential backoff (or a small fixed retry count) for the
preload call, and if retries fail, fall back to a deterministic local
initialization before calling register (e.g., set a safe default state or call
register with requestInitialStateSignals=true to force a subsequent fetch), so
that register(...) is invoked only after either a successful preload or an
explicit fallback state; reference the REST preload try/catch and the
register(false) call in MediaSessionStore to locate where to add
retries/fallback.
Comment on lines
+82
to
+84
| // answering -> active (REST ack received) | ||
| case (.answering, .restAck): | ||
| return (.active, []) |
Contributor
There was a problem hiding this comment.
answering should not ignore hangup/cancel events.
While waiting for restAck, both userDecline and remoteHangup are reachable. Returning .noop leaves the machine stuck in .answering, and a later restAck can incorrectly promote a cancelled call to .active.
Also applies to: 111-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Libraries/CallCoordinator.swift` around lines 82 - 84, The .answering
state currently treats .restAck by returning (.active, []), but it does not
handle .userDecline or .remoteHangup so the machine can get stuck or be
incorrectly promoted later; update the state handler for .answering to
explicitly handle (.answering, .userDecline) and (.answering, .remoteHangup) by
returning the terminal/cancelled state (e.g., .ended or .cancelled) with any
necessary side-effects instead of .noop, and adjust the (.answering, .restAck)
branch to only promote to .active when the call is not already cancelled/ended
(or simply ignore .restAck if already ended). Apply the same fixes to the
corresponding .answering branches referenced around lines 111-116.
Comment on lines
+86
to
+96
| // active -> ending (remote hangs up) | ||
| case (.active, .remoteHangup): | ||
| return (.ending, []) | ||
|
|
||
| // active -> ending (user ends call) | ||
| case (.active, .userDecline): | ||
| return (.ending, []) | ||
|
|
||
| // ending -> ended (DDP call-ended received) | ||
| case (.ending, .ddpCallEnded): | ||
| return (.ended, []) |
Contributor
There was a problem hiding this comment.
The end-of-call path never tells the OS to end the call.
In ios/Libraries/VoipService.swift, handleRemoteHangup and handleDdpCallEnded only call endOS when a transition emits endOS. The .active -> .ending and .ending -> .ended transitions currently emit no such output, so an already-active call can reach .ended without ever closing the CallKit UI.
📞 Minimal fix
- case (.ending, .ddpCallEnded):
- return (.ended, [])
+ case (.ending, .ddpCallEnded):
+ return (.ended, [.endOS])📝 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.
Suggested change
| // active -> ending (remote hangs up) | |
| case (.active, .remoteHangup): | |
| return (.ending, []) | |
| // active -> ending (user ends call) | |
| case (.active, .userDecline): | |
| return (.ending, []) | |
| // ending -> ended (DDP call-ended received) | |
| case (.ending, .ddpCallEnded): | |
| return (.ended, []) | |
| // active -> ending (remote hangs up) | |
| case (.active, .remoteHangup): | |
| return (.ending, []) | |
| // active -> ending (user ends call) | |
| case (.active, .userDecline): | |
| return (.ending, []) | |
| // ending -> ended (DDP call-ended received) | |
| case (.ending, .ddpCallEnded): | |
| return (.ended, [.endOS]) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Libraries/CallCoordinator.swift` around lines 86 - 96, The CallKit UI
never gets told to close because the state machine cases in
CallCoordinator.swift for (.active, .remoteHangup) and (.ending, .ddpCallEnded)
do not emit the endOS output; update those switch branches to include the endOS
output so VoipService.handleRemoteHangup and handleDdpCallEnded will call endOS.
Specifically, change the (.active, .remoteHangup) case currently returning
(.ending, []) to return (.ending, [.endOS]) (or the equivalent output enum
value), and change the (.ending, .ddpCallEnded) case currently returning
(.ended, []) to return (.ended, [.endOS]) so the OS call is terminated.
Comment on lines
+15
to
+16
| private var endedCallIds = Set<String>() | ||
| private let lock = NSLock() |
Contributor
There was a problem hiding this comment.
Starting a second call drops the first one.
endedCallIds is a set, so this object looks multi-call aware, but Line 31 replaces client.onCollectionMessage with a closure bound to only the latest callId. After startListening is called for call B, end events for call A are no longer observable. Either install one handler that routes by parsed callId, or make this type explicitly single-call.
Also applies to: 24-33
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Libraries/CallEndListener.swift` around lines 15 - 16, The current
startListening replaces client.onCollectionMessage with a closure bound to the
latest callId so end events for earlier calls are lost; change this by
installing a single persistent handler (register client.onCollectionMessage
once, e.g., in init or a lazy/once guard) that parses the incoming message to
extract callId and then routes to the existing endedCallIds Set<String>
(protected by lock) to mark or ignore ends, or alternatively make the type
explicitly single-call by removing endedCallIds and keeping the existing
per-startListening closure but enforcing only one active call; update
startListening and any teardown logic (stopListening) to avoid
re-registering/overwriting the collection message handler.
Comment on lines
+130
to
+142
| func test_isVoipIncomingCall_invalid() { | ||
| let wrongTypePayload = makeValidPayload(type: "call_ended") | ||
| XCTAssertNil(VoipPayload.fromDictionary(wrongTypePayload)) | ||
| var missingCallId = makeValidPayload() | ||
| missingCallId["callId"] = "" | ||
| XCTAssertNil(VoipPayload.fromDictionary(missingCallId)) | ||
| var emptyCaller = makeValidPayload() | ||
| emptyCaller["caller"] = ["name": "", "username": "johndoe", "avatarUrl": ""] | ||
| XCTAssertNil(VoipPayload.fromDictionary(emptyCaller)) | ||
| var emptyHost = makeValidPayload() | ||
| emptyHost["host"] = "" | ||
| XCTAssertNil(VoipPayload.fromDictionary(emptyHost)) | ||
| } |
Contributor
There was a problem hiding this comment.
These assertions don't match the current parser contract.
VoipPayload.fromDictionary() in ios/Libraries/VoipPayload.swift:17-74 doesn't reject type == "call_ended" or empty caller.name / host; it only rejects non-VoIP notifications, invalid UUIDs, missing required fields, and empty createdAt. As written, this case will fail unless production validation is tightened too. If the goal is to cover non-incoming payloads, decode the payload and assert isVoipIncomingCall() == false instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Rocket.ChatTests/VoipPayloadTests.swift` around lines 130 - 142, The test
asserts that VoipPayload.fromDictionary rejects payloads with type "call_ended",
empty caller.name, and empty host, but the parser (VoipPayload.fromDictionary)
currently allows those and instead exposes isVoipIncomingCall() to indicate
incoming status; update the test to call VoipPayload.fromDictionary(...) for
each case (e.g., wrongTypePayload, emptyCaller, emptyHost) and assert the
returned payload is non-nil then assert payload.isVoipIncomingCall() == false
for non-incoming cases; for the missing callId case keep
XCTAssertNil(VoipPayload.fromDictionary(missingCallId)) since the parser rejects
invalid/missing UUIDs. Ensure references to VoipPayload.fromDictionary and
isVoipIncomingCall are used so tests match the parser contract.
Comment on lines
+318
to
+319
| 7A1B58502F5F58FF002A6BDE /* VoipRESTClient.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A1B58522F5F58FF002A6BDE /* VoipRESTClient.swift */; }; | ||
| 7A1B58512F5F58FF002A6BDE /* VoipRESTClient.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A1B58522F5F58FF002A6BDE /* VoipRESTClient.swift */; }; |
Contributor
There was a problem hiding this comment.
Only one app target actually compiles VoipRESTClient.swift.
These two build files suggest both app targets should get the new source, but only 7A1B58502F5F58FF002A6BDE is later added to a PBXSourcesBuildPhase. 7A1B58512F5F58FF002A6BDE never is, so the Rocket.Chat target still compiles VoipService.swift without the VoipRESTClient implementation it now instantiates.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/RocketChatRN.xcodeproj/project.pbxproj` around lines 318 - 319, The
project file defines two PBXBuildFile entries for VoipRESTClient.swift
(7A1B58502F5F58FF002A6BDE and 7A1B58512F5F58FF002A6BDE) but only the first is
included in a PBXSourcesBuildPhase, leaving the Rocket.Chat target compiling
VoipService.swift without VoipRESTClient; open the PBXSourcesBuildPhase for the
Rocket.Chat app target and add the missing build file reference
7A1B58512F5F58FF002A6BDE to its "files" array (or remove the duplicate
PBXBuildFile and ensure a single correct PBXBuildFile entry pointing to the
VoipRESTClient.swift fileRef is present and referenced by both targets),
ensuring VoipRESTClient.swift is included for the Rocket.Chat target so
VoipService.swift can instantiate it.
Comment on lines
+1383
to
+1386
| buildPhases = ( | ||
| AABBCC0512345678900A75B9 /* Sources */, | ||
| AABBCC0612345678900A75B9 /* Frameworks */, | ||
| ); |
Contributor
There was a problem hiding this comment.
Rocket.ChatTests references a missing build phase.
buildPhases includes AABBCC0612345678900A75B9 /* Frameworks */, but there is no matching PBXFrameworksBuildPhase object anywhere in this project file. That leaves the target graph malformed and can prevent Xcode from loading or building the new test target.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/RocketChatRN.xcodeproj/project.pbxproj` around lines 1383 - 1386, The
RocketChatTests target references a missing PBXFrameworksBuildPhase
(AABBCC0612345678900A75B9) in its buildPhases alongside
AABBCC0512345678900A75B9, which breaks the target graph; fix by either adding a
PBXFrameworksBuildPhase entry with isa = PBXFrameworksBuildPhase, that exact ID
(AABBCC0612345678900A75B9), an empty or appropriate files array and
runOnlyForDeploymentPostprocessing = 0, placed with the other PBX... objects, or
remove the AABBCC0612345678900A75B9 reference from the RocketChatTests
buildPhases list so only valid build phase IDs remain.
Comment on lines
2124
to
2126
| 7A1B58422F5F58FF002A6BDE /* VoipPayload.swift in Sources */, | ||
| 7A1B58502F5F58FF002A6BDE /* VoipRESTClient.swift in Sources */, | ||
| 7A0000012F1BAFA700B6B4BD /* VoipService.swift in Sources */, |
Contributor
There was a problem hiding this comment.
Re-add MediaCallsAnswerRequest.swift before compiling VoipRESTClient.swift.
VoipRESTClient.swift is now part of the app sources here, and it constructs MediaCallsAnswerRequest in both accept and reject (ios/Libraries/VoipRESTClient.swift:15-64). This project file no longer references that request source anywhere, so the iOS build will fail on an undefined type.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/RocketChatRN.xcodeproj/project.pbxproj` around lines 2124 - 2126, The
project file is missing MediaCallsAnswerRequest.swift but VoipRESTClient.swift
(which defines methods accept/reject) constructs the MediaCallsAnswerRequest
type; re-add the MediaCallsAnswerRequest.swift entry to the Sources list in the
project.pbxproj (the same PBX group that contains VoipRESTClient.swift and
VoipService.swift) and ensure its file reference entry appears before
VoipRESTClient.swift so the compiler can resolve the MediaCallsAnswerRequest
symbol used in ios/Libraries/VoipRESTClient.swift (references:
MediaCallsAnswerRequest, VoipRESTClient.swift).
Comment on lines
+2355
to
+2363
| AABBCC0512345678900A75B9 /* Sources */ = { | ||
| isa = PBXSourcesBuildPhase; | ||
| buildActionMask = 2147483647; | ||
| files = ( | ||
| AABBCC0012345678900A75B9 /* VoipPayloadTests.swift in Sources */, | ||
| AABBCC0022345678900A75B9 /* CallCoordinatorTests.swift in Sources */, | ||
| AABBCC0032345678900A75B9 /* ActiveCallRegistryTests.swift in Sources */, | ||
| AABBCC0042345678900A75B9 /* CallEndListenerTests.swift in Sources */, | ||
| ); |
Contributor
There was a problem hiding this comment.
VoipRESTClientTests.swift never enters the test target.
The build file for VoipRESTClientTests.swift exists, but this PBXSourcesBuildPhase only includes four test files. That means the new REST client suite won't compile or run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/RocketChatRN.xcodeproj/project.pbxproj` around lines 2355 - 2363, The
PBXSourcesBuildPhase (isa = PBXSourcesBuildPhase, id AABBCC0512345678900A75B9)
currently lists four test files but is missing the entry for
VoipRESTClientTests.swift, so the new test suite never gets built; add the file
reference for VoipRESTClientTests.swift into the files array of that
PBXSourcesBuildPhase (and ensure the corresponding PBXBuildFile /
PBXFileReference for VoipRESTClientTests.swift exists and is assigned to the
test target) so the test file is included in the test target build.
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
2 participants
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.
Summary
iOS VoIP refactor — extracts pure Swift components from
VoipServicefor testability:CXProvider/PKPushRegistry/AVAudioSessiontypes cross the boundary.[callId: VoipPayload]registry replacing the oldobservedIncomingCallsdictionary. Protocol-based for testability.VoipServicetakes the protocol; real impl usesAPI.fetch(request: MediaCallsAnswerRequest).xcodebuild test -scheme Rocket.ChatTestson PRs touchingios/**.Rebased onto
refactor.ddp-ios(PR 7124) which introducedMediaCallsAnswerRequestREST accept/reject.Test plan
xcodebuild test -scheme Rocket.ChatTestspasses locallyFiles
ios/Libraries/CallCoordinator.swiftios/Libraries/ActiveCallRegistry.swiftios/Libraries/VoipRESTClient.swiftios/Libraries/CallEndListener.swiftios/Libraries/DDPClientProtocol.swiftios/Libraries/VoipService.swiftios/Libraries/VoipPerCallDdpRegistry.swiftios/Rocket.ChatTests/*.swift.github/workflows/test-ios.ymlPart of stacked PR stack: 1a→1b→2→3→4a→4b→5→6
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation
Tests