Repository navigation
fix(global-chat): lazy conversation creation — stop polluting history with empty chats - #1645
Conversation
📝 WalkthroughWalkthroughThis PR implements lazy conversation creation: conversations are now auto-created on the first message instead of beforehand. A new API helper validates ownership; repositories filter by message activity and omit ChangesLazy Conversation Creation and Ownership Validation
Sequence Diagram(s)sequenceDiagram
participant Client as Client Browser
participant Context as GlobalChatContext
participant ClientState as conversation-state
participant APIRoute as POST /api/ai/global/[id]/messages
participant Helper as resolveOrCreateConversation
participant DB as Conversations DB
Client->>Context: load conversation (first time or 404)
Context->>ClientState: createAndSetActiveConversation(type)
ClientState->>ClientState: createId() → CUID
ClientState->>ClientState: Set activeConversationId cookie
ClientState-->>Context: { id, type, title: null, lastMessageAt: null }
Context->>APIRoute: POST messages { conversationId, userId, ... }
APIRoute->>Helper: resolveOrCreateConversation(userId, conversationId)
Helper->>DB: SELECT WHERE id AND isActive
alt Conversation exists & user owns it
DB-->>Helper: existing conversation row
Helper-->>APIRoute: resolved row
else Conversation exists & different user
DB-->>Helper: row with different userId
Helper-->>APIRoute: reject ConversationOwnershipError
APIRoute-->>Context: 404 Conversation not found
Context->>Context: classifyConversationLoadResponse(404) → 'not-found'
Context->>Context: Set initialMessages=[], still set currentConversationId
else No conversation exists yet
DB-->>Helper: no rows
Helper->>DB: INSERT new global conversation
DB-->>Helper: inserted row
Helper-->>APIRoute: new conversation row
APIRoute-->>Context: 200 OK with messages
Context->>Context: classifyConversationLoadResponse(200) → 'ok'
Context->>Context: Parse messages, set initialMessages, set currentConversationId
end
Context-->>Client: Show messages or empty state, ready for first message
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fac1c8e374
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .where(and( | ||
| eq(conversations.id, conversationId), | ||
| eq(conversations.isActive, true), | ||
| )) |
There was a problem hiding this comment.
When a user reloads a bookmarked URL or stale cookie for a conversation that was soft-deleted, this isActive predicate hides the existing row, so the code proceeds to insert another row with the same primary key. The first message then fails with a database uniqueness error and a 500 response instead of rejecting the deleted conversation or assigning a fresh ID; query by ID regardless of activity before deciding to insert.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Acknowledged as P2. The onConflictDoNothing() fallback SELECT does return the soft-deleted row if ownership matches, which would allow a message to attach to a soft-deleted conversation. Fixing soft-deleted ID reuse correctly requires either (a) checking winner.isActive in the fallback and returning a new error, or (b) removing isActive from the initial SELECT. Deferring as a follow-up since this is an edge case with no current code path that soft-deletes global conversations by ID, and the blast radius here is isolated to a single user reusing a stale cookie.
| const [created] = await db | ||
| .insert(conversations) | ||
| .values({ | ||
| id: conversationId, |
There was a problem hiding this comment.
When two first-message POSTs for the same locally generated conversation ID overlap—for example from two tabs or the dashboard and sidebar—both requests can observe no existing row and reach this insert. One insert succeeds while the other violates the conversations.id primary key and loses its message with a 500 response; use an atomic conflict-safe insert followed by an ownership-aware reselect.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in commit 7e6f1f7: the insert now uses .onConflictDoNothing(), and when that returns an empty result (a concurrent write won the race), we fall back to a SELECT to find the winning row. The race is fully handled — no 500 possible from a PK conflict.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
apps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.ts (1)
106-111: ⚡ Quick winAdd a route-level test for
ConversationOwnershipError→ 404 mapping.You now mock
ConversationOwnershipError, but this suite doesn’t assert the new POST behavior when resolver ownership fails. Add one case to verify 404 response and no stream/lifecycle start.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/credit-gate.test.ts around lines 106 - 111, Add a test case that when the mocked resolveOrCreateConversation throws/returns a ConversationOwnershipError the POST handler responds with HTTP 404 and does not initiate any stream or lifecycle. Specifically, in the existing test suite (where resolveOrCreateConversation and ConversationOwnershipError are mocked), make resolveOrCreateConversation reject with new ConversationOwnershipError(), call the POST route the same way other tests do, assert the response status is 404, and assert that the mocks for starting the stream and starting the lifecycle (e.g., startStream / startConversationLifecycle or the existing stream/lifecycle mock functions used elsewhere in this file) were not called.apps/web/src/app/api/ai/global/[id]/messages/__tests__/resolve-or-create-conversation.test.ts (1)
74-79: ⚡ Quick winCurrent idempotency test does not cover the real conflict path.
Line 74 validates sequential re-entry, but it won’t catch the concurrent unique-key race on insert. Add a case that simulates insert conflict and verifies fallback resolution behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/resolve-or-create-conversation.test.ts around lines 74 - 79, Add a new test that simulates a concurrent unique-key insert conflict to exercise the real idempotency path: in the test, prepare the fake db from makeDb([CONV]) and stub db.insert to throw a unique-constraint/conflict error on the first call (e.g., an error object with code or message matching the DB client’s unique violation), then call resolveOrCreateConversation('user1','conv1', db) and assert it does not rethrow and that the function returns or resolves to the existing CONV and that db.insert was attempted exactly once; ensure the test references resolveOrCreateConversation, db.insert stub, and the existing CONV fixture so it reproduces the insert-conflict fallback behavior.apps/web/src/lib/ai/core/__tests__/conversation-state.test.ts (1)
38-41: ⚡ Quick winAlign the “no network fetch” test name with actual assertions.
This case currently asserts only
post, notfetchWithAuth.Proposed test hardening
-import { post } from '`@/lib/auth/auth-fetch`'; +import { post, fetchWithAuth } from '`@/lib/auth/auth-fetch`'; it('does NOT call post() or any network fetch', async () => { await conversationState.createAndSetActiveConversation({ type: 'global' }); expect(post).not.toHaveBeenCalled(); + expect(fetchWithAuth).not.toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/web/src/lib/ai/core/__tests__/conversation-state.test.ts` around lines 38 - 41, Test name promises "no network fetch" but only asserts post was not called; update the test for conversationState.createAndSetActiveConversation to either rename it to only mention post or, preferably, add the missing assertion by also checking expect(fetchWithAuth).not.toHaveBeenCalled(); so both post and fetchWithAuth are asserted as not called after calling createAndSetActiveConversation({ type: 'global' }).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@apps/web/src/app/api/ai/global/`[id]/messages/resolve-or-create-conversation.ts:
- Around line 23-25: Add server-side validation for the client-supplied
conversationId in the resolveOrCreateConversation function: before any DB
access, check that conversationId is non-empty, length <= 32, and matches the
CUID2-based regex ^[a-z][a-z0-9]{1,31}$; if the value fails, short-circuit
(throw a validation error or return a 4xx-style failure) so invalid IDs never
reach storage. Ensure the check runs at the top of the function handling the
parameters (userId, conversationId, db) and use the explicit regex and max
length constraint in the condition.
- Around line 36-38: The code currently reuses an existing conversation row
without checking its type; update the early-return block that inspects existing
and userId to also validate the conversation type (existing.type) equals
'global' before returning. If existing.type !== 'global', do not reuse that row:
either throw a ConversationTypeError (create this error class if it doesn't
exist) or fall through to the existing create-new-conversation path so a new
global conversation is created; keep the ownership check (if (existing.userId
!== userId) throw new ConversationOwnershipError()) and add the type check
immediately after it.
- Around line 41-49: The blind insert of conversations
(db.insert(conversations).values({ id: conversationId, userId, type: 'global',
isActive: true }).returning()) can race and throw a unique-key error; make the
creation idempotent by using an upsert/do-nothing on conflict and falling back
to a select: change the insert to use an ON CONFLICT DO NOTHING variant (e.g.
db.insert(conversations).values(...).onConflict((oc) =>
oc.column('id').doNothing()).returning()), then if the returned row is empty,
query the existing conversation by id (e.g.
db.select().from(conversations).where(eq(conversations.id, conversationId))).
Ensure the code that expects the created result uses the selected row when
onConflict yields no returning row so concurrent first-writes resolve to a
single conversation instead of raising a 500.
In `@apps/web/src/lib/ai/core/__tests__/conversation-state.test.ts`:
- Around line 49-52: The test in conversation-state.test.ts uses a
too-permissive regex for CUID2; update the assertion in the it('returns id
matching CUID2 format'...) test that checks result.id (from
conversationState.createAndSetActiveConversation) to use the canonical CUID2
shape by replacing /^[a-z0-9]{24,}$/ with /^[a-z][a-z0-9]{1,31}$/, ensuring IDs
start with a lowercase letter, contain only lowercase alphanumerics, and are at
most 32 chars.
In
`@apps/web/src/lib/repositories/__tests__/global-conversation-list-filter.test.ts`:
- Around line 87-89: The test description says "exactly once" but the assertion
uses expect(existsCallCountAtModuleLoad).toBeGreaterThanOrEqual(1); change this
to assert exact equality by replacing the assertion with
expect(existsCallCountAtModuleLoad).toBe(1) (or .toEqual(1)) so that the test
enforces a single call to exists() during module load; locate the assertion in
the test named "calls exists() exactly once when the module loads (to build
hasMessages)" and update it accordingly.
---
Nitpick comments:
In `@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/credit-gate.test.ts:
- Around line 106-111: Add a test case that when the mocked
resolveOrCreateConversation throws/returns a ConversationOwnershipError the POST
handler responds with HTTP 404 and does not initiate any stream or lifecycle.
Specifically, in the existing test suite (where resolveOrCreateConversation and
ConversationOwnershipError are mocked), make resolveOrCreateConversation reject
with new ConversationOwnershipError(), call the POST route the same way other
tests do, assert the response status is 404, and assert that the mocks for
starting the stream and starting the lifecycle (e.g., startStream /
startConversationLifecycle or the existing stream/lifecycle mock functions used
elsewhere in this file) were not called.
In
`@apps/web/src/app/api/ai/global/`[id]/messages/__tests__/resolve-or-create-conversation.test.ts:
- Around line 74-79: Add a new test that simulates a concurrent unique-key
insert conflict to exercise the real idempotency path: in the test, prepare the
fake db from makeDb([CONV]) and stub db.insert to throw a
unique-constraint/conflict error on the first call (e.g., an error object with
code or message matching the DB client’s unique violation), then call
resolveOrCreateConversation('user1','conv1', db) and assert it does not rethrow
and that the function returns or resolves to the existing CONV and that
db.insert was attempted exactly once; ensure the test references
resolveOrCreateConversation, db.insert stub, and the existing CONV fixture so it
reproduces the insert-conflict fallback behavior.
In `@apps/web/src/lib/ai/core/__tests__/conversation-state.test.ts`:
- Around line 38-41: Test name promises "no network fetch" but only asserts post
was not called; update the test for
conversationState.createAndSetActiveConversation to either rename it to only
mention post or, preferably, add the missing assertion by also checking
expect(fetchWithAuth).not.toHaveBeenCalled(); so both post and fetchWithAuth are
asserted as not called after calling createAndSetActiveConversation({ type:
'global' }).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65c1f1fc-ef6f-40f5-a019-0d5c8c7ace6e
📒 Files selected for processing (11)
apps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/resolve-or-create-conversation.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/resolve-or-create-conversation.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/contexts/GlobalChatContext.tsxapps/web/src/lib/ai/core/__tests__/conversation-state.test.tsapps/web/src/lib/ai/core/conversation-state.tsapps/web/src/lib/repositories/__tests__/global-conversation-hard-delete.test.tsapps/web/src/lib/repositories/__tests__/global-conversation-list-filter.test.tsapps/web/src/lib/repositories/global-conversation-repository.ts
|
Addressed all 5 actionable + 3 nitpick CodeRabbit comments in commit Actionable comments fixed:
Nitpick comments addressed:
Also fixed (CI was failing): |
…de, no eager DB write
On every new-chat action or app init, a POST to /api/ai/global created a DB row
immediately (with lastMessageAt=now), polluting conversation history with empty
untitled entries. The useChat({ id }) prop is a React state key only — it does not
require a DB record before the first message.
- conversation-state: createAndSetActiveConversation generates a CUID2 locally
(no network call); sets cookie as before; returns { id, type, title:null, lastMessageAt:null }
- Extract classifyConversationLoadResponse pure fn (ok/not-found/error) — testable
without React; 15 unit tests green
- GlobalChatContext.loadConversation: 404 now treated as empty (not an error),
so a locally generated ID survives a page reload before the first message is sent
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…sert) Extracted resolveOrCreateConversation() — pure-ish function with 4 unit tests. The messages route now calls it instead of the previous 404-on-missing-row check. When a locally generated conversation ID arrives with no prior DB row, the row is created here (with correct userId/type/isActive; lastMessageAt set later by the existing update after the message is saved). If the row exists but belongs to a different user, ConversationOwnershipError → 404. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…EXISTS subquery Adds hasMessages EXISTS subquery to listConversations and listConversationsPaginated so conversations with no active messages are hidden from history. Also fixes getActiveGlobalConversation ordering to prefer most-recently-messaged conversation (lastMessageAt DESC NULLS LAST) and removes the premature lastMessageAt=now that was set on conversation creation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ion in route tests Three test files mocked @pagespace/db/operators without `exists`, which now throws at module load when global-conversation-repository.ts initialises hasMessages. Also stub resolveOrCreateConversation in route integration tests — it has its own unit test file; the route tests shouldn't re-test its internals. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion, CI fix
- GlobalChatContext: use response.ok + status===404 directly (fixes CI — test
mocks return {ok:true} without .status; calling classifyConversationLoadResponse
on an undefined status caused the not-found path to never fire)
- resolveOrCreateConversation: add CUID2 regex validation before DB access;
verify existing.type==='global' before reusing; make insert idempotent with
ON CONFLICT DO NOTHING + fallback select for concurrent first-writes
- Tests: update CUID2 regex to canonical /^[a-z][a-z0-9]{1,31}$/; add fetchWithAuth
assertion; fix exists() call count to toBe(1); add ConversationOwnershipError→404
test to credit-gate suite; add race-condition and type-mismatch tests to
resolve-or-create-conversation suite
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
711389c to
7e6f1f7
Compare
Problem
Every time a user opened the Global Assistant or clicked "New Chat", the client eagerly called
POST /api/ai/globalto create aconversationsrow — even if the user never sent a message. Because history is ordered bylastMessageAt DESCwith no message-existence filter, these stale rows appeared in the sidebar. Users with frequent new-chat clicks ended up with dozens of untitled, empty entries — making history nearly unusable.AI SDK fact (no hallucination)
useChat({ id })uses theidprop as a client-side React state key only. The conversation ID reaches the server via the URL path (/api/ai/global/[id]/messages), not theidprop. Source:apps/web/src/lib/ai/core/useChatTransport.ts+GlobalChatContext.tsx:266-283. The SDK does NOT require a DB-backed record before the first message is sent.Solution — two-pronged
1. Lazy client-side ID generation (
conversation-state.ts)createAndSetActiveConversation()now generates a CUID2 locally — noPOST /api/ai/globalcallclassifyConversationLoadResponse(status)maps status codes →'ok' | 'not-found' | 'error'(tested in isolation)GlobalChatContexthandles 404 onGET /messagesas lazy-creation (empty messages, not an error)2. Server-side upsert on first message (
messages/route.ts)resolveOrCreateConversation(userId, conversationId, db?)— fully isolated and testableconversationIdagainst CUID2 regex before any DB accessexisting.type === 'global'before reusing a rowON CONFLICT DO NOTHING+ fallback SELECTConversationOwnershipError(caught at route boundary → 404) for invalid ID, ownership mismatch, or type mismatch3. EXISTS filter on history (
global-conversation-repository.ts)hasMessagesconstant:exists(db.select({one:sql\1`}).from(messages).where(...isActive...))— filters both null-lastMessageAt` rows AND any pre-existing stale rowslistConversationsandlistConversationsPaginatedgetActiveGlobalConversationordering fixed tolastMessageAt DESC NULLS LAST, createdAt DESC— reloading drops you back into the most recently active conversation, not a blank stubcreateConversationno longer setslastMessageAt: now(stays null until first message saved)Tests
All new code is TDD (RED→GREEN):
__tests__/conversation-state.test.ts— 15 tests:classifyConversationLoadResponse+createAndSetActiveConversation(including CUID2 format, no network calls, cookie persistence)resolve-or-create-conversation/__tests__/— 7 tests: existing, create, ownership error, idempotent re-entry, invalid CUID2, wrong-type conversation, concurrent-insert race condition__tests__/global-conversation-list-filter.test.ts— 7 tests: EXISTS filter present, result correctness, ordering args, null on empty,exists()called exactly once at module load__tests__/credit-gate.test.ts— added test:ConversationOwnershipError→ 404, no stream/lifecycle startedVerification
🤖 Generated with Claude Code