refactor: allow Type or Type[] values in useStateUrlSearchParams

using typescript function overloading. Multiple values in the search
params are provided by UrlSearchParams.getAll().

Also:
* removes the useListHelpers hook added in previous commits -- it is no
  longer needed.
* moves the useStateOrUrlSearchParam hook and TypesFilterData class to a
  separate search-manager/hooks.ts
* sanitizes tag search parameters using oel RESERVED_TAG_CHARS
* uses getBlockType to sanitize usage key search parameters

src/hooks.ts

@@ -86,40 +86,54 @@ export const useLoadOnScroll = (
 };
 
 /**
- * Types used by the useListHelpers and useStateWithUrlSearchParam hooks.
+ * Types used by the useStateWithUrlSearchParam hook.
  */
 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.
+ * This function is overloaded to accept simple Types or Array<Type> values.
  *
- * @param defaultValue: Type
+ * @param defaultValue: Type | Type[]
  *   Returned when no valid value is found in the url search parameter.
+ *   If an Array Type is used, then an Array Type of state values will be maintained.
  * @param paramName: name of the url search parameter to store this value in.
  * @param fromString: returns the Type equivalent of the given string value,
  *   or undefined if the value is invalid.
  * @param toString: returns the string equivalent of the given Type value.
  *   Return defaultValue to clear the url search paramName.
  */
+export function useStateWithUrlSearchParam<Type>(
+  defaultValue: Type[],
+  paramName: string,
+  fromString: FromStringFn<Type>,
+  toString: ToStringFn<Type>,
+): [value: Type[], setter: Dispatch<SetStateAction<Type[]>>];
 export function useStateWithUrlSearchParam<Type>(
   defaultValue: Type,
   paramName: string,
   fromString: FromStringFn<Type>,
   toString: ToStringFn<Type>,
-): [value: Type, setter: Dispatch<SetStateAction<Type>>] {
+): [value: Type, setter: Dispatch<SetStateAction<Type>>];
+export function useStateWithUrlSearchParam<Type>(
+  defaultValue: Type | Type[],
+  paramName: string,
+  fromString: FromStringFn<Type>,
+  toString: ToStringFn<Type>,
+): [value: Type | Type[], setter: Dispatch<SetStateAction<Type | 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
+  // for sequential calls to returnSetter in the same render loop (as we do 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
+  // fine with BrowserRouter, however 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()
@@ -128,25 +142,41 @@ export function useStateWithUrlSearchParam<Type>(
   const location = useLocation();
   const locationRef = useRef(location);
   const [searchParams, setSearchParams] = useSearchParams();
+  const paramValues = searchParams.getAll(paramName);
+
+  const returnValue: Type | Type[] = (
+    defaultValue instanceof Array
+      ? (paramValues.map(fromString).filter((v) => v !== undefined)) as Type[]
+      : fromString(paramValues[0])
+  ) ?? defaultValue;
 
-  const returnValue: Type = fromString(searchParams.get(paramName)) ?? defaultValue;
   // Update the url search parameter using:
   type ReturnSetterParams = (
     // a Type value
-    value?: Type
+    value?: Type | Type[]
     // or a function that returns a Type from the previous returnValue
-    | ((value: Type) => Type)
+    | ((value: Type | Type[]) => Type | Type[])
   ) => void;
-  const returnSetter: Dispatch<SetStateAction<Type>> = useCallback<ReturnSetterParams>((value) => {
-    setSearchParams((/* prev */) => {
+  const returnSetter: Dispatch<SetStateAction<Type | Type[]>> = useCallback<ReturnSetterParams>((value) => {
+    setSearchParams((/* previous -- see STATE WORKAROUND above */) => {
       const useValue = value instanceof Function ? value(returnValue) : value;
-      const paramValue = toString(useValue);
+      const paramValue: string | string[] | undefined = (
+        useValue instanceof Array
+          ? useValue.map(toString).filter((v) => v !== undefined) as string[]
+          : 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) {
+        // If the provided value was invalid (toString returned undefined) or
+        // the same as the defaultValue, remove it from the search params.
         newSearchParams.delete(paramName);
+      } else if (paramValue instanceof Array) {
+        // Replace paramName with the new list of values.
+        newSearchParams.delete(paramName);
+        paramValue.forEach((v) => v && newSearchParams.append(paramName, v));
       } else {
+        // Otherwise, just set the new (single) value.
         newSearchParams.set(paramName, paramValue);
       }
 
@@ -160,48 +190,3 @@ export function useStateWithUrlSearchParam<Type>(
   // 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

@@ -11,115 +11,9 @@ import { union } from 'lodash';
 import {
   CollectionHit, ContentHit, SearchSortOption, forceArray,
 } from './data/api';
+import { TypesFilterData, useStateOrUrlSearchParam } from './hooks';
 import { useContentSearchConnection, useContentSearchResults } from './data/apiHooks';
-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;
-}
-
-// Block / Problem type helper class
-export class TypesFilterData {
-  #blocks = new Set<string>();
-
-  #problems = new Set<string>();
-
-  #typeToList: FromStringFn<string[]>;
-
-  #listToType: ToStringFn<string[]>;
-
-  // Constructs a TypesFilterData from a string as generated from toString().
-  constructor(
-    value: string | null,
-    typeToList: FromStringFn<string[]>,
-    listToType: ToStringFn<string[]>,
-  ) {
-    this.#typeToList = typeToList;
-    this.#listToType = listToType;
-
-    const [blocks, problems] = (value || '').split('|');
-    this.union({ blocks, problems });
-  }
-
-  // Serialize the TypesFilterData to a string, or undefined if isEmpty().
-  toString(): string | undefined {
-    if (this.isEmpty()) {
-      return undefined;
-    }
-    return [
-      this.#listToType([...this.blocks]),
-      this.#listToType([...this.problems]),
-    ].join('|');
-  }
-
-  // Returns true if there are no block or problem types.
-  isEmpty(): boolean {
-    return !(this.#blocks.size || this.#problems.size);
-  }
-
-  get blocks() : Set<string> {
-    return this.#blocks;
-  }
-
-  get problems(): Set<string> {
-    return this.#problems;
-  }
-
-  clear(): TypesFilterData {
-    this.#blocks.clear();
-    this.#problems.clear();
-    return this;
-  }
-
-  union({ blocks, problems }: {
-    blocks?: string[] | Set<string> | string | undefined,
-    problems?: string[] | Set<string> | string | undefined,
-  }): void {
-    let newBlocks: string[];
-    if (!blocks) {
-      newBlocks = [];
-    } else if (typeof blocks === 'string') {
-      newBlocks = this.#typeToList(blocks) || [];
-    } else {
-      newBlocks = [...blocks];
-    }
-    this.#blocks = new Set<string>([...this.#blocks, ...newBlocks]);
-
-    let newProblems: string[];
-    if (!problems) {
-      newProblems = [];
-    } else if (typeof problems === 'string') {
-      newProblems = this.#typeToList(problems) || [];
-    } else {
-      newProblems = [...problems];
-    }
-    this.#problems = new Set<string>([...this.#problems, ...newProblems]);
-  }
-}
+import { getBlockType } from '../generic/key-utils';
 
 export interface SearchContextData {
   client?: MeiliSearch;
@@ -174,60 +68,46 @@ export const SearchContextProvider: React.FC<{
   );
 
   // Block + problem types use alphanumeric plus a few other characters.
-  // E.g ?type=html,video|multiplechoiceresponse,choiceresponse
-  const sanitizeType = (value: string | null | undefined): string | undefined => (
-    (value && /^[a-z0-9._-]+$/.test(value))
-      ? value
-      : undefined
-  );
-  const [typeToList, listToType] = useListHelpers<string>({
-    toString: sanitizeType,
-    fromString: sanitizeType,
-    separator: ',',
-  });
-  const stringToTypesFilter = (value: string | null): TypesFilterData | undefined => (
-    new TypesFilterData(value, typeToList, listToType)
-  );
-  const typesFilterToString = (value: TypesFilterData | undefined): string | undefined => (
-    value ? value.toString() : undefined
-  );
+  // E.g ?type=html&type=video&type=p.multiplechoiceresponse
   const [typesFilter, setTypesFilter] = useStateOrUrlSearchParam<TypesFilterData>(
-    new TypesFilterData('', typeToList, listToType),
-    'types',
-    stringToTypesFilter,
-    typesFilterToString,
-    // Returns e.g 'problem,text|multiplechoice,checkbox'
+    new TypesFilterData(),
+    'type',
+    (value: string | null) => new TypesFilterData(value),
+    (value: TypesFilterData | undefined) => (value ? value.toString() : undefined),
     skipUrlUpdate,
   );
 
-  // Tags can be almost any string value, except our separator (|)
-  // TODO how to sanitize tags?
-  // E.g ?tags=Skills+>+Abilities|Skills+>+Knowledge
+  // Tags can be almost any string value (see openedx-learning's RESERVED_TAG_CHARS)
+  // and multiple tags may be selected together.
+  // E.g ?tag=Skills+>+Abilities&tag=Skills+>+Knowledge
   const sanitizeTag = (value: string | null | undefined): string | undefined => (
-    (value && /^[^|]+$/.test(value))
-      ? value
-      : undefined
+    (value && /^[^\t;]+$/.test(value)) ? value : undefined
   );
-  const [tagToList, listToTag] = useListHelpers<string>({
-    toString: sanitizeTag,
-    fromString: sanitizeTag,
-    separator: '|',
-  });
-  const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam<string[]>(
+  const [tagsFilter, setTagsFilter] = useStateOrUrlSearchParam<string>(
     [],
-    'tags',
-    tagToList,
-    listToTag,
+    'tag',
+    sanitizeTag,
+    sanitizeTag,
     skipUrlUpdate,
   );
 
   // E.g ?usageKey=lb:OpenCraft:libA:problem:5714eb65-7c36-4eee-8ab9-a54ed5a95849
+  const sanitizeUsageKey = (value: string): string | undefined => {
+    try {
+      if (getBlockType(value)) {
+        return value;
+      }
+    } catch (error) {
+      // Error thrown if value cannot be parsed into a library usage key.
+      // Pass through to return below.
+    }
+    return undefined;
+  };
   const [usageKey, setUsageKey] = useStateOrUrlSearchParam<string>(
     '',
     'usageKey',
-    // TODO should sanitize usageKeys too.
-    (value: string) => value,
-    (value: string) => value,
+    sanitizeUsageKey,
+    sanitizeUsageKey,
     skipUrlUpdate,
   );