fix: improve session handling to avoid calling session refresh logic consecutively#3083
fix: improve session handling to avoid calling session refresh logic consecutively#3083
Conversation
mcstepp
requested review from
chronark,
ogzhanolguncu and
perkinsjr
as code owners
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThis pull request refactors the authentication flow across several modules. It replaces namespaced method calls (e.g., Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (15)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/new/page.tsx (1)
3-26: Consider removing unused variable
The_authvariable is assigned but not used. Removing it helps keep the code clean and maintainable.- const _auth = await getAuth();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/auth/layout.tsx(2 hunks)apps/dashboard/app/new/create-ratelimit.tsx(3 hunks)apps/dashboard/app/new/page.tsx(2 hunks)apps/dashboard/lib/auth.ts(1 hunks)apps/dashboard/lib/auth/base-provider.ts(1 hunks)apps/dashboard/lib/auth/get-auth.ts(0 hunks)apps/dashboard/lib/auth/utils.ts(1 hunks)apps/dashboard/lib/trpc/context.ts(1 hunks)apps/dashboard/middleware.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/lib/auth/get-auth.ts
🧰 Additional context used
🧬 Code Definitions (5)
apps/dashboard/app/auth/layout.tsx (1)
apps/dashboard/lib/auth.ts (1)
getCurrentUser(64-72)
apps/dashboard/lib/auth/utils.ts (2)
apps/dashboard/lib/auth.ts (1)
getAuth(24-31)apps/dashboard/lib/auth/get-auth.ts (1)
getAuth(10-64)
apps/dashboard/app/new/create-ratelimit.tsx (1)
apps/dashboard/lib/auth.ts (1)
getCurrentUser(64-72)
apps/dashboard/app/new/page.tsx (2)
apps/dashboard/lib/auth.ts (1)
getAuth(24-31)apps/dashboard/lib/auth/get-auth.ts (1)
getAuth(10-64)
apps/dashboard/lib/auth.ts (2)
apps/dashboard/lib/auth/get-auth.ts (1)
getAuth(10-64)apps/dashboard/lib/auth/types.ts (1)
User(9-17)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Test Agent Local / test_agent_local
- GitHub Check: Test Packages / Test ./packages/hono
- GitHub Check: Test Go API Local / Test (Shard 7/8)
- GitHub Check: Test Packages / Test ./packages/nextjs
- GitHub Check: Test Packages / Test ./internal/clickhouse
- GitHub Check: Test Packages / Test ./internal/keys
- GitHub Check: Test Packages / Test ./internal/billing
- GitHub Check: Build / Build
- GitHub Check: Test API / API Test Local
🔇 Additional comments (15)
apps/dashboard/middleware.ts (1)
28-28: Added authentication refresh endpoint to public pathsAdding
/api/auth/refreshto the public paths aligns with the PR objective of improving session handling to prevent consecutive refresh token calls. This change allows the refresh endpoint to be accessed without triggering authentication checks, which helps avoid the circular issue of refreshing auth during an auth refresh operation.apps/dashboard/lib/trpc/context.ts (1)
4-4: Updated import path for getAuthThe import path has been modified from an absolute path to a relative path. This change doesn't affect functionality but might improve code organization and module resolution.
apps/dashboard/app/auth/layout.tsx (2)
4-4: Replaced auth.getCurrentUser with cached getCurrentUser functionThis change aligns with the PR objective of leveraging React Server Components caching for authentication. By using the cached
getCurrentUserfunction, you're avoiding redundant auth calls that could trigger session refresh logic multiple times.
78-78: Updated user retrieval methodReplacing
auth.getCurrentUser()with the new cachedgetCurrentUser()function. This implements the PR's goal of leveraging RSC caching to avoid consecutive authentication calls, especially when edge middleware is running.apps/dashboard/app/new/create-ratelimit.tsx (3)
3-3: Migrated to cached getCurrentUser functionUsing the imported cached
getCurrentUserfunction instead ofauth.getCurrentUser()aligns with the PR objective of leveraging React Server Components caching for authentication operations.
16-16: Simplified user authenticationReplaced
auth.getCurrentUser()with the new cachedgetCurrentUser()function. This change benefits from RSC caching to avoid multiple authentication calls when components are rendered.
28-28: Simplified organization ID retrievalInstead of making a separate call to
getOrgId(), the code now usesuser.orgId!which comes from the enhancedgetCurrentUser()function. This reduces redundant async calls and potential session refreshes.apps/dashboard/lib/auth/utils.ts (2)
4-7: Imports align with the updated flow
The addition ofgetAuthandUNKEY_SESSION_COOKIEimports is consistent with the revised authentication approach.
10-15: Verify usage of the new return type
requireAuthnow returns{ userId, orgId }instead of a user object. Confirm that any existing callers handle the updated return type without expecting the old user shape.apps/dashboard/lib/auth/base-provider.ts (3)
137-137: Header addition looks good
Using thex-middleware-processedheader is a useful way to mark middleware processing.
141-160: Clear and comprehensive docstring
The new documentation forcreateMiddlewareclearly explains its purpose and usage.
170-182: Potential session validity gap
This middleware only checks for a cookie’s presence, not its validity or expiration. Ensure the server components downstream fully validate session tokens to prevent unauthorized requests.apps/dashboard/lib/auth.ts (3)
24-31: Redirect behavior check
getAuthredirects if no user is found. Double-check that this doesn't inadvertently affect pages meant to be publicly accessible.
43-51: Straightforward org check
When noorgIdis found, redirecting to/newis a concise flow. This seems fine, but ensure it aligns with future multi-org expansions if planned.
64-72: Ensures valid user
getCurrentUserconfirms bothuserIdand a corresponding user record. This aligns with the consolidated authentication model.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/dashboard/app/new/page.tsx (1)
27-27: Consider renaming the unused variable.The variable
_authis appropriately prefixed with an underscore to indicate it's not directly used, but the code could be even clearer by usingawait getAuth()directly without assignment, since you're only using it for its side-effect of redirecting unauthenticated users.- const _auth = await getAuth(); + await getAuth();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
apps/dashboard/app/auth/layout.tsx(2 hunks)apps/dashboard/app/new/create-ratelimit.tsx(3 hunks)apps/dashboard/app/new/page.tsx(2 hunks)apps/dashboard/lib/auth.ts(1 hunks)apps/dashboard/lib/auth/base-provider.ts(1 hunks)apps/dashboard/lib/auth/get-auth.ts(0 hunks)apps/dashboard/lib/auth/utils.ts(1 hunks)apps/dashboard/lib/trpc/context.ts(1 hunks)apps/dashboard/middleware.ts(1 hunks)
💤 Files with no reviewable changes (1)
- apps/dashboard/lib/auth/get-auth.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- apps/dashboard/middleware.ts
- apps/dashboard/lib/trpc/context.ts
- apps/dashboard/app/auth/layout.tsx
- apps/dashboard/app/new/create-ratelimit.tsx
- apps/dashboard/lib/auth/utils.ts
🧰 Additional context used
🧬 Code Definitions (2)
apps/dashboard/app/new/page.tsx (2)
apps/dashboard/lib/auth.ts (1)
getAuth(32-39)apps/dashboard/lib/auth/get-auth.ts (1)
getAuth(10-64)
apps/dashboard/lib/auth.ts (3)
apps/dashboard/lib/auth/get-auth.ts (1)
getAuth(10-64)apps/dashboard/lib/auth/workos.ts (1)
getCurrentUser(128-155)apps/dashboard/lib/auth/types.ts (1)
User(9-18)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Test Packages / Test ./internal/clickhouse
🔇 Additional comments (8)
apps/dashboard/app/new/page.tsx (1)
3-3: Good update to the auth import.The switch from importing the
authnamespace to importing the specificgetAuthfunction directly improves code clarity and aligns with the PR objective of leveraging RSC caching for authentication functions.apps/dashboard/lib/auth/base-provider.ts (3)
137-137: Good addition of the middleware tracking header.Adding
x-middleware-processedheader helps with debugging and provides visibility into the request processing pipeline.
141-160: Excellent documentation for the middleware factory.The added JSDoc comment clearly explains the purpose, behavior, and usage of the middleware function. The example is particularly helpful for implementers.
170-182: Well-optimized session verification approach.The middleware has been appropriately simplified to only perform lightweight cookie presence checks at the edge, delegating full authentication to server components. This aligns perfectly with the PR objective to streamline middleware and relocate session handling to RSC authentication components.
This change should help prevent the refresh token invalidation issues mentioned in the PR description by reducing duplicate validation calls in the request pipeline.
apps/dashboard/lib/auth.ts (4)
1-10: Good organization of authentication utilities.The separation of cached and non-cached versions of
getAuthis well-structured. The type definition forGetAuthResultis clear and appropriately placed.
20-39: Well-implemented caching strategy for authentication.The caching implementation for
getAuthis excellent. Using React'scachefunction ensures that:
- Authentication checks are performed only once per server request
- Results are consistent across components rendered during the same request
- Performance is optimized by eliminating redundant API calls
This directly addresses the PR objective of "leveraging RSC caching for authentication functions" and will help prevent the refresh token invalidation issues described in the PR.
41-59: Clean implementation of organization ID retrieval.The
getOrgIdfunction now leverages the cachedgetAuthfunction, maintaining a clean separation of concerns while ensuring consistent authentication state throughout the request lifecycle. The comprehensive JSDoc comments make the function's purpose and behavior clear.
61-80: Excellent addition of the getCurrentUser utility.The new
getCurrentUserfunction provides a complete user object while maintaining proper authentication checks. It correctly:
- Leverages the cached
getAuthto ensure consistent authentication state- Fetches the complete user object using the userId
- Includes proper error handling with redirection
- Returns a complete user object with organization context
This function will make it easier to access user data across the application while maintaining the performance benefits of caching.
|
All looks good now |
|
I'll be the judge of that.... :bugeyes: |
perkinsjr
left a comment
perkinsjr
left a comment
There was a problem hiding this comment.
Alrighty it looks good both code sniff and testing everything.
4 participants
What does this PR do?
Objective: Fix session refresh logic to not invalidate our refresh token with edge middleware running repeatedly
Type of change
How should this be tested?
Checklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
Refactor