feat(core): land the plain-language layer, and test that it covers the rack - #3184
Conversation
a0e3a75 to
ba14a9b
Compare
aa53c6d to
5b5c70c
Compare
terencecho
left a comment
There was a problem hiding this comment.
LGTM pending Preflight green + one non-blocking nit. Coverage test is genuinely load-bearing on the effect-copy completeness invariant.
Copy layer at packages/core/src/audioFxCopy.ts (new, 375L). Interfaces: Ends { low, high }, ParamCopy { label, hint?, ends? }, EffectCopy { title, does, reachFor, primary, primaryEnds, params, band? }. Main table: EFFECT_COPY: Record<string, EffectCopy> at :54 — 15 entries (gain, highpass, lowpass, peaking, lowshelf, highshelf, compressor, limiter, gate, saturate, bitcrush, delay, reverb, chorus, phaser). Companions: BANDS (7 non-overlapping 20-20 000 Hz ranges), PRESET_PROBLEM (18 preset-id → problem-statement), SUMMARY: Record<string, (p: P) => string> (closed-state summarizer per effect). Deliberate exclusions per module header: EQ tone module + levelling live at audioEqSummary / levellingSummary (chain-reading summaries, not table-driven).
Coverage test at packages/core/src/audioFxCopy.test.ts (75L). Enumeration is RUNTIME import of HF_AUDIO_FX + for (const def of HF_AUDIO_FX) at :17 — not a hardcoded list, not compile-time-only. Assertions per effect:
EFFECT_COPY[def.id]defined.- Per-param
copy.params[param.key]defined. - Primary-key coherence: if
copy.primary !== "strength", must be one ofdef.params.map(p => p.key)— prevents pointing the headline knob at a phantom parameter. SUMMARY[def.id]defined.- Every summary rendered at
defaultAudioFxParams(def.id)must not contain"undefined"/"NaN"and match/^[^u].*[^ ]$/(see nit).
Reverse-direction (dead-entry) checks at :44-49: Object.keys(EFFECT_COPY).filter(id => !shipped.has(id)) and same for SUMMARY — both must equal []. Catches removed / renamed effects.
Band-gap invariant at :67-73: BANDS starts at 20, ends at 20 000, and BANDS[i].from === BANDS[i-1].to for all i.
OLD-assumption counterfactual: adding {id: "gate2", params: [...]} to HF_AUDIO_FX WITHOUT touching audioFxCopy.ts — the outer describe loop iterates the new entry, EFFECT_COPY["gate2"] is undefined, expect(copy, "gate2 has no copy").toBeDefined() FAILS. Also SUMMARY[def.id] fails. Test IS load-bearing on the add-forgotten shape.
Export-shape flag for #3192. The exports are Record<string, T> maps + SUMMARY: Record<string, (p: P) => string> — dictionary-shape API, not accessor functions. Downstream #3192 will be reaching in with bracket lookup. See my #3192 review for the coupling verdict: mediated by typed EffectCopy interface, so shape refactors surface as TS breaks not silent runtime bugs.
CI: Preflight red on this PR's own plans/audio-fx-ux/README.md (unformatted per oxfmt --check). Needs bun run format.
Nits:
- No reverse-direction check for
PRESET_PROBLEM(unlikeEFFECT_COPYandSUMMARY) — if a preset is renamed / removed, a deadPRESET_PROBLEMentry passes silently. Mirror the:44-49pattern for presets. SUMMARYregex/^[^u].*[^ ]$/— fragile shorthand for "no undefined prefix"; the actual guard is.not.toContain("undefined")on the next line. Any legit copy starting with lowercaseuwould false-fail; none currently does, but the intent-vs-mechanism gap is worth a comment.- Consider thin accessors (
getEffectCopy(id): EffectCopy | undefined,summariseEffect(id, params): string | undefined) to give #3192 and later consumers a stable-shape API — see coupling discussion on #3192.
— Review by tai (pr-review)
miga-heygen
left a comment
There was a problem hiding this comment.
Review: feat(core): land the plain-language layer — #3184
Verdict: LGTM
Clean data layer with an unusually strong coverage test.
Coverage is exhaustive and bidirectional. The test enforces:
- Forward: every effect in
HF_AUDIO_FX(15 effects) has anEFFECT_COPYentry, aSUMMARYentry, and every param has aparams[key]entry. - Reverse: no orphaned copy for effects that don't ship (catches renames/removals).
- Presets: all 18 presets have
PRESET_PROBLEMentries. - Summary sanity: every function runs at defaults without producing
undefined,NaN, or starting with 'u'. - Spectrum continuity: BANDS contiguous from 20 Hz to 20 kHz, no gaps or overlaps.
Adding a new effect to the registry without updating the copy layer will fail CI — which is exactly the invariant the consuming PR (#3192) depends on.
Copy quality is accurate. Cross-referenced every EFFECT_COPY entry against the actual effect definitions. Gain, EQ, dynamics, nonlinear, and time-based descriptions are all technically correct. The reachFor fields describe the right symptom for each effect.
Export surface for #3192 is clean. #3192 imports exactly EFFECT_COPY, SUMMARY, and PRESET_PROBLEM — all exported with proper subpath registration in both package.json and package-subpaths.json. Via's load-bearing pair concern is structurally correct but low-risk: the coupling is shallow (four named exports read by import) and the test suite is the enforcement mechanism.
Data-only with minimal safe logic. SUMMARY functions are pure formatters over parameter values — n(), hz(), strength() helpers with no state, no DOM access, no side effects. Cannot break at runtime beyond producing an incorrect display string.
No issues found.
Review by Miga
🤖 Generated with Claude Code
5b5c70c to
4328674
Compare
ba14a9b to
dd9ec94
Compare
4328674 to
48bc17c
Compare
dd9ec94 to
ddca11a
Compare
48bc17c to
370fca8
Compare
ddca11a to
325bf65
Compare
370fca8 to
77a4171
Compare
325bf65 to
b2b6d7e
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
copy.mtslived inplans/and was read by one build script. Moving it topackages/core/src/audioFxCopy.tsputs it beside the registry it describes, and turns the coverage into a test: every shipped effect, every parameter, every preset must have copy.That check was a step in a build script, so it only fired when somebody remembered to rebuild a review page. Now it fires on the commit that adds an effect without a plain name for it.
Four assertions the build step never made, each a real hole: copy for an effect the registry no longer ships, a missing
SUMMARY, a summary renderingundefined/NaNat the effect's own defaults, and a gap or overlap in the shared frequency ruler.🤖 Generated with Claude Code