Skip to content

feat(studio): group rows in the timeline, and a split disclosure - #3286

Merged
vanceingalls merged 3 commits into
mainfrom
wa-23b-group-row
Aug 21, 2026
Merged

feat(studio): group rows in the timeline, and a split disclosure#3286
vanceingalls merged 3 commits into
mainfrom
wa-23b-group-row

Conversation

@vanceingalls

Copy link
Copy Markdown
Collaborator

Summary

  • A group renders as its own row with member rows beneath it; disclosure splits into two independent controls: caret shows/hides a group's member rows (structural), shows/hides any row's automation-lane rows.
  • Plain tracks lose their caret (nothing to disclose structurally) and keep only . expandedClipIds keeps its existing keyframe-lane-state job; expandedGroupIds/expandedLaneOwnerIds are new, independent sets.
  • TimelineLogicalRow.level widens 1 | 2 to 1 | 2 | 3 (group / member-under-group / lane), lanes always owner.level + 1.
  • All new UI/behavior is gated behind isCanaryEnabled("audio-groups"); disabled, groups resolves empty and every new code path no-ops.

Architecture note for reviewers

The original step doc assumed a group row could use the group's ID string as its position key in the timeline's row geometry. In practice rowGeometry/keyboard-nav/virtualization are built exclusively from real numeric tracks, so a synthetic string-keyed row would break focus, virtualization, and keyboard nav for that row (getRowIndex would silently return -1).

Per an explicit decision, this widens the row/geometry pipeline so group rows become real first-class rows — implemented via a synthetic fractional numeric anchor key (firstMemberTrack - 0.5, the same fractional-key convention sub-composition expansion already uses) rather than widening the key type to string | number. This achieves the same goal (group rows are real geometry rows, not visual-only overlays) with a smaller, less invasive diff. Group identity (used for expansion state, callbacks) stays the string groupId, carried on a separate field — only the position key is numeric.

Files touched for this: useTimelineTrackDerivations.ts (row re-emission + canary gate), useTimelineTrackLayout.ts (drag-preview insertion respects fractional keys), timelineKeyboardNavigation.ts (row model, level widening, group/track row emission), useTimelineLogicalRows.ts/useTimelineLogicalFocus.ts (threading), TimelineLanes.tsx (render wiring), new TimelineGroupHeader.tsx/TimelineGroupRow.tsx/useTimelineLaneRowIndexes.ts.

Stack

Depends on #3278 (B1), #3277 (P2), #3276 (P1), #3275 (A2), #3274 (A1) — should merge after all of them.

Test plan

  • bun run build clean
  • packages/studio full suite: 4245/4245 passed
  • Targeted: timelineKeyboardNavigation.test.ts, TimelineLanes.test.tsx, useTimelineLogicalRows.test.tsx, Timeline.test.ts
  • bunx oxfmt / bunx oxlint clean
  • fallow audit --base origin/main --fail-on-issues: complexity passes (0 new above-threshold findings introduced by this diff; 2 remaining are pre-existing in untouched audioFx.ts)
  • Manual: the four disclosure states from the design doc, reproduced in hyperframes preview with ?hf_canary_audio_groups=on

🤖 Generated with Claude Code

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>

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Approve. Independent read at exact head.

Clean group-row integration into the timeline. The two-level disclosure model (caret for structural member visibility, for automation lanes) is correctly separated — each has its own state set (expandedGroupIds / expandedLaneOwnerIds) and toggler, so they can't interfere. The isRowOpen union correctly lets either the legacy caret or the new button open lanes. Group anchor keys use the fractional-key convention (member - 0.5) that sub-composition expansion already uses, and the useDisplayTrackOrder insertion-sort correctly preserves fractional keys to avoid undoing group layout on drag.

The buildTimelineLogicalRows refactor splits cleanly into emitTrack/emitGroup with correct level assignment (ungrouped track = 1, grouped track = 2, their lanes = one deeper). Group keyboard nav identity via timelineGroupRowId is properly integrated. All existing tests updated for the new group-aware input shape.

No issues. Ship it.

— Miga

miga-heygen
miga-heygen previously approved these changes Aug 21, 2026

@miga-heygen miga-heygen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Approve. Independent read at current head.

Group row rendering is correct: fractional anchor key (trackNum - 0.5) follows the same convention sub-composition expansion uses. Split disclosure with distinct glyphs (caret for structural member toggle, for automation lanes) is the right call — prevents keyboard and visual confusion between the two scopes. Keyboard navigation correctly models the 3-level hierarchy (group → track → lane) with level: 1 | 2 | 3.

