Skip to content

fix(web): room stream propagation and track-switch sync#72

Merged
knzeng-e merged 2 commits into
mainfrom
fix/room-stream-sync
Jul 7, 2026
Merged

fix(web): room stream propagation and track-switch sync#72
knzeng-e merged 2 commits into
mainfrom
fix/room-stream-sync

Conversation

@knzeng-e

@knzeng-e knzeng-e commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Fixes three reported real-time defects in host-to-listener room streaming. All live in the WebRTC/audio path; none change the room access doctrine (host satisfies the policy, listeners receive only the ephemeral stream). Rationale + manual QA in docs/design/room-stream-sync-fixes.md.

Symptom A - listeners joining by link/QR often hear nothing

captureAudioStream threw when the captured MediaStream had zero audio tracks. Capture runs at loadedmetadata, before the element is actually playing, so a transient zero-track capture aborted the whole host stream and no offer was ever sent. Fix: capture no longer throws on zero tracks; the caller checks for a live track and retries on play (PersistentAudio now also prepares the stream in onPlay). The genuine autoplay-policy case (no gesture) already surfaces a "Start audio" control and is left as-is - that is honest, expected mobile behavior.

Symptom B - host changes track or loops, listeners do not hear it

  • Track change / next: a new source fires loadedmetadata -> prepareLocalStream re-captures and renegotiates. The old throw-on-zero-tracks could make this silently fail; fixed as in A.
  • Loop / replay: repeat replayed on the ended event, but when an element fires ended its captureStream() audio track ends too, silencing the room on loop. Fix: repeat now uses the element's native loop, which seeks back without firing ended, so the captured track stays live across the loop.

Symptom C - clicking next keeps playing the old track (visible delay)

selectTrack updates title/cover synchronously but only sets the new audioSource after an async access check + decrypt/fetch, so the old source played through the whole gap while the UI already showed the new track. Fix: selectTrack pauses the current element immediately; the new source autoplays on load.

Efficiency: no peer churn on play/pause/seek

prepareLocalStream now records which audioSource the live stream was captured from (capturedSourceRef) and skips re-capture/renegotiation when the source is unchanged and the stream still has a live track. Only a real source change re-offers listeners - removing the teardown/rebuild a naive re-capture on every event would cause.

Verification

  • lint 0 errors, test:unit 74/74, test:signal 30/30, build clean, fmt:check clean. ASCII-only.
  • Not done - browser QA required. These are real-time timing behaviors that unit tests cannot cover. The manual QA checklist (two devices, host + listener: link/QR join, next, loop, play/pause churn) is in the design doc and should be run before merge. The room-join Playwright e2e path is preserved (synthetic-capture branch and the source-change renegotiation it relies on are intact) but I could not run Playwright in this environment.

Pre-existing unrelated contracts/ working-tree changes are excluded.

Three real-time defects in host-to-listener room streaming:

- Listeners joining by link/QR often heard nothing: captureAudioStream threw
  when the captured MediaStream had zero audio tracks, which happens when
  capture runs at loadedmetadata before playback starts. That aborted the host
  stream and no offer was sent. Capture no longer throws on zero tracks; the
  caller checks for a live track and retries on play (PersistentAudio now also
  prepares the stream in onPlay).
- Loop/replay was not propagated: repeat replayed on the `ended` event, but an
  element firing `ended` also ends its captureStream() audio track, silencing
  the room on loop. Repeat now uses the element's native `loop`, which never
  fires `ended`, so the captured track stays live across the loop.
- Clicking next kept playing the old track during a visible delay: selectTrack
  resolves the new source asynchronously (access check + decrypt) and only then
  swaps audioSource, so the old source played through the whole gap. selectTrack
  now pauses the current element immediately; the new source autoplays on load.

Also stops peer churn: prepareLocalStream records the captured source and skips
re-capture/renegotiation on play/pause/seek, only re-offering listeners on a
real source change. Access doctrine unchanged (host satisfies policy, listeners
get only the stream). Rationale and manual QA in
docs/design/room-stream-sync-fixes.md.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@netlify

netlify Bot commented Jul 7, 2026

Copy link
Copy Markdown

Deploy Preview for muzinga ready!

Name Link
🔨 Latest commit c94f2e2
🔍 Latest deploy log https://app.netlify.com/projects/muzinga/deploys/6a4c86180e60a50008cc551c
😎 Deploy Preview https://deploy-preview-72--muzinga.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
🤖 Make changes Run an agent on this branch

To edit notification comments on pull requests, go to your Netlify project configuration.

@knzeng-e

knzeng-e commented Jul 7, 2026

Copy link
Copy Markdown
Owner Author

@claude please review this PR

@knzeng-e knzeng-e self-assigned this Jul 7, 2026
@claude

claude Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @knzeng-e's task in 5m 33s —— View job


Claude's review

  • Read trigger comment and gather PR context
  • Read design doc docs/design/room-stream-sync-fixes.md
  • Review diff in PersistentAudio.tsx, useCatalog.ts, usePlayback.ts, useSession.ts
  • Check for correctness/security/perf issues per CLAUDE.md engineering posture
  • Post final review

Traced all three symptoms end-to-end against the code (not just the design doc). Symptoms A and B check out; the WebRTC/room-access boundary is untouched as claimed. One regression found in the Symptom C fix.

Bug: reselecting the currently-playing track pauses it with no auto-resume

web/src/hooks/useCatalog.ts:496-503:

const outgoingAudio = localAudioRef.current;
if (outgoingAudio && !outgoingAudio.paused) {
  outgoingAudio.pause();
}

This runs unconditionally, before the async access-check/decrypt that resolves the new audioUrl. If a user clicks the track that is already playing — via ListenView.tsx:229, StageRail.tsx:120, or ArtistProfileView.tsx:185, none of which guard on track.id === selectedTrackId — the resolved audioUrl ends up identical to the current source (static track.localUrl, or a cached entry in resolvedAudioSourcesRef/preview-URL cache). setAudioSource(audioUrl) with an unchanged string is a React no-op, so <audio src> never changes, loadedmetadata never refires, and usePlayback's autoplay effect (usePlayback.ts:110-118, keyed on audioSource changing) never re-arms. The track is left paused with no automatic resume — previously (pre-PR) this same click was a harmless no-op since nothing paused the element.

This isn't a browser-timing case, so it's unit-testable, unlike the QA items in the design doc. Suggest guarding the pause (and/or forcing a reload) when the track being selected is already the active one, e.g. skip when track.id === selectedTrackId && !outgoingAudio.paused and just return the current mode, or explicitly audio.play() again if the resolved source turns out unchanged.

Fix this →

Minor / non-blocking

  • usePlayback.ts:256-273 (handleEnded): the manual if (repeatEnabled) { audio.currentTime = 0; audio.play(); } branch is now effectively dead for the local/host element, since audio.loop = repeatEnabled (native loop, usePlayback.ts:166-169) means ended never fires on that element while repeat is on. It's still reachable for the listener's remoteAudioRef MediaStream-ended case, so not safe to delete outright — but worth a comment noting it's listener-only now, to avoid future confusion.
  • useSession.ts:382-393 (captureAudioStream): dropping the "No audio track detected" throw removes host-facing error surfacing for a source that never produces a live track (unsupported codec, genuinely silent asset) — it'll just silently retry forever via onPlay with no error/timeout. Given the design doc treats this as intentional (retry-on-play), consider a bounded retry count that surfaces Capture unavailable if no live track appears after a few attempts, so a genuine failure doesn't look like a stuck/silent player.
  • No new unit tests were added for the capturedSourceRef no-churn guard or the pause-on-select change, even though both are plain logic (not timing-dependent) and could be covered without a browser. Worth adding alongside a fix for the regression above.

Not independently verified

Could not run lint/typecheck/build/test:unit myself in this environment (no node_modules installed, and installing wasn't in scope for a review). Taking the PR's reported green run at face value; the manual QA checklist in the design doc still needs a real two-device pass before merge as the author notes.

…review)

Three non-blocking review items on the room-stream fixes:

- captureAudioStream dropped its "no audio track" throw in favor of retry-on-
  play, but a source that never produces a live track (unsupported codec,
  genuinely silent asset) would retry forever with no error. Add a bounded
  counter (nextCaptureAttempt, ceiling MAX_CAPTURE_ATTEMPTS): after a few
  trackless attempts for the same source, surface "Capture unavailable"; a live
  track resets it. Normal pre-play trackless attempts are still tolerated.
- Extract the capture decisions into a pure module (features/rooms/streamCapture)
  - shouldReuseCapture (the no-churn guard) and nextCaptureAttempt - and cover
  them with unit tests (9 cases), since they are plain logic.
- Comment the handleEnded repeat branch: it is listener-only now, because the
  host element loops natively (audio.loop = repeatEnabled) and never fires
  `ended` while repeat is on.

The pause-on-select change is a direct DOM side effect (pause the outgoing
element) with no pure logic to extract, so it is left without a bespoke test.
83 unit tests (9 new), lint 0 errors, build/fmt/signaling green.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@knzeng-e
knzeng-e merged commit 319a5dd into main Jul 7, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant