Feat/message persistence using convex #13
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. WalkthroughReplaces WorkOS gating with unconditional AuthKit + Convex integration; adds Convex schema, server functions, Convex-backed client provider, DB wrappers, a real-time Chat component/page, chat persistence (messages, titles, todos), sidebar/UI primitives, middleware simplification, env/README reorganization, and sandbox/terminal API adjustments. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User (browser)
participant UI as Client UI (Chat)
participant API as /api/chat
participant Auth as AuthKit (WorkOS)
participant Convex as Convex (DB)
participant Tools as AI Tools
U->>UI: submit message (chatId?, messages, todos, regenerate?)
UI->>API: POST /api/chat { chatId, messages, mode, todos, regenerate }
API->>Auth: getUserID(req)
Auth-->>API: userId
API->>Convex: handleInitialChatAndUserMessage (get/save chat)
Convex-->>API: { isNewChat }
API->>Tools: createTools() -> { tools, getTodoManager }
API->>API: stream assistant via JsonToSseTransformStream
alt isNewChat
API->>API: generateTitleFromUserMessageWithWriter(...) -> emit data-title
API->>Convex: updateChat(title/todos/finishReason)
end
API->>Convex: saveMessage (per-message persistence)
API-->>UI: SSE stream (assistant chunks + data-title events)
sequenceDiagram
autonumber
participant Req as Incoming Request
participant MW as middleware.ts
participant Auth as authkitMiddleware
participant App as Next.js App
Req->>MW: incoming request
MW->>Auth: authkitMiddleware(enabled=true, unauthenticatedPaths)
Auth-->>MW: auth context
MW->>App: forward request (with auth)
App-->>Req: response
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (5)
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/api/chat/route.ts (1)
1-154: Add missing visibility mapping for the “auth” surfaceThe error codes themselves are registered in
getMessageByErrorCode, but thevisibilityBySurfacemap only includes thedatabaseandchatsurfaces. Becauseunauthorized:authuses theauthsurface, its visibility will beundefinedat runtime. Please add an entry forauthinvisibilityBySurface(e.g.,"auth": "response") so thattoResponse()always produces a consistent payload.• File: lib/errors.ts
Update thevisibilityBySurfaceobject to include:export const visibilityBySurface: Record<Surface, ErrorVisibility> = { database: "log", chat: "response", + auth: "response", };
🧹 Nitpick comments (30)
lib/ai/tools/run-terminal-cmd.ts (1)
146-152: Background runs still capped at 6 minutes; let them run indefinitelySandbox background jobs inherit timeoutMs from commonOptions, so they can be killed at 6 minutes despite being explicitly backgrounded. Adjust to avoid unintended termination.
Apply:
- const commonOptions = { - timeoutMs: MAX_COMMAND_EXECUTION_TIME, + const commonOptions = { + timeoutMs: is_background ? undefined : MAX_COMMAND_EXECUTION_TIME, user: "root" as const, cwd: "/home/user", onStdout: handler.stdout, onStderr: handler.stderr, };Optional: also consider a separate, longer cap for foreground long-running commands when interactive output is expected.
app/callback/route.ts (1)
5-5: Make post-auth redirect explicit and confirm env setup to avoid 500s in devTo avoid ambiguity and future regressions, pass returnPathname explicitly. Also, since gating was removed, ensure all required WorkOS/AuthKit env vars are present in local/staging; otherwise handleAuth() will fail at runtime.
-export const GET = handleAuth(); +export const GET = handleAuth({ returnPathname: "/" });Verification checklist:
- WORKOS_CLIENT_ID, WORKOS_API_KEY (or config required by your AuthKit version)
- NEXTAUTH_URL / app URL config as required by AuthKit
- Redirect URIs registered in the WorkOS dashboard match /callback
app/signup/route.ts (1)
4-7: Add defensive error handling around getSignUpUrl() to return a clean 5xx on misconfigCurrent flow is fine; a small guard improves DX when env/config is missing.
-export async function GET() { - const authorizationUrl = await getSignUpUrl(); - return redirect(authorizationUrl); -} +export async function GET() { + try { + const authorizationUrl = await getSignUpUrl(); + return redirect(authorizationUrl); + } catch (err) { + console.error("Signup redirect failed:", err); + return new Response("Unable to start sign-up flow. Please contact support.", { status: 500 }); + } +}app/login/route.ts (1)
4-7: Mirror defensive handling for sign-in flowSame rationale as sign-up: surface a clean error when configuration is incomplete.
-export async function GET() { - const authorizationUrl = await getSignInUrl(); - return redirect(authorizationUrl); -} +export async function GET() { + try { + const authorizationUrl = await getSignInUrl(); + return redirect(authorizationUrl); + } catch (err) { + console.error("Login redirect failed:", err); + return new Response("Unable to start sign-in flow. Please contact support.", { status: 500 }); + } +}lib/auth/get-user-id.ts (2)
1-3: Mark this helper as server-only to prevent accidental client bundlingThis module should never end up in the client bundle. Add the server-only directive at the top.
+import "server-only"; import type { NextRequest } from "next/server"; import { ChatSDKError } from "@/lib/errors";
17-22: Include error cause for observability; keep response sanitized by ChatSDKErrorSurface the underlying failure reason in the ChatSDKError’s
cause(your error responder already controls visibility by surface).- if (!session?.user?.id) { - throw new ChatSDKError("unauthorized:auth"); - } + if (!session?.user?.id) { + throw new ChatSDKError("unauthorized:auth", "Missing session.user.id"); + } return session.user.id; } catch (error) { if (error instanceof ChatSDKError) { throw error; } - console.error("Failed to get user session:", error); - throw new ChatSDKError("unauthorized:auth"); + console.error("Failed to get user session", { cause: error }); + const cause = error instanceof Error ? error.message : String(error); + throw new ChatSDKError("unauthorized:auth", cause); }app/api/chat/route.ts (2)
27-41: Authenticate before parsing the body to reduce unnecessary work on unauthenticated/rate-limited requestsMinor efficiency/readability win: extract the user id and check rate limit before
req.json(). This avoids JSON parsing and allocations for requests we’ll reject anyway.export async function POST(req: NextRequest) { try { - const { - messages, - mode, - todos, - }: { messages: UIMessage[]; mode: ChatMode; todos?: Todo[] } = - await req.json(); - - const userID = await getUserID(req); + const userID = await getUserID(req); const userLocation = geolocation(req); // Check rate limit for the user await checkRateLimit(userID); + const { + messages, + mode, + todos, + }: { messages: UIMessage[]; mode: ChatMode; todos?: Todo[] } = + await req.json();
52-58: Optional: guard PostHog failures to keep the stream resilient
posthog.captureis async and can occasionally throw; you’re already checking client presence, but a light try/catch around capture calls can further isolate analytics from chat execution.- posthog.capture({ - distinctId: userID, - event: "hackerai-" + mode, - }); + try { + posthog.capture({ distinctId: userID, event: "hackerai-" + mode }); + } catch {}And similarly inside
onChunk.Also applies to: 106-114
convex/auth.config.ts (1)
3-20: DRY up repeated literals for maintainabilityFactor repeated URL parts into constants to reduce future config drift.
-const authConfig = { +const jwksUri = `https://api.workos.com/sso/jwks/${clientId}`; +const baseIssuer = "https://api.workos.com/"; +const userMgmtIssuer = `https://api.workos.com/user_management/${clientId}`; + +const authConfig = { providers: [ { type: "customJwt", - issuer: `https://api.workos.com/`, + issuer: baseIssuer, - algorithm: "RS256", - applicationID: clientId, - jwks: `https://api.workos.com/sso/jwks/${clientId}`, + applicationID: clientId, + jwksUri, + algorithms: ["RS256"], }, { type: "customJwt", - issuer: `https://api.workos.com/user_management/${clientId}`, - algorithm: "RS256", - jwks: `https://api.workos.com/sso/jwks/${clientId}`, - applicationID: clientId, + issuer: userMgmtIssuer, + applicationID: clientId, + jwksUri, + algorithms: ["RS256"], }, ], };middleware.ts (1)
18-25: Matcher is comprehensive; consider allowing a healthcheck path without authIf you expose a health endpoint (e.g.,
/healthor/_health), add it tounauthenticatedPathsto keep infra probes simple..env.local.example (3)
9-13: Satisfy dotenv-linter ordering and quoting recommendations for WorkOS keysThe linter warns that NEXT_PUBLIC_WORKOS_REDIRECT_URI should precede WORKOS_* keys and that values needing quotes should be quoted. Reorder and quote the URI.
Run dotenv-linter locally to confirm the warnings are resolved.
-# WorkOS Authentication (Required for user management and conversation persistence) -WORKOS_API_KEY=sk_example_123456789 -WORKOS_CLIENT_ID=client_123456789 -# generate a secure password (min 32 characters) -WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret" -NEXT_PUBLIC_WORKOS_REDIRECT_URI=http://localhost:3000/callback +# WorkOS Authentication (Required for user management and conversation persistence) +# Redirect URI for WorkOS OAuth callbacks +NEXT_PUBLIC_WORKOS_REDIRECT_URI="http://localhost:3000/callback" +WORKOS_API_KEY=sk_example_123456789 +WORKOS_CLIENT_ID=client_123456789 +# generate a secure password (min 32 characters) +WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret"
6-7: Clarify Convex env expectationsBoth CONVEX_DEPLOYMENT and NEXT_PUBLIC_CONVEX_URL are currently blank. Consider adding example values or comments pointing to convex CLI output to reduce setup friction.
-CONVEX_DEPLOYMENT= -NEXT_PUBLIC_CONVEX_URL= +# e.g. from `npx convex dev`/`npx convex deploy` +CONVEX_DEPLOYMENT= +# Example: "https://<your-deployment>.convex.cloud" +NEXT_PUBLIC_CONVEX_URL=
19-20: Validate OpenRouter key presence at runtimeIf OPENROUTER_API_KEY is missing, the app will likely fail chat requests. Consider documenting a guard or adding a startup check.
I can add a small runtime assert in server route handlers to emit a clear error if the key is absent.
app/components/ChatInterface.tsx (6)
41-61: Avoid side-effects inside prepareSendMessagesRequest or document the intentCalling setMessages within prepareSendMessagesRequest mutates UI state during request assembly, which can be surprising and harder to trace. Consider normalizing just-in-time before sending (in handleSubmit/regenerate) or clearly document the side-effect.
-prepareSendMessagesRequest: ({ messages, body }) => { - // Normalize messages on the frontend before sending to API - const { messages: normalizedMessages, hasChanges } = - normalizeMessages(messages); - - // Only update messages if normalization made changes - if (hasChanges) { - setMessages(normalizedMessages); - } - - return { - body: { - messages: normalizedMessages, - ...body, - }, - }; -}, +prepareSendMessagesRequest: ({ messages, body }) => { + // Normalize messages before sending to API (read-only within this callback) + const { messages: normalizedMessages } = normalizeMessages(messages); + return { body: { messages: normalizedMessages, ...body } }; +},Alternatively, move normalization into handleSubmit and handleRegenerate before invoking sendMessage/regenerate.
68-75: Honor the merge flag for todo_write tool callsTool input includes a merge boolean, but the code always merges. Respect the flag to allow full replacement when requested by the tool.
- if (toolCall.toolName === "todo_write" && toolCall.input) { - const todoInput = toolCall.input as { merge: boolean; todos: Todo[] }; - if (todoInput.todos) { - mergeTodos(todoInput.todos); - } - } + if (toolCall.toolName === "todo_write" && toolCall.input) { + const { merge, todos: incoming } = toolCall.input as { + merge?: boolean; + todos?: Todo[]; + }; + if (incoming && incoming.length > 0) { + if (merge !== false) { + // default to merge for backward compatibility + mergeTodos(incoming); + } else { + // replace mode: clear then merge + mergeTodos(incoming.map((t) => ({ ...t }))); + } + } + }
76-84: Surface unknown errors to users, not just ChatSDKErrorCurrently only non-rate-limit ChatSDKError paths show a toast; other error types are silently ignored in the UI. Add a generic fallback toast to improve debuggability.
onError: (error) => { if (error instanceof ChatSDKError) { // For rate limit errors, let them flow to the Messages component // For other errors, show toast if (error.type !== "rate_limit") { toast.error(error.message); } - } + } else { + toast.error("Something went wrong. Please try again."); + } },
92-116: Consider clearing input after a successful send (minor UX nit)Clearing immediately can drop the user's text if the request fails early. Clearing after the first token or on success improves recoverability.
- sendMessage( + const promise = sendMessage( { text: input }, { body: { mode, todos, }, }, ); - clearInput(); + promise.finally(() => { + // clear regardless once the request is accepted + clearInput(); + });
86-88: Type-safety for refsCasting refs to RefObject is safe here, but if useMessageScroll already returns MutableRefObject<HTMLDivElement | null>, consider updating Messages prop types to accept that, avoiding casts.
I can adjust Messages props to accept MutableRefObject<HTMLDivElement | null> for stricter typing.
221-224: Comment mismatch: Footer visibilityThe comment says “only show when user is not logged in,” but ChatInterface renders Footer based solely on empty-state; the auth gating actually lives inside Footer. Clarify the comment to avoid confusion.
- {/* Footer - only show when user is not logged in */} + {/* Footer renders only when unauthenticated (internally gated) and in empty state */}app/components/Footer.tsx (1)
17-35: Harden target=_blank links with noopenerAdd rel="noopener noreferrer" to prevent the new page from controlling window.opener. Small but good practice even for first-party links.
- rel="noreferrer" + rel="noopener noreferrer"Apply to both Terms and Privacy links.
app/components/Header.tsx (1)
11-21: Prefer next/navigation router over window.location for SPA navigationUsing router.push keeps client-side routing fast and preserves state. Also drop unnecessary async on sign-out handler.
-import { useAuth } from "@workos-inc/authkit-nextjs/components"; +import { useAuth } from "@workos-inc/authkit-nextjs/components"; +import { useRouter } from "next/navigation"; @@ - const { user, loading } = useAuth(); + const { user, loading } = useAuth(); + const router = useRouter(); @@ - const handleSignIn = () => { - window.location.href = "/login"; - }; + const handleSignIn = () => router.push("/login"); @@ - const handleSignUp = () => { - window.location.href = "/signup"; - }; + const handleSignUp = () => router.push("/signup"); @@ - const handleSignOut = async () => { - window.location.href = "/logout"; - }; + const handleSignOut = () => router.push("/logout");README.md (4)
15-33: Strengthen Step 2 guidance with explicit env variablesSince conversation persistence depends on Convex and WorkOS, add a note to fill CONVEX_DEPLOYMENT, NEXT_PUBLIC_CONVEX_URL, NEXT_PUBLIC_WORKOS_REDIRECT_URI in .env.local after running npx convex dev.
HackerAI uses **Convex** for real-time database and **WorkOS** for authentication to provide persistent conversations and user management. +After initializing Convex, update the following in `.env.local`: +- CONVEX_DEPLOYMENT +- NEXT_PUBLIC_CONVEX_URL +- NEXT_PUBLIC_WORKOS_REDIRECT_URI (e.g., http://localhost:3000/callback)
60-67: Fix table rendering by ensuring blank lines before/after headings and tablesSome Markdown renderers require blank lines around tables for proper formatting; this also addresses the grammar tool noise.
-## 🔑 Required Services - -| Service | Purpose | Get Started | +## 🔑 Required Services + +| Service | Purpose | Get Started | | -------------- | ----------------------------------------- | --------------------------------------- | | **Convex** | Real-time database & conversation storage | [convex.dev](https://convex.dev/) | | **WorkOS** | User authentication & session management | [workos.com](https://workos.com/) | | **OpenRouter** | LLM access (Claude, GPT, etc.) | [openrouter.ai](https://openrouter.ai/) | +
68-82: Add spacing around Optional Enhancements section and E2B table for consistent renderingMinor Markdown spacing fixes to satisfy linters and ensure consistent rendering.
-## 🔧 Optional Enhancements +## 🔧 Optional Enhancements @@ -### Sandbox Mode - -Execute terminal commands in isolated containers instead of your local machine: +### Sandbox Mode + +Execute terminal commands in isolated containers instead of your local machine: @@ -| Service | Purpose | Get API Key | -| ------- | ------------------------- | --------------------------- | -| **E2B** | Secure isolated execution | [e2b.dev](https://e2b.dev/) | +| Service | Purpose | Get API Key | +| ------- | ------------------------- | --------------------------- | +| **E2B** | Secure isolated execution | [e2b.dev](https://e2b.dev/) |
83-99: Quote env examples to prevent accidental shell expansion and whitespace issuesQuoting URLs/tokens in the examples reduces copy-paste errors.
-EXA_API_KEY=your_exa_api_key_here +EXA_API_KEY="your_exa_api_key_here" @@ -UPSTASH_REDIS_REST_URL="https://your-redis-url.upstash.io" -UPSTASH_REDIS_REST_TOKEN="your-redis-token" -RATE_LIMIT_REQUESTS=10 +UPSTASH_REDIS_REST_URL="https://your-redis-url.upstash.io" +UPSTASH_REDIS_REST_TOKEN="your-redis-token" +RATE_LIMIT_REQUESTS="10"app/page.tsx (3)
10-13: Preserve the typed message and use Next.js router before redirectingCapture the guest’s input so you can auto-send it post-login (aligns with “message persistence”), and prefer router.push for SPA navigation.
Apply within this range:
-const UnauthenticatedContent = () => { - const { clearInput } = useGlobalState(); +const UnauthenticatedContent = () => { + const { input, clearInput } = useGlobalState(); + const router = useRouter();Add this import (outside this range):
import { useRouter } from "next/navigation";
14-19: Best-effort handoff of guest input to login flowStore the pending message to sessionStorage and include a “next” param. Clear input locally to avoid duplication.
const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); - // Clear the input and redirect to login - clearInput(); - window.location.href = "/login"; + // Preserve typed message for post-login handoff, then redirect + const value = input.trim(); + if (value) { + try { + sessionStorage.setItem("postLoginMessage", value); + } catch { + // ignore storage failures + } + } + clearInput(); + router.push("/login?next=/"); };Follow-up (optional): In ChatInterface, check sessionStorage for postLoginMessage on mount and auto-send once, then remove it. I can draft that if useful.
64-75: Consider an auth “loading” fallback to avoid UI flickerDepending on Convex/AuthKit init timing, users might briefly see the wrong branch during hydration. Consider a minimal loading/skeleton state while auth resolution occurs (e.g., derive “isLoading” from your auth hook and gate both branches behind it).
If you want, I can wire a tiny fallback component that uses your auth hook to suppress both branches until the status is known.
app/layout.tsx (2)
21-36: Metadata updates LGTMApplication name, title template, description, and keywords look coherent for SEO and sharing. Consider adding openGraph and twitter metadata later for richer previews.
58-61: Accessibility: allow pinch-zoomAvoid disabling zoom; it harms accessibility. Recommend removing maximum-scale and user-scalable=no.
- <meta - name="viewport" - content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no" - /> + <meta + name="viewport" + content="width=device-width, initial-scale=1" + />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (6)
convex/_generated/api.d.tsis excluded by!**/_generated/**convex/_generated/api.jsis excluded by!**/_generated/**convex/_generated/dataModel.d.tsis excluded by!**/_generated/**convex/_generated/server.d.tsis excluded by!**/_generated/**convex/_generated/server.jsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
.env.local.example(1 hunks)README.md(2 hunks)app/api/chat/route.ts(2 hunks)app/callback/route.ts(1 hunks)app/components/ChatInterface.tsx(1 hunks)app/components/Footer.tsx(1 hunks)app/components/Header.tsx(3 hunks)app/hooks/useAppAuth.ts(0 hunks)app/layout.tsx(3 hunks)app/login/route.ts(1 hunks)app/logout/route.ts(0 hunks)app/page.tsx(1 hunks)app/signup/route.ts(1 hunks)components/ConvexClientProvider.tsx(1 hunks)convex/auth.config.ts(1 hunks)lib/ai/tools/run-terminal-cmd.ts(1 hunks)lib/auth/client.ts(0 hunks)lib/auth/get-user-id.ts(1 hunks)lib/auth/server.ts(0 hunks)middleware.ts(1 hunks)package.json(2 hunks)
💤 Files with no reviewable changes (4)
- app/logout/route.ts
- app/hooks/useAppAuth.ts
- lib/auth/server.ts
- lib/auth/client.ts
🧰 Additional context used
🧬 Code graph analysis (7)
lib/auth/get-user-id.ts (1)
lib/errors.ts (1)
ChatSDKError(36-74)
app/components/ChatInterface.tsx (8)
app/contexts/GlobalState.tsx (1)
useGlobalState(137-143)lib/utils.ts (1)
fetchWithErrorHandlers(9-29)lib/utils/message-processor.ts (1)
normalizeMessages(50-130)types/chat.ts (1)
Todo(36-40)lib/errors.ts (1)
ChatSDKError(36-74)app/hooks/useMessageScroll.ts (1)
useMessageScroll(4-22)app/components/Messages.tsx (1)
Messages(22-181)app/components/ScrollToBottomButton.tsx (1)
ScrollToBottomButton(11-46)
app/callback/route.ts (3)
app/login/route.ts (1)
GET(4-7)app/signup/route.ts (1)
GET(4-7)app/logout/route.ts (1)
GET(3-5)
app/signup/route.ts (2)
app/callback/route.ts (1)
GET(5-5)app/login/route.ts (1)
GET(4-7)
app/layout.tsx (1)
components/ConvexClientProvider.tsx (1)
ConvexClientProvider(14-22)
app/page.tsx (3)
app/contexts/GlobalState.tsx (1)
useGlobalState(137-143)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/ChatInterface.tsx (1)
ChatInterface(20-258)
app/login/route.ts (1)
app/signup/route.ts (1)
GET(4-7)
🪛 LanguageTool
README.md
[grammar] ~60-~60: There might be a mistake here.
Context: ...ourney! 🎯 --- ## 🔑 Required Services | Service | Purpose ...
(QB_NEW_EN)
[grammar] ~68-~68: There might be a mistake here.
Context: ...uter.ai/) | ## 🔧 Optional Enhancements ### Sandbox Mode Execute terminal commands i...
(QB_NEW_EN)
[grammar] ~74-~74: There might be a mistake here.
Context: ... | Get API Key | | ------- | ------------------------- | ...
(QB_NEW_EN)
[grammar] ~75-~75: There might be a mistake here.
Context: ...-------- | --------------------------- | | E2B | Secure isolated execution | ...
(QB_NEW_EN)
🪛 dotenv-linter (3.3.0)
.env.local.example
[warning] 12-12: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 12-12: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 13-13: [UnorderedKey] The NEXT_PUBLIC_WORKOS_REDIRECT_URI key should go before the WORKOS_API_KEY key
(UnorderedKey)
🔇 Additional comments (13)
package.json (1)
16-16: Peer dependencies verified – no duplicate React trees detected
- npm ls confirms a single React 19.1.1 instance across Next 15.5.0, @workos-inc/authkit-nextjs 2.5.0, @convex-dev/workos 0.0.1, and convex 1.26.1; no multiple React copies are pulled in.
- All three direct packages declare peerDependencies compatible with React ^19.0.0 and Next ^15.x (convex@1.26.1, @convex-dev/workos@0.0.1, @workos-inc/authkit-nextjs@2.5.0).
Minor nit (optional): react is declared with a caret (
^19.1.1), whereas react-dom is pinned exactly (19.1.1). You may wish to align them (e.g.^19.1.1) so that both receive patch updates consistently.lib/auth/get-user-id.ts (1)
14-16: Confirmed:authkit(req)returns{ session }withsession.user.idThe
@workos-inc/authkit-nextjslibrary’s composable middleware API for Next.js App Router route handlers is indeed the recommended way to read a session from aNextRequest. When you call:import { authkit } from "@workos-inc/authkit-nextjs"; export async function GET(req: NextRequest) { const { session } = await authkit(req); const userId = session.user.id; // … }you get back an object containing:
session.accessToken: stringsession.refreshToken: stringsession.user: { object: "user"; id: string; email: string; emailVerified: boolean; firstName?: string; lastName?: string; profilePictureUrl?: string; createdAt: string; updatedAt: string; }session.impersonator?(when impersonated)- plus
headersandauthorizationUrlif you need them (github.com, workos.com)Because this matches the official API shape in 2025, no changes are needed here.
app/api/chat/route.ts (2)
15-15: Import path update to new auth helper looks goodSwitching to
@/lib/auth/get-user-idaligns this route with the new centralized auth helper.
140-152: Error handling is consistent with ChatSDKError contractCatching
ChatSDKErrorand delegating totoResponse()keeps responses consistent; unexpected errors are wrapped asoffline:chatwith a safe message. LGTM.components/ConvexClientProvider.tsx (1)
7-10: AuthKitProvider client setup confirmed – no props requiredThe current usage of
<AuthKitProvider>without any props in your App Router client components matches the official 2025 AuthKit Next.js documentation. All configuration (project ID, base URL, API keys) is handled server-side via environment variables or server handlers, so noprojectIdor other props should be passed into the client provider.No changes are needed here.
middleware.ts (1)
3-16: Middleware configuration looks consistent with always-on authThe unauthenticated path list covers the expected public routes. LGTM.
app/components/ChatInterface.tsx (1)
20-31: Overall integration looks solidClean separation of concerns, good use of useChat hooks, and thoughtful UI state wiring (title updates, sidebar auto-open reset, scroll helpers). Nice work.
app/components/Footer.tsx (1)
7-11: Auth-driven rendering is correctUsing useAuth and hiding the footer when loading or authenticated matches the product intent. LGTM.
app/components/Header.tsx (1)
35-67: Auth control rendering logic looks goodClear, minimal gating on loading; correct branching on user for desktop UI. Same pattern mirrored for mobile below. LGTM.
app/page.tsx (2)
3-4: Good switch to Convex Auth wrappersUsing Authenticated/Unauthenticated from convex/react makes the split between signed-in and guest experiences clear and keeps this page lean. Nice.
43-51: Guest ChatInput configuration looks safePassing status="ready" with a no-op onStop means the stop controls won’t render and Enter-to-submit cleanly triggers the login redirect. Looks good.
app/layout.tsx (2)
9-9: Right place for ConvexClientProviderWrapping the entire app body in ConvexClientProvider centralizes auth/state wiring and keeps pages/components clean. Solid.
67-67: ConvexClientProvider is correctly configuredConfirmed that
components/ConvexClientProvider.tsxbegins with a"use client"directive and only instantiates oneAuthKitProviderand oneConvexProviderWithAuth—no duplicate providers detected. This satisfies the requirement for a client component and proper provider ordering.
|
|
||
| const MAX_COMMAND_EXECUTION_TIME = 6 * 60 * 1000; // 6 minutes | ||
| const STREAM_TIMEOUT_SECONDS = 5; | ||
| const STREAM_TIMEOUT_SECONDS = 60; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix: local onTimeout misses cleanup (leak), and background jobs shouldn’t stream-timeout
- Local path’s onTimeout doesn’t call handler.cleanup(), leaving timers/listeners alive. This is a real leak in long sessions.
- Both local and sandbox paths apply a stream idle-timeout to background jobs; background tasks are expected to run without interactive output, so timing them out at 60s is misleading.
Apply the following diffs:
@@
-const STREAM_TIMEOUT_SECONDS = 60;
+const STREAM_TIMEOUT_SECONDS = 60;
@@
- const handler = createTerminalHandler(
+ const handler = createTerminalHandler(
(output, isStderr) =>
isStderr ? onStderr(output) : onStdout(output),
{
- timeoutSeconds: STREAM_TIMEOUT_SECONDS,
+ // Disable idle timeout for background commands
+ timeoutSeconds: is_background ? undefined : STREAM_TIMEOUT_SECONDS,
onTimeout: () => {
if (!resolved) {
resolved = true;
+ // Surface a consistent timeout message to the user
+ onStdout(TIMEOUT_MESSAGE(STREAM_TIMEOUT_SECONDS));
+ // Ensure we release timers/listeners
+ handler.cleanup();
const result = handler.getResult();
resolve({ result: { ...result, exitCode: null } });
}
},
},
);
@@
- const handler = createTerminalHandler(
+ const handler = createTerminalHandler(
(output) => createTerminalWriter(output),
{
- timeoutSeconds: STREAM_TIMEOUT_SECONDS,
+ // Disable idle timeout for background commands
+ timeoutSeconds: is_background ? undefined : STREAM_TIMEOUT_SECONDS,
onTimeout: () => {
if (!resolved) {
resolved = true;
// Send timeout message through streaming interface
createTerminalWriter(
TIMEOUT_MESSAGE(STREAM_TIMEOUT_SECONDS),
);
handler.cleanup();
const result = handler.getResult();
resolve({ result: { ...result, exitCode: null } });
}
},
},
);If createTerminalHandler doesn’t accept undefined for timeoutSeconds, use a very large value (e.g., Number.MAX_SAFE_INTEGER) or add a dedicated “disableTimeout” option in that utility.
Also applies to: 66-79, 127-144
🤖 Prompt for AI Agents
In lib/ai/tools/run-terminal-cmd.ts around line 14 (and similarly for the blocks
at 66-79 and 127-144), the STREAM_TIMEOUT_SECONDS constant and its usage cause
two problems: the local onTimeout handler does not call handler.cleanup()
(leaking timers/listeners), and both local and sandbox paths apply the 60s
stream idle-timeout to background jobs which should not be timed out. Fix by
updating the local onTimeout implementation to always call handler.cleanup()
before exiting the timeout handler, and change the timeout passed for background
jobs to disable the stream timeout (either by passing undefined if
createTerminalHandler supports it, or passing a very large value like
Number.MAX_SAFE_INTEGER, or extending createTerminalHandler with a
disableTimeout flag); ensure the change is applied consistently to the
occurrences at lines ~66-79 and ~127-144 so background tasks are not subject to
the 60s idle timer.
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 (1)
app/api/chat/route.ts (1)
36-49: Validate and sanitize request body (schema) to prevent 500s from malformed input
await req.json()is unchecked; invalid shapes will break downstream logic. Add runtime validation (e.g., zod) and return a friendly error.+import { z } from "zod"; + +const BodySchema = z.object({ + messages: z.array(z.any()), // refine if you have a UIMessage runtime schema + mode: z.union([z.literal("ask"), z.literal("agent")]), + chatId: z.string().min(1), + todos: z + .array( + z.object({ + id: z.string(), + content: z.string(), + status: z.union([ + z.literal("pending"), + z.literal("in_progress"), + z.literal("completed"), + z.literal("cancelled"), + ]), + }), + ) + .optional(), + regenerate: z.boolean().optional(), +}); export async function POST(req: NextRequest) { try { - const { - messages, - mode, - todos, - chatId, - regenerate, - }: { - messages: UIMessage[]; - mode: ChatMode; - chatId: string; - todos?: Todo[]; - regenerate?: boolean; - } = await req.json(); + const parsed = BodySchema.safeParse(await req.json()); + if (!parsed.success) { + throw new ChatSDKError( + "bad_request:input", + parsed.error.errors.map((e) => e.message).join("; "), + ); + } + const { messages, mode, todos, chatId, regenerate } = parsed.data;
♻️ Duplicate comments (1)
.env.local.example (1)
12-12: Fix invalid dotenv syntax for WORKOS_COOKIE_PASSWORDInline comments after the equals sign are parsed into the value; also quotes are required for secrets with spaces/special chars. Move the comment to its own line and quote the value.
Apply this diff:
-WORKOS_COOKIE_PASSWORD= # generate a secure password (min 32 characters) +# generate a secure password (min 32 characters) +WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret"
🧹 Nitpick comments (25)
lib/ai/tools/index.ts (1)
70-72: New getter looks good; consider typing the factory’s return and clarifying mutability.Exposing
getTodoManageris consistent withgetSandbox. To make downstream usage safer and catch regressions at compile time, explicitly type the factory’s return and document that callers receive a live, stateful manager (not a snapshot).Add an explicit return type:
// Outside selected lines (type helper) export interface ToolsFactoryResult { tools: typeof allTools; getSandbox: () => Sandbox | null; getTodoManager: () => TodoManager; } // Signature: export const createTools = (/* args */): ToolsFactoryResult => { /* ... */ }If you want to avoid external mutation of internal fields in the future, you can later expose a narrower interface for
TodoManagerviaPick<>or an adapter without changing the public getter name.types/chat.ts (1)
1-1: Use a type-only import to avoid unnecessary runtime dependency.
UIMessageis a type; importing it as a type-only keeps the JS bundle clean and avoids potential runtime import issues if it isn’t a value export.-import { UIMessage } from "ai"; +import type { UIMessage } from "ai";lib/utils.ts (2)
4-6: Prefer type-only imports; avoid over-specific generics.
- Mark
ChatMessage,UIMessagePart, andDocas type-only imports.- Unless you rely on
UIMessagePartgenerics, the plainUIMessagePart[]is more future-proof and avoids mismatches if the upstream generic arity changes.-import { ChatMessage } from "@/types/chat"; -import { UIMessagePart } from "ai"; -import { Doc } from "@/convex/_generated/dataModel"; +import type { ChatMessage } from "@/types/chat"; +import type { UIMessagePart } from "ai"; +import type { Doc } from "@/convex/_generated/dataModel";
34-42: Refactor convertToUIMessages: drop redundant sort, validate role, and default partsThe
messagesarray you pass in is already ordered ascending by the Convex query inconvex/messages.ts(it uses.order("asc")on line 86), so the local.sort(...)is redundant. However, it’s still valuable to:
- Defensively clamp
roleto"user" | "assistant" | "system"- Provide a safe default for
partsto avoid runtime errorsFile:
lib/utils.ts, functionconvertToUIMessagesSuggested diff:
export function convertToUIMessages( messages: Doc<"messages">[], ): ChatMessage[] { - return [...messages] - .sort((a, b) => a._creationTime - b._creationTime) - .map((message) => { + return messages.map((message) => { const role = (["user", "assistant", "system"].includes(message.role) ? message.role : "user") as "user" | "assistant" | "system"; return { id: String(message._id), role, - parts: (message.parts ?? []) as UIMessagePart[], + parts: (message.parts ?? []) as UIMessagePart<any, any>[], }; - }); + }); }components/ui/loading.tsx (2)
7-14: Avoid dynamic Tailwind classes; add basic a11y.
size-${size}will be purged in production unless safelisted. Prefer inline sizing or a fixed class map, and addrole="status"/aria-labelfor screen readers.-export default function Loading({ size = 12 }: LoadingProps): JSX.Element { - const sizeClass = `size-${size}`; - return ( - <div className="flex size-full flex-col items-center justify-center"> - <LoaderCircle className={`mt-4 ${sizeClass} animate-spin`} /> - </div> - ); -} +export default function Loading({ size = 48 }: LoadingProps): JSX.Element { + return ( + <div className="flex size-full flex-col items-center justify-center" role="status" aria-label="Loading"> + <LoaderCircle className="mt-4 animate-spin" style={{ width: size, height: size }} /> + <span className="sr-only">Loading</span> + </div> + ); +}If you prefer Tailwind classes, switch to a constrained map to keep JIT happy:
const sizeClass = { sm: "size-6", md: "size-8", lg: "size-12" }[variant];
1-3: Type import isn’t necessary here.You can omit the explicit
JSXreturn type and thereacttype import; inference is sufficient and reduces imports.-import type { JSX } from "react"; -interface LoadingProps { +interface LoadingProps { size?: number; } -export default function Loading({ size = 12 }: LoadingProps): JSX.Element { +export default function Loading({ size = 12 }: LoadingProps) {app/c/[id]/page.tsx (2)
26-33: Unauthenticated branch spins forever; redirect to login (with return URL)Showing a full-screen loader indefinitely for unauthenticated users degrades UX and looks like a hang. Redirect them to
/login?next=/c/{id}(keep loader during redirect).Apply this minimal change inside the Unauthenticated block:
- <Unauthenticated> - <div className="h-screen bg-background flex flex-col overflow-hidden"> - <div className="flex-1 flex items-center justify-center"> - <Loading /> - </div> - </div> - </Unauthenticated> + <Unauthenticated> + <AuthRedirect chatId={chatId} /> + </Unauthenticated>Add the helper below (anywhere in this file):
import { useEffect } from "react"; import { useRouter } from "next/navigation"; function AuthRedirect({ chatId }: { chatId: string }) { const router = useRouter(); useEffect(() => { const next = `/c/${encodeURIComponent(chatId)}`; router.replace(`/login?next=${encodeURIComponent(next)}`); }, [router, chatId]); return ( <div className="h-screen bg-background flex flex-col overflow-hidden"> <div className="flex-1 flex items-center justify-center"> <Loading /> </div> </div> ); }
8-11: Guard against missing/invalididRoutes should handle malformed URLs. If
idis missing or not a string, render a 404 or navigate away to the home page.Example:
if (!chatId || typeof chatId !== "string") { return <div className="p-6">Invalid chat ID.</div>; }convex/schema.ts (1)
29-31: Tightenroleandpartsvalidation (optional)
roleshould be constrained to"user" | "assistant", andpartsideally be a tagged union for known types. This prevents malformed data from breaking rendering.Example:
role: v.union(v.literal("user"), v.literal("assistant")), parts: v.array(v.object({ type: v.string() })), // gradually specialize per part typeapp/components/Messages.tsx (1)
32-35: Micro-optimization: computelastAssistantMessageIndexwithout materializing arraysMapping and reversing on every render allocates extra arrays. Iterate from the end once.
- const lastAssistantMessageIndex = messages - .map((msg, index) => ({ msg, index })) - .reverse() - .find(({ msg }) => msg.role === "assistant")?.index; + let lastAssistantMessageIndex = undefined as number | undefined; + for (let i = messages.length - 1; i >= 0; i--) { + if (messages[i].role === "assistant") { + lastAssistantMessageIndex = i; + break; + } + }app/page.tsx (1)
14-19: Use Next router instead ofwindow.location.hreffor login redirectionClient-side navigation keeps app state and avoids full reload.
+import { useRouter } from "next/navigation"; const UnauthenticatedContent = () => { - const { clearInput } = useGlobalState(); + const { clearInput } = useGlobalState(); + const router = useRouter(); const handleSubmit = (e: React.FormEvent) => { e.preventDefault(); // Clear the input and redirect to login clearInput(); - window.location.href = "/login"; + router.push("/login"); };app/api/chat/route.ts (2)
24-29: Remove unused importupdateChatTodos
updateChatTodosis imported but not used.-import { - handleInitialChatAndUserMessage, - saveMessage, - updateChat, - updateChatTodos, -} from "@/lib/db/actions"; +import { + handleInitialChatAndUserMessage, + saveMessage, + updateChat, +} from "@/lib/db/actions";
131-157: Consider setting an explicit error finish reason and persisting partial output ononErrorIf the model errors after streaming partial text, the assistant message won’t be persisted from the server side. You handle user-initiated stop in the client, but model errors won’t hit that path. Optionally save the last assistant message and mark
finish_reason: "error".app/components/chat.tsx (3)
194-205: Optimistically remove the last assistant message in UI during regenerateDeleting from DB and then calling regenerate can momentarily show the stale assistant response until Convex pushes a new snapshot. Remove it locally first to avoid flicker.
Apply this diff:
const handleRegenerate = async () => { - // Remove the last assistant message from the UI and database + // Remove the last assistant message from the UI and database (optimistic) + setMessages((prev) => { + const lastIdx = [...prev] + .map((m, i) => ({ m, i })) + .reverse() + .find(({ m }) => m.role === "assistant")?.i; + if (lastIdx === undefined) return prev; + return [...prev.slice(0, lastIdx), ...prev.slice(lastIdx + 1)]; + }); await deleteLastAssistantMessage({ chatId }); regenerate({ body: { mode, todos, regenerate: true, }, }); };
23-30: Non-reactive chatId when prop id changeschatId is initialized once and won’t track prop id changes if the component stays mounted. If this component can receive a different id without remount, consider syncing via useEffect or document that route changes remount Chat.
231-246: Show a loading skeleton while chat header data loadsThe blank title during chatData === undefined causes a small layout jump. Consider a lightweight skeleton/spinner for header text during load.
.env.local.example (2)
9-13: Place NEXT_PUBLIC_WORKOS_REDIRECT_URI before secret keys and quote URLAligns with dotenv-linter’s ordering hint and avoids surprises with URL characters.
Apply this diff:
-# WorkOS Authentication (Required for user management and conversation persistence) -WORKOS_API_KEY=sk_example_123456789 -WORKOS_CLIENT_ID=client_123456789 -# generate a secure password (min 32 characters) -WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret" -NEXT_PUBLIC_WORKOS_REDIRECT_URI=http://localhost:3000/callback +# WorkOS Authentication (Required for user management and conversation persistence) +NEXT_PUBLIC_WORKOS_REDIRECT_URI="http://localhost:3000/callback" +WORKOS_API_KEY="sk_example_123456789" +WORKOS_CLIENT_ID="client_123456789" +# generate a secure password (min 32 characters) +WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret"
48-48: Add a trailing newlineConforms to editor/linter expectations.
Apply this diff:
-# CONVEX_SERVICE_ROLE_KEY= +# CONVEX_SERVICE_ROLE_KEY= +convex/messages.ts (3)
1-4: Remove unused importId is imported but unused.
Apply this diff:
-import { Id } from "./_generated/dataModel";
25-33: Optional: Prevent orphan messages in saveMessageEven on the service-key path, consider verifying the chat exists before inserting to avoid orphaned message records if upstream logic mis-orders calls. Keep as a fast path if you’re confident in ordering guarantees.
83-89: Return ordering assumptionYou rely on by_chat_id + order("asc"). If a different ordering is desired (e.g., by update_time), consider adding a composite index to avoid full scans in the future. Not blocking.
convex/chats.ts (2)
79-87: Idempotency on saveChatConsider checking for an existing chat with the same id (using by_id) to avoid accidental duplicates if upstream retries. Upsert or short-circuit insert when present.
35-43: Service key check is good; log surfaceMinor: use structured logs including chat id to aid tracing Unauthorized/Invalid service key incidents. Non-blocking.
lib/db/actions.ts (2)
76-88: Guard against empty messages when deriving initial titleDefensive check avoids runtime errors if messages is empty or parts[0] is missing.
Apply this diff:
- const lastMessage = messages[messages.length - 1]; - const firstPart = lastMessage.parts[0]; - const title = ( - firstPart.type === "text" ? firstPart.text : "New Chat" - )?.substring(0, 100); + const lastMessage = messages[messages.length - 1]; + const firstPart = lastMessage?.parts?.[0]; + const baseTitle = + firstPart && (firstPart as any).type === "text" + ? (firstPart as any).text + : "New Chat"; + const title = (baseTitle || "New Chat").substring(0, 100);
40-60: Optional: normalize before persisting messages on server paths tooFor consistency with the client stop-path normalization, consider normalizing user/assistant parts on server before saveMessage to keep the database canonical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
convex/_generated/api.d.tsis excluded by!**/_generated/**convex/_generated/dataModel.d.tsis excluded by!**/_generated/**pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
.cursor/rules/convex_rules.mdc(1 hunks).env.local.example(1 hunks)app/api/chat/route.ts(6 hunks)app/c/[id]/page.tsx(1 hunks)app/components/Messages.tsx(1 hunks)app/components/chat.tsx(1 hunks)app/page.tsx(1 hunks)components/ui/loading.tsx(1 hunks)convex/chats.ts(1 hunks)convex/messages.ts(1 hunks)convex/schema.ts(1 hunks)lib/actions/index.ts(2 hunks)lib/ai/tools/index.ts(1 hunks)lib/db/actions.ts(1 hunks)lib/utils.ts(2 hunks)package.json(4 hunks)types/chat.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .cursor/rules/convex_rules.mdc
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🧬 Code graph analysis (9)
lib/utils.ts (2)
convex/_generated/dataModel.d.ts (1)
Doc(30-33)types/chat.ts (1)
ChatMessage(58-58)
convex/messages.ts (1)
lib/db/actions.ts (1)
saveMessage(40-60)
app/c/[id]/page.tsx (3)
app/page.tsx (1)
Page(70-81)components/ui/loading.tsx (1)
Loading(7-14)app/components/chat.tsx (1)
Chat(23-329)
app/components/chat.tsx (8)
app/contexts/GlobalState.tsx (1)
useGlobalState(137-143)convex/messages.ts (2)
deleteLastAssistantMessage(157-199)saveMessageFromClient(107-152)lib/utils.ts (2)
convertToUIMessages(34-42)fetchWithErrorHandlers(12-32)lib/utils/message-processor.ts (1)
normalizeMessages(50-130)app/hooks/useMessageScroll.ts (1)
useMessageScroll(4-22)app/components/Messages.tsx (1)
Messages(22-170)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/ScrollToBottomButton.tsx (1)
ScrollToBottomButton(11-46)
app/components/Messages.tsx (2)
app/components/MessagePartHandler.tsx (1)
MessagePartHandler(16-68)app/components/MessageActions.tsx (1)
MessageActions(20-106)
convex/chats.ts (1)
lib/db/actions.ts (4)
getChatById(8-18)saveChat(20-39)updateChat(113-139)updateChatTodos(141-164)
app/page.tsx (4)
app/contexts/GlobalState.tsx (1)
useGlobalState(137-143)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/chat.tsx (1)
Chat(23-329)app/c/[id]/page.tsx (1)
Page(8-35)
lib/db/actions.ts (3)
convex/chats.ts (4)
getChatById(7-57)saveChat(62-93)updateChat(98-176)updateChatTodos(181-232)lib/errors.ts (1)
ChatSDKError(36-74)convex/messages.ts (1)
saveMessage(8-40)
app/api/chat/route.ts (5)
lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(62-111)updateChat(113-139)saveMessage(40-60)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (1)
generateTitleFromUserMessageWithWriter(67-90)lib/ai/tools/utils/sandbox.ts (1)
pauseSandbox(152-169)
🪛 dotenv-linter (3.3.0)
.env.local.example
[warning] 12-12: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 12-12: [ValueWithoutQuotes] This value needs to be surrounded in quotes
(ValueWithoutQuotes)
[warning] 13-13: [UnorderedKey] The NEXT_PUBLIC_WORKOS_REDIRECT_URI key should go before the WORKOS_API_KEY key
(UnorderedKey)
[warning] 48-48: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (10)
lib/actions/index.ts (2)
1-1: Import change is appropriate.Importing
UIMessageStreamWriterfromaimatches the new streaming title helper below. No concerns.
67-90: It looks like the public docs don’t specify whetherUIMessageStreamWriter.writereturns a promise. Could you please share the TypeScript declaration (or source) forUIMessageStreamWriter.writefrom your localnode_modules/@vercel/ai(or from the GitHub repo)? For example, you might open:node_modules/@vercel/ai/dist/index.d.tsand copy the interface or method signature for
UIMessageStreamWriter.write(...). With that snippet, we can definitively confirm if it returns aPromise(e.g.Promise<void>orPromise<boolean>), and whetherawaitis required.types/chat.ts (1)
58-58: Alias looks correct and improves clarity.Re-exporting
UIMessageasChatMessagealigns domain terminology across the app without changing structure. Good move.convex/schema.ts (1)
24-25: Allupdate_timefields are properly set in Convex mutationsThe schema’s
update_timerequirement has been verified against all write operations. Every mutation that inserts or patches records for the tables definingupdate_timeusesDate.now(), so no writes will fail.• convex/messages.ts
– Creation: line 31 setsupdate_time: Date.now()
– Update handler: line 143 setsupdate_time: Date.now()• convex/chats.ts
– Creation: line 84 setsupdate_time: Date.now()
– Default values: line 152 setsupdate_time: Date.now()
– Patch/update: line 223 setsupdate_time: Date.now()No additional changes are needed.
app/page.tsx (1)
69-80: LGTM: clean Auth split and compositionThe Authenticated/Unauthenticated separation is clear, and delegating chat internals to the
Chatcomponent reduces page complexity.app/api/chat/route.ts (2)
136-141: Mapping Already Implemented – No Changes NeededThe
updateChatmutation inconvex/chats.tsalready translates thefinishReasonargument into thefinish_reasonfield expected by the schema:
- File:
convex/chats.tsat lines 159–160if (args.finishReason !== undefined) { updateData.finish_reason = args.finishReason; }You can safely ignore this review comment.
Likely an incorrect or invalid review comment.
179-185: Filter out user messages inonFinishto avoid duplicate persistenceIt looks like you’re already saving the initial user message in
handleInitialChatAndUserMessage(unlessregenerateis true). SincecreateUIMessageStream’smessagesarray appears to include both user and assistant messages, you could end up persisting the same user message twice. To guard against this, only save non-user messages in youronFinishhandler:onFinish: async ({ messages }) => { - for (const message of messages) { - await saveMessage({ - chatId, - message, - }); - } + for (const message of messages) { + // only persist assistant replies here; + // initial user message is already saved upstream + if (message.role !== "user") { + await saveMessage({ chatId, message }); + } + } },Before merging, please verify that:
handleInitialChatAndUserMessagedoes indeed persist the user message once (and only once).createUIMessageStream’smessageslist always includes the user prompt in non-regeneration flows.- No other code paths call
saveMessageon the same user message a second time.app/components/chat.tsx (2)
42-46: Good use of conditional Convex query with "skip"Using "skip" prevents unnecessary queries before a chat actually exists for new sessions. Clean and efficient.
78-96: Solid message preparation pipelineNormalizing messages in prepareSendMessagesRequest and attaching chatId keeps the transport payload canonical. The local setMessages when hasChanges ensures UI consistency pre-flight.
lib/db/actions.ts (1)
8-18: Confirmed server-only import: safe to embed service keyI verified that lib/db/actions.ts is only imported in a server-only Next.js route handler and not bundled into any client code:
- The only import found is in app/api/chat/route.ts (lines 26–29), which lives under Next.js’s app/api folder (always executed on the server).
No other imports of lib/db/actions.ts exist outside of server contexts, so exposing the service key here won’t leak it to client bundles.
| import { use } from "react"; | ||
|
|
||
| export default function Page(props: { params: Promise<{ id: string }> }) { | ||
| const params = use(props.params); | ||
| const chatId = params.id; | ||
|
|
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Avoid use() for route params in a client component; prefer useParams
Using use(props.params) relies on React’s use() and a Promise-shaped params, which is brittle and requires React 19+. In Next.js client components, useParams() is the recommended way to access dynamic segments and avoids hydration pitfalls.
Apply this diff and confirm your React/Next versions don’t rely on use() here:
-import { use } from "react";
+import { useParams } from "next/navigation";
-export default function Page(props: { params: Promise<{ id: string }> }) {
- const params = use(props.params);
- const chatId = params.id;
+export default function Page() {
+ const { id } = useParams<{ id: string }>();
+ const chatId = Array.isArray(id) ? id[0] : id;🏁 Script executed:
#!/bin/bash
grep -n "use client" "app/c/[id]/page.tsx" || echo "No 'use client' directive found"
sed -n '1,30p' "app/c/[id]/page.tsx"Length of output: 1001
🏁 Script executed:
#!/bin/bash
# Display project dependencies to verify Next.js and React versions
sed -n '1,200p' package.jsonLength of output: 1956
Replace use() with useParams() in this client component
The project is on Next.js 15.5.0 and React 19.1.1, so the useParams hook from next/navigation is fully supported. Using use() here to unwrap a Promise<{ id: string }> in a client component is brittle and can lead to hydration mismatches; switching to useParams() aligns with Next.js conventions and avoids relying on the experimental React 19 use() behavior.
Please update:
• File: app/c/[id]/page.tsx
• Lines: 6–11
Apply the following diff:
-use client
-
-import { use } from "react";
+use client
+
+import { useParams } from "next/navigation";
-export default function Page(props: { params: Promise<{ id: string }> }) {
- const params = use(props.params);
- const chatId = params.id;
+export default function Page() {
+ const { id } = useParams<{ id: string }>();
+ const chatId = Array.isArray(id) ? id[0] : id;This ensures you’re using Next.js’s built-in routing hook for dynamic segments in client components.
📝 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 { use } from "react"; | |
| export default function Page(props: { params: Promise<{ id: string }> }) { | |
| const params = use(props.params); | |
| const chatId = params.id; | |
| use client | |
| import { useParams } from "next/navigation"; | |
| export default function Page() { | |
| const { id } = useParams<{ id: string }>(); | |
| const chatId = Array.isArray(id) ? id[0] : id; |
🤖 Prompt for AI Agents
In app/c/[id]/page.tsx around lines 6 to 11, the component currently calls
React's experimental use() to unwrap a Promise for params; replace that with
Next.js's useParams() from next/navigation in this client component: import
useParams, remove use() and the Promise-typed props, call const params =
useParams(); then derive const chatId = params?.id as string (or handle
undefined) and adjust the component signature to not expect a
Promise<{id:string}> so it uses the supported Next.js hook and avoids hydration
mismatches.
app/components/chat.tsx
Outdated
| // Sync Convex real-time data with useChat messages | ||
| useEffect(() => { | ||
| if (!messagesData || messagesData === null) return; | ||
|
|
||
| const uiMessages = convertToUIMessages(messagesData); | ||
| setMessages(uiMessages); | ||
| }, [messagesData, setMessages]); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid clobbering in-flight assistant streaming with DB snapshots
When Convex pushes a real-time update during assistant streaming, this effect replaces the partially streamed assistant message with the persisted snapshot (which usually lacks the in-progress assistant), causing a visible flicker and loss of the partial content. Gate the sync while streaming and include status in deps.
Apply this diff:
- useEffect(() => {
- if (!messagesData || messagesData === null) return;
-
- const uiMessages = convertToUIMessages(messagesData);
- setMessages(uiMessages);
- }, [messagesData, setMessages]);
+ useEffect(() => {
+ if (!messagesData || messagesData === null) return;
+ // Don't overwrite in-progress assistant streaming with a DB snapshot
+ if (status === "streaming") return;
+ const uiMessages = convertToUIMessages(messagesData);
+ setMessages(uiMessages);
+ }, [messagesData, status, setMessages]);📝 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.
| // Sync Convex real-time data with useChat messages | |
| useEffect(() => { | |
| if (!messagesData || messagesData === null) return; | |
| const uiMessages = convertToUIMessages(messagesData); | |
| setMessages(uiMessages); | |
| }, [messagesData, setMessages]); | |
| // Sync Convex real-time data with useChat messages | |
| useEffect(() => { | |
| if (!messagesData || messagesData === null) return; | |
| // Don't overwrite in-progress assistant streaming with a DB snapshot | |
| if (status === "streaming") return; | |
| const uiMessages = convertToUIMessages(messagesData); | |
| setMessages(uiMessages); | |
| }, [messagesData, status, setMessages]); |
🤖 Prompt for AI Agents
In app/components/chat.tsx around lines 128-135, the effect currently replaces
UI messages with DB snapshots unconditionally, which clobbers any in-progress
assistant streaming; gate the sync so it does not run while an assistant message
is streaming by checking the streaming/status state (e.g., isAssistantStreaming
or messageStatuses) before calling setMessages, include that streaming/status in
the effect dependency array, and merge DB messages with current UI state so an
in-flight assistant message (partial content and streaming flag) is preserved
rather than overwritten by the persisted snapshot.
| const hasTextContent = message.parts?.some( | ||
| (part: { type: string; text?: string }) => | ||
| (part.type === "text" && part.text && part.text.trim() !== "") || | ||
| part.type === "step-start" || | ||
| part.type?.startsWith("tool-"), | ||
| ); | ||
|
|
||
| const shouldShowLoader = | ||
| isLastAssistantMessage && | ||
| status === "streaming" && | ||
| !hasTextContent; | ||
| const shouldShowLoader = | ||
| isLastAssistantMessage && status === "streaming" && !hasTextContent; |
There was a problem hiding this comment.
Loader logic misses non-text content (e.g., terminal output), causing a stuck spinner
hasTextContent only checks text, step-start, or tool-*. Parts like data-terminal (and other non-text outputs) are renderable content but currently ignored.
Apply:
- const hasTextContent = message.parts?.some(
- (part: { type: string; text?: string }) =>
- (part.type === "text" && part.text && part.text.trim() !== "") ||
- part.type === "step-start" ||
- part.type?.startsWith("tool-"),
- );
+ const hasRenderableContent = message.parts?.some(
+ (part: { type: string; text?: string }) =>
+ // Any non-text part renders something
+ part.type !== "text"
+ ? true
+ : Boolean(part.text && part.text.trim() !== ""),
+ );
- const shouldShowLoader =
- isLastAssistantMessage && status === "streaming" && !hasTextContent;
+ const shouldShowLoader =
+ isLastAssistantMessage && status === "streaming" && !hasRenderableContent;📝 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 hasTextContent = message.parts?.some( | |
| (part: { type: string; text?: string }) => | |
| (part.type === "text" && part.text && part.text.trim() !== "") || | |
| part.type === "step-start" || | |
| part.type?.startsWith("tool-"), | |
| ); | |
| const shouldShowLoader = | |
| isLastAssistantMessage && | |
| status === "streaming" && | |
| !hasTextContent; | |
| const shouldShowLoader = | |
| isLastAssistantMessage && status === "streaming" && !hasTextContent; | |
| const hasRenderableContent = message.parts?.some( | |
| (part: { type: string; text?: string }) => | |
| // Any non-text part renders something | |
| part.type !== "text" | |
| ? true | |
| : Boolean(part.text && part.text.trim() !== ""), | |
| ); | |
| const shouldShowLoader = | |
| isLastAssistantMessage && status === "streaming" && !hasRenderableContent; |
🤖 Prompt for AI Agents
In app/components/Messages.tsx around lines 68-76, the hasTextContent check only
recognizes text, step-start, and tool-* parts so non-text renderable parts
(e.g., data-terminal, code, html, or parts carrying non-empty payload fields)
are treated as empty and keep the loader spinning; update the predicate to treat
any part with a renderable type or non-empty render payload as content — e.g.,
expand the allowed type checks to include known renderable types like
"data-terminal", "code", "html" (and any other app-specific types), and/or
consider any part that has non-empty fields such as part.content, part.data,
part.output, or part.payload as content; ensure shouldShowLoader uses the
revised hasTextContent so the loader hides when those non-text parts are
present.
| const chat = await ctx.db | ||
| .query("chats") | ||
| .withIndex("by_user_id") | ||
| .filter((q) => q.eq(q.field("id"), args.id)) | ||
| .first(); | ||
|
|
||
| return chat || null; |
There was a problem hiding this comment.
Wrong index for lookup by chat id
withIndex("by_user_id") requires an equality filter on user_id, but this query filters by id. This likely errors at runtime. Introduce and use a by_id index.
Apply this diff:
- const chat = await ctx.db
- .query("chats")
- .withIndex("by_user_id")
- .filter((q) => q.eq(q.field("id"), args.id))
- .first();
+ const chat = await ctx.db
+ .query("chats")
+ .withIndex("by_id", (q) => q.eq("id", args.id))
+ .first();And add this index in convex/schema.ts:
// convex/schema.ts
defineTable({
// ...fields...
})
.index("by_user_id", ["user_id"])
.index("by_id", ["id"]);🤖 Prompt for AI Agents
In convex/chats.ts around lines 45-51 the query incorrectly uses
withIndex("by_user_id") while filtering by id; replace withIndex("by_user_id")
with withIndex("by_id") so the equality filter q.eq(q.field("id"), args.id) uses
the correct index, and add the new index in convex/schema.ts by calling
.index("by_id", ["id"]) on the chats table definition to register the index.
| const chat = await ctx.db | ||
| .query("chats") | ||
| .withIndex("by_user_id") | ||
| .filter((q) => q.eq(q.field("id"), args.chatId)) | ||
| .first(); | ||
|
|
There was a problem hiding this comment.
Same index issue here (client save path)
Bind the by_user_id index to the authenticated user to satisfy Convex’s index filter requirements.
Apply this diff:
- const chat = await ctx.db
- .query("chats")
- .withIndex("by_user_id")
- .filter((q) => q.eq(q.field("id"), args.chatId))
- .first();
+ const chat = await ctx.db
+ .query("chats")
+ .withIndex("by_user_id", (q) => q.eq("user_id", user.subject))
+ .filter((q) => q.eq(q.field("id"), args.chatId))
+ .first();📝 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 chat = await ctx.db | |
| .query("chats") | |
| .withIndex("by_user_id") | |
| .filter((q) => q.eq(q.field("id"), args.chatId)) | |
| .first(); | |
| const chat = await ctx.db | |
| .query("chats") | |
| .withIndex("by_user_id", (q) => q.eq("user_id", user.subject)) | |
| .filter((q) => q.eq(q.field("id"), args.chatId)) | |
| .first(); |
🤖 Prompt for AI Agents
In convex/messages.ts around lines 123-128, the query uses
.withIndex("by_user_id") without binding the authenticated user, violating
Convex index filter requirements; update the call to .withIndex("by_user_id",
[ctx.auth?.userId]) (or the concrete userId value), ensure ctx.auth.userId
exists (throw or return an auth error if not), then keep the .filter comparing
q.field("id") to args.chatId so the index is properly bound to the current user
when fetching the chat.
| const chat = await ctx.db | ||
| .query("chats") | ||
| .withIndex("by_user_id") | ||
| .filter((q) => q.eq(q.field("id"), args.chatId)) | ||
| .first(); | ||
|
|
There was a problem hiding this comment.
Same index issue (delete last assistant message)
Bind by_user_id to the current user to avoid invalid/expensive scans.
Apply this diff:
- const chat = await ctx.db
- .query("chats")
- .withIndex("by_user_id")
- .filter((q) => q.eq(q.field("id"), args.chatId))
- .first();
+ const chat = await ctx.db
+ .query("chats")
+ .withIndex("by_user_id", (q) => q.eq("user_id", user.subject))
+ .filter((q) => q.eq(q.field("id"), args.chatId))
+ .first();🤖 Prompt for AI Agents
In convex/messages.ts around lines 171-176, the query uses
withIndex("by_user_id") but only filters by chat id, which defeats the index and
may cause scans; update the filter to bind the index by adding a condition that
q.field("user_id") equals the current user's id (ctx.auth.userId) in addition to
matching q.field("id") to args.chatId, keeping withIndex("by_user_id") so the DB
can use that index; also ensure ctx.auth.userId is validated/present before
running the query.
| messages: defineTable({ | ||
| chat_id: v.string(), | ||
| role: v.string(), | ||
| parts: v.array(v.any()), | ||
| update_time: v.number(), | ||
| }).index("by_chat_id", ["chat_id"]), | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add composite index for message ordering by chat/time
Fetching messages by chat_id commonly needs chronological ordering. A composite index enables efficient ordered queries.
messages: defineTable({
chat_id: v.string(),
role: v.string(),
parts: v.array(v.any()),
update_time: v.number(),
- }).index("by_chat_id", ["chat_id"]),
+ })
+ .index("by_chat_id", ["chat_id"])
+ .index("by_chat_id_update_time", ["chat_id", "update_time"]),📝 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.
| messages: defineTable({ | |
| chat_id: v.string(), | |
| role: v.string(), | |
| parts: v.array(v.any()), | |
| update_time: v.number(), | |
| }).index("by_chat_id", ["chat_id"]), | |
| }); | |
| messages: defineTable({ | |
| chat_id: v.string(), | |
| role: v.string(), | |
| parts: v.array(v.any()), | |
| update_time: v.number(), | |
| }) | |
| .index("by_chat_id", ["chat_id"]) | |
| .index("by_chat_id_update_time", ["chat_id", "update_time"]), |
🤖 Prompt for AI Agents
In convex/schema.ts around lines 27-33, the messages table only has a
single-field index on chat_id but needs a composite index to support efficient
chronological queries; add a second index on ["chat_id","update_time"] (e.g.
.index("by_chat_id_update_time", ["chat_id","update_time"])) so queries can
filter by chat_id and order by update_time efficiently (choose ascending or
descending order semantics according to your query patterns).
| import { ConvexHttpClient } from "convex/browser"; | ||
| import { UIMessagePart } from "ai"; | ||
|
|
||
| const convex = new ConvexHttpClient(process.env.NEXT_PUBLIC_CONVEX_URL!); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fail fast if Convex URL is misconfigured
Avoid constructing the client with an undefined URL; throw a clear error early.
Apply this diff:
-const convex = new ConvexHttpClient(process.env.NEXT_PUBLIC_CONVEX_URL!);
+const convexUrl = process.env.NEXT_PUBLIC_CONVEX_URL;
+if (!convexUrl) {
+ throw new ChatSDKError(
+ "bad_request:database",
+ "NEXT_PUBLIC_CONVEX_URL is not set",
+ );
+}
+const convex = new ConvexHttpClient(convexUrl);📝 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 convex = new ConvexHttpClient(process.env.NEXT_PUBLIC_CONVEX_URL!); | |
| const convexUrl = process.env.NEXT_PUBLIC_CONVEX_URL; | |
| if (!convexUrl) { | |
| throw new ChatSDKError( | |
| "bad_request:database", | |
| "NEXT_PUBLIC_CONVEX_URL is not set", | |
| ); | |
| } | |
| const convex = new ConvexHttpClient(convexUrl); |
🤖 Prompt for AI Agents
In lib/db/actions.ts around line 6, the Convex client is constructed directly
from process.env.NEXT_PUBLIC_CONVEX_URL which can be undefined; instead, read
the env var into a local const, validate it and throw a clear Error if it's
missing or empty, then pass the validated URL to new ConvexHttpClient; this
ensures the app fails fast with a descriptive message rather than constructing
the client with an undefined value.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/components/chat.tsx (1)
128-135: Don’t clobber in-flight assistant streaming with DB snapshotsThis effect blindly replaces
useChatstate during streaming, causing flicker and loss of partial assistant output. Gate bystatus === "streaming"and include it in deps.Apply:
useEffect(() => { if (!messagesData || messagesData === null) return; - const uiMessages = convertToUIMessages(messagesData); - setMessages(uiMessages); - }, [messagesData, setMessages]); + // Don't overwrite in-progress assistant streaming with a DB snapshot + if (status === "streaming") return; + const uiMessages = convertToUIMessages(messagesData); + setMessages(uiMessages); + }, [messagesData, status, setMessages]);
🧹 Nitpick comments (3)
app/api/chat/route.ts (2)
68-70: Double truncation risk (frontend + backend)Comment states token-limit handling is done on the frontend, yet the route also truncates. This can lead to over-truncation and degraded UX.
Apply either:
- Remove backend truncation (preferred if FE normalization is guaranteed), or
- Guard with an idempotency check to avoid re-truncating already-normalized input.
34-48: Add request payload validation for robustnessDefensive checks for
chatId(non-empty),messages(non-empty array), andmode(in allowed enum) will convert unexpected inputs into clean 4xxChatSDKErrors instead of falling through to 5xxs.Apply something like:
const { messages, mode, todos, chatId, regenerate, }: { messages: UIMessage[]; mode: ChatMode; chatId: string; todos?: Todo[]; regenerate?: boolean; } = await req.json(); + + if (!chatId || typeof chatId !== "string") { + throw new ChatSDKError("bad_request:chat", "Missing or invalid chatId"); + } + if (!Array.isArray(messages) || messages.length === 0) { + throw new ChatSDKError("bad_request:chat", "Missing messages"); + } + if (mode !== "agent" && mode !== "ask") { + throw new ChatSDKError("bad_request:chat", "Invalid mode"); + }app/components/chat.tsx (1)
194-205: Regenerate UX: remove last assistant locally before triggeringYou delete the last assistant message in DB, but the UI keeps it until Convex pushes an update, causing a flicker. Optimistically update local state before
regenerate(...).Apply:
const handleRegenerate = async () => { - // Remove the last assistant message from the UI and database + // Remove the last assistant message from the UI and database + setMessages((prev) => { + let idx = -1; + for (let i = prev.length - 1; i >= 0; i--) { + if (prev[i].role === "assistant") { + idx = i; + break; + } + } + return idx >= 0 ? [...prev.slice(0, idx), ...prev.slice(idx + 1)] : prev; + }); await deleteLastAssistantMessage({ chatId }); regenerate({ body: { mode, todos, regenerate: true, }, }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
app/api/chat/route.ts(6 hunks)app/components/chat.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/components/chat.tsx (10)
app/contexts/GlobalState.tsx (1)
useGlobalState(137-143)convex/messages.ts (2)
deleteLastAssistantMessage(157-199)saveMessageFromClient(107-152)types/chat.ts (2)
ChatMessage(58-58)Todo(38-42)lib/utils.ts (2)
convertToUIMessages(34-42)fetchWithErrorHandlers(12-32)lib/utils/message-processor.ts (1)
normalizeMessages(50-130)lib/errors.ts (1)
ChatSDKError(36-74)app/hooks/useMessageScroll.ts (1)
useMessageScroll(4-22)app/components/Messages.tsx (1)
Messages(22-170)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/ScrollToBottomButton.tsx (1)
ScrollToBottomButton(11-46)
app/api/chat/route.ts (7)
types/chat.ts (2)
ChatMode(3-3)Todo(38-42)lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(62-111)updateChat(113-139)saveMessage(40-60)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (1)
generateTitleFromUserMessageWithWriter(67-90)convex/chats.ts (1)
updateChat(98-176)convex/messages.ts (1)
saveMessage(8-40)
🔇 Additional comments (10)
app/api/chat/route.ts (5)
56-63: Good: centralized initial chat setup + ownership check via DB layerDefers chat existence/ownership checks and initial user-message persistence to
handleInitialChatAndUserMessage. This keeps the route lean and consistent with server-side authorization.
84-92: Tooling setup is solidPassing
getTodoManageralongsidetoolsenables later persistence of todos and sandbox cleanup; interface aligns withcreateTools(...)contract.
93-101: Title generation only for new chats—niceRunning title generation in parallel only when
isNewChatavoids redundant calls and reduces latency for existing chats. Emitting a transientdata-titleevent is a good UX touch.
102-111: Verify hard stop at 25 steps is intended
stopWhen: stepCountIs(25)can prematurely halt longer tasks/tools, especially in "agent" mode.Would you like me to remove or parameterize this limit behind an env or per-mode default?
121-130: Resilient error cleanupCatching
onError, pausing the sandbox, and awaitingtitlePromiseprevents leaked resources and dangling tasks even on failure.app/components/chat.tsx (5)
25-30: Stable chatId generation is correctSeeding
chatIdonce withuseState(() => id || uuidv4())avoids rerender churn and keeps URL rewriting sane for new chats.
43-46: Good: conditional fetch via Convex "skip"Defers message loading for new chats until the first submit—saves reads and avoids empty snapshots overwriting local UI.
81-95: Nice: normalize outgoing assistant parts before sendApplying
normalizeMessagesinprepareSendMessagesRequestkeeps the server payload clean and mirrors UI state when transient parts are removed/transformed.
96-100: Solid UX: live title updates via "data-title"Handling the transient
data-titleevent to update UI title early feels responsive.
209-219: Header/title logic is sensibleShows "New Chat" for fresh sessions and uses streaming or Convex data to populate thereafter. This avoids empty headers on first render.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
app/api/chat/route.ts (2)
134-145: Duplicate updateChat calls still possible on abort.Both
streamText.onFinish(lines 134-145) andtoUIMessageStream.onFinish(lines 154-168) can execute when the stream is aborted, resulting in duplicateupdateChatcalls with potentially conflicting data.Use a shared flag to prevent duplicate updates:
execute: async ({ writer }) => { + let hasUpdatedChat = false; const { tools, getSandbox, getTodoManager } = createTools( userID, writer, mode, executionMode, userLocation, todos, ); // ... title generation ... const result = streamText({ // ... config ... onFinish: async ({ finishReason }) => { + if (hasUpdatedChat) return; const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); if (generatedTitle || finishReason || currentTodos.length > 0) { + hasUpdatedChat = true; await updateChat({ chatId, title: generatedTitle, finishReason, todos: currentTodos.length > 0 ? currentTodos : undefined, }); } }, // ... rest ... }); writer.merge( result.toUIMessageStream({ onFinish: async ({ isAborted }) => { if (isAborted) { + if (!hasUpdatedChat) { console.log("Stream was aborted 3"); const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); if (generatedTitle || currentTodos.length > 0) { + hasUpdatedChat = true; await updateChat({ chatId, title: generatedTitle, finishReason: "abort", todos: currentTodos.length > 0 ? currentTodos : undefined, }); } + } } else { console.log("Stream completed normally"); } // ... sandbox cleanup ... }, }), );Also applies to: 154-168
183-190: User messages persisted twice.The
onFinishhandler persists all messages including user messages, buthandleInitialChatAndUserMessagealready saves user messages (unlessregenerateis true). This creates duplicate user message entries.Only persist assistant messages here:
onFinish: async ({ messages }) => { for (const message of messages) { + if (message.role !== "assistant") continue; await saveMessage({ chatId, message, }); } },
🧹 Nitpick comments (3)
app/api/chat/route.ts (3)
125-126: Remove or make console.log statements more informative.The console.log statements for debugging stream aborts should either be removed for production or enhanced with more context (e.g., user ID, chat ID).
Either remove the debug logs or enhance them:
-console.log("Stream was aborted 1", error); +console.error(`Stream aborted during processing - chatId: ${chatId}, userId: ${userID}`, error); -console.log("Stream was aborted 2", error); +console.error(`Stream onAbort triggered - chatId: ${chatId}`, error); -console.log("Stream was aborted 3"); +console.log(`Handling stream abort cleanup - chatId: ${chatId}`); -console.log("Stream completed normally"); +console.log(`Stream completed successfully - chatId: ${chatId}`);Also applies to: 148-149, 156-156, 170-170
9-9: Unused import: consumeStream.The
consumeStreamimport from the "ai" module is not used anywhere in this file.Remove the unused import:
import { convertToModelMessages, createUIMessageStream, createUIMessageStreamResponse, stepCountIs, streamText, UIMessage, smoothStream, - consumeStream, } from "ai";
122-133: Inconsistent error handling between onError and stream aborts.The
onErrorhandler performs cleanup but doesn't update the chat with an error state, while abort handlers do update the chat. Consider also updating the chat with error information for consistency.Consider updating the chat with error information:
onError: async (error) => { console.error("Error:", error); console.log("Stream was aborted 1", error); + // Update chat with error state + const generatedTitle = await titlePromise; + const currentTodos = getTodoManager().getAllTodos(); + + if (generatedTitle || currentTodos.length > 0) { + await updateChat({ + chatId, + title: generatedTitle, + finishReason: "error", + todos: currentTodos.length > 0 ? currentTodos : undefined, + }); + } // Perform same cleanup as onFinish to prevent resource leaks const sandbox = getSandbox(); if (sandbox) { await pauseSandbox(sandbox); } - await titlePromise; },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/api/chat/route.ts(6 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/api/chat/route.ts (5)
lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(62-111)updateChat(113-139)saveMessage(40-60)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (1)
generateTitleFromUserMessageWithWriter(67-90)lib/ai/tools/utils/sandbox.ts (1)
pauseSandbox(152-169)
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/ai/tools/utils/sandbox.ts (1)
54-67: Implement a Runtime Fallback from betaPause() to pause()To future-proof against SDK API drift in the @e2b/code-interpreter→e2b Sandbox API, retain the existing betaPause() call but fall back at runtime to pause() if betaPause is ever renamed or removed. Although e2b v2.0.1 today only exposes betaPause(), this zero-cost guard ensures we won’t break on future minor/patch releases.
Please apply this refactor in both code paths (around lines 54–67 and 160–166 in lib/ai/tools/utils/sandbox.ts):
- lib/ai/tools/utils/sandbox.ts:54–67
- await runningSandbox.betaPause() + // Prefer betaPause, fall back to pause for SDK compatibility + const pauseFn = + (runningSandbox as any).betaPause ?? + (runningSandbox as any).pause; + if (!pauseFn) { + throw new Error("Sandbox pause method not available in this SDK"); + } + await pauseFn.call(runningSandbox);
- lib/ai/tools/utils/sandbox.ts:160–166
- safeWaitUntil( - sandbox.betaPause().catch((error) => { + safeWaitUntil( + (async () => { + const pauseFn = (sandbox as any).betaPause ?? (sandbox as any).pause; + if (!pauseFn) throw new Error("Sandbox pause method not available"); + await pauseFn.call(sandbox); + })().catch((error) => {Also update the secondary occurrence at lines 160–166 and similarly at lines 54–67. This ensures that if e2b ever standardizes on pause(), our code will continue to work without interruption.
♻️ Duplicate comments (4)
.env.local.example (1)
12-14: Past issue resolved: inline comment moved off the value line.The previous invalid dotenv syntax for
WORKOS_COOKIE_PASSWORDis fixed by moving the comment to its own line and providing a placeholder value. LGTM.app/api/chat/route.ts (2)
131-144: Avoid duplicate updateChat patches on abort vs. finish with a shared flagBoth streamText.onFinish and toUIMessageStream.onFinish can execute on abort and double-patch finishReason. Gate with a shared flag in the execute closure.
- const stream = createUIMessageStream({ - execute: async ({ writer }) => { + const stream = createUIMessageStream({ + execute: async ({ writer }) => { + let wasAborted = false; const { tools, getSandbox, getTodoManager } = createTools( userID, writer, mode, executionMode, userLocation, todos, ); @@ - onFinish: async ({ finishReason }) => { + onFinish: async ({ finishReason }) => { + if (wasAborted) return; // abort path handled in UI stream onFinish const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); if (generatedTitle || finishReason || currentTodos.length > 0) { await updateChat({ chatId, title: generatedTitle, finishReason, todos: currentTodos.length > 0 ? currentTodos : undefined, }); } }, @@ - onFinish: async ({ isAborted, messages }) => { + onFinish: async ({ isAborted, messages }) => { if (isAborted) { - console.log("Stream was aborted 3"); + console.log("Stream was aborted 3"); + wasAborted = true; // Handle abort-specific cleanup const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); if (generatedTitle || currentTodos.length > 0) { await updateChat({ chatId, title: generatedTitle, finishReason: "abort", todos: currentTodos.length > 0 ? currentTodos : undefined, }); } } else { console.log("Stream completed normally");Also applies to: 149-166
169-175: Prevent duplicate user-message persistence; only persist assistant messages hereUser messages are already saved by handleInitialChatAndUserMessage (unless regenerate). Persisting all messages here duplicates the last user entry.
- // Handle normal completion - for (const message of messages) { - await saveMessage({ - chatId, - message, - }); - } + // Handle normal completion: persist assistant messages only + for (const message of messages) { + if (message.role !== "assistant") continue; + await saveMessage({ chatId, message }); + }lib/db/actions.ts (1)
6-6: Fail fast on missing NEXT_PUBLIC_CONVEX_URLConstructing the client with an undefined URL will produce opaque errors later.
-const convex = new ConvexHttpClient(process.env.NEXT_PUBLIC_CONVEX_URL!); +const convexUrl = process.env.NEXT_PUBLIC_CONVEX_URL; +if (!convexUrl) { + throw new ChatSDKError("bad_request:database", "NEXT_PUBLIC_CONVEX_URL is not set"); +} +const convex = new ConvexHttpClient(convexUrl);
🧹 Nitpick comments (13)
.env.local.example (6)
13-13: QuoteCharacter warning is acceptable for secrets.dotenv-linter warns about quotes, but keeping the password quoted is safer for users who paste secrets containing spaces or special characters. I recommend keeping quotes and ignoring this warning for this line.
If you prefer a clean linter run, you can also change to an unquoted placeholder that contains only safe characters:
-WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret" +WORKOS_COOKIE_PASSWORD=replace-with-a-32+char-random-secretBut be sure to document that real values may require quotes.
5-8: Provide concrete local defaults for Convex to reduce setup friction.Empty placeholders slow down onboarding. Consider adding sensible local dev defaults and a short hint.
Apply:
-# Convex deployment configuration -CONVEX_DEPLOYMENT= -NEXT_PUBLIC_CONVEX_URL= +# Convex deployment configuration +# Example for local dev (adjust if your Convex dev server uses a different port) +CONVEX_DEPLOYMENT=dev +NEXT_PUBLIC_CONVEX_URL=http://localhost:3210
17-22: Clarify whether OpenRouter is truly “Required.”This section sits under “API CONFIGURATION (Required).” If the app won’t function without an LLM provider, keep it; otherwise, relabel as optional and note supported providers. Add a one-liner pointing users to where it’s read (e.g., server/client file).
Example tweak:
-# API CONFIGURATION (Required) +# API CONFIGURATION (Required for AI responses)
27-33: Optional keys look good; one small DX improvement.Consider adding a brief note that
TERMINAL_EXECUTION_MODE=sandboxrequires the E2B key, otherwise keep it commented.
34-36: Public model config: add guidance on defaults.Call out that these are public (safe to commit) and optional, and mention any defaults the app uses when unset.
Example:
-# AI Model Configuration -# NEXT_PUBLIC_AGENT_MODEL= -# NEXT_PUBLIC_TITLE_MODEL= +# AI Model Configuration (public; optional — app uses sensible defaults if unset) +# NEXT_PUBLIC_AGENT_MODEL= +# NEXT_PUBLIC_TITLE_MODEL=
49-49: Add a trailing newline.Satisfy the linter’s EndingBlankLine rule.
Apply:
-# CONVEX_SERVICE_ROLE_KEY= +# CONVEX_SERVICE_ROLE_KEY= +lib/ai/tools/utils/sandbox.ts (2)
63-66: Retry strategy: switch from fixed sleep to exponential backoff with jitterFixed 5s waits can amplify thundering herd under partial outages. Use exponential backoff with jitter for more resilient pause retries.
- await new Promise((resolve) => setTimeout(resolve, 5000)); + const base = 1000; // 1s + const delay = Math.min(5000, base * 2 ** attempt); + const jitter = Math.random() * 250; + await new Promise((r) => setTimeout(r, delay + jitter));
15-20: Minor: docstring and log label drift from actual function nameThe flow comments still mention “resume”, and the error log says createOrConnectTerminal while the function is createOrConnectPersistentTerminal.
- * 2a. If found with "running" state: pause first (3 retries), then resume - * 2b. If found with "paused" state: resume directly (no retries needed) + * 2a. If found with "running" state: pause first (3 retries), then connect + * 2b. If found with "paused" state: connect directly (no retries needed)- console.error(`[${userID}] Error in createOrConnectTerminal:`, error); + console.error(`[${userID}] Error in createOrConnectPersistentTerminal:`, error);Also applies to: 134-136
package.json (1)
51-53: Add engines to guard runtime compatibility (Next 15 + React 19 + Convex + UUID 11)Lock Node and npm versions to reduce CI/prod drift, especially with Next 15 and UUID 11.
"devDependencies": { ... "typescript": "^5" }, + "engines": { + "node": ">=18.18 <23", + "npm": ">=9" + }Also applies to: 60-60
app/api/chat/route.ts (2)
119-130: Consider updating finishReason on error path as wellCurrently, onError only pauses the sandbox and waits for title generation. Persisting finishReason: "error" improves the chat’s final state fidelity.
onError: async (error) => { console.error("Error:", error); console.log("Stream was aborted 1", error); // Perform same cleanup as onFinish to prevent resource leaks const sandbox = getSandbox(); if (sandbox) { await pauseSandbox(sandbox); } await titlePromise; + try { + await updateChat({ chatId, finishReason: "error" }); + } catch (e) { + console.warn("Failed to mark chat as errored:", e); + } },
32-50: ’ss current run appear to have no obvious type-safety issues withgeolocation(req)—the helper explicitly accepts a standardRequest(see Astro guide) and returns geo fields (astro.webredirect.org)—and since this file lives under the App Router, it runs as an Edge Function by default (noexport const runtimeoverride found), so at runtimereqwill indeed be aNextRequestinstance. Unless you have explicitly opted into the Node.js runtime viaexport const runtime = 'nodejs', your cast toNextRequestis safe but redundant; you can improve clarity by changing the handler signature directly.• Change:
export async function POST(req: Request) {to:
import { NextRequest } from 'next/server'; export async function POST(req: NextRequest) {This removes the
as NextRequestcast on line 48, strengthens the signature, and aligns with Vercel Edge conventions.Tagging as an optional refactor—your code works as-is, but this tweak improves type safety and readability.
lib/db/actions.ts (2)
90-97: Ownership check: clarify error code for consistencyThe thrown error uses "forbidden:chat". Confirm your error taxonomy expects "forbidden" to map to 403 and surfaces on the UI. Otherwise, align with your other auth errors (e.g., "unauthorized:chat").
If consistent, no change needed. If not, adjust the code or the error mapping in lib/errors.ts.
51-56: Use server-side service role keys via Convex’s Server SDK or HTTP APIConvex supports keeping your service role key entirely on the server—either by using the server-only SDK (which picks up
CONVEX_SERVICE_ROLE_KEYfrom your environment) or by sending it in anAuthorization: Bearer <key>header when calling the HTTP API—so you never need to pass it as a mutation argument.Refactor suggestions:
- Configure a dedicated server-side Convex client (e.g. in
lib/db/client.ts):import { Convex } from "convex/server"; // This client automatically uses process.env.CONVEX_SERVICE_ROLE_KEY export const serverConvex = new Convex();- In
lib/db/actions.ts, update your mutation calls (lines 51–56):
• Before• Afterreturn await convex.mutation(api.messages.saveMessage, { serviceKey: process.env.CONVEX_SERVICE_ROLE_KEY!, chatId, role: message.role, parts: message.parts, });return await serverConvex.mutation(api.messages.saveMessage, { chatId, role: message.role, parts: message.parts, });- Remove the
serviceKeyfield from thesaveMessagemutation signature (and any other mutations/queries on lines 129–135) so your API surface only accepts application data, not credentials.References:
• Convex Authentication & Service Role Keys (Authorization header or server SDK)
• Convex Server SDK / HTTP API examples
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
.env.local.example(1 hunks)app/api/chat/route.ts(4 hunks)lib/ai/tools/utils/sandbox.ts(4 hunks)lib/db/actions.ts(1 hunks)package.json(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/db/actions.ts (3)
convex/chats.ts (3)
getChatById(7-57)saveChat(62-93)updateChat(98-176)lib/errors.ts (1)
ChatSDKError(36-74)convex/messages.ts (1)
saveMessage(8-40)
app/api/chat/route.ts (8)
types/chat.ts (2)
ChatMode(3-3)Todo(38-42)lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(62-111)updateChat(113-139)saveMessage(40-60)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (1)
generateTitleFromUserMessageWithWriter(67-90)lib/ai/tools/utils/sandbox.ts (1)
pauseSandbox(152-169)convex/chats.ts (1)
updateChat(98-176)convex/messages.ts (1)
saveMessage(8-40)
🪛 dotenv-linter (3.3.0)
.env.local.example
[warning] 13-13: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 14-14: [UnorderedKey] The NEXT_PUBLIC_WORKOS_REDIRECT_URI key should go before the WORKOS_API_KEY key
(UnorderedKey)
[warning] 49-49: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
🔇 Additional comments (5)
.env.local.example (3)
43-46: Analytics block is clear and correct.Quoting the PostHog host is good; avoids parsing surprises. Nothing to change.
9-15: Reorder WorkOS keys for linting and clarify optional status
- dotenv-linter’s UnorderedKey rule enforces alphabetical order, so
NEXT_PUBLIC_WORKOS_REDIRECT_URImust come before theWORKOS_*entries (webinstall.dev).- Since WorkOS gating was relaxed in this PR, update the section heading to indicate these variables are optional and only required when enabling WorkOS login.
Apply this minimal diff:
-# WorkOS Authentication (Required for user management and conversation persistence) -WORKOS_API_KEY=sk_example_123456789 -WORKOS_CLIENT_ID=client_123456789 -# generate a secure password (min 32 characters) -WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret" -NEXT_PUBLIC_WORKOS_REDIRECT_URI=http://localhost:3000/callback +# WorkOS Authentication (Optional — only needed when enabling WorkOS login) +NEXT_PUBLIC_WORKOS_REDIRECT_URI=http://localhost:3000/callback +WORKOS_API_KEY=sk_example_123456789 +WORKOS_CLIENT_ID=client_123456789 +# generate a secure password (min 32 characters) +WORKOS_COOKIE_PASSWORD="replace-with-a-32+char-random-secret"Follow-up:
- Confirm that the redirect URI (
/callback) matches your Next.js route (e.g.pages/callback.tsxvspages/auth/callback.tsx) to prevent setup errors.
1-49: Env var usage verification completeI ran a repo-wide scan for all keys listed in
.env.local.exampleand here are the results:• Referenced in code:
- NEXT_PUBLIC_CONVEX_URL (lib/db/actions.ts, components/ConvexClientProvider.tsx)
- WORKOS_CLIENT_ID (convex/auth.config.ts)
- EXA_API_KEY (lib/ai/tools/index.ts & web-search.ts)
- TERMINAL_EXECUTION_MODE (app/api/chat/route.ts)
- NEXT_PUBLIC_AGENT_MODEL & NEXT_PUBLIC_TITLE_MODEL (lib/ai/providers.ts)
- UPSTASH_REDIS_REST_URL, UPSTASH_REDIS_REST_TOKEN & RATE_LIMIT_REQUESTS (lib/rate-limit.ts)
- NEXT_PUBLIC_POSTHOG_KEY & NEXT_PUBLIC_POSTHOG_HOST (app/posthog.js)
- CONVEX_SERVICE_ROLE_KEY (convex/chats.ts, convex/messages.ts, lib/db/actions.ts)
• Not found anywhere:
- CONVEX_DEPLOYMENT
- WORKOS_API_KEY
- WORKOS_COOKIE_PASSWORD
- NEXT_PUBLIC_WORKOS_REDIRECT_URI
- OPENROUTER_API_KEY
- E2B_API_KEY
Please remove any unused vars from
.env.local.exampleor add the missing references in code if you intend to use them.lib/ai/tools/utils/sandbox.ts (1)
76-80: Confirm SDK’s resume() support before adding fallbackI didn’t find any
static resumeimplementation inlib/ai/tools/utils/sandbox.ts(Sandbox is imported from@e2b/code-interpreter), so it’s unclear whether the external SDK actually exposes aresume()method. Please verify in the SDK’s documentation or source that:
Sandbox.connect()may fail on paused sandboxes, andSandbox.resume()exists with the same signature as shown in the diff.If both are true, add the suggested
try { connect() } catch { resume() }fallback. Otherwise, adjust your logic to match the SDK’s actual API.package.json (1)
14-18: SDK upgrade sanity check complete: no legacy pause()/resume() calls remain
- Searched all
.tsand.tsxfiles forSandbox.pause,Sandbox.resume, and other E2B APIs- Only occurrences found in
lib/ai/tools/utils/sandbox.tsuse the newSandbox.connect,sandbox.betaPause(), andSandbox.killmethods- No remaining
pause()orresume()calls detected outside the updated sandbox utilitiesAll usages align with the @e2b/code-interpreter 2.x API surface. No further changes required.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
app/api/chat/route.ts (3)
137-149: Duplicate updateChat on abort vs. normal finish — gate with a shared flag.Both streamText.onFinish and the UI stream’s onFinish can run on abort; each calls updateChat, causing double patches and races.
Introduce a boolean guard at the start of execute and use it in both onFinish handlers:
execute: async ({ writer }) => { + // Ensure updateChat is patched at most once per request + let didPatch = false; const { tools, getSandbox, getTodoManager } = createTools( userID, writer, mode, executionMode, userLocation, todos, ); @@ - onFinish: async ({ finishReason }) => { - const generatedTitle = await titlePromise; - const currentTodos = getTodoManager().getAllTodos(); - - if (generatedTitle || finishReason || currentTodos.length > 0) { - await updateChat({ - chatId, - title: generatedTitle, - finishReason, - todos: currentTodos.length > 0 ? currentTodos : undefined, - }); - } - }, + onFinish: async ({ finishReason }) => { + const generatedTitle = await titlePromise; + const currentTodos = getTodoManager().getAllTodos(); + if (didPatch) return; + if (generatedTitle || finishReason || currentTodos.length > 0) { + didPatch = true; + await updateChat({ + chatId, + title: generatedTitle, + finishReason, + todos: currentTodos.length > 0 ? currentTodos : undefined, + }); + } + },
155-171: Mirror the one-patch guard in the UI stream’s abort path.Without this, abort flows still double-patch.
Apply:
writer.merge( result.toUIMessageStream({ onFinish: async ({ isAborted, messages }) => { if (isAborted) { console.log("Stream was aborted 3"); // Handle abort-specific cleanup const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); - if (generatedTitle || currentTodos.length > 0) { - await updateChat({ - chatId, - title: generatedTitle, - finishReason: "abort", - todos: currentTodos.length > 0 ? currentTodos : undefined, - }); - } + if (!didPatch && (generatedTitle || currentTodos.length > 0)) { + didPatch = true; + await updateChat({ + chatId, + title: generatedTitle, + finishReason: "abort", + todos: currentTodos.length > 0 ? currentTodos : undefined, + }); + } } else { console.log("Stream completed normally"); // Handle normal completion
173-181: Message persistence duplicates entire history (and re-saves the user message). Persist only the final assistant message.Looping all messages will N× duplicate the transcript and re-save user/system messages that are already persisted upstream.
Replace the loop with a targeted save of the last assistant message:
- // Handle normal completion - for (const message of messages) { - await saveMessage({ - chatId, - message, - }); - } + // Handle normal completion: persist only the latest assistant message + const last = messages[messages.length - 1]; + if (last?.role === "assistant") { + await saveMessage({ chatId, message: last }); + }Please also verify the client no longer saves the assistant on stop to avoid server+client duplication. Run:
#!/bin/bash # Find all saveMessage call sites and client stop handlers rg -nP -C3 '\bsaveMessage\s*\(' rg -nP -C3 'onStop|onFinish|stop\(' app | sed -n '1,200p'
🧹 Nitpick comments (2)
app/api/chat/route.ts (2)
125-136: Consider marking error terminations in chat metadata (finishReason: "error").Currently errors only pause the sandbox. Updating the chat helps the UI reflect state and avoids limbo after failures. Guard with the same didPatch flag to prevent races.
onError: async (error) => { console.error("Error:", error); console.log("Stream was aborted 1", error); // Perform same cleanup as onFinish to prevent resource leaks const sandbox = getSandbox(); if (sandbox) { await pauseSandbox(sandbox); } - await titlePromise; + const generatedTitle = await titlePromise; + const currentTodos = getTodoManager().getAllTodos(); + if (typeof didPatch !== "undefined" && !didPatch) { + didPatch = true; + await updateChat({ + chatId, + title: generatedTitle, + finishReason: "error", + todos: currentTodos.length > 0 ? currentTodos : undefined, + }); + } },
72-74: Comment is misleading.The comment says truncation is “now done on frontend” but we still run truncateMessagesToTokenLimit on the server. Either remove the comment or clarify why we still run a defensive truncation here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
app/api/chat/route.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
app/api/chat/route.ts (5)
lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(62-111)updateChat(113-139)saveMessage(40-60)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (1)
generateTitleFromUserMessageWithWriter(67-90)lib/ai/tools/utils/sandbox.ts (1)
pauseSandbox(152-169)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
app/api/chat/route.ts (4)
35-39: Propagate request abort to the model to prevent runaway computeCurrently you only log the abort; the model stream keeps running because
controller.abort()is never called. Wirereq.signal→controllerso the stream cancels when the client disconnects.const controller = new AbortController(); req.signal.addEventListener("abort", () => { console.log("Request aborted"); + if (!controller.signal.aborted) { + controller.abort(); // propagate to streamText/generation + } });
41-54: Add payload validation and return 4xx for missing chatId/messagesA missing
chatIdor emptymessagescurrently trickles into internal errors (likely 5xx). Validate early and throw aChatSDKError("bad_request:payload", ...).}: { messages: UIMessage[]; mode: ChatMode; chatId: string; todos?: Todo[]; regenerate?: boolean; } = await req.json(); + if ( + !chatId || + typeof chatId !== "string" || + !Array.isArray(messages) || + messages.length === 0 + ) { + throw new ChatSDKError( + "bad_request:payload", + "chatId and at least one message are required", + ); + }
88-90: Gate updateChat on abort vs. normal finish to avoid double patchesBoth
streamText.onFinish(Lines 139-151) andtoUIMessageStream.onFinish(Lines 161-176) can run on abort, racing and causing duplicateupdateChatpatches. Use a shared flag to ensure only one path updates.- const stream = createUIMessageStream({ + let wasAborted = false; + const stream = createUIMessageStream({ execute: async ({ writer }) => { const { tools, getSandbox, getTodoManager } = createTools( userID, writer, mode, executionMode, userLocation, todos, ); @@ - onFinish: async ({ finishReason }) => { + onFinish: async ({ finishReason }) => { + if (wasAborted) return; // abort path already handled const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); if (generatedTitle || finishReason || currentTodos.length > 0) { await updateChat({ chatId, title: generatedTitle, finishReason, todos: currentTodos.length > 0 ? currentTodos : undefined, }); } }, onAbort: async (error) => { - console.log("Stream was aborted 2", error); + console.log("Stream was aborted 2", error); + wasAborted = true; }, }); @@ writer.merge( result.toUIMessageStream({ - onFinish: async ({ isAborted, messages }) => { + onFinish: async ({ isAborted, messages }) => { if (isAborted) { - console.log("Stream was aborted 3"); + console.log("Stream was aborted 3"); + wasAborted = true; // Handle abort-specific cleanup const generatedTitle = await titlePromise; const currentTodos = getTodoManager().getAllTodos(); if (generatedTitle || currentTodos.length > 0) { await updateChat({ chatId, title: generatedTitle, finishReason: "abort", todos: currentTodos.length > 0 ? currentTodos : undefined, }); } - } + } else { + // Normal completion: persistence happens here (see separate comment) + } const sandbox = getSandbox(); if (sandbox) { await pauseSandbox(sandbox); } }, }), );Also applies to: 139-151, 152-154, 159-176
187-196: Persist only the newly generated assistant message; avoid duplicating user/history
handleInitialChatAndUserMessagealready persisted the user message (unless regenerate). Here you re-save the entiremessagesarray, duplicating previous entries and user messages. Persist only the last assistant message on normal completion, and do nothing on abort.- onFinish: async ({ messages }) => { - console.log("Stream completed normally 3"); - // Handle normal completion - for (const message of messages) { - await saveMessage({ - chatId, - message, - }); - } - }, + onFinish: async () => { + console.log("Stream completed"); + // No-op here; persistence happens inside toUIMessageStream.onFinish + // to avoid duplicates and to know if it was aborted. + },And inside
toUIMessageStream({ onFinish })(Lines 161-176), persist only the latest assistant message when not aborted:- } else { - // Normal completion: persistence happens here (see separate comment) - } + } else { + // Normal completion: persist only the latest assistant message + const last = messages[messages.length - 1]; + if (last?.role === "assistant") { + await saveMessage({ chatId, message: last }); + } + }Also applies to: 159-176
🧹 Nitpick comments (2)
app/api/chat/route.ts (2)
127-131: Clarify error log messageThis log prints “Stream was aborted 1” inside
onError, which can be misleading for non-abort errors.- console.log("Stream was aborted 1", error); + console.log("Stream error (not necessarily abort)", error);
99-107: Title generation: confirm abort signal wiring after controller fixOnce abort propagation is added, passing
controller.signalhere is fine. If you later remove the controller and passabortSignal: req.signaldirectly intostreamText, also passreq.signalintogenerateTitleFromUserMessageWithWriterfor consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (2)
app/api/chat/route.ts(4 hunks)lib/ai/tools/utils/sandbox.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- lib/ai/tools/utils/sandbox.ts
🧰 Additional context used
🧬 Code graph analysis (1)
app/api/chat/route.ts (9)
types/chat.ts (2)
ChatMode(3-3)Todo(38-42)lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/rate-limit.ts (1)
checkRateLimit(6-63)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(62-111)updateChat(113-139)saveMessage(40-60)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (1)
generateTitleFromUserMessageWithWriter(67-90)lib/ai/tools/utils/sandbox.ts (1)
pauseSandbox(152-169)convex/chats.ts (1)
updateChat(98-176)convex/messages.ts (1)
saveMessage(8-40)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (9)
lib/db/actions.ts (1)
1-7: Mark server-only and fail fast on misconfigured Convex URL
- Prevent accidental client import (and secret exposure) by marking this module as server-only.
- Avoid constructing Convex client with an undefined URL; fail fast with a clear error.
+import "server-only"; import { api } from "@/convex/_generated/api"; import { ChatSDKError } from "../errors"; import { ConvexHttpClient } from "convex/browser"; import { UIMessagePart } from "ai"; -const convex = new ConvexHttpClient(process.env.NEXT_PUBLIC_CONVEX_URL!); +const convexUrl = process.env.NEXT_PUBLIC_CONVEX_URL; +if (!convexUrl) { + throw new ChatSDKError( + "bad_request:database", + "NEXT_PUBLIC_CONVEX_URL is not set", + ); +} +const convex = new ConvexHttpClient(convexUrl);app/api/chat/route.ts (4)
36-38: Propagate request abort to the model streamCurrently only logs abort; the stream continues running. Abort the controller to cancel model computation and free resources.
req.signal.addEventListener("abort", () => { console.log("Request aborted"); + if (!controller.signal.aborted) { + controller.abort(); + } });
41-53: Validate payload (chatId and messages) to return 4xx instead of 5xxMissing/invalid
chatIdormessagesleads to downstream crashes. Fail fast with a clear client error.}: { messages: UIMessage[]; mode: ChatMode; chatId: string; todos?: Todo[]; regenerate?: boolean; } = await req.json(); + + if ( + !chatId || + typeof chatId !== "string" || + !Array.isArray(messages) || + messages.length === 0 + ) { + throw new ChatSDKError( + "bad_request:payload", + "chatId and at least one message are required", + ); + }
159-171: Avoid duplicating user messages during persistence
handleInitialChatAndUserMessagealready saves the user message (unlessregenerate). Persist only assistant messages here.- writer.merge( - result.toUIMessageStream({ - generateMessageId: uuidv4, - onFinish: async ({ messages }) => { - for (const message of messages) { - await saveMessage({ - chatId, - message, - }); - } - }, - }), - ); + writer.merge( + result.toUIMessageStream({ + generateMessageId: uuidv4, + onFinish: async ({ messages }) => { + for (const message of messages) { + if (message.role !== "assistant") continue; + await saveMessage({ chatId, message }); + } + }, + }), + );
8-9: Return a proper SSE response (sets required headers)Use the helper to set
Content-Type: text/event-streamand related headers. Improves client compatibility.-import { - JsonToSseTransformStream, -} from "ai"; +import { createUIMessageStreamResponse } from "ai"; @@ - return new Response(stream.pipeThrough(new JsonToSseTransformStream())); + return createUIMessageStreamResponse(stream);Also applies to: 175-175
convex/messages.ts (2)
80-87: Bind the by_user_id index correctly to satisfy Convex index requirementsMust constrain the indexed field (
user_id) with an equality inwithIndexto avoid invalid/expensive scans.- const chat = await ctx.db - .query("chats") - .withIndex("by_user_id") - .filter((q) => q.eq(q.field("id"), args.chatId)) - .first(); + const chat = await ctx.db + .query("chats") + .withIndex("by_user_id", (q) => q.eq("user_id", user.subject)) + .filter((q) => q.eq(q.field("id"), args.chatId)) + .first();
140-145: Same index binding issue in saveMessageFromClientBind
by_user_idto the authenticated user before filtering by chat id.- const chat = await ctx.db - .query("chats") - .withIndex("by_user_id") - .filter((q) => q.eq(q.field("id"), args.chatId)) - .first(); + const chat = await ctx.db + .query("chats") + .withIndex("by_user_id", (q) => q.eq("user_id", user.subject)) + .filter((q) => q.eq(q.field("id"), args.chatId)) + .first();app/components/chat.tsx (2)
128-134: Don’t clobber in-progress assistant streaming with DB snapshotsSkip overwriting
messageswhile streaming to avoid flicker and partial-content loss.- useEffect(() => { - if (!messagesData || messagesData === null) return; - - const uiMessages = convertToUIMessages(messagesData); - setMessages(uiMessages); - }, [messagesData, setMessages]); + useEffect(() => { + if (!messagesData || messagesData === null) return; + if (status === "streaming") return; // preserve in-flight assistant + const uiMessages = convertToUIMessages(messagesData); + setMessages(uiMessages); + }, [messagesData, status, setMessages]);
172-193: Remove client-side save-on-stop to avoid duplicates and racesThe server persists assistant messages at stream end. Saving here duplicates rows and races with
stop().- const handleStop = async () => { - // Save the current assistant message before stopping - stop(); - - const lastMessage = messages[messages.length - 1]; - if ( - lastMessage && - lastMessage.role === "assistant" && - status === "streaming" - ) { - try { - await saveMessageFromClient({ - id: lastMessage.id, - chatId, - role: lastMessage.role, - parts: lastMessage.parts, - }); - } catch (error) { - console.error("Failed to save message on stop:", error); - } - } - }; + const handleStop = async () => { + stop(); + };If you keep a client fallback, persist before
stop(), normalize parts, and coordinate with the server to skip its save on abort.
🧹 Nitpick comments (2)
lib/db/actions.ts (1)
45-49: Tighten message role typingUse a string literal union to prevent invalid roles from being persisted.
message: { id: string; - role: string; + role: "user" | "assistant" | "system"; parts: UIMessagePart<any, any>[]; };app/components/chat.tsx (1)
54-59: Nit: remove unused mutation after dropping client-side save
saveMessageFromClientbecomes unused if the stop-path persistence is removed.const deleteLastAssistantMessage = useMutation( api.messages.deleteLastAssistantMessage, ); - const saveMessageFromClient = useMutation(api.messages.saveMessageFromClient);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
app/api/chat/route.ts(4 hunks)app/components/chat.tsx(1 hunks)convex/messages.ts(1 hunks)convex/schema.ts(1 hunks)lib/db/actions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/schema.ts
🧰 Additional context used
🧬 Code graph analysis (4)
convex/messages.ts (1)
lib/db/actions.ts (1)
saveMessage(40-62)
lib/db/actions.ts (3)
convex/chats.ts (3)
getChatById(7-57)saveChat(62-93)updateChat(98-176)lib/errors.ts (1)
ChatSDKError(36-74)convex/messages.ts (1)
saveMessage(8-55)
app/api/chat/route.ts (7)
lib/auth/get-user-id.ts (1)
getUserID(12-30)lib/rate-limit.ts (1)
checkRateLimit(6-63)lib/db/actions.ts (3)
handleInitialChatAndUserMessage(64-114)updateChat(116-142)saveMessage(40-62)lib/ai/tools/index.ts (1)
createTools(17-73)lib/actions/index.ts (2)
generateTitleFromUserMessageWithWriter(67-90)getAIHeaders(5-12)convex/chats.ts (1)
updateChat(98-176)convex/messages.ts (1)
saveMessage(8-55)
app/components/chat.tsx (9)
app/contexts/GlobalState.tsx (1)
useGlobalState(137-143)convex/messages.ts (2)
deleteLastAssistantMessage(175-217)saveMessageFromClient(123-170)types/chat.ts (2)
ChatMessage(58-58)Todo(38-42)lib/utils.ts (2)
convertToUIMessages(34-42)fetchWithErrorHandlers(12-32)lib/utils/message-processor.ts (1)
normalizeMessages(50-130)app/hooks/useMessageScroll.ts (1)
useMessageScroll(4-22)app/components/Messages.tsx (1)
Messages(22-170)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/ScrollToBottomButton.tsx (1)
ScrollToBottomButton(11-46)
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/ScrollToBottomButton.tsx (1)
26-31: Fix undefined Tailwind spacing classes and update positioningI searched the repo and didn’t find any
tailwind.config.*files, which means there are no customspacingentries for34or42. As written,bottom-42andbottom-34won’t generate any CSS, so the button will fall back to its container’s default positioning. Additionally, usingabsolutemay cause the button to scroll out of view if its nearest positioned ancestor isn’t the viewport.• File:
app/components/ScrollToBottomButton.tsx(lines 26–31)
• Notailwind.config.*in repo → custom spacing tokens 34/42 are missing
• Suggest using default spacing scale or arbitrary values, and switching tofixedpositioningProposed diff:
- const bottomPosition = hasActiveTodos ? "bottom-42" : "bottom-34"; + // Use default Tailwind spacing scale (e.g. 40 → 10rem, 32 → 8rem) + const bottomPosition = hasActiveTodos ? "bottom-40" : "bottom-32"; - <div - className={`absolute ${bottomPosition} left-1/2 -translate-x-1/2 z-40 transition-all duration-300`} - > + <div + className={`fixed ${bottomPosition} left-1/2 -translate-x-1/2 z-40 transition-all duration-300`} + >If you need the exact pixel values (e.g. 168px/136px), use Tailwind’s arbitrary-value syntax instead:
const bottomPosition = hasActiveTodos ? "bottom-[168px]" : "bottom-[136px]";This will ensure your button is always visible at the intended offset.
♻️ Duplicate comments (4)
app/components/chat.tsx (2)
134-141: Don’t clobber in‑progress assistant streaming with DB snapshotsThis effect overwrites partial assistant content mid-stream, causing flicker and loss of context. Gate on status !== "streaming".
- useEffect(() => { - if (!messagesData || messagesData === null) return; - - const uiMessages = convertToUIMessages(messagesData); - setMessages(uiMessages); - }, [messagesData, setMessages]); + useEffect(() => { + if (!messagesData || messagesData === null) return; + // Don't overwrite in-progress assistant streaming with a DB snapshot + if (status === "streaming") return; + const uiMessages = convertToUIMessages(messagesData); + setMessages(uiMessages); + }, [messagesData, status, setMessages]);
178-199: Client-side save-on-stop duplicates server persistence and races with stop()The route already persists at stream end; saving again here creates duplicates. Also, calling stop() before the check can flip status away from "streaming", skipping the save intermittently.
Recommended: remove client-side persistence and rely on server:
- const handleStop = async () => { - // Save the current assistant message before stopping - stop(); - - const lastMessage = messages[messages.length - 1]; - if ( - lastMessage && - lastMessage.role === "assistant" && - status === "streaming" - ) { - try { - await saveMessageFromClient({ - id: lastMessage.id, - chatId, - role: lastMessage.role, - parts: lastMessage.parts, - }); - } catch (error) { - console.error("Failed to save message on stop:", error); - } - } - }; + const handleStop = async () => { + stop(); + };If you must keep a client fallback, persist BEFORE stop() and normalize parts:
- const handleStop = async () => { - // Save the current assistant message before stopping - stop(); - - const lastMessage = messages[messages.length - 1]; + const handleStop = async () => { + const lastMessage = messages[messages.length - 1]; if ( lastMessage && lastMessage.role === "assistant" && status === "streaming" ) { try { - await saveMessageFromClient({ - id: lastMessage.id, - chatId, - role: lastMessage.role, - parts: lastMessage.parts, - }); + const { messages: normalized } = normalizeMessages([lastMessage as any]); + const partsToSave = normalized?.[0]?.parts ?? lastMessage.parts; + await saveMessageFromClient({ + id: lastMessage.id, + chatId, + role: lastMessage.role, + parts: partsToSave, + }); } catch (error) { console.error("Failed to save message on stop:", error); } } - }; + stop(); + };To confirm server-side persistence exists:
#!/bin/bash # Search the chat API route for save/persist calls at stream end. rg -nC3 'save(Message|Chat)|persist|updateChat|saveMessage' app/api/chat/route.tsconvex/chats.ts (2)
45-49: Wrong index for lookup by chat idThis query filters by id but uses the "by_user_id" index. Use a dedicated "by_id" index and use it directly in withIndex.
- const chat = await ctx.db - .query("chats") - .withIndex("by_user_id") - .filter((q) => q.eq(q.field("id"), args.id)) - .first(); + const chat = await ctx.db + .query("chats") + .withIndex("by_id", (q) => q.eq("id", args.id)) + .first();Also add this index in convex/schema.ts (outside this diff):
// chats table defineTable({ // ...fields... }) .index("by_user_id", ["user_id"]) .index("by_id", ["id"]);
242-274: updateChatTodos: same access control and index issues as updateChat
- Uses "by_user_id" while filtering by id.
- Lacks identity ownership checks when no serviceKey is provided.
handler: async (ctx, args) => { // Verify service role key if ( args.serviceKey && args.serviceKey !== process.env.CONVEX_SERVICE_ROLE_KEY ) { throw new Error("Unauthorized: Invalid service key"); } try { // Find the chat by chatId const chat = await ctx.db .query("chats") - .withIndex("by_user_id") - .filter((q) => q.eq(q.field("id"), args.chatId)) + .withIndex("by_id", (q) => q.eq("id", args.chatId)) .first(); if (!chat) { throw new Error("Chat not found"); } + if (!args.serviceKey) { + const identity = await ctx.auth.getUserIdentity(); + if (!identity) { + throw new Error("Unauthorized"); + } + if (chat.user_id !== identity.subject) { + throw new Error("Forbidden"); + } + } // Update the chat with new todos await ctx.db.patch(chat._id, { todos: args.todos, update_time: Date.now(), }); return null;
🧹 Nitpick comments (21)
app/components/ComputerSidebar.tsx (2)
128-128: Spacing change: verify safe-area handling for the fixed container on mobile.Removing vertical padding is fine visually on desktop, but this root wrapper is fixed-positioned on smaller viewports. Without top/bottom padding you risk content being flush with device notches/home indicators on iOS. Consider adding safe-area insets (desktop unaffected).
Apply this diff to preserve safe-area on mobile only:
- <div className="h-full w-full top-0 left-0 desktop:top-auto desktop:left-auto desktop:right-auto z-50 fixed desktop:relative desktop:h-full desktop:mr-4 flex-shrink-0"> + <div className="h-full w-full top-0 left-0 desktop:top-auto desktop:left-auto desktop:right-auto z-50 fixed desktop:relative desktop:h-full desktop:mr-4 flex-shrink-0 pt-[env(safe-area-inset-top)] pb-[env(safe-area-inset-bottom)] desktop:pt-0 desktop:pb-0">
143-146: Unnecessary tabIndex on a native button.
tabIndex={0}on a button is redundant; native buttons are already focusable in tab order. Removing it avoids potential tab order overrides.- className="w-7 h-7 relative rounded-md inline-flex items-center justify-center gap-2.5 cursor-pointer hover:bg-muted/50 transition-colors" - aria-label="Minimize sidebar" - tabIndex={0} + className="w-7 h-7 relative rounded-md inline-flex items-center justify-center gap-2.5 cursor-pointer hover:bg-muted/50 transition-colors" + aria-label="Minimize sidebar"app/contexts/GlobalState.tsx (1)
115-141: Stabilize context value to reduce app-wide re-renders.
valueis recreated each render, and several action functions are redefined. In large trees, this can cause avoidable re-renders of all consumers. Memoize callbacks and the context value.- const value: GlobalStateType = { + const value: GlobalStateType = React.useMemo(() => ({ input, setInput, mode, setMode, chatTitle, setChatTitle, sidebarOpen, setSidebarOpen, sidebarContent, setSidebarContent, chatSidebarOpen, setChatSidebarOpen, todos, setTodos, mergeTodos, isTodoPanelExpanded, setIsTodoPanelExpanded, - clearInput, - resetChat, - openSidebar, - updateSidebarContent, - closeSidebar, - toggleChatSidebar, - }; + clearInput, + resetChat, + openSidebar: React.useCallback(openSidebar, []), + updateSidebarContent: React.useCallback(updateSidebarContent, []), + closeSidebar: React.useCallback(closeSidebar, []), + toggleChatSidebar: React.useCallback(toggleChatSidebar, []), + }), [ + input, mode, chatTitle, sidebarOpen, sidebarContent, chatSidebarOpen, todos, + isTodoPanelExpanded, mergeTodos + ]);Note: If you wrap the action creators with
useCallbackwhere they’re defined, you won’t need the inneruseCallbackwrappers above.components/ui/sheet.tsx (1)
61-71: Sizing and border classes: minor polish.When
side="top"/"bottom", consider adding explicit widthw-fullfor consistency. Today it’s implied viainset-x-0, butw-fullmakes intent clear and helps with future refactors.- side === "top" && - "data-[state=closed]:slide-out-to-top data-[state=open]:slide-in-from-top inset-x-0 top-0 h-auto border-b", - side === "bottom" && - "data-[state=closed]:slide-out-to-bottom data-[state=open]:slide-in-from-bottom inset-x-0 bottom-0 h-auto border-t", + side === "top" && + "data-[state=closed]:slide-out-to-top data-[state=open]:slide-in-from-top inset-x-0 top-0 h-auto w-full border-b", + side === "bottom" && + "data-[state=closed]:slide-out-to-bottom data-[state=open]:slide-in-from-bottom inset-x-0 bottom-0 h-auto w-full border-t",hooks/use-mobile.ts (1)
10-18: Prefer mql.matches and add Safari fallback for MediaQueryList events.Using
window.innerWidthworks butmql.matchesis the source of truth and avoids edge cases. Also, older Safari versions requireaddListener/removeListener.React.useEffect(() => { - const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); - const onChange = () => { - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); - }; - mql.addEventListener("change", onChange); - setIsMobile(window.innerWidth < MOBILE_BREAKPOINT); - return () => mql.removeEventListener("change", onChange); + const mql = window.matchMedia(`(max-width: ${MOBILE_BREAKPOINT - 1}px)`); + const onChange = () => setIsMobile(mql.matches); + // Init + onChange(); + // Subscribe (modern + Safari fallback) + if ("addEventListener" in mql) { + mql.addEventListener("change", onChange); + return () => mql.removeEventListener("change", onChange); + } else { + // @ts-expect-error: Safari <14 + mql.addListener(onChange); + // @ts-expect-error: Safari <14 + return () => mql.removeListener(onChange); + } }, []);components/ui/skeleton.tsx (1)
1-11: Forward ref and accessibility default for decorative skeletonsForwarding a ref improves composability (measurement/animations), and defaulting to aria-hidden keeps skeletons out of the accessibility tree unless explicitly overridden.
Apply:
-import { cn } from "@/lib/utils"; +import * as React from "react"; +import { cn } from "@/lib/utils"; -function Skeleton({ className, ...props }: React.ComponentProps<"div">) { - return ( - <div - data-slot="skeleton" - className={cn("bg-accent animate-pulse rounded-md", className)} - {...props} - /> - ); -} +const Skeleton = React.forwardRef<HTMLDivElement, React.ComponentProps<"div">>( + ({ className, ...props }, ref) => { + return ( + <div + ref={ref} + data-slot="skeleton" + aria-hidden="true" + className={cn("bg-accent animate-pulse rounded-md", className)} + {...props} + /> + ); + }, +);components/ui/input.tsx (1)
5-19: Forward ref and sensible default type for InputForwarding the ref is important for form libs and focus management; defaulting type to "text" avoids accidental "submit" behavior when this is swapped with a button inside forms.
-function Input({ className, type, ...props }: React.ComponentProps<"input">) { - return ( - <input - type={type} +const Input = React.forwardRef< + HTMLInputElement, + React.ComponentProps<"input"> +>(({ className, type = "text", ...props }, ref) => { + return ( + <input + ref={ref} + type={type} data-slot="input" className={cn( "file:text-foreground placeholder:text-muted-foreground selection:bg-primary selection:text-primary-foreground dark:bg-input/30 border-input flex h-9 w-full min-w-0 rounded-md border bg-transparent px-3 py-1 text-base shadow-xs transition-[color,box-shadow] outline-none file:inline-flex file:h-7 file:border-0 file:bg-transparent file:text-sm file:font-medium disabled:pointer-events-none disabled:cursor-not-allowed disabled:opacity-50 md:text-sm", "focus-visible:border-ring focus-visible:ring-ring/50 focus-visible:ring-[3px]", "aria-invalid:ring-destructive/20 dark:aria-invalid:ring-destructive/40 aria-invalid:border-destructive", className, )} {...props} /> - ); -} + ); +});components/ui/separator.tsx (1)
8-26: Forward ref to Radix primitive for consistency with shadcn/Radix patternsThis keeps ref semantics available to consumers and aligns with typical shadcn UI wrappers.
-function Separator({ - className, - orientation = "horizontal", - decorative = true, - ...props -}: React.ComponentProps<typeof SeparatorPrimitive.Root>) { - return ( - <SeparatorPrimitive.Root - data-slot="separator" - decorative={decorative} - orientation={orientation} - className={cn( - "bg-border shrink-0 data-[orientation=horizontal]:h-px data-[orientation=horizontal]:w-full data-[orientation=vertical]:h-full data-[orientation=vertical]:w-px", - className, - )} - {...props} - /> - ); -} +const Separator = React.forwardRef< + React.ElementRef<typeof SeparatorPrimitive.Root>, + React.ComponentPropsWithoutRef<typeof SeparatorPrimitive.Root> +>(({ className, orientation = "horizontal", decorative = true, ...props }, ref) => { + return ( + <SeparatorPrimitive.Root + ref={ref} + data-slot="separator" + decorative={decorative} + orientation={orientation} + className={cn( + "bg-border shrink-0 data-[orientation=horizontal]:h-px data-[orientation=horizontal]:w-full data-[orientation=vertical]:h-full data-[orientation=vertical]:w-px", + className, + )} + {...props} + /> + ); +});app/components/ChatItem.tsx (2)
24-32: Only close the sidebar on mobile to preserve desktop UXCurrently, the sidebar closes on all viewports. Many apps keep the chat list open on desktop. Gate the close behavior by isMobile.
- const handleClick = () => { - closeSidebar(); - - if (isMobile) { - setChatSidebarOpen(false); - } - - router.push(`/c/${id}`); - }; + const handleClick = () => { + if (isMobile) { + // On mobile, collapse the sidebar to reveal content + setChatSidebarOpen(false); + closeSidebar(); + } + router.push(`/c/${id}`); + };
35-48: Improve semantics and a11y for navigation items
- Indicate the active item with aria-current for assistive tech.
- If your Button supports asChild (shadcn pattern), consider rendering an anchor via next/link for link semantics. If not, at least ensure type="button" to avoid accidental form submits.
Minimal a11y improvement:
- <Button + <Button variant="ghost" onClick={handleClick} + type="button" + aria-current={isActive ? "page" : undefined} className={`flex items-center gap-2 p-3 h-auto w-full text-left justify-start ${ isActive ? "bg-sidebar-accent text-sidebar-accent-foreground" : "hover:bg-sidebar-accent/50" }`} title={title} >Optional link semantics (if Button supports
asChild):import Link from "next/link"; <Button asChild variant="ghost" /* ...rest */> <Link href={`/c/${id}`} aria-current={isActive ? "page" : undefined"} title={title}> <span className="text-sm font-medium truncate flex-1 min-w-0">{title}</span> </Link> </Button>If Button does not support
asChild, keep the minimal change above and skip the Link refactor.app/components/UserDropdownMenu.tsx (2)
46-51: Prefer email before generic “User” in display name fallbackIf names are missing but email exists, show the email for a better UX.
- const getDisplayName = () => { - if (user.firstName && user.lastName) { - return `${user.firstName} ${user.lastName}`; - } - return user.firstName || user.lastName || 'User'; - }; + const getDisplayName = () => { + if (user.firstName && user.lastName) { + return `${user.firstName} ${user.lastName}`; + } + return user.firstName || user.lastName || user.email || "User"; + };
55-75: Make the trigger keyboard accessibleThe trigger child is a div. Add role and tabIndex (or switch to a button) for keyboard/AT accessibility.
- <div className="flex items-center gap-3 p-3 cursor-pointer hover:bg-sidebar-accent/50 rounded-md transition-colors"> + <div + className="flex items-center gap-3 p-3 cursor-pointer hover:bg-sidebar-accent/50 rounded-md transition-colors" + role="button" + tabIndex={0} + >app/components/ChatHeader.tsx (2)
3-9: Prefer Next.js router over window.location for navigationUsing next/navigation improves SPA UX and preserves state where appropriate.
-import React from "react"; +import React from "react"; +import { useRouter } from "next/navigation"; … - const handleSignIn = () => { - window.location.href = "/login"; - }; + const router = useRouter(); + const handleSignIn = () => router.push("/login"); … - const handleSignUp = () => { - window.location.href = "/signup"; - }; + const handleSignUp = () => router.push("/signup");Also applies to: 33-39
53-60: Make the sidebar toggle an actual button for a11yThe clickable icon lives inside passive containers. Wrap PanelLeft in a Button (variant="ghost") or add role/button semantics for keyboard users and AT.
Example minimal change:
- <div className="flex h-7 w-7 items-center justify-center cursor-pointer rounded-md hover:bg-muted/50 mr-2"> - <PanelLeft - className="size-5 text-muted-foreground cursor-pointer" - onClick={toggleChatSidebar} - /> - </div> + <button + type="button" + aria-label="Open chat sidebar" + onClick={toggleChatSidebar} + className="flex h-7 w-7 items-center justify-center rounded-md hover:bg-muted/50 mr-2" + > + <PanelLeft className="size-5 text-muted-foreground" /> + </button>Also applies to: 107-114, 166-173
app/components/ChatSidebar.tsx (3)
24-29: Remove unused isMobile prop from ChatSidebarHeaderThe prop is passed but never used, adding noise and risk of confusion.
-const ChatSidebarHeader: React.FC<{ - handleNewChat: () => void; - handleCloseSidebar: () => void; - isMobile: boolean; -}> = ({ handleNewChat, handleCloseSidebar, isMobile }) => ( +const ChatSidebarHeader: React.FC<{ + handleNewChat: () => void; + handleCloseSidebar: () => void; +}> = ({ handleNewChat, handleCloseSidebar }) => (Drop prop at call sites:
- <ChatSidebarHeader - handleNewChat={handleNewChat} - handleCloseSidebar={handleCloseSidebar} - isMobile={isMobile} - /> + <ChatSidebarHeader + handleNewChat={handleNewChat} + handleCloseSidebar={handleCloseSidebar} + />…and similarly in the desktop section.
Also applies to: 155-158, 181-185
95-106: Key vs. id inconsistencyYou’re using key={chat._id} but passing id={chat.id}. If both exist, that’s fine; otherwise, prefer a single consistent identifier (likely chat.id) for both key and routing.
- key={chat._id} + key={chat.id ?? chat._id}
55-59: Add lightweight types for chats and user propsThere’s a lot of any here. Introducing a minimal ChatListItem and AuthUser interface will improve maintainability and catch regressions.
I can add types for chats (id, title) and the auth user shape, plus derive them from Convex types if available.
Also applies to: 98-104, 116-123
app/components/chat.tsx (1)
67-71: Stable message identity across UI reloadsconvertToUIMessages uses message._id for UI id, while client-generated messages use uuid id. Consider using message.id ?? message._id to keep IDs stable across DB round-trips and reduce reconciliation edge cases.
I can open a follow-up PR to adjust convertToUIMessages in lib/utils.ts:
- id: message._id, + id: (message as any).id ?? message._id,convex/chats.ts (1)
98-118: Consider consolidating todos updates into updateChat to avoid duplicate logicupdateChatTodos overlaps significantly with updateChat. Unify into a single mutation to reduce surface area and potential drift.
Happy to propose a consolidated mutation signature if desired.
components/ui/sidebar.tsx (2)
72-90: Hydrate initial open state from cookieState is persisted to a cookie but never read. On first mount, initialize from the cookie when the component is uncontrolled.
const [_open, _setOpen] = React.useState(defaultOpen); const open = openProp ?? _open; const setOpen = React.useCallback( (value: boolean | ((value: boolean) => boolean)) => { const openState = typeof value === "function" ? value(open) : value; if (setOpenProp) { setOpenProp(openState); } else { _setOpen(openState); } // This sets the cookie to keep the sidebar state. - document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; path=/; max-age=${SIDEBAR_COOKIE_MAX_AGE}`; + const secure = typeof window !== "undefined" && window.location.protocol === "https:" ? "; Secure" : ""; + document.cookie = `${SIDEBAR_COOKIE_NAME}=${openState}; Path=/; Max-Age=${SIDEBAR_COOKIE_MAX_AGE}; SameSite=Lax${secure}`; }, [setOpenProp, open], ); + + // Initialize from cookie when uncontrolled. + React.useEffect(() => { + if (openProp !== undefined) return; + const match = document.cookie.match(new RegExp(`(?:^|; )${SIDEBAR_COOKIE_NAME}=([^;]+)`)); + if (match) { + _setOpen(match[1] === "true"); + } + }, [openProp]);
294-299: Nonstandard "in-data-*" variant; replace with supported arbitrary variant selectorin-data-[side=left] / in-data-[side=right] isn’t a default Tailwind variant. Prefer the bracket arbitrary variant you’re already using elsewhere.
- "in-data-[side=left]:cursor-w-resize in-data-[side=right]:cursor-e-resize", + "[[data-side=left]_&]:cursor-w-resize [[data-side=right]_&]:cursor-e-resize",If you do have a custom "in-*" variant configured, ignore this suggestion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (18)
app/components/ChatHeader.tsx(1 hunks)app/components/ChatItem.tsx(1 hunks)app/components/ChatSidebar.tsx(1 hunks)app/components/ComputerSidebar.tsx(1 hunks)app/components/ScrollToBottomButton.tsx(2 hunks)app/components/UserDropdownMenu.tsx(1 hunks)app/components/chat.tsx(1 hunks)app/contexts/GlobalState.tsx(6 hunks)components/ui/avatar.tsx(1 hunks)components/ui/input.tsx(1 hunks)components/ui/separator.tsx(1 hunks)components/ui/sheet.tsx(1 hunks)components/ui/sidebar.tsx(1 hunks)components/ui/skeleton.tsx(1 hunks)convex/chats.ts(1 hunks)convex/messages.ts(1 hunks)hooks/use-mobile.ts(1 hunks)package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- convex/messages.ts
🧰 Additional context used
🧬 Code graph analysis (14)
app/components/ChatItem.tsx (3)
app/contexts/GlobalState.tsx (1)
useGlobalState(150-156)hooks/use-mobile.ts (1)
useIsMobile(5-21)components/ui/button.tsx (1)
Button(59-59)
components/ui/skeleton.tsx (1)
lib/utils.ts (1)
cn(8-10)
components/ui/sheet.tsx (1)
lib/utils.ts (1)
cn(8-10)
components/ui/input.tsx (1)
lib/utils.ts (1)
cn(8-10)
app/components/ScrollToBottomButton.tsx (1)
app/contexts/GlobalState.tsx (1)
useGlobalState(150-156)
components/ui/separator.tsx (1)
lib/utils.ts (1)
cn(8-10)
app/components/ChatSidebar.tsx (2)
hooks/use-mobile.ts (1)
useIsMobile(5-21)app/contexts/GlobalState.tsx (1)
useGlobalState(150-156)
app/components/ChatHeader.tsx (2)
app/contexts/GlobalState.tsx (1)
useGlobalState(150-156)components/icons/hackerai-svg.tsx (1)
HackerAISVG(8-67)
app/contexts/GlobalState.tsx (1)
types/chat.ts (1)
SidebarContent(24-24)
app/components/chat.tsx (11)
hooks/use-mobile.ts (1)
useIsMobile(5-21)app/contexts/GlobalState.tsx (1)
useGlobalState(150-156)convex/messages.ts (2)
deleteLastAssistantMessage(175-217)saveMessageFromClient(123-170)lib/utils.ts (2)
convertToUIMessages(34-42)fetchWithErrorHandlers(12-32)lib/utils/message-processor.ts (1)
normalizeMessages(50-130)lib/utils/todo-utils.ts (1)
mergeTodos(11-37)lib/errors.ts (1)
ChatSDKError(36-74)app/hooks/useMessageScroll.ts (1)
useMessageScroll(4-22)app/components/Messages.tsx (1)
Messages(22-170)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/ScrollToBottomButton.tsx (1)
ScrollToBottomButton(11-42)
components/ui/avatar.tsx (1)
lib/utils.ts (1)
cn(8-10)
convex/chats.ts (1)
lib/db/actions.ts (3)
getChatById(8-18)saveChat(20-39)updateChat(116-142)
app/components/UserDropdownMenu.tsx (2)
components/ui/dropdown-menu.tsx (6)
DropdownMenu(242-242)DropdownMenuTrigger(244-244)DropdownMenuContent(245-245)DropdownMenuLabel(247-247)DropdownMenuSeparator(252-252)DropdownMenuItem(248-248)components/ui/avatar.tsx (3)
Avatar(53-53)AvatarImage(53-53)AvatarFallback(53-53)
components/ui/sidebar.tsx (2)
hooks/use-mobile.ts (1)
useIsMobile(5-21)lib/utils.ts (1)
cn(8-10)
🔇 Additional comments (18)
package.json (2)
14-16: Version alignment across AI SDK packages.You updated
@ai-sdk/openai,@ai-sdk/react, andai. Ensure their peer dependency ranges are compatible to avoid subtle runtime mismatch.If you see peer dep warnings locally, align versions per their release notes; otherwise, you’re good.
Also applies to: 30-31, 33-36, 37-37
17-17: Verify @e2b/code-interpreter v2.0.0 API UsageI scanned the codebase for legacy symbols (
CodeInterpreter,createWorkspace,E2B) and found none. Instead, the following named imports from@e2b/code-interpreterremain:
Sandboxin
•lib/ai/tools/index.ts:1
•lib/ai/tools/utils/sandbox.ts:1
•lib/ai/tools/utils/sandbox-utils.ts:1
•lib/ai/tools/utils/sandbox-manager.ts:1
•types/agent.ts:1CommandExitErrorinlib/ai/tools/run-terminal-cmd.ts:3CommandResultinapp/components/tools/TerminalToolHandler.tsx:3Please confirm that in v2.0.0:
Sandbox,CommandExitError, andCommandResultare still exported as named symbols.- Their constructors, method signatures, and behaviors haven’t changed in ways that could break existing usage.
app/contexts/GlobalState.tsx (2)
111-114: toggleChatSidebar implementation looks good.Simple state flip with functional update avoids stale closures. No concerns.
85-91: Consider whether resetting chat should also close sidebars.
resetChat()clears input/title/todos/UI panel state but leaves both sidebars as-is. If “Reset Chat” is intended to return to a clean slate, you may also want to close the right Computer sidebar and the left Chat sidebar.Would you like a follow-up PR to include
closeSidebar()andsetChatSidebarOpen(false)insideresetChat()?components/ui/sheet.tsx (3)
75-78: Verify “rounded-xs” exists in your Tailwind theme.
rounded-xsisn’t a default radius in Tailwind. If not extended, this will be a no-op. Considerrounded-smor extend the theme.- <SheetPrimitive.Close className="... rounded-xs ..."> + <SheetPrimitive.Close className="... rounded-sm ...">If you intentionally extended
rounded-xs, ignore this.
31-45: Overlay: good defaults and animations.Backdrop has correct z-index and state-driven transitions. Looks consistent with the rest of the UI primitives.
47-82: Export intent: do you want to re-use Overlay/Portal externally?
SheetOverlayandSheetPortalare defined but not exported. If you plan to compose custom sheet variants elsewhere, exporting them now avoids one-off redefinitions.export { Sheet, SheetTrigger, SheetClose, SheetContent, SheetHeader, SheetFooter, SheetTitle, SheetDescription, + SheetOverlay, + SheetPortal, };hooks/use-mobile.ts (1)
20-21: Hook behavior is sensible.Returning
!!isMobileavoids undefined consumers pre-hydration. Keep as-is.app/components/UserDropdownMenu.tsx (1)
96-99: DropdownMenuItem’svariantprop is supported—no changes neededI’ve verified in
components/ui/dropdown-menu.tsxthatDropdownMenuItem’s props includevariant?: "default" | "destructive";and that it applies
data-[variant=destructive]styles accordingly. The existing usage withvariant="destructive"will type-check and render correctly.components/ui/avatar.tsx (1)
8-21: LGTM — clean wrappers and sensible defaultsNice, minimal Radix wrappers with clear data-slot hooks and class merging.
Also applies to: 24-35, 37-51
convex/chats.ts (3)
141-166: Nice: minimal delta updates and consistent timestampingThe prepare-and-patch approach updates only provided fields and stamps update_time once. Clear and correct.
181-219: Good use of identity gate for user-scoped listinggetUserChats correctly requires identity and returns a scoped projection.
1-34: Return typing is precise and future-proofedExplicitly describing the returned chat shape (including optional finish_reason and todos) improves client DX and guards against accidental field omissions.
components/ui/sidebar.tsx (5)
154-181: Solid desktop/mobile branching and collapsible variantsThe split between mobile Sheet and desktop fixed container is clean, and the collapsible="none|offcanvas|icon" behavior reads well.
256-279: Accessible trigger with screen-reader label and small target sizeGood use of sr-only label and a compact icon button. The onClick delegates and toggleSidebar abstraction are tidy.
602-638: Nice skeleton UX detailsRandomized skeleton width and optional icon make loading states feel less repetitive.
498-546: Tooltip behavior is considerate on collapsed-only and desktop-onlyShowing tooltips only when collapsed and hiding them on mobile avoids noisy overlays.
221-239: Verify Tailwind configuration for custom arbitrary-value syntaxI ran searches but didn’t locate a
tailwind.config.*file or any plugin enabling thew-(--…)shorthand. Please confirm whether your Tailwind setup supports this syntax. If not, update the class names to use standard arbitrary values withvar(), for example:- className={cn( - "relative w-(--sidebar-width) bg-transparent transition-[width] duration-200 ease-linear", + className={cn( + "relative w-[var(--sidebar-width)] bg-transparent transition-[width] duration-200 ease-linear", "group-data-[collapsible=offcanvas]:w-0", "group-data-[side=right]:rotate-180", variant === "floating" || variant === "inset" ? "group-data-[collapsible=icon]:w-[calc(var(--sidebar-width-icon)+(--spacing(4)))]" : "group-data-[collapsible=icon]:w-(--sidebar-width-icon)",And similarly for
"w-(--sidebar-width-icon)"elsewhere.
• If you do have a plugin or custom parser that supportsw-(--…), could you point to its configuration?
• Otherwise, please switch these tow-[var(--...)]to ensure Tailwind compiles as expected.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/ChatInput.tsx (1)
82-87: Fix Enter-to-send type mismatch and unify submit paths via requestSubmit()onKeyDown passes a KeyboardEvent into a handler typed for FormEvent, which is unsafe and can break TS in strict mode. Use a form ref and HTMLFormElement.requestSubmit() to trigger the same onSubmit flow for both Enter and the send button.
Apply this diff:
@@ -import { Button } from "@/components/ui/button"; +import { Button } from "@/components/ui/button"; +import { useRef } from "react"; @@ }: ChatInputProps) => { const { input, setInput, mode, setMode } = useGlobalState(); const isGenerating = status === "submitted" || status === "streaming"; + const formRef = useRef<HTMLFormElement>(null); @@ - onKeyDown={(e) => { + onKeyDown={(e) => { if (e.key === "Enter" && !e.shiftKey) { e.preventDefault(); - handleSubmit(e); + // Trigger the form's submit so onSubmit receives a proper FormEvent + if (status === "ready" && input.trim()) { + formRef.current?.requestSubmit(); + } } }} @@ - <form onSubmit={handleSubmit}> + <form ref={formRef} onSubmit={handleSubmit}>Also applies to: 163-181, 1-21, 36-38
♻️ Duplicate comments (1)
app/components/chat.tsx (1)
171-191: Don’t clobber streaming assistant messages with DB snapshots (repeat of earlier feedback)While streaming, real-time DB updates can overwrite the in-progress assistant content, causing flicker. Gate the sync when status === "streaming" and include status in deps.
Apply this diff:
useEffect(() => { if (!messagesData || messagesData === null) return; + // Avoid replacing in-progress assistant streaming with a DB snapshot + if (status === "streaming") return; const uiMessages = convertToUIMessages(messagesData); @@ - }, [messagesData, setMessages, messages]); + }, [messagesData, status, setMessages, messages]);
🧹 Nitpick comments (5)
app/contexts/GlobalState.tsx (2)
90-95: Consider whether resetChat should also reset currentChatId (optional)Some flows may expect a “fresh” chat after reset. If that’s desired, either clear currentChatId here or add an option to resetChat to preserve/clear the id explicitly.
Example:
-const resetChat = () => { +const resetChat = (opts: { preserveChatId?: boolean } = {}) => { setInput(""); setChatTitle(null); setTodos([]); setIsTodoPanelExpanded(false); + if (!opts.preserveChatId) setCurrentChatId(null); };
120-148: Memoize provider value to reduce unnecessary re-renders (optional)The context value is recreated every render, which can re-render all consumers. Wrap it in useMemo keyed by its fields.
-const value: GlobalStateType = { +const value: GlobalStateType = useMemo(() => ({ input, setInput, mode, setMode, chatTitle, setChatTitle, currentChatId, setCurrentChatId, sidebarOpen, setSidebarOpen, sidebarContent, setSidebarContent, chatSidebarOpen, setChatSidebarOpen, todos, setTodos, mergeTodos, isTodoPanelExpanded, setIsTodoPanelExpanded, clearInput, resetChat, openSidebar, updateSidebarContent, closeSidebar, toggleChatSidebar, -}; +}), [ + input, mode, chatTitle, currentChatId, sidebarOpen, sidebarContent, + chatSidebarOpen, todos, isTodoPanelExpanded, + // stable fns can be kept out if wrapped in useCallback +]);app/components/ChatItem.tsx (1)
9-16: Use isActive as an override (or remove it) and improve a11yThe isActive prop is accepted but not used; active state is derived only from currentChatId. Either drop the prop or allow it to override. Also, expose aria-current on the active item for screen readers.
Apply this diff:
-interface ChatItemProps { +interface ChatItemProps { id: string; title: string; - isActive?: boolean; + isActive?: boolean; // optional override } @@ -const ChatItem: React.FC<ChatItemProps> = ({ id, title, isActive = false }) => { +const ChatItem: React.FC<ChatItemProps> = ({ id, title, isActive }) => { @@ - const isCurrentlyActive = currentChatId === id; + const isCurrentlyActive = isActive ?? (currentChatId === id); @@ <Button variant="ghost" onClick={handleClick} + aria-current={isCurrentlyActive ? "page" : undefined} className={`flex items-center gap-2 p-3 h-auto w-full text-left justify-start ${ isCurrentlyActive ? "bg-sidebar-accent text-sidebar-accent-foreground" : "hover:bg-sidebar-accent/50" }`} title={title} >Also applies to: 26-29, 41-56
app/hooks/useChatHandlers.ts (1)
91-102: Add error handling around deleteLastAssistantMessage to avoid blocking regenerateIf the delete fails, regenerate should still proceed; surface a non-blocking error.
- const handleRegenerate = async () => { - // Remove the last assistant message from the UI and database - await deleteLastAssistantMessage({ chatId }); - - regenerate({ + const handleRegenerate = async () => { + try { + // Remove the last assistant message from the UI and database + await deleteLastAssistantMessage({ chatId }); + } catch (err) { + console.error("Failed to delete last assistant message:", err); + // non-blocking: proceed anyway + } + regenerate({ body: { mode, todos, regenerate: true, }, }); };app/components/chat.tsx (1)
226-233: Bind SidebarProvider open state to chatSidebarOpen (optional)SidebarProvider is forced open with a no-op onOpenChange, while the container already animates based on chatSidebarOpen. Keep them consistent.
- <SidebarProvider - open={true} - onOpenChange={() => {}} - defaultOpen={true} - > + <SidebarProvider + open={chatSidebarOpen} + onOpenChange={setChatSidebarOpen} + defaultOpen={chatSidebarOpen} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
app/c/[id]/page.tsx(1 hunks)app/components/ChatInput.tsx(1 hunks)app/components/ChatItem.tsx(1 hunks)app/components/ChatSidebar.tsx(1 hunks)app/components/UserDropdownMenu.tsx(1 hunks)app/components/chat.tsx(1 hunks)app/contexts/GlobalState.tsx(5 hunks)app/hooks/useChatHandlers.ts(1 hunks)convex/chats.ts(1 hunks)convex/messages.ts(1 hunks)convex/schema.ts(1 hunks)lib/db/actions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- app/components/ChatSidebar.tsx
- app/c/[id]/page.tsx
- app/components/UserDropdownMenu.tsx
- convex/schema.ts
- lib/db/actions.ts
- convex/messages.ts
- convex/chats.ts
🧰 Additional context used
🧬 Code graph analysis (4)
app/hooks/useChatHandlers.ts (3)
types/chat.ts (1)
ChatMessage(58-58)app/contexts/GlobalState.tsx (1)
useGlobalState(157-163)convex/messages.ts (2)
deleteLastAssistantMessage(165-208)saveMessageFromClient(117-160)
app/components/chat.tsx (12)
hooks/use-mobile.ts (1)
useIsMobile(5-21)app/contexts/GlobalState.tsx (1)
useGlobalState(157-163)lib/ai/tools/utils/todo-manager.ts (1)
setTodos(32-72)lib/utils.ts (2)
convertToUIMessages(34-42)fetchWithErrorHandlers(12-32)lib/utils/message-processor.ts (1)
normalizeMessages(50-130)lib/utils/todo-utils.ts (1)
mergeTodos(11-37)lib/errors.ts (1)
ChatSDKError(36-74)app/hooks/useMessageScroll.ts (1)
useMessageScroll(4-22)app/hooks/useChatHandlers.ts (1)
useChatHandlers(19-109)app/components/Messages.tsx (1)
Messages(22-170)app/components/ChatInput.tsx (1)
ChatInput(30-188)app/components/ScrollToBottomButton.tsx (1)
ScrollToBottomButton(11-42)
app/contexts/GlobalState.tsx (2)
types/chat.ts (1)
SidebarContent(24-24)components/ui/sidebar.tsx (1)
SidebarContent(703-703)
app/components/ChatItem.tsx (2)
app/contexts/GlobalState.tsx (1)
useGlobalState(157-163)hooks/use-mobile.ts (1)
useIsMobile(5-21)
🔇 Additional comments (3)
app/components/ChatInput.tsx (1)
64-64: Spacing tweak looks goodDropping the extra margin (mb-4) here should reduce double-spacing with the reworked layout. No functional concerns.
app/contexts/GlobalState.tsx (1)
27-30: State additions for chat sidebar and currentChatId look solidThe new fields (currentChatId, chatSidebarOpen, toggleChatSidebar) are coherently initialized and exposed. Naming and responsibilities are clear.
Also applies to: 37-40, 56-56, 73-73, 78-78, 116-119, 127-135
app/hooks/useChatHandlers.ts (1)
72-89: Verify server persistence for aborted messagesI reviewed the Next.js API route at
app/api/chat/route.tsand confirmed that assistant messages are persisted on the server in theonFinishhandler of the UI message stream:writer.merge( result.toUIMessageStream({ generateMessageId: uuidv4, onFinish: async ({ messages }) => { for (const message of messages) { await saveMessage({ chatId, message }); } }, }), );However, the
onAborthandler only logs the abort and does not callsaveMessageorupdateChat. There is nosaveMessageFromClienton the server side. Therefore, before removing or adjusting the client fallback inuseChatHandlers.ts, please:
- Confirm whether
toUIMessageStream’sonFinishis invoked when the stream is aborted (i.e. does the server persist partial/aborted streams).- If aborted streams are persisted server-side, you can safely remove the client-side save (Option A) to avoid duplication and race conditions.
- If not, keep the client fallback but apply Option B: persist before calling
stop(), and normalize the message parts vianormalizeMessages.Please verify the stream-abort persistence behavior to decide between Options A and B.
rossmanko
changed the title
Summary by CodeRabbit
New Features
Authentication
Performance/Behavior
Documentation