fix: bank account not shown after added to workspace until page refresh#79577
fix: bank account not shown after added to workspace until page refresh#79577cead22 merged 7 commits intoExpensify:mainfrom
Conversation
|
@aimane-chnaif 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] |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b918155860
ℹ️ 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".
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8214a604c7
ℹ️ 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".
| isLoadingWorkspaceReimbursement: true, | ||
| reimburser: reimburserEmail, | ||
| achAccount: {reimburser: reimburserEmail, bankAccountID}, | ||
| achAccount: {reimburser: reimburserEmail, bankAccountID, accountNumber, addressName, bankName, state}, |
There was a problem hiding this comment.
Avoid overwriting existing achAccount fields with undefined
This optimistic merge now always writes accountNumber, addressName, bankName, and state into policy.achAccount. Call sites like WorkspaceWorkflowsPage invoke setWorkspaceReimbursement without these fields, so the merge will overwrite existing values with undefined, and because the success payload doesn’t restore them (and the API response doesn’t yet return achAccount), the bank account details/last‑4 can disappear after toggling reimbursement until a full policy refresh. Consider only including these keys when defined or merging from the existing policy.achAccount to avoid wiping the data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Hi, @aimane-chnaif
I am pretty unsure about this one. Could you please help?
There was a problem hiding this comment.
the API response doesn’t yet return
achAccount
Genius or stupid? Bot already tested api response 😄
For safety, let's add those fields only when exist, instead of undefined
There was a problem hiding this comment.
What should we do if not exist? The bankAccountID already existed the same way without checking undefined.
There was a problem hiding this comment.
We can skip this warning but instead please make sure to test workspace workflows page to confirm it doesn't cause any regression.
|
Hi, @aimane-chnaif Screen.Recording.2026-01-14.at.9.52.18.PM.movHowever, the above behaviour is expected as we now add |
|
Wait, I retested switching back to online after adding the BA when offline and the behavior on this branch looks more promising than staging or latest main. Screen.Recording.2026-01-14.at.9.59.30.PM.movAlso on staging, the |
|
Sounds good to me. Please merge main |
|
Done 👍 |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppandroid.movAndroid: mWeb Chromemchrome.moviOS: HybridAppios.moviOS: mWeb Safarimsafari.movMacOS: Chrome / Safariweb.movweb-offline.mov |
|
Hi @heyjennahay, we are waiting for you to review this PR. Thanks :) |
|
Friendly bump! @heyjennahay :) |
|
Can we merge this PR now? |
|
@rohit9625 @aimane-chnaif why do we show a spinner in these flows? I thought with optimistic data we wouldn't show a spinner, and the UI would update instantly. Similarly, when offline and we make the update, the update happens instantly, but when going back online we see a spinner again. Can we get rid of it? |
|
This one |
|
Hi @cead22, I looked into the code, and we have to remove the code that sets App/src/pages/workspace/workflows/WorkspaceWorkflowsPage.tsx Lines 310 to 315 in 3f9857e Only this way, we can avoid showing the indicator. Although I'm still not having a good feeling about making this change, as it was added for a reason. I understand we now update the BA in the optimistic data and showing loading indicator won't make sense. The |
|
Friendly bump! @cead22 on the above comment. |
|
Hi @cead22, I've removed the loading indicator as you suggested. This is the behavior right now: Screen.Recording.2026-01-30.at.12.44.19.PM.movPlease take a look and possibly try to merge if everything seems fine, @cead22 @aimane-chnaif |
|
Video looks great. @aimane-chnaif can you please give it another review? |
|
Friendly bump! @aimane-chnaif |
|
Hi @aimane-chnaif, please take a look at this PR :) |
|
While completely removing loading indicator makes our issue better, I think there's side effect. I'm afraid QA will mark it as regression. (this is just one example. there might be other cases which might confuse user) Screen.Recording.2026-02-03.at.4.31.02.pm.movAs such, can we restore loading indicator back? |
|
That looks fine to me actually |
|
Are we not supposed to show partially setup bank accounts, @aimane-chnaif? |
|
ok then we can proceed. Please fix conflict. |
We should show them |
Done 👍 |
|
🚧 @cead22 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! 🧪🧪
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by https://github.com/cead22 in version: 9.3.16-0 🚀
|
|
🚀 Deployed to production by https://github.com/lakchote in version: 9.3.16-9 🚀
|
Explanation of Change
The bank account wasn't displayed when added to a Workspace because we were only adding two fields to the
policy.achAccountoptimistic data. So, I updated thesetWorkspaceReimbursementfunction to add other fields to theachAccountthat are required to show the added bank account right away, without waiting for the backend response. However, the backend is also updated by cead22 to return theachAccountfield, and it will be deployed soon.Fixed Issues
$ #78459
PROPOSAL: $ #78459 (comment)
Tests
Precondition: Create a workspace, at Workflows payments, add bank account Plaid Saving Ending in 1111
Offline tests
None, we cannot add a bank account when offline, as the
Add bank accountbutton is disabled.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 stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, 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.ScrollViewcomponent to make it scrollable when more elements are added to the page.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_Native.mov
Android: mWeb Chrome
Android_Chrome.mov
iOS: Native
iOS_Native.mov
iOS: mWeb Safari
iOS_Safari.mov
MacOS: Chrome / Safari
Mac_Safari.mov