Skip to content

fix: prevent queue from interfering with manual approval workflow (#6996) #6997

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

Closed
wants to merge 1 commit into from
Closed

fix: prevent queue from interfering with manual approval workflow (#6996) #6997

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
15 changes: 11 additions & 4 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 @@ -569,7 +569,11 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
text = text.trim()

if (text || images.length > 0) {
if (sendingDisabled && !fromQueue) {
// Queue messages when:
Copy link
Contributor Author

@roomote roomote bot Aug 12, 2025

Choose a reason for hiding this comment

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

The comment block here is helpful but could be more concise. Consider consolidating it to something like: 'Queue messages when approval buttons are showing or sending is disabled to prevent auto-rejection during manual approval'

// 1. sendingDisabled is true (existing behavior)
// 2. OR when approval buttons are showing (enableButtons is true) to prevent auto-rejection
// This ensures that pressing Enter during manual approval doesn't immediately reject the request
if ((sendingDisabled || enableButtons) && !fromQueue) {
Copy link
Contributor Author

@roomote roomote bot Aug 12, 2025

Choose a reason for hiding this comment

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

Consider extracting the condition (sendingDisabled || enableButtons) into a named variable or function like shouldQueueMessage for better readability and maintainability, since this logic appears in multiple places.

// Generate a more unique ID using timestamp + random component
const messageId = `${Date.now()}-${Math.random().toString(36).substr(2, 9)}`
setMessageQueue((prev: QueuedMessage[]) => [...prev, { id: messageId, text, images }])
Expand Down Expand Up @@ -627,17 +631,20 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
// but for now we'll just log it
}
},
[handleChatReset, markFollowUpAsAnswered, sendingDisabled], // messagesRef and clineAskRef are stable
[handleChatReset, markFollowUpAsAnswered, sendingDisabled, enableButtons], // messagesRef and clineAskRef are stable
)

useEffect(() => {
// Early return if conditions aren't met
// Also don't process queue if there's an API error (clineAsk === "api_req_failed")
// IMPORTANT: Don't process queue when there's a pending approval request (enableButtons is true)
// This prevents the queue from interfering with manual approval workflows
if (
sendingDisabled ||
messageQueue.length === 0 ||
isProcessingQueueRef.current ||
clineAsk === "api_req_failed"
clineAsk === "api_req_failed" ||
enableButtons // Don't process queue when approval buttons are shown
Copy link
Contributor Author

@roomote roomote bot Aug 12, 2025

Choose a reason for hiding this comment

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

Is this intentional? The isProcessingQueueRef.current flag is set after the async check. If multiple effects trigger simultaneously, there could be a brief window where two queue processors start. Consider moving line 653 before the return statement to ensure the flag is set synchronously.

) {
return
}
Expand Down Expand Up @@ -682,7 +689,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
return () => {
isProcessingQueueRef.current = false
}
}, [sendingDisabled, messageQueue, handleSendMessage, clineAsk])
}, [sendingDisabled, messageQueue, handleSendMessage, clineAsk, enableButtons])
Copy link
Contributor Author

@roomote roomote bot Aug 12, 2025

Choose a reason for hiding this comment

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

This fix correctly prevents queue processing during approval states, but it would benefit from test coverage. Consider adding tests for the queue/manual approval interaction to prevent regression of this critical workflow.


const handleSetChatBoxMessage = useCallback(
(text: string, images: string[]) => {
Expand Down
Loading