fix(core): align preview transport with grouped audio - #3445
Conversation
jrusso1020
left a comment
There was a problem hiding this comment.
Reviewed at 2665278a71, base wa-26a-core-audio-model. No prior reviews. Read together with #3444, which I reviewed first (5003519240).
This is a strong PR — the media-element path reaching the bus, the fader sitting post-FX where the render bakes it, and the mock hardening are all real fixes with real tests. Two things below, one of which I flagged on #3444 and said I would hold this PR on. I am not holding it, and I want to say why rather than let that quietly change.
The CI position, stated because it is easy to misread
Seven of the eight required contexts — Build, Typecheck, Test, Test: runtime contract, Semantic PR title, and both windows jobs — have never run at this head. Only regression is present. That is not a defect and not a misconfiguration: .github/workflows/ci.yml filters on branches: [main], so a stacked rung gets nothing until it retargets, and the workflow already anticipates the repair —
types: [opened, synchronize, reopened, edited]— with a comment explaining that edited is exactly what re-fires it when Graphite sets the base back to main. So this self-repairs on retarget and needs no action. I checked that rather than assuming, because the same situation in our other repos does not self-repair and I would have filed a false warning.
The half that does matter for a reviewer: Test has not run, and this PR rewrites ~375 lines of test code. Nobody — me included — currently has green-suite evidence for it. Worth a re-look at the matrix after retarget rather than treating the present board as informative.
isMemberGroupHidden resolves by author id, and this PR is what makes it load-bearing
Carried from #3444, where the one-line fix lives. resolveAudioGroups resolves a member's bus as AUDIO_GROUP_RENDER_ID_ATTR ?? HF_AUDIO_GROUP_ATTR; isMemberGroupHidden reads only the author id. On a stamped document those disagree, because instance 1's render id is its author id, so every instance's members resolve instance 1's bus. Executed against the shipped modules on #3444's own two-instance fixture, with only instance 2 muted:
resolveAudioGroups (render): bed -> hidden=false bed__hf2 -> hidden=true
isMemberGroupHidden: m1 -> hidden=false m2 -> hidden=false <- m2's bus IS muted
This PR wires that predicate into three places that decide audibility:
init.ts—isSilencedByHiddenmedia.ts—syncRuntimeMedia, which setsel.volume = 0webAudioTransport.ts:408—resolveEl()re-resolves the bus on reanchor, then readsdata-volumeanddata-hiddenoff it, also by author id
The part I could not settle from the diff, and would rather ask than assert: which of these run against a stamped document? assignMediaRenderIds is called from packages/producer/src/services/htmlCompiler.ts and renderMediaCollector.ts, so compiled documents are real and the runtime executes inside them — but the WebAudio transport looks preview-only, and preview is unstamped by design (resolveAudioGroups' own comment says so). If all three are preview-only this is latent; if syncRuntimeMedia sees compiled output, it is live for multi-instance sub-compositions. You know that pipeline far better than I do.
Either way the one-line change in #3444 makes the question moot, and I would take it there rather than here:
const groupId =
el?.getAttribute?.(AUDIO_GROUP_RENDER_ID_ATTR) ?? el?.getAttribute?.(HF_AUDIO_GROUP_ATTR);Why I am not setting CHANGES_REQUESTED, having said on #3444 that I would. Reading this PR changed the grading. Before it, neither init.ts nor media.ts consulted the bus at all — closest("[data-hidden]") cannot see a group, so a muted bus was ignored in preview while the render dropped its members. This PR fixes that for the single-instance case, which is the overwhelmingly common one. So it is a strict improvement that stops short, not a regression — and blocking a strict improvement over an incomplete edge is the specific error I would be making. The finding stands and I would like it fixed before the stack lands; it does not warrant holding this rung.
The bus fader's ceiling is unpinned
clampGroupVolume → clampAudioGain → Math.max(0, Math.min(MAX_AUDIO_GAIN, v)), with MAX_AUDIO_GAIN = 10 ** (12/20) ≈ 3.981. The floor is tested (data-volume="-1" → 0). The ceiling is not: the three fader values exercised are 2, -1 and absent, and 2 is below 3.981, so deleting Math.min(MAX_AUDIO_GAIN, …) leaves every assertion in the file green.
The test carrying the intent is named for it — previews an over-unity bus fader at the render's ceiling, not unity — and its comment names the ceiling and the 12 dB divergence explicitly, so the property is clearly meant to be pinned; the value chosen just cannot pin it. One line:
document.body.innerHTML = `<hf-audio-group id="vo" data-volume="10"></hf-audio-group>`;
// ...
expect(mock.gainNodes[4]!.gain.value).toBeCloseTo(3.9811, 3);Worth having precisely because this ceiling is the shared constant that keeps Studio, preview and render from drifting — its own docblock says so.
Checked and correct
- The old
group volume rides the group's own data-volume…test asserted onlyresolves.not.toBeNull(). A name that quantified over a property while the body checked that scheduling did not throw — which is exactly how the bus sat at unity while the render applieddata-volume, unseen. Finding and splitting that into two real assertions is the best thing in this PR. - The mock hardening is the same class of catch. A bare
{ value }gain madecancelScheduledValuesthrow,schedulePlaybackswallows throws intoreturn null, so the mock's own gap read as "the member did not play". Stubbing the AudioParam surface andcreateMediaElementSourceremoves a failure mode that would have been diagnosed as product behaviour. - Fader post-FX matches
scheduleVolumeLane's stated contract and the order the render bakes in, so a nonlinear group effect previews as it renders. The comment explains whyinputwould have been wrong. clampNativeMediaVolumereplacing the inlineMath.max(0, Math.min(1, …))is the right call — it is the same boundwithUnclampedVolumeuses, so the two cannot drift.reanchorclearing the param lane before re-seeding matches the problem its #3444 docstring describes (a stuck fade-out owning the param for the rest of the session).
— Review by Rames (pr-review), James's assistant
2665278 to
a2e2884
Compare
|
Addressed: the over-unity bus test now supplies a gain above the actual render ceiling and asserts the clamped 3.9811 value. The focused transport suite passes (67 tests), along with both core typechecks. |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at a2e288420f. My earlier review (5003526168) was pinned to 2665278a71, which the restack replaced; the branch is now c4458d36 + a2e28842 "pin audio group gain ceiling".
Both things I raised are closed. One new finding below, and it is about merge order rather than about the code's end state.
Closed: the ceiling is pinned, and I re-derived it rather than checking it off
data-volume="2" → "10", expectation 2 → toBeCloseTo(3.9811, 3). Since this is my own suggestion coming back, I verified the number from source instead of recognising it: packages/core/src/audioGain.ts has MAX_AUDIO_GAIN_DB = 12 and MAX_AUDIO_GAIN = 10 ** (12 / 20) = 3.9810717055…, so 3.9811 at three decimals is exact (off by 2.8e-05, tolerance 5e-04). And the old value really was inert — 2 < 3.981, so Math.min(MAX_AUDIO_GAIN, …) could be deleted with the whole file still green. It cannot be now.
Closed: the isMemberGroupHidden carry-over
Fixed on #3444 at 62b6d35b, where the one-line fix belonged. I re-reviewed it there.
New: this rung deletes the solo bridge's writer, and the solo UI stays until #3454
This PR removes the only implementation of window.__hf.setAudioSolo:
- let soloedIds: ReadonlySet<string> = new Set();
window.__hf = window.__hf || {};
- window.__hf.setAudioSolo = (ids) => {
- soloedIds = new Set(ids);
- webAudio.setSolo(soloedIds);
- };setSolo has zero hits anywhere in the tree at this head, so the transport method goes with it. What does not go until #3454 — nine rungs up:
packages/studio/src/hooks/useAudioSoloBridge.ts:21still pushes,win?.__hf?.setAudioSolo?.([...soloed]). Optional-chained, so it no-ops in silence.TimelineSoloButton,audioSoloSliceand the solo banner all still render.TimelineGroupRow.tsx:116still callsisGroupHalfLitUnderSolo(soloed, group.id, memberIds), so the group row still half-lits.
So at this head, clicking Hear only this lights the button, half-lits the group row, renders the "N tracks" banner — and every track keeps playing at full level. A control that reports success is worse than a control that is gone.
The two predicates kept in audioGroups.ts:236,247 carry the comment "Compatibility bridge until the Studio solo controls are removed later in the stack." That is exactly the right intent; the same commit removed the bridge's writer. The predicates survived and the thing that populated their set did not, so nothing is being bridged.
Traced across every rung by reading init.ts and window.d.ts at each head:
| ref | setAudioSolo impl in init.ts |
decl in window.d.ts |
|---|---|---|
main |
present | present |
| #3444 | present | present |
| #3445 → #3455 | gone | present |
Not a blocker on the stack's end state. Solo is meant to go and by #3454 it is gone cleanly. It is a blocker on landing #3445 without #3454. Two ways out, either fine:
- Move the
setAudioSolo/setSoloremoval from here into #3454, which is what the compatibility-bridge comments already promise happens. - Or land #3445 → #3454 as one train and don't ship from a head in between.
Worth deciding explicitly rather than by merge timing, since #3444 is already retargeted to main and the rungs are being stamped individually.
The permanent half: window.d.ts is never touched by any rung
packages/core/src/runtime/window.d.ts:40-45 still declares
/**
* Studio's "Hear only this" push: the full set of soloed clip/group ids,
* replaced wholesale on every change. Session-only by design — never
* read from or written to any document attribute.
*/
setAudioSolo?: (ids: readonly string[]) => void;I checked all twelve rungs: none of them edits window.d.ts. After the whole stack lands, that declaration is the only surviving mention of solo anywhere in the tree — no implementation, no caller — and it is a documented public runtime API for a feature that no longer exists. Six lines. It is inert (optional member of a declare global), so a nit, but it is the one thing a future reader would take as evidence solo still ships.
My round-1 deletion audit on #3454 missed this because it was keyed on the exported symbols leaving, and setAudioSolo is an interface member rather than an export — so it fell outside the sweep. Filing it here because this is the rung where the declaration goes stale; #3454 or #3455 could equally carry the deletion.
— Review by Rames (pr-review), James's assistant
|
Fixed at aa78532. This rung now keeps the complete Studio solo compatibility path alive: |
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at aa7853286a (prior review 5003703245 @ a2e288420f).
My finding is closed, and closed in a stronger direction than the one I proposed.
I suggested moving the setAudioSolo removal into #3454. This keeps the bridge working at this head instead: soloedIds and window.__hf.setAudioSolo are back in init.ts:190-197, and the transport now carries a dedicated solo stage — soloGain per source, connectThroughSolo, setSolo, plus the isAudibleUnderSolo gate on the HTMLMedia fallback in media.ts:343.
Verified end to end at this head, by tracing the chain rather than reading the diff:
TimelineTrackPlainHeader.tsx:122 renders the button → App.tsx:85 calls useAudioSoloBridge → useAudioSoloBridge.ts:21 pushes win?.__hf?.setAudioSolo?.([...soloed]) → init.ts:197 implements it → webAudioTransport.setSolo re-evaluates every active source. Every link is present, so "Hear only this" actually silences audio at this rung.
The restructure is net-zero on the merged end state. I compared the full recursive trees at the old and new tops of the stack: 6,971 blobs each, and exactly one differs — packages/core/src/runtime/window.d.ts, which is my second finding being fixed on #3454. Everything this rung adds, #3454 removes. The solo code living on rungs #3445–#3453 costs nothing at merge.
Checks I ran on the new code rather than assuming, since it exists because I asked for it:
connectThroughSoloismain's implementation with one difference: the destination is resolved beforectx.createGain()rather than inline in theconnect()call. Behaviourally identical — the observable effect is gain-node creation order, which is exactly why the test indices moved to[5]/[7]. Nothing else in the reconstruction diverges frommain.- Teardown is complete. All three
gainNode.disconnect()sites (:517,:596,:692) carry the matchingsoloGain.disconnect(), including the buffer path's error branch, so no node is orphaned on failure. - The render is unaffected.
isAudibleUnderSoloreturnstrueunconditionally on an empty set (audioGroups.ts:242) and nothing in a headless render callssetAudioSolo, so everysoloGainis minted at 1. - Solo never attenuates a shared bus. The solo stage sits strictly between a member's own gain and the bus input, and the new test pins
firstGroupInput(mock).gain.value === 1while a sibling is silenced.
One nit, non-blocking, and it dies with this code at #3454. makeGain() defaults gain.value to 1, so the toBe(1) assertions in the three new solo tests cannot distinguish "this solo stage was set audible" from "this is some other node nobody wrote to." soloing a group keeps every member of that group audible is entirely toBe(1), so it survives any drift in the bus node count; the toBe(0) halves are what carry signal. Deriving the node from clipGain.connect.mock.calls[0][0] rather than gainNodes[5] would fix it the way firstGroupInput() already does elsewhere in this file.
No blockers.
— Review by Rames (pr-review), James's assistant
jrusso1020
left a comment
There was a problem hiding this comment.
Approving at aa7853286a, the exact head of my re-review above — that review is the audit; this is the stamp <@vanceingalls> asked for in Slack.
No blockers on the code. The one nit I raised on #3445 (the mock gain defaulting to 1, so the toBe(1) solo assertions are weaker than they read) is non-blocking and that code is deleted at #3454.
Scope of this approval, stated so it is checkable: it covers aa7853286a1c86380c2237e5786897a5fe5ad8ea only. main runs require_last_push_approval: true, so if this head moves the PR returns to REVIEW_REQUIRED rather than carrying my stamp forward — re-ping me and I will re-review at the new head.
— Review by Rames (pr-review), James's assistant
miguel-heygen
left a comment
There was a problem hiding this comment.
Independent exact-head pass at aa78532.\n\nI traced the live solo path end to end rather than relying on the restack summary: Studio pushes the session set into window.__hf.setAudioSolo, init forwards it to WebAudioTransport, and every active source has a dedicated solo gain before the shared bus. The bus fader remains post-FX, replay/seek reanchors the long-lived bus once per generation, and source teardown disconnects the FX, clip gain, and solo gain on normal end, discard, stop, and destroy paths. The later #3454 removal therefore changes no final-stack behavior while every intermediate rung stays functional.\n\nRames already recorded the non-blocking mock-signal nit on the toBe(1) solo assertions; I found no additional gap. All current required checks are green.\n\nVerdict: APPROVE\nReasoning: Preview routing, group gain, mute, automation, and temporary solo behavior line up with the core/render contract, with complete lifecycle handling at this head.\n\n— Magi
Part 2 of 12 replacing #3439. Base:
wa-26a-core-audio-model.Why
Preview must implement the same group routing, gain limits, hidden-state behavior, and automation clock as the model and renderer.
What
Verification
fallow audit --base main --fail-on-issuesThe final stack tip preserves the verified #3439 replacement and includes the review fixes landed across the stack.
Stack: #3444 ← #3445 → #3446