feat: enhance memory retrieval API with authentication and room valid#1617
Conversation
…ation - Added header validation and room ownership checks to the GET /api/memories/get endpoint. - Implemented error handling for missing room ID, unauthorized access, room not found, and forbidden access. - Created unit tests for various scenarios including authentication, room existence, and memory retrieval success. - Updated useMessageLoader and related hooks to include access token for fetching messages.
|
@pradipthaadhi is attempting to deploy a commit to the Recoupable Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThis PR adds authentication and authorization checks to the memory retrieval API endpoint and updates the client-side message loading flow to propagate access tokens, ensuring requests validate headers, confirm room ownership, and enforce account-based access control. Changes
Sequence DiagramsequenceDiagram
participant Client as Client App
participant Loader as useMessageLoader Hook
participant API as /api/memories/get
participant DB as Database/Room Service
Client->>Loader: useVercelChat passes accessToken
Loader->>Loader: Check accessToken exists
alt Missing accessToken
Loader->>Loader: Set isLoading=true, bail early
else accessToken Present
Loader->>API: getClientMessages(roomId, accessToken)
API->>API: validateHeaders(req)
alt Header Validation Fails
API-->>Loader: Return Response (early exit)
else Validation Passes
API->>API: Extract accountId from headers
alt No accountId
API-->>Loader: 401 Unauthorized
else accountId Present
API->>DB: getRoom(roomId)
alt Room Not Found
API-->>Loader: 404 Room not found
else Room Exists
API->>API: Compare room.account_id vs authResult.accountId
alt Access Denied
API-->>Loader: 403 Forbidden
else Access Granted
API->>DB: Query memories
API-->>Loader: Return memory data
Loader->>Client: Update messages state
end
end
end
end
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested Reviewers
Poem
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f84db0a94d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!accessToken) { | ||
| setIsLoading(true); | ||
| return; |
There was a problem hiding this comment.
This branch keeps isLoading stuck at true whenever accessToken is falsy, but never sets an error or terminal state. If useAccessToken() returns null (e.g., token fetch fails or user session is partially initialized), the chat page remains on the loading skeleton indefinitely with no recovery path. This became user-visible after making message loading depend on the token.
Useful? React with 👍 / 👎.
| const room = await getRoom(roomId); | ||
| if (!room) { | ||
| return Response.json({ error: "Room not found" }, { status: 404 }); |
There was a problem hiding this comment.
The handler treats any falsy getRoom result as "Room not found", but getRoom returns null for all Supabase errors as well as missing rows. That means transient DB/query failures are now reported as 404 instead of a server error, which can mislead clients and hide operational issues.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/supabase/getClientMessages.tsx (1)
8-11:⚠️ Potential issue | 🟠 MajorHandle non-2xx responses explicitly instead of treating them as empty history.
Right now
401/403/500can silently resolve to[], which hides real failures and makes auth issues look like “no messages”.Proposed fix
- const data = await response.json(); + if (!response.ok) { + const err = await response.json().catch(() => null); + throw new Error( + err?.error ?? `Failed to load messages (${response.status})`, + ); + } + const data = await response.json();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/supabase/getClientMessages.tsx` around lines 8 - 11, The code currently treats any non-OK HTTP response as an empty history by immediately reading response.json() and using data?.data || [], so update getClientMessages (the code handling response, response.json(), and the memories variable) to explicitly check response.ok (or response.status) before parsing; if !response.ok, read the response body for details and throw or return a clear error (including response.status and body) instead of returning []; this ensures 401/403/500 are surfaced rather than silently becoming an empty memories array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/api/memories/get/route.ts`:
- Around line 18-27: The route returns errors with two different shapes ({
error: ... } in early exits vs { message: ... } in the catch), so change the
catch-path response to use the same error key and shape as the rest of the
handler; locate the handler in app/api/memories/get/route.ts (references:
getRoom, authResult, Response.json) and update the catch block(s) that return {
message: ... } to return { error: "<same content>" } with the appropriate HTTP
status, and ensure any other error-returning lines (including the one around
line 40) follow this unified { error: ... } format.
In `@hooks/useMessageLoader.ts`:
- Around line 33-36: The hook useMessageLoader sets setIsLoading(true) and
immediately returns when accessToken is falsy, which can leave the component
permanently loading; update the early-accessToken branch in useMessageLoader to
clear the loading state and handle the unauthenticated case (e.g., call
setIsLoading(false) before return and optionally set an error or empty messages
via setHasError/setMessages) so the hook doesn't remain stuck when accessToken
is missing.
In `@lib/supabase/getClientMessages.tsx`:
- Line 3: The request URL uses chatId directly which can break for non-UUID IDs;
update the fetch call that constructs `/api/memories/get?roomId=${chatId}` (in
getClientMessages / the function performing this fetch) to URL-encode the chatId
using encodeURIComponent(chatId) when composing the query string so the
resulting request is always well-formed.
---
Outside diff comments:
In `@lib/supabase/getClientMessages.tsx`:
- Around line 8-11: The code currently treats any non-OK HTTP response as an
empty history by immediately reading response.json() and using data?.data || [],
so update getClientMessages (the code handling response, response.json(), and
the memories variable) to explicitly check response.ok (or response.status)
before parsing; if !response.ok, read the response body for details and throw or
return a clear error (including response.status and body) instead of returning
[]; this ensures 401/403/500 are surfaced rather than silently becoming an empty
memories array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a7c9968-9fd4-4b0a-b35c-7b080a0dece8
⛔ Files ignored due to path filters (1)
app/api/memories/get/__tests__/route.test.tsis excluded by!**/*.test.*and included byapp/**
📒 Files selected for processing (4)
app/api/memories/get/route.tshooks/useMessageLoader.tshooks/useVercelChat.tslib/supabase/getClientMessages.tsx
| return Response.json({ error: "Unauthorized" }, { status: 401 }); | ||
| } | ||
|
|
||
| const room = await getRoom(roomId); | ||
| if (!room) { | ||
| return Response.json({ error: "Room not found" }, { status: 404 }); | ||
| } | ||
| if (room.account_id !== authResult.accountId) { | ||
| return Response.json({ error: "Forbidden" }, { status: 403 }); | ||
| } |
There was a problem hiding this comment.
Unify error response shape across this route.
Early exits return { error: ... }, but the catch path returns { message: ... }. This creates an inconsistent API contract for clients handling failures.
Proposed fix
export async function GET(req: NextRequest) {
+ const jsonError = (status: number, error: string) =>
+ Response.json({ error }, { status });
+
const roomId = req.nextUrl.searchParams.get("roomId");
if (!roomId) {
- return Response.json({ error: "Room ID is required" }, { status: 400 });
+ return jsonError(400, "Room ID is required");
}
@@
if (!authResult.accountId) {
- return Response.json({ error: "Unauthorized" }, { status: 401 });
+ return jsonError(401, "Unauthorized");
}
@@
if (!room) {
- return Response.json({ error: "Room not found" }, { status: 404 });
+ return jsonError(404, "Room not found");
}
if (room.account_id !== authResult.accountId) {
- return Response.json({ error: "Forbidden" }, { status: 403 });
+ return jsonError(403, "Forbidden");
}
@@
} catch (error) {
console.error("[api/memories/get] Error:", error);
const message = error instanceof Error ? error.message : "failed";
- return Response.json({ message }, { status: 400 });
+ return jsonError(400, message);
}
}As per coding guidelines, API routes must “Return consistent response formats”.
Also applies to: 40-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/api/memories/get/route.ts` around lines 18 - 27, The route returns errors
with two different shapes ({ error: ... } in early exits vs { message: ... } in
the catch), so change the catch-path response to use the same error key and
shape as the rest of the handler; locate the handler in
app/api/memories/get/route.ts (references: getRoom, authResult, Response.json)
and update the catch block(s) that return { message: ... } to return { error:
"<same content>" } with the appropriate HTTP status, and ensure any other
error-returning lines (including the one around line 40) follow this unified {
error: ... } format.
| if (!accessToken) { | ||
| setIsLoading(true); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Avoid permanent loading state when accessToken is missing.
This branch sets loading to true and exits, so if token retrieval fails or user stays unauthenticated, loading can remain stuck.
Proposed fix
if (!accessToken) {
- setIsLoading(true);
+ setIsLoading(false);
+ setError(new Error("Authentication required to load messages"));
return;
}As per coding guidelines, hooks should “Handle edge cases and errors”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hooks/useMessageLoader.ts` around lines 33 - 36, The hook useMessageLoader
sets setIsLoading(true) and immediately returns when accessToken is falsy, which
can leave the component permanently loading; update the early-accessToken branch
in useMessageLoader to clear the loading state and handle the unauthenticated
case (e.g., call setIsLoading(false) before return and optionally set an error
or empty messages via setHasError/setMessages) so the hook doesn't remain stuck
when accessToken is missing.
| const getClientMessages = async (chatId: string, accessToken: string) => { | ||
| try { | ||
| const response = await fetch(`/api/memories/get?roomId=${chatId}`); | ||
| const response = await fetch(`/api/memories/get?roomId=${chatId}`, { |
There was a problem hiding this comment.
Encode chatId before composing the query string.
chatId should be URL-encoded to avoid malformed requests for non-UUID IDs.
Proposed fix
- const response = await fetch(`/api/memories/get?roomId=${chatId}`, {
+ const response = await fetch(
+ `/api/memories/get?roomId=${encodeURIComponent(chatId)}`,
+ {
headers: {
Authorization: `Bearer ${accessToken}`,
},
- });
+ },
+ );📝 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.
| const response = await fetch(`/api/memories/get?roomId=${chatId}`, { | |
| const response = await fetch( | |
| `/api/memories/get?roomId=${encodeURIComponent(chatId)}`, | |
| { | |
| headers: { | |
| Authorization: `Bearer ${accessToken}`, | |
| }, | |
| }, | |
| ); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/supabase/getClientMessages.tsx` at line 3, The request URL uses chatId
directly which can break for non-UUID IDs; update the fetch call that constructs
`/api/memories/get?roomId=${chatId}` (in getClientMessages / the function
performing this fetch) to URL-encode the chatId using encodeURIComponent(chatId)
when composing the query string so the resulting request is always well-formed.
1 participant
Summary by CodeRabbit