test(react-router): cover auth context isolation under concurrency#8889
Merged
Conversation
clerkMiddleware stores auth in the React Router request context and getAuth reads it back. Pin that a per-request context keeps auth isolated and that a shared context leaks one request's auth into another under concurrency.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Contributor
API Changes Report
Summary
No API Changes DetectedAll packages have stable APIs with no detected changes. Report generated by Break Check Last ran on |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA new Vitest test file verifies concurrent ChangesConcurrency Isolation Tests
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-router/src/server/__tests__/clerkMiddleware.concurrency.test.ts (1)
72-101: ⚡ Quick winConsider adding an explicit return type for clarity.
The
runInterleavedhelper returns a structured object used by multiple test cases. Adding an explicit return type would improve type safety and make the function's contract clearer.📝 Suggested return type annotation
-async function runInterleaved(contextFor: (req: Request) => RouterContextProvider) { +async function runInterleaved(contextFor: (req: Request) => RouterContextProvider): Promise<{ A?: string | null; B?: string | null }> {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-router/src/server/__tests__/clerkMiddleware.concurrency.test.ts` around lines 72 - 101, The runInterleaved function lacks an explicit return type annotation, which reduces type safety and clarity of its contract. Add a return type annotation to the function signature that reflects the Promise type it returns, which should be Promise with a structure containing optional properties A and B, both of type string or null. This will make the function's return type explicit and improve type checking for callers of this helper function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@packages/react-router/src/server/__tests__/clerkMiddleware.concurrency.test.ts`:
- Around line 72-101: The runInterleaved function lacks an explicit return type
annotation, which reduces type safety and clarity of its contract. Add a return
type annotation to the function signature that reflects the Promise type it
returns, which should be Promise with a structure containing optional properties
A and B, both of type string or null. This will make the function's return type
explicit and improve type checking for callers of this helper function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5b3ca5f8-16b3-4a78-9d4e-bbf06e1c4f97
📒 Files selected for processing (1)
packages/react-router/src/server/__tests__/clerkMiddleware.concurrency.test.ts
Add braces to the per-request context check for the curly rule, drop the unused async from the authenticateRequest mock, and add an empty changeset since this is a test-only change.
🦋 Changeset detectedLatest commit: ba9ea4a The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit