fix: confirm server generation state before flushing queued chat messages#1387
Merged
Conversation
…chat messages Re-entering a chat flushed the restored queued message based on the client's stale isGenerating flag (a route-level enrichment the chat:session:updated SSE payload lacks), firing a send that aborted the live generation server-side and could lose the message entirely. The restore path in useChat and useQuickChat now asks the server first: attach and defer the flush while generating, send immediately only when no generation is in flight, and keep the bubble on a failed check. Fixes #1279 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR fixes queued chat message loss after back-navigation by replacing stale client-side ChangesQueued Message Restoration Fix
Sequence DiagramsequenceDiagram
participant User as User/Client
participant Hook as Chat Hook
participant Server as fetchChatSession
participant Stream as attachChatStream
User->>Hook: navigate back, restore pending message
Hook->>Server: fetch authoritative session state
Server-->>Hook: session.isGenerating status
alt generation is active
Hook->>Stream: attach/replay to in-flight stream
Stream-->>Hook: stream completion/error
Hook->>Hook: flush queued message on completion
else no active generation
Hook->>Hook: flush pending message immediately
end
Hook-->>User: queued message sent or retained
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ncepts Document why the FN-5852 queued-message loss survived two fixes (client gates a destructive flush on a route-level enrichment field that SSE payloads lack) and seed CONCEPTS.md with Generation, Queued message, and Enrichment field. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR fixes a race condition in both
Confidence Score: 5/5Safe to merge — the changes narrow a destructive action (sending a message that aborts a live generation) to only fire after server confirmation, and add a cleanup flag to prevent stale async callbacks from acting on switched sessions. Both hooks now gate the queued-message flush on a live server fetch, with a proper cancelled cleanup on effect teardown. The queuedPreSessionCompletionRef guard in useQuickChat correctly separates the pre-session queue path from the restored-message path, preventing a race that the previous fix left open. The new regression tests model the previously-unreachable production fixture and assert no premature send, intact localStorage, and correct flush after stream completion. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User
participant Hook as useChat / useQuickChat
participant LS as localStorage
participant S as Server
U->>Hook: navigate back + re-enter session
Hook->>LS: getPersistedPendingChatMessage(sessionId)
LS-->>Hook: Queued follow-up
Hook->>Hook: restore pendingMessage ref + state
Note over Hook,S: New: server-authoritative check
Hook->>S: fetchChatSession(sessionId)
alt Server still generating
S-->>Hook: isGenerating true
Hook->>Hook: attachIfGenerating
Hook-->>U: restored bubble + Connecting
Hook->>Hook: stream onDone fires
Hook->>S: streamChatResponse
Hook->>LS: removePersistedPendingChatMessage
else Server idle
S-->>Hook: isGenerating false
Hook->>S: streamChatResponse immediate
Hook->>LS: removePersistedPendingChatMessage
else Fetch failed
S-->>Hook: error
Note over Hook: keep bubble for later trigger
end
Reviews (3): Last reviewed commit: "Update packages/dashboard/app/hooks/__te..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?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: 2
🧹 Nitpick comments (1)
packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts (1)
975-1028: ⚡ Quick winConsider verifying
attachChatStreamcall before triggering its handler.The test correctly models the FN-5852 regression scenario, but line 1018 accesses
attachHandlers[0]?.onDonewithout first asserting thatattachChatStreamwas called. If the attach flow doesn't execute as expected, the test would time out waiting forstreamChatResponserather than failing with a clear assertion.✅ Suggested verification before triggering the handler
expect(mockStreamChatResponse).not.toHaveBeenCalled(); expect(localStorage.getItem(getChatPendingMessageKey("session-a"))).toBe("Queued follow-up"); + // Verify that the hook attached to the in-flight generation + expect(mockAttachChatStream).toHaveBeenCalledTimes(1); + expect(mockAttachChatStream).toHaveBeenCalledWith( + "session-a", + expect.any(Object), + "proj-123", + expect.objectContaining({ lastEventId: expect.any(Number) }), + ); + // Once the attached generation completes, the queued message flushes. act(() => { attachHandlers[0]?.onDone?.({ messageId: "msg-001" });🤖 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 `@packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts` around lines 975 - 1028, The test calls attachHandlers[0]?.onDone without first asserting that mockAttachChatStream actually ran; add a verification step before triggering the handler (e.g., expect(mockAttachChatStream).toHaveBeenCalled() or expect(attachHandlers.length).toBeGreaterThan(0)) to fail fast if the attach flow didn't execute, then safely call attachHandlers[0].onDone({ messageId: "msg-001" }) to simulate completion; reference the mockAttachChatStream mock and the attachHandlers array/its onDone handler when adding the assertion.
🤖 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
`@docs/solutions/logic-errors/queued-chat-message-flush-trusts-stale-isgenerating.md`:
- Around line 1-27: The frontmatter is missing the required applies_when field;
open the YAML frontmatter at the top of this document and add an applies_when
key with a concise condition describing when the solution applies (e.g., the
reproduction trigger for this logic error), ensuring it sits alongside existing
keys like category, module, and problem_type so the frontmatter contains
category, module, problem_type, and applies_when.
In `@packages/dashboard/app/hooks/useQuickChat.ts`:
- Around line 659-696: The later auto-flush that calls flushPendingMessage is
not guarded against a newly restored pendingMessageRef.current set
synchronously, so the effect at activeSession (the one around
flushPendingMessage / isStreamingRef / streamRef) can run and send a stale queue
before fetchChatSession completes; update that later flush path to only proceed
for pre-session queued messages by checking
queuedPreSessionCompletionRef.current !== null (or another boolean that is set
before restoring pendingMessageRef.current) or otherwise suppress the flush
until this server validation finishes (i.e., add a “validationInProgress” guard
set before calling fetchChatSession and cleared in its then/catch, and have the
flush effect return early if validationInProgress is true); reference
pendingMessageRef.current, queuedPreSessionCompletionRef.current,
fetchChatSession, activeSession, isStreamingRef, streamRef, flushPendingMessage,
and attachIfGenerating when making the guard change.
---
Nitpick comments:
In `@packages/dashboard/app/hooks/__tests__/useQuickChat.test.ts`:
- Around line 975-1028: The test calls attachHandlers[0]?.onDone without first
asserting that mockAttachChatStream actually ran; add a verification step before
triggering the handler (e.g., expect(mockAttachChatStream).toHaveBeenCalled() or
expect(attachHandlers.length).toBeGreaterThan(0)) to fail fast if the attach
flow didn't execute, then safely call attachHandlers[0].onDone({ messageId:
"msg-001" }) to simulate completion; reference the mockAttachChatStream mock and
the attachHandlers array/its onDone handler when adding the assertion.
🪄 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 Plus
Run ID: 00021d7a-ecf6-4d41-9b1e-0fde708e862b
📒 Files selected for processing (7)
.changeset/fn-5852-queued-message-stale-flush.mdCONCEPTS.mddocs/solutions/logic-errors/queued-chat-message-flush-trusts-stale-isgenerating.mdpackages/dashboard/app/hooks/__tests__/useChat.test.tspackages/dashboard/app/hooks/__tests__/useQuickChat.test.tspackages/dashboard/app/hooks/useChat.tspackages/dashboard/app/hooks/useQuickChat.ts
- gate Quick Chat's session-activation auto-flush to the pre-session queue only, so a restored queue cannot be sent before the restore effect's authoritative fetchChatSession check resolves (real flaw — the original test passed only because the mocked fetch resolved in a microtask and beat the effect) - add slow-fetch regression tests in both hooks proving the restored queue stays un-flushed while server validation is pending - assert attachChatStream ran before triggering its onDone in the quick-chat regression test - drop redundant Promise.resolve() wrapper around fetchChatSession in useQuickChat's restore effect Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Collaborator
Author
Addressed in 47bd46e — the regression test now asserts |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/dashboard/app/hooks/__tests__/useChat.test.ts`:
- Around line 2176-2178: Replace the non-deterministic act + setTimeout sleep
with a deterministic waitFor that asserts the hook's expected state change:
import and use waitFor (from `@testing-library/react`) instead of new Promise;
wait until the hook restores pendingMessage and ensures streamChatResponse is
falsy/cleared (reference the pendingMessage and streamChatResponse
variables/state retrieved from useChat in this test), then proceed — this
removes the 25ms wall-clock sleep and reliably waits for the intended state
transition.
🪄 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 Plus
Run ID: 4b3311c0-3319-4534-9f40-8f6fd047ef2c
📒 Files selected for processing (3)
packages/dashboard/app/hooks/__tests__/useChat.test.tspackages/dashboard/app/hooks/__tests__/useQuickChat.test.tspackages/dashboard/app/hooks/useQuickChat.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/dashboard/app/hooks/useQuickChat.ts
- packages/dashboard/app/hooks/tests/useQuickChat.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
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.
Queuing a follow-up while the assistant was still responding, hitting back, and re-entering the chat made the queued message vanish — not sent, not in the composer — and could kill the in-flight reply too. Both prior attempts (FN-5852, FN-5921) fixed the persistence of the queued draft but left the restore path trusting the client's local
isGeneratingflag, which is reliably stale mid-generation: it's a route-level enrichment that thechat:session:updatedSSE payload lacks, so the client believes nothing is generating, flushes immediately, deletes the persisted copy, and fires a send whosebeginGenerationaborts the live generation server-side.The restore path in
useChatanduseQuickChatnow confirms with the server (fetchChatSession) before flushing: still generating → re-attach to the stream and letonDone/onErrordeliver the queued message; idle → flush immediately; fetch failed → keep the restored bubble for a later flush trigger.Regression tests in both hook suites reproduce the production state the previous tests never modeled — a stale falsy
isGeneratingin local state while the server reports an active generation — and assert no premature send, intact localStorage, and a correct flush once the generation completes.Fixes #1279 (FN-5852)
Summary by CodeRabbit
Bug Fixes
Documentation
Tests