Fix query state getting erased on location changes

Created by: lguychard

Fixes #13805

#13679 moved the logic that updates the navbar query from <GlobalNavbar/>'s constructor (admittedly not a great place to have it) to a useEffect() block, with location as a dependency.

As a result, every time the location was updated, for example when clicking on tokens in a code view, any query state that wasn't persisted to the URL (a partially edited query, or the scoped query added automatically when navigating to a repo/file) would be erased.

The immediate fix I saw was to narrow the dependency array of the useEffect() block, to avoid running it on every location change. But I'm wondering if there isn't still an issue left: the scoped query randomly can get randomly erased on page load (race condition), depending on which of the useEffect() blocks in <GlobalNavbar/> and <RepoContainer/> executes first.

I've simplified the useEffect() block in <GlobalNavbar/> (which could end up calling onNavbarQueryChange() multiple times, had some comments implying that it wouldn't update the query in interactive mode but still did it further down...), fixing the issue as a result, by restricting the cases in which we update the query state.

Created by: lguychard

Update: I've simplified the useEffect() block in <GlobalNavbar/> (which could end up calling onNavbarQueryChange() multiple times, had some comments implying that it wouldn't update the query in interactive mode but still did it further down...), fixing the issue as a result, by restricting the cases in which we update the query state.

Created by: codecov[bot]

Codecov Report

Merging #13954 into main will decrease coverage by 0.00%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main   #13954      +/-   ##
==========================================
- Coverage   51.10%   51.09%   -0.01%     
==========================================
  Files        1515     1515              
  Lines       77271    77269       -2     
  Branches     6895     7052     +157     
==========================================
- Hits        39487    39479       -8     
- Misses      34219    34226       +7     
+ Partials     3565     3564       -1     

Flag	Coverage Δ
#go	51.33% <ø> (-0.01%)
#integration	29.11% <100.00%> (-0.08%)
#storybook	18.24% <30.76%> (-0.01%)
#typescript	50.50% <100.00%> (-0.03%)
#unit	33.89% <23.07%> (-0.01%)

Impacted Files	Coverage Δ
web/src/nav/GlobalNavbar.tsx	97.36% <100.00%> (+4.86%)
web/src/tree/TreeRoot.tsx	81.03% <0.00%> (-6.90%)
web/src/repo/RepoRevisionContainer.tsx	58.06% <0.00%> (-6.46%)
web/src/repo/RepoContainer.tsx	73.41% <0.00%> (-1.27%)
cmd/frontend/graphqlbackend/zoekt.go	75.91% <0.00%> (-0.29%)

Created by: tjkandala

I was about to say that this reminds me of the problem that hamstrung the new breadcrumbs. I should add a warning in our refactoring guide that we cannot expect logic from constructors to work in useEffect.

However, is the conclusion that the logic itself was the problem? No race conditions from useEffect + dynamic imports?

Created by: lguychard

The problem was indeed that part of the logic that was ported from the constructor made no sense in a useEffect() block (the "if we have no component state..." bit, which did a weird carryover of location.state, and that is the part that would be problematic in a race condition scenario). AFAICT the new logic updates the query correctly in all cases that we care about. But would love a sanity check as it took me a minute to wrap my head around it

Created by: sqs

Thanks for fixing and for cleaning up the code!