Fix duplicate /users/me requests during console sign-in - #40
Fix duplicate /users/me requests during console sign-in#40PasinduYeshan wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 57 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 (8)
📝 WalkthroughWalkthroughChangesProtectedRoute sign-in flow
Session refresh coordination
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ProtectedRoute
participant ThunderIDRuntime
participant Navigation
participant Logger
ProtectedRoute->>ThunderIDRuntime: read authentication state
ProtectedRoute->>Navigation: navigate to signInUrl
ProtectedRoute->>ThunderIDRuntime: initiate signIn
ThunderIDRuntime-->>ProtectedRoute: return sign-in failure
ProtectedRoute->>Logger: log ThunderIDRuntimeError
sequenceDiagram
participant ThunderIDProvider
participant ThunderIDReactClient
participant SessionUpdate
participant UsersMeAPI
ThunderIDProvider->>ThunderIDReactClient: startAutoRefreshToken
ThunderIDProvider->>ThunderIDReactClient: check isSignedIn
ThunderIDProvider->>SessionUpdate: request updateSession
SessionUpdate->>UsersMeAPI: fetch user profile
ThunderIDProvider->>SessionUpdate: issue concurrent update
SessionUpdate-->>ThunderIDProvider: reuse pending promise
🚥 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: |
1 participant
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
Bug Fixes
/users/merequest.New Features
Tests
Chores