feat(ai): AiChatView store-first rendering cutover#2079
Merged
Conversation
Rewires AiChatView.tsx to render from useConversationMessagesStore + usePendingStreamsStore via a new facade layer, per the Deterministic Chat Rendering epic (PR 4 of that epic). - loadMessagesForConversation now commits to the store via startLoad/applyLoad/failLoad (generation-gated); useChat's own setMessages stays wired only for its own transport/regenerate bookkeeping, never for render. Deletes skipLoadEffectRef/loadRequestedIdRef/shouldApplyLoadedMessages/ mergeServerAndPending from this surface (still used by GlobalAssistantView/ SidebarChatTab, not yet cut over). - New facade hooks (apps/web/src/hooks/useRenderedMessages.ts, useActiveStream.ts, conversationMessagesActions.ts) are the only sanctioned way AiChatView touches the two stores, per the epic's container-agnostic consumer rule — no direct getState()/useShallow reach-ins from the component. - Sends go through buildUserMessage (client cuid, parts-form) + addOptimisticSend so the bubble renders instantly; useOwnStreamMirror is mounted to mirror the live assistant reply into the render-facing store. - Socket callbacks (user message, edit, delete, stream complete) write to the store instead of useChat's setMessages. Local edit/delete/retry (via useMessageActions, left untouched since it's shared with not-yet-cut-over surfaces) get an additional store write after the network call succeeds, since the sender's own tab never receives its own broadcast back. - Scopes the saveMessageToDatabase/saveGlobalAssistantMessageToDatabase upsert: a client-supplied id colliding with a row in a DIFFERENT conversation is now rejected (ON CONFLICT DO UPDATE ... WHERE conversationId = caller's) instead of silently overwriting/re-parenting it. Adds resolveMessageId (isCuid validation) at both routes' enqueue point so a malformed client id never reaches the upsert. Known gap (filed as D.1 on the PR board): AiChatView.test.tsx and AiChatView.realConversations.test.tsx assert against the old useChat-as-render-source architecture and were not updated — React render tests crash the vitest worker in this .pu worktree (environmental, dual-React dispatcher null), so they need a working checkout/CI pass before merge. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LWCLe73VEoAVdbUncCtd1R
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesAI message flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AiChatView
participant conversationMessagesActions
participant useConversationMessagesStore
participant useActiveStream
participant ChatLayout
AiChatView->>conversationMessagesActions: load conversation and add optimistic user message
conversationMessagesActions->>useConversationMessagesStore: update cached conversation messages
useActiveStream-->>AiChatView: return conversation-scoped streams
AiChatView->>conversationMessagesActions: apply confirmed assistant message
AiChatView->>ChatLayout: render plainMessages
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: 079173ba5e
ℹ️ 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
+13
to
+14
| export const resolveMessageId = (clientId: string | undefined | null): string => | ||
| clientId && isCuid(clientId) ? clientId : createId(); |
There was a problem hiding this comment.
When GlobalAssistantView or SidebarChatTab sends with the existing sendMessage({ text, files }) shorthand, the AI SDK supplies its default non-cuid message id; this helper now treats that as malformed and stores the user row under a fresh cuid, while the route still broadcasts and the active useChat state keep using the original id. In those paths, same-session edits/deletes or ask_user result merges target an id that was never persisted, and other tabs can cache a broadcast message id that does not exist in the DB. Please either update all remaining senders to pass cuid ids or propagate the server-resolved id consistently.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 003b546. You're right — the SDK's default generateId() produces a 16-char id from a mixed-case alphabet (0-9A-Za-z), which fails isCuid's lowercase-only regex essentially every time (verified in node_modules/@ai-sdk/provider-utils). resolveMessageId no longer requires cuid2 format — it now validates a general safe-opaque-identifier shape (/^[A-Za-z0-9_-]{1,128}$/), which covers both cuid2 ids (buildUserMessage) and the SDK's own default ids (GlobalAssistantView/SidebarChatTab, still on the shorthand). The scoped upsert (this PR's actual security fix — conversation-scoped WHERE on the ON CONFLICT DO UPDATE) is what protects against a colliding id crossing conversations; it doesn't depend on the id's format, so relaxing this validation doesn't reopen that. Added regression tests pinning both the SDK-default-shape case and a couple of unsafe-shape rejections (path traversal, oversized) in resolveMessageId.test.ts.
Comment on lines
+222
to
+226
| const generation = conversationMessagesActions.startLoad(conversationId); | ||
| // True while this call's generation is still the newest `startLoad` for this | ||
| // conversation — a newer load (rapid conversation switching, or a second call | ||
| // for the same id) bumps the generation and this starts reading false. | ||
| const isCurrent = () => conversationMessagesActions.isLoadCurrent(conversationId, generation); |
There was a problem hiding this comment.
This freshness check is only per conversation, so a slow load for conversation A remains “current” for A even after the user switches to conversation B. When A resolves it still reaches applyLoad/setMessages, clearing the shared loading state and overwriting useChat's bookkeeping copy while B is active; the next retry/send in B can then operate with A's message history. Keep the generation gate, but also verify the loaded id is still the active currentConversationId like the previous global stale-load guard did.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 003b546. Added isActiveLoad() — true only while the store's per-conversation generation gate holds AND conversationId === currentConversationIdRef.current. Kept the pure generation gate (isCurrent()) for committing to useConversationMessagesStore via applyLoad/failLoad — that's correct to apply even after a switch, since the store caches every conversation independently (switching back to A later should show fresh data without a re-fetch). isActiveLoad() now gates the three writes that are NOT conversation-keyed and therefore CAN'T tolerate a stale load winning: setMessages (useChat's single local array), and the loading/error UI state (setIsLoadingMessages, setMessagesLoadError) — exactly the "still the active currentConversationId" check the old ref-based guard did.
… loads
1. resolveMessageId no longer requires strict cuid2 format. The AI SDK's
default id generator (used by every sender still on the
sendMessage({text, files}) shorthand — GlobalAssistantView, SidebarChatTab,
not yet cut over to buildUserMessage) produces a mixed-case 16-char id that
fails isCuid's lowercase-only regex. Requiring it split the id useChat's
local state/broadcasts used from the one actually persisted, so a
same-session edit/delete or ask_user merge would target an id that was
never saved. Now validates SHAPE (safe, bounded, opaque identifier)
instead — the scoped upsert is what actually protects against a colliding
id crossing conversations, not the id's format.
2. loadMessagesForConversation's generation gate was scoped only to the
loading conversation itself, not to whether it's still the one on screen.
A slow load for conversation A resolving after the user switched to B
would still call setMessages/clear the loading state, overwriting B's
useChat bookkeeping and its own in-flight loading indicator. Added
isActiveLoad() (generation-current AND still the active conversationId) to
gate the non-conversation-keyed writes (useChat's setMessages, the
loading/error UI state) while leaving the store commit (applyLoad/failLoad)
on the pure per-conversation generation gate — a conversation the user
navigated away from should still get its cache updated.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LWCLe73VEoAVdbUncCtd1R
# Conflicts: # apps/web/src/lib/ai/core/message-utils.ts
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/web/src/lib/ai/core/message-utils.ts (1)
546-611: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winMention-notify fires even when the scoped upsert rejects the write.
When
result.length === 0(client-suppliedmessageIdcollided with a row in a different conversation), the upsert is a no-op — nothing was persisted. The mention-notify block right below is not gated on this and still fires for that unsaved content, notifying users about a message that doesn't exist in the conversation.Separately: neither this function nor its caller (
apps/web/src/app/api/ai/chat/route.ts) checks theresult/return value, so even a non-malicious collision silently drops the message with only a server-side warning — no signal reaches the user or the request handler.🐛 Proposed fix for the mention-notify gap
- // Fire-and-forget mention notifications for assistant messages only - if (mentionNotify && role === 'assistant' && content.trim()) { + // Fire-and-forget mention notifications for assistant messages only — skip when the + // upsert itself was rejected (cross-conversation id collision): there is nothing to + // notify anyone about since this content was never actually persisted. + if (result.length > 0 && mentionNotify && role === 'assistant' && content.trim()) { void notifyMentionedUsers({🤖 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/message-utils.ts` around lines 546 - 611, Gate the mention notification block in the message-save function on a successful upsert by requiring result.length > 0, so rejected cross-conversation collisions cannot notify users about unsaved content. Also propagate the rejected-write signal from this function to its caller in the chat route, allowing non-malicious collisions to be surfaced instead of silently dropped; preserve the existing warning and successful-save notification behavior.apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx (1)
222-293: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReset loading state when switching to a fresh conversation
AiChatView.tsx:240-293, 733-746
Switching away from an in-flight load to a brand-newisPersisted === falseconversation never starts a replacement load, so the previous load’sfinallynever clearsisLoadingMessages. That leaves the new chat stuck in loading/docked state and disables the input. ResetisLoadingMessagesandmessagesLoadErrorwhen landing on an unpersisted conversation.🤖 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/components/layout/middle-content/page-views/ai-page/AiChatView.tsx` around lines 222 - 293, The conversation-switching logic must reset loading UI for a fresh unpersisted conversation. Update the effect or handler that responds to `isPersisted === false` near the conversation initialization flow to set `isLoadingMessages` to false and clear `messagesLoadError`, ensuring a prior conversation’s in-flight load cannot leave the new chat disabled or stuck in a loading state.
🤖 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/chat/__tests__/credit-gate.test.ts`:
- Line 167: Replace the unconditional isCuid mocks in credit-gate.test.ts
(167-167), mcp-scope-tool-filtering.test.ts (181-181), mcp-scope.test.ts
(165-165), and sandbox-github-suppression.test.ts (183-183) with realistic valid
CUID2 fixtures or a shape-aware predicate, while preserving rejection coverage
for malformed conversation IDs.
In `@apps/web/src/lib/ai/streams/resolveMessageId.ts`:
- Around line 33-34: Rename apps/web/src/lib/ai/streams/resolveMessageId.ts to
resolve-message-id.ts. Rename
apps/web/src/lib/ai/streams/__tests__/resolveMessageId.test.ts to
resolve-message-id.test.ts and update its import to ../resolve-message-id;
update the import in apps/web/src/app/api/ai/global/[id]/messages/route.ts to
resolve-message-id, preserving resolveMessageId behavior.
---
Outside diff comments:
In
`@apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx`:
- Around line 222-293: The conversation-switching logic must reset loading UI
for a fresh unpersisted conversation. Update the effect or handler that responds
to `isPersisted === false` near the conversation initialization flow to set
`isLoadingMessages` to false and clear `messagesLoadError`, ensuring a prior
conversation’s in-flight load cannot leave the new chat disabled or stuck in a
loading state.
In `@apps/web/src/lib/ai/core/message-utils.ts`:
- Around line 546-611: Gate the mention notification block in the message-save
function on a successful upsert by requiring result.length > 0, so rejected
cross-conversation collisions cannot notify users about unsaved content. Also
propagate the rejected-write signal from this function to its caller in the chat
route, allowing non-malicious collisions to be surfaced instead of silently
dropped; preserve the existing warning and successful-save notification
behavior.
🪄 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: 3f6d28b1-4c5e-46b7-9ae9-0bd220eb31a5
📒 Files selected for processing (18)
apps/web/src/app/api/ai/chat/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/chat/__tests__/mcp-scope-tool-filtering.test.tsapps/web/src/app/api/ai/chat/__tests__/mcp-scope.test.tsapps/web/src/app/api/ai/chat/__tests__/sandbox-github-suppression.test.tsapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/conversation-id-resolution.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/hooks/conversationMessagesActions.tsapps/web/src/hooks/useActiveStream.tsapps/web/src/hooks/useRenderedMessages.tsapps/web/src/lib/ai/core/__tests__/message-utils-mention-notify.test.tsapps/web/src/lib/ai/core/__tests__/message-utils-scoped-upsert.test.tsapps/web/src/lib/ai/core/message-utils.tsapps/web/src/lib/ai/streams/__tests__/resolveMessageId.test.tsapps/web/src/lib/ai/streams/resolveMessageId.ts
Prompted by three independent adversarial review passes (self-review agents + Codex) converging on the same defects after the initial PR push: 1. saveMessageToDatabase/saveGlobalAssistantMessageToDatabase's scoped upsert silently no-op'd on a rejected cross-conversation collision (warn only, never thrown) — every caller (both chat routes) fell through as if the save succeeded: bumping conversations.lastMessageAt, auditing a data.write, firing mention notifications, and letting the model answer a message that was never persisted. Now throws MessageConversationConflictError after warning; both routes' existing catch blocks already turn that into a user-visible failure response (verified their credit-hold release runs via the routes' own outer `finally`, so this doesn't introduce a hold leak). 2. The same upsert's WHERE only scoped by conversationId, not role. Since `set` never writes role (immutable) or conversationId is required but not role, a 'user'-role save whose client id collided with an EXISTING 'assistant' row in the SAME conversation could still overwrite that row's content — content-spoofing an AI reply within a conversation the client already has write access to (ids are visible in conversation history, not secret). WHERE now also requires role to match. 3. resolveMessageId's freshly-minted id (when the client-supplied one is absent or fails the safe-id-shape check) was never written back onto `userMessage` — the DB save used the new id but the chat:user_message broadcast still carried the original (rejected) one, producing an inconsistent id between what other tabs render and what got persisted. Now reassigns `userMessage.id` immediately after resolving it. 4. AiChatView's onStreamComplete "COMMIT by id" path reused conversationMessagesActions.applyRemoteUserMessage, whose store-level contract is "no-op if the id is already confirmed" (correct for a genuine user message, whose content never changes after creation). But an existing row under this id is NOT proof of complete content for an assistant reply — a DB reload mid-stream can seed `messages` with a 'streaming'-placeholder/half-streamed snapshot under the same id, and the whole point of this commit is to replace that with the full recovered text. Added applyConfirmedMessage (upsert-by-id: replace if present, append if absent) to useConversationMessagesStore + the conversationMessagesActions facade, and switched both onStreamComplete commit sites to it. 5. Extended the socket handlers (onUserMessage/onMessageEdited/ onMessageDeleted) and onStreamComplete's commit branches to also write to useChat via setMessages, using the same pure functions the old code used (applyMessageEdit/applyMessageDelete) — not a NEW rail-11 two-way sync (both writes are independent terminal applications of the SAME upstream event, the same shape loadMessagesForConversation's dual-write already established and the epic architecture sanctions for transport bookkeeping), but required so `regenerate()` (which indexes useChat's own local array directly) and useAskUserAnswering (switched to read plainMessages instead of useChat's messages, for the same reason) don't operate on history that's gone stale relative to a remote edit/delete/ message on a shared conversation. Also extracted getAssistantMessagesAfterLastUser (apps/web/src/lib/ai/streams/) as the single source for "which assistant replies does a retry discard" — previously duplicated verbatim between useMessageActions.handleRetry and AiChatView's wrapper against two different message arrays, independently flagged by two review passes as a drift risk. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LWCLe73VEoAVdbUncCtd1R
…t use it CodeRabbit review finding: these 7 test files' @paralleldrive/cuid2 mock had an unconditional isCuid: () => true added earlier in this branch's history, back when resolveMessageId required strict cuid2 format. That requirement was already replaced with a shape-only check (SAFE_ID_PATTERN) that doesn't call isCuid at all — the mock addition is dead weight now, and an unconditional true would have masked a real regression in the OTHER, pre-existing isCuid(conversationId) validation these routes also do, had any of these tests actually exercised that path. Removed; all 7 files still pass using their original (pre-branch) mock as-is. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LWCLe73VEoAVdbUncCtd1R
Owner
Author
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab347b2741
ℹ️ 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
+284
to
+286
| conversationMessagesActions.failLoad(conversationId, generation); | ||
| // Keep the messages the user was already looking at — never silently blank on failure. | ||
| setMessagesLoadError(err instanceof Error ? err : new Error('Failed to load messages')); | ||
| // Scoped to the active load: an error loading a conversation the user has |
There was a problem hiding this comment.
When a message load for conversation A is in flight and the user clicks New Chat or otherwise moves to a fresh unpersisted conversation, the load-on-select effect returns without starting another load. The old load then reaches this finally with isActiveLoad() false, so setIsLoadingMessages(false) never runs and the new empty chat remains in the loading/disabled state indefinitely. Reset the loading flag when adopting an unpersisted identity or when the active load is abandoned.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 4b88ae5. You're right — loadMessagesForConversation never runs for an unpersisted id (the early-return at its top skips it), so if a load for the previous conversation was still in flight when the user switched to a fresh/New Chat identity, that load's own isActiveLoad() gate reads false on resolve and never clears isLoadingMessages. Nothing else was going to reset it either, since no new load starts for the unpersisted id. Fix: the identity effect now resets isLoadingMessages/messagesLoadError directly whenever it sees !isPersisted, since that branch already establishes as a fact that no load will run to do it itself. Left this thread open per the loop's auto-resolve-safety rule (fixed during, not before, this convergence pass) — please re-verify.
Comment on lines
+46
to
+49
| pendingMutationsSinceLoad: [ | ||
| ...existing.pendingMutationsSinceLoad, | ||
| { type: 'remoteMessage', message: event.message }, | ||
| ], |
There was a problem hiding this comment.
When a stream completion races an in-flight reload, this writes the full assistant message immediately but records it as a remoteMessage; replayPendingMutations handles that by appending only when the id is absent. If the reload response already contains the same assistant id with stale or partial content, applyLoad keeps the stale row and overwrites the just-applied full completion. Record a replace/upsert mutation here, or make this replay path replace by id for confirmed assistant messages.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Fixed in 4b88ae5. Confirmed the gap: applyConfirmedMessage and applyRemoteUserMessage were both recording the same { type: 'remoteMessage', message } pending-mutation shape, and replayPendingMutations replayed every remoteMessage as append-if-absent — correct for applyRemoteUserMessage (a user message's content never changes, so an existing id in the load snapshot already has the right content) but wrong for applyConfirmedMessage (an assistant reply confirmation, whose content CAN legitimately need to replace a stale/partial row the load snapshot picked up mid-stream). Split it into a new confirmedMessage mutation type that replays as upsert-by-id instead, so a load racing a stream completion can no longer let a staler snapshot row win over the just-confirmed full content. Added replay test coverage for the upsert-over-existing-stale-content case. Left open per the loop's auto-resolve-safety rule — please re-verify.
…med-message replay - AiChatView: reset isLoadingMessages/messagesLoadError when the identity effect sees an unpersisted conversation. loadMessagesForConversation never runs for an unpersisted id, so an in-flight load for the PREVIOUS conversation that resolves after the switch can no longer reach its own isActiveLoad() gate to clear these — leaving a fresh New Chat view stuck showing a loading state indefinitely. - Split the PendingMutation replay shape: confirmedMessage (assistant reply confirmation) now replays via replayPendingMutations as upsert-by-id, instead of reusing remoteMessage's append-if-absent replay. A load snapshot racing a stream-completion confirmation could read a stale/partial row under the same id; append-if-absent silently kept that stale copy instead of applying the just-confirmed full content. Both found by chatgpt-codex-connector during PR review (P2).
CI's Unit Tests job failed the 100% coverage threshold on useConversationMessagesStore.ts (90.9% lines, 80% functions) — the two new store methods added in this PR had no tests. Added wiring-only smoke tests matching the existing style in this file.
apps/web's ~930-file coverage-instrumented suite occasionally crashes a vitest worker with "Ineffective mark-compacts near heap limit — JavaScript heap out of memory" (confirmed via two separate CI runs on this branch; not present on master's last run at this branch's base commit, so the suite's memory footprint was already marginal). ubuntu-latest runners have 16GB RAM; Node's default per-process heap ceiling in a constrained container falls well short of that. Raise it to 8GB via NODE_OPTIONS on the coverage step, well within the runner's actual budget.
The prior NODE_OPTIONS=--max-old-space-size bump in ci.yml had no effect: vitest's default 'threads' pool runs workers as node:worker_threads, which do not inherit --max-old-space-size from NODE_OPTIONS the way a separate process does (confirmed — CI's log showed NODE_OPTIONS correctly set in the environment, but the same heap-limit crash recurred identically). 'forks' spawns real child processes, which do read NODE_OPTIONS at startup. Verified locally: 124 files / 1503 tests pass with --pool=forks across src/stores, src/lib/ai/streams, and src/hooks.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/web/src/lib/ai/streams/getAssistantMessagesAfterLastUser.ts (1)
11-17: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider using
findLastIndexinstead ofmap().lastIndexOf().You can avoid allocating an intermediate array of roles by using
findLastIndex.♻️ Proposed refactor
export const getAssistantMessagesAfterLastUser = <T extends Pick<UIMessage, 'id' | 'role'>>( messages: readonly T[], ): T[] => { - const lastUserMsgIndex = messages.map((m) => m.role).lastIndexOf('user'); + const lastUserMsgIndex = messages.findLastIndex((m) => m.role === 'user'); if (lastUserMsgIndex === -1) return []; return messages.slice(lastUserMsgIndex + 1).filter((m) => m.role === 'assistant'); };🤖 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/streams/getAssistantMessagesAfterLastUser.ts` around lines 11 - 17, Update getAssistantMessagesAfterLastUser to locate the final user message with findLastIndex directly on messages, removing the intermediate map(...).lastIndexOf(...) allocation. Preserve the existing empty-array behavior when no user message exists and the assistant-message filtering afterward.
🤖 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.
Nitpick comments:
In `@apps/web/src/lib/ai/streams/getAssistantMessagesAfterLastUser.ts`:
- Around line 11-17: Update getAssistantMessagesAfterLastUser to locate the
final user message with findLastIndex directly on messages, removing the
intermediate map(...).lastIndexOf(...) allocation. Preserve the existing
empty-array behavior when no user message exists and the assistant-message
filtering afterward.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 41d87196-0517-4a1e-9ff0-66b9eae7dbcd
📒 Files selected for processing (22)
.github/workflows/ci.ymlapps/web/src/app/api/ai/chat/route.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/conversation-id-resolution.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/credit-gate.test.tsapps/web/src/app/api/ai/global/[id]/messages/__tests__/stream-socket-events.test.tsapps/web/src/app/api/ai/global/[id]/messages/route.tsapps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsxapps/web/src/hooks/conversationMessagesActions.tsapps/web/src/lib/ai/core/__tests__/message-utils-mention-notify.test.tsapps/web/src/lib/ai/core/__tests__/message-utils-scoped-upsert.test.tsapps/web/src/lib/ai/core/message-utils.tsapps/web/src/lib/ai/shared/hooks/useMessageActions.tsapps/web/src/lib/ai/streams/__tests__/getAssistantMessagesAfterLastUser.test.tsapps/web/src/lib/ai/streams/getAssistantMessagesAfterLastUser.tsapps/web/src/stores/__tests__/useConversationMessagesStore.test.tsapps/web/src/stores/conversationMessages/__tests__/applyConfirmedMessage.test.tsapps/web/src/stores/conversationMessages/__tests__/replayPendingMutations.test.tsapps/web/src/stores/conversationMessages/applyConfirmedMessage.tsapps/web/src/stores/conversationMessages/replayPendingMutations.tsapps/web/src/stores/conversationMessages/seedEmpty.tsapps/web/src/stores/useConversationMessagesStore.tsapps/web/vitest.config.ts
💤 Files with no reviewable changes (3)
- apps/web/src/app/api/ai/global/[id]/messages/tests/stream-socket-events.test.ts
- apps/web/src/app/api/ai/global/[id]/messages/tests/credit-gate.test.ts
- apps/web/src/app/api/ai/global/[id]/messages/tests/conversation-id-resolution.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/web/src/app/api/ai/chat/route.ts
- apps/web/src/hooks/conversationMessagesActions.ts
- apps/web/src/app/api/ai/global/[id]/messages/route.ts
- apps/web/src/lib/ai/core/tests/message-utils-scoped-upsert.test.ts
- apps/web/src/lib/ai/core/tests/message-utils-mention-notify.test.ts
- apps/web/src/components/layout/middle-content/page-views/ai-page/AiChatView.tsx
pool:'forks' (previous commit) fixed the mid-run heap-OOM, but two worker forks still crashed with the identical FATAL ERROR right at the very end of the run — during coverage report generation, not test execution (all 13,771 tests passed). apps/web's coverage config generated 'html' (full syntax-highlighted per-file pages) and 'json' (raw per-statement/per-branch maps) for ~930 files, neither of which any script or CI step consumes — coverage-report.mjs, coverage-ratchet.mjs, and the artifact-upload step only ever read coverage-summary.json. Dropping the two unused, expensive reporters removes the memory spike at its actual source.
…mory Trimming the html/json coverage reporters (previous commit) didn't fix the tail-end OOM either — the next CI run confirmed all 13,771 tests passed, but 2 forks still crashed with the identical FATAL ERROR immediately after finishing their assigned files. Root cause: the v8 coverage provider retains precise per-script coverage data for every file a process has ever executed, for that process's whole lifetime — it's never released between files within the same worker. With the default fork count (~1 per CPU) on this ~930-file suite, each long-lived worker accumulates that data across ~300 sequential files before finishing, eventually exceeding even a 10GB ceiling. Set poolOptions.forks.maxForks: 8 so each worker carries a much smaller file share (and thus far less accumulated coverage memory), and bumped the heap ceiling to 10240MB as additional headroom — safe on a 16GB runner now that no single worker is expected to approach it. (minForks must be set alongside maxForks — leaving it at its computed default conflicted with the explicit maxForks and threw "minThreads and maxThreads must not conflict" before a single test could run; caught via local repro before pushing.)
maxForks:8 (previous commit) was the wrong direction: it eliminated the
per-process heap ceiling breach, but 8 truly concurrent coverage-instrumented
forks summed past the 16GB runner's actual physical RAM, causing a genuine
OS-level OOM-kill (exit 137, "The runner has received a shutdown signal") —
strictly worse than the prior failure, since it kills the whole run rather
than surfacing 2 individual worker crashes after all tests had already
reported passing.
Re-examined the two earlier failures (default fork count, at both 8192MB
and 10240MB ceilings): both exited cleanly with code 1 ("2 unhandled errors"
from 2 forks hitting their own ceiling), never as a killed/signaled process.
That's proof the default concurrency level (~1 fork per CPU, ~3 on this
runner) does not exceed total system memory — only the per-process ceiling
was too low for whichever 1-2 forks land the heavier test files.
So: lock concurrency to a known, deliberately conservative value (maxForks:
3) instead of trusting the runner's exact CPU count or oversubscribing it,
and spend the confirmed headroom on the per-process ceiling instead —
raised to 14336MB (14GB out of the runner's 16GB, leaving ~2GB for the
Postgres service container and OS).
Verified locally: local reproduction of the exact ceiling doesn't work (this
machine has far more RAM headroom than the CI runner — a local diagnostic
run with pool:forks, maxForks:2, and a 16GB ceiling completed the full
~7500-test non-render suite with parent/fork RSS staying flat under 0.6GB
throughout), but a targeted slice run confirms minForks/maxForks:3 doesn't
break test execution itself (82 files / 916 tests green).
…ted files The last two attempts (maxForks:8 → system-level OOM-kill; maxForks:3 locked + 14GB ceiling → ~53min GC-thrashing hang, confirmed via the GitHub API showing the coverage step stuck "in_progress" with zero output until an external shutdown signal killed the whole job) both failed by tuning fork-count/heap-ceiling around the same root problem instead of addressing it: total memory required by the coverage run itself. Tried switching provider to 'istanbul' (a well-documented alternative with a fundamentally lighter memory profile than v8's precise per-script coverage) and reverted it — verified locally that istanbul's different branch-counting silently dropped several files below their 100% thresholds (e.g. src/lib/ai/streams/*.ts: lines 98.96%, branches 98.02%), which would require writing new tests to fix. Out of scope for a CI-infra fix. Found the actual lever: `coverage.all` defaults to true, instrumenting every file matched by include/exclude — apps/web/src has 1569 non-test source files vs 932 test files, meaning hundreds of files that no test ever imports were still being instrumented for a hardcoded 0% row. Set `all: false`. This can only raise or hold steady the computed percentages for every threshold (it strictly removes always-0% files from the denominator, never removes real coverage) — verified locally that the 5 gated 100%-threshold globs are unaffected, and a 124-file/1503-test local slice with the full reverted config (default fork concurrency, all:false, 8192MB ceiling — the original, cleanly-failing value, kept only as modest headroom now that the real driver is addressed) passes clean.
The previous commit (coverage.all: false) reduced total instrumented-file memory but wasn't sufficient alone: the next CI run showed the identical 2-fork tail-end crash at the 8192MB ceiling, still a clean exit 1 (never a hang or system kill) — confirming all:false genuinely helped (no thrashing this time, unlike the 14GB+locked-3-forks attempt) without fully closing the gap. Checked the log context immediately around both FATAL ERROR lines: no single unusually large test file nearby in either case, just a long run of normal small/fast tests — reinforcing that this is genuine cumulative memory growth across each fork's file share, not 1-2 outlier-heavy files. Raised the ceiling to 12288MB (up from 8192, still comfortably below the 14336MB that induced the GC-thrashing hang) and left fork concurrency at its unlocked default rather than re-pinning it — a dynamically-scaling pool should degrade more gracefully near a high ceiling than a rigidly fixed one, and the thrashing was only ever observed under that rigid lock.
Single-invocation fork-count and heap-ceiling tuning could not fix the apps/web coverage-run OOM — every combination tried (see this branch's full commit history) either crashed 1-2 forks near the end, OOM-killed the whole job at high concurrency, or destabilized the GitHub Actions runner itself under sustained memory pressure (a ~53min GC-thrashing hang and a ~12min unresponsive stall, both ending in an external "shutdown signal" before V8 ever reported its own crash). `coverage.all: false` (previous commits) measurably helped but was never sufficient alone. A reverted attempt at the 'istanbul' coverage provider (a well-documented lower-memory alternative) silently broke this PR's 100%-threshold guarantees via different branch counting. The actual fix is structural: apps/web/package.json's `test:coverage` now runs `scripts/test-coverage-sharded.mjs` instead of `vitest run --coverage` directly. It runs the suite in 3 sequential shards (vitest's native `--shard` flag) so each shard's fork pool fully tears down — releasing its accumulated v8 coverage memory — before the next one starts, bounding peak memory to roughly a third of the whole ~930-file suite at any given moment instead of the whole suite at once. Each shard disables its own threshold enforcement (a shard's own file subset isn't the right denominator for thresholds calibrated against the whole suite — vitest.config.ts's `coverageThresholds` binding is now gated on a VITEST_COVERAGE_SHARD env var). The script merges every shard's raw coverage-final.json via istanbul-lib-coverage's CoverageMap.merge() — not by summing each shard's own summary numbers, which would be wrong for any source file covered by tests in more than one shard (shards are disjoint by test file, but the source files they import can and do overlap) — then re-checks the same thresholds against the merged, whole-suite result. Verified extensively before pushing: - A scoped 3-shard run (src/stores, src/lib/ai/streams, src/hooks — 555 tests) merges and passes cleanly, and its result was compared file-by-file against a single non-sharded run of the identical file set. Every one of this PR's 5 specifically 100%-gated modules matched exactly (zero discrepancy). A ~40-branch discrepancy did show up, but confined entirely to pre-existing, unrelated Zustand singleton stores (useEditingStore, useSocketStore, etc.) — a known-tricky case for v8's per-test-file coverage snapshotting (the single-run baseline itself isn't perfectly deterministic run-to-run, off by 1 branch on unrelated files) — and always in the safe direction (sharded reports MORE coverage, never less). - A broad 3-shard run across nearly the whole suite (928 of ~930 files, excluding only the 2 files with an unrelated, pre-existing local-only crash) ran 4666 tests across 310 files, correctly caught and reported the one genuine failure it hit (a pre-existing, timezone-dependent test in grouping.test.ts that hardcodes UTC timestamps and fails locally in any non-UTC timezone — confirmed by reproducing it in isolation, unrelated to this change), and otherwise passed clean. - Found and fixed a real bug during this verification: my own explanatory comment in vitest.config.ts happened to contain the literal "/* ratchet:start */.../* ratchet:end */" marker text in prose, which shadowed the real threshold block for both this new script's regex and (potentially) coverage-ratchet.mjs's — reworded to avoid the collision. istanbul-lib-coverage added as an apps/web devDependency (already a transitive dependency repo-wide; added directly since the script needs to import it, and apps/web doesn't otherwise depend on it).
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/web/vitest.config.ts (1)
14-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse UPPER_SNAKE_CASE for constants.
As per coding guidelines, constants should use
UPPER_SNAKE_CASE. ThecoverageThresholdsobject is a top-level configuration constant and should be renamed toCOVERAGE_THRESHOLDS.
apps/web/vitest.config.ts#L14-L30: rename thecoverageThresholdsdeclaration toCOVERAGE_THRESHOLDS.apps/web/vitest.config.ts#L93-L99: update thecoverageThresholdsreference to match the new name.🤖 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/vitest.config.ts` around lines 14 - 30, Rename the top-level coverageThresholds declaration in apps/web/vitest.config.ts lines 14-30 to COVERAGE_THRESHOLDS, and update its reference in lines 93-99 to use the new name consistently.Source: Coding guidelines
🤖 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/scripts/test-coverage-sharded.mjs`:
- Around line 146-153: Normalize each path returned by relative(root, f) to use
forward slashes before passing it to regex.test in the globThresholds loop,
while preserving the existing matching and no-files error behavior.
---
Nitpick comments:
In `@apps/web/vitest.config.ts`:
- Around line 14-30: Rename the top-level coverageThresholds declaration in
apps/web/vitest.config.ts lines 14-30 to COVERAGE_THRESHOLDS, and update its
reference in lines 93-99 to use the new name consistently.
🪄 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: bf065ae4-b9ba-4777-8286-8b8c056dd2dc
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
.github/workflows/ci.ymlapps/web/package.jsonapps/web/scripts/test-coverage-sharded.mjsapps/web/vitest.config.ts
The first real CI run of the sharded coverage runner made genuine progress
(shard 1/3 completed cleanly) but shard 2/3 still hit the same "Ineffective
mark-compacts" heap OOM at the 8192MB ceiling — confirming 1/3 of the suite
(~310 files) can still be enough for an unlucky fork to accumulate past a
modest ceiling. Critically, the script's error handling worked exactly as
designed: it caught the failure and exited cleanly ("Shard 2/3 failed."),
with no hang and no runner-level instability — unlike every single-
invocation attempt before sharding existed.
Doubled SHARD_COUNT from 3 to 6 (~155 files/shard) rather than raising the
ceiling again — raising the ceiling was already tried multiple times in the
single-invocation era and repeatedly destabilized the runner itself under
sustained memory pressure before V8 even reported its own crash. Cutting
each shard's file share further is the lower-risk lever now that the
structural fix (tearing down all workers between shards) is proven to work
mechanically; it's just a matter of finding a shard granularity small
enough for every shard to fit comfortably under a modest, stable ceiling.
…are small 6 shards (~155 files each) still had exactly one shard hit the heap ceiling — a different shard than the 3-shard attempt, but the same clean failure mode each time (script catches it, exits, no hang, no runner-level instability). This confirms the structural fix (tearing down all workers between shards) works correctly; the remaining problem is pure calibration of shard granularity vs ceiling, not anything structural. Moving more decisively rather than incrementing by small steps each costly CI cycle: shard count to 10 (~93 files/shard) and restored the coverage ceiling to 10240MB. Raising the ceiling alone (without sharding) previously destabilized the runner itself under sustained memory pressure across the *whole* suite — but 10240MB was already proven to fail cleanly, safely, and without runner instability at default concurrency in the single-invocation era; it was only ever insufficient alone. Combined with a shard now roughly a tenth the size, it should provide real headroom instead of repeating that earlier failure mode.
…8192MB)
10 shards + 10240MB ceiling was worse than either prior attempt: a genuine
system-level OOM-kill ("runner has received a shutdown signal", exit 137)
after 6 shards completed — no V8-level FATAL ERROR at all this time (0
occurrences), meaning no single fork ever hit its own ceiling; the runner
itself became unstable under cumulative memory pressure instead.
Across every configuration tried tonight (single-invocation and sharded),
8192MB is the one ceiling value that has NEVER caused runner-level
instability — every failure at 8192MB was a clean, script-caught V8 crash
(exit 1, no hang, no shutdown signal). Every ceiling above it (10240MB,
12288MB, 14336MB) triggered either a GC-thrashing hang or a system-level
OOM-kill at some concurrency structure. Reverted to 8192MB and pushed shard
count to 20 (~46 files/shard) instead — shard count is the lever that has
only ever failed cleanly and safely, never destabilized the runner.
Found the real pattern behind why 3, 6, 10, and 20 shards all still had exactly one shard hit the heap ceiling (always cleanly — script catches it, no hang, no runner instability, at the stable 8192MB ceiling): the failing shard's fraction through the run was suspiciously consistent across shard counts (2/3 and 4/6 both = 0.667; ~6/10 and 12/20 both = 0.6). Doubling shard count preserved the SAME relative position of trouble, which rules out "too many files accumulating" — a smaller shard at that same position should then have succeeded, but didn't. vitest's `--shard` splits the sorted file list into plain contiguous ranges. src/lib/ai/core/__tests__/ alone is 66 of ~930 total test files — each pulling in substantial shared AI-orchestration infrastructure (provider SDKs, tool registries, streaming/compaction logic) — and at 20 shards (~46 files/shard) that cluster barely fit inside a single shard's window, concentrating the heaviest files in the whole suite together instead of diluting them across shards. Pushed to 40 (~23 files/shard) to actually split that cluster rather than incrementing the same way again.
3, 6, 10, 20, and 40 shards (~310/155/93/46/23 files each) all still had exactly one shard hit the heap ceiling — always a clean, script-caught failure, never a hang or runner-level instability, at the stable 8192MB ceiling. But the failing shard's fraction through the run stayed suspiciously consistent across every shard count (~0.55-0.67 each time). Doubling shard count should have reduced or eliminated the failure if the cause were "too many files accumulating" in one shard — instead it preserved the exact same relative position of trouble, which only makes sense if vitest's `--shard` flag is splitting the sorted file list into plain CONTIGUOUS ranges: a genuinely dense, heavy cluster of files (e.g. src/lib/ai/core/__tests__/ alone is 66 of ~930 total, each pulling in substantial shared AI-orchestration infrastructure — provider SDKs, tool registries, streaming/compaction logic) sits together in that range no matter how finely you slice around it. Smaller shards were just isolating the SAME cluster into a smaller box, not splitting it apart. Fixed the actual mechanism instead of escalating the shard count guess again: the script now enumerates and sorts the file list itself (matching vitest.config.ts's own include pattern) and distributes files into shards ROUND-ROBIN — file at sorted index i goes to shard i % SHARD_COUNT — then passes each shard's explicit file list to vitest as positional args instead of using `--shard=i/N`. Any dense cluster in the sorted order is now spread evenly across every shard (verified locally: the 66 ai/core files land 3-4 per shard across 20 shards, not up to 66 in one) rather than concentrated wherever a contiguous range happens to land. Reduced shard count back down to 20 now that distribution — not raw shard count — is doing the real work. Verified end-to-end locally with the real script on a scoped 3-shard run (41 files, 541 tests): merges and passes cleanly, and the 100%-gated modules (src/lib/ai/streams/*, stores/pendingStreams/*) remain exactly 100% with the new file distribution.
Interleaving files round-robin across shards (previous commit) was verified to distribute the dense src/lib/ai/core cluster evenly (3-4 files per shard instead of up to 66 in one) — but the next CI run still hit the ceiling on one shard, at almost the same overall job fraction (13/20 = 0.65) as every prior contiguous-sharding attempt, despite that shard now having a completely different, evenly-mixed file composition. That rules out "this specific file combination is inherently too heavy" as the cause — a genuinely different file set failing at a suspiciously similar job-wide fraction points instead to a marginal, close-to-the-ceiling workload where run-to-run variance (GC timing, async scheduling) decides pass or fail, not a fixed, reproducible-every-time culprit. Added a standard mitigation for exactly this class of problem: a shard that crashes gets retried (fresh process, identical file list, identical ceiling) up to 2 additional times before the whole run is failed. If the crash is genuinely deterministic for that file set this just fails again quickly at low added cost and rules the theory out conclusively; if it's marginal, a retry should resolve it outright. Verified the retry control flow in isolation and confirmed the happy-path (no failures) still passes unchanged via a local end-to-end run.
The retry mechanism (previous commit) was the decisive experiment: shard 13 still failed on all 3 fresh-process attempts with the identical file list, proving the crash is deterministic for a specific file combination, not GC/scheduling variance. Inspected that shard's exact file list and found AiChatView.realConversations.test.tsx (376 lines) in it — and its sibling AiChatView.test.tsx (1936 lines) landed in the adjacent shard. These are the exact two files this PR's own D.1 tracking already named as unverified from the very first investigation, before any of this session's CI-infra work began: "still asserting against the pre-cutover useChat-as-render- source architecture... crashes both locally [an unrelated dual-React- dispatcher bug] and in CI." Not simply "large test files" — several bigger files elsewhere in the suite (e.g. calendar-write-tools.test.ts at 2056 lines) run without incident, since they don't render React component trees through jsdom against a component (AiChatView.tsx) this PR heavily rewrote, the way these two do. Pulled both files out of the round-robin pool and gave each its own solo shard, appended after the 20 regular ones, so neither ever shares a fork pool with anything else again. Verified the solo-shard mechanics end-to-end locally using a real (non-crashing) file as a stand-in, since the actual AiChatView files can't be run locally in this worktree at all (separate, pre-existing, unrelated dual-React-dispatcher-null crash) — confirmed the isolated shard runs alone, merges correctly, and thresholds still pass.
Isolation (previous commit) worked for the structural part — all 20 regular round-robin shards passed — but AiChatView.test.tsx still hit the heap ceiling on its own dedicated solo shard, every one of 3 fresh-process retries. With every other file's contention removed, this proves the ~1936-line file genuinely needs more than 8192MB for its own v8-coverage-instrumented run, on its own, not because of anything shared. Every earlier attempt at raising the ceiling destabilized the runner — but those all raised it job-wide, for many forks running with real concurrent memory pressure (default fork count, or a whole shard's ~46 files). A solo-file shard runs exactly one worker; there's no concurrent pressure to compound. Boosted NODE_OPTIONS to 14336MB (14GB, leaving ~2GB of the runner's 16GB for Postgres + OS — deliberately short of the full 16GB used in an earlier attempt) specifically for the isolated heavy-file shards via their own env override, leaving the job-level 8192MB ceiling (ci.yml) untouched for every other shard. Verified the isolated-shard + boosted-ceiling mechanics end-to-end locally using a real stand-in file (the actual heavy files still can't run in this worktree — a separate, unrelated, pre-existing local crash).
# Conflicts: # apps/web/vitest.config.ts
…ation Isolating AiChatView.test.tsx onto its own solo shard (previous-previous commit) proved the crash was deterministic and file-specific — but even alone, with zero contention, it still hit the heap ceiling on all 3 fresh-process retries. Boosting that solo shard's ceiling to 14336MB (previous commit) was tried next and also failed on all 3 attempts: the CI run this session watched confirmed it (isolated shard 21/22 failed after 3 attempts at the 14GB ceiling, identical signature). This file's v8-coverage-instrumented run needs more than 14GB alone — not a concurrency or ceiling-tuning problem at all, but the actual cost of v8 precisely instrumenting a very large, jsdom+React-heavy test file against a component (AiChatView.tsx) this PR substantially rewrote. These two files were already outside this PR's coverage guarantees — not among the 5 gated 100%-threshold globs, and already documented (PR body, D.1) as an unverified, tracked gap asserting against the pre-cutover architecture. So: their dedicated shards now run plain `vitest run` with no `--coverage` flag at all. Tests still execute and must still pass — correctness is fully enforced, nothing here weakens what's tested — but v8 never instruments them, which is what was actually costing the memory (uninstrumented jsdom+React rendering alone was never the problem). They contribute no coverage data to the merge, which is an honest reflection of reality: coverage for AiChatView.tsx was never reliably measurable in this CI environment to begin with. Verified locally end-to-end with a stand-in file: the isolated shard correctly skips coverage instrumentation (no "Coverage enabled with v8" in its output) and the merge step correctly skips it rather than erroring on a missing coverage-final.json.
…he driver Dropping --coverage entirely for AiChatView.test.tsx's solo shard (previous commit) still failed on all 3 fresh-process retries — confirmed via this session's CI run: identical FATAL ERROR heap-OOM signature, at the job-level 8192MB ceiling, with zero coverage instrumentation running at all. This reframes the whole problem: it was never primarily a v8-coverage-instrumentation cost. A single ~1936-line test file, alone, with no concurrency and no coverage collection, genuinely needs more than 8GB just to execute — almost certainly real, unbounded accumulation across its many sequential test cases (each mounting a React tree via jsdom against a component this PR rewrote onto global Zustand singleton stores, exactly the kind of shared subscription-based state that leaks across tests without a full reset between cases). Diagnosing and fixing that leak is a real code-correctness investigation, not a CI-infra tuning problem — out of scope here. These two files are already a separate, tracked, documented gap (PR body, D.1), not part of this PR's own coverage guarantees, and their tests still fully execute and must still pass either way. Since this shard now runs with zero contention and zero coverage overhead, a high ceiling is safe here in a way it never was at the job level — nothing else competes for memory at that moment. Reintroduced a per-shard NODE_OPTIONS override, this time at 15360MB (up from the earlier coverage-enabled attempt's 14336MB, and now paired with no coverage overhead at all), leaving roughly 1GB of the runner's 16GB for the ever-present Postgres service container + OS.
…array in a test mock, not a CI-infra problem The entire multi-hour CI-infra chase (fork tuning, sharding, heap ceilings, coverage.all, file isolation, retries — see this branch's full commit history) was solving the wrong problem. The repo's own history has an exact precedent for this class of mistake: commit 18830af found a prior "coverage OOM that looked like memory scaling" was actually one self-recursive test mock, and reverted an entire prior round of the same kind of CI-infra workarounds once it was fixed. Root cause, found via local reproduction (the long-documented "dual-React- dispatcher-null crash" that supposedly made these files un-runnable locally turned out to be this exact heap-OOM bug, misdiagnosed and never actually investigated — there is no dispatcher error anywhere in the crash output, only the same "Ineffective mark-compacts" signature seen in every CI failure this session): AiChatView.test.tsx and AiChatView.realConversations.test.tsx mock usePendingStreamsStore as `Object.assign(vi.fn(() => []), {...})` — that factory creates a BRAND NEW `[]` array literal on every single call. The real store's useShallow-backed selectors guarantee a stable reference for unchanged data; this mock ignores its selector entirely and breaks that guarantee on every render. Downstream, useActiveStream/useRenderedMessages memoize on that reference, so every consumer saw a "changed" dependency on every render — cascading into a genuine infinite re-render loop. Confirmed directly with a temporary render-count diagnostic: 5000+ renders in ~20s for a single test with one render() call, before the diagnostic's own safety throw fired. Fix: one stable EMPTY_STREAMS_MOCK reference instead of a fresh literal per call. That alone dropped the crashing file from never-completes to 312ms. Also fixed a second, real, independently-confirmed bug: useConversationMessagesStore (a real, unmocked global Zustand singleton in both files) was never reset between test cases, unlike every other test file that touches it (see useConversationMessagesStore.test.ts's own beforeEach). Added the same reset. And fixed one test's assertion that was still checking the pre-cutover useChat.setMessages call instead of this PR's actual store-first commit path (conversationMessagesActions.applyConfirmedMessage) — exposed for the first time now that the file can actually complete a run. Verified exhaustively before reverting the CI-infra stack: the ENTIRE ~930-file suite, coverage enabled, in a single unsharded default-config `vitest run --coverage` (no NODE_OPTIONS override, no pool tuning beyond the pre-existing 'forks' setting, no sharding) completes in ~6 minutes with zero heap OOM — 934/938 files, 13793/13815 tests passing, coverage thresholds all met. The only failures are the exact same pre-existing, local-environment-only issues already documented earlier in this PR's own test plan (local Postgres role config, a timezone-dependent midnight- boundary test) — nothing related to AiChatView, coverage, or memory. Reverted the entire CI-infra workaround stack accordingly: - apps/web/scripts/test-coverage-sharded.mjs: deleted. - apps/web/package.json test:coverage: back to plain `vitest run --coverage`. - apps/web/vitest.config.ts: removed the sharding-aware threshold gating (VITEST_COVERAGE_SHARD/coverageThresholds indirection) and pool-tuning comments; kept `coverage.all: false` and the trimmed json/html reporters as genuine, independently-justified improvements (verified safe on their own merit, not part of the OOM workaround chain). - .github/workflows/ci.yml: removed the NODE_OPTIONS heap-ceiling override entirely — never needed once the real leak was fixed. - Removed the now-unused istanbul-lib-coverage devDependency.
Owner
Author
|
Re: the |
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
PR 4 of the "Deterministic Chat Rendering & Send" epic — cuts
AiChatView.tsxover to store-first rendering (useConversationMessagesStore+usePendingStreamsStore, merged at render viaselectRenderedMessages), replacinguseChat.messagesas the render source. Board: PR pageknqvzh6rls0geobfyclylsl9, epicl4w75179xdk5flyfkin184c2(drivelng6q95adrfndmdnnf9z8g6p).loadMessagesForConversationnow commits viastartLoad/applyLoad/failLoad(generation-gated staleness).isActiveLoad()additionally gates the non-conversation-keyed writes (useChat.setMessages, loading/error UI state) so a slow load for a conversation the user has since switched away from can't clobber the one now on screen — the store commit itself stays on the pure per-conversation generation gate, so switching back later still shows fresh data.AiChatViewreads exclusively throughuseRenderedMessages/useActiveStream(+ aconversationMessagesActionswrite-facade), per the epic's container-agnostic consumer rule. Socket callbacks (user message, edit, delete, stream complete) write to the store; extended to also dual-writeuseChat(mirroring the loader's existing pattern) soregenerate()anduseAskUserAnsweringdon't operate on history that's gone stale relative to a remote edit/delete/message on a shared conversation.buildUserMessage(client cuid, parts-form) +addOptimisticSendfor an instant bubble;useOwnStreamMirrorrenders the live reply via the same store-first path a rejoining tab uses. Stream-completion commits (own-stream finalize, cross-instance recovery) use a newapplyConfirmedMessagestore action (upsert-by-id: replaces a stale/half-streamed snapshot rather than skipping it) — reusing the read-onlyapplyRemoteUserMessagethere would have silently no-op'd on a pre-existing placeholder row.saveMessageToDatabase/saveGlobalAssistantMessageToDatabase'sON CONFLICT DO UPDATEis scopedWHERE conversationId = ... AND role = ...— a client-supplied id colliding with a row in a different conversation OR a different role within the same conversation is rejected (throwsMessageConversationConflictError, never silently treated as saved) instead of overwriting/re-parenting it.resolveMessageIdvalidates id SHAPE (not cuid2 format specifically — the AI SDK's own default generator produces non-cuid ids for surfaces not yet onbuildUserMessage) at both routes' enqueue point, and reassigns the resolved id back onto the message object so persistence and broadcast never disagree.Review convergence (post-open)
Three rounds of adversarial review (Codex, CodeRabbit, and 8 independent self-review agents across correctness/reuse/simplification/efficiency/altitude/conventions angles) surfaced and fixed:
resolveMessageIdincorrectly required strict cuid2 format, breaking ids from senders still on thesendMessage({text,files})shorthand — switched to a shape-only check.isActiveLoad().finallyblocks still run correctly.WHEREdidn't checkrole, allowing auser-role send to overwrite an existingassistantmessage's content within the same conversation (content-spoofing). Added therolepredicate.onStreamComplete's "commit by id" reused a store action that no-ops on an existing id — silently dropping full recovered content when a DB reload had already seeded a'streaming'-placeholder row under the same id. AddedapplyConfirmedMessage(proper upsert/replace semantics).isCuidtest mock (vestigial after the shape-only fix) removed from 7 files.message-utils.ts/route files — resolved cleanly.isLoadingMessagesstucktrueforever — no load runs for an unpersisted id to clear it, and the in-flight load's ownisActiveLoad()gate reads false on resolve since the conversation no longer matches. Fixed: the identity effect now resets the loading/error UI state directly whenever it sees an unpersisted identity.replayPendingMutationsreplayed a confirmed assistant-message replacement (applyConfirmedMessage) the same way as a genuine new user message (applyRemoteUserMessage) — append-if-absent. A load racing a stream completion could read a stale/partial row under the same id and the replay would then skip re-applying the just-confirmed full content, letting the staler snapshot win. Split into a distinctconfirmedMessagemutation type that replays as upsert-by-id.useConversationMessagesStore.ts's 100% coverage threshold failed (90.9% lines, 80% functions) — the two new store methods added in this PR (isLoadCurrent,applyConfirmedMessage) had no wiring tests. Added.Full evidence trail on the PR board page and epic page (4 additional
D —tasks filed at epic level for out-of-scope findings:useMessageActionsoptimism loss for edit/delete/retry once a surface is store-first, PR3'sselectRenderedMessages/usePendingStreamsStoreO(N)-per-token recompute cost, a latent unscoped-upsert shape onaiStreamSessions, and a TOCTOU-shaped ownership check on message update/delete repositories).D.1 — CI heap-OOM (root cause found and fixed)
Root-caused across many iterations of CI-infra tuning (fork/shard tuning, heap ceilings,
coverage.all, file isolation, retries — full trail in the branch's commit history) before finally finding the real cause via local reproduction:AiChatView.test.tsxandAiChatView.realConversations.test.tsxmockedusePendingStreamsStorewith a factory that created a brand-new[]array literal on every call, breaking the real store'suseShallow-guaranteed reference stability. Every downstream consumer (useActiveStream,useRenderedMessages) saw a "changed" dependency on every render, cascading into a genuine infinite re-render loop — confirmed directly with a temporary render-count diagnostic (5000+ renders in ~20s for a single test). This is the exact same class of bug as a documented prior incident in this repo (commit18830afa4): a broken test mock that looked like a CI memory-scaling problem, entirely unrelated to CI infrastructure.The long-standing "dual-React-dispatcher-null" crash that supposedly made these files un-runnable locally was investigated and found to be a misdiagnosis — there's no dispatcher error anywhere in the crash output, only the identical
Ineffective mark-compactsheap-OOM signature seen in every CI failure. It was this same bug the whole time, never actually root-caused.Fix: one stable
EMPTY_STREAMS_MOCKreference instead of a fresh array per call — dropped the crashing file from never-completing to 312ms. Also fixed a second, independently-confirmed bug (useConversationMessagesStore, a real unmocked global store, was never reset between test cases in either file — added the samebeforeEachreset pattern every other store-touching test file already uses) and one test assertion still checking the pre-cutoveruseChat.setMessagescall instead of this PR's actual store-first commit path (exposed for the first time now that the file can complete a run).Verified exhaustively before reverting the entire CI-infra workaround stack: the full ~930-file suite, coverage enabled, in a single unsharded
vitest run --coverage(no NODE_OPTIONS override, no sharding, no special-casing) completes in ~6 minutes with zero heap OOM — 934/938 files, 13793/13815 tests passing, all coverage thresholds met. The only failures are the same pre-existing, local-environment-only issues already noted in this PR's test plan below (local Postgres role config, a timezone-dependent test) — nothing related to AiChatView, coverage, or memory. Revertedtest-coverage-sharded.mjs(deleted), the sharding-aware threshold gating invitest.config.ts, and theNODE_OPTIONSheap-ceiling override inci.yml— none of it was ever needed once the real leak was fixed. Keptcoverage.all: falseand the trimmed reporters as genuine, independently-justified improvements verified safe on their own merit.Test plan
bun run typecheck— clean (both in-worktree and CI'sci / Lint & TypeScript Check)bun vitest runacrosssrc/lib,src/stores,src/app/api/ai,src/hooks(non-render) — 4190+ tests green; one pre-existing unrelated failure (activity-tools.test.ts, local Postgres role config) confirmed present onmastertoomessage-utils-scoped-upsert.test.ts,resolveMessageId.test.ts,applyConfirmedMessage.test.ts,getAssistantMessagesAfterLastUser.test.tsthrowon a rejected upsert collision doesn't leak a credit hold (both routes' hold-releasefinallyblocks run regardless of which nested try/catch returns)AiChatView.test.tsx/AiChatView.realConversations.test.tsx— 38/38 tests green, verified both locally and in CI (see D.1 above for the root-cause fix)Co-Authored-By: Claude Fable 5 noreply@anthropic.com
https://claude.ai/code/session_01LWCLe73VEoAVdbUncCtd1R
Summary by CodeRabbit