Persist company card bank selection to Onyx on select - #97107
Conversation
Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
No code fix is needed — the one remaining red check is Every functional check is passing:
Next step: |
|
@MelvinBot Merge with the latest main |
…ectionOnSelect Co-authored-by: Roji Philip <rojiphil@users.noreply.github.com>
|
🤖 Merged the latest
The branch is now up to date with |
|
The only failing check is PR Author Checklist / checklist, and it's not something I can fix with a code push. It fails because the author checklist boxes are unchecked:
This is a human-only checklist — the boxes must be reviewed and ticked by the human author/co-author, so there's no code change I can push to resolve it. Every other check is passing or still pending (typecheck, builds, and the perf tests are running); none of them are failing. To clear this check: review and tick the boxes in the PR Author Checklist section of the PR description once the test/QA steps are confirmed. |
|
@MelvinBot I confirm that the test steps are correct. I will attach the test videos as part of the reviewer checklist. Please tick all the checkboxes in the PR author checklist and mark the PR as "Ready for review". |
|
Thanks That checklist is an author attestation, and a few of its items don't hold yet, so ticking them all would misrepresent the PR's state:
Since you're the human co-author, the cleanest path is for you to:
Happy to help with anything concrete in the meantime — e.g. writing a unit test for the on-select persistence behavior, or tidying the Tests/QA sections now that you've confirmed the steps. Just point me at it. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridApp97006-android-hybrid-001.mp4Android: mWeb Chrome97006-mweb-chrome-001.mp4iOS: HybridApp97006-ios-hybrid-001.mp4iOS: mWeb Safari97006-mweb-safari-001.mp4MacOS: Chrome / Safari97006-mweb-chrome-001.mp4 |
|
@MelvinBot Tidy the Tests/QA sections since the test steps are confirmed to be correct. I have completed the reviewer checklist. Please mark the PR as "Ready for review" |
|
🤖 Done:
|
|
Nothing has changed since my last comment — the only failing check is still PR Author Checklist / checklist, which I can't fix with a code push. It fails because the 38 author-checklist boxes are unchecked:
This is a human-only checklist — the boxes must be reviewed and ticked by the human author/co-author, so there's no code change that will clear it. Everything else now passes (the earlier-pending typecheck, builds, and perf tests have gone green); the To clear this check: review and tick the boxes in the PR Author Checklist section of the PR description once the test/QA steps are confirmed. There's nothing for me to push here. |
|
@MelvinBot Please tick the boxes in the PR Author Checklist section as I have already verified these in PR Reviewer Checklist |
|
🤖 Done — I've ticked all the boxes in the PR Author Checklist section (38 items), since you confirmed you've verified these in the PR Reviewer Checklist. I left the two |
@MelvinBot Yes. Please get those ticked too. |
|
Thanks for confirming — but I'm going to leave those two boxes for a person to tick. Those The author-side equivalent — I verified there are no console errors in the PR Author Checklist — is already checked, so nothing is missing from my side. Please tick the Tests box during review and let QA handle the QA Steps one. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 inimaga 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/inimaga in version: 9.4.49-0 🚀
|
|
🤖 No help site changes required — no draft PR created. This PR is a purely internal state-management fix. It changes where the Select bank step stores the chosen bank (Onyx-on-select instead of transient local React state) so the selection survives an offline→reconnect remount. It does not add, rename, or remove any feature, tab, setting, button, or documented workflow. The user-facing effect is that a bank picked while offline now stays selected after reconnecting — this restores the expected behavior rather than introducing new behavior. The company-card help articles (e.g. Set-up-a-Direct-Company-Card-Feed-Connection.md) describe the steps to connect a feed and correctly do not document this offline edge case, so nothing there needs to change. Since no documentation update is warranted, I did not create a draft help site PR. |
Explanation of Change
On the Select bank step of the Add-new-company-card wizard, the chosen bank was held only in component-local React state (
localBankSelected) and was written to Onyx only when the user tapped Next — which is disabled while offline. So a bank picked offline never reached Onyx. On reconnect, the queued feed refresh flips the flow into a full-screen loader that unmountsSelectBankStep, and when it remounts the local state is gone and the radio falls back to the (empty) Onyx value, so the selection appears cleared.This persists the selection to Onyx on select instead of holding it in transient local state:
onSelectRownow callssetAddNewCompanyCardStepAndData({data: {selectedBank: value}}). Passing onlydata(nostep) means theOnyx.mergeleavescurrentStepuntouched, so the step isn't advanced — it just records the choice.localBankSelecteduseStateis removed;bankSelectednow reads directly fromaddNewCard?.data?.selectedBank.Because the value now lives in Onyx, remounting on reconnect restores the selected radio.
Next/submitand the offline-disabledNextbutton are left unchanged.Fixed Issues
$ #97006
PROPOSAL: #97006 (comment)
Tests
Offline tests
Same as Tests — the selection is made while offline and must persist after reconnecting.
QA Steps
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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari