-
Notifications
You must be signed in to change notification settings - Fork 14
feat: enhance memory retrieval API with authentication and room valid #1617
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,135 @@ | ||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | ||
| import { NextRequest } from "next/server"; | ||
| import { GET } from "../route"; | ||
|
|
||
| const mockValidateHeaders = vi.fn(); | ||
| const mockGetRoom = vi.fn(); | ||
| const mockQueryMemories = vi.fn(); | ||
|
|
||
| vi.mock("@/lib/chat/validateHeaders", () => ({ | ||
| validateHeaders: (...args: unknown[]) => mockValidateHeaders(...args), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/supabase/getRoom", () => ({ | ||
| default: (...args: unknown[]) => mockGetRoom(...args), | ||
| })); | ||
|
|
||
| vi.mock("@/lib/supabase/queryMemories", () => ({ | ||
| default: (...args: unknown[]) => mockQueryMemories(...args), | ||
| })); | ||
|
|
||
| describe("GET /api/memories/get", () => { | ||
| const roomId = "11111111-1111-1111-1111-111111111111"; | ||
| const accountId = "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa"; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| }); | ||
|
|
||
| it("returns 400 when roomId is missing", async () => { | ||
| const req = new NextRequest("https://example.com/api/memories/get"); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(400); | ||
| const body = await res.json(); | ||
| expect(body.error).toBe("Room ID is required"); | ||
| }); | ||
|
|
||
| it("returns 401 when not authenticated", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({}); | ||
|
|
||
| const req = new NextRequest( | ||
| `https://example.com/api/memories/get?roomId=${roomId}`, | ||
| ); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(401); | ||
| const body = await res.json(); | ||
| expect(body.error).toBe("Unauthorized"); | ||
| }); | ||
|
|
||
| it("forwards validateHeaders error Response", async () => { | ||
| const errorRes = new Response(JSON.stringify({ status: "error" }), { | ||
| status: 401, | ||
| }); | ||
| mockValidateHeaders.mockResolvedValueOnce(errorRes); | ||
|
|
||
| const req = new NextRequest( | ||
| `https://example.com/api/memories/get?roomId=${roomId}`, | ||
| { headers: { Authorization: "Bearer bad" } }, | ||
| ); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(401); | ||
| }); | ||
|
|
||
| it("returns 404 when room does not exist", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId }); | ||
| mockGetRoom.mockResolvedValueOnce(null); | ||
|
|
||
| const req = new NextRequest( | ||
| `https://example.com/api/memories/get?roomId=${roomId}`, | ||
| { headers: { Authorization: "Bearer token" } }, | ||
| ); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(404); | ||
| const body = await res.json(); | ||
| expect(body.error).toBe("Room not found"); | ||
| }); | ||
|
|
||
| it("returns 403 when room belongs to another account", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId }); | ||
| mockGetRoom.mockResolvedValueOnce({ | ||
| id: roomId, | ||
| account_id: "bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb", | ||
| }); | ||
|
|
||
| const req = new NextRequest( | ||
| `https://example.com/api/memories/get?roomId=${roomId}`, | ||
| { headers: { Authorization: "Bearer token" } }, | ||
| ); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(403); | ||
| const body = await res.json(); | ||
| expect(body.error).toBe("Forbidden"); | ||
| }); | ||
|
|
||
| it("returns 200 with memories when caller owns the room", async () => { | ||
| const memories = [{ id: "m1", room_id: roomId, content: {}, updated_at: "" }]; | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId }); | ||
| mockGetRoom.mockResolvedValueOnce({ | ||
| id: roomId, | ||
| account_id: accountId, | ||
| }); | ||
| mockQueryMemories.mockResolvedValueOnce({ | ||
| data: memories, | ||
| error: null, | ||
| }); | ||
|
|
||
| const req = new NextRequest( | ||
| `https://example.com/api/memories/get?roomId=${roomId}`, | ||
| { headers: { Authorization: "Bearer token" } }, | ||
| ); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(200); | ||
| const body = await res.json(); | ||
| expect(body.data).toEqual(memories); | ||
| expect(mockQueryMemories).toHaveBeenCalledWith(roomId, { ascending: true }); | ||
| }); | ||
|
|
||
| it("returns 400 when queryMemories fails", async () => { | ||
| mockValidateHeaders.mockResolvedValueOnce({ accountId }); | ||
| mockGetRoom.mockResolvedValueOnce({ | ||
| id: roomId, | ||
| account_id: accountId, | ||
| }); | ||
| mockQueryMemories.mockResolvedValueOnce({ | ||
| data: null, | ||
| error: { message: "db error" }, | ||
| }); | ||
|
|
||
| const req = new NextRequest( | ||
| `https://example.com/api/memories/get?roomId=${roomId}`, | ||
| { headers: { Authorization: "Bearer token" } }, | ||
| ); | ||
| const res = await GET(req); | ||
| expect(res.status).toBe(400); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import { NextRequest } from "next/server"; | ||
| import queryMemories from "@/lib/supabase/queryMemories"; | ||
| import { validateHeaders } from "@/lib/chat/validateHeaders"; | ||
| import getRoom from "@/lib/supabase/getRoom"; | ||
|
|
||
| export async function GET(req: NextRequest) { | ||
| const roomId = req.nextUrl.searchParams.get("roomId"); | ||
|
|
@@ -8,9 +10,25 @@ export async function GET(req: NextRequest) { | |
| return Response.json({ error: "Room ID is required" }, { status: 400 }); | ||
| } | ||
|
|
||
| const authResult = await validateHeaders(req); | ||
| if (authResult instanceof Response) { | ||
| return authResult; | ||
| } | ||
| if (!authResult.accountId) { | ||
| 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 }); | ||
| } | ||
|
Comment on lines
+18
to
+27
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unify error response shape across this route. Early exits return 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 |
||
|
|
||
| try { | ||
| const { data, error } = await queryMemories(roomId, { ascending: true }); | ||
|
|
||
| if (error) { | ||
| throw error; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,12 +7,14 @@ import getClientMessages from "@/lib/supabase/getClientMessages"; | |
| * @param roomId - The room ID to load messages from (undefined to skip loading) | ||
| * @param userId - The current user ID (messages won't load if user is not authenticated) | ||
| * @param setMessages - Callback function to set the loaded messages | ||
| * @param accessToken - Privy access token for /api/memories/get (required for auth) | ||
| * @returns Loading state and error information | ||
| */ | ||
| export function useMessageLoader( | ||
| roomId: string | undefined, | ||
| userId: string | undefined, | ||
| setMessages: (messages: UIMessage[]) => void | ||
| setMessages: (messages: UIMessage[]) => void, | ||
| accessToken: string | null, | ||
| ) { | ||
| const [isLoading, setIsLoading] = useState(!!roomId); | ||
| const [error, setError] = useState<Error | null>(null); | ||
|
|
@@ -28,12 +30,17 @@ export function useMessageLoader( | |
| return; | ||
| } | ||
|
|
||
| if (!accessToken) { | ||
| setIsLoading(true); | ||
| return; | ||
|
Comment on lines
+33
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This branch keeps Useful? React with 👍 / 👎. |
||
| } | ||
|
Comment on lines
+33
to
+36
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid permanent loading state when This branch sets loading to 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 |
||
|
|
||
| const loadMessages = async () => { | ||
| setIsLoading(true); | ||
| setError(null); | ||
|
|
||
| try { | ||
| const initialMessages = await getClientMessages(roomId); | ||
| const initialMessages = await getClientMessages(roomId, accessToken); | ||
| if (initialMessages.length > 0) { | ||
| setMessages(initialMessages as UIMessage[]); | ||
| } | ||
|
|
@@ -48,7 +55,7 @@ export function useMessageLoader( | |
| }; | ||
|
|
||
| loadMessages(); | ||
| }, [userId, roomId]); | ||
| }, [userId, roomId, accessToken]); | ||
|
|
||
| return { | ||
| isLoading, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,6 +1,10 @@ | ||||||||||||||||||||
| const getClientMessages = async (chatId: string) => { | ||||||||||||||||||||
| 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}`, { | ||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Encode
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
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||
| headers: { | ||||||||||||||||||||
| Authorization: `Bearer ${accessToken}`, | ||||||||||||||||||||
| }, | ||||||||||||||||||||
| }); | ||||||||||||||||||||
| const data = await response.json(); | ||||||||||||||||||||
|
|
||||||||||||||||||||
| const memories = data?.data || []; | ||||||||||||||||||||
|
|
||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The handler treats any falsy
getRoomresult as "Room not found", butgetRoomreturnsnullfor 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 👍 / 👎.