Skip to content
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(audoedit): add the accept command callback #6543

Merged
merged 3 commits into from
Jan 8, 2025

Conversation

valerybugakov
Copy link
Member

@valerybugakov valerybugakov commented Jan 7, 2025

  • Adds a callback command to inline completion items used to render autoedit suggestions, triggering our acceptance logic needed to send accepted and suggested telemetry events.
  • Prevents inline completion item suggestions from being marked as rejected on acceptance, addressing issues caused by document change event listeners. When a completion item is accepted, the document changes before the acceptance command callback is triggered, which our autoedits renderer manager logic now handles properly.

Test plan

  • CI
  • Manually verify that accepting autoedit suggestions rendered as inline completion items triggers the accepted analytics event. The analytics events written through the telemetry logger can be found in the autoedits output channel.
Screenshot 2025-01-07 at 18 08 42
  • I asked @umpox to help with E2E tests for the analytics logger and different types of autoedits (inline completions vs. decorations).
  • I'll implement integration tests for this change in a follow-up.

Comment on lines -34 to -52
protected async acceptEdit(): Promise<void> {
const editor = vscode.window.activeTextEditor
const { activeRequest } = this
if (
!editor ||
!activeRequest ||
!areSameUriDocs(editor.document, this.activeRequest?.document)
) {
return this.rejectEdit()
}

await vscode.commands.executeCommand('editor.action.inlineSuggest.hide')
await editor.edit(editBuilder => {
editBuilder.replace(activeRequest.codeToReplaceData.range, activeRequest.prediction)
})

await this.handleDidHideSuggestion()
autoeditAnalyticsLogger.markAsAccepted(activeRequest.requestId)
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed because it's identical to the default renderer.

@@ -545,8 +546,6 @@ export class AutoeditAnalyticsLogger {
if (result?.updatedRequest) {
this.writeAutoeditRequestEvent('suggested', result.updatedRequest)
this.writeAutoeditRequestEvent('accepted', result.updatedRequest)

this.activeRequests.delete(result.updatedRequest.requestId)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't use this.activeRequests for caching; having them around for debugging is helpful. We can change this behavior later if necessary. For now, the LRU cache size limit will ensure that stale requests are deleted.

@valerybugakov valerybugakov enabled auto-merge (squash) January 7, 2025 15:06
@valerybugakov valerybugakov merged commit 5710fa7 into main Jan 8, 2025
20 of 21 checks passed
@valerybugakov valerybugakov deleted the vb/autoedits-analytics-logger-integration-8 branch January 8, 2025 00:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants