Skip to content

fix(sticky-headers): report the previous index in onChangeStickyIndex - #2503

Open
dennytosp wants to merge 1 commit into
Shopify:mainfrom
dennytosp:fix/sticky-header-previous-index
Open

fix(sticky-headers): report the previous index in onChangeStickyIndex#2503
dennytosp wants to merge 1 commit into
Shopify:mainfrom
dennytosp:fix/sticky-header-previous-index

Conversation

@dennytosp

Copy link
Copy Markdown

Problem

onChangeStickyIndex is typed and documented as (current: number, previous: number) => void, but under the default configuration previous is always -1.

RecyclerView.tsx passed the currentStickyIndex state as the second argument, and that state is only written inside if (stickyHeaderHideRelatedCell). stickyHeaderConfig.hideRelatedCell defaults to false (RecyclerView.tsx:103), so for anyone who has not opted into hiding the related cell the state never leaves its initial -1.

Both the prop's JSDoc (FlashListProps.ts:419-424) and documentation/docs/fundamentals/usage.md:403-411 promise the previous index unconditionally, with no mention of hideRelatedCell.

Observed on main, logging the real callback arguments while scrolling past headers 0, 5 and 10:

[[0,-1],[5,-1],[10,-1]]      // expected [[0,-1],[5,0],[10,5]]

Fix

Track the last reported index in a ref of its own, so it advances on every sticky change regardless of hideRelatedCell. currentStickyIndex keeps its remaining job — hiding the cell behind the stuck header — and drops out of the sticky-header memo's dependency list, since the callback no longer reads it.

Test plan

Two regressions added to src/__tests__/RecyclerView.test.tsx: one for the default config, one for hideRelatedCell: true.

With the source fix stashed:

✕ reports the previously stuck index with the default config
    Received  1: 0, -1
    ->        2: 5, -1     (expected 5, 0)
✓ reports the previously stuck index when hideRelatedCell is enabled

The hideRelatedCell: true case passes on main on purpose — it pins the path that already worked so this cannot regress it.

With the fix:

yarn test        Test Suites: 14 passed, Tests: 189 passed, 189 total
yarn type-check  clean
yarn lint        clean
yarn build       clean

No device testing: this is a pure callback-argument contract change with no rendering effect, fully covered by the existing jest harness.

`onChangeStickyIndex` is documented to receive the current and the
previous sticky header index, but it passed `currentStickyIndex`, a
piece of state that is only ever written inside
`if (stickyHeaderHideRelatedCell)`. `hideRelatedCell` defaults to
`false`, so under the default configuration the state stayed at its
initial `-1` and every invocation reported `previous === -1`:

    [[0, -1], [5, -1], [10, -1]]   // scrolling down past headers 0, 5, 10

Track the last reported index in a ref instead, independently of
`hideRelatedCell`, so both configurations report the real previous
index. `currentStickyIndex` keeps its remaining job of hiding the cell
behind the stuck header and is no longer read by the callback, so it
drops out of the sticky header memo's dependencies.

Adds two regression tests covering the default config and
`hideRelatedCell: true`; the first fails on main with `(5, -1)`.
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