Skip to content

search: Restructure "sync query from URL" logic

Administrator requested to merge fkling/query-state-from-url into main

Created by: fkling

tl;dr: Instead of relying on a couple of useEffects in multiple components to sync/update the search query from the URL we are now listening to URL changes in the app root and update the store directly from there.

It's possible that in some obscure situations the React effect for updating the query state doesn't happen. But there is also a more general issue with how we sync the query information from the URL to the app state, which causes multiple re-renders and "flashing" of the query in the search query input.

The "flashing" happens because we use useEffect in a lot of places to trigger updates to the search query store. useEffect callbacks are executed after the component has already rendered, which is too late.

I tried various things moving the "URL to store" syncing logic to various places, but all of them had some issues. The two main issues are:

  • Query information is used in multiple places, so ideally the store is update before any component that depends on it is rendered. That means a store update should be triggered close to the root component.
  • When syncing is tried to component re-rendering we restrict ourselves to updating the store after the containing component renders. We cannot update the store while the component re-renders because that would cause other components to update which causes a React error.

Eventually I ended up attaching a handler that listens to location changes in the web app root and updates the query store accordingly, which I think makes the most sense, at least for the time being. This causes the store to be updated independently of component re-rendering. The logic for updating the query and the search context, which was previously contained in Layout.tsx and GlobalNavbar.tsx, is now located in a single place.

I'm still not very happy about the fact that updateQueryStateFromLocation directly calls out to useNavbarQueryState but it did make it easier to write tests that cover all the cases. I'll likely revisit this at some point.

Before/after video:

https://user-images.githubusercontent.com/179026/189132525-c4409f68-48c8-4c04-b4d9-f0825fdd8c9d.mp4

Test plan

To (not) reproduce the "empty input bug":

  • Open Sourcegraph in a window that's < 1000px wide. It should show the collapsed menu bar.
  • Increase size to see full menu bar.
  • Enter and submit search query on search homepage
  • The query input should contain the query on the results page

index.test.ts (which was renamed from index.test.tsx) contains additional unit tests.

App preview:

Check out the client app preview documentation to learn more.

Merge request reports