Avoid race condition when setting initial BoxesView filters through URL (#2430)

pylipp · web-flow · commit adabc29460ef · 2025-10-24T21:23:45.000+02:00
diff --git a/front/src/hooks/useTableConfig.ts b/front/src/hooks/useTableConfig.ts
@@ -132,6 +132,18 @@ export const useTableConfig = ({
     }
     return urlFilters;
   }, [searchParams]);
+
+  // Compute the initial filters if there is no saved config
+  const initialColumnFilters = useMemo(() => {
+    if (!syncFiltersAndUrlParams) return defaultTableConfig.columnFilters;
+
+    // If URL params exist, merge them into defaults (URL wins)
+    const hasUrlParams = URL_FILTER_CONFIG.some(({ urlParam }) => searchParams.get(urlParam));
+    return hasUrlParams
+      ? createInitialColumnFilters(defaultTableConfig.columnFilters, urlFilters)
+      : defaultTableConfig.columnFilters;
+  }, [defaultTableConfig.columnFilters, syncFiltersAndUrlParams, searchParams, urlFilters]);
+
   const tableConfigsState = useReactiveVar(tableConfigsVar);
 
   const isInitialMount = useRef(true);
@@ -144,31 +156,45 @@ export const useTableConfig = ({
     [searchParams, setSearchParams],
   );
 
-  // Sync default filters to URL on first load if no URL parameters present
+  /* Initial mount logic (one-time):
+   *
+   * - We compute `initialColumnFilters` earlier so callers can synchronously read
+   *   URL-applied filters from the getters during render (avoids the race where
+   *   react-table/queries initialize with defaults before the effect runs).
+   *
+   * - Here in the effect we persist a table config only if there is no existing
+   *   saved config for the key. This avoids stomping user-saved settings on first load.
+   *
+   * - If URL params exist, we persist the URL-merged filters (URL wins over defaults).
+   *   If the URL is empty and there's no saved config, we persist the default filters
+   *   (and write them into the URL via the existing updateUrl logic).
+   *
+   * - Important: we avoid performing any synchronous state mutation during render.
+   *   The getters return the precomputed values and this effect performs the side
+   *   effect of persisting them after mount. This pattern prevents the race that
+   *   previously caused initial queries to use the default "InStock" filter even
+   *   when the URL requested e.g. "Donated".
+   */
   useEffect(() => {
     if (isInitialMount.current && syncFiltersAndUrlParams) {
       const hasUrlParams = URL_FILTER_CONFIG.some(({ urlParam }) => searchParams.get(urlParam));
 
-      if (!hasUrlParams) {
-        const currentConfig = tableConfigsState.get(tableConfigKey);
-        if (currentConfig?.columnFilters) {
-          updateUrl(currentConfig.columnFilters);
-        }
-      } else {
-        // Create initial column filters, prioritizing URL parameters
-        const initialColumnFilters = createInitialColumnFilters(
-          defaultTableConfig.columnFilters,
-          urlFilters,
-        );
-
+      const existingConfig = tableConfigsState.get(tableConfigKey);
+      if (!existingConfig) {
+        const initialFiltersToPersist = hasUrlParams
+          ? initialColumnFilters
+          : defaultTableConfig.columnFilters;
         const tableConfig: ITableConfig = {
           globalFilter: defaultTableConfig.globalFilter,
-          columnFilters: initialColumnFilters,
+          columnFilters: initialFiltersToPersist,
           sortBy: defaultTableConfig.sortBy,
           hiddenColumns: defaultTableConfig.hiddenColumns,
         };
         tableConfigsState.set(tableConfigKey, tableConfig);
         tableConfigsVar(tableConfigsState);
+      } else if (!hasUrlParams) {
+        // If URL is empty, write the default filters into the URL
+        updateUrl(existingConfig.columnFilters);
       }
 
       isInitialMount.current = false;
@@ -181,24 +207,29 @@ export const useTableConfig = ({
     updateUrl,
     defaultTableConfig,
     urlFilters,
+    initialColumnFilters,
   ]);
 
   // Note: URL sync happens via setColumnFilters when filters change through UI
 
   function getGlobalFilter() {
-    return (tableConfigsState.get(tableConfigKey) || defaultTableConfig).globalFilter;
+    const cfg = tableConfigsState.get(tableConfigKey);
+    return cfg?.globalFilter ?? defaultTableConfig.globalFilter;
   }
 
   function getColumnFilters() {
-    return (tableConfigsState.get(tableConfigKey) || defaultTableConfig).columnFilters;
+    const cfg = tableConfigsState.get(tableConfigKey);
+    return cfg?.columnFilters ?? initialColumnFilters;
   }
 
   function getSortBy() {
-    return (tableConfigsState.get(tableConfigKey) || defaultTableConfig).sortBy;
+    const cfg = tableConfigsState.get(tableConfigKey);
+    return cfg?.sortBy ?? defaultTableConfig.sortBy;
   }
 
   function getHiddenColumns() {
-    return (tableConfigsState.get(tableConfigKey) || defaultTableConfig).hiddenColumns;
+    const cfg = tableConfigsState.get(tableConfigKey);
+    return cfg?.hiddenColumns ?? defaultTableConfig.hiddenColumns;
   }
 
   function setGlobalFilter(globalFilter: string | undefined) {
diff --git a/front/src/views/Boxes/BoxesView.test.tsx b/front/src/views/Boxes/BoxesView.test.tsx
@@ -6,7 +6,7 @@ import { ErrorBoundary } from "@sentry/react";
 import { AlertWithoutAction } from "components/Alerts";
 import { TableSkeleton } from "components/Skeletons";
 import { Suspense } from "react";
-import { cache } from "queries/cache";
+import { cache, tableConfigsVar } from "queries/cache";
 import Boxes, { ACTION_OPTIONS_FOR_BOXESVIEW_QUERY, BOXES_FOR_BOXESVIEW_QUERY } from "./BoxesView";
 import { FakeGraphQLError, FakeGraphQLNetworkError } from "mocks/functions";
 import {
@@ -26,6 +26,8 @@ const mockedUseAuth0 = vi.mocked(useAuth0);
 
 beforeEach(() => {
   mockAuthenticatedUser(mockedUseAuth0, "dev_volunteer@boxcare.org");
+  void cache.reset();
+  tableConfigsVar(new Map());
 });
 
 const jotaiAtoms = [
@@ -47,7 +49,7 @@ const boxesQuery = ({
     query: BOXES_FOR_BOXESVIEW_QUERY,
     variables: {
       baseId: "2",
-      filterInput: { states: state2 ? [state, state2] : [state] },
+      filterInput: state === "ALL" ? {} : { states: state2 ? [state, state2] : [state] },
       paginationInput,
     },
   },
@@ -537,7 +539,7 @@ describe("4.8.1 - Initial load of Page", () => {
       </ErrorBoundary>,
       {
         routePath: "/bases/:baseId/boxes",
-        initialUrl: "/bases/2/boxes",
+        initialUrl: "/bases/2/boxes?state_ids=1",
         mocks: [
           boxesQuery({ state: "Scrap", paginationInput: 20 }),
           boxesQuery({ state: "Donated", paginationInput: 20 }),
@@ -593,7 +595,7 @@ describe("4.8.1 - Initial load of Page", () => {
           </ErrorBoundary>,
           {
             routePath: "/bases/:baseId/boxes",
-            initialUrl: "/bases/2/boxes",
+            initialUrl: "/bases/2/boxes?state_ids=1",
             mocks,
             cache,
             addTypename: true,
@@ -626,7 +628,7 @@ describe("4.8.1 - Initial load of Page", () => {
       </ErrorBoundary>,
       {
         routePath: "/bases/:baseId/boxes",
-        initialUrl: "/bases/2/boxes",
+        initialUrl: "/bases/2/boxes?state_ids=1",
         mocks: [
           boxesQuery({ state: "Scrap", paginationInput: 20 }),
           boxesQuery({ state: "Donated", paginationInput: 20 }),
@@ -660,7 +662,7 @@ describe("4.8.2 - Selecting rows and performing bulk actions", () => {
       </ErrorBoundary>,
       {
         routePath: "/bases/:baseId/boxes",
-        initialUrl: "/bases/2/boxes",
+        initialUrl: "/bases/2/boxes?state_ids=1",
         mocks: [
           boxesQuery({ state: "Scrap", paginationInput: 20 }),
           boxesQuery({ state: "Donated", paginationInput: 20 }),
@@ -760,7 +762,7 @@ describe("4.8.2 - Selecting rows and performing bulk actions", () => {
       </ErrorBoundary>,
       {
         routePath: "/bases/:baseId/boxes",
-        initialUrl: "/bases/2/boxes",
+        initialUrl: "/bases/2/boxes?state_ids=1",
         mocks: [
           boxesQuery({ state: "Scrap", paginationInput: 20 }),
           boxesQuery({ state: "Donated", paginationInput: 20 }),
@@ -873,10 +875,11 @@ describe("4.8.3 - URL Parameter Sync for Filters", () => {
         routePath: "/bases/:baseId/boxes",
         initialUrl: "/bases/2/boxes?state_ids=999", // Invalid state ID
         mocks: [
+          boxesQuery({ state: "ALL", paginationInput: 20 }),
+          boxesQuery({ state: "ALL" }),
           boxesQuery({ state: "Scrap", paginationInput: 20 }),
           boxesQuery({ state: "Donated", paginationInput: 20 }),
           boxesQuery({ paginationInput: 20 }),
-          boxesQuery({}),
           actionsQuery,
         ],
         cache,
@@ -947,7 +950,7 @@ describe("4.8.3 - URL Parameter Sync for Filters", () => {
         mocks: [
           boxesQuery({ state: "Scrap", paginationInput: 20 }),
           boxesQuery({ state: "Donated", paginationInput: 20 }),
-          boxesQuery({ paginationInput: 20 }),
+          boxesQuery({ state: "InStock", state2: "Donated", paginationInput: 20 }),
           boxesQuery({ state: "InStock", state2: "Donated" }),
           actionsQuery,
         ],
diff --git a/front/src/views/Boxes/BoxesView.tsx b/front/src/views/Boxes/BoxesView.tsx
@@ -43,6 +43,7 @@ import { selectedBaseIdAtom } from "stores/globalPreferenceStore";
 import { DateCell, ProductWithSPCheckmarkCell } from "components/Table/Cells";
 import { BoxState } from "queries/types";
 import BoxesTable from "./components/BoxesTable";
+import { boxStateIds } from "utils/constants"; // added import to map state names -> ids
 
 // TODO: Implement Pagination and Filtering
 export const BOXES_QUERY_ELEMENT_FIELD_FRAGMENT = graphql(
@@ -160,7 +161,7 @@ function Boxes({
   const tableConfig = useTableConfig({
     tableConfigKey,
     defaultTableConfig: {
-      columnFilters: [{ id: "state", value: ["1"] }], // for InStock (see boxStateIds)
+      columnFilters: [],
       sortBy: [{ id: "lastModified", desc: true }],
       hiddenColumns: [
         "qrLabel",
@@ -199,10 +200,24 @@ function Boxes({
       return;
     }
 
-    // Only on very initial mount, query 20 boxes of the most used other than InStock state to
-    // preload the data into Apollo cache.
-    const states = ["Donated", "Scrap"] satisfies Partial<BoxState>[];
+    // Only on very initial mount, query 20 boxes of the most used states to preload the data into
+    // Apollo cache.
+    // But skip preloading a state if the current table config already requests it via filters.
+    // e.g. if tableConfig.getColumnFilters() already contains the id for "Donated" (boxStateIds.Donated),
+    // do not query Donated here.
+    const states = ["InStock", "Donated", "Scrap"] satisfies Partial<BoxState>[];
+
+    // Read the current state filter values (these are state IDs like "5", "6" etc.)
+    const stateFilterValues: string[] =
+      (tableConfig.getColumnFilters().find((f) => f.id === "state")?.value as string[]) ?? [];
+
     for (const state of states) {
+      const stateId = boxStateIds[state];
+      // If the table is already filtered to this state ID, skip preloading it.
+      if (stateId && stateFilterValues.includes(stateId)) {
+        continue;
+      }
+
       apolloClient.query({
         query: BOXES_FOR_BOXESVIEW_QUERY,
         variables: {
diff --git a/front/src/views/Boxes/BoxesViewActions.test.tsx b/front/src/views/Boxes/BoxesViewActions.test.tsx
@@ -172,6 +172,7 @@ const mockedUseAuth0 = vi.mocked(useAuth0);
 
 beforeEach(() => {
   mockAuthenticatedUser(mockedUseAuth0, "dev_volunteer@boxaid.org");
+  void cache.reset();
   tableConfigsVar(new Map());
 });
 
@@ -410,6 +411,7 @@ const boxesViewActionsTests = [
       boxesQuery({}),
       boxesQuery({ state: "Donated", stateFilter: ["Donated"] }),
       boxesQuery({ state: "Scrap", stateFilter: ["Scrap"] }),
+      boxesQuery({ state: "MarkedForShipment", stateFilter: ["MarkedForShipment"] }),
       boxesQuery({
         state: "MarkedForShipment",
         stateFilter: ["MarkedForShipment"],
@@ -497,6 +499,7 @@ const boxesViewActionsTests = [
       boxesQuery({}),
       boxesQuery({ state: "Donated", stateFilter: ["Donated"] }),
       boxesQuery({ state: "Scrap", stateFilter: ["Scrap"] }),
+      boxesQuery({ state: "MarkedForShipment", stateFilter: ["MarkedForShipment"] }),
       boxesQuery({
         state: "MarkedForShipment",
         shipmentDetail: shipmentDetail1(),
@@ -522,6 +525,7 @@ const boxesViewActionsTests = [
       boxesQuery({}),
       boxesQuery({ state: "Donated", stateFilter: ["Donated"] }),
       boxesQuery({ state: "Scrap", stateFilter: ["Scrap"] }),
+      boxesQuery({ state: "MarkedForShipment", stateFilter: ["MarkedForShipment"] }),
       boxesQuery({
         state: "MarkedForShipment",
         shipmentDetail: shipmentDetail1(),
@@ -545,6 +549,7 @@ const boxesViewActionsTests = [
       boxesQuery({}),
       boxesQuery({ state: "Donated", stateFilter: ["Donated"] }),
       boxesQuery({ state: "Scrap", stateFilter: ["Scrap"] }),
+      boxesQuery({ state: "MarkedForShipment", stateFilter: ["MarkedForShipment"] }),
       boxesQuery({
         state: "MarkedForShipment",
         shipmentDetail: shipmentDetail1(),
diff --git a/front/src/views/QrReader/QrReaderMultiBoxAssignTags.test.tsx b/front/src/views/QrReader/QrReaderMultiBoxAssignTags.test.tsx
@@ -58,7 +58,7 @@ const mockTagsQuery = ({
           ? null
           : {
               shipments: [generateMockShipmentMinimal({ state: "Preparing", iAmSource: true })],
-              base: { locations, tags: tagsArray },
+              base: { locations, id: "1", tags: tagsArray },
             },
         errors: graphQlError ? [new FakeGraphQLError()] : undefined,
       },
