Skip to content
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

vscode: Remove App login #2099

Merged
merged 6 commits into from
Dec 5, 2023
Merged

vscode: Remove App login #2099

merged 6 commits into from
Dec 5, 2023

Conversation

dominiccooney
Copy link
Contributor

@dominiccooney dominiccooney commented Dec 5, 2023

This one is a doozy:

  • Removes App as a sign-in option.
  • If you're signed into App but you have a dotcom token, we'll try to use that.
  • If you're signed into App, but don't have a dotcom token, you'll have to sign in again.
  • Even if you're using classic sidebar chat, if you had an embeddings index generated by App, we'll pick it up and use it. However the context status indicator will indicate you have no embeddings. (We could plumb this in, but it is going away very soon.)
  • No more toasts or promotions to set up App.

Leaves the LocalAppDetector so we can delete App/toast you to delete App soon.

Part of #1966.

Test plan

Setup

The big secret to testing this is recreating the "signed in to App" state, now that all the sign in options are gone. You do this by abusing the Enterprise sign-in options:

  1. Install Cody App and log into it if you haven't got it already.
  2. Sign into dotcom, which caches a token used for the first test below.
  3. In VScode, Cmd-Shift-P, Cody: Switch Account... and enter http://localhost:3080 which is App's URL.
  4. cat ~/Library/Application\ Support/com.sourcegraph.cody/app.json and smurf the token.
  5. At this point, you should be signed in to App. You can verify this by chatting, quitting app, chatting again and seeing you get a connection error message.

Next sign in

  1. Simply reload the window.
  2. You should be signed in to dotcom with the token you had been using at the start of the previous step. You can verify this by chatting with App closed.
  3. Check that Cody App doesn't appear in any of the account switcher or sign in menus.

Signed into App, no dotcom token

Assuming you're still signed in to dotcom:

  1. Cmd Shift P, Cody: Sign out.
  2. Sign into App using the leetcode techniques above.
  3. Reload the window.

You should be dumped at the login pane.

Signed into App, cached invalid dotcom token

  1. Create a new dotcom auth token at https://sourcegraph.com/user/settings/tokens/new/callback
  2. Sign in to http://sourcegraph.com using that token
  3. Cmd Shift P, Cody: Switch Account... and follow the "setup" instructions above to sign in to app.
  4. You are now signed in to App, with a valid cached dotcom token. Switch back to the Sourcegraph token management UI and revoke the token.
  5. Reload the window. This encounters a long (60s?) pause, but eventually the login fails and you are dumped back to the login screen and can log in to get a fresh token.

@beyang
Copy link
Member

beyang commented Dec 5, 2023

Rather than try to solve for all auto-migration edge cases, could we have some sort of catch-all or fallback warning message that instructs the user to uninstall App and install Cody from scratch? This seems easier on both us and the user than trying to debug any errors that happen during migration.

Nit: I notice you prefix your PR titles with "VScode:", which seems a bit off:

  • If you want camel casing of "VS Code", then "VSCode" would be appropriate
  • If you're simply looking for a shorthand to indicate the general category of this change, then "vscode" seems sufficient and more aesthetically pleasing

Copy link
Member

@beyang beyang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not gone through the test plan (see my previous message on whether a coarse-grained catch-all message could suffice here), but the code changes LGTM.

// messages can use local embeddings. We delay to this point so
// local embeddings service start-up updating the context
// provider does not cause autocompletes to disappear.
void this.contextProvider.localEmbeddings?.start()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would local embeddings start-up interfere with autocomplete? Isn't it the case that autocomplete doesn't use embeddings?

Also: sidebar chat is going away, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would local embeddings start-up interfere with autocomplete? Isn't it the case that autocomplete doesn't use embeddings?

This is more to keep the tests green than a practical issue. But an autocomplete test failed with earlier initialization of local embeddings because:

  • Starting local embeddings can be slow, if cody-engine is not downloaded. So we try to not start it.
  • This means when we do actually start it, we need to tell the ContextProvider that the context may have changed.
  • I assume autocomplete detects this context provider change and cancels its pending completion, which broke the test. I have not verified the callstack of that down to the ground though.

Also: sidebar chat is going away, right?

Sidebar chat is going away, in which case this code gets deleted. But if we shipped sidebar chat this week, I thought it would be nice to not break local embeddings for sidebar chat users for that week.

@dominiccooney dominiccooney changed the title VScode: Remove App login vscode: Remove App login Dec 5, 2023
@dominiccooney
Copy link
Contributor Author

@beyang Rather than try to solve for all auto-migration edge cases, could we have some sort of catch-all or fallback warning message that instructs the user to uninstall App and install Cody from scratch? This seems easier on both us and the user than trying to debug any errors that happen during migration.

There's no messages to the user. They're either automatically logged in, or they land on the login screen.

The test plan just tries to cover all the bases.

@dominiccooney dominiccooney merged commit 5af5735 into main Dec 5, 2023
14 checks passed
@dominiccooney dominiccooney deleted the dpc/remove-app-login branch December 5, 2023 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants