Autocomplete: Log the number of lines of an accepted completion and fix a bug #53878
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
philipp-spiess
requested review from
valerybugakov,
lrhacker,
kelsey-brown and
a team
github-actions
bot
added
the
team/code-exploration
abeatrix
approved these changes
Jun 21, 2023
valerybugakov
approved these changes
Jun 22, 2023
| if (!suggestedAt) { | ||
| continue | ||
| // eslint-disable-next-line ban/ban | ||
| displayedCompletions.forEach(completionEvent => { |
There was a problem hiding this comment.
I would remove this ban from our config 🙂
There was a problem hiding this comment.
I'll do this as a follow up
ErikaRS
pushed a commit
that referenced
this pull request
Jun 22, 2023
…ix a bug (#53878) This PR does two things: - First we add the number of lines of an accepted completion. Cool - But then I noticed an oversight on my side. When you hover over the autocomplete preview, the VS Code backends sends a new completion request concurrently which our previous logic was using the _clear the number of open completion requests_. That means that whenever we accepted a solution _after that_, we would not log it because we've already GCed it. - To fix this, I’m now using the LRU cache to keep more completions around (which will also help later as we need to do more shenanigans with the logger) ## Test plan - Trigger a multi-line completion - Hover over the suggested completion - Select a different one with a lower number of lines - Accept the completion - Observe that the right number for `lines` is logged https://github.com/sourcegraph/sourcegraph/assets/458591/4618d890-2491-4ac8-880d-8eba5c3622ba <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
sanderginn
pushed a commit
that referenced
this pull request
Jun 23, 2023
…ix a bug (#53878) This PR does two things: - First we add the number of lines of an accepted completion. Cool - But then I noticed an oversight on my side. When you hover over the autocomplete preview, the VS Code backends sends a new completion request concurrently which our previous logic was using the _clear the number of open completion requests_. That means that whenever we accepted a solution _after that_, we would not log it because we've already GCed it. - To fix this, I’m now using the LRU cache to keep more completions around (which will also help later as we need to do more shenanigans with the logger) ## Test plan - Trigger a multi-line completion - Hover over the suggested completion - Select a different one with a lower number of lines - Accept the completion - Observe that the right number for `lines` is logged https://github.com/sourcegraph/sourcegraph/assets/458591/4618d890-2491-4ac8-880d-8eba5c3622ba <!-- All pull requests REQUIRE a test plan: https://docs.sourcegraph.com/dev/background-information/testing_principles -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR does two things:
Test plan
linesis loggedScreen.Recording.2023-06-21.at.18.17.01.mov