V1 new client extension API, for inline edits + document code #3445

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

This commit is part 1 of a joint effort between @dominiccooney and @steveyegge to make our client API simpler, more robust, and easier to test. The main change from the client's perspective is that the client no longer has to receive lenses as part of the protocol, focusing only on state transitions. This enabled a significant cleanup of the JetBrains-side logic.

The FixupController has been rewritten to use Strategy objects to abstract the operations and decouple the UI. There are several bug fixes in the Agent, as well as a new getFoldingRanges API for clients that need it. It's useful for figuring out the very first place to put a spinner, before the FixupController's task has finished initializing.

The commit goes hand in hand with stevey/inline-edits on the jetbrains repo, which implements v1 of Document Code and Edit Code.

Test plan

We will start immediately on the integration test framework for JetBrains. This commit is just a checkpoint so that we can start parallelizing some of this work.

agent/src/agent.ts

dominiccooney

Some minor feedback inline, PTAL. LGTM.

There's a format for bylines in commit descriptions. Also need to add @abeatrix here for the edit command:

Co-authored-by: Dominic Cooney <dominic.cooney@sourcegraph.com>
Co-authored-by: Beatrix <beatrix@sourcegraph.com>

dominiccooney · 2024-03-18T04:20:12Z

agent/src/agent.ts

