coder · Parkreiner · Sep 15, 2023 · Sep 13, 2023 · Sep 13, 2023 · Sep 14, 2023
diff --git a/site/src/components/Filter/filter.tsx b/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,17 +131,7 @@ export const MenuSkeleton = () => (
   <BaseSkeleton sx={{ minWidth: 200, flexShrink: 0 }} />
 );
 
-export const Filter = ({
-  filter,
-  isLoading,
-  error,
-  skeleton,
-  options,
-  learnMoreLink,
-  learnMoreLabel2,
-  learnMoreLink2,
-  presets,
-}: {
+type FilterProps = {
   filter: ReturnType<typeof useFilter>;
   skeleton: ReactNode;
   isLoading: boolean;
@@ -150,20 +141,42 @@ export const Filter = ({
   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);
+};
 
+export const Filter = ({
+  filter,
+  isLoading,
+  error,
+  skeleton,
+  options,
+  learnMoreLink,
+  learnMoreLabel2,
+  learnMoreLink2,
+  presets,
+}: 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,

diff --git a/site/src/hooks/debounce.test.ts b/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();
+    });
+  });
+});