Fix Wallet bank account setup progress not being preserved - #100648
nabi-ebrahimi wants to merge 42 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@mkhutornyi 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] |
![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: 61c0203593
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@mkhutornyi, I’ve addressed all AI review feedback, including the related CI lint issues. Could you please review the latest changes when you have a chance? thanks! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02042a6e7c
ℹ️ 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 hasMatchingCorpayFields = | ||
| corpayFields?.bankCountry === draftValues?.bankCountry && | ||
| (!draftValues?.bankCurrency || corpayFields?.bankCurrency === draftValues.bankCurrency) && | ||
| !!corpayFields?.formFields?.length; |
There was a problem hiding this comment.
When a saved Wallet international-personal draft is followed by a non-USD business setup for the same country and currency, getCorpayBankAccountFields() overwrites the shared ONYXKEYS.CORPAY_FIELDS entry with the business/withdrawal schema. This predicate considers that entry compatible based only on country, currency, and a nonempty field list, so reopening the personal flow skips the refresh and renders—and can submit—the business field schema. Include the account purpose/request mode in the compatibility check, or always refresh the personal schema when resuming.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@mkhutornyi, Quick confirmation before I update this: would you prefer the Wallet international personal resume to always refresh the personal Corpay fields, or should we avoid the extra loading state by adding an explicit personal/business compatibility check for cached CORPAY_FIELDS?
Always refreshing fixes the stale business-schema case, but it can briefly show a loader on each international resume.
There was a problem hiding this comment.
could you attach videos so I can compare?
There was a problem hiding this comment.
Current PR status:
Screencast.From.2026-09-17.18-14-22.mp4
Always refreshing fixes:
Screencast.From.2026-09-17.18-18-10.mp4
There was a problem hiding this comment.
I prefer avoiding extra loading state.
There was a problem hiding this comment.
Addressed. Compatible personal Corpay fields are now reused, avoiding the extra loading state, while incompatible business fields are refreshed. You can see the updated behavior in the demo below.
Screencast.From.2026-09-21.16-08-41.mp4
The same behavior also exists on main:
Screencast.From.2026-09-21.16-09-51.mp4
|
🤖 Code review — one blocker, plus a few things worth a second look. 🔴 Wallet marker overwrites entry context for non-Wallet flows
This screen is not Wallet-only. Both of these land here:
Failure: open "Add bank account" from a report, pick a country on this screen, finish the setup. Preserve what was seeded instead of hardcoding the marker — 🟡 The Corpay "wait for refreshed fields" gate doesn't wait
🟡 Business resume has no Wallet marker
The workspace flow normally clears the draft on unmount, but the unmount guard skips clearing when Smaller things
CI: The view run · no recording available |
Hi,@mkhutornyi, I plan to address the two behavior changes identified in the AI feedback: preserve workspace-owned business setup progress when switching to Get reimbursed, and consistently use the real account-validation state across both Wallet entry points. Could you please confirm this approach before I update and push the changes? |
|
@mkhutornyi, Gentle bump. thanks. |
Just follow the production behavior |
|
Hi, @mkhutornyi I’ve addressed all the feedback and ensured that the updated flow remains consistent with production behavior. Could you please review the changes again when you have a chance? thanks! |
|
🤖 Code review of 🔴 1. A failed Corpay fetch traps the user on a spinner with no way out
Worst case is a lock-out that survives restarts:
The only code that clears the poisoned draft lives inside the component that no longer renders. A non-admin has no other route to it. Retry-exhaustion is worse still: Fix: add 🔴 2.
|
|
🤖 Code quality review of The logic is sound and the comments are genuinely good — mostly why, not what, with no jargon. The problems are concentrated in three places: the new hook over-subscribes, the same call block is pasted three times, and a big chunk of resume arithmetic lives inline in a component where nothing can test it. Top 51. The new hook subscribes to four whole Onyx keys to serve an
|
|
Hi @mkhutornyi! I’ve completed a thorough pass on the Wallet bank-account resume behavior, fixed the issues I found, and addressed Milven’s feedback. The latest updates cover:
Draft persistence is limited to the relevant Wallet edit flows, leaving unrelated flows unchanged. I also added focused regression coverage for these scenarios, and all affected tests pass. Could you please review the latest changes when you have a chance? Thank you! |
|
🤖 Re-review of I couldn't test in the browser. Add bank account asks for a magic code that the test account can't receive, so everything below comes from reading the code. 🔴 1. In Wallet edit mode, Back no longer cancels an edit
That overrides Now every keystroke is saved to the draft. Back ( Repro: Wallet → Make payments → US → Business info confirmation → edit Tax ID → type Fix: Save the edit progress somewhere other than the live draft. Or discard the unsubmitted edit when Back is pressed in edit mode. 🟡 2. The saved US personal page is overwritten while data loadsWhile The persistence effect ( Fix: Skip the effect when 🟡 3. Workspace accounts are no longer cleared after Continue → ValidationThis is in shared code. When the user reopens the same workspace, the early return at line 443 skips Fix: Set the flag only for Wallet setups, or reset it when the page regains focus or the Validation screen unmounts. Lower-severity items, and status of my earlier findingsLower severity
Earlier findings: all fixed
Checked and cleared: Browser test attemptAfter onboarding, Wallet → Add bank account opened a "Validate your account" magic-code screen, so no step of the flow could be reached. 
Reply with |
| const {shouldSetTouchedOnBlurOnly, submitBehavior, shouldSubmitForm} = computeComponentSpecificRegistrationParams(props as InputComponentBaseProps); | ||
| const {key, ...registerInputProps} = registerInput(inputID, shouldSubmitForm, {ref, valueType, ...rest, shouldSetTouchedOnBlurOnly, submitBehavior}); | ||
| const {key, ...registerInputProps} = registerInput(inputID, shouldSubmitForm, { | ||
| ref, | ||
| valueType, | ||
| ...rest, | ||
| ...(shouldPersistDraft ? {shouldSaveDraft: true} : {}), | ||
| shouldSetTouchedOnBlurOnly, | ||
| submitBehavior, | ||
| }); |
There was a problem hiding this comment.
Bug: draft-persistence context overrides the "discard unless confirmed" convention
The confirmation step shows the updated value even after edit is discarded
Screen.Recording.2026-09-30.at.11.17.01.AM.mov
|
🤖 Regression check of Regressions
Lower-severity note and what traced cleanLow: abandoned Wallet business data now persists. On Traced clean:
Browser check (web): address form passes; bank flows blocked by magic code
Reply with |
This branch has not been deployed
4 participants
Explanation of Change
This PR preserves bank account setup progress when the Wallet modal is dismissed unintentionally.
For US personal bank accounts, it uses the existing
PERSONAL_BANK_ACCOUNT.sourcefield to identify setups started from Wallet and stores the last visited page incurrentPage. When the flow is reopened, the app waits for the relevant Onyx data to load before deciding whether to resume the existing setup or start a new one.For international personal bank accounts, it preserves the existing draft and refreshes the required Corpay fields without overwriting saved values.
For business bank accounts, it identifies Wallet setups using the `backTo`` route. This prevents an abandoned Wallet setup from affecting a bank account flow opened from Workspace settings.
Starting a new or incompatible setup clears stale data. Saved progress is also cleared after successful completion or when the user explicitly leaves from the first page.
This PR also:
Onyx.connectWithoutViewsubscriptions.Fixed Issues
$#95279
PROPOSAL:#95279 (comment)
Tests
US personal account — manual connection
US personal account — Plaid connection
International personal account
USD business account
Non-USD business account
Explicit exit
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
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
Android: Native
N/A
Android: mWeb Chrome
N/A
iOS: Native
N/A
iOS: mWeb Safari
N/A
MacOS: Chrome / Safari
Screen.Recording.2026-09-09.at.2.36.53.PM.mov
Screen.Recording.2026-09-09.at.2.39.13.PM.mov
Screen.Recording.2026-09-09.at.2.44.36.PM.mov
Screen.Recording.2026-09-09.at.2.47.29.PM.mov
Screen.Recording.2026-09-09.at.3.05.24.PM.mov
Screen.Recording.2026-09-09.at.3.11.24.PM.mov