fix: recompute full layout range on initial scroll adjustment - #2444
Open
theRizwan wants to merge 1 commit into
Open
fix: recompute full layout range on initial scroll adjustment#2444theRizwan wants to merge 1 commit into
theRizwan wants to merge 1 commit into
Conversation
Author
|
I have signed the CLA! |
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.
Fixes #2307
The problem
applyInitialScrollAdjustmentrecomputes layouts for[0, initialScrollIndex]and stops there. Items are positioned from their predecessor, so this moves the target item without moving anything after it, and the layout array is left out of order at that boundary.getVisibleLayoutsbinary searches that array.binarySearchVisibleIndexdocuments that it assumes the array is sorted, so once it is not, the search does not return a slightly wrong answer, it returns an unrelated one.With 600 items of 300px and
initialScrollIndex={250}:The search for offset 75000 then returns item 333, which matches what the issue reports on device.
The gap only opens when items are taller than the 200px the average window starts at. Below that the corrective pass moves items to a lower position, which leaves a forward gap and keeps the array sorted, which is why smaller items never show the bug.
The change
Recompute to the end of the list instead of stopping at
initialScrollIndex.This is the same range the layout managers already use when window size changes, and Masonry already recomputes to the end regardless of the range it is given.
_recomputeLayoutshas tail repair for partial recomputes, but this call site goes to the publicrecomputeLayoutsand bypasses it, and its condition compares the last item rather than the boundary item so it would not fire here anyway.I kept the change at the call site because the ordering only breaks when the prefix is corrected against a changed average, and positions chain, so anything after the target genuinely has to be recomputed. Happy to move it into
_recomputeLayoutsas a boundary check instead if you would rather have it there, though that path is shared with Masonry where offsets are not ordered by index by design.Test
src/__tests__/initialScrollIndex2307.test.tsxrenders the case from the issue with items measuring 300px and asserts item 250 is rendered and item 333 is not. It fails on main and passes with this change.Full suite is green, 188 tests across 15 suites, and type check passes.