Finalize and Optimize Supabase Integration#505
Conversation
…nsistency. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
|
WalkthroughThis PR consolidates the codebase from external nanoid imports to an internal utility export, migrates user and chat management from mock storage to database-backed operations using Drizzle ORM, extends the database schema with new tables and fields for locations and visualizations, and adds authentication gates to user management operations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…tency. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
![charliecreates[bot]](https://avatars.githubusercontent.com/in/1140780?s=60&v=4){kind=small}
There was a problem hiding this comment.
Key correctness/security issues remain: lib/actions/chat-db.ts conditionally skips updating visibility/updatedAt, leading to stale chat metadata, and user management actions can create public.users rows with random UUIDs that may diverge from auth.users.id—a likely break for RLS and ownership. The settings/user-management flow weakened role validation from z.enum(...) to z.string(), making it easy to persist invalid roles. Finally, the new user-management server actions lack explicit authorization checks and should be admin-gated even with RLS.
Additional notes (3)
-
Maintainability |
lib/actions/chat-db.ts:99-103
saveChatonly updatesvisibilityandupdatedAtwhenchatData.titleis truthy, due to the surroundingif (chatData.title) { ... }guard. That means updates that only change visibility/updatedAt (or attempt to keep title unchanged) will silently do nothing. This is a correctness bug and will lead to stale metadata (e.g.,updatedAtnever moving unless a title is set). -
Maintainability |
lib/actions/chat-db.ts:119-123
The message upsert usesonConflictDoUpdate({ target: messages.id, ... }), but the insert payload already providesidvalues from the incomingchat.messages. If those IDs are newly generated per request (common with client-side IDs), this upsert will never trigger and you'll keep inserting duplicates. If the intent is to make message persistence idempotent, you likely want a stable conflict target (e.g.,(chat_id, created_at, role, ...)or a deterministic message id) or to ensure the client reuses the same message IDs on retries.
Also, only content and role are updated on conflict; changes to other columns (e.g., userId) will be ignored—ensure that's intended.
- Security |
components/settings/components/settings.tsx:40-48
The settings form schema was loosened fromz.enum(["admin","editor","viewer"])toz.string()for bothusers[].roleandnewUserRole. This removes client-side validation and makes it easy to persist arbitrary role strings into the DB (users.roleistext). If RLS/policies or server-side checks rely on known roles, this becomes a correctness/security issue.
Even if you keep the DB column as text, the API boundary should still enforce the allowed set.
Summary of changes
Summary of changes
- Replaced
nanoiddependency usage across the app with a local UUID-based helpernanoid()from@/lib/utils/nanoid. - Updated history list typing to reuse the DB-returned
Chattype (Chat as DrizzleChat) and derivedpathviachat.path || "/search/${chat.id}". - Refactored Settings UI imports/formatting and integrated authenticated user context via
useCurrentUser(), including guards preventing reads/writes when not logged in. - Migrated system prompt and selected model persistence from Redis/filesystem to the
userstable via Drizzle (db.update(users)/db.select(...)). - Replaced in-memory user store actions with database-backed
getUsers,addUser,updateUserRole,removeUser, and addedsearchUsers. - Enhanced chat persistence:
saveChatnow updates additional chat metadata (visibility,updatedAt) and upserts messages usingonConflictDoUpdate.updateDrawingContextnow writes arole: 'data'message directly viadbCreateMessage.
- Expanded
lib/db/schema.tsto align with the live DB: added columns (e.g.,embedding, sharing fields) and introduced new tables/types (chat_participants,system_prompts,locations,visualizations, plus PostGIS/vector custom types) and relationships.
| export async function addUser(newUser: { email: string; role: UserRole }): Promise<{ user?: User; error?: string }> { | ||
| try { | ||
| // In Supabase, users are usually created via Auth. | ||
| // This action assumes we're adding a record to the public.users table. | ||
| // We'd typically need a UUID for 'id' that matches auth.users.id. | ||
| // If this is just for internal user management, we'll insert a new record. | ||
|
|
||
| const [insertedUser] = await db.insert(users).values({ | ||
| email: newUser.email, | ||
| role: newUser.role, | ||
| }).returning({ | ||
| id: users.id, | ||
| email: users.email, | ||
| role: users.role, | ||
| }); | ||
|
|
||
| // Check if user already exists (simple check, real DB would handle this better) | ||
| if (usersStore[userId].some(user => user.email === newUser.email)) { | ||
| console.warn(`[Action: addUser] User ${newUser.email} already exists for ${userId}`); | ||
| return { error: 'User with this email already exists.' }; | ||
| revalidatePath('/settings'); | ||
| return { user: insertedUser }; | ||
| } catch (error) { | ||
| console.error('[Action: addUser] Error adding user:', error); | ||
| return { error: 'Failed to add user. Email might already exist.' }; | ||
| } |
There was a problem hiding this comment.
addUser inserts into the app's public.users table without setting id. In your schema, users.id is defaultRandom(), which will not match auth.users.id in Supabase. Given the PR context mentions “a trigger to sync auth users with the public profile table”, creating separate random IDs here risks breaking joins/ownership checks and RLS assumptions (e.g., chats.user_id referencing users.id).
If the intended model is: public.users.id == auth.users.id, then this action should not generate a new UUID; it should either (a) accept an auth user id, or (b) create auth users via Admin API and let the trigger populate the profile row.
Suggestion
Align public.users.id with auth.users.id.
Options:
- Change
addUserto require anid(UUID from auth):
export async function addUser(newUser: { id: string; email: string; role: UserRole }) {
return db.insert(users).values({
id: newUser.id,
email: newUser.email,
role: newUser.role,
})
}- Or, remove this insert entirely and instead call Supabase Admin to create the auth user, relying on the trigger to create
public.users.
Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| /** | ||
| * Retrieves all users from the database. | ||
| * Restricted to admins in a real app, but following current structure for now. | ||
| */ | ||
| export async function getUsers(): Promise<{ users: User[] }> { | ||
| noStore(); | ||
| try { | ||
| const result = await db.select({ | ||
| id: users.id, | ||
| email: users.email, | ||
| role: users.role, | ||
| }).from(users); | ||
|
|
||
| export async function getUsers(userId: string = 'default-user'): Promise<{ users: User[] }> { | ||
| await simulateDBDelay(); | ||
| if (!usersStore[userId]) { | ||
| usersStore[userId] = []; | ||
| return { users: result }; | ||
| } catch (error) { | ||
| console.error('[Action: getUsers] Error fetching users:', error); | ||
| return { users: [] }; | ||
| } | ||
| console.log(`[Action: getUsers] Fetched users for ${userId}:`, usersStore[userId]); | ||
| return { users: usersStore[userId] }; | ||
| } | ||
|
|
||
| export async function addUser(userId: string = 'default-user', newUser: { email: string; role: UserRole }): Promise<{ user?: User; error?: string }> { | ||
| await simulateDBDelay(); | ||
| if (!usersStore[userId]) { | ||
| usersStore[userId] = []; | ||
| } | ||
| /** | ||
| * Adds a new user to the database. | ||
| * If the user already exists in auth.users, they should have a record in public.users. | ||
| * This action might be used for onboarding or admin management. | ||
| */ | ||
| export async function addUser(newUser: { email: string; role: UserRole }): Promise<{ user?: User; error?: string }> { | ||
| try { | ||
| // In Supabase, users are usually created via Auth. | ||
| // This action assumes we're adding a record to the public.users table. | ||
| // We'd typically need a UUID for 'id' that matches auth.users.id. | ||
| // If this is just for internal user management, we'll insert a new record. | ||
|
|
||
| const [insertedUser] = await db.insert(users).values({ | ||
| email: newUser.email, | ||
| role: newUser.role, | ||
| }).returning({ | ||
| id: users.id, | ||
| email: users.email, | ||
| role: users.role, | ||
| }); | ||
|
|
||
| // Check if user already exists (simple check, real DB would handle this better) | ||
| if (usersStore[userId].some(user => user.email === newUser.email)) { | ||
| console.warn(`[Action: addUser] User ${newUser.email} already exists for ${userId}`); | ||
| return { error: 'User with this email already exists.' }; | ||
| revalidatePath('/settings'); | ||
| return { user: insertedUser }; | ||
| } catch (error) { | ||
| console.error('[Action: addUser] Error adding user:', error); | ||
| return { error: 'Failed to add user. Email might already exist.' }; | ||
| } | ||
|
|
||
| const userToAdd: User = { ...newUser, id: Math.random().toString(36).substr(2, 9) }; | ||
| usersStore[userId].push(userToAdd); | ||
| console.log(`[Action: addUser] Added user ${newUser.email} for ${userId}:`, userToAdd); | ||
| revalidatePath('/settings'); // Assuming settings page path, adjust if needed | ||
| return { user: userToAdd }; | ||
| } | ||
|
|
||
| export async function updateUserRole(userId: string = 'default-user', userEmail: string, newRole: UserRole): Promise<{ user?: User; error?: string }> { | ||
| await simulateDBDelay(); | ||
| if (!usersStore[userId]) { | ||
| return { error: 'User list not found.' }; | ||
| } | ||
| /** | ||
| * Updates a user's role by their email. | ||
| */ | ||
| export async function updateUserRole(userEmail: string, newRole: UserRole): Promise<{ user?: User; error?: string }> { | ||
| try { | ||
| const [updatedUser] = await db.update(users) | ||
| .set({ role: newRole }) | ||
| .where(eq(users.email, userEmail)) | ||
| .returning({ | ||
| id: users.id, | ||
| email: users.email, | ||
| role: users.role, | ||
| }); | ||
|
|
||
| if (!updatedUser) { | ||
| return { error: 'User not found.' }; | ||
| } | ||
|
|
||
| const userIndex = usersStore[userId].findIndex(user => user.email === userEmail); | ||
| if (userIndex === -1) { | ||
| console.warn(`[Action: updateUserRole] User ${userEmail} not found for ${userId}`); | ||
| return { error: 'User not found.' }; | ||
| revalidatePath('/settings'); | ||
| return { user: updatedUser }; | ||
| } catch (error) { | ||
| console.error('[Action: updateUserRole] Error updating role:', error); | ||
| return { error: 'Failed to update user role.' }; | ||
| } | ||
|
|
||
| usersStore[userId][userIndex].role = newRole; | ||
| console.log(`[Action: updateUserRole] Updated role for ${userEmail} to ${newRole} for ${userId}`); | ||
| revalidatePath('/settings'); | ||
| return { user: usersStore[userId][userIndex] }; | ||
| } | ||
|
|
||
| export async function removeUser(userId: string = 'default-user', userEmail: string): Promise<{ success?: boolean; error?: string }> { | ||
| await simulateDBDelay(); | ||
| if (!usersStore[userId]) { | ||
| return { error: 'User list not found.' }; | ||
| } | ||
| /** | ||
| * Removes a user from the database by their email. | ||
| */ | ||
| export async function removeUser(userEmail: string): Promise<{ success?: boolean; error?: string }> { | ||
| try { | ||
| const result = await db.delete(users).where(eq(users.email, userEmail)).returning(); | ||
|
|
||
| const initialLength = usersStore[userId].length; | ||
| usersStore[userId] = usersStore[userId].filter(user => user.email !== userEmail); | ||
| if (result.length === 0) { | ||
| return { error: 'User not found.' }; | ||
| } | ||
|
|
||
| if (usersStore[userId].length === initialLength) { | ||
| console.warn(`[Action: removeUser] User ${userEmail} not found for ${userId}`); | ||
| return { error: 'User not found.' }; | ||
| revalidatePath('/settings'); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| console.error('[Action: removeUser] Error removing user:', error); | ||
| return { error: 'Failed to remove user.' }; | ||
| } |
There was a problem hiding this comment.
getUsers, addUser, updateUserRole, removeUser, and searchUsers do not enforce any authorization/role checks. Given you enabled RLS “on all tables” per PR context, you should still treat these server actions as sensitive and explicitly gate them (e.g., admin-only) to avoid accidental exposure via UI routes or future reuse.
Relying solely on UI hiding is not sufficient; enforce on the server action boundary.
Suggestion
Add an explicit authorization check using the current user and role (and fail closed). Example sketch:
async function requireAdmin() {
const userId = await getCurrentUserIdOnServer();
if (!userId) throw new Error('Not authenticated');
const [me] = await db.select({ role: users.role }).from(users).where(eq(users.id, userId)).limit(1);
if (me?.role !== 'admin') throw new Error('Forbidden');
return userId;
}
export async function getUsers() {
await requireAdmin();
// ...
}Reply with "@CharlieHelps yes please" if you'd like me to add a commit with this suggestion.
| export async function updateUserRole(userEmail: string, newRole: UserRole): Promise<{ user?: User; error?: string }> { | ||
| try { | ||
| const [updatedUser] = await db.update(users) | ||
| .set({ role: newRole }) | ||
| .where(eq(users.email, userEmail)) | ||
| .returning({ | ||
| id: users.id, | ||
| email: users.email, | ||
| role: users.role, | ||
| }); | ||
|
|
||
| if (!updatedUser) { | ||
| return { error: 'User not found.' }; | ||
| } | ||
|
|
||
| const userIndex = usersStore[userId].findIndex(user => user.email === userEmail); | ||
| if (userIndex === -1) { | ||
| console.warn(`[Action: updateUserRole] User ${userEmail} not found for ${userId}`); | ||
| return { error: 'User not found.' }; | ||
| revalidatePath('/settings'); | ||
| return { user: updatedUser }; | ||
| } catch (error) { | ||
| console.error('[Action: updateUserRole] Error updating role:', error); | ||
| return { error: 'Failed to update user role.' }; | ||
| } | ||
|
|
||
| usersStore[userId][userIndex].role = newRole; | ||
| console.log(`[Action: updateUserRole] Updated role for ${userEmail} to ${newRole} for ${userId}`); | ||
| revalidatePath('/settings'); | ||
| return { user: usersStore[userId][userIndex] }; | ||
| } | ||
|
|
||
| export async function removeUser(userId: string = 'default-user', userEmail: string): Promise<{ success?: boolean; error?: string }> { | ||
| await simulateDBDelay(); | ||
| if (!usersStore[userId]) { | ||
| return { error: 'User list not found.' }; | ||
| } | ||
| /** | ||
| * Removes a user from the database by their email. | ||
| */ | ||
| export async function removeUser(userEmail: string): Promise<{ success?: boolean; error?: string }> { | ||
| try { | ||
| const result = await db.delete(users).where(eq(users.email, userEmail)).returning(); | ||
|
|
||
| const initialLength = usersStore[userId].length; | ||
| usersStore[userId] = usersStore[userId].filter(user => user.email !== userEmail); | ||
| if (result.length === 0) { | ||
| return { error: 'User not found.' }; | ||
| } | ||
|
|
||
| if (usersStore[userId].length === initialLength) { | ||
| console.warn(`[Action: removeUser] User ${userEmail} not found for ${userId}`); | ||
| return { error: 'User not found.' }; | ||
| revalidatePath('/settings'); | ||
| return { success: true }; | ||
| } catch (error) { | ||
| console.error('[Action: removeUser] Error removing user:', error); | ||
| return { error: 'Failed to remove user.' }; |
There was a problem hiding this comment.
updateUserRole / removeUser use where(eq(users.email, userEmail)). Email can be NULL per your User interface and schema (email: text('email') without .notNull()). That makes email-based updates/deletes ambiguous and potentially ineffective.
Even if emails are non-null in practice, using email as the identifier is brittle (case sensitivity, change over time). Prefer updating by immutable id.
Suggestion
Change these actions to accept a userId and operate on users.id:
export async function updateUserRole(userId: string, newRole: UserRole) {
return db.update(users).set({ role: newRole }).where(eq(users.id, userId)) ...
}
export async function removeUser(userId: string) {
return db.delete(users).where(eq(users.id, userId)) ...
}Update the UI to pass id (you already have it after insert and in getUsers).
Reply with "@CharlieHelps yes please" if you’d like me to prepare a commit converting role/delete operations to be id-based.
|
|
||
| export const users = pgTable('users', { | ||
| id: uuid('id').primaryKey().defaultRandom(), // Assuming Supabase user IDs are UUIDs | ||
| // email: text('email'), // Supabase handles this in auth.users | ||
| // Other profile fields if necessary | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| email: text('email'), | ||
| role: text('role').default('viewer'), | ||
| selectedModel: text('selected_model'), | ||
| systemPrompt: text('system_prompt'), | ||
| }); | ||
|
|
||
| export const chats = pgTable('chats', { | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }), // References a user ID | ||
| title: varchar('title', { length: 256 }).notNull().default('Untitled Chat'), | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }), | ||
| title: text('title').notNull().default('Untitled Chat'), | ||
| visibility: text('visibility').default('private'), // 'private', 'public' | ||
| path: text('path'), | ||
| sharePath: text('share_path'), | ||
| shareableLinkId: uuid('shareable_link_id').defaultRandom().unique(), | ||
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| // RLS in Supabase will use policies, but marking public visibility can be a column | ||
| visibility: varchar('visibility', { length: 50 }).default('private'), // e.g., 'private', 'public' | ||
| // any other metadata for the chat | ||
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), | ||
| }); | ||
|
|
||
| export const messages = pgTable('messages', { | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| chatId: uuid('chat_id').notNull().references(() => chats.id, { onDelete: 'cascade' }), | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }), // Who sent the message | ||
| role: varchar('role', { length: 50 }).notNull(), // e.g., 'user', 'assistant', 'system', 'tool' | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }), | ||
| role: text('role').notNull(), // 'user', 'assistant', 'system', 'tool', 'data' | ||
| content: text('content').notNull(), | ||
| embedding: vector('embedding'), | ||
| locationId: uuid('location_id'), // Reference added via relation/manual constraint if needed | ||
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| }); | ||
|
|
||
| export const chatParticipants = pgTable('chat_participants', { | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| chatId: uuid('chat_id').notNull().references(() => chats.id, { onDelete: 'cascade' }), | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }), | ||
| role: text('role').notNull().default('collaborator'), // 'owner', 'collaborator' | ||
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| }); | ||
|
|
||
| export const systemPrompts = pgTable('system_prompts', { | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| userId: uuid('user_id').notNull().references(() => users.id, { onDelete: 'cascade' }), | ||
| prompt: text('prompt').notNull(), | ||
| createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(), | ||
| updatedAt: timestamp('updated_at', { withTimezone: true }).defaultNow().notNull(), | ||
| }); |
There was a problem hiding this comment.
You introduced both users.systemPrompt / users.selectedModel columns and a new system_prompts table, but the code now stores prompts in users.systemPrompt.
This creates a schema/API mismatch and likely dead code/data duplication. Decide on one source of truth: either a single prompt per user in users, or a versioned/history table in system_prompts—but not both without a clear reason and migration path.
Suggestion
Pick one model and remove the other:
- Single current prompt: keep
users.systemPrompt, deletesystem_prompts. - Prompt history/versioning: use
system_promptsand updategetSystemPrompt/saveSystemPromptto read/write it (likely upsert per user).
Reply with "@CharlieHelps yes please" if you want me to add a commit that removes system_prompts (or alternatively rewires the actions to use it).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
lib/actions/chat-db.ts (2)
93-108:⚠️ Potential issue | 🟠 MajorUpdate of
visibilityandupdatedAtis incorrectly gated onchatData.titlebeing truthy.Line 102 sets
title,visibility, andupdatedAtonly whenchatData.titleis truthy. This has two problems:
- If the caller wants to update visibility or touch
updatedAtwithout changing the title (e.g., making a chat public), the update is silently skipped.- If
chatData.visibilityorchatData.updatedAtareundefined, they will be written asNULL, potentially corrupting existing values.Proposed fix: decouple the update fields from the title guard
} else { - // Optionally update chat metadata here if needed, e.g., title - if (chatData.title) { - await tx.update(chats).set({ title: chatData.title, visibility: chatData.visibility, updatedAt: chatData.updatedAt }).where(eq(chats.id, chatId)); - } + // Update metadata on the existing chat + const updateFields: Partial<typeof chats.$inferInsert> = { + updatedAt: chatData.updatedAt ?? new Date(), + }; + if (chatData.title) updateFields.title = chatData.title; + if (chatData.visibility) updateFields.visibility = chatData.visibility; + + await tx.update(chats).set(updateFields).where(eq(chats.id, chatId)); }
223-223:⚠️ Potential issue | 🟡 MinorRemove debug
console.logfrom module scope.This
console.logexecutes on every module load in production. Remove it or guard it behind a development check.Proposed fix
-console.log('Chat DB actions loaded. Ensure getCurrentUserId() is correctly implemented for server-side usage if applicable.');components/settings/components/user-management-form.tsx (1)
143-148:⚠️ Potential issue | 🔴 Critical
remove(index)only removes the user from the local form state, not from the database.Clicking the trash icon calls
remove(index)which updates theuseFieldArraystate, but never callsremoveUserfrom@/lib/actions/users. The user remains in the database, creating a data inconsistency.Proposed fix: call the server action before removing locally
- <Button variant="ghost" size="icon" onClick={() => remove(index)}> + <Button variant="ghost" size="icon" onClick={async () => { + const result = await removeUser(user.email); + if (result.error) { + toast({ title: 'Error', description: result.error, variant: 'destructive' }); + return; + } + remove(index); + }}> <Trash2 className="h-4 w-4 text-destructive" /> </Button>You'll also need to import
removeUser:-import { addUser, type UserRole } from '@/lib/actions/users'; +import { addUser, removeUser, type UserRole } from '@/lib/actions/users';lib/actions/chat.ts (1)
68-88:⚠️ Potential issue | 🔴 Critical
redirect()insidetry/catchis caught and suppressed — the redirect will never execute.In Next.js,
redirect()works by throwing a specialNEXT_REDIRECTerror. Since it's called inside thetryblock (line 83), thecatchat line 84 intercepts it and returns{ error: 'Failed to clear chat history' }instead of allowing the redirect to propagate.Proposed fix: move redirect outside the try/catch
export async function clearChats( userId?: string | null ): Promise<{ error?: string } | void> { const currentUserId = userId || (await getCurrentUserIdOnServer()) if (!currentUserId) { console.error('clearChats: No user ID provided or found.') return { error: 'User ID is required to clear chats' } } + let cleared = false; try { const success = await dbClearHistory(currentUserId) if (!success) { return { error: 'Failed to clear chats from database.' } } - revalidatePath('/') - redirect('/') + cleared = true; } catch (error) { console.error('Error clearing chats from DB:', error) return { error: 'Failed to clear chat history' } } + + if (cleared) { + revalidatePath('/') + redirect('/') + } }
🤖 Fix all issues with AI agents
In `@app/page.tsx`:
- Line 2: The import line has inconsistent spacing around the named import;
update the import of nanoid (symbol: nanoid) to use consistent spacing like
"import { nanoid } from '@/lib/utils/nanoid'" (i.e., add the missing space after
the identifier inside the braces) and run the project's formatter/ESLint fixer
to ensure consistent import styling across the file (target: app/page.tsx,
symbol: nanoid).
In `@components/settings/components/settings.tsx`:
- Around line 43-47: The Zod schema loosened role and newUserRole from the enum
to z.string(), removing client-side validation; restore enum validation by using
the same enum values as UserRole (e.g., z.enum(["admin","editor","viewer"])) for
the role field inside the users array and for newUserRole so the form rejects
invalid roles before submission; update the schema definitions referencing role
and newUserRole in settings.tsx to use the enum validator consistent with the
UserRole type exported from lib/actions/users.ts.
- Line 70: The Settings component calls useCurrentUser() but ignores the loading
flag, causing the form to be interactive before auth resolves; update the
component to destructure loading from useCurrentUser() and either return the
SettingsSkeleton early when loading is true (render SettingsSkeleton inside
Settings) or keep rendering but disable form interactions (e.g., disable the
Save Changes submit button and any inputs while loading is true); ensure the
fetchData effect still waits for a valid userId by checking loading before
skipping so defaults aren't left stale until auth resolves.
In `@components/settings/components/user-management-form.tsx`:
- Line 49: The addUser flow inserts a standalone public.users row with an
auto-generated UUID that isn’t linked to Supabase Auth, and users.email lacks a
unique constraint so duplicate DB-only records can be created; fix by either (A)
preventing manual creation in addUser and instead rely on syncing auth.users →
public.users via a DB trigger (update/remove addUser UI/path), (B) enforce a
UNIQUE constraint on public.users.email and change addUser to create/get an auth
user via the Supabase Admin API then upsert the linked public.users record using
the auth user id, or (C) if keeping DB-only creation, modify addUser to check
for existing email (and return a precise error) and implement a reconciliation
step to link or merge records with auth.users; reference addUser, public.users,
auth.users, and users.email when making these changes.
In `@lib/actions/chat.ts`:
- Around line 123-124: Remove the redundant inline directive inside
updateDrawingContext: delete the "'use server';" statement present at the start
of the export async function updateDrawingContext(...) because the file already
has a file-level 'use server' and the per-function directive is unnecessary;
simply keep the function signature and body as-is without the inline directive.
In `@lib/actions/users.ts`:
- Around line 21-34: Add an authorization guard to all user-management server
actions so only admins can call them: call await requireAdmin() at the start of
getUsers, addUser, updateUserRole, and removeUser (the functions exported from
this module) and return/throw if it fails; ensure requireAdmin() is imported or
available in the file and place the call before any
db.select/db.insert/db.update/db.delete operations so unauthorized requests
cannot enumerate, create, modify, or delete users.
- Around line 155-172: searchUsers currently returns matching user emails to any
caller; restrict it to authenticated (and ideally admin) users by validating the
caller before running the query: in searchUsers, fetch the current user/session
(e.g., via getCurrentUser/getSession or injected auth context) and if missing
return/throw an unauthorized error, and if enforcing admin-only check a
user.isAdmin (or roles) flag and reject non-admins; only after passing
auth/authorization run the db.select against users (id, email) and return
results. Ensure you reference the existing searchUsers function and its
db.select/from(users)/where(ilike(...)) flow when adding the auth checks and
error handling.
- Around line 11-15: The User interface's role field is too permissive (string |
null) causing loss of type safety; change User.role to use the stricter union
(UserRole | null) so returned Users preserve the exact role type, then update
all functions that construct/return User (e.g., addUser, getUser, updateUser) to
ensure they assign values of type UserRole or null and adjust any type
assertions or casts accordingly; also update any tests or callers that expect
string to accept UserRole | null to keep consistency.
In `@lib/db/schema.ts`:
- Around line 11-16: The vector custom type currently hardcodes the embedding
dimension as 'vector(1536)' in the customType implementation (symbol: vector,
method: dataType), which ties the schema to a specific embedding model; update
the code by adding a clear comment above the vector definition stating the
assumed embedding model and dimension (e.g., text-embedding-ada-002 => 1536) and
note that switching models (e.g., text-embedding-3-large => 3072) requires a
schema migration or changing this value, and optionally document where to
centralize/configure this dimension if you plan to support multiple embedding
models.
- Around line 18-24: The users.email column lacks a uniqueness constraint which
makes operations in updateUserRole, removeUser, and searchUsers ambiguous;
update the users table definition in schema.ts by adding a unique constraint to
the email column (e.g., change text('email') to text('email').unique() and
consider .notNull() if appropriate) and create/apply a DB migration to enforce
uniqueness (handle or dedupe existing duplicate emails before applying the
migration so updateUserRole/removeUser affect a single row).
- Around line 49-55: Add a composite unique constraint on the chatParticipants
table to prevent duplicate participant rows: import unique from
'drizzle-orm/pg-core' and update the chatParticipants definition to include
unique([chatId, userId]) (referencing the chatParticipants table symbol and the
chatId and userId column symbols) so the pair (chatId, userId) is enforced as
unique at the DB/schema level.
- Around line 44-45: The messages table's locationId column is defined as
uuid('location_id') without a DB-level foreign key; update the column definition
for locationId in the messages schema to add a foreign-key reference to the
locations table (e.g., .references('locations.id')) and set onDelete: 'set null'
while preserving its nullable property so DB enforces referential integrity
consistent with the ORM relation referenced around lines 130–133.
In `@lib/utils/nanoid.ts`:
- Around line 1-5: The nanoid() function currently returns a UUID v4 (via
uuidv4) which duplicates generateUUID() in lib/utils/index.ts and misleads
readers; remove lib/utils/nanoid.ts, consolidate by importing generateUUID from
lib/utils in all call sites (or add a re-export in lib/utils/index.ts like
export { generateUUID as nanoid } with a comment if you need the shorter name),
and update all imports to reference the single utility (generateUUID or the
re-exported nanoid) to eliminate duplication and fix the misleading name.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (14)
app/actions.tsxapp/page.tsxcomponents/chat-panel.tsxcomponents/header-search-button.tsxcomponents/history-list.tsxcomponents/resolution-carousel.tsxcomponents/search-related.tsxcomponents/settings/components/settings.tsxcomponents/settings/components/user-management-form.tsxlib/actions/chat-db.tslib/actions/chat.tslib/actions/users.tslib/db/schema.tslib/utils/nanoid.ts
🧰 Additional context used
🧬 Code graph analysis (7)
lib/utils/nanoid.ts (1)
lib/utils/index.ts (1)
generateUUID(14-16)
lib/actions/chat-db.ts (1)
lib/db/schema.ts (2)
chats(26-36)messages(38-47)
lib/actions/users.ts (3)
lib/db/index.ts (1)
db(25-25)lib/db/schema.ts (1)
users(18-24)lib/auth/get-current-user.ts (1)
getCurrentUserIdOnServer(112-127)
components/history-list.tsx (3)
lib/db/schema.ts (1)
chats(26-36)components/sidebar/chat-history-client.tsx (3)
chat(157-161)ChatHistoryClientProps(26-28)ChatHistoryClient(30-197)components/history-item.tsx (1)
pathname(51-72)
components/settings/components/settings.tsx (3)
lib/auth/use-current-user.ts (1)
useCurrentUser(5-23)components/settings/components/settings-skeleton.tsx (1)
SettingsSkeleton(5-36)components/sidebar/chat-history-client.tsx (1)
ChatHistoryClientProps(26-28)
components/settings/components/user-management-form.tsx (1)
lib/actions/users.ts (2)
addUser(42-64)UserRole(9-9)
lib/actions/chat.ts (3)
lib/auth/get-current-user.ts (1)
getCurrentUserIdOnServer(112-127)lib/db/index.ts (1)
db(25-25)lib/db/schema.ts (1)
users(18-24)
🔇 Additional comments (18)
lib/actions/chat-db.ts (1)
122-122: Upsert on messages looks correct for the intent.The
onConflictDoUpdatetargetingmessages.idand updatingcontentandroleviaEXCLUDEDis valid PostgreSQL/Drizzle syntax and correctly implements the upsert semantics described in the PR objectives.One consideration: if a message's
createdAtshould also be preserved or updated on conflict, you may want to include it in thesetclause—currently it's silently dropped for existing rows.components/settings/components/user-management-form.tsx (1)
56-60: Defensive defaults forroleare appropriate.The fallback to
''for email and'viewer'for role handles the nullable return type from the server. This aligns well with theUsertype where both fields arestring | null.lib/db/schema.ts (1)
4-9: PostGIS custom types and geo tables are well-structured.The geometry type with SRID 4326, locations table with GeoJSON + geometry fields, and visualizations table provide a solid foundation for spatial data. Relations are properly defined.
Also applies to: 65-83
components/settings/components/settings.tsx (2)
83-100: Good:fetchDataproperly guards onuserIdand re-runs when auth resolves.The
useEffectdependency onuserIdensures that onceuseCurrentUser()resolves, data is fetched. The early return on!userIdprevents unnecessary calls.
102-110: Good:onSubmitguard prevents unauthenticated saves.Clear error toast when
userIdis missing. This is the right approach for a user-facing guard.lib/actions/users.ts (1)
114-130:getSelectedModelandsaveSelectedModelcorrectly guard on auth.Both functions call
getCurrentUserIdOnServer()and return early if unauthenticated. This is the pattern the other actions should follow.lib/actions/chat.ts (2)
90-121: Chat save mapping logic is well-structured.The translation from
OldChatTypetoDbNewChat/DbNewMessageis clean. TheeffectiveUserIdfallback, proper Date construction, andJSON.stringifyfor object content are all good patterns. The guard at lines 91–93 prevents saves without a user ID.
151-186:saveSystemPromptandgetSystemPromptare clean DB-backed implementations.Good input validation (lines 155–156), proper error handling, and consistent patterns with the rest of the file.
app/actions.tsx (2)
7-7: Imports: top-levelimportafterexport constand function declarations.Line 7 has an
importstatement after theexport const maxDurationon line 5 (inapp/page.tsxcontext — noting for cross-reference). In this file, imports are properly grouped at the top, which is fine.
10-10: The import at line 10 is correct. However, the premise of this review appears to be inaccurate: there is no behavioral change occurring here. The codebase uses an internalnanoid()wrapper that returns UUIDs, but this is by design—not a migration. The database schema was always UUID-based (see initial migration withuuid PRIMARY KEY DEFAULT gen_random_uuid()), and the external nanoid library was never used as a dependency. No ID format validation or route constraints reject UUIDs, so compatibility is not a concern.Likely an incorrect or invalid review comment.
components/header-search-button.tsx (1)
10-10: Import path update is consistent with the project-wide refactor.components/resolution-carousel.tsx (1)
15-15: Import path update is consistent with the project-wide refactor.components/search-related.tsx (1)
15-15: Import path update is consistent with the project-wide refactor.components/chat-panel.tsx (1)
11-11: Import path update is consistent with the project-wide refactor.components/history-list.tsx (4)
5-5: Type aliasingChat as DrizzleChatfor disambiguation — reasonable approach.The alias helps distinguish from the
Chattype in@/lib/typesused elsewhere (e.g.,app/actions.tsx). This is clear and consistent with the broader DB migration.Also applies to: 37-37
15-62: Good addition of error boundary with try/catch.The try/catch wrapping the async server component logic with a user-facing error state is a solid improvement over silent failures.
11-13: ThegetChatsimplementation inlib/actions/chat.tsalready handlesundefineduserId gracefully. The function has an explicit guard clause that returns an empty array when userId is missing, preventing any database query with an undefined filter. No changes needed.
40-43: No type compatibility issues exist with this spread operation.The
{ ...chat, path: chat.path ||/search/${chat.id}}spread creates exactly the typeHistoryItemexpects (DrizzleChat & { path: string }). AllDrizzleChatfields—includingcreatedAtasDate,visibility, andupdatedAt—are present and compatible. TheformatDateWithTimefunction (line 13 in history-item.tsx) already handles bothDateandstringinputs, so there's no actual type mismatch concern here.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| @@ -1,5 +1,5 @@ | |||
| import { Chat } from '@/components/chat' | |||
| import {nanoid } from 'nanoid' | |||
| import {nanoid } from '@/lib/utils/nanoid' | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor formatting inconsistency in import.
There's an asymmetric space: {nanoid } — missing space after {.
-import {nanoid } from '@/lib/utils/nanoid'
+import { nanoid } from '@/lib/utils/nanoid'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import {nanoid } from '@/lib/utils/nanoid' | |
| import { nanoid } from '@/lib/utils/nanoid' |
🤖 Prompt for AI Agents
In `@app/page.tsx` at line 2, The import line has inconsistent spacing around the
named import; update the import of nanoid (symbol: nanoid) to use consistent
spacing like "import { nanoid } from '@/lib/utils/nanoid'" (i.e., add the
missing space after the identifier inside the braces) and run the project's
formatter/ESLint fixer to ensure consistent import styling across the file
(target: app/page.tsx, symbol: nanoid).
|
|
||
| try { | ||
| const result = await addUser('default-user', { email: newUserEmail, role: newUserRole }); | ||
| const result = await addUser({ email: newUserEmail, role: newUserRole as UserRole }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking lib/actions/users.ts for addUser implementation ==="
fd -n "users.ts" -path "*/lib/actions/*" | head -1 | xargs cat -n | sed -n '40,65p'
echo ""
echo "=== Searching for schema files to find constraints ==="
fd -e sql | head -20
echo ""
echo "=== Checking for unique constraint on users.email in schema ==="
fd -e sql -o -e ts -o -e js | xargs grep -n "email.*unique\|unique.*email" 2>/dev/null | head -20Repository: QueueLab/QCX
Length of output: 700
🏁 Script executed:
#!/bin/bash
echo "=== Find lib/actions/users.ts ==="
git ls-files | grep "lib/actions/users.ts"
echo ""
echo "=== Read lib/actions/users.ts (full file to understand addUser) ==="
git ls-files | grep "lib/actions/users.ts" | xargs wc -l
echo ""
echo "=== Read drizzle migration files to check schema ==="
git ls-files | grep "drizzle/migrations" | head -2 | xargs wc -lRepository: QueueLab/QCX
Length of output: 373
🏁 Script executed:
#!/bin/bash
echo "=== Find and read lib/actions/users.ts ==="
git ls-files | grep "lib/actions/users.ts" | xargs cat -n
echo ""
echo "=== Check migration 0000 for schema ==="
cat drizzle/migrations/0000_sweet_metal_master.sql
echo ""
echo "=== Check migration 0001 for changes ==="
cat drizzle/migrations/0001_add_calendar_notes.sqlRepository: QueueLab/QCX
Length of output: 9144
🏁 Script executed:
#!/bin/bash
echo "=== Find and read database schema definition ==="
git ls-files | grep -E "lib/db/schema" | xargs cat -n
echo ""
echo "=== Search for RLS policies on users table ==="
rg "users.*POLICY|CREATE POLICY.*users" --type=sql
echo ""
echo "=== Search for triggers related to auth sync ==="
rg "trigger|CREATE.*FUNCTION" --type=sql -i
echo ""
echo "=== Check for any Supabase client admin methods ==="
rg "createUser|inviteUserByEmail|auth.*admin" --type=ts --type=jsRepository: QueueLab/QCX
Length of output: 8316
🏁 Script executed:
#!/bin/bash
echo "=== Search for auth callbacks, signup, or user sync flows ==="
rg "auth.*callback|onAuthStateChanged|session.*user|auth.*user.*create" --type=ts --type=js -i
echo ""
echo "=== Check for middleware or hooks handling auth state ==="
git ls-files | grep -E "middleware|auth.*hook|callback" | head -20
echo ""
echo "=== Search for any existing unique constraints in schema ==="
rg "unique()" --type=ts | head -20Repository: QueueLab/QCX
Length of output: 554
🏁 Script executed:
#!/bin/bash
echo "=== Read middleware.ts to understand auth flow ==="
cat -n middleware.ts
echo ""
echo "=== Check lib/auth/get-current-user.ts ==="
cat -n lib/auth/get-current-user.ts
echo ""
echo "=== Search for signup or user creation entry points ==="
rg "signup|register|createUser|newUser" --type=ts --type=tsx -i | head -30Repository: QueueLab/QCX
Length of output: 6827
🏁 Script executed:
#!/bin/bash
echo "=== Search for signup/register endpoints ==="
fd -e ts -e tsx -e js -e jsx | xargs grep -l "signup\|register\|auth.*route\|/api/auth" 2>/dev/null | head -20
echo ""
echo "=== Check for any API routes related to auth ==="
find . -path "*/api/auth*" -o -path "*/api/*auth*" 2>/dev/null | head -20
echo ""
echo "=== Verify: does addUser expect auth.users to already exist? ==="
rg "email.*already.*exist|duplicate.*email|constraint" lib/actions/users.tsRepository: QueueLab/QCX
Length of output: 253
🏁 Script executed:
#!/bin/bash
echo "=== Check lib/supabase/client.ts ==="
cat -n lib/supabase/client.ts
echo ""
echo "=== Look for any documentation or comments about user management ==="
rg "admin.*user|invite.*user|sync.*auth" --type=ts -i | head -20
echo ""
echo "=== Check where addUser is actually called from ==="
rg "addUser" --type=ts --type=tsxRepository: QueueLab/QCX
Length of output: 2638
🏁 Script executed:
#!/bin/bash
echo "=== Find where addUser is called ==="
rg "addUser" --type=ts | grep -v "export async function addUser"
echo ""
echo "=== Check user-management-form.tsx to see the context ==="
find . -name "user-management-form.tsx" | xargs cat -n | head -100Repository: QueueLab/QCX
Length of output: 5244
addUser creates records with disconnected UUIDs, not linked to auth.users.
The addUser function (lines 49–56) inserts a row with an auto-generated UUID that has no relationship to the user's Supabase Auth identity. Additionally, the users.email field lacks a unique constraint, so the error message "Email might already exist" is misleading—duplicates can be silently created in the database. Without a mechanism to sync these public.users records with auth.users, any actual Supabase authentication will not find a matching row in your app's user table.
Consider either:
- (a) Prevent manual user creation and instead sync Supabase Auth signups to
public.usersvia a trigger onauth.users - (b) Add a unique constraint on
emailand reconcile records during sign-up - (c) Use the Supabase Admin API to create actual Auth users (not just database rows) when onboarding new users
🤖 Prompt for AI Agents
In `@components/settings/components/user-management-form.tsx` at line 49, The
addUser flow inserts a standalone public.users row with an auto-generated UUID
that isn’t linked to Supabase Auth, and users.email lacks a unique constraint so
duplicate DB-only records can be created; fix by either (A) preventing manual
creation in addUser and instead rely on syncing auth.users → public.users via a
DB trigger (update/remove addUser UI/path), (B) enforce a UNIQUE constraint on
public.users.email and change addUser to create/get an auth user via the
Supabase Admin API then upsert the linked public.users record using the auth
user id, or (C) if keeping DB-only creation, modify addUser to check for
existing email (and return a precise error) and implement a reconciliation step
to link or merge records with auth.users; reference addUser, public.users,
auth.users, and users.email when making these changes.
…d schema integrity. Co-authored-by: ngoiyaeric <115367894+ngoiyaeric@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
components/settings/components/settings.tsx (1)
133-133:⚠️ Potential issue | 🟡 MinorRemove debug
console.logbefore merging.
console.log("Submitted data:", data)will print the full form payload (system prompt, user list, model) to the browser console in production. This appears to be a leftover debug statement.Proposed fix
- console.log("Submitted data:", data)lib/actions/chat.ts (3)
77-88:⚠️ Potential issue | 🔴 Critical
redirect()insidetry/catchis swallowed — clear-chats will never redirect.In Next.js 15,
redirect()works by throwing a specialNEXT_REDIRECTerror. Because it's called inside thetryblock at Line 83, the genericcatchat Line 84 intercepts it and returns{ error: 'Failed to clear chat history' }instead of performing the redirect.Proposed fix — move redirect outside try/catch
export async function clearChats( userId?: string | null ): Promise<{ error?: string } | void> { const currentUserId = userId || (await getCurrentUserIdOnServer()) if (!currentUserId) { console.error('clearChats: No user ID provided or found.') return { error: 'User ID is required to clear chats' } } + let success = false; try { - const success = await dbClearHistory(currentUserId) + success = await dbClearHistory(currentUserId) if (!success) { return { error: 'Failed to clear chats from database.' } } - revalidatePath('/') - redirect('/') } catch (error) { console.error('Error clearing chats from DB:', error) return { error: 'Failed to clear chat history' } } + + revalidatePath('/') + redirect('/') }
90-121:⚠️ Potential issue | 🟠 Major
saveChattrusts the caller-supplieduserId— consider server-side verification.Similar to
saveSystemPrompt,saveChatis a server action that acceptsuserIdas a parameter without verifying it matches the authenticated session. A client could potentially save or overwrite chats under another user's ID. While RLS policies at the database level may mitigate this (as mentioned in the PR objectives), the application layer should also enforce ownership.Proposed fix
export async function saveChat(chat: OldChatType, userId: string): Promise<string | null> { - if (!userId && !chat.userId) { - console.error('saveChat: userId is required either as a parameter or in chat object.') + const currentUserId = await getCurrentUserIdOnServer(); + if (!currentUserId) { + console.error('saveChat: User not authenticated.') return null; } - const effectiveUserId = userId || chat.userId; + const effectiveUserId = currentUserId;
55-66:⚠️ Potential issue | 🔴 Critical
getChatMessageshas no authorization check — any caller with achatIdcan read all messages.Unlike
getChatwhich takes auserIdand delegates todbGetChatwith access filtering,getChatMessagespasses onlychatIdtodbGetMessagesByChatIdwith no user verification. Since this is a'use server'action, any client code can invoke it with any chat ID to retrieve all messages from that chat, including messages from other users.While currently the only call site in
app/search/[id]/page.tsxis protected viagetChat, the exported function itself is vulnerable.dbGetMessagesByChatIdperforms no access control despite themessagestable having auserIdcolumn.Proposed fix — add user verification
-export async function getChatMessages(chatId: string): Promise<DrizzleMessage[]> { +export async function getChatMessages(chatId: string, userId?: string): Promise<DrizzleMessage[]> { - if (!chatId) { + const currentUserId = userId || (await getCurrentUserIdOnServer()); + if (!chatId || !currentUserId) { - console.warn('getChatMessages called without chatId'); + console.warn('getChatMessages called without chatId or userId'); return []; } try { + // Verify the user has access to this chat before returning messages + const chat = await dbGetChat(chatId, currentUserId); + if (!chat) { + console.warn(`getChatMessages: user ${currentUserId} has no access to chat ${chatId}`); + return []; + } return dbGetMessagesByChatId(chatId); } catch (error) { console.error(`Error fetching messages for chat ${chatId} in getChatMessages:`, error); return []; } }
🤖 Fix all issues with AI agents
In `@components/settings/components/settings.tsx`:
- Line 6: Remove the unused Next.js router import and its instantiation: delete
the "useRouter" import and any "const router = useRouter()" declaration in the
settings component (symbols: useRouter, router) so there is no dead code left;
before removing, search the file for any remaining calls to router.refresh() or
router usage and remove or replace them if found.
In `@lib/actions/chat.ts`:
- Around line 150-167: The saveSystemPrompt action currently trusts the userId
parameter and must instead verify it against the authenticated session: in
saveSystemPrompt, call getCurrentUserIdOnServer() (or reuse the same logic from
updateDrawingContext) to obtain the server-side user id, compare it with the
passed userId (or discard the passed userId entirely and use the session id),
and return an error if they mismatch or if no session exists; keep the existing
DB update (db.update(users).set(...).where(eq(users.id, userId))) but ensure the
id used there is the verified session id to prevent clients from overwriting
other users' prompts.
In `@lib/actions/users.ts`:
- Around line 188-210: searchUsers builds an ilike pattern directly from query
so `%` and `_` in user input become SQL wildcards; update searchUsers to escape
LIKE metacharacters before composing the pattern: escape backslash, percent and
underscore in the incoming query (e.g. replace backslash, % and _ with escaped
versions) then use the escaped string in `%${escapedQuery}%` passed to
ilike(users.email, ...) when calling db.select/from/users; ensure the escape
happens before the ilike call and keep the existing auth and error handling.
- Around line 41-57: getUsers currently swallows authorization failures from
requireAdmin by catching all errors and returning { users: [] }; change the
behavior so callers can distinguish "unauthorized" from "empty": either re-throw
authorization errors coming from requireAdmin (detect by checking the thrown
error's type/name/message or a specific Auth/Forbidden error class) instead of
returning an empty list, or extend the function's return type (e.g., Promise<{
users: User[]; error?: string }>) and populate an error field for auth failures
while still returning users for non-auth errors; update the catch in getUsers
accordingly and keep other DB errors logged and handled as before.
In `@lib/db/schema.ts`:
- Line 25: Add a DB-level CHECK constraint to the role column in
lib/db/schema.ts so only the allowed UserRole values are persisted: update the
role definition (currently role: text('role').default('viewer')) to include a
CHECK that restricts values to the same set as the UserRole type (e.g.,
'viewer','editor','admin') or switch to a DB enum type if preferred; also add a
migration reflecting this constraint so the database enforces the rule in
addition to the application-level UserRole and requireAdmin checks.
- Around line 22-28: Remove the unused systemPrompts table definition from the
schema: delete the pgTable declaration named systemPrompts (the unused symbol)
so only the users table with its systemPrompt column remains; ensure no other
code references systemPrompts (chat handling uses users.systemPrompt per
lib/actions/chat.ts lines ~159,175-180) and run tests/migrations to update
schema artifacts accordingly.
In `@lib/utils/index.ts`:
- Around line 18-22: The export alias `nanoid` in lib/utils/index.ts is
misleading because it forwards generateUUID (a UUID v4) under the nanoid name;
change this by either renaming the export to match its semantics (export
generateUUID or export { generateUUID as generateUUID } / re-export as uuid or
uuidv4) and update all call sites to use the new name, or if you must keep the
`nanoid` name for compatibility, add a clear `@deprecated` JSDoc on the nanoid
export pointing callers to generateUUID and schedule replacing usages; locate
the symbol `generateUUID` and the alias `nanoid` to apply the change and update
imports across the codebase accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
app/actions.tsxapp/page.tsxcomponents/chat-panel.tsxcomponents/header-search-button.tsxcomponents/resolution-carousel.tsxcomponents/search-related.tsxcomponents/settings/components/settings.tsxlib/actions/chat.tslib/actions/users.tslib/db/schema.tslib/utils/index.ts
🧰 Additional context used
🧬 Code graph analysis (3)
lib/actions/chat.ts (3)
lib/db/index.ts (1)
db(25-25)lib/db/schema.ts (1)
users(22-28)lib/actions/chat-db.ts (2)
saveChat(83-126)getChat(22-40)
lib/actions/users.ts (5)
lib/actions/chat-db.ts (1)
User(11-11)lib/auth/get-current-user.ts (1)
getCurrentUserIdOnServer(112-127)lib/db/index.ts (1)
db(25-25)lib/db/schema.ts (1)
users(22-28)components/settings/components/user-management-form.tsx (1)
UserManagementForm(19-164)
lib/db/schema.ts (2)
lib/types/index.ts (1)
Chat(49-57)lib/actions/chat-db.ts (1)
msg(117-121)
🔇 Additional comments (9)
components/resolution-carousel.tsx (1)
15-15: Import path update looks correct.The import now resolves to the internal utility module. No behavioral changes in this file beyond the ID format change covered in the
lib/utils/index.tsreview.components/search-related.tsx (1)
15-15: LGTM.Import path change is consistent with the rest of the PR.
components/header-search-button.tsx (1)
10-10: LGTM.Import path change is consistent with the rest of the PR.
components/chat-panel.tsx (1)
11-11: LGTM.Import path change is consistent with the rest of the PR.
app/page.tsx (1)
2-2: LGTM — import path updated and formatting issue from prior review is resolved.app/actions.tsx (1)
10-10: Migration to UUID v4 is already implemented; no downstream constraints found.The import from
@/lib/utilsalready provides UUID v4 (not legacy nanoid). Thenanoidexport is aliased togenerateUUID()which returnsuuidv4(), and the database schema uses Drizzle's nativeuuid()type without length constraints. URL segments handle 36-character UUIDs without issue.Likely an incorrect or invalid review comment.
components/settings/components/settings.tsx (1)
65-105: Auth-aware loading and data fetching look good.The
useCurrentUserhook is properly integrated:authLoadinggates both data fetching (Line 86) and rendering (Lines 103–105), andisSavingcorrectly replaces the oldisLoadingname to reflect its purpose. All hooks are called unconditionally before the early return, so the Rules of Hooks are respected.lib/actions/users.ts (1)
17-35: Good addition of therequireAdminauthorization gate.This addresses the previously identified privilege-escalation vulnerability. The guard is consistently applied to all admin-restricted actions.
lib/actions/chat.ts (1)
123-148:updateDrawingContextproperly resolves the user from the server session.This is the correct pattern — getting
userIdfromgetCurrentUserIdOnServer()rather than accepting it as a client-supplied parameter. The other actions (saveChat,saveSystemPrompt) should follow this same pattern.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| import React, { useState, useEffect } from 'react' | ||
| import { motion, AnimatePresence } from 'framer-motion' | ||
| import * as Tabs from '@radix-ui/react-tabs' | ||
| import { useRouter } from 'next/navigation' |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
useRouter is imported and instantiated but never used.
router was previously used for router.refresh(), which has been removed. The import and declaration are now dead code.
Proposed fix
-import { useRouter } from 'next/navigation'- const router = useRouter()Also applies to: 67-67
🤖 Prompt for AI Agents
In `@components/settings/components/settings.tsx` at line 6, Remove the unused
Next.js router import and its instantiation: delete the "useRouter" import and
any "const router = useRouter()" declaration in the settings component (symbols:
useRouter, router) so there is no dead code left; before removing, search the
file for any remaining calls to router.refresh() or router usage and remove or
replace them if found.
| export async function saveSystemPrompt( | ||
| userId: string, | ||
| prompt: string | ||
| ): Promise<{ success?: boolean; error?: string }> { | ||
| if (!userId) { | ||
| return { error: 'User ID is required' } | ||
| } | ||
|
|
||
| if (!prompt) { | ||
| return { error: 'Prompt is required' } | ||
| } | ||
| if (!userId) return { error: 'User ID is required' } | ||
| if (!prompt) return { error: 'Prompt is required' } | ||
|
|
||
| try { | ||
| await redis.set(`system_prompt:${userId}`, prompt) | ||
| await db.update(users) | ||
| .set({ systemPrompt: prompt }) | ||
| .where(eq(users.id, userId)); | ||
|
|
||
| return { success: true } | ||
| } catch (error) { | ||
| console.error('saveSystemPrompt: Error saving system prompt:', error) | ||
| console.error('saveSystemPrompt: Error:', error) | ||
| return { error: 'Failed to save system prompt' } | ||
| } | ||
| } |
There was a problem hiding this comment.
saveSystemPrompt accepts userId from the caller without server-side verification.
This 'use server' action trusts the userId parameter. A malicious client could call saveSystemPrompt('victim-uuid', 'malicious prompt') to overwrite another user's system prompt. The userId should be verified against the authenticated session, similar to how updateDrawingContext resolves it from getCurrentUserIdOnServer().
Proposed fix — verify userId matches the current session
export async function saveSystemPrompt(
userId: string,
prompt: string
): Promise<{ success?: boolean; error?: string }> {
- if (!userId) return { error: 'User ID is required' }
+ const currentUserId = await getCurrentUserIdOnServer();
+ if (!currentUserId) return { error: 'Not authenticated' }
+ if (userId !== currentUserId) return { error: 'Unauthorized' }
if (!prompt) return { error: 'Prompt is required' }
try {
await db.update(users)
.set({ systemPrompt: prompt })
.where(eq(users.id, userId));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function saveSystemPrompt( | |
| userId: string, | |
| prompt: string | |
| ): Promise<{ success?: boolean; error?: string }> { | |
| if (!userId) { | |
| return { error: 'User ID is required' } | |
| } | |
| if (!prompt) { | |
| return { error: 'Prompt is required' } | |
| } | |
| if (!userId) return { error: 'User ID is required' } | |
| if (!prompt) return { error: 'Prompt is required' } | |
| try { | |
| await redis.set(`system_prompt:${userId}`, prompt) | |
| await db.update(users) | |
| .set({ systemPrompt: prompt }) | |
| .where(eq(users.id, userId)); | |
| return { success: true } | |
| } catch (error) { | |
| console.error('saveSystemPrompt: Error saving system prompt:', error) | |
| console.error('saveSystemPrompt: Error:', error) | |
| return { error: 'Failed to save system prompt' } | |
| } | |
| } | |
| export async function saveSystemPrompt( | |
| userId: string, | |
| prompt: string | |
| ): Promise<{ success?: boolean; error?: string }> { | |
| const currentUserId = await getCurrentUserIdOnServer(); | |
| if (!currentUserId) return { error: 'Not authenticated' } | |
| if (userId !== currentUserId) return { error: 'Unauthorized' } | |
| if (!prompt) return { error: 'Prompt is required' } | |
| try { | |
| await db.update(users) | |
| .set({ systemPrompt: prompt }) | |
| .where(eq(users.id, userId)); | |
| return { success: true } | |
| } catch (error) { | |
| console.error('saveSystemPrompt: Error:', error) | |
| return { error: 'Failed to save system prompt' } | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@lib/actions/chat.ts` around lines 150 - 167, The saveSystemPrompt action
currently trusts the userId parameter and must instead verify it against the
authenticated session: in saveSystemPrompt, call getCurrentUserIdOnServer() (or
reuse the same logic from updateDrawingContext) to obtain the server-side user
id, compare it with the passed userId (or discard the passed userId entirely and
use the session id), and return an error if they mismatch or if no session
exists; keep the existing DB update
(db.update(users).set(...).where(eq(users.id, userId))) but ensure the id used
there is the verified session id to prevent clients from overwriting other
users' prompts.
| export async function getUsers(): Promise<{ users: User[] }> { | ||
| noStore(); | ||
| try { | ||
| await requireAdmin(); | ||
| const result = await db.select({ | ||
| id: users.id, | ||
| email: users.email, | ||
| role: users.role, | ||
| }).from(users); | ||
|
|
||
| // Type cast since we know roles in DB match UserRole or null | ||
| return { users: result as User[] }; | ||
| } catch (error: any) { | ||
| console.error('[Action: getUsers] Error:', error.message); | ||
| return { users: [] }; | ||
| } | ||
| } |
There was a problem hiding this comment.
getUsers swallows authorization errors — caller cannot distinguish "unauthorized" from "empty table".
When requireAdmin() throws, the catch block logs the error but returns { users: [] }. The client sees the same response whether the user is unauthorized or the table is genuinely empty, making it impossible to show an appropriate error message (e.g., "Access denied").
Consider adding an error field to the return type, or re-throwing authorization errors before the generic catch.
Proposed fix
-export async function getUsers(): Promise<{ users: User[] }> {
+export async function getUsers(): Promise<{ users: User[]; error?: string }> {
noStore();
try {
await requireAdmin();
const result = await db.select({
id: users.id,
email: users.email,
role: users.role,
}).from(users);
// Type cast since we know roles in DB match UserRole or null
return { users: result as User[] };
} catch (error: any) {
console.error('[Action: getUsers] Error:', error.message);
- return { users: [] };
+ return { users: [], error: error.message };
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function getUsers(): Promise<{ users: User[] }> { | |
| noStore(); | |
| try { | |
| await requireAdmin(); | |
| const result = await db.select({ | |
| id: users.id, | |
| email: users.email, | |
| role: users.role, | |
| }).from(users); | |
| // Type cast since we know roles in DB match UserRole or null | |
| return { users: result as User[] }; | |
| } catch (error: any) { | |
| console.error('[Action: getUsers] Error:', error.message); | |
| return { users: [] }; | |
| } | |
| } | |
| export async function getUsers(): Promise<{ users: User[]; error?: string }> { | |
| noStore(); | |
| try { | |
| await requireAdmin(); | |
| const result = await db.select({ | |
| id: users.id, | |
| email: users.email, | |
| role: users.role, | |
| }).from(users); | |
| // Type cast since we know roles in DB match UserRole or null | |
| return { users: result as User[] }; | |
| } catch (error: any) { | |
| console.error('[Action: getUsers] Error:', error.message); | |
| return { users: [], error: error.message }; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@lib/actions/users.ts` around lines 41 - 57, getUsers currently swallows
authorization failures from requireAdmin by catching all errors and returning {
users: [] }; change the behavior so callers can distinguish "unauthorized" from
"empty": either re-throw authorization errors coming from requireAdmin (detect
by checking the thrown error's type/name/message or a specific Auth/Forbidden
error class) instead of returning an empty list, or extend the function's return
type (e.g., Promise<{ users: User[]; error?: string }>) and populate an error
field for auth failures while still returning users for non-auth errors; update
the catch in getUsers accordingly and keep other DB errors logged and handled as
before.
| export async function searchUsers(query: string) { | ||
| noStore(); | ||
| if (!query) return []; | ||
|
|
||
| const userId = await getCurrentUserIdOnServer(); | ||
| if (!userId) { | ||
| throw new Error('Unauthorized'); | ||
| } | ||
|
|
||
| try { | ||
| const result = await db.select({ | ||
| id: users.id, | ||
| email: users.email, | ||
| }) | ||
| .from(users) | ||
| .where(ilike(users.email, `%${query}%`)) | ||
| .limit(10); | ||
|
|
||
| return result; | ||
| } catch (error) { | ||
| console.error('[Action: searchUsers] Error:', error); | ||
| return []; | ||
| } |
There was a problem hiding this comment.
LIKE-wildcard characters in query are not escaped — users can craft broad search patterns.
Since ilike treats % and _ as wildcards, a caller passing % as the query results in %%% which matches every email. While the endpoint is auth-gated and data exposure is limited to authenticated users, escaping these special characters prevents unintended broad matches.
Proposed fix — escape LIKE metacharacters
+function escapeLike(str: string): string {
+ return str.replace(/[%_\\]/g, '\\$&');
+}
+
export async function searchUsers(query: string) {
noStore();
if (!query) return [];
const userId = await getCurrentUserIdOnServer();
if (!userId) {
throw new Error('Unauthorized');
}
try {
const result = await db.select({
id: users.id,
email: users.email,
})
.from(users)
- .where(ilike(users.email, `%${query}%`))
+ .where(ilike(users.email, `%${escapeLike(query)}%`))
.limit(10);
return result;
} catch (error) {
console.error('[Action: searchUsers] Error:', error);
return [];
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function searchUsers(query: string) { | |
| noStore(); | |
| if (!query) return []; | |
| const userId = await getCurrentUserIdOnServer(); | |
| if (!userId) { | |
| throw new Error('Unauthorized'); | |
| } | |
| try { | |
| const result = await db.select({ | |
| id: users.id, | |
| email: users.email, | |
| }) | |
| .from(users) | |
| .where(ilike(users.email, `%${query}%`)) | |
| .limit(10); | |
| return result; | |
| } catch (error) { | |
| console.error('[Action: searchUsers] Error:', error); | |
| return []; | |
| } | |
| function escapeLike(str: string): string { | |
| return str.replace(/[%_\\]/g, '\\$&'); | |
| } | |
| export async function searchUsers(query: string) { | |
| noStore(); | |
| if (!query) return []; | |
| const userId = await getCurrentUserIdOnServer(); | |
| if (!userId) { | |
| throw new Error('Unauthorized'); | |
| } | |
| try { | |
| const result = await db.select({ | |
| id: users.id, | |
| email: users.email, | |
| }) | |
| .from(users) | |
| .where(ilike(users.email, `%${escapeLike(query)}%`)) | |
| .limit(10); | |
| return result; | |
| } catch (error) { | |
| console.error('[Action: searchUsers] Error:', error); | |
| return []; | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@lib/actions/users.ts` around lines 188 - 210, searchUsers builds an ilike
pattern directly from query so `%` and `_` in user input become SQL wildcards;
update searchUsers to escape LIKE metacharacters before composing the pattern:
escape backslash, percent and underscore in the incoming query (e.g. replace
backslash, % and _ with escaped versions) then use the escaped string in
`%${escapedQuery}%` passed to ilike(users.email, ...) when calling
db.select/from/users; ensure the escape happens before the ilike call and keep
the existing auth and error handling.
| export const users = pgTable('users', { | ||
| id: uuid('id').primaryKey().defaultRandom(), // Assuming Supabase user IDs are UUIDs | ||
| // email: text('email'), // Supabase handles this in auth.users | ||
| // Other profile fields if necessary | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| email: text('email').unique(), // Enforced unique for user identity | ||
| role: text('role').default('viewer'), | ||
| selectedModel: text('selected_model'), | ||
| systemPrompt: text('system_prompt'), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if the systemPrompts table is referenced anywhere in the codebase besides its schema definition
echo "=== References to systemPrompts (excluding schema definition) ==="
rg -n 'systemPrompts' --type=ts -g '!lib/db/schema.ts'
echo ""
echo "=== References to users.systemPrompt or systemPrompt column ==="
rg -n 'systemPrompt' --type=ts -g '!lib/db/schema.ts'Repository: QueueLab/QCX
Length of output: 2054
Remove unused systemPrompts table from schema.
The systemPrompts table (lines 74–80) is not referenced anywhere in the codebase. All system prompt functionality uses the systemPrompt column directly from the users table (see lib/actions/chat.ts lines 159, 175–180). Remove the unused table to eliminate schema confusion.
🤖 Prompt for AI Agents
In `@lib/db/schema.ts` around lines 22 - 28, Remove the unused systemPrompts table
definition from the schema: delete the pgTable declaration named systemPrompts
(the unused symbol) so only the users table with its systemPrompt column
remains; ensure no other code references systemPrompts (chat handling uses
users.systemPrompt per lib/actions/chat.ts lines ~159,175-180) and run
tests/migrations to update schema artifacts accordingly.
| // Other profile fields if necessary | ||
| id: uuid('id').primaryKey().defaultRandom(), | ||
| email: text('email').unique(), // Enforced unique for user identity | ||
| role: text('role').default('viewer'), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding a database-level check constraint for role values.
The role column is text with a default of 'viewer', but nothing at the DB level prevents invalid values like 'superadmin'. The application-level UserRole type provides safety, but a DB check constraint would add defense-in-depth.
This is low priority since the Drizzle ORM typed interface and the requireAdmin gate provide adequate protection in practice.
🤖 Prompt for AI Agents
In `@lib/db/schema.ts` at line 25, Add a DB-level CHECK constraint to the role
column in lib/db/schema.ts so only the allowed UserRole values are persisted:
update the role definition (currently role: text('role').default('viewer')) to
include a CHECK that restricts values to the same set as the UserRole type
(e.g., 'viewer','editor','admin') or switch to a DB enum type if preferred; also
add a migration reflecting this constraint so the database enforces the rule in
addition to the application-level UserRole and requireAdmin checks.
| /** | ||
| * Re-export generateUUID as nanoid for shorter naming and compatibility with existing code. | ||
| * Returns a UUID v4 string. | ||
| */ | ||
| export { generateUUID as nanoid }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Misleading alias: nanoid now returns a UUID v4, not a nanoid.
The original nanoid package produces short (21-char), URL-safe, collision-resistant IDs like V1StGXR8_Z5jdHi6B-myT. This alias silently changes the format to 36-char hyphenated UUIDs (xxxxxxxx-xxxx-4xxx-…). While the PR objective to standardize on UUIDs is fine, keeping the name nanoid is confusing—future developers (or even the current team in a few months) will assume nanoid semantics.
Consider renaming the alias to match what it actually produces, or updating all call sites to use generateUUID directly. If backward compatibility with the name is truly needed short-term, at minimum add a @deprecated JSDoc tag pointing callers to generateUUID.
-/**
- * Re-export generateUUID as nanoid for shorter naming and compatibility with existing code.
- * Returns a UUID v4 string.
- */
-export { generateUUID as nanoid };
+// All call sites should migrate to `generateUUID`.
+// This alias exists only for transitional compatibility and will be removed.
+/** `@deprecated` Use `generateUUID` instead. This does NOT produce nanoid-format IDs. */
+export { generateUUID as nanoid };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Re-export generateUUID as nanoid for shorter naming and compatibility with existing code. | |
| * Returns a UUID v4 string. | |
| */ | |
| export { generateUUID as nanoid }; | |
| // All call sites should migrate to `generateUUID`. | |
| // This alias exists only for transitional compatibility and will be removed. | |
| /** `@deprecated` Use `generateUUID` instead. This does NOT produce nanoid-format IDs. */ | |
| export { generateUUID as nanoid }; |
🤖 Prompt for AI Agents
In `@lib/utils/index.ts` around lines 18 - 22, The export alias `nanoid` in
lib/utils/index.ts is misleading because it forwards generateUUID (a UUID v4)
under the nanoid name; change this by either renaming the export to match its
semantics (export generateUUID or export { generateUUID as generateUUID } /
re-export as uuid or uuidv4) and update all call sites to use the new name, or
if you must keep the `nanoid` name for compatibility, add a clear `@deprecated`
JSDoc on the nanoid export pointing callers to generateUUID and schedule
replacing usages; locate the symbol `generateUUID` and the alias `nanoid` to
apply the change and update imports across the codebase accordingly.
2 participants
This PR finalizes the Supabase integration by:
PR created automatically by Jules for task 10488130424105591202 started by @ngoiyaeric
Summary by CodeRabbit
New Features
Bug Fixes
Improvements