♻️ Make user session optional #1515
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThis pull request introduces comprehensive changes to user session management across the web application. The modifications primarily focus on enhancing the handling of user sessions, making them more robust by introducing optional chaining, simplifying authentication checks, and providing more flexible guest user support. The changes span multiple files, affecting authentication, routing, middleware, and component-level logic, with a consistent approach to handling potentially undefined user sessions. Changes
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
apps/web/src/trpc/routers/polls/comments.ts (1)
21-21: Deprecation comment
ParameterhideParticipantsis marked as deprecated. Consider removing it fully once it’s no longer used to avoid confusion.apps/web/src/trpc/context.ts (1)
3-9: Check necessity of a mandatorygetEmailClient
getEmailClientis non-optional inUser, which may not always be relevant for guests. If guests will never call this method, consider making it optional or giving it a no-op implementation for guests to avoid confusion.apps/web/src/trpc/server/create-ssr-helper.ts (1)
18-18: Consider improving the guest user check.The current check
isGuest: session.user.email === nullcould be more explicit about the guest user condition.Consider this alternative:
-isGuest: session.user.email === null, +isGuest: !session.user.email,This change:
- Handles both
nullandundefinedcases- More clearly expresses the intent that a guest user is one without an email
apps/web/src/app/api/stripe/checkout/route.ts (1)
23-23: Simplify the null check condition.The current condition uses double optional chaining which might be overcautious:
if (!userSession?.user || userSession.user?.email === null)Consider simplifying to:
-if (!userSession?.user || userSession.user?.email === null) +if (!userSession?.user?.email)This achieves the same safety with cleaner code.
apps/web/src/components/poll/mutations.ts (1)
Line range hint
31-42: Consider optimistic updates for better UX.The mutation could benefit from optimistic updates to improve the user experience.
Consider implementing optimistic updates by moving the participant addition logic to
onMutateand handling rollback inonError.apps/web/src/components/user-provider.tsx (1)
89-105: LGTM! Consider enhancing type safety.The defensive user object construction with fallback values aligns well with making the user session optional.
Consider extracting the guest user object into a constant to improve maintainability:
+const GUEST_USER: UserData = { + id: "", + name: t("guest"), + isGuest: true, + tier: "guest", +}; - : { - id: "", - name: t("guest"), - isGuest: true, - tier: "guest", - }, + : GUEST_USER,apps/web/src/trpc/routers/user.ts (1)
70-72: Consider using type narrowing for better type safety.The null check is good, but we can improve type safety using type narrowing.
Consider this approach:
- subscription: publicProcedure.query( - async ({ ctx }): Promise<{ legacy?: boolean; active: boolean }> => { - if (!ctx.user || ctx.user.isGuest) { + subscription: publicProcedure.query(async ({ ctx }) => { + if (!ctx.user?.id || ctx.user.isGuest) {This ensures we specifically check for the required user property.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/web/declarations/next-auth.d.ts(1 hunks)apps/web/src/app/[locale]/(auth)/auth/login/components/login-page.tsx(1 hunks)apps/web/src/app/api/logout/route.ts(0 hunks)apps/web/src/app/api/notifications/unsubscribe/route.ts(1 hunks)apps/web/src/app/api/stripe/checkout/route.ts(1 hunks)apps/web/src/app/api/stripe/portal/route.ts(1 hunks)apps/web/src/app/api/trpc/[trpc]/route.ts(1 hunks)apps/web/src/app/api/user/verify-email-change/route.ts(1 hunks)apps/web/src/app/guest.ts(1 hunks)apps/web/src/auth.ts(4 hunks)apps/web/src/components/create-poll.tsx(3 hunks)apps/web/src/components/discussion/discussion.tsx(4 hunks)apps/web/src/components/poll/mutations.ts(3 hunks)apps/web/src/components/user-provider.tsx(1 hunks)apps/web/src/middleware.ts(2 hunks)apps/web/src/pages/api/auth/[...nextauth].ts(1 hunks)apps/web/src/trpc/context.ts(1 hunks)apps/web/src/trpc/routers/auth.ts(2 hunks)apps/web/src/trpc/routers/polls.ts(5 hunks)apps/web/src/trpc/routers/polls/comments.ts(2 hunks)apps/web/src/trpc/routers/polls/participants.ts(2 hunks)apps/web/src/trpc/routers/user.ts(1 hunks)apps/web/src/trpc/server/create-ssr-helper.ts(1 hunks)apps/web/src/trpc/trpc.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/app/api/logout/route.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Linting
- GitHub Check: Type check
- GitHub Check: Integration tests
🔇 Additional comments (34)
apps/web/src/trpc/trpc.ts (4)
6-6: Import statement looks good
No issues found with importingisQuickCreateEnabled.
27-35: Quick Create logic is consistent
This logic aligns well with the PR's objective to optionally allow guest usage when Quick Create is enabled.
57-71: Private procedure check appears solid
Ensures that guests cannot access private procedures. No immediate issues found.
73-90: Subscription check is well-structured
This logic properly handles both self-hosted and paid environments. Looks good as is.apps/web/src/trpc/routers/polls/comments.ts (3)
8-13: Import usage validated
ImportingrequireUserMiddlewareis consistent with your new authentication flow.
66-66: Unconditional return overshadowing hidden logic
Since line 66 always returns the full comment list, ensure that restricted lists are returned earlier whenhideParticipantsis enabled. Otherwise, the logic at lines 49–61 has no effect.
76-76: Authentication requirement enforced
AddingrequireUserMiddlewaretoaddensures only authenticated users can add comments. Good move.apps/web/src/trpc/context.ts (1)
12-13: Optional user property aligns with PR objective
No issues found. Making theuserproperty optional is consistent with the flexible session handling approach.apps/web/src/pages/api/auth/[...nextauth].ts (1)
12-12: LGTM! Good security practice.Adding
Cache-Control: no-storeheader is essential for auth endpoints to prevent caching of sensitive session data.apps/web/src/app/guest.ts (1)
3-3: LGTM! Clean simplification.The removal of guest user initialization logic aligns with the PR objective of making guest users optional. Good job preserving the necessary language detection functionality while removing unused code.
apps/web/declarations/next-auth.d.ts (1)
13-13: Verify all session.user accesses across the codebase.Making
useroptional in the Session interface is the key change enabling optional guest users. This change requires proper null checking throughout the codebase.Let's verify all session.user accesses to ensure they handle the optional user properly:
✅ Verification successful
All session.user accesses properly handle the optional user property
The codebase correctly handles the optional user property through:
- Optional chaining with
session?.user- Authentication guards with login redirects
- Safe assignments in authenticated contexts
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potentially unsafe session.user accesses echo "Searching for direct session.user accesses that might need optional chaining..." rg "session\.user[^?]" --type ts echo -e "\nSearching for destructuring that might need optional chaining..." rg "const.*\{.*user.*\}.*=.*session" --type tsLength of output: 1733
Script:
#!/bin/bash # Check if unsubscribe route has authentication middleware echo "Checking unsubscribe route authentication:" rg -B 5 -A 5 "userId: session.user.id" apps/web/src/app/api/notifications/unsubscribe/route.tsLength of output: 414
Script:
#!/bin/bash # Check complete unsubscribe route implementation echo "Checking complete unsubscribe route implementation:" rg -B 20 -A 20 "export async function" apps/web/src/app/api/notifications/unsubscribe/route.tsLength of output: 209
Script:
#!/bin/bash # Get route handler implementation using ast-grep echo "Checking unsubscribe route handler implementation:" ast-grep --pattern 'export async function $HANDLER($_) { $$$ }' apps/web/src/app/api/notifications/unsubscribe/route.tsLength of output: 234
apps/web/src/trpc/server/create-ssr-helper.ts (1)
15-24: LGTM! Clean handling of optional user.The context creation properly handles the optional user with appropriate null checks and optional chaining.
apps/web/src/app/api/trpc/[trpc]/route.ts (3)
11-14: LGTM! NextRequest type improves type safety.The change from
RequesttoNextRequesttype is appropriate for Next.js applications and provides better type safety.
19-28: LGTM! Improved user session handling.The conditional user object creation aligns well with making sessions optional. The code properly handles all user properties, including optional ones with fallbacks.
A few observations:
- Safely handles guest users through
isGuestflag- Provides proper fallbacks for optional fields
- Maintains type safety
31-33: LGTM! Enhanced type safety with satisfies operator.Good use of the
satisfiesoperator to ensure the returned context matches theTRPCContexttype while maintaining type inference.apps/web/src/app/api/user/verify-email-change/route.ts (1)
55-55: LGTM! Improved null safety in session check.The updated condition
!session?.user || !session.user.emailsafely handles cases where the session or user might be undefined, while maintaining the same redirect behavior.apps/web/src/app/api/notifications/unsubscribe/route.ts (1)
19-19: LGTM! Consistent session handling with optional chaining.The updated condition
!session || !session.user?.emailfollows the same pattern of safe null checking seen in other files.apps/web/src/app/api/stripe/portal/route.ts (1)
36-36: LGTM! Enhanced session validation with proper error handling.The updated condition
!userSession?.user || userSession.user.email === nullsafely handles undefined sessions while maintaining proper error reporting through Sentry.apps/web/src/app/[locale]/(auth)/auth/login/components/login-page.tsx (1)
Line range hint
27-33: LGTM! Safer user identification logic.The addition of optional chaining for
updatedSession?.userimproves null safety by ensuring PostHog identification only occurs when a valid user exists.Let's verify that PostHog identification is working as expected:
✅ Verification successful
PostHog identification is properly implemented across the codebase
All PostHog identify calls are properly guarded against null values:
user-provider.tsx: Guards withif (user?.email)otp-form.tsx: Guards with error check ensuring user existslogin-page.tsx: Guards withif (updatedSession?.user)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other PostHog identify calls to ensure consistent null checking rg -A 2 'posthog.*identify'Length of output: 914
Script:
#!/bin/bash # Check the surrounding context of PostHog identify calls echo "=== user-provider.tsx ===" rg -B 5 -A 2 'posthog.*identify' apps/web/src/components/user-provider.tsx echo -e "\n=== otp-form.tsx ===" rg -B 5 -A 2 'posthog.*identify' apps/web/src/app/[locale]/\(auth\)/register/verify/components/otp-form.tsxLength of output: 733
apps/web/src/middleware.ts (1)
59-62: Verify PostHog tracking for all user scenarios.The change to use
req.nextauth.tokendirectly for PostHog tracking is cleaner, but we should ensure it handles all scenarios correctly:
- Authenticated users
- Guest users
- Anonymous users
✅ Verification successful
PostHog tracking implementation verified and correct
The current implementation properly handles all user scenarios:
- Authenticated users are tracked with their user ID
- Anonymous users (including guests) aren't tracked until they explicitly authenticate
- Client-side identification occurs only after explicit user actions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check PostHog tracking implementation across the codebase echo "Checking PostHog tracking patterns..." rg -A 5 'withPostHog|posthog.*identify' # Verify token handling in auth-related files echo "Checking token usage patterns..." rg -A 3 'nextauth.token'Length of output: 3722
apps/web/src/components/poll/mutations.ts (1)
24-30: Verify error handling for failed guest sign-in.The automatic guest sign-in before mutation is a good approach, but we should ensure proper error handling.
Consider adding error handling:
onMutate: async () => { if (session.status !== "authenticated") { - await signIn("guest", { - redirect: false, - }); + const result = await signIn("guest", { + redirect: false, + }); + if (!result?.ok) { + throw new Error("Failed to create guest session"); + } } },apps/web/src/components/user-provider.tsx (2)
115-115: Great security improvement!The null check before calling
isOwnerensures that unauthenticated users cannot gain unauthorized access.
121-124: Good use of optional chaining!The defensive programming approach using optional chaining prevents runtime errors when accessing properties of an undefined user.
apps/web/src/trpc/routers/polls/participants.ts (1)
67-67: Good addition of authentication middleware!Adding
requireUserMiddlewareensures that only authenticated users (including guests) can add participants. The middleware ordering with rate limiting is correct.apps/web/src/components/discussion/discussion.tsx (3)
44-44: LGTM!The import of
signInanduseSessionfromnext-auth/reactaligns with the PR objective of making user session optional.
61-61: LGTM!The session initialization using
useSessionhook is correctly implemented.
87-93: LGTM!The
onMutatehandler correctly implements the guest user creation logic, creating a guest user only when needed (when adding a comment). This aligns with the PR objective of making user sessions optional.apps/web/src/auth.ts (2)
Line range hint
9-14: LGTM!The imports are correctly organized and include all necessary middleware and utilities.
Line range hint
35-35: Verify the necessity ofrequireUserMiddlewarein registration flow.Adding
requireUserMiddlewareto therequestRegistrationprocedure seems contradictory. This would require users to be authenticated before they can register, which could create a circular dependency in the authentication flow.Please review if this middleware is necessary for the registration flow or if it should be removed to allow unauthenticated users to register.
apps/web/src/trpc/routers/polls.ts (4)
134-134: LGTM: User authentication required for poll creation.The addition of
requireUserMiddlewareto thecreateprocedure ensures that only authenticated users can create polls, which aligns with the PR objective of making user sessions optional and creating guest users only when necessary.
Line range hint
337-352: LGTM: Access level change for getWatchers.The change from
possiblyPublicProceduretopublicProcedureforgetWatchersis appropriate as watching information can be safely exposed to all users.
353-362: LGTM: Access level changes for watch/unwatch operations.The change from
possiblyPublicProceduretoprivateProcedurefor bothwatchandunwatchoperations is correct as these mutations should only be performed by authenticated users.Also applies to: 363-381
445-449: Verify the null check in isOwner condition.The modification of the
isOwnercheck adds a null check forctx.user, which is good. However, we should verify that this doesn't affect any existing functionality.✅ Verification successful
✅ The null check in
isOwneris safe and consistent with codebase patternsThe added null check aligns with existing null-safety patterns in the codebase, particularly matching the pattern in
comments.tswhich also handles null cases gracefully.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any direct usage of isOwner variable to ensure the null check doesn't break existing code rg -A 5 "isOwner\s*=" apps/web/src/Length of output: 854
apps/web/src/trpc/routers/auth.ts (1)
9-14: LGTM: Clean import organization.The imports are well-organized and include all necessary middleware components.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/web/src/trpc/trpc.ts (1)
40-55: Consider enhancing error messages in requireUserMiddleware.While the middleware logic is correct, the error message could be more descriptive to help users understand why authentication failed.
throw new TRPCError({ code: "UNAUTHORIZED", - message: "This method requires a user", + message: "Authentication required. Please sign in or continue as a guest to perform this action.", });apps/web/src/trpc/routers/polls/comments.ts (1)
20-22: Consider removing deprecated parameter.The hideParticipants parameter is marked as deprecated but still used in the code. Consider removing it entirely since the value is now retrieved from the poll.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/web/src/app/api/trpc/[trpc]/route.ts(1 hunks)apps/web/src/components/create-poll.tsx(3 hunks)apps/web/src/trpc/context.ts(1 hunks)apps/web/src/trpc/routers/auth.ts(2 hunks)apps/web/src/trpc/routers/polls/comments.ts(2 hunks)apps/web/src/trpc/trpc.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/trpc/routers/auth.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Type check
- GitHub Check: Integration tests
- GitHub Check: Linting
🔇 Additional comments (12)
apps/web/src/components/create-poll.tsx (3)
26-26: LGTM! Import statement is correctly placed.The new imports for
signInanduseSessionfromnext-auth/reactare necessary for implementing optional user sessions.
46-46: LGTM! Session hook is properly implemented.The
useSessionhook is correctly used to manage the authentication state.
71-76: Consider adding error handling for sign-in failure.While the authentication flow is correct, it's missing error handling for cases where guest sign-in fails.
apps/web/src/trpc/context.ts (2)
3-9: LGTM! Well-structured User type definition.The User type captures all necessary properties and correctly handles optional fields.
11-14: LGTM! TRPCContext with optional user property.The optional user property aligns with the PR objective of making user sessions optional.
apps/web/src/app/api/trpc/[trpc]/route.ts (2)
12-16: LGTM! Updated handler signature to use NextRequest.The change to NextRequest type improves type safety and aligns with Next.js standards.
18-34: LGTM! Robust context creation with optional user handling.The context creation logic:
- Safely handles optional user sessions
- Properly retrieves locale from headers
- Correctly structures user object when session exists
apps/web/src/trpc/trpc.ts (3)
26-36: LGTM! Clear handling of public access with Quick Create feature.The possiblyPublicProcedure correctly checks for guest users and Quick Create status.
57-71: LGTM! Robust privateProcedure implementation.The procedure correctly handles both missing users and guest users with appropriate error handling.
73-90: LGTM! Well-structured proProcedure with clear subscription checks.The procedure properly:
- Handles self-hosted instances
- Verifies subscription status
- Provides clear error messages for unauthorized access
apps/web/src/trpc/routers/polls/comments.ts (2)
75-77: LGTM! Added user requirement for comment creation.The addition of requireUserMiddleware ensures proper authentication for comment creation.
24-64: 🛠️ Refactor suggestionFix potential issue with comment visibility logic.
The hideParticipants check at line 42 is redundant with the condition in the if statement. Also, the logic structure could be simplified.
- if (hideParticipants && !isOwner) { + if (hideParticipants) { // if hideParticipants is enabled and the user is not the owner if (!ctx.user) { // cannot see any comments if there is no user return []; } else {Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
apps/web/src/components/create-poll.tsx (1)
70-75:⚠️ Potential issueEnhance authentication flow robustness.
The current implementation needs additional error handling and session verification.
Apply this diff to improve the authentication flow:
onMutate: async () => { if (session.status !== "authenticated") { - await signIn("guest", { + const result = await signIn("guest", { redirect: false, }); + if (!result?.ok) { + throw new Error("Failed to create guest session"); + } + // Wait for the session to be updated + let retries = 0; + while (retries < 5 && session.status !== "authenticated") { + await new Promise((resolve) => setTimeout(resolve, 100)); + retries++; + } + if (session.status !== "authenticated") { + throw new Error("Failed to establish guest session"); + } } },
🧹 Nitpick comments (3)
apps/web/src/components/user-dropdown.tsx (1)
65-69: LGTM! Consider adding aria-label for accessibility.The simplified email display logic aligns well with making user sessions optional. The conditional rendering is clean and handles the absence of email gracefully.
Consider adding an aria-label to improve accessibility:
- <div className="text-muted-foreground text-xs font-normal"> + <div className="text-muted-foreground text-xs font-normal" aria-label="User email"> {user.email} </div>apps/web/src/auth.ts (2)
177-179: LGTM! Consider adding a debug log for guest user migration.The optional chaining is a good defensive programming practice. Consider adding a debug log to track guest user migrations for better observability.
if (session?.user && session.user.email === null) { + console.debug(`Migrating guest user ${session.user.id} to ${userId}`); await mergeGuestsIntoUser(userId, [session.user.id]); }
279-287: Consider adding type validation for guest user session.While the code structure is cleaner, consider adding runtime type validation for the session properties to catch potential issues early.
+import { z } from 'zod'; + +const guestSessionSchema = z.object({ + id: z.string(), + locale: z.string().optional(), + timeFormat: z.string().optional(), + timeZone: z.string().optional(), + weekStart: z.number().optional(), +}); if (token.sub?.startsWith("user-")) { + const userSession = { id: token.sub as string, locale: token.locale, timeFormat: token.timeFormat, timeZone: token.timeZone, weekStart: token.weekStart, - }; + }; + session.user = guestSessionSchema.parse(userSession); return session; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/web/src/app/api/trpc/[trpc]/route.ts(1 hunks)apps/web/src/auth.ts(4 hunks)apps/web/src/components/create-poll.tsx(3 hunks)apps/web/src/components/discussion/discussion.tsx(4 hunks)apps/web/src/components/layouts/poll-layout.tsx(2 hunks)apps/web/src/components/poll/mutations.ts(3 hunks)apps/web/src/components/user-dropdown.tsx(1 hunks)apps/web/src/trpc/routers/auth.ts(2 hunks)apps/web/src/trpc/routers/polls.ts(5 hunks)apps/web/src/trpc/routers/polls/comments.ts(2 hunks)apps/web/src/trpc/routers/polls/participants.ts(2 hunks)apps/web/src/trpc/server/create-ssr-helper.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/web/src/trpc/routers/auth.ts
- apps/web/src/trpc/server/create-ssr-helper.ts
- apps/web/src/app/api/trpc/[trpc]/route.ts
- apps/web/src/components/discussion/discussion.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Type check
- GitHub Check: Unit tests
- GitHub Check: Integration tests
- GitHub Check: Linting
🔇 Additional comments (16)
apps/web/src/auth.ts (3)
258-260: LGTM! Consistent with the earlier guest user handling.The optional chaining pattern is consistently applied here, making the code more robust against potential runtime errors.
276-278: LGTM! Early return pattern improves code clarity.The early return when
token.subis missing is a good defensive programming practice that prevents unnecessary processing.
308-317: Consider validating token.sub format before database query.Add validation for the
token.subformat before querying the database to prevent potential SQL injection or invalid queries.apps/web/src/components/layouts/poll-layout.tsx (1)
151-155: LGTM! Component structure has been simplified.The removal of the
Prefetchcomponent aligns with making user sessions optional, resulting in a cleaner and more straightforward component hierarchy.apps/web/src/trpc/routers/polls/participants.ts (2)
8-13: LGTM! Well-organized imports.The imports are properly structured and include the new
requireUserMiddleware.
67-67: LGTM! Enhanced security with user authentication.The addition of
requireUserMiddlewareensures that only authenticated users can add participants to polls.apps/web/src/components/poll/mutations.ts (2)
2-2: LGTM! Added authentication imports.The required next-auth imports are added for handling user authentication.
22-31: LGTM! Robust authentication flow implementation.The implementation correctly:
- Checks the session status before mutation
- Handles guest authentication seamlessly
- Prevents unwanted page reloads with
redirect: falseapps/web/src/trpc/routers/polls/comments.ts (4)
8-13: LGTM! Well-organized imports.The imports are properly structured and include the new
requireUserMiddleware.
24-63: LGTM! Fixed the comments visibility logic.The implementation now correctly:
- Determines poll ownership
- Handles visibility based on
hideParticipantsand ownership- Returns restricted comments early when needed
This fixes the previously reported logic bug where restricted comments were being bypassed.
76-76: LGTM! Enhanced security with user authentication.The addition of
requireUserMiddlewareensures that only authenticated users can add comments to polls.
21-21: Note the deprecation of hideParticipants parameter.The
hideParticipantsparameter is marked as deprecated. Consider updating dependent code to use the new ownership-based visibility logic.apps/web/src/components/create-poll.tsx (1)
13-13: LGTM! Session management changes look good.The addition of
useSessionhook and session initialization aligns with the PR objective of making user sessions optional.Also applies to: 45-45
apps/web/src/trpc/routers/polls.ts (3)
20-20: LGTM! Authentication middleware addition looks good.The addition of
requireUserMiddlewareto thecreateprocedure ensures proper user authentication before poll creation.Also applies to: 134-134
Line range hint
337-382: LGTM! Access control improvements for watcher operations.The changes appropriately:
- Make watcher list publicly accessible via
publicProcedure- Restrict watch/unwatch operations to authenticated users via
privateProcedure
445-449: LGTM! Robust ownership check implementation.The modified
isOwnercheck safely handles optional user sessions while maintaining the distinction between guest and regular users.
lukevella
force-pushed
the
opational-user-session
branch
from
402db5d to
f2dc9d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
apps/web/src/middleware.ts (1)
Line range hint
71-86: Improve authorization logic maintainability and type safety.The current authorization logic for self-hosted instances could be improved in several ways:
- Use route constants to avoid string literals
- Implement a more structured path matching approach
- Add type safety for route patterns
Here's a suggested improvement:
+ const GUEST_ALLOWED_ROUTES = { + invite: "/invite", + login: "/login", + register: "/register", + auth: "/auth", + preview: "/p", + } as const; + + const isAllowedGuestRoute = (pathname: string) => + Object.values(GUEST_ALLOWED_ROUTES).some( + (route) => pathname.startsWith(route) + ); callbacks: { authorized: ({ token, req }) => { const nextUrl = req.nextUrl; const isGuest = !token?.email; - if ( - isSelfHosted && - isGuest && - !( - nextUrl.pathname.startsWith("/invite") || - nextUrl.pathname.startsWith("/login") || - nextUrl.pathname.startsWith("/register") || - nextUrl.pathname.startsWith("/auth") || - nextUrl.pathname.startsWith("/p/") - ) - ) { + if (isSelfHosted && isGuest && !isAllowedGuestRoute(nextUrl.pathname)) { return false; } return true; }, },
🧹 Nitpick comments (8)
apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx (1)
Line range hint
22-24: Consider adding loading states and security measures.Given that this is a settings page with sensitive operations:
- Add loading states while user data is being fetched
- Consider adding a confirmation step for logout
- Implement rate limiting for sensitive operations
apps/web/src/components/layouts/poll-layout.tsx (1)
87-88: Consider improving error message clarity.The error message "If you are the poll creator, please login to administor your poll" contains a typo and could be more user-friendly.
- defaults="If you are the poll creator, please login to administor your poll." + defaults="If you are the poll creator, please log in to administer your poll."apps/web/src/components/discussion/discussion.tsx (1)
87-93: LGTM! Consider adding loading state during sign in.The guest user sign-in implementation is correct, but consider showing a loading state during the sign-in process.
const addComment = trpc.polls.comments.add.useMutation({ onMutate: async () => { if (session.status !== "authenticated") { + // Show loading state + toast({ title: "Signing in as guest..." }); await signIn("guest", { redirect: false, }); } },apps/web/src/components/user-provider.tsx (1)
16-16: Consider making the entire user object optional in UserContext.While making
idoptional is a good start, theUserContexttype still expectsuserto be required. This could lead to runtime errors when working with guest users.Consider updating the
UserContexttype to better handle undefined users:export const UserContext = React.createContext<{ - user: UserData; + user: UserData | undefined; refresh: (data?: Record<string, unknown>) => Promise<Session | null>; // ... rest of the type } | null>(null);apps/web/src/trpc/trpc.ts (1)
27-35: Consider improving the comment clarity.The comment "These procedurs are public if Quick Create is enabled" contains a typo and could be more descriptive about the actual behavior.
- // These procedurs are public if Quick Create is enabled + // Allow access if user is not a guest or if Quick Create is enabledapps/web/src/trpc/routers/polls.ts (1)
445-449: Consider simplifying the isOwner check.The current implementation can be made more concise while maintaining the same functionality.
- const userId = ctx.user?.id; - - const isOwner = ctx.user?.isGuest - ? userId === res.guestId - : userId === res.userId; + const isOwner = ctx.user?.isGuest + ? ctx.user.id === res.guestId + : ctx.user?.id === res.userId;apps/web/src/middleware.ts (2)
6-6: Consider moving locale-related functionality to a dedicated module.Since guest user initialization is being decoupled, consider moving
getLocaleFromHeaderto a dedicated locale utilities module (e.g.,@/utils/locale) to maintain better separation of concerns.
59-61: LGTM! Consider adding a comment explaining the PostHog integration.The conditional PostHog tracking is a good improvement that aligns with optional user sessions. Consider adding a comment explaining why we only track authenticated requests and the significance of using
token.subas the distinct ID.+ // Only track analytics for authenticated requests using the user's unique identifier if (req.nextauth.token) { await withPostHog(res, { distinctID: req.nextauth.token.sub }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (28)
apps/web/declarations/next-auth.d.ts(1 hunks)apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx(2 hunks)apps/web/src/app/[locale]/(auth)/auth/login/components/login-page.tsx(1 hunks)apps/web/src/app/api/logout/route.ts(0 hunks)apps/web/src/app/api/notifications/unsubscribe/route.ts(1 hunks)apps/web/src/app/api/stripe/checkout/route.ts(1 hunks)apps/web/src/app/api/stripe/portal/route.ts(1 hunks)apps/web/src/app/api/trpc/[trpc]/route.ts(1 hunks)apps/web/src/app/api/user/verify-email-change/route.ts(1 hunks)apps/web/src/app/guest.ts(1 hunks)apps/web/src/auth.ts(4 hunks)apps/web/src/auth/custom-prisma-adapter.ts(1 hunks)apps/web/src/components/create-poll.tsx(3 hunks)apps/web/src/components/discussion/discussion.tsx(4 hunks)apps/web/src/components/layouts/poll-layout.tsx(2 hunks)apps/web/src/components/poll/mutations.ts(3 hunks)apps/web/src/components/poll/notifications-toggle.tsx(2 hunks)apps/web/src/components/user-dropdown.tsx(1 hunks)apps/web/src/components/user-provider.tsx(4 hunks)apps/web/src/middleware.ts(2 hunks)apps/web/src/trpc/context.ts(1 hunks)apps/web/src/trpc/routers/auth.ts(2 hunks)apps/web/src/trpc/routers/polls.ts(5 hunks)apps/web/src/trpc/routers/polls/comments.ts(2 hunks)apps/web/src/trpc/routers/polls/participants.ts(2 hunks)apps/web/src/trpc/routers/user.ts(1 hunks)apps/web/src/trpc/server/create-ssr-helper.ts(1 hunks)apps/web/src/trpc/trpc.ts(2 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/app/api/logout/route.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- apps/web/src/app/[locale]/(auth)/auth/login/components/login-page.tsx
- apps/web/src/app/api/stripe/portal/route.ts
- apps/web/src/trpc/routers/auth.ts
- apps/web/src/app/api/notifications/unsubscribe/route.ts
- apps/web/src/trpc/routers/polls/participants.ts
- apps/web/src/app/api/stripe/checkout/route.ts
- apps/web/src/app/api/user/verify-email-change/route.ts
- apps/web/declarations/next-auth.d.ts
- apps/web/src/components/user-dropdown.tsx
- apps/web/src/components/create-poll.tsx
- apps/web/src/trpc/context.ts
- apps/web/src/trpc/server/create-ssr-helper.ts
- apps/web/src/app/api/trpc/[trpc]/route.ts
- apps/web/src/auth.ts
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Type check
- GitHub Check: Linting
- GitHub Check: Integration tests
🔇 Additional comments (22)
apps/web/src/components/poll/notifications-toggle.tsx (1)
Line range hint
47-51: LGTM! Good defensive programming.The additional check for
user.idprevents potential issues with adding watchers without valid user identifiers. This aligns well with making user sessions optional.apps/web/src/app/[locale]/(admin)/settings/profile/profile-page.tsx (2)
4-4: LGTM! Clean import organization.The icon imports are well-structured and use a modern icon library.
30-70: LGTM! Well-structured settings sections.The settings sections are well-organized with proper translations and clear user interface elements.
apps/web/src/auth/custom-prisma-adapter.ts (3)
19-22: LGTM! Good improvement in adapter initialization.Storing the adapter instance in a variable before spreading improves code clarity and debugging capabilities.
41-41: LGTM! Good use of 'satisfies' operator.The
satisfies Adapterassertion provides better type safety while maintaining precise return types.
19-41: Verify adapter usage and session handling.Since this adapter is part of broader auth system changes, let's verify its integration:
✅ Verification successful
CustomPrismaAdapter implementation is correct and safe ✅
The adapter properly extends NextAuth's PrismaAdapter while safely handling user data migration through transactions. The session management remains intact through standard NextAuth components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CustomPrismaAdapter usage and related session handling echo "Searching for CustomPrismaAdapter usage..." rg -A 5 "CustomPrismaAdapter" echo "\nChecking session-related configurations..." rg -l "authOptions|NextAuth|getServerSession"Length of output: 2074
Script:
#!/bin/bash # Check session provider and merge implementation echo "Checking session provider implementation..." cat apps/web/src/auth/session-provider.tsx echo "\nChecking merge functionality..." cat apps/web/src/auth/merge-user.ts echo "\nChecking for any direct PrismaAdapter usage..." rg "PrismaAdapter" -B 2 -A 2Length of output: 3430
apps/web/src/app/guest.ts (1)
Line range hint
1-15: LGTM! Verify removed functions aren't referenced elsewhere.The removal of guest user management functions aligns with making user sessions optional. The remaining
getLocaleFromHeaderfunction is correctly implemented.Let's verify that the removed functions aren't referenced elsewhere in the codebase:
✅ Verification successful
Verified: Removed functions are not referenced anywhere
The removed guest management functions are not referenced in the codebase. The only remaining imports from
guest.tsare for thegetLocaleFromHeaderfunction, which is still present. Other "guest"-related imports are from different files and are unrelated to the removed functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to removed functions rg -i "resetUser|initGuest|setCookie|getCookieSettings"Length of output: 143
Script:
#!/bin/bash # Check implementation of setCookie in cookies.ts echo "=== cookies.ts implementation ===" cat apps/web/src/utils/cookies.ts echo -e "\n=== Files importing from guest.ts ===" rg -l "from.*guest"Length of output: 683
Script:
#!/bin/bash # Check the specific imports from guest.ts echo "=== Checking imports from guest.ts ===" rg "from.*guest" -B 1 -A 1Length of output: 1653
apps/web/src/components/poll/mutations.ts (1)
22-30: LGTM! Robust guest user handling implementation.The implementation correctly handles guest users by:
- Checking authentication status before mutation
- Signing in as guest without page reload
- Maintaining existing success handling
apps/web/src/components/discussion/discussion.tsx (1)
129-129: LGTM! Key prop update is correct.The key prop update to use
user?.idis correct and maintains component state properly.apps/web/src/components/user-provider.tsx (4)
90-97: Well-implemented optional chaining!Good use of optional chaining operators and fallback values for all user properties. This makes the code more robust when handling undefined user sessions.
106-106: LGTM! Good defensive programming.The ownership check now safely handles undefined users by returning false, which is a sensible default.
Line range hint
73-83: Review effect dependencies for PostHog tracking.The effect's dependency array only includes
user?.id, but the effect uses several other properties (name,tier, etc.) that should trigger updates if they change.Consider adding all tracked properties to the dependency array:
}, [ user?.id, + user?.email, + user?.name, + tier, + user?.timeZone, + user?.image, + user?.locale, + i18n.language, + posthog ]);
112-115: Consider aligning types between PreferencesProvider and updatePreferences.While the optional chaining is well implemented, there might be a type mismatch between the PreferencesProvider's
initialValueprops and theupdatePreferencesmutation parameters.Let's verify the type consistency:
apps/web/src/trpc/trpc.ts (3)
40-55: LGTM! Well-structured middleware for user validation.The
requireUserMiddlewareimplementation is clean and follows best practices:
- Clear error message
- Proper type propagation through context
- Consistent error code usage
57-71: LGTM! Robust user validation in privateProcedure.The implementation correctly:
- Validates both user existence and non-guest status
- Provides clear error messaging
- Maintains type safety through context
73-90: LGTM! Clear procedure hierarchy established.The implementation creates a logical progression of access levels:
privateProcedureensures authenticated non-guest userproProcedureadds subscription validation on topapps/web/src/trpc/routers/polls/comments.ts (2)
76-76: LGTM! User authentication properly enforced.Adding
requireUserMiddlewareensures that only authenticated users can add comments.
40-63:⚠️ Potential issueFix potential comment visibility issue.
When
hideParticipantsis true and the user is not the owner, the code correctly filters comments for authenticated users but then continues to execute and returns all comments at line 64-71. This bypasses the visibility restrictions.if (hideParticipants && !isOwner) { // if hideParticipants is enabled and the user is not the owner if (!ctx.user) { // cannot see any comments if there is no user return []; } else { - // only show comments created by the current users - return await prisma.comment.findMany({ + // only show comments created by the current user + const userComments = await prisma.comment.findMany({ where: { pollId, ...(ctx.user.isGuest ? { guestId: ctx.user.id } : { userId: ctx.user.id }), }, orderBy: [ { createdAt: "asc", }, ], }); + return userComments; } }Likely invalid or redundant comment.
apps/web/src/trpc/routers/user.ts (1)
Line range hint
70-81: LGTM! Improved subscription status handling.The changes correctly:
- Make the endpoint public while maintaining proper access control
- Handle both missing user and guest user cases consistently
- Return appropriate subscription status
apps/web/src/trpc/routers/polls.ts (2)
134-134: LGTM! User authentication properly enforced for poll creation.Adding
requireUserMiddlewareensures that only authenticated users can create polls.
Line range hint
353-383: LGTM! Proper access control for watch/unwatch operations.The changes correctly:
- Restrict watch/unwatch operations to authenticated users
- Maintain consistent error handling
- Use proper database operations
apps/web/src/middleware.ts (1)
Line range hint
11-18: Verify route handling consistency.Let's ensure that
publicRoutesand guest-allowed routes in the authorization callback are consistent and handle all necessary cases.✅ Verification successful
Public routes are properly configured and secure ✅
The middleware implementation correctly handles authentication, public access, and routing requirements. The
publicRoutesarray includes all necessary paths for guest access while maintaining proper security controls for authenticated routes.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for route definitions and route handling across the codebase echo "Searching for route definitions..." rg -A 5 "const.*Routes.*=.*\[" --type typescript echo "Searching for route handling in components..." rg -A 5 "useRouter|router\.push|Link.*href" --type typescript echo "Searching for middleware route handling..." rg -A 5 "middleware.*pathname" --type typescriptLength of output: 545
Script:
#!/bin/bash # Search for route definitions and middleware handling echo "=== Searching for route definitions ===" rg -t ts -t tsx "const.*Routes.*=.*\[" -A 5 echo -e "\n=== Searching for middleware implementations ===" rg -t ts -t tsx "export.*middleware|function.*middleware" -A 10 echo -e "\n=== Searching for route handling in components ===" rg -t ts -t tsx "useRouter|router\.push|href.*=.*[\"\']\/[^\"\']+" -A 3Length of output: 711
Script:
#!/bin/bash # Find and examine relevant files echo "=== Finding TypeScript files ===" fd -e ts -e tsx echo -e "\n=== Examining middleware.ts content ===" find . -name "middleware.ts" -type f -exec cat {} \; echo -e "\n=== Looking for route-related patterns ===" find . -name "*.ts" -o -name "*.tsx" -type f -exec grep -l "publicRoutes\|middleware\|useRouter" {} \;Length of output: 25239
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
apps/web/src/components/create-poll.tsx (1)
Line range hint
76-109: Add error handling for form submission.The form submission lacks error handling for both the authentication and mutation processes. This could lead to unhandled exceptions and poor user experience.
Apply this diff to add proper error handling:
onSubmit={form.handleSubmit(async (formData) => { + try { const title = required(formData?.title); if (session.status !== "authenticated") { const result = await signIn("guest", { redirect: false, }); + if (!result?.ok) { + throw new Error("Failed to create guest session"); + } + // Wait for session update + let retries = 0; + while (retries < 5 && session.status !== "authenticated") { + await new Promise((resolve) => setTimeout(resolve, 100)); + retries++; + } + if (session.status !== "authenticated") { + throw new Error("Failed to establish guest session"); + } } await createPoll.mutateAsync( { title: title, location: formData?.location, description: formData?.description, timeZone: formData?.timeZone, hideParticipants: formData?.hideParticipants, disableComments: formData?.disableComments, hideScores: formData?.hideScores, requireParticipantEmail: formData?.requireParticipantEmail, options: required(formData?.options).map((option) => ({ startDate: option.type === "date" ? option.date : option.start, endDate: option.type === "timeSlot" ? option.end : undefined, })), }, { onSuccess: (res) => { posthog?.capture("created poll", { pollId: res.id, numberOfOptions: formData.options?.length, optionsView: formData?.view, tier: user.tier, $set: { last_poll_created_at: new Date().toISOString(), }, }); router.push(`/poll/${res.id}`); }, }, ); + } catch (error) { + // Handle error appropriately (e.g., show error message to user) + console.error("Failed to create poll:", error); + form.setError("root", { + message: error instanceof Error ? error.message : "Failed to create poll", + }); + } })}
♻️ Duplicate comments (1)
apps/web/src/components/create-poll.tsx (1)
77-81:⚠️ Potential issueAdd error handling for guest sign-in.
The authentication flow needs proper error handling and synchronization to prevent potential race conditions.
Apply this diff to ensure robust error handling:
- if (session.status !== "authenticated") { - await signIn("guest", { - redirect: false, - }); - } + if (session.status !== "authenticated") { + const result = await signIn("guest", { + redirect: false, + }); + if (!result?.ok) { + throw new Error("Failed to create guest session"); + } + // Wait for session update + let retries = 0; + while (retries < 5 && session.status !== "authenticated") { + await new Promise((resolve) => setTimeout(resolve, 100)); + retries++; + } + if (session.status !== "authenticated") { + throw new Error("Failed to establish guest session"); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/web/src/components/create-poll.tsx(3 hunks)apps/web/src/components/discussion/discussion.tsx(4 hunks)apps/web/src/components/new-participant-modal.tsx(3 hunks)apps/web/src/components/poll/mutations.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/web/src/components/poll/mutations.ts
- apps/web/src/components/discussion/discussion.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Linting
- GitHub Check: Integration tests
- GitHub Check: Type check
🔇 Additional comments (2)
apps/web/src/components/new-participant-modal.tsx (1)
10-10: LGTM!The import of
signInanduseSessionfrom next-auth/react is appropriate for implementing the optional user session functionality.apps/web/src/components/create-poll.tsx (1)
13-13: LGTM!The import of
signInanduseSessionfrom next-auth/react is appropriate for implementing the optional user session functionality.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/web/src/components/poll/mutations.ts (2)
24-30: Consider adding error handling for the sign-in process.While this logic ensures guests are signed in, it doesn't handle sign-in failures (e.g., network issues). Consider adding a safeguard to detect/prompt the user if sign-in fails.
onMutate: async () => { if (session.status !== "authenticated") { - await signIn("guest", { - redirect: false, - }); + const signInResult = await signIn("guest", { redirect: false }); + if (signInResult?.error) { + // Optionally handle sign-in error or notify the user + } } },
31-31: Async onSuccess is fine, but consider usage.Declaring
onSuccessasasyncis future-proof. However, since there’s currently noawaitcall, you might remove theasynckeyword or plan to leverage asynchronous tasks (e.g., subsequent data fetching) insideonSuccess.apps/web/src/components/discussion/discussion.tsx (1)
87-93: Incorporate error handling for sign-in.Similar to your poll mutation logic, you may want to handle potential sign-in errors to improve user experience and reliability.
onMutate: async () => { if (session.status !== "authenticated") { - await signIn("guest", { redirect: false }); + const signInResult = await signIn("guest", { redirect: false }); + if (signInResult?.error) { + // Notify the user about the error + } } },apps/web/src/features/quick-create/quick-create-widget.tsx (1)
Line range hint
13-15: Consider adding error handling and loading states.The component could benefit from:
- Error handling for
getGuestPolls()- Loading state while fetching polls
Here's a suggested implementation:
export async function QuickCreateWidget() { + let polls: Poll[] = []; + let error: Error | null = null; + + try { polls = await getGuestPolls(); + } catch (e) { + error = e as Error; + } + const { t } = await getTranslation(); + + if (error) { + return ( + <div className="text-destructive"> + <Trans + t={t} + ns="app" + i18nKey="errorLoadingPolls" + defaults="Error loading polls" + /> + </div> + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/web/src/components/create-poll.tsx(3 hunks)apps/web/src/components/discussion/discussion.tsx(4 hunks)apps/web/src/components/new-participant-modal.tsx(0 hunks)apps/web/src/components/poll/mutations.ts(3 hunks)apps/web/src/features/quick-create/quick-create-widget.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- apps/web/src/components/new-participant-modal.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/web/src/components/create-poll.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Unit tests
- GitHub Check: Type check
- GitHub Check: Integration tests
- GitHub Check: Linting
🔇 Additional comments (6)
apps/web/src/components/poll/mutations.ts (2)
2-2: Good job integrating session-based logic.Importing the
signInanduseSessionhooks fromnext-auth/reactis a clean approach to handle guest sign-ins.
22-22: Session retrieval is straightforward.Using
useSession()at this level is appropriate for retrieving the session status without unnecessary prop drilling.apps/web/src/components/discussion/discussion.tsx (3)
29-29: Leverage next-auth for uniform session handling.Importing
signInanduseSessionfrom “next-auth/react” aligns with the library’s recommended usage. Good move!
61-61: Session acquisition is consistent.Storing
session = useSession()is straightforward and consistent with the approach from other components, reducing confusion for maintainers.
129-129: Keying by user ID is correct.Switching
key={session.user?.id}tokey={user?.id}is consistent with your locally retrieved user object. This avoids potential null references ifsession.useris undefined.apps/web/src/features/quick-create/quick-create-widget.tsx (1)
Line range hint
1-134: Well-structured implementation aligning with PR objectives! 👍The component effectively supports the PR's goal of making user sessions optional by:
- Allowing guest users to create polls without signing in
- Maintaining access to guest-created polls
- Clearly communicating the benefits of account creation
Rather than creating a guest in middleware, we now create a guest when a user creates a resource that needs to be tracked.
Summary by CodeRabbit
Release Notes
Authentication
requireUserMiddlewareto enforce user authentication in several procedures.User Experience
Backend Improvements
QUICK_CREATE_ENABLEDto enhance quick creation functionality.Security