feat: implement Supabase Auth, document isolation by user, and dynami…#3
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code Review by Qodo
1. Callback session not stored
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+10
to
+18
| if (code) { | ||
| const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL || "https://placeholder-project.supabase.co"; | ||
| const supabaseAnonKey = process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY || "placeholder-anon-key"; | ||
|
|
||
| const supabase = createClient(supabaseUrl, supabaseAnonKey); | ||
| await supabase.auth.exchangeCodeForSession(code); | ||
| } | ||
|
|
||
| return NextResponse.redirect(`${requestUrl.origin}/upload`); |
There was a problem hiding this comment.
1. Callback session not stored 🐞 Bug ≡ Correctness
/auth/callback exchanges the auth code in a server route handler but never persists the resulting session to the browser (cookie/localStorage), so AuthContext will still see session=null and the user remains logged out after the email confirmation redirect.
Agent Prompt
## Issue description
The Supabase PKCE callback is handled by a server route (`app/auth/callback/route.ts`) that calls `exchangeCodeForSession(code)` but does not write the session into browser storage (or set auth cookies). The app’s auth state is client-driven via `supabaseBrowser.auth.getSession()`, so users will still appear logged out after confirming their email.
## Issue Context
- Signup sets `emailRedirectTo` to `/auth/callback`.
- Auth state is read in the browser via `supabaseBrowser`.
- A Next.js route handler cannot populate browser localStorage; cookie-based auth would require Supabase SSR helpers.
## Fix Focus Areas
- app/auth/callback/route.ts[1-19]
- app/auth/page.tsx[46-52]
- components/providers/AuthContext.tsx[21-42]
## Suggested fix
Option A (simplest for SPA-style auth):
1. Replace the route handler with a **client page** at `app/auth/callback/page.tsx` (and remove `route.ts` to avoid path conflicts).
2. In the client page, read `code` from `useSearchParams()`, call `supabaseBrowser.auth.exchangeCodeForSession(code)`, then `router.replace('/upload')`.
3. Show a spinner + error UI on failure.
Option B (cookie-based SSR auth):
1. Use `@supabase/ssr` (or official Next auth helpers) in the callback handler to exchange the code and **set response cookies** so the browser has a persisted session.
2. Update `AuthContext` to read session from cookies/SSR client appropriately.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
66
to
70
| query_embedding: queryEmbedding, | ||
| match_threshold: 0.2, // cosine similarity threshold | ||
| match_count: 5, // retrieve top 5 matching blocks | ||
| p_user_id: userId, | ||
| } |
There was a problem hiding this comment.
2. Rpc parameter mismatch 🐞 Bug ≡ Correctness
/api/chat passes p_user_id into the match_documents RPC call, but the repository’s documented SQL defines match_documents(query_embedding, match_threshold, match_count) only; this will cause the RPC call to fail at runtime against the documented setup.
Agent Prompt
## Issue description
The `match_documents` RPC call now includes a named parameter `p_user_id`, but the repo’s Supabase setup SQL (README) defines `match_documents` with only three parameters. With Supabase/PostgREST RPC, providing an unexpected named argument causes a runtime error.
## Issue Context
- Chat API uses `supabase.rpc('match_documents', { ..., p_user_id })`.
- README provides the only in-repo definition of the function and does not include `p_user_id`.
## Fix Focus Areas
- app/api/chat/route.ts[63-71]
- README.md[66-90]
## Suggested fix
Choose one:
1. **Update the database function** to accept `p_user_id uuid` and filter rows by `documents.user_id = p_user_id`.
2. If DB changes are not intended, **remove `p_user_id`** from the RPC call and rely on RLS instead (but then ensure RLS is enabled and policies exist).
Also update the README SQL snippet to match the new function signature and behavior.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
115
to
119
| name, | ||
| content: chunk, | ||
| embedding: embeddings[index], | ||
| user_id: userId, | ||
| })); |
There was a problem hiding this comment.
3. Missing documents.user_id 🐞 Bug ≡ Correctness
/api/upload now inserts and filters on documents.user_id, but the repo’s documented Supabase table definition for documents does not include that column; upload and overwrite-delete will fail unless the DB schema is updated.
Agent Prompt
## Issue description
The upload API writes `user_id` into the `documents` table and filters deletes by `user_id`. The in-repo Supabase setup SQL (README) does not create this column, so inserts/filters will error against the documented schema.
## Issue Context
- `/api/upload` builds `records` containing `user_id`.
- README SQL defines `documents` without `user_id`.
## Fix Focus Areas
- app/api/upload/route.ts[101-124]
- README.md[57-65]
## Suggested fix
1. Update the Supabase schema:
- Add `user_id uuid not null references auth.users(id)` (or the appropriate auth schema).
- Add an index on `(user_id, name)` for overwrite delete and retrieval.
2. Update/introduce RLS policies (recommended) so isolation is enforced at the DB layer.
3. Update README SQL to include the new column and any required policies.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary by Qodo
Add Supabase Auth, user-scoped documents, and auth-gated UI
✨ Enhancement⚙️ Configuration changes🕐 40+ MinutesWalkthroughs
User Description
…c UI elements
AI Description
Diagram
graph TD UI["Client UI"] --> AC["AuthContext"] --> SA{{"Supabase Auth"}} UI -- "Bearer token" --> AU["/api/upload"] --> DB[("documents")] UI -- "Bearer token" --> CH["/api/chat"] --> DB CH --> EMB{{"Cohere Embeddings"}} CH --> LLM{{"Groq Chat"}} subgraph Legend direction LR _db[("Database")] ~~~ _api["API/Component"] ~~~ _ext{{"External"}} endHigh-Level Assessment
The following are alternative approaches to this PR:
1. Use Supabase SSR helpers + cookie sessions
2. Enforce isolation via RLS + non-service server client
3. Centralize API auth guard helper (shared function/middleware)
Recommendation: Current approach is reasonable for a first iteration: validate the Supabase JWT on the server, derive userId, and scope all DB operations by that value. For stronger long-term safety, consider migrating to RLS-enforced isolation (and/or Supabase SSR cookie sessions) so the database becomes the final enforcement layer and the client no longer needs to pass access tokens explicitly.
File Changes
Enhancement (14)
Other (1)