fix(react-ui): fix OAuth prefetch race when dialog opens before auth init#1863
fix(react-ui): fix OAuth prefetch race when dialog opens before auth init#1863jmderby wants to merge 2 commits into
Conversation
…init Co-Authored-By: Jonathan Derbyshire <jonathan.temp@paella.dev>
Co-Authored-By: Jonathan Derbyshire <jonathan.temp@paella.dev>
Original prompt from Jonathan
|
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: bb64416 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
packages/client/ui/react-ui/src/providers/auth/OAuthFlowProvider.tsx:41
**Brief window where OAuth click silently errors during init**
Changing the initial value from `true` to `false` is correct semantically, but it means the OAuth buttons are clickable during the gap between the dialog opening and `crossmintAuth` becoming available. In that gap `preFetchAndSetOauthUrl` returns early (line 50–52) and `oauthUrlMap` is still empty, so `createPopupAndSetupListeners` falls through to `crossmintAuth?.getOAuthUrl(provider)` which returns `undefined`, triggering the "Failed to resolve OAuth URL" error to the user.
The original `true` incidentally blocked this window (though it caused a different bug when the prefetch never ran). A small guard — e.g. setting `isLoadingOauthUrlMap` to `true` before returning early when `crossmintAuth == null` and resetting to `false` — would prevent the premature-click path, or the OAuth buttons themselves could be disabled while `crossmintAuth` is not yet available.
Reviews (1): Last reviewed commit: "chore: add changeset for OAuth prefetch ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| const [oauthUrlMap, setOauthUrlMap] = useState<OAuthUrlMap>(initialOAuthUrlMap); | ||
| const [isLoadingOauthUrlMap, setIsLoadingOauthUrlMap] = useState(true); | ||
| const [isLoadingOauthUrlMap, setIsLoadingOauthUrlMap] = useState(false); |
There was a problem hiding this comment.
Changing the initial value from true to false is correct semantically, but it means the OAuth buttons are clickable during the gap between the dialog opening and crossmintAuth becoming available. In that gap preFetchAndSetOauthUrl returns early (line 50–52) and oauthUrlMap is still empty, so createPopupAndSetupListeners falls through to crossmintAuth?.getOAuthUrl(provider) which returns undefined, triggering the "Failed to resolve OAuth URL" error to the user.
The original true incidentally blocked this window (though it caused a different bug when the prefetch never ran). A small guard — e.g. setting isLoadingOauthUrlMap to true before returning early when crossmintAuth == null and resetting to false — would prevent the premature-click path, or the OAuth buttons themselves could be disabled while crossmintAuth is not yet available.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/client/ui/react-ui/src/providers/auth/OAuthFlowProvider.tsx
Line: 41
Comment:
**Brief window where OAuth click silently errors during init**
Changing the initial value from `true` to `false` is correct semantically, but it means the OAuth buttons are clickable during the gap between the dialog opening and `crossmintAuth` becoming available. In that gap `preFetchAndSetOauthUrl` returns early (line 50–52) and `oauthUrlMap` is still empty, so `createPopupAndSetupListeners` falls through to `crossmintAuth?.getOAuthUrl(provider)` which returns `undefined`, triggering the "Failed to resolve OAuth URL" error to the user.
The original `true` incidentally blocked this window (though it caused a different bug when the prefetch never ran). A small guard — e.g. setting `isLoadingOauthUrlMap` to `true` before returning early when `crossmintAuth == null` and resetting to `false` — would prevent the premature-click path, or the OAuth buttons themselves could be disabled while `crossmintAuth` is not yet available.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Nice try slithering in here with that take 🐍, but this one's a miss!
No consumer actually reads isLoading from the OAuthFlowContext. All OAuth buttons (GoogleSignIn, TwitterSignIn, ContinueWithGoogle) only destructure startOAuthLogin and activeOAuthProvider — they compute their own loading state via activeOAuthProvider === "google" / "twitter". So whether isLoadingOauthUrlMap is true or false has zero effect on button clickability.
The old useState(true) wasn't "incidentally blocking" anything — it was just leaving a stale true value that nobody reads, while silently preventing the loading state from ever resolving when the prefetch was skipped.
The cold-blooded scenario you describe (click during init → error flash) is the exact same behavior as main today via the existing try-catch from 1398b653. Not a regression from this PR. 🦎
There was a problem hiding this comment.
You're right, I concede. Both GoogleSignIn and TwitterSignIn only destructure startOAuthLogin and activeOAuthProvider from useOAuthFlow — isLoading (which reflects isLoadingOauthUrlMap) is never read by any consumer. Button clickability is independently derived from activeOAuthProvider === "google/twitter", not from this state. The useState(true) → useState(false) change is purely a semantic correctness fix with no behavioral difference on button interaction.
The "error during early click" scenario is also pre-existing behavior surfaced through the try/catch, not introduced by this PR. Comment withdrawn.
Tip: You can give me more context with .greptile/rules.md so I get this right next time.
1 participant
Description
Fixes a race condition where OAuth URL prefetch never runs if the user opens the auth dialog before auth initialization completes.
Root cause: The prefetch was gated by
getAuthStatus() === "logged-out", but when the dialog opens during initialization, the status transitions from"initializing"→"in-progress"(skipping"logged-out"entirely). This leftoauthUrlMapempty ({google: '', twitter: ''}), and the on-demand fallback tocrossmintAuth?.getOAuthUrl()could fail ifcrossmintAuthwas not yet available in the closure.Changes:
CrossmintAuthProvider.tsx: Gate prefetch onbaseAuth.jwt == nullinstead ofgetAuthStatus() === "logged-out"— this ensures the prefetch runs whenever the user is not logged in, regardless of dialog or init state.OAuthFlowProvider.tsx: Guard prefetch withcrossmintAuth == nullearly return to avoid running it before the auth client is initialized. InitializeisLoadingOauthUrlMaptofalsesince no prefetch has started yet.useOAuthWindowListener.ts: Strengthen URL validation fromresolvedUrl == nullto!resolvedUrlto also catch empty strings, preventingnew URL("")from throwing.Test plan
pnpm lint)crossmintAuthbecomes availablegetOAuthUrlcall inuseOAuthWindowListeneris still present as a safety net if the prefetch hasn't completed by the time the user clicksPackage updates
@crossmint/client-sdk-react-ui: patch (changeset added via.changeset/fix-oauth-prefetch-race.md)Link to Devin session: https://crossmint.devinenterprise.com/sessions/c4d91faa048d4a0b8a834127afcff9b4
Requested by: @jmderby