Remove double token exchange in dev strict mode #167
Conversation
initRef.current flag was being set after an asynchronous call (await checkAuth(...)), both invocations of init could proceed before the flag was updated, leading to the exchangeAuthCode function being called twice.
WalkthroughMoved the init guard setting (initRef.current = true) to occur before awaiting checkAuth({ domain, clientId }) in KindeProvider.tsx to prevent concurrent re-entrancy. No public APIs changed; URL parameter and error handling flows remain as before. Changes
Sequence Diagram(s)sequenceDiagram
actor App
participant KindeProvider
participant AuthService as checkAuth
Note over KindeProvider: New flow
App->>KindeProvider: init()
KindeProvider->>KindeProvider: initRef.current = true
KindeProvider->>AuthService: checkAuth(domain, clientId)
AuthService-->>KindeProvider: result
KindeProvider-->>App: init complete
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/state/KindeProvider.tsx (3)
487-499: Avoid second code exchange on StrictMode remount by stripping auth params before invoking callbacksOn success, you currently rely on the default onSuccess to clear the URL. If a consumer overrides onSuccess without removing query params, a StrictMode remount can re-run init and try to exchange the same code again. Unconditionally strip known auth params before invoking onSuccess.
Apply this diff:
if (codeResponse.success) { const user = await getUserProfile(); if (user) { setState((val) => ({ ...val, user, isAuthenticated: true })); + // Always remove auth-related query params to prevent re-exchange on remounts (e.g., React StrictMode) + try { + const url = new URL(window.location.href); + const sp = new URLSearchParams(url.search); + sp.delete("code"); + sp.delete("state"); + sp.delete("reauth_state"); + sp.delete("error"); + sp.delete("error_description"); + url.search = sp.toString(); + window.history.replaceState({}, document.title, url.toString()); + } catch (_) { + // no-op: URL manipulation best-effort + } mergedCallbacks.onSuccess?.( user, { ...returnedState, kinde: undefined, }, contextValue, );
625-635: Reduce unnecessary re-executions of init effectBecause init depends on contextValue (which depends on state), every state change recreates init and re-triggers this effect. The ref guard blocks re-entry, but the effect still churns. If you don’t need to re-run init when props change, run it once on mount.
Apply this diff:
- useEffect(() => { - const mounted = { current: true }; - - if (mounted.current) { - init(); - } - - return () => { - mounted.current = false; - }; - }, [init]); + useEffect(() => { + init(); + // Run only once on mount; initRef prevents any accidental re-entry. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []);If you do need to respond to dynamic prop changes (domain/clientId), keep the current approach but consider storing only those props in the dependency list (and remove contextValue) to minimize re-runs.
455-466: Potential base64url decoding mismatch for statestate is encoded with base64UrlEncode, but decoded using atob, which expects standard Base64. If base64url padding or url-safe chars appear, atob may throw. Consider decoding as base64url.
Example patch:
- const decoded = atob(searchParams.get("state") || ""); + const rawState = searchParams.get("state") || ""; + const decoded = (() => { + try { + // Convert base64url -> base64 and pad + const b64 = rawState.replace(/-/g, "+").replace(/_/g, "/"); + const padded = b64 + "=".repeat((4 - (b64.length % 4)) % 4); + return atob(padded); + } catch { + return ""; + } + })();Please verify whether base64UrlEncode indeed produces URL-safe Base64 (it likely does). If so, the above guards against malformed/URL-safe inputs without relying on consumer callbacks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/state/KindeProvider.tsx(1 hunks)
🔇 Additional comments (1)
src/state/KindeProvider.tsx (1)
553-556: Good fix: moving the init guard before the await prevents concurrent re-entrySetting initRef.current = true before awaiting checkAuth closes the race where init could be re-entered (due to state-driven re-renders and StrictMode dev behavior) and perform a second token exchange. This is the correct place for the guard.
| initRef.current = true; | ||
| await checkAuth({ domain, clientId }); | ||
| const params = new URLSearchParams(window.location.search); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling around checkAuth to avoid stuck loading state or silent failures
If checkAuth throws, init will abort before setting isLoading to false or proceeding to other branches. Catch and surface the error, but continue initialization so subsequent logic can still run.
Apply this diff:
if (initRef.current) return;
initRef.current = true;
- await checkAuth({ domain, clientId });
+ try {
+ await checkAuth({ domain, clientId });
+ } catch (error) {
+ console.error("checkAuth error:", error);
+ mergedCallbacks.onError?.(
+ { error: "ERR_CHECK_AUTH", errorDescription: String(error) },
+ {},
+ contextValue,
+ );
+ // Intentionally continue; subsequent logic will determine auth state and set isLoading.
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initRef.current = true; | |
| await checkAuth({ domain, clientId }); | |
| const params = new URLSearchParams(window.location.search); | |
| if (initRef.current) return; | |
| initRef.current = true; | |
| try { | |
| await checkAuth({ domain, clientId }); | |
| } catch (error) { | |
| console.error("checkAuth error:", error); | |
| mergedCallbacks.onError?.( | |
| { error: "ERR_CHECK_AUTH", errorDescription: String(error) }, | |
| {}, | |
| contextValue, | |
| ); | |
| // Intentionally continue; subsequent logic will determine auth state and set isLoading. | |
| } | |
| const params = new URLSearchParams(window.location.search); |
🤖 Prompt for AI Agents
In src/state/KindeProvider.tsx around lines 555 to 557, wrap the await
checkAuth({ domain, clientId }) call in a try/catch so thrown errors don’t abort
initialization; on catch, record or log the error (e.g. set an error state or
processLogger.error) and ensure isLoading is set to false (or otherwise clear
loading) before continuing, then let the function proceed to the subsequent
logic (parsing URLSearchParams and other branches) so the component won’t remain
stuck in a loading state or silently fail.
initRef.currentflag was being set after an asynchronous call (await checkAuth(...)), both invocations of init could proceed before the flag was updated, leading to theexchangeAuthCodefunction being called twice.Explain your changes
The root cause of the double token exchange request appears to be the race condition within the
initfunction inKindeProvider.tsx. React's StrictMode causes theuseEffectthat calls init to run twice. Because theinitRef.currentflag was being set after an asynchronous call (await checkAuth(...)), both invocations of init could proceed before the flag was updated, leading to theexchangeAuthCodefunction being called twice.To fix this, I've moved the line
initRef.current = true;to before theawait checkAuth({ domain, clientId });call. This ensures that the flag is set synchronously, preventing the second invocation from executing the token exchange logic.Checklist
🛟 If you need help, consider asking for advice over in the Kinde community.