-        this.registerNotification('textDocument/didFocus', document => {
+        this.registerNotification('textDocument/didFocus', (document: ProtocolTextDocument) => {
+            function isEmpty(range: Range | undefined): boolean {
+                return !range || range === new vscode.Range(0, 0, 0, 0)


This === isn't right; that's strict object identity just like JavaScript so trivially false.

vscode.Range has something called isEmpty that means the range start and end are the same, but that's not the intent here, so let's call it something else. (Since this comparison was wrong, can we just delete it? It means the 0-width range at (0,0)-(0,0) is still a valid thing.)

dominiccooney · 2024-03-18T04:29:55Z

agent/src/agent.ts

+                return !range || range === new vscode.Range(0, 0, 0, 0)
+            }
+            const documentWithUri = ProtocolTextDocumentWithUri.fromDocument(document)
+            // If the caller elided the content, as is the sensible thing to do, reconstruct it here.


Elide the judgement "as is the sensible thing to do" and just state it plainly, like "because the caller elides the content" if that always happens; "when the caller omits the content" if it may happen, etc.

Fair enough; elided. I do actually want to have it be recommended to client writers, as part of a broader effort to document the expected semantics of all the request and response objects. Not sure exactly where yet.

agent/src/agent.ts

dominiccooney · 2024-03-18T04:48:52Z

agent/src/agent.ts

+                'cody.command.edit-code',
+                args
+            )
+            // Wrap the task in a Thenable that returns a CommandResult, required by createEditTask().


In my experience, no good comes of providing Promise-like APIs when Promises are available, yet here we are...

This should be more succinct by not reiterating the types, and relying on failures to propagate through without mentioning them explicitly. Something like the following but with proper indentation.

return this.createEditTask( vscode.commands.executeCommand<FixupTask | undefined>('cody.command.edit-code', args) .then(task => task && { type: 'edit', task }) )

Works like a charm.

dominiccooney · 2024-03-18T04:50:48Z

vscode/src/commands/execute/edit.ts

+export async function executeEditCommand(
+    args: ExecuteEditArguments
+): Promise<EditCommandResult | undefined> {
+    return wrapInActiveSpan('command.test', async span => {


That span identifier looks like copypasta from the test command.

dominiccooney · 2024-03-18T04:51:55Z

vscode/src/commands/execute/edit.ts

+    return wrapInActiveSpan('command.test', async span => {
+        span.setAttribute('sampled', true)
+        const instruction = args.configuration?.instruction // get the instruction
+        const editor = getEditor()?.active // get the active editor


Just chain them, const document = getEditor()?.active.document

(Sorry for not using GitHub suggestions but something is wrong with multiline select in the web GitHub UI rn.)

I was able to fix it and commit the fix.

dominiccooney · 2024-03-18T04:52:27Z

vscode/src/commands/execute/edit.ts

+                    document,
+                    mode: 'edit',
+                },
+                source: DefaultEditCommands.Test,


This source also looks iffy for "edit" (or the source is right and the method name is iffy.)

Fixed all this code, which was definitely dubious.

abeatrix

From my testing I can confirm that all edit commands are working correctly!

Doc command:

Edit command:

Test command:

abeatrix · 2024-03-18T14:36:19Z

vscode/src/commands/execute/edit.ts

+                    document,
+                    mode: 'edit',
+                },
+                source: DefaultEditCommands.Test,


Suggested change

source: DefaultEditCommands.Test,

source: args.source || 'editor'

I think we can use editor here as the default source here

abeatrix · 2024-03-18T14:38:03Z

vscode/src/commands/execute/edit.ts

+
+import { wrapInActiveSpan } from '@sourcegraph/cody-shared/src/tracing'
+
+export async function executeEditCommand(


Prehaps we can just call the executeEdit function directly without this? (I might have missed some context so please ignore me if this is needed)

edit: Oh right, we wanted to move away from calling vs code command directly so yea ignore this :)

Fixed a serious bug with document contents disappearing in the Agent. The problem was that textDocument/didFocus would simply move the document from the wire into the workspace's document map for that URI. JetBrains was not sending the document content along with every didFocus, because it clutters the logs significantly, and the contents have not changed. I fixed this on the Agent side by copying in the existing document's content if none was passed. Also added 'editTask/getFoldingRanges' request to expose the indentation-based folding ranges API to clients (such as JetBrains) which cannot reliably compute folding ranges in a language-neutral way. This fixed a bug with not knowing the initial position for the spinner while we wait for the FixupController to reply to our initial commands/document request.

textDocument/didFocus was blowing away the cached selection as well as the cached document content. Changed to copy in the cached selection if none is present in the protocol document.

It was assuming that cody.commands.edit-code was returning an EditCommandResult, but it was just returning a FixupTask. So I had to write a little adapter.

Co-authored-by: Dominic Cooney <dominic.cooney@sourcegraph.com>

abeatrix · 2024-03-21T06:15:08Z

vscode/src/extension.common.ts

@@ -67,5 +67,5 @@ export async function activate(
        console.error(error)
    }

-    return api
+    return new ExtensionApi(context.extensionMode)


Suggested change

return new ExtensionApi(context.extensionMode)

return api

reverting this change fixed the integration test

abeatrix · 2024-03-21T06:15:42Z

vscode/src/extension.common.ts

 }

 export async function activate(
    context: vscode.ExtensionContext,
    platformContext: PlatformContext
 ): Promise<ExtensionApi> {
-    const api = new ExtensionApi(context.extensionMode)


reverting this change fixed the integration test

…#3445)" This reverts commit 5356bb5. This is missing code review feedback which went to stevey/extension-client-api branch.

Co-authored-by: Steve Yegge <stevey@sourcegraph.com>

umpox

Finally got chance to review this, great work @steveyegge and @dominiccooney. I think these are sensible abstractions 👍

umpox · 2024-03-21T09:13:21Z

vscode/src/non-stop/FixupController.ts

+        private readonly authProvider: AuthProvider,
+        client: ExtensionClient
+    ) {
+        this.controlApplicator = client.createFixupControlApplicator(this)


I like this abstraction 👍

umpox · 2024-03-21T09:14:34Z

vscode/src/non-stop/codelenses/provider.ts

+                        hasV2Event: true,
+                    }
+                )
+                telemetryRecorder.recordEvent('cody.fixup.codeLens', 'cancel')


Assuming that we are expecting that each client implements their own telemetry (I think this is fine)

umpox · 2024-03-21T09:15:31Z

vscode/src/non-stop/roles.ts

+export interface FixupActor {
+    /**
+     * Mark a task as accepted and stop tracking the task. Only applicable to
+     * tasks in the "applied" state. Sets the task state to "finished" and
+     * discards the task.
+     */
+    accept(task: FixupTask): void
+
+    /**
+     * Undo a task's edits and stop tracking the task. Only applicable to
+     * tasks in the "applied" state. If the undo succeeds, the task state is
+     * set to "finished" and the task is discarded.
+     */
+    undo(task: FixupTask): Promise<void>
+
+    /**
+     * Cancel a task. Sets the task state to "error" or "finished" and stops
+     * tracking the task. Tasks in any state can be cancelled.
+     */
+    cancel(task: FixupTask): void
+
+    /**
+     * Undo the task (see `undo`), prompt for updated instructions, and start
+     * a new task to try again. Only applicable to tasks in the "applied" state.
+     * @param task the task to retry.
+     * @param source the source of the retry, for event logging.
+     */
+    retry(task: FixupTask, source: ChatEventSource): Promise<FixupTask | undefined>
+}


umpox · 2024-03-21T09:17:36Z

vscode/src/non-stop/strategies.ts

+// An interface for decorating fixup tasks with controls.
+export interface FixupControlApplicator extends vscode.Disposable {
+    didUpdateTask(task: FixupTask): void
+    didDeleteTask(task: FixupTask): void
+    // Called when visible files changed.
+    // TODO: This API design is gross: this is *not* called when a new task
+    // is created in a file that is already visible. It *is* called every time
+    // visible files change, so be prepared to handle repeated calls with
+    // an empty or unchanged set of files efficiently. Unearth a consistent
+    // API here.
+    visibleFilesWithTasksMaybeChanged(files: readonly FixupFile[]): void
+}
+
+// A FixupControlApplicator which does not present any controls for fixup
+// tasks.
+export class NullFixupControlApplicator implements FixupControlApplicator {
+    public didUpdateTask(task: FixupTask): void {}
+    public didDeleteTask(task: FixupTask): void {}
+    public visibleFilesWithTasksMaybeChanged(files: readonly FixupFile[]): void {}
+    public dispose(): void {}
+}


Just a general comment, ideally we should use /** doc */ for doc strings, as they are attached for hovers

umpox · 2024-03-21T09:21:14Z

vscode/src/non-stop/strategies.ts

+    // TODO: This API design is gross: this is *not* called when a new task
+    // is created in a file that is already visible. It *is* called every time
+    // visible files change, so be prepared to handle repeated calls with
+    // an empty or unchanged set of files efficiently. Unearth a consistent
+    // API here.
+    visibleFilesWithTasksMaybeChanged(files: readonly FixupFile[]): void


We do need to listen to when the visible files change, regardless of if they are empty or not. This is currently used purely to enable/disable keyboard shortcuts when there is a visible edit task (in one of the visible open files). Just listening for a task being created is not sufficient here

steveyegge requested review from dominiccooney and umpox March 18, 2024 02:03

steveyegge mentioned this pull request Mar 18, 2024

Initial implementation for Inline Edit and Document Code sourcegraph/jetbrains#440

Merged

dominiccooney reviewed Mar 18, 2024

View reviewed changes

agent/src/agent.ts Outdated Show resolved Hide resolved

dominiccooney approved these changes Mar 18, 2024

View reviewed changes

abeatrix approved these changes Mar 18, 2024

View reviewed changes

steveyegge enabled auto-merge (squash) March 20, 2024 14:57

abeatrix disabled auto-merge March 21, 2024 05:05

dominiccooney and others added 17 commits March 21, 2024 14:55

EditManager does not need or use ContextProvider.

b8e1028

Wild times, let agent provide a client interface to the extension.

327ea40

Separate concerns in fixups.

72f8812

End-to-end working, needs some tidy up.

a1fd097

Add the selectionRange to EditTask.

1738b66

added Bee's edit-command API

f6984e1

added a getFoldingRanges RPC for clients without folding-range support

78d74dd

Another Range bugfix for textDocument/didFocus

e114c3e

textDocument/didFocus was blowing away the cached selection as well as the cached document content. Changed to copy in the cached selection if none is present in the protocol document.

added a warning log message

50ce5e6

fixed a type mismatch in the editComands/code request

c80e04e

It was assuming that cody.commands.edit-code was returning an EditCommandResult, but it was just returning a FixupTask. So I had to write a little adapter.

added a launch config for attaching to Agent

378a009

Fix automatic rebase scree.

7e2d105

Fix type import lint in agent.ts

b962525

biome format errors

5b3fe32

Update agent test for client side code lenses.

45a66e9

Update agent/src/agent.ts

286bffe

Co-authored-by: Dominic Cooney <dominic.cooney@sourcegraph.com>

abeatrix reviewed Mar 21, 2024

View reviewed changes

Initialize test support early to establish hooks.

c6ca5b8

dominiccooney force-pushed the stevey/extension-client-api-rebased branch from 8803528 to c6ca5b8 Compare March 21, 2024 06:24

dominiccooney merged commit 5356bb5 into main Mar 21, 2024
20 checks passed

dominiccooney deleted the stevey/extension-client-api-rebased branch March 21, 2024 06:33

dominiccooney added a commit that referenced this pull request Mar 21, 2024

Revert "V1 new client extension API, for inline edits + document code (…

2c7c8e8

…#3445)" This reverts commit 5356bb5. This is missing code review feedback which went to stevey/extension-client-api branch.

This was referenced Mar 21, 2024

Revert "V1 new client extension API, for inline edits + document code… #3490

Closed

Cherry-pick code review feedback missing from #3445 #3491

Merged

dominiccooney added a commit that referenced this pull request Mar 21, 2024

Cherry-pick code review feedback missing from #3445 (#3491)

4201825

Co-authored-by: Steve Yegge <stevey@sourcegraph.com>

umpox reviewed Mar 21, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

V1 new client extension API, for inline edits + document code #3445

V1 new client extension API, for inline edits + document code #3445

steveyegge commented Mar 18, 2024

dominiccooney left a comment

dominiccooney Mar 18, 2024

steveyegge Mar 20, 2024

dominiccooney Mar 18, 2024

steveyegge Mar 20, 2024

dominiccooney Mar 18, 2024

steveyegge Mar 20, 2024

dominiccooney Mar 18, 2024

dominiccooney Mar 18, 2024

steveyegge Mar 20, 2024 •

edited

dominiccooney Mar 18, 2024

steveyegge Mar 20, 2024

abeatrix left a comment

abeatrix Mar 18, 2024

abeatrix Mar 18, 2024

abeatrix Mar 21, 2024

abeatrix Mar 21, 2024

umpox left a comment

umpox Mar 21, 2024

umpox Mar 21, 2024

umpox Mar 21, 2024

umpox Mar 21, 2024

umpox Mar 21, 2024

	source: DefaultEditCommands.Test,
	source: args.source \|\| 'editor'


		import { wrapInActiveSpan } from '@sourcegraph/cody-shared/src/tracing'

		export async function executeEditCommand(

V1 new client extension API, for inline edits + document code #3445

V1 new client extension API, for inline edits + document code #3445

Conversation

steveyegge commented Mar 18, 2024

Test plan

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

Choose a reason for hiding this comment

steveyegge Mar 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abeatrix 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

umpox 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

steveyegge Mar 20, 2024 •

edited