Repository navigation
Show the offline message instead of a bare spinner on the magic link page - #102261
Conversation
…oaded Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
🤖 Android test steps verification results: ✅ pass The Tests section is still a TODO, so I ran the linked issue's repro steps. With the app in the background, a deep link opened a two-expense report ($40.00) straight away with no stuck loading skeleton, and the report stayed correct after I opened an expense and went back. One caveat: the same account created these expenses, so they were already on the device and the skeleton condition may never have triggered. The original bug involved a different user (the workspace owner) opening the report.
|
|
@MelvinBot pull main |
…keletonNeverExits Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
@MelvinBot close PR. No longer needed |
|
I couldn't close this PR. My permissions in this run don't allow closing PRs. Please close it yourself using the Close pull request button below. |
|
@MelvinBot close PR like you did in #101153 (comment) |
|
I still can't close this PR. This run blocks the close command, even though I'm the author and you're the assignee. The run that closed the earlier PR must have had different permissions. Please use the Close pull request button below. The branch |
|
@MelvinBot pull main, revert your changes and rework based on updated issue description |
…keletonNeverExits Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
…rt has loaded" This reverts commit b5b28e9.
…page Offline, the magic link sign-in request stays queued, so the page never leaves its loader. Wrap the loader in FullPageOfflineBlockingView so users see why, and the loader comes back once the network returns. Co-authored-by: mkhutornyi <mkhutornyi@users.noreply.github.com>
|
MelvinBot has made 5 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. |
|
I merged Cause: Offline, the Fix: In
I added offline tests for the web and native pages. I updated the PR title and description to match. Not verified:
|
|
@codex review |
|
@MelvinBot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Keep them coming! 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". |
|
No blocking issues. CI is green, including typecheck, ESLint, and all 8 Jest jobs, and the coding-standards check found no violations. I found three small suggestions.
What I checked
Reply |
|
@MelvinBot address #102261 (comment) |
|
🤖 Web test steps verification results: I found no regressions. The code review found nothing that breaks the online or signed-in magic link flows, and Inbox and Settings work normally in the app. The web tester can't open
|
I am not able to reproduce both. Signed-in user never hits the condition of rendering
App/src/pages/ValidateLoginPage/index.web.tsx Lines 179 to 180 in 1dad0ad not.repro.movnot.repro.mov |
…background" This reverts commit 81211b8.
|
I reverted 81211b8 in 1dd8ce4 and pushed it. The code now matches 04cd25f exactly. The offline view no longer checks sign-in state or adds a background. That commit also wrapped the network reset in |
|
@MelvinBot add QA Steps and mark this PR ready for review |
|
I added QA Steps to the PR description and marked the PR ready for review.
I also fixed the Explanation of Change. It still described the background and signed-in changes that were reverted. CI Jest, typecheck, and ESLint pass. The only failing check is I left the Tests and Offline tests sections for you to fill in. |
|
@mkhutornyi 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.movAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safariweb.mov |
|
We did not find an internal engineer to review this PR, trying to assign a random engineer to #100682 as well as to this PR... Please reach out for help on Slack if no one gets assigned! |
|
🚧 AndrewGable 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! 🧪🧪
|
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.5.28-4 🚀
|
|
No help site update needed. This bug fix only swaps a stuck spinner for the in-app offline message on the magic link page, and no help article describes that page or its offline behavior. |
|
This PR failing because of the issue #105778 |
Checking... |
|
Confirmed the bug #105778 is reproducible in iOS production app downloaded from appstore bug.repro.mov |
|
🚀 Deployed to staging by https://github.com/AndrewGable in version: 9.5.30-0 🚀
|
|
🚀 Deployed to production by https://github.com/blimpich in version: 9.5.30-0 🚀
Bundle Size Analysis (Sentry): |







Explanation of Change
Reworked for the updated issue description: a signed-out user who opens a magic link with
exitTowhile offline sees a bare spinner that never ends.Offline, the
SigninUserWithLinkrequest is saved to the queue but not sent.autoAuthStatestaysSIGNING_IN, and theexitTonavigation waits for an authToken that never arrives.ValidateLoginPagerenders onlyFullScreenLoadingIndicatorwithshouldUseGoBackButton={false}, so nothing replaces the spinner.This change wraps that loader in the existing
FullPageOfflineBlockingViewon native (index.tsx) and web (index.web.tsx):exitTonavigation continues as before.shouldUseGoBackButton={false}opt-out is unchanged, as requested in review on Enable the Go Back button by default on FullScreenLoadingIndicator.The earlier skeleton change on this branch was reverted.
AI tests run: I added offline tests to
tests/ui/ValidateLoginPageTest.tsx(web) andtests/ui/LoadingIndicatorGoBackOptOutTest.tsx(native). CI Jest, typecheck, and lint pass on this branch.Fixed Issues
$ #100682
PROPOSAL: #100682 (comment)
Tests
// TODO: The human co-author must fill out the tests you ran before marking this PR as "ready for review"
// Please describe what tests you performed that validates your changed worked.
Offline tests
QA Steps
Precondition: signed out of New Expensify. Know the accountID of a test account and the reportID of one of its reports.
Offline magic link
https://staging.new.expensify.com/v/<accountID>/123456?exitTo=/r/<reportID>. On mobile, open it so it launches the app.123456isn't a real code, the app then shows the sign-in page or an expired link message.Online regression
7. Still signed out and online, tap an action link in a real Expensify email, for example View on an expense report email.
8. Verify the app signs in and opens the linked report, without the offline message.
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)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