What is this? The interface for useNavbarQueryState store pulled out to be shared with the VS Code extension.

I implemented a separate zustand store (ignore the fact that it's instantiated in a component, it's a temporary hack :) ) in the VS Code prototype and it seems to work well!

Temporary settings must be defined here to prevent circular dependency (search-ui <-> shared). I tried some more flexible solutions, like making the temporary settings schema generic, but it didn't seem to worth the effort to makes the types work (and kind of defeats the purpose of autocomplete knowing about all the possible temporary settings keys)

The onboarding tour doesn't make sense for VS Code yet, so SearchSidebar shouldn't depend on it

The current implementation references the zustand store directly, so it should be passed as a prop by each client

I'll need to do a more thorough review, but some quick initial thoughts:

Somehow passing a store/hook as prop feels weird to me. If possible, I feel like refactoring the component to accept whatever they get from the store as props, and introducing "container" components that then implement/provide this as necessary (e.g. one of the the extension and one for the web app) would be cleaner. But maybe I'm overthinking this.

Eventually we should think about how to more elegantly manage our zustand stores across clients

Contexts could help with that. We tried to use the zustand stores via context, but their type definition was difficult to get right. But alternatively we could pass the stores themselves via context (those references won't change anyway). This could also be combined with my previous statement. Curious what @sourcegraph/frontend-platform thinks.

@fkling, it seems we can benefit from using zustand stores via context. Is it possible that type definition issues are solved in the new version? Maybe, if it's a couple of hours of effort, it makes sense to fix them ourselves in the third party? It will make the usage via context easier to understand since it will follow zustand documentation.

In general, I agree that it would be great to avoid prop drilling and rely more on React context, especially for cross-package depedencies.

Somehow passing a store/hook as prop feels weird to me. If possible, I feel like refactoring the component to accept whatever they get from the store as props, and introducing "container" components that then implement/provide this as necessary (e.g. one of the the extension and one for the web app) would be cleaner. But maybe I'm overthinking this.

This is a good solution because the reason I didn't do it in the first place was that I un-creatively only considered the current parent as a candidate for lifting the zustand subscription to, which would be counterproductive performance-wise. I'll try the context approach first and fall back to the container approach if it takes too long. Thank you both!

FWIW, I'm OK with having a less optimal solution now and then clean this up. While you can absolutely try context, I feel like this would be better in a separate PR.

And just to clarify what I and Valery meant (because we meant different things 😆 ): I thought that passing down the store functions (e.g. useNavbarQueryState) via context would be an OK compromise, but it does make it a bit awkward to use (don't actually know the context API off the top of my head but something along these lines:

const useNavbarQueryState = useContext(...)
const {state} = useNavbarQueryState(...)

which is not that common).

Zustand offers proper context support and the nice thing about it that you wouldn't even have to update the components that use the stores (afaik). I think you could get away with just updating the implementation of the stores.
However, there are couple of things to consider:

• In the past we argued that having multiple contexts (one per store) isn't desirable. At the same time I think that merging everything into a single store isn't great either. A compromise could be that we have a single store for "individual" single global values (theme, isSourcegraphDotCom, etc) and a separate store for more complex state (search query state).
• For search query state, I moved a lot of the "action" functions from the store the module level (example). I liked that because components didn't have to subscribe to the store just to get a value (the function) that never changes. But I don't think that would work with context anymore and they would have to be moved back to the store. I'm ok with that but it's additional work.

Not sure how we should best coordinate all that 😅

I went off and basically implemented what seems to be Valery's proposal before I read this comment 😅. I can do it the zustand way, just wondering now that we have it, does it still feel worth moving to a separate PR? I thought this was a change with limited scope since only search package components would have to worry about this, but I see that it would open a can of worms with the multiple contexts issue...

For search query state, I moved a lot of the "action" functions from the store the module level (example). I liked that because components didn't have to subscribe to the store just to get a value (the function) that never changes. But I don't think that would work with context anymore and they would have to be moved back to the store. I'm ok with that but it's additional work.

These are used only in web since <SearchBox> takes these as props, which is helpful since it means <SearchSidebar> is the only shared (search) component that directly references the zustand store. So it's not a problem we have to tackle yet? I don't see it as an issue that web helpers reference the web store directly

I can do it the zustand way, just wondering now that we have it, does it still feel worth moving to a separate PR?

Yeah, probably not worth it to do in a separate PR. I thought it might be weird to have some stores use context and not others, but maybe we shouldn't be dogmatic about this and use whether makes sense for that specific situations.

I know I proposed this, but seeing the store passed via context is weird too 😆 But it's OK for me if you want to keep it that way for now. I can also migrate it the "zustand way" at some point. I also feel like we might need to have a proper discussion again how we want to tackle this overall, outside of this specific PR.

which is helpful since it means <SearchSidebar> is the only shared (search) component that directly references the zustand store

Ah... I somehow didn't realize which components are actually all affected by this.

Got it, I opened an issue (#29984) to track this tech debt

I'm not sure this is related to search UI in particular. This is used in multiple places in the product, not just search UI. I think this component would live better in shared.

This component also seems to me more apt for shared than search-ui

I wonder if now that we'll have have temporary settings in shared if we could easily plug them into the browser and editor extensions for even better settings sync

Looks like a great candidate to be moved to the Wildcard package. We can work on it in a follow-up PR.