From 2f92f863d235a9be12f870690bf8260fcf78e9d2 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 18 Sep 2023 17:45:15 +0000 Subject: [PATCH 01/15] minor: Add useEffectEvent polyfill --- site/src/hooks/hookPolyfills.ts | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 site/src/hooks/hookPolyfills.ts diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts new file mode 100644 index 0000000000000..5744821fcad2b --- /dev/null +++ b/site/src/hooks/hookPolyfills.ts @@ -0,0 +1,35 @@ +/** + * @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 also do have the same ESLinter exceptions baked in that the official + * hooks do. + */ +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 where useCallback does not work, and you + * still need to avoid dependency array violations. + * + * @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); + }, []); +} From e2d3a67244d93448343395724646033f3d0ec2b7 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 18 Sep 2023 17:45:59 +0000 Subject: [PATCH 02/15] chore: update filter to have better callback support --- site/src/components/Filter/filter.tsx | 59 ++++++++++++++++++++------- 1 file changed, 44 insertions(+), 15 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index fe8d2656e40ad..458b552817908 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,28 +35,53 @@ export type PresetFilter = { type FilterValues = Record; +type UseFilterConfig = { + initialValue?: string | (() => string); + searchParamsResult: ReturnType; + onUpdate?: (newValue: string) => void; +}; + +const useFilterParamsKey = "filter"; + export const useFilter = ({ initialValue = "", onUpdate, searchParamsResult, -}: { - initialValue?: string; - searchParamsResult: ReturnType; - onUpdate?: () => void; -}) => { +}: UseFilterConfig) => { + // Fully expect the initialValue functions to have some impurity (e.g. reading + // from localStorage during a render path). (Ab)using useState's lazy + // initialization mode to guarantee impurities only exist on mount. Pattern + // has added benefit of locking down initialValue and ignoring any accidental + // value changes on re-renders + const [readonlyInitialQueryState] = useState(initialValue); + + // React Router doesn't give setSearchParams a stable memory reference; need + // extra logic to prevent on-mount effect from running too often const [searchParams, setSearchParams] = searchParamsResult; - const query = searchParams.get("filter") ?? initialValue; - const values = parseFilterQuery(query); + const syncSearchParamsOnMount = useEffectEvent(() => { + setSearchParams((current) => { + const currentFilter = current.get(useFilterParamsKey); + if (currentFilter !== readonlyInitialQueryState) { + current.set(useFilterParamsKey, readonlyInitialQueryState); + } - const update = (values: string | FilterValues) => { - if (typeof values === "string") { - searchParams.set("filter", values); - } else { - searchParams.set("filter", stringifyFilter(values)); - } + return current; + }); + }); + + useEffect(() => { + syncSearchParamsOnMount(); + }, [syncSearchParamsOnMount]); + + 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); } }; @@ -63,6 +90,8 @@ export const useFilter = ({ 500, ); + const query = searchParams.get("filter") ?? readonlyInitialQueryState; + const values = parseFilterQuery(query); const used = query !== "" && query !== initialValue; return { From eeb3083f976e2ed6f224d007a7671326f986aa37 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 18 Sep 2023 17:49:45 +0000 Subject: [PATCH 03/15] docs: Clean up comments --- site/src/hooks/hookPolyfills.ts | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts index 5744821fcad2b..89057cf7641f6 100644 --- a/site/src/hooks/hookPolyfills.ts +++ b/site/src/hooks/hookPolyfills.ts @@ -3,8 +3,8 @@ * released yet. * * These hooks should be deleted as soon as the official versions are available. - * They also do have the same ESLinter exceptions baked in that the official - * hooks do. + * 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"; @@ -16,8 +16,10 @@ import { useCallback, useEffect, useRef } from "react"; * 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 where useCallback does not work, and you - * still need to avoid dependency array violations. + * Should only be used as a last resort where 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, causing useEffect to run too often). * * @see {@link https://react.dev/reference/react/experimental_useEffectEvent} */ From 553ce890f7c38080fae044263cdabeac99eef310 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 18 Sep 2023 17:51:08 +0000 Subject: [PATCH 04/15] fix: add localStorage to useWorkspacesFilter --- .../src/pages/WorkspacesPage/WorkspacesPage.tsx | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index f5f8013963cbc..6461034cfe066 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -126,14 +126,27 @@ type UseWorkspacesFilterOptions = { pagination: ReturnType; }; +const filterPreferencesKey = "WorkspacesPage/filterPreferences"; + const useWorkspacesFilter = ({ searchParamsResult, pagination, }: UseWorkspacesFilterOptions) => { const filter = useFilter({ - initialValue: `owner:me`, searchParamsResult, - onUpdate: () => { + initialValue: () => { + const fallbackValue = "owner:me"; + + // Have to include check because initialValue will be called during the + // render itself; future-proofing for SSR, if we ever need that + if (typeof window === "undefined") { + return fallbackValue; + } + + return window.localStorage.getItem(filterPreferencesKey) ?? fallbackValue; + }, + onUpdate: (newValues) => { + window.localStorage.setItem(filterPreferencesKey, newValues); pagination.goToPage(1); }, }); From b06d77218b4bef58915fe49e9a198f83ba787d37 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 18 Sep 2023 18:00:59 +0000 Subject: [PATCH 05/15] refactor: Centralize stable useSearchParams --- site/src/components/Filter/filter.tsx | 16 ++++++---------- .../src/pages/WorkspacesPage/WorkspacesPage.tsx | 17 ++++++++++++++++- 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 458b552817908..ec7be4ce7365c 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -26,7 +26,6 @@ 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; @@ -48,6 +47,8 @@ export const useFilter = ({ onUpdate, searchParamsResult, }: UseFilterConfig) => { + const [searchParams, setSearchParams] = searchParamsResult; + // Fully expect the initialValue functions to have some impurity (e.g. reading // from localStorage during a render path). (Ab)using useState's lazy // initialization mode to guarantee impurities only exist on mount. Pattern @@ -55,10 +56,9 @@ export const useFilter = ({ // value changes on re-renders const [readonlyInitialQueryState] = useState(initialValue); - // React Router doesn't give setSearchParams a stable memory reference; need - // extra logic to prevent on-mount effect from running too often - const [searchParams, setSearchParams] = searchParamsResult; - const syncSearchParamsOnMount = useEffectEvent(() => { + // Sync the params with the value provided via the initialValue function; + // should behave only as an on-mount effect + useEffect(() => { setSearchParams((current) => { const currentFilter = current.get(useFilterParamsKey); if (currentFilter !== readonlyInitialQueryState) { @@ -67,11 +67,7 @@ export const useFilter = ({ return current; }); - }); - - useEffect(() => { - syncSearchParamsOnMount(); - }, [syncSearchParamsOnMount]); + }, [setSearchParams, readonlyInitialQueryState]); const update = (newValues: string | FilterValues) => { const serialized = diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 6461034cfe066..8f4c4fae2ceb7 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -21,13 +21,28 @@ 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 it even 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 + 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 { data, error, queryKey, refetch } = useWorkspacesData({ From df1f8df0ab9d5bb157432d76cef20a114c5d4f4f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 18 Sep 2023 20:12:06 +0000 Subject: [PATCH 06/15] refactor: clean up filter to be fully pure on mount --- site/src/components/Filter/filter.tsx | 40 +++++++++++++------ .../pages/WorkspacesPage/WorkspacesPage.tsx | 3 -- 2 files changed, 27 insertions(+), 16 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index ec7be4ce7365c..7ea03c2fff5c4 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -35,6 +35,14 @@ export type PresetFilter = { type FilterValues = Record; type UseFilterConfig = { + /** + * If initialValue is a string, that value will be used immediately from the + * first render. + * + * If it's a function, the function will be lazy-evaluated via an effect + * after the initial render. The initial render will use an empty string while + * waiting for the effect to run. + */ initialValue?: string | (() => string); searchParamsResult: ReturnType; onUpdate?: (newValue: string) => void; @@ -49,25 +57,31 @@ export const useFilter = ({ }: UseFilterConfig) => { const [searchParams, setSearchParams] = searchParamsResult; - // Fully expect the initialValue functions to have some impurity (e.g. reading - // from localStorage during a render path). (Ab)using useState's lazy - // initialization mode to guarantee impurities only exist on mount. Pattern - // has added benefit of locking down initialValue and ignoring any accidental - // value changes on re-renders - const [readonlyInitialQueryState] = useState(initialValue); + // Copying initialValue into state to lock the value down from the outside + const [initializedValue, setInitializedValue] = useState(() => { + return typeof initialValue === "string" ? initialValue : ""; + }); - // Sync the params with the value provided via the initialValue function; - // should behave only as an on-mount effect + // Lazy-evaluate initialValue only on mount; have to resolve via effect + // because initialValue is allowed to be impure (e.g., read from localStorage) + const lazyOnMountRef = useRef(initialValue); useEffect(() => { + if (typeof lazyOnMountRef.current === "string") { + return; + } + + const lazyInitialValue = lazyOnMountRef.current(); + setInitializedValue(lazyInitialValue); + setSearchParams((current) => { const currentFilter = current.get(useFilterParamsKey); - if (currentFilter !== readonlyInitialQueryState) { - current.set(useFilterParamsKey, readonlyInitialQueryState); + if (currentFilter !== lazyInitialValue) { + current.set(useFilterParamsKey, lazyInitialValue); } return current; }); - }, [setSearchParams, readonlyInitialQueryState]); + }, [setSearchParams]); const update = (newValues: string | FilterValues) => { const serialized = @@ -82,11 +96,11 @@ export const useFilter = ({ }; const { debounced: debounceUpdate, cancelDebounce } = useDebouncedFunction( - (values: string | FilterValues) => update(values), + update, 500, ); - const query = searchParams.get("filter") ?? readonlyInitialQueryState; + const query = searchParams.get("filter") ?? initializedValue; const values = parseFilterQuery(query); const used = query !== "" && query !== initialValue; diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 8f4c4fae2ceb7..90c55562849e5 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -151,9 +151,6 @@ const useWorkspacesFilter = ({ searchParamsResult, initialValue: () => { const fallbackValue = "owner:me"; - - // Have to include check because initialValue will be called during the - // render itself; future-proofing for SSR, if we ever need that if (typeof window === "undefined") { return fallbackValue; } From 29656c16702841d0497eed8e3540a1925d4f18ac Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 19 Sep 2023 16:16:08 +0000 Subject: [PATCH 07/15] chore: add tests for useEffectEvent --- site/src/hooks/hookPolyfills.test.ts | 48 ++++++++++++++++++++++++++++ site/src/hooks/hookPolyfills.ts | 4 +-- 2 files changed, 50 insertions(+), 2 deletions(-) create mode 100644 site/src/hooks/hookPolyfills.test.ts 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 index 89057cf7641f6..9adfa6b91059f 100644 --- a/site/src/hooks/hookPolyfills.ts +++ b/site/src/hooks/hookPolyfills.ts @@ -16,10 +16,10 @@ import { useCallback, useEffect, useRef } from "react"; * 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 where useCallback does not work, but you + * 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, causing useEffect to run too often). + * references, so useEffect/useMemo/useCallback run too often). * * @see {@link https://react.dev/reference/react/experimental_useEffectEvent} */ From 54a5940e51c9ba9e2fa3a41717b1a3a0549231f2 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 19 Sep 2023 18:05:52 +0000 Subject: [PATCH 08/15] wip: commit progress for searchbar fix --- site/src/components/Filter/filter.tsx | 44 ++---------- .../pages/WorkspacesPage/WorkspacesPage.tsx | 67 ++++++++++++++----- 2 files changed, 58 insertions(+), 53 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 7ea03c2fff5c4..6a41351d79d72 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -36,14 +36,10 @@ type FilterValues = Record; type UseFilterConfig = { /** - * If initialValue is a string, that value will be used immediately from the - * first render. - * - * If it's a function, the function will be lazy-evaluated via an effect - * after the initial render. The initial render will use an empty string while - * waiting for the effect to run. + * The fallback value to use in the event that no filter params can be used. + * This value is allowed to change on re-renders. */ - initialValue?: string | (() => string); + initialValue?: string; searchParamsResult: ReturnType; onUpdate?: (newValue: string) => void; }; @@ -51,38 +47,12 @@ type UseFilterConfig = { const useFilterParamsKey = "filter"; export const useFilter = ({ - initialValue = "", - onUpdate, + initialValue: fallbackFilter = "", searchParamsResult, + onUpdate, }: UseFilterConfig) => { const [searchParams, setSearchParams] = searchParamsResult; - // Copying initialValue into state to lock the value down from the outside - const [initializedValue, setInitializedValue] = useState(() => { - return typeof initialValue === "string" ? initialValue : ""; - }); - - // Lazy-evaluate initialValue only on mount; have to resolve via effect - // because initialValue is allowed to be impure (e.g., read from localStorage) - const lazyOnMountRef = useRef(initialValue); - useEffect(() => { - if (typeof lazyOnMountRef.current === "string") { - return; - } - - const lazyInitialValue = lazyOnMountRef.current(); - setInitializedValue(lazyInitialValue); - - setSearchParams((current) => { - const currentFilter = current.get(useFilterParamsKey); - if (currentFilter !== lazyInitialValue) { - current.set(useFilterParamsKey, lazyInitialValue); - } - - return current; - }); - }, [setSearchParams]); - const update = (newValues: string | FilterValues) => { const serialized = typeof newValues === "string" ? newValues : stringifyFilter(newValues); @@ -100,9 +70,9 @@ export const useFilter = ({ 500, ); - const query = searchParams.get("filter") ?? initializedValue; + const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter; const values = parseFilterQuery(query); - const used = query !== "" && query !== initialValue; + const used = query !== "" && query !== fallbackFilter; return { query, diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 90c55562849e5..7e8434a5fa0c5 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"; @@ -44,7 +44,11 @@ const WorkspacesPage: FC = () => { // each hook. const searchParamsResult = useSafeSearchParams(); const pagination = usePagination({ searchParamsResult }); - const filterProps = useWorkspacesFilter({ searchParamsResult, pagination }); + const filterProps = useWorkspacesFilter({ + searchParamsResult, + onPageChange: () => pagination.goToPage(1), + }); + const { data, error, queryKey, refetch } = useWorkspacesData({ ...pagination, query: filterProps.filter.query, @@ -136,30 +140,61 @@ 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; + onPageChange: () => void; }; -const filterPreferencesKey = "WorkspacesPage/filterPreferences"; - const useWorkspacesFilter = ({ searchParamsResult, - pagination, + onPageChange, }: 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({ + /** + * @todo Rename initialValue to fallbackFilter once changes have been tested + */ + initialValue: localStorageFilter, searchParamsResult, - initialValue: () => { - const fallbackValue = "owner:me"; - if (typeof window === "undefined") { - return fallbackValue; - } - - return window.localStorage.getItem(filterPreferencesKey) ?? fallbackValue; - }, onUpdate: (newValues) => { - window.localStorage.setItem(filterPreferencesKey, newValues); - pagination.goToPage(1); + window.localStorage.setItem(workspaceFilterKey, newValues); + onPageChange(); }, }); From ae5b3592f5539e036bf50e5f6447ee3fb9e13220 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 19 Sep 2023 18:38:48 +0000 Subject: [PATCH 09/15] chore: clean up WorkspacesPage --- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index 7e8434a5fa0c5..f746dbd1e2175 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -26,12 +26,12 @@ 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 it even has function update support + // 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 + // make the whole array readonly and cause type mismatches downstream return [searchParams, stableSetSearchParams] as ReturnType< typeof useSearchParams >; @@ -46,7 +46,7 @@ const WorkspacesPage: FC = () => { const pagination = usePagination({ searchParamsResult }); const filterProps = useWorkspacesFilter({ searchParamsResult, - onPageChange: () => pagination.goToPage(1), + onFilterChange: () => pagination.goToPage(1), }); const { data, error, queryKey, refetch } = useWorkspacesData({ @@ -165,12 +165,12 @@ function subscribeToFilterChanges(notifyReact: () => void) { type UseWorkspacesFilterOptions = { searchParamsResult: ReturnType; - onPageChange: () => void; + onFilterChange: () => void; }; const useWorkspacesFilter = ({ searchParamsResult, - onPageChange, + onFilterChange, }: UseWorkspacesFilterOptions) => { // Using useSyncExternalStore store to safely access localStorage from the // first render; both snapshot callbacks return primitives, so no special @@ -194,7 +194,7 @@ const useWorkspacesFilter = ({ searchParamsResult, onUpdate: (newValues) => { window.localStorage.setItem(workspaceFilterKey, newValues); - onPageChange(); + onFilterChange(); }, }); From 7dd1b4f6882a6b2590078ac259cbb212ff64f6a4 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 19 Sep 2023 18:39:25 +0000 Subject: [PATCH 10/15] fix: add logic for syncing queries with search params --- site/src/components/Filter/filter.tsx | 33 ++++++++++++++++++++++----- 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 6a41351d79d72..19ec3ceacfb0b 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -52,6 +52,31 @@ export const useFilter = ({ onUpdate, }: UseFilterConfig) => { const [searchParams, setSearchParams] = searchParamsResult; + const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter; + + // Using ref approach to be defensive against useSearchParams not returning a + // stable set function on each render. Effect must run before query effect + const setSearchParamsRef = useRef(setSearchParams); + useEffect(() => { + setSearchParamsRef.current = setSearchParams; + }, [setSearchParams]); + + // Keep params synced with query, even as query changes from outside sources + useEffect(() => { + const setSearchParams = setSearchParamsRef.current; + + setSearchParams((currentParams) => { + const currentQuery = currentParams.get(useFilterParamsKey); + + if (query === "") { + currentParams.delete(useFilterParamsKey); + } else if (currentQuery !== query) { + currentParams.set(useFilterParamsKey, query); + } + + return currentParams; + }); + }, [query]); const update = (newValues: string | FilterValues) => { const serialized = @@ -70,17 +95,13 @@ export const useFilter = ({ 500, ); - const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter; - const values = parseFilterQuery(query); - const used = query !== "" && query !== fallbackFilter; - return { query, update, debounceUpdate, cancelDebounce, - values, - used, + values: parseFilterQuery(query), + used: query !== "" && query !== fallbackFilter, }; }; From ab9de0cc31bf8d45bf325e8f70dae6269c9bb806 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 19 Sep 2023 18:50:00 +0000 Subject: [PATCH 11/15] chore: Rename initialValue to fallbackFilter --- site/src/components/Filter/filter.tsx | 9 +++++---- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 19ec3ceacfb0b..e23bf3f24ffed 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -36,10 +36,11 @@ type FilterValues = Record; type UseFilterConfig = { /** - * The fallback value to use in the event that no filter params can be used. - * This value is allowed to change on re-renders. + * 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. */ - initialValue?: string; + fallbackFilter?: string; searchParamsResult: ReturnType; onUpdate?: (newValue: string) => void; }; @@ -47,7 +48,7 @@ type UseFilterConfig = { const useFilterParamsKey = "filter"; export const useFilter = ({ - initialValue: fallbackFilter = "", + fallbackFilter = "", searchParamsResult, onUpdate, }: UseFilterConfig) => { diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index f746dbd1e2175..c40c55aa3650b 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -190,7 +190,7 @@ const useWorkspacesFilter = ({ /** * @todo Rename initialValue to fallbackFilter once changes have been tested */ - initialValue: localStorageFilter, + fallbackFilter: localStorageFilter, searchParamsResult, onUpdate: (newValues) => { window.localStorage.setItem(workspaceFilterKey, newValues); From 391648a42e75c109f86b35bcc60001b6292d33a6 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 19 Sep 2023 18:55:08 +0000 Subject: [PATCH 12/15] chore: Remove todo comment --- site/src/pages/WorkspacesPage/WorkspacesPage.tsx | 3 --- 1 file changed, 3 deletions(-) diff --git a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx index c40c55aa3650b..9bafa75e6d9b0 100644 --- a/site/src/pages/WorkspacesPage/WorkspacesPage.tsx +++ b/site/src/pages/WorkspacesPage/WorkspacesPage.tsx @@ -187,9 +187,6 @@ const useWorkspacesFilter = ({ ); const filter = useFilter({ - /** - * @todo Rename initialValue to fallbackFilter once changes have been tested - */ fallbackFilter: localStorageFilter, searchParamsResult, onUpdate: (newValues) => { From dfad46f54c4151239798d29a62567a1c73dc738b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 19:42:36 +0000 Subject: [PATCH 13/15] refactor: update code to use useEffectEvent --- site/src/components/Filter/filter.tsx | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index e23bf3f24ffed..2322818433bfa 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -26,6 +26,7 @@ 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; @@ -55,18 +56,13 @@ export const useFilter = ({ const [searchParams, setSearchParams] = searchParamsResult; const query = searchParams.get(useFilterParamsKey) ?? fallbackFilter; - // Using ref approach to be defensive against useSearchParams not returning a - // stable set function on each render. Effect must run before query effect - const setSearchParamsRef = useRef(setSearchParams); - useEffect(() => { - setSearchParamsRef.current = setSearchParams; - }, [setSearchParams]); + // Stabilizing reference to setSearchParams from one central spot, just to be + // on the extra careful side. Don't want effects over-running + const stableSetSearchParams = useEffectEvent(setSearchParams); // Keep params synced with query, even as query changes from outside sources useEffect(() => { - const setSearchParams = setSearchParamsRef.current; - - setSearchParams((currentParams) => { + stableSetSearchParams((currentParams) => { const currentQuery = currentParams.get(useFilterParamsKey); if (query === "") { @@ -77,7 +73,7 @@ export const useFilter = ({ return currentParams; }); - }, [query]); + }, [stableSetSearchParams, query]); const update = (newValues: string | FilterValues) => { const serialized = From dcd8691f72374df0e95568281ab6d2f946d5b5dd Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 20:26:46 +0000 Subject: [PATCH 14/15] docs: clean up comments for clarity --- site/src/components/Filter/filter.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 2322818433bfa..4ed99cb875037 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -57,7 +57,8 @@ export const useFilter = ({ 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 + // 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 From 3a7efd58f6b3fa528e99d62531247c72c4e2a385 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 20 Sep 2023 20:33:04 +0000 Subject: [PATCH 15/15] fix: update url check to use regex --- site/e2e/global.setup.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) 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 }); });