expandedGroupIds and expandedLaneOwnerIds are clean separate zustand slices with immutable toggle semantics. The preview-track insertion fix preserves fractional anchor keys during drag (doesn't re-sort numerically, which would break grouping). Canary-gated behind isCanaryEnabled("audio-groups").

Good housekeeping extractions: useAdjustedBeatAnalysis, useTimelineMultiDragActorWindows, useTimelineLaneRowIndexes — each stands on its own and reduces Timeline.tsx's inline complexity.

No issues. Ship it.

— Miga

terencecho
terencecho previously approved these changes Aug 21, 2026

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concur with Miga at head. Independent verification adds:

Gate-off byte-identity trace. With isCanaryEnabled("audio-groups")=false:

  • useTimelineTrackDerivations short-circuits → groups=[], trackGroupOf=new Map()tracks=rawTracks unchanged
  • buildTimelineLogicalRows: groupByAnchor empty, trackGroupOf.has(key) always false → the group/skip branches never fire, every key falls through to emitTrack(key, 1, null)
  • emitTrack's new isRowOpen(activeId, expandedClipIds, expandedLaneOwnerIds) reduces to expandedClipIds.has(activeId) because expandedLaneOwnerIds has no writers reachable when canary is off (only TimelineGroupHeader.onToggleLanes calls toggleLaneOwnerExpanded, and group headers only render when groupByAnchor.get(trackNum) returns non-null). LayerDisclosureRow's button still routes through onToggleClipExpandedexpandedClipIds, so plain-track lane state is unchanged.

Result: canary-off path is behaviorally byte-identical to prior code, matching the PR description's contract.

Keyspace disjointness in isRowOpen. The two sets are checked against the same activeId string (expandedClipIds.has(activeId) || expandedLaneOwnerIds.has(activeId)), but writers use disjoint identifier spaces: expandedClipIds holds clip element IDs (via LayerDisclosureRow.onToggleClipExpanded), expandedLaneOwnerIds holds group IDs from data-audio-group (via TimelineGroupHeader). Since group headers pass group.id and track lanes never open via a group toggle, cross-contamination requires a user-authored <hf-audio-group id="..."> colliding with a live clip id — not a runtime concern for this canary-gated step, but worth noting for the eventually-GA follow-up if the ID authoring surface widens.

Minor observability nit (non-blocking). groupLabelCache in timelineDOM.ts is a WeakMap<Document, Map<string, string>> — one scan per Document. If a <hf-audio-group data-label="..."> is renamed in-place on a persistent Document (studio hot-edit, not a reload), the cache returns the stale label for subsequent createTimelineElementFromManifestClip calls until the Document is GC'd. Given labels are currently authored (not editable in the timeline UI yet), fine as-is; leaving a marker in case group-label editing lands before the cache is retired.

CI green (all shards SUCCESS at head 69b3e69). Ship.

— Review by tai (pr-review)

vanceingalls and others added 2 commits August 21, 2026 09:21
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>
A group renders as its own row with member rows beneath it, and disclosure
splits into two independent controls: caret shows/hides a group's member
rows (structural), `∿` shows/hides any row's automation-lane rows. Plain
tracks lose their caret (nothing to disclose structurally) and keep only
`∿`. `expandedClipIds` keeps its existing keyframe-lane-state job;
`expandedGroupIds`/`expandedLaneOwnerIds` are new, independent sets.

Groups get a real position in the row/geometry pipeline rather than a
visual-only overlay: `useTimelineTrackDerivations` re-emits a group's member
tracks contiguously under a synthetic fractional anchor key
(firstMember - 0.5, the same fractional-key convention sub-composition
expansion already uses), so `rowGeometry`/keyboard-nav/virtualization treat
a group row as a first-class row without widening their key type away from
number. `TimelineLogicalRow.level` widens `1 | 2` to `1 | 2 | 3` (group /
member-under-group / lane), lanes always `owner.level + 1`.

All of it — grouped row emission, the header, the new expansion state — is
gated behind `isCanaryEnabled("audio-groups")`; disabled, `groups` resolves
empty and every new code path no-ops. `TimelineElement.audioGroup` (+
`audioGroupLabel`, resolved once per document via `resolveAudioGroups` from
B1) is parsed unconditionally, mirroring how `hidden`/`fxChain` already
flow DOM → manifest → TimelineElement — inert without the canary.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Base automatically changed from wa-23a-group-model to main August 21, 2026 16:34
@vanceingalls
vanceingalls dismissed stale reviews from terencecho and miga-heygen August 21, 2026 16:34

The base branch was changed.

@vanceingalls
vanceingalls merged commit acfa7c5 into main Aug 21, 2026
19 checks passed
@vanceingalls
vanceingalls deleted the wa-23b-group-row branch August 21, 2026 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants