perf(studio): add dev-gated timeline row virtualization - #2701
perf(studio): add dev-gated timeline row virtualization#2701miguel-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. |
9fda27f to
9b91ca5
Compare
13367e5 to
9a2c975
Compare
9b91ca5 to
8a8a531
Compare
9a2c975 to
d9c532f
Compare
8a8a531 to
0a94dbf
Compare
d9c532f to
eb24bd5
Compare
14895e2 to
e494530
Compare
eb24bd5 to
684ce21
Compare
e494530 to
6359e3f
Compare
684ce21 to
030583a
Compare
6359e3f to
bde178d
Compare
030583a to
028174c
Compare
bde178d to
9565e75
Compare
Fallow audit reportFound 5 findings. Duplication (4)
Health (1)
Generated by fallow. |
9565e75 to
ee01158
Compare
eab22e6 to
9e92dbc
Compare
ee01158 to
108bd87
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: COMMENTED — grade B — rubric PARTIAL
Role in stack: D6 behavior — playhead follow across virtualized rows. Ships (a) horizontal playback follow unconditionally to prod, (b) @tanstack/react-virtual-backed vertical row windowing behind import.meta.env.DEV && VITE_STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED === "1" — dev-only.
Root-cause fit: useTimelinePlayhead subscribes to liveTime (imperative store, not state re-render) and per-tick writes scroll.scrollLeft via getTimelinePlaybackFollowScrollLeft. Row virtualization touches only scrollTop; follow touches only scrollLeft — orthogonal axes, so "across virtualized rows" holds mechanically (follow never queries row DOM). focusIdentity resolves off logical TimelineElement.track, not mount state; pinnedRowKeys extractor unions overscan with focused/reveal/dragged/menu keys so out-of-band actors stay mounted.
Claims verified (file:line at HEAD 108bd87e):
- Follow trigger + gates:
useTimelinePlayhead.ts:124-149(isPlaying && !isDragging && zoomMode !== "fit"). InstantscrollLeftassign — no smooth-scroll, so reduced-motion is a non-issue. - Bounded mount:
Timeline.virtualization.test.tsx:76-79asserts ≤16 rows over 1,000 elements; focus preserved after scroll to row 500 (:89-98). - Hysteresis:
timelineLayout.ts:434-465— advance zonex > followLine (0.75), pull-back zonex < visibleStart; disjoint → no oscillation. Clampedmax(0, min(max, next)). - Pins survive session-epoch reset:
useTimelineVirtualRows.ts:78-86dispatches synthetic scroll so virtualizer re-observes the DOM offset Timeline just reset.
Adversarial findings:
- P2 — body-vs-code mismatch on "respecting explicit user scrolling": PR body says "Follow playback horizontally while respecting explicit user scrolling." Code has no user-scroll gate — the
liveTimetick correctsscrollLeftcontinuously. During playback the user cannot peek ahead or scrub-inspect earlier time without the next tick yanking the viewport. Only slack is the[visibleStart..75%]band. Either drop the claim or add a "user recently scrolled" ref (alastUserScrollTschecked before thescrollLeftassign inuseTimelinePlayhead.ts:139-148). - Nit —
useMountEffectsubscribes once;isPlayingsampled viagetState()each tick — fine, but a hot-reload / provider swap ofusePlayerStorewon't re-bind. Acceptable given store lifetime. - Nit — Row virtualization is DEV-only via
import.meta.env.DEV. That is intentional per the flag comment ("Disabled until horizontal windowing and stable gesture lifetime land") but the PR title "follow playhead across virtualized rows" oversells what ships to prod.
CI state: Preflight + all perf shards + Preview parity + player-perf + preview-regression + regression all pass. Graphite mergeability pending. Mergeable=MERGEABLE.
Suggested next step: Reconcile the body claim about "respecting explicit user scrolling" — either land a scroll-suspend gate or soften the claim to "advances the viewport to keep the playhead in the trailing 25%." Otherwise sound.
— Review by Via
108bd87 to
e454d16
Compare
9e92dbc to
b15627d
Compare
|
Addressed at
|
e454d16 to
32554de
Compare
b15627d to
02ca243
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-delta review at 32554de65c (additive to the existing review).
Important — the enabled path is unbounded precisely while the viewport is unmeasured or hidden. useTimelineRowVirtualization.ts:51-54 folds clientWidth/clientHeight > 0 into enabled; useTimelineVirtualRows.ts:82-86 then returns allRows whenever that value is false. The initial viewport snapshot is zero-sized, so a flagged 10k-row project first mounts all 10k rows before the ref/ResizeObserver can enable virtualization. A collapsed or transiently zero-height timeline does the same again. The integration test masks this by globally stubbing positive dimensions before the first render (Timeline.virtualization.test.tsx:30-40).
Please distinguish “feature flag disabled” (compatibility: all rows) from “flag enabled but geometry not ready” (bounded seed/pins or synchronous measurement). Add a zero-size-first-render test that proves the flagged path never mounts O(total rows).
The rest of the exact delta is coherent: actor/focus pins are identity-based, row height and scroll-margin geometry agree, epoch resets have a single writer, and listener/effect cleanup is balanced. I did not repeat the already-documented lack of a user-scroll suspension timer.
Verdict: REQUEST CHANGES
Reasoning: Steady-state virtualization is correct, but its initial/hidden-state fallback can perform the full mount that this mechanism is intended to prevent.
— Magi
32554de to
bd0dccf
Compare
|
Resolved at
Focused virtualization tests pass, the full Studio suite passes (291 files / 3,195 tests), and typecheck/format/lint pass. At the restacked tip, the default Chrome gate passed 5/5 and the 50k virtualized gate passed its required 4/5 quorum with memory returned. |
02ca243 to
eef5fb4
Compare
0690067 to
9f3ef4a
Compare
0546ad3 to
3702ced
Compare
a451e13 to
ca7d212
Compare
3702ced to
7a90b73
Compare
ca7d212 to
5fc5ef6
Compare
7a90b73 to
1aa53ce
Compare

What
Add development-gated timeline row virtualization plus playhead-follow behavior.
Scope boundary
Row virtualization in this PR is enabled only when
import.meta.env.DEVandVITE_STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED=1; the production/default path still mounts all rows. The playhead follow logic keeps playback inside the trailing 25% of the visible horizontal band. It does not implement a user-scroll suspension timer.How
The default-path scroll-cost fix lands in #2883, and the permanent two-state CI gate lands in #2884.
Test plan
Review fixes are included at head
bd0dccfd5.