Repository navigation
Enable the Go Back button by default on FullScreenLoadingIndicator - #99786
Conversation
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
melvin-bot
Bot
added
Melvin-Test-Android
Melvin-Test-Web
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Android test results: pass (3/3)Ran the three suggested test steps from the PR body on standalone Android NewDot. The default One gap:
No console errors observed. Evidence (5)FullScreenLoadingIndicator showing 'This is taking longer than expected…' and Go back button after timeout
Screen after tapping Go Back, confirming real navigation away from the loader
Home screen immediately after ValidateLoginPage deep link — no stray Go Back button
Home screen immediately after UnlinkLoginPage deep link — no stray Go Back button
SessionExpiredPage after 11s on the shared transition/LogOutPreviousUserPage route — no Go Back button
Session noteCI's three sign-in attempts failed with |
Web test results: partial — 0/3 verified liveI could not reproduce any of the three test steps on web. Every route to them needed either a fresh page load / deep link (the harness must not reopen or reload the session) or a magic-code "Validate your account" screen, which I can't solve. So this is a blocked run, not a passing one — treat the web platform as still needing manual coverage. Static code review supports the change on all three points, but that is not a substitute for the visual proof you asked for.
For contrast, the same three steps did pass live on Android — see my earlier comment for those screenshots. No console errors. The session was left clean (Force offline turned back off, Home loading normally). Evidence (3)Troubleshoot page after enabling Force offline (attempted lever for a stuck loader)
Troubleshoot page confirming Force offline restored to off (cleanup)
Home screen loading normally after cleanup, confirming session left in a healthy state
|
|
@MelvinBot pull main and see #96098 (comment) |
…ttonByDefault Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
…pt-in prop - tests/ui/FullScreenLoadingIndicatorTest.tsx: covers the flipped default, the timeout boundary, the goBack() press, the false opt-out, and unmount cleanup. - tests/ui/LoadingIndicatorGoBackOptOutTest.tsx: mounts the real auth and OldDot/NewDot transition screens plus the delegate-connect gate and asserts no Go Back button ever appears, with an ordinary loader as the positive control. - tests/ui/ValidateLoginPageTest.tsx: same assertion for the web /v/ variant. - DomainNameOrNotFoundWrapper landed on main after the flip with an explicit shouldUseGoBackButton, which is now redundant. Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Pulled
Tests added
That is the selection Checks
Browser verification — still blocked, second run in a row🤖 Web test steps verification results: ❌ fail The
The new unit tests now cover steps 1, 3 and 4 in CI, which removes most of what the blocked browser run was meant to prove. Step 2 (no stray button over an existing header) is the one gap that still needs a human eye — it depends on the bullet-1 |
|
There's nothing to fix — I didn't push anything. Every code check on this PR passes: ESLint, Oxfmt, typecheck, React Compiler Compliance, spellcheck, Bun tests, all 8 Jest shards, Storybook, unused styles, knip, perf tests, and all 4 native builds. The one red check is Because you have commits on the branch, you don't count as an independent approver, and the PR currently has zero reviews. This check will stay red until someone who has not committed here approves the PR. No code change can clear it. Two other things still gate the merge, both needing you rather than me:
|
|
@MelvinBot pull main and fix conflict |
…ttonByDefault Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com> # Conflicts: # src/components/FullscreenLoadingIndicator.tsx # src/pages/LogInWithShortLivedAuthTokenPage.tsx
|
Merged
ESLint passes on both files, and typecheck shows no errors in them. I couldn't run Jest here because the sandbox setup fails on every UI test, including ones this PR doesn't touch. CI will run the tests. Note: new redundant opt-ins from mainMain added five callers that pass |
…on mock Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the failing Cause: The latest merge from Fix: I added
|
|
@MelvinBot Post comment here with each reason for each usage of |
|
I checked all 88 usages that don't pass
Two facts explain why the rest are safe:
Unless noted, each loader below replaces the whole screen (no header behind it) and nothing is in flight while it shows. The button only appears after 10s. Reason for each of the 85 usages that keep Go BackWallet, payments, bank accounts
Expenses, search, merge, tasks, reports
Profile, subscription, travel, agents Domains
Workspaces Optional cleanups
These findings come from reading the code. I didn't test them in a browser. Reply |
|
@MelvinBot there are redundant |
…true Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
I removed the redundant
No other Typecheck passes. ESLint reports only older errors on lines this change doesn't touch. |
|
MelvinBot has made 6 consecutive commits trying to fix CI failures. Automatic CI fix dispatches are now paused for this PR. Next steps: A human should review the recent commits and resolve the conflicting CI errors manually. |
|
@MelvinBot implement 2nd part of Optional cleanups in #99786 (comment) |
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
I pushed the second optional cleanup in 2e46c37. In these four pages, the loader's Go Back button now runs the same handler as the page's header back button:
Typecheck and the loader Jest tests (12 of 12) pass. ESLint and the React Compiler check found no new errors. Their remaining errors were already there before this commit, on lines I didn't change. I didn't test this in a browser. |
@MelvinBot Good catch! Find all occurrences throughout the app and fix them |
…rappers
Central-pane headers hide their back button on wide layouts, so the loader's
Go Back there would leave the section or do nothing after a cold load. Match
the header with shouldUseGoBackButton={shouldUseNarrowLayout}. RHP screens
still get Go Back because shouldUseNarrowLayout is true inside the RHP.
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Addressed all three review comments in 66c5a4d. Each "Then" comment in Only comments changed. ESLint passes on the file, and I replied in each thread. |
JmillsExpensify
left a comment
JmillsExpensify
left a comment
There was a problem hiding this comment.
No product review required on this one.
|
🚧 Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
This comment has been minimized.
This comment has been minimized.
|
Testing well for me. There's a lot of possible cases so we'll have to see what comes up during QA testing. |
|
@MelvinBot pull main, fix conflict |
Julesssss
left a comment
Julesssss
left a comment
There was a problem hiding this comment.
Added a couple of tests.
…ttonByDefault Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com> # Conflicts: # src/pages/inbox/report/withReportOrNotFound.tsx
|
Merged
Typecheck shows no errors in the touched files. ESLint reports three |
Julesssss
left a comment
Julesssss
left a comment
There was a problem hiding this comment.
Two last things:
- I think FullscreenLoadingIndicator should also be opted out,
setIsLoaderVisible(false)hides it, else 'Go Back' would leave it stuck on top (I think) - Please merge main and resolve conflict
You meant FullScreenLoaderContext? It's already fixed in #103422 |
Oh gotcha, yeah! |
|
@situchan remaining test failure is related to changes: https://github.com/Expensify/App/actions/runs/37955224958/job/113903922332?pr=99786#step:6:371 |
Co-authored-by: Situ Chandra Shil <situchan@users.noreply.github.com>
|
Fixed the failing Cause: The latest Fix: I added All 3 of this PR's test files pass locally (25 tests), and ESLint passes. |
Fixed |
|
🚧 Julesssss has triggered a test Expensify/App build. You can view the workflow run here. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
5 participants
Explanation of Change
Final step of the recoverable-loader work: flips
shouldUseGoBackButtonto defaulttrueonFullScreenLoadingIndicatorso every fullscreen loader grows a "This is taking longer than expected" message plus a "Go Back" button afterCONST.TIMING.ACTIVITY_INDICATOR_TIMEOUT. This delivers the recoverable error page across all remaining trapped-user loaders in one change instead of ~66 one-line PRs.Three parts:
src/components/FullscreenLoadingIndicator.tsx— default changed fromfalsetotrue, and the prop doc updated to say what passingfalseis for.shouldUseGoBackButtonprops from the call sites that were already opting in. Behavior is unchanged at these sites; the prop is just noise now.shouldUseGoBackButton={false}only where going back is dead or harmful, each with a comment explaining why:LogOutPreviousUserPage— actively harmful. Going back can pop/transitioninto the outgoing user's session midOnyx.clear()/sign-out.DelegatorConnectGate— dead. The Suspense fallback replaces the whole AuthScreens navigator whileconnect()clears Onyx, so there is no screen to go back to.FullScreenLoaderContext— the global overlay is not a screen. Going back would pop the unrelated screen underneath and leave the overlay up.ValidateLoginPage,UnlinkLoginPage,LogInWithShortLivedAuthTokenPage) keep the button. With nothing to pop,Navigation.goBack()resets to the app root (the sign-in page), so it is a real escape from a hung sign-in.Where a page's header goes back to a specific route (
backPath,backToor a fixed route), the loader passes the same target asonGoBack, so both back buttons land in the same place.This flip was blocked on the bullet-1 refactors (#96093, #96094, #96095, #96096), which converted sibling-of-header loaders to
ActivityIndicator. All four are now closed and the last PRs (#96835, #96819) are merged, so the flip no longer draws a stray "Go Back" button on top of headers that already have one.AI tests run locally by MelvinBot
eslinton all 20 changed fileseslint-seatbeltwarnings, untouched)npm run typecheckoxfmt(npm run fmt)npm run spell-changednpm run react-compiler-compliance-check checkDelegatorConnectGate.tsx,withReportOrNotFound.tsx,IOURequestStepConfirmation.tsx) — verified byte-for-byte identical failures onorigin/main, so pre-existing and not caused by this PRnpm test --findRelatedTests(118 suites, 1353 tests)tests/unit/pages/HomePage.test.tsxfails — the identical failure reproduces onorigin/mainwith the same batch, and the suite passes in isolation on both branches, so it is a pre-existing test-isolation flaketests/ui/ValidateLoginPageTest.tsx,tests/ui/WorkspacePageWithSectionsTest.tsx(the two suites that reference this component)Browser verification could not be completed. The
agent-deviceweb session rendered a permanently blank page (0 DOM nodes) for the whole run, despite the rsbuild dev server compiling cleanly with no errors. This is an environment/session-handoff failure, not a signal about this change — but it means the loader timeout and "Go Back" click were not exercised in a real browser. Please cover that in theTestssection below.Evidence (1)
Blank white page in the web test session - agent-device session never rendered app UI
Fixed Issues
$ #96098
PROPOSAL:
Tests
// TODO: The human co-author must fill out the tests they ran before marking this PR as "ready for review".
// Suggested coverage, since MelvinBot's browser run was blocked by an environment failure:
// 1. Reach any slow fullscreen loader (e.g. a report deeplink that stays loading) and verify "This is taking longer than expected" + "Go Back" appear after the timeout, and that "Go Back" actually navigates away.
// 2. Verify no stray "Go Back" button is drawn on top of a header that already has one.
// 3. Cold deep-link a magic link (validate, unlink login, or short-lived auth token) while offline and verify "Go Back" appears and returns you to the sign-in page.
// 4. Verify NO "Go Back" appears on the log-out-previous-user transition, the delegator connect loader, or the global overlay loader (e.g. while the attachment picker is open).
Offline tests
// TODO: The human co-author must fill this out. Offline is the most likely way to reach a long-lived loader, so it is worth exercising here.
QA Steps
Prerequisite: slow network
The Go Back button only appears after a loader has been up for 10 seconds, so every test below needs requests to take longer than that:
Turn throttling on only right before the last step of each test. Otherwise the whole app becomes slow.
Test 1: Go Back now appears on page loaders that had none before
Setup: US account without a wallet set up.
Test 2: workspace Accounting shows Go Back on narrow layout only
Setup: be a member of 2+ workspaces. On a workspace other than your default, connect an accounting integration (QBO, Xero or NetSuite).
Test 3: Expensify Card page hides Go Back on wide layout
Setup: a workspace with Expensify Card enabled.
Test 4: Create expense flow keeps the header visible (#96825, #96835)
Test 5: Split expense
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