test multitab token cache failure #6899
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughAdds three new Playwright integration tests that exercise multi-tab and multi-session token cache behavior: cross-tab token sharing via BroadcastChannel, single-network token fetch when caching is used, and per-tab/session isolation when multiple sessions/users exist. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TabA as Tab A
participant TabB as Tab B
participant Clerk as Clerk SDK
participant TokenAPI as Token Endpoint
participant BC as BroadcastChannel
rect rgba(230,245,255,0.6)
Note over TabA,TabB: Initialize app & Clerk in both tabs
TabA->>Clerk: init
TabB->>Clerk: init
Clerk-->>TabA: loaded
Clerk-->>TabB: loaded
end
rect rgba(235,255,235,0.6)
Note over TabA: Sign in on Tab A
TabA->>Clerk: signIn(user)
Clerk-->>TabA: sessionId, userId
Clerk->>BC: broadcast session/state
BC-->>Clerk: deliver to Tab B
Clerk-->>TabB: update session/user
end
rect rgba(255,245,230,0.6)
Note over TabA,TabB: Token fetch + broadcast sharing
TabA->>Clerk: request token (clear per-tab caches)
Clerk->>TokenAPI: network fetch (1)
TokenAPI-->>Clerk: token
Clerk->>BC: broadcast token result
TabB->>Clerk: request token (after cache cleared)
Clerk-->>TabB: return broadcasted token (no network)
end
sequenceDiagram
autonumber
participant Tab1 as Tab 1
participant Tab2 as Tab 2
participant Clerk as Clerk SDK
participant TokenAPI as Token Endpoint
rect rgba(240,235,255,0.6)
Note over Tab1: User1 signs in on Tab1
Tab1->>Clerk: signIn(user1)
Clerk-->>Tab1: sessionA, tokenA
end
rect rgba(240,235,255,0.6)
Note over Tab2: Tab2 signs in as User2 (creates separate session)
Tab2->>Clerk: signIn(user2)
Clerk-->>Tab2: sessionB, tokenB
end
rect rgba(255,245,230,0.6)
Note over Tabs: Token isolation verification
Tab1->>Clerk: getToken(for sessionA) -> tokenA (cache)
Tab2->>Clerk: getToken(for sessionB) -> tokenB (cache)
Clerk-x TokenAPI: (no network if served from cache)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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: 2
🧹 Nitpick comments (7)
integration/tests/multitab-token-cache.test.ts (7)
28-34: Use test utils to open tabs for consistency with baseURL and flags.Prefer
tabs.runInNewTabfrom yourcreateTestUtilsto ensure consistent setup (e.g., baseURL, useTestingToken flags) across pages.Apply (illustrative) change:
- const page1 = await context.newPage(); - const page2 = await context.newPage(); + const { tabs } = createTestUtils({ app, page: await context.newPage() }); + const { page: page1 } = await tabs.runInNewTab(async () => {}); + const { page: page2 } = await tabs.runInNewTab(async () => {});
46-46: Replace fixed timeouts with deterministic waits to reduce flakiness.Static sleeps make the test brittle. Wait for observable state instead.
- await page1.waitForTimeout(1000); + // wait for session propagation on page2 to be possible + await page2.waitForFunction(() => !!(window as any).Clerk?.session?.id); @@ - await page1.waitForTimeout(1500); + // ensure page1’s token fetch was observed before page2 attempts reuse + await expect + .poll(() => tokenRequests.length, { message: 'Waiting for first token fetch to be recorded' }) + .toBe(1);Based on learnings.
Also applies to: 106-106
54-68: Don’t assert private internals like_broadcastChannel. Assert behavior instead.Accessing underscored internals is brittle. Drop these checks and rely on the “single network request” assertion to prove broadcast reuse.
- const clerkInfo1 = await page1.evaluate(() => { - const clerk = (window as any).Clerk; - return { - hasBroadcastChannel: !!clerk?._broadcastChannel, - loaded: clerk?.loaded, - }; - }); - const clerkInfo2 = await page2.evaluate(() => { - const clerk = (window as any).Clerk; - return { - hasBroadcastChannel: !!clerk?._broadcastChannel, - loaded: clerk?.loaded, - }; - }); - expect(clerkInfo1.loaded).toBe(true); - expect(clerkInfo2.loaded).toBe(true); - expect(clerkInfo1.hasBroadcastChannel).toBe(true); - expect(clerkInfo2.hasBroadcastChannel).toBe(true); + // Clerk is loaded on both pages by the earlier waits; behavior is asserted below via token reuse and single fetch.Also applies to: 73-75
92-97: Interception is good; optionally add method filter and debug context.If the token endpoint is POST‑only, filter by method and record pageId for better diagnostics.
- await context.route('**/v1/client/sessions/*/tokens*', async route => { - tokenRequests.push(route.request().url()); + await context.route('**/v1/client/sessions/*/tokens*', async route => { + const req = route.request(); + if (req.method() === 'POST') { + tokenRequests.push(`${req.method()} ${req.url()}`); + } await route.continue(); });
99-105: Strengthen token assertions.Also assert the type/shape to catch regressions.
const page1Token = await page1.evaluate(async () => { const clerk = (window as any).Clerk; return await clerk.session?.getToken({ skipCache: true }); }); - expect(page1Token).toBeTruthy(); + expect(page1Token).toBeTruthy(); + expect(typeof page1Token).toBe('string');
120-127: Assert “no extra fetch” after page2 getToken.Keep the final equality but also verify no additional network call happened after page2’s request.
expect(page2Result.token).toBe(page1Token); - // Verify only one token fetch happened (page1), proving page2 got it from BroadcastChannel - expect(tokenRequests.length).toBe(1); + // Verify only one token fetch happened (page1), proving page2 got it from BroadcastChannel + expect(tokenRequests.length).toBe(1);Additionally, consider recording which page triggered the token request:
// inside the route handler: const page = req.frame()?.page(); const pid = (await page?.evaluate(() => (window as any).__PAGE_ID__)) ?? 'unknown'; tokenRequests.push(`${req.method()} ${req.url()} | ${pid}`);
128-129: Optional: wrap phases withtest.stepand close pages.Improves readability and ensures resources are released even on failure.
- }); + }).finally(async () => { + // Close pages explicitly + await Promise.allSettled(context.pages().map(p => p.close())); + });You can also group logic:
await test.step('sign in on page1', async () => { /* ... */ }); await test.step('page2 picks up session', async () => { /* ... */ }); await test.step('token broadcast and reuse', 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 (1)
integration/tests/multitab-token-cache.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
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
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
integration/tests/multitab-token-cache.test.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
integration/tests/multitab-token-cache.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{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
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
integration/tests/multitab-token-cache.test.ts
integration/**
📄 CodeRabbit inference engine (.cursor/rules/global.mdc)
Framework integration templates and E2E tests should be placed under the integration/ directory
Files:
integration/tests/multitab-token-cache.test.ts
integration/**/*
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
End-to-end tests and integration templates must be located in the 'integration/' directory.
Files:
integration/tests/multitab-token-cache.test.ts
integration/**/*.{test,spec}.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Integration tests should use Playwright.
Files:
integration/tests/multitab-token-cache.test.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
integration/tests/multitab-token-cache.test.ts
🧬 Code graph analysis (1)
integration/tests/multitab-token-cache.test.ts (2)
integration/testUtils/index.ts (1)
createTestUtils(24-86)integration/presets/index.ts (1)
appConfigs(15-32)
⏰ 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: Analyze (javascript-typescript)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
integration/tests/multitab-token-cache.test.ts (3)
38-44: Explicitly disable testing tokens to ensure a real network fetch.If your page objects set
useTestingTokenby default, this test may observe 0 network calls. PassuseTestingToken: falsefor both pages.- const u1 = createTestUtils({ app, page: page1 }); + const u1 = createTestUtils({ app, page: page1, useTestingToken: false }); @@ - const u2 = createTestUtils({ app, page: page2 }); + const u2 = createTestUtils({ app, page: page2, useTestingToken: false });Can you confirm whether
useTestingTokenaffectsClerk.session.getTokenin your app under test?Also applies to: 51-53
23-26: Double teardown check.If
testAgainstRunningAppsalready tears the app down, this additionalapp.teardown()could be redundant or race with the harness. Please confirm.
1-9: Overall: solid scenario coverage for multi‑tab token reuse.Good end‑to‑end flow: sign‑in in tab A, verify tab B session, clear caches, assert single fetch and shared token. With the wait/broadcast tweaks above, this should be reliable in CI.
| await page1.waitForFunction(() => (window as any).Clerk?.loaded); | ||
| await page2.waitForFunction(() => (window as any).Clerk?.loaded); | ||
|
|
There was a problem hiding this comment.
Avoid truthy “loaded” checks — can pass too early.
waitForFunction(() => Clerk?.loaded) may resolve immediately if loaded is a Promise/object. Check explicitly for === true and a ready session.
- await page1.waitForFunction(() => (window as any).Clerk?.loaded);
- await page2.waitForFunction(() => (window as any).Clerk?.loaded);
+ await page1.waitForFunction(() => {
+ const c = (window as any).Clerk;
+ return !!c && c.loaded === true && !!c.session?.id;
+ });
+ await page2.waitForFunction(() => {
+ const c = (window as any).Clerk;
+ return !!c && c.loaded === true && !!c.session?.id;
+ });Based on learnings.
Also applies to: 49-50
🤖 Prompt for AI Agents
In integration/tests/multitab-token-cache.test.ts around lines 35-37 (also apply
same change at 49-50), the test uses a truthy check await
page.waitForFunction(() => (window as any).Clerk?.loaded) which can return
prematurely if loaded is a Promise/object; update the predicate to explicitly
check for boolean true and a ready session — e.g., waitForFunction(() => (window
as any).Clerk?.loaded === true && (window as any).Clerk.session !== undefined &&
(window as any).Clerk.session !== null) so the function only resolves when
loaded is strictly true and a session is present.
| await Promise.all([ | ||
| page1.evaluate(() => (window as any).Clerk.session?.clearCache()), | ||
| page2.evaluate(() => (window as any).Clerk.session?.clearCache()), | ||
| ]); | ||
|
|
There was a problem hiding this comment.
Guard clearCache() in case the API is unavailable.
If session.clearCache isn’t present in some builds, this will throw inside the page.
- await Promise.all([
- page1.evaluate(() => (window as any).Clerk.session?.clearCache()),
- page2.evaluate(() => (window as any).Clerk.session?.clearCache()),
- ]);
+ await Promise.all([
+ page1.evaluate(async () => {
+ const s = (window as any).Clerk?.session;
+ if (s && typeof s.clearCache === 'function') await s.clearCache();
+ }),
+ page2.evaluate(async () => {
+ const s = (window as any).Clerk?.session;
+ if (s && typeof s.clearCache === 'function') await s.clearCache();
+ }),
+ ]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await Promise.all([ | |
| page1.evaluate(() => (window as any).Clerk.session?.clearCache()), | |
| page2.evaluate(() => (window as any).Clerk.session?.clearCache()), | |
| ]); | |
| await Promise.all([ | |
| page1.evaluate(async () => { | |
| const s = (window as any).Clerk?.session; | |
| if (s && typeof s.clearCache === 'function') await s.clearCache(); | |
| }), | |
| page2.evaluate(async () => { | |
| const s = (window as any).Clerk?.session; | |
| if (s && typeof s.clearCache === 'function') await s.clearCache(); | |
| }), | |
| ]); |
🤖 Prompt for AI Agents
In integration/tests/multitab-token-cache.test.ts around lines 87 to 91, the
test directly calls page1.evaluate and page2.evaluate invoking (window as
any).Clerk.session?.clearCache(), which can throw if clearCache is not present
in some builds; update the page.evaluate calls to check that (window as
any).Clerk?.session?.clearCache is a function before calling it (e.g., use
optional chaining and typeof check inside the page context) so the call is only
executed when available, preventing runtime errors in builds without that API.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
integration/tests/multitab-token-cache.test.ts (2)
35-36: Explicit boolean check forloadedis still needed.The truthy check
Clerk?.loadedcan resolve prematurely ifloadedis a Promise or object. As flagged in the previous review, use an explicit=== truecheck along with session readiness verification.Based on learnings.
Also applies to: 49-50
87-90: Guard against missingclearCacheAPI.As noted in the previous review, calling
clearCache()without checking if it's a function can throw if the API is unavailable in some builds.
🧹 Nitpick comments (3)
integration/tests/multitab-token-cache.test.ts (3)
46-46: Replace hard-coded timeout with condition-based wait.Arbitrary
waitForTimeout(1000)suggests a race condition. Wait for a specific state (e.g., session token availability or broadcast message) instead of a fixed delay.- await page1.waitForTimeout(1000); + // Wait for session to be fully established + await page1.waitForFunction(() => { + const c = (window as any).Clerk; + return c?.session?.id && c?.user?.id; + });
73-74: Clarify or remove commented-out BroadcastChannel assertions.The test collects
hasBroadcastChanneldata but doesn't assert it. Either enable these assertions if they're meant to pass, or remove the dead code if BroadcastChannel verification isn't the test's goal.
106-106: Replace hard-coded timeout with BroadcastChannel state verification.The 1500ms delay appears to wait for token broadcast. Instead of an arbitrary timeout, verify that page2 can retrieve the token from cache or wait for a broadcast event.
- await page1.waitForTimeout(1500); + // Wait for token to be available in page2's cache via BroadcastChannel + await page2.waitForFunction(async () => { + const clerk = (window as any).Clerk; + // Check if token cache has the token without forcing a fetch + return clerk?.session?.__experimental_tokenCache?.has('__default_token__'); + }, { timeout: 5000 });Note: The exact cache check may vary based on internal implementation. Consider adding a more reliable signal or event.
📜 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)
integration/tests/multitab-token-cache.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
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
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
integration/tests/multitab-token-cache.test.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
integration/tests/multitab-token-cache.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{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
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
integration/tests/multitab-token-cache.test.ts
integration/**
📄 CodeRabbit inference engine (.cursor/rules/global.mdc)
Framework integration templates and E2E tests should be placed under the integration/ directory
Files:
integration/tests/multitab-token-cache.test.ts
integration/**/*
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
End-to-end tests and integration templates must be located in the 'integration/' directory.
Files:
integration/tests/multitab-token-cache.test.ts
integration/**/*.{test,spec}.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Integration tests should use Playwright.
Files:
integration/tests/multitab-token-cache.test.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
integration/tests/multitab-token-cache.test.ts
🧬 Code graph analysis (1)
integration/tests/multitab-token-cache.test.ts (2)
integration/testUtils/index.ts (2)
testAgainstRunningApps(88-88)createTestUtils(24-86)integration/presets/index.ts (1)
appConfigs(15-32)
⏰ 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: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
integration/tests/multitab-token-cache.test.ts (1)
92-97: Potential race between route setup and token fetch.
context.route()is async, and the firstgetToken()call happens immediately after (line 99). If route interception isn't ready, the first token fetch might not be captured, causing the assertion at line 126 to fail incorrectly.Add a small delay or a verification step after route setup:
await context.route('**/v1/client/sessions/*/tokens*', async route => { tokenRequests.push(route.request().url()); await route.continue(); }); + // Small delay to ensure route interception is active + await page1.waitForTimeout(100);Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
integration/tests/session-token-cache/single-session.test.ts (2)
49-130: Add explicit return type annotation.The test function lacks an explicit return type. Per coding guidelines, functions should have explicit return types, especially in TypeScript files.
As per coding guidelines.
Apply this diff:
- test('MemoryTokenCache multi-tab token sharing', async ({ context }) => { + test('MemoryTokenCache multi-tab token sharing', async ({ context }): Promise<void> => {
56-57: Consider extracting Clerk load wait into a helper.The pattern
await page.waitForFunction(() => (window as any).Clerk?.loaded)is repeated throughout both test files. Extracting this into a reusable helper would improve maintainability.For example, in a test utilities file:
export async function waitForClerkLoaded(page: Page): Promise<void> { await page.waitForFunction(() => (window as any).Clerk?.loaded); }Then use:
await waitForClerkLoaded(page1); await waitForClerkLoaded(page2);integration/tests/session-token-cache/multi-session.test.ts (2)
54-228: Add explicit return type annotation.The test function lacks an explicit return type. Per coding guidelines, functions should have explicit return types.
As per coding guidelines.
Apply this diff:
test('MemoryTokenCache multi-session - multiple users in different tabs with separate token caches', async ({ context, - }) => { + }): Promise<void> => {
110-137: Consider extracting programmatic sign-in logic.The programmatic sign-in using
clerk.client.signIn.create()is a complex operation that could be extracted into a reusable helper function for better maintainability and reuse across tests.For example:
async function programmaticSignIn( page: Page, email: string, password: string ): Promise<{ success: boolean; sessionCount: number; allSessions: Array<{ id: string; userId: string }> }> { return await page.evaluate( async ({ email, password }) => { const clerk = (window as any).Clerk; try { const signIn = await clerk.client.signIn.create({ identifier: email, password: password, }); await clerk.setActive({ session: signIn.createdSessionId }); return { allSessions: clerk?.client?.sessions?.map((s: any) => ({ id: s.id, userId: s.userId })) || [], sessionCount: clerk?.client?.sessions?.length || 0, success: true, }; } catch (error: any) { return { error: error.message || String(error), success: false, }; } }, { email, password } ); }
📜 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 (2)
integration/tests/session-token-cache/multi-session.test.ts(1 hunks)integration/tests/session-token-cache/single-session.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
**/*.{js,jsx,ts,tsx}: All code must pass ESLint checks with the project's configuration
Follow established naming conventions (PascalCase for components, camelCase for variables)
Maintain comprehensive JSDoc comments for public APIs
Use dynamic imports for optional features
All public APIs must be documented with JSDoc
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
Profile and optimize critical paths
Validate all inputs and sanitize outputs
Implement proper logging with different levels
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
**/*.{js,jsx,ts,tsx,json,css,scss,md,yaml,yml}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use Prettier for consistent code formatting
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Use proper TypeScript error types
**/*.{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
Useinterfacefor object shapes that might be extended
Usetypefor unions, primitives, and computed types
Preferreadonlyproperties for immutable data structures
Useprivatefor internal implementation details
Useprotectedfor inheritance hierarchies
Usepublicexplicitly for clarity in public APIs
Preferreadonlyfor properties that shouldn't change after construction
Prefer composition and interfaces over deep inheritance chains
Use mixins for shared behavior across unrelated classes
Implement dependency injection for loose coupling
Let TypeScript infer when types are obvious
Useconst assertionsfor literal types:as const
Usesatisfiesoperator for type checking without widening
Use mapped types for transforming object types
Use conditional types for type-level logic
Leverage template literal types for string manipulation
Use ES6 imports/exports consistently
Use default exports sparingly, prefer named exports
Use type-only imports:import type { ... } from ...
Noanytypes without justification
Proper error handling with typed errors
Consistent use ofreadonlyfor immutable data
Proper generic constraints
No unused type parameters
Proper use of utility types instead of manual type construction
Type-only imports where possible
Proper tree-shaking friendly exports
No circular dependencies
Efficient type computations (avoid deep recursion)
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
integration/**
📄 CodeRabbit inference engine (.cursor/rules/global.mdc)
Framework integration templates and E2E tests should be placed under the integration/ directory
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
integration/**/*
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
End-to-end tests and integration templates must be located in the 'integration/' directory.
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
integration/**/*.{test,spec}.{js,ts}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Integration tests should use Playwright.
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
**/*.{js,ts,tsx,jsx}
📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Support multiple Clerk environment variables (CLERK_, NEXT_PUBLIC_CLERK_, etc.) for configuration.
Files:
integration/tests/session-token-cache/single-session.test.tsintegration/tests/session-token-cache/multi-session.test.ts
🧬 Code graph analysis (2)
integration/tests/session-token-cache/single-session.test.ts (2)
integration/testUtils/index.ts (1)
createTestUtils(24-86)integration/presets/index.ts (1)
appConfigs(15-32)
integration/tests/session-token-cache/multi-session.test.ts (2)
integration/testUtils/index.ts (1)
createTestUtils(24-86)integration/presets/index.ts (1)
appConfigs(15-32)
⏰ 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). (27)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (tanstack-react-router, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (quickstart, chrome)
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (18, --filter=@clerk/astro --filter=@clerk/backend --filter=@clerk/express --filter=@c...
- GitHub Check: Static analysis
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
integration/tests/session-token-cache/multi-session.test.ts (3)
190-207: Good use of token cache isolation verification.The test correctly verifies that switching between sessions yields different tokens, and that no network requests occur (proving tokens are served from cache). This is a thorough validation of the token cache isolation behavior.
214-214: Proper cleanup of route handler.Good practice to call
unroute()after the route handler is no longer needed. This prevents interference with subsequent test assertions.
15-15: No action needed on environment config. The use ofwithSessionTasksvs.withEmailCodesis intentional to enable multi-session behaviors.integration/tests/session-token-cache/single-session.test.ts (1)
87-90: clearCache method exists and clears in-memory session cache
SessionResource.clearCache() is implemented (core/resources/Session.ts calls SessionTokenCache.clear()) and exposed on window.Clerk.session; no changes required.
| // eslint-disable-next-line playwright/no-wait-for-timeout | ||
| await page2.waitForTimeout(1000); |
There was a problem hiding this comment.
Replace hard-coded timeout with deterministic wait.
Similar to the single-session test, this hard-coded timeout is brittle. Consider waiting for a specific condition that indicates the session has been fully propagated to page2.
Apply this diff:
- // eslint-disable-next-line playwright/no-wait-for-timeout
- await page2.waitForTimeout(1000);
+ await page2.waitForFunction(() => (window as any).Clerk?.session?.id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // eslint-disable-next-line playwright/no-wait-for-timeout | |
| await page2.waitForTimeout(1000); | |
| await page2.waitForFunction(() => (window as any).Clerk?.session?.id); |
🤖 Prompt for AI Agents
integration/tests/session-token-cache/multi-session.test.ts around lines 91-92:
replace the brittle hard-coded await page2.waitForTimeout(1000) with a
deterministic wait that detects when the session has actually propagated to
page2 (for example, use page2.waitForFunction to poll for the expected session
token in localStorage/sessionStorage, use page2.waitForResponse to wait for the
known network call that completes session sync, or use page2.waitForSelector to
wait for a UI element that appears only when the session is present). Ensure the
condition you wait for is the same signal the single-session test uses (token
key name or element), add a sensible timeout, and remove the waitForTimeout
line.
| await u1.po.expect.toBeSignedIn(); | ||
|
|
||
| // eslint-disable-next-line playwright/no-wait-for-timeout | ||
| await page1.waitForTimeout(1000); |
There was a problem hiding this comment.
Replace hard-coded timeout with deterministic wait.
Hard-coded timeouts are brittle and can cause flakiness. Consider waiting for a specific condition instead, such as waiting for the session to be fully propagated.
For example, you could wait for a specific session state or element:
- // eslint-disable-next-line playwright/no-wait-for-timeout
- await page1.waitForTimeout(1000);
+ await page1.waitForFunction(() => (window as any).Clerk?.session?.id);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| await page1.waitForTimeout(1000); | |
| await page1.waitForFunction(() => (window as any).Clerk?.session?.id); |
🤖 Prompt for AI Agents
In integration/tests/session-token-cache/single-session.test.ts around line 68,
replace the brittle await page1.waitForTimeout(1000) with a deterministic wait
that polls for the actual session propagation condition (e.g., waitForSelector
for a DOM element that appears when the session is ready, waitForResponse that
matches the session propagation API call, or loop with page.evaluate checking a
session flag until true with a timeout). Ensure the replacement explicitly waits
for the specific state used by the test (selector, network response, or
evaluated session value) and include a reasonable overall timeout to fail fast.
| // Wait for broadcast to propagate between tabs (broadcast is nearly instant, but we add buffer) | ||
| // eslint-disable-next-line playwright/no-wait-for-timeout | ||
| await page2.waitForTimeout(2000); |
There was a problem hiding this comment.
Replace hard-coded timeout with event-based wait.
The 2-second timeout assumes broadcast propagation time but is non-deterministic. Consider listening for a BroadcastChannel message or polling for cache state instead.
For example, if the implementation exposes cache state, you could poll:
- // Wait for broadcast to propagate between tabs (broadcast is nearly instant, but we add buffer)
- // eslint-disable-next-line playwright/no-wait-for-timeout
- await page2.waitForTimeout(2000);
+ // Wait for the token to appear in page2's cache via broadcast
+ await page2.waitForFunction(
+ async () => {
+ const clerk = (window as any).Clerk;
+ const cached = await clerk.session?.__internal_getTokenCache?.();
+ return cached !== null;
+ },
+ { timeout: 5000 }
+ );Note: This assumes an internal API for checking cache state. If not available, consider adding one for testability, or accept the timeout as a pragmatic trade-off with increased duration for safety.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Wait for broadcast to propagate between tabs (broadcast is nearly instant, but we add buffer) | |
| // eslint-disable-next-line playwright/no-wait-for-timeout | |
| await page2.waitForTimeout(2000); | |
| // Wait for the token to appear in page2's cache via broadcast | |
| await page2.waitForFunction( | |
| async () => { | |
| const clerk = (window as any).Clerk; | |
| const cached = await clerk.session?.__internal_getTokenCache?.(); | |
| return cached !== null; | |
| }, | |
| { timeout: 5000 } | |
| ); |
🤖 Prompt for AI Agents
In integration/tests/session-token-cache/single-session.test.ts around lines 106
to 108, replace the hard-coded 2s waitForTimeout with an event-based wait:
either subscribe to the BroadcastChannel message emitted by the app and await
that message in the test, or poll the page state (e.g., via page.evaluate
calling an exposed test hook or reading the cache state) until the expected
cache change appears, with a sensible overall timeout and short poll interval;
if the app lacks a test hook for cache state, add a minimal read-only accessor
or a test-only BroadcastChannel message to make the wait deterministic.
Description
Running this test against main to verify that it fails correctly
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit