Save the workflow when "Expense from" is edited via the +N more fast edit - #100434
Save the workflow when "Expense from" is edited via the +N more fast edit#100434MelvinBot wants to merge 7 commits into
Conversation
The fast-edit entry point opens the expenses-from page straight from the workflows page, so the edit page - the only screen that computes membersToRemove and calls updateApprovalWorkflow - never mounts. Save only wrote the Onyx draft and navigated back, so the member the admin picked or dropped was silently discarded. Mark the session as a fast edit, keep the original members alongside the original approvers, and save from the expenses-from page when no edit page will do it. Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
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 the branch and push updates yourself: Then you can close this PR and open a new one from your fork. |
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Fixed the failing check — it was formatting, not logic. What failed:
Fix: ran Verified locally before pushing: One thing still open from the original PR, unchanged by this push: the fixed flow has not been exercised in a browser, and the |
|
🤖 Android test steps verification results: ✅ pass Editing a workflow's "Expense from" members via the "+N more" fast-edit chip now saves correctly on Android: removals and re-additions both persist immediately and across app relaunch, matching the fix in this PR.
|
|
🤖 Web test steps verification results: ✅ pass The "+N more" fast-edit chip on Workflows > Approvals now correctly saves member changes (both deselect and re-add), confirmed via UI, successful
view run · no recording available |
|
@MelvinBot Also, merge main, complete the checklist and test steps, and mark screenshots as done since they’ll be added in the reviewer checklist |
…itSave Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
Done — Merged AI tests re-run after the merge:
All CI checks now pass except PR body updated:
One thing to be aware of before you mark this ready. I checked the "all platforms" boxes as you asked, but here is exactly what was actually exercised, so the record is straight:
|
|
There's nothing to fix — no check is failing on code. Every test, lint, typecheck, build, and perf job passes on the current head ( The single red check is Check independent approval, and it's a review gate, not a test: The PR has zero approvals so far — the only requested reviewer is the Full check statusFailing: Passing: Jest unit tests (all 8 jobs), Bun tests, Storybook tests, Reassure performance tests (branch + baseline + validate), Earlier in the branch's history an The view run · no recording available |
|
@MelvinBot @codex review the PR and check for regressions, breaking changes or bugs |
|
Codex Review: Didn't find any major issues. You're on a roll. 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". |
Review: 2 bugs worth fixing before this mergesThe fix itself is sound and the approach is right. Two ordering problems in the new save block can lose an admin's edit, plus one narrower stale-state path. 🔴 1.
|
|
@MelvinBot please fix the following before this is ready for review. Items 1–3 are one coherent change to the save block in 1. Validate before navigating
This is reachable without the admin doing anything wrong: Move the validation above the 2. The draft is not reliably cleared on either save pathBoth branches have the same hole from opposite directions. Beta branch: Non-beta branch: the comment says // If there are no changes to the employees list, we can exit early
if (isEmptyObject(updatedEmployees) && !newDefaultApprover) {
return;
}
Since the fast edit now owns this draft's whole lifecycle, own the teardown explicitly instead of inheriting it from an action that is allowed to bail: const originalMembers = approvalWorkflow.originalMembers ?? [];
runAfterPredictedTransition(() => {
if (isMultipleApproversBetaEnabled) {
updateApprovalWorkflowRules({approvalWorkflow: workflowToSave, initialApprovalWorkflow: {...workflowToSave, members: originalMembers}, policy, rules: rulesCollection});
} else {
const membersToRemove = originalMembers.filter((originalMember) => !allMembers.some((member) => member.email === originalMember.email));
updateApprovalWorkflow(workflowToSave, membersToRemove, [], policy);
}
// This session owns the draft: no edit page will consume it, and both save paths can bail without clearing.
clearApprovalWorkflow();
});
3. An abandoned fast edit leaves
|
- Move validateApprovalWorkflow above Navigation.goBack so a failed validation keeps the admin on the page instead of navigating away and silently discarding the member change, and surface approvalWorkflow.errors in the footer alert. - Clear the draft explicitly after either save path. Neither one reliably clears it: updateApprovalWorkflowRules never does, and updateApprovalWorkflow only clears once it reaches its optimistic data, which it skips on an empty employee diff. That left isFastEdit sitting in persisted Onyx after a successful save. - Discard the draft when a fast edit unmounts unsaved, and drop isFastEdit when the edit page resumes a draft, so a stale flag can't let the sub-page save and clear the draft out from under a mounted edit page. - Add a fast-edit sentryLabel to the Save button now that it performs a write. - Cover the MULTIPLE_APPROVERS branch, draft teardown (including save with no effective change), the validate-before-navigate ordering, and the new action. Co-authored-by: abzokhattab <abzokhattab@users.noreply.github.com> Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
|
|
All five items are fixed in 1. Validate before navigating ✅
I also went one step further than the ask: you noted the errors land in 2. Draft teardown ✅Applied your suggested shape — one unconditional 3. Stale
|
| Check | Result |
|---|---|
npm run typecheck |
✅ passed |
npm run lint-changed |
✅ passed |
npm run spell-changed |
✅ passed |
npm run fmt (oxfmt) |
✅ no changes needed |
npm run react-compiler-compliance-check check on both changed pages |
✅ passed |
npm test — all 9 workflow suites |
✅ 260 passed |
npm run lint (whole repo) |
lint-changed covers every file in the diff |
forwardsTo), which has no browser coverage at all.
|
|
|
|
|
|
|
|
|
|
|
@MelvinBot review the pr |
|
|
|
|
|
|
|
@MelvinBot please address the two remaining issues below before this is ready for review. I re-reviewed at 1. Deferred save + unmount clear can wipe the next session's draft
Two clears then race with the next
If the admin taps another workflow's The edit page has a weaker version ( Fix: keep a generation (or the The deferred 2. Failed validation traps the admin on a page that cannot show the error
The footer only sets On a policy that already has a cycle, every fast edit looks like a no-op: Save → generic error → Back → selection gone. The edit page uses the same generic alert, but the errors are actually on that form. Fix (either): pass a translated TestsThe suite guards the last round of bugs, but Do not fold moving |
…can't fix - Add a session ID that setApprovalWorkflow bumps whenever a new draft is seeded. The expenses-from fast edit captures it before navigating and re-reads it inside runAfterPredictedTransition, which can run as late as MAX_TRANSITION_START_WAIT_MS + MAX_TRANSITION_DURATION_MS. A save superseded by a newer "+N more" session now skips the explicit clearApprovalWorkflow() and passes shouldClearApprovalWorkflowDraft false to updateApprovalWorkflow, so neither the clear nor the optimistic SET null wipes the draft that session is editing. The write itself still lands: cancelling it would silently drop the change the admin already confirmed. - Save from a fast edit through validateFastEditApprovalWorkflow, which drops the two approver rules this page has no field for (a circular forwardsTo, an approvalLimit and overLimitForwardsTo that don't agree). On a policy that already carries one, every fast edit failed with an alert the admin couldn't act on, and Back then discarded the member change. It keeps the structural checks a save depends on. - Translate the remaining errors into the footer's message instead of falling through to FormAlertWrapper's generic "please fix the errors in the form", which this form has no field to point at and no onFixTheErrorsLinkPressed to jump to. - Make the runAfterPredictedTransition test mock hold callbacks on demand, and cover the superseded save, unmount-clear on abandon, the draft surviving a non-fast-edit unmount, and the new validator. Co-authored-by: abzokhattab <abzokhattab@users.noreply.github.com> Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
|
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 review the pr |
|
Both are fixed in 1. Deferred save can no longer wipe the next session's draftAdded a session ID that One deviation from your prescription, please sign off: a superseded save still writes, it just stops touching I left the deferred 2. Validation no longer traps the adminWent with your second option. Fast edits now validate through I kept the checks a save structurally depends on — no members on a non-default workflow, or a missing approver slot — because those would write the workflow back truncated, not just imperfect. Neither is reachable from a workflow the workflows page rendered, so this only fires on a corrupted draft. It stays a type guard, so Did your first option too, since it's what makes the remaining cases legible: the footer now translates the error into TestsThe
Did not touch Checks run
Browser verification did not happen, and that's on me. I asked the web tester to drive the flow, but the session's browser tab had loaded during a window when my rename was half-applied and the dev bundle was momentarily broken. The server rebuilt fine afterwards, but the tab was stuck blank and the tester correctly declined to reload or restart the session to recover it. The code on disk is consistent — that transient error is not in any commit. The Jest suites above are the only verification I have for this round. |
- Assert the approver-slot error key by key. The object literal `{'approver-1': ...}`
tripped @typescript-eslint/naming-convention and was the only ESLint error on the PR.
- Capture the approval-workflow session ID above Navigation.goBack rather than below it.
Nothing can seed a draft synchronously during goBack, so this is behaviour-neutral, but
the comment already claimed the snapshot happened before navigating.
- Add the missing test for updateApprovalWorkflow's shouldClearApprovalWorkflowDraft:
the page-level test only asserts the flag is passed, so nothing exercised the
optimistic data actually omitting the SET null.
Co-authored-by: abzokhattab <abzokhattab@users.noreply.github.com>
Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
…erting The 'as ApprovalWorkflowOnyx' narrowing assertion tripped @typescript-eslint/no-unsafe-type-assertion. Co-authored-by: abzokhattab <abzokhattab@users.noreply.github.com> Co-authored-by: Abdelrahman Khattab <abzokhattab@users.noreply.github.com>
ReviewThe core of this PR is sound and the two mechanisms added last round are the right shape. CI was red when you asked; it's green now except the human-approval gate. Nothing here blocks merge on correctness — the open items are a stale description, one design call that needs your sign-off, and a few nits. Fixed while reviewingESLint was failing, and both errors were mine from the previous round. Pushed
26 of 27 checks pass. The one failure is Needs your decisionThe behaviour change still needs explicit sign-off. The PR body flags it and it's the biggest thing a reviewer has to agree to: Should fix before human reviewThe PR description is stale. Under Automated tests it still says "a failed validation calls neither Nits
Still unverifiedNobody has driven this flow in a browser on the current code. The web verification linked in the PR body predates the last three commits. I tried twice this session and both attempts died on session infrastructure — blank page, zero DOM nodes, no network activity at all, unrelated to this branch. The Jest coverage is good (268 tests across 9 workflow suites, including the superseded-save race and unmount teardown), but the actual |
|
@codex 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: 427c510829
ℹ️ 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".
| if (!approvalWorkflow?.isFastEdit) { | ||
| Navigation.goBack(backPath, {compareParams: false}); | ||
| return; | ||
| } | ||
| }, [route.params.policyID, selectedMembers, isInitialCreationFlow, backPath, policy?.employeeList]); | ||
|
|
||
| const workflowToSave = {...approvalWorkflow, members: allMembers}; |
There was a problem hiding this comment.
When a fast edit selects someone who is not yet a workspace member, the earlier usersToInvite branch returns before reaching this new save path. Because the +N more entry opens expenses-from without a nested backTo, WorkspaceInviteMessageComponent.sendInvitation() subsequently takes its no-nested-route branch and replaces the page with the Approver screen instead of invoking updateApprovalWorkflow; therefore the invite succeeds but the selected member is not assigned to this workflow unless the admin unexpectedly completes the full edit flow. The invite completion path needs to return to or directly perform the fast-edit save.
Useful? React with 👍 / 👎.
2 participants
Explanation of Change
Editing a workflow's "Expense from" members through the
+N morefast edit did nothing. The admin deselected someone, pressed Save, and the member was still there — after a reload and after clearing the cache.The fast edit never saved anything.
WorkspaceWorkflowsApprovalsEditPageis the only screen that works out which members were removed and callsupdateApprovalWorkflow, and the fast edit skips it:WorkflowsApprovalsTabopens the expenses-from page straight from the workflows page, souseDynamicBackPathresolves the parent to the workflows page and Save just wrote the Onyx draft and navigated back. Nothing ever reachedUpdateWorkspaceApproval, sopolicy.employeeList[email].submitsTowas untouched and the workflow re-rendered unchanged.This was a regression from #94482, which migrated the page to a dynamic route and dropped the explicit
backTothat used to send anEDITsession back to the edit page.The change makes the fast edit save its own work:
ApprovalWorkflowOnyxgainsoriginalMembers(the members counterpart of the existingoriginalApprovers) andisFastEdit, andselectApprovalWorkflowForEditpopulates both. The removed-members baseline previously lived only in the edit page's React state, so a sub-page entered directly had no way to compute it.WorkflowsApprovalsTabmarks the+N moresession withisFastEdit: true.nextStepnow saves whenisFastEditis set, callingupdateApprovalWorkflowRulesunder theMULTIPLE_APPROVERSbeta andupdateApprovalWorkflowotherwise. It validates before navigating, so a failed validation keeps the admin on the page with the footer alert showingapprovalWorkflow.errorsinstead of navigating away and silently dropping the change. The save itself is deferred withrunAfterPredictedTransitionso clearing the draft doesn't blank the list while the page is still sliding away.clearApprovalWorkflow()after either save path, and again if the session unmounts unsaved. Neither save action clears reliably —updateApprovalWorkflowRulesnever does, andupdateApprovalWorkflowonly clears once it reaches its optimistic data, which it skips when the employee diff comes out empty (pressing Save with no effective change on an already-saved workflow). Without this,isFastEdit: truesurvived in persisted Onyx after an apparently successful save.WorkspaceWorkflowsApprovalsEditPagedropsisFastEditwhen it resumes a draft, so a stale flag reaching the edit route via a refresh or deep link can't let the sub-page save and clear the draft out from under a mounted edit page.Adding a member that is already in the workspace was broken the same way and is fixed by the same change.
Important
Behavior change beyond the bug fix — please sign off on this explicitly.
With this PR, pressing Save in the
+N morefast edit returns the admin to the Workflows page and writes the change immediately. Before the regression in #94482, the same chip returned to the workflow Edit RHP, where the admin had to press Save a second time for anything to persist.The immediate write is what the linked issue's expected result asks for and it is what makes a one-step fast edit make sense, but it is a deliberate UX change and not just a restoration of the pre-regression flow. The alternative — restoring the explicit
backTothat #94482 dropped — would need no new Onyx fields, but it brings the two-step Save back.Known gap, not addressed here: picking someone who is not yet a workspace member still detours through the invite flow, which lands on the approver step instead of returning to the workflows page (
WorkspaceInviteMessageComponent), and that path still doesn't save. That was broken before this PR and needs its own fix — thanks toAnthggfor spotting it while proposing on the issue.Automated tests
tests/ui/DynamicWorkspaceWorkflowsApprovalsExpensesFromPageTest.tsx, covering:updateApprovalWorkflowwith the deselected member inmembersToRemove;MULTIPLE_APPROVERSbeta routes the save throughupdateApprovalWorkflowRuleswithoriginalMembersas the "before" side;updateApprovalWorkflowbails before its optimistic data);Navigation.goBacknor either save action, and leaves the errors on the draft;goBackis invoked before the save.selectApprovalWorkflowForEditcases totests/actions/WorkflowTest.tsfor the neworiginalMembers/isFastEditseeding, plus aclearApprovalWorkflowFastEditcase asserting the rest of the draft survives.AI tests run locally
npm run typechecknpm run lint-changednpm run spell-changednpm run fmt(oxfmt)npm run react-compiler-compliance-check checkon the changed pagesnpm test— all 9 workflow suites (260 tests)npm run lint(whole repo)lint-changedcovers every file in this diff.mainwas merged in cleanly and touched none of the files in this diff, so the merge carries no behavioral risk to the fix.Fixed Issues
$ #100408
PROPOSAL: #100408 (comment)
Tests
Setup: sign in as a workspace admin on a Collect or Control workspace with at least 8 members, and make sure Workflows > Add approvals is enabled.
Offline tests
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
Screenshots/videos will be attached in the reviewer checklist.
Melvin's automated runs of the test steps above are recorded on this PR:
Android: Native
See the Android verification comment linked above.
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
See the web verification comment linked above.