fix(react-wallet-kit): open OAuth popup before async work - #1472
Open
blockgroot wants to merge 1 commit into
Open
fix(react-wallet-kit): open OAuth popup before async work#1472blockgroot wants to merge 1 commit into
blockgroot wants to merge 1 commit into
Conversation
Safari revokes a click's user-activation flag as soon as an `await` runs, so `window.open` gets silently blocked. All five OAuth popup handlers (Discord, X, Google, Apple, Facebook) opened the popup only after awaiting key-pair creation and PKCE challenge generation, so login via popup was broken in Safari. Add `openOAuthPopupAndNavigate`, which opens the popup synchronously and only then runs the async work to build the auth URL, closing the popup instead of leaving it blank if that work throws. Wire it into all five handlers. Fixes tkhq#1451
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit dfc1e44:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary & Motivation
Fixes #1451.
OAuth popup login (Discord, X, Google, Apple, Facebook) is silently blocked by Safari's
popup blocker. In all five handlers in
Provider.tsx,window.open()was called only afterawait createApiKeyPair()(and, for Discord/X/Facebook,await generateChallengePair()). Safari revokes a click's user-activation flag as soon as anyawaitruns after the click handler starts, so by the timewindow.open()ran, it was nolonger inside a "trusted" user gesture and Safari blocked it — the popup call returned
nulland the flow threw
Failed to open <Provider> login window.even though the user did clickthe login button.
Chrome/Firefox are more lenient about this timing, which is presumably why this shipped
unnoticed.
Fix
Added
openOAuthPopupAndNavigateinutils/oauth/url.ts: it opens the popupsynchronously, then runs the async work needed to build the auth URL (key-pair creation,
nonce, PKCE), and only then navigates the already-open popup to that URL. If the async work
throws, the popup is closed instead of being left open on a blank page (a related rough edge
mentioned in the issue).
Each of the five handlers in
Provider.tsxnow wraps its key/nonce/PKCE/URL-building logic ina
buildAuthUrlclosure and passes it toopenOAuthPopupAndNavigate, so the popup opens beforeany
awaitin the popup-flow branch. TheopenInPage(redirect) branch is behaviorallyunchanged — it still awaits
buildAuthUrl()and then redirects.Out of scope
scope discipline):
src/tests/timers-test.tsfails to run becausejest-environment-jsdomisn't declared as a devDependency of this package, and
src/components/auth/wallet/QRCodeDisplay.tsxhas a stale
@ts-expect-errorthat now failstsc --noEmit. Both are present onmainbefore this change (confirmed via
git stash).is left for a follow-up/maintainer decision — it's a UX change beyond fixing the ordering bug.
How I Tested These Changes
Added
src/tests/oauth-popup-test.tscoveringopenOAuthPopupAndNavigatedirectly (popupopener and auth-URL builder are injected, so no
jsdom/realwindowis needed):buildAuthUrlruns, even whenbuildAuthUrlresolves after a realsetTimeouttick (not just a microtask) — proves the ordering fix isn't an artifact of howfast the async work happens to resolve.
Failed to open <Provider> login window.error is thrown, andbuildAuthUrlisnever called, when the popup is blocked.
buildAuthUrlrejects.All three fail with the pre-fix code (
openOAuthPopupAndNavigatedidn't exist) and pass after.Also ran (both clean aside from the pre-existing, unrelated issue noted above):
Did you add a changeset?
Yes —
.changeset/quiet-otters-safari-popup.md,@turnkey/react-wallet-kitpatch.