refactor(studio): remove inline timeline expansion rows

[miguel-heygen]

Summary

• remove inline expansion rows and automatic keyframe expansion from the Studio timeline
• keep nested elements and sub-compositions as one clip row
• keep audio automation rows and their disclosure controls
• align automation lane labels with the curves they describe

Before

The Studio timeline fixture has an audio clip with an open Volume envelope. The Volume label is rendered at the top of the track row, 48px above its curve.

After

The same Studio fixture at 100% with the Volume label aligned to the envelope curve.

Verification

• The focused timeline header regression asserts the automation label starts at the track lane offset.
• CI Build, Typecheck, lint, format, Fallow, viewport, preview, and Studio tests validate the current head.

[jrusso1020]

Requesting changes — one blocker, reproduced and measured against the real hooks at 214bced7de9ec6934475d4f334288d08df83ce59.

The removal itself reads clean and the header rewrite is a genuine improvement: one layout instead of the isKeyframeLayer fork, and the eye lands on PlainTrackHeader for every row rather than hanging off a hover-gated lane row. The blocker is not in what was deleted — it is in the one behaviour that was meant to survive.

Blocker — four places decide "is this row expanded", and they now disagree on screen

Before this branch they all answered the same question, because toggleRowExpanded wrote every clip key on the row into expandedClipIds and the readers asked elements.some(...). This branch replaces that with toggleLaneOwnerExpanded(owner), which writes one id (TimelineLanes.tsx:284-286), and the four readers did not converge on what to do with a single id:

decides	reads	site
are the lanes rendered?	isAudioTrack and any element on the row is in the set	TimelineLanes.tsx:210-211
how tall is the row?	the active clip (resolveTrackKeyframeClip) is in the set	useTimelineTrackLayout.ts:165-167
what does aria-expanded say?	the active clip is in the set	timelineKeyboardNavigation.ts:157
is the ∿ drawn at all?	groupAutomationLanes(trackElements).length > 0 — no audio gate	TimelineTrackHeader.tsx:178

Nothing re-syncs the set when the selection moves (it is written in exactly two places: TimelineLanes.tsx:118 and :286, both on a toggle). Two reachable consequences follow. Both are measured, by rendering useTimelineTrackLayout and calling buildTimelineLogicalRows at this head — not inferred from the diff.

(a) Selecting a sibling on a shared audio track collapses the row out from under its own lanes

Two audio clips on one track, both carrying envelopes, expandedLaneOwnerIds = {"narration-1"} — the fixture from your own deleted test:

selectedElementId	rowHeights[0]	logicalRow.expanded	lanes mounted
narration-1	192 (TRACK_H 48 + 2 × AUTOMATION_LANE_H 72)	true	yes
narration-2	48	false	yes

The lanes stay mounted across that selection change — the PR's own surviving test asserts exactly that, by node identity, on exactly this fixture: Timeline.test.ts:545, "keeps the automation lanes mounted when the selection moves along the row". So 144px of envelopes stay drawn inside a row that now reserves 48px. TimelineTrackRow sets an explicit height and a borderBottom with no overflow clip, and the following row is absolutely positioned at a top derived from the collapsed heights — so the border lands in the middle of the lanes and the neighbours' geometry (and getTimelineRowFromY hit-testing) is computed against a height the row is not drawing. The treegrid meanwhile emits aria-expanded="false" for a row whose lanes are on screen (TimelineTrackRow.tsx:48).

That invariant had a test, and this branch deletes it. useTimelineTrackLayout.test.ts lost:

it("stays open at the same height when the selection moves to a sibling", () => {
  // Expansion is stored per clip but reads as the row's: asking only about the
  // active clip collapsed the row the moment another was clicked.
  expect(rowHeight("narration-2")).toBe(rowHeight("narration-1"));
});

useTimelineTrackLayout.ts:165 now asks only about the active clip — the thing that comment says collapses the row. Worth being precise about what that test covered: its describe block passes new Map() for the animations, so zero tweens; both its cases are about AUTOMATION_LANE_H on fx.n1.gain and volume. It was never an inline-expansion test. It was the audio-automation row-height test, which is the surface this PR promises to keep.

(b) A video clip carrying an envelope opens a blank gap

