[Payment due @situchan] Refactor BusinessInfo TaxIdBusiness loader to ActivityIndicator - #96833
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
The only failing check is PR Author Checklist / checklist — that's not a code failure and there's nothing for me to fix in the branch. It fails because the human checklist isn't filled out yet: the job reports "You completed 0 out of 38 checklist items", and the Every real CI check passes ✅ — Bun tests, Jest To turn the checklist check green, the human co-author needs to:
Those are author attestations, so I can't complete them on your behalf. |
|
@MelvinBot add Tests/QA steps |
|
Added Since this PR swaps the loader in the Business info → Tax ID (EIN) substep of the USD Reimbursement Account setup ( Key points baked into the steps:
Please review the wording and adjust anything that doesn't match how you reach this substep in your setup before marking the PR ready. The |
|
The only failing check is It fails because the checklist is unchecked, not because of anything in the diff:
Every other check on this PR is green (tests, typecheck, lint, ESLint, Storybook, build, perf, etc.). This one is a human gate — I can't tick these boxes on your behalf, since several of them attest to manual testing on all platforms (Android/iOS native, mWeb, desktop), screenshots/videos, and console-error verification that a person has to actually perform. To clear it: complete the manual |
|
Re-checked — the status is unchanged. The only failing check is still It fails purely because the checklist boxes aren't ticked ( This is a human gate — I can't check these boxes for you, since they attest to manual testing you have to actually perform (native/mWeb/desktop, screenshots, console-error checks). Complete the manual |
…stifyContentCenter
Co-authored-by: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"} <{"message":"Not Found","documentation_url":"https://docs.github.com/rest/issues/comments#get-an-issue-comment","status":"404"}@users.noreply.github.com>
|
Nothing changed since my last comment — the only red check is still This is a human gate: it attests to manual cross-platform testing, screenshots, and console-error checks that a person has to actually do. Complete the |
DetailsExplanation of ChangeThis PR refactors a single Reimbursement Account step-machine loading state from Fixed Issues$ #96093 Tests
Offline testsN/A QA StepsN/A
PR Author Checklist
Screenshots/Videos5-TaxIdBusiness.mov5-TaxIdBusiness-mobile.mov@MelvinBot please set the PR body to the content in the details section above |
|
|
|
@MelvinBot please check all items on the author checklist. I also verified this across all platforms. |
|
Done, One heads-up: the Screenshots/Videos section is still empty placeholders. Your earlier comment included two video attachments — if you want those dropped into the platform sections, reply and I'll add them. |
|
@MelvinBot mark as ready for review |
|
🤖 Done — this PR is now marked Ready for review. A reminder for the human co-author before merge: the |
|
@jasperhuangg 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] |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp5-TaxIdBusiness-mobile.moviOS: mWeb SafariMacOS: Chrome / Safari5-TaxIdBusiness.mov |
|
🎯 @situchan, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
PR doesn’t need product input as a refactor PR. Unassigning and unsubscribing myself. |
|
🚧 jasperhuangg 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/jasperhuangg in version: 9.4.51-0 🚀
|
|
🤖 I reviewed the changes in this PR against Expensify's help site content under No help site changes are required. This PR is a purely internal, presentational refactor: it swaps the loading state in the Business info → Tax ID number substep from It does not change any feature name, tab, setting label, button, workflow, or user-facing behavior that the help site documents — it only alters how a transient loading spinner is rendered. Help articles describe how to connect a business bank account and complete the setup steps; none of them document loading-indicator appearance, so there's nothing to update. Since no changes are required, I did not open a draft docs PR. @situchan, let me know if you'd still like a docs update and I'll create one. |
|
🤖 Payment issue created: #98180 |
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.4.51-1 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Part of a series of PRs for #96093 — one PR per occurrence, as requested in the issue.
This PR refactors a single Reimbursement Account step-machine loading state from
FullScreenLoadingIndicatorto a centeredActivityIndicator(see the PR title and the one-file diff for the specific site). These pages render their loading state insideInteractiveStepWrapper, which shows the step header and back button. The previousFullScreenLoadingIndicator(an early-return in a substep, or a branch of the wrapper's children) rendered an absolute-fill overlay that painted over the visible back button while the reimbursement account data loaded — this violates UI-1. Wrapping anActivityIndicatorin a centeredViewkeeps the header/back button visible during loading and preserves the existingreasonAttributesskeleton-span telemetry. This mirrors the already-mergedBankInfostep, which uses the same pattern, and is in preparation for making the error/loading page recoverable (enabling the Go Back button by default).AI checks run locally
npm run typecheck-tsgo— passednpm run lint-changed— passednpm run react-compiler-compliance-check check <file>— passedThe full Jest suite and Storybook smoke test were not run: no unit/UI test renders these substep components, and the change is a like-for-like loader swap following an existing merged pattern. The human co-author should complete the manual
Tests/QAsteps below.Fixed Issues
#96093
PROPOSAL:
Tests
<) button remain visible — the spinner must NOT cover the back button with a full-screen overlay.Offline tests
QA Steps
<) button stay visible and are not covered by a full-screen overlay.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