Fix infinite loading page after unlinking secondary login from email - #97803
Conversation
UNLINK_LOGIN is a standalone public screen with no route beneath it, so a tab opened directly from the unlink email has an empty nav stack and UnlinkLoginPage's bare Navigation.goBack() no-ops forever, leaving the spinner stuck. Reset the stack to TAB_NAVIGATOR when there's nothing to pop, matching the pattern ValidateLoginPage already uses for this. Two more gaps kept the result invisible even once navigation worked: BaseLoginForm's mount effect cleared account.message before the success text could render, and unlinkLogin's failureData never wrote an error at all, so a failed unlink showed nothing on UnlinkLoginForm either.
Covers UnlinkLoginPage's stack reset on a fresh tab (and the stale- callback guard), BaseLoginForm's scoped clearAccountMessages skip for the unlink success message (with a control case for ordinary stale messages), and unlinkLogin's failureData now setting account.errors without touching credentials.login.
…translated text Without it translateLocal falls back to the raw key, since this action test doesn't render a LocaleContextProvider to load translations.
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
…oposal-71631c # Conflicts: # tests/actions/SessionTest.ts
|
Translations were added manually in this PR — |
- Reposition the naming-convention eslint-disable in BaseLoginForm.tsx: the merge's formatter reflowed the object literal and left the directive covering the wrong line, making it both unused and ineffective at once. - Replace an unsafe HttpUtils.xhr cast in the new unlinkLogin test with jest.mocked(), which pushed the file's grandfathered no-unsafe-type-assertion count from 12 to 13. - Drop an unused navigationRef import in UnlinkLoginPageTest.tsx. - Add unlinkLoginForm.unlinkError to el.ts, a new locale file main added after this branch's translations were written. - Simplify the canGoBack mock to a zero-arg call — spreading unknown[] into a function TypeScript infers as zero-arg doesn't type-check.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6268342b1e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…ge check When an expired/invalid unlink link is opened in a browser that never cached credentials.login for the flow (e.g. a different browser or profile than the one that requested the unlink), shouldShowLoginForm selects BaseLoginForm instead of UnlinkLoginForm. Its mount effect only exempted the success message from clearAccountMessages(), so the freshly-written unlink error was wiped before it could render, leaving a blank sign-in form. Extend the guard to also preserve that specific error by value, the same way the success message is matched — not by testing account.errors generally, which would strand unrelated stale errors on the page. Also extract the repeated 'unlinkLoginForm.successfullyUnlinkedLogin' literal (BaseLoginForm's accountMessage memo, its mount guard, and UnlinkLoginForm's message check) into a single isUnlinkLoginSuccessMessage predicate in SessionUtils so a future rename of the key can't silently break one of the sites.
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
Literal numeric object keys ({1: ...}) trip @typescript-eslint/naming-
convention. Switch to a computed key (Date.now()), matching how
production's ErrorUtils actually builds these error objects.
| key: ONYXKEYS.ACCOUNT, | ||
| value: { | ||
| isLoading: false, | ||
| errors: ErrorUtils.getMicroSecondOnyxErrorWithTranslationKey('unlinkLoginForm.unlinkError'), |
There was a problem hiding this comment.
I don't think we need this error as we do get it from the BE. Lets' remove this change
There was a problem hiding this comment.
Done — reverted, pushed in e6a738e. failureData is back to only flipping isLoading, since the BE's onyxData already supplies the error.
|
@ahmdshrif don't this change enough to fix the issue? 
|
|
@Pujan92 Yes — that 3-line change (nav reset + the success-message guard + the failureData error write) is the complete fix for the reported issue, and matches the approved proposal exactly. The extra bit in BaseLoginForm.tsx beyond that came from an automated Codex review on this PR, not from scope creep on my part: it flagged that a failed/expired unlink can also land on BaseLoginForm rather than UnlinkLoginForm — specifically when the link is opened in a browser/session that never cached The Happy to drop either or both if you'd rather keep this PR scoped strictly to the approved proposal — just say the word. |
|
Yes, let's drop both changes at the moment as I think setting the error this way looks like a workaround to me. |
Per @Pujan92's review: the backend already returns onyxData with a proper error message on unlink failure, so a client-side generic fallback (unlinkLoginForm.unlinkError) was redundant and looked like a workaround rather than a fix. Reverting to the approved proposal's scope: - unlinkLogin's failureData goes back to only flipping isLoading. - BaseLoginForm's mount-clear guard goes back to exempting only the success message, not a specific unlink error value. - UnlinkLoginForm and the accountMessage memo go back to their inline literal comparisons (drops the isUnlinkLoginSuccessMessage extraction along with it, since it existed only to serve the guard above). - Removes the now-unused unlinkLoginForm.unlinkError translation key from all locale files, and the tests that covered the reverted behavior.
|
@Pujan92 Pushed the revert (e6a738e) — back to the exact approved-proposal diff: nav reset + success-message guard only. Both extras (error-preservation guard + the DRY extraction that only existed to serve it) are gone, along with their tests and the now-unused |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / SafariScreen.Recording.2026-08-06.at.18.42.35.mov |
@ahmdshrif I think we need to update the step here, bcoz once the unlinked successfull -> opening the link again won't show the error, as credentials will be cleaned in the earlier success and we are no longer showing the unlink page. So we can swap the steps to check with the incorrect code first to see the error and then try with the correct code to see the success message. |
|
Good catch — you're right, and I've swapped the order in both the Tests and QA Steps sections. Confirmed the mechanism: Worth flagging one extra wrinkle I hit while checking this: Also updated the "Explanation of Change" section, which still described the reverted |
|
|
Good catch, dropped that parenthetical. Incognito/a separate browser starts with a fresh IndexedDB, so A fresh tab in the same browser is the right repro: shared Onyx keeps the credential, empty nav stack still triggers the spinner. Updated both Tests and QA Steps to say same browser, not incognito. |
|
🚧 pecanoro 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/pecanoro in version: 9.4.55-0 🚀
|
No help site changes requiredI reviewed this PR against the help site articles under Why: This is a bug fix, not a feature or behavior change. It fixes an infinite loading spinner that appeared when clicking the unlink validation link from a secondary-login email in a fresh browser tab, via two internal fixes:
There is no new UI, no new steps, no renamed feature/tab/button, and no changed user-facing flow — the fix simply makes the existing, already-documented unlink flow work as it's supposed to. The closest articles (Contact Methods and Update Email Address) already describe the expected behavior ("Follow the steps in the verification Email to complete the unlinking process"). They don't document the buggy spinner, so there's nothing to add, remove, or correct. No draft PR was created since no changes are warranted. @ahmdshrif, please review the linked help site PR and confirm it reflects the current behavior. Then mark the linked help site PR (Note: no help site PR was created because this bug fix doesn't change any documented behavior. If you believe a specific article should be updated, let me know which one and what to add.) |
|
🚀 Deployed to production by https://github.com/Beamanator in version: 9.4.55-4 🚀
Bundle Size Analysis (Sentry): |
5 participants
Explanation of Change
Fixes the infinite loading spinner reported when clicking the unlink validation link from a secondary-login email in a fresh browser tab. Two independent gaps caused this:
UnlinkLoginPageexits via a bareNavigation.goBack(). A tab opened from the unlink email hasUNLINK_LOGINas the only route in the public stack, socanGoBack()isfalseandgoBack()no-ops forever, leaving the spinner stuck. Fixed by resetting the stack toTAB_NAVIGATORwhen there's nothing to pop back to — the same patternValidateLoginPagealready uses for this exact situation, including its stale-callback cleanup guard.BaseLoginForm's mount effect unconditionally callsclearAccountMessages(), which wiped the "Secondary login successfully unlinked!" message before it could render. Fixed by scoping the skip to that one specific message value, leaving every other mount's cleanup unchanged.The failure path needs no client-side change:
credentials.loginis left untouched, soisUnvalidatedSecondaryLoginstays true and the user lands onUnlinkLoginForm, which already renders the backend'saccount.errors(delivered via responseonyxData) and has no mount-time clear.Fixed Issues
$ #96073
PROPOSAL: #96073 (comment)
Tests
UnlinkLoginForm, with the resend/unlink affordance intactcanGoBack()true) still exits via the originalgoBack()path, unchangedNote
Two constraints, both because the failure case needs
credentials.loginstill cached in order to render its error:credentials.loginviasuccessData, which makesshouldShowLoginFormtrue — so a later failed attempt lands onBaseLoginForminstead ofUnlinkLoginForm, and the backend error has no surface to render on.credentials.loginis present) while still starting with an empty navigation stack — the empty stack is what reproduces the original spinner. Incognito gives the empty stack but loses the credential.Offline tests
This flow requires a live network round-trip to the unlink API to reach either terminal state (success or failure). I put an Android emulator into airplane mode and opened the unlink deep link:
unlinkLogin's optimistic write setsisLoading: trueand the request queues (standard Onyx offline-first behavior), so neitherNavigation.goBack()nor the new stack-reset fires until the request actually settles —isLoadingnever flips tofalsewhile offline. That's unchanged from before this PR; nothing in this fix is offline-specific.(Note: with the dev-debug build fully offline, Metro itself becomes unreachable and the JS bundle can't load at all — an artifact of the local dev harness, not the app or this fix, since a release build ships its bundle embedded.)
QA Steps
Note
Two ordering/setup constraints, both for the same reason — the failure case needs the cached secondary-login credential in order to render its error:
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)(No new CSS styles or assets were added or modified in this PR.)
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.(This PR does not modify any UI — no new buttons, components, spacing/padding changes, or form-input styles; it only wires an existing message-rendering path to a new trigger.)
Screenshots/Videos
Android: Native
unLink is not open on native
Android: mWeb Chrome
iOS: Native
unLink is not open on native
iOS: mWeb Safari
MacOS: Chrome / Safari