-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: handle pending ask operations during checkpoint restore #8178
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
Conversation
- Add handleWebviewAskResponse call before checkpoint restore to resolve any pending ask promises - This prevents "Current ask promise was ignored" errors when restoring checkpoints - Add comprehensive tests for pending ask scenarios Fixes #8177
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| if (currentCline) { | ||
| // Handle any pending ask operations by providing a response | ||
| // This unblocks any waiting ask promises before the checkpoint restore | ||
| if (currentCline.lastMessageTs) { |
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.
Is using lastMessageTs the most reliable way to detect pending ask operations? This property could be set for other reasons. Consider adding a more explicit flag like hasPendingAsk to avoid potential false positives and make the intent clearer.
| // This unblocks any waiting ask promises before the checkpoint restore | ||
| if (currentCline.lastMessageTs) { | ||
| // Use the public method to set a response for any pending ask | ||
| currentCline.handleWebviewAskResponse("messageResponse", "", undefined) |
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.
The choice to always use "messageResponse" with empty strings to clear pending asks might not be appropriate for all ask types. Could we either:
- Use a more generic cancellation approach
- Document why "messageResponse" is the universally correct choice here
- Consider different response types based on the pending ask type?
| } | ||
|
|
||
| // For delete operations, abort the task | ||
| if (operation === "delete" && !currentCline.abort) { |
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.
For delete operations, we're now handling pending asks BEFORE aborting the task, which changes the original flow. Have we verified this doesn't introduce any side effects with the abort process? The order of operations can be critical in async cleanup scenarios.
| // Continue even if timeout - the abort flag should be set | ||
| }) | ||
| if (currentCline) { | ||
| // Handle any pending ask operations by providing a response |
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.
Consider extracting this pending ask handling logic into a separate helper method like clearPendingAskOperations(currentCline) for better code organization and potential reusability. This would also make the main function cleaner and the intent more explicit.
| ) | ||
| }) | ||
|
|
||
| it("should handle pending ask operations before checkpoint restore", async () => { |
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 test coverage! These tests effectively verify the pending ask handling for both delete and edit operations, as well as the case where no pending ask exists. Consider adding an edge case test where lastMessageTs is set to 0 or a very old timestamp to ensure the logic handles all scenarios correctly.
|
Closing for now, no repro steps |
This PR attempts to address Issue #8177. Feedback and guidance are welcome.
Problem
Users were experiencing "Error asking question: Current ask promise was ignored" errors when restoring checkpoints, particularly on Windows 11.
Solution
The fix handles pending ask operations before checkpoint restoration by:
lastMessageTs)handleWebviewAskResponsewith a dummy response to unblock any waiting promisesChanges
checkpointRestoreHandler.tsto handle pending ask operations for both delete and edit operationsTesting
Fixes #8177
Important
Fixes error during checkpoint restoration by handling pending ask operations in
handleCheckpointRestoreOperation.handleCheckpointRestoreOperation.lastMessageTsand resolves them withhandleWebviewAskResponse.checkpointRestoreHandler.spec.tsto verify handling of pending ask operations.checkpointRestoreHandler.tsto include logic for handling pending asks before restoration.This description was created by
for 760ef23. You can customize this summary. It will automatically update as commits are pushed.