-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add cancel button for context condensing operation #6927
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
feat: add cancel button for context condensing operation #6927
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 found that while the implementation is generally clean and follows good patterns, there are some critical issues that need attention before merging.
src/core/condense/index.ts
Outdated
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.
Critical Issue: The translation key is not defined in the i18n files. This will cause a runtime error when the cancellation occurs.
You need to add this key to under the section:
Also add translations for other supported languages.
src/core/task/Task.ts
Outdated
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 use of AbortController pattern for cancellation! The implementation is clean and follows best practices for handling async cancellation.
src/core/task/Task.ts
Outdated
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 proper TypeScript types instead of using type assertions. You could extend the Task interface to include these properties:
src/core/webview/ClineProvider.ts
Outdated
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 type assertions here and should be replaced with proper TypeScript interfaces. This would improve type safety and make the code more maintainable.
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 addition of the English translation! However, you should also add this translation key to all other supported language files to maintain complete i18n coverage.
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 state management for the condensing operation. The UI properly reflects the condensing state and handles the cancellation cleanly.
b22c050 to
3d12e7c
Compare
1a90efc to
c6193ed
Compare
|
Hey @f14XuanLv, I've been testing the cancel functionality and noticed a couple of issues:
I pushed a fix for the TypeScript type assertions to your branch, but these functional issues should probably be addressed before merging. The cancel state might need to be reset more thoroughly when starting a new condensing operation. |
…nslations - Fix failing tests by adding undefined abortSignal parameter to summarizeConversation calls - Add missing 'cancelCondense' translations for all supported languages
- Added 'condense_cancelled' error message translation for all 18 languages
- This fixes the missing translation key issue identified by the Roomote bot
- Backend uses t('common:errors.condense_cancelled') which was undefined
- Added isCondensing and cancelCondenseContext properties to TaskLike interface - Removed all 'as any' type assertions from Task.ts and ClineProvider.ts - State properties (customCondensingPrompt, condensingApiConfigId, listApiConfigMeta) are now properly typed through GlobalState
…tions - Add concurrent operation check to prevent multiple condensing operations - Implement cleanup delay after abort to ensure proper state transition - Resolve cancel->restart sequence state conflicts Note: After cancellation, there's an ~5 second cooldown period before new condensing operations can be initiated, which appears to be existing system behavior.
42cd68b to
95eda6d
Compare
|
Hey @daniel-lxs Thanks for the detailed feedback! I've addressed both issues you mentioned: Fixed Issues1. Condensing cooldown mechanism ✅Added concurrency control check to prevent multiple condensing operations from running simultaneously: if (this.isCondensing) {
console.warn("Context condensing is already in progress")
return
}2. Cancel state cleanup ✅Added intelligent cleanup waiting mechanism to avoid race conditions: if (this.condensingAbortController && this.condensingAbortController.signal.aborted) {
await new Promise(resolve => setTimeout(resolve, 100))
}✅ Testing ResultsVerified through actual testing:
Ready for re-review! |
|
Hey @f14XuanLv there's some logic that prevents frequent condesing:
This is not possible on the current version but seems possible on this PR, can you take a look? |
|
Hi @daniel-lxs I need some clarification on a few points:
Understanding these details will help me better identify and fix the problem you've found. Thanks for your patience! |
|
Moving to dans column so he answers |
|
Hi @f14XuanLv - quick clarification:
Important: concurrency is not the same as cooldown. An isCondensing guard only prevents overlap while one is running; we also need the time-based guard after a condense completes or is cancelled. The PR appears to short-circuit that check. What to update:
Once these are in, I can re-test. |
|
@hannesrudolph Could you please move this to dan's column so he can answer these questions? Hi @daniel-lxs, Thank you for the clarification. However, I'm confused about your statement regarding the existing cooldown logic on main. Current Analysis on main branch (commit: 87d50a7)After examining the codebase on the latest main branch (commit 1. Message-based prevention in
|
|
Thanks for working on this! I'm closing this PR as part of cleanup for inactive PRs. The implementation would need some additional backend wiring to fully support the cancellation flow. If you'd like to revisit this feature, feel free to open a new PR or let me know if you want to continue with this one. |

Related GitHub Issue
Closes: #6926
Roo Code Task Context (Optional)
Description
This PR adds a cancel functionality for context condensing operations, addressing the issue where users couldn't interrupt long-running condensing processes.
Key implementation details:
TaskHeadercomponent that appears whenisCondensingis truecancelCondenseContextmessage type)AbortControllerAPI to gracefully cancel async stream operationscondenseContextstream processingTest Procedure
Testing steps:
Pre-Submission Checklist
Screenshots / Videos
[Screenshot 1: Shows cancel button during condensing]

[Screenshot 2: State after clicking cancel]

Documentation Updates
Additional Notes
This feature improves user experience, especially when dealing with large contexts. The cancellation operation uses the standard AbortController pattern, ensuring proper resource cleanup.
Update: Fixed test failures by adding the missing
abortSignalparameter tosummarizeConversationcalls in sliding-window tests, and completed internationalization for all supported languages (18 languages total).Update 2: Fixed missing
condense_cancelledtranslation key incommon.jsonfiles for all languages, addressing the issue identified by Roomote bot review.Get in Touch
N/A - Available via GitHub @f14XuanLv