feat(studio): wire expanded keyframe timeline lanes#2791
Conversation
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
50b6822 to
92c23fd
Compare
e0f12da to
03d736e
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
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/focusedEaseSegmentread + write via store actions; auto-expand respects manual collapse (Timeline.test.tsline ~380 asserts collapse sticks). - timing/layout (#2782):
contentOriginthreaded throughgetTimelineFitPps,getTimelineDisplayContentWidth,getTimelinePlayheadLeft,getTimelineScrollLeftForZoomAnchor,resolveTimelineAssetDrop, marquee, range, geometry, and drag-drop. Zero residualGUTTER + TRACKS_LEFT_PADarithmetic outsideTimeline.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 inTimelineClipDiamonds.tsxnow 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 samerelativecontent div. Lanes areuseMemo'd — a playhead tick no longer re-renders every diamond in every expanded clip (was a real regression vector). - headers (#2785):
TimelineTrackHeaderdisclosure binds toexpandedClipIds; header sampler now uses per-segment ease (easedProgress— verified via new test atTimelineTrackHeader.test.tsx:198+assertingpower2.in(0.5) = 0.125 → 12.5/6.25, not linear). - callbacks (#2786): consolidated
useTimelineKeyframeHandlers— inlined lambdas removed fromTimeline.tsx. - hardening (#2787):
isStaticPositionHoldlifted togsapTweenSynth.tsas the single owner; bothuseGsapTweenCacheand the newkeyframeCacheAstLoadimport 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.
TimelinePropertyLanesnowuseMemo's bothlanesandlaneData— big win over the previous per-tick rebuild.getTimelineRowOffsetsgains aWeakMap<readonly number[], number[]>cache keyed by array identity; upstreamuseTimelineTrackLayoutmemoizesrowHeights, 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).resolveTrackKeyframeClipatTimelineLanes.tsx:214runs.filter + .sortper 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.
LayerDisclosureRowgainsaria-expanded; toggle button gainsaria-pressed+ state-specific labels ("Add {group} keyframe" / "Remove {group} keyframe"). Good WCAG alignment.- Chevron 12×20 buttons get a transparent
::beforeoverlay 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:
- Motion-path visibility widened (
useStudioContextValue.ts+PreviewOverlays.tsx) — called out in PR body. - AST-load extracted to
keyframeCacheAstLoad.tsto stay under the 600-line file gate — called out. gsapDragPositionCommit.tsswapsresolveTweenDuration→resolveEditableTweenDurationat 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—contentOriginjumps from 56 → 240 (184px) whenhasKeyframedClipsflips 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
resolveTrackKeyframeClipper-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.tsxreturns[clip, propertyLanes]from the outer.mapnon-passenger branch. Works because both nodes carry stable keys; a<Fragment key={clipKey}>reads a shade more explicitly.getTimelineRowOffsetscache: JSDoc note is the guardrail. If a future caller mutatesrowHeightsin place (readonly is types-only), stale offsets surface silently — considerObject.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
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
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:
- Per-segment custom cubic-bezier curve editor — only named eases are surfaced via keyframe.ease/easeEach.
- Spring physics (mass/stiffness/damping) — no store shape or UI.
- Overshoot/back easings surfaced as adjustable parameters rather than fixed ease names.
- 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.
- 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. - Per-property (not per-group) lane splitting — 'position' is one lane even when x and y have distinct keyframe timings.
- Copy/paste of keyframes (and paste-to-playhead) with selection preservation.
- 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.
- 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).
03d736e to
f3f242d
Compare
1f73607 to
5f9725e
Compare
f3f242d to
82735ce
Compare
- revert diamond selection when a rejected retime leaves the source in place - clear project-local ease focus and expansion on player store reset - share one static-position-hold predicate across the tween cache - invalidate the GSAP cache even when a group timing rewrite throws - stamp each lane keyframe's ease from its own source tween - memoize property lanes and row offsets so memo'd diamond lanes hold - use the editable tween duration for drag position commits - restore the pre-t=0 pad in the all-collapsed content origin - clamp the drag ghost and drop placeholder to the collapsed clip height - aria-expanded on the layer disclosure, aria-pressed plus state-specific labels on the keyframe toggle, 24px chevron targets, focus-visible parity
The keyframe cache was fetched for a single source file: the selected element's, else the active composition. On open nothing is selected, so a project whose clips live in sub-compositions loaded only index.html and every property lane rendered empty until a clip was clicked. Load the cache for each composition file the timeline has rows for, so keyframed clips are expanded on open as intended. The AST load path moves to keyframeCacheAstLoad.ts to keep useGsapTweenCache.ts under the 600-line cap.
Auto-expansion picks the composition whose clip window contains the playhead, and those windows are half-open. With the playhead parked on the last clip's end - where playback stops - it sat inside nothing, so every expanded sub-composition row and its keyframe lanes collapsed to a single host row. Accept the closing boundary, but only when the strict pass matched nothing, so a playhead on the seam between two adjacent clips still expands the one that is starting.
Carries the gsapAnimations/keyframeCache symmetry fix into the extracted AST loader: an ungrouped tween now lands in both stores, so the collapsed row and the expanded lanes agree on what a clip animates.
Validate the parse response before reading `.animations` instead of casting the JSON blind, and narrow the fetch's return type to the slice callers read. Route the AST cache load through the shared clip-keyframe and cache-key helpers so it can't drift from the other writer. Drop the unused numeric track-count branches from `trackHeights`/`getTimelineCanvasHeight`, and pick the widest keyframed clip with a reduce so there's no index assertion.
82735ce to
e1289f8
Compare
5f9725e to
e5c162e
Compare
3 participants
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
TimelinePropertyLanesreads the ease from each keyframe's own source tween, so a merged lane cannot label a segment with a different animation's ease.contentOriginrestores the pre-t=0breathing 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_editnow carries anactionfield so preview churn is distinguishable from a commit.onMoveKeyframeisPromise<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
KeyframeEaseListemitdata-ease-segment-pct, so theAnimationCardscroll-into-view effect that queries it is no longer a no-op.Two follow-up fixes from manual QA on this branch:
usePopulateKeyframeCacheForFilenow loads the GSAP cache for every composition file thetimeline 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.tstostay under the 600-line file gate; the public helpers are re-exported so no importer moved.
findActiveExpandableCompositionIdfalls back to closed-interval containment when thestrict 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.
Test plan
Both follow-up fixes ship with a regression test that was verified to fail with the fix
reverted:
usePopulateKeyframeCacheForFile.test.tsxasserts both composition files arefetched, and
useExpandedTimelineElements.test.tscovers the end-of-timeline playhead plusthe shared-seam tie-break. Both fixes were also confirmed in a running Studio against the
keyframe-playgroundproject.Stack validation on the promoted branches: 2,892 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.insegment the readout at 50.6% progress is 12.97, not the linear 50.6.Not covered
MotionPathOverlayvisibility is deliberately wider than the selection-box gate: it nowdraws 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?: stringis left as-is rather than widened to thePropertyGroupNameunion.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→truepackages/studio/src/player/components/TimelineLanes.tsx:214— resolveTrackKeyframeClip called inline in render — sort() per track per frame during playbackpackages/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-dropR1 review follow-ups
Both high findings (the
contentOriginlayout flip without scroll compensation, and the per-track per-render filter and sort inresolveTrackKeyframeClip) 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:
.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.toClipKeyframesandelementCacheKeysowners as the other writer.trackHeightsandgetTimelineCanvasHeightkept a numeric track-count branch no production caller used once rows gained variable heights. Both branches are gone and the tests pass row arrays.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.tsxis deleted, not lost. It hard-coded the ghost at oneTRACK_H, which is wrong once a row can be taller than a clip. The same render now lives inTimelineCanvas.tsx, where the row heights are, and takes its height from the dragged clip's own row.TimelineRazorInteraction.tsxis the razor overlay lifted out ofTimeline.tsxso that file stays under the 600-line gate.Stack validation after this pass: 2,901 Studio tests pass across 260 files,
tsc --noEmitclean forpackages/studio, oxlint and oxfmt clean, Fallow audit gate clean, file-size gate clean.