feat: make workspace search bar remember text (#9759)

Parkreiner · web-flow · commit b742661abde0 · 2023-09-20T16:44:19.000-04:00
* minor: Add useEffectEvent polyfill

* chore: update filter to have better callback support

* docs: Clean up comments

* fix: add localStorage to useWorkspacesFilter

* refactor: Centralize stable useSearchParams

* refactor: clean up filter to be fully pure on mount

* chore: add tests for useEffectEvent

* wip: commit progress for searchbar fix

* chore: clean up WorkspacesPage

* fix: add logic for syncing queries with search params

* chore: Rename initialValue to fallbackFilter

* chore: Remove todo comment

* refactor: update code to use useEffectEvent

* docs: clean up comments for clarity

* fix: update url check to use regex
diff --git a/site/e2e/global.setup.ts b/site/e2e/global.setup.ts
@@ -12,7 +12,6 @@ test("create first user", async ({ page }) => {
   await page.getByTestId("trial").click();
   await page.getByTestId("create").click();
 
-  await expect(page).toHaveURL("/workspaces");
-
+  await expect(page).toHaveURL(/\/workspaces.*/);
   await page.context().storageState({ path: STORAGE_STATE });
 });
diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx
@@ -24,7 +24,9 @@ import MenuList from "@mui/material/MenuList";
 import { Loader } from "components/Loader/Loader";
 import Divider from "@mui/material/Divider";
 import OpenInNewOutlined from "@mui/icons-material/OpenInNewOutlined";
+
 import { useDebouncedFunction } from "hooks/debounce";
+import { useEffectEvent } from "hooks/hookPolyfills";
 
 export type PresetFilter = {
   name: string;
@@ -33,45 +35,71 @@ export type PresetFilter = {
 
 type FilterValues = Record<string, string | undefined>;
 
+type UseFilterConfig = {
+  /**
+   * The fallback value to use in the event that no filter params can be parsed
+   * from the search params object. This value is allowed to change on
+   * re-renders.
+   */
+  fallbackFilter?: string;
+  searchParamsResult: ReturnType<typeof useSearchParams>;
+  onUpdate?: (newValue: string) => void;
+};
+
+const useFilterParamsKey = "filter";
+
 export const useFilter = ({
-  initialValue = "",
-  onUpdate,
+  fallbackFilter = "",
   searchParamsResult,
-}: {
-  initialValue?: string;
-  searchParamsResult: ReturnType<typeof useSearchParams>;
-  onUpdate?: () => void;
-}) => {
+  onUpdate,
+}: UseFilterConfig) => {
   const [searchParams, setSearchParams] = searchParamsResult;
-  const query = searchParams.get("filter") ?? initialValue;
-  const values = parseFilterQuery(query);
-
-  const update = (values: string | FilterValues) => {
-    if (typeof values === "string") {
-      searchParams.set("filter", values);
-    } else {
-      searchParams.set("filter", stringifyFilter(values));
-    }
+  const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter;
+
+  // Stabilizing reference to setSearchParams from one central spot, just to be
+  // on the extra careful side; don't want effects over-running. You would think
+  // this would be overkill, but setSearchParams isn't stable out of the box
+  const stableSetSearchParams = useEffectEvent(setSearchParams);
+
+  // Keep params synced with query, even as query changes from outside sources
+  useEffect(() => {
+    stableSetSearchParams((currentParams) => {
+      const currentQuery = currentParams.get(useFilterParamsKey);
+
+      if (query === "") {
+        currentParams.delete(useFilterParamsKey);
+      } else if (currentQuery !== query) {
+        currentParams.set(useFilterParamsKey, query);
+      }
+
+      return currentParams;
+    });
+  }, [stableSetSearchParams, query]);
+
+  const update = (newValues: string | FilterValues) => {
+    const serialized =
+      typeof newValues === "string" ? newValues : stringifyFilter(newValues);
+
+    searchParams.set(useFilterParamsKey, serialized);
     setSearchParams(searchParams);
-    if (onUpdate) {
-      onUpdate();
+
+    if (onUpdate !== undefined) {
+      onUpdate(serialized);
     }
   };
 
   const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction(
-    (values: string | FilterValues) => update(values),
+    update,
     500,
   );
 
-  const used = query !== "" && query !== initialValue;
-
   return {
     query,
     update,
     debounceUpdate,
     cancelDebounce,
-    values,
-    used,
+    values: parseFilterQuery(query),
+    used: query !== "" && query !== fallbackFilter,
   };
 };
 
diff --git a/site/src/hooks/hookPolyfills.test.ts b/site/src/hooks/hookPolyfills.test.ts
@@ -0,0 +1,48 @@
+import { renderHook } from "@testing-library/react";
+import { useEffectEvent } from "./hookPolyfills";
+
+function renderEffectEvent<TArgs extends unknown[], TReturn = unknown>(
+  callbackArg: (...args: TArgs) => TReturn,
+) {
+  return renderHook(
+    ({ callback }: { callback: typeof callbackArg }) => {
+      return useEffectEvent(callback);
+    },
+    {
+      initialProps: { callback: callbackArg },
+    },
+  );
+}
+
+describe(`${useEffectEvent.name}`, () => {
+  it("Should maintain a stable reference across all renders", () => {
+    const callback = jest.fn();
+    const { result, rerender } = renderEffectEvent(callback);
+
+    const firstResult = result.current;
+    for (let i = 0; i < 5; i++) {
+      rerender({ callback });
+    }
+
+    expect(result.current).toBe(firstResult);
+    expect.hasAssertions();
+  });
+
+  it("Should always call the most recent callback passed in", () => {
+    let value: "A" | "B" | "C" = "A";
+    const flipToB = () => {
+      value = "B";
+    };
+
+    const flipToC = () => {
+      value = "C";
+    };
+
+    const { result, rerender } = renderEffectEvent(flipToB);
+    rerender({ callback: flipToC });
+
+    result.current();
+    expect(value).toEqual("C");
+    expect.hasAssertions();
+  });
+});
diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts
@@ -0,0 +1,37 @@
+/**
+ * @file For defining DIY versions of official React hooks that have not been
+ * released yet.
+ *
+ * These hooks should be deleted as soon as the official versions are available.
+ * They do not have the same ESLinter exceptions baked in that the official
+ * hooks do, especially for dependency arrays.
+ */
+import { useCallback, useEffect, useRef } from "react";
+
+/**
+ * A DIY version of useEffectEvent.
+ *
+ * Works like useCallback, except that it doesn't take a dependency array, and
+ * always returns out a stable function on every single render. The returned-out
+ * function is always able to "see" the most up-to-date version of the callback
+ * passed in.
+ *
+ * Should only be used as a last resort when useCallback does not work, but you
+ * still need to avoid dependency array violations. (e.g., You need an on-mount
+ * effect, but an external library doesn't give their functions stable
+ * references, so useEffect/useMemo/useCallback run too often).
+ *
+ * @see {@link https://react.dev/reference/react/experimental_useEffectEvent}
+ */
+export function useEffectEvent<TArgs extends unknown[], TReturn = unknown>(
+  callback: (...args: TArgs) => TReturn,
+) {
+  const callbackRef = useRef(callback);
+  useEffect(() => {
+    callbackRef.current = callback;
+  }, [callback]);
+
+  return useCallback((...args: TArgs): TReturn => {
+    return callbackRef.current(...args);
+  }, []);
+}
diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx
@@ -4,7 +4,7 @@ import {
   useDashboard,
   useIsWorkspaceActionsEnabled,
 } from "components/Dashboard/DashboardProvider";
-import { FC, useEffect, useState } from "react";
+import { type FC, useEffect, useState, useSyncExternalStore } from "react";
 import { Helmet } from "react-helmet-async";
 import { pageTitle } from "utils/page";
 import { useWorkspacesData, useWorkspaceUpdate } from "./data";
@@ -21,15 +21,34 @@ import { MONOSPACE_FONT_FAMILY } from "theme/constants";
 import TextField from "@mui/material/TextField";
 import { displayError } from "components/GlobalSnackbar/utils";
 import { getErrorMessage } from "api/errors";
+import { useEffectEvent } from "hooks/hookPolyfills";
+
+function useSafeSearchParams() {
+  // Have to wrap setSearchParams because React Router doesn't make sure that
+  // the function's memory reference stays stable on each render, even though
+  // its logic never changes, and even though it has function update support
+  const [searchParams, setSearchParams] = useSearchParams();
+  const stableSetSearchParams = useEffectEvent(setSearchParams);
+
+  // Need this to be a tuple type, but can't use "as const", because that would
+  // make the whole array readonly and cause type mismatches downstream
+  return [searchParams, stableSetSearchParams] as ReturnType<
+    typeof useSearchParams
+  >;
+}
 
 const WorkspacesPage: FC = () => {
   const [dormantWorkspaces, setDormantWorkspaces] = useState<Workspace[]>([]);
   // If we use a useSearchParams for each hook, the values will not be in sync.
   // So we have to use a single one, centralizing the values, and pass it to
   // each hook.
-  const searchParamsResult = useSearchParams();
+  const searchParamsResult = useSafeSearchParams();
   const pagination = usePagination({ searchParamsResult });
-  const filterProps = useWorkspacesFilter({ searchParamsResult, pagination });
+  const filterProps = useWorkspacesFilter({
+    searchParamsResult,
+    onFilterChange: () => pagination.goToPage(1),
+  });
+
   const { data, error, queryKey, refetch } = useWorkspacesData({
     ...pagination,
     query: filterProps.filter.query,
@@ -121,20 +140,58 @@ const WorkspacesPage: FC = () => {
 
 export default WorkspacesPage;
 
+const workspaceFilterKey = "WorkspacesPage/filter";
+const defaultWorkspaceFilter = "owner:me";
+
+// Function should stay outside components as much as possible; if declared
+// inside the component, React would add/remove event listeners every render
+function subscribeToFilterChanges(notifyReact: () => void) {
+  const onStorageChange = (event: StorageEvent) => {
+    const { key, storageArea, oldValue, newValue } = event;
+
+    const shouldNotify =
+      key === workspaceFilterKey &&
+      storageArea === window.localStorage &&
+      newValue !== oldValue;
+
+    if (shouldNotify) {
+      notifyReact();
+    }
+  };
+
+  window.addEventListener("storage", onStorageChange);
+  return () => window.removeEventListener("storage", onStorageChange);
+}
+
 type UseWorkspacesFilterOptions = {
   searchParamsResult: ReturnType<typeof useSearchParams>;
-  pagination: ReturnType<typeof usePagination>;
+  onFilterChange: () => void;
 };
 
 const useWorkspacesFilter = ({
   searchParamsResult,
-  pagination,
+  onFilterChange,
 }: UseWorkspacesFilterOptions) => {
+  // Using useSyncExternalStore store to safely access localStorage from the
+  // first render; both snapshot callbacks return primitives, so no special
+  // trickery needed to prevent hook from immediately blowing up in dev mode
+  const localStorageFilter = useSyncExternalStore(
+    subscribeToFilterChanges,
+    () => {
+      return (
+        window.localStorage.getItem(workspaceFilterKey) ??
+        defaultWorkspaceFilter
+      );
+    },
+    () => defaultWorkspaceFilter,
+  );
+
   const filter = useFilter({
-    initialValue: `owner:me`,
+    fallbackFilter: localStorageFilter,
     searchParamsResult,
-    onUpdate: () => {
-      pagination.goToPage(1);
+    onUpdate: (newValues) => {
+      window.localStorage.setItem(workspaceFilterKey, newValues);
+      onFilterChange();
     },
   });
 
