Do not send uuid-named temp files to the client #4142
Conversation
| if (uri.scheme === 'untitled') await openUntitledDocument(uri) | ||
| return workspaceDocuments.openTextDocument(uri) | ||
| const result = toUri(uriOrString) | ||
| if (result) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
mkondratek
changed the title
mkondratek
force-pushed
the
mkondratek/add-missing-extension
branch
from
265c8dd
to
e32176c
Compare
| @@ -951,7 +951,7 @@ describe('Agent', () => { | |||
| .allUris() | |||
| .filter(uri => vscode.Uri.parse(uri).scheme === 'untitled') | |||
| expect(untitledDocuments).toHaveLength(2) | |||
| const [untitledDocument] = untitledDocuments | |||
There was a problem hiding this comment.
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?
Test plan
Tested with sourcegraph/jetbrains#1478 on both macOs and Windows - works