Skip to content

fix: recompute full layout range on initial scroll adjustment - #2444

Open
theRizwan wants to merge 1 commit into
Shopify:mainfrom
theRizwan:fix/initial-scroll-index-layout-order
Open

fix: recompute full layout range on initial scroll adjustment#2444
theRizwan wants to merge 1 commit into
Shopify:mainfrom
theRizwan:fix/initial-scroll-index-layout-order

Conversation

@theRizwan

Copy link
Copy Markdown

Fixes #2307

The problem

applyInitialScrollAdjustment recomputes 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.

getVisibleLayouts binary searches that array. binarySearchVisibleIndex documents 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}:

seeded      item 250 at 50000    item 251 at 50200
measured    item 250 at 50100    item 251 at 50400
corrected   item 250 at 75000    item 251 at 50400   <- steps backwards

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. _recomputeLayouts has tail repair for partial recomputes, but this call site goes to the public recomputeLayouts and 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 _recomputeLayouts as 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.tsx renders 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.

@theRizwan

Copy link
Copy Markdown
Author

I have signed the CLA!

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.

Flashlist v2 initialScrollIndex renders wrong items when item size exceeds 200

1 participant