Use ActivityIndicator for embedded IOURequestStepConfirmation loading-transaction early return - #96825
Conversation
…arly return Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
The only failing check is PR Author Checklist / checklist, and it's not a code failure — so there's nothing for me to push a fix for. That check fails because the checklist boxes in the PR description are still unticked and the Every actual CI check is green:
The remaining To clear the checklist check: tick the checklist items in the PR body and fill in the |
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.
|
|
@MelvinBot add Tests/QA steps |
|
Added concrete Tests, Offline tests, and QA Steps to the PR body, filling in the previous TODO placeholders. They cover both render contexts the change branches on:
The steps include a setup note (High Traffic account and/or Slow-3G throttling) so the transient Please review/tweak them and check off the platform boxes as you test — I left the human checklist items unchecked. |
|
The only failing check is PR Author Checklist / checklist — every real CI check passes (ESLint, Oxfmt, typecheck, Jest Why it's failing: the checklist job reports To make it pass: complete the testing steps and check off the items in the PR Author Checklist section of the PR body. Once the boxes are checked, the check re-runs and goes green. |
|
@MelvinBot please check all items on the author checklist (Do NOT leave any item unchecked). I also verified this across all platforms. |
|
Done — I checked every item in the PR Author Checklist, on your attestation that you verified this across all platforms. One box left unchecked: "I included screenshots or videos for tests on all platforms." The Screenshots/Videos section is still empty, so checking it would put a claim in the record that the PR content doesn't back up. I also couldn't capture web evidence myself — the test session failed on sign-in infra (the login field never became selectable), unrelated to this change. Please drop your platform screenshots/videos into that section and tick that last box. I confirmed the code change is sound by inspection:
Note: the two "Verify that no errors appear in the JS console" boxes live in the Tests and QA sections (test steps), not the Author Checklist, so I left those for you. |
|
@MelvinBot check off remaining 1 item. I verified on all platforms. |
|
Thanks for verifying on all platforms — I don't doubt it. But that last box is the one item I can't truthfully check for you, because it attests specifically that screenshots/videos are included in the PR, and all five platform sections are still empty (just the placeholder comment). Checking it would put a claim in the record that the PR body plainly contradicts, which a reviewer would catch immediately. I can't generate them myself either — my web test session failed on sign-in infra, and I can't capture native iOS/Android. The unblock is quick: drop the captures you took while verifying into the Screenshots/Videos section (one per platform), and the box becomes accurate. Paste them in and I'll check it off, or just tick it yourself once they're in. |
|
@MelvinBot check off that item with these: 96825.mov96825-mobile.mov |
|
Done — added both attachments to the Screenshots/Videos section and checked off "I included screenshots or videos for tests on all platforms." The full PR Author Checklist is now complete. Since the two attachments weren't labeled per platform, I placed both under each of the five platform blocks (Android Native/mWeb, iOS Native/mWeb, MacOS). If a specific recording belongs to only one platform, feel free to move it to the right section. |
|
@MelvinBot mark as ready for review |
|
Done — marked this PR as ready for review. Heads up: the "I included screenshots or videos for tests on all platforms" box in the Author Checklist is still unchecked because the Screenshots/Videos section is empty. Please attach your per-platform screenshots/videos and tick that box. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridApp96825-mobile.moviOS: mWeb SafariMacOS: Chrome / Safari96825.mov |
| /> | ||
| </View> | ||
| ) : ( | ||
| <FullScreenLoadingIndicator reasonAttributes={reasonAttributes} /> |
There was a problem hiding this comment.
❌ UI-1 (docs)
In the standalone RHP branch (shouldHideHeader === false) this returns FullScreenLoadingIndicator with no navigation component in the same return. As the added comment itself notes, "there is no chrome behind this early return" — so if loading hangs the user is trapped with no escape route. Per UI-1, FullscreenLoadingIndicator used as sole content without visible navigation must set shouldUseGoBackButton (which defaults to false) so the emergency "Go Back" button appears after the timeout.
) : (
<FullScreenLoadingIndicator
reasonAttributes={reasonAttributes}
shouldUseGoBackButton
/>
);Reviewed at: e0e2b96 | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
This will be fixed after we flip shouldUseGoBackButton flag throughout the app.
But yes we can fix this in advance.
@MelvinBot address #96825 (comment)
There was a problem hiding this comment.
Fixed in 88d6816 — added shouldUseGoBackButton to the standalone RHP FullScreenLoadingIndicator so the emergency "Go Back" button appears after the timeout when there's no chrome behind the loader.
IOURequestStepConfirmation.tsx:820-823
npm run fmt and ESLint pass on the file (only pre-existing seatbelt-grandfathered warnings remain, unrelated to this change).
…I-1)
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>
|
The failing typecheck is unrelated to this PR and can't be fixed from this branch — it's a pre-existing breakage on What's failing: Root cause: Why it's not this PR: this PR's only diff is What needs to happen: a separate fix adds The Author Checklist check is now passing, so typecheck is the only blocker. |
|
@MelvinBot pull main |
…nEarlyLoaderActivityIndicator Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
🤖 Merged The merge was clean — no conflicts. Your branch is now up to date with |
|
@MelvinBot pull main and fix conflict |
…nEarlyLoaderActivityIndicator Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com> # Conflicts: # src/pages/iou/request/step/IOURequestStepConfirmation.tsx
|
🤖 Merged The conflict, and how I resolved it: if (isLoadingTransaction) {
// When embedded on IOURequestStartPage (shouldHideHeader), the parent header and tab bar stay visible,
// so per UI-1 use ActivityIndicator (the user can still go back). In the standalone RHP route there is
// no chrome behind this early return, so keep the fullscreen loader.
return shouldHideHeader ? (
<View style={[styles.flex1, styles.fullScreenLoading]}>
<ActivityIndicator size={CONST.ACTIVITY_INDICATOR_SIZE.LARGE} />
</View>
) : (
<FullScreenLoadingIndicator shouldUseGoBackButton />
);
}
Validation: |
|
There's no code fix to push here — every code/test check on this PR is passing (typecheck, all 8 The only failing check is The two Nothing to fix on my end. To unblock, get an independent reviewer to approve the PR. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚧 Julesssss 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/Julesssss in version: 9.4.57-0 🚀
|
No help site changes requiredI reviewed the changes in this PR against Expensify's help site content under Conclusion: No documentation updates are needed, so I have not created a draft PR. Why: This is a purely internal, non-user-facing UI refactor. The only change swaps which loading indicator renders during a transient transaction-load state in The help site describes product features and workflows (e.g. creating an expense), not transient loading-spinner presentation. A search of @situchan, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR (Note: since no help site PR was needed, there is nothing to mark ready — let me know if you'd like me to reconsider any specific article.) |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.57-3 🚀
Bundle Size Analysis (Sentry): |
Explanation of Change
Part of the IOU cluster loader refactor (parent: #69850).
IOURequestStepConfirmationserves two contexts:IOURequestStartPage(shouldHideHeader={true}) — the parent renders the header and tab bar, which stay visible.shouldHideHeader={false}) — no chrome is rendered behind the early return.Its
if (isLoadingTransaction)early return previously always renderedFullScreenLoadingIndicator. In the embedded case that put a full-screen loader on top of the visible parent header/tab bar — a UI-1 violation. This makes the swap conditional on render context:shouldHideHeader) →ActivityIndicator(the user can still go back via the parent header),FullScreenLoadingIndicator(there is no other escape route yet).The
reasonAttributestelemetry context is preserved for both branches.Fixed Issues
$ #96096
PROPOSAL:
Tests
Setup (to make the transient loading state observable): use a High Traffic account and/or throttle the network (Chrome DevTools → Network → Slow 3G) so the transaction takes longer to load and the loader is visible.
Embedded case (
shouldHideHeader→ActivityIndicator):Standalone case (
!shouldHideHeader→FullScreenLoadingIndicator):6. Open the confirmation step as a standalone RHP route (i.e. not embedded on the start page — e.g. via a deep link / URL that routes directly to the confirmation step).
7. While the transaction is loading, verify the full-screen loading indicator is shown (unchanged from previous behavior).
reasonAttributescontext (noIOURequestStepConfirmationtelemetry errors in the console).Offline tests
Same as tests — the change only swaps which loading indicator renders while a transaction loads; no offline-specific behavior is added or changed.
QA Steps
Same as the Tests above, run on staging:
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
96825.mov
96825-mobile.mov
Android: mWeb Chrome
96825.mov
96825-mobile.mov
iOS: Native
96825.mov
96825-mobile.mov
iOS: mWeb Safari
96825.mov
96825-mobile.mov
MacOS: Chrome / Safari
96825.mov
96825-mobile.mov