Allow clients to redefine untitled files protocol during their creation #4841

pkukielka · 2024-07-11T16:00:49Z

Closes https://linear.app/sourcegraph/issue/CODY-2004/bug-cody-recreates-the-file-that-was-created-by-generate-unit-tests

Changes

Some IDEs (maybe even most IDEs except VSC) does not have notion of unsaved files.
Keeping two different models in IDE and in the agent is problematic and leads to hard to track errors.
It is easier to let client be honest and notify agent about real protocol which it is using (most likely file:// instead of untitled://).

As a result that change slightly modifies behaviour of the openTextDocument.
One could now call openTextDocument with untitled://abc parameter, and as a result get a TextDocument which uri points to file://abc (or any other path, really).

Test plan

Build JetBrains branch with this PR using CODY_DIR env var
Ask cody to generate tests for some file which does not have tests yet
Accept the changes
Delete that newly created file
Perform autocomplete in some other file
Make sure test file was not recreated

mkondratek · 2024-07-12T11:14:29Z

vscode/src/jsonrpc/agent-protocol.ts

@@ -294,7 +294,7 @@ export type ServerRequests = {
    'window/showMessage': [ShowWindowMessageParams, string | null]

    'textDocument/edit': [TextDocumentEditParams, boolean]
-    'textDocument/openUntitledDocument': [UntitledTextDocument, boolean]
+    'textDocument/openUntitledDocument': [UntitledTextDocument, ProtocolTextDocument | undefined | null]


is it something normal to have both undefined and null as a possible return type? for me it looks weird

It's typical declaration in our protocol, please look at the rest of the file.

mkondratek · 2024-07-12T11:15:22Z

agent/src/vscode-shim.ts

-                scheme: 'untitled',
-                path: `${uuid.v4()}.${extension}`,
-            }),
-            shouldOpenInClient: false,


mkondratek

works and looks good

pkukielka · 2024-07-15T09:15:34Z

agent/src/agent.ts

@@ -482,7 +482,7 @@ export class Agent extends MessageHandler implements ExtensionClient {

        this.registerNotification('textDocument/didSave', async params => {
            const uri = vscode.Uri.parse(params.uri)
-            const document = await vscode.workspace.openTextDocument(uri)
+            const document = await this.workspace.openTextDocument(uri)


Unrelated change but it was looking like bug to me.

pkukielka · 2024-07-16T14:04:45Z

agent/src/agent.ts

+            throw new Error(errorMessage)
+        }
+
+        const result = await this.request('textDocument/openUntitledDocument', {


Previously this call was in the openTextDocument, I think now it is in much more logical and better isolated place (openTextDocument should not have side effects like creating new files, and we cannot guarantee that for openUntitledDocument implementation as e.g. IntelliJ creates those files on the disk.).

pkukielka · 2024-07-16T14:05:44Z

agent/src/vscode-shim.ts

@@ -301,16 +301,14 @@ const _workspace: typeof vscode.workspace = {
            throw new Error('workspaceDocuments is uninitialized')
        }

-        const result = toUri(uriOrString)
-        if (result) {
-            if (result.uri.scheme === 'untitled' && result.shouldOpenInClient) {


That was source of some of our problems, as VSC does not expect any side effects from this function, but we do not control client implementation.

umpox · 2024-07-16T15:02:12Z

vscode/src/non-stop/FixupController.ts

+        const doc = await this.client.openNewDocument(vscode.workspace, newFileUri)
+        if (!doc) {
+            throw new Error(`Cannot create file for the fixup: ${newFileUri.toString()}`)
+        }


Can this not just be await vscode.workspace.openTextDocument(newFileUri), or will that cause problems?

My worry is that it won't be clear to everyone that we need to use this openNewDocument method. and we will introduce bugs with untitled files elsewhere

We do not need to use it for all untitled:// files (even more, we shouldn't), only files which we want to present to the users. I do not think there is right now any other such use case other than edits. Perhaps we should change the name to reflect that fact better.

Problem is fundamental:
Jetbrains do not have untitled:// (or unsaved) files support.
They need to be represented as normal file://
And unless we want to do a manual file mapping for every URI used in the protocol, and send request to IntelliJ for every openTextDocument call, this is best option we are left with.

I'm very open to discussion though. If you would like to jump on the call @umpox I can explain all corner cases.

umpox

Approving on the basis that this works and fixes a bug.

Will follow up when I'm back from OOO on if we can make this more robust for future usage too

…on (#1903) Closes https://linear.app/sourcegraph/issue/CODY-2004/bug-cody-recreates-the-file-that-was-created-by-generate-unit-tests ## Changes Some IDEs (maybe even most IDEs except VSC) does not have notion of unsaved files. Keeping two different models in IDE and in the agent is problematic and leads to hard to track errors. It is easier to let client be honest and notify agent about real protocol which it is using (most likely `file://` instead of `untitled://`). As a result that change slightly modifies behaviour of the `openTextDocument`. One could now call `openTextDocument` with `untitled://abc` parameter, and as a result get a `TextDocument` which uri points to `file://abc` (or any other path, really). ## Test plan 1. Build JetBrains with this PR using CODY_DIR env var and [this PR](sourcegraph/cody#4841) 2. Ask cody to generate tests for some file which does not have tests yet 3. Accept the changes 4. Delete that newly created file 5. Perform autocomplete in some other file 6. Make sure test file was not recreated

This was referenced Jul 11, 2024

Initial openDocument implementation #4351

Closed

Allow clients to redefine untitled files protocol during their creation sourcegraph/jetbrains#1903

Merged

mkondratek reviewed Jul 12, 2024

View reviewed changes

agent/src/vscode-shim.ts

scheme: 'untitled',

path: `${uuid.v4()}.${extension}`,

}),

shouldOpenInClient: false,

Copy link

Contributor

mkondratek Jul 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice 🚀

mkondratek approved these changes Jul 12, 2024

View reviewed changes

pkukielka commented Jul 15, 2024

View reviewed changes

pkukielka force-pushed the pkukielka/fix-untitled-files branch from 6fa7ee8 to 996d0a9 Compare July 16, 2024 13:21

pkukielka commented Jul 16, 2024

View reviewed changes

umpox reviewed Jul 16, 2024

View reviewed changes

umpox approved these changes Jul 16, 2024

View reviewed changes

umpox approved these changes Jul 22, 2024

View reviewed changes

Allow clients to redefine untitled files protocol during their creation

207572d

pkukielka force-pushed the pkukielka/fix-untitled-files branch from 996d0a9 to 207572d Compare July 31, 2024 10:43

pkukielka merged commit 6443a0b into main Jul 31, 2024
19 checks passed

pkukielka deleted the pkukielka/fix-untitled-files branch July 31, 2024 12:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow clients to redefine untitled files protocol during their creation #4841

Allow clients to redefine untitled files protocol during their creation #4841

pkukielka commented Jul 11, 2024 •

edited

Loading

mkondratek Jul 12, 2024

pkukielka Jul 16, 2024

mkondratek Jul 12, 2024

mkondratek left a comment

pkukielka Jul 15, 2024

pkukielka Jul 16, 2024 •

edited

Loading

pkukielka Jul 16, 2024 •

edited

Loading

umpox Jul 16, 2024

pkukielka Jul 16, 2024 •

edited

Loading

umpox left a comment

Allow clients to redefine untitled files protocol during their creation #4841

Allow clients to redefine untitled files protocol during their creation #4841

Conversation

pkukielka commented Jul 11, 2024 • edited Loading

Changes

Test plan

mkondratek Jul 12, 2024

Choose a reason for hiding this comment

pkukielka Jul 16, 2024

Choose a reason for hiding this comment

mkondratek Jul 12, 2024

Choose a reason for hiding this comment

mkondratek left a comment

Choose a reason for hiding this comment

pkukielka Jul 15, 2024

Choose a reason for hiding this comment

pkukielka Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

pkukielka Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

umpox Jul 16, 2024

Choose a reason for hiding this comment

pkukielka Jul 16, 2024 • edited Loading

Choose a reason for hiding this comment

umpox left a comment

Choose a reason for hiding this comment

pkukielka commented Jul 11, 2024 •

edited

Loading

pkukielka Jul 16, 2024 •

edited

Loading

pkukielka Jul 16, 2024 •

edited

Loading

pkukielka Jul 16, 2024 •

edited

Loading