diff --git a/site/e2e/global.setup.ts b/site/e2e/global.setup.ts index 08a66c317ddfb..254d982e0933d 100644 --- a/site/e2e/global.setup.ts +++ b/site/e2e/global.setup.ts @@ -12,7 +12,6 @@ test("create first user", async ({ page }) => { await page.getByTestId("trial").click(); await page.getByTestId("create").click(); - await expect(page).toHaveURL("/workspaces"); - + await expect(page).toHaveURL(/\/workspaces.*/); await page.context().storageState({ path: STORAGE_STATE }); }); diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index fe8d2656e40ad..4ed99cb875037 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -24,7 +24,9 @@ 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"; +import { useEffectEvent } from "hooks/hookPolyfills"; export type PresetFilter = { name: string; @@ -33,45 +35,71 @@ export type PresetFilter = { type FilterValues = Record; +type UseFilterConfig = { + /** + * The fallback value to use in the event that no filter params can be parsed + * from the search params object. This value is allowed to change on + * re-renders. + */ + fallbackFilter?: string; + searchParamsResult: ReturnType; + onUpdate?: (newValue: string) => void; +}; + +const useFilterParamsKey = "filter"; + export const useFilter = ({ - initialValue = "", - onUpdate, + fallbackFilter = "", searchParamsResult, -}: { - initialValue?: string; - searchParamsResult: ReturnType; - onUpdate?: () => void; -}) => { + onUpdate, +}: UseFilterConfig) => { const [searchParams, setSearchParams] = searchParamsResult; - const query = searchParams.get("filter") ?? initialValue; - const values = parseFilterQuery(query); - - const update = (values: string | FilterValues) => { - if (typeof values === "string") { - searchParams.set("filter", values); - } else { - searchParams.set("filter", stringifyFilter(values)); - } + const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter; + + // Stabilizing reference to setSearchParams from one central spot, just to be + // on the extra careful side; don't want effects over-running. You would think + // this would be overkill, but setSearchParams isn't stable out of the box + const stableSetSearchParams = useEffectEvent(setSearchParams); + + // Keep params synced with query, even as query changes from outside sources + useEffect(() => { + stableSetSearchParams((currentParams) => { + const currentQuery = currentParams.get(useFilterParamsKey); + + if (query === "") { + currentParams.delete(useFilterParamsKey); + } else if (currentQuery !== query) { + currentParams.set(useFilterParamsKey, query); + } + + return currentParams; + }); + }, [stableSetSearchParams, query]); + + const update = (newValues: string | FilterValues) => { + const serialized = + typeof newValues === "string" ? newValues : stringifyFilter(newValues); + + searchParams.set(useFilterParamsKey, serialized); setSearchParams(searchParams); - if (onUpdate) { - onUpdate(); + + if (onUpdate !== undefined) { + onUpdate(serialized); } }; const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction( - (values: string | FilterValues) => update(values), + update, 500, ); - const used = query !== "" && query !== initialValue; - return { query, update, debounceUpdate, cancelDebounce, - values, - used, + values: parseFilterQuery(query), + used: query !== "" && query !== fallbackFilter, }; }; diff --git a/site/src/hooks/hookPolyfills.test.ts b/site/src/hooks/hookPolyfills.test.ts new file mode 100644 index 0000000000000..1831bf6f6d2a1 --- /dev/null +++ b/site/src/hooks/hookPolyfills.test.ts @@ -0,0 +1,48 @@ +import { renderHook } from "@testing-library/react"; +import { useEffectEvent } from "./hookPolyfills"; + +function renderEffectEvent( + callbackArg: (...args: TArgs) => TReturn, +) { + return renderHook( + ({ callback }: { callback: typeof callbackArg }) => { + return useEffectEvent(callback); + }, + { + initialProps: { callback: callbackArg }, + }, + ); +} + +describe(`${useEffectEvent.name}`, () => { + it("Should maintain a stable reference across all renders", () => { + const callback = jest.fn(); + const { result, rerender } = renderEffectEvent(callback); + + const firstResult = result.current; + for (let i = 0; i < 5; i++) { + rerender({ callback }); + } + + expect(result.current).toBe(firstResult); + expect.hasAssertions(); + }); + + it("Should always call the most recent callback passed in", () => { + let value: "A" | "B" | "C" = "A"; + const flipToB = () => { + value = "B"; + }; + + const flipToC = () => { + value = "C"; + }; + + const { result, rerender } = renderEffectEvent(flipToB); + rerender({ callback: flipToC }); + + result.current(); + expect(value).toEqual("C"); + expect.hasAssertions(); + }); +}); diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts new file mode 100644 index 0000000000000..9adfa6b91059f --- /dev/null +++ b/site/src/hooks/hookPolyfills.ts @@ -0,0 +1,37 @@ +/** + * @file For defining DIY versions of official React hooks that have not been + * released yet. + * + * These hooks should be deleted as soon as the official versions are available. + * They do not have the same ESLinter exceptions baked in that the official + * hooks do, especially for dependency arrays. + */ +import { useCallback, useEffect, useRef } from "react"; + +/** + * A DIY version of useEffectEvent. + * + * Works like useCallback, except that it doesn't take a dependency array, and + * always returns out a stable function on every single render. The returned-out + * function is always able to "see" the most up-to-date version of the callback + * passed in. + * + * Should only be used as a last resort when useCallback does not work, but you + * still need to avoid dependency array violations. (e.g., You need an on-mount + * effect, but an external library doesn't give their functions stable + * references, so useEffect/useMemo/useCallback run too often). + * + * @see {@link https://react.dev/reference/react/experimental_useEffectEvent} + */ +export function useEffectEvent( + callback: (...args: TArgs) => TReturn, +) { + const callbackRef = useRef(callback); + useEffect(() => { + callbackRef.current = callback; + }, [callback]); + + return useCallback((...args: TArgs): TReturn => { + return callbackRef.current(...args); + }, []); +} diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index f5f8013963cbc..9bafa75e6d9b0 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -4,7 +4,7 @@ import { useDashboard, useIsWorkspaceActionsEnabled, } from "components/Dashboard/DashboardProvider"; -import { FC, useEffect, useState } from "react"; +import { type FC, useEffect, useState, useSyncExternalStore } from "react"; import { Helmet } from "react-helmet-async"; import { pageTitle } from "utils/page"; import { useWorkspacesData, useWorkspaceUpdate } from "./data"; @@ -21,15 +21,34 @@ import { MONOSPACE_FONT_FAMILY } from "theme/constants"; import TextField from "@mui/material/TextField"; import { displayError } from "components/GlobalSnackbar/utils"; import { getErrorMessage } from "api/errors"; +import { useEffectEvent } from "hooks/hookPolyfills"; + +function useSafeSearchParams() { + // Have to wrap setSearchParams because React Router doesn't make sure that + // the function's memory reference stays stable on each render, even though + // its logic never changes, and even though it has function update support + const [searchParams, setSearchParams] = useSearchParams(); + const stableSetSearchParams = useEffectEvent(setSearchParams); + + // Need this to be a tuple type, but can't use "as const", because that would + // make the whole array readonly and cause type mismatches downstream + return [searchParams, stableSetSearchParams] as ReturnType< + typeof useSearchParams + >; +} const WorkspacesPage: FC = () => { const [dormantWorkspaces, setDormantWorkspaces] = useState([]); // If we use a useSearchParams for each hook, the values will not be in sync. // So we have to use a single one, centralizing the values, and pass it to // each hook. - const searchParamsResult = useSearchParams(); + const searchParamsResult = useSafeSearchParams(); const pagination = usePagination({ searchParamsResult }); - const filterProps = useWorkspacesFilter({ searchParamsResult, pagination }); + const filterProps = useWorkspacesFilter({ + searchParamsResult, + onFilterChange: () => pagination.goToPage(1), + }); + const { data, error, queryKey, refetch } = useWorkspacesData({ ...pagination, query: filterProps.filter.query, @@ -121,20 +140,58 @@ const WorkspacesPage: FC = () => { export default WorkspacesPage; +const workspaceFilterKey = "WorkspacesPage/filter"; +const defaultWorkspaceFilter = "owner:me"; + +// Function should stay outside components as much as possible; if declared +// inside the component, React would add/remove event listeners every render +function subscribeToFilterChanges(notifyReact: () => void) { + const onStorageChange = (event: StorageEvent) => { + const { key, storageArea, oldValue, newValue } = event; + + const shouldNotify = + key === workspaceFilterKey && + storageArea === window.localStorage && + newValue !== oldValue; + + if (shouldNotify) { + notifyReact(); + } + }; + + window.addEventListener("storage", onStorageChange); + return () => window.removeEventListener("storage", onStorageChange); +} + type UseWorkspacesFilterOptions = { searchParamsResult: ReturnType; - pagination: ReturnType; + onFilterChange: () => void; }; const useWorkspacesFilter = ({ searchParamsResult, - pagination, + onFilterChange, }: UseWorkspacesFilterOptions) => { + // Using useSyncExternalStore store to safely access localStorage from the + // first render; both snapshot callbacks return primitives, so no special + // trickery needed to prevent hook from immediately blowing up in dev mode + const localStorageFilter = useSyncExternalStore( + subscribeToFilterChanges, + () => { + return ( + window.localStorage.getItem(workspaceFilterKey) ?? + defaultWorkspaceFilter + ); + }, + () => defaultWorkspaceFilter, + ); + const filter = useFilter({ - initialValue: `owner:me`, + fallbackFilter: localStorageFilter, searchParamsResult, - onUpdate: () => { - pagination.goToPage(1); + onUpdate: (newValues) => { + window.localStorage.setItem(workspaceFilterKey, newValues); + onFilterChange(); }, });