Skip to content

Fix duplicate /users/me requests during console sign-in #40

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
PasinduYeshan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix duplicate /users/me requests during console sign-in #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/react-router/.gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,3 +139,7 @@ dist
vite.config.js.timestamp-*
vite.config.ts.timestamp-*
.vite/

# Vitest browser screenshots and attachments
.vitest-attachments/
**/__screenshots__/
1 change: 1 addition & 0 deletions packages/react-router/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
},
"devDependencies": {
"@playwright/test": "catalog:",
"@testing-library/react": "catalog:",
"@thunderid/eslint-plugin": "catalog:",
"@thunderid/prettier-config": "catalog:",
"@thunderid/react": "workspace:^",
Expand Down
118 changes: 79 additions & 39 deletions packages/react-router/src/components/ProtectedRoute.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,15 @@
* under the License.
*/

import {ThunderIDRuntimeError, navigate, useThunderID} from '@thunderid/react';
import {FC, ReactElement, ReactNode} from 'react';
import {ThunderIDRuntimeError, createPackageComponentLogger, navigate, useThunderID} from '@thunderid/react';
import {FC, ReactElement, ReactNode, RefObject, useEffect, useRef} from 'react';
import {Navigate} from 'react-router';

const logger: ReturnType<typeof createPackageComponentLogger> = createPackageComponentLogger(
'@thunderid/react-router',
'ProtectedRoute',
);

/**
* Props for the ProtectedRoute component.
*/
Expand Down Expand Up @@ -49,7 +54,7 @@ export interface ProtectedRouteProps {
onSignIn?: (defaultSignIn: (options?: Record<string, any>) => void, signInOptions?: Record<string, any>) => void;
/**
* URL to redirect to when the user is not authenticated.
* Required unless a fallback element is provided.
* When neither this nor a fallback element is provided, sign-in is initiated instead.
*/
redirectTo?: string;
/**
Expand Down Expand Up @@ -92,8 +97,8 @@ export interface ProtectedRouteProps {
* It checks authentication status and either renders the protected content,
* shows a loading state, redirects, or shows a fallback.
*
* Either a `redirectTo` prop or a `fallback` prop must be provided to handle
* unauthenticated users.
* For unauthenticated users it renders the `fallback`, redirects to `redirectTo`, or, when neither
* is provided, initiates sign-in and renders the `loader` until the redirect happens.
*
* @example Basic usage with redirect
* ```tsx
Expand Down Expand Up @@ -161,6 +166,73 @@ const ProtectedRoute: FC<ProtectedRouteProps> = ({
}: ProtectedRouteProps) => {
const {isSignedIn, isLoading, signIn, signInOptions, tokenRequest, signInUrl} = useThunderID();

const hasInitiatedSignInRef: RefObject<boolean> = useRef<boolean>(false);

const needsSignIn: boolean = !isSignedIn && !fallback && !redirectTo;
const shouldInitiateSignIn: boolean = !isLoading && needsSignIn;

// Sign-in must be initiated from an effect, never from render: it updates the provider's state,
// and a render-phase update makes React discard the render and retry it, re-running the sign-in
// on every retry.
useEffect(() => {
if (!needsSignIn) {
// Reset so a later sign-out initiates sign-in again instead of rendering the loader forever.
// Keyed off `needsSignIn`, not `shouldInitiateSignIn`: the in-flight sign-in toggles the
// provider's loading state, and resetting on that would re-arm the guard and sign in twice.
hasInitiatedSignInRef.current = false;
return;
}

if (!shouldInitiateSignIn || hasInitiatedSignInRef.current) {
return;
}

hasInitiatedSignInRef.current = true;

if (signInUrl) {
navigate(signInUrl);
return;
}
if (onSignIn) {
onSignIn(signIn, overriddenSignInOptions);
return;
}

const mergedParams: Record<string, unknown> | undefined = (overriddenTokenRequest ?? tokenRequest)?.params;

signIn(
overriddenSignInOptions ?? signInOptions,
undefined,
undefined,
undefined,
mergedParams && Object.keys(mergedParams).length > 0 ? {params: mergedParams} : undefined,
).catch((error: unknown) => {
// Release the guard so a later render can retry instead of being blocked forever.
hasInitiatedSignInRef.current = false;

@JayaShakthi97 JayaShakthi97 Jul 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this cause re-render and re-fire sign-in after a failure? If so, will it end up in an infinite loop?

@PasinduYeshan PasinduYeshan Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CodeRabbit WDYT?

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PasinduYeshan, yes—this is a valid concern.

The rejection handler releases hasInitiatedSignInRef, so any subsequent render while the user remains unauthenticated and isLoading is false will re-enter the effect and call signIn() again. The .catch() itself does not trigger a render, but a typical failed sign-in updates provider loading/session state; that render is enough to retry. If the failure is persistent (for example, network/configuration failure), this can become a sign-in/logging loop.

Consider keeping the guard set after failure and exposing an explicit retry path, or recording the failed attempt and only retrying after a meaningful state/configuration change.

@PasinduYeshan PasinduYeshan Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As CodeRabbit mentioned, while resetting the guard doesn't itself trigger a re-render, a failed signIn() can update the provider state, causing the effect to run again and potentially retry indefinitely if the failure persists.

To address that, I'm thinking of adding a bounded retry mechanism (e.g. max 5 attempts). That would allow recovery from transient failures while preventing unbounded retries on persistent failures. Does that sound like a reasonable approach?

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

@PasinduYeshan PasinduYeshan Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for additional context, this retry path is a consequence of moving signIn() into useEffect to fix the original issue. Previously, signIn() was invoked during render, which could lead to repeated render-phase invocations and the synchronous request burst described in the PR. With the effect + guard, that issue is eliminated.

The remaining concern is only around persistent asynchronous failures if we explicitly re-arm the guard in .catch(). I think adding a bounded retry limit (e.g. 5 attempts) strikes a good balance—it still recovers from transient failures while preventing indefinite retries if sign-in continues to fail.

@CodeRabbit verify.

@coderabbitai coderabbitai Bot Jul 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PasinduYeshan, verified. Your diagnosis is correct: moving signIn() into the effect plus the guard eliminates the original render-phase retry/request burst. The remaining loop is specifically caused by re-arming the guard after an asynchronous rejection and then receiving another render while still unauthenticated.

A bounded retry policy is a reasonable fix, with two important details:

  • Track attempts in a ref and reset it only when the authentication episode genuinely ends—for example, when needsSignIn becomes false (signed in, fallback, or redirect path).
  • Schedule retries with a delay/backoff rather than immediately releasing the guard for the next render. Otherwise, five retries can still produce a short burst of failed calls and logs.

After the maximum is reached, keep the guard closed and leave the loader/error state stable (or expose an explicit consumer-controlled retry). That preserves transient-failure recovery without recreating an unbounded request loop.

✏️ Learnings added 
Learnt from: PasinduYeshan
Repo: thunder-id/javascript-sdks PR: 40
File: packages/react-router/src/components/ProtectedRoute.tsx:211-211
Timestamp: 2026-07-28T10:57:23.042Z
Learning: In `packages/react-router/src/components/ProtectedRoute.tsx`, moving `signIn()` from render into a guarded `useEffect` prevents the render-phase retry burst that caused duplicate authentication requests. If the effect re-arms its sign-in guard after an asynchronous `signIn()` rejection, retries must be bounded and preferably delayed/backed off; reset the retry state only when the current unauthenticated sign-in episode ends.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


// Logged rather than thrown: a rejection here cannot reach React, and CallbackRoute treats
// async auth failures the same way.
const signInError: ThunderIDRuntimeError = new ThunderIDRuntimeError(
'Sign-in failed in ProtectedRoute.',
'ProtectedRoute-SignInError-001',
'react-router',
error,
);

logger.error(signInError.message, signInError);
});
}, [
needsSignIn,
shouldInitiateSignIn,
signInUrl,
onSignIn,
signIn,
signInOptions,
overriddenSignInOptions,
tokenRequest,
overriddenTokenRequest,
]);

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Always wait for loading to finish before making authentication decisions
if (isLoading) {
return loader;
Expand All @@ -178,40 +250,8 @@ const ProtectedRoute: FC<ProtectedRouteProps> = ({
return <Navigate to={redirectTo} replace />;
}

if (!isSignedIn) {
if (signInUrl) {
navigate(signInUrl);
} else if (onSignIn) {
onSignIn(signIn, overriddenSignInOptions);
} else {
(async (): Promise<void> => {
try {
const mergedParams = (overriddenTokenRequest ?? tokenRequest)?.params;
await signIn(
overriddenSignInOptions ?? signInOptions,
undefined,
undefined,
undefined,
mergedParams && Object.keys(mergedParams).length > 0 ? {params: mergedParams} : undefined,
);
} catch (error) {
throw new ThunderIDRuntimeError(
'Sign-in failed in ProtectedRoute.',
'ProtectedRoute-SignInError-001',
'react-router',
`An error occurred during sign-in: ${(error as Error).message}`,
);
}
})();
}
}

throw new ThunderIDRuntimeError(
'ProtectedRoute misconfiguration.',
'ProtectedRoute-Misconfiguration-001',
'react-router',
'The internal handler failed to process the state. Please try with a fallback or redirectTo prop.',
);
// The effect above is taking the user to sign-in; render the loader until that resolves.
return loader;
};

export default ProtectedRoute;
Loading