feat(studio): enable timeline virtualization by default - #2926
Conversation
vanceingalls
left a comment
There was a problem hiding this comment.
Approving. The flag flip is clean, the CI-protocol changes (21 samples across 3 sweeps vs 7-observation max; one server at a time; production React with observed-runtime assertion) are genuine improvements over the prior gate that the PR body accurately calls out. A few small notes below, all nits or reviewer questions — none blocking.
1. Test-hook leak surface if anyone builds --mode development for a preview deployment. STUDIO_TEST_HOOKS_ENABLED = DEV === true || MODE === "development" (packages/studio/src/hooks/studioTestMode.ts:22). The intent is to keep hooks alive when CI runs NODE_ENV=production bun run dev (dev server, prod React) but strip them from vite build. That works today because the only build script is vite build && tsup (no --mode override), so vite build --mode development isn't a path anyone in this codebase takes. But it's a latent footgun: any future ephemeral prod-like deployment built with --mode development would ship the __studioTest API to real users. Belt-and-suspenders would be adding import.meta.env.PROD !== true && to the guard, since PROD is set by Vite from the build command, not the mode string. Not a blocker on this diff, but worth capturing before someone reaches for --mode development for a canary.
2. Disable predicate accepts only the literal "0". timelineRowVirtualizationFlag.ts:9 — VITE_STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED !== "0". Setting the var to false, off, no, or disabled all leave virtualization ON. That's fine for a documented CI/emergency flag, but the module comment "Setting the environment flag to '0' keeps one explicit rollback path" is the only breadcrumb. If a user in an outage reaches for =false (natural reflex), they'll be confused. Either the module doc could call out the exact string, or the predicate could normalize (v === "0" || v === "false" || v === "off" → disable). Small.
3. Legacy Timeline.test.ts now pins to the rollback path. The added vi.mock("./timelineRowVirtualizationFlag", () => ({ STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED: false })) forces the un-virtualized code path for the entire legacy suite. The virtualized default is separately covered by Timeline.virtualization.test.tsx, so coverage exists — but the file named Timeline.test.ts now tests the fallback behavior while the primary behavior lives in the virtualization file. Minor semantic inversion; consider renaming to Timeline.rollback.test.ts or dropping the mock and porting to virtualized expectations over time. Doesn't affect CI.
4. scrollSamplesPerRun < 20 guard message says "at least 20" but default is 21. timelineViewportBudgets.ts:128-131. The lower-bound of 20 is the p95-index math (⌈20 * 0.95⌉ = 19, so 20 observations is the tightest that puts the 95th percentile at a distinct index). Setting the default to 21 gives a one-sample cushion, which is fine — but if the intent is "20 is the minimum that yields a meaningful p95," a brief comment on the constant next to the value would save the next reader the derivation. Prose-polish nit.
5. test:timeline-default script is now an alias. package.json:55 — "test:timeline-default": "bun run test:timeline-virtualization". Since "default" now means "virtualized" (which was previously the non-default), the two script names carry the same run. Consider dropping the alias or renaming the rollback path's script to test:timeline-rollback to preserve the mental mapping. Cosmetic.
Correctness confidence. Perf gate is convincing (5/5 at 50k clips, 5/5 under 4× CPU throttle, matching results on the explicit 1k-rollback arm). Perf isn't the same as correctness — drag/select/multi-select/keyboard-nav across the virtualized boundary aren't asserted here — but the underlying virtualization has been shipping opt-in behind the DEV && VITE_...=="1" gate for a while, so Miguel has presumably been dogfooding those interactions. If there's a correctness suite I've missed that exercises editor-UI parity across the row-virtualization boundary, ignore this paragraph; if not, worth capturing as a separate follow-up rather than gating the default-on flip on it.
— Via
james-russo-rames-d-jusso
left a comment
There was a problem hiding this comment.
Reviewed at a18831d2.
The default-flip is clean: !== "0" inverts the semantics without changing shape, and the two runtime signals (STUDIO_TEST_HOOKS_ENABLED on Vite's dev-server mode, STUDIO_RUNTIME_MODE on the DEV/NODE_ENV flag) correctly split "is this the dev server" from "is React in production mode". The CI restructure — one server at a time, NODE_ENV=production, runtimeMode === "production" gate at the mjs — moves the guarantee out of documentation-by-assumption and into a hard failure. The 21-sample / 3-sweep change fixes a real measurement bug: with the 7-sample p95 formula Math.ceil(7 * 0.95) - 1 = 6, p95 was literally the max, matching the PR body.
Concerns
Timeline.test.tsnow pins the rollback path. The addedvi.mocksetsSTUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED: false, which preserves the pre-flip test premise but leaves the full Timeline component's virtualized default path exercised only by the specializedTimeline.virtualization.test.tsxsuite. The core component-level Timeline suite continues testing what users no longer get by default. Not a blocker for this PR (the mock is defensive and correct), but worth a follow-up to add a companion suite that flips the mock (or replaces it with avi.stubEnv("VITE_STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED", "1")style so both paths run through the real flag module).
Nits
- No local script for the disabled/rollback arm.
test:timeline-defaultis now an alias fortest:timeline-virtualization(both invoke the on-arm). The CI workflow inlines theVITE_STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED=0+TIMELINE_ROW_VIRTUALIZATION=off TIMELINE_ELEMENT_COUNT=1000invocation, but there's no npm script equivalent for a developer wanting to reproduce the rollback arm locally. Atest:timeline-rollbackscript would restore local parity with what CI runs. scrollSamplesPerRun >= 20guard allows incomplete sweeps. The 7-ratio cycle in the mjs ([0, 0.25, 0.5, 0.75, 1, 0.5, 0]) only maps to complete sweeps at multiples of 7. Override to 20 or 22 works for p95 arithmetic but no longer describes whole sweeps. Not user-facing (default 21 = 3 complete sweeps), but the error string reads as "≥20 for p95" when it could additionally document/enforce "multiple of 7 for whole sweeps" if the sweep-count claim is load-bearing.
What I didn't verify
- Windows CI (
Tests on windows-latest,Render on windows-latest) was stillIN_PROGRESSat review time; the Ubuntu Studio timeline viewport gate at this HEAD passes, but the full check matrix should be green before merge. - I reasoned about (didn't independently rerun) Vite's
NODE_ENV=production→import.meta.env.DEV = falsepropagation. The mjs'sruntimeMode !== "production"throw attimeline-virtualization.mjs:290-293is its own regression harness for this and passed the current CI run, so the assumption is self-verifying at CI time.
— Review by Rames D Jusso
Summary
Studio now ships row and horizontal timeline virtualization by default, so large projects mount only the visible work instead of every timeline element. Setting
VITE_STUDIO_TIMELINE_ROW_VIRTUALIZATION_ENABLED=0remains the single explicit rollback path.The rollout also corrects two CI measurement flaws without changing any performance ceiling:
The existing
75 msinteraction/frame,300 mslong-task, DOM-size, and memory-return budgets remain unchanged.Verification