Repository navigation
Conversation
After inviting a member, the Role page route still holds the optimistic accountID. Once AddMembersToWorkspace finishes, that personal details entry is removed, so the page lost the member and showed the not found view. Share the optimistic-ID fallback from the member details page and use it on the Role, Work arrangement, and custom field pages. Co-authored-by: Maruf Sharifi <[email protected]>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
|
I created this PR from an upstream branch because I don't have push access to your fork. To take ownership of this branch and push updates, run: Then you can close this PR and open a new one from your fork. |
|
The failing Analysis: One test fails: Evidence: This PR touches only the workspace member pages, their test, and
|
|
🤖 Android test steps verification results: ✅ pass On standalone Android, inviting a new user and then quickly opening their details, opening Role, changing the role, and going back to the list all worked with no "Hmm... it's not here" error. The PR has no written test steps yet, so I used the repro steps from [$175] WS settings - Error appear when changing role just after adding User.
|
|
🤖 Web test steps verification results: ✅ pass On Web, inviting a new user and then quickly opening their details, opening Role, changing the role, and going back to the list all worked with no "Hmm... it's not here" error. The PR has no written test steps yet, so I used the repro steps from [$175] WS settings - Error appear when changing role just after adding User.
|
BUG#1:Preconditions: Test Steps:
Expected result: First Back opens member details. Second Back returns to Members. Actual Result : Screen.Recording.2026-10-14.at.3.52.53.PM.mov |
BUG#2:Test Steps:
Expected Result: Actual Result: Screen.Recording.2026-10-14.at.8.27.21.PM.mov |
Bug#3:Test Steps:
Expected result: Actual result: Screen.Recording.2026-10-14.at.3.01.30.PM.mov |
|
@MelvinBot find the root causes of the above 3 reported bugs |
Root causes for the 3 reported bugsOnly Bug 1 is caused by this PR. Bugs 2 and 3 also happen on
Bug 1: second Back shows the same member againRoot cause: This PR sets That leaves this stack: Members → Details (optimistic ID) → Details (real ID). The Role and Work arrangement pages have the same problem: Fix: Keep the real Bug 2: "Hmm... it's not here" after inviting your own secondary emailRoot cause: Two things combine here. Neither is in this PR's diff.
Fix: Add the current user's logins (from Bug 3: role shows "Member" for a few seconds after choosing Workspace adminRoot cause: This is a race between a read request and the role update. When you open the Role URL in a new tab, the app also loads the Members page behind it (
The optimistic data is keyed by login ( Fix: This is outside this PR's scope. The general fix belongs in the network layer: re-apply pending write data after a read response that touches the same keys. Investigation notes
Next Steps: Reply with |
|
@MelvinBot fix only the bug 1 |
…tic ID still pops to the screen below Co-authored-by: Maruf Sharifi <[email protected]>
|
I pushed a fix for Bug 1 in da2f0de. I didn't change anything for Bugs 2 and 3. What changed: The Role, Work arrangement, and Custom field pages now go back using the accountID from their own route. The details screen below them was opened with that same ID, so Back now returns to it instead of adding a second details screen. The role, work arrangement, and custom field updates still use the member's real accountID.
Checks run:
view run · no recording available |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
BUG#1:Test Steps:
Expected Result: Actual Result: RF-1.mov |
BUG#2:Preconditions: Chrome, a Control workspace where you are an admin, and a new email never used in Expensify.
Expected: One Back press returns to Members. Actual Result: The same member details appear again, requiring another Back press. RF-2mov.mov |
|
@MelvinBot find the root cause of those two bugs reported |
Root causes for the two bugs reported todayNeither bug is a regression from this PR. Both also happen on
BUG#2: same details page after Role → Workspace admin → BackRoot cause: The details page keeps the optimistic ID (X) in its route, but resolves the real ID (Y) once the invite completes ( My last commit made Role go back with its own route ID. On this path, that ID is Y. My earlier fix only covers opening Role while still offline, when the details page still passes X. The new test renders Role directly with X, so it misses this path. Fix: In BUG#1: same details page on the second browser Back after a refreshRoot cause: Work arrangement uses a static route. After a refresh, the app rebuilds only Members plus Work arrangement, with no details screen between them ( When you press the in-app Back, Members → Details (before refresh) → Details (replaced) → Work arrangement That is why browser Back shows Details twice. The accountID doesn't matter here. Fix: Handle this separately from this PR. The codebase's existing approach is to make the member sub-pages ( Investigation notes
Next Steps: Reply with |
|
@MelvinBot implement the BUG#2 fix |
…s to the same details screen after the invite resolves Co-authored-by: Maruf Sharifi <[email protected]>
|
I pushed the BUG#2 fix in 3fbc4f5. I didn't change anything for BUG#1. What changed: The member details page now opens Role, Work arrangement, and both custom fields with the accountID from its own route. Before, it used the member's real accountID. When the details screen still had the old optimistic ID, Back from Role couldn't find it and added a second details screen. Now the sub-page's route matches the details screen below it, so Back returns to it. The sub-pages still use the real accountID for their updates.
Checks run:
|
Co-authored-by: Maruf Sharifi <[email protected]>
|
Fixed the failing The check flagged
This change is formatting only. The behavior is the same.
|
|
@MelvinBot Mark this as ready for review |
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}
| const routeAccountID = Number(route.params.accountID); | ||
| const policyID = route.params.policyID; | ||
| const showRuleBotGuardModal = useRuleBotGuardModal(); | ||
| const {login: currentUserLogin = ''} = useCurrentUserPersonalDetails(); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
This three-line member-resolution block — const memberLogin = personalDetails?.[routeAccountID]?.login ?? getMemberLoginByOptimisticAccountID(policy, routeAccountID); followed by const memberPersonalDetails = usePersonalDetailByLogin(memberLogin); and const accountID = memberPersonalDetails?.accountID ?? routeAccountID; — is now duplicated verbatim in WorkspaceMemberDetailsPage.tsx, WorkArrangementPage.tsx, and here. The PR already consolidated the getMemberLoginByOptimisticAccountID helper into PolicyUtils; this surrounding resolution logic should be consolidated the same way so a future change to how stale optimistic accountIDs are resolved only has to be made in one place.
Extract the block into a single shared hook in @hooks (e.g. useWorkspaceMemberFromRoute(policy, personalDetails, routeAccountID)) returning {memberLogin, accountID}, and call it from all three pages:
const {memberLogin, accountID} = useWorkspaceMemberFromRoute(policy, personalDetails, routeAccountID);Reviewed at: 35b9afd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in 5f0aaba. I moved the block into a new hook, useWorkspaceMemberFromRoute. This page, WorkArrangementPage, and WorkspaceMemberDetailsPage now call the hook instead of repeating the three lines.
| const isWorkArrangementBetaEnabled = isBetaEnabled(CONST.BETAS.COMMUTER_EXCLUSIONS_ARRANGEMENTS); | ||
|
|
||
| const memberLogin = personalDetails?.[accountID]?.login ?? ''; | ||
| const memberLogin = personalDetails?.[routeAccountID]?.login ?? getMemberLoginByOptimisticAccountID(policy, routeAccountID); |
There was a problem hiding this comment.
❌ CONSISTENCY-3 (docs)
The same three-line stale-accountID resolution block (memberLogin via getMemberLoginByOptimisticAccountID, then usePersonalDetailByLogin, then accountID fallback) added here is identical to the blocks in WorkspaceMemberDetailsPage.tsx and WorkspaceMemberDetailsRolePage.tsx. Duplicating it across pages means a future fix to the optimistic-accountID swap logic must be applied in several files and risks drift.
Move this block into a single shared hook (e.g. useWorkspaceMemberFromRoute) and consume it here instead:
const {memberLogin, accountID} = useWorkspaceMemberFromRoute(policy, personalDetails, routeAccountID);Reviewed at: 35b9afd | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Fixed in 5f0aaba. This page now calls the shared useWorkspaceMemberFromRoute hook, as do the Role and Details pages.
|
@MelvinBot, replace the PR Author checklist with the verified one below: PR author checklist |
|
I replaced the PR description with your verified version. It now has your Explanation of Change, Tests, Offline tests, QA Steps, and a fully checked PR Author Checklist. The TODO placeholders are gone. The Screenshots/Videos sections are still empty, so you still need to add recordings for each platform. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppScreen.Recording.2026-10-19.at.12.11.06.PM.movAndroid: mWeb ChromeScreen_Recording_20261019_121621_Chrome.mp4iOS: HybridAppScreen.Recording.2026-10-19.at.12.26.14.PM.moviOS: mWeb SafariScreen.Recording.2026-10-19.at.12.29.11.PM.movMacOS: Chrome / SafariScreen.Recording.2026-10-19.at.11.56.31.AM.mov |
garrettmknight
left a comment
garrettmknight
left a comment
There was a problem hiding this comment.
Looks good from a product perspective.
|
@MelvinBot Merge the main. |
…leOptimisticAccountID Co-authored-by: Maruf Sharifi <[email protected]> # Conflicts: # src/libs/PolicyUtils/index.ts
|
Merged One conflict: Checks
|
|
MelvinBot has made 5 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot check this: #103855 (comment), and #103855 (comment) |
…Route Co-authored-by: Maruf Sharifi <[email protected]>
|
I fixed both duplication comments in 5f0aaba and replied in each thread. The three pages repeated the same three lines for finding the member. Those lines now live in one hook,
Checks
|
|
@MelvinBot Merge the main. |
…leOptimisticAccountID Co-authored-by: Maruf Sharifi <[email protected]> # Conflicts: # src/pages/workspace/members/WorkArrangementPage.tsx # src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx
|
🤖 I merged
Typecheck passes, and the PR's 21 UI tests pass. One follow-up for you: |
| /** | ||
| * A member who was just invited is listed under an optimistic accountID derived from their login, and that entry is removed from | ||
| * personal details once the invite request finishes and the backend supplies the real accountID. A member page opened | ||
| * while the invite was still in flight therefore holds a stale accountID in its route. Recover the login from the employee list | ||
| * so the page keeps resolving the member across that swap. | ||
| */ | ||
| function getMemberLoginByOptimisticAccountID(policy: OnyxEntry<Policy>, accountID: number) { | ||
| const matchesOptimisticAccountID = (login: string) => generateAccountID(login) === accountID || generateAccountID(Str.removeSMSDomain(login)) === accountID; | ||
| // A member invited by one of their secondary logins ends up listed under their primary login, so a route ID derived | ||
| // from the secondary matches no employee — primaryLoginsInvited records that mapping and recovers the member. | ||
| const primaryLoginOfInvitedSecondary = Object.entries(policy?.primaryLoginsInvited ?? {}).find(([secondaryLogin]) => matchesOptimisticAccountID(secondaryLogin))?.[1]; | ||
| if (primaryLoginOfInvitedSecondary) { | ||
| return primaryLoginOfInvitedSecondary; | ||
| } | ||
| return Object.keys(policy?.employeeList ?? {}).find(matchesOptimisticAccountID) ?? ''; | ||
| } |
There was a problem hiding this comment.
Duplication / dead code: this function is a byte-for-byte copy of the existing getMemberLoginByAccountID in src/libs/WorkArrangementUtils.ts. Only the function name and the inner lambda name differ:
// existing — src/libs/WorkArrangementUtils.ts
function getMemberLoginByAccountID(policy: OnyxEntry<Policy>, accountID: number): string {
const matchesAccountID = (login: string) => generateAccountID(login) === accountID || generateAccountID(Str.removeSMSDomain(login)) === accountID;
const primaryLoginOfInvitedSecondary = Object.entries(policy?.primaryLoginsInvited ?? {}).find(([secondaryLogin]) => matchesAccountID(secondaryLogin))?.[1];
if (primaryLoginOfInvitedSecondary) {
return primaryLoginOfInvitedSecondary;
}
return Object.keys(policy?.employeeList ?? {}).find(matchesAccountID) ?? '';
}This PR removes the only two callers of getMemberLoginByAccountID: the WorkArrangementPage.tsx and WorkspaceMemberDetailsPage.tsx imports in this diff. There are no other references in src/ or tests/. After merge, the old function, its export, and the orphaned Str/generateAccountID imports in WorkArrangementUtils.ts all become dead code.
There was a problem hiding this comment.
Fixed in 20e2616. I removed getMemberLoginByOptimisticAccountID and pointed useWorkspaceMemberFromRoute at the existing getMemberLoginByAccountID. The PR no longer changes members.ts, and there's no dead code.
| const customFieldType = params.customFieldType; | ||
| const accountID = Number(params.accountID); | ||
| const memberLogin = personalDetails?.[accountID]?.login ?? ''; | ||
| const memberLogin = personalDetails?.[accountID]?.login ?? getMemberLoginByOptimisticAccountID(policy, accountID); |
There was a problem hiding this comment.
The other three pages in this PR use the new useWorkspaceMemberFromRoute hook, but this one calls getMemberLoginByOptimisticAccountID directly.
It's functionally correct: the submit uses memberLogin, and goBack intentionally uses the route's stale accountID. So this is non-blocking. Still, it leaves the "resolve member from route" fallback in two call paths instead of one. Consider using the hook here too, ignoring the real accountID it returns, so the pattern lives in one place:
const {memberLogin} = useWorkspaceMemberFromRoute(policy, personalDetails, accountID);Also, this new fallback path has no test coverage. A "custom field opens/saves under a stale optimistic accountID" test would guard it, the same way WorkspaceMemberDetailsRolePageTest guards the Role page.
There was a problem hiding this comment.
Fixed in 20e2616:
- This page now uses
useWorkspaceMemberFromRouteand keeps the route's accountID forgoBack. - I added
WorkspaceMemberCustomFieldPageTest. Under a stale optimistic accountID, it checks that the saved value prefills, that saving uses the member's login, and thatgoBackuses the stale ID.
| // Only an existing member has an account ID here. The invite flow reads its own arrangement draft and never looks up a member. | ||
| const accountID = route.name === SCREENS.WORKSPACE.MEMBER_WORK_ARRANGEMENT ? Number(route.params.accountID) : CONST.DEFAULT_NUMBER_ID; | ||
| const memberLogin = personalDetails?.[accountID]?.login ?? getMemberLoginByAccountID(policy, accountID); | ||
| const routeAccountID = route.name === SCREENS.WORKSPACE.MEMBER_WORK_ARRANGEMENT ? Number(route.params.accountID) : CONST.DEFAULT_NUMBER_ID; |
There was a problem hiding this comment.
This file ships two behavior changes that no test exercises:
navigateBackToDetailsnow goes back withrouteAccountID(the stale optimistic id) instead ofmemberAccountID(line 87).setEmployeeWorkArrangementis now called with the resolved realaccountIDinstead ofmemberAccountID(line 99).
Both mirror the Role-page fix, which has solid coverage in the new WorkspaceMemberDetailsRolePageTest. Please add an equivalent test for the Work arrangement flow under a stale optimistic accountID. It should assert that:
- (a)
setEmployeeWorkArrangementreceives the real account id. - (b)
goBacktargets the details route built with the stale id, so it returns to the existing details screen instead of stacking a new one.
There was a problem hiding this comment.
Fixed in 20e2616. main's WorkArrangementPageTest already covers both cases with an optimistic accountID that swaps to a synced one:
- (a)
uses the resolved account ID for the first arrangement change after an offline invite syncschecks thatsetEmployeeWorkArrangementreceives the real accountID. - (b) The back-navigation test still expected the synced ID, so it failed after the merge. I updated it to expect the stale optimistic ID.
All 33 tests across the four member-page suites pass.
|
@MelvinBot please validate the requested changes and address them. |
… useWorkspaceMemberFromRoute, and cover stale optimistic accountIDs in tests Remove the duplicate getMemberLoginByOptimisticAccountID in favor of the existing getMemberLoginByAccountID. Update the WorkArrangementPage back-navigation test to expect the stale optimistic accountID, and add WorkspaceMemberCustomFieldPage tests for a stale optimistic accountID. Co-authored-by: Maruf Sharifi <[email protected]>
|
🤖 I addressed all three review comments in 20e2616 and replied in each thread:
A correction to my last comment: that back-navigation test had been failing since my merge. I only ran the Details and Role page suites then. Typecheck passes. All 33 tests in the four member-page suites pass. |
|
🤖 I checked your three requested changes against the current head (20e2616). All three are done:
I made no new changes. Typecheck, ESLint, React Compiler, and Jest job 5 pass on CI. The other Jest jobs are still running. |
This branch has not been deployed
3 participants
Explanation of Change
The problem:
When an admin invites a new workspace member, the app first creates a temporary account ID. When the server returns the real account ID, the Role page may still use the temporary ID.
This causes the error "Hmm... it's not here" when opening the Role page immediately after inviting someone.
What this PR fixes:
Fixed Issues
$ #103058
PROPOSAL: #103058 (comment)
Tests
1: Open Workspace → Members → Invite member.
2: Invite a new user.
3: Immediately open their details → Role.
4: Verify the Role page opens without errors.
5: Change Member → Workspace admin.
6: Verify the role updates correctly.
7: Navigate back and verify no duplicate pages appear.
8: Verify Work arrangement and Custom fields open correctly, if available.
Offline tests
1: Go offline before confirming a new invitation.
2: Invite a member and open their Role page.
3: Reconnect and verify the page remains accessible.
4: Change the role and verify it saves correctly.
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