feat(studio): wire expanded keyframe timeline lanes

[miguel-heygen]

What

Wires the expanded keyframe lanes into the complete Studio timeline experience, and carries the review-fix commit for the whole Family B stack.

Why

The state, timing, retiming, property-lane, header, and callback layers need one integration boundary that preserves default expansion, manual collapse, selection, seeking, playback follow, and compact timeline layout.

How

Integrates the Family B primitives in Timeline/TimelineLanes, finalizes lane geometry and visual polish, and adds end-to-end component coverage. The cohesive 1,504-line delta is documented as an R9 integration exception because splitting it would create an incompatible intermediate lane contract. This is B8 and the tip of the Family B Graphite stack.

Review fixes folded into the tip commit:

• Per-keyframe ease attribution in TimelinePropertyLanes reads the ease from each keyframe's own source tween, so a merged lane cannot label a segment with a different animation's ease.

• contentOrigin restores the pre-t=0 breathing pad in the all-collapsed case (GUTTER + TRACKS_LEFT_PAD), matching the ruler's folded-pad contract.

• Keyframe toggle buttons use state-specific labels (Add <Group> keyframe / Remove <Group> keyframe) instead of one ambiguous label.

• studio_segment_ease_edit now carries an action field so preview churn is distinguishable from a commit.

• onMoveKeyframe is Promise<boolean> end to end (timelineCallbacks -> TimelineLanes -> TimelinePropertyLanes -> useTimelineEditCallbacks); the retiming commit is where the async contract begins, so the widening lands there rather than behind a shim.

• Track-header property values are sampled along the segment's own ease instead of linearly. Previously a mid-segment readout reported a value the element never has at that time, and an insert from the track header stamped that wrong value onto the new keyframe, deforming the curve.

• Bulk keyframe delete has a composition-level regression test asserting a stale selection
  key from another element is dropped instead of deleting on the active element.

• The ease-segment rows in KeyframeEaseList emit data-ease-segment-pct, so the
  AnimationCard scroll-into-view effect that queries it is no longer a no-op.

Two follow-up fixes from manual QA on this branch:

• usePopulateKeyframeCacheForFile now loads the GSAP cache for every composition file the
  timeline renders rows for, not just the file that owns the current selection. Property
  lanes are populated on project open instead of appearing only after a clip from that
  sub-composition is clicked. The AST-load half moved to hooks/keyframeCacheAstLoad.ts to
  stay under the 600-line file gate; the public helpers are re-exported so no importer moved.
• findActiveExpandableCompositionId falls back to closed-interval containment when the
  strict half-open pass matches nothing. A playhead parked exactly on the timeline's last
  frame is inside no clip window, which collapsed every expanded sub-composition row at the
  end of the video. The strict pass still wins on a shared seam, so a playhead on the
  boundary between two adjacent clips expands the one that is starting.

Regressions surfaced by a full-Studio QA sweep, each replayed against the merge-base
3b3d4f559 to confirm this stack caused it:

• Deleting one keyframe destroyed the whole tween. The lane-header remove toggle escalated a
  flat tween to a whole-animation delete, taking the authored tl.to(...) and its source
  comment with it. It posts remove-keyframe again and lets the writer refuse it, as the base
  build does.
• A keyframed layer could not be hidden. The visibility eye had moved off the always-mounted
  layer row onto a hover-gated property-group row, so it existed only while lanes were
  expanded and the pointer was over that lane, and its label named a track its row did not
  act on. It is back on the layer row.
• A drag from the centre of a clip bar did nothing: the 16px inline ease button sits there
  and swallowed the press. The press now passes through to the clip; only the click is kept,
  and dropped if the pointer travelled.
• Dragging a diamond onto a neighbour discarded the retime. The clamp bounded the dragged
  keyframe by the whole merged row, so two animations colliding at one percentage pinned each
  other. It clamps against the dragged keyframe's own tween instead.
• Dragging a flat tween's boundary diamond did nothing: the handler bailed on
  !anim.keyframes even though the retime resolver already resolves that case to a
  position/duration resize. The empty remap dispatches update-meta, which moves the window
  without rewriting the authored flat tween into keyframes form.
• The property panel's keyframe gutter guessed one animation per property group, so clicking
  a keyframe authored by a sibling tween on a merged row named the wrong tween and the writer
  silently removed nothing. The diamond reports the clicked keyframe's own animationId,
  with the group guess kept as the fallback for cache rows that carry no identity.
• Switching composition leaked the previous file's elements into keyframeCache and
  gsapAnimations, since each file only ever cleared its own entries. Files dropped from the
  scan set are pruned.
• The diamond hit box is floored at 12px (the gap-derived size fell to ~7px at the zoom
  floor) and the tooltip percentage is rounded.

Test plan

• Unit tests added/updated
• Manual testing performed
• Documentation updated (not applicable)

Both follow-up fixes ship with a regression test that was verified to fail with the fix
reverted: usePopulateKeyframeCacheForFile.test.tsx asserts both composition files are
fetched, and useExpandedTimelineElements.test.ts covers the end-of-timeline playhead plus
the shared-seam tie-break. Both fixes were also confirmed in a running Studio against the
keyframe-playground project.

The QA regressions above were each re-verified live in a running Studio on the fixed build:
the mutation now reaches disk and the file changes as expected, and reverting the fix
reproduces the original behaviour on the same fixture. KeyframeNavigationDiamond.test.tsx
and the rewritten flat-boundary case in useTimelineEditCallbacks.test.tsx cover the last
two.

Stack validation on the promoted branches: 2,910 Studio tests pass, every commit typechecks independently, oxlint and oxfmt clean over the 102 changed TS/TSX files, Fallow audit gate clean, file-size gate clean, full workspace build green.

Studio QA in a browser against a GSAP fixture with per-group keyframed tweens: lanes render and auto-expand, clip and element selection agree, segment ease editing round-trips to disk, track-header insert and delete round-trip to disk, drag-to-retime rewrites the keyframe percentage while preserving value and ease, the keyframe context menu works, collapse folds lanes onto the clip bar, comp switching resets timeline state, playback follows. No console errors across the sweep. Eased sampling verified live: on a power2.in segment the readout at 50.6% progress is 12.97, not the linear 50.6.

Not covered

• Keyboard-accessible diamond retiming (Lens 5). Diamonds are still pointer-only.
• Scroll compensation when the 232px label column toggles the content origin; the viewport does not hold its anchor across the flip.
• MotionPathOverlay visibility is deliberately wider than the selection-box gate: it now
  draws for any canvas selection while paused, instead of only while the Inspector panel is
  open. The on-canvas path handles are the arc-drag affordance, so gating them on a side
  panel would make keyframe path editing unreachable from the canvas alone. This is a
  behavior change relative to the base branch and is called out here rather than left silent.
• propertyGroup?: string is left as-is rather than widened to the PropertyGroupName union.
• Seven Figma-motion parity gaps remain open.
• Inserting a keyframe mid-segment still splits one eased segment into two segments that each re-run the original ease, rather than subdividing the curve. The inserted keyframe now lands on the true curve, but the shape between the new endpoints is an approximation.

Deferred review findings

Every blocker and high finding raised on this PR is fixed in the stack. The 3 remaining low/nit findings are parked, verbatim, in .scratch/studio-timeline-family-b/issues/08-pr-2690-deferred-review-findings.md:

• 🟡 packages/studio/src/player/components/Timeline.tsx:120 — First-keyframe layout flip — 232px jump when hasKeyframedClips flips false→true
• 🟡 packages/studio/src/player/components/TimelineLanes.tsx:214 — resolveTrackKeyframeClip called inline in render — sort() per track per frame during playback
• 🟢 packages/studio/src/player/components/TimelineOverlays.tsx:109 — Rest-args switch fixes a real bug but composes with feat(studio): add timeline keyframe retiming interactions #2685 latent handler-drop

R1 review follow-ups

Both high findings (the contentOrigin layout flip without scroll compensation, and the per-track per-render filter and sort in resolveTrackKeyframeClip) were already parked before this pass. They plus the 2 low findings are in .scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md.

R2 review follow-ups

Fixed in this PR:

• The parse response was cast blind before reading .animations, so a proxy or error page answering 200 crashed the cache load. A type guard checks the field the callers actually read, and the fetch's return type is narrowed to that slice.
• The AST cache load re-derived clip-relative rows and the three cache-key variants inline. It now calls the same toClipKeyframes and elementCacheKeys owners as the other writer.
• trackHeights and getTimelineCanvasHeight kept a numeric track-count branch no production caller used once rows gained variable heights. Both branches are gone and the tests pass row arrays.
• The widest keyframed clip was picked with sort(...)[0]!. A reduce over the already non-empty list gives the same result (widest wins, first on a tie) with no index assertion.

Deliberate, stated for the record:

• TimelineDragGhost.tsx is deleted, not lost. It hard-coded the ghost at one TRACK_H, which is wrong once a row can be taller than a clip. The same render now lives in TimelineCanvas.tsx, where the row heights are, and takes its height from the dragged clip's own row.
• TimelineRazorInteraction.tsx is the razor overlay lifted out of Timeline.tsx so that file stays under the 600-line gate.

Stack validation after this pass: 2,901 Studio tests pass across 260 files, tsc --noEmit clean for packages/studio, oxlint and oxfmt clean, Fallow audit gate clean, file-size gate clean.

[miguel-heygen]

• feat(studio): wire expanded keyframe timeline lanes #2791 👈 (View in Graphite)
• fix(studio): harden keyframe editing semantics #2787
• feat(studio): consolidate timeline editor callbacks #2786
• feat(studio): add keyframe track headers #2785
• feat(studio): add timeline property lanes #2784
• feat(studio): add timeline keyframe retiming interactions #2783
• feat(studio): add variable timeline timing and layout #2782
• feat(studio): add keyframe timeline state #2781
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[vanceingalls]

Verdict: APPROVE head SHA 03d736ee base 92c23fda

Strong, coherent wire-up of the Family B stack. Every deferred finding and body claim I sampled against /pulls/2791/files verifies. Two P2 concerns are the ones the PR body already parks in .scratch/…/08-pr-2690-deferred-review-findings.md; I raise them here for visibility only, not to block.

Adversarial lenses

A — Full-stack integration. All seven downstack contracts wire cleanly.

• state (#2781): expandedClipIds / focusedEaseSegment read + write via store actions; auto-expand respects manual collapse (Timeline.test.ts line ~380 asserts collapse sticks).
• timing/layout (#2782): contentOrigin threaded through getTimelineFitPps, getTimelineDisplayContentWidth, getTimelinePlayheadLeft, getTimelineScrollLeftForZoomAnchor, resolveTimelineAssetDrop, marquee, range, geometry, and drag-drop. Zero residual GUTTER + TRACKS_LEFT_PAD arithmetic outside Timeline.tsx:134 (the sole computation site) — grep at HEAD confirms.
• retiming (#2783): onMoveKeyframe: Promise<boolean> widens end-to-end (timelineCallbacks → TimelineLanes → TimelinePropertyLanes → useTimelineEditCallbacks). Rejected-drop revert path in TimelineClipDiamonds.tsx now clears the pending entry AND re-clicks the source position — matches the PR-body claim.
• property lanes (#2784): expanded view renders <TimelinePropertyLanes> as a sibling of <TimelineClip> inside the same relative content div. Lanes are useMemo'd — a playhead tick no longer re-renders every diamond in every expanded clip (was a real regression vector).
• headers (#2785): TimelineTrackHeader disclosure binds to expandedClipIds; header sampler now uses per-segment ease (easedProgress — verified via new test at TimelineTrackHeader.test.tsx:198+ asserting power2.in(0.5) = 0.125 → 12.5/6.25, not linear).
• callbacks (#2786): consolidated useTimelineKeyframeHandlers — inlined lambdas removed from Timeline.tsx.
• hardening (#2787): isStaticPositionHold lifted to gsapTweenSynth.ts as the single owner; both useGsapTweenCache and the new keyframeCacheAstLoad import it — the two stores can no longer diverge on hold semantics (which was a phantom-lane bug shape).

B — Feature flag / rollout. Gated on STUDIO_KEYFRAMES_ENABLED && hasKeyframedClips. Flag off ⇒ no label mode ever. Flag on + no keyframed clips ⇒ pre-existing contentOrigin = GUTTER + TRACKS_LEFT_PAD geometry preserved (Timeline.tsx:134). Mid-render flip recomputes through useMemo deps cleanly. Remove-safe when the flag lands.

C — Perf under many keyframes/lanes.

• TimelinePropertyLanes now useMemo's both lanes and laneData — big win over the previous per-tick rebuild.
• getTimelineRowOffsets gains a WeakMap<readonly number[], number[]> cache keyed by array identity; upstream useTimelineTrackLayout memoizes rowHeights, so the identity is stable across a gesture. Safe — but JSDoc rightly warns "callers must treat the result as read-only" (nothing runtime-freezes it).
• resolveTrackKeyframeClip at TimelineLanes.tsx:214 runs .filter + .sort per track per render during playback — this is deferred issue #2 in the PR body. Real cost scales with keyframed-clip count; not a blocker at current fixture sizes.
• No new unbounded .map() into a scroll container.

D — Layout & focus preservation.

• LayerDisclosureRow gains aria-expanded; toggle button gains aria-pressed + state-specific labels ("Add {group} keyframe" / "Remove {group} keyframe"). Good WCAG alignment.
• Chevron 12×20 buttons get a transparent ::before overlay to hit the WCAG 24×24 target without visual bloat. Nice.
• Scroll compensation on the label-column flip is explicitly deferred in "Not covered". Fair disclosure. See P2-b below.

E — Body-vs-diff scope. +1738/-682 is coherent for the wire-up thesis. Three items sit outside a literal reading of the title, all disclosed:

1. Motion-path visibility widened (useStudioContextValue.ts + PreviewOverlays.tsx) — called out in PR body.
2. AST-load extracted to keyframeCacheAstLoad.ts to stay under the 600-line file gate — called out.
3. gsapDragPositionCommit.ts swaps resolveTweenDuration → resolveEditableTweenDuration at 5 sites — NOT explicitly called out. Ties the drag-position commit to the same 0.5s fallback used elsewhere for zero-duration synthesized tweens. Correct behavior; would prefer a one-line PR-body note but not a blocker.

P2 (deferred, already parked — flagging for tracking)

• P2-a First-keyframe layout flip. Timeline.tsx:134 — contentOrigin jumps from 56 → 240 (184px) when hasKeyframedClips flips false→true. All time-anchored surfaces (ruler, ticks, clips, playhead, drop math) shift together, but scroll position doesn't compensate, so the user's viewport visibly reflows when they add their first keyframe. Documented in the scratch issue file.
• P2-b resolveTrackKeyframeClip per-frame cost. TimelineLanes.tsx:214 — filter + sort per track per render during playback. Fine at current fixture counts; would want memoization keyed by (els, laneCounts, selectedIds) before scaling editorial workflows past ~50 keyframed clips.

P3 nits (no action required)

• TimelineLanes.tsx returns [clip, propertyLanes] from the outer .map non-passenger branch. Works because both nodes carry stable keys; a <Fragment key={clipKey}> reads a shade more explicitly.
• getTimelineRowOffsets cache: JSDoc note is the guardrail. If a future caller mutates rowHeights in place (readonly is types-only), stale offsets surface silently — consider Object.freeze(rowHeights) at the source.

Cross-checked head SHA 03d736ee2063e3a1b5f4060573872ca6009742a4 against gh api /pulls/2791. Zero prior human/AI reviews on the PR — this is R1. No TRACKS_LEFT_PAD arithmetic left in production code paths.

— Via

[james-russo-rames-d-jusso]

Reviewed at 03d736ee2063e3a1b5f4060573872ca6009742a4.

R2 adversarial pass on the family-B integration tip. R9 verdict: split_path_exists — the contentOrigin/labelMode threading + useTimelineTrackLayout + TimelineLanes showsLanes cluster is genuinely integration-coupled (~600 lines), but at least 5 clusters ride along (TimelineRazorInteraction extraction, TimelineDragGhost deletion, TimelineTrackHeader ease-sampling fix, useExpandedTimelineElements clipTouchesTime fallback, useTimelineGroupEditing try/finally, PreviewOverlays MotionPathOverlay broadening). Two silent scope-creep behavior changes unmentioned in the PR body: (a) MotionPathOverlay now shows for ANY DOM selection while paused (previously required inspector panel open), (b) horizontal-scroll behavior of the pre-t=0 breathing area changed (merged sticky column keeps the whole contentOrigin visible under horizontal scroll). One orange on the labelMode transition: when hasKeyframedClips first flips true, contentOrigin jumps 80→264 while pps also changes, and the zoom-anchor useLayoutEffect uses NEW contentOrigin with pre-shift scrollLeft — the anchor lands wrong by 184/prevPps seconds.

Note: posted as body-only after one inline anchor failed to resolve on GH's diff view. All findings are inline below with file:line references.

R9 exception verdict — split_path_exists

Genuinely integration-coupled: contentOrigin/labelMode threading (timelineLayout signatures change atomically), useTimelineTrackLayout with rowHeights fanout, TimelineLanes showsLanes gating, TimelineClip clipHeight+TimelineCanvas draggedClipHeight clamp (must land together to avoid broken drag-ghost step).

Could split cleanly (each self-contained):

• TimelineRazorInteraction extraction — uses only existing helpers.
• TimelineDragGhost deletion — isolated removal, no dangling refs.
• TimelineTrackHeader ease-sampling fix — localized to propertyValueAt.
• useExpandedTimelineElements clipTouchesTime fallback — separate playhead-at-end bug.
• useTimelineGroupEditing try/finally invalidateGsapCache — separate error-path bug.
• isStaticPositionHold extraction — pure helper move.
• PreviewOverlays motion path visibility broadening — unrelated to lane wiring, and unmentioned in the PR body.

Silent scope creep beyond PR title: (a) motion-path overlay now shows on any DOM selection while paused instead of only with an inspector panel open, (b) horizontal-scroll behavior of the pre-t=0 breathing area changed — the previous TRACKS_LEFT_PAD spacer was non-sticky and could scroll out of view; the new merged sticky column keeps the whole contentOrigin visible under horizontal scroll.

Findings

🟡 Silent broadening: MotionPathOverlay now shows on plain canvas selection, not only with inspector open

packages/studio/src/components/nle/PreviewOverlays.tsx:279

selection={shouldShowMotionPath ? domEditSelection : null}

In pr-2787-r3 the overlay was gated on shouldShowSelectedDomBounds ((inspectorPanelActive || rightPanelTab === 'variables') && !isPlaying && !isGestureRecording). shouldShowMotionPath is deliberately wider — !!domEditSelection && !isPlaying && !isGestureRecording — so on-canvas motion-path handles now render whenever any DOM element is selected and composition is paused, regardless of side-panel state. PR body/title ('wire expanded keyframe timeline lanes') doesn't mention motion-path scope; rationale lives only in a useStudioContextValue comment. Can surface arc handles unexpectedly for users with panels collapsed.

Fix: Either land this widening as its own PR with a title that names it, or hold the widening until an explicit feature toggle exists. If keeping it, add a release-note bullet.

🟡 Concurrent AST populate for all files races on shared cache aliases

packages/studio/src/hooks/useGsapTweenCache.ts:398

Promise.all(files.map((sf) => populateKeyframeCacheFromAst(projectId, sf, doc))).then(() => {

populateKeyframeCacheFromAst writes three aliases per element: ${sf}#${id}, bare ${id}, and — for sub-comps — index.html#${id}. Under Promise.all fetches resolve out of order, so both bare-id alias and sub-comp→index.html cross-alias are written last-fetch-wins. If two files legitimately share a DOM id, or a sub-comp populates before index.html clears its own index.html#id entry, store ends up with a nondeterministic mix. clearKeyframeCacheForFile for a sub-comp deliberately clears index.html# aliases too, which can wipe a legitimate root-file entry if the sub-comp resolves after root has finished writing. Inline 'active file first' comment only justifies sequencing at compose time, not resolution ordering.

Fix: Await files sequentially (or at minimum await active sourceFile last so its aliases win), or key aliases with an owning-file suffix so no two files write the same key.

🟠 Zoom-anchor computes with new contentOrigin against pre-shift scrollLeft when labelMode flips

packages/studio/src/player/components/timelineLayout.ts:407

const timelineX = Math.max(
  0,
  getTimelineContentXFromClient({
    clientX: input.pointerX,
    rectLeft: 0,
    scrollLeft: input.currentScrollLeft,
    contentOrigin: input.contentOrigin,
  }),
);
const timeAtPointer = Math.max(0, Math.min(input.duration, timelineX / currentPps));

useTimelinePlayhead's useLayoutEffect only re-anchors on pps change, but when hasKeyframedClips first flips true, contentOrigin jumps 80→264 (LABEL_COL_W+GUTTER − GUTTER−TRACKS_LEFT_PAD = 184px) and pps changes together (fitPps depends on contentOrigin). getTimelineScrollLeftForZoomAnchor is passed NEW contentOrigin but currentScrollLeft was set under OLD one, so timeAtPointer = (scrollLeft + pointerX − NEW_contentOrigin)/prevPps mis-identifies time under viewport centre by 184/prevPps seconds. If user has scrolled and commits first keyframe of the comp, everything shifts and the anchor lands wrong. This is the 'per-first-keyframe-add scroll shift' pattern.

Fix: Thread OLD contentOrigin through the anchor call (add previousContentOriginRef alongside previousAnchorPpsRef, pass as currentContentOrigin while contentOrigin becomes nextContentOrigin), so pre-shift time under pointer is computed with pre-shift origin.

🟢 resolveTrackKeyframeClip does a filter + sort per track per render, including every playhead tick

packages/studio/src/player/components/useTimelineTrackLayout.ts:27

const keyframed = elements.filter(
  (element) => (laneCounts.get(element.key ?? element.id) ?? 0) >= 1,
);
if (keyframed.length === 0) return null;
const selected = keyframed.find((element) => {
  const key = element.key ?? element.id;
  return key === selectedElementId || selectedElementIds.has(key);
});
if (selected) return selected;
return [...keyframed].sort(
  (a, b) => (laneCounts.get(b.key ?? b.id) ?? 0) - (laneCounts.get(a.key ?? a.id) ?? 0),
)[0]!;

Timeline.tsx subscribes to currentTime (const currentTime = usePlayerStore((s) => s.currentTime);) so Timeline (and TimelineCanvas → TimelineLanes) re-renders on every playhead tick. Inside TimelineLanes, resolveTrackKeyframeClip(els, laneCounts, selectedElementId, selectedElementIds) runs once per displayTrackOrder entry per render — filter+sort per track per frame during playback. useTimelineRowHeights also calls it via useMemo but that's separate from the per-render call site in TimelineLanes.

Fix: Compute a keyframeClipByTrack: Map<number, TimelineElement | null> inside useTimelineTrackLayout's useMemo and pass down as a stable prop, so TimelineLanes does a Map lookup instead of fresh filter+sort per track per render.

🟡 hasKeyframedClips re-scans the whole gsapAnimations map even when STUDIO_KEYFRAMES_ENABLED is false

packages/studio/src/player/components/Timeline.tsx:121

const hasKeyframedClips = useMemo(
  () =>
    Array.from(gsapAnimations.values()).some((list) =>
      // Same lane-contribution predicate the layout uses: real keyframes OR a
      // synthesizable flat tween. Checking animation.keyframes alone left a
      // flat-tween-only comp without its reserved label column.
      list.some((animation) => animationContributesLane(animation)),
    ),
  [gsapAnimations],
);
const labelMode = STUDIO_KEYFRAMES_ENABLED && hasKeyframedClips;

Memo doesn't short-circuit on feature flag. When STUDIO_KEYFRAMES_ENABLED is false, hasKeyframedClips still walks every animation in every element — animationContributesLane synthesizes a flat tween for each — but result is discarded because labelMode is always false. Also flips gsapAnimations dependency to trigger recomputes on any tween write, even in shipping keyframes-off configuration.

Fix: Guard the memo body: useMemo(() => STUDIO_KEYFRAMES_ENABLED && Array.from(...).some(...), [gsapAnimations]).

🟡 TimelineTrackHeader still renders LegacyTrackHeader for non-keyframed tracks in label mode, widening the sticky column silently

packages/studio/src/player/components/TimelineTrackHeader.tsx:522

const showTrackLabel = contentOrigin >= LABEL_COL_W;

showTrackLabel is a proxy for labelMode derived from a numeric threshold. In compact mode (contentOrigin=80) header stays 80px wide with just eye + audio icon. In label mode (contentOrigin=264) every track — including ones without a keyframed clip — gets a 232-wide LABEL_COL_W column with track label text. Visible layout change for non-keyframed tracks in a comp that has any keyframed clip; not called out in PR body. Heuristic is also fragile: any future contentOrigin value between 232 and 264 would silently flip the header shape.

Fix: Pass an explicit labelMode boolean down from Timeline instead of inferring from contentOrigin, and either document label-column-per-track behavior or scope showTrackLabel to tracks that actually have a keyframed clip.

🟢 TimelineLanes returns [clip, propertyLanes] from .map without a Fragment wrapper

packages/studio/src/player/components/TimelineLanes.tsx:580

if (!isPassenger) return [clip, propertyLanes];

Both children carry keys (clipKey and ${clipKey}-property-lanes) so React reconciles correctly, but returning nested arrays from .map() is subtle — future refactor that drops propertyLanes key or adds a third sibling will regress with hard-to-read 'each child in a list should have a unique key' warning at runtime. Passenger branch already uses wrapping div; non-passenger branch inconsistent.

Fix: Wrap in <React.Fragment key={clipKey}>{clip}{propertyLanes}</React.Fragment> so sibling structure is explicit and outer key is stable.

🟡 populateKeyframeCacheFromAst clears then writes with no per-file coalescing — rapid selection change reruns entire fanout

packages/studio/src/hooks/useGsapTweenCache.ts:361

const compositionSrcKey = usePlayerStore((s) =>
  Array.from(new Set(s.elements.map((el) => el.compositionSrc).filter((src) => !!src)))
    .sort()
    .join("|"),
);

compositionSrcKey is a selector recomputed on every store update. Uses Array.from(new Set(...)).sort().join('|') — allocates + sorts on every store notification (currentTime included). While fetchKey memoizes effect body, store selector still runs each subscribed render. Combined with added compositionSrcKey dep on effect, any element list mutation triggers full-file re-fetch/clear cycle for every sub-comp, not just changed one.

Fix: Memoize composition src set upstream (e.g., store computed selector with reference stability) or gate effect on coarser fingerprint that changes only when sub-comp set actually changes, not on every element mutation.

🟢 getTimelineRowOffsets WeakMap cache is unbounded relative to array identities

packages/studio/src/player/components/timelineLayout.ts:94

const rowOffsetsCache = new WeakMap<readonly number[], number[]>();

/** Cumulative top offsets, including the final bottom boundary. */
export function getTimelineRowOffsets(rowHeights: readonly number[]): number[] {
  const cached = rowOffsetsCache.get(rowHeights);
  if (cached) return cached;

Cache assumes rowHeights identity is stable across a gesture (true when from useTimelineTrackLayout's useMemo). But callers can also pass literal arrays (tests, or displayRowHeights recomputed on each drag frame in useTimelineDisplayLayout). Each fresh array becomes new WeakMap key and offset array is never GC'd until source rowHeights is. Under long drag, displayRowHeights is a new .map() result each render, so every frame allocates a new [0, ...] array and holds in map for lifetime of source ref.

Fix: Either accept recompute cost (offset arrays are small) and drop cache, or memoize displayRowHeights with a stable structural equality guard so WeakMap key doesn't churn per frame.

ℹ️ TimelineRazorInteraction guide stays visible on tool switch until pointer leaves timeline

packages/studio/src/player/components/TimelineRazorInteraction.tsx:30

const updateRazorGuide = useCallback(
  (event: MouseEvent<HTMLDivElement>) => {
    const scroll = scrollRef.current;
    if (!active || !scroll) return;
    const rect = scroll.getBoundingClientRect();
    setRazorGuideX(event.clientX - rect.left + scroll.scrollLeft);
  },
  [active, scrollRef],
);

const clearRazorGuide = useCallback(() => setRazorGuideX(null), []);

When active tool switches from 'razor' to another tool while pointer is inside timeline, updateRazorGuide silently no-ops (correct) but razorGuideX still holds last x — render gate activeTool === 'razor' && razorGuideX !== null correctly hides the guide, so harmless today. If future consumer reads razorGuideX independently, would read stale value. Minor cleanup — state should reset when active flips false.

Fix: Add useEffect(() => { if (!active) setRazorGuideX(null); }, [active]); inside useTimelineRazorInteraction.

🟢 labelMode-triggered contentOrigin flip is not covered by tests despite being the PR's central invariant

packages/studio/src/player/components/Timeline.test.ts:79

it("reserves the label column and keeps expanded keyframes aligned with ruler time"

Added Timeline provider-boundary test asserts STATIC labelMode geometry (contentOrigin = LABEL_COL_W + GUTTER when expanded from mount) but doesn't cover TRANSITION from compact→label. Given the anchor drift finding above, a dynamic test that seeds a comp with no keyframes, scrolls, then writes the first tween and asserts a chosen time under viewport centre stays under it, would be the natural regression guard.

Fix: Add a Timeline test that scrolls to a known time, writes the first keyframed animation to a store element, and asserts the visible timestamp under the viewport centre is stable across the labelMode flip.

Family B parity check (cross-stack lens)

Against the implied 'Figma-motion parity' aim, this PR closes the compact-diamond→expanded-lane, per-property-group navigation, and header add/remove/toggle gaps for the currently-supported property groups.

Still uncovered against Figma motion:

1. Per-segment custom cubic-bezier curve editor — only named eases are surfaced via keyframe.ease/easeEach.
2. Spring physics (mass/stiffness/damping) — no store shape or UI.
3. Overshoot/back easings surfaced as adjustable parameters rather than fixed ease names.
4. Motion-path direct manipulation from the lane itself — arc drag exists on the preview canvas via MotionPathOverlay (now widened silently to any-selection-while-paused) but not from the timeline lane.
5. Hold vs linear interpolation as a first-class per-segment toggle — isStaticPositionHold suppresses set() diamonds but per-segment 'hold' between two authored keyframes is not exposed.
6. Per-property (not per-group) lane splitting — 'position' is one lane even when x and y have distinct keyframe timings.
7. Copy/paste of keyframes (and paste-to-playhead) with selection preservation.
8. Keyframe selection persistence across composition switches — deleteSelectedKeyframes now defensively drops stale-element keys (per the new timelineEditingHelpers test) which suggests the selection model still leaks across contexts.
9. Stagger visualization on grouped selectors resolved via resolveSelectorElementIds — the cache aliases every element identically, so a class-based stagger renders every animated child with the same keyframe times.

🟢 Verified clean

• TimelineDragGhost.tsx deletion — grep for 'TimelineDragGhost' across packages/studio/src returned no dangling imports or references
• TimelineClipDiamonds retime revert path — revertRetime clears pending entry AND calls onClickKeyframe?.(fromTarget), matching pending-key invariant
• useExpandedTimelineElements clipTouchesTime fallback — strict-first, fallback-second ordering correctly picks starting clip on shared seam (verified by 'prefers the starting clip over the ending one on a shared seam' test)
• useTimelineGroupEditing invalidateGsapCache moved to finally in both move and resize paths — cache invalidated even when fallback rewrite throws, matching single-element path
• isStaticPositionHold extraction — useGsapTweenCache and populateKeyframeCacheFromAst both use extracted predicate, resolving cache-vs-source-animations disagreement called out in doc comment
• TimelineTrackHeader propertyValueAt eased sampling — mid-segment readout honours segment's arriving ease (verified by 'samples mid-segment values along the segment's ease, not linearly' test); header-inserted keyframes stamp the eased value so curve is preserved

Complements Via's parallel adversarial pass (Family B rollup).

— Review by Rames D Jusso

[james-russo-rames-d-jusso]

Reviewed at c20c5366da34c1a3444d3ff9282e01d92bf274d8 — 60 files, +2451/-922. Family B tip / B8.

Pulled 4 adversarial sub-agents on this one: R9 splittability + wiring, correctness at integration seams, CONTRIBUTING compliance + types, test-coverage strength.

Headline: the wiring is real (TimelinePropertyLanes renders at TimelineLanes.tsx:527, useAutoExpandKeyframedClips fires at Timeline.tsx:183), so #2784's scaffolding concern is closed. All 7 foldback items land where advertised. All 8 QA-surfaced regressions are fixed at the cited files. My prior #2787 blocker on TimelineOverlays.tsx context-menu adapters is structurally resolved — the TimelineKeyframeTarget bag object now travels intact through the 2-arg adapters (onDelete={(elId, keyframe) => onDeleteKeyframe?.(elId, keyframe)} at TimelineOverlays.tsx:108-114), so identity cannot drop to arity anymore. Nice shape.

No blockers. Meaningful concerns below, roughly ranked by impact.

Concerns

🟠 useTimelineEditCallbacks.test.tsx:143 — the marquee "clicked-element selection" fix is not actually pinned by the test. mocks.actions.buildDomSelectionForTimelineElement.mockResolvedValue(mocks.selection) returns the SAME selection object regardless of which element is passed. Assertions like toHaveBeenCalledWith(otherFlatAnimation.id, 0, undefined, mocks.selection) (:241, :394) pass under both the fix (resolved from clicked circle) and the bug (falls back to selected box). To pin the fix, the mock needs mockImplementation(id => id === "circle" ? circleSelection : boxSelection) and the assertion needs to check the override is circleSelection (not just any selection). This was the R1 headline invariant — worth locking in properly before merge.

🟠 TimelineClipDiamonds.tsx:365-382 — optimistic onClickKeyframe before onMoveKeyframe resolves is a race hazard on rapid successive drags. onClickKeyframe({...target, percentage: res.toClipPct}) fires unconditionally, then revertRetime() calls onClickKeyframe(fromTarget) if the commit rejects. Under rapid drags the revert of drag #1 can fire while drag #2 is in-flight and park the selection on a now-stale target. Previous shape (no optimistic park) avoided this class entirely. Low-frequency but reproducible with fast drags on a slow-committing tween.

🟠 TimelinePropertyLanes.test.tsx:348 — cross-segment ease mislabeling is not caught. expect(new Set(paths).size).toBe(3) proves distinct paths across three eases (undefined→none / power2.out / custom) but if segment[1] rendered segment[2]'s ease (the exact merged-lane cross-labeling regression the fix targets), the paths are still all-distinct and the assertion still passes. Needs pinning each segment's path against its own known-ease shape (or at minimum a per-segment assertion). Same file :429 — toHaveBeenCalledWith("clip-1", expect.objectContaining({ percentage: 50 })) — the headline of the diamond-identity refactor is precisely the added fields (animationId, propertyGroup, tweenPercentage), which objectContaining({ percentage }) skips. toEqual({ ...full shape }) would lock the identity contract.

🟡 useGsapTweenCache.ts:378-392 — usePopulateKeyframeCacheForFile leak-on-race. pruneKeyframeCacheToFiles(files) runs at effect start, but a prior Promise.all(files.map(populateKeyframeCacheFromAst)) may still be in flight and write into a file the new prune just dropped. Only manifests on rapid compositionSrcKey changes mid-fetch (elements added/removed during rapid switch). Cache staleness, not write-correctness — a stale entry lingers until the next scan. If you want to close it deterministically, capture a scanId at the prune boundary and drop writes whose scanId no longer matches.

🟡 usePopulateKeyframeCacheForFile.test.tsx:65-66 — .some(u.endsWith("/index.html")) && .some(u.includes("keyframe-lab.html")) permits any number of extraneous fetches. The claim in the PR body is "loads for every composition file the timeline renders rows for, not just the file that owns the current selection" — but the test cannot reject a version that fetches 100 unrelated URLs. Set-equality against rendered rows would lock the actual invariant. Separately, the "no stale entries on drop" claim is unit-tested via pruneKeyframeCacheToFiles at gsapKeyframeCacheHelpers.test.ts:115-155 (strong) but not composed with usePopulateKeyframeCacheForFile for the end-user comp-switch flow.

🟡 gsapDragPositionCommit.ts:26-33 — buildTemporalArcKeyframes drops the replaced-keyframe's ease and any extra properties. Existing keyframe within 1% of the insert-% is filtered out and the replacement is bare {percentage, properties}. Practical exposure is narrow (caller is commitFlatViaKeyframes, flat→keyframes conversion), but the comment says "REPLACED, not kept" without acknowledging that the replaced entry's ease + non-position properties (rotation on a mixed keyframe) also die. If intentional, the comment should say so; if not, the replacement should carry those forward.

🟡 useTimelineEditCallbacks.ts:49,56 — local TimelineKeyframeTargetAnimation.propertyGroup?: string | null widens away the PropertyGroupName union. SSOT is packages/parsers/src/gsapConstants.ts:53 ("position" | "scale" | "size" | "rotation" | "visual" | "other"); studio already imports it in hooks/gsapRuntimeBridge.ts:11. Since PropertyGroupName ⊆ string, no active bug — but the compiler no longer catches a typo like propertyGroup: "positon" at this API surface. PR body says this was "left as-is" — flagging so the follow-up doesn't get lost.

🟡 R9 framing overstated in the PR body. The cohesive integration slice (Cluster 1 — Timeline/TimelineLanes/useTimelineTrackLayout/TimelineCanvas/TimelineRazorInteraction/timelineLayout) is ~1,300 net lines and legitimately couples via contentOrigin + useTimelineTrackLayout signature — that half is R9-worthy. But 4+ cleanly-separable clusters ride along: (a) cross-composition cache load + AST-load extraction (~278 lines, own commit + own test), (b) end-of-timeline containment fallback (own commit + own test), (c) MotionPathOverlay visibility widening — deliberate behavior change disclosed only in "Not covered", (d) invalidateGsapCache identity hoist (App.tsx:157-160), (e) useTimelineGroupEditing try/finally cache-invalidate on error path (useTimelineGroupEditing.ts:335-339, 462). Not blocking — these all touch this stack's surface and re-splitting would burn a day — but the framing "cohesive 1,504-line delta" reads as if it were one atomic thing. A one-line body edit ("Cluster 1 is the R9 core (~1,300 lines); the remaining ~200 lines are QA-surfaced fixes bundled for shipping speed") would honor the precedent.

🟡 MotionPathOverlay visibility widening (useStudioContextValue.ts:99-102). shouldShowMotionPath = !!domEditSelection && !isPlaying && !isGestureRecording — deliberately broader than Inspector-panel-gated. Called out in "Not covered" as a deliberate behavior change. Since keyframe path handles ARE the arc-drag affordance, the wider gate is defensible; but this belongs in a decision-log or its own tiny PR, not folded into a lane-integration ship. Overlay early-returns on !geometry && !armed so non-motion selections (text, image, group) draw nothing — but useMotionPathData still runs its DOM reads per render, and text-selection is a common state. Worth eyeballing perf under a scene with many selections toggling.

🟡 File-size gate ragged edge. Four touched files sit 1-17 lines below the 600-line studio gate at HEAD: App.tsx (599), PropertyPanel.tsx (598), Timeline.tsx (584), TimelineLanes.tsx (583). lefthook.yml:57 enforces >600 as ERROR. The AST-load extraction did its job for the cache path, but the next in-scope PR touching any of these four almost certainly trips the hook. Not a blocker for this PR, but consider a proactive decomposition follow-up on Timeline.tsx and TimelineLanes.tsx — landing something on the ragged edge is a paper-cut for the next author.

Nits

• packages/studio/src/hooks/gsapKeyframeCacheHelpers.test.ts — the "double-cast test fixtures" comment attributes as unknown as T to CONTRIBUTING.md, but CONTRIBUTING scopes the double-cast to "hard type-system boundaries (parsing untrusted JSON, FFI/postMessage)". Test fixtures aren't in the stated example set. Either loosen the comment or update CONTRIBUTING.md to enumerate fixtures as an accepted case.
• Hitchhiker orphan fixes (invalidateGsapCache hoist, useTimelineGroupEditing try/finally) should get one line in the PR body so future git-blame doesn't attribute them to lane wiring.

Questions

• KEYFRAME_PCT_MATCH widened 0.05 → 1 in gsapShared.ts:80 (applied at gsapDragPositionCommit.ts:295). Harmonizes tolerances, but 1% on a 10s clip = 100ms — two intentionally-close keyframes now merge silently. Was this intentional? If yes, worth calling out under "Not covered" as a UX-visible policy shift.

What I didn't verify

• Runtime perf under 100+-clip scenes (no repro).
• useMotionPathData DOM-read cost with non-motion selections in production HeyGen usage — the widened gate compounds on selection-heavy flows.
• MotionPathOverlay early-return path — verified by reading the code, not by running the app.
• Full 2,910-test-suite pass claim — trusted the PR body.

LGTM from my side — leaving as a comment. Approval is Vance's / owner's call; the test-lock-in and optimistic-park race are worth addressing before this hits main but neither is a "must-fix before merge" from my seat.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at c20c5366da34c1a3444d3ff9282e01d92bf274d8 (code-review max, R2, 4-slice attack fleet: writer path / store layer / UI+a11y / test coverage). Delta over my R1 at 03d736ee2063.

Verdict

COMMENT (approve-lean).

Every #2781 / #2783 / #2784 / #2785 / #2786 deferred residual verified closed at this tip. No blockers survive the four-slice attack. Twelve findings total, all 🟡 or 🟢 — all follow-up-worthy but none block merge of the stack tip.

Deferred fixes — status at tip

Origin	Class	Verified at	Status
#2781	focusedEaseSegment reset in reducer	playerStore.ts:534	closed
#2781	idFromSelector round-trip	gsapShared.ts:103-111 + gsapShared.test.ts:172-184	closed
#2781	isStaticPositionHold single-owner	gsapTweenSynth.ts:18-23	closed
#2783	Middle-man wrap/unwrap at TimelineClipDiamonds:549-555	Connector code extracted to TimelineDiamondConnectors.tsx (145-line component split, not a middle-man refactor)	closed
#2783	Proximity-vs-identity in pending cleanup	TimelineClipDiamonds.tsx:73-82 (identity + tolerance predicate)	closed
#2783	Tolerance constant convergence	gsapShared.ts:80 (KEYFRAME_PCT_MATCH = 1) — 5→3 percentage-match sites consolidated; remaining numerics are distinct concepts	closed
#2784	groupKeyframes → toClipKeyframes delegation	TimelinePropertyLanes.tsx:88 — threads canonical Math.round(… * 100000) / 1000 precision	closed
#2785	aria-pressed on toggle	TimelineTrackHeader.tsx:236	closed
#2785	Hover-eye keyboard reachability	TimelineTrackHeader.tsx:329-334 (VisibilityButton on always-mounted LayerDisclosureRow with visible={true})	closed
#2786	animationOverride 4th arg	useGsapSelectionHandlers.ts:341-361	closed
#2786	resolveWriteSelection strict === undefined	useGsapSelectionHandlers.ts:126-132	closed
#2786	handleGsapRemoveAllKeyframes off bare ?? fallback	useGsapSelectionHandlers.ts:421 (diff @@ -418)	closed — this was the last writer with the null-precedence bug class
#2786	Call sites thread CLICKED-element identity	useTimelineEditCallbacks.ts:192-357	closed

Every callback (onDelete/onMove/onToggle/onTogglePropertyGroup) uses resolveElementAnimations(elementKey) + buildDomSelectionForTimelineElement(element) — CLICKED-frame identity threaded end-to-end. No mixed-frame writes remain.

New findings — writer / mutation-authority slice

• 🟡 Return-semantics unevenness across onMoveKeyframe branches, useTimelineEditCallbacks.ts:289. The new flat-tween boundary-drag branch returns true synchronously before handleGsapUpdateMeta settles: if (decision.pctRemap.length === 0) { handleGsapUpdateMeta(target.animId, {position, duration}, sel); return true; }. Other onMoveKeyframe branches await Promise<boolean> reflecting result.changed. handleGsapUpdateMeta returns void (fire-and-forget via observeGsapMutation, useGsapSelectionHandlers.ts:178). The Promise<boolean> = PERSIST-success contract Rames set on #2783 R2 is now uneven across branches — this branch reports success unconditionally.
• 🟡 Selector-based .find in commitFlatViaKeyframes, gsapDragPositionCommit.ts:214. After convert-to-keyframes, the "converted" anim is found by targetSelector match rather than anim.id: const converted = fresh.find((a) => a.targetSelector === anim.targetSelector && a.keyframes) ?? anim. Multi-tween-same-selector scenarios could pick the wrong keyframed anim.
• 🟢 onDeleteAllKeyframes picks only the FIRST keyframed animation, useTimelineEditCallbacks.ts:197. Elements with multiple property-keyframed anims leave others behind despite the callback name.

New findings — UI + a11y slice

• 🟡 aria-controls missing on disclosure trigger, LayerDisclosureRow.tsx:37. Has aria-expanded={isExpanded} and aria-label, but no aria-controls — screen readers can't jump from the disclosure to the controlled lane panels. Sibling elements in TimelineTrackHeader have no id.
• 🟡 WCAG 2.5.8 target-size 24x24 missed on 2 buttons. Chevrons at TimelineTrackHeader.tsx:132-136 got the before:h-6 before:w-6 overlay; two other buttons at 20x16 without it: property-group ◆/◇ toggle at TimelineTrackHeader.tsx:239 (h-5 w-4) and disclosure caret at LayerDisclosureRow.tsx:40 (h-5 w-4).
• 🟡 LayerDisclosureRow hardcodes LABEL_COL_W, line 29. PropertyGroupHeaderRow was fixed to use adaptive columnWidth; when parent TimelineTrackHeader collapses to contentOrigin < LABEL_COL_W, the absolute-positioned LayerDisclosureRow extends past the parent and overlaps timeline lane content.
• 🟡 Proximity keying in React keys, TimelineClipDiamonds.tsx:389. Button key is ${i}-${kf.percentage}; mirrored at TimelineDiamondConnectors.tsx:66. Post-retime cache commit + sort-order swap would remount the diamond. Not user-visible today (drag lifecycle ends on pointerUp before mutation commits), but an outstanding proximity-keyed slot the earlier reviews said to close.

New findings — store / state slice

• 🟡 reset() incompleteness, playerStore.ts:520. Reset action sets 24 fields but omits lintFindingsByElement (Map keyed by elementId, line 224), activeKeyframePct (line 132), motionPathArmed (line 138), motionPathCreateAvailable (line 140), requestedSeekTime (line 210). All project-scoped, but not in the documented 5-field preserved-preference list (playbackRate/audioMuted/loopEnabled/zoomMode/manualZoomPercent). Pre-existing latent (playerStore.ts not touched by this PR); note for follow-up ticket, not a blocker.
• 🟢 Tolerance-literal naming hygiene, TimelineClipDiamonds.tsx:79 (< 0.2 for pending-retime cleanup) vs :240 (< 0.5 for atPlayhead) vs KEYFRAME_PCT_MATCH = 1 elsewhere. Three magic numeric tolerances in one file, undocumented naming. Distinct concepts, but a named PENDING_SETTLE_TOLERANCE / AT_PLAYHEAD_TOLERANCE would carry intent.

New findings — test coverage slice

• 🟡 expect.any(Number) matches NaN, useGsapSelectionHandlers.test.tsx:143. The animationOverride-consulted test uses expect(moveKeyframe).toHaveBeenCalledWith(selection, "anim-1", 50, expect.any(Number)). A future change that computes playhead pct from empty selectedGsapAnimations (fixture uses selectedGsapAnimations: [] deliberately) rather than the 4th-arg override yields NaN; NaN IS a Number and the assertion is silent. Tightening to expect.closeTo(N, 0) or a Number.isFinite guard would meaningfully strengthen.
• 🟡 Duplicate describe block, useGsapSelectionHandlers.test.tsx:119 and :167. Same describe("useGsapSelectionHandlers selection override", ...) at both positions; null-abort test bodies at lines 120-133 and 168-181 are byte-identical. Likely rebase artifact — harmless but should be cleaned so future edits don't touch one and forget the other.
• 🟡 Missing explicit-null-abort at callback layer, useTimelineEditCallbacks.test.tsx:338. Delete-path tests cover (id, pct, undefined, undefined|selection) — fallback semantics — but no test exercises (id, pct, undefined, null). A callback-layer regression that inadvertently forwards null (instead of undefined) would silently kill delete-path writes at the lower handler; only the useGsapSelectionHandlers-level suite would catch it.
• 🟢 Rapid-second-retime test doesn't cross keyframes, TimelineClipDiamonds.test.tsx:313. Walks 50%→75%→85% but never crosses another keyframe. Keyframes at 30% and 70%, dragging 30 past 70 — the case where identity-by-percentage would visibly fail — is not exercised.
• 🟢 Track-header a11y assertions absent, TimelineTrackHeader.test.tsx:121. Grep for aria-expanded|aria-pressed|onKeyDown|tabIndex|Enter|Space|focus returns only the "hover-gated eye is unusable by keyboard" comment at L125; no assertion on any of those attributes. Regressions in toggle diamond's aria-pressed, disclosure row's aria-expanded, or Enter-activation on the eye icon wouldn't surface.

Fix-internal remaining-silent-X audit

All six categories audited across the writer path:

• State-accumulation discard: resolveWriteSelection strict-undefined guard closes the ??-fallthrough class the earlier PRs shipped. handleGsapRemoveAllKeyframes migrated off the bare ?? fallback at line 421 — this was the last writer with the class.
• Return-boundary invariant: 1 🟡 exception — flat-tween boundary-drag branch reports success unconditionally.
• Library defaults on failure axis: N/A for this slice; earlier framesync default work was in the render pipeline, not the editor.
• Precedence in overlap rules: Every callback threads CLICKED-element identity; resolveElementAnimations(elementKey) + buildDomSelectionForTimelineElement(element) never fall back to current selection. Clean.
• Session/resource ownership: N/A for this slice.
• Discovery/enumeration completeness: pruneKeyframeCacheToFiles at gsapKeyframeCacheHelpers.ts:124 iterates both keyframeCache and gsapAnimations; safely skips bare-id aliases (they get taken by clearKeyframeCacheForElement alongside their prefixed key). Test coverage present.

Standards lens re-run

Mechanical grep on new lines only, across all four slices:

• Bare as T (excluding as unknown as, as const): 2 — both as unknown as GsapAnimation/DomEditSelection in test fixtures, CONTRIBUTING.md-documented double-cast, not a smell
• Non-null !. / ![ / !;: 0
• .message without instanceof Error guard: 0
• Angle-bracket cast <T>value: 0

Clean across the ~2400 added lines.

Independent observation

keyframeCacheAstLoad.ts adds a hasAnimations guard (was blind cast to ParsedGsap) that closes a real crash class on proxy/error/stale-200 responses. resolveClipTimingBasis correctly falls back to sub-comp HOST bounds for elements not in the flat elements[] list. pruneKeyframeCacheToFiles addresses a real cross-project 240-entry-per-switch leak — the code path is worth calling out as a nice defensive improvement beyond the deferred-fix scope.

Peer state

• My R1 at 03d736ee (2026-07-25): APPROVED. Body: "Strong, coherent wire-up of the Family B stack."
• Rames R2 at 03d736ee (2026-07-25): COMMENTED with R9 split_path_exists verdict. New head has moved substantially since then; his R9 finding predates the fixes from #2785/#2786/#2787 landing at the tip.
• CI: Preflight, player-perf, Preview parity, preview-regression, regression all green at this head. Graphite / mergeability_check in progress (stack orchestration, non-blocking).

Stamped.

— Review by Via

[james-russo-rames-d-jusso]

R1 concerns resolved. Delta commit 8470b88aa addresses every test-lock-in and race concern I flagged, plus three writer-layer bugs Miguel caught himself.

R1 concerns → verified closed

• Same-object mock erased the clicked-vs-selected discriminator (useTimelineEditCallbacks.test.tsx:143) → selectionForElement(el) returns a DIFFERENT selection per element id, wired via mockImplementation((el) => Promise.resolve(selectionForElement(el))); three previously-watered assertions at :276, :310, :491 now name selectionForElement(circle) explicitly. New "aborts every mutation when the clicked element resolves no selection" test at :361-388 pins the null-selection guard.
• Optimistic-park race in TimelineClipDiamonds.tsx:365-382 → new latestRetimeRef on the lane, revertRetime guards if (isLatest) onClickKeyframe?.(fromTarget). Pinned by a fresh test in TimelineClipDiamonds.test.tsx:313-395 that stages two back-to-back drags with the first's commit still in flight and asserts expect(onClickKeyframe).not.toHaveBeenCalled() after the stale failure resolves.
• expect.any(Number) accepted NaN, .size === 3 accepted swapped curves (TimelinePropertyLanes.test.tsx) → playhead assertion now asserts exact 25 after providing position: 2, resolvedStart: 2, duration: 4, currentTime: 3; ease-segment test now pins the label ORDER ["none", "power2.out", "custom(...)"]; collapsed-diamond test asserts the whole COLLAPSED_TARGET bag object.

Additional writer-layer fixes (self-found)

• onMoveKeyframe flat-tween boundary branch: handleGsapUpdateMeta now returns Promise<boolean> via observeGsapMutation resolving to landing status, and the boundary branch return handleGsapUpdateMeta(...) propagates it — no more premature true before the write settles.
• "Delete All Keyframes" only cleared the first keyframed tween: resolveElementAnimations(elementKey).filter(a => a.keyframes) + serial iteration for (const anim of anims) await handleGsapRemoveAllKeyframes(anim.id, selection). The serial comment names why (per-source-file conflict).
• commitFlatViaKeyframes post-convert lookup by selector picked arbitrary tween: id-first lookup fresh.find((a) => a.id === anim.id && a.keyframes) ?? fresh.find((a) => a.targetSelector === ...). Correct fallback order.

A11y / layout polish

• Disclosure caret + property-group toggle grow to 24×24 (WCAG 2.2 minimum); glyph stays 11px.
• LayerDisclosureRow now takes adaptive columnWidth matching sibling lane rows (no more hardcoded LABEL_COL_W overhang when the header column narrows over the canvas).
• Lanes container is always mounted with id={lanesId}, and the caret's aria-controls resolves in both expanded and collapsed states.

Prior R1 items outside this delta

• TimelineLanes.tsx/useTimelineEditCallbacks.ts/useGsapSelectionHandlers.ts/TimelineOverlays.tsx at 583-599 lines (below the 600-line gate but rapidly approaching). Not touched here; net effect of this delta on useGsapSelectionHandlers.ts is +8 (still below the gate). Watch on the next tip.
• MotionPathOverlay visibility widening + invalidateGsapCache identity hoist + useTimelineGroupEditing try/finally hitchhikers noted in R1 also not touched — no change, and this delta is scoped tightly to the review-follow-up commit message.

Clean execution on the delta. R1 test-lock-in family fully resolved.

— Review by Rames D Jusso

[vanceingalls]

Verified at 8470b88aa1ca53ca7843a049c5027a4e3dd41622 (R3, fix commit over R2 at c20c536). Fix commit: 8470b88a "settle boundary retimes, delete every keyframed tween, tighten the test locks."

Verdict

APPROVE. 12/15 R2 findings fully closed, 1 partial (Fragment-level proximity key on the connector row — remount doesn't cost pointer capture, low-severity), 2 open non-blocker test-coverage nits. Every 🟠 authority-class finding from R2 landed with a matching test in this delta. No new regressions on the 10 touched files; the 51 files outside the delta are byte-identical to R2 (12/12 blob-SHA spot-check confirms).

R2 findings — status at new head

Rames R3 🟠 findings

#	Finding	Status	Evidence at 8470b88a
1	Same-object mockResolvedValue erased clicked-vs-selected discriminator (useTimelineEditCallbacks.test.tsx:143)	CLOSED	New selectionForElement(el) returns a DIFFERENT selection per element id; wired via mockImplementation((el) => Promise.resolve(selectionForElement(el))) at line 155. Three previously-watered assertions at :279, :313, :494 now name selectionForElement(circle) explicitly.
2	Optimistic onClickKeyframe race in TimelineClipDiamonds.tsx:365-382	CLOSED	New latestRetimeRef on the lane at line 66; revertRetime gates if (isLatest) onClickKeyframe?.(fromTarget) at line 379. Pinned by fresh test leaves the selection alone when a stale retime fails after a newer drag at TimelineClipDiamonds.test.tsx:313-393 — stages two back-to-back drags with the first's commit in flight, asserts expect(onClickKeyframe).not.toHaveBeenCalled() after the stale failure resolves.
3	size===3 doesn't catch cross-segment mislabeling; objectContaining({ percentage }) skips diamond-identity fields (TimelinePropertyLanes.test.tsx)	CLOSED	Ease-segment test at :349-357 now asserts label ORDER: ["Edit none easing", "Edit power2.out easing", "Edit custom(...) easing"]. Collapsed-diamond test at :441-447 now asserts the full COLLAPSED_TARGET bag { animationId, propertyGroup, percentage: 50, tweenPercentage: 50 } — no objectContaining.

My R2 findings

#	Finding	Status	Evidence at 8470b88a
4	Return-semantics unevenness on onMoveKeyframe flat-tween branch (useTimelineEditCallbacks.ts:289)	CLOSED	handleGsapUpdateMeta now returns Promise<boolean> via observeGsapMutation (useGsapSelectionHandlers.ts:150-173); flat boundary branch at useTimelineEditCallbacks.ts:298-302 return handleGsapUpdateMeta(...) propagates it. New test reports an unsettled flat-boundary retime as uncommitted at useTimelineEditCallbacks.test.tsx:228-246 with mockResolvedValueOnce(false) asserts .resolves.toBe(false).
5	Selector-based .find in commitFlatViaKeyframes (gsapDragPositionCommit.ts:214)	CLOSED	Now id-first at :216-217: fresh.find((a) => a.id === anim.id && a.keyframes) ?? fresh.find((a) => a.targetSelector === anim.targetSelector && a.keyframes) ?? anim. Correct fallback ordering; comment names the multi-tween-same-selector risk.
6	onDeleteAllKeyframes picks only FIRST keyframed anim (useTimelineEditCallbacks.ts:197)	CLOSED	Now .filter(a => a.keyframes) + serial for (const anim of anims) await handleGsapRemoveAllKeyframes(anim.id, selection) at useTimelineEditCallbacks.ts:197-207; comment names the per-source-file conflict driving serial. New test deletes all keyframes on every keyframed tween of the layer, not just the first at useTimelineEditCallbacks.test.tsx:334-360 seeds two keyframed tweens (position + opacity) and asserts both fire.
7	aria-controls missing on disclosure trigger (LayerDisclosureRow.tsx:37)	CLOSED	Added at LayerDisclosureRow.tsx:45: aria-controls={lanesId}. TimelineTrackHeader.tsx always mounts <div id={lanesId}> at :341 regardless of isExpanded, so aria-controls resolves in both states.
8	WCAG 24×24 target-size on 2 buttons (property-group toggle at TimelineTrackHeader.tsx:239; disclosure caret at LayerDisclosureRow.tsx:40)	CLOSED	Property-group toggle: h-5 w-4 → h-6 w-6 at TimelineTrackHeader.tsx:240. Disclosure caret: h-5 w-4 → h-6 w-6 at LayerDisclosureRow.tsx:48. Glyph stays 11px per the added comments.
9	LayerDisclosureRow hardcodes LABEL_COL_W (line 29)	CLOSED	LABEL_COL_W import removed at :3; row now takes adaptive columnWidth prop at :20/:29. Parent passes showTrackLabel ? LABEL_COL_W : contentOrigin at TimelineTrackHeader.tsx:324 — matches sibling lane rows.
10	Proximity keying in React keys (TimelineClipDiamonds.tsx:389; TimelineDiamondConnectors.tsx:66)	PARTIAL	TimelineClipDiamonds.tsx:399 key is now ${kf.animationId ?? i}:${kf.propertyGroup ?? ""}:${kf.tweenPercentage ?? kf.percentage} — stable authored identity, comment names the pointer-capture-remount risk. TimelineDiamondConnectors.tsx:66 (blob unchanged, a06892...) still keys line-${i}-${previous.keyframe.percentage}-${kf.percentage} — but this is a <Fragment> key, not a host-element button, so a remount does not cost pointer capture. Lower-severity than the button case; not a blocker.
13	expect.any(Number) matches NaN (useGsapSelectionHandlers.test.tsx:143)	CLOSED	Now .toHaveBeenCalledWith(selection, "anim-1", 50, 25) at :152 — exact number. Test now seeds position: 2, resolvedStart: 2, duration: 4 and currentTime: 3 so 25% is the only correct answer (the added comment names the NaN failure mode).
14	Duplicate describe block (useGsapSelectionHandlers.test.tsx:119 and :167 byte-identical)	CLOSED	Second describe block removed (lines 163-181 in R2 → gone in R3).
15	Missing explicit-null-abort at callback layer (useTimelineEditCallbacks.test.tsx:338)	CLOSED	New test aborts every mutation when the clicked element resolves no selection at useTimelineEditCallbacks.test.tsx:361-388 — mocks buildDomSelectionForTimelineElement.mockResolvedValue(null), fires onDeleteAllKeyframes, onMoveKeyframeToPlayhead, onDeleteKeyframe, asserts none of handleGsapRemoveAllKeyframes / handleGsapMoveKeyframeToPlayhead / handleGsapRemoveKeyframe fire. Callback-layer null guard now pinned. (The removed handler-layer duplicate of this test in useGsapSelectionHandlers.test.tsx is fine — resolveWriteSelection still enforces the guard; the callback-level test covers the failure mode from the surface a real user would hit.)
16	Rapid-second-retime crossing (TimelineClipDiamonds.test.tsx:313) — no crossing scenario	OPEN (test-coverage nit)	The composes a rapid second retime from the pending position test at :396-473 still stages 50%→75%→85% with the 100% keyframe uncrossed. Non-blocker: identity keying by animationId + propertyGroup + tweenPercentage (see item 10) removes the sort-order-swap risk mechanically.
17	Track-header a11y assertions absent (TimelineTrackHeader.test.tsx:121)	OPEN (test-coverage nit)	Blob unchanged (46d4372b...). No test coverage added for aria-controls={lanesId}, the always-mounted <div id={lanesId}>, or the h-6/w-6 target-size upgrade. Non-blocker; the a11y attributes are static string literals in the source.

Skipped items 11 (playerStore.reset() pre-existing) and 12 (tolerance-literal naming) — not this PR's introduction and not touched by the delta.

New tests

TimelineClipDiamonds.test.tsx (+83/-0) — one new test: leaves the selection alone when a stale retime fails after a newer drag (lines 313-395). Stages two back-to-back drags on the 50% diamond in a 3-keyframe row (0/50/100 → 100). First drag's onMoveKeyframe promise is held open via a captured failFirstDrag resolver; second drag runs while the first is pending. Then the first drag rejects with false, and the test asserts onClickKeyframe was NOT called — proving the stale revertRetime no longer parks the selection back on the first drag's source now that latestRetimeRef gates it.

useTimelineEditCallbacks.test.tsx (+102/-9) — the additions:

• Element-discriminating selectionForElement(el) helper (:127-137) + mockImplementation((el) => Promise.resolve(selectionForElement(el))) beforeEach (:154-157); three existing assertions upgraded from mocks.selection to selectionForElement(circle).
• New test reports an unsettled flat-boundary retime as uncommitted (:228-246): mocks handleGsapUpdateMeta.mockResolvedValueOnce(false), asserts onMoveKeyframe?.(...) resolves to false — locks the return-plumbing fix on the flat-tween boundary branch (item 4).
• New test deletes all keyframes on every keyframed tween of the layer, not just the first (:334-360): seeds two keyframed animations (position + opacity), asserts handleGsapRemoveAllKeyframes fires for BOTH ids in order — locks the .filter + serial-await fix (item 6).
• New test aborts every mutation when the clicked element resolves no selection (:361-388): mocks buildDomSelectionForTimelineElement.mockResolvedValue(null), fires the three mutating callbacks, asserts none of the handler calls fire — locks the callback-layer null-abort (item 15).

Also lifted mocks: handleGsapUpdateMeta and handleGsapRemoveAllKeyframes now return Promise<boolean> in the mock setup (:19, :25), matching the real handler shape.

Byte-clean on unchanged surface

Spot-check on 12 of the largest untouched .ts/.tsx files in the PR — every one has identical blob SHA at c20c5366 and 8470b88a:

• Timeline.test.ts 81b09596...
• TimelineLanes.tsx d8b622f2...
• useGsapTweenCache.ts 0f89be65...
• keyframeCacheAstLoad.ts bca5f4b2...
• useTimelineTrackLayout.ts 4aad5a87...
• TimelineDiamondConnectors.tsx a0689213...
• timelineLayout.ts 0ac3cb85...
• Timeline.tsx 5279eeb3...
• useTimelineGroupEditing.ts 03abbb69...
• usePopulateKeyframeCacheForFile.test.tsx 6d2f609b...
• TimelinePropertyLanes.tsx 018aa4f8...
• trackHeaderLaneValues.ts 2ae0fe76...

Delta is tightly scoped to the 10 files the fix commit names. No collateral churn.

Standards lens re-run

On the 10 changed files at 8470b88a:

• Silent-catch + error-invariant. observeGsapMutation now returns Promise<boolean> where before it swallowed rejections (void mutation.catch(...)). Rejection is still caught via .then(_, error), and trackGsapHandlerFailure(error, selection, mutationType, label) still fires — telemetry parity preserved. No caller drops the returned promise now that flat-tween-boundary and delete-all-keyframes actually consume it. Empty.
• Commit semantics. onDeleteAllKeyframes iterates serially with for (const anim of anims) await handleGsapRemoveAllKeyframes(...); comment names the per-source-file conflict (later writes would read pre-edit document if parallel). Correct. Empty.
• Key stability. Diamond button key is now stable authored identity (animationId + propertyGroup + tweenPercentage), immune to sort-order swap during retime. Fragment key on connectors (item 10 partial) is Fragment, not host — remount is cheap. Empty.
• ARIA. aria-controls={lanesId} added; <div id={lanesId}> mounted in both expanded and collapsed states (comment names why: "so the caret's aria-controls resolves in both states"). No orphaned aria-controls. Empty.
• Keyboard. Target size on both audit-flagged controls now h-6 w-6 = 24×24 (WCAG 2.2 minimum), glyphs still 11px so visual weight is unchanged. Empty.
• Propagation. Existing event.stopPropagation() on the caret unchanged. Empty.
• Semantic-vs-symptom. Test additions are all semantic (assert the intent — landed/didn't-land, all-tweens-cleared, selection-not-parked) rather than surface-shape (asserting call counts alone). Diamond-identity assertion is the full identity object, not objectContaining on one field. Empty.
• Sibling helpers. resolveWriteSelection (handler layer) unchanged; new callback-layer test proves the null contract from the caller-facing surface. Correct level to lock. Empty.
• Parity audit. handleGsapUpdateMeta and handleGsapRemoveAllKeyframes both promoted to Promise<boolean> return in useGsapSelectionHandlers.ts (:169-175, :425-438); mocks in the test files match (:19, :25). Every other handler that already returned Promise<boolean> (handleGsapMoveKeyframe, handleGsapResizeKeyframedTween) is unchanged and their return-type parity preserved. Empty.
• Cross-mode. Both expanded and collapsed layer states share the <div id={lanesId}> mount and the aria-controls binding. Item 10's key fix applies to both diamond render paths. Empty.
• Perf audit. Serial iteration in onDeleteAllKeyframes is intentional (comment names the reason); a layer typically has at most a handful of keyframed tweens, so total latency is bounded. latestRetimeRef is a single useRef on the lane, no allocation churn. Empty.
• PR-body-vs-diff. Commit message "settle boundary retimes, delete every keyframed tween, tighten the test locks" — all three claims verify against the diff (items 4, 6, and 1+3+13+14+15 respectively). Empty.

Zero standards-lens findings on the delta.

Peer state

Rames posted his R3 confirmation review (4792353691) ~30 minutes after his R2 review, converging APPROVE-shape ("R1 concerns resolved. Delta commit 8470b88aa addresses every test-lock-in and race concern I flagged, plus three writer-layer bugs Miguel caught himself. … Clean execution on the delta. R1 test-lock-in family fully resolved.") We're aligned on all three of his 🟠 findings (items 1, 2, 3) and on the writer-layer trio (items 4, 5, 6). No adversarial split.

— Review by Via

The base branch was changed.