fix(ai-chat): close conversation-identity race causing sends to land in the wrong conversation#1849
Merged
Conversation
Sending a message to an existing conversation could create a new one instead of appending, and vice versa, because conversationId was tracked independently in several places and only updated after an async create/switch round trip resolved -- a send fired mid-transition would carry a stale id. - Add a pure conversation-identity state machine (idle/resolving/ready/error) as the shared primitive later phases will wire in. - Generate conversation ids client-side (cuid2) in useConversations.ts and usePageAgentDashboardStore.ts instead of waiting on the server response; the create POST becomes a fire-and-forget idempotent persist. The page-agents route now accepts an optional client-supplied conversationId. - Fix two regressions caught in review: the global-chat branch of createConversation no longer diverges from the server-generated id (that route doesn't support a client-supplied id), and loadMostRecentConversation's fallback to createNewConversation no longer leaves isConversationLoading stuck true. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
One hook to own the conversationIdentityReducer + dispatch-on-mount wiring, so AiChatView/GlobalAssistantView/GlobalChatContext each call it with their own resolve() function instead of hand-rolling the same useReducer + effect boilerplate three times over. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
Replaces the ad hoc currentConversationId/isInitialized useState pair with state derived from useConversationIdentity, closing the two remaining causes of the conversation-identity race in this surface: - New Chat and history-select now call setIdentity synchronously — history-select no longer waits for useConversations.loadConversation's messages fetch to resolve before adopting the selected id, which was the last window where an immediate send could land under the wrong conversation. - resolveConversation (passed to useConversationIdentity) distinguishes a genuine empty conversation list from a thrown fetch failure — a transient network error now surfaces a retry banner instead of silently falling back to the page-scoped placeholder id, fixing the "existing conversation treated as new" symptom at its second root cause. Also keys AiChatView by page.id in CenterPanel.tsx, matching the existing pattern already used for DocumentView/CodePageView/ TerminalView/CanvasPageView/SheetView. AiChatView can receive a new page prop without unmounting; remounting is the idiomatic way to reset a state machine for a new subject, so this replaces a manual "reset state when page.id changes" effect rather than adding a resetKey/RESET escape hatch to the reducer itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
…rfaces Completes the conversation-identity race fix across the remaining surfaces (GlobalAssistantView agent mode, GlobalChatContext global mode, SidebarChatTab verified as inheriting correctly). - usePageAgentDashboardStore: conversationId/isConversationLoading are now derived fields kept in sync by routing every transition through conversationIdentityReducer as a plain function call inside store actions (Zustand stores aren't React components, so this is the correct application of the shared reducer, not the useConversationIdentity hook). loadConversation/createNewConversation dispatch IDENTITY_SET synchronously; loadMostRecentConversation is now provably immune to the exact stale-clobber race code review caught earlier (a slow loadMostRecentConversation resolving after a newer createNewConversation/loadConversation can no longer overwrite it — the reducer ignores RESOLVED/RESOLVE_FAILED once state has moved past 'resolving'). - GlobalChatContext: same reducer wired via a local useReducer in the provider (correct here since it's a single instance shared by every consumer, not duplicated per-consumer). loadConversation adopts an id synchronously instead of only after its messages fetch resolves. - GlobalAssistantView: disabled now includes !isInitialized, closing a gap review flagged (isConversationLoading was computed into agentIsInitialized but never wired into the send-disabled state). - Added conversationIdFrom/isResolving pure selectors to conversation-identity.ts, shared by all three surfaces plus AiChatView (retrofitted to use them instead of inline ternaries). 164/164 tests green across every file this epic touched; full apps/web typecheck clean; 736/740 test files green project-wide (4 pre-existing, unrelated failures). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
Contributor
📝 WalkthroughWalkthroughThis PR introduces a shared conversation-identity state machine ( ChangesConversation Identity Refactor
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AiChatView
participant IdentityHook as useConversationIdentity
participant Store as ConversationStore/Context
participant API
User->>AiChatView: create/select conversation
AiChatView->>IdentityHook: setIdentity(clientGeneratedId)
IdentityHook-->>AiChatView: state = ready(id)
AiChatView->>Store: persist conversation (async)
AiChatView->>API: fetch messages(id)
API-->>AiChatView: messages
AiChatView->>AiChatView: loadMessagesForConversation (stale-guarded)
Store-->>AiChatView: persistence result (ignored if stale)
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: d72693ef8e
ℹ️ 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".
Comment on lines
+1118
to
+1119
| setIdentity(conversationId); | ||
| void loadConversation(conversationId); |
There was a problem hiding this comment.
When selecting a history item, setIdentity() updates currentConversationId before anything sets skipLoadEffectRef, so the currentConversationId effect immediately calls loadMessagesForConversation() while loadConversation() below is already fetching the same messages. On a slow or flaky request, one fetch can succeed and the other can later fail, leaving messagesLoadError displayed over successfully loaded messages; it also doubles the message-load request for every history selection. Set the skip/in-flight marker before adopting the identity, or make the effect ignore IDs already being loaded by useConversations.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 564c0de: handleSelectConversation now sets skipLoadEffectRef.current = conversationId before calling setIdentity, so the load-on-select effect sees the skip token already in place when it reacts to the identity change and no longer fires its own redundant fetch. onSelectConversation was also hoisted into a stable useCallback (was previously a fresh inline arrow every render, which separately broke the history list's memoization).
Verified with a regression test (AiChatView.test.tsx, "given a conversation is selected from history and onConversationLoad supplies messages, should NOT double-fetch that conversation over the network") that fails without the fix and passes with it.
Leaving this thread open for verification rather than resolving it myself, since the fix landed after this review comment was posted.
…1849 A full re-review of the combined diff (all prior commits together, not just incrementally) surfaced 10 concrete issues the per-commit reviews missed: - usePageAgentSidebarState.ts was a 4th conversation-identity surface the original refactor never touched — createNewConversation/ loadConversation both still awaited the server before adopting an id, reproducing the exact race this PR exists to close. Fixed with the same client-generated-id + synchronous-identity pattern used elsewhere. - usePageAgentDashboardStore's loadMostRecentConversation unconditionally fell back to createNewConversation on any failure/empty-result path, with no check that a newer identity (from loadConversation/ createNewConversation racing it) had already won — silently clobbering the user's more recent choice. Now guarded. - AiChatView's history-select handler set identity before the skip-load-effect token, defeating its own double-fetch guard and double-fetching messages on every history click. Also passed a fresh inline callback each render, breaking the history list's memoization — both fixed by hoisting to one stable useCallback with the correct ordering. - resolveConversation didn't catch an exception (vs. just a non-ok response) on the messages-prefetch fetch, so a network blip on that *secondary* request was misclassified as a full identity-resolution failure, blocking the whole chat even though the conversation was already known. - AiChatView's late-joiner stream-reconciliation callback called setIdentity with no staleness guard, unlike every other reconciliation path in the file — a stale background sync could snap the user back to a conversation they'd already navigated away from, since IDENTITY_SET always wins. - The reducer wrapper in usePageAgentDashboardStore wrote to the store even when the transition was a guaranteed no-op (a stale RESOLVED after a newer IDENTITY_SET), needlessly notifying every subscriber. - Making loadConversation adopt identity synchronously removed the window where isInitialized/isConversationLoading covered the messages fetch — a conversation switch in agent/global mode could flash the previous conversation's messages under the new one with no loading indicator. Added a decoupled isMessagesLoading / isConversationMessagesLoading flag (mirroring AiChatView's existing isLoadingMessages) across the store, GlobalChatContext, usePageAgentSidebarState, and both consumer views. 227 tests (35 new/updated) across all 11 touched files, all green. Full typecheck clean. Full web suite: 736/740 files, 11066/11082 tests green (same 4 pre-existing, unrelated failures as before this change). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
apps/web/src/lib/ai/shared/hooks/useConversationIdentity.ts (1)
50-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winGuard against redundant
resolve()invocations on repeated retry.
runResolvealways callsresolveRef.current()even when the corresponding dispatch is a no-op in the reducer (e.g. callingretry()again while alreadyresolving, orRETRYfrom a non-errorstate). The reducer correctly prevents a stale result from clobbering state, but it doesn't stop the redundant network/resolve call from firing in the first place — wasteful, and risky ifresolve()ever has a non-idempotent side effect.Mirror the "skip the effect on a no-op transition" pattern already used in
usePageAgentDashboardStore'sapplyIdentity.♻️ Proposed guard
const runResolve = useCallback((startAction: { type: 'RESOLVE_STARTED' } | { type: 'RETRY' }) => { - dispatch(startAction); - resolveRef.current().then( + dispatch(startAction); + setStateRef.current = conversationIdentityReducer(setStateRef.current, startAction); + // only invoke resolve() if the dispatch actually moved us into `resolving` + if (setStateRef.current.status !== 'resolving') return; + resolveRef.current().then( (result) => { if (isMountedRef.current) dispatch({ type: 'RESOLVED', conversationId: result.conversationId }); }, (error) => { if (!isMountedRef.current) return; const message = error instanceof Error ? error.message : 'Failed to resolve conversation'; dispatch({ type: 'RESOLVE_FAILED', message }); } ); }, []);(Requires tracking a ref mirroring
statealongside the reducer, sincerunResolve'suseCallbackhas no dependency on the lateststate.)Also applies to: 73-75
🤖 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/shared/hooks/useConversationIdentity.ts` around lines 50 - 62, `runResolve` in `useConversationIdentity` always triggers `resolveRef.current()` even when the reducer transition is a no-op, so add a state-mirroring ref and guard the call before dispatching `RESOLVE_STARTED` or `RETRY`. Update the `retry`/resolve flow to skip invoking `resolve()` when already resolving or when `RETRY` would not change state, following the same skip-on-no-op pattern used by `usePageAgentDashboardStore`’s `applyIdentity`. Make sure the guard covers both the initial retry path and any reused resolve helper so redundant network calls are avoided.apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.ts (1)
181-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage gap: no test for the new client-supplied
conversationIdpath.The existing route test only covers empty-body, custom-title, and invalid-JSON cases (per the supplied context snippet) — none exercise
body.conversationIdbeing honored, rejected, or malformed.🤖 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/page-agents/`[agentId]/conversations/route.ts around lines 181 - 191, Add test coverage for the new client-supplied conversation ID branch in the conversations route. Update the route tests around the request parsing in route.ts to verify that a valid string body.conversationId is used as the conversationId instead of createId(), and add cases for malformed/empty values to confirm the fallback behavior still generates a server-side ID. Keep the focus on the request.json parsing and conversationId selection logic.
🤖 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/page-agents/`[agentId]/conversations/route.ts:
- Around line 181-191: Validate the client-supplied conversation identifier
before using it in the conversations route, since the current
`body.conversationId` check in `route.ts` accepts any non-empty string. Update
the `conversationId` selection logic to verify the value with `isCuid()` (or the
CUID2 regex) before assigning it, and fall back to `createId()` when validation
fails so only valid row ids are persisted.
In
`@apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx`:
- Around line 137-141: The currentConversationIdRef assignment is happening
during render, which can expose an uncommitted value to late-joiner sync logic.
Move the currentConversationIdRef.current update into a useEffect in AiChatView,
matching the pageIdRef pattern, and keep the ref in sync there using
currentConversationId so async callbacks only see committed state.
In `@apps/web/src/contexts/GlobalChatContext.tsx`:
- Around line 147-154: The non-ok path in GlobalChatContext’s message loading
flow leaves the previous conversation’s messages visible after switching
identities. Update the else branch in the fetch logic (alongside the existing
catch handling) to clear initialMessages when messagesResponse.ok is false, and
keep setIsMessagesLoading(false) in place so the new conversation does not
render stale content.
In `@apps/web/src/stores/page-agents/usePageAgentDashboardStore.ts`:
- Around line 275-298: The createNewConversation flow in
usePageAgentDashboardStore leaves isConversationMessagesLoading stuck true when
a new conversation replaces a prior load. Update the same set(...) block that
resets conversationMessages, conversationAgentId, and conversationLoadSignal to
also clear isConversationMessagesLoading, so GlobalAssistantView and
SidebarChatTab stop showing the spinner for the newly created conversation.
---
Nitpick comments:
In `@apps/web/src/app/api/ai/page-agents/`[agentId]/conversations/route.ts:
- Around line 181-191: Add test coverage for the new client-supplied
conversation ID branch in the conversations route. Update the route tests around
the request parsing in route.ts to verify that a valid string
body.conversationId is used as the conversationId instead of createId(), and add
cases for malformed/empty values to confirm the fallback behavior still
generates a server-side ID. Keep the focus on the request.json parsing and
conversationId selection logic.
In `@apps/web/src/lib/ai/shared/hooks/useConversationIdentity.ts`:
- Around line 50-62: `runResolve` in `useConversationIdentity` always triggers
`resolveRef.current()` even when the reducer transition is a no-op, so add a
state-mirroring ref and guard the call before dispatching `RESOLVE_STARTED` or
`RETRY`. Update the `retry`/resolve flow to skip invoking `resolve()` when
already resolving or when `RETRY` would not change state, following the same
skip-on-no-op pattern used by `usePageAgentDashboardStore`’s `applyIdentity`.
Make sure the guard covers both the initial retry path and any reused resolve
helper so redundant network calls are avoided.
🪄 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: cb8b1955-d89d-4b90-be9d-3696c266c991
📒 Files selected for processing (21)
apps/web/src/app/api/ai/page-agents/[agentId]/conversations/route.tsapps/web/src/components/layout/middle-content/CenterPanel.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/components/layout/middle-content/page-views/ai-page/__tests__/AiChatView.test.tsxapps/web/src/components/layout/middle-content/page-views/dashboard/GlobalAssistantView.tsxapps/web/src/components/layout/right-sidebar/ai-assistant/SidebarChatTab.tsxapps/web/src/contexts/GlobalChatContext.tsxapps/web/src/contexts/__tests__/GlobalChatContext.test.tsxapps/web/src/hooks/page-agents/__tests__/usePageAgentSidebarState.test.tsapps/web/src/hooks/page-agents/usePageAgentSidebarState.tsapps/web/src/lib/ai/shared/__tests__/conversation-identity.test.tsapps/web/src/lib/ai/shared/agent-conversations.tsapps/web/src/lib/ai/shared/conversation-identity.tsapps/web/src/lib/ai/shared/hooks/__tests__/useConversationIdentity.test.tsapps/web/src/lib/ai/shared/hooks/__tests__/useConversations.test.tsapps/web/src/lib/ai/shared/hooks/index.tsapps/web/src/lib/ai/shared/hooks/useConversationIdentity.tsapps/web/src/lib/ai/shared/hooks/useConversations.tsapps/web/src/lib/ai/shared/index.tsapps/web/src/stores/page-agents/__tests__/usePageAgentDashboardStore.test.tsapps/web/src/stores/page-agents/usePageAgentDashboardStore.ts
…stale state Three real issues from CodeRabbit's review of the previous fix commit: - The conversations POST route accepted any non-empty string as a client-supplied conversationId, letting a malformed value get persisted as a row id. Now validated with isCuid() (already used elsewhere in this codebase for the same purpose), falling back to createId() when it fails. - GlobalChatContext's loadConversation cleared initialMessages on a thrown fetch error but not on a non-ok response — the exact "flash the previous conversation's messages under the new identity" bug isMessagesLoading was added to prevent, just on a different branch. Now both paths clear it. - usePageAgentDashboardStore's createNewConversation didn't reset isConversationMessagesLoading. If a loadConversation fetch was still in flight when the user started a new conversation, the flag could get stuck true forever (the superseded loadConversation's own resolution sees itself stale and skips clearing it), showing a permanent loading spinner on the new conversation. Also evaluated and explicitly declined one CodeRabbit suggestion (move currentConversationIdRef's update into a useEffect, matching pageIdRef): doing so broke 2 existing tests because this component uses no concurrent-rendering features that could discard an in-progress render (the theoretical hazard the suggestion guards against), while an effect introduces a real timing gap — an async callback firing before React flushes a just-scheduled effect reads a stale ref. Replied on the PR thread explaining the tradeoff instead of applying it. 10 new/updated tests across 5 files, all green. Full typecheck clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Sending a message to an existing conversation could create a new one instead of appending, and starting a new conversation could append to the previous one instead. Root cause: conversation identity was tracked independently in
useConversations.ts,usePageAgentDashboardStore.ts,GlobalChatContext.tsx, andAiChatView.tsxlocal state, each writing it asynchronously after a network round trip — a send fired mid-transition could carry a stale id, and a stale async response could overwrite an identity the user had already moved past.conversation-identity.ts):idle → resolving → ready/error.IDENTITY_SETalways wins regardless of current state; a staleRESOLVED/RESOLVE_FAILEDthat arrives after a newer identity was set is a guaranteed no-op. One shared, independently-tested primitive.pu/cli-loginownership-guard fix protects) a client-supplied id.AiChatView.tsx: the new shareduseConversationIdentityhook (single-instance component).usePageAgentDashboardStore.ts: the reducer called directly inside Zustand actions (a store isn't a React component, so the hook doesn't apply — the plain reducer function does).GlobalChatContext.tsx: a localuseReducerin the provider (shared by every consumer via context, not duplicated per-consumer).usePageAgentSidebarState.ts: a 4th independent surface (sidebar agent chat) discovered during proactive review — same reducer, same Zustand-action wiring pattern as the dashboard store.AiChatViewkeyed bypage.idinCenterPanel.tsx, matching the existing pattern already used forDocumentView/SheetView/etc. — the component can receive a newpageprop without unmounting, and React's own documented pattern for resetting state on a new subject iskey, not a hand-rolled reset-on-prop-change effect.AiChatView's init effect misclassified a transient list-fetch failure as "no conversations exist," silently replacing a real conversation with a fresh disconnected one. It now surfaces a retry banner instead.Hardening pass (proactive + reviewer-driven, post-initial-implementation)
AiChatViewhistory-select (skip-token set after, not before, adopting identity) and restored history-list memoization via a stableonSelectConversation.resolveConversation's messages-prefetch exception no longer misclassifies as a full identity-resolution failure when the conversation itself was already found.loadMostRecentConversation's catch/no-conversation-found paths, the late-joiner stream-reconciliationsetIdentitycall, and a stale Zustand no-opset()— all against clobbering a newer identity the user has since moved to.isConversationMessagesLoading/isMessagesLoadingflag (store, context, sidebar-state hook, both consumer views) — adopting identity synchronously closed the race but also removed the window that used to cover the messages fetch, so a conversation switch could otherwise flash the previous conversation's messages under the new identity with no loading indicator.conversationIdon the create route is now validated withisCuid()before being trusted as a row id.GlobalChatContext's non-ok messages-fetch path now clearsinitialMessages(previously only the thrown-exception path did).Test plan
bun run typecheck— cleanAiChatView, route)bun run --filter web test— 736/740 files green project-wide (4 pre-existing failures unrelated to this change: a missing Postgres test role and a midnight-boundary date test)🤖 Generated with Claude Code
https://claude.ai/code/session_01JT8umuwBmmtKiwmBhyq5uH
Summary by CodeRabbit
New Features
Bug Fixes