feat(studio): make the FX rack speak the author's language - #3192
Conversation
aa53c6d to
5b5c70c
Compare
1549312 to
36bddd2
Compare
terencecho
left a comment
terencecho
left a comment
There was a problem hiding this comment.
LGTM with nits. Coupling to #3184 is dictionary-shape via bracket lookup — mediated by the typed exported interface (EffectCopy / ParamCopy / Ends), so refactors that renamed shape fields would surface as TypeScript breaks, not silent runtime bugs.
Consumption sites (all import from @hyperframes/core/audio-fx-copy):
propertyPanelFxNodeRow.tsx:16, 18—EFFECT_COPY, SUMMARY; readsEFFECT_COPY[def.id],copy.params[param.key].label|.hint(inplainDefat ~L420-431),EFFECT_COPY[node.type]?.title(header at ~L465),SUMMARY[node.type]?.(params)(at ~L451).propertyPanelFxPresetMenu.tsx:15—PRESET_PROBLEM;PRESET_PROBLEM[preset.id] ?? preset.descriptionat ~L589.- Test files —
propertyPanelAudioFxGroup.test.tsx:7andpropertyPanelFxSection.test.tsx:11-12useplainLabel(effectId, key)helper that throws on missing copy — enforces completeness in tests while prod degrades gracefully.
Coupling verdict. 4/4 sites use EFFECT_COPY[...].params[key].label-style shape. No getCopy() function exists — none is called. However: the shape is exported as a typed interface (EffectCopy, ParamCopy, Ends), so a #3184-side refactor renaming title→name would break at the TypeScript layer, not silently at runtime.
Pair-mergeability. #3192's base ref IS #3184's head branch — graphite naturally sequences #3184 first. Not a "must merge atomic" case: #3184 can ship alone (rack falls back to registry names via the 3-tier fallback chain); #3192 cannot ship without #3184 (import fails). This is a softer coupling than wa-18's #3212+#3213 or wa-19's #3177+#3178 — the graphite sequencing carries the load.
UI changes (all propertyPanelFxNodeRow.tsx unless noted):
- Rack row header:
label={node.label ?? EFFECT_COPY[node.type]?.title ?? registryDef.label}(~L465) — 3-tier fallback. - New summary line under header:
<p className="hf-fx-node-summary">(~L474-477) fromSUMMARY[node.type]?.(params). - New "Details — {registryDef.label}" strip inside open module (~L495-497) — parks the DSP name where the mechanism lives.
plainDef()(~L419-432) overlays plain names/hints onto registrydef.params, preserving range/step/unit/automatability.propertyPanelFxPresetMenu.tsx:588-593— preset button now stacks.hf-fx-preset-problemsentence over the preset's own name.propertyPanelFxSection.tsx— big new hover-audition apparatus (auditionBase ref,audition(), previewRef indirection, unmount teardown, per-shelf leave handlers, levelling audition wiring).propertyPanelAudioFxGroup.tsx— newdecodedref caches decoded audio persrcfor the leveller (drops stale decodes on hover-away).
Accessibility. Header text reads "Remove Rumble" (plain) instead of "highpass" (DSP) — better for screen readers. No aria-label / aria-pressed changed.
Fallback chain (no undefined leaks to DOM):
- Missing
EFFECT_COPY[node.type]?.title→registryDef.labelrenders (DSP name). - Missing param copy →
plainDefreturns registry param unchanged. - Missing
PRESET_PROBLEM[preset.id]→?? preset.description. - Missing
SUMMARY[node.type]→{summary ? <p>… : null}renders nothing.
OLD-assumption counterfactual: the OLD tests spelled out rowFor(host, "Cutoff"). With copy-driven code, "Cutoff" no longer appears (plain label for lowpass.frequency is "Cut above"), so those pins would fail. Positive pins at propertyPanelFxSection.test.tsx:695-696: expect(name).toBe(EFFECT_COPY.highpass?.title) AND expect(name).not.toBe(getAudioFxDef("highpass")?.label) — explicitly forbid a regression to hardcoded/registry labels.
CI: Preflight red on the same plans/audio-fx-ux/README.md as #3184. Needs bun run format.
Nits:
SUMMARY[node.type]?.(params)invokes a function-valued record entry; if #3184 ever changes thePshape (Record<string, unknown>) to a strongerPartial<HfAudioFxParamValues>type, that call signature is silent-breaking at runtime, not a TS break. Consider asummaryFor(type, params)wrapper in #3184 to anchor the call signature.plainLabelin tests throws on miss while production silently falls back to registry — the asymmetry is fine (tests enforce completeness, prod degrades gracefully); worth a comment.
— Review by tai (pr-review)
miga-heygen
left a comment
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat(studio): make the FX rack speak the author's language — #3192
Verdict: LGTM
Coupling with #3184 is export-surface only. Imports exactly EFFECT_COPY, SUMMARY, and PRESET_PROBLEM — no reach into internals, no structural assumptions. Via's concern is unfounded here; this is the exact kind of import-surface dependency that package boundaries are for. The completeness test in #3184 guarantees every registry effect has copy, so these lookups are safe.
Fallback behavior is thorough. Every lookup has a graceful chain:
plainDef(): no copy → returns registry def unchanged. No copy for a param → keeps original label. Never crashes, never blanks.- Header label:
node.label ?? EFFECT_COPY[type]?.title ?? registryDef.label— three tiers. - Summary:
SUMMARY[type]?.(params)with a null guard — missing summary renders nothing. - Preset problem:
PRESET_PROBLEM[id] ?? preset.description— falls back gracefully.
Intentional naming asymmetry is well-justified. The rack and preset menu speak plain language. The "Add effect" menu keeps DSP registry names. This is documented in the plan's "Decided" section — the expert's tool stays in signal-processing vocabulary. The test "names an effect for the job it does" confirms the intent.
Hover-audition machinery is well-tested. Tests cover: preview without commit, revert on leave, no revert after apply, revert on unmount, survival through re-render (the playhead-tick stability bug via previewRef indirection), stale levelling measurement dropped, cross-neighbour cancel. The auditionRun counter pattern for stale results is correctly designed — increment on every gesture, check after every async boundary.
auditionBase ref + cleanup interaction is the trickiest part: the empty-deps cleanup captures previewRef.current to avoid re-render teardown, while auditionBase.current holds the pre-audition chain. The applyPreset/addEffect paths null out auditionBase.current to prevent the revert from undoing the write. Correct.
No blocking issues.
Review by Miga
🤖 Generated with Claude Code
36bddd2 to
2e544eb
Compare
5b5c70c to
4328674
Compare
2e544eb to
fd0def0
Compare
4328674 to
48bc17c
Compare
fd0def0 to
ae0663c
Compare
48bc17c to
370fca8
Compare
ae0663c to
ba507d7
Compare
370fca8 to
77a4171
Compare
…Flat CI caught it on PR #3026 (wa-12-panel-params); a later refactor in the stack removed the last use of the type here without removing the import.
# Conflicts: # packages/studio/src/components/editor/propertyPanelAudioFxGroup.test.tsx # packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # skills-manifest.json # skills/hyperframes-audio/SKILL.md # skills/hyperframes-audio/references/attributes.md # skills/hyperframes-audio/scripts/carve.mjs
Fallow audit reportFound 23 findings. Duplication (18)
Health (5)
Generated by fallow. |
3 participants
Wires the plain-language layer into the rack, to three UX decisions in
plans/audio-fx-ux/README.md§Decided.The plain name replaces the DSP name, which moves inside as
Details — High-pass. Every knob is renamed too —plainDefwrites the copy over the registry's labels while leaving range, step, unit and automatability alone, because those are facts and only the words were ever the problem.Presets read as the complaint they answer — "My voice sounds boomy", with the name underneath, which is also how the name gets learned.
Hover-audition. Four things have to hold, each with a test that fails without it: leaving reverts, applying does not revert (the audition WAS the preset), the panel unmounting reverts, and moving between two entries in a shelf is not leaving it.
Two of those four were bugs caught by review rather than tests — a static mount never re-renders and never moves the pointer.
🤖 Generated with Claude Code