-
Notifications
You must be signed in to change notification settings - Fork 730
fix(amazonq): block completion before active edit is accepted/rejected #8141
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
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
|
✅ I finished the code review, and didn't find any security or code quality issues. |
94f66f0 to
9b2ab1d
Compare
9b2ab1d to
d9d0b54
Compare
|
/retryBuilds |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
|
|
||
| await setContext('aws.amazonq.editSuggestionActive' as any, true) | ||
| EditSuggestionState.setEditSuggestionActive(true) | ||
| // Clear old decorations but don't reset state (state is already set in displaySvgDecoration) |
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.
Why do we need to move the logic to displaySvgDecoration? I think we clear decoration after making sure that all condition passes which is in displayEditSuggestion? should we just need the condition in recommendationService.ts?
if (!isTriggerByDeletion && !request.partialResultToken && !EditSuggestionState.isEditSuggestionActive()) {
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 check in recommendationService.ts alone isn't sufficient. Without setting the state early in displaySvgDecoration(), there's a race condition window between when the edit flow starts and when the state is finally set. During this window (which includes async operations like isCompletionActive() and applyPatch()), completion requests can check the state, see it as false, and proceed. Both changes are needed for proper mutual exclusion.
| ) { | ||
| const originalCode = editor.document.getText() | ||
|
|
||
| // Set edit state immediately to prevent race condition with completion requests |
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.
what if the state is set to false later and we already skip completion request?
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.
Setting state to TRUE early and then potentially setting it to FALSE later (if edit fails) is the correct behavior. When we set state to TRUE, we're indicating "an edit operation is in progress" - completion should be skipped during this time regardless of whether the edit ultimately succeeds or fails. If the edit fails, we clean up the state, and future completion requests will work normally. The alternative (not setting state early) is what causes the bug where both suggestions appear simultaneously.
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
✅ I finished the code review, and didn't find any security or code quality issues. |
Problem
Both completion suggestions (ghost text) and edit suggestions (diff view) could appear simultaneously due to a race condition. The EditSuggestionState flag was set too late in the edit display flow, allowing completion requests to check the state, see it as false, and proceed even while an edit was being displayed.
Solution
Fixed the race condition by setting the edit state immediately at the start of displaySvgDecoration() before any async operations:
This ensures only one suggestion type is displayed at a time with proper mutual exclusion.
feature/xbranches will not be squash-merged at release time.