Refactor useUrlValueProvider (#6269)

Author: witoszekdev

src/components/ConditionalFilter/ValueProvider/useUrlValueProvider.ts

@@ -1,43 +1,45 @@
-/* eslint-disable react-hooks/exhaustive-deps */
 import { stringify } from "qs";
-import { useEffect, useMemo, useState } from "react";
+import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import useRouter from "use-react-router";
 
-import { InitialAttributesAPIState } from "../API/initialState/attributes/useInitialAttributesState";
-import { InitialCollectionAPIState } from "../API/initialState/collections/useInitialCollectionsState";
-import { InitialGiftCardsAPIState } from "../API/initialState/giftCards/useInitialGiftCardsState";
-import { InitialOrderAPIState } from "../API/initialState/orders/useInitialOrderState";
-import { InitialPageAPIState } from "../API/initialState/page/useInitialPageState";
-import { InitialProductAPIState } from "../API/initialState/product/useProductInitialAPIState";
-import { InitialProductTypesAPIState } from "../API/initialState/productTypes/useInitialProdutTypesState";
-import { InitialStaffMembersAPIState } from "../API/initialState/staffMembers/useInitialStaffMemebersState";
-import { InitialVoucherAPIState } from "../API/initialState/vouchers/useInitialVouchersState";
 import { FilterContainer, FilterElement } from "../FilterElement";
 import { FilterValueProvider } from "../FilterValueProvider";
 import { FilterProviderType, InitialAPIState } from "../types";
 import { TokenArray } from "./TokenArray";
-import {
-  AttributesFetchingParams,
-  CollectionFetchingParams,
-  FetchingParams,
-  getEmptyFetchingPrams,
-  GiftCardsFetchingParams,
-  OrderFetchingParams,
-  PageFetchingParams,
-  ProductTypesFetchingParams,
-  StaffMembersFetchingParams,
-  VoucherFetchingParams,
-} from "./TokenArray/fetchingParams";
+import { getEmptyFetchingPrams } from "./TokenArray/fetchingParams";
 import { prepareStructure } from "./utils";
 
+/*
+ * Race condition fix for URL ↔ React state synchronization.
+ *
+ * Problem: Filter value has two sources:
+ * 1. Rehydration: computed from URL params + fetched GraphQL data
+ * 2. User action: when user selects a filter, we update URL and set value directly
+ *
+ * When user selects a filter:
+ * - persist() calls router.history.replace() which changes the URL
+ * - URL change causes tokenizedUrl/rehydratedValue to recompute
+ * - But GraphQL data is stale (from before the selection)
+ * - So rehydratedValue overwrites user's selection with old data
+ *
+ * Solution: UserOverride stores user's selection + URL snapshot.
+ * When computing final value, we check if URL still matches the snapshot.
+ * If yes → user just set this, use their value.
+ * If no → URL changed externally, use rehydrated value.
+ */
+interface UserOverride {
+  urlSnapshot: string;
+  value: FilterContainer;
+}
+
 export const useUrlValueProvider = (
   locationSearch: string,
   type: FilterProviderType,
   initialState?: InitialAPIState,
 ): FilterValueProvider => {
   const router = useRouter();
   const params = new URLSearchParams(locationSearch);
-  const [value, setValue] = useState<FilterContainer>([]);
+  const [userOverride, setUserOverride] = useState<UserOverride | null>(null);
   const activeTab = params.get("activeTab");
   const query = params.get("query");
   const before = params.get("before");
@@ -50,115 +52,119 @@ export const useUrlValueProvider = (
   params.delete("before");
   params.delete("after");
 
-  const tokenizedUrl = useMemo(() => new TokenArray(params.toString()), [params.toString()]);
+  const filterQueryString = params.toString();
+  const tokenizedUrl = useMemo(() => new TokenArray(filterQueryString), [filterQueryString]);
   const paramsFromType = getEmptyFetchingPrams(type);
   const fetchingParams = paramsFromType
     ? tokenizedUrl.getFetchingParams(paramsFromType, type)
     : null;
 
+  const data = initialState?.data;
+  const loading = initialState?.loading ?? false;
+
+  // Store fetchQueries in a ref to avoid triggering useEffect when initialState changes
+  // but the function reference is the same (common with hooks returning stable callbacks).
+  // Type assertion needed because InitialAPIState is a union type where each variant
+  // has fetchQueries accepting different params. The correlation between `type` and
+  // `initialState` ensures the correct params are passed, but TS can't narrow the union.
+  const fetchQueriesRef = useRef<((params: typeof fetchingParams) => Promise<void>) | null>(null);
+
   useEffect(() => {
-    if (initialState) {
-      switch (type) {
-        case "product":
-          (initialState as InitialProductAPIState).fetchQueries(fetchingParams as FetchingParams);
-          break;
-        case "order":
-          (initialState as InitialOrderAPIState).fetchQueries(
-            fetchingParams as OrderFetchingParams,
-          );
-          break;
-        case "voucher":
-          (initialState as InitialVoucherAPIState).fetchQueries(
-            fetchingParams as VoucherFetchingParams,
-          );
-          break;
-        case "page":
-          (initialState as InitialPageAPIState).fetchQueries(fetchingParams as PageFetchingParams);
-          break;
-        case "gift-cards":
-          (initialState as InitialGiftCardsAPIState).fetchQueries(
-            fetchingParams as GiftCardsFetchingParams,
-          );
-          break;
-        case "collection":
-          (initialState as InitialCollectionAPIState).fetchQueries(
-            fetchingParams as CollectionFetchingParams,
-          );
-          break;
-        case "product-types":
-          (initialState as InitialProductTypesAPIState).fetchQueries(
-            fetchingParams as ProductTypesFetchingParams,
-          );
-          break;
-        case "staff-members":
-          (initialState as InitialStaffMembersAPIState).fetchQueries(
-            fetchingParams as StaffMembersFetchingParams,
-          );
-          break;
-        case "attributes":
-          (initialState as InitialAttributesAPIState).fetchQueries(
-            fetchingParams as AttributesFetchingParams,
-          );
-          break;
-      }
-    }
-  }, [locationSearch]);
+    fetchQueriesRef.current = initialState?.fetchQueries as
+      | ((params: typeof fetchingParams) => Promise<void>)
+      | null;
+  }, [initialState?.fetchQueries]);
 
   useEffect(() => {
-    if (!initialState) return;
+    if (!fetchQueriesRef.current || !fetchingParams) return;
 
-    const { data, loading } = initialState;
+    fetchQueriesRef.current(fetchingParams);
+  }, [fetchingParams]);
 
-    if (loading) return;
+  const rehydratedValue = useMemo((): FilterContainer => {
+    if (initialState) {
+      if (loading || !data) {
+        return [];
+      }
 
-    setValue(tokenizedUrl.asFilterValuesFromResponse(data));
-    // Only run after fetching the initial data; otherwise, a race condition may occur.
-  }, [initialState?.data, initialState?.loading]);
+      return tokenizedUrl.asFilterValuesFromResponse(data);
+    }
 
-  useEffect(() => {
-    if (initialState) return;
+    return tokenizedUrl.asFilterValueFromEmpty();
+  }, [initialState, loading, data, tokenizedUrl]);
 
-    setValue(tokenizedUrl.asFilterValueFromEmpty());
-  }, [locationSearch, tokenizedUrl, initialState]);
+  const value = useMemo((): FilterContainer => {
+    if (userOverride && userOverride.urlSnapshot === filterQueryString) {
+      return userOverride.value;
+    }
 
-  const persist = (filterValue: FilterContainer) => {
-    router.history.replace({
-      pathname: router.location.pathname,
-      search: stringify({
-        ...prepareStructure(filterValue),
-        ...{ activeTab: activeTab || undefined },
-        ...{ query: query || undefined },
-        ...{ before: before || undefined },
-        ...{ after: after || undefined },
-      }),
-    });
-    setValue(filterValue);
-  };
+    return rehydratedValue;
+  }, [userOverride, filterQueryString, rehydratedValue]);
 
