Migrate useSearchSelector.base.ts from useOptionsList to usePersonalDetailOptions (part 5) - #94179
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@shubham1206agra 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] |
|
@hoangzinh Can you leave attendees part for now? I want you to actually migrate useSearchSelector hook to use useFilteredOptions internally? That is the part you have to do |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d423ed8d77
ℹ️ 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".
|
No product review needed |
@shubham1206agra Oops, I thought you mentioned the other parts, similar to your Part1-Part4. Let me rework on it. |
…age to usePersonalDetailSearchSelector Same migration as MoneyRequestAttendeeSelector: replaces useSearchSelector with usePersonalDetailSearchSelector and renames availableOptions.recentReports to availableOptions.recentOptions. Part of the useSearchSelector -> usePersonalDetailSearchSelector migration (issue Expensify#82191).
d423ed8 to
54a5108
Compare
|
@hoangzinh Lemme know when you are done |
|
@shubham1206agra Migration PR for WorkspaceWorkflowsApprovalsExpensesFromPage is ready |
|
@shubham1206agra all yours |
|
|
||
| // Add search results that are not already workspace members | ||
| const searchResults = [...availableOptions.recentReports, ...availableOptions.personalDetails].filter((option) => { | ||
| const searchResults = [...availableOptions.recentOptions, ...availableOptions.personalDetails].filter((option) => { |
There was a problem hiding this comment.
@hoangzinh Why do we need recentOptions here? Can't we work with personalDetails only here?
There was a problem hiding this comment.
Agreed. We don't need recentOptions here. I updated in this commit 332f98f
| // Add search results that are not already workspace members | ||
| const searchResults = [...availableOptions.recentReports, ...availableOptions.personalDetails].filter((option) => { | ||
| const searchResults = [...availableOptions.recentOptions, ...availableOptions.personalDetails].filter((option) => { | ||
| const isMember = policy?.employeeList?.[normalizeLogin(option.login)]; |
There was a problem hiding this comment.
@hoangzinh Also, why do we need to check for isMember here? excludeUsers should remove them already.
There was a problem hiding this comment.
You're right. We don't need isMember check here. I updated in this commit 822545d
This page selects individual people as workspace approvers, not chat contacts, so it doesn't need recent-report-based suggestions. Setting includeRecentReports to false matches the pattern used by other non-chat invite flows (DynamicWorkspaceInvitePage, MoneyRequestAttendeeSelector, DomainAddAdminPage) and lets the search results rely solely on availableOptions.personalDetails, which is no longer filtered against recentOptions.
…sults excludeLogins (via getExcludedUsers) intentionally leaves pending-delete and error-state policy employees unfiltered so they remain searchable, but the isMember check was blocking pending-delete employees from resurfacing in this search. Dedup against the members already in the list (selected approvers, active workspace members, userToInvite) already covers duplicates, so isMember was both redundant and incorrectly restrictive.
Reviewer Checklist
Screenshots/VideosScreen.Recording.2026-06-25.at.10.44.28.PM.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. |
|
🚧 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.4.21-2 🚀
|
|
🚀 Deployed to production by https://github.com/mountiny in version: 9.4.21-9 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
This PR continues the useSearchSelector → usePersonalDetailSearchSelector migration, covering Part 5: WorkspaceWorkflowsApprovalsExpensesFromPage.
Fixed Issues
$ #82191
PROPOSAL:
Tests
Same as QA
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, 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.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-06-22.at.23.03.08.mov