feat(studio): two faces, and the shared frequency ruler - #3186
Conversation
8281023 to
ee878f6
Compare
13f574f to
72210c5
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict: LGTM_WITH_NITS. Two-faces UX and log-spaced shared frequency ruler both verified against the diff; tests earn their weight; CI failure is the stack-wide pre-existing oxfmt drift, not this PR.
Verified claims (file:line at 72210c5):
- Two faces — module opens on name + does + one knob; Details behind disclosure.
packages/studio/src/components/editor/propertyPanelFxNodeRow.tsx:274-296(primary-onlyonlyPrimary,useState(false)fordetails),:323-329(.hf-fx-node-does),:330-360(primary knob +primaryEnds+FxBandRuler),:370-378(Details disclosure,aria-expanded, DSP label moves here). - Five effects with
primary: "strength"open on all controls, no disclosure.primaryParamOfreturns null whenprimaryisn't a registry param key (:33-40);.hf-fx-node-detailsbutton gated onprimary(:370); test assertsEFFECT_COPY.compressor?.primary === "strength"and no.hf-fx-node-details(propertyPanelFxSection.test.tsx:399-405). BANDSreused, not duplicated. Ruler importsaudioBandAt, BANDSfrom@hyperframes/core/audio-fx-copy(propertyPanelFxBandRuler.tsx:19); segments map overBANDS(:49); segment widths use log-scalepositionOf(:23-27).- Ruler bottom segment >10% of bar under log layout.
audioFxCopy.ts:294-300+positionOfgives rumble 20-80Hz ≈ 20% (log10(80/20)/log10(20000/20) = 0.602/3). Linear layout gives 0.3%. Test atpropertyPanelFxSection.test.tsx:365-372asserts>10.
OLD-assumption counterfactuals (new tests):
audioBandAtboundary test (audioFxCopy.test.ts:85-91) — fails an OLDhz > band.from && hz <= band.to(closed-upper). Currenthz >= from && hz < topasses.clamps past both ends(:93-98) — fails an OLDfind-only impl (no clamps); 5Hz and 30000Hz would be nameless.NaN → undefined(:100-102) — fails an OLD impl missing theNumber.isFiniteguard;Math.log10(NaN) = NaNcascade would yield unhelpful behavior.- Rumble >10% (
propertyPanelFxSection.test.tsx:370-372) — fails a linear-scale ruler. - Reorder Details closes (
:298-313) — fails an OLD "expand always" or shared-state disclosure;useStatelocal toFxNodeRow+key={node.id}in parent gives per-effect state. - Compressor no-disclosure (
:390-405) — fails an OLD "primary is any truthy string" impl that would treat"strength"as a real param key.
Cross-PR coupling:
- No new
HfAudioFxNodefield.detailsis ReactuseState— view-local, not persisted. Parse/serialize allow-list untouched. ✓ - Ruler consumes #3184's
BANDSdirectly, no duplication. ✓ FxBandRulerrenders only whencopy?.band && typeof params.frequency === "number"(propertyPanelFxNodeRow.tsx:355) — limiter case correctly excluded, test at:381-386.
Nits (non-blocking):
- Ruler bar has no
role="img"oraria-label— a screen-reader user getsdata-band="Mud"and per-segmenttitletooltips (propertyPanelFxBandRuler.tsx:41,55), but no spoken "acting in Mud (200-500 Hz)" summary. Consideraria-label={Acting in ${here.name}, ${here.says}}on the container. -
FxBandRulerrecomputesMath.log10(HIGH) - Math.log10(LOW)per segment viapositionOfcalls in a.map— cheap but memoizable as a module const alongsideLOW/HIGH.
CI: Preflight FAILURE across all three workflows is the recurring 9-file oxfmt --check drift (README.md, plans/*, skills/hyperframes-audio/*) — none of the drifted files are in this PR's diff. Stack-wide pre-existing signal. No THIS-PR real Preflight issue.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat(studio): two faces, and the shared frequency ruler — #3186
Verdict: LGTM
Frequency ruler is correct. BANDS covers 20Hz-20kHz in seven contiguous non-overlapping ranges. positionOf() uses Math.log10 spacing — industry standard for audio frequency visualization. 20-200Hz gets one-third of the ruler width (matching the ear's perception), verified by the log-spacing test asserting Rumble's width >10% (would be ~0.3% on a linear ruler).
No CDN fonts. All typography uses existing Tailwind utilities and the project's established font stack. Arbitrary sizes (text-[9px], text-[10px]) match the pattern used throughout the studio panel components.
Two-face design is clean: primary view shows one knob + ends + ruler (the outcome face), Details disclosure reveals all params (the mechanism face). The primaryParamOf fallback for "strength" returns null so profiled effects don't split — forward-compatible for when profiles land as a real registry parameter.
Tests cover: ruler renders with correct band, no ruler for non-spectral effects, strength primary fallback, audioBandAt boundary assignment, edge clamping, NaN handling. The log-spacing assertion is a nice proof that the ruler isn't accidentally linear.
aria-expanded on the Details button, aria-hidden="true" on decorative elements, descriptive title tooltips on ruler segments. Adequate accessibility.
Review by Miga
🤖 Generated with Claude Code
72210c5 to
d9da574
Compare
ee878f6 to
d10375c
Compare
d9da574 to
fa461bf
Compare
d10375c to
a97b9b2
Compare
fa461bf to
9afd02d
Compare
a97b9b2 to
f1a2b5b
Compare
9afd02d to
d2934c2
Compare
f1a2b5b to
8388acb
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 19 findings. Duplication (18)
Health (1)
Generated by fallow. |
Two faces. A module opens on its name, a line saying what it is for, and the single control that carries it; everything else is behind a Details disclosure, which is also where the DSP name lives. Ten of fifteen effects — the other five want a derived knob that has not shipped yet, and they open on all their controls until it does.
Worth naming:
peaking's primary is "how much", and that only became coherent in the PR below this one. Two faces is honest for it because the jobs landed first.The ruler.
BANDShas named seven ranges since the copy layer landed and nothing showed them. Log-spaced, because hearing is: rumble is 20–80 Hz, three tenths of one percent of the range linearly and about a fifth of it by ear. A test asserts the bottom segment is over a tenth of the bar, which a linear layout fails.🤖 Generated with Claude Code