Revert "use useSearchSelector hook and remove unused and duplicated code"#74302
Revert "use useSearchSelector hook and remove unused and duplicated code"#74302luacmartins merged 1 commit intomainfrom
Conversation
melvin-bot
bot
requested review from
MonilBhavsar and
joekaufmanexpensify
and removed request for
a team
|
@MonilBhavsar 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] |
|
🚧 @lakchote has triggered a test Expensify/App build. You can view the workflow run here. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| const initialSelectedOptions = useMemo(() => { | ||
| return value.reduce<OptionData[]>((acc, id) => { | ||
| const participant = personalDetails?.[id]; | ||
| if (!participant) { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
This useMemo hook depends on the entire personalDetails object. Changes to any property will trigger re-computation even when only specific properties are needed.
Instead, extract and depend on only the specific properties required:
const initialSelectedOptions = useMemo(() => {
return value.reduce<OptionData[]>((acc, id) => {
const participant = personalDetails?.[id];
if (!participant) {
return acc;
}
// ... rest of logic
}, []);
}, [value, ...value.map(id => personalDetails?.[id])]); // or extract needed keys
| countryCode, | ||
| ); | ||
| }, [options.reports, options.personalDetails, draftComments, countryCode]); | ||
|
|
There was a problem hiding this comment.
❌ PERF-6 (docs)
This useMemo depends on entire objects availableOptions.personalDetails and availableOptions.recentReports. Changes to any property in these arrays will trigger re-computation unnecessarily.
Consider extracting specific properties needed or using more granular dependencies to prevent unnecessary recalculations when unrelated data changes.
| @@ -117,11 +157,11 @@ function SearchFiltersParticipantsSelector({initialAccountIDs, onFiltersUpdate}: | |||
| sections: newSections, | |||
| headerMessage: message, | |||
There was a problem hiding this comment.
❌ PERF-6 (docs)
This useEffect depends on the entire personalDetails object. It will re-execute whenever any property in personalDetails changes, even if the changes are unrelated to the initialAccountIDs.
Extract and depend on only the specific personal detail entries needed:
// eslint-disable-next-line react-compiler/react-compiler, react-hooks/exhaustive-deps
}, [initialAccountIDs, ...initialAccountIDs.map(id => personalDetails?.[id])]);
| const [didScreenTransitionEnd, setDidScreenTransitionEnd] = useState(false); | ||
| const {contacts} = useContactImport(); | ||
| const {options: listOptions, areOptionsInitialized} = useOptionsList({ | ||
| shouldInitialize: didScreenTransitionEnd, |
There was a problem hiding this comment.
❌ PERF-6 (docs)
This useMemo depends on entire objects availableOptions and selectedOptions. Changes to any property will trigger re-computation unnecessarily.
Consider depending on specific properties that are actually used in the computation instead of the entire objects.
| personalDetails: (listOptions.personalDetails ?? []).concat(contacts), | ||
| }, | ||
| draftComments, | ||
| { |
There was a problem hiding this comment.
❌ PERF-6 (docs)
This useMemo depends on entire objects options.personalDetails, options.recentReports, options.userToInvite, and selectedOptions. Changes to any property in these objects will trigger unnecessary re-computation.
Depend on specific properties needed for the header message calculation instead of entire objects.
| undefined, | ||
| reportAttributesDerived, | ||
| ); | ||
| sectionsList.push(formatResults.section); |
There was a problem hiding this comment.
❌ PERF-6 (docs)
This useMemo depends on entire objects selectedOptions, recentReports, personalDetails, and reportAttributesDerived. Changes to any property in these objects will trigger re-computation of sections unnecessarily.
Extract and depend on specific properties needed for section construction to prevent unnecessary recalculations.
| return filteredOptions; | ||
| }, [unselectedOptions, cleanSearchTerm, selectedOptions, countryCode]); | ||
|
|
||
| const {sections, headerMessage} = useMemo(() => { |
There was a problem hiding this comment.
❌ PERF-4 (docs)
This code creates new object instances with spread operator on every render inside the useMemo for sections. While the sections array itself is memoized, the .map() creates new object references for each option on every computation.
Consider memoizing the transformed data or ensure this transformation only happens when the underlying data actually changes by having more specific dependencies in the parent useMemo.
Codecov Report❌ Patch coverage is
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
lakchote
changed the title
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
No product considerations. Removing my review and unsubscribing |
luacmartins
changed the title
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariMacOS: Desktop |
|
Straight revert, gonna merge without checklists and with the failing ESLint test |
|
@luacmartins looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
Revert "use useSearchSelector hook and remove unused and duplicated code" (cherry picked from commit 4ad44ed) (cherry-picked to staging by luacmartins)
|
🚀 Cherry-picked to staging by https://github.com/luacmartins in version: 9.2.44-2 🚀
|
|
FYI #72318 is a backend thing, not related to the reverted PR 👍 |
|
Ah thanks for pointing that out |
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.44-5 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/luacmartins in version: 9.2.45-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 9.2.45-6 🚀
|
7 participants
Holding the revert for now, see https://expensify.slack.com/archives/C01GTK53T8Q/p1762339274693719?thread_ts=1762332149.929999&cid=C01GTK53T8Q
Reverts #71427
Fixed issues
$ #74277
$ #74239
$ #74236