feat(studio): add variable timeline timing and layout#2782
feat(studio): add variable timeline timing and layout#2782miguel-heygen wants to merge 1 commit into
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. |
5dd41e9 to
e4d7bde
Compare
1e99115 to
514a219
Compare
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
COMMENT — R1 adversarial pass (--tier max, head 514a219).
Foundation for a variable-height timeline. Row-geometry helpers (trackHeights, getTimelineRowOffsets, getTimelineRowFromY/getTimelineRowPositionFromY, getTimelineInsertBoundaryBand) are well factored, backward-preserving when rowHeights = [], and covered by both variable-row unit tests and collapsed-row characterization tests. SDK + fallback move/resize paths now converge on the same GSAP resync + cache invalidation — a real symmetry win. No blockers; three low-severity observations.
Adversarial lenses
(Lens A — timing determinism, Lens B — layout coord, Lens C — hit-testing, Lens D — resize/responsive, Lens E — #2781 state consistency.)
- Lens C — track-create threshold now scales with the row-0 pixel height.
packages/studio/src/player/components/timelineClipDragPreview.ts:132-166feedsresolveTimelineMovewithtrackHeight: 1and row-float coordinates.EDGE_TRACK_CREATE_THRESHOLD = 0.55(timelineEditing.ts:41) is now measured in row-index units, so on a collapsed first row it stays at0.55 × 48 ≈ 26 px(parity), but on an expanded first row (TRACK_H + 2·LANE_H = 104 px) the same 0.55 threshold requires ~57 px above the top gutter before a new top track spawns. Same story below the last lane except that path falls back toTRACK_HingetTimelineRowOffset— so top-edge creation scales with row-0 height, bottom-edge creation stays fixed. Likely intended (bigger row → farther travel), but worth an explicit muscle-memory decision in a follow-up before the wiring PR ships. P3 / nit. - Lens A —
.finally(() => invalidateGsapCache?.())fires even on rejection.packages/studio/src/hooks/useTimelineEditing.ts:186-215and:294-329bump the GSAP cache regardless of whetherfinishClipTimingFallbacksucceeded. If the server rewrite failed but the DOM/store still holds an optimistic delta, a cache bump forces the animation parser to re-read whatever is on disk. In practice this is likely benign (parser sees stale server state, animation looks correct-ish), but the intent ofinvalidateGsapCachewas "the server just rewrote positions." Consider.then(...)for the success-only invalidation, or leave a comment saying failure-path invalidation is deliberate. P3 / nit. - Lens E —
trackHeights()exported without a production consumer. GitHub code search finds no callers outside tests at514a219. Wiring lands in a later Family-B PR — acceptable for B2 — but flag on the merge-order plan so it doesn't sit dormant if the stack is cherry-picked. P3 / nit.
Positive callouts: INSERT_BOUNDARY_BAND is now geometry-exact per row (CLIP_Y / rowHeight), so an expanded row's insert band shrinks in row-fraction while staying at 3 px in absolute — the anti-phantom-insert guard survives variable heights. The originRow / currentRow refactor with trackHeight: 1 is a clever way to route variable-pixel drags through the existing threshold logic without touching resolveTimelineMove. getTimelineClipRect and computeMarqueeSelection grew optional params with backward-compatible defaults, so the sole external caller (useTimelineRangeSelection.ts) needs no change.
— 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 514a219433d1.
R2 adversarial pass on the variable-timeline timing + shared geometry slice. Cluster of concerns around cache-invalidation policy asymmetry (single-clip .finally vs group post-await), row-extrapolation asymmetry past first/last row, and one closure-instability item that cascades through the TimelineEditContext.
🟢 Verified clean
- timelineMoveAdapter.ts — TimelineMoveEdit / TimelineMoveEditsHandler exports match pre-existing anonymous shape used at App.tsx and TimelinePane.tsx
- useTimelineClipDrag.ts — rowHeightsRef threaded through as optional prop, callback deps updated correctly
- timelineCollision.ts — replacing local INSERT_BAND=3/48 with imported INSERT_BOUNDARY_BAND=CLIP_Y/TRACK_H preserves numeric default
- timelineMarquee.ts — contentOrigin default (GUTTER + TRACKS_LEFT_PAD) matches previous hardcoded origin; rowHeights default of [] preserves collapsed-row math
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.
| sdkSession: editFlowSdkSession, | ||
| publishSdkSession: sdkHandle.publish, | ||
| forceReloadSdkSession: sdkHandle.forceReload, | ||
| invalidateGsapCache: () => invalidateGsapCacheRef.current(), |
There was a problem hiding this comment.
🟡 invalidateGsapCache prop is a fresh closure every render, defeating downstream memoization
invalidateGsapCache: () => invalidateGsapCacheRef.current(),This arrow is recreated on every StudioApp render. Threaded into useTimelineEditing/useTimelineGroupEditing as a dependency of the useCallback wrapping handleTimelineElementMove/Resize (useTimelineEditing.ts:259 and :359 list invalidateGsapCache in the deps array), so every unrelated re-render invalidates those memoized handlers, which then invalidates handleTimelineElementsMove useCallback in App.tsx:180-186, which invalidates the whole TimelineEditContext value cascade. invalidateGsapCacheRef exists precisely so it can be a stable target.
Fix: Wrap the arrow with useCallback with no deps, or pass invalidateGsapCacheRef and destructure .current() inside the hook.
| const offsets = getTimelineRowOffsets(rowHeights); | ||
| if (row <= 0) return row * getTimelineRowHeight(0, rowHeights); | ||
| if (row >= rowHeights.length) { | ||
| return (offsets[rowHeights.length] ?? 0) + (row - rowHeights.length) * TRACK_H; |
There was a problem hiding this comment.
🟡 Row-extrapolation past the last row uses TRACK_H instead of the last row's height (asymmetric with pre-first-row branch)
if (row >= rowHeights.length) {
return (offsets[rowHeights.length] ?? 0) + (row - rowHeights.length) * TRACK_H;
}Symmetric pre-first-row branch four lines above uses row 0's actual height (row * getTimelineRowHeight(0, rowHeights)), so a pointer above the top pad extrapolates with the first row's concrete height. But past-last always divides by TRACK_H, and getTimelineRowFromY at line 137 has the same TRACK_H fallback. When bottom track is expanded, a pointer in the bottom breathing pad maps to a fractional row that doesn't match top-pad extrapolation-scale. getDefaultDroppedTrack and resolveInsertRow at trackCount are integer-clamped so unlikely to change a landing decision today, but the two branches should be symmetric to avoid subtle drift.
Fix: Extrapolate past the last row using getTimelineRowHeight(rowHeights.length - 1, rowHeights) (mirroring pre-first-row branch), or standardize both branches on TRACK_H — but pick one and document.
| coalesceKey, | ||
| recordEdit, | ||
| edit: { kind: "shift", delta: updates.start - element.start }, | ||
| }).finally(() => invalidateGsapCache?.()); |
There was a problem hiding this comment.
🔵 Single-clip vs group timing writers invalidate GSAP cache at different points on failure
}).finally(() => invalidateGsapCache?.());Single-clip finishMoveGsapSync/finishResizeGsapSync chain the invalidation in .finally() on finishClipTimingFallback (lines 203 and 312) — cache invalidated even when GSAP mutation rejects. Group path in useTimelineGroupEditing.ts:333 and :440 does invalidateGsapCache?.(); on the line AFTER await finishGroupTimingGsapFallback(...) — invalidation is skipped when the fallback rejects. Also, SDK single-clip path only calls invalidateGsapCache via finishMoveGsapSync (line 234), so an SDK persist that throws before finishMoveGsapSync runs never invalidates. Pick one policy and apply consistently.
Fix: Either move group invalidation into a try/finally that always runs after SDK+fallback sequence, or drop single-clip .finally and only invalidate on committed success — but match the two paths.
| currentScrollTop: 0, | ||
| pixelsPerSecond: pps, | ||
| trackHeight: TRACK_H, | ||
| trackHeight: 1, |
There was a problem hiding this comment.
🟡 resolveTimelineMove trackDeltaRaw is now piecewise row-space, changing stacking sensitivity in expanded rows
trackHeight: 1,
maxStart: dragMaxStart,
trackOrder,
},
clientX,
currentRow,
);Previously trackDeltaRaw = (clientY - originClientY + scrollΔ) / TRACK_H, a linear pixel-to-row measure. Now originRow/currentRow are computed through getTimelineRowFromY(..., ctx.rowHeights) and input.trackHeight is 1, so trackDeltaRaw = currentRow - originRow, a piecewise-linear quantity. Fine for Math.round(trackDeltaRaw) integer track-crossing logic (a taller row physically requires more pixels to cross — arguably correct). BUT same trackDeltaRaw flows unchanged into resolveTimelineLayerStackingMove (timelineEditing.ts:144, timelineLayerDrag.ts:362: const targetPosition = currentIndex + input.trackDeltaRaw;) whose thresholds (ONTO_ROW_THRESHOLD, Math.round(targetPosition), Math.ceil(targetPosition)) were tuned against uniform-row assumption. In an expanded row, pixel travel required to trigger stacking reorder is now noticeably larger. No test covers stacking with variable rowHeights.
Fix: Add a stacking-in-expanded-row test that pins the intended pixel/threshold contract, or thread caller's active-row height back into stacking so thresholds remain expressible in pixels rather than piecewise row-fractions.
| return shiftGsapPositions(projectId, changePath, domId, delta); | ||
| }, | ||
| }); | ||
| invalidateGsapCache?.(); |
There was a problem hiding this comment.
🟡 Group-mode invalidateGsapCache runs after every non-trackOnly batch — including audio-only or zero-delta shifts producing no GSAP mutations
invalidateGsapCache?.();
}).catch((error) => {The if (trackOnly) return; early-return above only gates on batch having no track change; a mixed batch where every timing delta is 0 (e.g. a group where only one clip has a real start delta and others resolved to δ=0) or where every change is an audio clip (no GSAP animations) still hits invalidateGsapCache?.(). mutateChange callback returns null for delta===0 or missing domId, so those clips issue no server mutation, yet the whole GSAP cache is invalidated anyway. Not a correctness bug — parity with single-clip path (finishClipTimingFallback gates on timingChanged && domId && projectId) is broken.
Fix: Only invalidate when at least one mutateChange actually returned a non-null promise, or when at least one animated change in the batch.
| contentY: number, | ||
| rowHeights: readonly number[] = [], | ||
| ): { rowFloat: number; row: number; fraction: number; rowHeight: number } { | ||
| const rowFloat = getTimelineRowFromY(contentY, rowHeights); |
There was a problem hiding this comment.
🟢 getTimelineRowPositionFromY recomputes offsets internally, doubling piecewise scan on every pointermove
export function getTimelineRowPositionFromY(
contentY: number,
rowHeights: readonly number[] = [],
): { rowFloat: number; row: number; fraction: number; rowHeight: number } {
const rowFloat = getTimelineRowFromY(contentY, rowHeights);
const row = Math.floor(rowFloat);
return {
rowFloat,
row,
fraction: rowFloat - row,
rowHeight: getTimelineRowHeight(row, rowHeights),
};
}getTimelineRowFromY builds getTimelineRowOffsets(rowHeights) from scratch on every call (line 126 allocates offsets array + linear scan). computeDragPreview invokes getTimelineRowFromY twice per pointermove (origin + current) at lines 137/141, then resolveDropPlacement invokes getTimelineRowPositionFromY which invokes it a third time. For a 3-tick pointermove burst that's 3× (allocate array + scan). Trivial today but scales with track count and drag rate.
Fix: Compute offsets array once per pointermove (or memoize behind rowHeightsRef identity) and pass it through row math instead of rebuilding on each helper call.
| coalesceKey, | ||
| recordEdit, | ||
| edit: { kind: "shift", delta: updates.start - element.start }, | ||
| }).finally(() => invalidateGsapCache?.()); |
There was a problem hiding this comment.
🟢 invalidateGsapCache is passed as an unconditional arrow but callers still use ?.()
.finally(() => invalidateGsapCache?.());Prop is optional on both hook types (useTimelineEditingTypes.ts line 50 and useTimelineGroupEditing.ts line 57 mark it ?), but only production caller — App.tsx line 177 — always passes a defined arrow. Either option-typing is intentional (in which case ref-based indirection is wrong shape — should be stable useCallback) or it isn't, in which case ?.() guards are dead code. Choose one for clarity.
Fix: Make invalidateGsapCache required (dropping the ?.), or actually make the prop optional in App.tsx by not passing when DOM edit session hasn't populated the ref yet.
|
|
||
| /** Fractional insert band for the concrete row under a pointer. */ | ||
| export function getTimelineInsertBoundaryBand(rowHeight: number): number { | ||
| return CLIP_Y / validRowHeight(rowHeight); |
There was a problem hiding this comment.
ℹ️ getTimelineInsertBoundaryBand has no direct unit test; only exercised via drag-preview integration
export function getTimelineInsertBoundaryBand(rowHeight: number): number {
return CLIP_Y / validRowHeight(rowHeight);
}New helper is the only production hook keeping resolveInsertRow's band aligned with rendered clip inset when rows are variable-height. Exported but not covered by any test in timelineLayout.test.ts — only exercised via two new expanded-row integration tests in timelineClipDragPreview.test.ts (149-193). Direct unit test would prevent regressions where a future edit changes CLIP_Y or validRowHeight fallbacks without updating the band.
Fix: Add a small dedicated test asserting the band for TRACK_H, expanded (TRACK_H + n*LANE_H), and invalid inputs (0/NaN/undefined) all reduce to intended clip-inset fraction.
514a219 to
97dd041
Compare
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 movesSupersedes #2684, which was closed when
mainwas rewritten to unwind an early landing of this stack. Same head commit, same review history.R1 review follow-ups
No high findings. The 3 low findings (edge track-create threshold scaling with row-0 height,
.finallyinvalidation on rejection,trackHeights()with no production consumer) are parked in.scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md.