feat(api): migrate GET/POST /api/sessions/[sessionId]/chats from open-agents#552
Open
arpitgupta1214 wants to merge 9 commits into
Open
feat(api): migrate GET/POST /api/sessions/[sessionId]/chats from open-agents#552arpitgupta1214 wants to merge 9 commits into
arpitgupta1214 wants to merge 9 commits into
Conversation
Ports the chat list + create endpoints from open-agents'
apps/web/app/api/sessions/[sessionId]/chats/route.ts. GET returns
`{ chats, defaultModelId }` with per-account unread state derived from
`chat_reads`. POST accepts an optional `{ id }` for deterministic /
idempotent retries (409 on cross-session conflict).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR implements session chat management endpoints ( ChangesSession Chat API Endpoints
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
The discriminated-union shape (`{ok: true} | {ok: false}`) failed to
narrow under Next 16's typechecker on Vercel, breaking the production
build with "Property 'response' does not exist on type". Switch to the
codebase's existing `NextResponse | Context` convention (same as
`validateAuthContext`) so callers can early-return via
`instanceof NextResponse`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: This PR adds new API endpoints with authentication, authorization, database queries, and response formatting—non-trivial business logic that could affect data integrity and production reliability, so it warrants human review despite passing automated checks.
There was a problem hiding this comment.
5 issues found across 12 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/sessions/createSessionChatHandler.ts: the chat ID claim flow has a check-then-insert race, so concurrent claims can surface as 500s instead of the intended conflict response. - The current tests in
lib/sessions/__tests__/createSessionChatHandler.test.tsmay miss this behavior because one case validates status only, allowing an internal-error body to pass unnoticed. - The remaining findings are lower-risk maintainability/consistency items (100-line test-file limit and standardizing 500 payload text), so this is not far from mergeable once the race/error-path handling is tightened.
- Pay close attention to
lib/sessions/createSessionChatHandler.ts,lib/sessions/__tests__/createSessionChatHandler.test.ts,lib/sessions/__tests__/getSessionChatsHandler.test.ts- race-condition conflict handling and error/assertion consistency are the main risk areas.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/sessions/__tests__/getSessionChatsHandler.test.ts">
<violation number="1" location="lib/sessions/__tests__/getSessionChatsHandler.test.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
Test file exceeds the repository’s 100-line file-size limit.</violation>
</file>
<file name="lib/sessions/__tests__/createSessionChatHandler.test.ts">
<violation number="1" location="lib/sessions/__tests__/createSessionChatHandler.test.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
New test file exceeds the 100-line maximum required by the code-style rule.</violation>
<violation number="2" location="lib/sessions/__tests__/createSessionChatHandler.test.ts:183">
P2: Assert the 500 response body here too. Checking only the status leaves the test green even if the handler returns an internal error message like `Failed to create chat`.
(Based on your team's feedback about 500 error responses.) [FEEDBACK_USED]</violation>
</file>
<file name="lib/sessions/createSessionChatHandler.ts">
<violation number="1" location="lib/sessions/createSessionChatHandler.ts:63">
P1: The id-claim path is vulnerable to a check-then-insert race: insert failures after the pre-check are treated as 500, so concurrent chat-id claims can return 500 instead of the intended conflict behavior.</violation>
<violation number="2" location="lib/sessions/createSessionChatHandler.ts:71">
P3: Use the standard 500 payload `{ error: "Internal server error" }` instead of a custom failure string.
(Based on your team's feedback about using a fixed internal-error message for 500 responses.) [FEEDBACK_USED]</violation>
</file>
Architecture diagram
sequenceDiagram
participant Client as Frontend Client
participant API as API Route
participant Handler as Session Handler
participant Auth as validateAuthContext
participant Sessions as selectSessions (Supabase)
participant Chats as selectChats / insertChat (Supabase)
participant Reads as selectChatReads (Supabase)
Note over Client,Reads: GET /api/sessions/{sessionId}/chats
Client->>API: GET /api/sessions/{sessionId}/chats
API->>Handler: getSessionChatsHandler(req, sessionId)
Handler->>Auth: validateAuthContext(req)
alt Unauthorized (401)
Auth-->>Handler: NextResponse 401
Handler-->>API: 401 response
API-->>Client: 401 Unauthorized
else Authenticated
Auth-->>Handler: AuthContext { accountId }
Handler->>Sessions: selectSessions({ id: sessionId })
alt Session not found (404)
Sessions-->>Handler: []
Handler-->>API: 404 Session not found
API-->>Client: 404 { status: "error", error: "Session not found" }
else Wrong owner (403)
Sessions-->>Handler: session with different account_id
Handler-->>API: 403 Forbidden
API-->>Client: 403 { status: "error", error: "Forbidden" }
else Owned session found
Sessions-->>Handler: session (owned)
Handler->>Chats: selectChats({ sessionId })
alt Has chats
Chats-->>Handler: list of chat rows
Handler->>Reads: selectChatReads({ accountId, chatIds })
Reads-->>Handler: list of chat_read rows
Note over Handler: Sort by created_at<br/>Derive hasUnread from last_read_at<br/>Derive isStreaming from active_stream_id
else No chats
Chats-->>Handler: []
Note over Handler: Skip chat_reads lookup
end
Handler-->>API: 200 { chats, defaultModelId }
API-->>Client: 200 { chats: [...], defaultModelId: "openai/gpt-5.4" }
end
end
Note over Client,Reads: POST /api/sessions/{sessionId}/chats
Client->>API: POST /api/sessions/{sessionId}/chats { id?: string }
API->>Handler: createSessionChatHandler(req, sessionId)
Handler->>Auth: validateAuthContext(req)
alt Unauthorized (401)
Auth-->>Handler: NextResponse 401
Handler-->>API: 401 response
API-->>Client: 401 Unauthorized
else Authenticated
Auth-->>Handler: AuthContext { accountId }
Handler->>Sessions: selectSessions({ id: sessionId })
alt Session missing/wrong owner
Sessions-->>Handler: 404 or 403
Handler-->>API: error response
API-->>Client: 404 or 403
else Session owned
Sessions-->>Handler: session (owned)
Note over Handler: Parse & validate request body
alt Invalid body (400)
Handler-->>API: 400 { error: "Invalid chat id" }
API-->>Client: 400 { error: "Invalid chat id" }
else Valid body
alt Request has chat id
Handler->>Chats: selectChats({ id: requestedId })
alt Existing in same session
Chats-->>Handler: chat row (same session)
Handler-->>API: 200 { chat: existing }
API-->>Client: 200 { chat: { id, title, ... } }
else Existing in different session
Chats-->>Handler: chat row (different session)
Handler-->>API: 409 { error: "Chat ID conflict" }
API-->>Client: 409 { error: "Chat ID conflict" }
else Not found
Chats-->>Handler: []
Handler->>Chats: insertChat({ id: requestedId, sessionId, title: "New chat" })
alt Insert failed
Chats-->>Handler: null
Handler-->>API: 500 { error: "Failed to create chat" }
API-->>Client: 500 error
else Insert success
Chats-->>Handler: new chat row
Handler-->>API: 200 { chat: newChat }
API-->>Client: 200 { chat: { id, title: "New chat", ... } }
end
end
else No chat id
Handler->>Chats: insertChat({ id: generateUUID(), sessionId, title: "New chat" })
alt Insert failed
Chats-->>Handler: null
Handler-->>API: 500
API-->>Client: 500 error
else Success
Chats-->>Handler: new chat row
Handler-->>API: 200 { chat: newChat }
API-->>Client: 200 { chat: { id: "generated-uuid", ... } }
end
end
end
end
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| } | ||
|
|
||
| const chatRow = await insertChat({ |
There was a problem hiding this comment.
P1: The id-claim path is vulnerable to a check-then-insert race: insert failures after the pre-check are treated as 500, so concurrent chat-id claims can return 500 instead of the intended conflict behavior.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/createSessionChatHandler.ts, line 63:
<comment>The id-claim path is vulnerable to a check-then-insert race: insert failures after the pre-check are treated as 500, so concurrent chat-id claims can return 500 instead of the intended conflict behavior.</comment>
<file context>
@@ -0,0 +1,80 @@
+ }
+ }
+
+ const chatRow = await insertChat({
+ id: requestedChatId ?? generateUUID(),
+ session_id: sessionId,
</file context>
| @@ -0,0 +1,185 @@ | |||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |||
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
New test file exceeds the 100-line maximum required by the code-style rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/__tests__/createSessionChatHandler.test.ts, line 1:
<comment>New test file exceeds the 100-line maximum required by the code-style rule.</comment>
<file context>
@@ -0,0 +1,185 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { NextRequest, NextResponse } from "next/server";
+import { baseSessionRow } from "@/lib/sessions/__tests__/baseSessionRow";
</file context>
| vi.mocked(insertChat).mockResolvedValue(null); | ||
|
|
||
| const res = await createSessionChatHandler(makeReq({}), "sess_1"); | ||
| expect(res.status).toBe(500); |
There was a problem hiding this comment.
P2: Assert the 500 response body here too. Checking only the status leaves the test green even if the handler returns an internal error message like Failed to create chat.
(Based on your team's feedback about 500 error responses.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/__tests__/createSessionChatHandler.test.ts, line 183:
<comment>Assert the 500 response body here too. Checking only the status leaves the test green even if the handler returns an internal error message like `Failed to create chat`.
(Based on your team's feedback about 500 error responses.) </comment>
<file context>
@@ -0,0 +1,185 @@
+ vi.mocked(insertChat).mockResolvedValue(null);
+
+ const res = await createSessionChatHandler(makeReq({}), "sess_1");
+ expect(res.status).toBe(500);
+ });
+});
</file context>
|
|
||
| if (!chatRow) { | ||
| return NextResponse.json( | ||
| { status: "error", error: "Failed to create chat" }, |
There was a problem hiding this comment.
P3: Use the standard 500 payload { error: "Internal server error" } instead of a custom failure string.
(Based on your team's feedback about using a fixed internal-error message for 500 responses.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/createSessionChatHandler.ts, line 71:
<comment>Use the standard 500 payload `{ error: "Internal server error" }` instead of a custom failure string.
(Based on your team's feedback about using a fixed internal-error message for 500 responses.) </comment>
<file context>
@@ -0,0 +1,80 @@
+
+ if (!chatRow) {
+ return NextResponse.json(
+ { status: "error", error: "Failed to create chat" },
+ { status: 500, headers: getCorsHeaders() },
+ );
</file context>
…chats The route shell is a one-liner that just awaits params and delegates to the handler — covered by the handler tests. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Aligns the chats endpoints with the project's validator pattern
(see validateCreateSessionBody). Handlers now consume the validated
{ auth, session[, body] } payload directly instead of running their
own auth/ownership/body gates.
- requireOwnedSession → validateOwnedSessionRequest (GET path)
- validateCreateSessionChatBody → validateCreateSessionChatRequest
(POST path, now bundles auth + ownership + body)
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 10 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/sessions/__tests__/validateCreateSessionChatRequest.test.ts">
<violation number="1" location="lib/sessions/__tests__/validateCreateSessionChatRequest.test.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
Test file exceeds the 100-line limit required by the style rule.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
| @@ -0,0 +1,156 @@ | |||
| import { describe, it, expect, vi, beforeEach } from "vitest"; | |||
There was a problem hiding this comment.
P2: Custom agent: Enforce Clear Code Style and Maintainability Practices
Test file exceeds the 100-line limit required by the style rule.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At lib/sessions/__tests__/validateCreateSessionChatRequest.test.ts, line 1:
<comment>Test file exceeds the 100-line limit required by the style rule.</comment>
<file context>
@@ -0,0 +1,156 @@
+import { describe, it, expect, vi, beforeEach } from "vitest";
+import { NextRequest, NextResponse } from "next/server";
+import { baseSessionRow } from "@/lib/sessions/__tests__/baseSessionRow";
</file context>
Tip: Review your code locally with the cubic CLI to iterate faster.
Each endpoint now has a self-contained validator that does auth + ownership (+ body for POST) inline, matching the pattern set by validateCreateSessionBody. Drops the shared validateOwnedSessionRequest helper so the per-endpoint validators are the single point of gating. - GET: validateGetSessionChatsRequest (new) - POST: validateCreateSessionChatRequest (inlined) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pushes the camelCase mapping + chat_reads join into a single get<Descriptive> supabase function (mirrors the lib/supabase naming convention for joined reads). Handler now just calls getChatSummariesBySessionId and forwards the result. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/supabase/chats/__tests__/getChatSummariesBySessionId.test.ts">
<violation number="1" location="lib/supabase/chats/__tests__/getChatSummariesBySessionId.test.ts:1">
P2: Custom agent: **Enforce Clear Code Style and Maintainability Practices**
Test file exceeds the repository’s 100-line maintainability limit.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
- getChatSummariesBySessionId → getChatSummaries - Removes "mirrors open-agents", "cut over", "wire format parity" references from endpoint/handler/validator/db JSDoc Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ats/ - Move handlers, validators, and getChatSummaries into lib/sessions/chats/ - Drop the redundant lib/supabase/chats/getChatSummaries test (it's domain code, not a thin db reader) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 tasks
Collaborator
Author
Smoke test resultsRan against the preview deployment (api-git-feat-port-session-chats-route-recoup.vercel.app).
All paths green end-to-end. |
| export const DEFAULT_MODEL = "openai/gpt-5-mini"; | ||
| export const LIGHTWEIGHT_MODEL = "openai/gpt-4o-mini"; | ||
| /** Default model id surfaced to clients that have no explicit preference. */ | ||
| export const APP_DEFAULT_MODEL_ID = "openai/gpt-5.4"; |
Contributor
There was a problem hiding this comment.
KISS / DRY
- why do we need a new APP_DEFAULT_MODEL_ID when DEFAULT_MODEL already exists?
- Why not just update DEFAULT_MODEL to openai/gpt-5.4?
Contributor
Manual verification on the preview deploymentPreview URL: https://api-git-feat-port-session-chats-route-recoup.vercel.app Branch was already up to date with Results matrix
Highlights
🤖 Generated with Claude Code |
Contributor
Follow-up: GET reflects all subsequent POSTs (idempotency double-check)Caught a gap in the original matrix — I only ran curl -s -H "x-api-key: $KEY" "$PREVIEW/api/sessions/db2fa131-.../chats" | jqResult: {
"count": 3,
"defaultModelId": "openai/gpt-5.4",
"chats": [
{ "id": "90b67504-...", "createdAt": "...23.300012" },
{ "id": "80efb679-...", "createdAt": "...24.267985" },
{ "id": "00000000-0000-4000-8000-aaaaaaaaaaaa", "createdAt": "...41.023638" }
]
}Confirms:
All 13 + 1 follow-up checks now pass on the rebased preview. |
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
2 participants
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.
Summary
Ports the chat list + create endpoints from open-agents'
apps/web/app/api/sessions/[sessionId]/chats/route.tsto api so the frontend can cut over without code changes.GET /api/sessions/[sessionId]/chats→{ chats, defaultModelId }. Per-chathasUnreadderived fromchat_reads;isStreamingderived fromactive_stream_id.defaultModelIdreturnsAPP_DEFAULT_MODEL_ID(openai/gpt-5.4) until user-preferences are migrated.POST /api/sessions/[sessionId]/chats→{ chat }. Optional{ id }for deterministic claim; idempotent on same-session re-request; 409 on cross-session conflict.Auth is via
validateAuthContext(Privy Bearer /x-api-key). 404 when the session is missing, 403 when owned by a different account.Test plan
GET /api/sessions/{sessionId}/chatswith valid auth returns 200 +{ chats, defaultModelId }POST /api/sessions/{sessionId}/chatswithout body creates a new chatPOSTwith{"id":"existing-in-other-session"}returns 409POSTwith{"id":""}returns 400{ error: "Invalid chat id" }🤖 Generated with Claude Code
Summary by cubic
Migrated session chat list and create endpoints into the API. Adds
GET/POSTfor/api/sessions/[sessionId]/chatswith CORS (including preflight) and per-endpoint auth/ownership checks.New Features
GET /api/sessions/[sessionId]/chats: returns{ chats, defaultModelId }; chats sorted bycreatedAt;hasUnreadfromchat_reads;isStreamingfromactive_stream_id;defaultModelIdfromAPP_DEFAULT_MODEL_ID(openai/gpt-5.4).POST /api/sessions/[sessionId]/chats: creates a chat; optional{ id }for idempotent retries; 409 on cross-session conflict; 400 on invalid id.Refactors
getChatSummariesunderlib/sessions/chats/; addedselectChatReadsfor unread state.validateGetSessionChatsRequest,validateCreateSessionChatRequest).Written for commit 9c3edc1. Summary will update on new commits.
Summary by CodeRabbit