Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Jul 17, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
9f35d9b
fix(site): speed up state syncs and add input validation for debounce…
Parkreiner Jul 15, 2025
01850ca
fix: shrink diff
Parkreiner Jul 15, 2025
264069e
fix: update tests
Parkreiner Jul 15, 2025
9cd1c57
fix: add even more tests
Parkreiner Jul 15, 2025
af376a2
fix: remove default type parameter
Parkreiner Jul 15, 2025
9ca576b
refactor: move test harness functions back to old spots
Parkreiner Jul 15, 2025
11217cb
fix: remove bad variable name
Parkreiner Jul 15, 2025
4266b85
fix: update formatting again
Parkreiner Jul 15, 2025
b9b5c9f
Merge branch 'main' into mes/debounce-fix
Parkreiner Jul 17, 2025
c25cc8f
fix: apply feedback
Parkreiner Jul 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix: add even more tests
  • Loading branch information
@Parkreiner
Parkreiner committed Jul 15, 2025
commit 9cd1c571542409b1525a28508b7211deb59882af
70 changes: 55 additions & 15 deletions site/src/hooks/debounce.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,25 @@ function renderDebouncedValue<T = unknown>(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);
Expand Down Expand Up @@ -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<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(`${useDebouncedFunction.name}`, () => {
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("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", () => {
Expand Down
70 changes: 35 additions & 35 deletions site/src/hooks/debounce.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,28 +35,28 @@ export function useDebouncedFunction<
Args extends unknown[] = unknown[],
>(
callback: (...args: Args) => void | Promise<void>,
debounceTimeMs: number,
debounceTimeoutMs: number,
): UseDebouncedFunctionReturn<Args> {
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`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provided debounce value -1 must be a non-negative integer

I'm not sure this wording makes much sense. how about something like...

Suggested change
`Provided debounce value ${debounceTimeoutMs} must be a non-negative integer`,
`Invalid value ${debounceTimeoutMS} for debounceTimeoutMs. Value must be greater than or equal to zero.`,

);
}

const timeoutIdRef = useRef<number | null>(null);
const timeoutIdRef = useRef<number | undefined>(undefined);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated ref type to match the type used by the browser's setTimeout

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(() => {
Expand Down Expand Up @@ -85,31 +85,31 @@ export function useDebouncedFunction<
* Takes any value, and returns out a debounced version of it.
*/
export function useDebouncedValue<T>(value: T, debounceTimeoutMs: number): T {
Copy link
Member Author

@Parkreiner Parkreiner Jul 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed the default parameter here because if you, the user of the hook, don't know the type of a value you're trying to debounce, you're probably doing something really wrong

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`,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here

);
}

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;
}
Loading