fix(voip): pending-accept test mocks#7239
Conversation
Phase 1 of Strategy C (native holds call until JS signals readiness). - useCallStore: add PENDING_ACCEPT_TTL_MS = 15_000 (native TTL, NOT STALE_NATIVE_MS) - MediaCallEvents: add isCallIdQueued() + queuedCallIds Set for pending-accept queue - MediaSessionInstance: add pendingInitCallbacks + registerOnInitComplete for JS-ready handoff - MediaSessionInstance: add tryAnswerIfNativeAcceptedNotification race guard — blocks live DDP notification/accepted from firing answerCall for queued callIds; proceedAccept is the sole answerCall trigger during pending window - MediaSessionInstance: add invariant class comment documenting the protocol - MediaSessionInstance.test.ts: comprehensive suite covering init, teardown, newCall, stream-notify-user gating, REST state replay, roomId population
… pendingAccept payload - Add `handleVoipPendingAccept()` to MediaCallEvents.ts: queues callId, navigates to CallView, registers init-complete callback to call `NativeVoipModule.proceedAccept(callId)`. - Add `clearPendingCallView()` helper that resets native call id and navigates back from CallView. - Add `pendingAccept?: boolean` to VoipPayload interface and all native implementations (Swift, Kotlin). - Add `proceedAccept(callId)` to NativeVoip.ts, VoipModule.mm, and VoipModule.kt (no-op on Android). - Add `VoipPendingAccept` to iOS/Android supported events. - iOS VoipService: store pending-accept payloads and re-invoke `handleNativeAccept` on `proceedAccept` after JS init completes. - Extend `handleVoipAcceptSucceeded` and `handleVoipAcceptFailed` to clean up queued callIds. - Extend cold-start `getInitialMediaCallEvents` to detect `initialEvents.pendingAccept` and route to `handleVoipPendingAccept`.
- Add setContact to useCallStore for seeding caller info during pending accept - handleVoipPendingAccept seeds contact from VoipPayload (caller/username) - CallView renders when call === null && nativeAcceptedCallId !== null (connecting state) - CallerInfo shows "Connecting…" label with fontHint color in connecting state - CallButtons hidden in connecting state - Update stories to test connecting state with null call + nativeAcceptedCallId
…itter emit Tests AC1, AC5, AC7 were failing because DeviceEventEmitter.emit() from tests didn't reach handlers registered via NativeEventEmitter.addListener(). This happened because NativeEventEmitter stores handlers in the native module (RCTDeviceEventEmitter), not in JavaScript-accessible maps. Solution: mock react-native completely with a shared __nativeEventBus__ Map accessible by both NativeEventEmitter.addListener (which stores handlers) and DeviceEventEmitter.emit (which retrieves and invokes them). Also added __esModule: true to appNavigation and NativeVoip mocks, and proceedAccept to NativeVoip mock.
diegolmello
had a problem deploying
to
approve_e2e_testing
GitHub Actions
Failure
WalkthroughThis change introduces a pending-accept fast-accept flow for VoIP calls across native Android/iOS and JavaScript layers. When a call arrives, it's queued and deferred until media session initialization completes, displaying a "Connecting" state in CallView. A new Changes
Sequence Diagram(s)sequenceDiagram
participant Native as Native (iOS/Android)
participant JS as JS Runtime
participant Store as Call Store
participant MediaSession as Media Session
participant UI as CallView
Note over Native,UI: Pending Accept Fast-Accept Flow
Native->>JS: emit VoipPendingAccept<br/>(callId, VoipPayload)
JS->>Store: enqueue callId<br/>set nativeAcceptedCallId
JS->>Store: setContact(caller)
JS->>UI: navigate to CallView
UI->>UI: render "Connecting" state
JS->>MediaSession: registerOnInitComplete(cb)
Note over JS,MediaSession: Wait for media session init
MediaSession->>MediaSession: init() completes<br/>REST state replayed
MediaSession->>MediaSession: drainQueued callbacks
JS->>JS: invoke proceedAccept(callId)
JS->>Native: proceedAccept(callId)
Native->>Native: lookup pendingAccept<br/>payload, remove from map
Native->>Native: handleNativeAccept()<br/>(with dedup guard)
alt Accept Succeeds
Native->>JS: emit VoipAcceptSucceeded
JS->>Store: clear queued callId
UI->>UI: render active call
else Accept Fails (TTL)
Native->>JS: emit VoipAcceptFailed
JS->>Store: clear queued callId<br/>resetNativeCallId()
JS->>UI: clearPendingCallView()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/views/CallView/components/CallerInfo.tsx (1)
32-37:⚠️ Potential issue | 🟡 MinorDisable the controls toggle while connecting.
When
isConnectingis true,CallViewhidesCallButtons, but thisPressablestill advertises “Toggle call controls” and keeps button semantics. That leaves a no-op action in the UI and a misleading accessibility label.Suggested fix
const CallerInfo = ({ isConnecting = false }: CallerInfoProps): React.ReactElement => { const { colors } = useTheme(); const contact = useCallContact(); const toggleControlsVisible = useCallStore(state => state.toggleControlsVisible); const controlsVisible = useControlsVisible(); const isScreenReaderEnabled = useIsScreenReaderEnabled(); + const canToggleControls = !isConnecting && !isScreenReaderEnabled; return ( <Pressable style={styles.callerInfoContainer} testID='caller-info-toggle' - onPress={isScreenReaderEnabled ? undefined : toggleControlsVisible} - accessibilityLabel={I18n.t('Toggle_call_controls')} - accessibilityRole={isScreenReaderEnabled ? 'none' : 'button'}> + onPress={canToggleControls ? toggleControlsVisible : undefined} + accessibilityLabel={canToggleControls ? I18n.t('Toggle_call_controls') : undefined} + accessibilityRole={canToggleControls ? 'button' : undefined}>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/CallView/components/CallerInfo.tsx` around lines 32 - 37, The Pressable in CallerInfo.tsx still exposes a toggle when the app is connecting; update its props so when isConnecting is true it becomes non-interactive and non-button for accessibility: set onPress to undefined (or noop) and accessibilityRole to 'none', and remove or change accessibilityLabel (e.g., clear or provide a connecting label) while isConnecting is true; use the existing isConnecting, toggleControlsVisible and isScreenReaderEnabled variables to conditionally apply these props so the control is inert and not announced as a toggle during connection.ios/Libraries/VoipService.swift (1)
502-545:⚠️ Potential issue | 🟡 MinorFailure branch leaves payload in
pendingAcceptPayloads.On
finishAccept(true)the payload is removed frompendingAcceptPayloads, but thefinishAccept(false)branch doesn't perform the same cleanup. When the native REST accept fails (including the 10-second timeout path),clearNativeAcceptDeduperuns but the stored payload remains indefinitely. That leaks one entry per failed callId and also means a laterproceedAccept(callId)from JS would retrieve the already-failed payload and re-drivehandleNativeAccept(a new REST attempt, since the dedupe set was cleared). Consider removing frompendingAcceptPayloadsin the failure branch as well, or gatingproceedAcceptto skip when the accept has terminally failed.🔒️ Proposed cleanup in the failure branch
} else { + bridgeStateQueue.sync { + pendingAcceptPayloads.removeValue(forKey: payload.callId) + } clearNativeAcceptDedupe(for: payload.callId) RNCallKeep.endCall(withUUID: payload.callId, reason: 6)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ios/Libraries/VoipService.swift` around lines 502 - 545, The failure branch of the finishAccept closure leaks the payload because pendingAcceptPayloads is only removed on success; update the failure path in finishAccept (the closure defined as let finishAccept: (Bool) -> Void) to also remove the entry from pendingAcceptPayloads under bridgeStateQueue.sync (mirror the removal done in the success branch), and/or add gating in proceedAccept to ignore callIds that have a terminal failure flag so handleNativeAccept isn't retried for already-failed payloads; ensure you still call clearNativeAcceptDedupe(callId:) and post the VoipAcceptFailed notification as before.
🧹 Nitpick comments (2)
app/lib/services/voip/MediaCallEvents.test.ts (1)
682-693: AC5 could assert thatproceedAcceptwas actually invoked.The mock invokes the
registerOnInitCompletecallback synchronously, so the real end-to-end behavior you care about is thatNativeVoipModule.proceedAcceptis called with the queued callId. Right now the test only verifiesregisterOnInitCompletewas called once; a faulty implementation that forgot to pass() => NativeVoipModule.proceedAccept(callId)would still pass.🧪 Strengthen the assertion
expect(mediaSessionInstance.registerOnInitComplete).toHaveBeenCalledTimes(1); - // Callback was invoked immediately since init was complete in the mock + // Mock invokes the callback synchronously, so proceedAccept should have fired + expect(NativeVoipModule.proceedAccept).toHaveBeenCalledWith('init-callback-test');🤖 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 682 - 693, The test currently only checks that mediaSessionInstance.registerOnInitComplete was called; update the test to also assert that NativeVoipModule.proceedAccept was invoked with the queued callId when the registered callback runs. Specifically, after emitting DeviceEventEmitter.emit('VoipPendingAccept', { callId: 'init-callback-test', payload }), capture the registered callback from mediaSessionInstance.registerOnInitComplete's mock (or trigger it if the mock already invokes it) and verify NativeVoipModule.proceedAccept was called with 'init-callback-test'; keep the existing teardown() call.app/lib/services/voip/MediaSessionInstance.ts (1)
52-73: Optional: narrowsignal.typefirst to avoidas anyforcallId.The
isCallIdQueued((signal as any).callId)check fires for every inbound signal and loses type safety. SincecallIdonly exists on thenotificationvariant ofServerMediaSignal, narrowing up front removes the cast and keeps the guard colocated with the accepted-notification logic:♻️ Proposed refactor
private tryAnswerIfNativeAcceptedNotification(signal: ServerMediaSignal): void { - // Live-DDP race guard: during pending-accept window, `proceedAccept` is the sole - // trigger for `answerCall`. Covers both call sites (`applyRestStateSignals` and the - // stream-notify-user listener). See class-level Invariant comment. - // TypeScript doesn't narrow ServerMediaSignal union via `type === 'notification'` alone, - // so we cast to access callId on the notification variant. - if (isCallIdQueued((signal as any).callId)) { - return; - } - const { call, nativeAcceptedCallId } = useCallStore.getState(); - if ( - signal.type === 'notification' && - signal.notification === 'accepted' && - signal.signedContractId === getUniqueIdSync() && - nativeAcceptedCallId === signal.callId && - call == null - ) { + if (signal.type !== 'notification') { + return; + } + // Live-DDP race guard: during the pending-accept window, `proceedAccept` is the + // sole trigger for `answerCall`. See class-level Invariant comment. + if (isCallIdQueued(signal.callId)) { + return; + } + const { call, nativeAcceptedCallId } = useCallStore.getState(); + if ( + signal.notification === 'accepted' && + signal.signedContractId === getUniqueIdSync() && + nativeAcceptedCallId === signal.callId && + call == null + ) { this.answerCall(signal.callId).catch(error => { console.error('[VoIP] Error answering call on notification/accepted:', error); }); } }As per coding guidelines, "Use TypeScript for type safety; add explicit type annotations".
🤖 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 52 - 73, Move the check that reads signal.callId behind a type-narrow for the notification variant so you can avoid the (signal as any) cast; specifically, in tryAnswerIfNativeAcceptedNotification first test signal.type === 'notification' (and optionally signal.notification === 'accepted') then call isCallIdQueued(signal.callId) and early-return if queued, and keep the existing checks (signedContractId === getUniqueIdSync(), nativeAcceptedCallId === signal.callId, call == null) before invoking this.answerCall(signal.callId). This preserves behavior while restoring TypeScript type-safety for the notification.callId access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt`:
- Around line 88-101: In emitPendingAcceptEvent, don't pass a Kotlin Map to
DeviceEventManagerModule.emit; build and pass a React WritableMap instead:
obtain a WritableMap via Arguments.createMap(), putString("callId",
voipPayload.callId) and putMap("payload", voipPayload.toWritableMap()), then
emit that map for EVENT_VOIP_PENDING_ACCEPT; update the emit call in
emitPendingAcceptEvent (referencing reactContextRef, EVENT_VOIP_PENDING_ACCEPT
and VoipPayload.toWritableMap) accordingly.
In `@app/lib/services/voip/clearPendingCallView.ts`:
- Around line 4-7: The function clearPendingCallView currently always calls
Navigation.back() which can pop the wrong screen; change it so after calling
useCallStore.getState().resetNativeCallId() it first verifies the active route
is the CallView (e.g. via your navigation API's current/active route getter) and
only then calls Navigation.back(); reference clearPendingCallView,
Navigation.back, and the CallView route name when implementing the guard.
In `@app/lib/services/voip/MediaCallEvents.test.ts`:
- Around line 150-153: The failing lint rule is caused by space indentation in
the jest.mock block; update the indentation in the
jest.mock('../../navigation/appNavigation', ...) mock so lines for "__esModule:
true," and "default: { navigate: jest.fn(), back: jest.fn() }" use tabs instead
of four spaces; ensure the jest.mock call and the properties (default, navigate,
back) keep the same structure and single quotes but replace leading spaces with
tabs to satisfy the project's indentation rule.
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Around line 240-245: The log label `TTL_EXPIRED` is misleading because any
VoipAcceptFailed for a queued call (native REST failure, timeout, constructor
error, etc.) will hit this branch; update the branch in MediaCallEvents where
you check queuedCallIds and call clearPendingCallView to log a neutral failure
instead (e.g., use mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT FAILED`, {
callId: data.callId, reason: data.reason || 'unknown' }) ) and only change the
label to `TTL_EXPIRED` when you have explicit TTL evidence (e.g., a ttl flag or
timer expiry value in the event payload `data`); keep references to
queuedCallIds, mediaCallLogger, TAG, clearPendingCallView and include the raw
payload/reason in the log to aid triage.
In `@app/views/CallView/CallView.stories.tsx`:
- Around line 159-165: TabletConnectingCall story sets call: null but
TabletCallView currently returns early on !call, so add the same
nativeAcceptedCallId handling as in CallView: in TabletCallView (the component
that currently exits on !call) update the early-return logic to check for
nativeAcceptedCallId (e.g., if (!call && !nativeAcceptedCallId) return null) and
when nativeAcceptedCallId is present render the tablet "connecting" state path
that CallView uses; reference the nativeAcceptedCallId prop/state and reuse the
same connecting-state rendering branch or helper used by CallView so the story
shows the connecting UI.
In `@ios/Libraries/VoipModule.mm`:
- Around line 37-39: The supportedEvents list includes "VoipPendingAccept" but
startObserving does not subscribe to that notification, so pending-accept
notifications are dropped; update startObserving to add an observer for the
"VoipPendingAccept" notification (using the same pattern as the other observers
in startObserving) and ensure its selector calls
sendEventWithName(@"VoipPendingAccept", ...) (create a new handler like
onVoipPendingAccept: if none exists). Also mirror the change in stopObserving to
remove that observer if the class removes observers explicitly.
- Around line 120-123: The VoipService class is missing a declaration for the
class selector +proceedAccept:, which is used by VoipModule's -proceedAccept:
method; add a declaration for +proceedAccept:(NSString *)callId to the local
VoipService interface (the `@interface` VoipService ... block in this file) so the
compiler knows the selector signature and avoids warnings—ensure the declaration
matches the call site (class method taking an NSString *) and is placed before
VoipModule's implementation.
---
Outside diff comments:
In `@app/views/CallView/components/CallerInfo.tsx`:
- Around line 32-37: The Pressable in CallerInfo.tsx still exposes a toggle when
the app is connecting; update its props so when isConnecting is true it becomes
non-interactive and non-button for accessibility: set onPress to undefined (or
noop) and accessibilityRole to 'none', and remove or change accessibilityLabel
(e.g., clear or provide a connecting label) while isConnecting is true; use the
existing isConnecting, toggleControlsVisible and isScreenReaderEnabled variables
to conditionally apply these props so the control is inert and not announced as
a toggle during connection.
In `@ios/Libraries/VoipService.swift`:
- Around line 502-545: The failure branch of the finishAccept closure leaks the
payload because pendingAcceptPayloads is only removed on success; update the
failure path in finishAccept (the closure defined as let finishAccept: (Bool) ->
Void) to also remove the entry from pendingAcceptPayloads under
bridgeStateQueue.sync (mirror the removal done in the success branch), and/or
add gating in proceedAccept to ignore callIds that have a terminal failure flag
so handleNativeAccept isn't retried for already-failed payloads; ensure you
still call clearNativeAcceptDedupe(callId:) and post the VoipAcceptFailed
notification as before.
---
Nitpick comments:
In `@app/lib/services/voip/MediaCallEvents.test.ts`:
- Around line 682-693: The test currently only checks that
mediaSessionInstance.registerOnInitComplete was called; update the test to also
assert that NativeVoipModule.proceedAccept was invoked with the queued callId
when the registered callback runs. Specifically, after emitting
DeviceEventEmitter.emit('VoipPendingAccept', { callId: 'init-callback-test',
payload }), capture the registered callback from
mediaSessionInstance.registerOnInitComplete's mock (or trigger it if the mock
already invokes it) and verify NativeVoipModule.proceedAccept was called with
'init-callback-test'; keep the existing teardown() call.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 52-73: Move the check that reads signal.callId behind a
type-narrow for the notification variant so you can avoid the (signal as any)
cast; specifically, in tryAnswerIfNativeAcceptedNotification first test
signal.type === 'notification' (and optionally signal.notification ===
'accepted') then call isCallIdQueued(signal.callId) and early-return if queued,
and keep the existing checks (signedContractId === getUniqueIdSync(),
nativeAcceptedCallId === signal.callId, call == null) before invoking
this.answerCall(signal.callId). This preserves behavior while restoring
TypeScript type-safety for the notification.callId access.
🪄 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: 1b115349-d7e2-43f4-bc3c-5331bcf74ea7
📒 Files selected for processing (16)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.ktapp/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/clearPendingCallView.tsapp/lib/services/voip/useCallStore.tsapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallerInfo.tsxapp/views/CallView/index.tsxios/Libraries/VoipModule.mmios/Libraries/VoipPayload.swiftios/Libraries/VoipService.swift
📜 Review details
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{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/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/clearPendingCallView.tsapp/views/CallView/index.tsxapp/lib/services/voip/useCallStore.tsapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallerInfo.tsxapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/clearPendingCallView.tsapp/views/CallView/index.tsxapp/lib/services/voip/useCallStore.tsapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallerInfo.tsxapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/clearPendingCallView.tsapp/views/CallView/index.tsxapp/lib/services/voip/useCallStore.tsapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallerInfo.tsxapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
app/definitions/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Define shared TypeScript types and interfaces in app/definitions/ directory
Files:
app/definitions/Voip.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/clearPendingCallView.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
app/views/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create view components (screens) in app/views/ directory
Files:
app/views/CallView/index.tsxapp/views/CallView/CallView.stories.tsxapp/views/CallView/components/CallerInfo.tsx
🧠 Learnings (7)
📓 Common learnings
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
📚 Learning: 2026-04-22T22:57:58.545Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/lib/services/voip/**/*.{ts,tsx} : Implement VoIP features in app/lib/services/voip/ directory using Zustand stores for WebRTC peer-to-peer audio calls with native CallKit (iOS) and Telecom (Android) integration
Applied to files:
app/definitions/Voip.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/clearPendingCallView.tsapp/views/CallView/index.tsxios/Libraries/VoipModule.mmapp/lib/services/voip/useCallStore.tsapp/views/CallView/CallView.stories.tsxios/Libraries/VoipService.swiftapp/views/CallView/components/CallerInfo.tsxapp/lib/services/voip/MediaSessionInstance.tsandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.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.tsapp/lib/services/voip/MediaCallEvents.test.ts
📚 Learning: 2026-03-10T15:21:45.098Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7046
File: app/containers/InAppNotification/NotifierComponent.stories.tsx:46-75
Timestamp: 2026-03-10T15:21:45.098Z
Learning: In `app/containers/InAppNotification/NotifierComponent.tsx` (React Native, Rocket.Chat), `NotifierComponent` is exported as a Redux-connected component via `connect(mapStateToProps)`. The `isMasterDetail` prop is automatically injected from `state.app.isMasterDetail` and does not need to be passed explicitly at call sites or in Storybook stories that use the default (connected) export.
Applied to files:
app/views/CallView/index.tsxapp/views/CallView/components/CallerInfo.tsx
📚 Learning: 2026-04-22T22:57:58.545Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-04-22T22:57:58.545Z
Learning: Applies to app/AppContainer.tsx : Use AppContainer.tsx as the root navigation container that switches between authentication states
Applied to files:
app/views/CallView/index.tsx
📚 Learning: 2026-03-05T06:06:12.277Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:12.277Z
Learning: Do not re-activate or reset the WCSession singleton in iOS Objective-C/Swift bridge modules. Ensure WCSession is activated and its delegate is set in a single, central place (e.g., ios/RocketChat Watch App/Loaders/WatchSession.swift) and avoid duplicating activation or delegate assignment in other iOS bridge files like ios/RCTWatchModule.mm. If WCSession is already activated via the central loader, relying on WCSession.defaultSession is sufficient and maintains a single session lifecycle.
Applied to files:
ios/Libraries/VoipModule.mm
📚 Learning: 2026-03-15T13:55:42.038Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6911
File: app/containers/markdown/Markdown.stories.tsx:104-104
Timestamp: 2026-03-15T13:55:42.038Z
Learning: In Rocket.Chat React Native, the markdown parser requires a space between the underscore wrapping italic text and a mention sigil (_ mention _ instead of _mention_). Ensure stories and tests that include italic-wrapped mentions follow this form to guarantee proper parsing. Specifically, for files like app/containers/markdown/Markdown.stories.tsx, and any test/content strings that exercise italic-mentions, use the pattern _ mention _ (with spaces) to prevent the mention from being treated as plain text. Validate any test strings or story content accordingly.
Applied to files:
app/views/CallView/CallView.stories.tsx
🪛 ESLint
app/lib/services/voip/MediaCallEvents.test.ts
[error] 151-151: Replace ···· with ↹
(prettier/prettier)
[error] 152-152: Replace ···· with ↹
(prettier/prettier)
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
app/lib/services/voip/MediaCallEvents.test.ts
[failure] 152-152:
Replace ···· with ↹
[failure] 151-151:
Replace ···· with ↹
🔇 Additional comments (2)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt (1)
46-46: LGTM — serialization parity looks right.The
pendingAcceptfield threads throughtoBundle/toWritableMap/fromBundleconsistently, mirrors the iOS conditional-emission pattern intoWritableMap(), and defaults tofalseeverywhere (includingRemoteVoipPayload.toVoipPayload()via the ctor default).Also applies to: 77-77, 97-99, 204-205
app/lib/services/voip/MediaCallEvents.test.ts (1)
24-82: Mock bus design looks right for the described failure mode.Hoisting the shared bus onto
globalso bothNativeEventEmitter.addListener(production-path registration inMediaCallEvents) andDeviceEventEmitter.emit(test trigger) route through the sameMap<string, Set<handler>>correctly fixes the "handlers registered but never invoked" issue on iOS paths.removeAllListenersforwarding tonativeModule?.removeListeners(1)preserves the TurboModule contract, and theremovereturned fromaddListenerproperly deletes the exact handler soteardown()works. Nice, compact fake.
| @JvmStatic | ||
| fun emitPendingAcceptEvent(voipPayload: VoipPayload) { | ||
| try { | ||
| reactContextRef?.get()?.let { context -> | ||
| if (context.hasActiveReactInstance()) { | ||
| context | ||
| .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter::class.java) | ||
| .emit(EVENT_VOIP_PENDING_ACCEPT, mapOf("callId" to voipPayload.callId, "payload" to voipPayload.toWritableMap())) | ||
| } | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Failed to emit VoipPendingAccept", e) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the VoipModule.kt file to see the current state
cat -n android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt | head -120Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 5411
🏁 Script executed:
# Search for other emit() calls in the codebase to understand the pattern
rg "\.emit\(" --type kotlin -B 2 -A 2 android/app/src/main/javaRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2618
🏁 Script executed:
# Check if there are any imports related to WritableMap or Arguments in the file
rg "import.*React|import.*facebook" android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 244
🏁 Script executed:
# Check VideoConfModule.kt to see what type of payload it uses
rg -A 5 "emit\(EVENT_VIDEO_CONF_ACTION" android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.ktRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 296
🏁 Script executed:
# Search for Arguments usage in the android app to understand WritableMap creation pattern
rg "Arguments\." android/app/src/main/java --type kotlin -B 2 -A 2 | head -40Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 593
🏁 Script executed:
# Check what type actionJson is in VideoConfModule.kt
rg -B 10 "emit\(EVENT_VIDEO_CONF_ACTION, actionJson" android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt | grep -E "actionJson|WritableMap|Arguments"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 216
🏁 Script executed:
# Look at the full context of VideoConfModule.kt emit call
cat android/app/src/main/java/chat/rocket/reactnative/notification/VideoConfModule.kt | grep -A 20 -B 20 "EVENT_VIDEO_CONF_ACTION"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2759
🏁 Script executed:
# Let me check the type signature of RCTDeviceEventEmitter.emit to understand what it accepts
# First, let's see if there are any type hints or documentation in the codebase
rg "RCTDeviceEventEmitter" android/app/src/main/java --type kotlin -A 3 | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 2201
🌐 Web query:
RCTDeviceEventEmitter emit method signature React Native Android bridge WritableMap
💡 Result:
The RCTDeviceEventEmitter.emit method signature in React Native Android bridge is: void emit(String eventName, @Nullable Object data) This is defined in the Java interface DeviceEventManagerModule.RCTDeviceEventEmitter, which is a JavaScriptModule used via getJSModule from ReactContext. Usage example from official docs and source: reactContext.getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class).emit(eventName, params); Where params is typically a WritableMap created with Arguments.createMap, or null. Note: This is for the legacy (bridge) architecture. In the New Architecture (TurboModules/Fabric), events use spec-defined emitters like CodegenTypes.EventEmitter with generated emit methods taking WritableMap/ReadableMap.
Citations:
- 1: https://github.com/lenaten/react-native-relay/blob/master/node_modules/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/core/DeviceEventManagerModule.java
- 2: https://reactnative.dev/docs/legacy/native-modules-android
- 3: https://github.com/invertase/react-native-firebase/blob/master/packages/app/android/src/reactnative/java/io/invertase/firebase/common/ReactNativeFirebaseEventEmitter.java
- 4: DeviceEventEmitter.addListener is not working in Release build for Android. react/react-native#43651
- 5: https://stackoverflow.com/questions/50868852/sending-events-to-javascript-from-android-react-native-rctdeviceeventemitter
- 6: DeviceEventEmitter.addListener not working in Release apk react/react-native#32792
🏁 Script executed:
# Check VoipPayload.toWritableMap to see the established pattern
cat android/app/src/main/java/chat/rocket/reactnative/voip/VoipPayload.kt | grep -A 15 "toWritableMap"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 733
Use WritableMap instead of Kotlin Map for the event payload.
Line 95 passes a Kotlin Map created with mapOf(...) to emit(). While the method accepts Object, React Native's bridge expects WritableMap for structured payloads. Kotlin Map serialization can fail at runtime when JavaScript tries to access the data. The other emit calls in this file (lines 46 and 80) correctly use WritableMap directly. Build the payload with Arguments.createMap() and use putString() and putMap() for fields, matching the pattern in VoipPayload.toWritableMap().
Suggested fix
+import com.facebook.react.bridge.Arguments
import com.facebook.react.bridge.ReactApplicationContext
import com.facebook.react.bridge.WritableMap
import com.facebook.react.modules.core.DeviceEventManagerModule
@@
fun emitPendingAcceptEvent(voipPayload: VoipPayload) {
try {
reactContextRef?.get()?.let { context ->
if (context.hasActiveReactInstance()) {
+ val event = Arguments.createMap().apply {
+ putString("callId", voipPayload.callId)
+ putMap("payload", voipPayload.toWritableMap())
+ }
context
.getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter::class.java)
- .emit(EVENT_VOIP_PENDING_ACCEPT, mapOf("callId" to voipPayload.callId, "payload" to voipPayload.toWritableMap()))
+ .emit(EVENT_VOIP_PENDING_ACCEPT, event)
}
}
} catch (e: Exception) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt` around
lines 88 - 101, In emitPendingAcceptEvent, don't pass a Kotlin Map to
DeviceEventManagerModule.emit; build and pass a React WritableMap instead:
obtain a WritableMap via Arguments.createMap(), putString("callId",
voipPayload.callId) and putMap("payload", voipPayload.toWritableMap()), then
emit that map for EVENT_VOIP_PENDING_ACCEPT; update the emit call in
emitPendingAcceptEvent (referencing reactContextRef, EVENT_VOIP_PENDING_ACCEPT
and VoipPayload.toWritableMap) accordingly.
| export function clearPendingCallView(): void { | ||
| useCallStore.getState().resetNativeCallId(); | ||
| // If we're on CallView, navigate back | ||
| Navigation.back(); |
There was a problem hiding this comment.
Guard the back navigation to CallView only.
This helper always calls Navigation.back(), so a late VoipAcceptFailed can pop whatever screen the user is currently on. The inline comment already describes the intended behavior; the implementation should check that CallView is actually active before navigating back.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/lib/services/voip/clearPendingCallView.ts` around lines 4 - 7, The
function clearPendingCallView currently always calls Navigation.back() which can
pop the wrong screen; change it so after calling
useCallStore.getState().resetNativeCallId() it first verifies the active route
is the CallView (e.g. via your navigation API's current/active route getter) and
only then calls Navigation.back(); reference clearPendingCallView,
Navigation.back, and the CallView route name when implementing the guard.
| jest.mock('../../navigation/appNavigation', () => ({ | ||
| __esModule: true, | ||
| default: { navigate: jest.fn(), back: jest.fn() } | ||
| })); |
There was a problem hiding this comment.
ESLint/Prettier: use tabs for indentation.
Lines 151–152 use 4 spaces; the file (and the repo config) expects tabs. This is exactly what the failing run-eslint-and-test check is reporting (Replace ···· with ↹), so CI will stay red until it's fixed.
🧹 Fix indentation
jest.mock('../../navigation/appNavigation', () => ({
- __esModule: true,
- default: { navigate: jest.fn(), back: jest.fn() }
+ __esModule: true,
+ default: { navigate: jest.fn(), back: jest.fn() }
}));As per coding guidelines, "Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible".
📝 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.
| jest.mock('../../navigation/appNavigation', () => ({ | |
| __esModule: true, | |
| default: { navigate: jest.fn(), back: jest.fn() } | |
| })); | |
| jest.mock('../../navigation/appNavigation', () => ({ | |
| __esModule: true, | |
| default: { navigate: jest.fn(), back: jest.fn() } | |
| })); |
🧰 Tools
🪛 ESLint
[error] 151-151: Replace ···· with ↹
(prettier/prettier)
[error] 152-152: Replace ···· with ↹
(prettier/prettier)
🪛 GitHub Check: ESLint and Test / run-eslint-and-test
[failure] 152-152:
Replace ···· with ↹
[failure] 151-151:
Replace ···· with ↹
🤖 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 150 - 153, The
failing lint rule is caused by space indentation in the jest.mock block; update
the indentation in the jest.mock('../../navigation/appNavigation', ...) mock so
lines for "__esModule: true," and "default: { navigate: jest.fn(), back:
jest.fn() }" use tabs instead of four spaces; ensure the jest.mock call and the
properties (default, navigate, back) keep the same structure and single quotes
but replace leading spaces with tabs to satisfy the project's indentation rule.
| describe('AC7: resetMediaCallEventsStateForTesting clears all sentinels', () => { | ||
| it('after resetMediaCallEventsStateForTesting, previously queued callIds can be re-processed', () => { | ||
| setupMediaCallEvents(makeTestAdapters()); | ||
| const payload = buildIncomingPayload({ | ||
| callId: 'reset-sentinel', | ||
| host: 'https://workspace-b.example.com' | ||
| }); | ||
|
|
||
| // Queue a callId | ||
| DeviceEventEmitter.emit('VoipPendingAccept', { callId: 'reset-sentinel', payload }); | ||
| let { isCallIdQueued } = jest.requireActual('./MediaCallEvents'); | ||
| expect(isCallIdQueued('reset-sentinel')).toBe(true); | ||
|
|
||
| // Reset all sentinels | ||
| resetMediaCallEventsStateForTesting(); | ||
|
|
||
| // Same callId can now be queued again | ||
| DeviceEventEmitter.emit('VoipPendingAccept', { callId: 'reset-sentinel', payload }); | ||
| ({ isCallIdQueued } = jest.requireActual('./MediaCallEvents')); | ||
| expect(isCallIdQueued('reset-sentinel')).toBe(true); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
AC7 doesn't actually verify "re-processing" after reset.
resetMediaCallEventsStateForTesting() only calls clearVoipAcceptDedupeSentinels(); it does not clear queuedCallIds. So in this test:
- First emit queues
'reset-sentinel'(set contains it). resetMediaCallEventsStateForTesting()leavesqueuedCallIdsuntouched.- Second emit hits the dedup early-return in
handleVoipPendingAccept, never reachingsetNativeAcceptedCallId/setContact/Navigation.navigate. isCallIdQueued('reset-sentinel')is stilltrue— but that's because it was never removed, not because it was re-queued.
The assertion is trivially true and doesn't prove what the test name claims. Either extend the reset helper to also clear queuedCallIds, or change the assertions to reflect the real invariant (e.g. verify setNativeAcceptedCallId was called once after reset, or assert queue dedup survives reset).
🧪 Option A — make the reset also clear the queue and assert true re-processing
In MediaCallEvents.ts:
export function resetMediaCallEventsStateForTesting(): void {
clearVoipAcceptDedupeSentinels();
+ queuedCallIds.clear();
}In the test:
// Queue a callId
DeviceEventEmitter.emit('VoipPendingAccept', { callId: 'reset-sentinel', payload });
- let { isCallIdQueued } = jest.requireActual('./MediaCallEvents');
+ const { isCallIdQueued } = jest.requireActual('./MediaCallEvents');
expect(isCallIdQueued('reset-sentinel')).toBe(true);
+ expect(mockSetNativeAcceptedCallId).toHaveBeenCalledTimes(1);
// Reset all sentinels
resetMediaCallEventsStateForTesting();
+ expect(isCallIdQueued('reset-sentinel')).toBe(false);
- // Same callId can now be queued again
+ // Same callId can now be queued again and must re-drive the handler
DeviceEventEmitter.emit('VoipPendingAccept', { callId: 'reset-sentinel', payload });
- ({ isCallIdQueued } = jest.requireActual('./MediaCallEvents'));
expect(isCallIdQueued('reset-sentinel')).toBe(true);
+ expect(mockSetNativeAcceptedCallId).toHaveBeenCalledTimes(2);| // If this callId was queued, remove from set and treat as TTL expiry | ||
| if (data.callId && queuedCallIds.has(data.callId)) { | ||
| queuedCallIds.delete(data.callId); | ||
| mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT TTL_EXPIRED`, { callId: data.callId }); | ||
| clearPendingCallView(); | ||
| } |
There was a problem hiding this comment.
"TTL_EXPIRED" log fires on every failure, not only TTL expiry.
This branch runs for any VoipAcceptFailed received while the callId is queued — the native REST failure, the 10s timeout, API construction errors, etc. Logging it as TTL_EXPIRED will mislead triage when debugging non-TTL failures. Consider a neutral label (e.g. VoIP FAST ACCEPT FAILED) and only tag as TTL if you actually have TTL evidence from the payload or a timer.
📝 Proposed tweak
- mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT TTL_EXPIRED`, { callId: data.callId });
+ mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT FAILED`, { callId: data.callId });📝 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.
| // If this callId was queued, remove from set and treat as TTL expiry | |
| if (data.callId && queuedCallIds.has(data.callId)) { | |
| queuedCallIds.delete(data.callId); | |
| mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT TTL_EXPIRED`, { callId: data.callId }); | |
| clearPendingCallView(); | |
| } | |
| // If this callId was queued, remove from set and treat as TTL expiry | |
| if (data.callId && queuedCallIds.has(data.callId)) { | |
| queuedCallIds.delete(data.callId); | |
| mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT FAILED`, { callId: data.callId }); | |
| clearPendingCallView(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/lib/services/voip/MediaCallEvents.ts` around lines 240 - 245, The log
label `TTL_EXPIRED` is misleading because any VoipAcceptFailed for a queued call
(native REST failure, timeout, constructor error, etc.) will hit this branch;
update the branch in MediaCallEvents where you check queuedCallIds and call
clearPendingCallView to log a neutral failure instead (e.g., use
mediaCallLogger.debug(`${TAG} VoIP FAST ACCEPT FAILED`, { callId: data.callId,
reason: data.reason || 'unknown' }) ) and only change the label to `TTL_EXPIRED`
when you have explicit TTL evidence (e.g., a ttl flag or timer expiry value in
the event payload `data`); keep references to queuedCallIds, mediaCallLogger,
TAG, clearPendingCallView and include the raw payload/reason in the log to aid
triage.
| setStoreState({ | ||
| call: null, | ||
| nativeAcceptedCallId: 'pending-call-id', | ||
| callState: 'none', | ||
| callStartTime: null, | ||
| contact: { displayName: 'Bob Burnquist', username: 'bob.burnquist' } | ||
| }); |
There was a problem hiding this comment.
TabletConnectingCall now renders nothing.
This story sets call: null, but TabletCallView still exits early on !call at Line 142, so the tablet connecting state never appears. Please mirror CallView’s nativeAcceptedCallId handling in TabletCallView before switching the story to this state.
Suggested fix
const TabletCallView = () => {
const { colors } = useTheme();
const call = useCallStore(state => state.call);
- if (!call) return null;
+ const nativeAcceptedCallId = useCallStore(state => state.nativeAcceptedCallId);
+ const isConnecting = !call && !!nativeAcceptedCallId;
+ if (!call && !isConnecting) return null;
return (
<ResponsiveLayoutContext.Provider value={{ ...responsiveLayoutProviderLargeFontValue(1), width: 700 }}>
<View style={[callViewStyles.contentContainer, { backgroundColor: colors.surfaceLight }]}>
- <CallerInfo />
- <CallButtons />
+ <CallerInfo isConnecting={isConnecting} />
+ {isConnecting ? null : <CallButtons />}
</View>
</ResponsiveLayoutContext.Provider>
);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/views/CallView/CallView.stories.tsx` around lines 159 - 165,
TabletConnectingCall story sets call: null but TabletCallView currently returns
early on !call, so add the same nativeAcceptedCallId handling as in CallView: in
TabletCallView (the component that currently exits on !call) update the
early-return logic to check for nativeAcceptedCallId (e.g., if (!call &&
!nativeAcceptedCallId) return null) and when nativeAcceptedCallId is present
render the tablet "connecting" state path that CallView uses; reference the
nativeAcceptedCallId prop/state and reuse the same connecting-state rendering
branch or helper used by CallView so the story shows the connecting UI.
| - (NSArray<NSString *> *)supportedEvents { | ||
| return @[@"VoipPushTokenRegistered", @"VoipAcceptFailed", @"VoipAcceptSucceeded"]; | ||
| return @[@"VoipPushTokenRegistered", @"VoipAcceptFailed", @"VoipAcceptSucceeded", @"VoipPendingAccept"]; | ||
| } |
There was a problem hiding this comment.
VoipPendingAccept is exposed but never forwarded to JS.
Adding the event name here is not enough: startObserving still subscribes only to VoipPushTokenRegistered, VoipAcceptFailed, and VoipAcceptSucceeded. As written, warm iOS VoipPendingAccept notifications will be dropped instead of reaching the JS listener.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Libraries/VoipModule.mm` around lines 37 - 39, The supportedEvents list
includes "VoipPendingAccept" but startObserving does not subscribe to that
notification, so pending-accept notifications are dropped; update startObserving
to add an observer for the "VoipPendingAccept" notification (using the same
pattern as the other observers in startObserving) and ensure its selector calls
sendEventWithName(@"VoipPendingAccept", ...) (create a new handler like
onVoipPendingAccept: if none exists). Also mirror the change in stopObserving to
remove that observer if the class removes observers explicitly.
| - (void)proceedAccept:(NSString *)callId { | ||
| dispatch_async(dispatch_get_main_queue(), ^{ | ||
| [VoipService proceedAccept:callId]; | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find VoipModule.mm and VoipService related files
find . -type f -name "VoipModule.mm" -o -name "*VoipService*" 2>/dev/null | head -20Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 142
🏁 Script executed:
#!/bin/bash
# Search for VoipService interface declarations
rg "interface VoipService" --type objc -B2 -A10Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Search for proceedAccept method calls and declarations
rg "proceedAccept" --type objc -B2 -A2Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 60
🏁 Script executed:
#!/bin/bash
# Check VoipModule.mm content and size
wc -l ./ios/Libraries/VoipModule.mmRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Read VoipModule.mm around lines 120-123
sed -n '110,130p' ./ios/Libraries/VoipModule.mmRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 649
🏁 Script executed:
#!/bin/bash
# Check VoipService.swift
cat ./ios/Libraries/VoipService.swiftRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 26117
🏁 Script executed:
#!/bin/bash
# Look for bridging header files
find ./ios -name "*-Bridging-Header.h" -o -name "*Bridge*"Repository: RocketChat/Rocket.Chat.ReactNative
Length of output: 254
🏁 Script executed:
#!/bin/bash
# Read the bridging header
cat ./ios/RocketChatRN-Bridging-Header.hRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 537
🏁 Script executed:
#!/bin/bash
# Read the top of VoipModule.mm to see imports and any interface declarations
head -50 ./ios/Libraries/VoipModule.mmRepository: RocketChat/Rocket.Chat.ReactNative
Length of output: 1450
Add +proceedAccept: declaration to the VoipService interface.
The local VoipService interface in this file does not declare the +proceedAccept: selector, yet it is called at lines 120–123. This creates an incomplete contract and can trigger compiler warnings under strict Objective-C settings.
Suggested fix
`@interface` VoipService : NSObject
+ (void)voipRegistration;
+ (NSDictionary * _Nullable)getInitialEvents;
+ (void)clearInitialEvents;
+ (NSString * _Nonnull)getLastVoipToken;
+ (void)stopDDPClient;
++ (void)proceedAccept:(NSString * _Nonnull)callId;
`@end`📝 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.
| - (void)proceedAccept:(NSString *)callId { | |
| dispatch_async(dispatch_get_main_queue(), ^{ | |
| [VoipService proceedAccept:callId]; | |
| }); | |
| `@interface` VoipService : NSObject | |
| (void)voipRegistration; | |
| (NSDictionary * _Nullable)getInitialEvents; | |
| (void)clearInitialEvents; | |
| (NSString * _Nonnull)getLastVoipToken; | |
| (void)stopDDPClient; | |
| (void)proceedAccept:(NSString * _Nonnull)callId; | |
| `@end` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ios/Libraries/VoipModule.mm` around lines 120 - 123, The VoipService class is
missing a declaration for the class selector +proceedAccept:, which is used by
VoipModule's -proceedAccept: method; add a declaration for
+proceedAccept:(NSString *)callId to the local VoipService interface (the
`@interface` VoipService ... block in this file) so the compiler knows the
selector signature and avoids warnings—ensure the declaration matches the call
site (class method taking an NSString *) and is placed before VoipModule's
implementation.
PR #7239 Critical ReviewTitle: OverviewImplements a "pending-accept" fast-path for VoIP calls: when iOS CallKit accepts a call before JS is initialized, the native layer holds it briefly, emits The code is functionally correct in its core logic. The issues below are real but non-blocking for the stated goal. Blocking Issues1. In public reset() {
this.notifyNativeReset(); // ← no-op stub; TODO comment in source
...
}
private notifyNativeReset(): void {
// TODO: wired in Phase 6
}
2.
3. In export const PENDING_ACCEPT_TTL_MS = 15_000;The comment says "native sides (iOS/Android) must match this value," but there is no JS-side enforcement of this TTL. The 15-second window exists only in native code. If the native TTL fires but JS hasn't received any signal, the queued callId silently stays in Non-Blocking Issues4. If 5. The
const Emitter = isIOS ? new NativeEventEmitter(NativeVoipModule) : DeviceEventEmitter;This happens before jest mocks are applied. The 6. PR description accuracy The description says:
The source file shows no What's Solid
VerdictApprove with comments. The implementation is sound and the test fixes are legitimate. The three blocking items ( |
1 participant
Summary
Fixes 3 failing VoIP acceptance tests (AC1, AC5, AC7) that verify the pending-accept fast-path behavior in
MediaCallEvents.test.ts.Root Cause
The tests used
DeviceEventEmitter.emit()to triggerVoipPendingAcceptevents, but handlers registered viaNativeEventEmitter.addListener()store handlers in the nativeRCTDeviceEventEmitter, not in any JavaScript-accessible location. So the emitted events never reached the listeners.Fix
Mock
react-nativecompletely with a shared__nativeEventBus__Map onglobalthat bothNativeEventEmitter.addListener(which stores handlers) andDeviceEventEmitter.emit(which invokes them) use. This allows tests to synchronously trigger events that the module under test listens for.Additional fixes:
__esModule: trueto theappNavigationmock so its default export is recognizedproceedAcceptandresetFromLogoutto theNativeVoipmock (missing from the original mock)console.errorfrom the VoipPendingAccept handler inMediaCallEvents.tsTest Plan
yarn test -- --testPathPattern='MediaCallEvents.test.ts' --testNamePattern="AC1.*navigates"— passesyarn test -- --testPathPattern='MediaCallEvents.test.ts' --testNamePattern="AC5"— passesyarn test -- --testPathPattern='MediaCallEvents.test.ts' --testNamePattern="AC7"— passesSummary by CodeRabbit
Release Notes