fix: prevent workspace search bar text from getting garbled (#9703)

* chore: Reorganize hook calls for useWorkspacesFilter

* refactor: Clean up some filter logic

* refactor: Create debounce utility hooks

* docs: Clean up comments for clarity

* fix: Update focus logic to apply for any inner focus

* fix: Add onBlur behavior for state syncs

* chore: Add progress for debounce test

* chore: Finish tests for debounce hooks

* docs: Add file description and warning

site/src/components/Filter/filter.tsx

@@ -20,11 +20,11 @@ import {
 } from "api/errors";
 import { useFilterMenu } from "./menu";
 import { BaseOption } from "./options";
-import debounce from "just-debounce-it";
 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";
 
 export type PresetFilter = {
   name: string;
@@ -58,7 +58,7 @@ export const useFilter = ({
     }
   };
 
-  const debounceUpdate = debounce(
+  const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction(
     (values: string | FilterValues) => update(values),
     500,
   );
@@ -69,6 +69,7 @@ export const useFilter = ({
     query,
     update,
     debounceUpdate,
+    cancelDebounce,
     values,
     used,
   };
@@ -130,6 +131,18 @@ export const MenuSkeleton = () => (
   <BaseSkeleton sx={{ minWidth: 200, flexShrink: 0 }} />
 );
 
+type FilterProps = {
+  filter: ReturnType<typeof useFilter>;
+  skeleton: ReactNode;
+  isLoading: boolean;
+  learnMoreLink: string;
+  learnMoreLabel2?: string;
+  learnMoreLink2?: string;
+  error?: unknown;
+  options?: ReactNode;
+  presets: PresetFilter[];
+};
+
 export const Filter = ({
   filter,
   isLoading,
@@ -140,30 +153,30 @@ export const Filter = ({
   learnMoreLabel2,
   learnMoreLink2,
   presets,
-}: {
-  filter: ReturnType<typeof useFilter>;
-  skeleton: ReactNode;
-  isLoading: boolean;
-  learnMoreLink: string;
-  learnMoreLabel2?: string;
-  learnMoreLink2?: string;
-  error?: unknown;
-  options?: ReactNode;
-  presets: PresetFilter[];
-}) => {
-  const shouldDisplayError = hasError(error) && isApiValidationError(error);
-  const hasFilterQuery = filter.query !== "";
-  const [searchQuery, setSearchQuery] = useState(filter.query);
-  const inputRef = useRef<HTMLInputElement>(null);
+}: FilterProps) => {
+  // Storing local copy of the filter query so that it can be updated more
+  // aggressively without re-renders rippling out to the rest of the app every
+  // single time. Exists for performance reasons - not really a good way to
+  // remove this; render keys would cause the component to remount too often
+  const [queryCopy, setQueryCopy] = useState(filter.query);
+  const textboxInputRef = useRef<HTMLInputElement>(null);
 
+  // Conditionally re-syncs the parent and local filter queries
   useEffect(() => {
-    // We don't want to update this while the user is typing something or has the focus in the input
-    const isFocused = document.activeElement === inputRef.current;
-    if (!isFocused) {
-      setSearchQuery(filter.query);
+    const hasSelfOrInnerFocus =
+      textboxInputRef.current?.contains(document.activeElement) ?? false;
+
+    // This doesn't address all state sync issues - namely, what happens if the
+    // user removes focus just after this synchronizing effect fires. Also need
+    // to rely on onBlur behavior as an extra safety measure
+    if (!hasSelfOrInnerFocus) {
+      setQueryCopy(filter.query);
     }
   }, [filter.query]);
 
+  const shouldDisplayError = hasError(error) && isApiValidationError(error);
+  const hasFilterQuery = filter.query !== "";
+
   return (
     <Box
       sx={{
@@ -198,12 +211,17 @@ export const Filter = ({
                 "aria-label": "Filter",
                 name: "query",
                 placeholder: "Search...",
-                value: searchQuery,
-                ref: inputRef,
+                value: queryCopy,
+                ref: textboxInputRef,
                 onChange: (e) => {
-                  setSearchQuery(e.target.value);
+                  setQueryCopy(e.target.value);
                   filter.debounceUpdate(e.target.value);
                 },
+                onBlur: () => {
+                  if (queryCopy !== filter.query) {
+                    setQueryCopy(filter.query);
+                  }
+                },
                 sx: {
                   borderRadius: "6px",
                   borderTopLeftRadius: 0,

site/src/hooks/debounce.test.ts

@@ -0,0 +1,193 @@
+import { renderHook } from "@testing-library/react";
+import { useDebouncedFunction, useDebouncedValue } from "./debounce";
+
+beforeAll(() => {
+  jest.useFakeTimers();
+  jest.spyOn(global, "setTimeout");
+});
+
+afterAll(() => {
+  jest.useRealTimers();
+  jest.clearAllMocks();
+});
+
+// Most UI tests should be structure from the user's experience, but just
+// because these are more abstract, general-purpose hooks, it seemed harder to
+// do that. Had to bring in some mocks
+function renderDebouncedValue<T = unknown>(value: T, time: number) {
+  return renderHook(
+    ({ value, time }: { value: T; time: number }) => {
+      return useDebouncedValue(value, time);
+    },
+    {
+      initialProps: { value, time },
+    },
+  );
+}
+
+function renderDebouncedFunction<Args extends unknown[]>(
+  callbackArg: (...args: Args) => void | Promise<void>,
+  time: number,
+) {
+  return renderHook(
+    ({ callback, time }: { callback: typeof callbackArg; time: number }) => {
+      return useDebouncedFunction<Args>(callback, time);
+    },
+    {
+      initialProps: { callback: callbackArg, time },
+    },
+  );
+}
+
+describe(`${useDebouncedValue.name}`, () => {
+  it("Should immediately return out the exact same value (by reference) on mount", () => {
+    const value = {};
+    const { result } = renderDebouncedValue(value, 2000);
+
+    expect(result.current).toBe(value);
+    expect.hasAssertions();
+  });
+
+  it("Should not immediately resync state as the hook re-renders with new value argument", async () => {
+    let value = 0;
+    const time = 5000;
+
+    const { result, rerender } = renderDebouncedValue(value, time);
+    expect(result.current).toEqual(0);
+
+    for (let i = 1; i <= 5; i++) {
+      setTimeout(() => {
+        value++;
+        rerender({ value, time });
+      }, i * 100);
+    }
+
+    await jest.advanceTimersByTimeAsync(time - 100);
+    expect(result.current).toEqual(0);
+    expect.hasAssertions();
+  });
+
+  it("Should resync after specified milliseconds pass with no change to arguments", async () => {
+    const initialValue = false;
+    const time = 5000;
+
+    const { result, rerender } = renderDebouncedValue(initialValue, time);
+    expect(result.current).toEqual(false);
+
+    rerender({ value: !initialValue, time });
+    await jest.runAllTimersAsync();
+
+    expect(result.current).toEqual(true);
+    expect.hasAssertions();
+  });
+});
+
+describe(`${useDebouncedFunction.name}`, () => {
+  describe("hook", () => {
+    it("Should provide stable function references across re-renders", () => {
+      const time = 5000;
+      const { result, rerender } = renderDebouncedFunction(jest.fn(), time);
+
+      const { debounced: oldDebounced, cancelDebounce: oldCancel } =
+        result.current;
+
+      rerender({ callback: jest.fn(), time });
+      const { debounced: newDebounced, cancelDebounce: newCancel } =
+        result.current;
+
+      expect(oldDebounced).toBe(newDebounced);
+      expect(oldCancel).toBe(newCancel);
+      expect.hasAssertions();
+    });
+
+    it("Resets any pending debounces if the timer argument changes", async () => {
+      const time = 5000;
+      let count = 0;
+      const incrementCount = () => {
+        count++;
+      };
+
+      const { result, rerender } = renderDebouncedFunction(
+        incrementCount,
+        time,
+      );
+
+      result.current.debounced();
+      rerender({ callback: incrementCount, time: time + 1 });
+
+      await jest.runAllTimersAsync();
+      expect(count).toEqual(0);
+      expect.hasAssertions();
+    });
+  });
+
+  describe("debounced function", () => {
+    it("Resolve the debounce after specified milliseconds pass with no other calls", async () => {
+      let value = false;
+      const { result } = renderDebouncedFunction(() => {
+        value = !value;
+      }, 100);
+
+      result.current.debounced();
+
+      await jest.runOnlyPendingTimersAsync();
+      expect(value).toBe(true);
+      expect.hasAssertions();
+    });
+
+    it("Always uses the most recent callback argument passed in (even if it switches while a debounce is queued)", async () => {
+      let count = 0;
+      const time = 500;
+
+      const { result, rerender } = renderDebouncedFunction(() => {
+        count = 1;
+      }, time);
+
+      result.current.debounced();
+      rerender({
+        callback: () => {
+          count = 9999;
+        },
+        time,
+      });
+
+      await jest.runAllTimersAsync();
+      expect(count).toEqual(9999);
+      expect.hasAssertions();
+    });
+
+    it("Should reset the debounce timer with repeated calls to the method", async () => {
+      let count = 0;
+      const { result } = renderDebouncedFunction(() => {
+        count++;
+      }, 2000);
+
+      for (let i = 0; i < 10; i++) {
+        setTimeout(() => {
+          result.current.debounced();
+        }, i * 100);
+      }
+
+      await jest.runAllTimersAsync();
+      expect(count).toBe(1);
+      expect.hasAssertions();
+    });
+  });
+
+  describe("cancelDebounce function", () => {
+    it("Should be able to cancel a pending debounce", async () => {
+      let count = 0;
+      const { result } = renderDebouncedFunction(() => {
+        count++;
+      }, 2000);
+
+      const { debounced, cancelDebounce } = result.current;
+      debounced();
+      cancelDebounce();
+
+      await jest.runAllTimersAsync();
+      expect(count).toEqual(0);
+      expect.hasAssertions();
+    });
+  });
+});

site/src/hooks/debounce.ts

@@ -0,0 +1,99 @@
+/**
+ * @file Defines hooks for created debounced versions of functions and arbitrary
+ * values.
+ *
+ * It is not safe to call a general-purpose debounce utility inside a React
+ * render. It will work on the initial render, but the memory reference for the
+ * value will change on re-renders. Most debounce functions create a "stateful"
+ * version of a function by leveraging closure; but by calling it repeatedly,
+ * you create multiple "pockets" of state, rather than a centralized one.
+ *
+ * Debounce utilities can make sense if they can be called directly outside the
+ * component or in a useEffect call, though.
+ */
+import { useCallback, useEffect, useRef, useState } from "react";
+
+type useDebouncedFunctionReturn<Args extends unknown[]> = Readonly<{
+  debounced: (...args: Args) => void;
+
+  // Mainly here to make interfacing with useEffect cleanup functions easier
+  cancelDebounce: () => void;
+}>;
+
+/**
+ * Creates a debounce function that is resilient to React re-renders, as well as
+ * a function for canceling a pending debounce.
+ *
+ * The returned-out functions will maintain the same memory references, but the
+ * debounce function will always "see" the most recent versions of the arguments
+ * passed into the hook, and use them accordingly.
+ *
+ * If the debounce time changes while a callback has been queued to fire, the
+ * callback will be canceled completely. You will need to restart the debounce
+ * process by calling the returned-out function again.
+ */
+export function useDebouncedFunction<
+  // Parameterizing on the args instead of the whole callback function type to
+  // avoid type contra-variance issues
+  Args extends unknown[] = unknown[],
+>(
+  callback: (...args: Args) => void | Promise<void>,
+  debounceTimeMs: number,
+): useDebouncedFunctionReturn<Args> {
+  const timeoutIdRef = useRef<number | null>(null);
+  const cancelDebounce = useCallback(() => {
+    if (timeoutIdRef.current !== null) {
+      window.clearTimeout(timeoutIdRef.current);
+    }
+
+    timeoutIdRef.current = null;
+  }, []);
+
+  const debounceTimeRef = useRef(debounceTimeMs);
+  useEffect(() => {
+    cancelDebounce();
+    debounceTimeRef.current = debounceTimeMs;
+  }, [cancelDebounce, debounceTimeMs]);
+
+  const callbackRef = useRef(callback);
+  useEffect(() => {
+    callbackRef.current = callback;
+  }, [callback]);
+
+  // Returned-out function will always be synchronous, even if the callback arg
+  // is async. Seemed dicey to try awaiting a genericized operation that can and
+  // will likely be canceled repeatedly
+  const debounced = useCallback(
+    (...args: Args): void => {
+      cancelDebounce();
+
+      timeoutIdRef.current = window.setTimeout(
+        () => void callbackRef.current(...args),
+        debounceTimeRef.current,
+      );
+    },
+    [cancelDebounce],
+  );
+
+  return { debounced, cancelDebounce } as const;
+}
+
+/**
+ * Takes any value, and returns out a debounced version of it.
+ */
+export function useDebouncedValue<T = unknown>(
+  value: T,
+  debounceTimeMs: number,
+): T {
+  const [debouncedValue, setDebouncedValue] = useState(value);
+
+  useEffect(() => {
+    const timeoutId = window.setTimeout(() => {
+      setDebouncedValue(value);
+    }, debounceTimeMs);
+
+    return () => window.clearTimeout(timeoutId);
+  }, [value, debounceTimeMs]);
+
+  return debouncedValue;
+}

site/src/pages/WorkspacesPage/WorkspacesPage.tsx

@@ -130,7 +130,6 @@ const useWorkspacesFilter = ({
   searchParamsResult,
   pagination,
 }: UseWorkspacesFilterOptions) => {
-  const orgId = useOrganizationId();
   const filter = useFilter({
     initialValue: `owner:me`,
     searchParamsResult,
@@ -138,6 +137,7 @@ const useWorkspacesFilter = ({
       pagination.goToPage(1);
     },
   });
+
   const permissions = usePermissions();
   const canFilterByUser = permissions.viewDeploymentValues;
   const userMenu = useUserFilterMenu({
@@ -146,12 +146,15 @@ const useWorkspacesFilter = ({
       filter.update({ ...filter.values, owner: option?.value }),
     enabled: canFilterByUser,
   });
+
+  const orgId = useOrganizationId();
   const templateMenu = useTemplateFilterMenu({
     orgId,
     value: filter.values.template,
     onChange: (option) =>
       filter.update({ ...filter.values, template: option?.value }),
   });
+
   const statusMenu = useStatusFilterMenu({
     value: filter.values.status,
     onChange: (option) =>

site/src/pages/WorkspacesPage/filter/filter.tsx

@@ -18,9 +18,22 @@ import { UserFilterMenu, UserMenu } from "components/Filter/UserFilter";
 import { workspaceFilterQuery } from "utils/filters";
 import { docs } from "utils/docs";
 
-const PRESET_FILTERS = [
-  { query: workspaceFilterQuery.me, name: "My workspaces" },
-  { query: workspaceFilterQuery.all, name: "All workspaces" },
+type FilterPreset = {
+  query: string;
+  name: string;
+};
+
+// Can't use as const declarations to make arrays deep readonly because that
+// interferes with the type contracts for Filter
+const PRESET_FILTERS: FilterPreset[] = [
+  {
+    query: workspaceFilterQuery.me,
+    name: "My workspaces",
+  },
+  {
+    query: workspaceFilterQuery.all,
+    name: "All workspaces",
+  },
   {
     query: workspaceFilterQuery.running,
     name: "Running workspaces",
@@ -31,11 +44,16 @@ const PRESET_FILTERS = [
   },
 ];
 
-export const WorkspacesFilter = ({
-  filter,
-  error,
-  menus,
-}: {
+// Defined outside component so that the array doesn't get reconstructed each render
+const PRESETS_WITH_DORMANT: FilterPreset[] = [
+  ...PRESET_FILTERS,
+  {
+    query: workspaceFilterQuery.dormant,
+    name: "Dormant workspaces",
+  },
+];
+
+type WorkspaceFilterProps = {
   filter: ReturnType<typeof useFilter>;
   error?: unknown;
   menus: {
@@ -43,14 +61,15 @@ export const WorkspacesFilter = ({
     template: TemplateFilterMenu;
     status: StatusFilterMenu;
   };
-}) => {
-  const presets = [...PRESET_FILTERS];
-  if (useIsWorkspaceActionsEnabled()) {
-    presets.push({
-      query: workspaceFilterQuery.dormant,
-      name: "Dormant workspaces",
-    });
-  }
+};
+
+export const WorkspacesFilter = ({
+  filter,
+  error,
+  menus,
+}: WorkspaceFilterProps) => {
+  const actionsEnabled = useIsWorkspaceActionsEnabled();
+  const presets = actionsEnabled ? PRESET_FILTERS : PRESETS_WITH_DORMANT;
 
   return (
     <Filter