Fix duplicate /users/me requests during console sign-in - #40
Conversation
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughProtectedRoute now performs guarded effect-based sign-in with navigation, custom handlers, option forwarding, and logged failures. ThunderIDProvider coalesces concurrent session updates and adjusts mount-time session restoration. Tests and package configuration cover both flows. ChangesProtectedRoute sign-in flow
Session refresh coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProtectedRoute
participant ThunderIDState
participant navigate
participant signIn
participant logger
ProtectedRoute->>ThunderIDState: read authentication state
ProtectedRoute->>navigate: navigate to redirect or signInUrl
ProtectedRoute->>signIn: initiate default sign-in with merged options
signIn-->>ProtectedRoute: resolve or reject
ProtectedRoute->>logger: log ThunderIDRuntimeError on failure
sequenceDiagram
participant ThunderIDProvider
participant ThunderIDReactClient
participant updateSession
participant getUsersMe
ThunderIDProvider->>ThunderIDReactClient: startAutoRefreshToken
ThunderIDProvider->>ThunderIDReactClient: check isSignedIn
ThunderIDProvider->>updateSession: request profile update
updateSession->>getUsersMe: fetch user profile
ThunderIDProvider->>updateSession: issue concurrent request
updateSession-->>ThunderIDProvider: reuse pending promise
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-router/src/components/ProtectedRoute.tsx`:
- Around line 169-228: Update the guard reset logic in the ProtectedRoute
sign-in useEffect so isLoading changes do not clear hasInitiatedSignInRef while
sign-in remains needed; reset it only when isSignedIn, fallback, or redirectTo
indicates sign-in is no longer required. In the signIn rejection handler, clear
hasInitiatedSignInRef before logging the error so a later recovery can retry.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fff3a076-f714-4b99-8920-6df21515c9ad
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (8)
packages/react-router/.gitignorepackages/react-router/package.jsonpackages/react-router/src/components/ProtectedRoute.tsxpackages/react-router/src/components/__tests__/ProtectedRoute.test.tsxpackages/react-router/tsconfig.spec.jsonpackages/react/src/contexts/ThunderID/ThunderIDProvider.tsxpackages/react/src/contexts/ThunderID/__tests__/ThunderIDProvider.test.tsxpackages/react/src/index.ts
4222022 to
b13c2de
Compare
|
After the fix: |
| 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; |
There was a problem hiding this comment.
Will this cause re-render and re-fire sign-in after a failure? If so, will it end up in an infinite loop?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
needsSignInbecomes 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.
There was a problem hiding this comment.
Removed the retry mechanism because the pre-PR behavior did not provide an intentional or reliable retry policy. Any apparent recovery came from duplicate sign-in calls caused by rerenders or React recovery, rather than a controlled retry flow.
Without an explicit requirement for SDK-owned transient-failure recovery, a zero-retry approach is simpler and safer. It prevents render-driven request loops, avoids additional timer and state-management complexity, and leaves retry decisions to an explicit user action or higher-level application logic.
@CodeRabbit
There was a problem hiding this comment.
@PasinduYeshan, verified. The zero-retry approach resolves the reported loop risk.
After a failed signIn() or onSignIn(), ProtectedRoute logs the error and keeps hasInitiatedSignInRef set. Later renders in the same unauthenticated episode cannot start another automatic sign-in attempt. The ref resets only when needsSignIn becomes false.
This avoids an implicit SDK retry policy and preserves control for the application to provide an explicit retry action when needed.
✏️ Learnings added
Learnt from: PasinduYeshan
Repo: thunder-id/javascript-sdks PR: 40
File: packages/react-router/src/components/ProtectedRoute.tsx:0-0
Timestamp: 2026-07-31T04:15:47.943Z
Learning: In `packages/react-router/src/components/ProtectedRoute.tsx`, failed automatic sign-in attempts use a zero-retry policy: `hasInitiatedSignInRef` remains set after a `signIn()` or `onSignIn()` failure and resets only when `needsSignIn` becomes false. This prevents render- or effect-driven retry loops. Applications must implement any explicit retry behavior.
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.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
You are interacting with an AI system.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/src/contexts/ThunderID/ThunderIDProvider.tsx (1)
162-163: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLog the swallowed session-update error.
The outer
catchonly has a TODO and never logs, so any failure ingetDecodedIdToken()/profile hydration duringperformSessionUpdate()is completely invisible —updateSession()will resolve normally either way.loggeris already used a few lines above (line 149); reuse it here.🪵 Proposed fix
} catch (error) { - // TODO: Add an error log. + logger.error('Failed to update session.', error); } finally {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/src/contexts/ThunderID/ThunderIDProvider.tsx` around lines 162 - 163, Update the outer catch in performSessionUpdate to log the caught session-update error through the existing logger, replacing the TODO while preserving updateSession’s current resolution behavior.
🧹 Nitpick comments (1)
packages/react-router/src/components/ProtectedRoute.tsx (1)
264-271: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider surfacing terminal sign-in failure to the host app.
Once
MAX_SIGN_IN_ATTEMPTSis hit,ProtectedRoutekeeps renderingloaderindefinitely with no consumer-visible signal beyond the internal logger — a persistent misconfiguration (e.g. badsignInUrl/network outage) looks like an infinite spinner to the end user. An optionalonSignInErrorcallback (or similar) would let host apps swap in an error UI instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/components/ProtectedRoute.tsx` around lines 264 - 271, The terminal retry branch in ProtectedRoute should notify the host app when sign-in attempts reach MAX_SIGN_IN_ATTEMPTS instead of only logging and leaving the loader visible indefinitely. Add an optional onSignInError-style callback to the component’s public API, invoke it from the sign-in failure path at the retry limit with the relevant error, and preserve the current loader behavior when no callback is supplied.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/react/src/contexts/ThunderID/ThunderIDProvider.tsx`:
- Around line 162-163: Update the outer catch in performSessionUpdate to log the
caught session-update error through the existing logger, replacing the TODO
while preserving updateSession’s current resolution behavior.
---
Nitpick comments:
In `@packages/react-router/src/components/ProtectedRoute.tsx`:
- Around line 264-271: The terminal retry branch in ProtectedRoute should notify
the host app when sign-in attempts reach MAX_SIGN_IN_ATTEMPTS instead of only
logging and leaving the loader visible indefinitely. Add an optional
onSignInError-style callback to the component’s public API, invoke it from the
sign-in failure path at the retry limit with the relevant error, and preserve
the current loader behavior when no callback is supplied.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a726a68f-3dbd-4384-b925-bec3a2749a60
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
packages/react-router/.gitignorepackages/react-router/package.jsonpackages/react-router/src/components/ProtectedRoute.tsxpackages/react-router/src/components/__tests__/ProtectedRoute.test.tsxpackages/react-router/tsconfig.spec.jsonpackages/react/src/contexts/ThunderID/ThunderIDProvider.tsxpackages/react/src/contexts/ThunderID/__tests__/ThunderIDProvider.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-router/tsconfig.spec.json
e908cc6 to
53d37ee
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-router/src/components/__tests__/ProtectedRoute.test.tsx`:
- Around line 278-283: Update the test around the renderRoute() call to capture
its rerender function, then invoke rerender after each mocks.state.isLoading
assignment so ProtectedRoute reevaluates the changed state before asserting
signIn call counts. Preserve the existing expectation that toggling loading does
not re-arm the failed guard.
In `@packages/react-router/src/components/ProtectedRoute.tsx`:
- Around line 55-58: Update the onSignIn prop type to allow void or
Promise<void>, then in the ProtectedRoute sign-in effect invoke custom handlers
through a Promise.resolve().then chain and attach the existing sign-in error
logger/handling in catch, matching the default sign-in path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a05f054-cdff-4e11-acf9-22096601bec0
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (3)
packages/react-router/package.jsonpackages/react-router/src/components/ProtectedRoute.tsxpackages/react-router/src/components/__tests__/ProtectedRoute.test.tsx
Initiate sign-in from an effect instead of render, and guard it with a ref so it only fires once per unauthenticated episode. On failure the guard stays closed until the episode genuinely ends (sign-in, fallback, or redirect), instead of releasing immediately and letting a re-render retry it. Also stop re-exporting createPackageComponentLogger from @thunderid/react's public API; consumers that need it (react-router, matching vue) import it from @thunderid/browser directly. Fixes thunder-id/thunderid#4115
53d37ee to
0211c18
Compare
2 participants
Fixes thunder-id/thunderid#4115 — a single console sign-in issued ~10
GET /users/me(measured: 9 and 12) instead of 1.Two independent defects, measured separately at runtime.
1.
ProtectedRouteretried its own render, re-issuing sign-in each timesignIn()was called from the render body, which updatesThunderIDProviderstate during render:Worse, the
throw new ThunderIDRuntimeError('ProtectedRoute misconfiguration.', ...)sat after theif (!isSignedIn)block and no branch inside returned. SinceisSignedInis already knownfalseat that point, it fired on every unauthenticated render, including ones the component had just handled correctly (signInUrl,onSignIn, and the default sign-in all fell through to it).React discarded each throw and re-rendered the root synchronously (
11× There was an error during concurrent rendering but React was able to recover), and every retry re-ran the render-phasesignIn(). That loop is the request burst. It self-terminated once auth state settled, which is why the bug was invisible except in the network tab.Decisions:
useEffect, guarded by a ref so it fires once per unauthenticated state. The ref resets when the state leaves "unauthenticated", so a later sign-out re-initiates rather than rendering the loader forever.isLoading→loader,isSignedIn→children,fallback,redirectTo→<Navigate>, otherwise initiate sign-in and render the loader), soProtectedRoute-Misconfiguration-001is unreachable by construction. It never detected a misconfiguration; it was a missingreturn.ProtectedRoute-SignInError-001is kept but reported, not thrown. It previously lived inside anasyncIIFE, so it could never reach React and became an unhandled rejection. It is now constructed with the same code and origin, carries the original error asdetails, and goes to the package logger — matching howCallbackRoutealready handles async auth failures.2.
ThunderIDProviderresumed the session twice per mountThe mount effect took
isAlreadySignedIn = await client.isSignedIn(), ranresumeSession()if true, then unconditionally scheduled the refresh, re-checkedisSignedIn(), and ranresumeSession()again. The pre-check can only be true when the token is valid — a state in which the re-check is also trivially true — so an already-signed-in load always paid 2/users/meand 3startAutoRefreshToken().Decisions:
startAutoRefreshToken()is still called unconditionally on every mount, before the sign-in check. Keeping it there preserves the case it exists for: when the access token expired but the refresh token is still valid,startAutoRefreshToken()refreshes immediately (timeUntilRefresh <= 0), so theisSignedIn()check that follows observes the refreshed state. A regression test covers exactly this ordering.isSignedIn()pre-check was dropped, along with the duplicateresumeSession()it guarded. The second check is retained because it is the one that can observe a silent refresh. This also matches@thunderid/vue, which has always done this as a singleif.updateSession()is now single-flight. Concurrent callers share the in-flight promise. Deliberately not a cache: the ref clears on settle, so a later call (e.g. revalidation after a profile update) fetches again.Scope decisions
<ProtectedRoute>elements and Gate mounts its own provider; making the prop-less usage correct fixes both, instead of editing 33 call sites withloader/fallbackand relying on convention for new routes.signIn()await the session sync before clearing the loading state closed the window whereisLoading === falsewhileisSignedIn === false. OnceProtectedRouteno longer loops on that window, it made no difference to the request count, so it is not included. The window itself still exists and is worth a separate look.Verification
Measured against a real console sign-in (Playwright, counting requests), building each variant separately:
ProtectedRoutefix onlyupdateSession()single-flightFinal state re-measured 3× at 1/1.
POST /oauth2/tokenunchanged (1 on login, 0 on reload). All render-phase errors gone.Tests: 11 new for
ProtectedRoute(7 fail without this change) and 5 new forThunderIDProvider.pnpm buildandpnpm format:checkpass; lint findings on the touched files are unchanged or one fewer; the 2TokenCallbackfailures and thenuxtvue-tsctooling gap are pre-existing onmain.Supporting changes: re-export
createPackageComponentLoggerfrom@thunderid/react(react-routerdepends only onreactand it was not re-exported); add@testing-library/reactdevDep, the missing*.tsxentries intsconfig.spec.json, and a.vitest-attachments/ignore toreact-router— it had no component-test setup before.Summary by CodeRabbit