feat: hi-fi audio - #2305
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:
📝 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. 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 💡 1📝 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
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 (3)
packages/react-native-callingx/ios/CallingxImpl.swift (1)
417-422: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset the engine-startup flag when unwiring.
If unwiring occurs after
.willEnableAudioEngineand before.willStartAudioEngine,isAudioEngineStartingremainstrue. The next call can then suppress a genuine system mute action. Reset this flag onpendingActionsQueueinunwireAudioEngineSubscription().Proposed fix
engineSubscription?.cancel() engineSubscription = nil subscribedADM = nil +pendingActionsQueue.sync { + isAudioEngineStarting = false +} CallingxLog.core.debugPublic("[unwireEngineSubscription]")🤖 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 417 - 422, Update unwireAudioEngineSubscription() to reset isAudioEngineStarting on pendingActionsQueue after cancelling the engine subscription, ensuring the startup state is cleared when unwiring occurs before engine start.packages/react-native-sdk/ios/StreamInCallManager.swift (2)
426-443: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPersist the forced speaker route.
setForceSpeakerphoneOndoes not update state used byapplyConfigForEngineEnable(). After an engine rebuild, that method reappliesselectedOutputand can undo the forced speaker route. Store the force state separately, or update the route state only after the native route call succeeds.🤖 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/ios/StreamInCallManager.swift` around lines 426 - 443, The setForceSpeakerphoneOn method does not persist the requested speakerphone state across engine rebuilds. Add or update dedicated force-speaker state used by applyConfigForEngineEnable(), committing it only after the native routing operation succeeds, and ensure disabling clears that state so rebuilds restore the normal selectedOutput route.
219-225: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winCheck for
AudioDeviceModule?before invoking members.
getAudioDeviceModule()returns an optional, butadmin,adm.setEngineAvailability(...), andadm?.publishercompile and run even fornil. Guard and unwrap before setup/subscription, then callsetEngineAvailability()and bindpublisher.Proposed fix
- let adm = getAudioDeviceModule() + guard let adm = getAudioDeviceModule() else { return }🤖 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/ios/StreamInCallManager.swift` around lines 219 - 225, Update the no-CallKit audio setup around getAudioDeviceModule() to guard and unwrap the optional AudioDeviceModule before accessing it. Perform setup, setEngineAvailability(.default), and publisher binding only within the successful unwrap path, while preserving the existing behavior when the module is unavailable.
🤖 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 `@packages/react-native-callingx/ios/CallingxImpl.swift`:
- Around line 417-422: Update unwireAudioEngineSubscription() to reset
isAudioEngineStarting on pendingActionsQueue after cancelling the engine
subscription, ensuring the startup state is cleared when unwiring occurs before
engine start.
In `@packages/react-native-sdk/ios/StreamInCallManager.swift`:
- Around line 426-443: The setForceSpeakerphoneOn method does not persist the
requested speakerphone state across engine rebuilds. Add or update dedicated
force-speaker state used by applyConfigForEngineEnable(), committing it only
after the native routing operation succeeds, and ensure disabling clears that
state so rebuilds restore the normal selectedOutput route.
- Around line 219-225: Update the no-CallKit audio setup around
getAudioDeviceModule() to guard and unwrap the optional AudioDeviceModule before
accessing it. Perform setup, setEngineAvailability(.default), and publisher
binding only within the successful unwrap path, while preserving the existing
behavior when the module is unavailable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b0cf985e-a330-405b-a02b-7c6cfb54413e
📒 Files selected for processing (10)
packages/client/src/Call.tspackages/react-native-callingx/ios/CallingxImpl.swiftpackages/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/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/registerSDKGlobals.ts
💤 Files with no reviewable changes (2)
- packages/react-native-callingx/src/spec/NativeCallingx.ts
- packages/react-native-sdk/src/utils/internal/callingx/callingx.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-native-sdk/src/modules/call-manager/native-module.d.ts
- packages/react-native-callingx/src/types.ts
- packages/react-native-sdk/ios/StreamInCallManager.m
- packages/react-native-sdk/src/modules/call-manager/CallManager.ts
- packages/client/src/Call.ts
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)
packages/react-native-callingx/ios/CallingxImpl.swift (1)
430-436: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCall
unwireAudioEngineSubscription()from leave/reload teardown.
wireAudioEngineSubscription()subscribes to the current ADM, andunwireAudioEngineSubscription()cancels/releases that subscription. There is currently no call path that invokes it when a call/ADM is disposed, so the old subscription can retain the previous ADM and receive later engine events. Wire only when the ADM is ready, and untie the same lifecycle point: leave and JS reload.🤖 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 430 - 436, Invoke unwireAudioEngineSubscription() during both call/ADM leave teardown and JavaScript reload teardown, ensuring it runs before the previous ADM is disposed or replaced. Keep wireAudioEngineSubscription() gated on ADM readiness and preserve the existing subscription cleanup behavior.
🤖 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/react-native-callingx/ios/CallingxImpl.swift`:
- Around line 991-993: Make updates in setDesiredEngineAvailability atomic with
the reads performed by wireAudioEngineSubscription: serialize the
desiredEngineAvailability assignment, audio-device-module lookup, and
setEngineAvailability replay on the same serial queue or protect both paths with
the same lock, ensuring no stale availability can be applied.
---
Outside diff comments:
In `@packages/react-native-callingx/ios/CallingxImpl.swift`:
- Around line 430-436: Invoke unwireAudioEngineSubscription() during both
call/ADM leave teardown and JavaScript reload teardown, ensuring it runs before
the previous ADM is disposed or replaced. Keep wireAudioEngineSubscription()
gated on ADM readiness and preserve the existing subscription cleanup behavior.
🪄 Autofix
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 Plus
Run ID: d79e75ab-708d-413e-a59e-15b01b40b534
📒 Files selected for processing (5)
packages/client/src/types.tspackages/react-native-callingx/ios/CallingxImpl.swiftpackages/react-native-callingx/src/types.tspackages/react-native-sdk/ios/StreamInCallManager.swiftpackages/react-native-sdk/src/modules/call-manager/CallManager.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/react-native-callingx/src/types.ts
- packages/react-native-sdk/ios/StreamInCallManager.swift
- packages/client/src/types.ts
- packages/react-native-sdk/src/modules/call-manager/CallManager.ts
This reverts commit c740cd9.
Bundle sizeBuilt package output. Sizes in KB; delta vs
|
|
🎉 The changes from this pull request have been released. Shipped with:
|
Re-plumbs the Android communication-mode keep-alive opt-out onto the call-manager architecture introduced by hi-fi audio (#2305). #2305 turned the public `callManager.start(config)` into a config store: it no longer drives the native module, and the SDK's internal call manager applies the stored config at join time. The per-call `disableCommunicationModeWorkaround` forwarding lived in the deleted `start()` branch, so it moves to `registerSDKGlobals.ts` alongside `setAudioRole`, before the native `start()` (native rejects the change once the audio manager is activated). Telecom-managed calls are still excluded, so a per-call config cannot clobber the sticky preference on a call where the keep-alive never runs. Also fixes a latent test-harness bug: `registerSDKGlobals()` no-ops once `globalThis.streamRNVideoSDK` is set, and that global outlives `jest.resetModules()`, so every test after the first bound the internal call manager to the first test's mocked native module. The Kotlin keep-alive is unaffected - #2305 did not touch android/.
1 participant
💡 Overview
Hi-fi audio
callManager.start({ audioRole: 'listener' })enables stereo output for both platforms. Should be invoked on pre-join stage.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