refact: proper one-way data flow from URL -> Redux in search

src/js/components/Search/MakeQueryOption.tsx

@@ -1,33 +1,42 @@
-import React from 'react';
+import React, { useCallback } from 'react';
 import { Row, Col, Checkbox } from 'antd';
 
 import OptionDescription from './OptionDescription';
-import { addQueryParam, makeGetKatsuPublic, removeQueryParam } from '@/features/search/query.store';
 import SelectOption from './SelectOption';
 
-import { useAppDispatch, useAppSelector, useTranslationCustom, useTranslationDefault } from '@/hooks';
+import { useAppSelector, useTranslationCustom, useTranslationDefault } from '@/hooks';
 import type { Field } from '@/types/search';
+import { useLocation, useNavigate } from 'react-router-dom';
+import { buildQueryParamsUrl, queryParamsWithoutKey } from '@/utils/search';
 
 const MakeQueryOption = ({ queryField }: MakeQueryOptionProps) => {
   const t = useTranslationCustom();
   const td = useTranslationDefault();
-  const dispatch = useAppDispatch();
+
+  const { pathname } = useLocation();
+  const navigate = useNavigate();
 
   const { title, id, description, config, options } = queryField;
 
   const maxQueryParameters = useAppSelector((state) => state.config.maxQueryParameters);
   const { queryParamCount, queryParams } = useAppSelector((state) => state.query);
 
-  const isChecked = Object.prototype.hasOwnProperty.call(queryParams, id);
+  const isChecked = id in queryParams;
 
-  const onCheckToggle = () => {
+  const onCheckToggle = useCallback(() => {
+    let url: string;
     if (isChecked) {
-      dispatch(removeQueryParam(id));
+      // If currently checked, uncheck it
+      url = buildQueryParamsUrl(pathname, queryParamsWithoutKey(queryParams, id));
     } else {
-      dispatch(addQueryParam({ id, value: options[0] }));
+      // If currently unchecked, check it
+      url = buildQueryParamsUrl(pathname, { ...queryParams, [id]: options[0] });
     }
-    dispatch(makeGetKatsuPublic());
-  };
+
+    console.debug('[MakeQueryOption] Redirecting to:', url);
+    navigate(url, { replace: true });
+    // Don't need to dispatch - the code handling the URL change will dispatch the fetch for us instead.
+  }, [id, isChecked, navigate, options, pathname, queryParams]);
 
   // TODO: allow disabling max query parameters for authenticated and authorized users when Katsu has AuthZ
   // useQueryWithAuthIfAllowed()

src/js/components/Search/Search.tsx

@@ -24,11 +24,13 @@ const RoutedSearch = () => {
   const location = useLocation();
   const navigate = useNavigate();
 
-  const maxQueryParameters = useAppSelector((state) => state.config.maxQueryParameters);
+  const { maxQueryParameters } = useAppSelector((state) => state.config);
   const {
     querySections: searchSections,
     queryParams,
     isFetchingFields: isFetchingSearchFields,
+    isFetchingData: isFetchingSearchData,
+    attemptedFieldsFetch,
     attemptedFetch,
   } = useAppSelector((state) => state.query);
 
@@ -70,41 +72,40 @@ const RoutedSearch = () => {
 
   // Synchronize Redux query params state from URL
   useEffect(() => {
-    if (isFetchingSearchFields) return;
     if (!location.pathname.endsWith('/search')) return;
-    const queryParam = new URLSearchParams(location.search);
-    const { valid, validQueryParamsObject } = validateQuery(queryParam);
+
+    // Wait until we have search fields to try and build a valid query. Otherwise, we will mistakenly remove all URL
+    // query parameters and effectively reset the form.
+    if (!attemptedFieldsFetch || isFetchingSearchFields || isFetchingSearchData) return;
+
+    const { valid, validQueryParamsObject } = validateQuery(new URLSearchParams(location.search));
     if (valid) {
       if (!attemptedFetch || !checkQueryParamsEqual(validQueryParamsObject, queryParams)) {
         // Only update the state & refresh if we have a new set of query params from the URL.
+        // [!!!] This should be the only place setQueryParams(...) gets called. Everywhere else should use URL
+        //       manipulations, so that we have a one-way data flow from URL to state!
         dispatch(setQueryParams(validQueryParamsObject));
         dispatch(makeGetKatsuPublic());
       }
     } else {
       const url = buildQueryParamsUrl(location.pathname, validQueryParamsObject);
-      console.debug('Redirecting to:', url);
+      console.debug('[Search] Redirecting to:', url);
       navigate(url);
+      // Then, the new URL will re-trigger this effect but with only valid query parameters.
     }
   }, [
     attemptedFetch,
+    attemptedFieldsFetch,
     dispatch,
     isFetchingSearchFields,
+    isFetchingSearchData,
     location.search,
     location.pathname,
     navigate,
     queryParams,
     validateQuery,
   ]);
 
-  // Synchronize URL from Redux query params state
-  useEffect(() => {
-    if (!location.pathname.endsWith('/search')) return;
-    if (!attemptedFetch) return;
-    const url = buildQueryParamsUrl(location.pathname, queryParams);
-    console.debug('Redirecting to:', url);
-    navigate(url, { replace: true });
-  }, [attemptedFetch, location.pathname, navigate, queryParams]);
-
   return <Search />;
 };
 

src/js/components/Search/SelectOption.tsx

@@ -1,32 +1,43 @@
-/* eslint-disable @typescript-eslint/no-explicit-any */
-import React from 'react';
+import React, { type CSSProperties, useCallback, useMemo } from 'react';
 import { Select } from 'antd';
 
-import { addQueryParam, makeGetKatsuPublic } from '@/features/search/query.store';
-import { useAppDispatch, useAppSelector, useTranslationCustom } from '@/hooks';
+import { useAppSelector, useTranslationCustom } from '@/hooks';
+import { useLocation, useNavigate } from 'react-router-dom';
+import { buildQueryParamsUrl } from '@/utils/search';
+
+const SELECT_STYLE: CSSProperties = { width: '100%' };
 
 const SelectOption = ({ id, isChecked, options }: SelectOptionProps) => {
   const t = useTranslationCustom();
-  const dispatch = useAppDispatch();
 
-  const queryParams = useAppSelector((state) => state.query.queryParams);
+  const { pathname } = useLocation();
+  const navigate = useNavigate();
+
+  const { queryParams } = useAppSelector((state) => state.query);
   const defaultValue = queryParams[id] || options[0];
 
-  const handleValueChange = (newValue: string) => {
-    dispatch(addQueryParam({ id, value: newValue }));
-    dispatch(makeGetKatsuPublic());
-  };
+  const handleValueChange = useCallback(
+    (newValue: string) => {
+      const url = buildQueryParamsUrl(pathname, { ...queryParams, [id]: newValue });
+      console.debug('[SelectOption] Redirecting to:', url);
+      navigate(url, { replace: true });
+      // Don't need to dispatch - the code handling the URL change will dispatch the fetch for us instead.
+    },
+    [id, navigate, pathname, queryParams]
+  );
+
+  const selectOptions = useMemo(() => options.map((item) => ({ value: item, label: t(item) })), [options, t]);
 
   return (
     <Select
       id={id}
       disabled={!isChecked}
       showSearch
-      style={{ width: '100%' }}
+      style={SELECT_STYLE}
       onChange={handleValueChange}
       value={defaultValue}
       defaultValue={defaultValue}
-      options={options.map((item) => ({ value: item, label: t(item) }))}
+      options={selectOptions}
     />
   );
 };

src/js/features/search/query.store.ts

@@ -8,6 +8,7 @@ import type { ChartData } from '@/types/data';
 
 export type QueryState = {
   isFetchingFields: boolean;
+  attemptedFieldsFetch: boolean;
   isFetchingData: boolean;
   attemptedFetch: boolean;
   querySections: SearchFieldResponse['sections'];
@@ -23,6 +24,7 @@ export type QueryState = {
 
 const initialState: QueryState = {
   isFetchingFields: false,
+  attemptedFieldsFetch: false,
   isFetchingData: false,
   attemptedFetch: false,
   message: '',
@@ -40,16 +42,6 @@ const query = createSlice({
   name: 'query',
   initialState,
   reducers: {
-    addQueryParam: (state, { payload }) => {
-      if (!(payload.id in state.queryParams)) state.queryParamCount++;
-      state.queryParams[payload.id] = payload.value;
-    },
-    removeQueryParam: (state, { payload: id }) => {
-      if (id in state.queryParams) {
-        delete state.queryParams[id];
-        state.queryParamCount--;
-      }
-    },
     setQueryParams: (state, { payload }) => {
       state.queryParams = payload;
       state.queryParamCount = Object.keys(payload).length;
@@ -83,13 +75,15 @@ const query = createSlice({
     builder.addCase(makeGetSearchFields.fulfilled, (state, { payload }) => {
       state.querySections = payload.sections;
       state.isFetchingFields = false;
+      state.attemptedFieldsFetch = true;
     });
     builder.addCase(makeGetSearchFields.rejected, (state) => {
       state.isFetchingFields = false;
+      state.attemptedFieldsFetch = true;
     });
   },
 });
 
-export const { addQueryParam, removeQueryParam, setQueryParams } = query.actions;
+export const { setQueryParams } = query.actions;
 export { makeGetKatsuPublic, makeGetSearchFields };
 export default query.reducer;

src/js/utils/search.ts

@@ -1,4 +1,10 @@
 import type { QueryParams } from '@/types/search';
 
+export const queryParamsWithoutKey = (qp: QueryParams, key: string): QueryParams => {
+  const qpc = { ...qp };
+  delete qpc[key];
+  return qpc;
+};
+
 export const buildQueryParamsUrl = (pathName: string, qp: QueryParams): string =>
   `${pathName}?${new URLSearchParams(qp).toString()}`;