feat(studio): add variable timeline timing and layout - #2782
feat(studio): add variable timeline timing and layout#2782miguel-heygen wants to merge 3 commits 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.
41b0a68 to
bb55462
Compare
james-russo-rames-d-jusso
left a comment
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Delta review from R1 head 514a21943 to current head bb55462ac — two new HF#2782-specific commits (854849783 R2 refactor + bb55462ac docblock hoist), plus the HF#2781 rebases-in that I already verified at the parent PR. Also verified the 8 R1 findings against stack tip 6ee750fee.
Deferral to stack tip — verified
Same shape as #2781: most of the R1 findings ship as fixes at stack tip. Per finding:
| Finding | This PR (bb55462ac) |
Stack tip (6ee750fee) |
|---|---|---|
🟡 F#1 invalidateGsapCache prop is a fresh closure every render (App.tsx:177) |
still invalidateGsapCache: () => invalidateGsapCacheRef.current() — new arrow every render |
fixed — extracted const invalidateGsapCache = useCallback(() => invalidateGsapCacheRef.current(), []) at App.tsx:162, passed as-is |
🟡 F#2 Row-extrapolation past last row uses TRACK_H while pre-first-row uses row 0's actual height (timelineLayout.ts) |
still (offsets[rowHeights.length] ?? 0) + (row - rowHeights.length) * TRACK_H |
explicit WONTFIX with rationale at stack tip: a comment now documents "Deliberately TRACK_H, not the last row's height: rows past the end do not exist yet, and a row created by dropping there starts unexpanded. The pre-first-row branch above uses row 0's concrete height because that row DOES exist." Fair — the two branches have different semantics, and calling it out explicitly is the right move |
🔵 F#3 Group-timing writer invalidates on success only, single-clip uses .finally (asymmetric on failure) |
still invalidateGsapCache?.(); outside a try, so failure path skips it |
fixed — wrapped in try { await persistServerBatch(...) } finally { invalidateGsapCache?.(); } so both success AND failure invalidate, matching the single-clip writer |
🟡 F#4 resolveTimelineMove trackDeltaRaw piecewise row-space changes stacking sensitivity in expanded rows |
fixed here in 854849783 — vertical axis is now a row index (currentRow - input.originRow), scroll and per-row heights moved to the caller. Cleanest possible fix — kills the piecewise-pixel problem at the API instead of patching over it |
(same fix carried) |
🟡 F#5 Group-mode invalidateGsapCache runs after every non-trackOnly batch, including audio-only / zero-delta shifts |
still fires unconditionally after every group persist | fixed at stack tip alongside F#3 — same try/finally wrap, and I read line 452's context — still fires on all non-trackOnly, but that's now the intentional shape post-fix |
🟢 F#6 getTimelineRowPositionFromY recomputes offsets internally, doubling piecewise scan per pointermove |
still allocates offsets array on every call to getTimelineRowFromY |
fixed — rowOffsetsCache = new WeakMap<readonly number[], number[]>() memoizes by array identity (useTimelineTrackLayout upstream returns a stable ref for the life of a gesture, so this cache hits) |
🟢 F#7 invalidateGsapCache passed as unconditional arrow but callers still use ?.() |
unchanged | unchanged at tip — cosmetic/type-clarity, not a functional issue; ?.() is defensive against the optional prop type |
ℹ️ F#8 getTimelineInsertBoundaryBand no direct unit test |
still only exercised via drag-preview integration | fixed — new describe("getTimelineInsertBoundaryBand") block at timelineLayout.test.ts:196-210 covering TRACK_H, expanded rows (TRACK_H + n*LANE_H), and invalid-input fallback |
Merge-order coupling is the same as #2781: if this PR were merged solo without the stack tip, F#1 (closure identity → dropped memoization), F#3/F#5 (invalidate on-failure gap), F#6 (offsets recomputed per pointermove), and F#8 (no unit test) would all ship live. Trusting the Graphite stack merge sequence.
The two new HF#2782-specific commits — LGTM
-
854849783—refactor(studio): give resolveTimelineMove a row-based vertical axis. This is the correct fix for F#4. The prior signature(originClientY, currentClientY, originScrollTop, currentScrollTop, trackHeight)was already being called withtrackHeight: 1and both scrollTops zeroed by the sole production caller — the parameter names were describing units the values no longer carried. Refactored signature drops those five fields for a singleoriginRow: numberplus acurrentRow: numberargument, andcomputeDragPreviewnow foldsgetTimelineRowFromY(clientY - scrollRectTop + scrollTop, ctx.rowHeights)intooriginRow/currentRowat the call site.trackDeltaRaw = currentRow - input.originRowis now pure integer row-index math — no piecewise pixel scaling, no expanded-row sensitivity drift. Tests attimelineEditing.test.tswere rewritten to pass row indices; they look internally consistent to me.One residual to note (already parked in your deferred-issues file per the PR body):
originRowis recomputed at everycomputeDragPreviewinvocation usingdrag.originClientY - scrollRectTop + drag.originScrollTopwherescrollRectTopis the current bounding-rect top. If the scroll container's rect shifts mid-drag (window resize, layout thrash),originRowdrifts. Not a bug for the current gesture surface where the scroll container is stable during a live drag, but noted as a coupling. -
bb55462ac—refactor(studio): drop the duplicated row-top docblock. Zero-risk hoist — thegetTimelineRowTopdocblock had a duplicate sitting on theTimelineTrackHeightClipinterface where it described nothing. Now sits only on the function it documents. No behavior change.
Foundational-slice lens
This is B2 on the stack — the timing/layout foundation that B1 (#2781) and everything downstream reads from. Same lens as B1:
- Row-space math consistency.
getTimelineRowOffsets,getTimelineRowHeight,getTimelineRowOffset,getTimelineRowTop,getTimelineRowFromY,getTimelineRowPositionFromY,getTimelineInsertBoundaryBand— I traced the invariant:getTimelineRowTop(row) - RULER_H - TRACKS_TOP_PAD = getTimelineRowOffset(row), andgetTimelineRowFromYinverts againstcontentY - RULER_H - TRACKS_TOP_PAD. The pad terms cancel exactly. The pre-first-row and past-last-row branches use different height bases (row 0's height vsTRACK_H) but that's the F#2 asymmetry, now documented at tip. - Signature drift on
TimelineMoveInput. The R2 signature simplification is a public-ish contract change — I confirmed no other production caller ofresolveTimelineMoveexists beyondcomputeDragPreviewat this SHA.timelineEditing.test.tswas updated in the same commit; nothing else broken. invalidateGsapCachefan-out. App.tsx wires the ref throughdomEditSession. TheuseTimelineEditing+useTimelineGroupEditinghooks receive it as an optional prop. F#7 (unconditional arrow vs.?.()callers) is cosmetic — the fresh-closure identity issue (F#1) is the meaningful one, and it lands at the tip.
What I didn't verify
- Cross-stack consumers of
getTimelineInsertBoundaryBanddownstream of #2782 — the tip's new tests cover the helper's outputs; trusting the consumers respect those. - Manual expanded-row drag behavior — you called out 2,910 Studio tests passing and I'm trusting that includes coverage of the new row-index-based drag math.
- Whether
rowOffsetsCache(the WeakMap memoization at stack tip) collides with a rowHeights array that gets mutated in place. If the identity is stable per gesture but the contents change (e.g. an expansion during drag), the cache would return stale offsets. If upstreamuseTimelineTrackLayoutalways returns a NEW array on layout change (rather than mutating in place), you're safe — worth confirming that invariant if you haven't already.
Current CI shows regression, preview-regression, player-perf, Preview parity, Preflight all SUCCESS at head; some earlier attempts CANCELLED (looks like restart cascade), no live FAILUREs. LGTM from my side on the delta.
vanceingalls
left a comment
vanceingalls
left a comment
There was a problem hiding this comment.
Re-reviewed at bb55462a.
Verdict
APPROVE. Two new commits since R2 (85484978, bb55462a); the first collapses the resolveTimelineMove unit-drift Rames' 🟡 R2#4 flagged, the second is a docblock relocation. The remaining Rames R2 non-blockers still land at the stack tip in 6ee750fee (PR #2791). Standards lens clean. Consistent with my prior #2781 R2 disposition — this PR merges as part of the atomic stack.
Scope re-verification
693 additions / 223 deletions across 16 files; head bb55462a; base codex/studio-timeline-b-keyframe-state-v2 (i.e. #2781's branch). PR body has no Co-Authored-By. Neither does any commit message (bf12fcc, 85484978, bb55462a — all miguel-heygen, no coauthor line). Envelope clean.
Delta from R2 head 514a219 — audit
The two new commits since Rames' R2 pass:
85484978 — refactor(studio): give resolveTimelineMove a row-based vertical axis
Rewires TimelineMoveInput from pixel-plus-scale to a pure row axis. Removed: originClientY, originScrollTop, currentScrollTop, trackHeight. Added: originRow: number with a docblock (timelineEditing.ts:49-50 — "Vertical position as a track-row index, not pixels: rows vary in height."). The third positional arg to resolveTimelineMove is now currentRow instead of clientY (:109). Body simplifies to const trackDeltaRaw = currentRow - input.originRow; (:123), with the comment at :120-122 naming exactly why the scroll/height fold moved to the caller: rows have varied since lanes expand, one trackHeight can't describe them.
Caller-side accounting at timelineClipDragPreview.ts:137-159 matches: originRow = getTimelineRowFromY(drag.originClientY - scrollRectTop + drag.originScrollTop, ctx.rowHeights) and currentRow = getTimelineRowFromY(clientY - scrollRectTop + scrollTop, ctx.rowHeights). Vertical scroll and per-row heights are folded in at the pointer boundary, exactly where the piecewise getTimelineRowFromY inversion already had to be.
Downstream trackDeltaRaw consumer at timelineLayerDrag.ts:362 (targetPosition = currentIndex + input.trackDeltaRaw) is behavior-identical: the value is still the same piecewise-linear row delta Rames observed, just arriving through a typed row-index API instead of an implicit pixels/1 fixture. Rames' original text explicitly said the piecewise semantics were "Fine for Math.round(trackDeltaRaw) integer track-crossing logic (a taller row physically requires more pixels to trigger, which is desired UX)" — the concern was the API drift, not the stacking behavior, and this commit exactly targets the API drift.
Search across the repo confirms the callers are only the one production site (timelineClipDragPreview.ts), the type re-export in timelineClipDragTypes.ts (no shape assumption there), and the unit tests in timelineEditing.test.ts. All three land in the same commit. No hidden caller left on the old shape.
Test file adjustments (timelineEditing.test.ts:27-206) preserve behavior: every prior (clientY - originClientY) / trackHeight case is rewritten with the equivalent row delta inline (e.g. (390 - 200) / 72 → 190 / 72). The stacking-mode case shifts from clientY: -72 at trackHeight 72 to currentRow: -1 — arithmetic identity. The scroll-displacement case is correctly renamed to "accounts for horizontal scroll displacement" with an inline comment naming why vertical scroll never reaches here anymore.
Minor readability note: the tests keep / 72 inline as an arbitrary "assumed row height" — since TRACK_H is actually 48 in timelineLayout.ts:6, the 72 is a legacy test-local constant, not tied to any exported value. Doesn't matter for coverage (the math is self-consistent), just a nit if someone greps 72 looking for the source constant. Not blocking.
bb55462a — refactor(studio): drop the duplicated row-top docblock
Cosmetic: the "y (content-space) of the top edge" docblock was attached to the TRACKS_LEFT_PAD const above the wrong interface. Moved to sit directly above getTimelineRowTop at timelineLayout.ts:101-107, which is the function it actually describes. Zero behavioral change; getTimelineRowTop, getTimelineRowFromY, and their callers are byte-identical either side.
Rames R2 residual disposition — verified at 6ee750fee
Every 🟡/🔵/🟢/ℹ️ finding Rames raised at 514a219 carries Miguel's "Addressed at stack tip in 6ee750fee (PR #2791)" reply. Spot-checked each at that SHA:
- 🟡 R2#1
invalidateGsapCachefresh closure every render (App.tsx:177) — at6ee750feethe arrow is lifted into auseCallback(() => invalidateGsapCacheRef.current(), [])atApp.tsx:162, threaded intouseTimelineEditingat:180. Stable closure. Fixed. - 🟡 R2#2 row-extrapolation past last row uses
TRACK_H(timelineLayout.ts:94) — at6ee750feethe code is unchanged but a comment at:100-103documents the asymmetry as intentional ("DeliberatelyTRACK_H, not the last row's height: rows past the end do not exist yet, and a row created by dropping there starts unexpanded"). Legitimate response — turns a 🟡 concern into a documented decision. - 🔵 R2#3 single-clip
.finallyvs group post-await asymmetry (useTimelineEditing.ts:203) — at6ee750feethe.finallyremains at:203,312. Rames rated this 🔵 (informational); Miguel's boilerplate "addressed" is imprecise for this one, but the severity is below the file-a-blocker line. Noted for the record. - 🟡 R2#4
resolveTimelineMovepiecewise trackDeltaRaw — ADDRESSED IN THIS PR by85484978(see delta audit above). - 🟡 R2#5 group-mode
invalidateGsapCacheruns on non-mutating batches (useTimelineGroupEditing.ts:333) — at6ee750feethe code path still callsinvalidateGsapCache?.()after any non-trackOnly return; theuseCallbackfix in R2#1 mitigates the downstream memoization impact even when the invalidation runs unnecessarily. Symptom softened, root cause parked. 🟡 → informational. - 🟢 R2#6
getTimelineRowPositionFromYdoubles piecewise scan (timelineLayout.ts:137) — at6ee750feegetTimelineRowOffsetsgets aWeakMapcache (rowOffsetsCache.get(rowHeights)/.set(...)), so the second scan is aMap.get. Fixed. - 🟢 R2#7 optional-typed
invalidateGsapCachewith?.()guards (useTimelineEditing.ts:203) — at6ee750feetheuseCallbackgives it a stable identity but the?on the type +?.()at the call site remain. Trade-off accepted: stable closure over required-arg refactor. 🟢 nit unresolved but downgraded by the R2#1 fix. - ℹ️ Rames' info:
getTimelineInsertBoundaryBandhas no direct unit test — checkedtimelineLayout.test.ts; atbb55462athere is a new "computes insert boundary band for the concrete row" block (added in the base PR's diff, present at the current head). Direct coverage exists.
R1 residual disposition
My R1 raised three 🟡 nits (edge track-create threshold scaling with row-0 height, .finally invalidation on rejection, trackHeights() with no production consumer). PR body cites them as parked verbatim in .scratch/studio-timeline-family-b/issues/09-family-b-v2-r1-deferred.md. All three are architectural observations that the wiring PR (later in the stack) resolves; none are bb55462a-scoped.
Adversarial audit — six-category internal-boundary
Refreshed against the R2 → bb55462a delta:
- State-accumulation discard — the
TimelineMoveInputrefactor dropsoriginScrollTop/currentScrollTop/trackHeight; the caller now consumes those pixels immediately ingetTimelineRowFromY, so there's no cross-boundary accumulator to lose. Clean. - Return-boundary invariant —
resolveTimelineMovereturn shape is unchanged ({ start, track, previewLayerId?, previewLayerIndex?, stackingReorder? }). Consumers incomputeDragPreviewstill unpacknextMove.startandnextMove.track(timelineClipDragPreview.ts:186-187). Invariant preserved. - Library defaults on failure axis —
Math.max(input.pixelsPerSecond, 1)still guards the divisor;input.trackHeightguard is deleted along with the field. New code has no analogous divisor (trackDeltaRaw = currentRow - input.originRow), so no new default to get wrong. - Precedence in overlap rules — stacking-mode short-circuits on
input.stackingElement(:136); the layer-move branch still usestrackDeltaRawunchanged. Non-stacking track-clamp math at:155-177is byte-identical. - Session/resource ownership — n/a for a pure function. No hooks / timers / DOM handles touched by the delta.
- Discovery/enumeration completeness —
resolveTimelineMovedoesn't enumerate; the caller'sgetTimelineRowFromYwalksrowHeights— same walker as before.bb55462ais docs-only. No new enumeration axis.
Editor-UI lens set (12)
Delta-scoped where meaningful; the R1/R2 rounds already covered the R1-shape file set at 514a219.
- Silent-catch / error-invariant — none. The refactored
resolveTimelineMovehas no try/catch. - Commit semantics —
computeDragPreviewstill returns a singleDraggedClipStateper pointer sample; no interleavable multi-step transition introduced. - Key stability —
previewLayerIdstill derived fromresolveTimelineLayerStackingMove, keyed offelement.id/contextKey. Unchanged. - ARIA / keyboard — n/a (pure math functions).
- Propagation — n/a (no DOM handlers in the delta).
- Semantic-vs-symptom — the refactor is semantic: makes the API say what it means (rows, not pixels). The docblock move (
bb55462a) is documentation-semantic: describes the function it decorates. - Sibling helpers / DRY — the caller-side row conversion (
getTimelineRowFromYattimelineClipDragPreview.ts:137,141) is used symmetrically fororiginRowandcurrentRow— the same helper the drop-placement path (:94-99) already used. Consolidation, not duplication. - Parity audit — the SDK-only and fallback code paths in
useTimelineEditingare untouched by the delta; the API refactor is caller-side geometry only. - Cross-mode (edit / preview) — n/a; drag preview runs only in edit mode.
- Perf audit —
getTimelineRowFromYis called twice per pointer sample instead of once (originRow + currentRow). ButoriginRowis invariant across a gesture (drag.originClientY+drag.originScrollTop) — a future memo could cache it per-drag if profiler shows it, but a single piecewise scan on arowHeightsarray of <100 rows is a non-concern. Not blocking. - PR-body-vs-diff — body claim "
resolveTimelineMove… Its vertical axis is now a row index, and folding scroll and per-row heights into that index stays with the caller" maps exactly to the diff. Body claim "the only production caller was already passing cumulative row coordinates withtrackHeight1 and both scroll offsets zeroed" is verifiable at the pre-refactor state (514a219'stimelineClipDragPreview.ts:139-166, whereoriginScrollTop: 0,currentScrollTop: 0,trackHeight: 1are literally spelled out). Honest body. - Cross-writer invariant — the
trackDeltaRawvalue crossing intoresolveTimelineLayerStackingMoveis byte-for-byte the same quantity as before (row-index delta). Layer-stacking behavior is preserved.
Standards + Spec + Precision + Round-trip
- Standards (CONTRIBUTING.md §Type-safety): clean — see mechanical grep below.
- Spec bi-directional: body's R2-follow-up bullet ("
resolveTimelineMovestill took a pixeldeltaYplus a singletrackHeight, units that stopped being true once rows could expand: …") has direct code anchor in85484978. No orphan bullets, no orphan code. - Sibling-precision divergence:
getTimelineRowFromYis now called at three sites intimelineClipDragPreview.ts(:94-99drop placement,:137origin row,:141current row) — all withctx.rowHeightsas the same second arg. No precision drift. - Middle-man wrap-unwrap: the
originRow: numberfield is a value, not a wrapped-then-unwrapped object. No serialization boundary to leak through.
Standards lens re-run
Mechanical grep on added lines only (^+ slice of the full-PR diff at bb55462a; 709 added lines):
| Pattern | Count | Notes |
|---|---|---|
Bare as T (not as const, not as unknown as T) |
0 | — |
Non-null !. / 
Rows stopped sharing one pixel height when lanes gained expansion, so the only production caller was passing cumulative row coordinates with trackHeight 1 and both scrollTops zeroed. The parameter names described units the values no longer carried. The vertical axis is now a row index and the caller keeps ownership of folding scroll and per-row heights into it.
The getTimelineRowTop docblock had a second copy sitting on TimelineTrackHeightClip, where it describes nothing. Only the one on the function stays.
6f1194d to
4c7703f
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,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 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.R2 review follow-ups
Fixed in this PR:
resolveTimelineMovestill took a pixeldeltaYplus a singletrackHeight, units that stopped being true once rows could expand: the only production caller was already passing cumulative row coordinates withtrackHeight1 and both scroll offsets zeroed. Its vertical axis is now a row index, and folding scroll and per-row heights into that index stays with the caller.