-
Notifications
You must be signed in to change notification settings - Fork 746
fix(amazonq): Don't show inline completions when a edit is displayed #7839
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. |
| if (this.isEditSuggestionActive()) { | ||
| // Emit DISCARD telemetry for completion suggestions that can't be shown due to active edit | ||
| for (const item of items) { | ||
| if (!item.isInlineEdit && item.itemId) { | ||
| const params: LogInlineCompletionSessionResultsParams = { | ||
| sessionId: session.sessionId, | ||
| completionSessionResult: { | ||
| [item.itemId]: { | ||
| seen: false, | ||
| accepted: false, | ||
| discarded: true, | ||
| }, | ||
| }, | ||
| firstCompletionDisplayLatency: session.firstCompletionDisplayLatency, | ||
| totalSessionDisplayTime: performance.now() - session.requestStartTime, | ||
| } | ||
| this.languageClient.sendNotification(this.logSessionResultMessageName, params) | ||
| } | ||
| } |
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 UTD we only want to send 1 event for trigger (especially for completion suggestions)
all the items in this completion are discarded we can just send a list of items in completionSessionResult and only one call of this.languageClient.sendNotification(this.logSessionResultMessageName, params)
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.
yep we only need 1 call of logSessionResult per trigger
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.
Updated code to batch completionSessionResult and only send telemetry once.
| */ | ||
| public isCompletionActive(): boolean { | ||
| const session = this.sessionManager.getActiveSession() | ||
| return session !== undefined && session.displayed && !session.suggestions.some((item) => item.isInlineEdit) |
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.
session.displayed is a flag that indicates if a session has ever been displayed or not. There is an edge case when it is displayed but user did not reject it, instead moved cursor around or kept typing, in that case, the session.display is true but nothing is rendered on screen.
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.
await vscode.commands.executeCommand(aws.amazonq.checkInlineSuggestionVisibility) is the real method to check if anything is rendered on screen or not because this command is defined under a context variable of VS Code and it only runs if inline suggest is visible
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.
Fixed as suggested.
| * Check if an edit suggestion is currently active | ||
| */ | ||
| private isEditSuggestionActive(): boolean { | ||
| return getContext('aws.amazonq.editSuggestionActive') || 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.
getContext does not necessarily work due to vs code limitations
/**
- Returns the value of a context key set via {@link setContext} wrapper for this session.
- Warning: this does not guarantee the state of the context key in vscode because it may have
- been set via
vscode.commands.executeCommand('setContext'). It has no connection the - context keys stored in vscode itself because an API for this is not exposed.
*/
export function getContext(key: contextKey): any {
return contextMap[key]
}
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.
Fixed - The command approach I tried had reliability issues with when clause evaluation. I ended up using a simple boolean property synchronized with setContext calls instead.
| // Check if an edit suggestion is currently active - if so, discard completion suggestions | ||
| if (this.isEditSuggestionActive()) { | ||
| this.batchDiscardTelemetryForEditSuggestion(items, session) | ||
| this.sessionManager.clear() | ||
| logstr += `- completion suggestions discarded due to active edit suggestion` | ||
| return [] | ||
| } | ||
|
|
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.
A newer edit suggestion arriving from the server. In such cases, the currently displayed edit suggestion is hidden from the user with a DISCARD state, and replaced with the newer edit suggestion.
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.
+1,
if (isEditSuggestionActive && !isInlineEdit) {
return
}
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 other way i think you can also do
if (!isTriggerByDeletion && !request.partialResultToken && !isEditSuggestionActive) {
promise.push(completionPromise)
}
here in recommendationService https://github.com/aws/aws-toolkit-vscode/blob/master/packages/amazonq/src/app/inline/recommendationService.ts#L135-L141
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 think since the only scenario where 2 overlapping suggestion will happen is , correct me if i'm wrong ralph
- when there is an Edit being shown
- the Edit is not dismissed by users
- users again start typing and trigger "Completion"
by doing this, "Completion" is not even fired and you dont even need to consider those suggestion states
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.
Implemented Will's second solution - added !EditSuggestionState.isEditSuggestionActive() to the condition in recommendationService.ts
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.
we can remove this
// Check if an edit suggestion is currently active - if so, discard completion suggestions
if (this.isEditSuggestionActive()) {
this.batchDiscardTelemetryForEditSuggestion(items, session)
this.sessionManager.clear()
logstr += `- completion suggestions discarded due to active edit suggestion`
return []
}
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.
ow it will preventing
A newer edit suggestion arriving from the server. In such cases, the currently displayed edit suggestion is hidden from the user with a DISCARD state, and replaced with the newer edit suggestion.
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.
Removed
| await vscode.commands.executeCommand('aws.amazonq.checkInlineSuggestionVisibility') | ||
| return true | ||
| } catch { | ||
| return 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.
have you managed to get into this catch block?
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've implemented your timestamp-based solution using performance.now() in the checkInlineSuggestionVisibility()
method and checking if it was called within the last 50ms.
Resolved conflict in displayImage.ts by keeping both completion check and patch validation logic
- Add lastVisibleTime property to CodeWhispererSession - Update checkInlineSuggestionVisibility to record timestamp - Replace try/catch with timestamp comparison in isCompletionActive - Addresses Lei's concern about catch block never executing
…check - Update tests to use lastVisibleTime property in mock sessions - Replace try/catch test logic with timestamp-based assertions - Test both recent timestamp (returns true) and old timestamp (returns false) scenarios
…ider - Remove downstream edit suggestion active check since completion requests are now prevented entirely in recommendationService when edit is active - Remove unused isEditSuggestionActive method and EditSuggestionState import - Remove obsolete tests for deleted functionality - Addresses Will's feedback about redundant code after implementing Solution 2
| // Use VS Code command to check if inline suggestion is actually visible on screen | ||
| // This command only executes when inlineSuggestionVisible context is true | ||
| await vscode.commands.executeCommand('aws.amazonq.checkInlineSuggestionVisibility') | ||
| const isInlineSuggestionVisible = performance.now() - session.lastVisibleTime < 50 |
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.
emmm ok
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.
This workaround was provided by leigaol@ due to not being able to use getContext in VSC extensions.
…ws#7839) ## Problem Inline completion suggestions were being shown even when edit suggestions were active, creating a confusing user experience where both types of suggestions could appear simultaneously. ## Solution 1. Added conflict prevention logic in provideInlineCompletionItems() to check for active edit suggestions using the existing aws.amazonq.editSuggestionActive context flag 1. Created isEditSuggestionActive() method to encapsulate context checking 1. Implemented DISCARD telemetry for completion suggestions that can't be shown due to active edits 1. Added unit tests covering the new functionality, including edge cases for mixed suggestion types and items without IDs 1. Updated displayImage.ts to properly set/unset the context flag when edit suggestions are displayed/cleared 1. The solution ensures only one type of suggestion is active at a time while maintaining telemetry compliance and following existing codebase patterns. --- Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time. Your code changes must meet the guidelines in [CONTRIBUTING.md](https://github.com/aws/aws-toolkit-vscode/blob/master/CONTRIBUTING.md#guidelines). License: I confirm that my contribution is made under the terms of the Apache 2.0 license.
Problem
Inline completion suggestions were being shown even when edit suggestions were active, creating a confusing user experience where both types of suggestions could appear simultaneously.
Solution
Treat all work as PUBLIC. Private feature/x branches will not be squash-merged at release time.
Your code changes must meet the guidelines in CONTRIBUTING.md.
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.