Refactor searchMatchUtils out from optionsListUtils#86982
Refactor searchMatchUtils out from optionsListUtils#86982sharabai wants to merge 1 commit intoExpensify:mainfrom
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06b029bcd0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return searchTerms.every((term) => | ||
| isPersonalDetailMatchingSearchTerm(personalDetail, currentUserAccountID, term, { | ||
| useLocaleLowerCase: true, | ||
| transformSearchText: (concatenatedSearchTerms) => deburr(`${concatenatedSearchTerms} ${personalDetail.text ?? ''}`), |
There was a problem hiding this comment.
Keep transformed search text case-insensitive
This callback appends personalDetail.text after isPersonalDetailMatchingSearchTerm() has already lowercased the base terms, so any uppercase characters in personalDetail.text are now matched case-sensitively. Before this refactor, the whole concatenated string (terms + text) was lowercased first, so lowercase queries would still match. In cases where the relevant token exists only in text (not in participantsList/displayName/login), search can now miss valid results.
Useful? React with 👍 / 👎.
06b029b to
9f3a79c
Compare
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
Explanation of Change
Motivation
While looking at how
NewChatPageprepares its data, two things stood out:A repeated pattern across 4+ files:
getPersonalDetailSearchTerms(item, id).join(' ').toLowerCase().includes(term)-- identical logic copy-pasted with no shared abstraction.NewChatPagecallingformatSectionsFromSearchTermwith 7 parameters (includingprivateIsArchivedMap,allPolicies,allPersonalDetails,reportAttributesDerived) that are completely unused in the code path NewChatPage actually hits. This adds unnecessary Onyx subscriptions and widens the dependency surface for React Compiler memoization.What changed
Created
src/libs/OptionsListUtils/searchMatchUtils.tswithisPersonalDetailMatchingSearchTerm-- a single function that encapsulates the "does this option match a search term" check, with an optional config for locale-aware lowercasing and text transformation.Moved
getPersonalDetailSearchTermsandgetCurrentUserSearchTermsinto the new file (re-exported fromindex.tsfor backward compatibility).Replaced
formatSectionsFromSearchTerminNewChatPagewith a 4-line inline filter. This removed theusePrivateIsArchivedMap()hook, theallPoliciesOnyx subscription, and theallPersonalDetailscontext read from the component body -- all of which were only needed for that one call.Replaced the inline pattern in
MoneyRequestParticipantsSelectorandMoneyRequestAttendeeSelector.Gains
NewChatPagecomponent: removedusePrivateIsArchivedMap()(subscribes toCOLLECTION.REPORT_NAME_VALUE_PAIRS),allPolicies(COLLECTION.POLICY), andallPersonalDetails(PERSONAL_DETAILS_LIST) from the component body. Fewer subscriptions = fewer re-renders = less cache invalidation.NewChatPage's section building.SearchMatchConfigparameter supportsuseLocaleLowerCaseandtransformSearchTextfor call sites that need diacritic-insensitive matching or appended text.Fixed Issues
$ #86089
Tests
Offline tests
QA Steps
Same as 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))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