perf(studio): centralize timeline viewport geometry - #2700
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. |
73d1bb5 to
ecf76d7
Compare
af82757 to
13367e5
Compare
ecf76d7 to
a69af1c
Compare
13367e5 to
9a2c975
Compare
a69af1c to
832f7b3
Compare
9a2c975 to
d9c532f
Compare
832f7b3 to
57aa9f0
Compare
d9c532f to
eb24bd5
Compare
57aa9f0 to
6f4ff16
Compare
eb24bd5 to
684ce21
Compare
6f4ff16 to
c41fac2
Compare
684ce21 to
030583a
Compare
c41fac2 to
42579a9
Compare
030583a to
028174c
Compare
028174c to
f43747b
Compare
Fallow audit reportFound 5 findings. Duplication (4)
Health (1)
Generated by fallow. |
f43747b to
eab22e6
Compare
eab22e6 to
9e92dbc
Compare
29b22a9 to
4edbc70
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade B — rubric PARTIAL
Role in stack: D5 refactor — viewport geometry centralization (SSOT for scroll/zoom/time-to-x + row-index-to-y across drag, marquee, layout, active clips, scrub).
Root-cause fit: Good. New timelineViewportGeometry.ts owns time-range + row anchoring; useTimelineScrollViewport owns DOM reads (scrollLeft/Top/clientW/H + isScrolling + RO wiring + settled-timer). timelineLayout.ts owns row geometry as a frozen TimelineRowGeometry (binary-search y↔row over cumulative offsets, WeakMap-cached per height-array), consumed by drag/marquee/drop through rowGeometryRef.
Claims verified (file:line at HEAD 9e92dbc9):
- New module
timelineViewportGeometry.ts:1-31—getTimelineVisibleTimeRange+getTimelineScrollTopForGeometryChange. - Snapshot hook
useTimelineScrollViewport.ts:47-155— rAF-throttled viewport publish + settled timer + RO + shortcut-hint sync. - Row geometry SSOT
timelineLayout.ts:78-172— immutableTimelineRowGeometry, WeakMap cache, binary search ingetRowFromY. - Element indexes
timelineElementIndexes.ts:1-43— one scan per snapshot, WeakMap-cached; consumed byTimeline.tsx:118,useTimelineClipDrag.ts:761,playbackScrub.ts:6. - Test coverage
Timeline.test.ts:47-66,useTimelineScrollViewport.test.tsx,timelineElementIndexes.test.ts.
Adversarial findings:
- P2 — Callsite migration is partial. PR body claims "one geometry model owns scroll, drag, selection, layout, playback".
useTimelineRangeSelection.ts:1155-1339still does its ownscrollRef.current?.getBoundingClientRect() / .scrollLeft / .scrollTopreads for pointer→content mapping;useTimelinePlayhead.ts(16 refs),useTimelineRevealClip.ts(10 refs),timelineEditing.ts(9 refs),timelineClipDragPreview.tsalso bypass the snapshot. Defensible for pointer-freshness (state snapshot lags by rAF + settled timeout), but the body language is stronger than what shipped — worth naming the exceptions. - P3 —
setTimelineScalewriter with no reader.playerStore.ts:480-486adds non-reactivetimelinePps/timelineFitPps, published fromuseTimelineGeometry.ts:465-467. No consumer exists.useTimelineEditPinning.ts:41still pins viappsRef.current/fitPpsRef.current. Docstring namespinTimelineZoomToCurrent, absent. Prep for D6 — call it out or drop it. - P3 — In-place mutation on Zustand state.
state.timelinePps = ppssidestepsset()— silent hazard if anyone selects this field viausePlayerStore(s => s.timelinePps). Comment warns "never read as render input", but the type is public. ASymbol-keyed scratch or plain module-level ref would advertise the contract more loudly. - Nit —
getTimelineVisibleTimeRangeships zero overscan; callers should know.endclamps toduration(comp), notdisplayDuration(extent) — semantically "visible clip time," which the name doesn't disclose. - Nit — Reference stability:
TimelineScrollViewportSnapshotis a fresh object every rAF while scrolling; downstreamuseMemo([viewport])will re-run each frame. Consumers should destructure fields.
Standards: No new any, empty catch, TODO, disabled test. oxfmt/oxlint/typecheck pass. useEffect uses are external-system sync — permitted.
CI state: All 8 required checks pass. Graphite mergeability_check pending (transient).
Suggested next step: Land as-is (awaits Family D landing order). Follow-up: either delete the unused setTimelineScale/timelinePps writer or ship its consumer in D6, and document the pointer-freshness carve-out in the geometry module header.
— Review by Via
9e92dbc to
b15627d
Compare
4edbc70 to
2b89498
Compare
|
Addressed at
|
2b89498 to
f5cd679
Compare
b15627d to
02ca243
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-delta review at 02ca243f1a (additive to the existing review).
I audited all 29 changed files: the row-geometry/store/index production paths, every migrated caller, focused tests, and the Chromium harness. I am not treating the rest of the stack as evidence for this PR.
No new actionable finding. The geometry conversion remains symmetric at the boundaries; row snapshots are rebuilt from immutable array identities; session changes reset scroll ownership once; and the resource audit is clean (ResizeObserver, rAF, scroll-settle timer, PerformanceObserver, probe page, and browser are all released on their terminal paths). The PR body now accurately limits the SSOT claim to stable layout geometry and leaves gesture-time DOM reads explicitly live.
The exact functional CI is green. Graphite's remaining state is a stack/dependency gate, not a failing check in this delta.
Verdict: APPROVE
Reasoning: The exact delta preserves behavior while centralizing the stable geometry contract, and I found no additional correctness, lifecycle, or cleanup defect beyond feedback already recorded on the PR.
— Magi
02ca243 to
eef5fb4
Compare
60e3c4f to
3f74418
Compare
eef5fb4 to
0546ad3
Compare
3f74418 to
f8cd5e9
Compare
0546ad3 to
3702ced
Compare

What
Centralize the stable, layout-owned timeline viewport geometry used by row layout, visible-range derivation, scrolling snapshots, indexes, and playback consumers.
Scope boundary
This PR does not migrate every coordinate read to the cached geometry model. Gesture-time code in range selection, playhead interaction, reveal, editing, and drag preview intentionally keeps direct
getBoundingClientRect()/scrollLeftreads where pointer freshness matters. The shared model owns stable layout geometry; live pointer handlers own live DOM coordinates.How
TimelineRowGeometrysnapshots with identity-based caching.Test plan
Review fixes are included at head
b15627d5e.