diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 4b8b71f42ff37..fe8d2656e40ad 100644 --- 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 = () => ( ); -export const Filter = ({ - filter, - isLoading, - error, - skeleton, - options, - learnMoreLink, - learnMoreLabel2, - learnMoreLink2, - presets, -}: { +type FilterProps = { filter: ReturnType; 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(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(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 ( { - 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 new file mode 100644 index 0000000000000..5a8ed78c026aa --- /dev/null +++ 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(value: T, time: number) { + return renderHook( + ({ value, time }: { value: T; time: number }) => { + return useDebouncedValue(value, time); + }, + { + initialProps: { value, time }, + }, + ); +} + +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(`${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(); + }); + }); +}); diff --git a/site/src/hooks/debounce.ts b/site/src/hooks/debounce.ts new file mode 100644 index 0000000000000..38eea91da9933 --- /dev/null +++ b/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 = 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, + debounceTimeMs: number, +): useDebouncedFunctionReturn { + const timeoutIdRef = useRef(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( + 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; +} diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 65b4262788f5d..f5f8013963cbc 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/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) => diff --git a/site/src/pages/WorkspacesPage/filter/filter.tsx b/site/src/pages/WorkspacesPage/filter/filter.tsx index 355598a09c107..b1fe3edb677d3 100644 --- a/site/src/pages/WorkspacesPage/filter/filter.tsx +++ b/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; 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 (