perf(studio): virtualize timeline thumbnail media - #2718
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. |
692ade6 to
b3cc374
Compare
7b9b479 to
28df3e3
Compare
b3cc374 to
dd2a3c3
Compare
28df3e3 to
f58e857
Compare
dd2a3c3 to
39721de
Compare
f58e857 to
5c9fe27
Compare
39721de to
ea46952
Compare
5c9fe27 to
5e021b3
Compare
ea46952 to
ed6e367
Compare
5e021b3 to
c77556b
Compare
ed6e367 to
b7cc6a1
Compare
c77556b to
f885ac4
Compare
b7cc6a1 to
b1a3e3d
Compare
b1a3e3d to
a70911d
Compare
f8b7c78 to
abc967f
Compare
a70911d to
6fedf02
Compare
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at 6fedf021.
The last-mile virtualization: *Thumbnail.tsx components hand over decode + IO state entirely to useThumbnailLease/thumbnailScheduler, and the actual DOM windowing is the earlier useTimelineVirtualRows + useTimelineRowVirtualization machinery. Executed cleanly.
Verified clean:
- 60/385 mount ratio driver:
rowOverscanPerSide: 2attimelineViewportBudgets.ts:59, wired viaoverscanatuseTimelineVirtualRows.ts:85; clip-window is time-based (timeOverscanViewportRatio: 0.25). Thumbnail components render only when their host row+clip is mounted. - Selected-clip retention:
pinnedRowKeysatuseTimelineRowVirtualization.ts:76-94unionsselectedIdentity?.rowKey,focusedRowKey,draggedRowKey,resizingRowKeys, and both context-menu row keys.Timeline.virtualization.test.tsx:274-349explicitly assertsclip-490survives an off-window horizontal scroll. - State preservation across remount: cached snapshots return immediately from
ThumbnailScheduler.getSnapshotatthumbnailScheduler.ts:192-198; the "which frame" state is derived fromrequest.key. - Fast-scroll cancellation:
controller?.abort()atthumbnailScheduler.ts:181when leases hit zero;throwIfAborted(signal)between each frame atthumbnailVideoDecoder.ts:85, 89. - Keyboard navigation: focused row pinned via
domFocusedRowKey/focusedRowKeyatuseTimelineRowVirtualization.ts:58-64, 102.Timeline.virtualization.test.tsx:218-232assertsdocument.activeElementsurvives a big scroll after focusing row 0. - Video handle release: no
HTMLVideoElementin the new path; Mediabunnyinput.dispose()atthumbnailVideoDecoder.ts:165+ object-URL revoke atthumbnailVideoDecoder.ts:55-62. - -447 deletions: removed the old
HTMLVideoElement+canvas.toDataURL+IntersectionObserverextractor along with its three tightly-coupled tests. New test names (renders a scheduler-provided sparse poster,requests a rich filmstrip only for interaction actors) are semantically consistent with the new plumbing — nomin↔max/floor↔ceiling/keep-mounted↔virtualizeinversion.
Two nits (body-only):
-
CompositionThumbnailinlines strip-layout math atCompositionThumbnail.tsx:117-118(frameWidth = Math.max(48, …); frameCount = Math.max(1, Math.ceil(containerWidth / frameWidth))).thumbnailUtils.ts:62exposescomputeThumbnailStrip(containerWidth, aspect, clipHeight)used by bothVideoThumbnail.tsx:69andImageThumbnail.tsx:54— Composition duplicates the logic with a distinct 48-px minimum. A future change to the shared helper (e.g. clamping tile width to physical-pixel budgets) wouldn't apply to the Composition path; the two would drift. Non-blocking cleanup. -
No direct test asserts shimmer clears when the scheduler enters
errorstate forVideoThumbnail. The two #2214-era guards (SecurityError / no-CORS 0-frame error) were removed along with their now-dead HTMLVideoElement extractor. The equivalent semantic —snapshot.status === "error"yielding no.animate-pulse— is coded correctly atVideoThumbnail.tsx:111, but a future refactor could weaken the guard (e.g.snapshot.status !== "ready") with no test failing. Consider arenders no shimmer when the scheduler entry is in error statecase; #2214 regression risk is low but nonzero.
Nothing blocking. Solid stack — the metrics story checks out against the mechanism.
vanceingalls
left a comment
There was a problem hiding this comment.
APPROVE — grade B+
Reviewed at head 6fedf0214. Focused on: DOM/SPA retainer-leak posture (the primary risk of a wrapper-replacement PR that touches <video>/<img>/blob URL lifetimes), element-identity keying, and the runtime contract between the thumbnail components and their callers.
What I verified
Retainer-leak posture — clean sweep across all three components.
Each of ImageThumbnail, VideoThumbnail, CompositionThumbnail used to own its own IntersectionObserver + on-visible useEffect that mounted a hidden <img> or <video>. Those effects were the exact anti-patterns from the feedback_dom_spa_retainer_leak_review_lens axes (Blink pending-activity keeps the Fiber reachable via the un-cancelled request even when the DOM node is hidden). This PR retires them all:
probeImageAspect(thumbnailUtils.ts:16-58) — off-DOMnew Image(), explicitonload=null; onerror=null; signal.removeEventListener; image.src = ""on abort. Matches axis-1 verbatim: nulling handlers alone doesn't cancel Blink's pending-decode task;src = ""triggers the abort. Also handles the "already-aborted-at-entry" case.loadCompositionImage(CompositionThumbnail.tsx:69-84) — fetch → blob →URL.createObjectURL→ probe → return{ value, weight, dispose }. Object URL revocation lives indispose, called by the scheduler at eviction or on final-release; thecatcharoundprobeImageAspectalso revokes on failure. No dangling blob URL on failure or unmount.VideoThumbnail.tsx(source) — the whole hidden-<video>+ seek + canvas.toDataURL pipeline is gone; delegates todecodeVideoThumbnailfrom #2717 (which itself has clean dispose semantics — reviewed there).- The abort test at
CompositionThumbnail.test.ts:100-118proves the unmount path:probe.srcbecomes"", handlers becomenull,URL.revokeObjectURLis called with the blob URL. Direct evidence for the retainer chain being cut.
Element-identity keying — createThumbnailKey(...) scoping includes kind/source/optional rich/start/duration/frames so two clips referencing the same source but with different sourceStart/sourceRangeDuration/rich get distinct entries. React key={index} inside Array.from({length: frameCount}, ...) is length-based and collision-free.
Snapshot-driven rendering — no more useEffect(setDraft, [value]) echo snap-back or intermediate state syncing. The old components maintained loaded/aspect/frames/failed local state and mutated them across three useEffects; the new components derive value = snapshot.status === "ready" ? snapshot.value : null and aspect per-render from value. Cleaner + matches the no useEffect for state syncing rule.
Fallback UX — shimmer via snapshot.status === "loading", disappears when ready OR when the request settled to error. The VideoThumbnail shimmer is now urls.length === 0 && status === "loading" — a filmstrip in flight still shows shimmer, once any URL is ready the strip renders. Same failure contract as before (no shimmer on error), but the failure signal is now the scheduler snapshot instead of ad-hoc failed state.
Findings (non-blocking follow-ups)
1. useRenderClipContent at head does NOT plumb the new props through to the thumbnail components — the priority-tiered scheduler is inert against the real callers.
The dispatcher at useRenderClipContent.ts:112-165 still calls each component with only imageSrc/videoSrc/previewUrl, label, labelColor, duration. The new projectId / sessionEpoch / priority / rich props all take their defaults, which are:
projectId = imageSrc | videoSrc | previewUrl(media URL, NOT the real project id)sessionEpoch = 0(never bumps)priority = "visible"(uniform across visible and overscan rows)rich = false(posters only)
Consequences today:
- The scheduler's per-project cache budget (
thumbnailCacheEntriesPerProject = 96) segments by URL, not by project — every unique source URL is its own "project" bucket, so the cap is effectively per-source-URL rather than per-Studio-project. thumbnailScheduler.invalidateProject(projectId)is never called from production code anywhere (verified: only tests reference it). Cached blob URLs and scheduler entries survive project switches until LRU eviction (256 entries / 64 MiB) instead of being released promptly. Bounded, not a hard leak, but not the crisp lifecycle the scheduler is designed for.- Row-overscan rows request at the same priority as visible; the scheduler ordering never actually differentiates for the real timeline.
Presumably #2719 (prioritize timeline thumbnail work) / #2720 (coordinate cancelable thumbnail generation) is where these get wired. Worth verifying in those reviews — if either merges without threading projectId + priority + a project-switch invalidateProject call through useRenderClipContent, the family's per-project budgets and priority-tiered ordering stay theoretical. Not a blocker for THIS PR (this PR's stated scope is the media-component swap; the caller-side props are a separate concern), but worth surfacing to the family reviewer.
2. Feature-flag gate coverage — the family lives behind VITE_STUDIO_TIMELINE_THUMBNAIL_POLICY from #2716. force-hidden short-circuits inside useRenderClipContent before any thumbnail component mounts, so force-hidden is a clean rollback lever for this PR too. legacy-default seeds new users to hidden — safe.
Envelope
Clean: single commit, no Co-Authored-By: Claude trailer, no AI-attribution footer.
What I didn't independently verify
- 385 → 60 mounted clips after scroll — that measurement lives at the timeline row-virtualizer boundary (
useTimelineVirtualRows), not the thumbnail components themselves. This PR removes the eager-decode work from the mounted clips, which is what makes the 60 remaining clips affordable. The mount-count reduction itself is the virtualizer's; this PR is the multiplier that makes each mounted clip cheap.
Review by Via
abc967f to
9cd0e30
Compare
6fedf02 to
5ae9a8a
Compare
|
Exact-head follow-up at 5ae9a8a: CompositionThumbnail now reuses the shared computeThumbnailStrip helper while preserving its 48 px minimum, and VideoThumbnail has explicit coverage that decode failure clears the shimmer and image content. I also audited the downstream handoff: #2719 is the single caller-side owner that threads the real project/session identity, priority, and rich mode into all thumbnail components; active scheduler entries are protected from live invalidation by #2716. |
jrusso1020
left a comment
There was a problem hiding this comment.
Verified at 5ae9a8a96a. Approving.
Required CI is present and green at this exact head — all eight contexts enumerated from the branch ruleset itself, with cancelled filtered before taking the latest per context: Build, Test, Typecheck, Test: runtime contract, Render on windows-latest, Tests on windows-latest, Semantic PR title, regression. That closes the absence I flagged while this was based on a codex/* branch, where the ruleset's ~DEFAULT_BRANCH target never applied and most of the matrix simply never ran. Graphite / mergeability_check is still pending and is not in the required set.
This branch carries the earlier scheduler work, so I checked that it carries the fixed version of it rather than a pre-review snapshot. Both findings I cared about are present at this head in thumbnailScheduler.ts:
invalidateProjectnow skips entries with live leases (:217,if (entry.leases.size > 0) continue;). This was the one I most wanted covered — it was the only path that could delete an entry out from under a mounted consumer, and becausesubscribeis dep-gated on[identity, scheduler]that consumer would have rendered empty and never re-acquired.deleteEntryremoves from the map before notifying, with anentries.get(key) !== entryre-entrancy guard (:447-450), so a synchronous re-acquire inside a listener can no longer produce a zombie lease.
loadWithTimeout is present too, so the hung-loader watchdog came along with it.
Worth stating plainly since it changes what this stamp means: because the branch is cumulative, approving and merging it lands the two preceding PRs' content as well. Ascending merge order keeps that coherent, and merged that way each later branch's diff shrinks without a new push, so approvals stay valid.
Not merging.
Review by Rames Jusso
vanceingalls
left a comment
There was a problem hiding this comment.
Fresh-stamp at 5ae9a8a — required CI green (all 8), no unresolved threads, diff-shape matches prior-approved review (includes 2716+2717 content since neither on main yet; 2718-specific delta within ~1% of prior, rebase friction). — Via
|
Superseded by consolidated Family G tip #2720, which is now merged with the complete reviewed and tested stack. |

What
Virtualize image, video, and composition thumbnail media inside timeline clip shells.
Why
Row and clip virtualization should also prevent expensive media elements from decoding outside the active window.
How
Keep stable clip geometry while mounting rich media only for eligible visible work, using the shared scheduler and decoder leases. Update image/video component tests for lifecycle and fallback behavior.
Test plan
Family G tip validation: 78 changed-surface Studio tests, 19 focused server tests, 3,026 full Studio tests, 406 full Studio Server tests, both typechecks, format, lint, file-size gate, and full workspace build.