Switch to webview-based sign-in flow #2382

pkukielka · 2024-10-03T15:02:01Z

Test plan

Full QA, I will add more instructions later.

Depends on:

mkondratek

in general looks good - I will try it in the morning 🌅

mkondratek · 2024-10-03T21:18:20Z

src/main/kotlin/com/sourcegraph/cody/listeners/CodyDocumentListener.kt

 import com.sourcegraph.cody.telemetry.TelemetryV2
 import com.sourcegraph.cody.vscode.InlineCompletionTriggerKind
 import com.sourcegraph.utils.CodyEditorUtil

 class CodyDocumentListener(val project: Project) : BulkAwareDocumentListener {

+  // TODO: Looks like this functionality is broken after the migration to webview


let us have a linear issue for this one

mkondratek · 2024-10-03T21:21:20Z

src/main/kotlin/com/sourcegraph/cody/config/migration/AccountsMigration.kt

+    codyAccountManager.getAccounts().forEach { oldAccount ->
+      val token = codyAccountManager.getTokenForAccount(oldAccount)
+      if (token != null) {
+        CodyAccount(oldAccount.server).storeToken(token)


mkondratek · 2024-10-03T21:27:27Z

src/main/kotlin/com/sourcegraph/cody/api/SourcegraphApiRequests.kt

+        val id: String,
+        val username: String,
+        val displayName: String?,
+        val avatarURL: String?


we do not use avatarURL anymore - I think we can remove it

mkondratek · 2024-10-03T21:30:04Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+  @JsonRequest("secrets/get")
+  fun secrets_get(params: Secrets_GetParams): CompletableFuture<String?> {
+    return CompletableFuture.completedFuture(
+        CodyAccount(SourcegraphServerPath(params.key)).getToken())
+  }
+
+  @JsonRequest("secrets/store")
+  fun secrets_store(params: Secrets_StoreParams): CompletableFuture<Null?> {
+    CodyAccount(SourcegraphServerPath(params.key)).storeToken(params.value)
+    return CompletableFuture.completedFuture(null)
+  }
+
+  @JsonRequest("secrets/delete")
+  fun secrets_delete(params: Secrets_DeleteParams): CompletableFuture<Null?> {
+    CodyAccount(SourcegraphServerPath(params.key)).storeToken(null)
+    return CompletableFuture.completedFuture(null)
+  }
+


maybe a stupid question but... is it safe? are there any security issues? would the secrets be present in the trace logs?

We log the secrets in the agent trace, not the log. You have to set an extra environment variable to turn on that level of logging... I think it should be fine.

We already log tokens in that file today... the ExtensionConfiguration message from JetBrains to Agent has the token in it... see build/sourcegraph/cody-agent-trace.json after a debugging session.

It is a s Dominic said :)

mkondratek · 2024-10-03T21:33:17Z

src/main/java/com/sourcegraph/config/CodyAuthNotificationActivity.java

@@ -79,15 +64,4 @@ public void actionPerformed(@NotNull AnActionEvent anActionEvent) {
    notification.addAction(neverShowAgainAction);
    Notifications.Bus.notify(notification);
  }
-
-  private void showMissingTokenNotification() {


btw, I wonder... what if the token expires (or is removed)? 🤔 what will happen (with chat, with autocomplete)?

I will test this scenario today. In general I think we should then (if user try to call some action explicitly) show cody panel, and it should be showing login panel.

By the way, VSCode has various APIs for showing views, we don't implement them. If we need to show the view, we should see if the extension is trying to do it anyway. Maybe we just need to implement a couple of those methods/properties in the VSCode shim.

mkondratek · 2024-10-03T21:38:57Z

src/main/kotlin/com/sourcegraph/cody/chat/actions/ExportChatsAction.kt

Don't we want to keep this action? I know it is possible to export chats from History tab in the webview. Simply, this action is an actual jetbrains action and can be searched for and run from anywhere

Yeah, for now we should keep this, and Explain, etc.

It would be a nice cleanup project later to ingest the VSCode package.json and consider wiring up those commands automatically--or at least linting that we have them.

Ok, I reverted that changes.

dominiccooney · 2024-10-04T01:01:38Z

src/main/kotlin/com/sourcegraph/cody/CodyToolWindowContent.kt

-  private val cardPanel = JPanel(cardLayout)
-  val allContentPanel: JComponent = JPanel(GridLayout(1, 1))
+  val allContentPanel: JComponent = JPanel(CardLayout())
+  private val mainPanel = JPanel(GridBagLayout())


We were using the cardlayout to swap between the loading, login and onboarding components.

We can't put the webview in a CardLayout because JetBrains remote doesn't render it, so we are swapping the components.

But now there are only two components to swap: Loading and webview. So we don't need this panel anymore.

Let's just have allContentPanel with whatever trivial layout (GridLayout(1,1) etc.)

So this is weird, because I tried to simplify it at the start but it was not working without CardLayout.
But maybe it was error on my side, I will check again :)

dominiccooney · 2024-10-04T01:06:12Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+  }
+
+  @JsonRequest("secrets/store")
+  fun secrets_store(params: Secrets_StoreParams): CompletableFuture<Null?> {


A nullable null... Why not a nullable nullable null 🤔

dominiccooney · 2024-10-04T01:08:53Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+
+  @JsonNotification("window/didChangeContext")
+  fun window_didChangeContext(params: Window_DidChangeContextParams) {
+    println(params)


Let's not spam the console. Info logging it might be fine.

Is this, in fact, important for refreshing the status bar icon? "cody.activated" will be true when the extension thinks it is good to go.

Yeah, refreshing status icon bar is part I/ know I missed but I forget to add it at the end.
Thanks, I will add it now!

dominiccooney · 2024-10-04T01:12:11Z

src/main/kotlin/com/sourcegraph/cody/auth/depracated/DeprecatedCodyAccountManager.kt

@@ -0,0 +1,101 @@
+package com.sourcegraph.cody.auth.depracated


Spelling: deprecated

dominiccooney · 2024-10-04T01:19:39Z

src/main/kotlin/com/sourcegraph/cody/auth/depracated/DeprecatedCodyAccountManager.kt

+
+  @RequiresEdt
+  internal fun updateAccountToken(account: DeprecatedCodyAccount, newToken: String) {
+    service<DeprecatedCodyPersistentAccounts>().accounts = (getAccounts() - account) + account


This seems tricky, maybe the method should be called addOrUpdateAccountToken or something?

dominiccooney · 2024-10-04T01:21:36Z

src/main/kotlin/com/sourcegraph/cody/autocomplete/CodyAutocompleteManager.kt

@@ -172,7 +172,7 @@ class CodyAutocompleteManager {

    val textDocument: TextDocument = IntelliJTextDocument(editor, project)

-    if (isTriggeredExplicitly && CodyAuthenticationManager.getInstance().hasNoActiveAccount()) {
+    if (isTriggeredExplicitly && !CodyAccount.hasActiveAccount()) {
      HintManager.getInstance().showErrorHint(editor, "Cody: Sign in to use autocomplete")


Ugh, we should link to the sign in panel here! Maybe add a TODO?

Actually I think we should just show it.
I will do the changes, thanks!

dominiccooney · 2024-10-04T01:26:40Z

src/main/kotlin/com/sourcegraph/cody/chat/actions/SmellCommand.kt

@@ -1,8 +0,0 @@
-package com.sourcegraph.cody.chat.actions


This too, I believe we should keep these commands.

dominiccooney · 2024-10-04T01:27:23Z

src/main/kotlin/com/sourcegraph/cody/chat/actions/NewChatAction.kt

@@ -12,8 +12,7 @@ class NewChatAction : DumbAwareEDTAction() {
  }

  override fun update(event: AnActionEvent) {
-    val hasActiveAccount = CodyAuthenticationManager.getInstance().hasActiveAccount()
-    event.presentation.isEnabled = hasActiveAccount
+    event.presentation.isEnabled = CodyAccount.hasActiveAccount()


This CodyAccount.hasActiveAccount, we should be using the flag we got from window/didChangeContext "cody.activated" instead.

I agree in principle, but it is not as simple.
I need to also know account, in particular to be able to check if it's enterprise/pro/free, which in turn I need to properly render LLM selection in edit window.
But now I think maybe we should add additional flag to the model, and avoid this problem in the first place.
It would allow us to remove quite a bit of code on the jetbrains side.

OK. Let's work out a practical split to get this landed, because there's a lot of great cleanup here. And we can smooth out the details later.

I agree for chat/models returning the models with the flag sounds good.

I tend to disagree in the specific case of NewChatAction, all we need to know is whether we have an active account. But let's land this first and take a fresh look at account type dependencies after this change is done.

dominiccooney · 2024-10-04T01:29:33Z

src/main/kotlin/com/sourcegraph/cody/config/migration/SettingsMigration.kt

    RunOnceUtil.runOnceForProject(project, "CodyMigrateChatHistory-v2") {
      ChatHistoryMigration.migrate(project)
    }
+    RunOnceUtil.runOnceForProject(project, "AccountsToCodyMigration") {
+      AccountsMigration.migrate()


Just curious but are these guaranteed to run in this order? Is it important?

AFAIR their are, this code is synchronous.

dominiccooney

Feedback inline, please take a look.

Approving to unblock since you have multiple sides to land.

We can go further than this, I think, by listening to window/didChangeContext and hooking a notification to the AuthStatus observer, but we can work on those in follow up patches if it is easier.

dominiccooney · 2024-10-04T01:54:14Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+    println(params)
+  }
+
+  @JsonNotification("globalStorage/didChange")


Let's not sniff global storage for this. We should be listening to the configuration observable and pushing a semantic, application-level message to the IDE.

mkondratek · 2024-10-17T10:41:01Z

src/main/java/com/sourcegraph/cody/CodyToolWindowFactory.java

@@ -30,7 +29,6 @@ public void createToolWindowContent(@NotNull Project project, @NotNull ToolWindo
          content.setPreferredFocusableComponent(null);
          toolWindow.getContentManager().addContent(content);
          DefaultActionGroup customCodySettings = new DefaultActionGroup();
-          customCodySettings.add(new OpenPluginSettingsAction("Cody Settings..."));


Probably this is not an item for this PR but our settings seems to be orphaned now 😢 Can we still have them under Tools > Sourcegraph & Cody?

Thanks for spotting it!
It is definitely for this PR, in fact I removed too much (update channel selection).
I added it back now.

mkondratek · 2024-10-17T10:42:56Z

src/main/kotlin/com/sourcegraph/cody/ui/web/WebUIHost.kt

-    if ((command == "auth" && decodedJson.get("authKind")?.asString == "signout") ||
-        (isCommand && id == "cody.auth.signout")) {
-      CodyAuthenticationManager.getInstance().setActiveAccount(null)
-    } else if (isCommand && id == "cody.auth.switchAccount") {


cody.auth.switchAccount - we do not handle this command now 😕

Because it shouldn't handled by JetBrains anymore.
JetBrains is now only responsible for storing the tokens in the secure store.
Other than that all account management is done by Cody and we just receive notifications about it.

mkondratek · 2024-10-17T10:44:10Z

src/main/kotlin/com/sourcegraph/cody/listeners/CodyDocumentListener.kt

 import com.sourcegraph.cody.telemetry.TelemetryV2
 import com.sourcegraph.cody.vscode.InlineCompletionTriggerKind
 import com.sourcegraph.utils.CodyEditorUtil

 class CodyDocumentListener(val project: Project) : BulkAwareDocumentListener {

+  // TODO: Looks like this functionality is broken after the migration to webview


is it still broken or did you manage fix it? if it need more attention can you pls create an issue for that? 🙏

It's still broken, fix will be a bit more involved, I linked an linear issue

@dominiccooney

## Changes I've added `cody.serverEndpoint` context variable, which on the clients side is received through `window/didChangeContext` notification. We need it on the jetbrains side in few places, e.g. when we are checking if current account is a dotcom account. I think it might be better idea to replace this with a dedicated endpoint, but I'm not sure if `cody.activated` is currently used anywhere? @dominiccooney @sqs WDYT? ## Test plan Full QA with sourcegraph/jetbrains#2382 is needed. It does not change anything on the Cody side.

pkukielka changed the title ~~Switch to webview-based sign-iin flow~~ Switch to webview-based sign-in flow Oct 3, 2024

pkukielka marked this pull request as draft October 3, 2024 15:02

pkukielka mentioned this pull request Oct 3, 2024

Add globalStorage/didChange agent endpoint sourcegraph/cody#5795

Closed

mkondratek reviewed Oct 3, 2024

View reviewed changes

dominiccooney self-requested a review October 4, 2024 00:41

dominiccooney reviewed Oct 4, 2024

View reviewed changes

dominiccooney approved these changes Oct 4, 2024

View reviewed changes

dominiccooney reviewed Oct 4, 2024

View reviewed changes

mkondratek mentioned this pull request Oct 7, 2024

JetBrains: User doesn't signout from cody even after removing all the account #2346

Open

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch 2 times, most recently from 7a32f34 to a856cfd Compare October 16, 2024 14:34

pkukielka mentioned this pull request Oct 16, 2024

Add setContext notification for cody.serverEndpoint sourcegraph/cody#5918

Merged

pkukielka marked this pull request as ready for review October 16, 2024 15:15

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch from 48da69c to 1aaddb9 Compare October 16, 2024 15:19

pkukielka added 3 commits October 17, 2024 10:51

Switch to webview-based sign-in flow

66f59e3

Fix tests

f184f9b

Fix after rebase

3a8922f

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch from 1aaddb9 to 3a8922f Compare October 17, 2024 09:02

mkondratek reviewed Oct 17, 2024

View reviewed changes

mkondratek approved these changes Oct 17, 2024

View reviewed changes

pkukielka added 2 commits October 18, 2024 10:21

Fixes for review

5a091ac

Add back settings panel for choosing update channel

1c108e3

		@@ -0,0 +1,101 @@
		package com.sourcegraph.cody.auth.depracated

Switch to webview-based sign-in flow #2382

Are you sure you want to change the base?

Switch to webview-based sign-in flow #2382

Conversation

pkukielka commented Oct 3, 2024 • edited Loading

Test plan

mkondratek left a comment

Choose a reason for hiding this comment

mkondratek Oct 3, 2024 • edited by pkukielka Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominiccooney Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominiccooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkukielka commented Oct 3, 2024 •

edited

Loading

mkondratek Oct 3, 2024 •

edited by pkukielka

Loading

dominiccooney Oct 4, 2024 •

edited

Loading