fix(core,studio): silence hidden audio in preview, and call it mute - #3275
Conversation
Presets button becomes the stacked primary control (bold, filled outline);
Add-effect demoted to a small trailing link ("+ effect"). Button onClick
bodies and audition-revert logic are unchanged.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Preview scheduled every audio[data-start] regardless of data-hidden, so a hidden audio track was silent in the export but audible in preview — render was already correct, this was a preview-only parity bug. Web Audio scheduling now skips (and re-syncs on toggle) any audio clip under a data-hidden ancestor; the HTMLMedia per-tick volume path folds the same check into effectiveVolume without touching el.muted (transport-owned). Ships unflagged since it's a bugfix restoring parity. Also relabels the eye as Mute/Muted on audio-only track rows (icon, strikethrough label, undo-history copy), gated behind the new audio-track-mute canary — the relabel is a copy/UX change, kept separate from the behavior fix above. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
terencecho
left a comment
There was a problem hiding this comment.
Read end-to-end. Verdict: COMMENT — implementation looks correct; the new init.test.ts cases don't actually exercise the WebAudio path they claim to, and Test: runtime contract is red on that.
Preview / export parity — correct
- Render already skips
data-hiddeninpackages/engine/src/services/audioMixer.ts(isHiddenwalks ancestors, thenelements.pushis skipped foraudioanddata-has-audiovideo). This PR adds the mirror on the preview side in two places (scheduleWebAudioForActiveClips+dockNativePlaybackToWebAudioshort-circuit +syncRuntimeMediaeffective-volume zero), both usingclosest("[data-hidden]"). Consistent with the render-side ancestor walk — same flag, same semantics. el.mutedis deliberately untouched — transport ownership stays intact. Good, and the media test pins it.- Non-hidden audio unchanged in every path (both scheduler filters are a single
continue, andeffectiveVolumeonly changes when the closest selector matches).
Canary shape — correct
audio-track-muteat 0% gates only the copy/UX (icon swap, strikethrough label, "Mute/Unmute track N" undo copy). The preview-silence behavior ships unflagged. That split matches the PR description: bugfix vs. UX rename.- Undo label in
timelineTrackVisibility.tssays "Mute/Unmute" for an audio-only track and stays "Hide/Show" for mixed tracks — tests pin all three shapes.
Blocker: Test: runtime contract red on this PR's own new tests (deterministic, not a flake)
packages/core/src/runtime/init.test.ts:1361—expected "decodeAudioElement" to be called 1 times, but got 0 timespackages/core/src/runtime/init.test.ts:1416—expected "decodeAudioElement" to be called 2 times, but got 0 times
Root cause: initSandboxRuntimeModular sets webAudioReady = false and only flips it via void webAudio.init().then(ok => webAudioReady = ok) (init.ts:174-177). Under vitest's jsdom env (packages/core/vitest.config.ts) AudioContext is undefined, so WebAudioTransport.init catches the throw and returns false — webAudioReady never becomes true. Both call sites of scheduleWebAudioForActiveClips inside player.play() (init.ts:2362) and applyWebAudioRate (init.ts:3098) are gated on webAudioReady, so decodeAudioElement is never invoked → 0 calls in CI.
bun run test locally may have passed if Bun's global AudioContext shim exists, but Node-jsdom in CI won't. Suggested minimal fix in the two new tests (before initSandboxRuntimeModular()):
vi.spyOn(WebAudioTransport.prototype, "init").mockResolvedValue(true);then await Promise.resolve() (or await vi.waitFor(...)) after initSandboxRuntimeModular() so the .then microtask flips webAudioReady before player.play(). Without that, these tests silently pass by not-exercising the guarded path even when the implementation is right.
Nit: the second test's keepPlaying: true comment is helpful, but note that the visibility-sweep path at init.ts:1988 bypasses the webAudioReady gate, so once webAudioReady is stubbed true the mid-playback branch is what you're actually asserting on — assertion decodeSpy toHaveBeenCalledTimes(2) will fire from there.
Non-blocking observations:
nodeAffectsAudiousesmatches("audio[data-start]") || querySelector("audio[data-start]")— correct for the timed-audio case, and matches the ancestor[data-hidden]walk used for filtering.- Stack overlap with the (now-merged) #3274 shows in the 3 FX-label test files; a restack drops them cleanly and mergeStateStatus stays
MERGEABLE.
— Review by tai (pr-review)
…nt, not the decode fallback CI was red on `Test`, `Test: runtime contract` and `Tests on windows-latest` — all three on the same two tests, both reporting `decodeAudioElement` called 0 times. Not a bug in this branch. The tests pass on the branch tip and fail on the MERGE with main, which is what CI actually builds. Main had moved 66 commits ahead, and #3322 ("make creator media edits render-safe") added `WebAudioTransport.scheduleMediaElementPlayback`: media-element clips now route straight through the Web Audio graph instead of being decoded into an AudioBuffer. `decodeAudioElement` survives only as the fallback for the rate-shifted case (`Math.abs(effectiveRate - 1) > 1e-9`), so on the ordinary path it is correctly never called: void webAudio.scheduleMediaElementPlayback(...).then((scheduled) => { if (scheduled || !clock.isPlaying()) return; // <- returns here now ... void webAudio.decodeAudioElement(rawEl) // <- fallback only Both tests used `decodeAudioElement` as a proxy for "this clip reached Web Audio scheduling", which was accurate before #3322 and is not any more. Retargeted to `scheduleMediaElementPlayback`, which is that signal now and takes the element as its first argument, so the assertions keep their exact shape and meaning. Confirmed by instrumenting the run rather than inferring: on the merged tree the scheduler is called exactly once, with the audible element — the feature under test works, only the probe was pointed at the wrong method. Still non-vacuous: deleting the `rawEl.closest("[data-hidden]")` guard from `scheduleWebAudioForActiveClips` fails the first test with "expected 1 times, but got 2 times", so it genuinely catches a hidden clip being scheduled. `init.test.ts` 77/77, and 1259 passed across packages/core `src/runtime` + `src/audio` on the merged tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI is green ( The failure was not in this branch's logic. Both failing tests reported void webAudio.scheduleMediaElementPlayback(...).then((scheduled) => {
if (scheduled || !clock.isPlaying()) return; // <- returns here now
...
void webAudio.decodeAudioElement(rawEl) // <- fallback onlyBoth tests were using I confirmed the feature itself was fine by instrumenting the run rather than inferring: on the merged tree the scheduler is called exactly once, with the audible element, and the hidden clip is excluded. Only the probe was pointed at the wrong method. The assertions are still non-vacuous — deleting the Also merged main in, since the branch was 66 behind and the retarget only makes sense against the new scheduling path. Verified locally: Still needs a reviewer — it's |
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed at head ef782969. My prior COMMENT at 7b10657c (review 4990502881) flagged the two new init.test.ts cases as inert — I read webAudioReady as gating every scheduler entry point, and under jsdom WebAudioTransport.init() returns false.
I missed one path: the new hiddenAudioDirty call at init.ts:1988 fires inside syncTimedElementVisibility regardless of webAudioReady, and scheduleWebAudioForActiveClips itself only guards on state.nativeMediaSyncDisabled || state.webAudioMediaDisabled. The tests take that path — player.play() → syncMediaForCurrentState → syncTimedElementVisibility → visits the audio elements → the hidden one marks hiddenAudioDirty → the guard fires with clock.isPlaying() true. The spy on WebAudioTransport.prototype.scheduleMediaElementPlayback captures the outer method call (_ctx is unset, so it early-returns null internally, but the spy has already fired).
Net: both cases now genuinely discriminate the filter. Test 1 asserts only audibleAudio (not hiddenAudio) reaches scheduleMediaElementPlayback; Test 2 asserts that mid-playback unhiding of two clips (via seek(1, { keepPlaying: true }), which re-enters play() → visibility pass) batches into one startGeneration + two scheduleMediaElementPlayback calls. Implementation itself is unchanged from the last look: scheduler skips data-hidden, HTMLMedia effectiveVolume zeroed under a data-hidden ancestor with el.muted untouched (transport-ownership rule), and the one-reschedule-per-visibility-pass batching lives in the sync loop rather than in each toggle site. Parent #3274 merged; Test: runtime contract green at head along with the rest of CI.
— Review by tai (pr-review)
Summary
audio[data-start]regardless ofdata-hidden, so a hidden audio track was silent in the export but audible in preview — render was already correct (isHiddeninaudioMixer.ts); this restores parity. Ships unflagged since it's a bugfix, not a new behavior.scheduleWebAudioForActiveClipsininit.ts) now skips any audio clip under adata-hiddenancestor, and re-syncs (batched, one call per visibility pass) when the attribute flips mid-playback.media.ts) folds the same check intoeffectiveVolume—el.mutedis untouched, since that flag is the transport's playback-ownership signal, not author intent.audio-track-mutecanary (0%) since it's a copy/UX change, kept separate from the unflagged behavior fix above.Depends on
Stacks on #3274 (A1) — review/merge that first.
Test plan
bun run build(core changed)cd packages/core && bun run test— 2339/2339 passcd packages/studio && bun run test— 4244/4244 passbunx oxfmt/bunx oxlintclean on changed filesinit.test.ts);effectiveVolumezeroed underdata-hiddenwithout touchingel.muted(media.test.ts); audio-only track gets Mute/Unmute labels, mixed track keeps Hide/Show (timelineTrackVisibility.test.ts)hyperframes previewa composition with music, hide the music track, press play — silence in preview; toggle the eye mid-playback and confirm it silences without the transport restarting🤖 Generated with Claude Code