refactor(audio): the review's cleanup list, and the design record behind the rack - #3176
Open
vanceingalls wants to merge 8 commits into
Open
refactor(audio): the review's cleanup list, and the design record behind the rack#3176vanceingalls wants to merge 8 commits into
vanceingalls wants to merge 8 commits into
Conversation
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-19d-cleanups
branch
from
August 11, 2026 08:33
574d198 to
540056a
Compare
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.
The last slice of the old #3156.
Four of the nine cleanups the review listed: the non-null assertion and two casts the guards already covered, one enabled-node helper instead of three restatements, and dataset keys derived from their attributes.
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: a bypassed node shifts every later node's parameters into its neighbour with an identical shape either way. Now covered.Also carries the design record for the rack work that follows: the preset research and catalogue rationale, and the schematic direction.
🤖 Generated with Claude Code