Fix push list resetting to latest 10 pushes on query param changes - #9721
Open
camd wants to merge 1 commit into
Open
Fix push list resetting to latest 10 pushes on query param changes#9721camd wants to merge 1 commit into
camd wants to merge 1 commit into
Conversation
replaceLocation() and PushList.fetchNextPushes wrote the URL with raw window.history.pushState, which React Router never sees. The next popstate-dispatching action (job click, mousedown on empty push-list space, filter toggle, back button) woke the router with a stale location.search, and PushList.handleUrlChanges misread the long-gone revision/startdate params as a range change, triggering clearPushes() and a refetch -- resetting to the latest 10 pushes when no fromchange was present. - replaceLocation() now uses replaceUrlSearch(): true replaceState semantics (it previously pushed, despite its name) plus a popstate dispatch so React Router stays in sync. - fetchNextPushes() uses updateUrlSearch() and preemptively updates prevRouterSearch so the intentional revision->tochange rewrite is not treated as a range change. - New regression test PushListUrlDrift_test.jsx uses BrowserRouter (MemoryRouter cannot observe raw history writes) to reproduce the drift and the spurious clear/refetch deterministically.
This file contains hidden or 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
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.
Problem
Users paging back through pushes ("get next N") or toggling filters would sporadically have the jobs view clear and re-render — sometimes resetting to the latest 10 pushes — losing their place while searching for a push. There was no clear STR; it "seemed related to adding or removing query string parameters."
Root cause
The app had URL writers that React Router never saw:
replaceLocation()/setUrlParam()(ui/helpers/location.js) wrote the URL with a rawwindow.history.pushState(despite the name), with no popstate dispatch.PushList.fetchNextPushesdid the same when rewritingrevision=X→tochange=X(and droppingstartdate) for "get next N".React Router's
location.search(andPushList'sprevRouterSearch) went stale. The next popstate-dispatching action — clicking a job, clicking empty push-list space (clear-selection handler), toggling a filter, or the back button — woke the router, andPushList.handleUrlChangescompared the stale search against the new one, misreading the long-gonerevision/startdateparams as a range change. That triggeredupdateRange()→clearPushes()+fetchPushes(): the whole push list wiped and refetched, falling back toDEFAULT_PUSH_COUNT = 10when the URL had nofromchange. The outcome depended on how raw writes interleaved with router-visible navigations, which is why no reliable STR existed.Deterministic STR (reproducible on production): open
/jobs?repo=autoland&revision=<sha>, click "get next 10", then click any empty space in the push list → entire page clears and refetches.Fix
Single-writer principle — every URL mutation is now visible to React Router:
replaceLocation()delegates toreplaceUrlSearch(): truereplaceStatesemantics (it previously pushed, bloating history on everyfromchangerewrite and param normalization) plus a popstate dispatch so the router stays in sync.fetchNextPushesusesupdateUrlSearch()and preemptively updatesprevRouterSearchso the intentionalrevision → tochangerewrite is not treated as a range change.Side benefit: browser Back after "get next" now lands on a consistent single-revision state, since the address bar and router agree at every step.
Tests
tests/ui/job-view/PushListUrlDrift_test.jsxreproduces both the router/URL drift and the spurious clear-and-refetch. It deliberately usesBrowserRouter—MemoryRoutercannot observe rawhistorywrites, which is why existing tests never caught this. Both tests fail on master and pass with this fix.pushes_test.jsxupdated to assertreplaceStateinstead of the old rawpushStateimplementation detail;PushList_test.jsxnow stubsreplaceStatealongsidepushStateto keep the jsdom URL isolated between tests.