Conversation
StickyHeaders.compute() validated sticky indices against the data array length, but the binary search that follows indexes the layout array via getLayout(index), which throws "index out of bounds, not enough layouts" when the index is beyond the layouts computed so far. The two arrays are updated in separate passes: getDataLength() reads props on every render while layouts only grow in processDataUpdate(), which is gated on hasLayout(). When data grows while no layout manager exists, a scroll event landing in that window crashed the scroll handler. compute() now skips the frame when the last sticky index has no layout yet (a layout for the last index guarantees one for every other sticky index) and the binary search reads positions through tryGetLayout, matching the other layout reads in the same function. Fixes Shopify#2509
Author
|
I have signed the CLA! |
This branch has not been deployed
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
1 participant
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.
Root cause: the early-return guard in
StickyHeaders.compute()validated sticky indices against the data length, but the binary search then indexed the layout array through the throwinggetLayout(). Layouts only grow inprocessDataUpdate()(gated onhasLayout()), so on the frame right after a data change the arrays legitimately diverge andgetLayoutthrowsindex out of bounds, not enough layouts.Two changes:
tryGetLayout(sortedIndices[sortedIndices.length - 1])undefined). A layout for the last index guarantees one for every lower sticky index since layouts are a dense array, so this one check covers the whole lag window.tryGetLayout(index)?.y ?? 0, matching the three sibling reads already in the same function.In steady state
layouts.length >= dataLength, so the guard is a no-op on the normal path.Tests: new regression test
should skip compute without throwing when data length exceeds layout count— against the unfixed code it fails with the exact production error from the issue; with the fix, mount and scroll during the lag window skip cleanly and compute resumes once layouts catch up. The mock factory gained alayoutCountknob that mirrors the realLayoutManagercontract (getLayoutthrows,tryGetLayoutreturns undefined).StickyHeaders20/20,RecyclerView19/19, full unit suite 15 suites 200/200,tsc --noEmitand prettier clean.Fixes #2509