Skip to content

fix(client): don't start audio playback after the element is unbound - #2477

Open
RaphaelFakhri wants to merge 1 commit into
GetStream:mainfrom
RaphaelFakhri:fix/bind-audio-cancel-deferred-attach
Open

RaphaelFakhri wants to merge 1 commit into
GetStream:mainfrom
RaphaelFakhri:fix/bind-audio-cancel-deferred-attach

Conversation

@RaphaelFakhri

@RaphaelFakhri RaphaelFakhri commented Sep 29, 2026 •

Copy link
Copy Markdown

💡 Overview

DynascaleManager.bindAudioElement() can start audio playback on an element after the function that unbinds it has run. This happens when the element is unbound in the same tick as the stream update, for example when a component mounts and unmounts right away or when a participant leaves just after the bind.

📝 Implementation notes

The stream subscription defers the attach with setTimeout. The callback assigns srcObject, calls play(), creates a MediaPlaybackWatchdog, and connects a Web Audio source node when Web Audio is in use. The function returned by bindAudioElement() unsubscribes and clears srcObject, but it doesn't cancel the pending timer. When the timer fires afterwards, the unbound element plays audio, the watchdog is never disposed, and the source node stays connected to the audio context.

The unbind function now sets a flag and the deferred callback returns early when the flag is set.

A regression test in DynascaleManager.test.ts updates the participant's audio stream, unbinds the element, runs the timers, and checks that srcObject stays null and play() isn't called. The test fails on the previous code and passes with this change.

🎫 Ticket: n/a

📑 Docs: n/a

Summary by CodeRabbit

  • Bug Fixes

    • Prevented delayed audio updates from attaching a stream or starting playback after the audio element has been unbound.
  • Tests

    • Added coverage for audio elements unbound before delayed updates run.

@coderabbitai

coderabbitai Bot commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b10bbe53-6e9c-4bc0-8cb5-769f292a22d8

📥 Commits

Reviewing files that changed from the base of the PR and between 1f7a160 and 074b84c.

📒 Files selected for processing (2)
  • packages/client/src/helpers/DynascaleManager.ts
  • packages/client/src/helpers/__tests__/DynascaleManager.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The audio binding now skips delayed stream updates after cleanup. A regression test checks that cleanup before deferred work prevents source attachment and playback.

Changes

Audio binding cleanup

Layer / File(s) Summary
Guard delayed stream updates
packages/client/src/helpers/DynascaleManager.ts, packages/client/src/helpers/__tests__/DynascaleManager.test.ts
Cleanup marks the binding as unbound. Delayed callbacks exit before assigning the stream or configuring playback. The regression test checks that deferred work does not attach a source or call play().

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: oliverlaz

Merge Risk: ⚪ Minimal · up to 074b8

Deferred audio updates are guarded after unbinding, and the regression test covers that timing. No concrete merge-blocking risk remains.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 074b8

The change prevents delayed playback after unbinding without adding a new way to reach the audio-binding API. Some browser-dependent completion paths remain outside this fix.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The affected exposure is playback and resource state of a client audio element bound to a participant; the unchanged Call wrapper provides no new binding route.

Trust Boundaries and Controls

  • inferred — The new check narrows post-unbind media attachment without changing participant selection or the Call-to-manager boundary.

Resilience and Maintainability Implications

  • inferred — A play() rejection arriving after cleanup can still repopulate the element-wide blocked-audio tracker. This is an unchanged lifecycle limitation, not an introduced attack path.

Hardening Proposals

  • proposed — Consider checking binding liveness in the play() rejection continuation so it cannot restore blocked-audio state after unbinding.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: preventing audio playback after the audio element is unbound.
Description check ✅ Passed The description includes the required Overview and Implementation notes sections. It explains the deferred callback issue, the implementation, and the regression test. The ticket and documentation fie…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant