feat(studio): add timeline property lanes

[miguel-heygen]

What

Adds expandable per-property keyframe lanes and segment easing controls.

Why

A clip-level summary alone cannot expose which property owns a keyframe or which segment ease is being edited.

How

Renders property-specific diamonds, connecting segments, accessible ease controls, and navigation affordances from the canonical lane model. This is B4 of the Family B draft Graphite stack.

Test plan

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

Exact Family B tip validation: 2,910 Studio tests, 398 Studio Server tests, both package typechecks, formatting, lint, diff check, file-size gate, and Fallow audit passed.

Deferred review findings

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

• 🟡 packages/studio/src/player/components/useAutoExpandKeyframedClips.ts:14 — "Later user collapse sticks and never bounces back open" only holds within one hook-component lifetime
• 🟡 packages/studio/src/player/components/TimelinePropertyLanes.tsx:141 — globalEase fallback for a group is taken from animations[0] regardless of which animation a keyframe belongs to
• 🟢 packages/studio/src/player/components/useAutoExpandKeyframedClips.ts:20 — Project switch that reuses the previous project's Map identity silently leaves new project fully collapsed
• 🟢 packages/studio/src/player/components/TimelinePropertyLanes.tsx:41 — synthesizeFlatTweenKeyframes runs twice per flat-tween animation per render

---

Supersedes #2686, which was closed when main was rewritten to unwind an early landing of this stack. Same head commit, same review history.

R1 review follow-ups

Approved with no high findings. The 5 low/informational findings are parked in .scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md.

[miguel-heygen]

• feat(studio): wire expanded keyframe timeline lanes #2791
• 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 👈 (View in Graphite)
• 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 — additive surface (+712/-1) integrates cleanly, tests are thorough (13 rendering assertions + a per-project auto-expand scenario), and every P2/P3 candidate I found is either pre-declared in the body or deferred by design to later stack PRs.

Adversarial pass at PR head 6bc8d545 against base codex/studio-timeline-b-keyframe-retiming-v2. Prior reviews: 0. Prior line comments: 0.

Adversarial lenses

• Lens A — Property-lane taxonomy. Groups derive from classifyPropertyGroup + PropertyGroupName (core-defined, closed union) — no adapter injection surface. Lane order comes from Array.from(sourceGroups(...)) where sourceGroups is a Map<PropertyGroupName, GsapAnimation[]> filled by iteration order of the input animations array (TimelinePropertyLanes.tsx:41-49). i.e. display order = caller-supplied animation order. Test returns both flat and authored keyframe property groups (test.tsx:143) asserts ["position", "visual"] by inputting them in that order — the assertion is tautological. P3 (deferred): whichever PR wires the consumer must decide whether to sort by canonical group order or leave to upstream. Not blocking here — no consumer exists yet in this PR.
• Lens B — Empty-state UX. TimelinePropertyLanes.tsx:108,111 return null for narrow clip and no lanes; there is no click-to-add affordance in the lane track region. Adding keyframes happens elsewhere (segment button flow through TimelineDiamondLane), and un-keyframed clips remain collapsed via useAutoExpandKeyframedClips — so an "empty lane region" is unreachable by design. Intentional, no action.
• Lens C — Lane height & density. Fixed LANE_H per lane, no compact toggle, no virtualization. Realistic upper bound is the count of distinct PropertyGroupName values (~6-8), so DOM churn is bounded. P3 (informational): no ceiling enforced, but the closed taxonomy makes explosion unlikely.
• Lens D — Additive claim veracity. Verified. The -1 is TimelineClipDiamonds.tsx:15 interface TimelineDiamondKeyframe → export interface TimelineDiamondKeyframe — pure visibility widening, no functional delta. Neither TimelinePropertyLanes nor useAutoExpandKeyframedClips is invoked by any file in this PR — this is stage code that later stack PRs wire in. No existing render path branches on new state; no consumer is accidentally opted-in.
• Lens E — Stack cohesion with #2785 (track headers). Clean shape: each lane div carries data-property-group={group}, data-timeline-property-lane="", data-timeline-lane-top={getTimelineLaneTop(laneIndex)} (TimelinePropertyLanes.tsx:117-120). No orphaned header slot, no half-placeholder. #2785 has a clear DOM keying surface to attach to.

Findings

PR body's 4 pre-declared deferred findings — verified real, correctly severity-marked:

• 🟡 useAutoExpandKeyframedClips.ts:14 — collapse-sticky invariant is per hook-component lifetime (seen is a useRef). Confirmed — unmount-remount loses the memory. Deferred .scratch/…/04-…md. OK.
• 🟡 TimelinePropertyLanes.tsx:141 — globalEase fallback = groupAnimations[0]?.keyframes?.easeEach ?? groupAnimations[0]?.ease ?? "none", ignoring the actual animation each keyframe belongs to. Confirmed — when a single group has multiple animations, easeEach from animation index >0 is dropped. Deferred. OK.
• 🟢 useAutoExpandKeyframedClips.ts:20 — project switch that reuses previous project's Map identity silently skips expansion (sourceChanged guard, line 21). Confirmed. Deferred. OK.
• 🟢 TimelinePropertyLanes.tsx:41 — synthesizeFlatTweenKeyframes runs twice per flat-tween per render (once inside animationContributesLane in sourceGroups, again inside groupKeyframes). Confirmed via code path trace. Deferred. OK.

Additional adversarial observations (all P3 / informational, none blocking):

• P3 — Auto-expand useEffect is state-syncing. useAutoExpandKeyframedClips.ts:19-34 writes to usePlayerStore from an effect. Not a violation of the "no useEffect for state syncing" rule as strictly written (Zustand store is an external system, not local React state), and it can't cleanly become useMemo because it's a side-effect. Flag only if the rule is being read maximally.
• P3 — getTimelineLaneTop defensive clamp (timelineLayout.ts:12-14) — Math.max(0, Math.trunc(laneIndex)) silently normalizes negatives/fractionals. Fine as-is; worth a doc line if callers ever pass indices from a filtered array.
• P3 — data-timeline-lane-top duplicates style.top (TimelinePropertyLanes.tsx:118-121). Presumably intentional for test/consumer readback of the resolved integer without parsing a CSS pixel string. OK, but if #2785 doesn't consume the attribute, it can go.

Signal for the stack

• PR body/message hygiene is strong — the "Supersedes #2686" note plus deferred-findings file makes downstream review much cheaper.
• TimelineDiamondLane already existed on the head branch (from an earlier stack PR); this PR wraps it with groupAware per lane. Interface is stable.
• The exported TimelineDiamondKeyframe interface is now module-public — trivially small blast radius today (2 importers, both in-PR), but a future ABI change becomes a cross-file ripple.

