perf(studio): virtualize timeline clip windows - #2703
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. |
0afcb3c to
b7a9c06
Compare
6883a5e to
99125c6
Compare
b7a9c06 to
da7adb9
Compare
99125c6 to
994baf5
Compare
da7adb9 to
4d007db
Compare
994baf5 to
ef83c66
Compare
4d007db to
6826c9f
Compare
ef83c66 to
2dd814a
Compare
6826c9f to
1964f10
Compare
2dd814a to
b09e9f9
Compare
1964f10 to
0fad6e5
Compare
b09e9f9 to
c2cae90
Compare
0fad6e5 to
0c6561c
Compare
236fc38 to
1761957
Compare
1761957 to
79f7704
Compare
c588a64 to
143d281
Compare
vanceingalls
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — grade A — rubric CORRECT
Role in stack: D8 payoff — clip-window virtualization delivering the -99.3% / -93.3% / -35.3% wins on the D1-D8 stack.
Root-cause fit: TimelineLanes.tsx:122 iterates virtualRows.map(...) (bounded row set), and inside each row queryTimelineClipIndex(clipIndex, trackNum, renderTimeRange, pinnedClipIdentities) (:129) returns only overlap + pinned clips. Off-window clips genuinely unmount: renderElements = els.filter(el => indexedRenderSet.has(el) || <multi-drag passenger>). No accidental keep-alive path.
Perf claim verification: PR body cites two consecutive strict 50k-element Chromium passes at 207 mounted clip roots (rebase update: 240) against maxMountedClipRoots: 512 / maxMountedClipRootsPerRow: 128 (timelineViewportBudgets.ts). Budgets internally consistent; observed well under cap. Overscan timeOverscanViewportRatio: 0.5 (2× viewport window), rowOverscanPerSide: 4. player-perf + preview-regression + Preview parity all green. Magnitudes plausible; independent Chromium re-run not attempted per read-only scope.
Claims verified (file:line at HEAD 79f77049):
- Pinned actors — selected/dragged/resized/reveal/focused-ease/clip-context/keyframe-context — all seven:
useTimelineClipRenderWindow.ts:53-75, threaded fromTimeline.tsx:339-345. - Scrollbar accuracy: canvas outer at
TimelineCanvas.tsx:114usestotalH+ fulltrackContentWidth, list atTimelineLanes.tsx:118overlaysabsolute inset-0. - Sticky ticks:
useTimelineTicks.ts:14takesrenderTimeRangeandgenerateTicksfilters to window (Timeline.tsx:436). Major-tick interval derived from full duration, so labels don't shift as the window scrolls. - Active-clip lookup stable across mount churn:
useTimelineActiveClips.ts:117callsapplyActiveClipDiff(..., syncAll=true)onelementStateVersionchange; a remounted clip regainsdata-active. - Reveal defer / coordinate-first:
useTimelineRevealClip.ts:33-49scrolls from model coordinates (nogetBoundingClientRect);scrolledRequestRefgate +viewportVersiondep pins the request across scroll→mount cycle. - Drag survival:
pinnedClipIdentitiesincludesdraggedElementIdAND drag listeners are onwindow(timelineClipDragGestureLifecycle.ts:250-253) — two independent defenses. - Selection:
selectedElementIdslives inusePlayerStore, not DOM. Test atTimeline.virtualization.test.tsx:198,224,242,252scrolls 8000px away fromclip-490and asserts it stays mounted (via pin) AND reveal from off-window works.
Adversarial findings (all non-blocking):
- No direct test asserts a mid-drag clip survives virtualization. Two orthogonal defenses (pin + window listeners) cover it; a "scroll during drag" assertion would close the loop.
- Only reveal pins
revealElementId; a clip that gained focus via ordinary click and then scrolls out has nofocusedClipIdentitypin, so focus falls to<body>. Clips aretabIndex={-1}so focus is programmatic-only — practical hit small, but adocument.activeElement-derived pin would close it. useTimelineRevealClip.ts:49synthesizesnew Event("scroll")after programmaticscrollLeft/scrollTop, which also emit their own native scroll. Likely double-fires the snapshot listener; the debounce probably absorbs it, worth confirming.renderClipContent={viewport.isScrolling ? undefined : renderClipContent}(Timeline.tsx:529) is a nice second-order defense against rich-content churn during scroll.
Standards: No any, no new empty catch, no .skip/.todo, no TODO/FIXME. File-list matches (16 files); +775/-228 arithmetic checks.
CI state: All required green on 79f77049. Body claims 288 studio files / 3,168 tests pass and 29 server files / 398 tests pass on rebase.
Suggested next step: Land. Optional follow-ups: (1) explicit drag-across-virtualization test, (2) focusedClipIdentity pin off document.activeElement, (3) verify the synthetic "scroll" dispatch isn't duplicate-firing. None gate D8.
— Review by Via
143d281 to
4f820c4
Compare
79f7704 to
76db7a2
Compare
|
Addressed at
|
4f820c4 to
e5ce10a
Compare
76db7a2 to
46c0d25
Compare
e5ce10a to
06c0f1d
Compare
46c0d25 to
383514d
Compare
miguel-heygen
left a comment
There was a problem hiding this comment.
🔴 Changes requested at exact head 383514d48407a55d40670a926b4bf292cced6a1f.
I reviewed this as the exact #2703 delta (06c0f1df..383514d4), not as a blended stack. Two assumptions in the core windowing path do not hold.
Findings
P1 — the interval index is followed by a full-row scan on every render
TimelineLanes.tsx:128-155 queries queryTimelineClipIndex(...), converts the result to a Set, and then calls els.filter(...) across every clip in the row. That makes the hot scroll/render path O(n) in the total clips on each visible row, even though #2702 built the interval index specifically to make the same query proportional to the visible result.
The index already restores matches to original projection order (timelineClipIndex.ts:146-173, ordinal sort), so the full scan is not needed for z/DOM order. The only extra behavior in the filter is multi-drag passenger projection. That can be merged only when a multi-drag preview exists, without making every normal scroll walk the full row.
The browser gate cannot catch this shape: the 50k dense-short fixture intentionally distributes clips so each row has at most 128 roots (timelinePerformanceFixture.ts:63-69, and its test pins <= 128). A real long track with thousands of sequential clips still performs a thousands-element scan per visible row per scroll update.
Please render directly from the indexed result on the normal path, merge projected multi-drag passengers separately when active, and add a regression/perf fixture with one long row containing thousands of off-window clips.
P1 — an early reveal can be consumed before the viewport has usable geometry
useTimelineRevealClip.ts:117-133 marks a request as scrolled, focuses the pinned clip, and clears the request without requiring a non-degenerate viewport. On initial/hidden layout, clientWidth / clientHeight can still be zero. computeRevealScroll deliberately returns { left: null, top: null } for that geometry, but the reveal target is pinned so it can mount and focus successfully; the hook then clears the request. A later ResizeObserver / viewportVersion update has nothing left to retry, so the requested clip remains off-screen.
The new test sets clientWidth=300 and clientHeight=100 synchronously in the ref callback, so it never exercises the zero-size → ready transition.
Please keep the request pending until the relevant viewport axes are usable (or pass readiness explicitly), and add a regression test that starts at zero dimensions, mounts/focuses the pinned clip, then resizes and verifies the reveal scroll occurs before consumption.
Verification
git diff --checkpasses for the exact delta.- Current exact-head checks have no active failure, but the stacked branch only exposes the preview/regression subset; this review does not treat that as proof of the two missing shapes above.
Coverage ledger
Audited: all 16 changed files — Timeline.tsx, Timeline.virtualization.test.tsx, TimelineCanvas.tsx, TimelineClip.tsx, TimelineLanes.test.tsx, TimelineLanes.tsx, timelineClipDragGestureLifecycle.ts, timelineLaneProps.ts, useTimelineActiveClips.test.ts, useTimelineActiveClips.ts, useTimelineClipRenderWindow.ts, useTimelineRevealClip.test.tsx, useTimelineRevealClip.ts, useTimelineTicks.ts, useTimelinePlayer.seek.test.ts, useTimelinePlayer.ts.
Trusting: none.
— Magi
9b5b8b2 to
b8c4dd9
Compare
06c0f1d to
83f737a
Compare
b8c4dd9 to
2922d03
Compare
83f737a to
3a8a488
Compare
3a8a488 to
e2d7d99
Compare
2922d03 to
027a269
Compare
|
Addressed both exact-delta blockers on the current head
Verification: 84 focused tests pass, Studio typecheck passes, and the exact stacked 50k browser gate mounts 160 clip roots / 707 timeline descendants (4/5 hosted-runner quorum; all DOM budgets pass). Exact job: https://github.com/heygen-com/hyperframes/actions/runs/30512973405/job/90776821797 |

What
Render only visible clip roots and defer horizontal reveal until virtual scroll geometry is ready.
Why
The final scalability boundary must bound clips as well as rows without hiding selected or active clips, losing reveal requests, shifting ticks, or allowing an early reveal to use stale scroll geometry.
How
This is D8 of the Family D draft review sequence. It targets current main after Family C landed; its Family D patch was replayed without semantic changes. It supersedes legacy PR #2640, which remains open for audit until landing. The unpublished local scroll-deferral source was folded here because the combined diff is 971 changed lines and both changes own the same lifecycle boundary.
Test plan
Validated on the exact Family D tip with virtualization, active-clip, reveal, and player-seek coverage; the full Studio suite (2,939 passed; 18 todos); Studio Server files tests (67 passed); both typechecks; formatting/lint/file-size/Fallow gates; and two consecutive strict 50,000-element Chromium passes bounded to 207 mounted clip roots.
2026-07-29 rebase verification
No merge was performed; this PR remains a draft.