-
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 VS Code: Add setup notification #53321
Conversation
client/cody/src/main.ts
Outdated
@@ -315,6 +316,17 @@ const register = async ( | |||
if (initialConfig.serverEndpoint && initialConfig.accessToken) { |
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 really understand how this guards against "AND when they load the editor for the first time.". 🤔
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.
When they load the editor for the first time and already have "access token" configured it would means they are a return user so we can try to log the user in automatically, or at least that's what it was meant to do 😆
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.
But neither the access token nor the server endpoint is set if you don't ever configure cody or what am I misunderstanding?
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.
Yea you're right, if they have never configured Cody this would always be false.
If the user has signed into Cody previously (maybe restarted their computer or reload vs code after signed in) then this should be true when the extension is activated.
(Or are you talking about something else?)
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 just read the PR description and saw the "AND when they load the editor for the first time" part 😅 @umpox can confirm, but I think what he meant by "load the editor for the first time" is referring to when the extension is first activated (Cause this doesn't get run again)?
But if this is referring to when loading the extension for the first time, then this might not be the right approach yea
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 should specify better.
This file will run when a fresh VS Code window is opened. The main idea is that this isn't happening whilst a user is coding or whatever, it's just when they open a project so shouldn't be distracting.
So a user will see this if they:
- Have Cody
- Haven't yet configured it (i.e. set their login configuration)
- Are on a new VS Code window
They will see the notification.
The caveat to #3 as you've pointed out is that it will show on a fresh install. We probably don't want that as we already have the walkthrough.
Will update and make the code a bit more obvious!
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.
Gotcha! Makes sense!!
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.
Updated, no built in way to check for first runs so used the context. Cleanest I could get it but LMK if you think there's a better way!
Now it will show on a new VS Code window as long as this is not the first time the extension is being activated :)
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.
Btw tested this on a fresh VS Code build and it works nicely. Walkthrough shows immediately after install, notification on future editor windows
/** | ||
* Displays a VS Code information message with actions. | ||
*/ | ||
export const showActionNotification = async ({ message, options = {}, actions }: ActionNotification): Promise<void> => { |
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.
We should consolidate this with the ones in vscode-editor.ts?
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 looked at doing this but it feels quite cumbersome for just displaying notifications. They don't use anything from controllers
so you end up having to pass around an instance of VSCodeEditor
just to access the 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.
Words look good! But will leave the code review to others…
Description
Adds a notification to prompt users to setup Cody if they haven't already.
Only shows if a user has no configuration for server endpoint / access token (i.e. they've never actually set things up) AND when they load the editor for the first time.
Test plan
Run locally on VS Code with: