Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Do not send uuid-named temp files to the client #4142

Merged
merged 9 commits into from
May 15, 2024
Merged

Do not send uuid-named temp files to the client #4142

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ee69947
Add missing extension
mkondratek May 9, 2024
583ee7e
Do not send uuid-named temp files to the client
mkondratek May 10, 2024
bbe59c9
fixes
mkondratek May 10, 2024
6d5d931
Remove this.insertionPromise from handleFileCreationResponse
mkondratek May 13, 2024
f02a6a5
Add comment on `toUri`
mkondratek May 13, 2024
b6a8332
Update tests
mkondratek May 13, 2024
e32176c
Fix tests
mkondratek May 14, 2024
fb59911
Fix tests
mkondratek May 15, 2024
6979182
Fix tests
mkondratek May 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions agent/src/TestClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ export class TestClient extends MessageHandler {
public readonly name: string
public workspace = new AgentWorkspaceDocuments()
public workspaceEditParams: WorkspaceEditParams[] = []
public textDocumentEditParams: TextDocumentEditParams[] = []

get serverEndpoint(): string {
return this.params.credentials.serverEndpoint
Expand Down Expand Up @@ -330,6 +331,7 @@ export class TestClient extends MessageHandler {
return Promise.resolve(true)
})
this.registerRequest('textDocument/edit', params => {
this.textDocumentEditParams.push(params)
return Promise.resolve(this.editDocument(params).success)
})
this.registerRequest('textDocument/show', () => {
Expand Down
6 changes: 2 additions & 4 deletions agent/src/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,7 @@ describe('Agent', () => {
.allUris()
.filter(uri => vscode.Uri.parse(uri).scheme === 'untitled')
expect(untitledDocuments).toHaveLength(2)
const [untitledDocument] = untitledDocuments
Copy link
Contributor

Choose a reason for hiding this comment

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

That looks fragile :) Which document are we getting there, the one with UUID in the name or the real one? Maybe we can get it by name or filter out the second one, so it will be clear what is going on there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

const [untitledDocument] = untitledDocuments.slice(1)
const testDocument = client.workspace.getDocument(vscode.Uri.parse(untitledDocument))
expect(trimEndOfLine(testDocument?.getText())).toMatchInlineSnapshot(
`
Expand Down Expand Up @@ -984,9 +984,7 @@ describe('Agent', () => {
explainPollyError
)

// Just to make sure the edit happened via `workspace/edit` instead
// of `textDocument/edit`.
expect(client.workspaceEditParams).toHaveLength(1)
expect(client.textDocumentEditParams).toHaveLength(1)
}, 30_000)

describe('Edit code', () => {
Expand Down
32 changes: 23 additions & 9 deletions agent/src/vscode-shim.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import path from 'node:path'
import * as uuid from 'uuid'
import type * as vscode from 'vscode'

import { logDebug, logError, setClientNameVersion } from '@sourcegraph/cody-shared'
import { extensionForLanguage, logDebug, logError, setClientNameVersion } from '@sourcegraph/cody-shared'

// <VERY IMPORTANT - PLEASE READ>
// This file must not import any module that transitively imports from 'vscode'.
Expand Down Expand Up @@ -292,10 +292,12 @@ const _workspace: typeof vscode.workspace = {
throw new Error('workspaceDocuments is uninitialized')
}

const uri = toUri(uriOrString)
if (uri) {
if (uri.scheme === 'untitled') await openUntitledDocument(uri)
return workspaceDocuments.openTextDocument(uri)
const result = toUri(uriOrString)
if (result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is causing tests hanging because the way TestClient is implemented:
https://github.com/sourcegraph/cody/blame/main/agent/src/TestClient.ts#L327

I think part of the problem is that TestClient do not inherit from Agent class but just from MessageHandler and add a lot of custom logic for things like workspace/edit. I'm sure there was some reason to do it that way, but I'm missing the full context.
@olafurpg would be the best person to comment on that.

Copy link
Member

Choose a reason for hiding this comment

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

TestClient intentionally implements endpoints like that from scratch to document how a real-world client could/would implement the endpoint. Ideally, TestClient should use auto-generated TypeScript bindings so it's not even using agent-protocol.ts.

Copy link
Member

Choose a reason for hiding this comment

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

It's unclear to me in this PR why shouldOpenInClient should be false when the argument is an object. This feels like the wrong abstraction/interpretation

Copy link
Contributor

@pkukielka pkukielka May 13, 2024 •
edited

Choose a reason for hiding this comment

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

Thanks Olaf, I think I was confused for a moment what is a client and what is a server code there, silly error on my side.
As for for the wrong abstraction you are not totally wrong, it's a bit hacky approach.
This object is used only when we want to create in memory temp files, and those we do not want to send to the clients.
@mkondratek maybe we could use named interface instead of the unnamed object to make that fact clear.

/edit
We might not be able to do that, as it comes from VSCode API:

		/**
		 * Opens an untitled text document. The editor will prompt the user for a file
		 * path when the document is to be saved. The `options` parameter allows to
		 * specify the *language* and/or the *content* of the document.
		 *
		 * @param options Options to control how the document will be created.
		 * @return A promise that resolves to a {@link TextDocument document}.
		 */
		export function openTextDocument(options?: { language?: string; content?: string }): Thenable<TextDocument>;

So maybe at least a comment.

if (result.uri.scheme === 'untitled' && result.shouldOpenInClient) {
await openUntitledDocument(result.uri)
}
return workspaceDocuments.openTextDocument(result.uri)
}
return Promise.reject(
new Error(`workspace.openTextDocument:unsupported argument ${JSON.stringify(uriOrString)}`)
Expand Down Expand Up @@ -452,20 +454,32 @@ const defaultTreeView: vscode.TreeView<any> = {
title: undefined,
}

/**
* @returns An object with a URI and a boolean indicating whether the URI should be opened in the client.
* This object with UUID path is used only when we want to create in-memory temp files, and those we do not want to send to the clients.
*/
function toUri(
uriOrString: string | vscode.Uri | { language?: string; content?: string } | undefined
): Uri | undefined {
): { uri: Uri; shouldOpenInClient: boolean } | undefined {
if (typeof uriOrString === 'string') {
return Uri.file(uriOrString)
return { uri: Uri.file(uriOrString), shouldOpenInClient: true }
}
if (uriOrString instanceof Uri) {
return uriOrString
return { uri: uriOrString, shouldOpenInClient: true }
}
if (
typeof uriOrString === 'object' &&
((uriOrString as any)?.language || (uriOrString as any)?.content)
) {
return Uri.from({ scheme: 'untitled', path: `${uuid.v4()}` })
const language = (uriOrString as any)?.language ?? ''
const extension = extensionForLanguage(language) ?? language
return {
uri: Uri.from({
scheme: 'untitled',
path: `${uuid.v4()}.${extension}`,
}),
shouldOpenInClient: false,
}
}
return
}
Expand Down
5 changes: 1 addition & 4 deletions vscode/src/edit/provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,13 +329,10 @@ export class EditProvider {
if (!fileIsFound) {
newFileUri = newFileUri.with({ scheme: 'untitled' })
}
this.insertionPromise = this.config.controller.didReceiveNewFileRequest(task.id, newFileUri)
try {
await this.insertionPromise
await this.config.controller.didReceiveNewFileRequest(task.id, newFileUri)
} catch (error) {
this.handleError(new Error('Cody failed to generate unit tests', { cause: error }))
} finally {
this.insertionPromise = null
}
}
}
Expand Down