groupAutomationLanes is gated on isAudioOrVideoTimelineElement; TimelineLanes' isAudioTrack is els.some(isAudioTimelineElement). A video clip with a volume envelope, measured:

groupAutomationLanes([video]).length = 1   → header disclosable = true, the ∿ is drawn
isAudioTimelineElement(video)        = false → rowExpanded is pinned false
rowHeights[0] while expanded         = 120  (48 + 72) → the row still grows
logicalRow.expandable/expanded       = true/true      → aria says open

Both render gates gate on rowExpanded (TimelineTrackHeader.tsx:340, TimelineLanes.tsx:507), so the click grows the row by one lane height, announces it open, and draws nothing in it. The isAudioTrack && conjunct at TimelineLanes.tsx:211 is new on this branch — isTrackRowExpanded had no such gate.

Fix direction

Pick one question and give it to all four. The cheapest version that restores the old semantics: have the toggles write every element identity on the row (as toggleRowExpanded(keys) did), and have useTimelineTrackLayout.ts:165 and timelineKeyboardNavigation.ts:157 ask elements.some(el => expandedLaneOwnerIds.has(el.key ?? el.id)) instead of testing activeId alone. Then drop isAudioTrack && from rowExpanded, since disclosable — the thing that decides whether the control exists — does not have it. Restoring the deleted stays open at the same height case against expandedLaneOwnerIds is the regression test for (a); (b) needs a video-with-envelope case, which nothing covers today.

2. The deletion ledger records two files as deleted that this branch does not delete

scripts/check-no-main-deletions.mjs gains five ALLOWED_DELETIONS entries; the branch deletes three files. timelineKeyboardNavigation.test.ts and useTimelineTrackLayout.test.ts are registered with the reason "…so its dedicated tests are removed", but both still exist at head — they were emptied (−141 and −135, one line added between them), not removed.

The mechanism is intact and I would not call the two extra entries a weakening: main() filters status D only, so the entries are inert and nothing is being silenced. The problem is what the ledger now asserts. Those two entries are the written-down reason for the removals in this PR, and for useTimelineTrackLayout.test.ts the reason is wrong twice over — the file was not deleted, and what actually left it was the audio-automation height coverage from finding (a), not "the inline expansion row layout feature". The guard's own test (check-no-main-deletions.test.mjs:44) only asserts each entry has a non-empty path and a reason over ten characters, so nothing else will catch it. Please drop the two entries for files that survive, and leave the three real ones.

It is also worth noticing what the guard cannot see: emptying a test file passes it silently. That is by design and I am not asking you to change it here — but it does mean the 2.4:1 test-to-implementation deletion ratio on this branch (≈1,650 test lines against ≈683 implementation lines) went through unexamined, and (a) is what was in the gap.

3. trackStudioKeyframeLaneExpand loses its only caller

Deleting useTimelineClipDisclosure.ts removes the only production emitter of studio_keyframe_lane_expand — both of its callbacks fired it. The replacement, toggleLaneOwnerExpanded, is a bare store action (useTimelineLaneRowIndexes.ts:12 → keyframeSlice.ts:165) with no telemetry. So the ∿ control survives and its adoption signal does not, which is the opposite of the intent: the comment on the function still reads "Adoption signal for the per-clip keyframe-lane caret toggle" and the caret it refers to is the one you kept.

trackStudioExpandedClipEdit is the contrast and it is handled correctly — the function and all five call sites go together. trackStudioKeyframeLaneExpand keeps its definition (events.ts:88) and its unit test (events.test.ts:109), which calls it directly, so the suite stays green over a function nothing invokes. Either wire it into the toggle or delete it with its test.

4. Debris

• TimelinePane.tsx:185 and :225 — two if blocks whose bodies are now empty, each still under a comment describing the telemetry that used to be in them ("Match the sibling handlers: report the telemetry when…"). The predicates are pure, so this is cosmetic, but both should go with their comments.
• keyframeSlice.ts:91-92 — two orphaned doc comments ("Clips whose keyframe property lanes are expanded", "Union-expand clips…") left above the fields that replaced them.
• timelineKeyboardNavigation.ts:73 — expandedLaneOwnerIds became optional with a new Set() default. The only production caller always passes it; making it optional trades a compile error for a silent "nothing is expanded" in exactly the code path above. Worth keeping required.
• TimelineTrackHeader.tsx:354 getTimelineLaneTop(0) and TimelineLanes.tsx:516 laneCount={0} are now constants threaded through parameters that can only be zero.

Verified, no action needed

Recording these so nobody re-derives them:

• The PR body's testing claim holds. "tests retain the nested one-row regression and audio automation behavior" — useTimelineLogicalRows.test.tsx gains "keeps a nested element in one row as the playhead crosses it", and the rewritten TimelineTrackHeader.test.tsx covers envelope rows rendering, removing, hiding while collapsed, and the shared-track naming case. Both halves are real.
• resolveTrackKeyframeClip still counts automation (automationLaneCountOf + laneCounts), so an audio clip with envelopes and no tweens still resolves and still gets a caret. Removing property lanes did not re-open the bug its comment describes — this is the obvious thing to suspect here and it is not broken.
• expandedParentStart is still live, with readers in timelineGaps, timelineZMirror, timelineDragLanding, timelineZones, gsapShared and razorSplitTransaction. Removing the inline rows did not strand the sub-comp rebasing in TimelinePane — only its telemetry went.
• gutterFill's de-export is safe — no importer outside TimelineTrackHeader.tsx.
• The track branch emitting no child rows while expanded is true is pre-existing, not new here: the old emitTrack looped over getTimelinePropertyLanes(...), which is empty for an automation-only track, so a track's automation rows were never in the logical model either way.

— Rames

[jrusso1020]

Re-review at 738926f63e7c388df816f5b4fe5f76e0f762d5c5. Both prior blockers are fixed, and the fix for them is the right shape — one predicate, isTimelineRowExpanded (useTimelineTrackLayout.ts:97), handed to all four readers instead of three different questions. Requesting changes only because the debris cleanup in the same commit moved every automation-lane label 48px up the gutter.

Prior blockers — resolved

(a) Four readers disagreeing on "is this row expanded". Fixed, and fixed on the case that actually broke rather than the one the new test uses. Rendering useTimelineTrackLayout and buildTimelineLogicalRows at this head, with the partial set — only narration-1 expanded, narration-2 selected, which is the fixture that produced 48-vs-192 last round:

rowHeights[0]                  120   (TRACK_H 48 + 1 x AUTOMATION_LANE_H 72)
logicalRow.expandable/expanded true / true

All three now answer from the same elements.some(...), and isTimelineRowExpanded's first conjunct is groupAutomationLanes(elements).length > 0 — the same expression disclosable is built from (TimelineTrackHeader.tsx:178, over the same els/trackElements, TimelineLanes.tsx:275). So expanded ⊆ disclosable holds by construction now, which is what I was asking for.

Two things I checked because they're the way this kind of fix usually leaks, and both are clean:

• The identity spaces match. elementId() (timelineKeyboardNavigation.ts:85), getTimelineElementIdentity, and isTimelineRowExpanded are all element.key ?? element.id. The keyboard onToggleRow path writes ids the row-height reader can actually find.
• toggleLaneOwnerExpanded(ids) converges a split row. shouldExpand = ids.some(id => !next.has(id)) (keyframeSlice.ts:162) means a row holding one expanded and one collapsed clip expands rather than half-collapsing.

(b) Video clip with an envelope opening a blank gap. Fixed — the isAudioTrack && conjunct is gone and isAudioTrack is still live for the header layout (TimelineLanes.tsx:202, :279). Your new renders an expanded envelope on a video track is the regression test for it and it passes.

2. Deletion ledger. Both entries for files that survive are gone; the three real ones remain. 3. trackStudioKeyframeLaneExpand. Deleted with its test — no references left anywhere in the repo. 4. Debris. Empty if blocks, orphaned doc comments, and the optional expandedLaneOwnerIds are all cleaned.

Blocker — the lane labels moved 48px up, off their curves

TimelineTrackHeader.tsx:354:

- top={getTimelineLaneTop(0) + index * AUTOMATION_LANE_H}
+ top={index * AUTOMATION_LANE_H}

getTimelineLaneTop(0) is not 0. It is TRACK_H + 0 * LANE_H = 48 (timelineLayout.ts:53). My nit was about laneCount={0} — a parameter that could only be zero — and getTimelineLaneTop(0) is a constant too, but its value is TRACK_H. Dropping the term deleted the offset that puts the labels below the track's name line.

Measured at this head, one audio clip with two envelopes, rendering TimelineLanes and reading the authored tops off both columns of the same row:

lane	gutter label (data-timeline-lane-top)	canvas curve (.hf-automation-lane top)
0 — "Peaking EQ 1 kHz · Q"	0px	48px
1 — "Volume"	72px	120px

Two consequences, both on screen:

• Label 0 is drawn over the track name and its controls. That row owns exactly height: TRACK_H in normal flow (TimelineTrackHeader.tsx:271) and the lane labels are absolute inside the sticky header (:240) — which is what the comment at :260 is about: "opening one pushed the name and its controls down THROUGH the lane rows below, which are absolutely positioned from the top."
• "Volume" now sits over the Q curve. The comment three lines above the change says it: "the two have to agree or a name labels the wrong curve." The canvas side is unchanged — topOffset ?? getTimelineLaneTop(laneCount) with laneCount defaulting to 0 (TimelineAutomationLaneSlot.tsx:189), so it still starts at 48.

The same file contradicts itself now: the keyframe lane header rows still use top: getTimelineLaneTop(laneIndex) and start at 48 (trackHeaderLabelRows.tsx:125), while AutomationLaneHeaderRow (:188, same file, same absolute left-0 shape) starts at 0.

Fix is top={TRACK_H + index * AUTOMATION_LANE_H} — or just keep getTimelineLaneTop(0); the indirection was the only thing I was objecting to, not the value.

Nothing in the suite catches this. Every automation-label assertion is toHaveLength — TimelineTrackHeader.test.tsx:91/:104, TimelineLanes.test.tsx:267/:278/:295. A single expect(label.getAttribute("data-timeline-lane-top")).toBe(String(TRACK_H)) would have. TimelineAutomationLaneSlot.test.tsx pins its side (ROW_0 = getTimelineLaneTop(0), :131); the gutter side has no equivalent, which is exactly the asymmetry that let this through.

Nit — the restored row-height test can no longer fail

useTimelineTrackLayout.test.ts, stays open when selection moves to a sibling on the same track: useTimelineTrackLayout no longer takes selectedElementId/selectedElementIds, and useTimelineRowHeights reads only expandedLaneOwnerIds from the store. So re-rendering after setState({selectedElementId: "narration-2"}) cannot change rowHeights[0] — the assertion is true by construction, not by behaviour. Seeding both ids makes it weaker still.

Seed new Set(["narration-1"]) with the selection on narration-2 instead. That is the state the old code got wrong, it is what I measured above (120, not 48), and it stays falsifiable if the reader ever goes back to asking about the active clip.

Verified, no action needed

• laneCount going optional is behaviour-preserving. Both dropped laneCount={0} sites land on the = 0 default and the same getTimelineLaneTop(0) they had before. TimelineGroupRow.tsx:199 keeps its topOffset={TRACK_H}, which overrides it anyway.
• The Timeline.test.ts:555 fixture widening is not a weakening. {"narration-2"} and {"narration-1","narration-2"} both read as expanded under some(); the wider one just matches what a real toggle now writes.
• Neither formerly-emptied test file is a shell. timelineKeyboardNavigation.test.ts has 5 cases and useTimelineTrackLayout.test.ts has 10 at this head.

What I ran

Eight studio suites in a worktree at this head — useTimelineTrackLayout, timelineKeyboardNavigation, TimelineTrackHeader, TimelineAutomationLaneSlot, TimelineLanes, Timeline, useTimelineLogicalRows, telemetry/events — 128 passed, including both of your new cases. The two measurements above come from probes rendering the real components in that same rig, deleted afterwards. I did not treat CI as a gate.

— Rames

[jrusso1020]

