Skip to content

test(studio): drive the thumbnail-lease probe through props, not a captured let - #3775

Draft
miguel-heygen wants to merge 1 commit into
refactor/studio-c1-compiler-bailout-ratchetfrom
refactor/studio-c4-compiler-latent-tests
Draft

test(studio): drive the thumbnail-lease probe through props, not a captured let#3775
miguel-heygen wants to merge 1 commit into
refactor/studio-c1-compiler-bailout-ratchetfrom
refactor/studio-c4-compiler-latent-tests

Conversation

@miguel-heygen

Copy link
Copy Markdown
Collaborator

Lands unit C4 (latent test files) of the Studio React Compiler coverage plan. Stacked on the ratchet PR (#3773). The exhaustive scan found one remaining case, not the forty-seven the first estimate gave; the scan rule and the widening passes are recorded below.

What

Unit C4 of the React Compiler coverage plan: the Studio test files that drive a re-render by mutating a captured let binding. An exhaustive AST scan of every test file under packages/studio/src found one remaining instance, in src/hooks/useThumbnailLease.test.tsx, and this PR fixes it. Tests only; no production file changes.

Files changed:

File Case Binding Fix
packages/studio/src/hooks/useThumbnailLease.test.tsx "updates priority without restarting the active request" priority passed as a prop; assertion added so the change is observable

The test file is not imported by any module under src (verified by grep), so nothing that ships changes.

Why

The React Compiler treats a value read from an enclosing closure as non-reactive. A probe component that reads a let the test reassigns between renders therefore keeps the memoized value from the first render: the second render produces the same object, and whatever the hook was supposed to observe never changes. The test then measures nothing, and the failure only surfaces later, as a mystery, when someone touches the code under test.

useThumbnailLease.test.tsx had two such cases. The earlier compiler PR fixed one (kind / rich) because it failed loudly. The second (priority) survived because its only assertion, expect(load).toHaveBeenCalledTimes(1), is true whether or not the new priority ever reaches the scheduler. That is the worse of the two shapes: a green test observing nothing.

How

priority is a prop:

function Probe({ priority }: { priority: ThumbnailRequest["priority"] }) { ... }
root.render(React.createElement(Probe, { priority: "overscan" }));
root.render(React.createElement(Probe, { priority: "interaction" }));

The scheduler keeps the priority per lease and exposes no getter, so the test wraps scheduler.acquire to record the updatePriority calls, and asserts ["interaction"]. Without that assertion the fix would be untestable, and the test would stay vacuous in the other direction.

How the set was found

A one-off TypeScript-AST scan over all 420 test files under packages/studio/src (139 .test.tsx, 281 .test.ts). Rule:

Flag a let binding that is (1) declared in a scope enclosing a component or hook body, (2) read inside that body, and (3) reassigned somewhere in the binding's own scope but outside that body.

A "component or hook body" is a function referenced as a JSX tag or as the first argument of createElement, an inline function passed to createElement or renderHook, or, transitively, any locally declared function called from one of those.

Result: 1 hit, 1 file (the priority case above).

Three widening passes were run so the negative is a search and not a sample, and every candidate they produced was read by hand:

  • Ignore lexical scope entirely (name-level matching): 40 files touch a mutable let from a probe, 3 of which have a read-in-body plus an assignment-outside. Two are false positives of the loose rule and were confirmed by reading them: useElementLifecycleOps.test.tsx and useSlideshowTabState.test.ts both use the output-capture idiom (let current; function Harness(){ current = useX(...) }), which writes from inside the component and is not affected by memoization of an input.
  • vi.mock factories that read a reassigned let (a read that also happens during render): 3 files. useGsapAwareEditing.test.tsx is a parameter name, not a capture. feedbackTrigger.test.ts renders nothing. useRenderQueueFfmpegGate.test.tsx reassigns before a fresh mount, never between renders of one tree, and mocks a hook, which the compiler re-runs every render anyway.
  • Transitive helper calls from a component body, so a let read through a locally defined helper still counts. No additional hits.

The plan's estimate for this unit was about 47 files. That figure matches the shape of the loosest pass (40 files whose probes touch a mutable let), which is dominated by the harmless output-capture idiom. The count of files carrying the actual hazard, after the earlier compiler PR, is 1.

Proof the pattern is real, not theoretical

Restoring the pre-fix version of the other case in the same file (kind / rich) and running it on this branch, with no other change:

FAIL  src/hooks/useThumbnailLease.test.tsx > resubscribes when the request work shape changes
AssertionError: expected "vi.fn()" to be called 1 times, but got 0 times

Worth recording because the brief expected this to require locally removing the hook's own compiler bail-out first: it does not. The hook still bails out ("cannot access refs during render") and the test fails anyway, because the compiled function that matters is Probe in the test file - test files go through the same transform. A production file's bail-out status has no bearing on whether its test is armed.

And the new assertion is not vacuous: putting priority back on a captured let, keeping the assertion, fails.

AssertionError: expected [] to deeply equal [ 'interaction' ]

Test plan

Run from packages/studio unless noted. No foreign Studio dev server was listening; an unrelated Storybook dev server from another worktree was, and no suite depends on it.

  • bunx vitest run --maxWorkers=4: 428 passed | 1 skipped (429) files, 4753 passed | 18 todo (4771) tests. Identical to the base branch: no test was added, one existing test gained an assertion.
  • bunx vitest run src/styles/compilerBailouts.test.ts --maxWorkers=4: 10 passed. Baseline total unchanged at 126 before and after, and correctly so: the scan skips *.test.tsx? (isScanned in scripts/compiler-bailouts.mjs), so a test-only unit cannot move it.
  • bun run typecheck: exit 0.
  • bun run build (repo root, before the change, to make the workspace resolvable): exit 0.
  • bunx oxlint and bunx oxfmt --check on the changed file: 0 warnings, 0 errors; correctly formatted.
  • bunx fallow audit --base origin/main --fail-on-issues (repo root): exit 0. The duplication warnings it prints are inherited findings in a file this PR does not touch.
  • Studio load smoke, the CI job's own command (node scripts/studio-runtime-smoke.mjs against a dev server): PASS: studio loaded with schema-valid API fixtures and no runtime errors.
  • Studio timeline viewport gate, both arms, the CI job's own commands and fixture: both exit 0. Default arm (virtualization on, 50k elements): interaction p95 32.2 ms, frame interval p95 16.8 ms, 5/5 runs passing, memory returned. Disabled arm (virtualization off, 1k elements): interaction p95 32.7 ms, frame interval p95 16.7 ms, 5/5 runs passing, memory returned.

Not covered

  • The render-parity capture (design:shots) was not run. The diff is one *.test.tsx file that no module under src imports, so it contributes nothing to a render. Running the capture would compare a build against itself.
  • No guard stops the pattern coming back. The bail-out ratchet deliberately skips test files, so a new test with a captured-let probe is invisible to every check in the repo. The scan used here was a one-off; turning it into a standing check is a separate change and is proposed rather than smuggled in here.
  • Only let bindings. A probe that reads a mutable const object and has its properties reassigned between renders is the same hazard, and is out of this unit's scope. It was not scanned.
  • Other units' folders were not touched, per the unit split.

The priority case still re-rendered by reassigning a `let` the probe closed
over. Under the React Compiler that value is not reactive, so the memoized
request object is reused across both renders and the new priority never
reaches the lease. The old assertion only counted `load` calls, which is true
either way, so the test passed while observing nothing.

Priority is a prop now, and the test asserts the lease actually received the
update by wrapping the scheduler's acquire. An exhaustive AST scan of all 420
test files under src finds no other instance of the pattern.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant