fix(sticky-headers): report the previous index in onChangeStickyIndex - #2503
Open
dennytosp wants to merge 1 commit into
Open
fix(sticky-headers): report the previous index in onChangeStickyIndex#2503dennytosp wants to merge 1 commit into
dennytosp wants to merge 1 commit into
Conversation
`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)`.
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.
Problem
onChangeStickyIndexis typed and documented as(current: number, previous: number) => void, but under the default configurationpreviousis always-1.RecyclerView.tsxpassed thecurrentStickyIndexstate as the second argument, and that state is only written insideif (stickyHeaderHideRelatedCell).stickyHeaderConfig.hideRelatedCelldefaults tofalse(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) anddocumentation/docs/fundamentals/usage.md:403-411promise the previous index unconditionally, with no mention ofhideRelatedCell.Observed on
main, logging the real callback arguments while scrolling past headers 0, 5 and 10:Fix
Track the last reported index in a ref of its own, so it advances on every sticky change regardless of
hideRelatedCell.currentStickyIndexkeeps 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 forhideRelatedCell: true.With the source fix stashed:
The
hideRelatedCell: truecase passes onmainon purpose — it pins the path that already worked so this cannot regress it.With the fix:
No device testing: this is a pure callback-argument contract change with no rendering effect, fully covered by the existing jest harness.