Skip to content

fix: prevent message loss during attempt_completion streaming #7115

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: prevent message loss during attempt_completion streaming #7115

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 22 additions & 7 deletions webview-ui/src/components/chat/ChatView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Messages will be queued and processed after the user starts a new task
setSendingDisabled(true)
setClineAsk("completion_result")
setEnableButtons(!isPartial)
setPrimaryButtonText(t("chat:startNewTask.title"))
Expand Down Expand Up @@ -635,13 +637,19 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro

useEffect(() => {
// Early return if conditions aren't met
// Also don't process queue if there's an API error (clineAsk === "api_req_failed")
if (
sendingDisabled ||
// Don't process queue if:
// - sending is disabled (except for completion_result which is a special case)
// - there's an API error
// - we're in the middle of streaming (except completion_result)
const isCompletionResult = clineAsk === "completion_result"
const shouldBlockQueue =
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

(sendingDisabled && !isCompletionResult) ||
messageQueue.length === 0 ||
isProcessingQueueRef.current ||
clineAsk === "api_req_failed"
) {
clineAsk === "api_req_failed" ||
(isStreaming && !isCompletionResult)

if (shouldBlockQueue) {
return
}

Expand Down Expand Up @@ -685,7 +693,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
return () => {
isProcessingQueueRef.current = false
}
}, [sendingDisabled, messageQueue, handleSendMessage, clineAsk])
}, [sendingDisabled, messageQueue, handleSendMessage, clineAsk, isStreaming])

const handleSetChatBoxMessage = useCallback(
(text: string, images: string[]) => {
Expand Down Expand Up @@ -753,6 +761,13 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
case "resume_completed_task":
// Waiting for feedback, but we can just present a new task button
startNewTask()
// Process any queued messages after starting new task
// We don't need to check messageQueue.length here as it will be
// processed automatically once sendingDisabled is set to false
// Give the new task a moment to initialize, then enable sending
setTimeout(() => {
setSendingDisabled(false)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}, 100)
break
case "command_output":
vscode.postMessage({ type: "terminalOperation", terminalOperation: "continue" })
Expand Down
144 changes: 140 additions & 4 deletions webview-ui/src/components/chat/__tests__/ChatView.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1469,13 +1469,81 @@ describe("ChatView - Message Queueing Tests", () => {
it("shows sending is enabled when no task is active", async () => {
const { getByTestId } = renderChatView()

// Hydrate state with completed task
// Hydrate state with no active task (empty messages)
mockPostMessage({
clineMessages: [],
})

// Wait for state to be updated
await waitFor(() => {
expect(getByTestId("chat-textarea")).toBeInTheDocument()
})

// Check that sending is enabled
const chatTextArea = getByTestId("chat-textarea")
const input = chatTextArea.querySelector("input")!
expect(input.getAttribute("data-sending-disabled")).toBe("false")
})

it("keeps sending disabled during completion_result streaming to prevent message loss", async () => {
const { getByTestId } = renderChatView()

// Hydrate state with initial task
mockPostMessage({
clineMessages: [
{
type: "say",
say: "task",
ts: Date.now() - 2000,
text: "Initial task",
},
],
})

// Add completion_result that is not partial (streaming finished)
mockPostMessage({
clineMessages: [
{
type: "say",
say: "task",
ts: Date.now() - 2000,
text: "Initial task",
},
{
type: "ask",
ask: "completion_result",
ts: Date.now(),
text: "Task completed successfully",
partial: false, // Not partial, streaming is done
},
],
})

// Wait for state to be updated
await waitFor(() => {
const chatTextArea = getByTestId("chat-textarea")
const input = chatTextArea.querySelector("input")!
// Should remain disabled during completion_result to prevent message loss
expect(input.getAttribute("data-sending-disabled")).toBe("true")
})
})

it("keeps sending disabled during completion_result to queue messages properly", async () => {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

const { getByTestId } = renderChatView()

// Hydrate state with completion_result
mockPostMessage({
clineMessages: [
{
type: "say",
say: "task",
ts: Date.now() - 2000,
text: "Initial task",
},
{
type: "ask",
ask: "completion_result",
ts: Date.now() - 1000,
text: "Task completed",
partial: false,
},
Expand All @@ -1484,12 +1552,80 @@ describe("ChatView - Message Queueing Tests", () => {

// Wait for state to be updated
await waitFor(() => {
expect(getByTestId("chat-textarea")).toBeInTheDocument()
const chatTextArea = getByTestId("chat-textarea")
const input = chatTextArea.querySelector("input")!
// Verify that sending is disabled during completion_result
// This ensures messages will be queued instead of lost
expect(input.getAttribute("data-sending-disabled")).toBe("true")
})

// Check that sending is enabled
// Clear any initial calls
vi.mocked(vscode.postMessage).mockClear()

// Simulate user typing a message while completion_result is active
// This would queue the message since sending is disabled
const chatTextArea = getByTestId("chat-textarea")
const input = chatTextArea.querySelector("input")!
expect(input.getAttribute("data-sending-disabled")).toBe("false")

// Trigger the onChange handler which calls onSend
act(() => {
const event = new Event("change", { bubbles: true })
Object.defineProperty(event, "target", { value: { value: "New task message" }, writable: false })
input.dispatchEvent(event)
})

// The message should be queued (not sent immediately) because sending is disabled
expect(vscode.postMessage).not.toHaveBeenCalledWith({
type: "newTask",
text: "New task message",
images: [],
})
})

it("does not process queue during API errors", async () => {
const { getByTestId } = renderChatView()

// Hydrate state with API error
mockPostMessage({
clineMessages: [
{
type: "say",
say: "task",
ts: Date.now() - 2000,
text: "Initial task",
},
{
type: "ask",
ask: "api_req_failed",
ts: Date.now(),
text: "API request failed",
partial: false,
},
],
})

// Clear any initial calls
vi.mocked(vscode.postMessage).mockClear()

// Try to send a message - it should be queued but not processed
const chatTextArea = getByTestId("chat-textarea")
const input = chatTextArea.querySelector("input")!

act(() => {
const event = new Event("change", { bubbles: true })
Object.defineProperty(event, "target", { value: { value: "Message during API error" }, writable: false })
input.dispatchEvent(event)
})

// Wait a bit to ensure no processing happens
await new Promise((resolve) => setTimeout(resolve, 200))

// Message should not be sent during API error
expect(vscode.postMessage).not.toHaveBeenCalledWith({
type: "askResponse",
askResponse: "messageResponse",
text: "Message during API error",
images: [],
})
})
})
Loading