fix: prevent message loss during attempt_completion streaming #7115
Conversation
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
- Keep sendingDisabled as true during completion_result to queue messages - Add isStreaming check to message queue processing to prevent premature processing - Process queued messages after user starts new task from completion_result - Add comprehensive tests for message queueing behavior Fixes issue where messages sent during attempt_completion streaming were lost
dosubot
bot
added
size:L
| // processed automatically once sendingDisabled is set to false | ||
| // Give the new task a moment to initialize, then enable sending | ||
| setTimeout(() => { | ||
| setSendingDisabled(false) |
There was a problem hiding this comment.
Is this 100ms delay necessary? Could we use a more deterministic approach like waiting for a specific state change instead? This timeout could potentially create a race condition if the new task initialization takes longer than expected.
| // - there's an API error | ||
| // - we're in the middle of streaming (except completion_result) | ||
| const isCompletionResult = clineAsk === "completion_result" | ||
| const shouldBlockQueue = |
There was a problem hiding this comment.
The queue processing conditions are getting quite complex here. Could we improve readability by extracting this logic into a named function like shouldProcessMessageQueue()? This would make the intent clearer and easier to test in isolation.
| @@ -368,7 +368,9 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro | |||
| if (!isPartial) { | |||
| playSound("celebration") | |||
| } | |||
| setSendingDisabled(isPartial) | |||
| // Keep sendingDisabled as true during completion_result to prevent message loss | |||
There was a problem hiding this comment.
Good fix! Consider expanding this comment to mention that this prevents the race condition where messages sent during the transition between completion_result and new task initialization would be lost.
| }) | ||
| }) | ||
|
|
||
| it("keeps sending disabled during completion_result to queue messages properly", async () => { |
There was a problem hiding this comment.
Nice test coverage! Consider adding an edge case test for when multiple messages are queued during completion_result and then all get processed after starting a new task. This would validate the queue processing order is maintained.
|
This bug definitely annoys me. |
|
Yeah... I don't think this is the right fix though, unfortunately |
hannesrudolph
added
the
Issue/PR - Triage
hannesrudolph
added
PR - Needs Preliminary Review
and removed
Issue/PR - Triage
Problem
Messages sent by users while attempt_completion is streaming were getting lost and not queued properly.
Root Cause
When
completion_resultwas received,sendingDisabledwas being set tofalse(line 371 in ChatView.tsx), but the streaming state might still be active. This caused messages to be queued but never processed because the queue processing logic was checking forsendingDisabledwhich was incorrectly set to false.Solution
sendingDisabledastrueduringcompletion_resultto ensure messages are properly queuedisStreamingcheck to message queue processing to prevent premature processingChanges
ChatView.tsxto keep sending disabled during completion_resultTesting
Fixes the issue where messages sent during attempt_completion streaming were lost.
Important
Fixes message loss during
completion_resultstreaming by keepingsendingDisabledtrue and updating queue processing logic inChatView.tsx.sendingDisabledastrueduringcompletion_resultinChatView.tsxto prevent message loss.isStreamingcheck in message queue processing to prevent premature processing.ChatView.spec.tsxto verify message queueing duringcompletion_result.This description was created by
for ba80e90. You can customize this summary. It will automatically update as commits are pushed.