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.

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.

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.

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.

Switch to webview-based sign-iin flow

c0a233d

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to webview-based sign-in flow #2382

Switch to webview-based sign-in flow #2382

pkukielka commented Oct 3, 2024 •

edited

Loading

mkondratek left a comment

mkondratek Oct 3, 2024

mkondratek Oct 3, 2024

mkondratek Oct 3, 2024

mkondratek Oct 3, 2024

dominiccooney Oct 4, 2024

pkukielka Oct 4, 2024

mkondratek Oct 3, 2024

pkukielka Oct 4, 2024

mkondratek Oct 3, 2024

dominiccooney Oct 4, 2024

dominiccooney Oct 4, 2024

pkukielka Oct 4, 2024

dominiccooney Oct 4, 2024

dominiccooney Oct 4, 2024 •

edited

Loading

pkukielka Oct 4, 2024

dominiccooney Oct 4, 2024

dominiccooney Oct 4, 2024

dominiccooney Oct 4, 2024

pkukielka Oct 4, 2024

dominiccooney Oct 4, 2024

dominiccooney Oct 4, 2024

pkukielka Oct 4, 2024

dominiccooney Oct 4, 2024

pkukielka Oct 4, 2024

dominiccooney left a comment

dominiccooney Oct 4, 2024

		@@ -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

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

dominiccooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkukielka commented Oct 3, 2024 •

edited

Loading

dominiccooney Oct 4, 2024 •

edited

Loading