Shopify / Shopify/flash-list

StickyHeaders.compute() throws "index out of bounds, not enough layouts" — its guard checks getDataLength() but the binary search indexes the layout array

Open Beginner friendly
#2,509 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
TypeScript
Stars
7.2k
Forks
392
Avg merge
1d 3h
Merged PRs (30d)
1

Description

Description

StickyHeaders.compute() can throw index out of bounds, not enough layouts from the scroll handler, taking down the tree. Its early-return guard validates the sticky indices against the data array, but the binary search immediately after indexes into the layout array through the unguarded getLayout(index).

Same error and same getLayout family as #2291 and #2440, different call site again — those are the validateItemSize measurement callback and the ViewHolderCollection render path; this one is reached from reportScrollEvent on every scroll tick.

Current behavior

The guard checks getDataLength():

const lengthInvalid =
  sortedIndices.length === 0 ||
  recyclerViewManager.getDataLength() <=
    sortedIndices[sortedIndices.length - 1];

https://github.com/Shopify/flash-list/blob/435b5141df4960ebbbc0b93264db3f5a4b23fffd/src/recyclerview/components/StickyHeaders.tsx#L93-L96

…and then the comparator reads layouts:

const currentIndexInArray = findCurrentStickyIndex(
  sortedIndices,
  adjustedScrollOffset + stickyHeaderOffset,
  (index) => recyclerViewManager.getLayout(index).y
);

https://github.com/Shopify/flash-list/blob/435b5141df4960ebbbc0b93264db3f5a4b23fffd/src/recyclerview/components/StickyHeaders.tsx#L105-L109

Those two arrays are sized in separate passes:

source when it updates
getDataLength() propsRef.data?.lengthRecyclerViewManager.ts#L346 every render via updatePropsuseRecyclerViewManager.ts#L16
layouts.length LayoutManager.layouts only in processDataUpdate(), wrapped in if (this.hasLayout())RecyclerViewManager.ts#L302-L303

So when data grows while layoutManager is still undefined, getDataLength() already reports the new length, lengthInvalid passes, and a scroll event landing in that window calls LayoutManager.getLayout(index) with index >= this.layouts.length, which throws by design:

https://github.com/Shopify/flash-list/blob/435b5141df4960ebbbc0b93264db3f5a4b23fffd/src/recyclerview/layout-managers/LayoutManager.ts#L230-L233

The guard is effectively checking a different array from the one it protects.

Expected behavior

A sticky index without a layout yet should be skipped or treated as unresolved, not throw. tryGetLayout() already exists for exactly this and returns undefined out of range:

https://github.com/Shopify/flash-list/blob/435b5141df4960ebbbc0b93264db3f5a4b23fffd/src/recyclerview/RecyclerViewManager.ts#L145-L153

Worth noting this looks like an oversight rather than intent: the same compute() reads layouts twice more and both use the safe accessortryGetLayout(newNextStickyIndex)?.y ?? 0 and tryGetLayout(newStickyIndex)?.height ?? 0, plus another at L173. Only the binary-search comparator uses the throwing variant, and it's the one on the hot scroll path.

Suggested fix

Two small changes that reinforce each other:

  1. Extend the early return to also require a layout for the highest sticky index, so compute() skips the frame rather than searching a layout array that isn't sized yet:
if (!recyclerViewManager.tryGetLayout(sortedIndices[sortedIndices.length - 1])) {
  return;
}
  1. Move the comparator onto the nullable accessor, matching its siblings:
-      (index) => recyclerViewManager.getLayout(index).y
+      (index) => recyclerViewManager.tryGetLayout(index)?.y ?? 0

With (1) in place, (2) is defence-in-depth; on its own a missing layout reads as y = 0 ("already scrolled past"), which can pick the wrong sticky header for one frame and self-corrects on the next compute() once layouts settle. Either is strictly preferable to throwing.

Happy to open a PR if you'd like it in that form.

Reproduction

I don't have a Snack for this one, and I want to be upfront about why rather than pad the issue: it's a timing window between a data commit and the layout pass, observed in production rather than constructed. It needs sticky headers armed on a list whose data grows while hasLayout() is still false, with a scroll event landing inside that window — reliable at scale, fiddly to force deterministically in a small app.

What I can offer instead is the exact mechanism above, which is source-level and checkable by reading: the guard reads getDataLength(), the very next call indexes layouts, and the two are written in different passes with only one of them gated on hasLayout(). The sibling reads in the same function already use tryGetLayout, so the fix is self-consistent with the file.

A unit test would be the honest repro here — drive compute() with a recyclerViewManager whose getDataLength() exceeds its layout count and assert it doesn't throw. Glad to write that if it would help move this along.

Platform

  • iOS
  • Android
  • Web (if applicable)

Not platform-specific — it's in the shared JS. Observed on both; the crash sample below is iOS.

Environment

  • @shopify/flash-list: 2.3.2 (latest published; bug is also present on main at the lines linked above, commit 435b5141df)
  • react-native: 0.86.3
  • expo: 57.0.20
  • Architecture: Fabric / new architecture, Hermes
  • Scale: ~70 occurrences across ~50 users in 90 days, from the sticky-header scroll path specifically

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start in src/recyclerview/components/StickyHeaders.tsx at StickyHeaders.compute(), then compare its layout reads with tryGetLayout() in the same function. Check RecyclerViewManager.ts for the data and layout accessors. Add coverage using a manager whose data length exceeds its layout count, and verify compute() does not throw during that unresolved-layout window.

Written by the indexing model from the issue text.

Assessment

Tech stack
react-native, typescript
Domain
mobile
Issue type
Bug
Difficulty
2/5
Estimated time
1-3 hours
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.