feat(studio): fold a preset shut, and give each one its own title design - #3191
Conversation
83f09a3 to
7eb2dcb
Compare
1164623 to
0b032ca
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict: LGTM_WITH_NITS — CI: Preflight (oxfmt --check) is the only real failure; player-perf/preview-regression/regression are its transitive dependents (per repo GHA pattern where perf-shards="skipped" cascades to workflow FAIL). Drift list is 10 files; ~6 match the standing stack-wide docs drift on master, but 4 are files this PR touches or adds — plans/audio-fx-ux/README.md, skills/hyperframes-audio/SKILL.md, references/attributes.md, and the newly added references/presets.md. A bun run format on those before merge resolves it. TS/TSX code is clean.
Empirical claim verification (at 7443ac2):
- "Collapse, view-local":
propertyPanelFxSection.tsx:697useState<ReadonlySet<string>>(new Set())— not on chain, no parse/serialize impact. Key is${run.preset}-${firstIndex}so a reorder survives. - "Open by default":
data-collapsed={collapsed ? "" : undefined}at:727+aria-expanded={!collapsed}at:746, initial empty Set → verified. - "Node count when folded":
.hf-fx-preset-run-countat:767,{run.items.length}. - "Whole-preset controls stay reachable while folded":
{collapsed ? null : rows}gates only the innerrows; the toggle+×+FxParamRow sit outside → asserted by test:1770. - "Every stack ends in a generic keyword, at least two fallbacks": test
:1293-1305— pattern/(sans-serif|serif|monospace|fantasy|cursive|ui-monospace)\s*$/+.split(",").length > 2. - "Every hue ≥20° clear of the accent (160)": test
:1334-1342, min circular distance. - "Backgrounds derived, not picked":
fxTintWashatpropertyPanelFxPresetStyle.ts:1605-1615— preserves hue, snaps to22%, 11%. Character-family bg uniqueness test:1362-1364. - "Decimal HSL support (fxFamilyTint emits
62.0%)": regex\d+(?:\.\d+)?at:1609+ explicit test:1385. Real bug this fix caught.
OLD-assumption counterfactuals (per new test):
audioFxGraph.test.ts"rebuilds…across preset boundary" — OLDshapeOflacked%${fromPreset}, so type-identical reorder took update path →.update()===truewhere new expectsfalse.- "gives every run its own blend" — OLD
presetTargets[p.id] = mixTargets(...)overwrote; split preset yielded 2 params where new (??= []; push) yields 4. - "amount into each run of a split preset" — OLD
presets.find(p => p.id === run.preset)returned first wrap only; second leg stays full-strength → wets≠0 or drys≠1. audioFxCopy.test.ts4× SPEECH audits — fail under old "Thins the voice out", "Some words are much louder", "Cuts quiet words too", "chest", "the body of a voice", "consonants, intelligibility". Non-vacuity guard at:198.audioFxJobs.test.ts— fails under "Too much chest" / "Words are hard to make out".audioFxProfiles.test.tspiecewise round-trip — OLD linear inverse: 0.5 in → size 0.55 stored → 0.46 back. New tolerance 0.01 (wastoBeCloseTo(s, 1)= 0.05) catches it; explicitparams.size === 0.55pin at:501.media.test.ts— OLD[^;)]stopped at)closingfadeTime(2); new depth-counted scan crosses it. Test asserts finding IS raised for{ duration: fadeTime(2), volume: 0.2 }.propertyPanelAudioFxGroup.test.tsx"levels the clip window" — OLDlevellingResult(chain, audio.samples, …)measured file 0-8s (mostly loud); new slices viamedia-start=2, duration=6and t=0.5 sits in quiet zone. OLD →v < 4.- "removes every lane a preset owned" — OLD per-node loop, last-write-wins;
n1.frequencywould survive. Also thefx.preset.telephonelane test — OLD never touched it. propertyPanelFxSection.test.tsx"drops voice shelf on music/sfx" — OLD had no trackKind prop wired; Voice family always present.- Fold tests — OLD had no
.hf-fx-preset-run-caret/count/togglestructure; label was a<span>, not a button witharia-expanded. - Title-treatment test — OLD
.hf-fx-preset-run-labelhadfont-mono text-[9px]uniformly.
Cross-PR coupling:
- vs #3184 audioFxCopy:
doofus-worbleentry added toPRESET_PROBLEM(audioFxCopy.ts:328); coverage test ataudioFxCopy.test.ts:41(!PRESET_PROBLEM[p.id]) would catch a missing entry. Wired cleanly. - vs #3187 family lettering: preset title design is orthogonal — family tint is unchanged; carve module imports the shared
fxTintWash(new) alongsidefxFamilyTint. Nests cleanly. - vs #3189 preset off/ramp:
removeRun(items, presetId)now clears thefx.preset.<id>lane in the same batched write — direct improvement to #3189's amount-lane hygiene (test atgroup.test.tsx:731). Fold state does NOT hide the on/off toggle or the amountFxParamRow— verified by test atsection.test.tsx:1770. - Scope-contained: yes. The +1433/-130 breaks down as fold+title (studio ~500), Doofus Worble preset (core ~15), character-preset resonance rebalance (~50), seven-defects fix (core ~50 + studio ~50), copy audit (core ~30), voice-hide-on-music (~30), WCAG panel-text-4→2 (~40 across 6 files), docs (~250). Each commit has an isolated falsifying test.
Nits:
runKey = ${preset}-${firstIndex}: index is stable across a reorder that preserves the split's first slot, but a preset split→remerge→resplit sequence could reuse a key across topologically different runs. Not a bug for the reachable UI paths, but worth noting if fold-state ever gains persistence.- The new SPEECH audit tests overlap in shape across audioFxCopy/Jobs/Profiles (nearly identical regex/loop). A shared helper would cut duplication.
fxTintWashreturnshsl(H, 22%, 11%)with literal 22/11; if the wash ever needs tuning per-family, the two magic numbers become the extension seam.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat(studio): fold a preset shut, and give each one its own title design — #3191
Verdict: LGTM
The biggest PR in the batch at 1433 lines, bundling ~10 concerns (fold/title, graph boundary fix, copy de-voicing, profile inverse fix, lint paren-depth fix, leveller clip-window fix, preset removal orphan fix, shelf hiding, new preset, preset retuning, skill doc). Each is well-done individually.
Fold state is clean. useState<ReadonlySet<string>> in FxSection, keyed by preset id + first node index. Functional updater avoids stale closures. Rendering skips children via conditional null (not CSS hiding). Controls remain reachable while folded (tested).
Title design per preset is excellent. propertyPanelFxPresetStyle.ts maps each preset id to {type, color, family} with a FX_PRESET_STYLE_DEFAULT fallback. fxTintWash derives background from title color (same hue, 22% sat, 11% lightness) — eliminates the class of bug where hand-picked pairs clash. Test coverage is genuinely impressive: catalog completeness (bidirectional), character uniqueness, font stack validity, size bounds, color vibrancy/contrast, hue distance from accent, background derivation consistency.
Two findings worth noting:
-
presets.mdskill doc ships stale copy. Lines 2267-2269 say"Weight": "chest, body, warmth"butaudioFxCopy.tsin this same PR changed those to"body, warmth, low end". The doc contradicts the de-voicing it sits next to. -
wrapIndexwalk inaudioFxGraph.tsintroduces an implicit contract: thepresetsarray and preset runs fromenabledAudioFxNodes(next)must be in the same order. If they ever diverge, all subsequent wraps silently apply to the wrong runs. The architecture enforces this today, but a comment naming the invariant would make the coupling visible.
PR scope observation: the graph boundary fix is a subtle correctness change to shapeOf()/buildFxChain() that deserves its own review focus. Mixing it with copy changes, retuning, a new preset, a lint fix, AND the fold/title feature makes bisection harder if any regresses.
Review by Miga
🤖 Generated with Claude Code
7443ac2 to
86d8d5d
Compare
7eb2dcb to
3972215
Compare
86d8d5d to
eca1927
Compare
3972215 to
0a6bf88
Compare
eca1927 to
f33ed99
Compare
0a6bf88 to
788bf68
Compare
f33ed99 to
9d69919
Compare
788bf68 to
890c0fd
Compare
# 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 29 findings. Duplication (20)
Health (9)
Generated by fallow. |
# Conflicts: # packages/core/src/audio/audioFxGraph.ts # packages/studio/src/components/editor/propertyPanelAudioFxGroup.tsx # packages/studio/src/components/editor/propertyPanelFxNodeRow.tsx # packages/studio/src/components/editor/propertyPanelFxPresetMenu.tsx # packages/studio/src/components/editor/propertyPanelFxSection.tsx # packages/studio/src/player/components/TimelineTrackRow.tsx # plans/audio-automation-lanes/SPEC.md # plans/audio-fx-presets.md # plans/audio-fx-ux/README.md # skills-manifest.json # skills/hyperframes-audio/SKILL.md
Collapse. A preset is one thing the author added; once set, the modules inside are detail. The title is the disclosure, and folded it carries the node count — which is what keeps a collapsed preset reading as a chain rather than one opaque effect.
A title treatment per preset. The rack already letters by family, which says what kind of thing it is; this says which character. Eight named faces with documented fallback chains — the studio has no webfont pipeline, so these are system faces and a test enforces every stack ends in a generic keyword.
Colour. Saturation roughly doubles. Two hues moved rather than brightened: the voice presets sat on the accent's hue (#3CE6AC is hue 160), and saturating them there would have made "Clean Voice" read as automated. A test keeps every title ≥20° clear of the accent. Backgrounds are derived from each title hue rather than picked — nineteen hand-chosen pairs is nineteen chances for one to clash.
Also two preset fixes: Megaphone and AM Radio sounded the same. A log sweep showed both flattened to a dead −19 dB line — the saturation thresholds were low enough that clipping erased the resonances. Every character preset's clipper is backed off, and the ones still alike were separated by what physically distinguishes them. Every pair in the family is now positive where telephone/AM was −5.9 dB.
🤖 Generated with Claude Code