-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: eliminate UI rerender during task cancellation #6781
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
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.
Thank you for your contribution! I've reviewed the changes and this is a good architectural improvement that successfully eliminates UI flicker by preserving the Task instance. I have some suggestions for improvement, particularly around resource cleanup and error handling.
| * Reset the task to a resumable state without recreating the instance. | ||
| * This is used when canceling a task to avoid unnecessary rerenders. | ||
| */ | ||
| public async resetToResumableState() { |
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.
Potential memory leak concern: This method resets many properties but doesn't dispose of resources like urlContentFetcher, browserSession, or terminalProcess. These could accumulate if tasks are repeatedly cancelled and resumed.
Consider adding cleanup for these resources:
| } | ||
|
|
||
| public async abortTask(isAbandoned = false) { | ||
| public async abortTask(isAbandoned = false, skipSave = false) { |
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 adding JSDoc documentation for the new skipSave parameter to clarify when it should be used:
| this.getCurrentCline()!.abandoned = true | ||
| } | ||
| // Just set the abort flag - the task will handle its own resumption | ||
| cline.abort = true |
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 simplified cancellation removes the timeout and abandonment logic. While cleaner, this could potentially leave tasks hanging if the streaming loop doesn't detect the abort flag quickly. Is this intentional? You might want to keep a timeout as a safety net:
|
@roomote-agent use the switch_mode tool to switch to pr-fixer mode and solve the conflicts on this PR, do not create a new one |
|
Hi @daniel-lxs! I see your request to resolve the merge conflicts on this PR. I'll work on resolving them now and push the changes to the same branch. Will update you once completed. |
- Add resetToResumableState() method to Task class to reset internal state without recreation - Simplify cancellation flow to handle everything within Task class - Update abortStream() to properly mark API requests as cancelled - Remove initClineWithHistoryItem() call from ClineProvider.cancelTask() - Task instance now persists through cancellation, preventing UI flicker The task now handles its own cancellation and resumption internally, maintaining the same instance throughout. This provides a seamless user experience with no visual disruption when cancelling and resuming tasks.
- Add proper resource disposal in resetToResumableState() to prevent memory leaks - Add JSDoc documentation for skipSave parameter in abortTask() - Add error handling for resetToResumableState() and ask() operations - Extract resumption logic into dedicated handleUserCancellationResume() method - Add 30-second timeout safety net for cancellation in ClineProvider These changes ensure proper cleanup of resources (browsers, terminals) during task cancellation while maintaining the no-rerender behavior.
9f263ed to
adcc10a
Compare
| // Add a timeout safety net to ensure the task doesn't hang indefinitely | ||
| // If the task doesn't respond to cancellation within 30 seconds, force abort it | ||
| const timeoutMs = 30000 // 30 seconds | ||
| const timeoutPromise = new Promise<void>((resolve) => { |
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 timeout safety net in cancelTask is defined but not awaited. Consider using Promise.race or otherwise awaiting the timeout promise to ensure the forced abort occurs reliably if the task doesn’t respond to cancellation.
|
✅ Merge conflicts resolved successfully! I've successfully resolved the merge conflicts on this PR by rebasing onto the latest main branch. Here's what was done: Changes Made:
Key Architectural Changes Preserved:
Test Results:
The PR is ready for review once all CI checks complete. The merge conflicts have been cleanly resolved while preserving all the intended functionality of this fix. |
| * IMPORTANT: This method cleans up resources to prevent memory leaks | ||
| * while preserving the task instance for resumption. | ||
| */ | ||
| public async resetToResumableState() { |
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.
I feel nervous about this logic getting out of sync with logic in other parts of the codebase (like the "// Reset streaming state" logic a couple hundred lines down). Think there's any way to DRY it up so we don't forget to add things?
…addresses review by @mrubens
| * high-level control flags like isStreaming/isWaitingForFirstChunk. | ||
| * Those are managed by their respective flows. | ||
| */ | ||
| private async resetStreamingState(options: { resetDiffView?: "always" | "ifEditing" | "never" } = {}) { |
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.
Inconsistency in parameter naming: The comment describes the resetDiffView option as accepting 'always, only-if-editing, or never', but the code defines it as 'always', 'ifEditing', or 'never'. Consider updating the comment or the parameter type for consistency.
Fixes #6726
Problem
When cancelling a task, the entire UI would rerender because
initClineWithHistoryItemwas being called, which recreated the Task instance from scratch. This caused a noticeable flicker/blink in the UI.Solution
This PR refactors the cancellation flow to preserve the Task instance throughout the cancellation and resumption process:
Key Changes:
resetToResumableState()method to the Task class that resets internal state without recreating the instanceabortStream()to properly mark API requests as cancelled in the UIinitClineWithHistoryItem()call fromClineProvider.cancelTask()- now just sets the abort flagHow it works:
task.abort = trueabortStream()to update API statusresetToResumableState()Result
✅ No UI flicker when cancelling tasks
✅ API request properly shows as cancelled (not stuck loading)
✅ Immediate resumption - users can send messages right after cancellation
✅ Cleaner architecture with clear separation of concerns
Testing
Important
Refactor task cancellation flow to prevent UI rerender by introducing
resetToResumableState()inTask.tsand updatingClineProvider.ts.Task.tsandClineProvider.ts.resetToResumableState()inTaskto reset state without recreating the instance.Task.abortTask()inTask.tsto includeskipSaveparameter.initClineWithHistoryItem()call fromClineProvider.cancelTask().handleUserCancellationResume()inTask.tsfor resumption logic.ClineProvider.cancelTask()to force abort if task doesn't respond.This description was created by
for 8f9fcc3. You can customize this summary. It will automatically update as commits are pushed.