feat: store selected tags in the URL search string

Related features:

1. We can filter on more than one tag, so the search field needs to
   store an Array of values which still need to be sanitised. This
   change adds useListHelpers to assist with the parsing and validating
   of an Array of Typed values.
2. SearchManager takes a skipUrlUpdate property which should disable
   using search params for state variables. Our "sort" parameters
   respected this, but none of the other search parameters did. So this
   change also adds a wrapper hook called useStateOrUrlSearchParam that
   handles this switch cleanly.

This feature also revealed two bugs fixed in useStateWithUrlSearchParam:

1. When the returnSetter is called with a function instead of a simple
   value, we need to pass in previous returnValue to the function so it
   can generate the new value.

2. When the returnSetter is called multiple times by a single callback
   (like with clearFilters), the latest changes to the UrlSearchParams
   weren't showing up.

   To fix this, we had to use the location.search string as the "latest"
   previous url search, not the prevParams passed into setSearchParams,
   because these params may not have the latest updates.

Author: pomegranited

src/hooks.ts

@@ -3,6 +3,7 @@ import {
   type SetStateAction,
   useCallback,
   useEffect,
+  useRef,
   useState,
 } from 'react';
 import { history } from '@edx/frontend-platform';
@@ -85,10 +86,13 @@ export const useLoadOnScroll = (
 };
 
 /**
- * Hook which stores state variables in the URL search parameters.
- *
- * It wraps useState with functions that get/set a query string
- * search parameter when returning/setting the state variable.
+ * Types used by the useListHelpers and useStateWithUrlSearchParam hooks.
+ */
+export type FromStringFn<Type> = (value: string | null) => Type | undefined;
+export type ToStringFn<Type> = (value: Type | undefined) => string | undefined;
+
+/**
+ * Hook that stores/retrieves state variables using the URL search parameters.
  *
  * @param defaultValue: Type
  *   Returned when no valid value is found in the url search parameter.
@@ -101,26 +105,103 @@ export const useLoadOnScroll = (
 export function useStateWithUrlSearchParam<Type>(
   defaultValue: Type,
   paramName: string,
-  fromString: (value: string | null) => Type | undefined,
-  toString: (value: Type) => string | undefined,
+  fromString: FromStringFn<Type>,
+  toString: ToStringFn<Type>,
 ): [value: Type, setter: Dispatch<SetStateAction<Type>>] {
+  // STATE WORKAROUND:
+  // If we use this hook to control multiple state parameters on the same
+  // page, we can run into state update issues. Because our state variables
+  // are actually stored in setSearchParams, and not in separate variables like
+  // useState would do, the searchParams "previous" state may not be updated
+  // for sequential calls to returnSetter in the same render loop (like in
+  // SearchManager's clearFilters).
+  //
+  // One workaround could be to use window.location.search as the "previous"
+  // value when returnSetter constructs the new URLSearchParams. This works
+  // fine with BrowserRouter, but our test suite uses MemoryRouter, and that
+  // router doesn't store URL search params, cf
+  // https://github.com/remix-run/react-router/issues/9757
+  //
+  // So instead, we maintain a reference to the current useLocation()
+  // object, and use its search params as the "previous" value when
+  // initializing URLSearchParams.
+  const location = useLocation();
+  const locationRef = useRef(location);
   const [searchParams, setSearchParams] = useSearchParams();
+
   const returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue;
-  // Function to update the url search parameter
-  const returnSetter: Dispatch<SetStateAction<Type>> = useCallback((value: Type) => {
-    setSearchParams((prevParams) => {
-      const paramValue: string = toString(value) ?? '';
-      const newSearchParams = new URLSearchParams(prevParams);
-      // If using the default paramValue, remove it from the search params.
-      if (paramValue === defaultValue) {
+  // Update the url search parameter using:
+  type ReturnSetterParams = (
+    // a Type value
+    value?: Type
+    // or a function that returns a Type from the previous returnValue
+    | ((value: Type) => Type)
+  ) => void;
+  const returnSetter: Dispatch<SetStateAction<Type>> = useCallback<ReturnSetterParams>((value) => {
+    setSearchParams((/* prev */) => {
+      const useValue = value instanceof Function ? value(returnValue) : value;
+      const paramValue = toString(useValue);
+      const newSearchParams = new URLSearchParams(locationRef.current.search);
+      // If the provided value was invalid (toString returned undefined)
+      // or the same as the defaultValue, remove it from the search params.
+      if (paramValue === undefined || paramValue === defaultValue) {
         newSearchParams.delete(paramName);
       } else {
         newSearchParams.set(paramName, paramValue);
       }
+
+      // Update locationRef
+      locationRef.current.search = newSearchParams.toString();
+
       return newSearchParams;
     }, { replace: true });
