feat: Improve warm cody web startup time #6947
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This change is part of the following stack: Change managed by git-spice. |
3 tasks
dominiccooney
approved these changes
Feb 12, 2025
vovakulikov
approved these changes
Feb 12, 2025
fkling
changed the base branch from
main
to
fkling/fix-agent-workspace-inconsistencies
fkling
force-pushed
the
fkling/improve-cody-web-warm-startup
branch
from
fb79d9c to
7a0c391
Compare
fkling
force-pushed
the
fkling/fix-agent-workspace-inconsistencies
branch
from
d2a5b3f to
fb43d28
Compare
fkling
force-pushed
the
fkling/improve-cody-web-warm-startup
branch
3 times, most recently
from
89ee8d3 to
a8e9c6e
Compare
fkling
force-pushed
the
fkling/fix-agent-workspace-inconsistencies
branch
from
e4170cc to
c016044
Compare
fkling
force-pushed
the
fkling/improve-cody-web-warm-startup
branch
2 times, most recently
from
6620e15 to
23ac20a
Compare
fkling
added a commit
that referenced
this pull request
Feb 24, 2025
While trying to adapt #6947 to the changes from #6909, I noticed that workspace handling is inconsistent in the agent. In particular: - The agent accepts a `workspaceRootUri` upon initialization and stores it on `AgentWorkspaceDocuments`. This value eventually ends up in `workspaceFolders`. - We support `workspaceFolder/didChange`, which updates `workspaceFolders` but doesn't affect `workspaceRootUri` in any way. - `getWorkspaceFolder` and `asRelativePath` ignore `workspaces` and work directly with `workspaceRootUri` So it seems that any client that wants to use `workspaceFolder/didChange` will eventually end up with an inconsistent state. This PR aims to fix that. It removes `workspaceRootUri` and updates all places that used it to work with `workspaceFolders` instead. `asAbsolutePath` was updated to use the extension URI instead, which seems more in line with what the purpose of that method is (though it doesn't seem like we use it anywhere). `workspaceFolders` is still initialized from the client info, directly instead of going through `workspaceDocuments`. ## Test plan I've tested this manually in the context of #6947 and sourcegraph/sourcegraph#3333. The following secanarios work as expected: - On the chat page, remote repo mentions work (i.e. they appear in the mentions menu). No workspace is set. - When opening the cody sidebar on a repo page, the workspace is updated (deduced by seeing the `Current repository` at-mentions entry point to the current repository, as well the default file filtering logic only suggest files from the current repository) - With the cody sidebar open, navigate directory to a different repository via fuzzy finder. As above, the menu entry and the file search correctly resolve to the new repository. > [!NOTE] > What I've not been able to successfully test is the `Rules` provider. But I suspect that there might be unrelated issues with it, because the network requests it made in the web demo resulted in [428](https://www.rfc-editor.org/rfc/rfc6585#section-3) errors, saying that the `X-Requested-Width` header was missing.
Base automatically changed from
fkling/fix-agent-workspace-inconsistencies
to
main
February 24, 2025 11:35
fkling
force-pushed
the
fkling/improve-cody-web-warm-startup
branch
from
23ac20a to
ae5e5f5
Compare
fkling
force-pushed
the
fkling/improve-cody-web-warm-startup
branch
from
ae5e5f5 to
7038423
Compare
|
@vovakulikov, @dominiccooney I've changed the implementation to move the agent initialization back into cody web, to keep things encapsulated. We still get the benefit of reusing a single agent instance. |
vovakulikov
approved these changes
Feb 25, 2025
| // Empty root URI leads to openctx configuration resolution failure, any non-empty | ||
| // mock value (Cody Web doesn't really use any workspace related features) | ||
| workspaceRootUri: `repo:${repository ?? ''}`, | ||
| workspaceRootUri: '', |
There was a problem hiding this comment.
I wonder if we still need to have this field if the main way to specify workspace is through sending did/workspaceChange event
how do you specify the workspace path in JB cc @dominiccooney
There was a problem hiding this comment.
JetBrains populates that field for the initial workspace here: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/cody/-/blob/jetbrains/src/main/kotlin/com/sourcegraph/cody/agent/CodyAgent.kt?L137-139
Cody for JetBrains does not send workspaceFolder/didChange.
JetBrains does have multi-root workspaces, but I believe the feature is pretty obscure.
fkling
commented
Feb 25, 2025
…rcegraph/cody into fkling/improve-cody-web-warm-startup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One major problem with cody web right now is that we recreate the cody agent whenever we navigate to a page that shows cody web components.
Worse even, on e.g. the prompts page where we should three prompt inputs, we create three separate instances of the agent.
This initialization step takes time and uses server resources (having already caused Cloudflare overages (https://sourcegraph.slack.com/archives/C05EA9KQUTA/p1737688274086469)).
This PR proposes to only use a single instance of cody agent.
Important
My understanding of cody web and cody agent is limited. I don't know what implication it has to 'reuse' the same cody agent instance. I've manually tested the typical workflows (chatting, creating new chat, switching files with cody sidebar open, etc), but there are likely things I missed or am not aware of.
Important
This does not improve the initial loading time of cody web.
sg-cody-web.mp4
Test plan
Manual testing.