-
Notifications
You must be signed in to change notification settings - Fork 1
Clone jrmy/guest session #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,13 @@ | ||||||||||||||||||||||||||||||||||||||||||||
| import { redirect } from 'next/navigation'; | ||||||||||||||||||||||||||||||||||||||||||||
| import { auth, signIn } from '@/app/(auth)/auth'; | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| export async function GET() { | ||||||||||||||||||||||||||||||||||||||||||||
| const session = await auth(); | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| if (!session?.user?.id) { | ||||||||||||||||||||||||||||||||||||||||||||
| await signIn('guest', { redirect: false }); | ||||||||||||||||||||||||||||||||||||||||||||
| redirect('/'); | ||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||
| return new Response('Unauthorized', { status: 401 }); | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+5
to
+12
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation doesn't handle potential errors from const session = await auth();
if (session?.user?.id) {
return new Response('Unauthorized', { status: 401 });
}
try {
await signIn('guest', { redirect: false });
} catch (error) {
console.error('Guest sign-in failed:', error);
return new Response('Guest sign-in failed', { status: 500 });
}
redirect('/');There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unreachable Return StatementThe return statement on line 12 is unreachable because the redirect() call on line 9 will always redirect users without sessions. This creates dead code that never executes and indicates a logical flaw in the control flow design. Standards
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: When a valid session already exists (i.e. a user id is present), this handler still returns a 401 "Unauthorized" response, which misrepresents the user's authenticated state and will make clients treat valid sessions as unauthorized; redirecting authenticated users instead avoids this logic error. [logic error] Severity Level: Minor Why it matters? ⭐This is a real logic bug: when session?.user?.id exists the handler falls through to returning 401, which incorrectly treats authenticated users as unauthorized. Replacing this with a redirect or a success response aligns behavior with intent (guest flow vs authenticated flow) and fixes incorrect HTTP semantics. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(auth)/api/auth/guest/route.ts
**Line:** 12:12
**Comment:**
*Logic Error: When a valid session already exists (i.e. a user id is present), this handler still returns a 401 "Unauthorized" response, which misrepresents the user's authenticated state and will make clients treat valid sessions as unauthorized; redirecting authenticated users instead avoids this logic error.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+4
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clarify the 401 response for authenticated users. The logic returns 401 when a session already exists (Line 12). This seems counterintuitive - if a user is already authenticated (whether guest or regular), returning "Unauthorized" appears incorrect. Possible scenarios:
If the intent is to prevent authenticated users from creating new guest sessions, consider:
Suggested fix: export async function GET() {
const session = await auth();
if (!session?.user?.id) {
await signIn('guest', { redirect: false });
redirect('/');
}
- return new Response('Unauthorized', { status: 401 });
+ // User already has a session, redirect to home
+ redirect('/');
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -2,7 +2,7 @@ import { compare } from 'bcrypt-ts'; | |||||||||
| import NextAuth, { type User, type Session } from 'next-auth'; | ||||||||||
| import Credentials from 'next-auth/providers/credentials'; | ||||||||||
|
|
||||||||||
| import { getUser } from '@/lib/db/queries'; | ||||||||||
| import { createAnonymousUser, getUser } from '@/lib/db/queries'; | ||||||||||
|
|
||||||||||
| import { authConfig } from './auth.config'; | ||||||||||
|
|
||||||||||
|
|
@@ -21,12 +21,24 @@ export const { | |||||||||
| Credentials({ | ||||||||||
| credentials: {}, | ||||||||||
| async authorize({ email, password }: any) { | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The Severity Level: Minor
Suggested change
Why it matters? ⭐The provider authorize currently destructures its first argument. NextAuth may call authorize with undefined/absent credentials in some flows — destructuring undefined will throw. Changing the signature to accept credentials and safely default before destructuring prevents a runtime TypeError and is a small, safe hardening. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(auth)/auth.ts
**Line:** 23:23
**Comment:**
*Null Pointer: The `authorize` function directly destructures `email` and `password` from its first argument, which will throw a runtime error if NextAuth ever calls it with `credentials` undefined or null; instead, safely handle a possibly missing argument before destructuring.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||
| const users = await getUser(email); | ||||||||||
| if (users.length === 0) return null; | ||||||||||
| // biome-ignore lint: Forbidden non-null assertion. | ||||||||||
| const passwordsMatch = await compare(password, users[0].password!); | ||||||||||
| const [user] = await getUser(email); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Minor
Suggested change
Why it matters? ⭐If credentials are missing or email is falsy, calling getUser(email) may produce an unexpected DB query or error depending on its implementation. Short-circuiting when email is falsy is defensive and avoids relying on getUser to validate inputs; it's a sensible guard to add. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(auth)/auth.ts
**Line:** 24:24
**Comment:**
*Logic Error: `getUser(email)` is called without verifying that `email` is present; if `email` is missing, this can result in an invalid database query or unexpected behavior, so the function should short-circuit when `email` is falsy.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||
|
|
||||||||||
| if (!user) return null; | ||||||||||
| if (!user.password) return null; | ||||||||||
|
|
||||||||||
| const passwordsMatch = await compare(password, user.password); | ||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The password comparison runs even if Severity Level: Minor
Suggested change
Why it matters? ⭐The code already checks for a stored user.password but does not check the provided password. Passing undefined into bcrypt's compare can throw or behave unexpectedly. Adding an explicit check (return null when password is falsy) prevents that runtime error and clarifies intent. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(auth)/auth.ts
**Line:** 29:29
**Comment:**
*Null Pointer: The password comparison runs even if `password` is missing, which can cause `bcrypt-ts` to throw when given an undefined value; add an explicit check to return early when no password is provided.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||
|
|
||||||||||
| if (!passwordsMatch) return null; | ||||||||||
| return users[0] as any; | ||||||||||
|
|
||||||||||
| return user; | ||||||||||
| }, | ||||||||||
| }), | ||||||||||
| Credentials({ | ||||||||||
| id: 'guest', | ||||||||||
| credentials: {}, | ||||||||||
| async authorize() { | ||||||||||
| const [anonymousUser] = await createAnonymousUser(); | ||||||||||
| return anonymousUser; | ||||||||||
| }, | ||||||||||
| }), | ||||||||||
| ], | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ import { AuthForm } from '@/components/auth-form'; | |
| import { SubmitButton } from '@/components/submit-button'; | ||
|
|
||
| import { login, type LoginActionState } from '../actions'; | ||
| import { useSession } from 'next-auth/react'; | ||
|
|
||
| export default function Page() { | ||
| const router = useRouter(); | ||
|
|
@@ -23,6 +24,8 @@ export default function Page() { | |
| }, | ||
| ); | ||
|
|
||
| const { update: updateSession } = useSession(); | ||
|
|
||
| useEffect(() => { | ||
| if (state.status === 'failed') { | ||
| toast({ | ||
|
|
@@ -36,6 +39,7 @@ export default function Page() { | |
| }); | ||
| } else if (state.status === 'success') { | ||
| setIsSuccessful(true); | ||
| updateSession(); | ||
| router.refresh(); | ||
|
Comment on lines
+42
to
43
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The async session update is triggered without waiting for it to complete before calling the navigation refresh, which can cause the refreshed page to read stale session data and not reflect the cloned guest session as intended; chaining the refresh after the update resolves ensures correct ordering. [logic error] Severity Level: Minor Why it matters? ⭐updateSession() from next-auth is asynchronous; calling router.refresh() immediately can make the refreshed page read the previous session state before the update completes. Chaining the refresh after updateSession resolves ensures the client sees the new session data (avoids a race). This change fixes a real ordering bug rather than being purely cosmetic. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(auth)/login/page.tsx
**Line:** 42:43
**Comment:**
*Logic Error: The async session update is triggered without waiting for it to complete before calling the navigation refresh, which can cause the refreshed page to read stale session data and not reflect the cloned guest session as intended; chaining the refresh after the update resolves ensures correct ordering.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||
| } | ||
| }, [state.status]); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,5 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| UIMessage, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type UIMessage, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| appendResponseMessages, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The POST handler returns HTTP 404 for any unexpected error in the try/catch, which misrepresents server-side failures as "not found" and can break client error handling that relies on correct status codes. [logic error] Severity Level: Minor
Suggested change
Why it matters? ⭐A catch-all error in the POST handler represents a server-side failure and should return a 5xx status (500) rather than 404, which means resource not found. This is a real semantics bug that affects clients and observability. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(chat)/api/chat/route.ts
**Line:** 3:3
**Comment:**
*Logic Error: The POST handler returns HTTP 404 for any unexpected error in the try/catch, which misrepresents server-side failures as "not found" and can break client error handling that relies on correct status codes.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| createDataStreamResponse, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| smoothStream, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -10,6 +10,7 @@ import { systemPrompt } from '@/lib/ai/prompts'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| deleteChatById, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getChatById, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| getMessageCountByUserId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| saveChat, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| saveMessages, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from '@/lib/db/queries'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -23,8 +24,12 @@ import { createDocument } from '@/lib/ai/tools/create-document'; | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { updateDocument } from '@/lib/ai/tools/update-document'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { requestSuggestions } from '@/lib/ai/tools/request-suggestions'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { getWeather } from '@/lib/ai/tools/get-weather'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { isProductionEnvironment } from '@/lib/constants'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { anonymousRegex, isProductionEnvironment } from '@/lib/constants'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { myProvider } from '@/lib/ai/providers'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| entitlementsByMembershipTier, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| type MembershipTier, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } from '@/lib/ai/capabilities'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const maxDuration = 60; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -42,10 +47,33 @@ export async function POST(request: Request) { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const session = await auth(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!session || !session.user || !session.user.id) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!session?.user?.id) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new Response('Unauthorized', { status: 401 }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const membershipTier: MembershipTier = anonymousRegex.test( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| session.user.email ?? '', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ? 'guest' | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : 'free'; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Although entitlements define which chat models each membership tier may use, the route never enforces this and will accept any Severity Level: Critical 🚨 Why it matters? ⭐The code computes membership tier and imports entitlements, but never checks whether the provided selectedChatModel is allowed for that tier. Enforcing entitlements prevents privilege escalation (e.g., guests using paid or reasoning models) and is a valid security/authorization fix. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(chat)/api/chat/route.ts
**Line:** 59:59
**Comment:**
*Security: Although entitlements define which chat models each membership tier may use, the route never enforces this and will accept any `selectedChatModel` from the client, allowing guests to access disallowed models simply by changing the request payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const messageCount = await getMessageCountByUserId({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| id: session.user.id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| differenceInHours: 24, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if ( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| messageCount > | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The daily message limit check uses a strict greater-than comparison, allowing one more message than the configured maximum before blocking further requests; the comparison should be inclusive so that once the maximum is reached, additional messages are rejected. [off-by-one] Severity Level: Minor
Suggested change
Why it matters? ⭐The PR checks the user's messageCount before allowing the next message. If maxMessagesPerDay is intended to be the allowed quota, then when messageCount === maxMessagesPerDay the code should block further messages. The current strict '>' check lets one extra message through. Changing to '>=' fixes a real off-by-one bug affecting rate limiting. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** app/(chat)/api/chat/route.ts
**Line:** 66:66
**Comment:**
*Off By One: The daily message limit check uses a strict greater-than comparison, allowing one more message than the configured maximum before blocking further requests; the comparison should be inclusive so that once the maximum is reached, additional messages are rejected.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| entitlementsByMembershipTier[membershipTier].maxMessagesPerDay | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+65
to
+67
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rate Limit LogicRate limiting uses strict greater-than comparison which allows exactly maxMessagesPerDay + 1 messages before blocking. For guest tier with 20 message limit, users can send 21 messages before hitting the rate limit, violating the intended business rule constraint. Standards
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return new Response( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'You have exceeded your maximum number of messages for the day', | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| status: 429, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+54
to
+75
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Off-by-one: rate limit allows one extra message. The condition if (
- messageCount >
+ messageCount >=
entitlementsByMembershipTier[membershipTier].maxMessagesPerDay
) {📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const userMessage = getMostRecentUserMessage(messages); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!userMessage) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,8 +1,9 @@ | ||||||
| 'use client'; | ||||||
|
|
||||||
| import { ChevronUp } from 'lucide-react'; | ||||||
| import Image from 'next/image'; | ||||||
| import type { User } from 'next-auth'; | ||||||
| import { signOut } from 'next-auth/react'; | ||||||
| import { signOut, useSession } from 'next-auth/react'; | ||||||
| import { useTheme } from 'next-themes'; | ||||||
|
|
||||||
| import { | ||||||
|
|
@@ -17,26 +18,50 @@ import { | |||||
| SidebarMenuButton, | ||||||
| SidebarMenuItem, | ||||||
| } from '@/components/ui/sidebar'; | ||||||
| import { anonymousRegex } from '@/lib/constants'; | ||||||
| import { useRouter } from 'next/navigation'; | ||||||
| import { toast } from './toast'; | ||||||
| import { LoaderIcon } from './icons'; | ||||||
|
|
||||||
| export function SidebarUserNav({ user }: { user: User }) { | ||||||
| const router = useRouter(); | ||||||
| const { data, status } = useSession(); | ||||||
| const { setTheme, theme } = useTheme(); | ||||||
|
|
||||||
| const isGuest = anonymousRegex.test(data?.user?.email ?? ''); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The guest detection uses Severity Level: Minor
Suggested change
Why it matters? ⭐The PR renders the avatar and email from the user prop (Image src uses user.email and the displayed string is user?.email) but computes isGuest from the session data (data?.user?.email). Those two sources can diverge (stale server-provided prop vs. live client session), causing the UI to show the prop's email while treating the user as a guest (or vice versa). Using the same source for both rendering and guest detection fixes a real logic inconsistency rather than being a purely cosmetic change. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** components/sidebar-user-nav.tsx
**Line:** 31:31
**Comment:**
*Logic Error: The guest detection uses `data?.user?.email` from `useSession` while the UI and avatar use the `user` prop, so if these two sources ever diverge (e.g. due to stale server props or session changes) the same user can be rendered as a guest or non-guest inconsistently; deriving the guest flag from the same `user` prop fixes this inconsistency.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||
|
|
||||||
| return ( | ||||||
| <SidebarMenu> | ||||||
| <SidebarMenuItem> | ||||||
| <DropdownMenu> | ||||||
| <DropdownMenuTrigger asChild> | ||||||
| <SidebarMenuButton className="data-[state=open]:bg-sidebar-accent bg-background data-[state=open]:text-sidebar-accent-foreground h-10"> | ||||||
| <Image | ||||||
| src={`https://avatar.vercel.sh/${user.email}`} | ||||||
| alt={user.email ?? 'User Avatar'} | ||||||
| width={24} | ||||||
| height={24} | ||||||
| className="rounded-full" | ||||||
| /> | ||||||
| <span className="truncate">{user?.email}</span> | ||||||
| <ChevronUp className="ml-auto" /> | ||||||
| </SidebarMenuButton> | ||||||
| {status === 'loading' ? ( | ||||||
| <SidebarMenuButton className="data-[state=open]:bg-sidebar-accent bg-background data-[state=open]:text-sidebar-accent-foreground h-10 justify-between"> | ||||||
| <div className="flex flex-row gap-2"> | ||||||
| <div className="size-6 bg-zinc-500/30 rounded-full animate-pulse" /> | ||||||
| <span className="bg-zinc-500/30 text-transparent rounded-md animate-pulse"> | ||||||
| Loading auth status | ||||||
| </span> | ||||||
| </div> | ||||||
| <div className="animate-spin text-zinc-500"> | ||||||
| <LoaderIcon /> | ||||||
| </div> | ||||||
| </SidebarMenuButton> | ||||||
| ) : ( | ||||||
| <SidebarMenuButton className="data-[state=open]:bg-sidebar-accent bg-background data-[state=open]:text-sidebar-accent-foreground h-10"> | ||||||
| <Image | ||||||
| src={`https://avatar.vercel.sh/${user.email}`} | ||||||
| alt={user.email ?? 'User Avatar'} | ||||||
| width={24} | ||||||
| height={24} | ||||||
| className="rounded-full" | ||||||
| /> | ||||||
| <span className="truncate"> | ||||||
| {isGuest ? 'Guest' : user?.email} | ||||||
| </span> | ||||||
| <ChevronUp className="ml-auto" /> | ||||||
| </SidebarMenuButton> | ||||||
| )} | ||||||
| </DropdownMenuTrigger> | ||||||
| <DropdownMenuContent | ||||||
| side="top" | ||||||
|
|
@@ -54,12 +79,26 @@ export function SidebarUserNav({ user }: { user: User }) { | |||||
| type="button" | ||||||
| className="w-full cursor-pointer" | ||||||
| onClick={() => { | ||||||
| signOut({ | ||||||
| redirectTo: '/', | ||||||
| }); | ||||||
| if (status === 'loading') { | ||||||
| toast({ | ||||||
| type: 'error', | ||||||
| description: | ||||||
| 'Checking authentication status, please try again!', | ||||||
| }); | ||||||
|
|
||||||
| return; | ||||||
| } | ||||||
|
|
||||||
| if (isGuest) { | ||||||
| router.push('/login'); | ||||||
| } else { | ||||||
| signOut({ | ||||||
| redirectTo: '/', | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The sign-out call uses the Severity Level: Minor
Suggested change
Why it matters? ⭐next-auth's client signOut does not recognize redirectTo — the correct option to request a redirect after sign-out is callbackUrl (or relying on server-side helpers). As written the intent to redirect to '/' may be ignored at runtime; changing to callbackUrl fixes actual sign-out behavior and prevents surprising UX. Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** components/sidebar-user-nav.tsx
**Line:** 96:96
**Comment:**
*Logic Error: The sign-out call uses the `redirectTo` option, which matches your custom server-side `signOut` helper but is not a recognized option on `next-auth`'s client `signOut` function, so the intended redirect to `/` will be ignored and default behavior used instead; using `callbackUrl` aligns with the actual API and ensures redirect works as expected.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. |
||||||
| }); | ||||||
| } | ||||||
| }} | ||||||
| > | ||||||
| Sign out | ||||||
| {isGuest ? 'Login to your account' : 'Sign out'} | ||||||
| </button> | ||||||
| </DropdownMenuItem> | ||||||
| </DropdownMenuContent> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import type { ChatModel } from './models'; | ||
|
|
||
| export type MembershipTier = 'guest' | 'free'; | ||
|
|
||
| interface Entitlements { | ||
| maxMessagesPerDay: number; | ||
| chatModelsAvailable: Array<ChatModel['id']>; | ||
| } | ||
|
|
||
| export const entitlementsByMembershipTier: Record< | ||
| MembershipTier, | ||
| Entitlements | ||
| > = { | ||
| /* | ||
| * For users without an account | ||
| */ | ||
| guest: { | ||
| maxMessagesPerDay: 20, | ||
| chatModelsAvailable: ['chat-model', 'chat-model-reasoning'], | ||
| }, | ||
|
|
||
| /* | ||
| * For user with an account | ||
| */ | ||
| free: { | ||
| maxMessagesPerDay: 100, | ||
| chatModelsAvailable: ['chat-model', 'chat-model-reasoning'], | ||
| }, | ||
|
|
||
| /* | ||
| * TODO: For users with an account and a paid membership | ||
| */ | ||
| }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unhandled Database Errors
Guest authentication process lacks exception handling around database operations. Database connection failures or user creation errors will cause unhandled exceptions leading to 500 responses. Service availability degrades when authentication system fails without graceful error recovery.
Commitable Suggestion
Standards