Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx bun ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@codex review |
|
@MelvinBot review |
![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: 7316a47f7e
ℹ️ 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".
Review — replace vacation delegate policy diff warning with an RHP invite stepOverall this is a clean, well-tested change. The refactor of 1. Confirm the "Skip" behavior in the mixed case (member of some + admin of some)In the mixed diff, the copy
So the admins of the workspaces you don't manage are supposed to be emailed regardless of the Invite/Skip choice — that choice is only about the workspaces you do administer. But the "Skip" button calls Could you confirm the backend semantics of 2. Minor robustness nits
What I checked
I did not exercise the flow in a live session: reaching the new RHP step requires a 305 policy-diff response, which needs a delegate who is missing from a specific mix of admin/non-admin workspaces — not reproducible against the test account. The mixed-case Invite/Skip path (item 1) is the key scenario to cover in manual QA. |
- Debounce rapid delegate-row taps so overlapping SET_VACATION_DELEGATE requests can't race and navigate on a stale response - Keep the missing-workspaces screen rendering from the submitted input instead of Onyx once submit clears the flow state, so it no longer flashes NotFoundPage/the previous delegate while the RHP is closing - Only roll back the delegate on unmount if this screen ever saw an active policy-diff flow, so a stray visit after the flow ended can't wipe out an unrelated, already-saved delegate - Fall back to a placeholder name for a workspace missing from Onyx
|
@MelvinBot could you check backend/OldDot parity with the current implementation in this PR? For the "Admin of some" (mixed) case, does Classic send an email to the owners of the workspaces you don't administer when skipping, or does it match this PR's current behavior of sending no email at all? |
|
Classic sends the email. In the "admin of some" (mixed) case, Classic emails the owner (with the other admins in cc) of every missing workspace you don't administer, whether you invite now or do it later. It has no way to suppress that — "do it later" issues the same plain One more signal that Skip is over-reaching: the backend only ever emails workspaces where you are not an admin. In the "admin of all" case Suggested change: use Caveat before you change it: the FE issue's own spec table says Skip → emails "none", so the divergence may be intentional. How I verified this
I could not read the Auth/PHP implementations directly in this run (no access to those repos), so the "not scoped to admin workspaces" claim rests on the written backend spec rather than on the merged code. If you want that nailed down, ask |
…tic delegate when request rejects, disable invite until every admin policy that is missing is loaded, foramt sms delegate logins as phone numbers, add selectors to onyx subscriptions, switch setDomainVacationDelegate to API.write
…selecting a delegate
…ff-warning-rhp-vacation-delegate
|
@MelvinBot could you re-review it? |
Re-review at
|
| Step | Status | Observation |
|---|---|---|
| Navigate to Settings > Profile > Status > Vacation delegate, no console errors | fail | Render tree empty (0 nodes), screenshot pure white; dev server returned 200 with valid HTML and a clean webpack build, so the blockage was client/session-side |
| List offers a selectable person, or shows the cannot-set message | fail | Settings/Status never reachable |
| Selecting a delegate doesn't flash a not-found page and saves | fail | Not reachable |
| No JS console errors throughout | fail | Not obtainable — log access unsupported for this web session |
Evidence (3)
Last known good state: CI prestart reached the sign-up screen at 11:14 UTC
Blank white page when the tester took over at 11:30 UTC
Still blank after waits and a probe press
…isting selector instead of newly created policyExpenseChatReportsSelector, build the policy expense chat report actions at invite time instead of in a per render useOnyx selector, remove no-op guard from delete branch, skip setVacationDelegate response navigation when the screen has lost focus, show loading indicator while vacation delegate is still hydrating
…-warning-rhp-vacation-delegate
…r clearing up the selected delegate
…ff-warning-rhp-vacation-delegate
…PI and align the styling
|
@mkhutornyi can you also do a full review please? thanks |
It matches pre-existing behavior, so I assume it's not a bug - you have chosen a delegate, but invites and emails haven't been sent so I think that's intentional
As said above, it matches the pre-existing behavior Screen.Recording.2026-09-09.at.16.02.13.mov |
|
I've never seen that before, I can't see it in the OG design doc either. I don't think it really makes sense either for a few reasons:
|
…acesPage component, change the copy message, minor performance issues, code clean-up
…lly set a vacation delegate
|
@mkhutornyi I've addressed all the comments - could you verify them? |
|
kindly bumping! @mkhutornyi |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-09-14.at.6.21.33.PM.movScreen.Recording.2026-09-14.at.6.23.17.PM.mov
|
|
Please pull main and fix lint |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8b54c271ff
ℹ️ 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".
Re-review at
|
Code quality review at
|
| Location | Missing /** */ |
|---|---|
WorkspaceRow.tsx:15 |
policyID |
WorkspaceSection.tsx:15,20 |
title, policies |
MissingWorkspacesFooter.tsx:18 |
onInvite |
VacationDelegate.ts:181-182 |
translate, formatPhoneNumber |
Separately, SetVacationDelegateOptions documents nothing while its sibling InviteVacationDelegateToWorkspacesOptions twelve lines below documents most fields. shouldOverridePolicyDiffWarning in particular carries the whole persisted-write-vs-side-effect branch and deserves a line.
2. Collapse submit / submitOnce / invite into one guarded function
index.tsx:83-119 is three functions where one does the job. invite re-implements the isSubmittingRef guard rather than reusing submitOnce, then calls the unguarded submit — so the guard lives in two places and the reader has to check both to convince themselves there's no double-submit path.
Taking the policies to invite as an argument removes the duplication and the mechanism-named submitOnce:
const submit = (policiesToInvite?: Policy[]) => {
if (isSubmittingRef.current) {
return;
}
isSubmittingRef.current = true;
if (policiesToInvite) {
inviteVacationDelegateToWorkspaces({delegate, policies: policiesToInvite, inviter: {...}, translate, formatPhoneNumber});
}
setSubmittedInput({delegate, policyDiff});
setVacationDelegate({creator, delegate, currentDelegate: previousDelegate, shouldOverridePolicyDiffWarning: true});
Navigation.goBack(ROUTES.SETTINGS_STATUS);
};Then onSkip={() => submit()} and onInvite={() => submit(adminWorkspaces)}. The hasUnresolvedAdminPolicy re-check inside invite can go too — the button is already disabled by isInviteDisabled from the same value.
3. onSkip drives the Confirm button too
Its own JSDoc admits the mismatch: "backing both Skip and Confirm". A prop whose name is wrong for half its call sites is worth renaming — onSubmitWithoutInvite or onConfirm says what it does in both branches. See MissingWorkspacesFooter.tsx:20-21.
4. The policy-key lookup is written twice (CONSISTENCY-3)
The same policies?.[POLICY_COLLECTION_KEY + policyID] template lookup appears at both index.tsx:60 and WorkspaceSection.tsx:40. The parent already resolves the collection for adminWorkspaces, so drilling the raw collection down two levels just so the child can resolve it again is avoidable — resolve once in the parent and give WorkspaceSection the resolved {policyID, policy} entries. That also drops the ONYXKEYS/OnyxCollection imports from the child and makes it purely presentational.
Considered and deliberately not flagged
createPoliciesByIDsSelectorreturning wholePolicyobjects — looks like aPERF-19candidate, but isn't:adminWorkspacesis passed intact intoinviteVacationDelegateToWorkspaces→addMembersToWorkspace, which readsemployeeListandname. The rule only applies when nothing the component keeps from the selector output is an object. Moving it tosrc/selectors/Policy.tswith its own test was the right call.- Missing file headers on the four new sub-components (
CONSISTENCY-14) — they're 45-65 lines of self-explanatory presentational code with named exports, which the rule's "short and self explanatory" exemption covers.index.tsxhas the header that matters. FullScreenLoadingIndicator shouldUseGoBackButton— correct perUI-1: it's in a separate return with no header alongside it.onSelectRowinVacationDelegatePage—CONSISTENCY-12would prefer a behavior name, but it's a pre-existing name that mirrors the prop it feeds.- Comment density in
setVacationDelegate— high, but every block explains a non-obvious backend or queueing behavior rather than restating the code.
This pass was static only — I couldn't drive the app in a browser on this run, and this workspace's node_modules is incomplete so I couldn't run ESLint locally either. Everything above comes from reading the code against App/.claude/skills/coding-standards. My separate correctness findings are in the earlier comment and still stand.
Next Steps: Reply with @MelvinBot <your feedback> to refine this review.
…ff-warning-rhp-vacation-delegate
|
done! @mkhutornyi |
|
Please check this weird behavior. Abandoning the missing-workspaces step leaves a delegate that reads as saved
Screen.Recording.2026-09-11.at.3.31.20.PM.mov |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b39f0deea
ℹ️ 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".
Regression hunt at
|
| Step | Status |
|---|---|
Status page renders the Vacation delegate section and rowHeading, description and a tappable row all present. |
✅ |
Selecting a user saves and returns to Status with the delegate shownPicked a user, navigated back automatically, row shows the delegate.POST /api/UpdateStatus returned 200. |
✅ |
Profile Status row shows no error dot after a successful saveNo red indicator on the Status row or the Account tab icon. |
✅ |
Re-tapping the same delegate clears it and the row stays renderedDelegate cleared and the Vacation delegate section is still present — the32a13a3 fix holds. |
✅ |
Reload, then check the delegate row and error stateNot run — this session cannot reload without risking an unrecoverable auth wall. |
|
Deep-link to settings/profile/status/vacation-delegateNot run — direct URL navigation is unavailable in this session. |
|
Deep-link to settings/profile/status/vacation-delegate/missing-workspacesNot run — direct URL navigation is unavailable in this session. |
Checked and cleared
SetVacationDelegatein bothWRITE_COMMANDSandSIDE_EFFECT_REQUEST_COMMANDS— not a problem.CompleteGuidedSetupis already registered both ways, so this is an established pattern.Domain.setDomainVacationDelegateswitching frommakeRequestWithSideEffectstoAPI.write— the response was never read (the old comment said so), the request becomes persisted and replayable offline, andtests/actions/DomainTest.tswas updated to match. Reads as a straight improvement.- The new nested route
settings/profile/status/vacation-delegate/missing-workspacesis registeredexact: truealongside the parent, so it doesn't shadowSETTINGS_VACATION_DELEGATE. Deep-linking it with no diff rendersNotFoundPageby design — I just couldn't confirm that in a browser. - Domain page error display is unaffected by the new gate; it passes
getLatestError(domainErrors…)directly rather than going throughgetVacationDelegateErrors. - CI at this head: ESLint, typecheck, React Compiler Compliance, Bun tests, all 8
testjobs, and both perf jobs pass. Onlychecklistis red.
Next Steps: Reply with @MelvinBot <your feedback> to refine this.
9b39f0d to
35cac1f
Compare
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@mkhutornyi should be ready! |
| invitedEmailsToAccountIDs, | ||
| // Writes resolve in queue order, so only the last invitation may clean the optimistic delegate up from under the ones still in flight. | ||
| isLastInvite ? personalDetailsOnyxData : {optimisticData: personalDetailsOnyxData.optimisticData}, | ||
| `# ${inviter.displayName ?? ''} invited you to ${policy.name}\n\n${translate('workspace.common.welcomeNote')}`, |
There was a problem hiding this comment.
The invite message's heading is hardcoded English.
Only the second half goes through translate
| setVacationDelegate({creator: currentUserLogin, delegate: option?.login ?? '', currentDelegate}) | ||
| .then((response) => { | ||
| if (!navigation.isFocused()) { | ||
| if (response?.data?.policyDiff) { | ||
| clearVacationDelegateError(currentDelegate); | ||
| } |
There was a problem hiding this comment.
I think this is expected: leaving the selector before the response lands silently discards the pick
On main the policy-diff prompt was a global confirm modal, so it appeared over whatever screen you were on and Confirm still saved the delegate. Now the RHP push is gated on navigation.isFocused(), and the else-branch just calls clearVacationDelegateError() and returns — no step, no modal, no error. Same in .catch at L79.
Steps
- Have a workspace the delegate isn't a member of.
- Settings > Profile > Status > Vacation delegate.
- Tap a user who isn't a member of that workspace.
- Immediately tap the header back arrow, before the request returns.
- Check the vacation delegate row on Settings > Profile > Status.
main: the prompt is shown so the choice can still be completed.
this branch: the row shows the previous delegate (or stays empty) — the pick is dropped with nothing shown anywhere.
Edge case though, not blocker
Screen.Recording.2026-09-14.at.6.17.48.PM.mov
There was a problem hiding this comment.
This is because we are waiting for a response with side effects to come back with policyDiff; that's why you see it flickering for a while. By removing a modal and using RHP here just it just widens a window when we can have this race. I can get the same bug on main
Screen.Recording.2026-09-14.at.19.57.35.mov
|
Found 4 regressions — behavior that worked on 1. The RHP shows the raw email instead of the delegate's display name
Assign "Jane Doe" ( This is also the one case from the deleted test file that isn't replaced — see #4. 2. Picking a delegate now blocks behind the entire write queue
Send 20 messages offline, reconnect, then open Vacation delegate and tap someone: no checkmark, no spinner, nothing, for as long as the unrelated queue takes — and every further tap is swallowed by the 3. A 305 without
|
|
@MelvinBot are those regressions caused by this PR, not happening on |
|
Yes — all four are PR-caused. I re-checked each one by diffing base One correction first: the screenshot I attached to #1 was not proof. 1. Display name → raw login
The strongest evidence is the test this PR deleted, which asserted exactly this: await selectRowAndFlush(EMAIL_DELEGATE, 'Jane Doe');
expect(mockShowConfirmModal).toHaveBeenCalledWith(expect.objectContaining({prompt: 'statusPage.vacationDelegateWarning(Jane Doe)'}));So on 2. Selection blocked behind the write queue
Confirmed by code only — I didn't build an offline backlog and reproduce the visible stall. 3. 305 without
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35cac1ff22
ℹ️ 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 [policies] = useOnyx(ONYXKEYS.COLLECTION.POLICY, {selector: createPoliciesByIDsSelector([...adminPolicies, ...nonAdminPolicies])}); | ||
|
|
||
| // Only loaded workspaces can be invited into, so a missing one has to block the invite rather than be dropped from it. | ||
| const adminWorkspaces = adminPolicies.map((policyID) => policies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`]).filter((policy): policy is Policy => !!policy?.id); |
There was a problem hiding this comment.
When an admin policy exists only as an Onyx summary (isFromFullPolicy is false) or otherwise lacks a complete employeeList, this filter treats it as resolved solely because it has an id. Invite is then enabled and the incomplete membership is passed through getMemberAccountIDsForWorkspace() to addMembersToWorkspace(), which uses those account IDs to construct reportCreationData and decide whether to create the #announce room; this can omit existing members or fail to create the room when the invitation crosses the three-member threshold. Require or fetch the full policies before considering these workspaces invitable.
Useful? React with 👍 / 👎.
9 participants
Explanation of Change
Replaces the vacation delegate's blocking "policy diff" warning with an RHP step (
VacationDelegateMissingWorkspacesPage). When the delegate isn't a member of every affected workspace, it lets the user invite them straight into the workspaces they admin - any workspace they don't admin still gets its real admins emailed, or skip entirely and just save the delegate, with no invites or emails sent to admin policies.Fixed Issues
$ #99078
PROPOSAL:
Tests
"member of" section and a single "Confirm" button > tap Confirm > verify the delegate is saved.
of" section with Invite/Skip buttons > tap Invite > verify the delegate is saved and the user is
added as a member to each listed workspace.
buttons > tap Skip > verify the delegate is saved without adding the user to any workspace.
delegate reverts to what it was before.
Offline tests
N/A
QA Steps
Same as Tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, 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
MacOS: Chrome / Safari
Screen.Recording.2026-09-04.at.08.36.53.mov