Autocomplete: Fix suggestion event over counting #649
Merged
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
abeatrix
approved these changes
Aug 10, 2023
philipp-spiess
changed the title
|
Thank you for the fix and sorry for the regression! |
vovakulikov
reviewed
Aug 10, 2023
| const insertion = completion.insertText | ||
| let j = 0 | ||
| // eslint-disable-next-line @typescript-eslint/prefer-for-of | ||
| for (let i = 0; i < insertion.length; i++) { |
There was a problem hiding this comment.
Why do we need to compare strings here symbol by symbol? Can we just insertion === suffix?
There was a problem hiding this comment.
@vovakulikov VS Code is actually pretty advanced here and will diff the insertion with the text that is already at the range. It will render the completion as long as it is only additions to the text. These are clustered but it can be more than one cluster. Here's an example:
The suffix in this case is , ] and the insertion is 2, 3]. A simple string match won't work here, so I’m instead looping over the insertion to ensure it contains all characters from the suffix, in our case, it will find all 3 chars:
2, 3]
^^ ^
vovakulikov
approved these changes
Aug 10, 2023
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 fixes an issue that sometimes caused completion suggestion events to not fire correctly. The issue here was that we decided wether or not a completion is visible in the
getInlineCompletionscode that was now change to still yield completions.The yielding is fine, we have relied on these results to be used for the last candidate cache. However, this would also mean that we would store a log id that was deemed as not visible but that never reconsidered this condition. This meant that such a completion could become visible later and we would be able to accept it, even though it was never logged as suggested.
To fix this, we move this logic into the VS Code specific part (which conceptually makes more sense anyways since it's full of VS Code specific logic) and also test the visibility after every cache retrieval.
Test plan
I've added three conditions to test invariants that should not be possible. To test locally, I've added debugger break points into either of them and repeatedly triggered a bazillion of completions. After these changes, I don't seem to be able to trigger them anymore. I’m leaving some logs in there, though, so we can see if they happen on production.