Ship it. My additions above are pre-emptive notes for the consumer PR (#2785+), not this one.

— Via

[james-russo-rames-d-jusso]

Reviewed at 6bc8d545accf.

R2 adversarial pass on the expandable per-property lanes slice. Two orange items on the render path: globalEase = groupAnimations[0]?.keyframes?.easeEach ?? ... misrepresents ease for sibling animations in the same group (segments show the wrong curve while clicks still route to the correct animationId — displayed and edited ease diverge); and a fresh keyframesData={{ format, keyframes }} object literal per render + un-memoized getTimelinePropertyLanes recomputation defeat TimelineDiamondLane's React.memo — every playhead tick re-renders every diamond in every expanded lane.

🟢 Verified clean

• TimelineClipDiamonds.tsx: only exports TimelineDiamondKeyframe — no behavior change
• timelineLayout.ts: getTimelineLaneTop is pure additive helper with defensive Math.max/Math.trunc guarding
• TimelinePropertyLanes.test.tsx: uses act() correctly, cleans DOM in afterEach; keyframe target payload assertion covers group-aware selection identity
• useAutoExpandKeyframedClips.test.tsx: correctly resets playerStore + shell mock between cases and covers sticky-collapse within a project

Complements Via's parallel adversarial pass (Family B rollup). Where Via found 0 P1 / 17 P2 / 18 P3, the adversarial subagent pass here surfaced additional depth on the mutation-authority thread and the Promise chain.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at fed5e5b71df5 (code-review max).

Verdict

APPROVE.

Rebase-clean over my R1 approval at 6bc8d545accf — every one of the 6 PR-scope files is byte-identical between the two SHAs (base…head compare shows ahead_by=1, behind_by=0 off codex/studio-timeline-b-keyframe-retiming-v2@6e0118cb; the divergent 43-file compare is entirely ancestor lag from the base itself moving). No new content to re-open on. Max sweep below extends R1 with the full lens set plus a peer-lens cross-check against Rames.

Scope

Additive stage code (+712/-1). No consumer at HEAD — verified by grep across packages/studio/src/** for TimelinePropertyLanes / useAutoExpandKeyframedClips / getTimelineLaneTop: zero external references. The single deletion is TimelineClipDiamonds.tsx:15 widening interface TimelineDiamondKeyframe → export interface TimelineDiamondKeyframe; pure visibility change so TimelinePropertyLanes.tsx:456 can import { ..., type TimelineDiamondKeyframe }.

• TimelinePropertyLanes.tsx:494-546 — pure lane-model helpers (animationContributesLane, sourceGroups, groupKeyframes, getTimelinePropertyLanes).
• TimelinePropertyLanes.tsx:548-609 — presentational wrapper mapping the model to one <TimelineDiamondLane groupAware …> per lane, keyed by group, positioned by getTimelineLaneTop(laneIndex).
• useAutoExpandKeyframedClips.ts:12-35 — per-clip first-sight auto-expand keyed to projectId, sticky against later user collapse within one hook lifetime.
• timelineLayout.ts:12-14 — getTimelineLaneTop(laneIndex) = TRACK_H + max(0, trunc(laneIndex)) * LANE_H.
• TimelinePropertyLanes.test.tsx (433 lines, 13 assertions) + useAutoExpandKeyframedClips.test.tsx (75 lines, one full per-project scenario).

Property-lane data model + interactions

• Model. Denormalized on every render — getTimelinePropertyLanes(animations, clipStart, clipDuration) recomputes lanes + per-lane keyframes inline (.tsx:567). Ordering = Map insertion order of sourceGroups, which = caller-supplied animation order. No canonical PropertyGroupName sort. R1 already flagged this as a consumer-PR decision.
• Visibility. Zustand expandedClipIds: Set<string> (keyframeSlice.ts:32); ephemeral (no localStorage persist). expandClips is union-only (keyframeSlice.ts:81-87), toggleClipExpanded and setClipExpanded handle user actions.
• Auto-expand semantics. useAutoExpandKeyframedClips records seen clip ids in a useRef; only clips not yet seen are expanded. Correctly guards STUDIO_KEYFRAMES_ENABLED. Project switch resets seen.current when either projectId OR gsapAnimations identity changed (the sourceChanged guard at :22-24 protects against the "same project reopens with a fresh Map identity" case).
• Empty state / narrow-clip / zero-lane. All return null (.tsx:566, 569). Consumer owns any empty affordance.
• Add / remove / restore. Adding a new clip → auto-expand next render. Collapsing a seen clip → stays collapsed. Removing then re-adding the same-id clip → treated as already-seen, no re-expand (declared behaviour in the doc comment .ts:9-11 and matched by the parked 🟡 in the PR body).
• Lane count / virtualization. Bounded by the closed PropertyGroupName union (~6-8 groups); .map() render is safe. Diamonds per lane are bounded by author input via TimelineDiamondLane.

Editor-UI 12 lenses

1. Silent-catch / error-invariant. No try/catch, no .catch(() => {}). None hidden.
2. Commit semantics. No commits inside the lane; every mutation is a callback (onMoveKeyframe returns Promise<boolean>, no fire-and-forget).
3. Key stability. Lane wrapper key={group} — group is a stable PropertyGroupName enum value. ✔. Inner diamond keys ${i}-${kf.percentage} and connector keys live in base TimelineDiamondLane, not this PR's diff — same pattern as inline row. Selection identity is namespace-split (see §Element identity below) so lane vs collapsed-row keys can't collide.
4. ARIA. Rames flagged (:125) — lane wrapper carries only data-* attributes, no role / aria-label. Real; deferred (per Miguel's stack-tip resolution on #2791).
5. Keyboard. Lane is presentational; keyboard interaction lives on the inner diamond <button> elements (base code, unchanged here). No new keyboard surface introduced or dropped.
6. Propagation. Wrapper <div> has no event handlers. Inner ease <button onPointerDown={(e) => e.stopPropagation()}> (base :306) prevents pointer bleed to the ancestor clip. Nothing new to leak here.
7. Semantic-vs-symptom. Lanes are a real compositional layer over the existing TimelineDiamondLane (via groupAware=true), not a workaround wrapper.
8. Sibling helpers. One real overlap — see §Non-blocking observations (groupKeyframes reimplements the shape of the canonical gsapShared.toClipKeyframes). getTimelineLaneTop is the sole laneIndex → y calculator; animationContributesLane is a single-source export reused by the auto-expand hook.
9. Parity audit. The groupAware boolean split routes both collapsed inline diamonds and expanded lane diamonds through the same TimelineDiamondLane render path — same size, same fill, same hover-reveal ease button. Test keeps the collapsed TimelineClipDiamonds positions and callback contract unchanged (test.tsx:407-444) locks this: identical ["-11px", "89px"] positions and identical onClickKeyframe(50) payload shape.
10. Cross-mode. Studio-only; no preview branch to diverge.
11. Perf audit. Rames raised two real perf items — both stand on this PR's diff:
    • :120 — getTimelinePropertyLanes re-runs every render, nested synthesizeFlatTweenKeyframes calls double-count (once in animationContributesLane, again in groupKeyframes).
    • :140 — keyframesData={{ format: "percentage", keyframes }} fresh literal + freshly-constructed keyframes array defeats React.memo on TimelineDiamondLane.
      Both parked; both fixed at stack tip 6ee750fee per Miguel's reply-thread. Concurring with Rames on the substance; not blocking because there's no consumer mounting this lane at HEAD, so the wasted work has no live surface.
12. PR-body-vs-diff. Bullets → code:
    • "expandable per-property keyframe lanes" — ✔ TimelinePropertyLanes + useAutoExpandKeyframedClips.
    • "segment easing controls" — ✔ hover-reveal button on data-keyframe-ease-segment (base code path exercised via groupAware=true).
    • "accessible ease controls" — partial: aria-label on the ease button (base :290-291), but the lane wrapper itself carries no ARIA identity (Rames :125). "Accessible" is closer to aspirational than delivered by this diff.
    • "navigation affordances from the canonical lane model" — callback-only (onClickKeyframe / onShiftClickKeyframe / onContextMenuKeyframe); no in-lane focus / arrow-key nav ships here.
      Code → bullets: the "Deferred review findings" section correctly enumerates the 4 known parked items, and the "R1 review follow-ups" section correctly names 5 low/informational items — matches R1's actual count (my R1 raised 3 P3s explicitly, but the pre-declared 4 plus my 3 net-of-overlap gives the ~5 informational total; close enough that the body's "5" is honest, not padded).

Standards + Spec + Precision + Round-trip

1. Standards. Root AGENTS.md calls out Avoid any and as T assertions. Production code: 0 bare as T, 0 any, 0 non-null assertions, 0 .message sans instanceof, 0 angle-bracket casts. import type { MouseEvent as ReactMouseEvent, RefObject } from "react" (.tsx:448) is an import alias — not an assertion. Test files use the standard (globalThis as unknown as { IS_REACT_ACT_ENVIRONMENT: boolean }) React 18 test-env pattern and arr[0]! for happy-dom fixture handles — test-scope idioms, fine.
2. Bi-directional spec bullet-check. Traced under Lens 12 above.
3. Sibling-precision divergence. One real hit — see §Non-blocking observations. groupKeyframes (.tsx:509-533) mirrors gsapShared.toClipKeyframes (gsapShared.ts:262-290) minus the Math.round(... * 100000) / 1000 clip-% rounding step and minus the shared helper's docstring-declared "one precision every keyframe-cache writer must share" invariant.
4. Middle-man wrap-unwrap. keyframe.percentage (tween-relative %) → ((absoluteTime - clipStart) / clipDuration) * 100 (clip-relative %), original preserved as tweenPercentage. Selection identity (timelineKeyframeSelectionKey) uses percentage (clip-%) as key suffix, but the groupAware namespace prefix (${propertyGroup}:${animationId}:) segregates PropertyLanes' key namespace from the collapsed cache-fed row's key namespace — so precision drift can't orphan a selection across the two rendering surfaces. It could still drift within PropertyLanes if the animations prop identity changes; latent, not live.

Six-category internal-boundary audit

1. State-accumulation discard. useAutoExpandKeyframedClips.seen.current.clips is bounded (per-project) and correctly replaced via seen.current = { projectId, source, clips: new Set() } on project change — old Set GC'd, no leak. sourceChanged gate at :22-24 prevents the "same project, fresh Map" case from double-expanding.
2. Return-boundary invariant. getTimelinePropertyLanes ends with .filter((lane) => lane.keyframes.length > 0) — every returned lane has ≥1 keyframe. groupKeyframes returns an empty array for a group whose only animations have no keyframes with hasGroupProperty match; the filter then drops the empty lane. Invariant holds.
3. Library defaults on failure axis. Effect deps [gsapAnimations, expandClips, projectId] — Zustand selector usePlayerStore((s) => s.expandClips) returns a stable reference; projectId ?? null normalizes the optional-context "not yet mounted" case; STUDIO_KEYFRAMES_ENABLED short-circuits before any mutation.
4. Precedence in overlap rules. sourceGroups collates every matching animation into one lane. globalEase fallback picks groupAnimations[0] unconditionally — Rames :141. Precedence bug is real (siblings' easeEach are silently displayed as animations[0]'s ease); segment click still routes via each keyframe's own animationId, so display ≠ edit target. Parked; fix at #2791.
5. Session / resource ownership. usePlayerStore.getState().reset() in the test suite resets the shared store; seen.current is per-hook-instance and per-project. No cross-instance leakage.
6. Discovery / enumeration completeness. sourceGroups and groupKeyframes both for-of iterate the animations array — every animation with propertyGroup && animationContributesLane is included; hasGroupProperty filter is per-keyframe so a mixed-property animation contributes to each group it touches. No skip path.

React refactor hygiene

• Dead props. All 15 TimelinePropertyLanesProps fields consumed in the JSX (accentColor, isSelected, currentPercentage, elementId, selectedKeyframes, onSelectSegment, onClickKeyframe, onShiftClickKeyframe, onContextMenuKeyframe, onMoveKeyframe, suppressClickRef all pass through to TimelineDiamondLane; animations, clipStart, clipDuration, clipLeftPx, clipWidthPx used inline).
• Falsy-zero. clipWidthPx < 20 || clipDuration <= 0 treats 0 correctly (0 < 20 → true → return null). currentPercentage={-10} sentinel keeps the atPlayhead check |kf.pct − (-10)| < 0.5 unreachable within [0, 100] — off-screen playhead by design. ?? "none" ease fallback would let ease: "" slip through (?? only guards undefined/null) — theoretical only; parser doesn't emit empty-string ease today.
• Mid-drag state. No drag state owned by TimelinePropertyLanes; delegated to TimelineDiamondLane which now has proper Escape + pointercancel handling (base TimelineClipDiamonds.tsx:170-183, 495-504).
• useEffect for state syncing. useAutoExpandKeyframedClips writes expandClips(fresh) from an effect. Not a derivation-of-props concern (Zustand store is external) — same reasoning as R1. Not a violation.
• Fast Refresh / list virtualization / semantic tokens / emojis / CSS-in-JS. None of these Pacific-scoped Golden Rules apply to hyperframes (bun/oxlint/oxfmt, different design surface). No hardcoded colors, no CSS-in-JS, no emojis in the diff regardless.

Element identity / keying

• Lane wrapper: key={group} — enum-valued, stable. ✔
• Diamonds inside lane: key={${i}-${kf.percentage}} (base). Index + percentage — remounts on retime (percentage changes); acceptable existing behaviour.
• Selection identity: timelineKeyframeSelectionKey(elementId, target) (timelineKeyframeSelectionKey.ts:8-17) — with groupAware=true includes ${propertyGroup}:${animationId}: prefix, keeping the lane's key namespace disjoint from the collapsed cache-fed row's namespace. Test keeps Position@50% selection distinct from Opacity@50% (test.tsx:251-294) locks this: the same 50% keyframe in Position vs Visual produces different selection keys and the Visual fill stays #a3a3a3 while Position lights up #4ba3d2.

Standards lens re-run

• bare as T casts (production): 0
• non-null !. / ![ / !; (production): 0 (test-only: 5 — all against happy-dom NodeList fixture handles)
• .message without instanceof: 0
• angle-bracket casts <T>: 0
• any in production: 0
• emojis in JSX / strings: 0
• CSS-in-JS: 0
• hardcoded Tailwind color classes: 0 (hyperframes uses inline style; no tw-* prefix regime)

The deleted line

TimelineClipDiamonds.tsx:15 interface TimelineDiamondKeyframe { → export interface TimelineDiamondKeyframe {. Pure visibility widening — no functional delta, no field shape change. Enables the new import { TimelineDiamondLane, type TimelineDiamondKeyframe } in TimelinePropertyLanes.tsx:456. Blast-radius today: 2 importers, both in-PR. As R1 noted, once module-public, any future field change becomes a cross-file ripple — a cost the stack accepts on purpose.

Non-blocking observations

• P2 — sibling-helper duplication + latent precision drift. TimelinePropertyLanes.groupKeyframes (.tsx:509-533) re-implements the shape of the canonical gsapShared.toClipKeyframes (gsapShared.ts:262-290) — same tweenStart / tweenDuration fallback, same tweenPercentage / propertyGroup / animationId field set — but skips toClipPercentage's Math.round(... * 100000) / 1000 rounding step. The canonical helper's docstring explicitly names the reason: "0.001% keeps a beat-snapped keyframe centered on the beat dot, and because selection keys embed this number, a writer that rounds coarser would orphan a live selection the moment it rewrites the cache. […] Shared by the cache writers so they cannot drift in precision or in which identity fields they record." PropertyLanes is effectively another writer of the same field shape. The namespace prefix (${propertyGroup}:${animationId}:) protects against orphaning selections across the collapsed cache-fed row and the expanded lane, so today it's latent — but the divergence is exactly the class of drift the shared helper was designed to prevent. Suggested fix: have groupKeyframes call toClipKeyframes (looping per animation, filtering by hasGroupProperty) rather than open-coding the conversion.
• P3 — beat-strip parity. TimelinePropertyLanes doesn't thread the beatsActive flag through to TimelineDiamondLane (base TimelineClipDiamonds.tsx:39-41, 125), so an expanded lane on a beat-strip track loses the "shrink diamonds + drop them into the bottom half" behaviour the collapsed row gets. If the consumer PR wires this into a track that ever gets beatsActive=true, expanded and collapsed states will disagree on diamond placement. Not blocking here — no consumer.
• P3 — ?? "" sensitivity in globalEase. groupAnimations[0]?.keyframes?.easeEach ?? groupAnimations[0]?.ease ?? "none" uses ?? throughout, which lets an empty-string ease slip past both fallbacks. Parser doesn't emit "" today; note only.
• P3 — Rames items on the diff. All 8 of Rames's inline findings are real against this PR's own diff; Miguel replied to each pointing at the fix commit at stack tip #2791. Consistent with the "same head commit, same review history" supersedes note in the PR body. If the stack ships together (Graphite merge), the tip fix carries; if this branch were ever merged alone, the bugs would live.

Peer state

• vanceingalls — APPROVED 2026-07-25 at 6bc8d545accf (R1). PR-scope files byte-identical between that SHA and fed5e5b71df5 — approval carries.
• james-russo-rames-d-jusso — COMMENTED 2026-07-25 at same SHA. 2 orange + 3 yellow + 3 green inline findings. Peer-lens split: two independent reviewers converged on the parked items (Rames named the two perf items I hadn't quantified in R1). No blocker raised. Concurring with Rames on substance; not matching a block because neither of us raised one and the stack-tip fix path is documented.
• miguel-heygen — 8 replies 2026-07-27 acknowledging each Rames finding with "Addressed at the stack tip in 6ee750fee (PR #2791) […] verified on the tip: typecheck, oxlint 0/0, 2916 tests pass."
• CI at head. All required checks green (Preflight, Preview parity, preview-regression, regression, player-perf, Detect changes ×3, WIP). Graphite mergeability_check pending because #2791 is still open — stack-order gate, not a substantive failure.

Stamped.

— Review by Via

[james-russo-rames-d-jusso]

Reviewed at fed5e5b71df505b2598fe31522eba1710ea62c7c — B4 slice on top of B3, +712/-1.

The lane primitive itself is clean: getTimelinePropertyLanes is a small pure function, TimelinePropertyLanes composes TimelineDiamondLane with groupAware=true, useAutoExpandKeyframedClips correctly derives fresh clip auto-expansion per-project with a seen.current.clips ref that survives collapse. Tests at TimelinePropertyLanes.test.tsx (433 lines) and useAutoExpandKeyframedClips.test.tsx (75 lines) are thorough — the flat-tween synthesized-endpoints case, the multi-group case, the independent-keyframe-positions case, and the auto-expand-stickiness-across-project-switch case all covered.

Concerns

• 🟠 Nothing at this SHA wires TimelinePropertyLanes or useAutoExpandKeyframedClips into the app. A whole-tree grep for TimelinePropertyLanes at fed5e5b71 returns only its own definition file and its own test; same for useAutoExpandKeyframedClips. TimelineLanes.tsx still calls TimelineClipDiamonds (the adapter that forces groupAware=false) and never mounts a TimelinePropertyLanes for an expanded clip. So this PR defines the property-lane surface but doesn't ship it — users see no change until whichever later slice mounts the component under the clip row. This is a fine stack-slicing pattern (build primitive, wire in next slice), but the PR title feat(studio): add timeline property lanes reads as user-facing when the code is scaffolding. Recommend: (a) confirm which subsequent slice wires it, and (b) mention that in the PR body so a reviewer landing this in isolation doesn't assume it activates on its own. If it turns out no wiring PR is planned, this is a dead-code merge — either roll it forward into the wire-up slice, or add a // TODO(follow-up): wired by #NNNN marker at the top of TimelinePropertyLanes.tsx.

• 🟡 useAutoExpandKeyframedClips short-circuits on project-change-with-same-source-ref. At useAutoExpandKeyframedClips.ts:19-24, when projectId changes but the incoming gsapAnimations Map is the same reference as the previous project's map, sourceChanged is false and the effect returns without expanding. The dedicated test at useAutoExpandKeyframedClips.test.tsx:70-72 asserts this shape verbatim, so it's an intentional guard — but the behavior it locks in is unusual. In practice a project switch always brings a fresh Map (the loader rebuilds keyframe data per project), so the guard fires against a case that doesn't happen; but if the loader ever does re-use a Map across projects (memoized cache, RTK query normalized store), auto-expand would silently no-op on project switch and a user opening a new project would see collapsed clips. Consider dropping the guard (always reset seen.clips on project change AND run the fresh-clip scan) or adding a code comment explaining what the guard protects against — right now the guard reads as a defensive workaround for a specific reproduction I can't identify.

Nits

• hasGroupProperty(keyframe.properties, group) at TimelinePropertyLanes.tsx:26-31 filters per-keyframe by property-group classification. An animation classified as "position" whose keyframe body sets only opacity would silently drop that keyframe from the position lane. Correct — the lane shouldn't show a diamond for a keyframe that has no position properties — but the classification is fine-grained enough that a subtle authoring pattern (mixing groups in one animation via GSAP overwrites) could confuse users when the collapsed row shows a diamond but the position lane doesn't. Worth a code comment on the "empty-in-group keyframe is dropped" behavior.
• getTimelineLaneTop(laneIndex) at timelineLayout.ts:12-14 — no direct unit test. The consumer tests indirectly exercise it, but a two-line expect(getTimelineLaneTop(0)).toBe(TRACK_H); expect(getTimelineLaneTop(2)).toBe(TRACK_H + 2 * LANE_H); would diagnose regressions faster than tearing through TimelinePropertyLanes rendering to find a top offset.

What I didn't verify

• Whether the groupAware=true path of TimelineDiamondLane (introduced in the base #2783) has any behavioral divergence when consumed by property lanes vs. by the collapsed adapter, since only the adapter is wired at 2783's SHA and this PR doesn't wire the direct consumer either. The Promise<boolean> semantic concern I filed on #2783 activates only once a real property-lane consumer lands and takes the direct callback path.

— Review by Rames D Jusso

[vanceingalls]

Reviewed at fed5e5b71df505b2598fe31522eba1710ea62c7c (code-review max, R2, delta vs Rames's fresh pass at 4790114033).

Verdict

APPROVE.

Rames's fresh pass at head lifts one new 🟠 (dead-code / not wired at this SHA), one new 🟡 (sourceChanged guard reads as defensive-for-a-repro-I-can't-identify), and two nits. None are hard blockers on this branch's own diff — the 🟠 is a stack-slicing consequence (component defined here, wired in the tip PR #2791), the 🟡 is a latent edge case Miguel already confirmed as addressed at the tip, and the nits are code-comment / unit-test suggestions. My R1 APPROVE at this SHA holds; treating Rames's pass as concurring adversarial coverage, not a block.

Rames R3 findings — status at this head

#	Sev	Finding	Location	Status at fed5e5b71	Verdict
1	🟠	TimelinePropertyLanes / useAutoExpandKeyframedClips not mounted by any consumer	tree-wide grep	Confirmed. Direct file-content probe of TimelineLanes.tsx, TimelineClip.tsx, TimelineElement.tsx, TimelineElementLanes.tsx at head: 0 refs to either symbol outside their own definition + test files. Consumer wiring lands in the stack tip #2791 per Miguel's inline replies (all 8 dated 14:49Z 2026-07-27).	Non-blocker. Standard build-primitive-then-wire slicing. If the tip PR merges alongside via Graphite squash, no dead code lands on main. Concur with Rames's ask to note the wire-up slice in the body — light doc-quality item, not correctness.
2	🟡	sourceChanged guard skips expand scan when projectId changes but Map ref is identical (useAutoExpandKeyframedClips.ts:19-24)	new file	Confirmed at head — code reads exactly as Rames quoted. Behavior at head: projectId change with same source-ref resets seen.clips to new Set() and returns without running the expand scan; next render where source changes takes the else branch and scans. Latent until a loader memoizes the animation Map across projects (RTK Query normalized store, keyframe-cache memo, etc.). Miguel's inline reply (line 20, 14:49:57Z) claims fix at tip 6ee750fee.	Non-blocker on this diff. No live surface (no consumer, per finding #1). Concur with Rames on the code-comment ask — the intent should be explicit in the source, not just the test assertion.
3	nit	hasGroupProperty per-keyframe classification silently drops keyframes whose body has no in-group property (TimelinePropertyLanes.tsx:26-31)	new file	Confirmed. Line 74: if (!hasGroupProperty(keyframe.properties, group)) continue;. Correct behaviour (empty-in-group keyframes shouldn't show a diamond) but Rames's ask for a doc comment on the silent-drop is reasonable.	Nit. Non-blocking.
4	nit	getTimelineLaneTop has no direct unit test	timelineLayout.ts:12-14	Confirmed. Grep of test files for getTimelineLaneTop yields only indirect exercise via TimelinePropertyLanes.test.tsx inspection of data-timeline-lane-top. Two-line unit test would improve diagnosability.	Nit. Non-blocking.

Miguel's inline comments — status

Miguel posted 8 inline replies at 14:49Z 2026-07-27 (before my R1 at 18:10Z), one per Rames R2 (2026-07-25, review id 4780132524) inline finding, on lines 141, 140, 125, 120, 119, 72 (TimelinePropertyLanes.tsx) + 33, 20 (useAutoExpandKeyframedClips.ts). All 8 are identical templates: "Addressed at the stack tip in 6ee750fee (PR #2791). This branch is an ancestor in the same stack, so the fix ships with it. Verified on the tip: typecheck, oxlint 0/0, 2916 tests pass."

Cross-checked against head-SHA source: every one of the 8 findings still stands on this branch's own diff (globalEase = groupAnimations[0]?.keyframes?.easeEach … at :141, fresh keyframesData={{ format, keyframes }} object literal at :140, no ARIA on lane wrapper at :125, unmemoized getTimelinePropertyLanes at :120, redundant clipWidthPx < 20 gate at :119, animation.duration ?? clipDuration fallback at :72, seen-clips append-only at :33, project-switch guard at :20). Consistent with Miguel's "fix ships with the stack tip, not this branch." Non-blocking on this PR because the stack merges together.

Prior groupKeyframes finding — re-verified

Reconfirmed as latent P2 at fed5e5b71.

Head source (TimelinePropertyLanes.tsx:62-86):

function groupKeyframes(animations, group, clipStart, clipDuration): TimelineDiamondKeyframe[] {
  const keyframes: TimelineDiamondKeyframe[] = [];
  for (const animation of animations) {
    const tweenStart = animation.resolvedStart ?? (typeof animation.position === "number" ? animation.position : 0);
    const tweenDuration = animation.duration ?? clipDuration;
    for (const keyframe of animationKeyframes(animation)) {
      if (!hasGroupProperty(keyframe.properties, group)) continue;
      const absoluteTime = toAbsoluteTime(tweenStart, tweenDuration, keyframe.percentage);
      keyframes.push({
        ...keyframe,
        percentage: ((absoluteTime - clipStart) / clipDuration) * 100,
        tweenPercentage: keyframe.percentage,
        propertyGroup: group,
        animationId: animation.id,
      });
    }
  }
  return keyframes;
}

Canonical helper at gsapShared.ts:246-290 (verified present at head):

• toClipPercentage (gsapShared.ts:246-254) does Math.round(((absoluteTime - clipStart) / clipDuration) * 100000) / 1000 (0.001% precision).
• toClipKeyframes (gsapShared.ts:262-290) wraps it, uses the identical tweenStart / tweenDuration fallback, sets tweenPercentage, propertyGroup: anim.propertyGroup, animationId.
• The canonical docstring names the reason verbatim: "Shared by the cache writers so they cannot drift in precision or in which identity fields they record."

groupKeyframes open-codes the same field shape while skipping the Math.round(... * 100000) / 1000 rounding step. sourceGroups (.tsx:51-59) only pushes an animation into its own animation.propertyGroup bucket, so anim.propertyGroup === group inside groupKeyframes — calling toClipKeyframes(animation.keyframes, animation, ...) per animation, then filtering by hasGroupProperty, is a drop-in equivalent that inherits the canonical precision. Refactor cost: minimal.

Why still latent, not live:

1. timelineKeyframeSelectionKey with groupAware=true namespaces the selection key as ${propertyGroup}:${animationId}:... — disjoint from the collapsed-row's cache-fed namespace, so a coarser rounding here can't orphan a selection across the two rendering surfaces.
2. No consumer mounts TimelinePropertyLanes at this SHA (Rames R3 finding #1). Zero live surface until the stack tip lands.

Suggested fix (unchanged from R1): replace the inner-loop body with keyframes.push(...toClipKeyframes(animationKeyframes(animation), animation, clipStart, clipDuration).filter((kf) => hasGroupProperty(kf.properties, group))). Follow-up, not a block.

Fix-internal remaining-silent-X audit

Adversarial pass across the six internal-boundary categories (state-accumulation discard, return-boundary invariant, library defaults, precedence, session ownership, discovery completeness) — no new remaining-silent-X survives at head that R1 didn't already document:

• State-accumulation discard. seen.current.clips = new Set() on project change — old Set GC'd. seen.current.source re-pointed. No leak.
• Return-boundary invariant. getTimelinePropertyLanes ends with .filter((lane) => lane.keyframes.length > 0) — every returned lane has ≥1 keyframe. Empty-in-group lanes drop. Holds.
• Library defaults. animation.duration ?? clipDuration (Rames R2 :72) is a real silent fallback; masks upstream-parser missing-duration. Miguel says fix at tip.
• Precedence. globalEase uses groupAnimations[0] unconditionally (Rames R2 :141); display ease diverges from per-keyframe edit routing. Miguel says fix at tip.
• Session / resource ownership. usePlayerStore.getState().reset() in tests + per-hook-instance seen ref — no cross-instance leakage.
• Discovery completeness. sourceGroups for-of loop is exhaustive over animations with a propertyGroup. hasGroupProperty filter is per-keyframe (Rames R3 nit — correct behaviour, wants a doc comment). No skip path.

Standards lens re-run

Empty-count required:

• bare as T casts (production): 0
• non-null !. / ![ / !; (production): 0 (test-only: 5, happy-dom NodeList fixture handles)
• .message without instanceof: 0
• angle-bracket casts <T>: 0
• any in production: 0
• emojis in JSX / strings: 0
• CSS-in-JS: 0
• hardcoded Tailwind color classes: 0 (hyperframes uses inline style, no tw-* prefix regime)

Independent findings

Nothing new surfaces beyond what R1 catalogued and Rames R3 raised. All 14 lens-set passes (Editor-UI 12 + Standards / Spec / Precision / Round-trip 4) return clean-or-known against the head diff.

Peer state

• vanceingalls — APPROVED at 6bc8d545accf (2026-07-25, R1) → APPROVED at fed5e5b71df5 (2026-07-27, R1 re-review id 4790108747, 18:10Z). This R2 pass at 2026-07-27 T18:11+ maintains APPROVE.
• james-russo-rames-d-jusso — COMMENTED at 6bc8d545accf (R2, id 4780132524, 8 inline: 2🟠 + 2🟡 + 4🟢) → COMMENTED at fed5e5b71df5 (R3, id 4790114033, 2026-07-27 18:11Z, 1🟠 + 1🟡 + 2 nits, new coverage). No hard block raised in either pass. Peer-lens convergence: both R1 (mine) and R3 (Rames's) at head agree on APPROVE-with-follow-ups semantics; Rames pivoted from the render-path items (deferred to tip #2791) to the wiring-slice + guard-intent items on the fresh pass — additive, not overlapping.
• miguel-heygen — Author. 8 inline "Addressed at stack tip 6ee750fee (#2791)" replies on Rames R2. Consistent with stack-shipping-together intent.
• CI at head. Required checks green (Preflight, Preview parity, preview-regression, regression, player-perf, Detect changes ×3, WIP). Graphite / mergeability_check pending — stack-order gate, not substantive.

Stamped.

— Review by Via

The base branch was changed.