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.
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(auto-edits): adding autoedits onboarding setup for dotcom users #6463
base: main
Are you sure you want to change the base?
feat(auto-edits): adding autoedits onboarding setup for dotcom users #6463
Changes from 13 commits
8907266
1116bc2
a393484
f72b35d
bee12bc
0f22a0e
f2d10d7
3a7e56b
fd2d31f
2c82597
a7a5cfa
0fb8d6d
692dbc7
e60cf2c
03bad60
57d3094
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't love "as you navigate the codebase" though — does this trigger when you just open files, or when you're actively working on a file, or…?
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.
Ah, I just realised this feature replaces the standard autocomplete behaviour too, which people probably have a very strong familiarity with. Is it easy to toggle this feature on/off via the Cody statusbar menu in the bottom right corner?
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.
"as you navigate the codebase" seems to reflect that user doesn't have to type anything to get suggestions, they can get suggestion even if they move the cursor in the file.
Yes, they can get suggestion even if you just open the file. One example is - if user have made modification in some other files, model might suggest the change in the current file based on where your cursor is currently placed
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.
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.
done
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.
Can we add a "Don't Show Again" option here? (like
cody/vscode/src/notifications/cody-pro-expiration.ts
Line 34 in fabed61
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.
added the text
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.
What's the thinking behind opening the settings panel?
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 to make them aware where to find the settings in case they want to turn on/off or switch between autocomplete/auto-edits.
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 we decide to show it multiple times, we should add a progressive timeout between prompts to avoid coming across as spammy.
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.
added the min 1 hour timeout between 2 calls
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.
nit: no need to
await
in cases where we only need to return a promiseThere 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.
done
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.
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.
done
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.
Can we move this check one level higher to the
registerAutoEdits
function inmain.ts
. If a user is not eligible, we should not attempt to call thecreateAutoEditsProvider
function.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.
any specific reason why
main.ts
could be better here ?I liked it here since that puts all the logic for creating/decide if not to create provider in a single place. Also, if the logic becomes more complex (such as the proposal to show pop-up to the non eligible users ) we want to add condition under the if, so we can do this in the same file.
Do you think it would be okay to just keep it here ?
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 assumed we still have a check for the feature flag at the top level, but now I see that we moved all conditions into one place. That was the primary purpose of this suggestion 👍
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 would be helpful for debugging if we named the reason here or at least logged it to the output channel. That way, when users come to us with questions about this error, we can triage it faster.
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.
Added the message which we will show to the user as per the discussion. A quick snapshot below, when free user tried to access
auto-edits
: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.
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.
done