feat(voip): introduce CallLifecycle.end and CallNavRouter#7274
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughCentralizes VoIP teardown into a new CallLifecycle singleton, adds CallNavRouter to defer post-call navigation until navigation is ready, and updates callers/tests to delegate teardown to CallLifecycle and to mount CallNavRouter once on app startup. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant App as AppContainer
participant Media as MediaSessionInstance
participant CallLC as CallLifecycle
participant Native as VoipNative
participant Store as useCallStore
participant NavRouter as CallNavRouter
participant Nav as Navigation
User->>Media: trigger endCall()
App->>NavRouter: mount() (once)
Media->>CallLC: end('local')
CallLC->>Store: getState() -> callId / nativeAcceptedCallId
CallLC->>Media: hangup() / reject() (if media present)
CallLC->>Native: call.end(effectiveCallId)
CallLC->>Native: call.markActive('')
CallLC->>Native: call.markAvailable(effectiveCallId ?? '')
CallLC->>Store: reset()
CallLC->>Native: call.stopAudio()
CallLC->>CallLC: emit callEnded{callId, reason}
CallLC->>NavRouter: callEnded event
NavRouter->>Nav: getCurrentRoute()
alt currentRoute.name == "CallView"
NavRouter->>Nav: back()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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: 3
🧹 Nitpick comments (4)
app/lib/services/voip/MediaSessionInstance.ts (1)
211-214: 💤 Low valueConsider removing the unused
_callIdparameter or documenting the reason for its retention.The
_callIdparameter is now entirely ignored sincecallLifecycle.end()reads the call ID fromuseCallStore. If this signature must remain for API compatibility, a brief comment explaining that constraint would help future maintainers. Otherwise, removing it would reduce surface area.🤖 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 211 - 214, The endCall method in MediaSessionInstance currently accepts an unused parameter _callId; either remove the _callId parameter from the method signature or, if it must remain for external API compatibility, add a concise comment on MediaSessionInstance.endCall explaining that the call ID is intentionally ignored because callLifecycle.end('local') reads the active call ID from useCallStore; ensure the change updates any external typings or callers if you remove the parameter to keep signatures consistent.app/lib/services/voip/CallLifecycle.test.ts (2)
99-101: 💤 Low valueEmpty
afterEachblock can be removed.The
afterEachhook contains only a comment and no actual cleanup logic.♻️ Suggested fix
- afterEach(() => { - // Clean up any listeners - });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/CallLifecycle.test.ts` around lines 99 - 101, Remove the empty test teardown hook: delete the no-op afterEach block in CallLifecycle.test.ts (the afterEach() that only contains a comment) or replace it with real cleanup logic if teardown is required; this keeps tests clean and avoids dead code.
350-358: 💤 Low valueBrittle constructor access pattern — consider exporting
CallLifecycleclass directly.Using
callLifecycle.constructorto create a fresh instance is fragile and relies on implementation details. If the class is minified or the export structure changes, this test will break.♻️ Alternative approach
Export the
CallLifecycleclass directly from the module:// In CallLifecycle.ts -export const callLifecycle = new CallLifecycle(); +export class CallLifecycle { ... } +export const callLifecycle = new CallLifecycle();Then in the test:
-const freshLifecycle = new (callLifecycle.constructor as new () => typeof callLifecycle)(); +import { CallLifecycle } from './CallLifecycle'; +const freshLifecycle = new CallLifecycle();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/CallLifecycle.test.ts` around lines 350 - 358, The test uses a brittle pattern new (callLifecycle.constructor as new () => typeof callLifecycle)() to create a fresh instance; instead export the CallLifecycle class from the module and update the test to instantiate it directly (e.g., new CallLifecycle()) so it no longer depends on callLifecycle.constructor; ensure the test still calls the same internal teardown helper (_runTeardown('local')) on the fresh instance and update imports to reference the exported CallLifecycle.app/lib/services/voip/CallLifecycle.ts (1)
117-158: 💤 Low valueThe
asynckeyword is unnecessary since_runTeardowncontains noawaitexpressions.The
eslint-disable require-awaitcomment acknowledges this. If future changes don't require async, consider removing both the comment and theasynckeyword to reduce noise.♻️ Alternative
- // eslint-disable-next-line require-await - private async _runTeardown(reason: CallEndReason): Promise<void> { + private _runTeardown(reason: CallEndReason): void {Note: This would require updating line 111 to wrap the result:
-this._endPromise = this._runTeardown(reason).finally(() => { +this._endPromise = Promise.resolve(this._runTeardown(reason)).finally(() => {However, keeping it
asyncis fine if you anticipate adding awaited calls in future slices.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/CallLifecycle.ts` around lines 117 - 158, The _runTeardown method is marked async and has an eslint-disable for require-await but contains no awaits; remove the unnecessary async keyword and the eslint-disable comment, change the signature from "private async _runTeardown(reason: CallEndReason): Promise<void>" to a synchronous signature (e.g. "private _runTeardown(reason: CallEndReason): void"), and then update any callers that expect a Promise-returning _runTeardown to handle a synchronous return (wrap the call with Promise.resolve(...) or make the caller async and await Promise.resolve if needed) so call semantics remain correct.
🤖 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/CallNavRouter.test.ts`:
- Around line 27-30: The test file has imports (callLifecycle, CallNavRouter,
emitter, Navigation) placed after executable code which triggers the ESLint
import/first rule; open CallNavRouter.test.ts and move these import statements
(callLifecycle, CallNavRouter, emitter, Navigation) to the very top of the
module, before any executable statements (e.g., variable declarations, mocks, or
test setups) so all imports run first and the lint error is resolved.
In `@app/lib/services/voip/CallNavRouter.ts`:
- Around line 45-62: unmount() currently doesn't remove the deferred
navigationReady listener (onceNavigationReady), so if the router is unmounted
before nav is ready that listener can later re-subscribe callEnded; fix by
storing the listener reference in a module-scoped variable (e.g.
_pendingNavigationReadyListener) when you create onceNavigationReady in the code
that calls emitter.on('navigationReady', onceNavigationReady) and then in
unmount() call emitter.off('navigationReady', _pendingNavigationReadyListener),
null out that variable and then proceed to clear _unsubscribeCallEnded and set
_mounted = false; ensure the same function reference created for emitter.on is
used for emitter.off.
In `@app/sagas/deepLinking.js`:
- Around line 87-91: callLifecycle.end('error') returns a Promise and must be
awaited inside the saga to preserve ordering; replace the direct call with a
yielded redux-saga call effect (e.g. yield call([callLifecycle,
callLifecycle.end], 'error')) before invoking resetVoipState(), and ensure call
is imported from 'redux-saga/effects' in this module so the teardown completes
before resetVoipState() and subsequent navigation.
---
Nitpick comments:
In `@app/lib/services/voip/CallLifecycle.test.ts`:
- Around line 99-101: Remove the empty test teardown hook: delete the no-op
afterEach block in CallLifecycle.test.ts (the afterEach() that only contains a
comment) or replace it with real cleanup logic if teardown is required; this
keeps tests clean and avoids dead code.
- Around line 350-358: The test uses a brittle pattern new
(callLifecycle.constructor as new () => typeof callLifecycle)() to create a
fresh instance; instead export the CallLifecycle class from the module and
update the test to instantiate it directly (e.g., new CallLifecycle()) so it no
longer depends on callLifecycle.constructor; ensure the test still calls the
same internal teardown helper (_runTeardown('local')) on the fresh instance and
update imports to reference the exported CallLifecycle.
In `@app/lib/services/voip/CallLifecycle.ts`:
- Around line 117-158: The _runTeardown method is marked async and has an
eslint-disable for require-await but contains no awaits; remove the unnecessary
async keyword and the eslint-disable comment, change the signature from "private
async _runTeardown(reason: CallEndReason): Promise<void>" to a synchronous
signature (e.g. "private _runTeardown(reason: CallEndReason): void"), and then
update any callers that expect a Promise-returning _runTeardown to handle a
synchronous return (wrap the call with Promise.resolve(...) or make the caller
async and await Promise.resolve if needed) so call semantics remain correct.
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 211-214: The endCall method in MediaSessionInstance currently
accepts an unused parameter _callId; either remove the _callId parameter from
the method signature or, if it must remain for external API compatibility, add a
concise comment on MediaSessionInstance.endCall explaining that the call ID is
intentionally ignored because callLifecycle.end('local') reads the active call
ID from useCallStore; ensure the change updates any external typings or callers
if you remove the parameter to keep signatures consistent.
🪄 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: b3405a22-23c9-4789-807e-4e1d1e2afc1f
📒 Files selected for processing (11)
app/AppContainer.tsxapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallNavRouter.test.tsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.tsapp/lib/services/voip/useCallStore.tsapp/sagas/deepLinking.js
📜 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/AppContainer.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallNavRouter.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
**/*.{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/AppContainer.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallNavRouter.test.tsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
**/*.{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/AppContainer.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallNavRouter.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
app/AppContainer.tsx
📄 CodeRabbit inference engine (CLAUDE.md)
Use AppContainer.tsx as the root navigation container that switches between authentication states
Files:
app/AppContainer.tsx
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/CallNavRouter.test.tsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create reusable UI components in app/containers/ directory
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
🧠 Learnings (10)
📓 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/AppContainer.tsx : Use AppContainer.tsx as the root navigation container that switches between authentication states
Applied to files:
app/AppContainer.tsxapp/lib/services/voip/useCallStore.ts
📚 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/index.tsx : Set up Redux provider, theme, navigation, and notifications in app/index.tsx
Applied to files:
app/AppContainer.tsxapp/sagas/deepLinking.jsapp/lib/services/voip/useCallStore.ts
📚 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/containers/**/*.{ts,tsx} : Create reusable UI components in app/containers/ directory
Applied to files:
app/AppContainer.tsx
📚 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/AppContainer.tsxapp/sagas/deepLinking.js
📚 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/AppContainer.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallNavRouter.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.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/theme.tsx : Define theming context in app/theme.tsx
Applied to files:
app/AppContainer.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/sagas/videoConf.ts,app/lib/methods/videoConf.ts} : Implement video conferencing in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using Redux actions, reducers, and sagas for server-managed Jitsi integration
Applied to files:
app/sagas/deepLinking.jsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.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/lib/store/**/*.{ts,tsx} : Configure Redux middleware (saga, app state, internet state) in app/lib/store/ directory
Applied to files:
app/sagas/deepLinking.js
📚 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/sagas/**/*.{ts,tsx} : Implement side effects and Redux-Saga middleware in app/sagas/ directory
Applied to files:
app/sagas/deepLinking.js
🪛 ESLint
app/lib/services/voip/CallNavRouter.test.ts
[error] 27-27: Import in body of module; reorder to top.
(import/first)
[error] 28-28: Import in body of module; reorder to top.
(import/first)
[error] 29-29: Import in body of module; reorder to top.
(import/first)
[error] 30-30: Import in body of module; reorder to top.
(import/first)
app/lib/services/voip/CallLifecycle.test.ts
[error] 43-43: Import in body of module; reorder to top.
(import/first)
[error] 45-45: Import in body of module; reorder to top.
(import/first)
[error] 46-46: Import in body of module; reorder to top.
(import/first)
[error] 47-47: Import in body of module; reorder to top.
(import/first)
[error] 48-48: Import in body of module; reorder to top.
(import/first)
[error] 114-114: Use object destructuring.
(prefer-destructuring)
[error] 252-252: Unexpected await inside a loop.
(no-await-in-loop)
🔇 Additional comments (13)
app/lib/services/voip/MediaSessionInstance.test.ts (1)
805-815: Nice contract-focused test update.This keeps
MediaSessionInstance.endCallcoverage focused on delegation and avoids duplicating teardown-order assertions already covered inCallLifecycletests.app/lib/services/voip/useCallStore.test.ts (1)
330-337: Good ownership-boundary assertion.The updated expectation correctly enforces that
reset()no longer ownsstopAudio, preventing regressions in the new teardown orchestration split.app/lib/services/voip/CallLifecycle.test.ts (4)
12-48: Import ordering is correct for Jest mocking.The ESLint
import/firsterrors are false positives. Jest'sjest.mock()calls must appear before imports to properly intercept module loading. This is the standard Jest pattern.
243-257: Sequentialawaitin loop is intentional for isolated reason testing.The ESLint
no-await-in-loopwarning is a false positive here. Each iteration requires the previouscallLifecycle.end()to complete before resetting state for the next reason. Parallel execution would cause race conditions.
103-212: LGTM — Comprehensive teardown ordering tests.The tests thoroughly verify the documented teardown sequence (steps 1-6), including edge cases like skipping step 1 when no active call exists. The use of
InMemoryVoipNative.recordedfor command verification is clean.
308-337: LGTM — Solid idempotency coverage.Testing that concurrent
end()calls share the same in-flight promise (expect(p1).toBe(p2)) is a good pattern for verifying the re-entry guard.app/lib/services/voip/MediaSessionInstance.ts (2)
18-24: LGTM!The imports for
callLifecycleandNavigationare correctly added to support the new centralized teardown orchestration. The delegation pattern aligns well with the PR's goal of consolidating end-call paths.
140-143: LGTM!Routing the remote call-end event through
callLifecycle.end('remote')centralizes teardown logic and ensures consistent sequencing across all end-call scenarios.app/lib/services/voip/CallLifecycle.ts (5)
1-16: LGTM — Excellent documentation.The header comment clearly documents the teardown sequence with step numbers that align with both the implementation and the test assertions. This makes the contract easy to verify and maintain.
48-70: LGTM — Clean typed event emitter implementation.The custom emitter provides proper type safety for event payloads while keeping a minimal API surface. The unsubscribe pattern (returning a cleanup function from
on()) is idiomatic.
106-115: LGTM — Well-implemented idempotency guard.The in-flight promise pattern ensures concurrent
end()calls share a single teardown sequence. Using.finally()to clear the guard handles both success and error cases correctly.
137-146: The concern about passing an empty string tomarkAvailableis based on a misunderstanding of the implementation.The actual
markAvailableimplementation inVoipNative.ts(lines 122–125) ignores thecallUuidparameter entirely. It always callsRNCallKeep.setCurrentCallActive('')andRNCallKeep.setAvailable(true)regardless of whether the parameter is an empty string or a valid ID. PassingeffectiveCallId ?? ''has no unintended effect—the behavior is identical whether a valid ID or empty string is passed.> Likely an incorrect or invalid review comment.
128-135: Thestateproperty is already part ofIClientMediaCall—remove the unnecessary cast.The
stateproperty is accessed directly without casting inuseCallStore.ts(lines 152, 163, 173) on the sameIClientMediaCalltype. Theas anycast at line 130 is inconsistent and unnecessary. Change to:if (mediaCall.state === 'ringing') {> Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Line 92: callLifecycle.end('remote') returns a Promise and must not be left
unhandled; update the synchronous event dispatcher call to either await the
callLifecycle.end('remote') inside an async handler or explicitly attach a
.catch(...) to it so any rejection is logged/handled (e.g.,
callLifecycle.end('remote').catch(err => /* log/handle */)). Apply the same fix
to the other occurrences referenced (useCallStore.ts at the calls around the
handlers and MediaSessionInstance.ts where callLifecycle.end is invoked) so all
callLifecycle.end invocations are properly awaited or have a rejection handler
attached.
In `@app/lib/services/voip/VoipNative.ts`:
- Around line 127-129: The setSpeaker async method should wrap the call to
InCallManager.setForceSpeakerphoneOn(on) in a try/catch to satisfy the async
error-handling guideline; update the setSpeaker implementation inside VoipNative
(the setSpeaker function) to await the call within a try block and log or
rethrow the caught error in the catch so the Promise<void> explicitly handles
failures.
🪄 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: 70de25e1-6a60-4068-b491-ccdd434e3eb0
📒 Files selected for processing (4)
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/VoipNative.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/VoipNative.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/VoipNative.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/VoipNative.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/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/VoipNative.ts
🧠 Learnings (4)
📓 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/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/VoipNative.ts
📚 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/sagas/videoConf.ts,app/lib/methods/videoConf.ts} : Implement video conferencing in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using Redux actions, reducers, and sagas for server-managed Jitsi integration
Applied to files:
app/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/MediaCallEvents.test.ts
🔇 Additional comments (3)
app/lib/services/voip/MediaCallEvents.ts (1)
4-4: Good move to centralize teardown throughCallLifecycle.This import change aligns with the single teardown orchestrator design and keeps event handling focused.
app/lib/services/voip/MediaCallEvents.test.ts (1)
61-67: Nice test realignment with lifecycle-based remote termination.The new mock plus assertion directly validates the intended delegation path (
endCall→callLifecycle.end('remote')).Also applies to: 361-365
app/lib/services/voip/VoipNative.ts (1)
57-60: Good contract-preserving cleanup in the adapters.These changes keep
VoipNativePortcompatibility intact while simplifying internals (Promisereturn shape retained, and unusedcallUuidmade explicit).Also applies to: 73-85, 123-126
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/lib/services/voip/MediaCallEvents.ts (1)
97-97:⚠️ Potential issue | 🟠 MajorUnhandled Promise from
callLifecycle.end('remote')in synchronous dispatcher.On Line 97, the Promise is fire-and-forget without rejection handling; teardown failures can bubble as unhandled async errors. Please keep this call explicit with
void ...catch(...)(or await in an async boundary).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaCallEvents.ts` at line 97, The callLifecycle.end('remote') invocation is currently fire-and-forget and can produce an unhandled rejected Promise; update the synchronous dispatcher to either await the Promise (make the calling function async and use await callLifecycle.end('remote')) or explicitly handle it as a fire-and-forget by prefixing with void and attaching a .catch(...) to log/handle errors (e.g., void callLifecycle.end('remote').catch(err => { processLogger.error('callLifecycle.end failed', err); })). Ensure you modify the invocation of callLifecycle.end in MediaCallEvents.ts so all rejections are handled.
🧹 Nitpick comments (1)
app/lib/services/voip/CallLifecycle.ts (1)
27-27: ⚡ Quick winPrefer an enum for
CallEndReasonto align with project TS conventions.The current string union works, but this constant-set is a good enum candidate and matches the repository TypeScript guideline.
As per coding guidelines, "Use enums for sets of related constants rather than magic strings or numbers".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/CallLifecycle.ts` at line 27, Replace the string-union export CallEndReason with a TypeScript enum (e.g., export enum CallEndReason { Local = 'local', Remote = 'remote', Rejected = 'rejected', Error = 'error', Cleanup = 'cleanup' }) in CallLifecycle.ts and update all references/usages to use the enum members (CallEndReason.Local, etc.) instead of string literals; ensure any type annotations expecting the union are updated to the enum type and any comparisons or assignments use the enum constants so imports/usages remain type-safe and aligned with project TS conventions.
🤖 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/CallLifecycle.test.ts`:
- Line 114: Change the non-destructured assignment to use object destructuring
(replace "const recorded = native.recorded" with "const { recorded } = native"
in CallLifecycle.test.ts) to satisfy prefer-destructuring, and replace the
sequential awaits inside the test loop (the no-await-in-loop violation around
the loop at line ~252) by collecting the inner async operations into an array of
Promises and awaiting them together with await Promise.all(promises) (or
otherwise restructure the test to run those independent async steps in parallel)
to remove the no-await-in-loop lint error.
- Around line 43-48: The ESLint import/first error is caused by jest.mock(...)
calls appearing before the import block; move all jest.mock(...) lines so they
come after the top-level import statements (the imports for IClientMediaCall,
callLifecycle, CallEndReason, InMemoryVoipNative, and useCallStore) so that
every import is declared before any other executable statements in
CallLifecycle.test.ts.
In `@app/lib/services/voip/CallLifecycle.ts`:
- Around line 158-178: The teardown sequence in CallLifecycle can abort if any
of native.call.end, native.call.markActive, native.call.markAvailable,
useCallStore.getState().reset, or native.call.stopAudio throws, preventing
this.emitter.emit('callEnded') from running; wrap each teardown step in its own
try/catch (or execute them inside a single try with per-step inner catches) so
failures are logged but do not stop subsequent steps, ensuring
native.call.end(...), native.call.markActive(''),
native.call.markAvailable(...), useCallStore.getState().reset(),
native.call.stopAudio() all get attempted and that
this.emitter.emit('callEnded', { callId: effectiveCallId, reason }) always runs
in a finally-like guarantee.
---
Duplicate comments:
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Line 97: The callLifecycle.end('remote') invocation is currently
fire-and-forget and can produce an unhandled rejected Promise; update the
synchronous dispatcher to either await the Promise (make the calling function
async and use await callLifecycle.end('remote')) or explicitly handle it as a
fire-and-forget by prefixing with void and attaching a .catch(...) to log/handle
errors (e.g., void callLifecycle.end('remote').catch(err => {
processLogger.error('callLifecycle.end failed', err); })). Ensure you modify the
invocation of callLifecycle.end in MediaCallEvents.ts so all rejections are
handled.
---
Nitpick comments:
In `@app/lib/services/voip/CallLifecycle.ts`:
- Line 27: Replace the string-union export CallEndReason with a TypeScript enum
(e.g., export enum CallEndReason { Local = 'local', Remote = 'remote', Rejected
= 'rejected', Error = 'error', Cleanup = 'cleanup' }) in CallLifecycle.ts and
update all references/usages to use the enum members (CallEndReason.Local, etc.)
instead of string literals; ensure any type annotations expecting the union are
updated to the enum type and any comparisons or assignments use the enum
constants so imports/usages remain type-safe and aligned with project TS
conventions.
🪄 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: b2235a4b-6679-4bb7-970b-55ac5d1e15d6
📒 Files selected for processing (5)
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
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/MediaCallEvents.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/CallLifecycle.test.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create reusable UI components in app/containers/ directory
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.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/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
📚 Learning: 2026-04-07T17:49:25.836Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T17:49:25.836Z
Learning: Applies to **/*.{js,ts,jsx,tsx} : Use explicit error handling with try/catch blocks for async operations
Applied to files:
app/lib/services/voip/MediaCallEvents.ts
📚 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/sagas/videoConf.ts,app/lib/methods/videoConf.ts} : Implement video conferencing in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using Redux actions, reducers, and sagas for server-managed Jitsi integration
Applied to files:
app/lib/services/voip/MediaCallEvents.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
📚 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/MediaCallEvents.test.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.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/index.tsx : Set up Redux provider, theme, navigation, and notifications in app/index.tsx
Applied to files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
📚 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/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
🪛 ESLint
app/lib/services/voip/CallLifecycle.test.ts
[error] 43-43: Import in body of module; reorder to top.
(import/first)
[error] 45-45: Import in body of module; reorder to top.
(import/first)
[error] 46-46: Import in body of module; reorder to top.
(import/first)
[error] 47-47: Import in body of module; reorder to top.
(import/first)
[error] 48-48: Import in body of module; reorder to top.
(import/first)
[error] 114-114: Use object destructuring.
(prefer-destructuring)
[error] 252-252: Unexpected await inside a loop.
(no-await-in-loop)
🔇 Additional comments (3)
app/lib/services/voip/MediaCallEvents.test.ts (1)
265-277: Good regression coverage for lifecycle delegation paths.These additions protect both the
acceptFailedcallId pre-bind path and OS-originated end-call routing throughcallLifecycle.end('remote').Also applies to: 375-380
app/lib/services/voip/MediaCallEvents.ts (1)
71-75: Pre-bindingnativeAcceptedCallIdhere is the right fix.Stashing the call id before deep-link handoff correctly protects the downstream
callId ?? nativeAcceptedCallIdteardown path.app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx (1)
451-463: Nice integration-contract realignment to CallLifecycle/VoipNative seams.The updated assertions validate the new ownership boundaries (teardown via lifecycle + native command recording) without over-coupling to navigation internals.
Also applies to: 644-676, 825-845
Add CallLifecycle (ordered, idempotent end-of-call teardown) and
CallNavRouter (post-call navigation subscriber).
Teardown order enforced by CallLifecycle._runTeardown:
1. mediaCall.reject() if ringing, else hangup()
2. voipNative.call.end(callUuid)
3. voipNative.call.markActive('')
4. voipNative.call.markAvailable(callUuid)
5. useCallStore.reset()
6. voipNative.call.stopAudio() ← after reset so callEnded sees clean state
7. emit callEnded { callId, reason }
Concurrent callers share the in-flight Promise (re-entry guard).
callId uses callId ?? nativeAcceptedCallId (Pre-bind-safe).
'cleanup' reason reserved for slice 08 Pre-bind FSM cleanupAt elapse.
Migrations (six end-call paths now route through lifecycle.end):
- MediaSessionInstance.endCall → lifecycle.end('local')
- useCallStore.endCall → lifecycle.end('local')
- useCallStore.setCall handleEnded → lifecycle.end('remote') (removes inline Navigation.back)
- MediaSessionInstance.init per-call 'ended' listener → lifecycle.end('remote')
- deepLinking.js handleVoipAcceptFailed → lifecycle.end('error')
- CallNavRouter: on callEnded, Navigation.back() if current route is CallView
stopAudio ownership moved from useCallStore.reset to CallLifecycle step 6;
waitForNavigationReady removed from MediaSessionInstance.answerCall.
… call sites
New test files:
- CallLifecycle.test.ts: teardown ordering (steps 2-4,6), hangup vs reject,
idempotency under concurrent end(), callEnded exactly-once emission,
callId ?? nativeAcceptedCallId resolution, reason payload threading.
- CallNavRouter.test.ts: event-in to nav-out, subscription only after
navigationReady, goBack when CallView route, no-op on other routes,
mount() idempotency, unmount/remount cycle.
Updated tests:
- MediaSessionInstance.test.ts: endCall now verifies lifecycle.end('local')
delegate instead of direct voipNative commands.
- useCallStore.test.ts: reset() no longer calls stopAudio (owned by
CallLifecycle step 6); updated assertion documents ownership change.
- VoipCallLifecycle.integration.test.tsx: adapt B1-B3 to new teardown
routing; 'ended' event path asserts store cleared instead of
Navigation.back (now owned by CallNavRouter). 3 pre-existing failures
(A1, C4, E1) remain unchanged from before this slice.
The CallKit/Telecom endCall listener fires when the OS terminates the call,
not when the user presses the in-app end button. The previous code delegated
to mediaSessionInstance.endCall(), which always tagged the reason as 'local'.
Now dispatches callLifecycle.end('remote') directly so the callEnded event
carries the correct reason. MediaSessionInstance.endCall remains the 'local'
(in-app button) path called from CallView.
…dCall test
A1 (answerCall markActive), C4 (setSpeaker on/off), and E1 (stateChange markActive)
assertions were reverted by the previous agent to direct RNCallKeep/InCallManager
mock checks, which slice-04 no longer hits. Restore the InMemoryVoipNative.recorded
seam assertions matching the slice-04 base.
Also remove the re-added `import InCallManager` that was no longer needed.
In MediaCallEvents.test.ts: add a jest.mock for CallLifecycle and update the
endCall test to assert callLifecycle.end('remote') is called rather than the
now-removed mediaSessionInstance.endCall delegation.
Blocker 1: mediaCall.hangup() emits 'ended' synchronously
(@rocket.chat/media-signaling Call.js line 703). The 'ended' listener in
useCallStore re-calls callLifecycle.end('remote') BEFORE the outer
end() finishes assigning _endPromise — bypassing the re-entry guard and
triggering double teardown (callEnded twice, end command twice, infinite
recursion in tests). Defer _runTeardown to a microtask so _endPromise
is captured first; the synchronous re-entrant call now hits the guard
and shares the in-flight promise.
Blocker 3: wrap step 1 (mediaCall.reject/hangup) in try/catch with a
logger.warn so a throw does not abort subsequent teardown steps
(markActive, markAvailable, store reset, stopAudio, callEnded), which
would leak listeners and native CallKit/Telecom state.
Tests: regression for synchronous re-entry asserts callEnded fires
exactly once (toHaveBeenCalledTimes/toHaveLength) and the end command
issues exactly once. Step-1 throw isolation tests assert the remaining
six steps still run when reject/hangup throws. Integration tests
updated to flush the new teardown microtask via await act().
Blocker 2: handleAcceptFailedEvent only forwarded the deep-link payload
without setting nativeAcceptedCallId. The downstream
deepLinking saga (handleVoipAcceptFailed) calls
callLifecycle.end('error'), which resolves the native callId via
`callId ?? nativeAcceptedCallId`. With neither set, end() has no
callUuid and the CallKit/Telecom session is never torn down.
Mirror the success path: call setNativeAcceptedCallId(payload.callId)
before opening the failure deep link so the saga's lifecycle.end()
can issue voipNative.call.end with the right id.
Adds a unit test asserting the failed-accept path stashes the callId.
diegolmello
force-pushed
the
voip-refactor/05-call-lifecycle-end-and-nav-router
branch
from
66a3a96 to
a0e1444
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
156-163:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReintroduce a navigation-ready gate before entering
CallView.This path still runs during cold-start/native-accept flows, but
CallNavRouteronly handles post-call back navigation. Removing the readiness wait here can drop the initialCallViewnavigation entirely and leave an accepted call active without UI.🤖 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 156 - 163, Reintroduce a navigation-ready gate before navigating to CallView: after mainCall.accept() and before Navigation.navigate('CallView') (around the block that calls voipNative.call.markActive, useCallStore.getState().setCall, setDirection and resolveRoomIdFromContact), await the existing waitForNavigationReady (or equivalent helper used by CallNavRouter) and only call Navigation.navigate('CallView') once it resolves; keep the resolveRoomIdFromContact error handling but ensure the navigation await is done first so cold-start/native-accept flows never skip initial UI.
♻️ Duplicate comments (4)
app/lib/services/voip/CallNavRouter.test.ts (1)
26-30:⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test still fails
import/first.Lines 27-30 are imports after executable module code, matching the ESLint errors in the static analysis. Convert these late imports to
require(...)after the mocks, or move the mocks below top-level imports.Suggested fix
-// Import after mocks are set up. -import { callLifecycle } from './CallLifecycle'; -import { CallNavRouter } from './CallNavRouter'; -import { emitter } from '../../methods/helpers'; -import Navigation from '../../navigation/appNavigation'; +// Import after mocks are set up. +const { callLifecycle } = require('./CallLifecycle'); +const { CallNavRouter } = require('./CallNavRouter'); +const { emitter } = require('../../methods/helpers'); +const Navigation = require('../../navigation/appNavigation').default;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/CallNavRouter.test.ts` around lines 26 - 30, The test currently performs executable setup before importing modules (CallLifecycle, CallNavRouter, emitter, Navigation), triggering import/first ESLint errors; fix by moving these top-level imports to the very top of CallNavRouter.test.ts (above any mock/setup code) OR convert the late imports (CallLifecycle, CallNavRouter, emitter, Navigation) into require(...) calls placed immediately after the mocks so that all static imports remain first; update references to use the required bindings and ensure no executable statements precede the static import statements.app/lib/services/voip/MediaCallEvents.ts (1)
95-98:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle the teardown Promise in this synchronous dispatcher.
dispatchVoipNativeEvent()is sync, butcallLifecycle.end('remote')is async. If teardown rejects here, it escapes as an unhandled Promise rejection from the native end-call path.Suggested fix
- callLifecycle.end('remote'); + void callLifecycle.end('remote').catch(error => { + mediaCallLogger.error(`${TAG} callLifecycle.end failed:`, error); + }); return false;As per coding guidelines "Use explicit error handling with try/catch blocks for async operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/lib/services/voip/MediaCallEvents.ts` around lines 95 - 98, dispatchVoipNativeEvent currently calls the async callLifecycle.end('remote') synchronously which can produce unhandled rejections; change the call to explicitly handle the Promise with try/catch or a .catch handler so any rejection is logged and swallowed. Specifically, in MediaCallEvents' 'endCall' branch where mediaCallLogger.log(`${TAG} End call event listener:`, e.callUuid) and callLifecycle.end('remote') are called, wrap the async teardown in an awaited try/catch (or attach .catch(...)) and use mediaCallLogger.error to record the error before returning false so the dispatcher stays synchronous and no unhandled Promise escapes.app/sagas/deepLinking.js (1)
89-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAwait the lifecycle teardown before
resetVoipState().This generator calls
callLifecycle.end('error')as fire-and-forget, so Line 93 can run before the deferred teardown resolvescallId ?? nativeAcceptedCallId. On the accept-failure path, that can clear the only native UUID before CallKit/Telecom teardown runs, and the outertry/catchalso won't see a rejected teardown promise.Suggested fix
- callLifecycle.end('error'); + yield call([callLifecycle, callLifecycle.end], 'error'); resetVoipState();As per coding guidelines "Use explicit error handling with try/catch blocks for async operations".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/sagas/deepLinking.js` around lines 89 - 93, The callLifecycle.end('error') call is being invoked fire-and-forget causing resetVoipState() to run before teardown; change this to await the teardown and handle errors explicitly: call await callLifecycle.end('error') inside a try/catch immediately before calling resetVoipState(), log or rethrow the caught error as appropriate so the promise rejection is observed and the native callId/nativeAcceptedCallId is not cleared before CallKit/Telecom teardown completes (refer to callLifecycle.end and resetVoipState to locate the change).app/lib/services/voip/useCallStore.ts (1)
202-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBoth
callLifecycle.end(...)calls need explicit rejection handling.These are synchronous store callbacks, so both branches currently fire async teardown and drop the returned Promise. If
CallLifecycle.end()rejects, the failure is unhandled and the caller cannot observe that cleanup broke. A small shared helper that doesvoid ...catch(logger)would cover both sites.As per coding guidelines "Use explicit error handling with try/catch blocks for async operations".
Also applies to: 274-277
🤖 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 202 - 205, handleEnded currently calls callLifecycle.end('remote') without handling its returned Promise; do the same fix at the other site noted (the async teardown call around lines 274-277). Add a small local helper (e.g. safeEnd = (reason: string) => void callLifecycle.end(reason).catch(logger.error)) and replace direct calls to callLifecycle.end(...) in handleEnded and the other callback with safeEnd(...), ensuring you reference the existing logger (processLogger / logger) used in this module so rejections are explicitly caught and logged.
🤖 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 140-143: The 'ended' emitter handler should ignore stale
post-teardown emissions: change the current call.emitter.on('ended', ...) to
either use a guarded handler or once-removal that checks the session's active
teardown state (e.g., verify this._endPromise is present) before invoking
callLifecycle.end('remote'); also capture the relevant call/context in a local
variable if needed and remove the listener after firing to avoid mapping later
emissions from an old call to callLifecycle.end('remote').
---
Outside diff comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 156-163: Reintroduce a navigation-ready gate before navigating to
CallView: after mainCall.accept() and before Navigation.navigate('CallView')
(around the block that calls voipNative.call.markActive,
useCallStore.getState().setCall, setDirection and resolveRoomIdFromContact),
await the existing waitForNavigationReady (or equivalent helper used by
CallNavRouter) and only call Navigation.navigate('CallView') once it resolves;
keep the resolveRoomIdFromContact error handling but ensure the navigation await
is done first so cold-start/native-accept flows never skip initial UI.
---
Duplicate comments:
In `@app/lib/services/voip/CallNavRouter.test.ts`:
- Around line 26-30: The test currently performs executable setup before
importing modules (CallLifecycle, CallNavRouter, emitter, Navigation),
triggering import/first ESLint errors; fix by moving these top-level imports to
the very top of CallNavRouter.test.ts (above any mock/setup code) OR convert the
late imports (CallLifecycle, CallNavRouter, emitter, Navigation) into
require(...) calls placed immediately after the mocks so that all static imports
remain first; update references to use the required bindings and ensure no
executable statements precede the static import statements.
In `@app/lib/services/voip/MediaCallEvents.ts`:
- Around line 95-98: dispatchVoipNativeEvent currently calls the async
callLifecycle.end('remote') synchronously which can produce unhandled
rejections; change the call to explicitly handle the Promise with try/catch or a
.catch handler so any rejection is logged and swallowed. Specifically, in
MediaCallEvents' 'endCall' branch where mediaCallLogger.log(`${TAG} End call
event listener:`, e.callUuid) and callLifecycle.end('remote') are called, wrap
the async teardown in an awaited try/catch (or attach .catch(...)) and use
mediaCallLogger.error to record the error before returning false so the
dispatcher stays synchronous and no unhandled Promise escapes.
In `@app/lib/services/voip/useCallStore.ts`:
- Around line 202-205: handleEnded currently calls callLifecycle.end('remote')
without handling its returned Promise; do the same fix at the other site noted
(the async teardown call around lines 274-277). Add a small local helper (e.g.
safeEnd = (reason: string) => void
callLifecycle.end(reason).catch(logger.error)) and replace direct calls to
callLifecycle.end(...) in handleEnded and the other callback with safeEnd(...),
ensuring you reference the existing logger (processLogger / logger) used in this
module so rejections are explicitly caught and logged.
In `@app/sagas/deepLinking.js`:
- Around line 89-93: The callLifecycle.end('error') call is being invoked
fire-and-forget causing resetVoipState() to run before teardown; change this to
await the teardown and handle errors explicitly: call await
callLifecycle.end('error') inside a try/catch immediately before calling
resetVoipState(), log or rethrow the caught error as appropriate so the promise
rejection is observed and the native callId/nativeAcceptedCallId is not cleared
before CallKit/Telecom teardown completes (refer to callLifecycle.end and
resetVoipState to locate the change).
🪄 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: f837037f-0b3e-4d18-b58f-5b087dea04bf
📒 Files selected for processing (13)
app/AppContainer.tsxapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallNavRouter.test.tsapp/lib/services/voip/CallNavRouter.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/useCallStore.test.tsapp/lib/services/voip/useCallStore.tsapp/sagas/deepLinking.js
🚧 Files skipped from review as they are similar to previous changes (2)
- app/AppContainer.tsx
- app/lib/services/voip/CallNavRouter.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Use descriptive names for functions, variables, and classes that clearly convey their purpose
Write comments that explain the 'why' behind code decisions, not the 'what'
Keep functions small and focused on a single responsibility
Use const by default, let when reassignment is needed, and avoid var
Prefer async/await over .then() chains for handling asynchronous operations
Use explicit error handling with try/catch blocks for async operations
Avoid deeply nested code; refactor complex logic into helper functions
Files:
app/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/useCallStore.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallNavRouter.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/useCallStore.test.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallNavRouter.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use tabs for indentation with single quotes, 130 character line width, no trailing commas, and avoid arrow function parentheses when possible
Use ESLint with@rocket.chat/eslint-configbase including React, React Native, TypeScript, and Jest plugins
Files:
app/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/useCallStore.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallNavRouter.test.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/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/useCallStore.test.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallNavRouter.test.ts
app/containers/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Create reusable UI components in app/containers/ directory
Files:
app/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
🧠 Learnings (9)
📓 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/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/useCallStore.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallNavRouter.test.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/MediaCallEvents.test.tsapp/lib/services/voip/CallLifecycle.test.ts
📚 Learning: 2026-04-30T16:41:37.607Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7279
File: app/lib/services/voip/playCallEndedSound.ts:39-46
Timestamp: 2026-04-30T16:41:37.607Z
Learning: In `app/lib/services/voip/playCallEndedSound.ts`, the `isPlaying` lock is intentionally released only via `didJustFinish` (natural completion) plus a 5-second watchdog timer as a stuck-lock safety net. Treating `status.isPlaying === false && status.isLoaded === true` as a terminal state is explicitly avoided because it would falsely trigger on transient audio pauses (focus loss, iOS session interruption, Bluetooth handoff), prematurely releasing the lock and risking doubled playback when audio resumes.
Applied to files:
app/lib/services/voip/MediaCallEvents.test.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/useCallStore.test.tsapp/sagas/deepLinking.jsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsxapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/useCallStore.ts
📚 Learning: 2026-04-07T17:49:25.836Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T17:49:25.836Z
Learning: Applies to **/*.{js,ts,jsx,tsx} : Use explicit error handling with try/catch blocks for async operations
Applied to files:
app/lib/services/voip/MediaCallEvents.ts
📚 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/sagas/videoConf.ts,app/lib/methods/videoConf.ts} : Implement video conferencing in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using Redux actions, reducers, and sagas for server-managed Jitsi integration
Applied to files:
app/lib/services/voip/MediaCallEvents.tsapp/sagas/deepLinking.jsapp/lib/services/voip/MediaSessionInstance.tsapp/containers/NewMediaCall/VoipCallLifecycle.integration.test.tsx
📚 Learning: 2026-04-07T17:49:25.836Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T17:49:25.836Z
Learning: Applies to **/*.{js,ts,jsx,tsx} : Prefer async/await over .then() chains for handling asynchronous operations
Applied to files:
app/lib/services/voip/CallLifecycle.test.ts
📚 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/index.tsx : Set up Redux provider, theme, navigation, and notifications in app/index.tsx
Applied to files:
app/lib/services/voip/useCallStore.ts
📚 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/lib/services/voip/useCallStore.ts
🪛 ESLint
app/lib/services/voip/CallLifecycle.test.ts
[error] 43-43: Import in body of module; reorder to top.
(import/first)
[error] 45-45: Import in body of module; reorder to top.
(import/first)
[error] 46-46: Import in body of module; reorder to top.
(import/first)
[error] 47-47: Import in body of module; reorder to top.
(import/first)
[error] 48-48: Import in body of module; reorder to top.
(import/first)
[error] 114-114: Use object destructuring.
(prefer-destructuring)
[error] 252-252: Unexpected await inside a loop.
(no-await-in-loop)
app/lib/services/voip/CallNavRouter.test.ts
[error] 27-27: Import in body of module; reorder to top.
(import/first)
[error] 28-28: Import in body of module; reorder to top.
(import/first)
[error] 29-29: Import in body of module; reorder to top.
(import/first)
[error] 30-30: Import in body of module; reorder to top.
(import/first)
🔇 Additional comments (1)
app/lib/services/voip/CallLifecycle.ts (1)
158-178: Still need per-step isolation after step 1.
mediaCall.reject()/hangup()is protected, butnative.call.end(),markActive(),markAvailable(),useCallStore.getState().reset(), andstopAudio()can still throw and abort the remainder of teardown. That would skipcallEndedemission and leave navigation/cleanup subscribers waiting forever.
- CallNavRouter: track and clean up the deferred navigationReady listener in unmount(); prevents leak that could re-subscribe callEnded after unmount when nav was not yet ready at mount time. Adds regression test. - deepLinking saga: yield call(callLifecycle.end, 'error') instead of a bare invocation so teardown completes before resetVoipState() and navigation proceed (avoids race with in-flight teardown). - MediaCallEvents / useCallStore / MediaSessionInstance: attach .catch() to all callLifecycle.end() invocations from synchronous callers so a rejection during teardown is logged via mediaCallLogger instead of becoming an unhandled promise rejection. - CallLifecycle: wrap teardown steps 2-6 (native.call.end / markActive / markAvailable, useCallStore.reset, native.call.stopAudio) in a local safe(label, fn) helper so a throw in any step is logged but does not abort the rest of the sequence; callEnded MUST always emit. Adds per-step regression tests. - Test files: reorder imports above jest.mock() to satisfy import/first; use destructuring for native.recorded; eslint-disable no-await-in-loop in the reason payload sequencing test.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/lib/services/voip/CallLifecycle.ts`:
- Around line 67-72: The emit method in CallLifecycle.ts currently lets a
throwing listener abort the loop and cause end() to reject; wrap each listener
invocation inside a try/catch in emit<K extends keyof EventMap>(event: K,
payload: EventMap[K]) so one subscriber failure doesn't stop notifying the rest
or cause teardown to fail—catch and log the error (using the class logger or
console.error) and continue to the next listener, ensuring end() always
completes even if some callEnded listeners throw.
🪄 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: 2887ff31-595b-46ab-b8e4-e47f200538b2
📒 Files selected for processing (8)
app/lib/services/voip/CallLifecycle.test.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallNavRouter.test.tsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/MediaCallEvents.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/useCallStore.tsapp/sagas/deepLinking.js
🚧 Files skipped from review as they are similar to previous changes (3)
- app/lib/services/voip/MediaCallEvents.ts
- app/lib/services/voip/MediaSessionInstance.ts
- app/lib/services/voip/CallNavRouter.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.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/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbers
**/*.{ts,tsx}: Use TypeScript with strict mode enabled and baseUrl set to app/ for module imports
Support iOS 13.4+ and Android 6.0+ as minimum target platforms
Files:
app/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.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/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.ts
🧠 Learnings (8)
📓 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/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.ts
📚 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/sagas/videoConf.ts,app/lib/methods/videoConf.ts} : Implement video conferencing in app/sagas/videoConf.ts and app/lib/methods/videoConf.ts using Redux actions, reducers, and sagas for server-managed Jitsi integration
Applied to files:
app/sagas/deepLinking.jsapp/lib/services/voip/useCallStore.ts
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In the RocketChat/Rocket.Chat.ReactNative codebase, the ESLint configuration enforces `no-void: error`, so floating promises must be handled with `.catch(...)` rather than the `void` prefix. Do not suggest `void somePromise()` as a fix for unhandled promises in this project.
Applied to files:
app/sagas/deepLinking.js
📚 Learning: 2026-04-30T16:41:37.607Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7279
File: app/lib/services/voip/playCallEndedSound.ts:39-46
Timestamp: 2026-04-30T16:41:37.607Z
Learning: In `app/lib/services/voip/playCallEndedSound.ts`, the `isPlaying` lock is intentionally released only via `didJustFinish` (natural completion) plus a 5-second watchdog timer as a stuck-lock safety net. Treating `status.isPlaying === false && status.isLoaded === true` as a terminal state is explicitly avoided because it would falsely trigger on transient audio pauses (focus loss, iOS session interruption, Bluetooth handoff), prematurely releasing the lock and risking doubled playback when audio resumes.
Applied to files:
app/sagas/deepLinking.jsapp/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.ts
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/voip/CallNavRouter.tsapp/lib/services/voip/useCallStore.tsapp/lib/services/voip/CallLifecycle.tsapp/lib/services/voip/CallLifecycle.test.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/CallLifecycle.test.ts
📚 Learning: 2026-04-07T17:49:25.836Z
Learnt from: CR
Repo: RocketChat/Rocket.Chat.ReactNative PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-07T17:49:25.836Z
Learning: Applies to **/*.{js,ts,jsx,tsx} : Prefer async/await over .then() chains for handling asynchronous operations
Applied to files:
app/lib/services/voip/CallLifecycle.test.ts
🔇 Additional comments (1)
app/lib/services/voip/useCallStore.ts (1)
287-299: Verify no remaining directreset()paths depend on audio shutdown.After auditing all direct
reset()call sites: the architectural change is sound and properly guarded.All production paths calling
reset()are either (1) withinCallLifecycle.end()where step 6 explicitly stops audio, or (2) during session teardown (app disconnect or startup permission check) when no active audio session exists. Test invocations are cleanup fixtures. No call site can leave in-call audio orphaned.
The closure-captured `call` in the `newCall` 'ended' subscription could
fire after teardown completed (delayed server signal, late-arriving
event), triggering a spurious second `callLifecycle.end('remote')` and
duplicate `callEnded` event with the wrong reason. Short-circuit when
the active call/callId in the store no longer matches the captured one.
codex: address PR review feedback (#7274)
|
[codex] Slice 05 walkthrough — TL;DRSlice 05 collapses end-of-call teardown and post-call navigation into two focused modules. Before: teardown logic was scattered across
|
| Site | File | Reason | Context |
|---|---|---|---|
| OS endCall (CallKit / Telecom) | MediaCallEvents.ts:97 |
'remote' |
sync; .catch() logs |
MediaCall 'ended' (store handler) |
useCallStore.ts:208 (handleEnded) |
'remote' |
sync; .catch() logs |
| In-app hangup button | useCallStore.ts:282 (endCall) |
'local' |
sync; .catch() logs |
Per-call 'ended' listener |
MediaSessionInstance.ts:150 (init newCall) |
'remote' |
sync; stale-call guard: bails if activeCall.callId !== call.callId && activeCallId !== call.callId |
MediaSessionInstance.endCall |
MediaSessionInstance.ts:223 |
'local' |
sync; one-line delegate |
handleVoipAcceptFailed saga |
app/sagas/deepLinking.js:94 |
'error' |
awaited via yield call([callLifecycle, callLifecycle.end], 'error') so resetVoipState() and waitForNavigationReady() cannot race teardown |
The acceptFailed event handler (MediaCallEvents.handleAcceptFailedEvent) stashes nativeAcceptedCallId before opening the failure deep link. That's how the saga's later end('error') can resolve effectiveCallId and actually tear down the native session — without it, the failure path was leaking the CallKit / Telecom call.
Invariants the tests lock in
- Concurrent
end()callers share one in-flight promise (_endPromisere-entry guard). - Synchronous re-entry from inside teardown (microtask deferral) hits that same guard.
- A throw in any step 1–6 is logged and the remaining steps still run;
callEndedalways emits. - A throwing
callEndedlistener does not reject the awaitedend()promise and does not skip subsequent listeners. - A stale
'ended'emission on a capturedcallwhose id no longer matches the active store state is ignored (no double teardown, no spuriouscallEnded). acceptFailedstashesnativeAcceptedCallIdso the saga'send('error')can resolve the right id.CallNavRouterdoes not subscribecallEndedbeforenavigationReadyand does not retain anavigationReadylistener afterunmount().
Strict-once tests assert toHaveBeenCalledTimes(1) and events.toHaveLength(1) rather than toContainEqual, so duplicate emissions cannot pass silently.
Not in this slice (scope guard)
- Slice 06 —
answerIncoming/beginOutgoinglifecycle methods (the start-of-call counterparts ofend). - Slice 07 — toggle methods (mute / hold / speaker) routed through the lifecycle / native seam.
- Slice 08 — Pre-bind FSM (
cleanupAt-driven cleanup; the'cleanup'reason inCallEndReasonis reserved for it but has no producer yet).
diegolmello
merged commit 6465723
into
voip-refactor/04-voip-native-attach-and-cold-start
diegolmello
deleted the
voip-refactor/05-call-lifecycle-end-and-nav-router
branch
1 participant
Proposed changes
Introduces
CallLifecycleandCallNavRouter, the two new orchestration modules for slice 05 of the VoIP native-call-seam refactor.CallLifecycleowns the end-of-call teardown sequence with a documented, tested order and idempotency guarantee:mediaCall.reject()if ringing, elsemediaCall.hangup()(wrapped in try/catch — a throw does not abort subsequent steps)voipNative.call.end(callUuid)voipNative.call.markActive('')voipNative.call.markAvailable(callUuid)useCallStore.reset()voipNative.call.stopAudio()(after reset — subscribers see clean state whencallEndedemits)callEnded { callId, reason }Steps 2-6 are also wrapped in a local
safe(label, fn)helper so a throw in any teardown step is logged but does not abort the rest of the sequence;callEndedis guaranteed to emit.Concurrent callers share the in-flight Promise (re-entry guard). The teardown body is deferred to a microtask so the in-flight promise is captured BEFORE step 1 runs — synchronous re-entry from
mediaCall.hangup()'s'ended'emit hits the guard.callIdusescallId ?? nativeAcceptedCallId(Pre-bind-safe).CallNavRouteris a lightweight subscriber mounted atAppContainer. It subscribes toCallLifecycleevents only afterNavigationContaineris ready (via the existingemitter.emit('navigationReady')hook), and callsNavigation.back()whencallEndedfires and the current route isCallView.unmount()cleans up both thecallEndedlistener AND the deferrednavigationReadylistener.Six end-call paths consolidated to
lifecycle.end(reason):MediaSessionInstance.endCall→lifecycle.end('local')useCallStore.endCall→lifecycle.end('local')useCallStore.setCallhandleEnded→lifecycle.end('remote')(removes inlineNavigation.back)MediaSessionInstance.initper-call'ended'listener →lifecycle.end('remote')deepLinking.jshandleVoipAcceptFailed→lifecycle.end('error')(yielded viaredux-sagacallso teardown completes before navigation continues)waitForNavigationReadyremoved fromMediaSessionInstance.answerCallOS-originated end-call events (
endCallfrom CallKit/Telecom) now dispatchcallLifecycle.end('remote')directly rather than routing throughmediaSessionInstance.endCall(), ensuring thecallEndedevent carries the correct'remote'reason instead of'local'.All synchronous callers attach
.catch(error => mediaCallLogger.error(...))to the returnedPromise<void>so a teardown rejection is logged instead of becoming an unhandled promise rejection.stopAudioownership moved fromuseCallStore.resettoCallLifecyclestep 6.Production blockers fixed in audit pass
mediaCall.hangup()synchronously emits'ended'(@rocket.chat/media-signalingCall.js line 703) →useCallStore.handleEndedsynchronously re-callscallLifecycle.end('remote')BEFORE the outerend()finishes assigning_endPromise. Fix: defer_runTeardownto a microtask viaPromise.resolve().then(...)so_endPromiseis captured first.handleAcceptFailedEventdropped the callId: the success path stashesnativeAcceptedCallId, but the failure path didn't — so the downstream saga'slifecycle.end('error')had nocallUuidto issue to CallKit/Telecom. Fix: mirror the success path and setnativeAcceptedCallIdbefore opening the failure deep link.mediaCall.reject()ormediaCall.hangup()aborted the remaining six teardown steps, leaking listeners and native state. Fix: wrap step 1 in try/catch withlogger.warn; subsequent steps still run.CodeRabbit feedback addressed (commit 3227728)
CallNavRouter.unmount()listener leak: the deferrednavigationReadylistener registered when nav was not yet ready was never cleaned up. Ifunmount()ran beforenavigationReadyemitted, the listener stayed alive and could later subscribecallEndedwhile the router was "unmounted". Fix: track the listener's off-handle in a module-scoped_unsubscribeNavigationReady, clear it in bothunmount()and insideonceNavigationReady. Regression test added.deepLinking.jssaga did not yield the teardown Promise: the barecallLifecycle.end('error')call returnedPromise<void>not awaited by the saga, soresetVoipState()andwaitForNavigationReady()could proceed before teardown completed. Fix:yield call([callLifecycle, callLifecycle.end], 'error').callLifecycle.end()at five sync sites: each site now attaches a.catch(error => mediaCallLogger.error(...)). Sites updated:MediaCallEvents.ts:97,useCallStore.ts(handleEnded,endCall),MediaSessionInstance.ts:142(newCall ended),MediaSessionInstance.ts:213(endCall).safe(label, fn)helper inside_runTeardownthat try/catches each step and logs warn.callEndednow always emits even ifnative.call.end/markActive/markAvailable/useCallStore.reset/native.call.stopAudiothrows. Five per-step regression tests added.jest.mock(...)in both test files (import/first);const { recorded } = native;instead ofnative.recorded(prefer-destructuring);eslint-disable-next-line no-await-in-loopfor the reasons-sequencing test loop.Issue(s)
.scratch/voip-native-call-seam/issues/05-call-lifecycle-end-and-nav-router.mdHow to test or reproduce
useCallStore.endCall) — store clears, native teardown runs, navigation returns to previous screenendCallevent) — same teardown fires,callEndedreason is'remote''ended'event from WebRTC) — same teardown, navigation backvoipAcceptFailed) — same teardown path, with the native CallKit/Telecom session correctly torn down via the stashednativeAcceptedCallIdScreenshots
N/A (no visible UI changes — orchestration layer only)
Types of changes
Checklist
Further comments
Test plan
Unit tests (all pass — 1331 across 151 suites in scoped run):
CallLifecycle.test.ts: teardown ordering, hangup vs reject, concurrentend()idempotency,callEndedexactly-once, Pre-bindcallId ?? nativeAcceptedCallId, reason threading, re-entry guard against synchronous'ended'emission, step-1 throw isolation (hangup and reject paths), steps 2-6 throw isolation (5 new tests, one per step)CallNavRouter.test.ts: callEnded→goBack on CallView, no-op on other routes, subscription only afternavigationReady,mount()idempotency, unmount/remount, deferrednavigationReadylistener cleanup on unmount-before-readyMediaSessionInstance.test.ts:endCalldelegates tolifecycle.end('local')useCallStore.test.ts:reset()no longer ownsstopAudioMediaCallEvents.test.ts: OSendCallevent callscallLifecycle.end('remote'); failed-accept path stashesnativeAcceptedCallIdVoipCallLifecycle.integration.test.tsx: passing —InMemoryVoipNative.recordedassertions, microtask flush after teardown triggerStrict-once assertions: regression tests for the re-entry guard use
toHaveBeenCalledTimes(1)and.toHaveLength(1)rather thantoContainEqual, so duplicate emissions cannot silently pass.Manual verification (iOS/Android):
callEnded.reason === 'remote'Stacks on
This PR stacks on
voip-refactor/04-voip-native-attach-and-cold-start(PR #7272).Summary by CodeRabbit
Bug Fixes
Improvements
Tests