fix(audio): code-review fixes — clip-before-duck, phaser lanes, rate reschedule - #3156
Closed
vanceingalls wants to merge 28 commits into
Closed
fix(audio): code-review fixes — clip-before-duck, phaser lanes, rate reschedule#3156vanceingalls wants to merge 28 commits into
vanceingalls wants to merge 28 commits into
Conversation
vanceingalls
changed the title
…nder ffmpeg exits 0 and writes a structurally valid but EMPTY wav whenever a clip's trim starts past the end of its source — `-ss 10 -t 5` on a 2 s file. That track reached `new OfflineAudioContext(channels, 0, rate)`, which throws NotSupportedError, and the error travels past the mixer's per-track failure collector: one mis-set `data-media-start` on one clip with one enabled effect took the WHOLE render down with a RenderQualityError. An empty track has nothing to process, so applyAudioFxChain now hands the input back the way it already does for a chain with nothing enabled — before paying for a browser to decide it. The runtime stub guards the same case at the point the constructor would actually blow up, since it is reachable by any other caller of the injected script. The test falsifies against the real browser path: removing the guard reproduces `NotSupportedError: The number of frames provided (0) is less than the minimum bound (1)`.
The lease was acquired above the try, with `mkdtempSync` between the two — so a failure there (a full disk, a read-only tmpdir) returned without ever reaching the `finally` that releases it. A leaked pooled browser is worse than the error that caused it: a pool with no leases left hangs every later render instead of failing one. The temp dir is now taken first and the lease inside the try, with the release made conditional. The abort check moves above the acquire too, so an already-cancelled track no longer takes a browser out of the pool just to hand it straight back. No test: the fix is the ordering of two resource acquisitions, and the only way to observe it is to make mkdtempSync throw, which needs an injection seam that does not exist here and would be more risk than the three lines it guards.
…d wet/dry `in_gain` and `out_gain` trim the signal entering and leaving the phaser, and apply() drives them through inTrim/outTrim while pinning wet and dry to 1. The automation map aimed both lanes at wet.gain and dry.gain instead — so an envelope on either knob modulated a constant, the trim it was supposed to move stayed wherever apply() last left it, and any values-only edit re-ran apply() and slammed wet/dry back to 1 over the running ramp. Heard as: "fade the phaser to silence" ends at full dry signal. Wrong in preview and in the render, since both share this builder. The builder's own comment records that wiring these knobs to wet/dry was already found wrong once and moved to the trims; that refactor updated apply() and missed the map. The existing exposure test only asserted that SOME param was present under each key, which a mis-aimed target passes — the new test asserts by value, which is the only thing a lane actually cares about.
…lace Reordering two effects of the same type leaves `shapeOf` identical — it carries type, poles and the one-pole frequency, but no id — so the chain updates in place instead of rebuilding. That is right for the audio: the params move with the position. The ids did not move with them. They were captured once at build time, so after a reorder each handle named whichever effect used to sit in that slot, and scheduleChainAutomation built its byId map from those stale names: a lane on `fx.n2.frequency` drove the band that is now n1. This is the invariant HfAudioFxNode.id documents itself as protecting — "reordering the chain never re-points a lane at a different effect" — and it is easiest to hit on the all-peaking chains the voiceover carve produces. Preview only, since the render rebuilds from scratch, so the symptom is preview quietly ceasing to predict the render. Fixed by following the id, not by adding it to the shape: a same-type reorder genuinely does not need a rebuild, only a correct id map.
…der bakes it A lane's `t` is "seconds from the start of the clip" — HfAutomationPoint says so, and the render honours it: prepareAudioTrack cuts the wav with `-ss mediaStart`, so its t=0 IS the clip's start, and normaliseEnvelope subtracts trackStart. Preview fed `relTime` instead, which is MEDIA time: it carries mediaStart, scales by playbackRate and wraps on a loop. So the same envelope played somewhere else than it rendered. `data-media-start` past the last point held the final value from the first frame and never faded; `playbackRate: 2` ran the envelope at double speed in preview only; a looping clip restarted the envelope every lap in preview while the render baked it once. The FX lanes shipped in this same stack already use clip-local elapsed. There is one time base for automation, and this makes the volume lane use it.
…rsisting it
Two failures with one root: the row never tracked whether a gesture actually
edited anything.
The number field could not be typed into. It was bound to the committed value,
while every keystroke was clamped into range and written live — and a live
write does not refresh the prop, so React put the old number straight back.
Typing 5000 into a 20..20000 knob wrote 20 on the first keystroke and got no
further; the knob could only be dragged. Worse, `Number("") === 0` passes
Number.isFinite, so select-all + Delete before retyping instantly live-wrote the
parameter MINIMUM — 20 Hz on a cutoff, -40 dB on a gain. The field now holds
what is being typed as text, and an empty field is a state on the way to a
number rather than a number to clamp and persist.
`commit()` hangs off pointerup, keyup and blur, all reachable with no edit —
clicking a slider thumb without moving it, or tabbing through the field. It
unconditionally wrote `latest`, which was seeded at mount and only ever written
by an edit. So: open an EQ row, run the carve or press undo, then click the
slider and release without moving it, and the row silently re-persisted its
mount-time number over the change. It also fired a full persisting write —
source patch, selection resync, preview reload, audio restart — for a gesture
that moved nothing.
Each fix is falsified by deletion. A fourth guard (resyncing `latest` on every
inbound value) was dropped rather than kept: the edit flag subsumes it, and no
test could be made to fail without it.
The pattern allowed `[^;]{0,200}` between an element's selector and `volume:`,
and `[^;]` crosses `)`, `,` and newlines. A chained GSAP timeline has no
semicolon until the end of the whole chain, so
gsap.timeline().to("#bgm", { x: 10 }).to("#vo", { volume: 1 });
matched for `#bgm` — an element whose only automation is the lane. The warning
then told the author "the lane wins, the tween is ignored" about a tween that
is on a different track, and its fixHint invited them to delete the lane that
was actually working.
Refusing to cross the closing paren keeps the match inside the call the
selector belongs to. It costs a false negative when another value in the same
object is a call result, which is the safe direction for a rule that only
warns and, by its own comment, already only guesses. The `s` flag went with it:
inert, since the character class already matched newlines.
`src/generated/audio-fx-runtime-inline.ts` is what the engine injects into the headless page to do the actual DSP, and nothing kept it in step with its sources. Its sibling has `check:position-edits-render`, which rebuilds and then `git diff --exit-code`s the artifact — but that form needs the file tracked, and this one is gitignored. Measured tonight while fixing the zero-frame guard: editing the stub and re-running the engine's render tests injected the PREVIOUS bundle, so the fix appeared not to work and the old DSP ran with nothing reporting the drift. That is the exact preview/render divergence this stack exists to prevent, aimed at whoever is developing it. Making `test` regenerate it is the version of the gate that works without touching .gitignore: the artifact cannot be stale when the tests read it. Not covered: a fresh clone still cannot resolve `@hyperframes/core/audio-fx-runtime` from `packages/engine` until core has been built at least once. CI survives on build ordering. Tracking the artifact, or a pretest that builds core from the engine, are both bigger calls than this one.
…s stopped The `ended` listener spliced the source out of `_activeSources` and restored the element's muted flag, but never disposed the FX handle — and the splice is what made it unreachable, because `stopAll()` disposes by walking that same array. So every clip that finished naturally leaked its graph for the rest of the session. The leak is not just memory. Each stale handle keeps a MutationObserver bound to `data-fx-chain`/`data-automation`, so every later panel edit runs rebuild() against a dead source: a fresh graph per abandoned clip, including a ~576 KB reverb impulse and chorus/phaser oscillators that are started and never stopped. Play a five-clip composition three times and fifteen observers answer every subsequent knob turn. At this point in the stack `attachElementFxChain` has no chainless early return — it watches every audio clip, chain or not — so this leaked for all of them, not only the ones carrying effects. Disposal is skipped when the entry was already gone: `stopAll()` disposes its own, and `stop()` is what fires this event.
`analyse` captures the chain and the automation before its fetch and decode, then rewrites the whole `data-fx-chain` from that snapshot. Anything committed during those seconds — adding an effect, moving a knob — landed first and was silently discarded when the analysis returned. Only the Analyse control was gated on `analysing`; every other control in the rack stayed live throughout. FxSection already accepts `disabled` and threads it to every row; the carve panel simply never passed it. Refusing the edit for the duration is the honest answer — merging it into a measurement that did not account for it is not, and last-write-wins across an async gap is what this was. The test had to stub OfflineAudioContext as well as fetch: without a constructor, `analyse` returns before the decode and the window under test never opens, which is why the existing carve tests never saw this. Does not cover a write arriving from somewhere other than this panel — a timeline lane edit, or an undo — during the same gap. That needs the write to re-read rather than the reader to lock.
… them
The internal AbortController exists so a fatal FX error can cancel in-flight
ffmpeg before the finally-block deletes workDir. It reached the entry guard and
applyAudioFxChain, and nothing else: the trim, the video extract and the
download were all still handed the CALLER's signal. So `internalController
.abort()` cancelled nothing, and the `rmSync(workDir, { recursive: true })`
immediately after it ran while those children were still writing into the
directory.
Concretely: track A's FX render fails while track B is mid-trim, with no
external cancellation. B's ffmpeg keeps writing `${id}-trimmed.wav` into an
unlinked directory until it finishes. The comment above the controller
describes a mechanism that was never wired to the processes it names.
The download also had no signal at all, though downloadToTemp accepts one.
No test: the three changes are which variable is passed to a child process
helper, and observing the difference means driving real ffmpeg to the point of
cancellation. The old wiring's inertness is provable by reading — the caller's
signal is never aborted by this function — and the new wiring is the same three
calls with the controller the function already built.
`writeWav` clamps and quantises the chain's float output to int16, and the
mixer only ran `applyVolumeEnvelopeToWav` over that file afterwards. So a
chain that overshoots full scale was destructively clipped even when the
volume lane immediately pulled the track 12 dB down — audible distortion
baked into the render that preview, working in float throughout, never had.
Measured on 12 s of narration through a +12 dB peaking band into saturation
with the lane holding the track at -12 dB: the chain peaks at 1.346, so the
old order sheared 19,166 samples (1.7%) flat against ±1 before ducking. Level
matched, the two renders differ by an error signal 26.3 dB below the audio,
worst single-sample delta 0.242.
The envelope now travels into `applyAudioFxChain` and lands on the float
planes before `writeWav` sees them; the mixer runs its own pass only for a
track the FX pass did not bake. `writeWav`'s clamp stays — it is the correct
last resort for a signal still hot after the duck, just no longer the first
thing that happens.
The per-frame segment-cursor gain walk is extracted as `createEnvelopeWalker`
so both bakers share one implementation, and the ffmpeg-expression fallback
for non-int16 WAVs is untouched.
`applyAudioFxChain` now returns `{ path, envelopeBaked }` rather than a bare
path: inferring the bake from `path !== inputWav` would silently break on the
two early returns (empty chain, empty track).
The new test is stereo with a step in the envelope, so it also falsifies a
per-plane walk — restarting the cursor for a second channel hands it the tail
gain for its whole length. Falsified against three mutations: clamp-then-duck
(0.5 instead of 0.9), envelope dropped (1.0), plane-outer walk (0.449).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The whole track crossed in one `page.evaluate` pair — ~184 MB of base64 for a 3-minute stereo 48 kHz clip, in a single WebSocket frame each way. puppeteer-core caps its frames at 256 MB (NodeWebSocketTransport, verified in the installed copy) and the browser pool does not pass `pipe: true`, so stereo 48 kHz past ~8.7 minutes raised AudioFxRenderError and failed the render. V8's max string length is a second wall not much beyond it. Both were fatal: an FX failure takes the whole mix down rather than dropping one track. Input and output now move 8 MiB at a time, staying separate byte arrays page-side rather than being concatenated into one string, so neither the frame cap nor the string limit ever sees the whole track. That also bounds the peak Node-side allocation: `processCompositionAudio` renders tracks through an unbounded `Promise.all`, so every track's payload used to be live at once. The page drops the input chunks before the render allocates its buffers, so it no longer holds two copies of the track. Also drops the `Array.from(u8.subarray(...))` in the encoder, which boxed every byte of each 32 KiB window into a JS array for nothing — `apply` takes array-likes. The 8.7-minute ceiling itself has no test: reproducing it needs a 256 MB payload. What is tested is the assembly, which is where chunking actually goes wrong — a 45 s stereo ramp spanning two chunks per plane, checked for length, for values at the seam, and for monotonicity across all 2.16 M frames. Falsified against three mutations: chunks reassembled in reverse (1.75 off at the seam), the trailing partial chunk dropped (short by 125,696 frames), and an unbounded input subarray (long by 125,696). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`attachElementFxChain` gets a frozen `{scheduledAt, elapsed, rate}` and commits
every lane to absolute context times from it. `setRate` only assigned
`playbackRate.value`, so the audio changed speed and the envelopes did not: a
lowpass sweeping over 10 clip-seconds, switched to 2x mid-play, eats 20 s of
material in 10 s of wall clock while the sweep keeps its original schedule.
The runtime's recovery — `stopAll()` plus a reschedule — only runs when
`hasBoundedActiveSources()` is true, and a project-level music bed with no
`data-duration` is unbounded, so that case never recovered at all.
The timing a chain measures from is now a mutable reference frame that
`setRate` rebases: `elapsed` is advanced to the playhead the OLD rate carried
it to, then the lanes are replayed from there at the new one. That also fixes
the second consequence — `timingNow()` was advancing `elapsed` with the stale
rate, so every later chain edit re-aimed the envelope at the wrong clip
position for as long as the track played.
`attachElementFxChain`'s return type is now the named `ElementFxHandle`, so the
transport's `ScheduledSource.fx` carries `setRate` rather than a structural
`{ dispose(): void }`.
Falsified against four mutations: `setRate` a no-op (envelope stays 8 s long),
rescheduled without adopting the new rate (6 s instead of 3), rate swapped
without rebasing the frame (booked from t=0 instead of t=2), and the guard
dropped (rate 0 books a NaN span).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both analysis loops allocated a fresh `Float64Array(4096)` pair inside the per-window loop. A 5-minute 48 kHz voiceover is ~7030 windows, so one carve churned ~460 MB of transient Float64Array through the main thread — the panel's thread — for a measurement that needs 64 KiB of scratch. Now one pair is reused; `re` is fully overwritten each window and only `im` has to be cleared. Wall-clock is unchanged (591 ms vs 596 ms on that file, within noise): the FFT dominates and a bump allocation is nearly free. This is a GC-pressure fix, not a latency one, and the band values it produces are bit-identical. The review's third loop is `analyseCarveDuck`, which runs `windowDb` rather than an FFT and allocates nothing per window — nothing to fix there. Striding the Welch hops was tried and REJECTED. It is 27x faster on a 5-minute voiceover (427 ms -> 15 ms), but it moves the result: at strength 0.9 the chosen band set changed (630 Hz became 160 Hz), and it did not converge back to the full read even at 2048 windows. That is the author's carve being silently redrawn to save time on a measurement that already locks the rack while it runs. The reason is recorded in the code so it is not re-attempted. Falsified by deleting `im.fill(0)` from both loops: five tests fail, including the two added here — a steady tone must measure the same bands whatever its length, and its dynamics envelope must sit still rather than sliding as the contamination accumulates. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ensureAudioFxWorklets` cached its promise per context and never removed a rejected one, while `readyContexts` was only written on success. So callers correctly kept asking and every ask replayed the same rejection: one transient `addModule` failure left the limiter, compressor, gate and bitcrush silent for the life of that AudioContext, with no path back short of a page reload. The entry is now dropped on failure so the next attempt actually retries. Falsified by removing the `registered.delete(ctx)`: the retry replays the rejection instead of resolving. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…onnecting it An AudioWorkletProcessor lives until its `process()` returns false; all four returned true unconditionally, and `dispose` only called `node.disconnect()`. So every chain rebuild that dropped a limiter, compressor, gate or bitcrush left it running on the audio thread for the rest of the session — and a structural edit rebuilds the whole graph, so a few passes over a carved bed stacked up abandoned processors that nothing could reach to stop. `dispose` now posts `__hfDispose`, which each processor treats as its cue to return false from then on. A later parameter update cannot revive it. Falsified two ways: dropping the `if (this.dead) return false;` guards (each processor keeps running after dispose) and dropping the postMessage (dispose sends nothing). The processor test evaluates the module source pulled back out of the data: URL that registration hands to `addModule`, so it exercises what actually ships rather than a copy. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`normalizeAudioFxParams` coerced with a bare `Number(raw)`, and `Number(null)`,
`Number("")`, `Number(false)` and `Number([])` are all 0 — every one of them
finite. So a parameter that arrived missing or blanked clamped to 0 instead of
falling back to the effect's declared default, and because 0 is a legal setting
for most of these knobs nothing downstream could tell the difference: a
compressor whose threshold came through as null sat at 0 dB and never engaged.
Now only a number, or a string that actually spells one, counts — the same rule
`numberOrNull` in audioAutomation.ts already applies. Panel inputs arrive as
strings, so those still work.
Falsified by restoring the bare `Number(raw)`: null reads as 0 dB instead of
-24.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`runAudioStage` catches the rejection `processCompositionAudio` throws when an FX failure cannot be degraded past — and returned `audioFailures: undefined` with it. `applyDistributedAudioWarningPolicy` reads owner, retryability, reason and stage off that list, so the FATAL failure was reported with an empty reasons array, an empty stages array and no owner: strictly less classification than a single dropped track gets, which is the opposite of what the stage's own comment says it exists to do. A failure is now synthesised from the error. "internal" is the honest bucket — the stage names enumerate ffmpeg steps and this is the FX render, which is none of them — and system/non-retryable is right for a browser or chain that will not build. The detail is bounded at 2000 characters like the mixer's own. Falsified by restoring `audioFailures: undefined`: both assertions fail. Producer's suite has 50 pre-existing collect failures under vitest (files that import `bun:test`); passing goes 533 -> 534, unchanged otherwise. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both symbols the entry named are gone: `AUDIO_FX_WORKLET_SOURCE` is a module-private const rather than an export, and `__resetAudioFxWorkletsForTests` does not exist at all. The entry has been inert since c3291f2 added it, and an ignore that names nothing only makes the next reader look for something that is not there. Left alone deliberately: the file-scoped `health.ignore` on `packages/core/src/runtime/media.ts`. It is over-broad — its comment says it is for `refreshRuntimeMediaCache`, but it also exempts `syncRuntimeMedia`, which IS what this stack changed. `health.ignore` takes file paths only, so there is no way to narrow it in config; removing it wants a fallow run to confirm what it then reports, and fallow already fails on this stack. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The rack's rows were keyed `${type}-${index}`. Two effects of the SAME type
keep those keys through a reorder — position 0 is `peaking-0` before and after
— so React reused each row where it stood rather than moving it with its node.
The controls hold real state (a number field mid-edit is held as text, a drag
holds a local value), and that state stayed at the position: moving an effect
handed its half-typed value to whichever effect took its slot.
Different types happened to be safe, because the type is in the key. Same types
were not, and a rack with two EQ bands is the common case.
Now keyed by `node.id`, which the carve module's own list above already does,
falling back to the old form for a node minted without one.
Falsified by restoring the index key: the slot keeps showing 123 after the
effect carrying it moved away.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…y cover
Three violations of the repo's "avoid `any`/`as T`" rule and the standing
no-`!` rule, all in the carve's analysis path.
`resolveAutomationRange` cast to `HfAudioFxNumberParam` immediately after
`if (!param || param.kind !== "number") return null` — the discriminated union
was already narrowed, so the cast asserted what the compiler knew. Removing it
leaves tsc clean, which is the proof it was dead.
`analyse` read `doc!.baseURI` and `el.getAttribute("src")!`, and laundered
`getElementById`'s `HTMLElement | null` through `as HTMLAudioElement | null`
into a predicate that only checked for a `src` attribute — so the "is this an
audio element" claim was never actually tested by anything. The document is now
guarded once at the top, and the voices are read out to `{ src, start }` values
as they are found, which makes both fields non-null by construction rather than
by assertion.
The element check is by `tagName`, not `instanceof HTMLAudioElement`: these
elements belong to the composition's iframe document, so this realm's
constructor never matches them. It is not a narrowing either — `sourceOptions`
is built from `doc.querySelectorAll("audio[id]")`, so a carve source is an
`<audio>` element by construction.
No behaviour change, so no new test. The path is covered: forcing the voice
collection to yield nothing fails 8 existing tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`enabledAudioFxNodes()` existed and was used only by the engine, while audioFxGraph restated the same predicate three times in three shapes — `.filter((n) => n.enabled !== false)` in `shapeOf`, `if (node.enabled === false) continue` in `buildFxChain`, and the filter again in `update`. All three now call the helper. Also drops the `shape = shapeOf(next)` at the end of `update`. The early return above it already established that `shapeOf(next)` equals `shape`, so this was a full normalise-and-join of the chain per observer tick to write back the string that was already there. `shape` is `const` now, which is the compiler enforcing the same thing. Consolidating found a genuine gap: `update`'s filter had NO test. A bypassed node is not in the graph, so the update must walk the ENABLED nodes to stay aligned with what was built — walking every node shifts each one's parameters into its neighbour, and the shape is identical either way so nothing forces a rebuild that would expose it. Both other call sites failed under mutation; this one passed. Now covered, and falsified: the lowpass keeps its old cutoff while the bypassed peaking's values land on it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The studio writes these attributes through `HF_AUDIO_FX_ATTR` and `HF_AUDIO_AUTOMATION_ATTR` but reads them out of `dataAttributes`, which is keyed without the `data-` prefix — so the read side carried its own `"fx-chain"` and `"automation"` literals in four places. The two spellings had no link, so a rename could only half-land: the writes would move and the panel would go on reading an attribute nothing writes any more. Both keys are now derived from their attribute (`HF_AUDIO_FX_DATA_KEY`, `HF_AUDIO_AUTOMATION_DATA_KEY`) and the four read sites use them. Falsified two ways. Repointing the derived key breaks 24 existing panel and summary tests, which is what proves the reads actually go through it. And the new one-line pairing assertion in each module fails the moment the derivation is replaced by a literal again — which is the regression it exists for, since a hardcoded key that happens to be right today passes every other test in the suite. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… set Voice carve made a pile of effects into one understandable feature. This asks what else can, using only the 15 effects already in the registry. Surveys what consumer tools ship and splits it into three tiers: ML enhancement (Adobe/Descript — NOT reachable, and it is the most-requested thing in our own internal feedback, so the doc says so plainly rather than dodging it), intent panels over conventional DSP (Premiere's Essential Sound — the model worth copying), and character effects (CapCut — fully reachable, except the pitch and formant half, which needs DSP we do not have). Proposes ~20 static presets with concrete values, all inside the registry's declared ranges and in the node order the audio skill already fixes, plus five adaptive scripts. The load-bearing finding: none of the scripts needs new DSP — de-ess, leveller and tone-match are different questions asked of analyseCarveDynamics, windowDb and powerSpectrum, which already run in the panel. Carve is not one feature, it is the script engine. Framed against the VST cancellation, which is the governing precedent: that was killed for a sidecar process, an external repo, an unbundled dependency and render-path liability. Preset values are data and the scripts reuse shipped analysis, so this is the inverse of all four. Four traps recorded so they are not hit later: a de-esser cannot be a static EQ cut; boost presets must end in a limiter (yesterday's overshoot sweep is the rationale); the leveller's lane must ride a `gain` node because VOLUME_RANGE is 0..1 and normaliseEnvelope clamps to it, so a volume lane can only attenuate; and analyseCarveDynamics' 85-150 ms hop is too coarse for 50-150 ms sibilants, so de-ess needs it re-parameterised rather than called as-is. Nothing built. Research and design only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ets doc The rack is a 292px column of stacked modules, which is near enough a Eurorack case — so each effect gets a faceplate: a 3px family-hue rail, lettering per family, and a tint step per module within the family. Costs almost no plumbing. `group` is already on every effect in the registry so hue and lettering are derived rather than hand-assigned, and every module already renders `data-fx-node="<type>"`, so the identity layer is CSS on an attribute that exists today. Only the Smart family is new, and carve already lives there in spirit. Records the type budget as the real decision (self-hosted only, two faces recommended — one is too subtle at 11px, five reads as a collage), and the preset menu design: grouped by the same families, module counts per row so a preset visibly IS a chain, and "measures" instead of a count on the adaptive ones, which delivers the static-vs-script distinction in one word. Mockup published as an artifact at the real panel width. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… why Four directions were mocked at the real 292px panel width — hardware silkscreen, vintage test equipment, risograph, schematic — and schematic was chosen. It won because it is the only one that adds information rather than decoration. Chain order is load-bearing in audio (a limiter first and a limiter last are different sounds) and nothing in the panel said so. Drawing the rack as its signal path also lets bypass be a route rather than an opacity, gives a preset a visible brace over the nodes it wrote, lets an automated parameter draw its lane instead of showing the stale number the lane replaced, and marks a measuring module with a second ring — which is the static-versus-script distinction from section 4 delivered as drawing rather than prose. Drops the 3px coloured rail from the first pass. It is the single most overused device in dashboard UI, and forcing each direction to find another answer is what separated them. Adds .impeccable.md with the design context the direction was chosen against — users, personality, principles, palette, anti-references — so the next session does not re-derive it or re-ask. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… lane An earlier pass drew each automation lane's envelope shape inside its module. Dropped: the lane already has a home in the timeline, and a second small drawing of it in the rack is decoration. What the rack is actually missing is the CURRENT value. The panel already receives that — FxSectionProps.liveAutomationValues exists because "an automated parameter's stored number is only the seed the lane replaced, so a rack that shows it stands still while the carve is audibly working". So an automated row is now an ordinary parameter row whose number is live at the playhead, marked `~` to say it is driven rather than set, with the marker moving with it. Stopped, the values go grey and hold. Better on its own terms and cheaper to build: no new drawing, and it consumes a prop that already exists for exactly this. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vanceingalls
force-pushed
the
wa-19-review-fixes
branch
from
August 10, 2026 22:39
19e0ed7 to
574d198
Compare
This was referenced Aug 10, 2026
Collaborator
Author
|
Superseded — split into smaller PRs to stay under 1000 lines each. The same commits now land across the chain from #3173 upward; no code changed, only the boundaries. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Code-review fixes for the audio FX stack. 35 commits, and several change
rendered audio — see below before reviewing this as a cleanup pass.
What changes what you hear
55f852a72the FX output is ducked before it is quantised, not after.The old order clipped the chain's output to ±1 and then applied the volume
lane, so a hot chain lost everything above full scale before the duck brought
it back down. Measured on real narration: the level-matched difference is
audible distortion, not rounding. Written up with the numbers in
plans/and reproducible from~/audio-fx-clip-ab/.accac9984the phaser's trim lanes aim at the trims. They were pointed ata wet/dry pair the builder pins to 1, so an envelope modulated a constant and
the trim it was meant to move stayed frozen — "fade the phaser out" left the
dry leg at full level.
5e166ad47FX automation reschedules when the transport changes rate.Lanes are committed to absolute context times, so a lowpass sweeping over ten
clip-seconds kept its wall-clock plan while the audio underneath ran at 2×.
f6f0e4cf2the volume lane is sampled at clip-local time, the way therender does it.
Correctness fixes with no audible change
eb60133e3retires a worklet processor on dispose instead of onlydisconnecting it — they live until
process()returns false, so every chainrebuild that dropped a limiter left one running on the audio thread for the
session.
05d081c61disposes a clip's graph when it ends.85257571clets afailed worklet registration be retried.
b754e72d0moves a node's id with itsslot on an in-place update, so a lane cannot end up addressing its neighbour.
eb0080decstops a blank parameter reading as zero.c9448aec0moves atrack's PCM across the wire in bounded chunks (a long track was ~184 MB in one
page.evaluate).43c88961b,714f662ed,bfa27381e,b1faacb3aare enginerobustness: an empty track, the browser lease, the ffmpeg abort signal, and the
audio failure that used to take the whole mix down silently.
Studio
fddd596e8keys FX rows by node id — on${type}-${index}two effects of thesame type keep their keys through a reorder, so a half-typed number or an
in-flight drag lands on whichever effect moved into that slot.
436db2506letsa parameter be typed and stops a bare blur from re-persisting.
371920695locks the rack while the carve measures.
Cleanups
Four of the nine listed in the review:
ac0e37247,ce7c9e486,5a2247435.The rest have written reasons in
plans/audio-review-fixes-handoff.md§4c —the largest is blocked behind the FlatSlider throttle question in #2190.
Lining up the three enabled-node call sites was not cosmetic: it exposed that
buildFxChain's filter had no test, and its failure is silent, because abypassed node shifts every later node's parameters into its neighbour with an
identical shape either way. Now covered.
Tests
core 1721 (110 files) · studio 3674 + 18 todo · engine services 736 + 3 skipped
· producer 534 vitest + 203 bun.
Commit bodies carry the falsification notes — each test was broken on purpose
and confirmed to fail.
Reviewing
plans/audio-review-fixes-handoff.mdis the map: §4b the two items thereviewer's cap cut, §4c the cleanup ledger with reasons, §8 the fixture.
One pre-existing failure, not from this branch:
producer src/services/coreRuntimeBrowser.test.ts > removes the control bridge during teardowntimes out at 5 s and does so identically with core and enginechecked out before this work.
🤖 Generated with Claude Code