fix(android/voip): start microphone FGS for outgoing calls so audio survives backgrounding#7346
Conversation
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughAdds microphone to Android VoIP foreground-service types, introduces a JS → native startVoipCallService promise flow with success/failure/timeout handling, integrates native start into outgoing Android call startup, updates tests/docs, and adds an iOS no-op TurboModule method. ChangesVoIP Foreground Service Permissions
Sequence DiagramsequenceDiagram
participant MediaSessionInstance
participant Platform
participant NativeVoipModule
participant VoipModule
participant VoipCallService
MediaSessionInstance->>Platform: isAndroid()
alt Android && outgoing caller
MediaSessionInstance->>NativeVoipModule: startVoipCallService(callId)
NativeVoipModule->>VoipModule: startVoipCallService(callId, promise)
VoipModule->>VoipCallService: startService(callId)
VoipCallService->>VoipCallService: startForegroundWithNotification(type: MICROPHONE|PHONE_CALL)
VoipCallService-->>VoipModule: notifyFgsStarted(callId) / notifyFgsFailed(callId, throwable)
VoipModule-->>NativeVoipModule: resolve / reject promise
NativeVoipModule-->>MediaSessionInstance: promise resolved / rejected
else not Android or not caller
MediaSessionInstance->>MediaSessionInstance: continue without native start
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 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.
🧹 Nitpick comments (1)
app/lib/services/voip/docs/PLATFORMS.md (1)
71-71: ⚡ Quick winConsider expanding the FSI acronym for clarity.
The term "FSI" refers to FullScreenIntent, which may not be immediately familiar to all readers of this documentation. Consider expanding it on first use for improved clarity.
📝 Suggested clarification
- - `phoneCall` satisfies FGS-**start** eligibility on the incoming-accept path. That path starts the service from `VoipNotification.handleAcceptAction` _after_ `connection.onAnswer()` puts the self-managed Telecom call into `STATE_ACTIVE`, which is what makes `phoneCall` startable from a non-foreground entry point (FSI / `onNewIntent`). + - `phoneCall` satisfies FGS-**start** eligibility on the incoming-accept path. That path starts the service from `VoipNotification.handleAcceptAction` _after_ `connection.onAnswer()` puts the self-managed Telecom call into `STATE_ACTIVE`, which is what makes `phoneCall` startable from a non-foreground entry point (FullScreenIntent / `onNewIntent`).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/services/voip/docs/PLATFORMS.md` at line 71, In the sentence referencing "FSI / `onNewIntent`", expand the acronym FSI to "FullScreenIntent" on first use (e.g., "FSI (FullScreenIntent)") so readers understand the term; update the documentation near the `phoneCall` / `VoipNotification.handleAcceptAction` / `connection.onAnswer()` / `STATE_ACTIVE` / `onNewIntent` discussion to use the full form on first mention and you can keep the acronym thereafter.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/lib/services/voip/docs/PLATFORMS.md`:
- Line 71: In the sentence referencing "FSI / `onNewIntent`", expand the acronym
FSI to "FullScreenIntent" on first use (e.g., "FSI (FullScreenIntent)") so
readers understand the term; update the documentation near the `phoneCall` /
`VoipNotification.handleAcceptAction` / `connection.onAnswer()` / `STATE_ACTIVE`
/ `onNewIntent` discussion to use the full form on first mention and you can
keep the acronym thereafter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4326d72e-848d-4f97-856e-dc30e0a77917
📒 Files selected for processing (7)
android/app/src/main/AndroidManifest.xmlandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/docs/PLATFORMS.md
📜 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/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types
Prefer interfaces over type aliases for defining object shapes in TypeScript
Use enums for sets of related constants rather than magic strings or numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
VoIP service implementation should use Zustand stores (not Redux) and include native CallKit (iOS) and Telecom (Android) integration in app/lib/services/voip/
Files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.ts
🔇 Additional comments (6)
android/app/src/main/AndroidManifest.xml (1)
156-161: LGTM!Also applies to: 166-166
android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.kt (1)
103-109: LGTM!Also applies to: 113-113
android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt (1)
174-186: LGTM!app/lib/native/NativeVoip.ts (1)
38-47: LGTM!Also applies to: 103-103
app/lib/services/voip/MediaSessionInstance.ts (1)
11-11: LGTM!Also applies to: 17-17, 133-144
app/lib/services/voip/MediaSessionInstance.test.ts (1)
115-115: LGTM!Also applies to: 118-121, 419-455
|
Android Build Available Rocket.Chat 4.73.0.108958 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNQo_0Nd4NQDTF4EalfL0AhqAXbVkARZkMVLb_Acc91h3F1WXNEPnVtiAvKqPMCt6F1Ybz1HdvCbSWLQInxW |
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
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/native/NativeVoip.ts (1)
96-105: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMake the fallback fail closed on Android.
If
TurboModuleRegistry.get('VoipModule')returnsnullon Android, this stub resolves and the caller will keep the outgoing call alive even though no FGS was started. That bypasses the new teardown path this PR adds for FGS-start failures. Please reject here on Android instead of silently succeeding, and make the stub signature explicit while you’re touching it.♻️ Proposed fix
-import { TurboModuleRegistry } from 'react-native'; +import { Platform, TurboModuleRegistry } from 'react-native'; ... - startVoipCallService: () => Promise.resolve(), + startVoipCallService: (_callId: string): Promise<void> => + Platform.OS === 'android' + ? Promise.reject(new Error('VoipModule.startVoipCallService is unavailable on Android')) + : Promise.resolve(),As per coding guidelines
**/*.{ts,tsx}: Use TypeScript for type safety; add explicit type annotations to function parameters and return types.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/native/NativeVoip.ts` around lines 96 - 105, The fallback NativeVoipModule stub currently silently succeeds when TurboModuleRegistry.get('VoipModule') returns null, which keeps outgoing calls alive on Android; change the stub so on Android (Platform.OS === 'android') startVoipCallService returns a rejected Promise (e.g. Promise.reject(new Error('VoipModule unavailable'))) to fail closed and propagate the error to the caller, and add explicit TypeScript signatures for the stubbed methods (registerVoipToken, getInitialEvents, clearInitialEvents, getLastVoipToken, stopNativeDDPClient, startVoipCallService, stopVoipCallService) to match the Spec interface for type safety.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 140-145: The Android FGS start is async so you must gate the rest
of the outgoing-call setup on a successful start: move the navigation to
CallView and the DM/room lookup logic currently running after the
startVoipCallService call into the success path (the .then / resolved branch) of
NativeVoipModule.startVoipCallService so they only run when the FGS starts; keep
the existing .catch to log, show the error alert and call
this.endCall(call.callId) on failure. Ensure you reference
NativeVoipModule.startVoipCallService, the code that navigates to CallView, and
the room/DM resolution logic and wrap them in the success handler so a rejected
start remains a clean failure.
---
Outside diff comments:
In `@app/lib/native/NativeVoip.ts`:
- Around line 96-105: The fallback NativeVoipModule stub currently silently
succeeds when TurboModuleRegistry.get('VoipModule') returns null, which keeps
outgoing calls alive on Android; change the stub so on Android (Platform.OS ===
'android') startVoipCallService returns a rejected Promise (e.g.
Promise.reject(new Error('VoipModule unavailable'))) to fail closed and
propagate the error to the caller, and add explicit TypeScript signatures for
the stubbed methods (registerVoipToken, getInitialEvents, clearInitialEvents,
getLastVoipToken, stopNativeDDPClient, startVoipCallService,
stopVoipCallService) to match the Spec interface for type safety.
🪄 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: b7c7c298-3b1d-4df0-88a1-54024f8c931b
📒 Files selected for processing (6)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/docs/PLATFORMS.mdios/Libraries/VoipModule.mm
✅ Files skipped from review due to trivial changes (1)
- app/lib/services/voip/docs/PLATFORMS.md
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: ESLint and Test / run-eslint-and-test
- GitHub Check: format
🧰 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/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.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 numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
VoIP service implementation should use Zustand stores (not Redux) and include native CallKit (iOS) and Telecom (Android) integration in app/lib/services/voip/
Files:
app/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.ts
🧠 Learnings (3)
📚 Learning: 2026-03-05T06:06:12.277Z
Learnt from: divyanshu-patil
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6957
File: ios/RCTWatchModule.mm:19-24
Timestamp: 2026-03-05T06:06:12.277Z
Learning: Do not re-activate or reset the WCSession singleton in iOS Objective-C/Swift bridge modules. Ensure WCSession is activated and its delegate is set in a single, central place (e.g., ios/RocketChat Watch App/Loaders/WatchSession.swift) and avoid duplicating activation or delegate assignment in other iOS bridge files like ios/RCTWatchModule.mm. If WCSession is already activated via the central loader, relying on WCSession.defaultSession is sufficient and maintains a single session lifecycle.
Applied to files:
ios/Libraries/VoipModule.mm
📚 Learning: 2026-05-12T21:58:10.053Z
Learnt from: Rohit3523
Repo: RocketChat/Rocket.Chat.ReactNative PR: 6991
File: ios/Libraries/Challenge.mm:29-37
Timestamp: 2026-05-12T21:58:10.053Z
Learning: For iOS code using react-native-mmkv, when calling `[MMKV initializeMMKV:nil groupDir:groupDir logLevel:...]` (and when passing `rootPath` to MMKVBridge), pass the raw App Group container URL path only (e.g., `[[NSFileManager defaultManager] containerURLForSecurityApplicationGroupIdentifier:appGroup] path]`). Do NOT append `/mmkv` yourself. The MMKV SDK already appends `/mmkv` internally (`...stringByAppendingPathComponent:@"mmkv"`), so passing `<AppGroup>/mmkv` will cause double-nesting (`<AppGroup>/mmkv/mmkv`) and can break shared storage/SSL cert lookups.
Applied to files:
ios/Libraries/VoipModule.mm
📚 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/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.ts
🔇 Additional comments (3)
ios/Libraries/VoipModule.mm (1)
120-125: LGTM!android/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.kt (1)
174-189: LGTM!app/lib/native/NativeVoip.ts (1)
38-47: LGTM!
diegolmello
left a comment
diegolmello
left a comment
There was a problem hiding this comment.
Single inline finding from a structural review — otherwise this PR is clean (focused, no spaghetti added to unrelated flows, no file pushed past 1k lines, TurboModule contract is explicit, failure path correctly tears down rather than silently degrading).
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.73.0.108965 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNTRLpZ8RydHT9xtZe1ufHif875I6tmpLS26bLxRi-Jw8cbtltCDdtZOlcvDmV0fz1PXN6SkzivuoqsCFUZt |
|
iOS Build Available Rocket.Chat 4.73.0.108966 |
diegolmello
temporarily deployed
to
approve_e2e_testing
GitHub Actions
Inactive
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/lib/native/NativeVoip.ts (1)
99-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject when
VoipModuleis unavailable on Android.
startVoipCallService()is now the success gate for outgoing-call setup. IfTurboModuleRegistry.get('VoipModule')returnsnullon Android, this fallback resolves and the app proceeds as if the FGS started, which masks the broken native integration and reintroduces the background mic-drop path instead of failing fast.Proposed fix
import type { TurboModule } from 'react-native'; -import { TurboModuleRegistry } from 'react-native'; +import { Platform, TurboModuleRegistry } from 'react-native'; @@ getInitialEvents: () => null, clearInitialEvents: () => undefined, getLastVoipToken: () => '', stopNativeDDPClient: () => undefined, - startVoipCallService: () => Promise.resolve(), + startVoipCallService: () => + Platform.OS === 'android' + ? Promise.reject(new Error('VoipModule is unavailable on Android')) + : Promise.resolve(), stopVoipCallService: () => undefined, setSpeakerOn: () => Promise.resolve(false),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/native/NativeVoip.ts` around lines 99 - 116, The fallback for TurboModuleRegistry.get('VoipModule') silently returns no-op implementations (NativeVoipModule) which makes startVoipCallService succeed on Android when the native module is missing; change the fallback so that missing module causes failures: when TurboModuleRegistry.get<Spec>('VoipModule') is null/undefined, replace the no-op object with an implementation whose startVoipCallService (and any other critical async startup methods like startAudioRouteSync/startRingback) return a rejected Promise (or otherwise throw) indicating the VoipModule is unavailable, and ensure getInitialEvents/getLastVoipToken behave safely but do not hide the missing-module error so callers of NativeVoipModule.startVoipCallService see a rejection instead of a resolved Promise.
♻️ Duplicate comments (1)
app/lib/services/voip/MediaSessionInstance.ts (1)
141-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGate the DM lookup on FGS success as well.
Navigation.navigate()is gated now, butresolveRoomIdFromContact()still starts before the FGS promise settles. IfstartVoipCallService()rejects,endCall()resets the store and the in-flight lookup can write a staleroomIdback afterward.Proposed fix
if (Platform.OS === 'android') { NativeVoipModule.startVoipCallService(call.callId) - .then(() => Navigation.navigate('CallView')) + .then(() => { + Navigation.navigate('CallView'); + if (useCallStore.getState().roomId == null) { + this.resolveRoomIdFromContact(call.remoteParticipants[0]?.contact).catch(error => { + log(error); + }); + } + }) .catch(error => { log(error); showErrorAlert(I18n.t('VoIP_Call_Issue'), I18n.t('Oops')); this.endCall(call.callId); }); } else { Navigation.navigate('CallView'); - } - if (useCallStore.getState().roomId == null) { - this.resolveRoomIdFromContact(call.remoteParticipants[0]?.contact).catch(error => { - log(error); - }); + if (useCallStore.getState().roomId == null) { + this.resolveRoomIdFromContact(call.remoteParticipants[0]?.contact).catch(error => { + log(error); + }); + } } }Also applies to: 152-156
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/lib/services/voip/MediaSessionInstance.ts` around lines 141 - 150, The room ID lookup (resolveRoomIdFromContact) must be deferred until the FGS/startVoipCallService promise settles to avoid stale writes if startVoipCallService rejects; modify the logic around NativeVoipModule.startVoipCallService(call.callId) and the non-Android branch so that you only call resolveRoomIdFromContact(...) and Navigation.navigate('CallView') after startVoipCallService() resolves successfully, and if it rejects call this.endCall(call.callId) and do not run the lookup or navigate; ensure you reference the existing methods startVoipCallService, resolveRoomIdFromContact, endCall and Navigation.navigate when moving the lookup to the success path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@app/lib/native/NativeVoip.ts`:
- Around line 99-116: The fallback for TurboModuleRegistry.get('VoipModule')
silently returns no-op implementations (NativeVoipModule) which makes
startVoipCallService succeed on Android when the native module is missing;
change the fallback so that missing module causes failures: when
TurboModuleRegistry.get<Spec>('VoipModule') is null/undefined, replace the no-op
object with an implementation whose startVoipCallService (and any other critical
async startup methods like startAudioRouteSync/startRingback) return a rejected
Promise (or otherwise throw) indicating the VoipModule is unavailable, and
ensure getInitialEvents/getLastVoipToken behave safely but do not hide the
missing-module error so callers of NativeVoipModule.startVoipCallService see a
rejection instead of a resolved Promise.
---
Duplicate comments:
In `@app/lib/services/voip/MediaSessionInstance.ts`:
- Around line 141-150: The room ID lookup (resolveRoomIdFromContact) must be
deferred until the FGS/startVoipCallService promise settles to avoid stale
writes if startVoipCallService rejects; modify the logic around
NativeVoipModule.startVoipCallService(call.callId) and the non-Android branch so
that you only call resolveRoomIdFromContact(...) and
Navigation.navigate('CallView') after startVoipCallService() resolves
successfully, and if it rejects call this.endCall(call.callId) and do not run
the lookup or navigate; ensure you reference the existing methods
startVoipCallService, resolveRoomIdFromContact, endCall and Navigation.navigate
when moving the lookup to the success path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6dca8a43-46d9-4224-afd1-462c23a17ff6
📒 Files selected for processing (6)
android/app/src/main/java/chat/rocket/reactnative/voip/VoipCallService.ktandroid/app/src/main/java/chat/rocket/reactnative/voip/VoipModule.ktapp/lib/native/NativeVoip.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/docs/PLATFORMS.md
✅ Files skipped from review due to trivial changes (1)
- app/lib/services/voip/docs/PLATFORMS.md
📜 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/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.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 numbersUse TypeScript with strict mode and baseUrl set to app/ for import resolution
Files:
app/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx,js,jsx}: Use Prettier with tabs, single quotes, 130 char width, no trailing commas, arrow parens avoid, bracket same line
Use@rocket.chat/eslint-configbase with React, React Native, TypeScript, Jest plugins
Files:
app/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.ts
app/lib/services/voip/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
VoIP service implementation should use Zustand stores (not Redux) and include native CallKit (iOS) and Telecom (Android) integration in app/lib/services/voip/
Files:
app/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.ts
🧠 Learnings (1)
📚 Learning: 2026-04-30T17:07:51.020Z
Learnt from: diegolmello
Repo: RocketChat/Rocket.Chat.ReactNative PR: 7274
File: app/lib/services/voip/MediaCallEvents.ts:0-0
Timestamp: 2026-04-30T17:07:51.020Z
Learning: In this Rocket.Chat React Native codebase, the ESLint rule `no-void: error` is enforced. When you see a promise returned from an async call that is not awaited (a “floating promise”), do not silence it with the `void somePromise()` pattern. Instead, handle the promise explicitly by attaching `.catch(...)` (or otherwise awaiting/handling the error) so unhandled-rejection risks are addressed in a way that satisfies the existing ESLint configuration.
Applied to files:
app/lib/services/voip/MediaSessionInstance.tsapp/lib/services/voip/MediaSessionInstance.test.tsapp/lib/native/NativeVoip.ts
diegolmello
had a problem deploying
to
upload_android
GitHub Actions
Error
|
Android Build Available Rocket.Chat 4.73.0.108969 Internal App Sharing: https://play.google.com/apps/test/RQQ8k09hlnQ/ahAO29uNR00sAFzvGdrvIvH8g5fS8MTwkffID-Frd3wN20AGZvPK8xNLx-p7mQjpXC7VT0BRke9RPZhAbimw5hrIU_ |
|
iOS Build Available Rocket.Chat 4.73.0.108970 |
OtavioStasiak
left a comment
OtavioStasiak
left a comment
There was a problem hiding this comment.
LGTM!
Tested on both platforms.
…urvives backgrounding Outgoing calls went through `MediaSessionInstance.startCall` directly to the media-signaling SDK and never touched `VoipNotification`, so no foreground service was started. With no FGS in the active set, `RECORD_AUDIO` (while-in-use) is revoked ~5s after the user backgrounds the app, dropping the caller's audio while the peer's still plays. Reported in NATIVE-1178. The incoming-accept path already starts `VoipCallService` from native after Telecom is active. This mirrors that for outgoing by exposing `startVoipCallService(callId)` on the TurboModule and invoking it Android-only from the `newCall` 'caller' branch, while the initiating activity is still visible (so `microphone` FGS-start eligibility is satisfied). FGS type widens from `phoneCall` to `microphone|phoneCall`: - `phoneCall` keeps the incoming-accept entry point startable from IncomingCallActivity / MainActivity.onNewIntent via the active self-managed Telecom call (PR #7233 rationale). - `microphone` is what actually authorises background mic capture — `phoneCall` alone is documented to keep the call alive via ConnectionService but does not exempt `RECORD_AUDIO` from the while-in-use clock. Both bits are safe to start in either path because both paths reach `startForeground()` from a while-in-use eligible state: outgoing from a visible activity, incoming after `connection.onAnswer()` puts Telecom into STATE_ACTIVE.
…outgoing call Continuing without an FGS reproduces the silent mic-drop bug this PR exists to prevent. Make startVoipCallService return Promise<void>; on rejection JS calls endCall and shows VoIP_Call_Issue so the user sees a real failure instead of a degraded call.
… nav on success
VoipCallService now reports back via VoipModule.notifyFgsStarted / notifyFgsFailed
once startForeground actually returns, so async failures
(ForegroundServiceTypeNotAllowedException, SecurityException at startForeground
time) reach JS instead of being masked by a synchronous resolve. A 7s timeout
rejects with E_VOIP_FGS_TIMEOUT if the service never signals back.
MediaSessionInstance gates Navigation.navigate('CallView') on the FGS promise
resolving on Android. On rejection it stays on the current screen and tears the
call down — previously the catch handler would alert + endCall but the eager
navigate had already happened, stranding the user on a CallView whose 'ended'
listener was detached by useCallStore.reset() before async hangup() could fire
Navigation.back().
diegolmello
force-pushed
the
native-1178-fix
branch
from
066a4b5 to
a5ffc4c
Compare
diegolmello
requested a deployment
to
approve_e2e_testing
GitHub Actions
Waiting
diegolmello
requested a deployment
to
android_build
GitHub Actions
Waiting
2 participants
Proposed changes
Outgoing VoIP calls went through
MediaSessionInstance.startCalldirectly to the media-signaling SDK and never touchedVoipNotification, so no foreground service was started. With no FGS in the active set, Android revokesRECORD_AUDIO(while-in-use) ~5 s after the user backgrounds the app, dropping the caller's audio while the peer's still plays.The incoming-accept path already starts
VoipCallServicefrom native after Telecom is active. This PR mirrors that behaviour for outgoing calls by:startVoipCallService(callId): Promise<void>on theRNVoipModuleTurboModule. The promise is held untilVoipCallService.onStartCommandreports back viaVoipModule.notifyFgsStarted/notifyFgsFailed, so async failures (ForegroundServiceTypeNotAllowedException,ForegroundServiceDidNotStartInTimeException,SecurityExceptionatstartForegroundtime) reach JS instead of being masked by a synchronousresolve(null). A 7-second timeout rejects withE_VOIP_FGS_TIMEOUTif the service never signals back. Synchronous failures fromstartForegroundServicestill reject withE_VOIP_FGS_START.newCall'caller'branch inMediaSessionInstance.ts, while the initiating activity is still visible (so themicrophoneFGS-start eligibility is satisfied).Navigation.navigate('CallView')on the FGS promise resolving. On rejection JS tears the outgoing call down viaendCall(callId)+VoIP_Call_Issuealert and stays on the current screen — previously the eager navigate stranded the user on aCallViewwhose'ended'listener was detached byuseCallStore.reset()before asynchangup()could fireNavigation.back().VoipModule.mmgets a no-opresolve(nil)stub to satisfy the codegen protocol.FGS type widens from
phoneCalltomicrophone|phoneCall:phoneCallkeeps the incoming-accept entry point startable fromIncomingCallActivity/MainActivity.onNewIntentvia the active self-managed Telecom call (PR fix(android/voip): use phoneCall FGS type and fix inverted accept/decline guard #7233 rationale).microphoneis what actually authorises background mic capture —phoneCallalone keeps the call alive viaConnectionServicebut does not exemptRECORD_AUDIOfrom the while-in-use clock.Both call sites reach
startForeground()from a while-in-use eligible state: outgoing from a visible activity, incoming afterconnection.onAnswer()puts Telecom intoSTATE_ACTIVE. Doc note atapp/lib/services/voip/docs/PLATFORMS.mdwas wrong and is corrected in the same commit.Issue(s)
https://rocketchat.atlassian.net/browse/NATIVE-1178
How to test or reproduce
Android manual repro (outgoing audio drop):
Android manual repro (FGS-rejection no-strand):
ForegroundServiceStartNotAllowedException, or temporarily revokeFOREGROUND_SERVICE_MICROPHONEin the manifest).VoIP_Call_Issuealert and stays on the originating screen — no blankCallView.Screenshots
N/A — audio-only fix.
Types of changes
Checklist
Further comments
Five regression tests in
MediaSessionInstance.test.ts: outgoing-caller starts the FGS on Android, incoming-callee does not, an FGS-start rejection tears down the call, navigation runs after FGS resolves, and navigation does NOT run on FGS rejection. ThemicrophoneFGS type is deliberately scoped to the two existing call sites (outgoing from visible activity, incoming afterconnection.onAnswer()); no new code path startsVoipCallServicefrom a non-foreground context, preserving the PR #7233 fix for theSecurityExceptioncrash class.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation