-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Inline ghost completion #3158
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
base: main
Are you sure you want to change the base?
Inline ghost completion #3158
Conversation
…sed. Currently using deletion box as well.
…multi-suggestions
…lace as part of the replace. Make sure we don't introduce unnecessary newlines.
|
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.
It's not clear to me what exactly this PR is doing, are we replacing SVG or only sometimes. It seems to me some parts of the code have a fallback, but others dont.
There also still seem to be a lot of unrelated changes lumped in, in pieces of the code which are quite complex and deserve their own thorough review
The AUTOCOMPLETE design doc quite extensively explains what I'm trying to do here. And yes it has some additional changes in here, but nothing too complex IMHO (e.g. make sure intellisense suggestions don't break the ghost completion) |
…ine. Removed AUTOCOMPLETE DESIGN doc.
| } | ||
|
|
||
| // Added content starts with newline - indicates LLM wants to add content after current line | ||
| return addedContent.startsWith("\n") || addedContent.startsWith("\r\n") |
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.
| return addedContent.startsWith("\n") || addedContent.startsWith("\r\n") | |
| return addedContent.startsWith("\n") || addedContent.startsWith("\r") |
but i don't get this logic; why would it be an addition if some content is deleted, and the added content starts with a newline?
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.
If I understand this correctly a newline addition is always green light for the inline suggestion to add the group.
Even if there is a deletion before it. Strictly speaking this is a modification, for which we trigger the inline completion.
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.
that's what this method states, but why is that so?
| this.isPrefix(deletedContent, addedContent) || | ||
| this.isPlaceholderContent(deletedContent) || | ||
| addedContent.startsWith("\n") || | ||
| addedContent.startsWith("\r\n") |
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.
all the newline manipulation feels very tricky to me, especially with how we parse this
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.
Yeah, it tries to "fix" cases where unwanted newlines are introduces or missing. Some of this is in the test coverage, but tricky indeed.
|
|
||
| // Check for common prefix | ||
| const commonPrefix = this.findCommonPrefix(deletedContent, addedContent) | ||
| return commonPrefix.length > 0 |
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.
is this enough? Just the addition and deletion starting with a space because each line does? This feels like a bug tbh
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 is indeed a potential bug, because it should probably use isPrefix and not match only part of the prefix.
Introduce Inline Ghost Completion
Only does inline ghost completions to show pure code additions. This provides a cleaner, less intrusive autocomplete experience similar to GitHub Copilot.
Disables SVG decoration suggestions for now. Will be used when we need to jump around the file for next edit.