Refactor provided component to use useSearchSelector hook and remove unused and duplicated code (Task 5)#71450
Conversation
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
melvin-bot
bot
requested review from
linhvovan29546
and removed request for
a team
|
@linhvovan29546 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] |
|
can I be added as additional reviewer? @lorretheboy @pecanoro @linhvovan29546 |
| useOptions(); | ||
| const headerMessage = useMemo(() => { | ||
| return getHeaderMessage((availableOptions.recentReports?.length || 0) + (availableOptions.personalDetails?.length || 0) !== 0, !!availableOptions.userToInvite, searchTerm); | ||
| }, [availableOptions.recentReports, availableOptions.personalDetails, availableOptions.userToInvite, searchTerm]); |
There was a problem hiding this comment.
Is there a reason we're not including the .length in the dependencies, for example: [availableOptions.recentReports?.length, availableOptions.personalDetails?.length, availableOptions.userToInvite, searchTerm]?
There was a problem hiding this comment.
Hmm I rarely see this pattern in our codebase, however, I agree that we can update to add .length for deps. Lemme update it
There was a problem hiding this comment.
if we care only about length then adding .length in dependencies is good choice
src/pages/settings/Profile/CustomStatus/VacationDelegatePage.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Profile/CustomStatus/VacationDelegatePage.tsx
Outdated
Show resolved
Hide resolved
| shouldInitialize: true, | ||
| getValidOptionsConfig: { | ||
| excludeLogins, | ||
| maxRecentReportsToShow: CONST.IOU.MAX_RECENT_REPORTS_TO_SHOW, |
There was a problem hiding this comment.
We had this configuration in the old change, but I don't see it in the new changes. I'm curious how we are handling this option now, as I can't locate it in the useSearchSelecter.
There was a problem hiding this comment.
@lorretheboy maxRecentReportElements is still missing.
| chatOptions.selfDMChat, | ||
| chatOptions?.userToInvite, | ||
| chatOptions.workspaceChats, | ||
| availableOptions.personalDetails, |
There was a problem hiding this comment.
I know this isn't related to the new changes, but I think we should update the dependencies to use .length, since getHeaderMessage uses the .length.
cc @sosek108 What are your thoughts on this?
There was a problem hiding this comment.
Hmm in this case, if we use .length, it would show a warning since since we access the whole object inside useMemo. However, technically, I think we could use .length for this case and plus a eslint disabling rule comment. Pls lemme know if we want to update to this new way
There was a problem hiding this comment.
If content is important and not only .length then we should not add .length.
and I think that we use only length for availableOptions.*
|
|
||
| const {searchTerm, setSearchTerm, availableOptions, selectedOptions, selectedOptionsForDisplay, toggleSelection, areOptionsInitialized, onListEndReached, contactState} = | ||
| useSearchSelector({ | ||
| selectionMode: isIOUSplit ? CONST.SEARCH_SELECTOR.SELECTION_MODE_MULTI : CONST.SEARCH_SELECTOR.SELECTION_MODE_SINGLE, |
There was a problem hiding this comment.
@lorretheboy Did you test this in multi selection mode? I’m unable to test it on the dev env because the web app crashes for me when I select an option.
There was a problem hiding this comment.
Let's me check it again
There was a problem hiding this comment.
Hi @linhvovan29546, I found the issue and have fixed it. But to be able to test, I have to set a hard code multiple selection mode in codebase. Could you please share me the steps to access that flow?
There was a problem hiding this comment.
You can follow the steps in the video below, just update the URL to access the split page. This causes the issue where the selected checkbox is displayed incorrectly. You can compare the behavior with production or staging.
Screen.Recording.2025-10-02.at.14.47.42.mov
There was a problem hiding this comment.
Thanks. I have fixed this issue @linhvovan29546
|
@lorretheboy This PR (#71557) has been merged. Could you please address this comment #71450 (comment)? |
|
There’s a conflict again! |
|
@linhvovan29546 I have resolved the conflicts |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2025-11-03.at.10.50.35.movAndroid: mWeb ChromeScreen.Recording.2025-11-03.at.10.52.09.moviOS: HybridAppScreen.Recording.2025-11-03.at.10.29.51.moviOS: mWeb SafariScreen.Recording.2025-11-03.at.10.38.18.movMacOS: Chrome / SafariScreen.Recording.2025-11-03.at.10.13.16.movMacOS: DesktopScreen.Recording.2025-11-03.at.10.57.21.mov |
|
@linhvovan29546 done now. My bad |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| enablePhoneContacts: isNative, | ||
| contactOptions: contacts, | ||
| initialSelected: participants as OptionData[], | ||
| onSelectionChange: (options: OptionData[]) => { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
The onSelectionChange callback creates new objects via .map() but is not memoized with useCallback. This creates a new function reference on every render, potentially causing unnecessary re-renders of components that receive this callback.
Suggested fix:
const handleSelectionChange = useCallback(
(options: OptionData[]) => {
if (!isIOUSplit) {
return;
}
const sanitizedParticipants: Participant[] = options.map((option) => sanitizedSelectedParticipant(option, iouType));
onParticipantsAdded(sanitizedParticipants);
},
[isIOUSplit, iouType, onParticipantsAdded],
);
const {searchTerm, setSearchTerm, availableOptions, selectedOptions, toggleSelection, areOptionsInitialized, onListEndReached, contactState} = useSearchSelector({
// ... other config
onSelectionChange: handleSelectionChange,
// ...
});
| availableOptions.selfDMChat, | ||
| availableOptions?.userToInvite, | ||
| availableOptions.workspaceChats, | ||
| cleanSearchTerm, |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The inputHelperText useMemo dependencies include full objects (availableOptions.selfDMChat, availableOptions?.userToInvite, availableOptions.workspaceChats) instead of specific properties. This causes the useMemo to re-execute whenever these object references change, even if the relevant data hasn't changed.
Suggested fix:
Since the useMemo body checks the existence and length of these values, depend on specific properties:
const inputHelperText = useMemo(
() =>
getHeaderMessage(
(availableOptions.personalDetails ?? []).length + (availableOptions.recentReports ?? []).length + (availableOptions.workspaceChats ?? []).length !== 0 ||
!isEmptyObject(availableOptions.selfDMChat),
!!availableOptions?.userToInvite,
searchTerm.trim(),
countryCode,
participants.some((participant) => getPersonalDetailSearchTerms(participant).join(' ').toLowerCase().includes(cleanSearchTerm)),
),
[
availableOptions.personalDetails?.length,
availableOptions.recentReports?.length,
availableOptions.workspaceChats?.length,
// For objects used in existence checks, you can depend on their length or other identifying property
availableOptions.selfDMChat?.reportID, // or another stable identifier
availableOptions?.userToInvite?.accountID, // or another stable identifier
cleanSearchTerm,
searchTerm,
participants,
countryCode,
],
);
| showImportContacts, | ||
| reportAttributesDerived, | ||
| inputHelperText, | ||
| ]); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
The sections useMemo dependencies include full arrays/objects (availableOptions.workspaceChats, availableOptions.userToInvite, availableOptions.recentReports, availableOptions.personalDetails, availableOptions.selfDMChat) instead of specific properties. This causes unnecessary re-computations when these references change even if their contents are the same.
Suggested fix:
Depend on specific properties like .length or identifying properties:
}, [
areOptionsInitialized,
didScreenTransitionEnd,
searchTerm,
participants,
personalDetails,
reportAttributesDerived,
translate,
availableOptions.workspaceChats?.length,
availableOptions.userToInvite?.accountID, // or another stable identifier
availableOptions.recentReports?.length,
availableOptions.personalDetails?.length,
availableOptions.selfDMChat?.reportID, // or another stable identifier
isWorkspacesOnly,
isPerDiemRequest,
showImportContacts,
inputHelperText,
]);
| @@ -163,12 +127,12 @@ function VacationDelegatePage() { | |||
| shouldShowSubscript: option.shouldShowSubscript ?? undefined, | |||
There was a problem hiding this comment.
❌ PERF-6 (docs)
The sections useMemo dependencies include full arrays/objects (availableOptions.personalDetails, availableOptions.recentReports, availableOptions.userToInvite) instead of specific properties. This causes the useMemo to re-execute whenever these array/object references change, even if the actual data hasn't meaningfully changed.
Suggested fix:
Depend on specific properties that actually affect the computation:
}, [
vacationDelegate,
delegatePersonalDetails,
availableOptions.personalDetails?.length,
availableOptions.recentReports?.length,
availableOptions.userToInvite?.accountID, // or another stable identifier
translate
]);Note: You may need to also check if the arrays themselves are present (not just their length) if the logic depends on array existence. Consider using a pattern like availableOptions.personalDetails?.length ?? -1 to distinguish between empty and undefined arrays if needed.
|
@lorretheboy Please check the comments above. Thanks! |
melvin-bot
bot
requested review from
joekaufmanexpensify
and removed request for
a team
|
@linhvovan29546 I resolved conflicts & addressed the comment from bot |
This comment was marked as resolved.
This comment was marked as resolved.
|
Refactor PR. Removing my review and unsubscribing |
|
@lorretheboy Conflict again |
|
@linhvovan29546 Pls check again. Thx |
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.2.49-0 🚀
|
1 similar comment
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.2.49-0 🚀
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.2.52-0 🚀
|
|
🚀 Deployed to production by https://github.com/cristipaval in version: 9.2.54-1 🚀
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.2.52-0 🚀
|
6 participants
Explanation of Change
Fixed Issues
$ #71062
PROPOSAL: #71062 (comment)
Tests
Offline 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))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
https://drive.google.com/file/d/1jx6IOlCvuepcIsn7xfDRp8wN4o1_b87k/view?usp=sharing
Android: mWeb Chrome
https://drive.google.com/file/d/1fGdtcr3QrQT6EEXB6N4yrvnWhTNcOcX7/view?usp=sharing
iOS: Native
https://drive.google.com/file/d/1KnW3_8-8nsy-r5n7xorChj0x4tW8q4Nu/view?usp=sharing
iOS: mWeb Safari
https://drive.google.com/file/d/1KBNh7Xf_cAo3NnJ94p8oozFuBJ4x0mXx/view?usp=sharing
MacOS: Chrome / Safari
https://drive.google.com/file/d/1PIfWXK8vQzQJOO5z1_Ll5UuJlzHvIe5u/view?usp=sharing
https://drive.google.com/file/d/1EclrXTkaG_Ty7u0dm-MObDG6rrqJp2dJ/view?usp=sharing
MacOS: Desktop
https://drive.google.com/file/d/1LdJ9RnWPIUJcD8zj9T0EZoyBShL_k4J4/view?usp=sharing