fix(clerk-js): Correct race condition when fetching tokens #7304
Conversation
🦋 Changeset detectedLatest commit: 4a6769d The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughImplements cross-tab SafeLock with timeouts and degraded fallback; adds per-token locks to Session token fetching to prevent duplicate refreshes; removes locking from SessionCookiePoller; reorders AuthCookieService initialization; adds LruMap and tests for SafeLock, SessionCookiePoller, and LruMap; adds a changeset for the patch. Changes
Sequence Diagram(s)sequenceDiagram
participant TabA as Browser Tab A
participant TabB as Browser Tab B
participant Lock as SafeLock (per-token)
participant API as Token API
rect rgb(245,250,255)
note over TabA,TabB: Both tabs request same token (poller/focus/user)
TabA->>Lock: acquireLockAndRun(tokenId, refreshCb)
TabB->>Lock: acquireLockAndRun(tokenId, refreshCb)
end
rect rgb(220,255,220)
note over Lock,API: TabA obtains lock, performs refresh
Lock->>TabA: lock granted
TabA->>API: fetch token
API-->>TabA: token returned
TabA->>TabA: cache token & emit events
Lock-->>TabA: release
end
rect rgb(255,245,220)
note over Lock,TabB: TabB acquires lock later and re-checks cache
Lock->>TabB: lock granted
TabB->>TabB: sees cached token → skip API
Lock-->>TabB: release
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/core/auth/safeLock.ts (1)
16-38: Fix inconsistent return type behavior.The
acquireLockAndRunfunction has inconsistent return behavior:
- Navigator.locks path (line 25-28): Returns
falseon error via.catch()- Fallback path (line 31-37): Returns
undefinedimplicitly when lock acquisition fails (after line 37)This creates ambiguity for callers: should they check for
false,undefined, or both to detect failure?Consider one of these approaches:
Option 1 (Recommended): Return a consistent sentinel value
const acquireLockAndRun = async (cb: () => Promise<unknown>) => { if ('locks' in navigator && isSecureContext) { const controller = new AbortController(); const lockTimeout = setTimeout(() => controller.abort(), 4999); return await navigator.locks .request(key, { signal: controller.signal }, async () => { clearTimeout(lockTimeout); return await cb(); }) .catch(() => { // browser-tabs-lock never seems to throw, so we are mirroring the behavior here return false; }); } if (await lock.acquireLock(key, 5000)) { try { return await cb(); } finally { await lock.releaseLock(key); } } + + // Return false when lock acquisition fails to match navigator.locks behavior + return false; };Option 2: Use a Result/Option type for clearer semantics
This would make success/failure explicit but requires more extensive changes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.changeset/fix-token-refresh-race-condition.md(1 hunks)packages/clerk-js/src/core/auth/AuthCookieService.ts(6 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/safeLock.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/AuthCookieService.ts (4)
packages/clerk-js/src/core/auth/cookies/clientUat.ts (2)
ClientUatCookieHandler(12-15)createClientUatCookie(23-65)packages/clerk-js/src/core/auth/SessionCookiePoller.ts (2)
SessionCookiePoller(9-42)REFRESH_SESSION_TOKEN_LOCK_KEY(6-6)packages/clerk-js/src/core/auth/cookies/session.ts (2)
SessionCookieHandler(10-14)createSessionCookie(28-59)packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLockReturn(3-5)SafeLock(7-41)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLockReturn(3-5)SafeLock(7-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
6-6: Good: Exporting the lock key enables cross-component coordination.Exporting
REFRESH_SESSION_TOKEN_LOCK_KEYallowsAuthCookieServiceto create a shared lock using the same key, which is essential for the cross-tab synchronization mechanism.packages/clerk-js/src/core/auth/AuthCookieService.ts (4)
72-75: Excellent: Clear documentation of the locking mechanism.The comment clearly explains the purpose of the
tokenRefreshLockand how it coordinates cross-tab token refresh. This helps future maintainers understand the synchronization strategy.
137-137: Good: Shared lock injected into poller.Passing
this.tokenRefreshLockto theSessionCookiePollerconstructor ensures both the poller and the focus handler use the same cross-tab lock, which is essential for preventing the race condition described in the PR objectives.
158-162: Good: Focus handler now uses the shared lock.Wrapping the
refreshSessionTokencall inthis.tokenRefreshLock.acquireLockAndRun()prevents concurrent token fetches when multiple tabs gain focus simultaneously or when focus events fire while the poller is already refreshing.The updated comment clearly explains the cross-tab coordination mechanism.
47-47: Properties are properly initialized in the constructor — no issue exists.The properties
clientUatandsessionCookieare declared without explicit initialization on lines 47 and 50, but they are both assigned values in the constructor (lines ~88–95 viacreateClientUatCookie()andcreateSessionCookie()). TypeScript'sstrictPropertyInitializationrule only flags properties that are declared without initialization AND never assigned in the constructor. Since these properties are assigned before any use, they comply with strict mode requirements and require no changes.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
packages/clerk-js/src/core/auth/safeLock.ts (2)
42-46: Memory leak: event listener is never removed.Each call to
SafeLock()adds a newbeforeunloadlistener that is never cleaned up. IfSafeLockis called multiple times (e.g., during hot reloads in development), listeners accumulate.Consider returning a cleanup function or ensuring
SafeLockis only instantiated once per key:export function SafeLock(key: string): SafeLockReturn { const lock = new Lock(); + const handleBeforeUnload = async () => { + await lock.releaseLock(key); + }; + // Release any held locks when the tab is closing to prevent deadlocks // eslint-disable-next-line @typescript-eslint/no-misused-promises - window.addEventListener('beforeunload', async () => { - await lock.releaseLock(key); - }); + window.addEventListener('beforeunload', handleBeforeUnload);Since this is scoped to single-instance usage (via
AuthCookieService), this is low-risk but worth noting for future reuse.
48-74: Consider adding explicit return type annotation for type safety.Per coding guidelines, explicit return types should be defined for functions. While TypeScript can infer the return type, an explicit annotation would improve clarity and catch type mismatches earlier.
- const acquireLockAndRun = async (cb: () => Promise<unknown>) => { + const acquireLockAndRun = async (cb: () => Promise<unknown>): Promise<unknown> => {packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
82-96: Consider testing lock acquisition failure scenario.The tests cover the happy path well, but there's no test for when
acquireLockAndRunreturnsfalse(timeout/failure). This edge case is part of theSafeLockReturncontract.it('handles lock acquisition timeout gracefully', async () => { const sharedLock: SafeLockReturn = { acquireLockAndRun: vi.fn().mockResolvedValue(false), // Lock timeout }; const poller = new SessionCookiePoller(sharedLock); const callback = vi.fn().mockResolvedValue(undefined); poller.startPollingForSessionToken(callback); // Callback should not be invoked when lock returns false expect(callback).not.toHaveBeenCalled(); poller.stopPollingForSessionToken(); });
100-121: Unnecessaryasyncon test function.The test doesn't use
awaitat the top level. While not harmful, removingasyncwould be cleaner.- it('allows restart after stop', async () => { + it('allows restart after stop', () => {packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
83-104: Test name is misleading - mock doesn't actually simulate sequential execution.The test "mock lock can simulate sequential execution" fires both promises concurrently via
Promise.all. The mock implementation executes callbacks in parallel, not sequentially as a real lock would. This doesn't validate actual sequential behavior.Consider either:
- Renaming to clarify it tests concurrent invocation tracking
- Or implementing actual sequential simulation with a queue
- it('mock lock can simulate sequential execution', async () => { + it('tracks multiple concurrent lock invocations', async () => {
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
packages/clerk-js/src/core/auth/AuthCookieService.ts(6 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(1 hunks)packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts(1 hunks)packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/__tests__/safeLock.test.tspackages/clerk-js/src/core/auth/AuthCookieService.tspackages/clerk-js/src/core/auth/safeLock.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
packages/clerk-js/src/core/auth/safeLock.ts (1)
SafeLockReturn(6-15)packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
SessionCookiePoller(27-60)
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (1)
packages/clerk-js/src/core/auth/safeLock.ts (2)
SafeLock(39-77)SafeLockReturn(6-15)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (11)
packages/clerk-js/src/core/auth/safeLock.ts (2)
3-15: JSDoc documentation added - addresses past review feedback.The interface now has proper JSDoc documentation explaining the cross-tab lock coordination purpose and the
acquireLockAndRunmethod behavior including the timeout return value.
17-39: JSDoc documentation added - addresses past review feedback.The function now has comprehensive JSDoc including purpose, parameters, return type, and a practical example demonstrating cross-tab coordination.
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (2)
6-14: LGTM - Clean test setup.Proper use of
beforeEach/afterEachfor fake timer management and mock restoration.
16-64: Good coverage for shared lock coordination pattern.These tests effectively validate the core fix - that an external lock can be injected and shared between the poller and focus handler, which is the key mechanism for preventing the race condition.
packages/clerk-js/src/core/auth/__tests__/safeLock.test.ts (2)
6-23: LGTM - Good interface contract tests.These tests validate the API surface and mockability of
SafeLockReturn, which is important for the dependency injection pattern used inSessionCookiePoller.
25-45: Good defensive testing with environment check.The conditional skip for environments without Web Locks API is appropriate. The test validates the callback is invoked and the result propagates correctly.
packages/clerk-js/src/core/auth/AuthCookieService.ts (5)
23-25: LGTM - Clean imports for the new lock coordination feature.Proper separation of type import (
SafeLockReturn) from value imports (SafeLock,REFRESH_SESSION_TOKEN_LOCK_KEY).
51-54: Good documentation for the shared lock member.The JSDoc clearly explains the purpose of
tokenRefreshLockfor cross-tab coordination.
75-78: Lock creation is appropriately positioned.Creating the lock early in the constructor ensures it's available before
refreshTokenOnFocus()andstartPollingForToken()are called.
138-142: Correctly passes shared lock to poller.The poller now receives the same
tokenRefreshLockthat the focus handler uses, enabling cross-tab coordination.
152-168: Core fix: Focus handler now uses shared lock - LGTM.This is the key change that fixes the race condition. By wrapping
refreshSessionTokenintokenRefreshLock.acquireLockAndRun(), the focus handler will coordinate with both the poller and other tabs, preventing duplicate API calls.The comment clearly explains the coordination mechanism.
| // Create shared lock for cross-tab token refresh coordination. | ||
| // This lock is used by both the poller and the focus handler to prevent | ||
| // concurrent token fetches across tabs. | ||
| this.tokenRefreshLock = SafeLock(REFRESH_SESSION_TOKEN_LOCK_KEY); |
There was a problem hiding this comment.
🤔 I'm thinking we should add the lock in getToken() itself, otherwise we'll have to patch any other places getToken() is used, and users won't have this taken care of out of the box
There was a problem hiding this comment.
Adjusted the implementation to live inside getToken()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/core/resources/Session.ts (1)
45-64: Potential memory leak:tokenLocksMap grows unboundedly.The
tokenLocksMap accumulates entries for each unique tokenId (session + org + template combination) but never removes them. Over a long-lived session where users switch organizations or use different JWT templates, this could lead to memory growth.Consider adding a cleanup mechanism, such as:
- Using a
WeakMapif the key could be an object- Adding a maximum size with LRU eviction
- Clearing entries when the session ends
For now, this is low risk since each entry is lightweight (just a SafeLock instance), but worth noting for long-term maintenance.
packages/clerk-js/src/core/auth/safeLock.ts (1)
18-22: Multiplebeforeunloadlisteners accumulate with each SafeLock instance.Since
SafeLockis called once per unique tokenId (viagetTokenLockin Session.ts), a newbeforeunloadlistener is registered for each lock instance. Over time with many different token types, this accumulates listeners.Consider:
- Moving the listener registration outside the function (module-level, once)
- Tracking all lock instances in a module-level registry and releasing them in a single listener
- Using
{ once: true }isn't suitable here since we need it to fire every time+const lockRegistry = new Map<string, Lock>(); + +// Single listener to release all locks on tab close +window.addEventListener('beforeunload', async () => { + for (const [key, lock] of lockRegistry) { + await lock.releaseLock(key); + } +}); + export function SafeLock(key: string) { const lock = new Lock(); - - // Release any held locks when the tab is closing to prevent deadlocks - // eslint-disable-next-line @typescript-eslint/no-misused-promises - window.addEventListener('beforeunload', async () => { - await lock.releaseLock(key); - }); + lockRegistry.set(key, lock);
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
.changeset/fix-token-refresh-race-condition.md(1 hunks)packages/clerk-js/src/core/auth/SessionCookiePoller.ts(2 hunks)packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts(1 hunks)packages/clerk-js/src/core/auth/safeLock.ts(1 hunks)packages/clerk-js/src/core/resources/Session.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/clerk-js/src/core/auth/SessionCookiePoller.ts
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/Session.tspackages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.tspackages/clerk-js/src/core/auth/safeLock.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (1)
packages/clerk-js/src/core/auth/SessionCookiePoller.ts (1)
SessionCookiePoller(11-39)
packages/clerk-js/src/core/auth/safeLock.ts (1)
packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
.changeset/fix-token-refresh-race-condition.md (1)
1-14: LGTM!The changeset provides clear and comprehensive documentation of the race condition fix, including the key implementation details: cross-tab locking, per-tokenId coordination, double-checked locking pattern, and graceful timeout handling.
packages/clerk-js/src/core/auth/__tests__/SessionCookiePoller.test.ts (1)
1-142: LGTM!Comprehensive test coverage for
SessionCookiePollerwith well-structured tests covering:
- Immediate callback execution on start
- Prevention of concurrent polling sessions
- Proper stop and restart behavior
- Correct scheduling that waits for callback completion
The use of fake timers and proper cleanup in each test is appropriate.
packages/clerk-js/src/core/resources/Session.ts (1)
416-473: Well-implemented double-checked locking pattern.The implementation correctly:
- Checks cache before acquiring lock (fast path)
- Re-checks cache after acquiring lock (prevents duplicate fetches when another tab populated it)
- Caches the promise immediately to prevent concurrent in-tab duplicates
- Holds the lock until the token is resolved and cached
packages/clerk-js/src/core/auth/safeLock.ts (1)
29-59: Solid lock acquisition implementation with appropriate degraded mode handling.The implementation correctly:
- Uses Web Locks API in secure contexts with abort-based timeout
- Falls back to localStorage-based locking in non-secure contexts
- Ensures operations complete in degraded mode rather than failing silently
- Properly releases locks in the fallback path via
finallyblockThe generic return type
<T>preserves type safety for callers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/clerk-js/src/core/resources/Session.ts (1)
38-64: Per-token SafeLock map looks good; consider lifecycle/typing polishThe per-tokenId SafeLock registry is a reasonable way to allow parallelism across token types while preventing duplicate fetches for the same token. One minor consideration:
tokenLocksis module-global and never pruned, so in very long-lived apps with many distinct tokenIds this Map could grow without bound. In practice tokenId cardinality is likely low, but if this ever becomes a concern, you could consider:
- Clearing entries when a session is ended/removed, or
- Introducing a small LRU or size cap.
You might also consider giving
getTokenLockan explicit return type (ReturnType<typeof SafeLock>) and extracting that into atype TokenLock = ReturnType<typeof SafeLock>;alias to make the intent clearer.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/clerk-js/src/core/resources/Session.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/core/resources/Session.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (2)
packages/clerk-js/src/core/resources/Session.ts (2)
394-407: Fast-path cache read before acquiring the lock is correctThe “check cache, then lock” flow looks solid: you avoid unnecessary lock acquisition on hot cache hits, preserve the existing TokenUpdate emission for cached tokens, and still normalize empty raw strings to
null. This aligns well with the new cross-tab locking behavior without adding contention to the common case.
409-465:skipCachesemantics are intentional and working correctly—no action neededVerification confirms the behavior matches the review's description and is intentional:
Behavior verified: When
skipCache=true, the code explicitly bypasses both pre-lock and post-lock cache reads (lines 395, 415:skipCache ? undefined : SessionTokenCache.get()), while still caching the result (line 450). ConcurrentskipCache=truecalls for the sametokenIdwill each trigger API requests rather than deduping in-flight promises.Intent is explicit: The conditional checks at lines 395 and 415 confirm this is deliberate—not an accidental oversight.
Usage context: Only one call site uses
skipCache=true(packages/clerk-js/src/core/clerk.ts:2774) during outage recovery. This scenario doesn't have multiple concurrent calls, so the edge case doesn't manifest in practice.The implementation is sound:
skipCachemeans "bypass my local cache read for this call" (ensuring fresh data), while still caching results for future calls (and forskipCache=falsecalls in other tabs). The double-check-lock pattern correctly prevents duplicate fetches betweenskipCache=falsecallers.
| // Cache the promise immediately to prevent concurrent calls from triggering duplicate requests | ||
| SessionTokenCache.set({ tokenId, tokenResolver }); |
There was a problem hiding this comment.
It's interesting that we have this here yet it doesn't solve the race condition. I suppose if N getToken calls are fired at the same time it would happen.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.changeset/fix-token-refresh-race-condition.md (1)
5-5: Refine the message to focus more on user impact than implementation.While this is already much improved, the mention of "cross-tab lock" remains implementation-focused. Consider rephrasing to emphasize the user-visible benefit (proper synchronization) rather than the mechanism (locking).
Suggested alternative:
-Fix race condition where multiple browser tabs could fetch session tokens simultaneously. `getToken()` now uses a cross-tab lock to coordinate token refresh operations +Fix race condition where multiple browser tabs could fetch session tokens simultaneously. Token refresh operations are now properly coordinated across tabs.This version emphasizes what users care about (proper synchronization) without describing the technical mechanism. Aligns with the feedback that release notes should be "light on implementation details and focus on the user impact."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
.changeset/fix-token-refresh-race-condition.md(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/clerk-js/src/utils/__tests__/lru-map.test.ts (1)
5-73: Test coverage is comprehensive for core LRU behaviors.The tests cover the essential LRU functionality: basic operations, eviction, MRU promotion, updates, and edge case with maxSize=1.
Consider adding tests for these edge cases:
maxSizeof 0: The implementation'swhile (this.size >= this.maxSize)loop could behave unexpectedly or loop infinitely ifmaxSizeis 0.- Storing
undefinedas a value: Theget()implementation usesif (value !== undefined)to decide whether to refresh access, meaning entries withundefinedvalues won't be promoted to MRU position.+ it('handles storing undefined as a value', () => { + const map = new LruMap<string, number | undefined>(3); + map.set('a', undefined); + map.set('b', 2); + map.set('c', 3); + + // Access 'a' - should still exist but won't be promoted due to implementation + expect(map.get('a')).toBeUndefined(); + expect(map.has('a')).toBe(true); + }); + + it('throws or handles maxSize of 0', () => { + // Document expected behavior + const map = new LruMap<string, number>(0); + // This could cause issues - verify expected behavior + });packages/clerk-js/src/utils/lru-map.ts (3)
10-17: Edge case: entries withundefinedvalues won't receive MRU promotion.The condition
if (value !== undefined)prevents MRU promotion for entries that storeundefinedas a value. If a key maps toundefined,get()returns it but doesn't refresh its position, making it vulnerable to early eviction.For the current usage (storing
SafeLockobjects), this isn't an issue. However, for a generic utility class, consider usingsuper.has(key)instead:override get(key: K): V | undefined { - const value = super.get(key); - if (value !== undefined) { + if (super.has(key)) { + const value = super.get(key); super.delete(key); super.set(key, value); + return value; } - return value; + return undefined; }
5-8: Consider validatingmaxSizein the constructor.A
maxSizeof 0 or negative would result in an LruMap that can never store entries (all sets immediately evict). While the implementation handles this gracefully due to thebreakcondition, explicit validation would prevent misuse and clarify intent.constructor(private maxSize: number) { super(); + if (maxSize < 1) { + throw new Error('LruMap maxSize must be at least 1'); + } }
1-8: JSDoc could be enhanced with@paramand@exampletags.Per coding guidelines, public APIs should include comprehensive JSDoc with
@param,@returns,@throws, and@exampletags./** * A simple Map with LRU (Least Recently Used) eviction. * When the map exceeds maxSize, the oldest entries are removed. + * + * @template K - The type of map keys + * @template V - The type of map values + * @param maxSize - Maximum number of entries before eviction occurs (must be >= 1) + * + * @example + * const cache = new LruMap<string, number>(3); + * cache.set('a', 1); + * cache.get('a'); // Returns 1 and marks 'a' as most recently used */
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
packages/clerk-js/src/core/resources/Session.ts(3 hunks)packages/clerk-js/src/utils/__tests__/lru-map.test.ts(1 hunks)packages/clerk-js/src/utils/lru-map.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
All code must pass ESLint checks with the project's configuration
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
TypeScript is required for all packages
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Follow established naming conventions (PascalCase for components, camelCase for variables)
Prefer importing types from
@clerk/shared/typesinstead of the deprecated@clerk/typesalias
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
packages/**/src/**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
packages/**/src/**/*.{ts,tsx,js,jsx}: Maintain comprehensive JSDoc comments for public APIs
Use tree-shaking friendly exports
Validate all inputs and sanitize outputs
All public APIs must be documented with JSDoc
Use dynamic imports for optional features
Provide meaningful error messages to developers
Include error recovery suggestions where applicable
Log errors appropriately for debugging
Lazy load components and features when possible
Implement proper caching strategies
Use efficient data structures and algorithms
Implement proper logging with different levels
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.ts?(x)
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
**/*.{ts,tsx}: Always define explicit return types for functions, especially public APIs
Use proper type annotations for variables and parameters where inference isn't clear
Avoidanytype - preferunknownwhen type is uncertain, then narrow with type guards
Implement type guards forunknowntypes using the patternfunction isType(value: unknown): value is Type
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details in classes
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Use mixins for shared behavior across unrelated classes in TypeScript
Use generic constraints with bounded type parameters like<T extends { id: string }>
Use utility types likeOmit,Partial, andPickfor data transformation instead of manual type construction
Use discriminated unions instead of boolean flags for state management and API responses
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation at the type level
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Document functions with JSDoc comments including @param, @returns, @throws, and @example tags
Create custom error classes that extend Error for specific error types
Use the Result pattern for error handling instead of throwing exceptions
Use optional chaining (?.) and nullish coalescing (??) operators for safe property access
Let TypeScript infer obvious types to reduce verbosity
Useconst assertionswithas constfor literal types
Usesatisfiesoperator for type checking without widening types
Declare readonly arrays and objects for immutable data structures
Use spread operator and array spread for immutable updates instead of mutations
Use lazy loading for large types...
Files:
packages/clerk-js/src/utils/lru-map.tspackages/clerk-js/src/utils/__tests__/lru-map.test.tspackages/clerk-js/src/core/resources/Session.ts
**/*.{test,spec}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{test,spec}.{ts,tsx,js,jsx}: Unit tests are required for all new functionality
Verify proper error handling and edge cases
Include tests for all new features
Files:
packages/clerk-js/src/utils/__tests__/lru-map.test.ts
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use real Clerk instances for integration tests
Files:
packages/clerk-js/src/utils/__tests__/lru-map.test.ts
🧬 Code graph analysis (2)
packages/clerk-js/src/utils/__tests__/lru-map.test.ts (1)
packages/clerk-js/src/utils/lru-map.ts (1)
LruMap(5-34)
packages/clerk-js/src/core/resources/Session.ts (9)
packages/clerk-js/src/utils/lru-map.ts (1)
LruMap(5-34)packages/clerk-js/src/core/auth/safeLock.ts (1)
SafeLock(15-62)packages/clerk-js/src/core/tokenCache.ts (1)
SessionTokenCache(413-413)packages/clerk-js/src/utils/debug.ts (1)
debugLogger(150-179)packages/clerk-js/src/core/events.ts (2)
eventBus(32-32)events(7-15)packages/clerk-js/rspack.config.js (1)
path(4-4)packages/clerk-js/src/core/resources/Client.ts (1)
path(161-163)packages/clerk-js/src/core/resources/User.ts (1)
path(111-113)packages/clerk-js/src/core/resources/Token.ts (1)
Token(7-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: Static analysis
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
packages/clerk-js/src/core/resources/Session.ts (4)
46-64: Well-designed per-tokenId locking with bounded memory.The implementation correctly uses LRU eviction to prevent unbounded growth while maintaining per-tokenId locks for parallel fetches of different token types.
The design is safe even if a
SafeLockwrapper is evicted while the underlying browser lock is held—navigator.locks.requestis keyed by the string key (clerk.lock.getToken.${tokenId}), not the wrapper object. A newly createdSafeLockfor the same tokenId will still correctly coordinate with the existing browser lock.
394-407: LGTM: Fast path avoids lock overhead for cache hits.The unlocked cache check for immediate hits is the correct optimization pattern—it avoids cross-tab lock acquisition overhead for the common case where a valid token is already cached.
409-466: Double-checked locking pattern correctly implemented.The implementation follows the standard pattern:
- Check cache without lock (fast path)
- Acquire lock on miss
- Re-check cache after lock (another tab may have populated it)
- Fetch and cache if still missing
One observation: when
skipCacheistrue, the code still acquires the lock but always performs a fetch. This is intentional and correct—it ensures that even explicit cache-bypassing requests are serialized to prevent thundering herd on the API.
447-450: Based on my verification of the codebase, I can now provide the rewritten review comment:
The concern about rejected promises being cached is not valid — rejected promises are automatically cleaned up.
The
tokenResolverpromise is cached immediately at line 450, but ifToken.create()rejects, thetokenCache.tsimplementation automatically removes the entry via its.catch()handler (line 368:deleteKey()). This means failed token fetches do not persist in cache. On retry (line 130-137 of Session.ts), the cache miss triggers a freshToken.create()call, allowing retries to succeed.
Description
Fix race condition where multiple browser tabs could fetch session tokens simultaneously. The
refreshTokenOnFocushandler now uses the same cross-tab lock as the session token poller, preventing duplicate API calls when switching between tabs or when focus events fire while another tab is already refreshing the token.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
Bug Fixes
Refactor
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.