-
Notifications
You must be signed in to change notification settings - Fork 23
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
Initial implementation for Inline Edit and Document Code #440
Changes from all commits
67cfdca
c1dbd0a
2e20cdd
760430f
b5b6674
baaf829
52e854e
dc98524
2fcf387
51f9243
2593cb1
52ca409
c8faca0
3a8b828
5016cc3
c530b2a
90377d6
b535c71
2b69a69
0ca7116
83c432c
bca7b6c
ea69818
f6cba1a
a710304
e120cbd
5f06a55
f52d91e
a615714
bfdf688
be5f32c
188d861
69b9ad7
aa5f2e3
c9be1a0
9e8693a
fe445c9
5ff5056
3dbcdb5
d360a33
3c1793a
e51b607
70abd1a
8152863
7a3ba22
552687e
1bc0937
46a04ce
a612960
21e437c
ee40482
4ae13e5
4c95564
d25ded4
0a143f8
1f6360a
16f4a34
d2898ae
5fe3844
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,19 +3,26 @@ | |
import com.intellij.openapi.application.ApplicationManager; | ||
import com.intellij.openapi.diagnostic.Logger; | ||
import com.intellij.openapi.editor.Editor; | ||
import com.sourcegraph.cody.agent.protocol.DebugMessage; | ||
import com.sourcegraph.cody.agent.protocol.*; | ||
import java.util.concurrent.CompletableFuture; | ||
import java.util.function.Consumer; | ||
import java.util.function.Supplier; | ||
import org.eclipse.lsp4j.jsonrpc.services.JsonNotification; | ||
import org.eclipse.lsp4j.jsonrpc.services.JsonRequest; | ||
import org.jetbrains.annotations.NotNull; | ||
import org.jetbrains.annotations.Nullable; | ||
|
||
/** Implementation of the client part of the Cody agent protocol. */ | ||
/** | ||
* Implementation of the client part of the Cody agent protocol. This class dispatches the requests | ||
* and notifications sent by the agent. | ||
*/ | ||
@SuppressWarnings("unused") | ||
public class CodyAgentClient { | ||
|
||
private static final Logger logger = Logger.getInstance(CodyAgentClient.class); | ||
|
||
@Nullable public Editor editor; | ||
|
||
// Callback that is invoked when the agent sends a "chat/updateMessageInProgress" notification. | ||
@Nullable public Consumer<WebviewPostMessageParams> onNewMessage; | ||
|
||
|
@@ -26,14 +33,92 @@ public class CodyAgentClient { | |
// onSetConfigFeatures | ||
@Nullable public Consumer<WebviewPostMessageParams> onReceivedWebviewMessage; | ||
|
||
@Nullable public Editor editor; | ||
// Callback for the "editTask/didUpdate" notification from the agent. | ||
@Nullable private Consumer<EditTask> onEditTaskDidUpdate; | ||
|
||
// Callback for the "editTask/didDelete" notification from the agent. | ||
@Nullable private Consumer<EditTask> onEditTaskDidDelete; | ||
|
||
// Callback for the "textDocument/edit" request from the agent. | ||
@Nullable private Consumer<TextDocumentEditParams> onTextDocumentEdit; | ||
|
||
// Callback for the "workspace/edit" request from the agent. | ||
@Nullable private Consumer<WorkspaceEditParams> onWorkspaceEdit; | ||
|
||
public void setOnEditTaskDidUpdate(@Nullable Consumer<EditTask> callback) { | ||
onEditTaskDidUpdate = callback; | ||
} | ||
|
||
public void setOnEditTaskDidDelete(@Nullable Consumer<EditTask> callback) { | ||
onEditTaskDidDelete = callback; | ||
} | ||
|
||
@JsonNotification("editTask/didUpdate") | ||
public void editTaskDidUpdate(EditTask params) { | ||
onEventThread( | ||
() -> { | ||
if (onEditTaskDidUpdate != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For brevity, what about providing a default callback which logs, making the callback non-nullable everywhere, and just calling directly here? |
||
onEditTaskDidUpdate.accept(params); | ||
} else { | ||
logger.warn("No callback registered for editTask/didUpdate"); | ||
} | ||
return null; | ||
}); | ||
} | ||
|
||
@JsonNotification("editTask/didDelete") | ||
public void editTaskDidDelete(EditTask params) { | ||
onEventThread( | ||
() -> { | ||
if (onEditTaskDidDelete != null) { | ||
onEditTaskDidDelete.accept(params); | ||
} else { | ||
logger.warn("No callback registered for editTask/didDelete"); | ||
} | ||
return null; | ||
}); | ||
} | ||
|
||
public void setOnTextDocumentEdit(@Nullable Consumer<TextDocumentEditParams> callback) { | ||
onTextDocumentEdit = callback; | ||
} | ||
|
||
@JsonRequest("textDocument/edit") | ||
public CompletableFuture<Boolean> textDocumentEdit(TextDocumentEditParams params) { | ||
return onEventThread( | ||
() -> { | ||
if (onTextDocumentEdit != null) { | ||
onTextDocumentEdit.accept(params); | ||
} else { | ||
logger.warn("No callback registered for textDocument/edit"); | ||
} | ||
return true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated to this change, but I think a lot of our future hopes and dreams hinge on this boolean. The VSCode edit API can fail, and this boolean indicates the success or failure of the edit. When we have concurrent edits (in the sense that the Agent's model of the document isn't caught up to the JetBrains reality because the user has edited but the extension sends an edit), we could detect that and fail here. Punchline is that extension code doesn't handle those failures! We would need to have the extension handle that. But there's probably some not terribly burdensome abstraction, a "retryable edit", that could do that. Interested in your thoughts here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, let's do it -- I've seen it fail to apply once (no code returned by LLM) and would love to get more feedback to the user when this happens. |
||
}); | ||
} | ||
|
||
public void setOnWorkspaceEdit(@Nullable Consumer<WorkspaceEditParams> callback) { | ||
onWorkspaceEdit = callback; | ||
} | ||
|
||
@JsonRequest("workspace/edit") | ||
public CompletableFuture<Boolean> workspaceEdit(WorkspaceEditParams params) { | ||
return onEventThread( | ||
() -> { | ||
if (onWorkspaceEdit != null) { | ||
onWorkspaceEdit.accept(params); | ||
} else { | ||
logger.warn("No callback registered for workspace/edit"); | ||
} | ||
return true; | ||
}); | ||
} | ||
|
||
/** | ||
* Helper to run client request/notification handlers on the IntelliJ event thread. Use this | ||
* helper for handlers that require access to the IntelliJ editor, for example to read the text | ||
* contents of the open editor. | ||
*/ | ||
private <T> CompletableFuture<T> onEventThread(Supplier<T> handler) { | ||
private <T> @NotNull CompletableFuture<T> onEventThread(Supplier<T> handler) { | ||
CompletableFuture<T> result = new CompletableFuture<>(); | ||
ApplicationManager.getApplication() | ||
.invokeLater( | ||
|
@@ -47,24 +132,24 @@ private <T> CompletableFuture<T> onEventThread(Supplier<T> handler) { | |
return result; | ||
} | ||
|
||
// Webviews | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious where this comes from, seems unrelated to edits. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is old chat API which was removed since, it should be removed from there. |
||
@JsonRequest("webview/create") | ||
public CompletableFuture<Void> webviewCreate(WebviewCreateParams params) { | ||
logger.error("webview/create This request should not happen if you are using chat/new."); | ||
return CompletableFuture.completedFuture(null); | ||
} | ||
|
||
// ============= | ||
// Notifications | ||
// ============= | ||
|
||
@JsonNotification("debug/message") | ||
public void debugMessage(DebugMessage msg) { | ||
public void debugMessage(@NotNull DebugMessage msg) { | ||
logger.warn(String.format("%s: %s", msg.getChannel(), msg.getMessage())); | ||
} | ||
|
||
// Webviews | ||
@JsonRequest("webview/create") | ||
public CompletableFuture<Void> webviewCreate(WebviewCreateParams params) { | ||
logger.error("webview/create This request should not happen if you are using chat/new."); | ||
return CompletableFuture.completedFuture(null); | ||
} | ||
|
||
@JsonNotification("webview/postMessage") | ||
public void webviewPostMessage(WebviewPostMessageParams params) { | ||
public void webviewPostMessage(@NotNull WebviewPostMessageParams params) { | ||
ExtensionMessage extensionMessage = params.getMessage(); | ||
|
||
if (onNewMessage != null | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,3 @@ | ||
package com.sourcegraph.cody.agent | ||
|
||
data class CommandExecuteParams(val command: String, val args: List<String>) | ||
data class CommandExecuteParams(val command: String, val arguments: List<String>) |
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 fine for now, but longer term is this inviting disaster with overwriting a single callback? Should this throw if the callback is already set, or can the wire-up move to some run once initialization step?
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 would say let's remove
setOnEditTaskDidUpdate
method and moveonEditTaskDidUpdate
update code intoCodyAgentService::onStartup
as other similar callback.Current code won't work is some scenarios, e.g. after agent restart after a crash or account change.
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 agree, but there are 8 of these callbacks (I think my PR only added one), and we should rearchitect all of them in one go. I've added issue 1136 to track it.