Conversation
|
📝 WalkthroughWalkthroughAdded AudioBindingsWatchdog to track/register remote audio element bindings and emit warnings for missing bindings; removed a Safari-specific audio-output detection fallback; changed DynascaleManager to disable WebAudio by default and integrate the watchdog; updated bindAudio/bindVideo logic and added tests for watchdog and binding behaviors. (37 words) Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dyn as rgba(66,133,244,0.5) DynascaleManager
participant WB as rgba(219,68,55,0.5) AudioBindingsWatchdog
participant State as rgba(15,157,88,0.5) CallState
participant Log as rgba(244,180,0,0.5) Tracer/Logger
State->>WB: callingState$ emits JOINED → start interval
Dyn->>WB: register(audioElement, sessionId, trackType)
WB->>WB: store binding in Map[sessionId/trackType] = element
WB-->>State: periodic scan (every 3s) reads participants & streams
alt remote participant has stream but no binding
WB->>Log: emit tracer + logger.warn with affected userIds
end
Dyn->>WB: unregister(sessionId, trackType)
State->>WB: callingState$ leaves JOINED → stop interval
WB->>WB: dispose() stops interval and unsubscribes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…lements Extracts audio binding tracking from DynascaleManager into a dedicated AudioBindingsWatchdog class that periodically warns about remote participants with audio/screenShareAudio streams but no bound element. Skipped in React Native where audio elements don't apply.
oliverlaz
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/client/src/helpers/AudioBindingsWatchdog.ts`:
- Around line 91-107: The warning currently collects userId for dangling audio
bindings but should report the actual binding identity used by
AudioBindingsWatchdog; update the loop that builds danglingUserIds to collect
the binding key(s) produced by toBindingKey(sessionId) and
toBindingKey(sessionId, 'screenShareAudioTrack') (or collect an object/strings
containing sessionId and trackType) instead of userId, and then change the
this.logger.warn call to include those binding keys (or sessionId+trackType) so
the log shows the exact binding identity that is unbound.
- Around line 44-64: unregister currently deletes bindings by
sessionId/trackType only, which causes bind(el1)->bind(el2)->unregister(el1) to
remove el2 and leave a dangling state; make unregister element-aware by
accepting the audioElement (signature: unregister(sessionId: string, trackType:
AudioTrackType, audioElement?: HTMLAudioElement)) and only remove the map entry
if the stored element equals the passed audioElement (or if no element passed,
fallback to deleting like today), or alternatively change register to store a
Set per key and remove only the given element from that Set; update the logic in
AudioBindingsWatchdog.register/unregister and add a regression test covering
bind(el1)->bind(el2)->unregister(el1) to ensure the active binding remains.
In `@packages/client/src/helpers/DynascaleManager.ts`:
- Line 76: Add a regression test to assert the new default-off behavior for web
audio: when a DynascaleManager instance is created without calling
setUseWebAudio(), its playback should remain in the direct element path (i.e.,
useWebAudio stays false) and Safari-specific behavior should continue to use
element playback; add a unit/test case that constructs DynascaleManager, does
not call setUseWebAudio(), simulates or stubs the Safari environment if needed,
and verifies the manager's behavior/state (inspect the private/use-exposed flag
or resulting playback method) to prevent regressions of the previous default-on
behavior.
🪄 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: 3aa15114-84fa-4aba-a211-c6f199b13376
📒 Files selected for processing (4)
packages/client/src/helpers/AudioBindingsWatchdog.tspackages/client/src/helpers/DynascaleManager.tspackages/client/src/helpers/__tests__/AudioBindingsWatchdog.test.tspackages/client/src/helpers/__tests__/DynascaleManager.test.ts
| register = ( | ||
| audioElement: HTMLAudioElement, | ||
| sessionId: string, | ||
| trackType: AudioTrackType, | ||
| ) => { | ||
| const key = toBindingKey(sessionId, trackType); | ||
| const existing = this.bindings.get(key); | ||
| if (existing && existing !== audioElement) { | ||
| this.logger.warn( | ||
| `Audio element already bound to ${sessionId} and ${trackType}`, | ||
| ); | ||
| this.tracer.trace('audioBinding.alreadyBoundWarning', trackType); | ||
| } | ||
| this.bindings.set(key, audioElement); | ||
| }; | ||
|
|
||
| /** | ||
| * Removes the audio element binding for the given session and track type. | ||
| */ | ||
| unregister = (sessionId: string, trackType: AudioTrackType) => { | ||
| this.bindings.delete(toBindingKey(sessionId, trackType)); |
There was a problem hiding this comment.
Make unregister() element-aware.
register() overwrites the tracked element for a key, but unregister() deletes by sessionId/trackType only. In a bind(el1) -> bind(el2) -> cleanup(el1) sequence, the old cleanup removes the active el2 entry and the watchdog starts reporting a dangling binding even though audio is still bound. Please either track a Set per key or pass the element into unregister(), and add a regression test for that cleanup order.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/helpers/AudioBindingsWatchdog.ts` around lines 44 - 64,
unregister currently deletes bindings by sessionId/trackType only, which causes
bind(el1)->bind(el2)->unregister(el1) to remove el2 and leave a dangling state;
make unregister element-aware by accepting the audioElement (signature:
unregister(sessionId: string, trackType: AudioTrackType, audioElement?:
HTMLAudioElement)) and only remove the map entry if the stored element equals
the passed audioElement (or if no element passed, fallback to deleting like
today), or alternatively change register to store a Set per key and remove only
the given element from that Set; update the logic in
AudioBindingsWatchdog.register/unregister and add a regression test covering
bind(el1)->bind(el2)->unregister(el1) to ensure the active binding remains.
| const danglingUserIds: string[] = []; | ||
| for (const p of this.state.participants) { | ||
| if (p.isLocalParticipant) continue; | ||
| const { audioStream, screenShareAudioStream, sessionId, userId } = p; | ||
| if (audioStream && !this.bindings.has(toBindingKey(sessionId))) { | ||
| danglingUserIds.push(userId); | ||
| } | ||
| if ( | ||
| screenShareAudioStream && | ||
| !this.bindings.has(toBindingKey(sessionId, 'screenShareAudioTrack')) | ||
| ) { | ||
| danglingUserIds.push(userId); | ||
| } | ||
| } | ||
| if (danglingUserIds.length > 0) { | ||
| this.logger.warn( | ||
| `Dangling audio bindings detected. Did you forget to bind the audio element? user_ids: ${danglingUserIds}.`, |
There was a problem hiding this comment.
Log the binding identity, not userId.
The watchdog tracks bindings by sessionId/trackType, but the warning only emits userId. That gets ambiguous as soon as one user has multiple sessions, and it doesn't tell you which track type is actually unbound. Logging the binding key (or at least sessionId) will make this warning actionable.
As per coding guidelines, "Participants should use sessionId as the unique identifier, not userId".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/helpers/AudioBindingsWatchdog.ts` around lines 91 - 107,
The warning currently collects userId for dangling audio bindings but should
report the actual binding identity used by AudioBindingsWatchdog; update the
loop that builds danglingUserIds to collect the binding key(s) produced by
toBindingKey(sessionId) and toBindingKey(sessionId, 'screenShareAudioTrack') (or
collect an object/strings containing sessionId and trackType) instead of userId,
and then change the this.logger.warn call to include those binding keys (or
sessionId+trackType) so the log shows the exact binding identity that is
unbound.
| private speaker: SpeakerManager; | ||
| private tracer: Tracer; | ||
| private useWebAudio = isSafari(); | ||
| private useWebAudio = false; |
There was a problem hiding this comment.
Add a regression test for the new default-off path.
This line is the bug fix, but the provided Safari test only covers the opt-in case via setUseWebAudio(true). Please add a regression that verifies Safari stays on direct element playback when setUseWebAudio() is never called, otherwise the old default can creep back unnoticed.
As per coding guidelines, "Add regression tests for bug fixes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/client/src/helpers/DynascaleManager.ts` at line 76, Add a regression
test to assert the new default-off behavior for web audio: when a
DynascaleManager instance is created without calling setUseWebAudio(), its
playback should remain in the direct element path (i.e., useWebAudio stays
false) and Safari-specific behavior should continue to use element playback; add
a unit/test case that constructs DynascaleManager, does not call
setUseWebAudio(), simulates or stubs the Safari environment if needed, and
verifies the manager's behavior/state (inspect the private/use-exposed flag or
resulting playback method) to prevent regressions of the previous default-on
behavior.
2 participants
💡 Overview
AudioBindingsWatchdog— a helper that periodically checks for remote participants with active audio/screenShareAudio streams but no bound<audio>element, logging a warning to help integrators catch missing audio bindings early.📝 Implementation notes
DynascaleManagerinto a standaloneAudioBindingsWatchdogclass withregister/unregister/setEnabled/disposeAPI.callingStatebecomesJOINEDand stops on leave.audioStreamandscreenShareAudioStream.DynascaleManager.audioBindingsWatchdogis public and nullable for direct access.🎫 Ticket: https://linear.app/stream/issue/REACT-933/add-visibility-into-audio-element-registration