Skip to content

fix(sticky-headers): guard binary search against unmeasured layouts - #2507

Open
priyanshu-cashbook wants to merge 2 commits into
Shopify:mainfrom
priyanshu-cashbook:fix/sticky-header-unmeasured-layout
Open

priyanshu-cashbook wants to merge 2 commits into
Shopify:mainfrom
priyanshu-cashbook:fix/sticky-header-unmeasured-layout

Conversation

@priyanshu-cashbook

Copy link
Copy Markdown

Description

StickyHeaders.compute() throws index out of bounds, not enough layouts out of the scroll handler, taking the tree down. Seen in production on 2.0.2 across iOS and Android — 45 users in four days on a single screen, all of them a paginating list with stickyHeaderIndices.

The window

compute() bails out on a data-prop length but then reads positions from the layout table:

const lengthInvalid =
  sortedIndices.length === 0 ||
  recyclerViewManager.getDataLength() <= sortedIndices[sortedIndices.length - 1];
  • getDataLength() is props.data.length, set by updateProps() during render.
  • getLayout() throws on index >= this.layouts.length, and layouts only grows in modifyLayout(), reached from modifyChildrenLayout() after the children have laid out.

So when a page is appended, data and stickyHeaderIndices update together in one render and lengthInvalid is satisfied for the new trailing header, but layouts is still the previous length for at least one more commit. onScrollHandler calls stickyHeaderRef.current?.reportScrollEvent() → compute(), and any scroll event landing in that gap throws.

It needs a list that both paginates and sets stickyHeaderIndices, which is why it is not constant. Shrinks are safe (the table is truncated on the first line of modifyLayout, so it is never shorter than the data); only growth exposes it.

Fix

Read through tryGetLayout() and treat a missing layout as below the viewport.

The two reads immediately below the binary search already do exactly this, and #2460 assumes this file is already safe on that basis ("as StickyHeaders, the measurement effect and the public getLayout ref already do") — the getY callback is the one read in here that was still unguarded. That PR covers ViewHolderCollection and validateItemSize on the shrink path, so the two are complementary rather than overlapping; this one needs no coordination with it.

Number.MAX_SAFE_INTEGER is also the correct answer and not just a non-throwing one: what goes unmeasured is the freshly appended tail, which really is below everything measured, so the last measured header stays selected instead of one that may not be on screen yet. The guard is inert whenever the index is valid, so no list that is not already in this state changes behaviour.

Reviewers' hat-rack 🎩

  • src/__tests__/StickyHeaders.test.tsx — two cases added, driving the real component through a manager whose getLayout throws past measuredCount and whose tryGetLayout returns undefined, matching RVLayoutManager / RecyclerViewManager. Both fail on main with the exact production message and pass with the fix; the 19 existing cases in the file are untouched by it.
  • Worth a second opinion on the sentinel: bailing out of compute() early instead was the other option, but lengthInvalid is derived from data rather than layouts, so nothing would re-run compute() once the layouts caught up and the header could stay stale until the next scroll event.
  • yarn test --forceExit 189 passed / 14 suites, yarn type-check and yarn lint clean.

Related: #2440, #2291, #2460.

`compute()` bails out on `getDataLength()`, which is `props.data.length` and
grows during render, then reads positions through `getLayout()`, which is
backed by `layoutManager.layouts` and only grows in `modifyChildrenLayout()`
once the children have laid out. A scroll event arriving between those two
commits passes the `lengthInvalid` check with a sticky index the layout table
has not reached, and `getLayout` throws "index out of bounds, not enough
layouts" out of `onScroll`, taking the tree down.

Read through `tryGetLayout()` — as the two reads directly below already do —
and treat a missing layout as below the viewport. The newly appended tail is
what goes unmeasured, so that is also the correct answer, not just a safe one:
the last measured header stays selected instead of one that may not be on
screen yet.
@priyanshu-cashbook

Copy link
Copy Markdown
Author

I have signed the CLA

This branch has not been deployed

No deployments
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.

1 participant