Migrate useSearchSelector.base.ts from useOptionsList to useCurrentUserPersonalDetails (part 1)#90127
Conversation
|
@hoangzinh 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] |
shubham1206agra
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb92697b2f
ℹ️ 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".
| const sanitizedPhoneLogin = isValidPhoneNumber ? addSMSDomainIfPhoneNumber(parsedPhoneNumber.number?.e164 ?? normalizedPhoneNumber) : ''; | ||
| const login = email ? effectiveSearchValue : (sanitizedPhoneLogin ?? searchValue); | ||
|
|
||
| const isCurrentUserLogin = Object.keys(loginList ?? {}).some((loginToTest) => loginToTest.toLowerCase() === login); |
There was a problem hiding this comment.
The current-user filter compares loginToTest.toLowerCase() against login without normalizing login, so mixed-case emails from device contacts (for example User@Example.com) can bypass the current-user check. In that case the user can be suggested as an invite candidate even though they should be excluded.
Useful? React with 👍 / 👎.
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.
|
There was a problem hiding this comment.
Pull request overview
This PR is part of the ongoing migration away from OptionListContextProvider-driven option calculations by introducing a new personal-details-focused search/selection hook and adopting it in a couple of high-traffic selectors (Room invite + Search user filter).
Changes:
- Added
usePersonalDetailSearchSelector(web + native) backed byPersonalDetailOptionsListUtils, including optional device-contacts integration on native. - Refactored contact option construction by moving the “contact -> option” logic into
PersonalDetailOptionsListUtilsand updating contact-import hooks accordingly. - Updated consumers/import paths (e.g.,
useSearchSelector/basepath change) and migratedRoomInvitePage+UserSelectorto the new hook.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/useSearchSelectorTest.tsx | Updates the import path for the refactored useSearchSelector base hook. |
| src/pages/RoomInvitePage.tsx | Migrates room-invite participant selection/search to usePersonalDetailSearchSelector. |
| src/libs/PersonalDetailOptionsListUtils/types.ts | Extends config/types to support contact option building + soft exclusions. |
| src/libs/PersonalDetailOptionsListUtils/index.ts | Adds getContactOption() and updates getValidOptions() to respect soft-exclusions for suggestions. |
| src/libs/OptionsListUtils/index.ts | Removes the old contact-option builder export. |
| src/libs/ContactUtils.ts | Switches contact transformation to produce OptionData + adds an adapter for legacy SearchOption<PersonalDetails>. |
| src/hooks/useSearchSelector/index.ts | Adjusts to new ./base module layout. |
| src/hooks/useSearchSelector/index.native.ts | Adjusts imports to new layout and uses aliased useContactImport. |
| src/hooks/useSearchSelector/base.ts | Updates imports to use @hooks/* aliases (migration preparation). |
| src/hooks/usePersonalDetailSearchSelector/index.ts | New web entrypoint for the personal-details selector hook. |
| src/hooks/usePersonalDetailSearchSelector/index.native.ts | New native entrypoint with contact import integration. |
| src/hooks/usePersonalDetailSearchSelector/base.ts | New core implementation for personal-details search/selection. |
| src/hooks/useContactImportNew.ts | New contact import hook returning OptionData[] for the new selector. |
| src/hooks/useContactImport.ts | Updates existing contact import to use the refactored contact utilities. |
| src/components/Search/FilterComponents/UserSelector.tsx | Migrates the search “User” filter selector to usePersonalDetailSearchSelector. |
| config/eslint/eslint.seatbelt.tsv | Updates seatbelt allowlist paths for the moved/added native hook files. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-05-15.at.18.47.14.movAndroid: mWeb ChromeScreen.Recording.2026-05-15.at.18.41.28.moviOS: HybridAppScreen.Recording.2026-05-15.at.18.53.55.moviOS: mWeb SafariScreen.Recording.2026-05-15.at.18.52.20.movMacOS: Chrome / SafariScreen.Recording.2026-05-15.at.18.37.59.mov |
|
@MelvinBot review this PR |
PR ReviewThis PR creates a new Issues1. Missing
The original 2. The 3. The old hook passed 4. Selection model changed from login-based to accountID-based in The old Nits
SummaryThe core refactor is clean and the component simplifications are welcome. The |
There was a problem hiding this comment.
One note for everyone. We will gradually switch the use of useContactImport to the newer implementation in subsequent PRs.
|
No product review needed |
| // Falls back to creating from personal details to handle pagination edge cases. | ||
| let currentUserOption: OptionData | undefined; | ||
| if (!isCurrentUserSelected && currentUserAccountID) { | ||
| const currentUserPersonalDetail = personalDetailsList.find((p) => p.accountID === currentUserAccountID) ?? recentReports.find((r) => r.accountID === currentUserAccountID); |
There was a problem hiding this comment.
Hmm, interesting that we have recentReports in old code
|
@shubham1206agra can you merge latest main? I will start to test today |
|
@hoangzinh Merged main |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Thanks! All the comments are NAB
| selectedOptions: OptionData[]; | ||
| recentOptions: OptionData[]; | ||
| personalDetails: OptionData[]; | ||
| userToInvite: OptionData | null; | ||
| extraOptions: OptionData[]; | ||
| currentUserOption: OptionData | null; |
| searchValue: string; | ||
| countryCode: number; | ||
| formatPhoneNumber: LocaleContextProps['formatPhoneNumber']; | ||
| loginList: OnyxEntry<LoginList>; | ||
| firstName: string; | ||
| lastName: string; | ||
| email: string; | ||
| phone: string; | ||
| avatar: Icon['source']; |
| ); | ||
| } | ||
|
|
||
| function getContactOption({searchValue, firstName, lastName, email, phone, avatar, countryCode, loginList, formatPhoneNumber}: GetContactConfig): OptionData | null { |
There was a problem hiding this comment.
Could you add unit test for this method please
|
Please address the NAB comments in future PR, thank you! |
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
✋ 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/mountiny in version: 9.3.76-0 🚀
|
|
I reviewed the changes in this PR. This is a purely internal code refactoring that migrates There are no user-facing behavior changes, no new features, no UI label changes, and no terminology updates. No help site documentation changes are required. |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.3.77-3 🚀
|
Explanation of Change
Fixed Issues
$ #82191
Tests
Offline tests
Same as 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
Screen.Recording.2026-05-10.at.9.00.16.PM.mov