Re-reviewed at af9b917950978b6551f3fee72623310569efc3d0. Two commits since the head I blocked: 47f99049 (the label fix) and af9b9179 (the import removal — that one is correct, AUTOMATION_LANE_H's only other appearance in the file is prose inside the comment at :261).

The 48px base is back and lane 0 is now exactly right. The stride isn't.

getTimelineLaneTop steps by LANE_H — the keyframe lane height, 28px (timelineLayout.ts:8, :53). Automation lanes are AUTOMATION_LANE_H tall, 72px (automationLaneHeight.ts:15). So top={getTimelineLaneTop(index)} (TimelineTrackHeader.tsx:353) produces 48, 76, 104 where the curves are at 48, 120, 192.

Measured at this head, same probe and fixture as last round — one audio clip with fx.n1.q + volume, rendered through TimelineLanes, reading style.top off both columns:

lane	gutter label	canvas curve
0 — Peaking EQ 1 kHz · Q	48px	48px	fixed
1 — Volume	76px	120px	44px off

It compounds with lane count: lane 2 is 104 vs 192, lane 3 is 132 vs 264. And since trackHeaderLabelRows.tsx:238 gives each of those rows height: AUTOMATION_LANE_H, the label rows are 72px tall stacked 28px apart — they overlap each other by 44px while the curves they name march away down the canvas. The same component says the row is 72 tall (:238) and places it as if it were 28 (TimelineTrackHeader.tsx:353).

Three places already agree on the right expression, which is what makes this mechanical rather than a judgement call:

• TimelineAutomationLaneSlot.tsx:93 — topPx={top + rowIndex * AUTOMATION_LANE_H} — the canvas, i.e. the thing being aligned to.
• TimelineGroupLaneLabels.tsx:69 — top: top + index * AUTOMATION_LANE_H — the group label column, the direct sibling of this code doing this exact job.
• useTimelineTrackLayout.ts:159 — TRACK_H + trackAutomationLaneCount(elements) * AUTOMATION_LANE_H — the height the row reserves for these lanes.

One line:

- top={getTimelineLaneTop(index)}
+ top={TRACK_H + index * AUTOMATION_LANE_H}

TRACK_H is already imported at :17. This does put the AUTOMATION_LANE_H import back, so af9b9179 comes back with it — that commit was right for the line as written, just not for the line that's correct.

Worth saying plainly, since this is round three on two lines: the expression the PR started with — getTimelineLaneTop(0) + index * AUTOMATION_LANE_H — was correct. My round-1 nit called getTimelineLaneTop(0) a constant threaded through a parameter that can only be zero, which is true of its shape and false of its value: it is TRACK_H, 48. Each rewrite since has kept one half of the original and dropped the other — round 2 kept the 72 stride and lost the 48 base, round 3 kept the 48 base and lost the 72 stride. TRACK_H + index * AUTOMATION_LANE_H is the same number the original produced.

Optional, if you'd rather this stop recurring: the y of automation lane N under a track with K keyframe lanes is getTimelineLaneTop(K) + N * AUTOMATION_LANE_H, and it is currently spelled out separately at all four sites above. A getAutomationLaneTop(index, laneCount = 0) living next to AUTOMATION_LANE_H would make the gutter and the canvas unable to drift apart again. Bigger than the one-liner, and not a condition of my approval — your call.

The new test can't see this, for the same reason the old toHaveLength ones couldn't. TimelineTrackHeader.test.tsx:93 pins 48px on a single-lane fixture, so it asserts the base and is silent on the stride. What catches both failure modes is a two-lane fixture comparing the two columns to each other rather than to a literal:

const labels = [...host.querySelectorAll<HTMLElement>("[data-automation-lane-label]")];
const curves = [...host.querySelectorAll<HTMLElement>("[data-automation-lane]")];
expect(labels.map((el) => el.style.top)).toEqual(curves.map((el) => el.style.top));

One line, fails at 76-vs-120 today, and would have failed at 0-vs-48 last round too.

Round 2's fixes are untouched — the whole delta is TimelineTrackHeader.tsx:353 plus that import — so isTimelineRowExpanded and everything I credited as resolved last round still stands. I'm not re-litigating any of it.

What I ran at this head: 7 studio suites — Timeline (80), TimelineLanes (10), useTimelineTrackLayout (10), telemetry/events (10), TimelineAutomationLaneSlot (6), TimelineTrackHeader (5), timelineKeyboardNavigation (5). 126 passed, including the new 48px assertion. Green suite, live bug — which is exactly the gap the assertion above closes.

Per the standing rule I didn't treat CI as a gate; this is a code finding backed by the measurement, not a red check.

— Rames