Repository navigation
[Payment due @linhvovan29546] Exclude non-manageable agents from workspace invite list - #104110
yuwenmemon merged 12 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: c8a6bd3e2f
ℹ️ 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".
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@linhvovan29546 This is good for review / test |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-22.at.17.29.38.movAndroid: mWeb ChromeScreen.Recording.2026-10-22.at.17.26.46.movScreen.Recording.2026-10-22.at.17.26.59.moviOS: HybridAppScreen.Recording.2026-10-22.at.17.31.16.moviOS: mWeb SafariScreen.Recording.2026-10-22.at.17.25.34.movMacOS: Chrome / SafariScreen.Recording.2026-10-22.at.17.14.19.mov |
|
@MelvinBot Review |
|
The filtering logic looks correct. I found one possible gap and two smaller cleanups.
|
| const [personalDetails] = useAllPersonalDetails(); | ||
| const [account] = useOnyx(ONYXKEYS.ACCOUNT); | ||
| const employeePersonalDetails = usePersonalDetailsByLogins(Object.keys(policy?.employeeList ?? {})); | ||
| const [allAgents] = useOnyx(ONYXKEYS.COLLECTION.AGENT); |
There was a problem hiding this comment.
COLLECTION.AGENT is only populated by openAgentsPage() (called from useAgents / ProfilePage), and this page never calls it. So on a fresh session allAgents is undefined and managedAgentsAccountIDs is an empty Set. includeAgentsOnly is still truthy, so all agents are filtered out, including the user's own.
Reproduced on dev: Troubleshoot → Clear cache and restart → go straight to Workspace → Members → Invite. The list is empty even though the user's own agent (personal) and a workspace-owned agent are both in personal details with isCustomAgent: true. After opening Settings → Agents and coming back, they appear.
Should we call openAgentsPage() when this page opens (and/or gate on ARE_AGENTS_LOADED)?
const [areAgentsLoaded] = useOnyx(ONYXKEYS.ARE_AGENTS_LOADED);
// ...
includeAgentsOnly: areAgentsLoaded ? managedAgentsAccountIDs : undefined,There was a problem hiding this comment.
Updated to use useAgents. This fixes the issue as it opens the agents page. However this also meants that now we are calling two API commands on page open OpenWorkspaceInvitePage and OpenAgentsPage. We should not do that but given the urgency it's fine for now. I also just noticed that the Agents page has same issue (calling both. OpenWorkspaceMembersPage and OpenAgentsPage)
| } | ||
| const accountIDToReportIDMap = generateAccountIDToReportIDMap(reports, accountID); | ||
|
|
||
| return memoizedCreateOptionList(accountID, personalDetails, accountIDToReportIDMap, reports, reportAttributes, reportNameValuePairs ?? {}, formatPhoneNumber, translate, { |
There was a problem hiding this comment.
NAB: memoizedCreateOptionList has maxSize: 1, and the invite page now always requests a narrowed list, so switching between this page and any full-list consumer rebuilds the whole option list each time. Probably fine, but the doc comment above memoizedCreateOptionList only mentions includeLoginsOnly evicting the whole-list entry. Can we mention includeAgentsOnly too?
melvin-bot
Bot
changed the title
|
🎯 @linhvovan29546, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
@s77rt Could you please update the fixed issue section? Thanks! |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 yuwenmemon 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! 🧪🧪
|
5 participants
This is based on #103817 so let's merge that one first
Explanation of Change
Added
includeAgentsOnlyparam to the personal details list selector that is similar toincludeLoginsOnly. HoweverincludeAgentsOnlyonly applies to agents i.e. other regular accounts are not affected, it will only make sure that if an agent is displayed then it's part ofincludeAgentsOnlyset.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/681641
$ https://github.com/Expensify/Expensify/issues/694594
PROPOSAL:
Tests
Offline tests
Same as Tests
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)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
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari