search

sourcegraph / cody

AI that knows your entire codebase
https://cody.dev
Apache License 2.0
2.22k stars 213 forks source link

Support async local storage #4714

Closed vovakulikov closed 2 days ago

vovakulikov commented 3 days ago

Part of SRCH-632

This is the first PR that comes from the biggest change we did for Cody Web in this main PR here

This PR makes changes in the LocalStorage abstraction in order to support newly created IndexDB storage. We need to support index db storage since this is the only storage that can be run within web-worker freely. (web workers don't support standard browser local storage)

Test plan

vovakulikov commented 3 days ago

@sourcegraph/cody-agent maybe you can help me with agent-bindings CI says that I do have some changes in this PR diff that possibly changes something in the agent protocol API, but I haven't found anything directly related to this. In fact, I ran bindings generation locally and it didn't produce any changes in the JSON protocol.

Maybe some of you can take a look at this (I'm probably missing something obvious here)

olafurpg commented 3 days ago

The current design of the agent is very intentional, we are committed to using VS Code APIs as our official SDK. I am concerned this PR goes against that design by introducing custom types into our SDK without a strong justification.

olafurpg commented 3 days ago

I ran pnpm generate-agent-kotlin-bindings locally and got this diff here https://gist.github.com/olafurpg/708bfaed4edd12d1c91d2fbc94ac63e9

olafurpg commented 3 days ago

Feel free to cherry-pick 2cf6ca7470f3c8a0c215aa21f69ca1c14d0d5bc8

vovakulikov commented 2 days ago

Thanks for the binding commit, it looks like they were outdated in the main branch.

TL;DR: Since I have to move fast with these changes I've created a new PR with a sync API for IndexDB storage here https://github.com/sourcegraph/cody/pull/4719. I'm going to close this PR in favour of sync API to eliminate the friction of big changes in the extension.

About async API: I understand the concern that big changes in core extension might look scary but this bothers me a bit that we're not entitled to make any "big" changes in cody agent and extension API (even though we have as I believe a good test coverage with multiple test levels)

I've spent a significant amount of time dealing with dangling promises so I'd love to avoid having async storage if we can avoid it.

@olafurpg if you can provide some specific examples of problems you've had before I would love to test them with these changes.

My justifications for async API:

olafurpg commented 2 days ago

lead to the situation when we have a dirty state when a user has multiple browser tabs open. (Since each tab manages its own in-memory store). Observer Index DB API isn't supported yet googlechrome.github.io/samples/indexeddb-observers)

I think it's OK to address this problem later, it's a low-priority issue.

I can see that for the final version of Cody Web, we will store chat history in some remote source

Agreed. We need to move away from Memento for chat history anyways but this can be done separately/later when we have more time to think about the design/implementation tradeoffs.

While I was updating core places with async/await I noticed that most of the places were already async (in general the whole extension activation is async)

We try to have some baseline code coverage but there are many critical features/workflows that are impractical to write automated tests for (and we need to rely on manual code review instead). When I started working on the agent, the codebase had a lot of throwaway promises that were causing problems in other clients made tests brittle. It's taken a lot of effort to refactor parts of the codebase to pass around promises so we can await on them. At a quick glance, I don't think the diff in this PR was problematic per se, but I don't think we should make APIs async unless we have strong justification they can't be sync.