-  const clear = () => {
+  const persist = useCallback(
+    (filterValue: FilterContainer) => {
+      const newParams = {
+        ...prepareStructure(filterValue),
+        ...(activeTab ? { activeTab } : {}),
+        ...(query ? { query } : {}),
+        ...(before ? { before } : {}),
+        ...(after ? { after } : {}),
+      };
+
+      router.history.replace({
+        pathname: router.location.pathname,
+        search: stringify(newParams),
+      });
+
+      const filterStructureParams = prepareStructure(filterValue);
+      const newUrlSnapshot = Object.entries(filterStructureParams)
+        .map(([k, v]) => `${k}=${encodeURIComponent(String(v))}`)
+        .join("&");
+
+      setUserOverride({
+        urlSnapshot: newUrlSnapshot,
+        value: filterValue,
+      });
+    },
+    [router, activeTab, query, before, after],
+  );
+
+  const clear = useCallback(() => {
     router.history.replace({
       pathname: router.location.pathname,
     });
-    setValue([]);
-  };
-
-  const isPersisted = (element: FilterElement) => {
-    return value.some(p => FilterElement.isFilterElement(p) && p.equals(element));
-  };
-
-  const getTokenByName = (name: string) => {
-    return tokenizedUrl.asFlatArray().find(token => token.name === name);
-  };
-
-  const count = value.filter(v => typeof v !== "string").length;
-
-  return {
-    value,
-    loading: initialState?.loading || false,
-    persist,
-    clear,
-    isPersisted,
-    getTokenByName,
-    count,
-  };
+    setUserOverride({
+      urlSnapshot: "",
+      value: [],
+    });
+  }, [router]);
+
+  const isPersisted = useCallback(
+    (element: FilterElement) => {
+      return value.some(p => FilterElement.isFilterElement(p) && p.equals(element));
+    },
+    [value],
+  );
+
+  const getTokenByName = useCallback(
+    (name: string) => {
+      return tokenizedUrl.asFlatArray().find(token => token.name === name);
+    },
+    [tokenizedUrl],
+  );
+
+  const count = useMemo(() => value.filter(v => typeof v !== "string").length, [value]);
+
+  return useMemo(
+    () => ({
+      value,
+      loading: initialState?.loading || false,
+      persist,
+      clear,
+      isPersisted,
+      getTokenByName,
+      count,
+    }),
+    [value, initialState?.loading, persist, clear, isPersisted, getTokenByName, count],
+  );
 };