Repository navigation
Conversation
Author
|
I have signed the CLA! |
pupaxxo
pushed a commit
to one-am-it/react-native-simple-image-slider
that referenced
this pull request
Sep 8, 2026
… photo The mispark is upstream, not ours: Shopify/flash-list#2307, open and P1. `applyInitialScrollAdjustment` recomputes layouts only as far as the opening index, which leaves the layout array out of order at that boundary — and the binary search in `getVisibleLayouts` assumes it is sorted, so it resolves an unrelated item. It needs items larger than the 200pt default estimate to bite, which a full-width slide always is, and it arrived in 2.3.1 with the reorder that reads the target offset after recomputing rather than before. Measured here on the example's gallery, twenty-two slides at 420pt opening on index 14: the scroll is *correct* — `offset 5880`, and `getLayout(14).x` is also 5880 — but the content is 6380 against a true 9240 and the item resolved under that offset is 21. Every signal from outside reads healthy, which is why this looked like a timing problem for so long. This carries the one line from the upstream fix, Shopify/flash-list#2444: recomputeLayouts(0, initialScrollIndex) → recomputeLayouts(0, this.getDataLength() - 1) Nothing in `src/` changes. The slider stays exactly as published, and the patch goes when a release carries the fix. The version is also locked now. `example/package.json` asked for 2.3.2 while the lockfile still resolved 2.0.2, so a clean install produced the version where the defect does not exist — the reproduction would have quietly stopped reproducing. The root devDependency moves to 2.3.2 as well, so the library is typechecked against the version its consumers actually get.
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
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.