feat: hi-fi audio - #2305
Open
greenfrvr wants to merge 13 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe client adds per-call media-engine creation and disposal, join/leave supersession guards, React Native optimistic device handling, and updated audio-manager orchestration. CallingX gains explicit audio subscription lifecycle methods, while the lobby adopts a permission-aware camera preview component. ChangesMedia engine and audio lifecycle
Lobby camera preview
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Call
participant CallMediaEngineProvider
participant CallManager
participant CallingxModule
Call->>CallMediaEngineProvider: ensureMediaFactory()
CallMediaEngineProvider-->>Call: create CallFactory-backed engine
Call->>CallingxModule: wireAudioEngineSubscription()
Call->>CallManager: start with cid and audio configuration
Call->>CallingxModule: unwireAudioEngineSubscription()
Call->>CallMediaEngineProvider: dispose engine
Call->>CallManager: stop conditionally
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
greenfrvr
marked this pull request as ready for review
July 17, 2026 13:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
packages/react-native-sdk/src/utils/internal/registerMediaEngine.ts (1)
37-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate stereo-output-requested derivation across two files. Both sites independently re-derive "is stereo output requested" from the same
callManager.getStoredConfig()shape (audioRole === 'listener' && enableStereoAudioOutput === true); a future change to this rule could be applied in one place and missed in the other, causing the factory's voice-processing bypass decision and the native stereo-output enable call to disagree.
packages/react-native-sdk/src/utils/internal/registerMediaEngine.ts#L37-L44: extract this predicate into a shared helper (e.g.isStereoOutputRequested(config)exported from thecall-managermodule) and call it here.packages/react-native-sdk/src/utils/internal/registerSDKGlobals.ts#L105-L108: use the same shared helper instead of re-derivingstereoOutputinline.🤖 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 `@packages/react-native-sdk/src/utils/internal/registerMediaEngine.ts` around lines 37 - 44, The stereo-output predicate is duplicated across the media engine and SDK globals flows. Add and export a shared isStereoOutputRequested(config) helper from the call-manager module, then use it in packages/react-native-sdk/src/utils/internal/registerMediaEngine.ts:37-44 and packages/react-native-sdk/src/utils/internal/registerSDKGlobals.ts:105-108 instead of deriving audioRole and enableStereoAudioOutput inline.packages/react-native-callingx/ios/CallingxImpl.swift (1)
845-851: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUnresolved TODO on ADM reset removal during audio-session deactivation.
The prior
getAudioDeviceModule()?.reset()call was removed and replaced with a commented-out line and an open TODO ("verify if we need to reset the ADM here"). This sits in the post-interruption/call-end audio recovery path. Given the file's own note that "audio recovery is WebRTC's ... we do not touch the session here" this removal is plausible, but the TODO itself signals unresolved uncertainty that should be confirmed (or the dead commented line removed) before merge.🤖 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 `@packages/react-native-callingx/ios/CallingxImpl.swift` around lines 845 - 851, Resolve the ADM reset uncertainty in provider(_:didDeactivate:) by confirming whether subscribedADM?.reset() is required after RTCAudioSession.sharedInstance().audioSessionDidDeactivate(audioSession). If it is not required, remove the commented-out reset and TODO; otherwise, restore the reset using the existing ADM symbol and preserve the intended WebRTC audio-recovery flow.packages/react-native-callingx/src/CallingxModule.ts (1)
152-163: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMatch the class's arrow-function-field convention for consistency.
Every other method on
CallingxModuleis an arrow-function class field; these two use plain method syntax. No functional bug today since neither referencesthis, but for consistency with the rest of the class:♻️ Proposed fix
- wireAudioEngineSubscription(): void { - if (Platform.OS !== 'ios') return; - - NativeCallingModule.wireAudioEngineSubscription(); - } - - unwireAudioEngineSubscription(): void { - if (Platform.OS !== 'ios') return; - - NativeCallingModule.unwireAudioEngineSubscription(); - } + wireAudioEngineSubscription = (): void => { + if (Platform.OS !== 'ios') return; + + NativeCallingModule.wireAudioEngineSubscription(); + }; + + unwireAudioEngineSubscription = (): void => { + if (Platform.OS !== 'ios') return; + + NativeCallingModule.unwireAudioEngineSubscription(); + };🤖 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 `@packages/react-native-callingx/src/CallingxModule.ts` around lines 152 - 163, Convert CallingxModule.wireAudioEngineSubscription and CallingxModule.unwireAudioEngineSubscription from prototype methods to arrow-function class fields, preserving their iOS guards and native module calls.packages/client/src/devices/MicrophoneManager.ts (1)
193-239: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNew
enable/disable/toggleoverrides use method syntax instead of arrow-function class fields.This mirrors an existing pattern in
CameraManager, but per coding guidelines all class methods inpackages/client/src/(including overrides) should be arrow-function class fields to preserve automaticthisbinding. Overload signatures make this awkward with arrow properties (would require a type-literal call-signature declaration), which may be why method syntax was chosen — but it's worth aligning intentionally (here and inCameraManager) rather than by precedent.As per coding guidelines, "All class methods (including private/protected) must be arrow-function class fields, not method syntax, to preserve
thisbinding automatically across callbacks and observable subscriptions."🤖 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 `@packages/client/src/devices/MicrophoneManager.ts` around lines 193 - 239, Convert the MicrophoneManager enable, disable, and toggle overrides from method syntax to arrow-function class fields so they preserve automatic this binding. Retain the existing React Native pending-status behavior and super-call forwarding; represent disable’s overload contract with an appropriate callable field type, and apply the same alignment to the corresponding CameraManager overrides.Source: Coding guidelines
🤖 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 `@packages/client/src/Call.ts`:
- Around line 1225-1244: The join flow in Call.join must treat superseded joins
as failures: add a distinct superseded error and throw it from both existing
supersession bailouts instead of returning normally. Move or guard
callingX.wireAudioEngineSubscription() with the first supersession check so it
cannot reattach the audio engine after leave() has unwired or disposed it, while
preserving normal joining behavior.
In `@packages/client/src/devices/CameraManager.ts`:
- Around line 128-174: Convert enable, disable, toggle, and getStream in
packages/client/src/devices/CameraManager.ts at lines 128-174 and 253-258 to
arrow-function class fields; consolidate disable’s overloads into one arrow
function with a boolean-or-options union parameter while preserving existing
behavior.
In `@packages/client/src/devices/DeviceManager.ts`:
- Around line 512-528: Update reconcileOptimisticStatus so the target ===
'disabled' branch invokes muteStream, matching the behavior of disable(), before
setting the state to disabled. Preserve the existing abort guard and
pending-status reconciliation while ensuring the underlying media tracks are
actually muted or stopped.
In `@packages/react-native-sdk/src/utils/internal/callingx/callingx.ts`:
- Around line 215-237: Wrap the native calls in wireAudioEngineSubscription and
unwireAudioEngineSubscription with try/catch blocks, matching the error-handling
pattern used by registerOutgoingCall, joinCallingxCall, and endCallingxCall.
Preserve the existing setup checks, iOS guards, logging, and native method calls
while ensuring bridge exceptions are caught and handled consistently.
---
Nitpick comments:
In `@packages/client/src/devices/MicrophoneManager.ts`:
- Around line 193-239: Convert the MicrophoneManager enable, disable, and toggle
overrides from method syntax to arrow-function class fields so they preserve
automatic this binding. Retain the existing React Native pending-status behavior
and super-call forwarding; represent disable’s overload contract with an
appropriate callable field type, and apply the same alignment to the
corresponding CameraManager overrides.
In `@packages/react-native-callingx/ios/CallingxImpl.swift`:
- Around line 845-851: Resolve the ADM reset uncertainty in
provider(_:didDeactivate:) by confirming whether subscribedADM?.reset() is
required after
RTCAudioSession.sharedInstance().audioSessionDidDeactivate(audioSession). If it
is not required, remove the commented-out reset and TODO; otherwise, restore the
reset using the existing ADM symbol and preserve the intended WebRTC
audio-recovery flow.
In `@packages/react-native-callingx/src/CallingxModule.ts`:
- Around line 152-163: Convert CallingxModule.wireAudioEngineSubscription and
CallingxModule.unwireAudioEngineSubscription from prototype methods to
arrow-function class fields, preserving their iOS guards and native module
calls.
In `@packages/react-native-sdk/src/utils/internal/registerMediaEngine.ts`:
- Around line 37-44: The stereo-output predicate is duplicated across the media
engine and SDK globals flows. Add and export a shared
isStereoOutputRequested(config) helper from the call-manager module, then use it
in packages/react-native-sdk/src/utils/internal/registerMediaEngine.ts:37-44 and
packages/react-native-sdk/src/utils/internal/registerSDKGlobals.ts:105-108
instead of deriving audioRole and enableStereoAudioOutput inline.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3b9bbdfc-8b86-4e0f-8641-142f75362079
📒 Files selected for processing (28)
packages/client/index.tspackages/client/src/Call.tspackages/client/src/devices/AudioDeviceManager.tspackages/client/src/devices/CameraManager.tspackages/client/src/devices/DeviceManager.tspackages/client/src/devices/MicrophoneManager.tspackages/client/src/devices/ScreenShareManager.tspackages/client/src/devices/SpeakerManager.tspackages/client/src/devices/__tests__/mocks.tspackages/client/src/rtc/index.tspackages/client/src/rtc/mediaEngine.tspackages/client/src/types.tspackages/react-native-callingx/android/src/newarch/java/io/getstream/rn/callingx/CallingxModule.ktpackages/react-native-callingx/ios/Callingx.mmpackages/react-native-callingx/ios/CallingxImpl.swiftpackages/react-native-callingx/src/CallingxModule.tspackages/react-native-callingx/src/spec/NativeCallingx.tspackages/react-native-callingx/src/types.tspackages/react-native-sdk/ios/StreamInCallManager.mpackages/react-native-sdk/ios/StreamInCallManager.swiftpackages/react-native-sdk/src/components/Call/Lobby/Lobby.tsxpackages/react-native-sdk/src/components/Call/Lobby/LobbyCameraPreview.tsxpackages/react-native-sdk/src/components/Call/Lobby/index.tspackages/react-native-sdk/src/modules/call-manager/CallManager.tspackages/react-native-sdk/src/modules/call-manager/native-module.d.tspackages/react-native-sdk/src/utils/internal/callingx/callingx.tspackages/react-native-sdk/src/utils/internal/registerMediaEngine.tspackages/react-native-sdk/src/utils/internal/registerSDKGlobals.ts
Comment on lines
+1225
to
+1244
| const joinLeaveGeneration = this.leaveGeneration; | ||
| const supersededByLeave = () => | ||
| this.leaveGeneration !== joinLeaveGeneration; | ||
|
|
||
| this.joinCallData = data; | ||
|
|
||
| this.logger.debug('Starting join flow'); | ||
| this.state.setCallingState(CallingState.JOINING); | ||
|
|
||
| // Ensure the per-call media engine exists before any peer connection | ||
| // (codec probe, subscriber, publisher) or capture happens, so the WebRTC | ||
| // globals resolve to the call's factory. Idempotent across | ||
| // reconnect/migration attempts. | ||
| await this.ensureMediaFactory(); | ||
|
|
||
| const callingX = globalThis.streamRNVideoSDK?.callingX; | ||
| if (callingX) { | ||
| callingX.wireAudioEngineSubscription(); | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file sections with line numbers.
sed -n '1140,1410p' packages/client/src/Call.ts | cat -n
# Also locate the join/leave generation checks and retry loop entry points.
rg -n "joinLeaveGeneration|supersededByLeave|wireAudioEngineSubscription|unwireAudioEngineSubscription|doJoin|join\(" packages/client/src/Call.tsRepository: GetStream/stream-video-js
Length of output: 13989
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the join() wrapper and the two bailout regions in doJoin().
sed -n '1100,1405p' packages/client/src/Call.ts | cat -n
# Focus on the exact bailout blocks and any code that runs after them.
sed -n '1298,1395p' packages/client/src/Call.ts | cat -n
# Inspect leave/dispose behavior around the audio-engine unsubscribe path.
sed -n '760,870p' packages/client/src/Call.ts | cat -nRepository: GetStream/stream-video-js
Length of output: 23162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for an existing supersession/abort error or related join-flow handling.
rg -n "superseded|abort.*join|join.*abort|JOIN_SUPER|leave superseded|giveUpAndLeave|doJoin\(" packages/client/src
# Inspect any tests or call sites that rely on join() resolving/rejecting in this scenario.
rg -n "call\.join\(|\.join\(\{|\bjoin\(\)" packages/client/src packages/client/test packages/client/src/__tests__ || trueRepository: GetStream/stream-video-js
Length of output: 5013
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the tail of doJoin() and nearby guards that run after the supersession check.
sed -n '1400,1465p' packages/client/src/Call.ts | cat -n
# Inspect the join-rejection caching / retry-related logic referenced in comments.
sed -n '1745,1788p' packages/client/src/Call.ts | cat -n
# Inspect the giveUpAndLeave path inside reconnect handling.
sed -n '1848,2025p' packages/client/src/Call.ts | cat -nRepository: GetStream/stream-video-js
Length of output: 13668
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find where leaveGeneration is incremented/used and how leave synchronizes with join.
rg -n "leaveGeneration|this\.leaveGeneration|generation" packages/client/src/Call.ts
# Inspect the leave implementation around the points that advance generation and dispose the engine.
sed -n '700,860p' packages/client/src/Call.ts | cat -n
# Inspect the join-related call sites that might wait on join/leave sequencing.
sed -n '2035,2145p' packages/client/src/Call.ts | cat -nRepository: GetStream/stream-video-js
Length of output: 12263
Reject superseded joins instead of resolving them
In packages/client/src/Call.ts, both supersession bailouts currently return normally, so call.join() can resolve even though a concurrent leave() prevented JOINED from ever being set. callingX.wireAudioEngineSubscription() also runs before the first supersession check, which can reattach the audio engine after leave() has already unwired/disposed it. Surface a distinct superseded error from these exits and guard the wiring call.
🤖 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 `@packages/client/src/Call.ts` around lines 1225 - 1244, The join flow in
Call.join must treat superseded joins as failures: add a distinct superseded
error and throw it from both existing supersession bailouts instead of returning
normally. Move or guard callingX.wireAudioEngineSubscription() with the first
supersession check so it cannot reattach the audio engine after leave() has
unwired or disposed it, while preserving normal joining behavior.
Comment on lines
+512
to
+528
| protected reconcileOptimisticStatus = async (): Promise<void> => { | ||
| const target = this.state.optimisticStatus; | ||
| await withCancellation(this.statusChangeConcurrencyTag, async (signal) => { | ||
| try { | ||
| if (target === 'enabled' && this.state.status !== 'enabled') { | ||
| await this.unmuteStream(); | ||
| if (!signal.aborted) this.state.setStatus('enabled'); | ||
| } else if (target === 'disabled' && this.state.status === 'enabled') { | ||
| // mirror whatever disable() does to stop/pause the track per disableMode | ||
| if (!signal.aborted) this.state.setStatus('disabled'); | ||
| } | ||
| } finally { | ||
| if (!signal.aborted) this.state.setPendingStatus(this.state.status); | ||
| } | ||
| }); | ||
| }; | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Missing stream muting when reconciling to disabled state.
When reconciling the status from enabled to disabled, the code only updates the state but fails to actually mute or stop the underlying media tracks. This leaves the camera/microphone physically active despite the SDK reporting them as disabled.
You must invoke muteStream just like the standard disable() method does.
🐛 Proposed fix
protected reconcileOptimisticStatus = async (): Promise<void> => {
const target = this.state.optimisticStatus;
await withCancellation(this.statusChangeConcurrencyTag, async (signal) => {
try {
if (target === 'enabled' && this.state.status !== 'enabled') {
await this.unmuteStream();
if (!signal.aborted) this.state.setStatus('enabled');
} else if (target === 'disabled' && this.state.status === 'enabled') {
- // mirror whatever disable() does to stop/pause the track per disableMode
+ const stopTracks = this.state.disableMode === 'stop-tracks';
+ await this.muteStream(stopTracks);
if (!signal.aborted) this.state.setStatus('disabled');
}
} finally {
if (!signal.aborted) this.state.setPendingStatus(this.state.status);
}
});
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| protected reconcileOptimisticStatus = async (): Promise<void> => { | |
| const target = this.state.optimisticStatus; | |
| await withCancellation(this.statusChangeConcurrencyTag, async (signal) => { | |
| try { | |
| if (target === 'enabled' && this.state.status !== 'enabled') { | |
| await this.unmuteStream(); | |
| if (!signal.aborted) this.state.setStatus('enabled'); | |
| } else if (target === 'disabled' && this.state.status === 'enabled') { | |
| // mirror whatever disable() does to stop/pause the track per disableMode | |
| if (!signal.aborted) this.state.setStatus('disabled'); | |
| } | |
| } finally { | |
| if (!signal.aborted) this.state.setPendingStatus(this.state.status); | |
| } | |
| }); | |
| }; | |
| protected reconcileOptimisticStatus = async (): Promise<void> => { | |
| const target = this.state.optimisticStatus; | |
| await withCancellation(this.statusChangeConcurrencyTag, async (signal) => { | |
| try { | |
| if (target === 'enabled' && this.state.status !== 'enabled') { | |
| await this.unmuteStream(); | |
| if (!signal.aborted) this.state.setStatus('enabled'); | |
| } else if (target === 'disabled' && this.state.status === 'enabled') { | |
| const stopTracks = this.state.disableMode === 'stop-tracks'; | |
| await this.muteStream(stopTracks); | |
| if (!signal.aborted) this.state.setStatus('disabled'); | |
| } | |
| } finally { | |
| if (!signal.aborted) this.state.setPendingStatus(this.state.status); | |
| } | |
| }); | |
| }; |
🤖 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 `@packages/client/src/devices/DeviceManager.ts` around lines 512 - 528, Update
reconcileOptimisticStatus so the target === 'disabled' branch invokes
muteStream, matching the behavior of disable(), before setting the state to
disabled. Preserve the existing abort guard and pending-status reconciliation
while ensuring the underlying media tracks are actually muted or stopped.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
💡 Overview
Hi-fi audio
hifi_audio_enabledflag.Presented media engine
getGenericSdpandinitPublisherAndSubscriber(RTCPeerConnection constructor). The guard prevents creating new instance if the join flow was interrupted by leave invocation.Made RN lobby independent from webrtc
Hardened call manager and audio wiring pipeline
🎫 Ticket: https://linear.app/stream/issue/RN-402/hi-fi-audio
📑 Docs: https://github.com/GetStream/docs-content/pull/1444
Corresponding WebRTC PR: GetStream/react-native-webrtc#50
Summary by CodeRabbit
New Features
Bug Fixes