-  }, [setSearchParams]);
+  }, [returnValue, setSearchParams]);
 
   // Return the computed value and wrapped set state function
   return [returnValue, returnSetter];
 }
+
+/**
+ * Helper hook for useStateWithUrlSearchParam<Type[]>.
+ *
+ * useListHelpers provides toString and fromString handlers that can:
+ * - split/join a list of values using a separator string, and
+ * - validate each value using the provided functions, omitting any invalid values.
+ *
+ * @param fromString
+ *   Serialize a string to a Type, or undefined if not valid.
+ * @param toString
+ *   Deserialize a Type to a string.
+ * @param separator : string to use when splitting/joining the types.
+ *   Defaults value is ','.
+ */
+export function useListHelpers<Type>({
+  fromString,
+  toString,
+  separator = ',',
+}: {
+  fromString: FromStringFn<Type>,
+  toString: ToStringFn<Type>,
+  separator?: string;
+}): [ FromStringFn<Type[]>, ToStringFn<Type[]> ] {
+  const isType = (item: Type | undefined): item is Type => item !== undefined;
+
+  // Split the given string with separator,
+  // and convert the parts to a list of Types, omiting any invalid Types.
+  const fromStringToList : FromStringFn<Type[]> = (value: string) => (
+    value
+      ? value.split(separator).map(fromString).filter(isType)
+      : []
+  );
+  // Convert an array of Types to strings and join with separator.
+  // Returns undefined if the given list contains no valid Types.
+  const fromListToString : ToStringFn<Type[]> = (value: Type[]) => {
+    const stringValues = value.map(toString).filter((val) => val !== undefined);
+    return (
+      stringValues && stringValues.length
+        ? stringValues.join(separator)
+        : undefined
+    );
+  };
+  return [fromStringToList, fromListToString];
+}

src/search-manager/SearchManager.ts

@@ -12,7 +12,35 @@ import {
   CollectionHit, ContentHit, SearchSortOption, forceArray,
 } from './data/api';
 import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks';
-import { useStateWithUrlSearchParam } from '../hooks';
+import {
+  type FromStringFn,
+  type ToStringFn,
+  useListHelpers,
+  useStateWithUrlSearchParam,
+} from '../hooks';
+
+/**
+ * Typed hook that returns useState if skipUrlUpdate,
+ * or useStateWithUrlSearchParam if it's not.
+ *
+ * Provided here to reduce some code overhead in SearchManager.
+ */
+function useStateOrUrlSearchParam<Type>(
+  defaultValue: Type,
+  paramName: string,
+  fromString: FromStringFn<Type>,
+  toString: ToStringFn<Type>,
+  skipUrlUpdate?: boolean,
+): [value: Type, setter: React.Dispatch<React.SetStateAction<Type>>] {
+  const useStateManager = React.useState<Type>(defaultValue);
+  const urlStateManager = useStateWithUrlSearchParam<Type>(
+    defaultValue,
+    paramName,
+    fromString,
+    toString,
+  );
+  return skipUrlUpdate ? useStateManager : urlStateManager;
+}
 
 export interface SearchContextData {
   client?: MeiliSearch;
@@ -58,51 +86,63 @@ export const SearchContextProvider: React.FC<{
   overrideSearchSortOrder, skipBlockTypeFetch, skipUrlUpdate, ...props
 }) => {
   // Search parameters can be set via the query string
-  // E.g. q=draft+text
-  // TODO -- how to scrub search terms?
-  const keywordStateManager = React.useState('');
-  const keywordUrlStateManager = useStateWithUrlSearchParam<string>(
+  // E.g. ?q=draft+text
+  // TODO -- how to sanitize search terms?
+  const [searchKeywords, setSearchKeywords] = useStateOrUrlSearchParam<string>(
     '',
     'q',
     (value: string) => value || '',
     (value: string) => value || '',
-  );
-  const [searchKeywords, setSearchKeywords] = (
-    skipUrlUpdate
-      ? keywordStateManager
-      : keywordUrlStateManager
+    skipUrlUpdate,
   );
 
   const [blockTypesFilter, setBlockTypesFilter] = React.useState<string[]>([]);
   const [problemTypesFilter, setProblemTypesFilter] = React.useState<string[]>([]);
-  const [tagsFilter, setTagsFilter] = React.useState<string[]>([]);
-  const [usageKey, setUsageKey] = useStateWithUrlSearchParam(
+  // Tags can be almost any string value, except our separator (|)
+  // TODO how to sanitize tags?
+  // E.g ?tags=Skills+>+Abilities|Skills+>+Knowledge
+  const sanitizeTag = (value: string | null | undefined): string | undefined => (
+    (value && /^[^|]+$/.test(value))
+      ? value
+      : undefined
+  );
+  const [tagToList, listToTag] = useListHelpers<string>({
+    toString: sanitizeTag,
+    fromString: sanitizeTag,
+    separator: '|',
+  });
+  const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam<string[]>(
+    [],
+    'tags',
+    tagToList,
+    listToTag,
+    skipUrlUpdate,
+  );
+
+  // E.g ?usageKey=lb:OpenCraft:libA:problem:5714eb65-7c36-4eee-8ab9-a54ed5a95849
+  const [usageKey, setUsageKey] = useStateOrUrlSearchParam<string>(
     '',
     'usageKey',
+    // TODO should sanitize usageKeys too.
     (value: string) => value,
     (value: string) => value,
+    skipUrlUpdate,
   );
 
   let extraFilter: string[] = forceArray(props.extraFilter);
   if (usageKey) {
     extraFilter = union(extraFilter, [`usage_key = "${usageKey}"`]);
   }
 
-  // The search sort order can be set via the query string
-  // E.g. ?sort=display_name:desc maps to SearchSortOption.TITLE_ZA.
   // Default sort by Most Relevant if there's search keyword(s), else by Recently Modified.
   const defaultSearchSortOrder = searchKeywords ? SearchSortOption.RELEVANCE : SearchSortOption.RECENTLY_MODIFIED;
-  let sortStateManager = React.useState<SearchSortOption>(defaultSearchSortOrder);
-  const sortUrlStateManager = useStateWithUrlSearchParam<SearchSortOption>(
+  const [searchSortOrder, setSearchSortOrder] = useStateOrUrlSearchParam<SearchSortOption>(
     defaultSearchSortOrder,
     'sort',
     (value: string) => Object.values(SearchSortOption).find((enumValue) => value === enumValue),
     (value: SearchSortOption) => value.toString(),
+    skipUrlUpdate,
   );
-  if (!skipUrlUpdate) {
-    sortStateManager = sortUrlStateManager;
-  }
-  const [searchSortOrder, setSearchSortOrder] = sortStateManager;
   // SearchSortOption.RELEVANCE is special, it means "no custom sorting", so we
   // send it to useContentSearchResults as an empty array.
   const searchSortOrderToUse = overrideSearchSortOrder ?? searchSortOrder;