Fix SectionList crash when a section keyExtractor is combined with onViewableItemsChanged - #57939
Open
giaBaoJS wants to merge 1 commit into
Open
Fix SectionList crash when a section keyExtractor is combined with onViewableItemsChanged#57939giaBaoJS wants to merge 1 commit into
giaBaoJS wants to merge 1 commit into
Conversation
…actor
VirtualizedSectionList._convertViewable ran every viewable row through the
section's keyExtractor. For section header and footer rows _getItem returns
the section object itself, so a keyExtractor written for items received
{title, data, keyExtractor} and crashed. Use the key _subExtractor already
derived for those rows instead, matching what _keyExtractor does on the
non-viewability path.
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.
Summary:
Fixes #46588.
A
SectionListwhose sections define their ownkeyExtractorrenders fine. AddonViewableItemsChangedand it crashes inside the section'skeyExtractor, because the extractor is handed the section object instead of an item:Why it is gated on
onViewableItemsChangedA section list flattens to
header, ...items, footerrows per section.VirtualizedSectionList._getItemdeliberately returns the section itself for the header and footer rows ("When returning a header or footer item the section itself is the item"), and_subExtractorreportsindex: nullfor them.The two key paths then disagree:
_keyExtractor(the normal path, always active) returnsinfo.key. For a header/footer that iskey + ':header'/key + ':footer', computed from the section, so no key extractor is ever invoked with a section._convertViewable(only reachable whenonViewableItemsChangedis set, via_onViewableItemsChanged) instead calledinfo.section.keyExtractor(viewable.item, info.index)for every viewable row, including headers and footers. So the section object{title, data, keyExtractor}was passed to a key extractor written for items, and any property access on the expected item shape threw.That asymmetry is exactly why the crash only appears once
onViewableItemsChangedis added.The change
In
_convertViewable, branch oninfo.index == null(the header/footer case) and reuse the key_subExtractoralready derived from the section, instead of invoking a key extractor. Item rows keep going through the section'skeyExtractor(or the list-widekeyExtractor/defaultKeyExtractor) exactly as before, and header/footer view tokens are still reported toonViewableItemsChanged— just with a safe key — so viewability reporting is not narrowed.With the index-null case handled up front,
info.indexis a number on the remaining path, so theinfo.index ?? 0fallback and theWithNullableIndex/WithNonNullableIndexnaming are no longer meaningful and were dropped.Changelog:
[GENERAL] [FIXED] - Fix
SectionListcrash when a section defines its ownkeyExtractorandonViewableItemsChangedis setTest Plan:
Added a regression test to the existing
packages/virtualized-lists/Lists/__tests__/VirtualizedSectionList-test.js. It renders a two-section list with a per-sectionkeyExtractorthat reads a nested item field, scrolls, and asserts three things:keyExtractoris only ever given items fromsection.data, never a section;0:header/0:footer/1:header;keyExtractor.Assertion 2 is there on purpose: silently dropping header/footer view tokens would also make the crash go away, and would be a regression.
Counterfactual — with the test in place and only the
VirtualizedSectionList.jschange reverted:Restoring the fix makes it pass.
User-visible path. Before writing the unit test I reproduced the same crash through the public
SectionListcomponent (packages/react-native/Libraries/Lists/SectionList.js) with a scratch test, to confirm the reported symptom rather than only the internal helper. It failed with the identical stack onmain, and after the fix reported:i.e. no crash, headers/footers still reported, items still keyed by the section's
keyExtractor. That scratch test was removed; the committed test covers the same behaviour at theVirtualizedSectionListlevel, where the rest of the section-list tests live.Commands run:
No RNTester change: this restores existing behaviour of the current
SectionList/onViewableItemsChangedexamples rather than adding a user-facing capability, so there is no new surface to demonstrate. Happy to add a focused example if reviewers would like a manual repro in RNTester.