-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Cody: Inline Assist #51679
Cody: Inline Assist #51679
Conversation
Bundle size report 📦
Look at the Statoscope report for a full comparison between the commits ad4da31 and d230a04 or learn more. Open explanation
|
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.
Amazing work!
Didn't do a full review as I’m not super familiar with the product direction but I've played around with it and it works well.
One question I have is if we should put this behind a feature flag, similar to completions?
After testing this branch, I am seeing this new comment sidebar which was a bit much for me personally (especially since the + icon moves around when I use the mouse 😐). Given the feedback we have for completions (and basically that people use Cody because it does not enable completions by default), I wonder if we should use the same for this feature since it, IMO, is in the same level of intrusiveness.
Screen.Recording.2023-05-10.at.11.45.26.mov
Also this PR is missing a changelog 😎
This is part of the file {fileName}. The part of the file I have selected is highlighted with <selection> tags. You are helping me to work on that part as my coding assistant. | ||
Follow the instructions in the selected part plus the additional instructions to produce a rewritten replacement for only the selected part. | ||
Put the rewritten replacement inside <selection> tags. I only want to see the code within <selection>. | ||
Do not move code from outside the selection into the selection in your reply. | ||
Do not remove code inside the <selection> tags that might be being used by the code outside the <selection> tags. | ||
It is OK to provide some commentary within the replacement <selection>. | ||
Only return provide me the replacement <selection> and nothing else. | ||
If it doesn't make sense, you do not need to provide <selection>. |
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.
How is this placeholder insertion working in your test? I tried something similar for completions to simulate a "fill-in-middle" behavior but occasionally Claude wasn't responding with sensible content, e.g. it responded with adding more of these tags or creating invalid code.
Maybe the different is Claude 1.3 vs. Claude Instant?
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 been working well for me but it might be because I've only been making changes to small chunks of code 🤔 This is the original fixup prompt though so it should work the same as it currently is now but that's a good thing to note!
To add to the message above: I don't mind if we put this feature flag on by default but I would personally like to have an option to turn it off other than removing Cody! But also cc @jdorfman who has shared the feedback for completions being off by default in Slack before (c.f. this thread) |
Yea 100% agree! Just put it behind a feature flag and set it to false by default. |
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.
Looks good.
I look forward to getting the finer-grained fixup highlighting, fixup task list, etc. working with this.
I would love to talk offline with you and @philipp-spiess about prompting.
const promptText = Fixup.prompt | ||
.replace('{humanInput}', truncateText(humanChatInput, MAX_HUMAN_INPUT_TOKENS)) | ||
.replace('{responseMultiplexerPrompt}', context.responseMultiplexer.prompt()) | ||
.replace('{truncateFollowingText}', truncateText(selection.followingText, quarterFileContext)) |
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 readability it makes more sense to have the preceding text, the selected text, and the following text in that order because that's the order they're in the file and the prompt. Doing it backwards here adds to the cognitive burden of which end of the context text is being truncated.
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.
ahhh yea I can see how it is confusing. The reason I did it backward is because .replace
would only replace the first match. So if I replace {truncateFollowingText}
first, which is in the first part of the original prompt, and then replace {humanInput}
afterward, it would look for the first {humanInput}
match. So if the replaced context from {truncateFollowingText}
happens to have the word {humanInput}
, i think it will replace that instead (if I understand it correctly).
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.
Makes perfect sense. Maybe a brief comment that these are replaced in reverse order to avoid colliding with the placeholder if it occurs in the input. You could also note in the template that if the order of the placeholders changes you have to update this to insert them in reverse order.
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.
true that's a good point, i need to remember about adding comments and tests! I will do it in another PR tomorrow since I just merged this 😅 Thank you!
As my coding assistant, please help me with my questions: | ||
{humanInput} | ||
|
||
## Instruction |
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'd love some background offline about how you arrived at these instructions.
` | ||
|
||
public static readonly displayPrompt = ` | ||
\nQuestions based on the code below:\n\`\`\`\n{selectedText}\n\`\`\`\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.
Did you try giving Cody some of the surrounding context, as well as the selection, and see if that made responses better?
Are there any cases you feel are particularly easy or hard for Cody with this prompt?
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 did! I originally had it in the same message (link to code), but it seems like it works the same if i include it in the getContextMessages
below
Thanks for the review! I'll put time on your calendar to chat with you and @philipp-spiess on prompting, or we can do it async :D ? |
Close #51363
This PR introduces the ability to interact with Cody in File View using
Comments
.:/fix
or/f
to your comment would initiate a fixup task, which just runs the existingfixup
recipe (using/fix
in non-comments view is not supported)fixup
tasks made from comments will highlight the selected code in Cody's color based on current state:pending state:
cody.experimental.inline
flagexecuted state:
Other minor changes:
command
folder toservices
Note:
additional instructions
when available.Test plan
All tests have passed
Check Loom for details: https://www.loom.com/share/28e7eac473b44501a7e011d9f26ac0ce