fix(react-virtual): skip flushSync while measuring from the ref callback - #1282
fix(react-virtual): skip flushSync while measuring from the ref callback#1282Jigoooo wants to merge 1 commit into
Conversation
`measureElement` is passed to item elements as a ref, so React calls it during the commit phase. A measured size that differs from `estimateSize` makes `resizeItem` compensate scroll and raise a sync notify, and the adapter then called `flushSync(rerender)` while React was still committing — React runs the callback but cannot flush, and warns in development. React sets the update priority to discrete inside `flushSync` and the commit phase already runs at discrete priority, so for that window `flushSync` and a plain `rerender()` produce the same lane and the same flush point. Track the measure-from-ref window and skip `flushSync` only there; notifies from every other path (ResizeObserver re-measures, scroll adjustments) still flush synchronously, which `useFlushSync: false` would have given up.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe React adapter now skips ChangesReact virtualizer flush handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~15 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The commit-phase warning is addressed without an established regression to other measurement or scrolling behavior. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
packages/react-virtual/tests/index.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 |
1 participant
🎯 Changes
Fixes #1281.
Problem
measureElementis handed to item elements as a ref, so React calls it during the commit phase. When the measured size differs fromestimateSizefor an item above the current scroll offset,resizeItemcompensates the scroll position and raises a sync notify — and this adapter then callsflushSync(rerender)while React is still committing. React runs the callback but cannot flush, so development builds logonce per measured item (21 on first mount and 11 per list switch in the app measured below). Previously reported in #628 and #1094.
Fix
Mark the window in which the virtualizer measures from its ref callback, and skip
flushSynconly for notifies raised inside it:instance.measureElementonce, where the instance is created, to set and clearmeasuringFromRefonChangekeepsflushSyncfor every other sync notify —ResizeObserverre-measures and scroll adjustments arrive outside the commit phase and still flush synchronouslyWhy this keeps behaviour identical. React's
flushSyncsetsInternals.p = 2(DiscreteEventPriority) and runs the callback; while committing it only skips the flush (Internals.d.f()→flushSyncWork()returnstrue, which is what produces the warning). The commit phase itself already runs atDiscreteEventPriority, so an update scheduled from there lands in the same lane and is flushed at the same point with or withoutflushSync.useFlushSync: false, the workaround suggested on the earlier issues, is broader: it also drops theResizeObserverpath to default priority, so rows that grow after mount (late-loading images) settle a frame later.Test verification (RED -> GREEN)
New test in
packages/react-virtual/tests/index.test.tsx. The existingListhelper gained aninitialOffsetprop so that measured items sit above the fold:console.errorreceives theflushSyncwarning 3 times.packages/react-virtualpasses 8/8 (7 existing + the new one).Browser verification in a real app
React 19.2.8,
@tanstack/react-virtual3.14.12 +virtual-core3.17.10, headless Chromium. One virtualized chat timeline mounted in isolation (80 rows,anchorTo: 'end',estimateSize56 vs real 90–300), three phases: first mount → switch to another list → switch back. Three runs of each build; runs of the same build were frame-identical, so the comparison is not noise.flushSyncwarningsReact.Profiler)scrollTop+ rowtransform, 40 frames per phaseThe same change (applied to the published build with
pnpm patch) is running in our application.✅ Checklist
Local run on Node 24.8.0 (
.nvmrc) with pnpm 11.9.0 (packageManager):NX_BASE=<TanStack/virtual main> NX_HEAD=HEAD CI=1 pnpm run test:pr --parallel=2 --skip-nx-cache— every target passed except@tanstack/react-virtual:test:e2e, which could not launch a browser on this machine (Executable doesn't exist … chromium_headless_shell-1194).After
pnpm exec playwright install chromium,nx run @tanstack/react-virtual:test:e2e --skip-nx-cachepasses: 36 passed.nx run-many --targets=test:eslint,test:lib,test:types,test:build,build --parallel=2 --skip-nx-cache— 68 projects, all successful.Prettier passes for the changed TypeScript files and the changeset.
🚀 Release Impact
@tanstack/react-virtual).Summary by CodeRabbit