Repository navigation
fix: reuse layout nodes built while scrolling in ListLayout - #10707
Open
romansndlr wants to merge 1 commit into
Open
romansndlr wants to merge 1 commit into
romansndlr wants to merge 1 commit into
Conversation
layoutIfNeeded unions each new visible rect into requestedRect and rebuilds, but only update() set validRect, so every node laid out while scrolling failed isValid on the next rebuild and was laid out again. In TableLayout that re-ran buildRow for every row scrolled past on each frame, so scroll cost grew with scroll distance. Mark the freshly built rect valid, as update() does. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0119dQGPf1YcHgA29G1CfxqP
romansndlr
marked this pull request as ready for review
October 3, 2026 04:57
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #10706
Problem
ListLayout.layoutIfNeededunions each new visible rect intorequestedRectand rebuilds the collection, butvalidRectis only set inupdate(). Every node laid out while scrolling therefore stays outsidevalidRect,isValid()rejects it on the next rebuild, and it is laid out again. InTableLayoutthat meansbuildRowruns for every row from the lastupdate()down to the viewport on each scroll frame, so frame cost grows with scroll distance. Details and a repro are in #10706.Intent
Layout work per scroll frame should depend on the rows coming into view, not on how far the user has scrolled. Which rows get laid out, and where, should not change.
How
After
layoutIfNeededrebuilds for a new rect, setvalidRecttorequestedRect, the same thingupdate()does at the end of a rebuild. The nodes it just built are current, so later rebuilds can reuse them.isValidstill requires the same node and the same offset, so a row whose node or position changed is laid out again as before.update()andupdateItemSizekeep managingvalidRectas they do today.Measurements
The #10706 repro: 5000 rows, production build, headless Chromium 145, 4× CPU throttling, 120px per frame. Average frame time per range of rows, median of 3 runs:
On a variant of the repro with
estimatedRowHeightand rows of six different measured heights (18–92px), I recorded every rendered row's position and cell height at 66 checkpoints while scrolling 2000 rows down and back up. Before and after were identical at every checkpoint, and so was the total scroll height.✅ Pull Request Checklist:
📝 Test Instructions:
yarn jest packages/react-aria-components/test/Table.test.js -t "only lay out rows that come into view". It passes with this change and fails without it.buildRowtime grows with scroll distance; after it, frame times stay flat.What I tested:
yarn test: 384 suites, 8439 passed, 16 skippedyarn test:ssr: 60 suites, 74 passedyarn lint: passesyarn test:browser: not run to completion yetscrollTopsteps in headless Chromium, with fixed and measured row heights, LTR only🧢 Your Project:
groundcover