Fix push list resetting to latest 10 pushes on query param changes

tests/ui/job-view/PushListUrlDrift_test.jsx

@@ -0,0 +1,178 @@
+import fetchMock from 'fetch-mock';
+import { BrowserRouter, useLocation } from 'react-router';
+import { render, waitFor, fireEvent, cleanup } from '@testing-library/react';
+
+import { getProjectUrl } from '../../../ui/helpers/location';
+import FilterModel from '../../../ui/models/filter';
+import pushListFixture from '../mock/push_list';
+import jobListFixtureOne from '../mock/job_list/job_1';
+import PushList from '../../../ui/job-view/pushes/PushList';
+import {
+  usePushesStore,
+  fetchPushes,
+  initialState as pushesInitialState,
+} from '../../../ui/shared/stores/pushesStore';
+import { getApiUrl } from '../../../ui/helpers/url';
+
+// These tests use BrowserRouter (not MemoryRouter) on purpose: the bug under
+// test is an interleaving of raw window.history.pushState calls (which React
+// Router does NOT see) with popstate-dispatching URL updates (which it does).
+// MemoryRouter can't reproduce that drift.
+
+const repoName = 'autoland';
+const push1Revision = 'ba9c692786e95143b8df3f4b3e9b504dfbc589a0'; // id 511138
+
+// Router-visible search string, captured via a probe component.
+let routerSearch;
+function LocationProbe() {
+  routerSearch = useLocation().search;
+  return null;
+}
+
+describe('PushList URL drift between window.location and React Router', () => {
+  const currentRepo = {
+    id: 4,
+    repository_group: { name: 'development', description: 'meh' },
+    name: repoName,
+    dvcs_type: 'hg',
+    url: 'https://hg.mozilla.org/autoland',
+    branch: null,
+    codebase: 'gecko',
+    description: '',
+    active_status: 'active',
+    performance_alerts_enabled: false,
+    expire_performance_data: true,
+    is_try_repo: false,
+    pushLogUrl: 'https://hg.mozilla.org/autoland/pushloghtml',
+    revisionHrefPrefix: 'https://hg.mozilla.org/autoland/rev/',
+    getRevisionHref: () => 'foo',
+    getPushLogHref: () => 'foo',
+  };
+
+  // The URL fetchPushes builds when it falls back to a "reset" fetch after
+  // updateRange clears the store (fromchange/tochange present -> count=100).
+  const resetFetchUrl = `begin:${getProjectUrl(
+    '/push/?full=true&count=100',
+    repoName,
+  )}`;
+
+  beforeAll(() => {
+    // Initial load of a single-revision view.
+    fetchMock.get(
+      getProjectUrl(
+        `/push/?full=true&count=10&revision=${push1Revision}`,
+        repoName,
+      ),
+      { ...pushListFixture, results: pushListFixture.results.slice(0, 1) },
+    );
+    // "get next 10" fetch (count is incremented to 11 with push_timestamp__lte).
+    fetchMock.get(
+      `begin:${getProjectUrl(
+        '/push/?full=true&count=11&push_timestamp__lte=',
+        repoName,
+      )}`,
+      { ...pushListFixture, results: pushListFixture.results.slice(1, 3) },
+    );
+    // The buggy full-range refetch after the phantom range change.
+    fetchMock.get(resetFetchUrl, {
+      ...pushListFixture,
+      results: pushListFixture.results.slice(0, 3),
+    });
+    fetchMock.get(`begin:${getApiUrl('/jobs/?push_id=', repoName)}`, {
+      ...jobListFixtureOne,
+    });
+    fetchMock.get(
+      'begin:https://firefox-ci-tc.services.mozilla.com/api/index/v1/task/gecko.v2',
+      404,
+    );
+    fetchMock.get('begin:https://bugzilla.mozilla.org/rest/bug', { bugs: [] });
+  });
+
+  beforeEach(() => {
+    window.history.replaceState(
+      null,
+      '',
+      `/jobs?repo=${repoName}&revision=${push1Revision}`,
+    );
+  });
+
+  afterEach(() => {
+    cleanup();
+    usePushesStore.setState({ ...pushesInitialState });
+  });
+
+  afterAll(() => {
+    fetchMock.reset();
+  });
+
+  const renderPushList = () => {
+    // Manually trigger fetchPushes since outside testing the App does it.
+    fetchPushes();
+
+    return render(
+      <BrowserRouter>
+        <LocationProbe />
+        <div id="th-global-content">
+          <PushList
+            user={{ isLoggedIn: false }}
+            repoName={repoName}
+            currentRepo={currentRepo}
+            filterModel={new FilterModel(jest.fn(), window.location)}
+            duplicateJobsVisible={false}
+            groupCountsExpanded={false}
+            pushHealthVisibility="None"
+            getAllShownJobs={() => {}}
+          />
+        </div>
+      </BrowserRouter>,
+    );
+  };
+
+  const pushCount = () =>
+    document.querySelectorAll('[data-testid="push-header"]').length;
+
+  test('"get next" keeps React Router location in sync with window.location', async () => {
+    const { getByTestId } = renderPushList();
+
+    await waitFor(() => expect(pushCount()).toBe(1));
+
+    fireEvent.click(getByTestId('get-next-10'));
+    await waitFor(() => expect(pushCount()).toBe(3));
+
+    // fetchNextPushes rewrites revision -> tochange and addPushes appends
+    // fromchange.  React Router must see the same URL, otherwise its next
+    // navigation is computed from a stale search string.
+    await waitFor(() => expect(routerSearch).toBe(window.location.search));
+  });
+
+  test('clicking empty push-list space after "get next" must not clear and refetch the push range', async () => {
+    const { getByTestId } = renderPushList();
+
+    await waitFor(() => expect(pushCount()).toBe(1));
+
+    fireEvent.click(getByTestId('get-next-10'));
+    await waitFor(() => expect(pushCount()).toBe(3));
+
+    // Sanity: the raw pushState calls rewrote the URL from revision=X to
+    // tochange=X&fromchange=Y.
+    expect(window.location.search).toContain('tochange=');
+    expect(window.location.search).not.toContain('revision=');
+
+    // A mousedown on empty push-list space triggers clearJobViaUrl ->
+    // updateUrlSearch -> popstate.  React Router wakes up, sees `revision`
+    // "disappear" relative to its stale location, and PushList treats that as
+    // a range change: clearPushes() + fetchPushes() -- wiping the pushes the
+    // user just loaded.
+    fireEvent.mouseDown(document.getElementById('push-list'));
+
+    // Let the popstate-driven effects and any (buggy) refetch settle.
+    await new Promise((resolve) => {
+      setTimeout(resolve, 100);
+    });
+
+    // The user's effective push range did not change, so nothing should have
+    // been cleared or refetched.
+    expect(fetchMock.called(resetFetchUrl)).toBe(false);
+    expect(usePushesStore.getState().pushList).toHaveLength(3);
+  });
+});

tests/ui/job-view/PushList_test.jsx

@@ -50,8 +50,9 @@ describe('PushList', () => {
 
   beforeEach(() => {
     mockNavigate = jest.fn();
-    // Mock window.history.pushState for URL updates
+    // Mock window.history state updates so the jsdom URL stays clean
     jest.spyOn(window.history, 'pushState').mockImplementation(() => {});
+    jest.spyOn(window.history, 'replaceState').mockImplementation(() => {});
   });
 
   const currentRepo = {

tests/ui/job-view/stores/pushes_test.jsx

@@ -30,6 +30,7 @@ describe('Pushes Zustand store', () => {
     fetchMock.get(getApiUrl('/jobs/?push_id=1', repoName), jobListFixtureOne);
     fetchMock.get(getApiUrl('/jobs/?push_id=2', repoName), jobListFixtureTwo);
     jest.spyOn(window.history, 'pushState').mockImplementation(() => {});
+    jest.spyOn(window.history, 'replaceState').mockImplementation(() => {});
     delete window.location;
     window.location = { ...originalLocation, search: '', pathname: '/jobs' };
     // Reset store to initial state before each test
@@ -147,9 +148,9 @@ describe('Pushes Zustand store', () => {
 
     await usePushesStore.getState().fetchPushes(10, true);
 
-    expect(window.history.pushState).toHaveBeenCalledWith(
-      null,
-      null,
+    expect(window.history.replaceState).toHaveBeenCalledWith(
+      {},
+      '',
       expect.stringContaining(
         'tochange=ba9c692786e95143b8df3f4b3e9b504dfbc589a0',
       ),

ui/helpers/location.js

@@ -1,4 +1,5 @@
 import { thDefaultRepo } from './constants';
+import { replaceUrlSearch } from './router';
 import { createQueryParams, getApiUrl } from './url';
 
 export const getAllUrlParams = function getAllUrlParams(
@@ -19,9 +20,8 @@ export const getRepo = function getRepo() {
   return getUrlParam('repo') || thDefaultRepo;
 };
 
-// This won't update the react router history object
 export const replaceLocation = function replaceLocation(params) {
-  window.history.pushState(null, null, createQueryParams(params));
+  replaceUrlSearch(createQueryParams(params));
 };
 
 export const setUrlParam = function setUrlParam(field, value) {

ui/job-view/pushes/PushList.jsx

@@ -18,6 +18,7 @@ import {
   pollPushes,
 } from '../../shared/stores/pushesStore';
 import { updatePushParams } from '../../helpers/location';
+import { updateUrlSearch } from '../../helpers/router';
 
 import Push from './Push';
 import PushLoadErrors from './PushLoadErrors';
@@ -124,7 +125,11 @@ function PushList({
   const fetchNextPushes = useCallback(
     (count) => {
       const params = updatePushParams(location);
-      window.history.pushState(null, null, params);
+      // The revision -> tochange (or startdate removal) rewrite keeps the same
+      // top of range, so it must not be treated as a range change by the
+      // URL-watching effect below.
+      prevRouterSearch.current = params;
+      updateUrlSearch(params);
       fetchPushes(count, true);
     },
     [fetchPushes, location],