fix(cli): propagate AbortSignal in /compress command - #28506
fix(cli): propagate AbortSignal in /compress command#28506kunalrawat425 wants to merge 4 commits into
Conversation
The /compress command fired tryCompressChat without an AbortSignal, making the background compression impossible to cancel. This could leave dangling network requests when the user starts a new prompt or presses Escape. Changes: - Create an AbortController and pass its signal to tryCompressChat - Check signal.aborted after await to discard stale results - Swallow AbortError in catch when signal is aborted - Export abortActiveCompression() for external cancellation Addresses review feedback from: google-gemini#28483 (comment)
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability and responsiveness of the /compress command by implementing proper cancellation handling. By propagating an AbortSignal to the compression service, the application can now cleanly terminate background network requests when a user initiates a new action or cancels the current operation, preventing dangling processes and UI inconsistencies. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
📊 PR Size: size/M
|
|
You already have 7 pull requests open. Please work on getting existing PRs merged before opening more. |
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to abort in-flight chat compression operations in the CLI by integrating an AbortController into the /compress command and updating the corresponding unit tests. A review comment points out a potential race condition in the finally block where ui.setPendingItem(null) is called; if a new prompt has already set its own pending item, this will incorrectly clear it. The reviewer suggests only clearing the pending item if it matches the current command's message instance.
| } finally { | ||
| if (activeAbortController === abortController) { | ||
| activeAbortController = null; | ||
| } | ||
| ui.setPendingItem(null); | ||
| } |
There was a problem hiding this comment.
There is a potential race condition here. If the compression is aborted (e.g., because the user started a new prompt or cancelled), the finally block will still execute and call ui.setPendingItem(null). If a new prompt has already set its own pending item in the meantime, this will incorrectly clear the new pending item, breaking the UI state.
To prevent this, only clear the pending item if it is still the same pendingMessage instance that was set by this command.
} finally {
if (activeAbortController === abortController) {
activeAbortController = null;
}
if (ui.pendingItem === pendingMessage) {
ui.setPendingItem(null);
}
}
Summary
The
/compresscommand firedtryCompressChatwithout anAbortSignal, making the background compression impossible to cancel. This could leave dangling network requests when the user starts a new prompt or presses Escape.Changes
AbortControllerand pass itssignaltotryCompressChatsignal.abortedafterawaitto discard stale resultsAbortErrorincatchwhen signal is abortedabortActiveCompression()for external cancellationContext
Addresses review feedback raised in #28483 (comment)
Testing
All 8 tests pass (6 existing updated + 2 new):
AbortSignalis passed totryCompressChat