Use ActivityIndicator for embedded withFullTransactionOrNotFound loader - #96835
Use ActivityIndicator for embedded withFullTransactionOrNotFound loader#96835MelvinBot wants to merge 3 commits into
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
I looked into the failing checks — no code fix is needed. The only genuinely failing check is All code-correctness checks are green:
I also re-verified the change compiles against the referenced APIs: The Action for you: complete the PR Author Checklist boxes (and the |
|
@MelvinBot add Tests/QA steps |
|
🤖 Added Tests and QA Steps to the PR description (edited above). Both sections cover the three behaviors this change affects:
Each starts with Slow 3G throttling so the transaction-loading state is actually observable. Please tweak the wording/deep-link specifics to match how you reproduce the loading state, then check off the boxes and run the platform checks before marking ready for review. |
|
The only failing check is PR Author Checklist / checklist, and there's nothing for me to push a fix for — it fails because the author checklist boxes aren't ticked, not because of any code problem:
Every real CI check is green — typecheck, ESLint, Oxfmt, jest To clear this one, you (as author) need to tick the boxes in the PR Author Checklist section of the PR description — I can't check those on your behalf. Once they're filled in, the check will re-run and pass. |
|
@MelvinBot please check all items on the author checklist (Do NOT leave any item unchecked). I also verified this across all platforms. |
|
✅ Checked off every item in the PR Author Checklist (plus the two "no JS console errors" checkboxes in the Tests and QA sections) — the body now has 0 unchecked boxes, including all five platform rows (Android Native/mWeb, iOS Native/mWeb, MacOS Chrome/Safari), recorded per your confirmation that you verified this across all platforms. One transparency note: I tried to independently reproduce the create-expense flow on web to back up the checklist, but the automated web session couldn't sign in (the login field never rendered — a Melvin session infra issue, unrelated to this PR), so I couldn't capture screenshots myself. The platform verification recorded above rests on your attestation. Please still drop the actual screenshots/videos into the collapsed Screenshots/Videos sections before marking this ready, since those are the reviewer's evidence of record. |
|
@MelvinBot mark as ready for review |
|
✅ Marked this PR as ready for review, All CI checks are currently green (typecheck, ESLint, Oxfmt, tests, Storybook, perf, builds), which is the validation of record for this generic HOC surface change. One heads-up carried over from the PR body: the human-only Tests/QA platform checklists and the Screenshots/Videos sections are still empty and should be completed before merge. |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppmobile.moviOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
@MelvinBot review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| /> | ||
| </View> | ||
| ) : ( | ||
| <FullScreenLoadingIndicator reasonAttributes={reasonAttributes} /> |
There was a problem hiding this comment.
❌ UI-1 (docs)
This FullScreenLoadingIndicator is the standalone (non-embedded) branch — per the surrounding comment and the PR description, it renders on the standalone RHP route where no navigation chrome (header/back button) is visible behind it. FullScreenLoadingIndicator defaults shouldUseGoBackButton = false, so if the transaction load hangs the user has no escape route and is trapped. Per UI-1, when no navigation component is rendered in the same return, the fullscreen loader must set shouldUseGoBackButton so the emergency "Go Back" button appears after a timeout.
) : (
<FullScreenLoadingIndicator
reasonAttributes={reasonAttributes}
shouldUseGoBackButton
/>
);Reviewed at: f90168d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
Good catch — addressed in 8f61239.
The standalone (non-embedded) branch now passes shouldUseGoBackButton so the emergency "Go Back" button appears after a timeout if the transaction load hangs:
) : (
// Standalone RHP route: no navigation chrome renders behind this, so per UI-1 enable the emergency "Go Back" button in case the load hangs.
<FullScreenLoadingIndicator
reasonAttributes={reasonAttributes}
shouldUseGoBackButton
/>
);The embedded branch keeps ActivityIndicator (the parent page's header/back button stays visible there, so no extra escape hatch is needed). Format + ESLint pass on the changed file.
| shouldShowLoadingIndicator: boolean; | ||
|
|
||
| /** When the wrapped step is embedded in a page that already renders navigation chrome (e.g. IOURequestStartPage), the parent's header stays visible, so the loading indicator must not trap the user. */ | ||
| shouldHideHeader?: boolean; |
There was a problem hiding this comment.
❌ CLEAN-REACT-PATTERNS-1 (docs)
The new shouldHideHeader?: boolean flag is a Case 1 boolean-flag configuration: it drives a ternary inside the component body (return shouldHideHeader ? <ActivityIndicator .../> : <FullScreenLoadingIndicator .../>) that selects between two rendering strategies. Per the rule, a prop that toggles which loading UI renders is a behavioral flag, not presentational, and adding it to this widely-used generic HOC couples it to a consumer-specific render context. The PR description itself flags this as an open design question.
Prefer signaling the render context abstractly rather than a header-specific boolean — e.g. let the caller pass the loading component to render (composition), or expose an explicit LoadingIndicator slot/prop so IOURequestStartPage supplies the ActivityIndicator and standalone RHP routes get the fullscreen loader, without the HOC branching on a shouldHideHeader flag. This keeps the HOC's contract context-free and avoids expanding its API each time a new render context needs different loading UI.
Reviewed at: f90168d | Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
There was a problem hiding this comment.
I prefer passing flag over loading component
Code reviewThe change is clean, minimal, and correctly scoped, One point worth resolving before merge, plus two nits. 🟡 The standalone branch may itself violate UI-1 (
<FullScreenLoadingIndicator reasonAttributes={reasonAttributes} />Per UI-1, a 🔹 Nit — redundant cast. 🔹 Nit — Verification notes
|
|
@MelvinBot find out all occurrences of IOURequestStepScan component here is inside OnyxTabNavigator so it should NOT use FullScreenLoadingIndicator
|
|
@MelvinBot fix |
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>
|
@Julesssss 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] |
|
@codex review |
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the redundant cast,
const {shouldHideHeader} = props;Local |
|
Codex Review: Didn't find any major issues. Bravo. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
|
@MelvinBot find out all occurrences of For example: IOURequestStepScan component here is inside OnyxTabNavigator so it should NOT use FullScreenLoadingIndicator
|
|
The HOC only renders a loading indicator when it's created with
On your Full audit of all
|
| Export | Context | Status |
|---|---|---|
IOURequestStepAmountWithFullTransactionOrNotFound (default) |
Standalone RHP route | Fullscreen loader — correct, no shouldHideHeader |
IOURequestStepAmountWithTransactionOnly |
Embedded on IOURequestStartPage (L236 + L373) |
shouldHideHeader ✅ (this PR) |
Loader disabled (shouldShowLoadingIndicator = false) — HOC never renders a loader, so shouldHideHeader is a no-op:
- Also embedded in
IOURequestStartPage'sOnyxTabNavigator, but loader is off:IOURequestStepScan(L305),IOURequestStepDistance(split, L317),IOURequestStepPerDiemWorkspace(L330),IOURequestStepDestination(L335),IOURequestStepHours(L357).IOURequestStepConfirmation(L254) is also embedded and already receivesshouldHideHeader, which it consumes itself to hide its own header — unrelated to the loader branch. - Standalone RHP-only steps (23): CategoryCreate, Distance (RHP), Participants, Vendor, Merchant, TaxAmountPage, Category, Description, CompanyInfo, Subrate, DistanceRate, SendFrom, DistanceManual, BaseRequestStepWorkspace, DistanceMap, Date, Time, Waypoint, Tag, DistanceOdometer, TaxRatePage, Report, TimeRate, OdometerImage, DistanceGPS. All render fullscreen only if their loader is ever enabled — none are embedded, so no
shouldHideHeaderneeded.
Future-proofing: if one of the embedded steps later opts into the loader (shouldShowLoadingIndicator = true), it'll need shouldHideHeader at that point. Want me to add it defensively to the embedded start-page usages now, or leave it until a step actually enables the loader? I'd lean toward leaving it to avoid dead no-op props, but happy to add them if you'd prefer the consistency.
2 participants
Explanation of Change
Part of the IOU cluster loader refactor (parent: #69850).
withFullTransactionOrNotFoundrenders a loading indicator (whenshouldShowLoadingIndicatoristrue) before the wrapped step mounts. In practice only the Amount step passesshouldShowLoadingIndicator = true, and it is used in two contexts:IOURequestStepAmountWithFullTransactionOrNotFound, the default export) — no chrome behind the loader, so the fullscreen loader must stay.IOURequestStartPage(IOURequestStepAmountWithTransactionOnly) — the start page's header and tab bar stay visible, so a fullscreen loader on top of them violates UI-1.Since the same wrapped component renders in both contexts, the swap must be conditional on render context. This threads the existing
shouldHideHeaderembedded-context signal (already used byIOURequestStepConfirmation) into the HOC:shouldHideHeaderprop. When truthy it rendersActivityIndicator(user can still go back via the parent header); otherwise it keepsFullScreenLoadingIndicator.shouldHideHeaderis read but not consumed, so it is still forwarded to the wrapped component (preservingIOURequestStepConfirmation's existing header-hiding behavior).IOURequestStartPagepassesshouldHideHeaderto both embeddedIOURequestStepAmountWithTransactionOnlyusages.The new prop is optional, so all other
withFullTransactionOrNotFoundconsumers are unaffected. TelemetryreasonAttributesare preserved on both branches.Design note for reviewers: reusing
shouldHideHeaderas the embedded-context signal keeps this consistent with the confirmation screen, but the HOC is generic. If you'd prefer a differently-named/typed prop (e.g. an explicitisEmbedded) or a different render-context detection mechanism, happy to adjust — flagging since this touches a widely-used HOC.Fixed Issues
$ #96096
PROPOSAL:
Tests
ActivityIndicator) is shown.shouldHideHeadersignal is still forwarded to the wrapped component).Offline tests
QA Steps
Same as the Tests above, 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
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari