Fix infinite loading page after unlinking secondary login from email - #97803
Fix infinite loading page after unlinking secondary login from email#97803ahmdshrif wants to merge 8 commits into
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 / Safari |
2 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. Three 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.unlinkLogin'sfailureDatanever wrote an error, so a failed/expired unlink showed nothing. Fixed by writingunlinkLoginForm.unlinkError(added to all locale files) without touchingcredentials.login, so the failure correctly lands onUnlinkLoginForm, which already rendersaccount.errorsand has no mount-time clear.Fixed Issues
$ #96073
PROPOSAL: #96073 (comment)
Tests
UnlinkLoginForminstead (with the resend/unlink affordance intact)canGoBack()true) still exits via the originalgoBack()path, unchangedOffline 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
It's acceptable to write "Same as tests" if the QA team is able to run the tests in the above "Tests" section. Same as tests.
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