feat(studio): add variable timeline timing and layout#2684
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 1c0422662a16.
Centralizes timing conversion + row geometry — B3-B8 all consume these primitives. Two structural risks worth folding in before dependents wire more callers into the group path: (a) group-path cache invalidation gap (finding 1) is exactly the drift the PR is trying to close; (b) marquee perf regression (finding 3) will compound with #2686's un-memoized lane render — every marquee tick would re-run O(N × R) rect math AND re-render every lane.
🟢 Verified clean
- useTimelineEditing.ts single-element flow (:203, :312) correctly uses .finally for invalidateGsapCache — establishes the pattern that finding 1 flags absent from group flow
- timelineCollision.ts (-13) — simplification consolidates around new geometry model without capability loss
- useTimelineClipDrag (+4/-1) minor threading of new context; behavior parity preserved
- timelineLayout.test.ts (+75) — covers new piecewise row-Y math directly
- timelineMarquee.test.ts covers cross-row selection with variable heights (:120-138 codifies the finding 6 behavior)
- App.tsx (+6/-15) cleanup — props removed were consumers not wired outside diff
Cross-stack notes
This slice standardizes timing conversion + row geometry — B3-B8 all consume these primitives. Two structural risks: (1) unstable invalidateGsapCache prop (finding 2) will keep propagating through the callback chain B5-B8 imitate; (2) group-path cache invalidation gap (finding 1) is the exact drift the PR is trying to close — worth fixing before dependents wire more callers into the group path. Marquee perf regression (finding 3) will compound with #2686's un-memoized lane render (see #2686 finding 1) — every marquee tick re-runs O(N × R) rect math AND re-renders every lane.
| return shiftGsapPositions(projectId, changePath, domId, delta); | ||
| }, | ||
| }); | ||
| invalidateGsapCache?.(); |
There was a problem hiding this comment.
🟠 Group edits only invalidate the GSAP cache on the happy path; single-element uses .finally
await finishGroupTimingGsapFallback({
projectId,
iframe: previewIframeRef.current,
...
});
invalidateGsapCache?.();In useTimelineEditing.ts the equivalent post-commit sync is wrapped as finishClipTimingFallback({...}).finally(() => invalidateGsapCache?.()) (:203 and :312), so cache is invalidated even when the GSAP rewrite errors. Group paths (move at ~:334, resize at ~:439) call invalidateGsapCache?.() on the line AFTER await finishGroupTimingGsapFallback(...), so if the fallback throws it's skipped and control jumps to the outer .catch that only rolls back duration + shows a toast. Persist to disk has already committed — new file timing + stale in-memory GSAP animation cache until the next full reload. Exactly the drift this PR is trying to close.
Fix: Wrap the finish call in .finally (or try/finally) to match the single-element flow.
| reloadPreview: () => setRefreshKey((k) => k + 1), | ||
| pendingTimelineEditPathRef, | ||
| }); | ||
| const invalidateGsapCacheRef = useRef<() => void>(() => {}); |
There was a problem hiding this comment.
🟡 invalidateGsapCache prop is a fresh function on every render, destabilizing every useTimelineEditing callback
const invalidateGsapCacheRef = useRef<() => void>(() => {});
const timelineEditing = useTimelineEditing({
...
invalidateGsapCache: () => invalidateGsapCacheRef.current(),
handleDomZIndexReorderCommitRef,
});Inline arrow re-created on every StudioApp render. In useTimelineEditing.ts this prop is in the useCallback dep arrays for handleTimelineElementMove (:259), handleTimelineElementResize (:359), and inside the useTimelineGroupEditing config passed at :122. Every StudioApp render invalidates memoization of the main timeline commit callbacks, and by extension the props of any children that memoize on them. Ref-latch pattern already isolates the mutable target — hoist the bridge into a stable useCallback(() => invalidateGsapCacheRef.current(), []) to actually get the benefit.
Fix: const invalidateGsapCache = useCallback(() => invalidateGsapCacheRef.current(), []);
| * A clip's rendered rect in canvas/content coordinates (the same space the | ||
| * marquee rect lives in): x from GUTTER + start * pps, y from the clip's row | ||
| * index within the visible track order (RULER_H + row * TRACK_H + CLIP_Y). | ||
| * marquee rect lives in): x from the shared content origin + start * pps, y from the clip's row |
There was a problem hiding this comment.
🟡 Marquee selection is now O(clips × rowHeights) — offsets recomputed per clip
return {
left: contentOrigin + clip.start * pps,
top: getTimelineRowTop(row, rowHeights) + CLIP_Y,
width: Math.max(clip.duration * pps, MIN_CLIP_W),
height: getTimelineRowHeight(row, rowHeights) - CLIP_Y * 2,
};computeMarqueeSelection iterates every clip + calls getTimelineClipRect. Each rect now calls getTimelineRowTop(row, rowHeights) → getTimelineRowOffset → unconditionally recomputes getTimelineRowOffsets(rowHeights), an O(R) pass — even for row===0. On a live marquee drag (pointermove + auto-scroll RAF via applyMarqueeAtClient/stepMarqueeAutoScroll in useTimelineRangeSelection.ts), this is O(N × R) work per tick where previously it was O(1).
Fix: Precompute getTimelineRowOffsets(rowHeights) once in computeMarqueeSelection and thread it into getTimelineClipRect, or short-circuit getTimelineRowOffset when row <= 0 before building the offsets array.
| ): DraggedClipState { | ||
| const { scroll, pps, duration, trackOrder, elements, selectedKeys, buildSnapTargets } = ctx; | ||
| const dragMaxStart = resolveDragMaxStart(scroll, pps, duration); | ||
| const scrollTop = scroll?.scrollTop ?? drag.originScrollTop; |
There was a problem hiding this comment.
🟡 originRow uses the CURRENT bounding-rect top even though it references the ORIGIN pointer/scroll
const scrollTop = scroll?.scrollTop ?? drag.originScrollTop;
const scrollRectTop = scroll?.getBoundingClientRect().top ?? 0;
const originRow = getTimelineRowFromY(
drag.originClientY - scrollRectTop + drag.originScrollTop,
ctx.rowHeights,
);
const currentRow = getTimelineRowFromY(clientY - scrollRectTop + scrollTop, ctx.rowHeights);scrollRectTop is the CURRENT bounding rect, but reconstructs BOTH the origin content-Y and the current content-Y. When getTimelineRowFromY was linear, scrollRectTop cancelled in the delta. No longer cancels once row heights differ: the two content-Y values are fed to a piecewise function, and effective row-delta depends on which rows each falls in — not just the pixel delta. Under a layout shift during a drag (e.g. lane expand toggled mid-gesture), originRow becomes wrong, so resolveTimelineMove's trackDeltaRaw = currentRow - originRow can jump.
Fix: Capture the original scroll rect at pointerdown alongside originScrollTop, or cache originRow once at drag start rather than recomputing every frame.
| const y = contentY - RULER_H - TRACKS_TOP_PAD; | ||
| if (rowHeights.length === 0) return y / TRACK_H; | ||
| if (y < 0) return y / getTimelineRowHeight(0, rowHeights); | ||
|
|
There was a problem hiding this comment.
🟢 getTimelineRowFromY beyond last row divides by TRACK_H even when the last row is expanded
const offsets = getTimelineRowOffsets(rowHeights);
for (let row = 0; row < rowHeights.length; row += 1) {
const bottom = offsets[row + 1] ?? 0;
if (y < bottom) {
const top = offsets[row] ?? 0;
return row + (y - top) / getTimelineRowHeight(row, rowHeights);
}
}
return rowHeights.length + (y - (offsets[rowHeights.length] ?? 0)) / TRACK_H;For pointer past the last row (bottom breathing pad), fractional overhang divided by constant TRACK_H regardless of whether last row is expanded. Consumers use rowFloat to decide resolveInsertRow and asset drops (:525) — insert boundary near bottom of expanded last row triggers at a different pixel depth than bottom of collapsed row. The parallel row <= 0 branch (:103) uses getTimelineRowHeight(0, rowHeights) for the same reason.
Fix: Use getTimelineRowHeight(rowHeights.length - 1, rowHeights) for symmetry (or accept the TRACK_H convention explicitly).
| * A clip's rendered rect in canvas/content coordinates (the same space the | ||
| * marquee rect lives in): x from GUTTER + start * pps, y from the clip's row | ||
| * index within the visible track order (RULER_H + row * TRACK_H + CLIP_Y). | ||
| * marquee rect lives in): x from the shared content origin + start * pps, y from the clip's row |
There was a problem hiding this comment.
🟢 getTimelineClipRect uses full row height — marquee hit-boxes for clips in expanded rows extend under property lanes
height: getTimelineRowHeight(row, rowHeights) - CLIP_Y * 2Previously rect height was TRACK_H - CLIP_Y*2 = 42 (actual rendered clip body). With variable rows, height is rowHeight - CLIP_Y*2, so for a row expanded by two lanes (104 - 6 = 98) the marquee hit-box covers the property-lane region below the clip — but the rendered clip body itself is only ~42px tall (TimelinePropertyLanes renders lanes at height: LANE_H beneath the clip body). A marquee drawn purely across the property-lane area will select the parent clip even though visually no clip body is under it. New test at timelineMarquee.test.ts:120-138 bakes this in as intended.
Fix: If unintended: keep clip-body height (TRACK_H - CLIP_Y*2) and only shift top via getTimelineRowTop. If intended: document why in a comment on the test.
| } from "./useTimelineGroupEditing"; | ||
|
|
||
| interface MoveEdit { | ||
| export interface TimelineMoveEdit { |
There was a problem hiding this comment.
🟢 TimelineMoveEdit no longer allows stackingReorder — silent drop in group moves
export interface TimelineMoveEdit {
element: TimelineElement;
updates: Pick<TimelineElement, "start" | "track">;
}Prior inline signature in App.tsx (deleted :43-48 of diff) permitted updates.stackingReorder?: TimelineStackingReorderIntent | null. Single-element handleTimelineElementMove still reads updates.stackingReorder (useTimelineEditing.ts :160, :244), but the group path via persistTimelineMoveEditsAtomically explicitly discards everything except start + track. Formalizing TimelineMoveEdit without stackingReorder removes even the TS hint that a caller might expect the field to survive the group path.
Fix: Either lift stackingReorder into TimelineGroupMoveChange (atomic path threads it through) or add a TSDoc note that stackingReorder on a group edit is dropped by design.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Reviewed at 1c0422662a16.
Timing + geometry primitives that B3-B8 all consume. Rames's R1 landed the structural risks (group-path cache invalidation gap, unstable invalidateGsapCache prop through the callback chain, marquee O(N × R) rect-math per tick that compounds with #2686's un-memoized lane render). I re-verified the local primitives read clean and defer blocker judgment to that write-up.
Prior review
Rames D Jusso's R1 anchors two structural risks: (1) group-path cache invalidation gap — exactly the drift this PR claims to close, and (2) marquee perf regression that will compound with #2686. My additive read below only confirms clean pieces and adds no new blocker.
🟢 Verified
useTimelineEditing.tssingle-element path uses.finally(invalidateGsapCache)(:203, :312) — establishes the pattern Rames flags as absent from the group flow.timelineLayout.test.ts(+75) covers the new piecewise row-Y math directly;timelineMarquee.test.tscovers cross-row selection with variable heights.App.tsx(+6/-15) removed props are not wired outside this diff — clean cleanup, no orphan consumers.timelineCollision.ts(-13) is consolidation around the new geometry model with no capability loss visible in-diff.timelineClipDragPreview.ts(+37/-15) diamond-preservation math threads the new geometry constants consistently.
Cross-slice note
The new TimelineLaneBaseProps shape (contentOrigin, contentGutter, rowHeights, laneCounts) is the load-bearing primitive that #2690 will thread through useTimelineTrackLayout — worth re-checking there that all six additions are consumed and none go orphaned.
Verdict
Local primitives clean; blocker-tier concerns are cross-stack (Rames covered).
— Review by Via
915a4e4 to
f499121
Compare
1c04226 to
c3219cc
Compare
c3219cc to
1e99115
Compare
f499121 to
5dd41e9
Compare
The base branch was changed.
Fallow audit reportFound 1 finding. Details
Generated by fallow. |
3 participants
What
Adds variable timeline timing and the shared geometry/layout model used by keyframe lanes.
Why
Keyframe placement must be derived consistently from clip-local timing and the visible timeline scale; duplicating that math across components causes drift and incorrect seeking.
How
Centralizes timing conversion and lane geometry behind focused helpers and tests. This is B2 of the Family B draft Graphite stack.
Test plan
Exact Family B tip validation: 2,865 Studio tests, 67 Studio Server route 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 5 remaining low/nit findings are parked, verbatim, in
.scratch/studio-timeline-family-b/issues/02-pr-2684-deferred-review-findings.md:packages/studio/src/player/components/timelineMarquee.ts:71— Marquee selection is now O(clips × rowHeights) — offsets recomputed per clippackages/studio/src/player/components/timelineClipDragPreview.ts:135— originRow uses the CURRENT bounding-rect top even though it references the ORIGIN pointer/scrollpackages/studio/src/player/components/timelineLayout.ts:121— getTimelineRowFromY beyond last row divides by TRACK_H even when the last row is expandedpackages/studio/src/player/components/timelineMarquee.ts:71— getTimelineClipRect uses full row height — marquee hit-boxes for clips in expanded rows extend under property lanespackages/studio/src/hooks/timelineMoveAdapter.ts:7— TimelineMoveEdit no longer allows stackingReorder — silent drop in group moves