From 9f35d9ba38b4e139a001d3d01559133de9c27690 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:00:43 +0000 Subject: [PATCH 1/9] fix(site): speed up state syncs and add input validation for debounce hook logic --- site/src/hooks/debounce.ts | 62 ++++++++++++++++++++++++-------------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index 945c927aad00c..05c41f7f24274 100644 --- a/site/src/hooks/debounce.ts +++ b/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 = Readonly<{ +type UseDebouncedFunctionReturn = Readonly<{ debounced: (...args: Args) => void; // Mainly here to make interfacing with useEffect cleanup functions easier @@ -34,12 +31,18 @@ type useDebouncedFunctionReturn = 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, debounceTimeMs: number, -): useDebouncedFunctionReturn { +): UseDebouncedFunctionReturn { + if (!Number.isInteger(debounceTimeMs) || debounceTimeMs < 0) { + throw new Error( + `Provided debounce value ${debounceTimeMs} is not a non-negative integer`, + ); + } + const timeoutIdRef = useRef(null); const cancelDebounce = useCallback(() => { if (timeoutIdRef.current !== null) { @@ -81,19 +84,32 @@ export function useDebouncedFunction< /** * Takes any value, and returns out a debounced version of it. */ -export function useDebouncedValue( - value: T, - debounceTimeMs: number, -): T { - const [debouncedValue, setDebouncedValue] = useState(value); +export function useDebouncedValue(value: T, debounceTimeoutMs: number): T { + if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { + throw new Error( + `Provided debounce value ${debounceTimeoutMs} is not a non-negative integer`, + ); + } - useEffect(() => { - const timeoutId = window.setTimeout(() => { - setDebouncedValue(value); - }, debounceTimeMs); + const [debounced, setDebounced] = 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 !== debounced && debounceTimeoutMs === 0) { + setDebounced(value); + } + useEffect(() => { + if (debounceTimeoutMs === 0) { + return; + } - return () => window.clearTimeout(timeoutId); - }, [value, debounceTimeMs]); + const timeoutId = window.setTimeout(() => { + setDebounced(value); + }, debounceTimeoutMs); + return () => window.clearTimeout(timeoutId); + }, [value, debounceTimeoutMs]); - return debouncedValue; + return debounced; } From 01850ca9f762d21a894e7f9b92127ed312ad44be Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:03:43 +0000 Subject: [PATCH 2/9] fix: shrink diff --- site/src/hooks/debounce.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index 05c41f7f24274..79349c036824b 100644 --- a/site/src/hooks/debounce.ts +++ b/site/src/hooks/debounce.ts @@ -91,14 +91,14 @@ export function useDebouncedValue(value: T, debounceTimeoutMs: number): T { ); } - const [debounced, setDebounced] = useState(value); + 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 !== debounced && debounceTimeoutMs === 0) { - setDebounced(value); + if (value !== debouncedValue && debounceTimeoutMs === 0) { + setDebouncedValue(value); } useEffect(() => { if (debounceTimeoutMs === 0) { @@ -106,10 +106,10 @@ export function useDebouncedValue(value: T, debounceTimeoutMs: number): T { } const timeoutId = window.setTimeout(() => { - setDebounced(value); + setDebouncedValue(value); }, debounceTimeoutMs); return () => window.clearTimeout(timeoutId); }, [value, debounceTimeoutMs]); - return debounced; + return debouncedValue; } From 264069e20df3acecf178c9ad22da9cab5827ecd1 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:11:01 +0000 Subject: [PATCH 3/9] fix: update tests --- site/src/hooks/debounce.test.ts | 40 ++++++++++++++++++++++++--------- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index 6f0097d05055d..3c12c674fccef 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -11,18 +11,18 @@ afterAll(() => { jest.clearAllMocks(); }); -describe(`${useDebouncedValue.name}`, () => { - function renderDebouncedValue(value: T, time: number) { - return renderHook( - ({ value, time }: { value: T; time: number }) => { - return useDebouncedValue(value, time); - }, - { - initialProps: { value, time }, - }, - ); - } +function renderDebouncedValue(value: T, time: number) { + return renderHook( + ({ value, time }: { value: T; time: number }) => { + return useDebouncedValue(value, time); + }, + { + initialProps: { value, time }, + }, + ); +} +describe(`${useDebouncedValue.name}`, () => { it("Should immediately return out the exact same value (by reference) on mount", () => { const value = {}; const { result } = renderDebouncedValue(value, 2000); @@ -58,6 +58,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 we want + rerender({ value: !initialValue, time: 0 }); + expect(result.current).toBe(true); + }) }); describe(`${useDebouncedFunction.name}`, () => { From 9cd1c571542409b1525a28508b7211deb59882af Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:21:26 +0000 Subject: [PATCH 4/9] fix: add even more tests --- site/src/hooks/debounce.test.ts | 70 ++++++++++++++++++++++++++------- site/src/hooks/debounce.ts | 70 ++++++++++++++++----------------- 2 files changed, 90 insertions(+), 50 deletions(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index 3c12c674fccef..c4b2215ee0cc5 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -23,6 +23,25 @@ function renderDebouncedValue(value: T, time: number) { } 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( + `Provided debounce value ${input} must be a non-negative integer`, + ); + } + }); + it("Should immediately return out the exact same value (by reference) on mount", () => { const value = {}; const { result } = renderDebouncedValue(value, 2000); @@ -72,26 +91,47 @@ describe(`${useDebouncedValue.name}`, () => { rerender({ value: !initialValue, time }); expect(result.current).toEqual(false); - // Then do the real re-render we want + // Then do the real re-render once we know the coast is clear rerender({ value: !initialValue, time: 0 }); expect(result.current).toBe(true); - }) + }); }); +function renderDebouncedFunction( + callbackArg: (...args: Args) => void | Promise, + time: number, +) { + return renderHook( + ({ callback, time }: { callback: typeof callbackArg; time: number }) => { + return useDebouncedFunction(callback, time); + }, + { + initialProps: { callback: callbackArg, time }, + }, + ); +} + describe(`${useDebouncedFunction.name}`, () => { - function renderDebouncedFunction( - callbackArg: (...args: Args) => void | Promise, - time: number, - ) { - return renderHook( - ({ callback, time }: { callback: typeof callbackArg; time: number }) => { - return useDebouncedFunction(callback, time); - }, - { - initialProps: { callback: callbackArg, time }, - }, - ); - } + 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 dummyValue = false; + for (const input of invalidInputs) { + expect(() => { + renderDebouncedValue(dummyValue, input); + }).toThrow( + `Provided debounce value ${input} must be a non-negative integer`, + ); + } + }); + }); describe("hook", () => { it("Should provide stable function references across re-renders", () => { diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index 79349c036824b..9e338e17f9019 100644 --- a/site/src/hooks/debounce.ts +++ b/site/src/hooks/debounce.ts @@ -35,28 +35,28 @@ export function useDebouncedFunction< Args extends unknown[] = unknown[], >( callback: (...args: Args) => void | Promise, - debounceTimeMs: number, + debounceTimeoutMs: number, ): UseDebouncedFunctionReturn { - if (!Number.isInteger(debounceTimeMs) || debounceTimeMs < 0) { - throw new Error( - `Provided debounce value ${debounceTimeMs} is not a non-negative integer`, - ); - } + if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { + throw new Error( + `Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`, + ); + } - const timeoutIdRef = useRef(null); + const timeoutIdRef = useRef(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(() => { @@ -85,31 +85,31 @@ export function useDebouncedFunction< * Takes any value, and returns out a debounced version of it. */ export function useDebouncedValue(value: T, debounceTimeoutMs: number): T { - if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { - throw new Error( - `Provided debounce value ${debounceTimeoutMs} is not a non-negative integer`, - ); - } + if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { + throw new Error( + `Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`, + ); + } - const [debouncedValue, setDebouncedValue] = useState(value); + 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; - } + // 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); - }, debounceTimeoutMs); - return () => window.clearTimeout(timeoutId); - }, [value, debounceTimeoutMs]); + const timeoutId = window.setTimeout(() => { + setDebouncedValue(value); + }, debounceTimeoutMs); + return () => window.clearTimeout(timeoutId); + }, [value, debounceTimeoutMs]); - return debouncedValue; + return debouncedValue; } From af376a252be46011619a265cfca0f62f533a19f2 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:24:43 +0000 Subject: [PATCH 5/9] fix: remove default type parameter --- site/src/hooks/debounce.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index c4b2215ee0cc5..d00ff1f57395e 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -11,7 +11,7 @@ afterAll(() => { jest.clearAllMocks(); }); -function renderDebouncedValue(value: T, time: number) { +function renderDebouncedValue(value: T, time: number) { return renderHook( ({ value, time }: { value: T; time: number }) => { return useDebouncedValue(value, time); From 9ca576b3beb3457992d5d502443fb6dfb45ee4d3 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:27:47 +0000 Subject: [PATCH 6/9] refactor: move test harness functions back to old spots --- site/src/hooks/debounce.test.ts | 50 ++++++++++++++++----------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index d00ff1f57395e..332b2ca2dff32 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -11,18 +11,18 @@ afterAll(() => { jest.clearAllMocks(); }); -function renderDebouncedValue(value: T, time: number) { - return renderHook( - ({ value, time }: { value: T; time: number }) => { - return useDebouncedValue(value, time); - }, - { - initialProps: { value, time }, - }, - ); -} - describe(`${useDebouncedValue.name}`, () => { + function renderDebouncedValue(value: T, time: number) { + return renderHook( + ({ value, time }: { value: T; time: number }) => { + return useDebouncedValue(value, time); + }, + { + initialProps: { value, time }, + }, + ); + } + it("Should throw for non-nonnegative integer timeouts", () => { const invalidInputs: readonly number[] = [ Number.NaN, @@ -97,21 +97,21 @@ describe(`${useDebouncedValue.name}`, () => { }); }); -function renderDebouncedFunction( - callbackArg: (...args: Args) => void | Promise, - time: number, -) { - return renderHook( - ({ callback, time }: { callback: typeof callbackArg; time: number }) => { - return useDebouncedFunction(callback, time); - }, - { - initialProps: { callback: callbackArg, time }, - }, - ); -} - describe(`${useDebouncedFunction.name}`, () => { + function renderDebouncedFunction( + callbackArg: (...args: Args) => void | Promise, + time: number, + ) { + return renderHook( + ({ callback, time }: { callback: typeof callbackArg; time: number }) => { + return useDebouncedFunction(callback, time); + }, + { + initialProps: { callback: callbackArg, time }, + }, + ); + } + describe("input validation", () => { it("Should throw for non-nonnegative integer timeouts", () => { const invalidInputs: readonly number[] = [ From 11217cb07e30ef47647edf07495eb65ffc4a3da9 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 14:39:03 +0000 Subject: [PATCH 7/9] fix: remove bad variable name --- site/src/hooks/debounce.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index 332b2ca2dff32..9e44816cc35b1 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -122,10 +122,10 @@ describe(`${useDebouncedFunction.name}`, () => { -42, ]; - const dummyValue = false; + const dummyFunction = jest.fn() for (const input of invalidInputs) { expect(() => { - renderDebouncedValue(dummyValue, input); + renderDebouncedFunction(dummyFunction, input); }).toThrow( `Provided debounce value ${input} must be a non-negative integer`, ); From 4266b85a3eb8091fd06f797c6f937309a911aaeb Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Tue, 15 Jul 2025 15:23:50 +0000 Subject: [PATCH 8/9] fix: update formatting again --- site/src/hooks/debounce.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index 9e44816cc35b1..d0523d2b89f3b 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -122,7 +122,7 @@ describe(`${useDebouncedFunction.name}`, () => { -42, ]; - const dummyFunction = jest.fn() + const dummyFunction = jest.fn(); for (const input of invalidInputs) { expect(() => { renderDebouncedFunction(dummyFunction, input); From c25cc8ff933c98865a85ad49db9d034a572d4ec8 Mon Sep 17 00:00:00 2001 From: Michael Smith Date: Thu, 17 Jul 2025 21:56:09 +0000 Subject: [PATCH 9/9] fix: apply feedback --- site/src/hooks/debounce.test.ts | 6 +++--- site/src/hooks/debounce.ts | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/site/src/hooks/debounce.test.ts b/site/src/hooks/debounce.test.ts index d0523d2b89f3b..6de4a261f3797 100644 --- a/site/src/hooks/debounce.test.ts +++ b/site/src/hooks/debounce.test.ts @@ -11,7 +11,7 @@ afterAll(() => { jest.clearAllMocks(); }); -describe(`${useDebouncedValue.name}`, () => { +describe(useDebouncedValue.name, () => { function renderDebouncedValue(value: T, time: number) { return renderHook( ({ value, time }: { value: T; time: number }) => { @@ -37,7 +37,7 @@ describe(`${useDebouncedValue.name}`, () => { expect(() => { renderDebouncedValue(dummyValue, input); }).toThrow( - `Provided debounce value ${input} must be a non-negative integer`, + `Invalid value ${input} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, ); } }); @@ -127,7 +127,7 @@ describe(`${useDebouncedFunction.name}`, () => { expect(() => { renderDebouncedFunction(dummyFunction, input); }).toThrow( - `Provided debounce value ${input} must be a non-negative integer`, + `Invalid value ${input} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, ); } }); diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts index 9e338e17f9019..0ed3d960d0ab2 100644 --- a/site/src/hooks/debounce.ts +++ b/site/src/hooks/debounce.ts @@ -39,7 +39,7 @@ export function useDebouncedFunction< ): UseDebouncedFunctionReturn { if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { throw new Error( - `Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`, + `Invalid value ${debounceTimeoutMs} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, ); } @@ -87,7 +87,7 @@ export function useDebouncedFunction< export function useDebouncedValue(value: T, debounceTimeoutMs: number): T { if (!Number.isInteger(debounceTimeoutMs) || debounceTimeoutMs < 0) { throw new Error( - `Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`, + `Invalid value ${debounceTimeoutMs} for debounceTimeoutMs. Value must be an integer greater than or equal to zero.`, ); }