feat(refactor): unified message architecture with provider adapters and session store#558
Merged
Merged
Conversation
… store
- Add provider adapter layer (server/providers/) with registry pattern
- Claude, Cursor, Codex, Gemini adapters normalize native formats to NormalizedMessage
- Shared types.js defines ProviderAdapter interface and message kinds
- Registry enables polymorphic provider lookup
- Add unified REST endpoint: GET /api/sessions/:id/messages?provider=...
- Replaces four provider-specific message endpoints with one
- Delegates to provider adapters via registry
- Add frontend session-keyed store (useSessionStore)
- Per-session Map with serverMessages/realtimeMessages/merged
- Dedup by ID, stale threshold for re-fetch, background session accumulation
- No localStorage for messages — backend JSONL is source of truth
- Add normalizedToChatMessages converter (useChatMessages)
- Converts NormalizedMessage[] to existing ChatMessage[] UI format
- Wire unified store into ChatInterface, useChatSessionState, useChatRealtimeHandlers
- Session switch uses store cache for instant render
- Background WebSocket messages routed to correct session slot
Contributor
📝 WalkthroughWalkthroughThis PR introduces a unified message normalization system across all provider integrations. New adapter-based architecture standardizes message formats from Claude, Cursor, Codex, and Gemini before transmission to clients. Frontend state management refactored to use centralized session store managing normalized messages. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (WebSocket)
participant Provider as Provider SDK/CLI<br/>(Claude/Cursor/etc)
participant Adapter as Provider Adapter<br/>normalizeMessage()
participant Server as Server<br/>(index.js)
participant Store as Frontend<br/>Session Store
Provider->>Adapter: raw event<br/>(content_block_delta, etc)
Adapter->>Adapter: normalize to<br/>NormalizedMessage
Adapter->>Server: normalized event
Server->>Client: ws.send<br/>NormalizedMessage
Client->>Store: onWebSocketMessage
Store->>Store: appendRealtime()<br/>or updateStreaming()
Store->>Store: computeMerged()<br/>(server + realtime)
Note over Store: Triggers component<br/>re-render with<br/>merged messages
alt Session Messages Load
Store->>Server: GET /api/sessions/:id/messages<br/>?provider=claude
Server->>Adapter: fetchHistory(sessionId)
Adapter->>Adapter: load & normalize<br/>history from backend
Adapter->>Server: normalized history[]
Server->>Store: history with metadata
Store->>Store: setServerMessages +<br/>recompute merged
end
Possibly Related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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: 12
🧹 Nitpick comments (8)
server/providers/claude/adapter.js (1)
71-89: Minor: Redundant fallback logic.The fallback at lines 71-89 only triggers when
messages.length === 0(no tool_results pushed, no non-internal text parts found). It then rejoins the same text parts and rechecks withisInternalContent. However, if individual text parts were filtered out as internal content, their concatenation would typically start with the same prefix and fail the check again.This fallback appears to be dead code in practice. Consider removing it for clarity, or document the edge case it's meant to handle.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/providers/claude/adapter.js` around lines 71 - 89, Remove the redundant fallback block that re-parses raw.message.content when messages.length === 0: delete the entire conditional that recomputes textParts, rechecks with isInternalContent and pushes createNormalizedMessage with id `${baseId}_text`; this logic duplicates earlier filtering and is effectively dead code (affects symbols: messages, raw.message.content, isInternalContent, createNormalizedMessage, baseId, sessionId, ts, PROVIDER). After removal, run tests/lint to confirm no behavioral change and add a brief comment or unit test only if there is a documented edge case that must be preserved.server/providers/codex/adapter.js (1)
169-176: Consider logging unknown item types.Unknown
itemTypevalues silently become generictool_usemessages. This could make debugging harder if new Codex event types are introduced. Consider adding a console.debug or similar to track unknown types.🔍 Optional: Add debug logging
default: + console.debug(`[CodexAdapter] Unknown itemType: ${raw.itemType}`); // Unknown item type — pass through as generic tool_use return [createNormalizedMessage({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/providers/codex/adapter.js` around lines 169 - 176, The default branch that returns a generic tool_use via createNormalizedMessage currently swallows unknown itemType values; update the default case in the switch handling (around createNormalizedMessage and PROVIDER) to emit a debug/log message (e.g., console.debug or a logger) that includes the raw.itemType, the raw payload (or a summary), and the baseId/sessionId/timestamp so developers can trace unexpected Codex event types without changing the normalized return shape.server/providers/utils.js (1)
11-20: Prefix list appears incomplete compared to existing patterns.The
isSystemMessagefunction inserver/projects.js(lines 1883-1898) includes additional prefixes that aren't present here:
'Invalid API key'- Checks for
'{"subtasks":'and'CRITICAL: You MUST respond with ONLY a JSON''Warmup'Consider whether these should also be included for consistency, or document why they differ.
♻️ Suggested addition for consistency
export const INTERNAL_CONTENT_PREFIXES = Object.freeze([ '<command-name>', '<command-message>', '<command-args>', '<local-command-stdout>', '<system-reminder>', 'Caveat:', 'This session is being continued from a previous', '[Request interrupted', + 'Invalid API key', + 'Warmup', ]);Note: The JSON content checks (
{"subtasks":,CRITICAL: You MUST respond) useincludes()rather thanstartsWith(), so they may need a separate helper if needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/providers/utils.js` around lines 11 - 20, Update the INTERNAL_CONTENT_PREFIXES constant to include the additional system prefixes used by isSystemMessage (add 'Invalid API key', 'Warmup') and decide how to represent the JSON-content checks used there (the '{"subtasks":' and 'CRITICAL: You MUST respond with ONLY a JSON' patterns) — either add them as entries and switch matching logic where INTERNAL_CONTENT_PREFIXES is used from startsWith to a helper that supports both startsWith and includes, or keep them out and document why; adjust or add a helper (e.g., a new matchSystemPrefixes function) used by isSystemMessage and callers so the JSON substrings are matched with includes() while other prefixes use startsWith().server/providers/cursor/adapter.js (1)
27-28: Fallback toprocess.cwd()may cause incorrect hash.If
projectPathis an empty string (the default infetchHistory),crypto.createHash('md5').update('')produces a hash that likely won't match any Cursor project directory. This would cause the DB open to fail silently with "no such file" rather than indicating a missing required parameter.Consider throwing an explicit error if
projectPathis falsy, or document this requirement more clearly.🛡️ Proposed validation
async function loadCursorBlobs(sessionId, projectPath) { + if (!projectPath) { + throw new Error('projectPath is required for Cursor session loading'); + } // Lazy-import sqlite so the module doesn't fail if sqlite3 is unavailable const { default: sqlite3 } = await import('sqlite3'); const { open } = await import('sqlite'); - const cwdId = crypto.createHash('md5').update(projectPath || process.cwd()).digest('hex'); + const cwdId = crypto.createHash('md5').update(projectPath).digest('hex');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/providers/cursor/adapter.js` around lines 27 - 28, The current logic computes cwdId using crypto.createHash(...).update(projectPath || process.cwd()) which silently falls back to process.cwd() when projectPath is an empty string and can produce an incorrect hash; update the code that computes cwdId (where projectPath is read—e.g., in fetchHistory or the adapter initialization that defines cwdId) to validate projectPath and throw a clear error if projectPath is falsy/empty, or otherwise require an explicit flag; ensure the validation triggers before computing cwdId/storeDbPath so callers receive a descriptive error instead of a failing DB open (reference symbols: projectPath, cwdId, storeDbPath).server/routes/messages.js (1)
39-39: Consider validatingoffsetis a valid number.If
req.query.offsetis a non-numeric string like"abc",parseIntreturnsNaN, which could cause unexpected behavior in adapters.🛡️ Suggested defensive fix
- const offset = parseInt(req.query.offset || '0', 10); + const parsedOffset = parseInt(req.query.offset || '0', 10); + const offset = Number.isNaN(parsedOffset) ? 0 : parsedOffset;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/routes/messages.js` at line 39, The parsed offset may be NaN when req.query.offset is non-numeric; update the offset handling in server/routes/messages.js where const offset = parseInt(req.query.offset || '0', 10) is declared to validate the parsed value (e.g., use Number.isNaN or Number.isFinite) and either default to 0 or return a 400 error when invalid; ensure you reference req.query.offset and the offset variable so callers/adapters always receive a safe numeric offset.server/providers/gemini/adapter.js (1)
29-36:stream_endmessages missing explicitidfield.At line 33, the
stream_endmessage is created without anid, relying oncreateNormalizedMessageto auto-generate one. However, thestream_deltaat line 29 usesbaseId. This inconsistency could cause deduplication issues in the session store if the same event is processed twice.Consider providing an explicit
idforstream_endas well:♻️ Suggested fix
if (raw.delta !== true) { - msgs.push(createNormalizedMessage({ sessionId, timestamp: ts, provider: PROVIDER, kind: 'stream_end' })); + msgs.push(createNormalizedMessage({ id: `${baseId}_end`, sessionId, timestamp: ts, provider: PROVIDER, kind: 'stream_end' })); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/providers/gemini/adapter.js` around lines 29 - 36, The stream_end message is created without an explicit id while stream_delta uses baseId, causing inconsistent IDs; update the call to createNormalizedMessage that creates the 'stream_end' (the msgs.push with kind: 'stream_end') to include id: baseId (or another deterministic id derived from baseId) so both stream_delta and stream_end share a stable identifier (referencing createNormalizedMessage, baseId, raw.delta, PROVIDER, sessionId, ts).src/components/chat/hooks/useChatMessages.ts (1)
72-82: Consider adding type safety forsubagentTools.The
as any[]type assertion at line 73 bypasses TypeScript's type checking. IfNormalizedMessagehas a defined shape forsubagentTools, using that type would catch potential mismatches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/chat/hooks/useChatMessages.ts` around lines 72 - 82, Replace the unsafe any[] cast by using the actual type of msg.subagentTools from NormalizedMessage: check and import the subagent tool interface (e.g., SubagentTool or whatever type defines toolId/toolName/toolInput/toolResult/timestamp) and iterate over msg.subagentTools as that type instead of any; update the loop in the isSubagentContainer branch (the block that pushes into childTools) to use the proper typed variable so TypeScript can validate properties on msg.subagentTools and catch mismatches at compile time.server/index.js (1)
401-402: Add rate limiting to the unified messages endpoint.The
/api/sessionsroute performs potentially expensive async I/O operations (fetching message history viaadapter.fetchHistory()) without rate limiting. WhileauthenticateTokenprotects against unauthenticated access, an authenticated user could repeatedly fetch large message histories with thelimitandoffsetparameters, causing unnecessary load on storage and the server.Consider implementing rate limiting middleware, such as
express-rate-limit, especially given that this endpoint performs async I/O. For authenticated endpoints like this, best practices recommend tiered limits—for example, 100 requests per 15 minutes per user.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/index.js` around lines 401 - 402, Add per-user rate limiting to the unified messages endpoint by inserting an express-rate-limit middleware between authenticateToken and messagesRoutes: create a limiter (e.g., 100 requests per 15 minutes) that keys limits by authenticated user id (from authenticateToken) and apply it in the route registration so requests to app.use('/api/sessions', authenticateToken, rateLimiter, messagesRoutes) are throttled; ensure the limiter is async-safe and configured to return appropriate 429 responses to avoid excessive calls to adapter.fetchHistory() inside the messagesRoutes handlers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/providers/cursor/adapter.js`:
- Around line 170-181: The Cursor adapter currently paginates forward using
allNormalized.slice(start, start + limit) (variables: allNormalized, offset,
limit) which returns oldest messages for offset=0; change it to mirror Claude's
reverse pagination by computing total = allNormalized.length and slicing
allNormalized.slice(total - offset - limit, total - offset) so offset=0 returns
the most recent messages, and update hasMore/total/offset semantics accordingly
(ensure hasMore uses offset + limit < total and guard negative start indices
when limit > total).
In `@src/components/chat/hooks/useChatComposerState.ts`:
- Around line 530-532: The fallback to sessionStorage.getItem('cursorSessionId')
should only be used for the Cursor provider; update the logic that computes
effectiveSessionId (where currentSessionId, selectedSession?.id and
sessionStorage.getItem('cursorSessionId') are used) to include the cursor-only
gate (e.g., use provider === 'cursor' ?
sessionStorage.getItem('cursorSessionId') : null) so that sessionToActivate will
not resume a Cursor session for non-Cursor providers; adjust in
useChatComposerState where effectiveSessionId and sessionToActivate are defined.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts`:
- Around line 227-243: The session_created branch in useChatRealtimeHandlers
currently applies UI side effects for any session id; guard these mutations so
they only run when the created session corresponds to the active/temporary
session. Concretely, in the case 'session_created' handler (and similarly in the
other session-related branches between the noted range), wrap the code that
calls sessionStorage.setItem, mutates pendingViewSessionRef.current.sessionId,
setCurrentSessionId, onReplaceTemporarySession, setPendingPermissionRequests,
and onNavigateToSession with a check that the newSessionId matches the active
session context (for example: newSessionId === currentSessionId ||
currentSessionId?.startsWith('new-session-')). Apply the same scoping pattern to
the other affected branches so background/session store updates do not clear
spinners, overwrite token/ status, open prompts, or navigate when the session is
not the visible/active one.
- Around line 183-217: The streaming buffers and timer must be tracked
per-session and use the incoming msg.provider rather than the global provider:
replace the single shared streamTimerRef/accumulatedStreamRef/streamBufferRef
with a Map keyed by sid (e.g. streamStateBySid) so each session accumulates its
own text and schedules its own timer that closes over that sid and msg.provider
when calling sessionStore.updateStreaming; stop appending raw 'stream_delta'
chunks to background sessions (sid !== activeViewSessionId) — instead only
produce the finalized assistant entry on 'stream_end' for background sessions;
on 'stream_end' read and clear the per-sid buffer, call
sessionStore.updateStreaming(sid, accumulated, msg.provider) then
sessionStore.finalizeStreaming(sid) and delete the per-sid state to avoid
cross-session contamination.
In `@src/components/chat/hooks/useChatSessionState.ts`:
- Around line 374-391: Before calling sessionStore.fetchFromServer, capture the
session id in a local variable (e.g. const fetchSessionId = selectedSession.id)
and in both the then and catch handlers only update setHasMoreMessages,
setTotalMessages, setTokenBudget and setIsLoadingSessionMessages if the current
selectedSession.id still equals fetchSessionId; do this around the existing
sessionStore.fetchFromServer(...) call and its promise handlers so stale
responses from the previous session are ignored.
- Around line 165-170: The current reset clears viewHiddenCount on any
storeMessages length change which makes a follow-up appended immediately after
rewindMessages() re-show hidden messages; to fix, add a ref flag (e.g.,
isRewindingRef) and set isRewindingRef.current = true before calling
rewindMessages() and clear it after the rewind completes, then update the
existing logic that compares prevStoreLenRef and storeMessages to only call
setViewHiddenCount(0) when isRewindingRef.current is false (leave
prevStoreLenRef update as-is); reference symbols: prevStoreLenRef,
storeMessages, viewHiddenCount, setViewHiddenCount, rewindMessages(), and
introduce isRewindingRef to skip clearing during rewind-originated changes.
- Around line 199-202: clearMessages currently only calls
sessionStore.clearRealtime(activeSessionId), which leaves loaded
server/transcript messages intact; update clearMessages to clear both realtime
and server/transcript message stores (e.g., call
sessionStore.clearRealtime(activeSessionId) and
sessionStore.clearServerMessages(activeSessionId) or
sessionStore.clearTranscript(activeSessionId) depending on the store API) and
then notify/update the merged view (e.g., emit a change or call
sessionStore.refreshSession(activeSessionId)) so the UI no longer renders the
old history; also include any newly used sessionStore methods in the useCallback
dependency array.
- Around line 40-51: chatMessageToNormalized currently creates a new random
optimistic id every call which prevents server-persisted messages from replacing
the optimistic entry (useSessionStore merges by id); modify
chatMessageToNormalized to use a stable reconciliation key: if the incoming
ChatMessage has a stable client-side identifier (e.g., msg.id, msg.clientId,
msg.localId or a provided tempId) use that as the NormalizedMessage id
(optionally prefixed with "local_"), otherwise fall back to a deterministic id
derived from stable properties (e.g., hash of msg.content + msg.timestamp)
instead of Date.now() and Math.random(); update the id creation inside
chatMessageToNormalized and ensure the returned base NormalizedMessage uses that
stable id so later server-fetched messages can merge correctly.
- Around line 151-160: The flush logic in useChatSessionState is replaying any
queued ChatMessage into a newly active session (including temporary
"new-session-*" ids) because addMessage can queue non-user messages and the
check fires on any activeSessionId change; fix it by restricting what gets
queued and when it flushes: ensure addMessage only enqueues unsent user prompts
(check message.role === 'user' and message.isUnsent or similar) and change the
flush condition around prevActiveSessionRef/activeSessionId to only run when
activeSessionId is a real session (not starting with "new-session-") and when
pendingUserMessage is the unsent user prompt; use chatMessageToNormalized,
sessionStore.appendRealtime and setPendingUserMessage unchanged but update
prevActiveSessionRef after a successful flush; apply the same filtering and
real-session check to the other flush block referenced (around lines 186-197).
In `@src/components/chat/view/ChatInterface.tsx`:
- Around line 206-218: handleWebSocketReconnect currently calls
sessionStore.refreshFromServer which clears realtimeMessages and can drop
in-progress stream content; guard against that by checking for an active
streaming flag before refreshing (e.g., use
sessionStore.isStreaming(selectedSession.id) or selectedSession.isStreaming) and
skip or delay calling sessionStore.refreshFromServer while streaming is active,
or alternatively preserve realtimeMessages by copying them into a temp buffer
before refresh and restoring after the fetch completes; update the
handleWebSocketReconnect logic to use that guard/preserve approach and add a
short comment documenting the chosen behavior (references:
handleWebSocketReconnect, sessionStore.refreshFromServer, realtimeMessages,
selectedSession).
In `@src/stores/useSessionStore.ts`:
- Around line 386-403: finalizeStreaming currently replaces the streaming
message with a brand-new random id so server-fetched assistant messages can't
dedupe; instead preserve a stable client-side dedupe key and avoid mutating to a
random id: in finalizeStreaming (operating on slot.realtimeMessages) stop
generating the random `text_*` id and either keep the original
`__streaming_${sessionId}` as a clientGeneratedId (e.g. set a new property
clientGeneratedId = stream.id) or assign a deterministic pending id, mark
kind:'text' and role:'assistant', then update the dedupe/merge logic used by
recomputeMergedIfNeeded to prefer server messages by matching on
clientGeneratedId (when present) before falling back to id comparison so
persisted history collapses the realtime message into the server record and
prevents duplication; call notify(sessionId) as before.
---
Nitpick comments:
In `@server/index.js`:
- Around line 401-402: Add per-user rate limiting to the unified messages
endpoint by inserting an express-rate-limit middleware between authenticateToken
and messagesRoutes: create a limiter (e.g., 100 requests per 15 minutes) that
keys limits by authenticated user id (from authenticateToken) and apply it in
the route registration so requests to app.use('/api/sessions',
authenticateToken, rateLimiter, messagesRoutes) are throttled; ensure the
limiter is async-safe and configured to return appropriate 429 responses to
avoid excessive calls to adapter.fetchHistory() inside the messagesRoutes
handlers.
In `@server/providers/claude/adapter.js`:
- Around line 71-89: Remove the redundant fallback block that re-parses
raw.message.content when messages.length === 0: delete the entire conditional
that recomputes textParts, rechecks with isInternalContent and pushes
createNormalizedMessage with id `${baseId}_text`; this logic duplicates earlier
filtering and is effectively dead code (affects symbols: messages,
raw.message.content, isInternalContent, createNormalizedMessage, baseId,
sessionId, ts, PROVIDER). After removal, run tests/lint to confirm no behavioral
change and add a brief comment or unit test only if there is a documented edge
case that must be preserved.
In `@server/providers/codex/adapter.js`:
- Around line 169-176: The default branch that returns a generic tool_use via
createNormalizedMessage currently swallows unknown itemType values; update the
default case in the switch handling (around createNormalizedMessage and
PROVIDER) to emit a debug/log message (e.g., console.debug or a logger) that
includes the raw.itemType, the raw payload (or a summary), and the
baseId/sessionId/timestamp so developers can trace unexpected Codex event types
without changing the normalized return shape.
In `@server/providers/cursor/adapter.js`:
- Around line 27-28: The current logic computes cwdId using
crypto.createHash(...).update(projectPath || process.cwd()) which silently falls
back to process.cwd() when projectPath is an empty string and can produce an
incorrect hash; update the code that computes cwdId (where projectPath is
read—e.g., in fetchHistory or the adapter initialization that defines cwdId) to
validate projectPath and throw a clear error if projectPath is falsy/empty, or
otherwise require an explicit flag; ensure the validation triggers before
computing cwdId/storeDbPath so callers receive a descriptive error instead of a
failing DB open (reference symbols: projectPath, cwdId, storeDbPath).
In `@server/providers/gemini/adapter.js`:
- Around line 29-36: The stream_end message is created without an explicit id
while stream_delta uses baseId, causing inconsistent IDs; update the call to
createNormalizedMessage that creates the 'stream_end' (the msgs.push with kind:
'stream_end') to include id: baseId (or another deterministic id derived from
baseId) so both stream_delta and stream_end share a stable identifier
(referencing createNormalizedMessage, baseId, raw.delta, PROVIDER, sessionId,
ts).
In `@server/providers/utils.js`:
- Around line 11-20: Update the INTERNAL_CONTENT_PREFIXES constant to include
the additional system prefixes used by isSystemMessage (add 'Invalid API key',
'Warmup') and decide how to represent the JSON-content checks used there (the
'{"subtasks":' and 'CRITICAL: You MUST respond with ONLY a JSON' patterns) —
either add them as entries and switch matching logic where
INTERNAL_CONTENT_PREFIXES is used from startsWith to a helper that supports both
startsWith and includes, or keep them out and document why; adjust or add a
helper (e.g., a new matchSystemPrefixes function) used by isSystemMessage and
callers so the JSON substrings are matched with includes() while other prefixes
use startsWith().
In `@server/routes/messages.js`:
- Line 39: The parsed offset may be NaN when req.query.offset is non-numeric;
update the offset handling in server/routes/messages.js where const offset =
parseInt(req.query.offset || '0', 10) is declared to validate the parsed value
(e.g., use Number.isNaN or Number.isFinite) and either default to 0 or return a
400 error when invalid; ensure you reference req.query.offset and the offset
variable so callers/adapters always receive a safe numeric offset.
In `@src/components/chat/hooks/useChatMessages.ts`:
- Around line 72-82: Replace the unsafe any[] cast by using the actual type of
msg.subagentTools from NormalizedMessage: check and import the subagent tool
interface (e.g., SubagentTool or whatever type defines
toolId/toolName/toolInput/toolResult/timestamp) and iterate over
msg.subagentTools as that type instead of any; update the loop in the
isSubagentContainer branch (the block that pushes into childTools) to use the
proper typed variable so TypeScript can validate properties on msg.subagentTools
and catch mismatches at compile time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fe8ae3fc-a5b8-46e0-b3db-0d06d43c083c
📒 Files selected for processing (26)
server/claude-sdk.jsserver/cursor-cli.jsserver/gemini-cli.jsserver/gemini-response-handler.jsserver/index.jsserver/openai-codex.jsserver/projects.jsserver/providers/claude/adapter.jsserver/providers/codex/adapter.jsserver/providers/cursor/adapter.jsserver/providers/gemini/adapter.jsserver/providers/registry.jsserver/providers/types.jsserver/providers/utils.jsserver/routes/codex.jsserver/routes/gemini.jsserver/routes/messages.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatMessages.tssrc/components/chat/hooks/useChatRealtimeHandlers.tssrc/components/chat/hooks/useChatSessionState.tssrc/components/chat/utils/chatStorage.tssrc/components/chat/utils/messageTransforms.tssrc/components/chat/view/ChatInterface.tsxsrc/stores/useSessionStore.tssrc/utils/api.js
💤 Files with no reviewable changes (3)
- src/components/chat/utils/chatStorage.ts
- server/routes/gemini.js
- src/components/chat/utils/messageTransforms.ts
Comment on lines
+197
to
208
| case 'result': { | ||
| // Session complete — send stream end + lifecycle complete with result payload | ||
| console.log('Cursor session result:', response); | ||
|
|
||
| // Do not emit an extra content_block_stop here. | ||
| // The UI already finalizes the streaming message in cursor-result handling, | ||
| // and emitting both can produce duplicate assistant messages. | ||
| ws.send({ | ||
| type: 'cursor-result', | ||
| sessionId: capturedSessionId || sessionId, | ||
| data: response, | ||
| success: response.subtype === 'success' | ||
| }); | ||
| const resultText = typeof response.result === 'string' ? response.result : ''; | ||
| ws.send(createNormalizedMessage({ | ||
| kind: 'complete', | ||
| exitCode: response.subtype === 'success' ? 0 : 1, | ||
| resultText, | ||
| isError: response.subtype !== 'success', | ||
| sessionId: capturedSessionId || sessionId, provider: 'cursor', | ||
| })); | ||
| break; |
Contributor
There was a problem hiding this comment.
Cursor now emits complete twice.
The JSON result branch sends kind: 'complete', and the process close handler sends another one for the same run. The client handles complete as a terminal lifecycle event, so this duplicates teardown/finalization and store entries.
Also applies to: 278-286
Comment on lines
+170
to
+181
| // Apply pagination | ||
| if (limit !== null && limit > 0) { | ||
| const start = offset; | ||
| const page = allNormalized.slice(start, start + limit); | ||
| return { | ||
| messages: page, | ||
| total: allNormalized.length, | ||
| hasMore: start + limit < allNormalized.length, | ||
| offset, | ||
| limit, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how frontend handles pagination for different providers
rg -n "offset|limit" --type=ts --type=js -g 'src/*' -g '!server/*' -C2 | head -100Repository: siteboon/claudecodeui
Length of output: 1961
🏁 Script executed:
# First, find and examine the Claude adapter code mentioned (projects.js lines 1065-1067)
rg -n "getSessionMessages" server/ -A 10 -B 2 | head -50Repository: siteboon/claudecodeui
Length of output: 2902
🏁 Script executed:
# Find the Cursor adapter fetchHistory method to confirm the pagination implementation
rg -n "fetchHistory" server/providers/cursor/adapter.js -A 35 -B 2 | head -80Repository: siteboon/claudecodeui
Length of output: 1402
🏁 Script executed:
# Look for how frontend calls these pagination endpoints
find src -type f \( -name "*.ts" -o -name "*.tsx" -o -name "*.js" \) -exec grep -l "offset\|limit" {} \; 2>/dev/nullRepository: siteboon/claudecodeui
Length of output: 682
🏁 Script executed:
# Get full getSessionMessages implementation from projects.js
sed -n '983,1050p' server/projects.jsRepository: siteboon/claudecodeui
Length of output: 2477
🏁 Script executed:
# Check frontend API call patterns
cat src/utils/api.js | head -100Repository: siteboon/claudecodeui
Length of output: 3820
🏁 Script executed:
# Look at how the frontend's session store calls the API
rg -n "getSessionMessages\|fetchHistory" src/ -A 5 -B 2Repository: siteboon/claudecodeui
Length of output: 47
🏁 Script executed:
# Get the rest of getSessionMessages to see pagination handling
sed -n '1050,1100p' server/projects.jsRepository: siteboon/claudecodeui
Length of output: 1635
🏁 Script executed:
# Also check if there are other references to reverse pagination in Claude adapter
rg -n "slice.*total.*offset\|offset.*slice" server/projects.js -B 3 -A 3Repository: siteboon/claudecodeui
Length of output: 47
Pagination direction inconsistency between Cursor and Claude adapters.
Cursor uses forward pagination (slice(offset, offset + limit) — lines 172-173), while Claude uses reverse pagination (slice(total - offset - limit, total - offset) — projects.js:1064-1065 with explicit comment "offset 0 should give us the most recent messages").
The frontend unifies both endpoints in api.unifiedSessionMessages() and passes identical offset/limit parameters to both providers. This creates inconsistent behavior:
- Claude, offset=0, limit=20: returns 20 most recent messages
- Cursor, offset=0, limit=20: returns 20 oldest messages
Align the pagination direction across both adapters or add documentation explaining the intentional difference.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/providers/cursor/adapter.js` around lines 170 - 181, The Cursor
adapter currently paginates forward using allNormalized.slice(start, start +
limit) (variables: allNormalized, offset, limit) which returns oldest messages
for offset=0; change it to mirror Claude's reverse pagination by computing total
= allNormalized.length and slicing allNormalized.slice(total - offset - limit,
total - offset) so offset=0 returns the most recent messages, and update
hasMore/total/offset semantics accordingly (ensure hasMore uses offset + limit <
total and guard negative start indices when limit > total).
Comment on lines
+530
to
+532
| const effectiveSessionId = | ||
| currentSessionId || selectedSession?.id || sessionStorage.getItem('cursorSessionId'); | ||
| const sessionToActivate = effectiveSessionId || `new-session-${Date.now()}`; |
Contributor
There was a problem hiding this comment.
Don't reuse cursorSessionId for non-Cursor sends.
This fallback is now provider-agnostic, so a brand-new Claude/Codex/Gemini submission can accidentally resume the last Cursor session when currentSessionId and selectedSession are empty. Gate the sessionStorage.getItem('cursorSessionId') path behind provider === 'cursor'.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatComposerState.ts` around lines 530 - 532,
The fallback to sessionStorage.getItem('cursorSessionId') should only be used
for the Cursor provider; update the logic that computes effectiveSessionId
(where currentSessionId, selectedSession?.id and
sessionStorage.getItem('cursorSessionId') are used) to include the cursor-only
gate (e.g., use provider === 'cursor' ?
sessionStorage.getItem('cursorSessionId') : null) so that sessionToActivate will
not resume a Cursor session for non-Cursor providers; adjust in
useChatComposerState where effectiveSessionId and sessionToActivate are defined.
Comment on lines
+183
to
+217
| // --- Streaming: buffer for performance --- | ||
| if (msg.kind === 'stream_delta') { | ||
| const text = msg.content || ''; | ||
| if (!text) return; | ||
| streamBufferRef.current += text; | ||
| accumulatedStreamRef.current += text; | ||
| if (!streamTimerRef.current) { | ||
| streamTimerRef.current = window.setTimeout(() => { | ||
| streamTimerRef.current = null; | ||
| if (sid) { | ||
| sessionStore.updateStreaming(sid, accumulatedStreamRef.current, provider); | ||
| } | ||
| } | ||
| break; | ||
| }, 100); | ||
| } | ||
|
|
||
| case 'claude-interactive-prompt': | ||
| // Interactive prompts are parsed/rendered as text in the UI. | ||
| // Normalize to string to keep ChatMessage.content shape consistent. | ||
| { | ||
| const interactiveContent = | ||
| typeof latestMessage.data === 'string' | ||
| ? latestMessage.data | ||
| : JSON.stringify(latestMessage.data ?? '', null, 2); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content: interactiveContent, | ||
| timestamp: new Date(), | ||
| isInteractivePrompt: true, | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
|
|
||
| case 'claude-permission-request': | ||
| if (provider !== 'claude' || !latestMessage.requestId) { | ||
| break; | ||
| } | ||
| { | ||
| const requestId = latestMessage.requestId; | ||
|
|
||
| setPendingPermissionRequests((previous) => { | ||
| if (previous.some((request) => request.requestId === requestId)) { | ||
| return previous; | ||
| } | ||
| return [ | ||
| ...previous, | ||
| { | ||
| requestId, | ||
| toolName: latestMessage.toolName || 'UnknownTool', | ||
| input: latestMessage.input, | ||
| context: latestMessage.context, | ||
| sessionId: latestMessage.sessionId || null, | ||
| receivedAt: new Date(), | ||
| }, | ||
| ]; | ||
| }); | ||
| } | ||
|
|
||
| setIsLoading(true); | ||
| setCanAbortSession(true); | ||
| setClaudeStatus({ | ||
| text: 'Waiting for permission', | ||
| tokens: 0, | ||
| can_interrupt: true, | ||
| }); | ||
| break; | ||
|
|
||
| case 'claude-permission-cancelled': | ||
| if (!latestMessage.requestId) { | ||
| break; | ||
| } | ||
| setPendingPermissionRequests((previous) => | ||
| previous.filter((request) => request.requestId !== latestMessage.requestId), | ||
| ); | ||
| break; | ||
|
|
||
| case 'claude-error': | ||
| finalizeLifecycleForCurrentView(latestMessage.sessionId, currentSessionId, selectedSession?.id); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'error', | ||
| content: `Error: ${latestMessage.error}`, | ||
| timestamp: new Date(), | ||
| }, | ||
| ]); | ||
| break; | ||
|
|
||
| case 'cursor-system': | ||
| try { | ||
| const cursorData = latestMessage.data; | ||
| if ( | ||
| cursorData && | ||
| cursorData.type === 'system' && | ||
| cursorData.subtype === 'init' && | ||
| cursorData.session_id | ||
| ) { | ||
| if (!isSystemInitForView) { | ||
| return; | ||
| } | ||
|
|
||
| if (currentSessionId && cursorData.session_id !== currentSessionId) { | ||
| setIsSystemSessionChange(true); | ||
| onNavigateToSession?.(cursorData.session_id); | ||
| return; | ||
| } | ||
|
|
||
| if (!currentSessionId) { | ||
| setIsSystemSessionChange(true); | ||
| onNavigateToSession?.(cursorData.session_id); | ||
| return; | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.warn('Error handling cursor-system message:', error); | ||
| } | ||
| break; | ||
|
|
||
| case 'cursor-user': | ||
| break; | ||
|
|
||
| case 'cursor-tool-use': | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content: `Using tool: ${latestMessage.tool} ${latestMessage.input ? `with ${latestMessage.input}` : '' | ||
| }`, | ||
| timestamp: new Date(), | ||
| isToolUse: true, | ||
| toolName: latestMessage.tool, | ||
| toolInput: latestMessage.input, | ||
| }, | ||
| ]); | ||
| break; | ||
|
|
||
| case 'cursor-error': | ||
| finalizeLifecycleForCurrentView(latestMessage.sessionId, currentSessionId, selectedSession?.id); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'error', | ||
| content: `Cursor error: ${latestMessage.error || 'Unknown error'}`, | ||
| timestamp: new Date(), | ||
| }, | ||
| ]); | ||
| break; | ||
|
|
||
| case 'cursor-result': { | ||
| const cursorCompletedSessionId = latestMessage.sessionId || currentSessionId; | ||
| const pendingCursorSessionId = sessionStorage.getItem('pendingSessionId'); | ||
|
|
||
| finalizeLifecycleForCurrentView( | ||
| cursorCompletedSessionId, | ||
| currentSessionId, | ||
| selectedSession?.id, | ||
| pendingCursorSessionId, | ||
| ); | ||
|
|
||
| try { | ||
| const resultData = latestMessage.data || {}; | ||
| const textResult = typeof resultData.result === 'string' ? resultData.result : ''; | ||
|
|
||
| if (streamTimerRef.current) { | ||
| clearTimeout(streamTimerRef.current); | ||
| streamTimerRef.current = null; | ||
| } | ||
| const pendingChunk = streamBufferRef.current; | ||
| streamBufferRef.current = ''; | ||
|
|
||
| setChatMessages((previous) => { | ||
| const updated = [...previous]; | ||
| const lastIndex = updated.length - 1; | ||
| const last = updated[lastIndex]; | ||
| const normalizedTextResult = textResult.trim(); | ||
|
|
||
| if (last && last.type === 'assistant' && !last.isToolUse && last.isStreaming) { | ||
| const finalContent = | ||
| normalizedTextResult | ||
| ? textResult | ||
| : `${last.content || ''}${pendingChunk || ''}`; | ||
| // Clone the message instead of mutating in place so React can reliably detect state updates. | ||
| updated[lastIndex] = { ...last, content: finalContent, isStreaming: false }; | ||
| } else if (normalizedTextResult) { | ||
| const lastAssistantText = | ||
| last && last.type === 'assistant' && !last.isToolUse | ||
| ? String(last.content || '').trim() | ||
| : ''; | ||
|
|
||
| // Cursor can emit the same final text through both streaming and result payloads. | ||
| // Skip adding a second assistant bubble when the final text is unchanged. | ||
| const isDuplicateFinalText = lastAssistantText === normalizedTextResult; | ||
| if (isDuplicateFinalText) { | ||
| return updated; | ||
| } | ||
|
|
||
| updated.push({ | ||
| type: resultData.is_error ? 'error' : 'assistant', | ||
| content: textResult, | ||
| timestamp: new Date(), | ||
| isStreaming: false, | ||
| }); | ||
| } | ||
| return updated; | ||
| }); | ||
| } catch (error) { | ||
| console.warn('Error handling cursor-result message:', error); | ||
| } | ||
|
|
||
| if (cursorCompletedSessionId && !currentSessionId && cursorCompletedSessionId === pendingCursorSessionId) { | ||
| setCurrentSessionId(cursorCompletedSessionId); | ||
| sessionStorage.removeItem('pendingSessionId'); | ||
| if (window.refreshProjects) { | ||
| setTimeout(() => window.refreshProjects?.(), 500); | ||
| } | ||
| } | ||
| break; | ||
| // Also route to store for non-active sessions | ||
| if (sid && sid !== activeViewSessionId) { | ||
| sessionStore.appendRealtime(sid, msg as NormalizedMessage); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| case 'cursor-output': | ||
| try { | ||
| const raw = String(latestMessage.data ?? ''); | ||
| const cleaned = raw | ||
| .replace(/\x1b\[[0-9;?]*[A-Za-z]/g, '') | ||
| .replace(/[\x00-\x08\x0B\x0C\x0E-\x1F]/g, '') | ||
| .trim(); | ||
|
|
||
| if (cleaned) { | ||
| streamBufferRef.current += streamBufferRef.current ? `\n${cleaned}` : cleaned; | ||
| if (!streamTimerRef.current) { | ||
| streamTimerRef.current = window.setTimeout(() => { | ||
| const chunk = streamBufferRef.current; | ||
| streamBufferRef.current = ''; | ||
| streamTimerRef.current = null; | ||
| appendStreamingChunk(setChatMessages, chunk, true); | ||
| }, 100); | ||
| } | ||
| } | ||
| } catch (error) { | ||
| console.warn('Error handling cursor-output message:', error); | ||
| } | ||
| break; | ||
|
|
||
| case 'claude-complete': { | ||
| const pendingSessionId = sessionStorage.getItem('pendingSessionId'); | ||
| const completedSessionId = | ||
| latestMessage.sessionId || currentSessionId || pendingSessionId; | ||
|
|
||
| finalizeLifecycleForCurrentView( | ||
| completedSessionId, | ||
| currentSessionId, | ||
| selectedSession?.id, | ||
| pendingSessionId, | ||
| ); | ||
|
|
||
| if (pendingSessionId && !currentSessionId && latestMessage.exitCode === 0) { | ||
| setCurrentSessionId(pendingSessionId); | ||
| sessionStorage.removeItem('pendingSessionId'); | ||
| console.log('New session complete, ID set to:', pendingSessionId); | ||
| } | ||
|
|
||
| if (selectedProject && latestMessage.exitCode === 0) { | ||
| safeLocalStorage.removeItem(`chat_messages_${selectedProject.name}`); | ||
| } | ||
| break; | ||
| if (msg.kind === 'stream_end') { | ||
| if (streamTimerRef.current) { | ||
| clearTimeout(streamTimerRef.current); | ||
| streamTimerRef.current = null; | ||
| } | ||
|
|
||
| case 'codex-response': { | ||
| const codexData = latestMessage.data; | ||
| if (!codexData) { | ||
| break; | ||
| } | ||
|
|
||
| if (codexData.type === 'item') { | ||
| switch (codexData.itemType) { | ||
| case 'agent_message': | ||
| if (codexData.message?.content?.trim()) { | ||
| const content = decodeHtmlEntities(codexData.message.content); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content, | ||
| timestamp: new Date(), | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
|
|
||
| case 'reasoning': | ||
| if (codexData.message?.content?.trim()) { | ||
| const content = decodeHtmlEntities(codexData.message.content); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content, | ||
| timestamp: new Date(), | ||
| isThinking: true, | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
|
|
||
| case 'command_execution': | ||
| if (codexData.command) { | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content: '', | ||
| timestamp: new Date(), | ||
| isToolUse: true, | ||
| toolName: 'Bash', | ||
| toolInput: codexData.command, | ||
| toolResult: codexData.output || null, | ||
| exitCode: codexData.exitCode, | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
|
|
||
| case 'file_change': | ||
| if (codexData.changes?.length > 0) { | ||
| const changesList = codexData.changes | ||
| .map((change: { kind: string; path: string }) => `${change.kind}: ${change.path}`) | ||
| .join('\n'); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content: '', | ||
| timestamp: new Date(), | ||
| isToolUse: true, | ||
| toolName: 'FileChanges', | ||
| toolInput: changesList, | ||
| toolResult: { | ||
| content: `Status: ${codexData.status}`, | ||
| isError: false, | ||
| }, | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
|
|
||
| case 'mcp_tool_call': | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'assistant', | ||
| content: '', | ||
| timestamp: new Date(), | ||
| isToolUse: true, | ||
| toolName: `${codexData.server}:${codexData.tool}`, | ||
| toolInput: JSON.stringify(codexData.arguments, null, 2), | ||
| toolResult: codexData.result | ||
| ? JSON.stringify(codexData.result, null, 2) | ||
| : codexData.error?.message || null, | ||
| }, | ||
| ]); | ||
| break; | ||
|
|
||
| case 'error': | ||
| if (codexData.message?.content) { | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'error', | ||
| content: codexData.message.content, | ||
| timestamp: new Date(), | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
|
|
||
| default: | ||
| console.log('[Codex] Unhandled item type:', codexData.itemType, codexData); | ||
| } | ||
| } | ||
|
|
||
| if (codexData.type === 'turn_complete') { | ||
| finalizeLifecycleForCurrentView(latestMessage.sessionId, currentSessionId, selectedSession?.id); | ||
| if (sid) { | ||
| if (accumulatedStreamRef.current) { | ||
| sessionStore.updateStreaming(sid, accumulatedStreamRef.current, provider); | ||
| } | ||
|
|
||
| if (codexData.type === 'turn_failed') { | ||
| finalizeLifecycleForCurrentView(latestMessage.sessionId, currentSessionId, selectedSession?.id); | ||
| setChatMessages((previous) => [ | ||
| ...previous, | ||
| { | ||
| type: 'error', | ||
| content: codexData.error?.message || 'Turn failed', | ||
| timestamp: new Date(), | ||
| }, | ||
| ]); | ||
| } | ||
| break; | ||
| sessionStore.finalizeStreaming(sid); | ||
| } | ||
| accumulatedStreamRef.current = ''; | ||
| streamBufferRef.current = ''; | ||
| return; |
Contributor
There was a problem hiding this comment.
The stream buffer can't be global anymore.
The timer callback captures a single sid, but accumulatedStreamRef keeps growing across all incoming deltas. If session A arms the timer and session B emits before it fires, B's text gets written into A. On top of that, the background path appends raw stream_delta messages and then also finalizes the synthetic stream entry, so background sessions keep both chunk deltas and the finalized assistant message. updateStreaming() should also use msg.provider, not the currently selected provider.
Also applies to: 246-255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 183 - 217,
The streaming buffers and timer must be tracked per-session and use the incoming
msg.provider rather than the global provider: replace the single shared
streamTimerRef/accumulatedStreamRef/streamBufferRef with a Map keyed by sid
(e.g. streamStateBySid) so each session accumulates its own text and schedules
its own timer that closes over that sid and msg.provider when calling
sessionStore.updateStreaming; stop appending raw 'stream_delta' chunks to
background sessions (sid !== activeViewSessionId) — instead only produce the
finalized assistant entry on 'stream_end' for background sessions; on
'stream_end' read and clear the per-sid buffer, call
sessionStore.updateStreaming(sid, accumulated, msg.provider) then
sessionStore.finalizeStreaming(sid) and delete the per-sid state to avoid
cross-session contamination.
Comment on lines
+227
to
243
| case 'session_created': { | ||
| const newSessionId = msg.newSessionId; | ||
| if (!newSessionId) break; | ||
|
|
||
| if (codexPendingSessionId && !currentSessionId) { | ||
| setCurrentSessionId(codexActualSessionId); | ||
| setIsSystemSessionChange(true); | ||
| if (codexActualSessionId) { | ||
| onNavigateToSession?.(codexActualSessionId); | ||
| if (!currentSessionId || currentSessionId.startsWith('new-session-')) { | ||
| sessionStorage.setItem('pendingSessionId', newSessionId); | ||
| if (pendingViewSessionRef.current && !pendingViewSessionRef.current.sessionId) { | ||
| pendingViewSessionRef.current.sessionId = newSessionId; | ||
| } | ||
| sessionStorage.removeItem('pendingSessionId'); | ||
| } | ||
|
|
||
| if (selectedProject) { | ||
| safeLocalStorage.removeItem(`chat_messages_${selectedProject.name}`); | ||
| setCurrentSessionId(newSessionId); | ||
| onReplaceTemporarySession?.(newSessionId); | ||
| setPendingPermissionRequests((prev) => | ||
| prev.map((r) => (r.sessionId ? r : { ...r, sessionId: newSessionId })), | ||
| ); | ||
| } | ||
| onNavigateToSession?.(newSessionId); | ||
| break; |
Contributor
There was a problem hiding this comment.
Scope these UI side effects to the active session.
After routing the message into the session store, these branches still mutate global loading/status/token-budget/navigation/permission state for any sid. A background session can now clear the visible spinner, overwrite the active status/token budget, open the wrong permission prompt, or navigate the user unexpectedly.
Also applies to: 246-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatRealtimeHandlers.ts` around lines 227 - 243,
The session_created branch in useChatRealtimeHandlers currently applies UI side
effects for any session id; guard these mutations so they only run when the
created session corresponds to the active/temporary session. Concretely, in the
case 'session_created' handler (and similarly in the other session-related
branches between the noted range), wrap the code that calls
sessionStorage.setItem, mutates pendingViewSessionRef.current.sessionId,
setCurrentSessionId, onReplaceTemporarySession, setPendingPermissionRequests,
and onNavigateToSession with a check that the newSessionId matches the active
session context (for example: newSessionId === currentSessionId ||
currentSessionId?.startsWith('new-session-')). Apply the same scoping pattern to
the other affected branches so background/session store updates do not clear
spinners, overwrite token/ status, open prompts, or navigate when the session is
not the visible/active one.
Comment on lines
+165
to
+170
| // Reset viewHiddenCount when store messages change | ||
| const prevStoreLenRef = useRef(0); | ||
| if (storeMessages.length !== prevStoreLenRef.current) { | ||
| prevStoreLenRef.current = storeMessages.length; | ||
| if (viewHiddenCount > 0) setViewHiddenCount(0); | ||
| } |
Contributor
There was a problem hiding this comment.
This makes /rewind self-cancel.
viewHiddenCount is cleared on any store length change. The assistant follow-up appended right after rewindMessages() changes the length immediately, so the hidden messages reappear right away.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatSessionState.ts` around lines 165 - 170, The
current reset clears viewHiddenCount on any storeMessages length change which
makes a follow-up appended immediately after rewindMessages() re-show hidden
messages; to fix, add a ref flag (e.g., isRewindingRef) and set
isRewindingRef.current = true before calling rewindMessages() and clear it after
the rewind completes, then update the existing logic that compares
prevStoreLenRef and storeMessages to only call setViewHiddenCount(0) when
isRewindingRef.current is false (leave prevStoreLenRef update as-is); reference
symbols: prevStoreLenRef, storeMessages, viewHiddenCount, setViewHiddenCount,
rewindMessages(), and introduce isRewindingRef to skip clearing during
rewind-originated changes.
Comment on lines
+199
to
+202
| const clearMessages = useCallback(() => { | ||
| if (!activeSessionId) return; | ||
| sessionStore.clearRealtime(activeSessionId); | ||
| }, [activeSessionId, sessionStore]); |
Contributor
There was a problem hiding this comment.
clearMessages() no longer clears loaded transcripts.
This only clears realtimeMessages. Once a session has serverMessages, the merged view still renders the full history, so the new /clear path effectively does nothing for already-loaded sessions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatSessionState.ts` around lines 199 - 202,
clearMessages currently only calls sessionStore.clearRealtime(activeSessionId),
which leaves loaded server/transcript messages intact; update clearMessages to
clear both realtime and server/transcript message stores (e.g., call
sessionStore.clearRealtime(activeSessionId) and
sessionStore.clearServerMessages(activeSessionId) or
sessionStore.clearTranscript(activeSessionId) depending on the store API) and
then notify/update the merged view (e.g., emit a change or call
sessionStore.refreshSession(activeSessionId)) so the UI no longer renders the
old history; also include any newly used sessionStore methods in the useCallback
dependency array.
Comment on lines
+374
to
+391
| // Fetch from server → store updates → chatMessages re-derives automatically | ||
| setIsLoadingSessionMessages(true); | ||
| sessionStore.fetchFromServer(selectedSession.id, { | ||
| provider: (selectedSession.__provider || provider) as SessionProvider, | ||
| projectName: selectedProject.name, | ||
| projectPath: selectedProject.fullPath || selectedProject.path || '', | ||
| limit: MESSAGES_PER_PAGE, | ||
| offset: 0, | ||
| }).then(slot => { | ||
| if (slot) { | ||
| setHasMoreMessages(slot.hasMore); | ||
| setTotalMessages(slot.total); | ||
| if (slot.tokenUsage) setTokenBudget(slot.tokenUsage as Record<string, unknown>); | ||
| } | ||
|
|
||
| setTimeout(() => { | ||
| isLoadingSessionRef.current = false; | ||
| }, 250); | ||
| }; | ||
|
|
||
| loadMessages(); | ||
| setIsLoadingSessionMessages(false); | ||
| }).catch(() => { | ||
| setIsLoadingSessionMessages(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
Ignore stale fetches after a session switch.
This async load writes hasMoreMessages, totalMessages, tokenBudget, and the loading flag unconditionally. If the user switches sessions before it resolves, the old response can overwrite the newly selected session's UI state.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/hooks/useChatSessionState.ts` around lines 374 - 391,
Before calling sessionStore.fetchFromServer, capture the session id in a local
variable (e.g. const fetchSessionId = selectedSession.id) and in both the then
and catch handlers only update setHasMoreMessages, setTotalMessages,
setTokenBudget and setIsLoadingSessionMessages if the current selectedSession.id
still equals fetchSessionId; do this around the existing
sessionStore.fetchFromServer(...) call and its promise handlers so stale
responses from the previous session are ignored.
Comment on lines
+206
to
+218
| // On WebSocket reconnect, re-fetch the current session's messages from the server | ||
| // so missed streaming events are shown. Also reset isLoading. | ||
| const handleWebSocketReconnect = useCallback(async () => { | ||
| if (!selectedProject || !selectedSession) return; | ||
| const provider = (localStorage.getItem('selected-provider') as any) || 'claude'; | ||
| const messages = await loadSessionMessages(selectedProject.name, selectedSession.id, false, provider); | ||
| if (messages && messages.length > 0) { | ||
| setChatMessages(messages); | ||
| } | ||
| // Reset loading state — if the session is still active, new WebSocket messages will | ||
| // set it back to true. If it died, this clears the permanent frozen state. | ||
| const providerVal = (localStorage.getItem('selected-provider') as SessionProvider) || 'claude'; | ||
| await sessionStore.refreshFromServer(selectedSession.id, { | ||
| provider: (selectedSession.__provider || providerVal) as SessionProvider, | ||
| projectName: selectedProject.name, | ||
| projectPath: selectedProject.fullPath || selectedProject.path || '', | ||
| }); | ||
| setIsLoading(false); | ||
| setCanAbortSession(false); | ||
| }, [selectedProject, selectedSession, loadSessionMessages, setChatMessages, setIsLoading, setCanAbortSession]); | ||
| }, [selectedProject, selectedSession, sessionStore, setIsLoading, setCanAbortSession]); |
Contributor
There was a problem hiding this comment.
Potential race condition during WebSocket reconnect while streaming.
Per the context snippet from useSessionStore.ts, refreshFromServer clears realtimeMessages (line 330 in store). If a stream is active when reconnect occurs, the accumulated streaming content in realtimeMessages will be lost before the server response arrives.
Consider either:
- Preserving
realtimeMessagesuntil server fetch completes - Checking if streaming is active before calling
refreshFromServer - Accepting this as expected behavior (reconnect implies stream interruption)
If the current behavior is intentional (reconnect resets everything), adding a comment would clarify this design choice.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/components/chat/view/ChatInterface.tsx` around lines 206 - 218,
handleWebSocketReconnect currently calls sessionStore.refreshFromServer which
clears realtimeMessages and can drop in-progress stream content; guard against
that by checking for an active streaming flag before refreshing (e.g., use
sessionStore.isStreaming(selectedSession.id) or selectedSession.isStreaming) and
skip or delay calling sessionStore.refreshFromServer while streaming is active,
or alternatively preserve realtimeMessages by copying them into a temp buffer
before refresh and restoring after the fetch completes; update the
handleWebSocketReconnect logic to use that guard/preserve approach and add a
short comment documenting the chosen behavior (references:
handleWebSocketReconnect, sessionStore.refreshFromServer, realtimeMessages,
selectedSession).
Comment on lines
+386
to
+403
| const finalizeStreaming = useCallback((sessionId: string) => { | ||
| const slot = storeRef.current.get(sessionId); | ||
| if (!slot) return; | ||
| const streamId = `__streaming_${sessionId}`; | ||
| const idx = slot.realtimeMessages.findIndex(m => m.id === streamId); | ||
| if (idx >= 0) { | ||
| const stream = slot.realtimeMessages[idx]; | ||
| slot.realtimeMessages = [...slot.realtimeMessages]; | ||
| slot.realtimeMessages[idx] = { | ||
| ...stream, | ||
| id: `text_${Date.now()}_${Math.random().toString(36).slice(2, 8)}`, | ||
| kind: 'text', | ||
| role: 'assistant', | ||
| }; | ||
| recomputeMergedIfNeeded(slot); | ||
| notify(sessionId); | ||
| } | ||
| }, [notify]); |
Contributor
There was a problem hiding this comment.
Finalized streams still can't dedupe against persisted history.
finalizeStreaming() swaps __streaming_${sessionId} for a brand-new text_* id. Since the store dedupes by id, the server-fetched assistant message can never collapse this realtime copy, so completed answers duplicate after the next history fetch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/stores/useSessionStore.ts` around lines 386 - 403, finalizeStreaming
currently replaces the streaming message with a brand-new random id so
server-fetched assistant messages can't dedupe; instead preserve a stable
client-side dedupe key and avoid mutating to a random id: in finalizeStreaming
(operating on slot.realtimeMessages) stop generating the random `text_*` id and
either keep the original `__streaming_${sessionId}` as a clientGeneratedId (e.g.
set a new property clientGeneratedId = stream.id) or assign a deterministic
pending id, mark kind:'text' and role:'assistant', then update the dedupe/merge
logic used by recomputeMergedIfNeeded to prefer server messages by matching on
clientGeneratedId (when present) before falling back to id comparison so
persisted history collapses the realtime message into the server record and
prevents duplication; call notify(sessionId) as before.
viper151
changed the title
blackmammoth
approved these changes
Mar 19, 2026
6 tasks
This was referenced Mar 23, 2026
This was referenced Apr 14, 2026
This was referenced Apr 27, 2026
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
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What was changed in this PR
Add provider adapter layer (server/providers/) with registry pattern
Add unified REST endpoint: GET /api/sessions/:id/messages?provider=...
Add frontend session-keyed store (useSessionStore)
Add normalizedToChatMessages converter (useChatMessages)
Wire unified store into ChatInterface, useChatSessionState, useChatRealtimeHandlers
Summary by CodeRabbit
New Features
Refactor