Make SearchSingleSelectionPicker use new SelectionListWithSections#81001
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@dukenv0307 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| }, | ||
| [shouldAutoSave, backToRoute, onSaveSelection], | ||
| ); | ||
| const initiallySelectedItemSection = initiallySelectedItem?.name.toLowerCase().includes(debouncedSearchTerm?.toLowerCase()) |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The sections computation is expensive (filtering, sorting, mapping on every render) and should be memoized with useMemo. Without memoization, the entire items array is processed on every render, which can cause performance issues with large lists.
Suggested fix:
const {sections, noResultsFound} = useMemo(() => {
const initiallySelectedItemSection = initiallySelectedItem?.name.toLowerCase().includes(debouncedSearchTerm?.toLowerCase())
? [
{
text: initiallySelectedItem.name,
keyForList: initiallySelectedItem.value,
isSelected: selectedItem?.value === initiallySelectedItem.value,
value: initiallySelectedItem.value,
},
]
: [];
const remainingItemsSection = items
.filter((item) => item.value !== initiallySelectedItem?.value && item.name.toLowerCase().includes(debouncedSearchTerm?.toLowerCase()))
.sort((a, b) => sortOptionsWithEmptyValue(a.name.toString(), b.name.toString(), localeCompare))
.map((item) => ({
text: item.name,
keyForList: item.value,
isSelected: selectedItem?.value === item.value,
value: item.value,
}));
const noResultsFound = !initiallySelectedItemSection.length && !remainingItemsSection.length;
const sections = noResultsFound
? []
: [
{
title: undefined,
data: initiallySelectedItemSection,
sectionIndex: 0,
},
{
title: pickerTitle,
data: remainingItemsSection,
sectionIndex: 1,
},
];
return {sections, noResultsFound};
}, [initiallySelectedItem, selectedItem?.value, items, pickerTitle, debouncedSearchTerm, localeCompare]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
AFAIK this component can be compiled by React Compiler so the useMemo is not needed
| sectionIndex: 1, | ||
| }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
❌ PERF-6 (docs)
The onSelectItem function should be wrapped in useCallback to prevent unnecessary re-renders of the SelectionList component. Without memoization, a new function reference is created on every render, causing SelectionList to re-render even when props haven't meaningfully changed.
Suggested fix:
const onSelectItem = useCallback(
(item: Partial<OptionData & SearchSingleSelectionPickerItem>) => {
if (\!item.text || \!item.keyForList || \!item.value) {
return;
}
if (shouldAutoSave) {
onSaveSelection(item.isSelected ? '' : item.value);
Navigation.goBack(backToRoute ?? ROUTES.SEARCH_ADVANCED_FILTERS.getRoute());
return;
}
if (\!item.isSelected) {
setSelectedItem({name: item.text, value: item.value});
}
},
[shouldAutoSave, backToRoute, onSaveSelection],
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
AFAIK this component can be compiled by React Compiler so the useMemo is not needed
| setSelectedItem({name: item.text, value: item.value}); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
❌ PERF-6 (docs)
The resetChanges function should be wrapped in useCallback to prevent unnecessary re-renders of child components. Without memoization, a new function reference is created on every render, causing the SearchFilterPageFooterButtons component to re-render unnecessarily.
Suggested fix:
const resetChanges = useCallback(() => {
setSelectedItem(undefined);
}, []);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
AFAIK this component can be compiled by React Compiler so the useMemo is not needed
| setSelectedItem(undefined); | ||
| }, []); | ||
| }; | ||
|
|
There was a problem hiding this comment.
❌ PERF-6 (docs)
The applyChanges function should be wrapped in useCallback to prevent unnecessary re-renders of child components. Without memoization, a new function reference is created on every render, causing the SearchFilterPageFooterButtons component to re-render unnecessarily.
Suggested fix:
const applyChanges = useCallback(() => {
onSaveSelection(selectedItem?.value);
Navigation.goBack(backToRoute ?? ROUTES.SEARCH_ADVANCED_FILTERS.getRoute());
}, [onSaveSelection, selectedItem?.value, backToRoute]);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
AFAIK this component can be compiled by React Compiler so the useMemo is not needed
| ), | ||
| [resetChanges, applyChanges], | ||
| }; | ||
|
|
There was a problem hiding this comment.
❌ PERF-6 (docs)
The footerContent JSX should be wrapped in useMemo to prevent unnecessary re-renders. Without memoization, a new JSX element is created on every render, causing React to treat it as a new prop value even when the underlying functions (applyChanges and resetChanges) haven't changed.
Suggested fix:
const footerContent = useMemo(
() => (
<SearchFilterPageFooterButtons
applyChanges={applyChanges}
resetChanges={resetChanges}
/>
),
[resetChanges, applyChanges],
);Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
AFAIK this component can be compiled by React Compiler so the useMemo is not needed
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-02-01.at.11.53.37.movAndroid: mWeb ChromeScreen.Recording.2026-02-01.at.11.50.40.moviOS: HybridAppScreen.Recording.2026-02-01.at.11.53.37.moviOS: mWeb SafariScreen.Recording.2026-02-01.at.11.48.50.movMacOS: Chrome / SafariScreen.Recording.2026-02-01.at.11.48.03.mov |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/grgia in version: 9.3.11-19 🚀
|
|
This PR failing because of the issue |
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.3.12-1 🚀
|
| shouldShowTooltips | ||
| ListItem={SingleSelectListItem} | ||
| shouldUpdateFocusedIndex | ||
| shouldStopPropagation |
There was a problem hiding this comment.
This migration from the old SectionList-based SelectionListWithSections to the new FlashList-based one missed adding disableMaintainingScrollPosition. The new BaseSelectionListWithSections defaults disableMaintainingScrollPosition to false, which means FlashList's maintainVisibleContentPosition is enabled. It caused this bug where searching for a term (e.g. "Travel") filters the list to a few items, and then clearing the search doesn't show the full list because FlashList tries to maintain the scroll position from the filtered view. Fixed in #81294.
6 participants
Explanation of Change
Fixed Issues
$ #65658
PROPOSAL:
Tests
Add merchant ruleCategory,TagorTaxOffline tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
Screen.Recording.2026-01-30.at.14.54.45.mov