feat(studio): wire expanded keyframe timeline lanes#2690
Conversation
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 0ac47c0ec5cb.
Family B stack tip. R9 verdict: split_path_exists — genuine integration is ~600 lines (contentOrigin/labelMode threading + useTimelineTrackLayout + TimelineLanes showsLanes), but ~250 lines split cleanly (TimelineRazorInteraction extraction, TimelineDragGhost inline move) and one preview-overlay behavior BROADENING rides along unmentioned in the body. Three UX behavior deltas the PR body does not call out: motion-path visibility broadened, compact-mode 48px TRACKS_LEFT_PAD silently dropped, and 232px content-origin flip on first-keyframe-add.
R9 exception verdict — split_path_exists
Three of the 21 files are pure refactors with zero lane-contract dependency and could easily land as separate PRs. (a) TimelineRazorInteraction.tsx (+60, new) is a straight extraction of three handlers + a TimelineRazorGuide component from Timeline.tsx — pre-PR code path preserved 1:1. (b) TimelineDragGhost.tsx (-59) is confirmed dead at base — git grep TimelineDragGhost da9f1722 returns only the file itself; ghost renderer inlined into TimelineCanvas.tsx:225-264. (c) PreviewOverlays.tsx MotionPathOverlay gating change (shouldShowSelectedDomBounds → shouldShowMotionPath at :279) is a preview-overlay behavior BROADENING with no connection to lane wiring. What IS genuinely coupled: (1) contentOrigin/labelMode threaded through useTimelinePlayhead / useTimelineGeometry / useTimelineRangeSelection / timelineDragDrop / timelineMarquee / timelineLayout, (2) useTimelineTrackLayout owning rowHeights derivation from expandedClipIds+laneCounts, (3) TimelineLanes showsLanes rendering. R9 exception partially warranted (~600 lines of integrated lane wiring genuinely won't split), but ~250 lines of orthogonal refactor + one preview-overlay behavior change ride along under the same banner.
🟢 Verified clean
- TimelineDragGhost.tsx deletion:
git grep TimelineDragGhost da9f1722returned only the file itself — confirmed dead at base - TimelineRazorInteraction.tsx extraction: pure move of three handlers + guide component out of Timeline.tsx
- TimelineCanvas.tsx is a plain , not a element — no HTML-in-canvas concern
- Playback follow: useTimelinePlayhead uses contentOriginRef to avoid stale capture across contentOrigin flips; playback deliberately does NOT chase the playhead (comment verbatim)
- Selection semantics preserved during playback: resolveTrackKeyframeClip falls back to lane-count ordering when nothing on the track is selected
Cross-stack notes
Family B stack tip. Genuine coupling: contentOrigin/labelMode + useTimelineTrackLayout row-height derivation + TimelineLanes showsLanes rendering (~600 of the 1098 additions). Splitable: TimelineRazorInteraction extraction (~60), TimelineDragGhost inline move (~40 + 59 deleted), useTimelineKeyframeHandlers wire-in (~30), MotionPathOverlay preview-gating change (~10). Real UX behavior deltas the PR body does not call out: (1) MotionPathOverlay broadening (finding 1), (2) 48px TRACKS_LEFT_PAD loss in compact mode (finding 3), (3) 232px flip on first-keyframe-add (finding 4). Composes with #2685: this PR's TimelineOverlays rest-args switch fixes #2685's context-menu drop — flag in both.
Family B parity check
Family B CLOSES on 1:1 Figma-motion parity: (a) per-property-group expanded lanes for position/scale/size/rotation/visual/other (PropertyGroupName in packages/parsers/src/gsapConstants.ts:53), (b) diamond retiming interactions on lanes, (c) segment ease editing via focusedEaseSegment, (d) per-group toggling from track header, (e) auto-expand-with-sticky-collapse default. STILL UNCOVERED: (1) custom bezier curve editor — ease is passed as string (KeyframeDiamondContextMenu:29 onChangeEase: (elementId, percentage, ease: string) => void), users pick from preset keywords not draw curves; (2) spring physics parameters (stiffness/damping) — no UI; (3) overshoot / back easing beyond named GSAP presets (same gap Family A had); (4) motion-path direct manipulation from lanes — MotionPathOverlay lives in preview iframe, not on timeline; (5) hold vs linear keyframe interpolation flag (no UI); (6) per-PROPERTY (not per-group) lane splitting — a position lane still batches x+y+z together; (7) copy/paste of full-clip keyframes across clips. '1:1 Figma-motion parity' claim ultimately lands with roughly the SAME gaps as Family A; Family B adds authoring surface but not the missing curve expressiveness.
| <MotionPathOverlay | ||
| iframeRef={previewIframeRef} | ||
| selection={shouldShowSelectedDomBounds ? domEditSelection : null} | ||
| selection={shouldShowMotionPath ? domEditSelection : null} |
There was a problem hiding this comment.
🟠 MotionPathOverlay behavior broadened silently — scope creep unrelated to lane wiring
// prop switches from shouldShowSelectedDomBounds → shouldShowMotionPath
// useStudioContextValue.ts:99:
shouldShowMotionPath: !!domEditSelection && !isPlaying && !isGestureRecording
// previous condition (:104-107):
shouldShowSelectedDomBounds: (inspectorPanelActive || rightPanelTab === "variables") && !isPlaying && !isGestureRecordingNew condition is strictly WIDER than the previous one. Motion path was previously visible ONLY when the inspector panel was open (or variables tab); it will now render for any selection whenever the composition is paused. Preview-overlay behavior broadening, entirely unrelated to 'wire expanded keyframe timeline lanes.' Not mentioned in the PR body. This is a real UX behavior delta that will land silently.
Fix: Either (a) revert to shouldShowSelectedDomBounds (keep prior behavior) and land the broadening as a separate PR with dedicated review, or (b) call it out in the body + get UX sign-off explicit.
| left: activeDraggedPosition.left, | ||
| width: Math.max(activeDraggedElement.duration * props.pps, 4), | ||
| height: TRACK_H - CLIP_Y * 2, | ||
| height: draggedRowHeight - CLIP_Y * 2, |
There was a problem hiding this comment.
🟠 Inlined drag ghost geometry regression — ghost stretches to full expanded-row height
// wrapper height: draggedRowHeight - CLIP_Y * 2
// clipY={0}, no clipHeight prop → TimelineClip uses bottom: 0 and stretches to fill wrapperThe inlined ghost sets wrapper height: draggedRowHeight - CLIP_Y * 2 and passes clipY={0} with no clipHeight to inner TimelineClip. TimelineClip then uses bottom: clipY (0) and stretches to fill. When dragging a keyframed clip whose lanes are expanded (draggedRowHeight = TRACK_H + n·LANE_H), the ghost renders as tall as the ENTIRE expanded row (clip bar + property-lanes area), while the SOURCE clip in TimelineLanes explicitly clamps to clipHeight={showsLanes ? TRACK_H - 2 * CLIP_Y : undefined} (TimelineLanes.tsx:356). Ghost shape differs from source clip's shape mid-drag — visible regression when dragging expanded keyframed clips. The removed TimelineDragGhost.tsx used constant TRACK_H - CLIP_Y * 2 for wrapper height — behavior change introduced by the inline move.
Fix: Pass clipHeight={TRACK_H - 2 * CLIP_Y} when rendering the inline ghost for a keyframed clip, matching TimelineLanes.tsx:356.
| [gsapAnimations], | ||
| ); | ||
| const labelMode = STUDIO_KEYFRAMES_ENABLED && hasKeyframedClips; | ||
| const contentOrigin = labelMode ? LABEL_COL_W + GUTTER : GUTTER; |
There was a problem hiding this comment.
🟠 Compact layout regression — 48px TRACKS_LEFT_PAD silently dropped in non-labelMode
const contentOrigin = labelMode ? LABEL_COL_W + GUTTER : GUTTER;Non-labelMode contentOrigin is now GUTTER = 32, whereas at base (da9f172) the entire layout hardcoded GUTTER + TRACKS_LEFT_PAD = 80. The 48px TRACKS_LEFT_PAD breathing pad before t=0 is silently dropped for any comp without keyframed clips. The Timeline.tsx guard const labelMode = STUDIO_KEYFRAMES_ENABLED && hasKeyframedClips means this hits EVERY existing non-keyframed comp on upgrade. Timeline.test.ts assertion (:124 expect(trackHeader.style.width).toBe("32px")) encodes the new behavior. Contradicts PR body's claim to 'preserve compact timeline layout' — compact mode is now 48px narrower to the left. Also affects getTimelineFitPps (fewer viewport px per second on short comps).
Fix: Restore the breathing pad in non-labelMode: const contentOrigin = labelMode ? LABEL_COL_W + GUTTER : GUTTER + TRACKS_LEFT_PAD; OR document why compact mode intentionally loses the pad.
| const gsapAnimations = usePlayerStore((s) => s.gsapAnimations); | ||
| // Label mode = comp has keyframed clips (not just when expanded): keeps the layer | ||
| // disclosure + property column visible and reserves a GUTTER before 0s (Figma). | ||
| const hasKeyframedClips = useMemo( |
There was a problem hiding this comment.
🟡 First-keyframe layout flip — 232px jump when hasKeyframedClips flips false→true
// labelMode = STUDIO_KEYFRAMES_ENABLED && hasKeyframedClips
// contentOrigin = labelMode ? LABEL_COL_W + GUTTER : GUTTER → 32 → 264Adding the FIRST keyframe to a composition flips hasKeyframedClips false→true, which flips contentOrigin from GUTTER = 32 to LABEL_COL_W + GUTTER = 264. Every clip and the playhead shift 232px to the right in a single frame (playhead sync effect in useTimelinePlayhead.ts:92 will update immediately since contentOrigin is a dep). User's scrollLeft is unchanged, so a keyframe added while zoomed-in on the middle of the ruler causes a jarring visual jump. No transition, no scroll-preserve compensation.
Fix: Compensate on the flip: capture contentOrigin before + after, adjust scrollLeft by the delta so the visible timeline stays put. Alternatively, animate the flip via CSS transition.
| // clean until 00:00 (the header-boundary line belongs to the track | ||
| // rows below, not the ruler). | ||
| width: contentOrigin, | ||
| // Ruler corner uses the panel surface — same as the ruler strip itself. |
There was a problem hiding this comment.
🟡 Stale comment cites GUTTER + TRACKS_LEFT_PAD but Timeline.tsx computes differently
// New comment: 'Breathing pad before 00:00 is folded into contentOrigin (see Timeline.tsx: GUTTER + TRACKS_LEFT_PAD), so no separate pad div.'
// Timeline.tsx:131 actually: labelMode ? LABEL_COL_W + GUTTER : GUTTER — NEVER GUTTER + TRACKS_LEFT_PADComment names GUTTER + TRACKS_LEFT_PAD but the runtime literally cannot produce that value. Misleading breadcrumb for future readers.
Fix: Update comment to reflect actual runtime formula (or update the formula to match the comment — see finding 3).
| // The one keyframed element this track shows lanes for (selected, else | ||
| // most lanes). A track can hold several elements; scoping to one keeps | ||
| // their keyframes from cramming into a single row. | ||
| const keyframeClip = STUDIO_KEYFRAMES_ENABLED |
There was a problem hiding this comment.
🟡 resolveTrackKeyframeClip called inline in render — sort() per track per frame during playback
// resolveTrackKeyframeClip(els, laneCounts, selectedElementId, selectedElementIds) called inline
// inside displayTrackOrder.map(...) — once per track per render, NOT memoized.
// Inside: [...keyframed].sort(...) allocates + sorts on every call.Also called ONCE inside useTimelineRowHeights (useTimelineTrackLayout.ts:80) — that call IS memoized. Since the render call recomputes what the memoized call resolves to, the second computation is redundant. On playback ticks (currentTime → Timeline re-render), N tracks × sort() per frame. Not a correctness bug but a per-frame allocation cascade during playback.
Fix: Memoize the render-side call OR return resolveTrackKeyframeClip output from useTimelineRowHeights alongside rowHeights so the render loop reuses it.
| state={kfContextMenu} | ||
| onClose={() => setKfContextMenu(null)} | ||
| onDelete={(elId, pct) => onDeleteKeyframe?.(elId, pct)} | ||
| onDelete={(...args) => onDeleteKeyframe?.(...args)} |
There was a problem hiding this comment.
🟢 Rest-args switch fixes a real bug but composes with #2685 latent handler-drop
// Base: onDelete={(elId, pct) => onDeleteKeyframe?.(elId, pct)} — dropped 3rd–5th args
// This PR: switches to rest-args forwardingThis PR's rest-args switch actually fixes a real bug I flagged on #2685 (context-menu drops propertyGroup/tweenPercentage/animationId). But it's now a functional behavior change — previously ignored args are now delivered to consumers. Confirm downstream onDeleteKeyframe / onMoveKeyframeToPlayhead handlers in useTimelineEditCallbacks.ts actually respect the propertyGroup/tweenPercentage/animationId args; if not, this could unmask a different latent bug.
Fix: Add explicit test coverage for the wider signature. Consider hoisting this fix into #2685 where the drop originates.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 0ac47c0ec5cb.
Family B integration tip. Rames's R1 landed split_path_exists (~600 lines genuinely coupled — contentOrigin/labelMode threading, useTimelineTrackLayout row-height derivation, TimelineLanes showsLanes rendering) plus three UX behavior deltas the PR body does not call out (MotionPathOverlay visibility broadening, 48px TRACKS_LEFT_PAD dropped in compact mode, 232px content-origin flip on first-keyframe-add) and a Family B parity check that finds Figma-motion parity STILL leaves ~7 gaps (custom bezier editor, spring physics, overshoot, motion-path from lanes, hold-vs-linear flag, per-property lane split, cross-clip copy/paste). I verified the cross-slice consumption of #2683-#2687 exports.
Prior review
Rames D Jusso R1: R9 split_path_exists, three UX deltas, Family B parity gap enumeration, TimelineOverlays rest-args fixes #2685's context-menu drop. Overlap: I confirmed the cross-slice import graph and re-verified the showsLanes conditional.
🟢 Cross-slice consumption audit (per Miguel-flagged R9 for #2690)
useTimelineTrackLayout.ts:4importsanimationContributesLanefromTimelinePropertyLanes— resolves because #2689 promotedfunction→export functionon the source. Verified at HEAD.TimelineLanes.tsxpassesonMoveKeyframe(elementKey, target.percentage, toClipPercentage, target.propertyGroup, target.tweenPercentage, target.animationId) ?? Promise.resolve(false)— Promise return type matchesTimelinePropertyLanes.onMoveKeyframe: (target, toClipPercentage) => Promise<boolean>per #2689, and matchesTimelineDiamondLane's expectation per #2685. Full chain consistent.TimelineTrackHeaderreceiveskeyframeClip(fromresolveTrackKeyframeClip),isExpanded(fromexpandedClipIds.has(clipKey)),animations(fromgsapAnimations.get(clipKey)),onToggleClipExpanded,onTogglePropertyGroupKeyframe,onSeek— every wired-in callback comes fromuseTimelineEditCallbacks(#2688) or the store slice (#2683). No orphan/renamed symbol.TimelinePropertyLaneswired only whenelementKey === keyframeClipKey && keyframeClipExpanded— one lane render per track, not one per element on the track. Matches Rames'sresolveTrackKeyframeClipscoping analysis.trackStudioKeyframeLaneExpand({ expanded })telemetry event is defined in #2683 (events.ts:63) — consumer wired correctly here.TimelineOverlays.tsxin this diff uses rest-args ((...args) => onDeleteKeyframe?.(...args)) — patches the #2685 wiring gap Rames identified. Verified.
Cross-slice consumption is clean — no dangling reference to a #2683-#2687 symbol.
🟢 Verified
TimelineCanvas.tsxis a plain<div>— no<canvas>, no HTML-in-canvas concern.TimelineRazorInteraction.tsx(new, +60) is a pure extraction of three handlers + aTimelineRazorGuidefromTimeline.tsx; behavior parity preserved 1:1 (Rames anchored the dead-code confirmation forTimelineDragGhost.tsx).- Playback follow:
useTimelinePlayheadusescontentOriginRefto survivecontentOriginflips across playback — no stale capture. Matches the code comment. - Row-height derivation via
useTimelineTrackLayoutshort-circuits onlaneCounts.get(clipId) ?? 0— a clip with zero property lanes never expands the row. getTimelineRowHeight(row, rowHeights)fallback toTRACK_Hwhenrow < 0— safe for out-of-range display order (e.g. drag preview track not in the base order).
Cross-stack synthesis (Lens 6 — pattern propagation across the whole stack)
Applying the ten-lens set across #2683-#2690:
- Lens 1 (silent-catch): No silent-catch chains found in the diff surface across the stack. Only defensive
try/catchinuseStudioTestHooksforimport.meta.env.DEV, which is well-scoped. - Lens 3 (key stability): Diamond keys route through
timelineKeyframeSelectionKey(elementId, target)— stable identity, not user-interaction-driven. Clean. - Lens 5 (keyboard drag): The diamond retime surface is mouse-only through the whole stack (called out on #2685). Not repaired downstream.
- Lens 6 (cross-slice pattern):
Promise<boolean>return contract broken at #2686, repaired at #2689.animationContributesLaneexport gap at #2686, repaired at #2689 (called out per-slice). - Lens 9 (parity claim): Family B doesn't claim strict "1:1" Figma parity in the PR bodies (unlike Family A), but Rames enumerated 7 remaining gaps. Worth naming in the roll-up so the next stack scopes them.
- Lens 10 (cross-mode): Motion-path vs keyframe modes both handled in
TimelineToolbarat #2689 with distinct tooltips + aria-labels. Clean.
Verdict
Approving. Rames's structural findings (R9 split, UX deltas, parity gap) are the load-bearing concerns; my cross-slice consumption audit found no orphan references. Family B ships with the ten-lens Lens 5 gap unaddressed (keyboard drag) — worth surfacing to the ProdAI channel as a follow-up rather than blocking Family B.
— Review by Via
2b4a520 to
67d2ef1
Compare
0ac47c0 to
b8bc522
Compare
67d2ef1 to
50b6822
Compare
2fa0d61 to
948acd5
Compare
The base branch was changed.
- 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
948acd5 to
0b86dc7
Compare
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.
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.Test plan
Stack validation on the promoted branches: 2,887 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-drop