-
Notifications
You must be signed in to change notification settings - Fork 2.7k
fix: resolve 'Current ask promise was ignored' error in multi-file diffs #8048
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -739,14 +739,18 @@ export class Task extends EventEmitter<TaskEvents> implements TaskLike { | |||||||||
| // saves, and only post parts of partial message instead of | ||||||||||
| // whole array in new listener. | ||||||||||
| this.updateClineMessage(lastMessage) | ||||||||||
| throw new Error("Current ask promise was ignored (#1)") | ||||||||||
| // Return a placeholder response for partial updates | ||||||||||
| // This prevents the error from disrupting the flow | ||||||||||
| return { response: "messageResponse" as ClineAskResponse, text: undefined, images: undefined } | ||||||||||
| } else { | ||||||||||
| // This is a new partial message, so add it with partial | ||||||||||
| // state. | ||||||||||
| askTs = Date.now() | ||||||||||
| this.lastMessageTs = askTs | ||||||||||
| await this.addToClineMessages({ ts: askTs, type: "ask", ask: type, text, partial, isProtected }) | ||||||||||
| throw new Error("Current ask promise was ignored (#2)") | ||||||||||
| // Return a placeholder response for partial updates | ||||||||||
| // This prevents the error from disrupting the flow | ||||||||||
| return { response: "messageResponse" as ClineAskResponse, text: undefined, images: undefined } | ||||||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same suggestion here - add a clarifying comment for consistency:
Suggested change
|
||||||||||
| } | ||||||||||
| } else { | ||||||||||
| if (isUpdatingPreviousPartial) { | ||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ import { TelemetryService } from "@roo-code/telemetry" | |||||
| import { DEFAULT_WRITE_DELAY_MS } from "@roo-code/types" | ||||||
|
|
||||||
| import { ClineSayTool } from "../../shared/ExtensionMessage" | ||||||
| import { ClineAskResponse } from "../../shared/WebviewMessage" | ||||||
| import { getReadablePath } from "../../utils/path" | ||||||
| import { Task } from "../task/Task" | ||||||
| import { ToolUse, RemoveClosingTag, AskApproval, HandleError, PushToolResult } from "../../shared/tools" | ||||||
|
|
@@ -101,7 +102,13 @@ export async function applyDiffTool( | |||||
| path: getReadablePath(cline.cwd, filePath), | ||||||
| } | ||||||
| const partialMessage = JSON.stringify(sharedMessageProps) | ||||||
| await cline.ask("tool", partialMessage, block.partial).catch(() => {}) | ||||||
| // Handle partial message updates gracefully | ||||||
| try { | ||||||
| await cline.ask("tool", partialMessage, block.partial) | ||||||
| } catch (error) { | ||||||
| // Log the error for debugging but don't disrupt the flow | ||||||
| console.debug(`Partial message update handled: ${error instanceof Error ? error.message : String(error)}`) | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The error message could be more descriptive. Consider:
Suggested change
This provides better context when debugging multi-file operations. |
||||||
| } | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -306,7 +313,23 @@ Original error: ${errorMessage}` | |||||
| isProtected: hasProtectedFiles, | ||||||
| } satisfies ClineSayTool) | ||||||
|
|
||||||
| const { response, text, images } = await cline.ask("tool", completeMessage, hasProtectedFiles) | ||||||
| let response: ClineAskResponse | ||||||
| let text: string | undefined | ||||||
| let images: string[] | undefined | ||||||
|
|
||||||
| try { | ||||||
| const askResult = await cline.ask("tool", completeMessage, hasProtectedFiles) | ||||||
| response = askResult.response | ||||||
| text = askResult.text | ||||||
| images = askResult.images | ||||||
| } catch (error) { | ||||||
| // If ask fails, provide clear feedback to the model | ||||||
| const errorMessage = `Failed to get approval for batch diff operations: ${error instanceof Error ? error.message : String(error)}` | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good error handling here! The clear feedback to the model when batch operations fail is exactly what we need. Consider extracting the error message construction to reduce duplication if this pattern is used elsewhere. |
||||||
| await cline.say("error", errorMessage) | ||||||
| pushToolResult(errorMessage) | ||||||
| cline.processQueuedMessages() | ||||||
| return | ||||||
| } | ||||||
|
|
||||||
| // Process batch response | ||||||
| if (response === "yesButtonClicked") { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good fix for the partial message handling. However, consider adding a comment explaining why we return placeholder responses instead of throwing errors. Something like:
This would help future maintainers understand the design decision.