fix(calls): wire voice processing presets to the published mic track#2409
fix(calls): wire voice processing presets to the published mic track#2409rozagerardo wants to merge 4 commits into
Conversation
`voiceProcessingPreset` (standard/voice_isolation/music) was fully built in the UI and persisted to localStorage but never applied to the actual LiveKit audio track - constraintsForVoicePreset() had no caller besides its own test. Wires it at join and on mid-call switch (via applyConstraints on the live track), fixes the standard/voice_isolation constraint table so they differ on both axes, removes the isScreensharing AGC override that made the existing screenshare-auto-isolation behavior a no-op, resets a persisted `music` choice to `standard` on join, and greys out any preset menu option whose constraint isn't supported on the current device/browser. GH-2383 item (c).
|
🔗 Custom preview URL: https://pr-2409.preview-app.hypha.earth |
…ge load Live-testing on the PR #2409 preview build found that switching to `music`, leaving, and rejoining without a page reload still showed `music` as active — the mount-time readVoiceProcessingPreset() effect that resolves a stale `music` value only runs once per page load, so it never caught an in-session rejoin. Resolve music -> standard at the actual join point instead, in addition to the existing mount-time check.
WalkthroughVoice-processing presets now configure LiveKit microphone tracks, detect unsupported browser capabilities, normalize persisted presets, and propagate capability state to disable incompatible in-call options with localized explanations. ChangesVoice processing capability handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant useSpaceGroupCall
participant LiveKitMicrophoneTrack
participant HumanChatPanelInCallControls
Caller->>useSpaceGroupCall: Join call with selected voice preset
useSpaceGroupCall->>LiveKitMicrophoneTrack: Publish microphone with preset constraints
LiveKitMicrophoneTrack-->>useSpaceGroupCall: Report constraint capabilities
useSpaceGroupCall->>HumanChatPanelInCallControls: Pass unsupported constraint state
HumanChatPanelInCallControls-->>Caller: Disable unsupported presets and show hints
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/matrix/client/hooks/use-space-group-call.ts (2)
2070-2079: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the active preset constraints when unmuting the microphone.
When the microphone is unmuted (
!muted === true), LiveKit may occasionally need to acquire a new media track from the browser (e.g., if the previous track was released by the OS or stopped). BecausesetMicrophoneEnabledis called here without options, a newly acquired track would revert to the original connection constraints instead of the currently active voice processing preset.Pass the current preset constraints to ensure they are preserved across track restarts.
💡 Proposed fix
const setMicrophoneMuted = useCallback( async (muted: boolean) => { const lkRoom = liveKitRoomRef.current; if (!lkRoom) return; - await lkRoom.localParticipant.setMicrophoneEnabled(!muted); + await lkRoom.localParticipant.setMicrophoneEnabled( + !muted, + constraintsForVoicePreset(voiceProcessingPresetRef.current) + ); syncLocalMuteState(lkRoom); scheduleFeedBatched(); }, [scheduleFeedBatched, syncLocalMuteState], );🤖 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/core/src/matrix/client/hooks/use-space-group-call.ts` around lines 2070 - 2079, Update setMicrophoneMuted to pass the currently active voice-processing preset constraints as options when calling localParticipant.setMicrophoneEnabled during unmute, while preserving the existing behavior for mute and subsequent state synchronization.
1336-1339: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winApply constraints to the microphone track when the preset is changed by the screenshare flow.
Both sites update the internal preset state during the screenshare lifecycle, but miss applying the newly mapped constraints to the active hardware track. Without this step, the microphone continues using the old constraints, defeating the purpose of the automatic voice boost.
packages/core/src/matrix/client/hooks/use-space-group-call.ts#L1336-L1339: callapplyConstraintson the mic track withplan.effectivePresetto enforce the boost.packages/core/src/matrix/client/hooks/use-space-group-call.ts#L1413-L1417: callapplyConstraintson the mic track with therestorepreset to revert the boost.🤖 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/core/src/matrix/client/hooks/use-space-group-call.ts` around lines 1336 - 1339, Update both screenshare preset transition sites in use-space-group-call.ts: at lines 1336-1339, apply constraints to the active microphone track using plan.effectivePreset after updating the preset state; at lines 1413-1417, apply constraints using the restore preset when reverting. Use the existing microphone track and applyConstraints flow.
🤖 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/core/src/matrix/client/hooks/use-space-group-call.ts`:
- Around line 2287-2296: Update the preset handling near the micTrack constraint
application to also set the LiveKit room’s audioCaptureDefaults using
constraintsForVoicePreset(preset), so newly created microphone tracks preserve
the selected preset after device changes; keep the existing active-track
applyConstraints behavior unchanged.
---
Outside diff comments:
In `@packages/core/src/matrix/client/hooks/use-space-group-call.ts`:
- Around line 2070-2079: Update setMicrophoneMuted to pass the currently active
voice-processing preset constraints as options when calling
localParticipant.setMicrophoneEnabled during unmute, while preserving the
existing behavior for mute and subsequent state synchronization.
- Around line 1336-1339: Update both screenshare preset transition sites in
use-space-group-call.ts: at lines 1336-1339, apply constraints to the active
microphone track using plan.effectivePreset after updating the preset state; at
lines 1413-1417, apply constraints using the restore preset when reverting. Use
the existing microphone track and applyConstraints flow.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fe12498d-e7c4-440e-acce-315607fca8a5
📒 Files selected for processing (13)
packages/core/src/matrix/client/hooks/__tests__/screenshare-voice-boost.test.tspackages/core/src/matrix/client/hooks/use-space-group-call.tspackages/core/src/matrix/client/hooks/voice-processing-constraints.tspackages/epics/src/common/global-call-dock-overlay.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-call-banner.tsxpackages/epics/src/common/human-chat-panel/human-chat-panel-in-call-controls.tsxpackages/epics/src/common/human-right-panel.tsxpackages/i18n/src/messages/de.jsonpackages/i18n/src/messages/en.jsonpackages/i18n/src/messages/es.jsonpackages/i18n/src/messages/fr.jsonpackages/i18n/src/messages/mk.jsonpackages/i18n/src/messages/pt.json
Addresses CodeRabbit review on PR #2409: - The screenshare auto-switch (WCUX-SHARE-VOICE-1's applyPresenterVoiceBoostForScreenshare) and its restore counterpart updated the preset state but never called applyConstraints() on the live mic track, so the auto-switch changed the UI checkmark without changing any actual audio behavior — the same bug this PR otherwise fixes for the manual preset switch, just missed for the automatic path (major finding). - setMicrophoneMuted's setMicrophoneEnabled(!muted) call on unmute passed no options, so a track re-acquired after being released by the OS/browser would fall back to the room's original constraints instead of the active preset (minor finding). - Neither path updated the room's audioCaptureDefaults, so a mid-call microphone device switch (which makes LiveKit create a brand new track) would also silently drop back to the room's original constraints (minor finding, inline comment). Factored the shared apply-to-room-and-track logic into applyVoiceProcessingPresetToRoom() and used it from setVoiceProcessingPreset, the screenshare auto-switch, and its restore.
|
Addressed all 3 CodeRabbit findings in b99393a:
Factored the shared logic into |
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)
packages/core/src/matrix/client/hooks/use-space-group-call.ts (1)
1843-1847: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAvoid falsely disabling presets when
getCapabilitiesis unsupported.If
getCapabilitiesis not implemented by the browser (e.g., Firefox before version 116),capabilitieswill beundefined. Due to optional chaining,capabilities?.autoGainControl === undefinedevaluates totrue, which incorrectly marks the capability as unsupported and disables the UI control.To prevent locking users out of features their browser might actually support, assume the capabilities are supported (i.e., not unsupported) when
getCapabilitiesis missing.🛠️ Proposed fix
setUnsupportedVoiceProcessingConstraints({ - autoGainControl: capabilities?.autoGainControl === undefined, - echoCancellation: capabilities?.echoCancellation === undefined, - noiseSuppression: capabilities?.noiseSuppression === undefined, + autoGainControl: capabilities ? capabilities.autoGainControl === undefined : false, + echoCancellation: capabilities ? capabilities.echoCancellation === undefined : false, + noiseSuppression: capabilities ? capabilities.noiseSuppression === undefined : 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 `@packages/core/src/matrix/client/hooks/use-space-group-call.ts` around lines 1843 - 1847, Update the setUnsupportedVoiceProcessingConstraints call so each capability is marked unsupported only when capabilities exists and its corresponding property is undefined; when capabilities is undefined because getCapabilities is unavailable, treat all capabilities as supported.
🤖 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/core/src/matrix/client/hooks/use-space-group-call.ts`:
- Around line 1843-1847: Update the setUnsupportedVoiceProcessingConstraints
call so each capability is marked unsupported only when capabilities exists and
its corresponding property is undefined; when capabilities is undefined because
getCapabilities is unavailable, treat all capabilities as supported.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b46b5a5d-30c8-41ee-b640-1446c8bc56cc
📒 Files selected for processing (1)
packages/core/src/matrix/client/hooks/use-space-group-call.ts
1 participant
Summary
voiceProcessingPreset(standard/voice_isolation/music) was a fully working UI (radio menu, checkmarks, persisted to localStorage) but was never actually applied to the published LiveKit audio track —constraintsForVoicePreset()had no caller in the repo besides its own unit test.setMicrophoneEnabled(true, constraintsForVoicePreset(...))) and on a mid-call switch (applyConstraints()on the live published track), so a change takes effect without leaving/rejoining or muting/unmuting.standardnow{autoGainControl: true, echoCancellation: true, noiseSuppression: false}(previouslynoiseSuppression: true, which made it differ fromvoice_isolationon only one axis).isScreensharingAGC override, which forcedautoGainControl: trueregardless of preset — this made the existing screenshare-auto-switch-to-voice_isolationbehavior (WCUX-SHARE-VOICE-1) a complete no-op. It now has a real, audible effect for the first time.musicpreset resets tostandardwhen read back at join —musicis situational to one call's content and shouldn't silently carry into an unrelated future call.getCapabilities()check on the mic track at join; if a device/browser can't actually adjustnoiseSuppressionorautoGainControl, the corresponding preset button (voice_isolation/music) is disabled with an inline note instead of silently no-opping.GH-2383 item (c). Items (b), (d), (e) are separate, not touched here.
Test plan
vitest run screenshare-voice-boost— updated constraint-table + capability assertions passtsc --noEmitonpackages/coreandpackages/epics— no new errors (confirmed against baseline viagit stashdiff)track.getSettings()in devtools that the requested constraints actually landedstandarddisables AGC without introducing distracting volume/level issues (previously masked by the removed no-op override)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests