Initial implementation for Inline Edit and Document Code #440

steveyegge · 2024-01-30T19:17:40Z

Committing before it is ready because Olaf would like to play with it. Current state: Document Code makes a successful roundtrip to the agent. And we have Edit Instructions dialog. The rest is not wired up yet.

Test plan

Locally tested, but the code is by no means ready to use yet.

steveyegge · 2024-01-30T19:21:14Z

Note: Please do not approve or merge this PR.

Update March 18th -- the PR is ready for review & merge.

pkukielka · 2024-02-02T09:47:25Z

src/main/java/com/sourcegraph/cody/agent/CodyAgentClient.java

+
+  @JsonRequest("textDocument/edit")
+  public CompletableFuture<Boolean> textDocumentEdit(TextDocumentEditParams params) {
+    var future = new CompletableFuture<Boolean>();


That could be val I think :)
As a side note if you never set result to false and only ned it to indicate there were no error maybe CompletableFuture<Unit> could be sufficient.

pkukielka · 2024-02-02T09:55:28Z

src/main/kotlin/com/sourcegraph/cody/edit/EditCodeInlayRenderer.kt

+  init {
+    // Call validate() initially to ensure the layout is updated
+    rendererPane.add(component)
+    component.validate()


General comment so I won't forget later:
be careful with EDT calls in init, as then every object creation of EditCodeInlayRenderer need to be in the EDT as well, and in past we had hard to spot bugs because of this.

pkukielka · 2024-02-02T09:59:29Z

src/main/kotlin/com/sourcegraph/cody/edit/EditCodeInlayRenderer.kt

+  ) {
+    // Various failed attempts to get something to render in the inlay area.
+    rendererPane.bounds = targetRegion
+    component.setBounds(0, 0, targetRegion.width, targetRegion.height)


Probably won't be the case in your situation, but please double check CPU consumption when changing bounds or recalculating layout in paint or repaint function.
I introduced nasty bug which spiked our CPU usage significantly in past because of this.

I'm unsure where this is used, but I assume component isn't part of the layout, it is just being used to paint into the renderer pane. So laying it out here should have less potential for mayhem.

steveyegge · 2024-03-18T02:11:56Z

This branch now has the full implementation, months in the making, of Document Code and Edit Code. It requires the stevey/extension-client-api Cody PR in order to function properly.

This is essentially just a "Happy Path Only" implementation. There is quite a bit more work left to do, but if you are careful, you can use the feature pretty reliably.

Document Code and Edit Code are available from the right-click context menu (only)
They are disabled by default; there is a new Setting for enabling them
Cancel, Undo, and Accept all work

There are still missing features (e.g. Diff, Retry), issues with concurrent editing tasks, behavioral annoyances (such as not auto-formatting the code after rewriting it), and so on. I will create GH issues for each of them.

Test Plan

Will start on e2e integration testing framework next.

dominiccooney

I've only looked through half of this but some feedback inline.

dominiccooney · 2024-03-18T07:06:04Z

CONTRIBUTING.md

+## Debugging VS Code
+
+Sometimes, the Agent behaves differently when called from IntelliJ than
+when called from VS Code, and you may need to debug the same code paths


What does in mean for the agent to be called from VSCode? When the extension calls VSCode APIs and hits agent shims?

This is fair feedback; I'll rewrite it to use the right terminology.

dominiccooney · 2024-03-18T07:06:21Z

CONTRIBUTING.md

+through the Agent twice. First, when called from the JetBrains extension
+side, and again when called from the VS Code extension side.
+
+To accomplish the latter, you can use VSC to debug itself. It works very


VSC => VSCode, ditto below.

Suggested change

To accomplish the latter, you can use VSC to debug itself. It works very

To accomplish the latter, you can use VSCode to debug itself. It works very

dominiccooney · 2024-03-18T07:07:25Z

CONTRIBUTING.md

+
+## Known Issues
+
+- Force-stopping the target often corrupts the indexes, requiring an


Naive question but what are "the" indexes?

Nit but can we tighten this up... a list with one bullet, with a sub bullet, and workarounds mentioned in the sub bullet and the next paragraph?

dominiccooney · 2024-03-18T07:13:42Z

src/main/java/com/sourcegraph/cody/agent/CodyAgentClient.java

+          } else {
+            logger.warn("No callback registered for textDocument/edit");
+          }
+          return true;


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.

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.

dominiccooney · 2024-03-18T07:15:21Z

src/main/java/com/sourcegraph/cody/agent/CodyAgentClient.java

+  @Nullable private Consumer<WorkspaceEditParams> onWorkspaceEdit;
+
+  public void setOnEditTaskDidUpdate(@Nullable Consumer<EditTask> callback) {
+    onEditTaskDidUpdate = callback;


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?

I would say let's remove setOnEditTaskDidUpdate method and move onEditTaskDidUpdate update code into CodyAgentService::onStartup as other similar callback.
Current code won't work is some scenarios, e.g. after agent restart after a crash or account change.

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.

dominiccooney · 2024-03-18T07:39:16Z

src/main/kotlin/com/sourcegraph/cody/agent/protocol/Range.kt

+  fun isZero() = start.isZero() && end.isZero()
+
+  companion object {
+    fun nullRange() = Range(Position(0, 0), Position(0, 0))


Mildly nervous about this "null range", "zero position", etc. idea. Because if you start a new file and tell Cody to do stuff, the position is going to be like this.

Could we make the protocol actually use undefined or null for missing, and trust when zeros do appear that we meant it?

Apologies; there were 3 unused methods here and this was one. I've done a sweep for other warnings as well.

dominiccooney · 2024-03-18T07:45:22Z

src/main/kotlin/com/sourcegraph/cody/edit/DocumentCodeSession.kt

+
+  // Brings up a diff view showing the changes the AI made.
+  override fun diff() {
+    // The FixupController issues a vscode.diff command to show the smart diff in the


No longer true. The VSCode code lens UI does this, but we're using the "client UI" now and I made the vscode.diff an implementation detail of the VSCode code lens UI. (Of course, it is software, we can do whatever we want. But that's where the Agent/extension/VSCode side changes are at right now.)

dominiccooney · 2024-03-18T07:51:45Z

src/main/kotlin/com/sourcegraph/cody/edit/EditCodeInlayRenderer.kt

+import javax.swing.CellRendererPane
+import javax.swing.JPanel
+
+class EditCodeInlayRenderer(private val component: JPanel, private val requiredHeight: Int) :


Sorry to be dense, but where is this used?

Was a leftover, not used - removed.

dominiccooney · 2024-03-18T07:54:38Z

src/main/kotlin/com/sourcegraph/cody/edit/EditCodeInlayRenderer.kt

+  ) {
+    // Various failed attempts to get something to render in the inlay area.
+    rendererPane.bounds = targetRegion
+    component.setBounds(0, 0, targetRegion.width, targetRegion.height)


I'm unsure where this is used, but I assume component isn't part of the layout, it is just being used to paint into the renderer pane. So laying it out here should have less potential for mayhem.

dominiccooney · 2024-03-18T07:55:05Z

src/main/kotlin/com/sourcegraph/cody/edit/EditCodeInlayRenderer.kt

+    component.setBounds(0, 0, targetRegion.width, targetRegion.height)
+    component.validate()
+    rendererPane.validate()
+    rendererPane.background = JBColor.WHITE


pkukielka · 2024-03-18T11:20:58Z

src/main/kotlin/com/sourcegraph/cody/agent/protocol/TextEdit.kt

+    val type: String,
+
+    // Valid for replace & delete.
+    val range: Range? = null,


Wouldn't it make sense to create two different types of edits, one position based and one renge based? This way we don't need this comments and all fields would always be required and used.

Probably, but it's a bigger change; maybe we can wait until after this PR is merged?

pkukielka · 2024-03-18T11:22:42Z

src/main/kotlin/com/sourcegraph/cody/agent/protocol/WorkspaceEditOperation.kt

+
+data class WorkspaceEditOperation(
+    val type: String, // all
+    val uri: String? = null, // created, delete, edit


Can we make sure URI parameters are sent as URIs and not strings?
Generally I think we should be extra careful around uri/path objects, they were source of many conversion problems in paths between VSCode <-> Intellij, especially adding additional platforms as windows.

They have been bouncing back and forth in origin/main between URI and String and causing me no end of merge headaches. My understanding is that URI has trouble serializing in some cases.

It works now, and I don't want to break it, so let's plan an API cleanup pass afterwards.

pkukielka · 2024-03-18T11:34:41Z

src/main/kotlin/com/sourcegraph/cody/edit/EditCommandPrompt.kt

+import javax.swing.event.DocumentListener
+
+/** Pop up a user interface for giving Cody instructions to fix up code at the cursor. */
+class EditCommandPrompt(val controller: FixupService, val editor: Editor, val dialogTitle: String) {


Could you please annotate methods which requires dispatch thread with @RequiresEdt?
It can give us additional safety net in the newest IJ versions.

pkukielka · 2024-03-18T11:42:00Z