feat(core): the audio group model — element, membership, helpers - #3278
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
The member-held model and document-order resolver are a good foundation: resolveAudioGroups recomputes from live DOM state, tolerates a missing metadata element, and the package subpath is wired consistently. Two model-contract gaps need closing before downstream routing/UI build on it.
-
blocker — the advertised “non-rendering” group element is not non-rendering.
audioGroups.ts:12introduces<hf-audio-group>as a normal unknown custom element, but nothing gives itdisplay:none/hiddensemantics. Empty inline content is not equivalent to being absent from layout: inside a flex/grid composition root it is an extra item (and therefore adds a gap, affects placement/alignment and:nth-childselectors); in inline formatting it can also affect the line box. The PR explicitly skipped the execution plan'sdisplay:nonerequirement, and the lint smoke cannot detect this visual/layout effect. Enforce inertness universally (runtime stylesheet/custom-element registration, or a required hidden representation with a contract test) rather than relying on “empty childless” markup. -
blocker — the two public membership readers disagree on what can be a member.
resolveAudioGroupscorrectly restricts v1 membership toaudio[...]and documents that video is ignored (audioGroups.ts:24-34), butaudioGroupOfat:54-58returns the attribute from any non-hf-audio-groupelement. SoaudioGroupOf(<video data-audio-group="voiceover">)returns"voiceover"while the same DOM resolves to no group; an empty attribute also returns""despite the function's “or null” contract and the resolver ignoring it. Make this helper enforce the same audio-only/non-empty membership predicate and add the mirror negative tests.
CI at the exact head is green; these are contract failures not surfaced by the current eight pure-DOM tests.
Verdict: REQUEST CHANGES
Reasoning: the metadata element can change authored layout despite being specified as non-rendering, and the exported helpers give contradictory answers for the same video/empty membership shapes.
— Magi
cd95622 to
e470beb
Compare
Chipmunk, Giant, and Monster ship as presets on the pitchshift worklet P1 added: Chipmunk pitches up and adds sparkle, Giant pitches down with weight and a compressor to hold the extra low end together, Monster pitches down further with saturation growl and a close, tight reverb. Every param verified against the live effect registry rather than sketched — the compressor/reverb/saturate/shelf keys all match exactly. Each gets its own title treatment (font, size, tracking, hue) so the FX rack's per-preset styling coverage and hue-distance/background-uniqueness tests extend cleanly to the three new entries, and complaint-line copy in the non-voice vocabulary the audit test enforces (no speech words — "Giant" over CapCut's "Deep Voice", as the design doc records). Updates plans/audio-fx-presets.md's two limits paragraphs to record that pitch shift landed and this half of the character list now ships; Robot and Alien stay out of scope (ring modulation, still unbuilt). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
e470beb to
ba7500a
Compare
c8e87cd to
fc4b410
Compare
Introduces <hf-audio-group> and data-audio-group as the group model B2–B7 and C1 build on: a non-rendering group element carries a label and (later) an FX chain, membership lives on the member's own data-audio-group attribute rather than DOM nesting, so a track removed from the document simply drops out of the group on the next resolve — nothing dangles. Groups do not nest: data-audio-group on the group element itself is ignored. A group with members but no <hf-audio-group> element still resolves, label falling back to the id, so hand-authored HTML degrades gracefully. Audio only in v1 — video members are ignored. Parse-only: nothing routes or sums audio yet (B3/B4). Adds the audio-groups canary at percentage: 0 gating the future Studio UI; the element and attribute parse and play regardless of enrollment. Verified rather than assumed per this plan's standing rule: the timeline's clip-collection selector ([data-start], [data-track-index], [data-composition-id], video, audio, img) already excludes the group element with zero changes, and no lint rule flags unknown elements or data-* attributes, so neither needed touching — confirmed by grep and by running `hyperframes lint` against a fixture containing the element (0 findings referencing it). The step doc's suggested display:none injection point (an existing base stylesheet in the runtime) does not exist in this codebase; skipped rather than inventing new infrastructure, since an empty, childless custom element already renders as a zero-size inline box with no visible output — the same reasoning the lint check above confirms empirically. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fc4b410 to
6c63143
Compare
miga-heygen
left a comment
There was a problem hiding this comment.
Review
Approve. Independent read at exact head. Sound foundation for the 6 PRs above it that I already approved.
Group model: Clean DOM-centric design — membership held by the member (data-audio-group pointing at a group id), not by nesting, so a removed track simply disappears on re-resolve. HfAudioGroup interface is minimal (id, label, memberIds). The module is correctly scoped as parse-only.
Consistency invariant: Both resolveAudioGroups and audioGroupOf derive from MEMBER_SELECTOR, so the two readers can never disagree about a given element. The docstring explains the real bug this fixed (preview routing a track the export would never group because video elements were handled inconsistently). Five edge cases pinned: absent group element, video exclusion, empty attribute, group-element-is-not-a-member, DOM removal.
Inert style: !important is justified — the test proves a #voiceover{display:flex} author rule (higher specificity) would override without it. Injection is idempotent and runs at init before timeline bind.
Character presets: chipmunk/giant/monster follow the existing preset pattern in all three maps (presets, copy, style). Plans doc honestly updated on what landed and what's still out of scope.
Canary: audio-groups at 0% gates Studio UI only; parse+play works regardless.
No issues. Ship it.
— Miga
Twelve of the stack's feature commits landed on main as squashes (#3274 through #3292, plus #3401's canary removal); the 96 review-and-fix commits that followed them here did not, and main moved 64 commits on in the meantime. This reconciles the two. 58 files conflicted. 44 were audio-only — main's side there is the squashed form of commits this branch already carries and has since superseded, so the branch side stands. The rest needed real work, in both directions: **Taken from main, absent here.** - `ensureAudioGroupInertStyle` (#3278's review). An `<hf-audio-group>` is an unknown custom element, so it still takes a flex/grid slot and can open a line box — adding a group shifted authored layout. The helper and its `init.ts` call never came back to the branch, and this branch is what emits the element. - `#3383`'s ended-audio replay: `canSeekEndedMediaBackward` and its five siblings in `media.ts`, with all six tests. Not present here in any form. - `#3380`'s `asetpts=N/SR/TB` between `apad` and `atrim`. Also applied to `mixGroupMembers`, the group submix, which is new on this branch and so had the same bug in a path main's fix could not reach: delayed members padded then amix'd, where a group of four or more silently loses one. - `#3401`'s `displayNumber` thread. The header derives its row from the group-aware order and the undo label from ascending element keys, so once a group exists the same click said "Hide track 2" and recorded "Hide track 1". - `#3413`/`#3421`'s viewport handling — the popover's height cap and `inset()`, and `resolveFloatingPanelPosition` for the grouping dialog, which lives in a track header at the bottom of the window. - Two extractions this branch had inline and at exactly the 600-line cap: `useTimelineDeleteOps` and `editingModeSlice`. Bodies were identical. **Kept from the branch, against main.** Mute and solo are gone by deliberate breaking change (`remove mute and solo from tracks and groups`, `remove the group volume slider and level meter`), so eight files main still carries are deleted again, `PlayerControls` keeps no `previewIframeRef` (it existed only to feed `SoloBanner`), the group-levels branch comes out of main's new `previewMessageRouter`, and `STRIP_H` goes with the bus strip it sized. Main's `TimelineTrackPlainHeader.test.tsx` is rewritten against the control that actually exists — the visibility eye, withheld from an audible audio row and offered back once hidden, which is the only way out of `data-hidden`. **Unioned.** `TimelineFxPopover` — main's positioning, this branch's audition telemetry (`auditionPresetChain`, `storedChain`, `onAuditionTracked`); `SKILL.md` — main's #3416 "keep the carve group a voice group" beside this branch's bus section, with the canary paragraph dropped since the canaries no longer exist. Every port is mutation-checked. core 2508, studio 4460, lint 528, engine 1630, cli 2813, sdk 549, producer green; tsc, oxlint, oxfmt, fallow and the 600-line cap clean.
Summary
<hf-audio-group>+data-audio-group: a non-rendering group element and member-held membership attribute, parse-only foundation for B2–B7 (routing, UI, mute/solo, carve, bus strip) and C1 (timeline FX entry).packages/core/src/audioGroups.ts:resolveAudioGroups(root)andaudioGroupOf(el), pure DOM reads, no state. Groups don't nest (self-referentialdata-audio-groupignored); a group with members but no element still resolves (label = id); removing a member from the DOM drops it on next resolve../audio-groupspackage subpath (mirrors./audio-carve).audio-groupscanary atpercentage: 0,sunsetAfter: 2027-01-15— gates the future Studio UI only.handleDomAttributeLiveCommit) verified to set/removedata-audio-grouplike any other data attribute (new test inuseDomEditCommits.test.tsx).Deviations from the step doc (verified, not guessed — see commit message)
display:noneinjection point (an existing runtime base stylesheet) doesn't exist in this codebase — grepped, confirmed absent. Skipped rather than inventing new infrastructure: an empty, childless custom element already renders as a zero-size inline box, andhyperframes lintagainst a fixture with the element confirms it produces no visible/lint findings.data-*attributes — confirmed by grep, added nothing per the step's own instruction.data-start/data-track-index/data-composition-id, notvideo/audio/img) — confirmed by readingtimeline.ts, zero changes needed.Depends on
Stacked on #3277 (P2) → #3276 (P1) → #3275 (A2) → #3274 (A1). Should merge after all four.
Test plan
bun run buildcleanpackages/coretest: 2352/2352 (newaudioGroups.test.ts: round-trip, group-absent, no-nesting, dangling-member, video-ignored)packages/studiotest: 4245/4245 (new attribute set/remove test)hyperframes linton a fixture containing<hf-audio-group>— 0 findings reference the element or its attributes