fix(site): speed up state syncs and validate input for debounce hook logic (coder#18877)

No issue to link – I'm basically pushing some updates upstream from the
version of the hook I copied over for the Registry website.

## Changes made
- Updated debounce functions to have input validation for timeouts
- Updated `useDebouncedValue` to flush state syncs immediately if
timeout value is `0`
- Updated tests to reflect changes
- Cleaned up some comments and parameter names to make things more clear

Author: buenos-nachos

site/src/hooks/debounce.test.ts

@@ -11,8 +11,8 @@ afterAll(() => {
 	jest.clearAllMocks();
 });
 
-describe(`${useDebouncedValue.name}`, () => {
-	function renderDebouncedValue<T = unknown>(value: T, time: number) {
+describe(useDebouncedValue.name, () => {
+	function renderDebouncedValue<T>(value: T, time: number) {
 		return renderHook(
 			({ value, time }: { value: T; time: number }) => {
 				return useDebouncedValue(value, time);
@@ -23,6 +23,25 @@ describe(`${useDebouncedValue.name}`, () => {
 		);
 	}
 
+	it("Should throw for non-nonnegative integer timeouts", () => {
+		const invalidInputs: readonly number[] = [
+			Number.NaN,
+			Number.NEGATIVE_INFINITY,
+			Number.POSITIVE_INFINITY,
+			Math.PI,
+			-42,
+		];
+
+		const dummyValue = false;
+		for (const input of invalidInputs) {
+			expect(() => {
+				renderDebouncedValue(dummyValue, input);
+			}).toThrow(
+				`Invalid value ${input} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`,
+			);
+		}
+	});
+
 	it("Should immediately return out the exact same value (by reference) on mount", () => {
 		const value = {};
 		const { result } = renderDebouncedValue(value, 2000);
@@ -58,6 +77,24 @@ describe(`${useDebouncedValue.name}`, () => {
 		await jest.runAllTimersAsync();
 		await waitFor(() => expect(result.current).toEqual(true));
 	});
+
+	// Very important that we not do any async logic for this test
+	it("Should immediately resync without any render/event loop delays if timeout is zero", () => {
+		const initialValue = false;
+		const time = 5000;
+
+		const { result, rerender } = renderDebouncedValue(initialValue, time);
+		expect(result.current).toEqual(false);
+
+		// Just to be on the safe side, re-render once with the old timeout to
+		// verify that nothing has been flushed yet
+		rerender({ value: !initialValue, time });
+		expect(result.current).toEqual(false);
+
+		// Then do the real re-render once we know the coast is clear
+		rerender({ value: !initialValue, time: 0 });
+		expect(result.current).toBe(true);
+	});
 });
 
 describe(`${useDebouncedFunction.name}`, () => {
@@ -75,6 +112,27 @@ describe(`${useDebouncedFunction.name}`, () => {
 		);
 	}
 
+	describe("input validation", () => {
+		it("Should throw for non-nonnegative integer timeouts", () => {
+			const invalidInputs: readonly number[] = [
+				Number.NaN,
+				Number.NEGATIVE_INFINITY,
+				Number.POSITIVE_INFINITY,
+				Math.PI,
+				-42,
+			];
+
+			const dummyFunction = jest.fn();
+			for (const input of invalidInputs) {
+				expect(() => {
+					renderDebouncedFunction(dummyFunction, input);
+				}).toThrow(
+					`Invalid value ${input} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`,
+				);
+			}
+		});
+	});
+
 	describe("hook", () => {
 		it("Should provide stable function references across re-renders", () => {
 			const time = 5000;

site/src/hooks/debounce.ts

@@ -2,18 +2,15 @@
  * @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.
+ * It is not safe to call most general-purpose debounce utility functions inside
+ * a React render. This is because the state for handling the debounce logic
+ * lives in the utility instead of React. If you call a general-purpose debounce
+ * function inline, that will create a new stateful function on every render,
+ * which has a lot of risks around conflicting/contradictory state.
  */
 import { useCallback, useEffect, useRef, useState } from "react";
 
-type useDebouncedFunctionReturn<Args extends unknown[]> = Readonly<{
+type UseDebouncedFunctionReturn<Args extends unknown[]> = Readonly<{
 	debounced: (...args: Args) => void;
 
 	// Mainly here to make interfacing with useEffect cleanup functions easier
@@ -34,26 +31,32 @@ type useDebouncedFunctionReturn<Args extends unknown[]> = Readonly<{
  */
 export function useDebouncedFunction<
 	// Parameterizing on the args instead of the whole callback function type to
-	// avoid type contra-variance issues
+	// avoid type contravariance issues
 	Args extends unknown[] = unknown[],
 >(
 	callback: (...args: Args) => void | Promise<void>,
-	debounceTimeMs: number,
-): useDebouncedFunctionReturn<Args> {
-	const timeoutIdRef = useRef<number | null>(null);
+	debounceTimeoutMs: number,
+): UseDebouncedFunctionReturn<Args> {
+	if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) {
+		throw new Error(
+			`Invalid value ${debounceTimeoutMs} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`,
+		);
+	}
+
+	const timeoutIdRef = useRef<number | undefined>(undefined);
 	const cancelDebounce = useCallback(() => {
-		if (timeoutIdRef.current !== null) {
+		if (timeoutIdRef.current !== undefined) {
 			window.clearTimeout(timeoutIdRef.current);
 		}
 
-		timeoutIdRef.current = null;
+		timeoutIdRef.current = undefined;
 	}, []);
 
-	const debounceTimeRef = useRef(debounceTimeMs);
+	const debounceTimeRef = useRef(debounceTimeoutMs);
 	useEffect(() => {
 		cancelDebounce();
-		debounceTimeRef.current = debounceTimeMs;
-	}, [cancelDebounce, debounceTimeMs]);
+		debounceTimeRef.current = debounceTimeoutMs;
+	}, [cancelDebounce, debounceTimeoutMs]);
 
 	const callbackRef = useRef(callback);
 	useEffect(() => {
@@ -81,19 +84,32 @@ export function useDebouncedFunction<
 /**
  * Takes any value, and returns out a debounced version of it.
  */
-export function useDebouncedValue<T = unknown>(
-	value: T,
-	debounceTimeMs: number,
-): T {
+export function useDebouncedValue<T>(value: T, debounceTimeoutMs: number): T {
+	if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) {
+		throw new Error(
+			`Invalid value ${debounceTimeoutMs} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`,
+		);
+	}
+
 	const [debouncedValue, setDebouncedValue] = useState(value);
 
+	// If the debounce timeout is ever zero, synchronously flush any state syncs.
+	// Doing this mid-render instead of in useEffect means that we drastically cut
+	// down on needless re-renders, and we also avoid going through the event loop
+	// to do a state sync that is *intended* to happen immediately
+	if (value !== debouncedValue && debounceTimeoutMs === 0) {
+		setDebouncedValue(value);
+	}
 	useEffect(() => {
+		if (debounceTimeoutMs === 0) {
+			return;
+		}
+
 		const timeoutId = window.setTimeout(() => {
 			setDebouncedValue(value);
-		}, debounceTimeMs);
-
+		}, debounceTimeoutMs);
 		return () => window.clearTimeout(timeoutId);
-	}, [value, debounceTimeMs]);
+	}, [value, debounceTimeoutMs]);
 
 	return debouncedValue;
 }