From b4211f389e3294bed32437f1ecdd3697facd8e4e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 13 Nov 2023 21:52:37 +0000 Subject: [PATCH 01/63] wip: commit current progress on usePaginatedQuery --- .../PaginationWidget/usePaginatedQuery.ts | 126 ++++++++++++++++++ 1 file changed, 126 insertions(+) create mode 100644 site/src/components/PaginationWidget/usePaginatedQuery.ts diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts new file mode 100644 index 0000000000000..51169783b0369 --- /dev/null +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -0,0 +1,126 @@ +import { useEffect } from "react"; +import { useSearchParams } from "react-router-dom"; +import { useEffectEvent } from "hooks/hookPolyfills"; +import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; + +import { + type QueryKey, + type UseQueryOptions, + useQueryClient, + useQuery, +} from "react-query"; + +const PAGE_PARAMS_KEY = "page"; + +// Any JSON-serializable object with a count property representing the total +// number of records for a given resource +type PaginatedData = { + count: number; +}; + +// All the type parameters just mirror the ones used by React Query +type PaginatedOptions< + TQueryFnData extends PaginatedData = PaginatedData, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Omit< + UseQueryOptions, + "keepPreviousData" | "queryKey" +> & { + /** + * A function that takes a page number and produces a full query key. Must be + * a function so that it can be used for the active query, as well as + * prefetching + */ + queryKey: (pageNumber: number) => TQueryKey; + + searchParamsResult: ReturnType; + prefetchNextPage?: boolean; +}; + +export function usePaginatedQuery< + TQueryFnData extends PaginatedData = PaginatedData, + TError = unknown, + TData extends PaginatedData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +>(options: PaginatedOptions) { + const { searchParamsResult, queryKey, prefetchNextPage = false } = options; + const [searchParams, setSearchParams] = searchParamsResult; + const currentPage = parsePage(searchParams); + + // Can't use useInfiniteQuery because that hook is designed to work with data + // that streams in and can't be cut up into pages easily + const query = useQuery({ + ...options, + queryKey: queryKey(currentPage), + keepPreviousData: true, + }); + + const pageSize = DEFAULT_RECORDS_PER_PAGE; + const pageOffset = (currentPage - 1) * pageSize; + const totalRecords = query.data?.count ?? 0; + const totalPages = Math.ceil(totalRecords / pageSize); + const hasPreviousPage = currentPage > 1; + const hasNextPage = pageSize * pageOffset < totalRecords; + + const queryClient = useQueryClient(); + const prefetch = useEffectEvent((newPage: number) => { + if (!prefetchNextPage) { + return; + } + + const newKey = queryKey(newPage); + void queryClient.prefetchQuery(newKey); + }); + + useEffect(() => { + if (hasPreviousPage) { + prefetch(currentPage - 1); + } + + if (hasNextPage) { + prefetch(currentPage + 1); + } + }, [prefetch, currentPage, hasNextPage, hasPreviousPage]); + + // Tries to redirect a user if they navigate to a page via invalid URL + const navigateIfInvalidPage = useEffectEvent( + (currentPage: number, totalPages: number) => { + const clamped = Math.max(1, Math.min(currentPage, totalPages)); + + if (currentPage !== clamped) { + searchParams.set(PAGE_PARAMS_KEY, String(clamped)); + setSearchParams(searchParams); + } + }, + ); + + useEffect(() => { + navigateIfInvalidPage(currentPage, totalPages); + }, [navigateIfInvalidPage, currentPage, totalPages]); + + const onPageChange = (newPage: number) => { + const safePage = Number.isInteger(newPage) + ? Math.max(1, Math.min(newPage)) + : 1; + + searchParams.set(PAGE_PARAMS_KEY, String(safePage)); + setSearchParams(searchParams); + }; + + return { + ...query, + onPageChange, + currentPage, + totalRecords, + hasNextPage, + pageSize, + isLoading: query.isLoading || query.isFetching, + } as const; +} + +function parsePage(params: URLSearchParams): number { + const parsed = Number(params.get("page")); + return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; +} From 173e823bf3e060c3e3af2fb2e814904b760b4b08 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 13 Nov 2023 21:55:44 +0000 Subject: [PATCH 02/63] chore: add cacheTime to users query --- site/src/api/queries/users.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 2b6900df13ac8..991cd4ab06aa7 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -15,6 +15,7 @@ export const users = (req: UsersRequest): UseQueryOptions => { return { queryKey: ["users", req], queryFn: ({ signal }) => API.getUsers(req, signal), + cacheTime: 5 * 60 * 1000, }; }; From d715d738850e0a17a62f3cf5c08e50876ef653a8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 13 Nov 2023 21:56:21 +0000 Subject: [PATCH 03/63] chore: update cache logic for UsersPage usersQuery --- site/src/pages/UsersPage/UsersPage.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index dd60039056f02..83f271cbe089b 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -44,13 +44,14 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { const [searchParams] = searchParamsResult; const pagination = usePagination({ searchParamsResult }); - const usersQuery = useQuery( - users({ + const usersQuery = useQuery({ + ...users({ q: prepareQuery(searchParams.get("filter") ?? ""), limit: pagination.limit, offset: pagination.offset, }), - ); + keepPreviousData: true, + }); const organizationId = useOrganizationId(); const groupsByUserIdQuery = useQuery(groupsByUserId(organizationId)); From 8a199b737785442ad74df344024c52fe0fa56bf5 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 13 Nov 2023 21:56:38 +0000 Subject: [PATCH 04/63] wip: commit progress on Pagination --- .../PaginationWidget/Pagination.tsx | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 site/src/components/PaginationWidget/Pagination.tsx diff --git a/site/src/components/PaginationWidget/Pagination.tsx b/site/src/components/PaginationWidget/Pagination.tsx new file mode 100644 index 0000000000000..7fc882345f316 --- /dev/null +++ b/site/src/components/PaginationWidget/Pagination.tsx @@ -0,0 +1,63 @@ +import { type PropsWithChildren, useEffect, useRef } from "react"; +import { PaginationWidgetBase } from "./PaginationWidgetBase"; + +type PaginationProps = PropsWithChildren<{ + fetching: boolean; + currentPage: number; + pageSize: number; + totalRecords: number; + onPageChange: (newPage: number) => void; +}>; + +export function Pagination({ + children, + fetching, + currentPage, + totalRecords, + pageSize, + onPageChange, +}: PaginationProps) { + const scrollAfterPageChangeRef = useRef(false); + useEffect(() => { + const onScroll = () => { + scrollAfterPageChangeRef.current = false; + }; + + document.addEventListener("scroll", onScroll); + return () => document.removeEventListener("scroll", onScroll); + }, []); + + const previousPageRef = useRef(undefined); + const paginationTopRef = useRef(null); + useEffect(() => { + const paginationTop = paginationTopRef.current; + const isInitialRender = previousPageRef.current === undefined; + + const skipScroll = + isInitialRender || + paginationTop === null || + !scrollAfterPageChangeRef.current; + + previousPageRef.current = currentPage; + if (!skipScroll) { + paginationTop.scrollIntoView(); + } + }, [currentPage]); + + return ( + <> +
+ {children} + + { + scrollAfterPageChangeRef.current = true; + onPageChange(newPage); + }} + /> + + ); +} From 98ae96d46cd97cb8b65d6be96115e0bc74c72aad Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 13 Nov 2023 23:28:17 +0000 Subject: [PATCH 05/63] chore: add function overloads to prepareQuery --- site/src/utils/filters.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/site/src/utils/filters.ts b/site/src/utils/filters.ts index beb850a65e218..389b866d0e111 100644 --- a/site/src/utils/filters.ts +++ b/site/src/utils/filters.ts @@ -1,3 +1,5 @@ -export const prepareQuery = (query?: string) => { +export function prepareQuery(query: undefined): undefined; +export function prepareQuery(query: string): string; +export function prepareQuery(query?: string): string | undefined { return query?.trim().replace(/ +/g, " "); -}; +} From 37ea50b42b88fa32ebdbc329fbdacde3b50d4165 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 14 Nov 2023 22:45:27 +0000 Subject: [PATCH 06/63] wip: commit progress on usePaginatedQuery --- .../PaginationWidget/usePaginatedQuery.ts | 156 ++++++++++++------ 1 file changed, 108 insertions(+), 48 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 51169783b0369..975aa25219793 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -1,9 +1,14 @@ import { useEffect } from "react"; import { useSearchParams } from "react-router-dom"; import { useEffectEvent } from "hooks/hookPolyfills"; + +import { type Pagination } from "api/typesGenerated"; import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; +import { prepareQuery } from "utils/filters"; +import { clamp } from "lodash"; import { + type QueryFunction, type QueryKey, type UseQueryOptions, useQueryClient, @@ -11,15 +16,30 @@ import { } from "react-query"; const PAGE_PARAMS_KEY = "page"; +const PAGE_FILTER_KEY = "filter"; + +// Only omitting after_id for simplifying initial implementation; property +// should probably be added back in down the line +type PaginationInput = Omit & { + q: string; + limit: number; + offset: number; +}; -// Any JSON-serializable object with a count property representing the total -// number of records for a given resource -type PaginatedData = { +/** + * Any JSON-serializable object returned by the API that exposes the total + * number of records that match a query + */ +interface PaginatedData { count: number; -}; +} +/** + * A more specialized version of UseQueryOptions built specifically for + * paginated queries. + */ // All the type parameters just mirror the ones used by React Query -type PaginatedOptions< +export type UsePaginatedQueryOptions< TQueryFnData extends PaginatedData = PaginatedData, TError = unknown, TData = TQueryFnData, @@ -28,15 +48,21 @@ type PaginatedOptions< UseQueryOptions, "keepPreviousData" | "queryKey" > & { + prefetch?: boolean; + /** - * A function that takes a page number and produces a full query key. Must be - * a function so that it can be used for the active query, as well as - * prefetching + * A function that takes pagination information and produces a full query key. + * + * Must be a function so that it can be used for the active query, as well as + * any prefetching. */ - queryKey: (pageNumber: number) => TQueryKey; + queryKey: (pagination: PaginationInput) => TQueryKey; - searchParamsResult: ReturnType; - prefetchNextPage?: boolean; + /** + * A version of queryFn that is required and that has access to the current + * page via the pageParams context property + */ + queryFn: QueryFunction; }; export function usePaginatedQuery< @@ -44,65 +70,81 @@ export function usePaginatedQuery< TError = unknown, TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, ->(options: PaginatedOptions) { - const { searchParamsResult, queryKey, prefetchNextPage = false } = options; - const [searchParams, setSearchParams] = searchParamsResult; +>(options: UsePaginatedQueryOptions) { + const { queryKey, queryFn, prefetch = false, ...otherOptions } = options; + + const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePage(searchParams); - // Can't use useInfiniteQuery because that hook is designed to work with data - // that streams in and can't be cut up into pages easily + const pageSize = DEFAULT_RECORDS_PER_PAGE; + const pageOffset = (currentPage - 1) * pageSize; + const query = useQuery({ - ...options, - queryKey: queryKey(currentPage), + ...otherOptions, + queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: currentPage }), + queryKey: queryKey({ + q: preparePageQuery(searchParams, currentPage), + limit: pageSize, + offset: pageOffset, + }), keepPreviousData: true, }); - const pageSize = DEFAULT_RECORDS_PER_PAGE; - const pageOffset = (currentPage - 1) * pageSize; - const totalRecords = query.data?.count ?? 0; - const totalPages = Math.ceil(totalRecords / pageSize); - const hasPreviousPage = currentPage > 1; - const hasNextPage = pageSize * pageOffset < totalRecords; - const queryClient = useQueryClient(); - const prefetch = useEffectEvent((newPage: number) => { - if (!prefetchNextPage) { + const prefetchPage = useEffectEvent((newPage: number) => { + if (!prefetch) { return; } - const newKey = queryKey(newPage); - void queryClient.prefetchQuery(newKey); + void queryClient.prefetchQuery({ + queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: newPage }), + queryKey: queryKey({ + q: preparePageQuery(searchParams, newPage), + limit: pageSize, + offset: pageOffset, + }), + }); }); + const totalRecords = query.data?.count ?? 0; + const totalPages = Math.ceil(totalRecords / pageSize); + const hasNextPage = pageSize * pageOffset < totalRecords; + const hasPreviousPage = currentPage > 1; + + // Have to split hairs and sync on both the current page and the hasXPage + // variables because hasXPage values are derived from server values and aren't + // immediately ready on each render useEffect(() => { - if (hasPreviousPage) { - prefetch(currentPage - 1); + if (hasNextPage) { + prefetchPage(currentPage + 1); } + }, [prefetchPage, currentPage, hasNextPage]); - if (hasNextPage) { - prefetch(currentPage + 1); + useEffect(() => { + if (hasPreviousPage) { + prefetchPage(currentPage - 1); } - }, [prefetch, currentPage, hasNextPage, hasPreviousPage]); + }, [prefetchPage, currentPage, hasPreviousPage]); - // Tries to redirect a user if they navigate to a page via invalid URL - const navigateIfInvalidPage = useEffectEvent( - (currentPage: number, totalPages: number) => { - const clamped = Math.max(1, Math.min(currentPage, totalPages)); + // Mainly here to catch user if they navigate to a page directly via URL + const updatePageIfInvalid = useEffectEvent(() => { + const clamped = clamp(currentPage, 1, totalPages); - if (currentPage !== clamped) { - searchParams.set(PAGE_PARAMS_KEY, String(clamped)); - setSearchParams(searchParams); - } - }, - ); + if (currentPage !== clamped) { + searchParams.set(PAGE_PARAMS_KEY, String(clamped)); + setSearchParams(searchParams); + } + }); useEffect(() => { - navigateIfInvalidPage(currentPage, totalPages); - }, [navigateIfInvalidPage, currentPage, totalPages]); + if (!query.isFetching) { + updatePageIfInvalid(); + } + }, [updatePageIfInvalid, query.isFetching]); const onPageChange = (newPage: number) => { const safePage = Number.isInteger(newPage) - ? Math.max(1, Math.min(newPage)) + ? clamp(newPage, 1, totalPages) : 1; searchParams.set(PAGE_PARAMS_KEY, String(safePage)); @@ -112,10 +154,13 @@ export function usePaginatedQuery< return { ...query, onPageChange, + goToPreviousPage: () => onPageChange(currentPage - 1), + goToNextPage: () => onPageChange(currentPage + 1), currentPage, + pageSize, totalRecords, hasNextPage, - pageSize, + hasPreviousPage, isLoading: query.isLoading || query.isFetching, } as const; } @@ -124,3 +169,18 @@ function parsePage(params: URLSearchParams): number { const parsed = Number(params.get("page")); return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } + +function preparePageQuery(searchParams: URLSearchParams, page: number) { + const paramsPage = Number(searchParams.get(PAGE_FILTER_KEY)); + + let queryText: string; + if (paramsPage === page) { + queryText = searchParams.toString(); + } else { + const newParams = new URLSearchParams(searchParams); + newParams.set(PAGE_FILTER_KEY, String(page)); + queryText = newParams.toString(); + } + + return prepareQuery(queryText); +} From 2c1e9e31030a8a3846fb9a42ff9e3445dd164d13 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 14 Nov 2023 22:55:52 +0000 Subject: [PATCH 07/63] docs: add clarifying comment about implementation --- site/src/components/PaginationWidget/usePaginatedQuery.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 975aa25219793..3aeb6e99bfd08 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -79,6 +79,8 @@ export function usePaginatedQuery< const pageSize = DEFAULT_RECORDS_PER_PAGE; const pageOffset = (currentPage - 1) * pageSize; + // Not using infinite query right now because that requires a fair bit of list + // virtualization as the lists get bigger (especially for the audit logs) const query = useQuery({ ...otherOptions, queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: currentPage }), From b3a9ab44a03f02a5785974f9ec9cca95697c4157 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 03:58:43 +0000 Subject: [PATCH 08/63] chore: remove optional prefetch property from query options --- .../PaginationWidget/usePaginatedQuery.ts | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 3aeb6e99bfd08..2417eb158d1a8 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -48,8 +48,6 @@ export type UsePaginatedQueryOptions< UseQueryOptions, "keepPreviousData" | "queryKey" > & { - prefetch?: boolean; - /** * A function that takes pagination information and produces a full query key. * @@ -71,8 +69,7 @@ export function usePaginatedQuery< TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, >(options: UsePaginatedQueryOptions) { - const { queryKey, queryFn, prefetch = false, ...otherOptions } = options; - + const { queryKey, queryFn, ...otherOptions } = options; const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePage(searchParams); @@ -94,10 +91,6 @@ export function usePaginatedQuery< const queryClient = useQueryClient(); const prefetchPage = useEffectEvent((newPage: number) => { - if (!prefetch) { - return; - } - void queryClient.prefetchQuery({ queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: newPage }), queryKey: queryKey({ @@ -163,6 +156,10 @@ export function usePaginatedQuery< totalRecords, hasNextPage, hasPreviousPage, + + // Have to hijack the isLoading property slightly because keepPreviousData + // is true; by default, isLoading will be false after the initial page + // loads, even if new pages are loading in isLoading: query.isLoading || query.isFetching, } as const; } From 39a2cedc020e6733256d3151be706b18575712d6 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 04:12:28 +0000 Subject: [PATCH 09/63] chore: redefine queryKey --- .../PaginationWidget/usePaginatedQuery.ts | 59 ++++++++----------- 1 file changed, 23 insertions(+), 36 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 2417eb158d1a8..ba4f85ee06a27 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -2,9 +2,7 @@ import { useEffect } from "react"; import { useSearchParams } from "react-router-dom"; import { useEffectEvent } from "hooks/hookPolyfills"; -import { type Pagination } from "api/typesGenerated"; import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; -import { prepareQuery } from "utils/filters"; import { clamp } from "lodash"; import { @@ -15,15 +13,19 @@ import { useQuery, } from "react-query"; -const PAGE_PARAMS_KEY = "page"; -const PAGE_FILTER_KEY = "filter"; +/** + * The key to use for getting/setting the page number from the search params + */ +const PAGE_NUMBER_PARAMS_KEY = "page"; -// Only omitting after_id for simplifying initial implementation; property -// should probably be added back in down the line -type PaginationInput = Omit & { - q: string; - limit: number; - offset: number; +/** + * All arguments passed into the queryKey functions. + */ +type QueryKeyFnArgs = { + pageNumber: number; + pageSize: number; + pageOffset: number; + extraQuery?: string; }; /** @@ -54,11 +56,11 @@ export type UsePaginatedQueryOptions< * Must be a function so that it can be used for the active query, as well as * any prefetching. */ - queryKey: (pagination: PaginationInput) => TQueryKey; + queryKey: (args: QueryKeyFnArgs) => TQueryKey; /** - * A version of queryFn that is required and that has access to the current - * page via the pageParams context property + * A version of queryFn that is required and that exposes page numbers through + * the pageParams context property */ queryFn: QueryFunction; }; @@ -82,9 +84,9 @@ export function usePaginatedQuery< ...otherOptions, queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: currentPage }), queryKey: queryKey({ - q: preparePageQuery(searchParams, currentPage), - limit: pageSize, - offset: pageOffset, + pageNumber: currentPage, + pageSize, + pageOffset, }), keepPreviousData: true, }); @@ -94,9 +96,9 @@ export function usePaginatedQuery< void queryClient.prefetchQuery({ queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: newPage }), queryKey: queryKey({ - q: preparePageQuery(searchParams, newPage), - limit: pageSize, - offset: pageOffset, + pageNumber: newPage, + pageSize, + pageOffset, }), }); }); @@ -126,7 +128,7 @@ export function usePaginatedQuery< const clamped = clamp(currentPage, 1, totalPages); if (currentPage !== clamped) { - searchParams.set(PAGE_PARAMS_KEY, String(clamped)); + searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); setSearchParams(searchParams); } }); @@ -142,7 +144,7 @@ export function usePaginatedQuery< ? clamp(newPage, 1, totalPages) : 1; - searchParams.set(PAGE_PARAMS_KEY, String(safePage)); + searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(safePage)); setSearchParams(searchParams); }; @@ -168,18 +170,3 @@ function parsePage(params: URLSearchParams): number { const parsed = Number(params.get("page")); return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } - -function preparePageQuery(searchParams: URLSearchParams, page: number) { - const paramsPage = Number(searchParams.get(PAGE_FILTER_KEY)); - - let queryText: string; - if (paramsPage === page) { - queryText = searchParams.toString(); - } else { - const newParams = new URLSearchParams(searchParams); - newParams.set(PAGE_FILTER_KEY, String(page)); - queryText = newParams.toString(); - } - - return prepareQuery(queryText); -} From 4dcfd29e1b824396882f94785dcb10c3c5035cff Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 04:28:09 +0000 Subject: [PATCH 10/63] refactor: consolidate how queryKey/queryFn are called --- site/src/api/queries/users.ts | 13 +++++++ .../PaginationWidget/usePaginatedQuery.ts | 36 ++++++++++--------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 991cd4ab06aa7..40e9afa7479e2 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -10,6 +10,19 @@ import { } from "api/typesGenerated"; import { getAuthorizationKey } from "./authCheck"; import { getMetadataAsJSON } from "utils/metadata"; +import { UsePaginatedQueryOptions } from "components/PaginationWidget/usePaginatedQuery"; + +export function usersKey(req: UsersRequest) { + return ["users", req] as const; +} + +export function paginatedUsers(req: UsersRequest) { + return { + queryKey: (pagination) => usersKey(pagination), + queryFn: () => API.getUsers(req), + cacheTime: 5 * 60 * 1000, + } as const satisfies UsePaginatedQueryOptions; +} export const users = (req: UsersRequest): UseQueryOptions => { return { diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index ba4f85ee06a27..527b7f9fd8668 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -7,6 +7,7 @@ import { clamp } from "lodash"; import { type QueryFunction, + type QueryFunctionContext, type QueryKey, type UseQueryOptions, useQueryClient, @@ -73,34 +74,35 @@ export function usePaginatedQuery< >(options: UsePaginatedQueryOptions) { const { queryKey, queryFn, ...otherOptions } = options; const [searchParams, setSearchParams] = useSearchParams(); - const currentPage = parsePage(searchParams); + const currentPage = parsePage(searchParams); const pageSize = DEFAULT_RECORDS_PER_PAGE; const pageOffset = (currentPage - 1) * pageSize; + const queryOptionsFromPage = (pageNumber: number) => { + return { + queryFn: (queryCxt: QueryFunctionContext) => { + return queryFn({ ...queryCxt, pageParam: pageNumber }); + }, + queryKey: queryKey({ + pageNumber: currentPage, + pageSize, + pageOffset, + }), + } as const; + }; + // Not using infinite query right now because that requires a fair bit of list // virtualization as the lists get bigger (especially for the audit logs) const query = useQuery({ + ...queryOptionsFromPage(currentPage), ...otherOptions, - queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: currentPage }), - queryKey: queryKey({ - pageNumber: currentPage, - pageSize, - pageOffset, - }), keepPreviousData: true, }); const queryClient = useQueryClient(); const prefetchPage = useEffectEvent((newPage: number) => { - void queryClient.prefetchQuery({ - queryFn: (queryCxt) => queryFn({ ...queryCxt, pageParam: newPage }), - queryKey: queryKey({ - pageNumber: newPage, - pageSize, - pageOffset, - }), - }); + return queryClient.prefetchQuery(queryOptionsFromPage(newPage)); }); const totalRecords = query.data?.count ?? 0; @@ -113,13 +115,13 @@ export function usePaginatedQuery< // immediately ready on each render useEffect(() => { if (hasNextPage) { - prefetchPage(currentPage + 1); + void prefetchPage(currentPage + 1); } }, [prefetchPage, currentPage, hasNextPage]); useEffect(() => { if (hasPreviousPage) { - prefetchPage(currentPage - 1); + void prefetchPage(currentPage - 1); } }, [prefetchPage, currentPage, hasPreviousPage]); From 86f8437a8a556d5e0b61646648e30dfaf6156020 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 13:51:40 +0000 Subject: [PATCH 11/63] refactor: clean up pagination code more --- .../PaginationWidget/usePaginatedQuery.ts | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 527b7f9fd8668..6863596bd0053 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -100,19 +100,20 @@ export function usePaginatedQuery< keepPreviousData: true, }); - const queryClient = useQueryClient(); - const prefetchPage = useEffectEvent((newPage: number) => { - return queryClient.prefetchQuery(queryOptionsFromPage(newPage)); - }); - const totalRecords = query.data?.count ?? 0; const totalPages = Math.ceil(totalRecords / pageSize); const hasNextPage = pageSize * pageOffset < totalRecords; const hasPreviousPage = currentPage > 1; + const queryClient = useQueryClient(); + const prefetchPage = useEffectEvent((newPage: number) => { + return queryClient.prefetchQuery(queryOptionsFromPage(newPage)); + }); + // Have to split hairs and sync on both the current page and the hasXPage - // variables because hasXPage values are derived from server values and aren't - // immediately ready on each render + // variables, because the page can change immediately client-side, but the + // hasXPage values are derived from the server and won't be immediately ready + // on the initial render useEffect(() => { if (hasNextPage) { void prefetchPage(currentPage + 1); From f612d8f3b29e6bea1dc5c894228fdbbc6dc4d06b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 15:35:31 +0000 Subject: [PATCH 12/63] fix: remove redundant properties --- site/src/components/PaginationWidget/usePaginatedQuery.ts | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 6863596bd0053..cfcc3b6a75dcb 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -72,7 +72,7 @@ export function usePaginatedQuery< TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, >(options: UsePaginatedQueryOptions) { - const { queryKey, queryFn, ...otherOptions } = options; + const { queryKey, queryFn } = options; const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePage(searchParams); @@ -96,7 +96,6 @@ export function usePaginatedQuery< // virtualization as the lists get bigger (especially for the audit logs) const query = useQuery({ ...queryOptionsFromPage(currentPage), - ...otherOptions, keepPreviousData: true, }); From 5878326b1a6491f5ed26952c69032a7c9e28af30 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 15:48:21 +0000 Subject: [PATCH 13/63] refactor: clean up code --- .../PaginationWidget/usePaginatedQuery.ts | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index cfcc3b6a75dcb..a2d7d0e7dbc18 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -22,7 +22,7 @@ const PAGE_NUMBER_PARAMS_KEY = "page"; /** * All arguments passed into the queryKey functions. */ -type QueryKeyFnArgs = { +type QueryPageParams = { pageNumber: number; pageSize: number; pageOffset: number; @@ -57,13 +57,13 @@ export type UsePaginatedQueryOptions< * Must be a function so that it can be used for the active query, as well as * any prefetching. */ - queryKey: (args: QueryKeyFnArgs) => TQueryKey; + queryKey: (args: QueryPageParams) => TQueryKey; /** * A version of queryFn that is required and that exposes page numbers through * the pageParams context property */ - queryFn: QueryFunction; + queryFn: QueryFunction; }; export function usePaginatedQuery< @@ -72,7 +72,7 @@ export function usePaginatedQuery< TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, >(options: UsePaginatedQueryOptions) { - const { queryKey, queryFn } = options; + const { queryKey, queryFn, ...extraReactQueryOptions } = options; const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePage(searchParams); @@ -80,21 +80,24 @@ export function usePaginatedQuery< const pageOffset = (currentPage - 1) * pageSize; const queryOptionsFromPage = (pageNumber: number) => { + const pageParam: QueryPageParams = { + pageNumber, + pageOffset, + pageSize, + }; + return { - queryFn: (queryCxt: QueryFunctionContext) => { - return queryFn({ ...queryCxt, pageParam: pageNumber }); + queryKey: queryKey(pageParam), + queryFn: (qfc: QueryFunctionContext) => { + return queryFn({ ...qfc, pageParam }); }, - queryKey: queryKey({ - pageNumber: currentPage, - pageSize, - pageOffset, - }), } as const; }; // Not using infinite query right now because that requires a fair bit of list // virtualization as the lists get bigger (especially for the audit logs) const query = useQuery({ + ...extraReactQueryOptions, ...queryOptionsFromPage(currentPage), keepPreviousData: true, }); From 5cc1c2de4d96cafcc083758a7151dddbe8166437 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 17:42:03 +0000 Subject: [PATCH 14/63] wip: commit progress on usePaginatedQuery --- site/src/api/queries/users.ts | 26 +++++-- .../PaginationWidget/usePaginatedQuery.ts | 67 +++++++++++++------ 2 files changed, 67 insertions(+), 26 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 40e9afa7479e2..2fbb7d1518061 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -1,6 +1,6 @@ import { QueryClient, type UseQueryOptions } from "react-query"; import * as API from "api/api"; -import { +import type { AuthorizationRequest, GetUsersResponse, UpdateUserPasswordRequest, @@ -11,15 +11,33 @@ import { import { getAuthorizationKey } from "./authCheck"; import { getMetadataAsJSON } from "utils/metadata"; import { UsePaginatedQueryOptions } from "components/PaginationWidget/usePaginatedQuery"; +import { prepareQuery } from "utils/filters"; export function usersKey(req: UsersRequest) { return ["users", req] as const; } -export function paginatedUsers(req: UsersRequest) { +export function paginatedUsers() { return { - queryKey: (pagination) => usersKey(pagination), - queryFn: () => API.getUsers(req), + searchParamsKey: "filter", + + queryKey: ({ pageSize, pageOffset, searchParamsQuery }) => { + return usersKey({ + q: prepareQuery(searchParamsQuery ?? ""), + limit: pageSize, + offset: pageOffset, + }); + }, + queryFn: ({ pageSize, pageOffset, searchParamsQuery, signal }) => { + return API.getUsers( + { + q: prepareQuery(searchParamsQuery ?? ""), + limit: pageSize, + offset: pageOffset, + }, + signal, + ); + }, cacheTime: 5 * 60 * 1000, } as const satisfies UsePaginatedQueryOptions; } diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index a2d7d0e7dbc18..9c7df260c2de4 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -6,7 +6,6 @@ import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; import { clamp } from "lodash"; import { - type QueryFunction, type QueryFunctionContext, type QueryKey, type UseQueryOptions, @@ -20,22 +19,26 @@ import { const PAGE_NUMBER_PARAMS_KEY = "page"; /** - * All arguments passed into the queryKey functions. + * Information about a paginated request. Passed into both the queryKey and + * queryFn functions on each render */ type QueryPageParams = { pageNumber: number; pageSize: number; pageOffset: number; - extraQuery?: string; + searchParamsQuery?: string; }; /** * Any JSON-serializable object returned by the API that exposes the total * number of records that match a query */ -interface PaginatedData { +type PaginatedData = { count: number; -} +}; + +type QueryFnContext = QueryPageParams & + Omit, "pageParam">; /** * A more specialized version of UseQueryOptions built specifically for @@ -49,21 +52,28 @@ export type UsePaginatedQueryOptions< TQueryKey extends QueryKey = QueryKey, > = Omit< UseQueryOptions, - "keepPreviousData" | "queryKey" + "keepPreviousData" | "queryKey" | "queryFn" > & { + /** + * The key to use for parsing additional query information + */ + searchParamsKey?: string; + /** * A function that takes pagination information and produces a full query key. * * Must be a function so that it can be used for the active query, as well as * any prefetching. */ - queryKey: (args: QueryPageParams) => TQueryKey; + queryKey: (params: QueryPageParams) => TQueryKey; /** - * A version of queryFn that is required and that exposes page numbers through - * the pageParams context property + * A version of queryFn that is required and that exposes the pagination + * information through the pageParams context property */ - queryFn: QueryFunction; + queryFn: ( + context: QueryFnContext, + ) => TQueryFnData | Promise; }; export function usePaginatedQuery< @@ -72,33 +82,44 @@ export function usePaginatedQuery< TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, >(options: UsePaginatedQueryOptions) { - const { queryKey, queryFn, ...extraReactQueryOptions } = options; - const [searchParams, setSearchParams] = useSearchParams(); + const { + queryKey, + searchParamsKey, + queryFn: outerQueryFn, + ...extraOptions + } = options; + const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePage(searchParams); const pageSize = DEFAULT_RECORDS_PER_PAGE; const pageOffset = (currentPage - 1) * pageSize; - const queryOptionsFromPage = (pageNumber: number) => { + const getQueryOptionsFromPage = (pageNumber: number) => { + const searchParamsQuery = + searchParamsKey !== undefined + ? searchParams.get(searchParamsKey) ?? undefined + : undefined; + const pageParam: QueryPageParams = { pageNumber, pageOffset, pageSize, + searchParamsQuery, }; return { queryKey: queryKey(pageParam), - queryFn: (qfc: QueryFunctionContext) => { - return queryFn({ ...qfc, pageParam }); + queryFn: (context: QueryFunctionContext) => { + return outerQueryFn({ ...context, ...pageParam }); }, } as const; }; // Not using infinite query right now because that requires a fair bit of list // virtualization as the lists get bigger (especially for the audit logs) - const query = useQuery({ - ...extraReactQueryOptions, - ...queryOptionsFromPage(currentPage), + const query = useQuery({ + ...extraOptions, + ...getQueryOptionsFromPage(currentPage), keepPreviousData: true, }); @@ -109,7 +130,7 @@ export function usePaginatedQuery< const queryClient = useQueryClient(); const prefetchPage = useEffectEvent((newPage: number) => { - return queryClient.prefetchQuery(queryOptionsFromPage(newPage)); + return queryClient.prefetchQuery(getQueryOptionsFromPage(newPage)); }); // Have to split hairs and sync on both the current page and the hasXPage @@ -164,9 +185,11 @@ export function usePaginatedQuery< hasNextPage, hasPreviousPage, - // Have to hijack the isLoading property slightly because keepPreviousData - // is true; by default, isLoading will be false after the initial page - // loads, even if new pages are loading in + // Hijacking the isLoading property slightly because keepPreviousData is + // true; by default, isLoading will always be false after the initial page + // loads, even if new pages are loading in. Especially since + // keepPreviousData is an implementation detail, simplifying the API felt + // like the better option, at the risk of it becoming more "magical" isLoading: query.isLoading || query.isFetching, } as const; } From 0138f214bea134d85509e1830d3a8ef372df1651 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 15 Nov 2023 19:26:14 +0000 Subject: [PATCH 15/63] wip: commit current pagination progress --- site/src/api/queries/users.ts | 23 ++++------ .../PaginationWidget/usePaginatedQuery.ts | 42 ++++++++++++------- 2 files changed, 33 insertions(+), 32 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 2fbb7d1518061..301d37e13a8b5 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -19,25 +19,16 @@ export function usersKey(req: UsersRequest) { export function paginatedUsers() { return { - searchParamsKey: "filter", - - queryKey: ({ pageSize, pageOffset, searchParamsQuery }) => { - return usersKey({ - q: prepareQuery(searchParamsQuery ?? ""), + queryPayload: ({ pageSize, pageOffset, searchParams }) => { + return { + q: prepareQuery(searchParams.get("filter") ?? ""), limit: pageSize, offset: pageOffset, - }); - }, - queryFn: ({ pageSize, pageOffset, searchParamsQuery, signal }) => { - return API.getUsers( - { - q: prepareQuery(searchParamsQuery ?? ""), - limit: pageSize, - offset: pageOffset, - }, - signal, - ); + }; }, + + queryKey: ({ payload }) => usersKey(payload), + queryFn: ({ payload, signal }) => API.getUsers(payload, signal), cacheTime: 5 * 60 * 1000, } as const satisfies UsePaginatedQueryOptions; } diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 9c7df260c2de4..7ef8895508e4b 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -1,6 +1,6 @@ import { useEffect } from "react"; -import { useSearchParams } from "react-router-dom"; import { useEffectEvent } from "hooks/hookPolyfills"; +import { useSearchParams } from "react-router-dom"; import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; import { clamp } from "lodash"; @@ -26,7 +26,11 @@ type QueryPageParams = { pageNumber: number; pageSize: number; pageOffset: number; - searchParamsQuery?: string; + searchParams: URLSearchParams; +}; + +type QueryPageParamsWithPayload = QueryPageParams & { + payload: T; }; /** @@ -37,8 +41,9 @@ type PaginatedData = { count: number; }; -type QueryFnContext = QueryPageParams & - Omit, "pageParam">; +type QueryFnContext = + QueryPageParamsWithPayload & + Omit, "pageParam">; /** * A more specialized version of UseQueryOptions built specifically for @@ -50,14 +55,19 @@ export type UsePaginatedQueryOptions< TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, + TQueryPayload = unknown, > = Omit< UseQueryOptions, "keepPreviousData" | "queryKey" | "queryFn" > & { /** - * The key to use for parsing additional query information + * A function for defining values that should be shared between queryKey and + * queryFn. The value will be exposed via the "payload" property in + * QueryPageParams. + * + * Mainly here for convenience and minimizing copy-and-pasting. */ - searchParamsKey?: string; + queryPayload?: (params: QueryPageParams) => TQueryPayload; /** * A function that takes pagination information and produces a full query key. @@ -65,7 +75,7 @@ export type UsePaginatedQueryOptions< * Must be a function so that it can be used for the active query, as well as * any prefetching. */ - queryKey: (params: QueryPageParams) => TQueryKey; + queryKey: (params: QueryPageParamsWithPayload) => TQueryKey; /** * A version of queryFn that is required and that exposes the pagination @@ -84,7 +94,7 @@ export function usePaginatedQuery< >(options: UsePaginatedQueryOptions) { const { queryKey, - searchParamsKey, + queryPayload, queryFn: outerQueryFn, ...extraOptions } = options; @@ -95,22 +105,22 @@ export function usePaginatedQuery< const pageOffset = (currentPage - 1) * pageSize; const getQueryOptionsFromPage = (pageNumber: number) => { - const searchParamsQuery = - searchParamsKey !== undefined - ? searchParams.get(searchParamsKey) ?? undefined - : undefined; - const pageParam: QueryPageParams = { pageNumber, pageOffset, pageSize, - searchParamsQuery, + searchParams, + }; + + const withPayload: QueryPageParamsWithPayload = { + ...pageParam, + payload: queryPayload?.(pageParam), }; return { - queryKey: queryKey(pageParam), + queryKey: queryKey(withPayload), queryFn: (context: QueryFunctionContext) => { - return outerQueryFn({ ...context, ...pageParam }); + return outerQueryFn({ ...context, ...withPayload }); }, } as const; }; From a38fd308d367f9d17a6b3c7b550d1cacfd2fc18d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 17 Nov 2023 14:54:40 +0000 Subject: [PATCH 16/63] docs: clean up comments for clarity --- .../PaginationWidget/usePaginatedQuery.ts | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 7ef8895508e4b..e86dbc718942d 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -19,8 +19,8 @@ import { const PAGE_NUMBER_PARAMS_KEY = "page"; /** - * Information about a paginated request. Passed into both the queryKey and - * queryFn functions on each render + * Information about a paginated request. This information is passed into the + * queryPayload, queryKey, and queryFn properties of the hook. */ type QueryPageParams = { pageNumber: number; @@ -41,7 +41,7 @@ type PaginatedData = { count: number; }; -type QueryFnContext = +type PaginatedQueryFnContext = QueryPageParamsWithPayload & Omit, "pageParam">; @@ -65,7 +65,8 @@ export type UsePaginatedQueryOptions< * queryFn. The value will be exposed via the "payload" property in * QueryPageParams. * - * Mainly here for convenience and minimizing copy-and-pasting. + * Mainly here for convenience and minimizing copy-and-pasting between + * queryKey and queryFn. */ queryPayload?: (params: QueryPageParams) => TQueryPayload; @@ -82,7 +83,7 @@ export type UsePaginatedQueryOptions< * information through the pageParams context property */ queryFn: ( - context: QueryFnContext, + context: PaginatedQueryFnContext, ) => TQueryFnData | Promise; }; @@ -126,7 +127,8 @@ export function usePaginatedQuery< }; // Not using infinite query right now because that requires a fair bit of list - // virtualization as the lists get bigger (especially for the audit logs) + // virtualization as the lists get bigger (especially for the audit logs). + // Keeping initial implementation simple. const query = useQuery({ ...extraOptions, ...getQueryOptionsFromPage(currentPage), From 22d6c243f414273a92a9c34ba78b9258717f19f8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 16:40:47 +0000 Subject: [PATCH 17/63] wip: get type signatures compatible (breaks runtime logic slightly) --- site/src/api/queries/users.ts | 2 +- .../PaginationWidget/usePaginatedQuery.ts | 90 +++++++++++-------- 2 files changed, 54 insertions(+), 38 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 301d37e13a8b5..6f1dd378e56e3 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -30,7 +30,7 @@ export function paginatedUsers() { queryKey: ({ payload }) => usersKey(payload), queryFn: ({ payload, signal }) => API.getUsers(payload, signal), cacheTime: 5 * 60 * 1000, - } as const satisfies UsePaginatedQueryOptions; + } as const satisfies UsePaginatedQueryOptions; } export const users = (req: UsersRequest): UseQueryOptions => { diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index e86dbc718942d..8135b38524266 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -29,8 +29,12 @@ type QueryPageParams = { searchParams: URLSearchParams; }; -type QueryPageParamsWithPayload = QueryPageParams & { - payload: T; +/** + * Query page params, plus the result of the queryPayload function. + * This type is passed to both queryKey and queryFn. + */ +type QueryPageParamsWithPayload = QueryPageParams & { + payload: [TPayload] extends [never] ? undefined : TPayload; }; /** @@ -41,9 +45,21 @@ type PaginatedData = { count: number; }; -type PaginatedQueryFnContext = - QueryPageParamsWithPayload & - Omit, "pageParam">; +type PaginatedQueryFnContext< + TQueryKey extends QueryKey = QueryKey, + TPayload = never, +> = Omit, "pageParam"> & + QueryPageParamsWithPayload; + +type BasePaginationOptions< + TQueryFnData extends PaginatedData = PaginatedData, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Omit< + UseQueryOptions, + "keepPreviousData" | "queryKey" | "queryFn" +>; /** * A more specialized version of UseQueryOptions built specifically for @@ -52,47 +68,47 @@ type PaginatedQueryFnContext = // All the type parameters just mirror the ones used by React Query export type UsePaginatedQueryOptions< TQueryFnData extends PaginatedData = PaginatedData, + TQueryPayload = never, TError = unknown, TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, - TQueryPayload = unknown, -> = Omit< - UseQueryOptions, - "keepPreviousData" | "queryKey" | "queryFn" -> & { - /** - * A function for defining values that should be shared between queryKey and - * queryFn. The value will be exposed via the "payload" property in - * QueryPageParams. - * - * Mainly here for convenience and minimizing copy-and-pasting between - * queryKey and queryFn. - */ - queryPayload?: (params: QueryPageParams) => TQueryPayload; - - /** - * A function that takes pagination information and produces a full query key. - * - * Must be a function so that it can be used for the active query, as well as - * any prefetching. - */ - queryKey: (params: QueryPageParamsWithPayload) => TQueryKey; - - /** - * A version of queryFn that is required and that exposes the pagination - * information through the pageParams context property - */ - queryFn: ( - context: PaginatedQueryFnContext, - ) => TQueryFnData | Promise; -}; +> = BasePaginationOptions & + ([TQueryPayload] extends [never] + ? { queryPayload?: never } + : { queryPayload: (params: QueryPageParams) => TQueryPayload }) & { + /** + * A function that takes pagination information and produces a full query + * key. + * + * Must be a function so that it can be used for the active query, as well + * as any prefetching. + */ + queryKey: (params: QueryPageParamsWithPayload) => TQueryKey; + + /** + * A version of queryFn that is required and that exposes the pagination + * information through the pageParams context property + */ + queryFn: ( + context: PaginatedQueryFnContext, + ) => TQueryFnData | Promise; + }; export function usePaginatedQuery< TQueryFnData extends PaginatedData = PaginatedData, TError = unknown, TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, ->(options: UsePaginatedQueryOptions) { + TPayload = never, +>( + options: UsePaginatedQueryOptions< + TQueryFnData, + TPayload, + TError, + TData, + TQueryKey + >, +) { const { queryKey, queryPayload, From e717da1dff1844f30e91b43c48f8056e780b517f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 18:45:36 +0000 Subject: [PATCH 18/63] refactor: clean up type definitions --- .../PaginationWidget/usePaginatedQuery.ts | 116 ++++++++++-------- 1 file changed, 65 insertions(+), 51 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 8135b38524266..8b8cce74e5340 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -18,49 +18,6 @@ import { */ const PAGE_NUMBER_PARAMS_KEY = "page"; -/** - * Information about a paginated request. This information is passed into the - * queryPayload, queryKey, and queryFn properties of the hook. - */ -type QueryPageParams = { - pageNumber: number; - pageSize: number; - pageOffset: number; - searchParams: URLSearchParams; -}; - -/** - * Query page params, plus the result of the queryPayload function. - * This type is passed to both queryKey and queryFn. - */ -type QueryPageParamsWithPayload = QueryPageParams & { - payload: [TPayload] extends [never] ? undefined : TPayload; -}; - -/** - * Any JSON-serializable object returned by the API that exposes the total - * number of records that match a query - */ -type PaginatedData = { - count: number; -}; - -type PaginatedQueryFnContext< - TQueryKey extends QueryKey = QueryKey, - TPayload = never, -> = Omit, "pageParam"> & - QueryPageParamsWithPayload; - -type BasePaginationOptions< - TQueryFnData extends PaginatedData = PaginatedData, - TError = unknown, - TData = TQueryFnData, - TQueryKey extends QueryKey = QueryKey, -> = Omit< - UseQueryOptions, - "keepPreviousData" | "queryKey" | "queryFn" ->; - /** * A more specialized version of UseQueryOptions built specifically for * paginated queries. @@ -80,8 +37,8 @@ export type UsePaginatedQueryOptions< * A function that takes pagination information and produces a full query * key. * - * Must be a function so that it can be used for the active query, as well - * as any prefetching. + * Must be a function so that it can be used for the active query, and then + * reused for any prefetching queries. */ queryKey: (params: QueryPageParamsWithPayload) => TQueryKey; @@ -129,15 +86,13 @@ export function usePaginatedQuery< searchParams, }; - const withPayload: QueryPageParamsWithPayload = { - ...pageParam, - payload: queryPayload?.(pageParam), - }; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Have to do this because proving the type's soundness to the compiler will make the code so much more convoluted and harder to maintain + const payload = queryPayload?.(pageParam) as any; return { - queryKey: queryKey(withPayload), + queryKey: queryKey({ ...pageParam, payload }), queryFn: (context: QueryFunctionContext) => { - return outerQueryFn({ ...context, ...withPayload }); + return outerQueryFn({ ...context, ...pageParam, payload }); }, } as const; }; @@ -226,3 +181,62 @@ function parsePage(params: URLSearchParams): number { const parsed = Number(params.get("page")); return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } + +/** + * Information about a paginated request. This information is passed into the + * queryPayload, queryKey, and queryFn properties of the hook. + */ +type QueryPageParams = { + pageNumber: number; + pageSize: number; + pageOffset: number; + searchParams: URLSearchParams; +}; + +/** + * The query page params, appended with the result of the queryPayload function. + * This type is passed to both queryKey and queryFn. If queryPayload is + * undefined, payload will always be undefined + */ +type QueryPageParamsWithPayload = QueryPageParams & { + payload: [TPayload] extends [never] ? undefined : TPayload; +}; + +/** + * Any JSON-serializable object returned by the API that exposes the total + * number of records that match a query + */ +type PaginatedData = { + count: number; +}; + +/** + * React Query's QueryFunctionContext (minus pageParam, which is weird and + * defaults to type any anyway), plus all properties from + * QueryPageParamsWithPayload. + */ +type PaginatedQueryFnContext< + TQueryKey extends QueryKey = QueryKey, + TPayload = never, +> = Omit, "pageParam"> & + QueryPageParamsWithPayload; + +/** + * The set of React Query properties that UsePaginatedQueryOptions derives from. + * + * Three properties are stripped from it: + * - keepPreviousData - The value must always be true to keep pagination feeling + * nice, so better to prevent someone from trying to touch it at all + * - queryFn - Removed to simplify replacing the type of its context argument + * - queryKey - Removed so that it can be replaced with the function form of + * queryKey + */ +type BasePaginationOptions< + TQueryFnData extends PaginatedData = PaginatedData, + TError = unknown, + TData = TQueryFnData, + TQueryKey extends QueryKey = QueryKey, +> = Omit< + UseQueryOptions, + "keepPreviousData" | "queryKey" | "queryFn" +>; From 7624d94f7aaccc9ee5fd004b4aed2fc47fd334d8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 18:54:51 +0000 Subject: [PATCH 19/63] chore: add support for custom onInvalidPage functions --- .../PaginationWidget/usePaginatedQuery.ts | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 8b8cce74e5340..75ef54fc67a04 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -49,6 +49,15 @@ export type UsePaginatedQueryOptions< queryFn: ( context: PaginatedQueryFnContext, ) => TQueryFnData | Promise; + + /** + * A custom, optional function for handling what happens if the user + * navigates to a page that doesn't exist for the paginated data. + * + * If this function is not defined/provided, usePaginatedQuery will navigate + * the user to the closest valid page. + */ + onInvalidPage?: (currentPage: number, totalPages: number) => void; }; export function usePaginatedQuery< @@ -69,6 +78,7 @@ export function usePaginatedQuery< const { queryKey, queryPayload, + onInvalidPage, queryFn: outerQueryFn, ...extraOptions } = options; @@ -134,8 +144,12 @@ export function usePaginatedQuery< // Mainly here to catch user if they navigate to a page directly via URL const updatePageIfInvalid = useEffectEvent(() => { - const clamped = clamp(currentPage, 1, totalPages); + if (onInvalidPage !== undefined) { + onInvalidPage(currentPage, totalPages); + return; + } + const clamped = clamp(currentPage, 1, totalPages); if (currentPage !== clamped) { searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); setSearchParams(searchParams); From 26231312d70e5d80a6cb254d87ae0628586ec08c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 19:12:17 +0000 Subject: [PATCH 20/63] refactor: clean up type definitions more for clarity reasons --- .../PaginationWidget/usePaginatedQuery.ts | 31 ++++++++++++++----- 1 file changed, 24 insertions(+), 7 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 75ef54fc67a04..b47acccba502e 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -30,9 +30,7 @@ export type UsePaginatedQueryOptions< TData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, > = BasePaginationOptions & - ([TQueryPayload] extends [never] - ? { queryPayload?: never } - : { queryPayload: (params: QueryPageParams) => TQueryPayload }) & { + QueryPayloadExtender & { /** * A function that takes pagination information and produces a full query * key. @@ -144,13 +142,14 @@ export function usePaginatedQuery< // Mainly here to catch user if they navigate to a page directly via URL const updatePageIfInvalid = useEffectEvent(() => { - if (onInvalidPage !== undefined) { - onInvalidPage(currentPage, totalPages); + const clamped = clamp(currentPage, 1, totalPages); + if (currentPage === clamped) { return; } - const clamped = clamp(currentPage, 1, totalPages); - if (currentPage !== clamped) { + if (onInvalidPage !== undefined) { + onInvalidPage(currentPage, totalPages); + } else { searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); setSearchParams(searchParams); } @@ -196,6 +195,24 @@ function parsePage(params: URLSearchParams): number { return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } +/** + * Papers over how the queryPayload function is defined at the type level, so + * that UsePaginatedQueryOptions doesn't look as scary. + * + * You're going to see these tuple types in a few different spots in this file; + * it's a "hack" to get around the function contravariance that pops up when you + * normally try to share the TQueryPayload between queryPayload, queryKey, and + * queryFn via the direct/"obvious" way. By throwing the types into tuples + * (which are naturally covariant), it's a lot easier to share the types without + * TypeScript complaining all the time or getting so confused that it degrades + * the type definitions into a bunch of "any" types + */ +type QueryPayloadExtender = [TQueryPayload] extends [ + never, +] + ? { queryPayload?: never } + : { queryPayload: (params: QueryPageParams) => TQueryPayload }; + /** * Information about a paginated request. This information is passed into the * queryPayload, queryKey, and queryFn properties of the hook. From 29242e9f194b7eb91cbdca3500055366d403734c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 19:28:34 +0000 Subject: [PATCH 21/63] chore: delete Pagination component (separate PR) --- .../PaginationWidget/Pagination.tsx | 63 ------------------- 1 file changed, 63 deletions(-) delete mode 100644 site/src/components/PaginationWidget/Pagination.tsx diff --git a/site/src/components/PaginationWidget/Pagination.tsx b/site/src/components/PaginationWidget/Pagination.tsx deleted file mode 100644 index 7fc882345f316..0000000000000 --- a/site/src/components/PaginationWidget/Pagination.tsx +++ /dev/null @@ -1,63 +0,0 @@ -import { type PropsWithChildren, useEffect, useRef } from "react"; -import { PaginationWidgetBase } from "./PaginationWidgetBase"; - -type PaginationProps = PropsWithChildren<{ - fetching: boolean; - currentPage: number; - pageSize: number; - totalRecords: number; - onPageChange: (newPage: number) => void; -}>; - -export function Pagination({ - children, - fetching, - currentPage, - totalRecords, - pageSize, - onPageChange, -}: PaginationProps) { - const scrollAfterPageChangeRef = useRef(false); - useEffect(() => { - const onScroll = () => { - scrollAfterPageChangeRef.current = false; - }; - - document.addEventListener("scroll", onScroll); - return () => document.removeEventListener("scroll", onScroll); - }, []); - - const previousPageRef = useRef(undefined); - const paginationTopRef = useRef(null); - useEffect(() => { - const paginationTop = paginationTopRef.current; - const isInitialRender = previousPageRef.current === undefined; - - const skipScroll = - isInitialRender || - paginationTop === null || - !scrollAfterPageChangeRef.current; - - previousPageRef.current = currentPage; - if (!skipScroll) { - paginationTop.scrollIntoView(); - } - }, [currentPage]); - - return ( - <> -
- {children} - - { - scrollAfterPageChangeRef.current = true; - onPageChange(newPage); - }} - /> - - ); -} From 5a9aa2dd9bd560f532170d8b55f699cfb99d1ac8 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 19:29:20 +0000 Subject: [PATCH 22/63] chore: remove cacheTime fixes (to be resolved in future PR) --- site/src/api/queries/users.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 6f1dd378e56e3..712262499fb4a 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -29,7 +29,6 @@ export function paginatedUsers() { queryKey: ({ payload }) => usersKey(payload), queryFn: ({ payload, signal }) => API.getUsers(payload, signal), - cacheTime: 5 * 60 * 1000, } as const satisfies UsePaginatedQueryOptions; } @@ -37,7 +36,6 @@ export const users = (req: UsersRequest): UseQueryOptions => { return { queryKey: ["users", req], queryFn: ({ signal }) => API.getUsers(req, signal), - cacheTime: 5 * 60 * 1000, }; }; From 3d673042a0c6c60a987c7bab0607f37d410b9ffb Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 19:43:42 +0000 Subject: [PATCH 23/63] docs: add clarifying/intellisense comments for DX --- .../PaginationWidget/usePaginatedQuery.ts | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index b47acccba502e..7db9aec4315b6 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -36,7 +36,7 @@ export type UsePaginatedQueryOptions< * key. * * Must be a function so that it can be used for the active query, and then - * reused for any prefetching queries. + * reused for any prefetching queries (swapping the page number out). */ queryKey: (params: QueryPageParamsWithPayload) => TQueryKey; @@ -211,7 +211,17 @@ type QueryPayloadExtender = [TQueryPayload] extends [ never, ] ? { queryPayload?: never } - : { queryPayload: (params: QueryPageParams) => TQueryPayload }; + : { + /** + * An optional function for defining reusable "patterns" for taking + * pagination data (current page, etc.), which will be evaluated and + * passed to queryKey and queryFn for active queries and prefetch queries. + * + * queryKey and queryFn can each access the result of queryPayload + * by accessing the "payload" property from their main function argument + */ + queryPayload: (params: QueryPageParams) => TQueryPayload; + }; /** * Information about a paginated request. This information is passed into the @@ -258,7 +268,8 @@ type PaginatedQueryFnContext< * Three properties are stripped from it: * - keepPreviousData - The value must always be true to keep pagination feeling * nice, so better to prevent someone from trying to touch it at all - * - queryFn - Removed to simplify replacing the type of its context argument + * - queryFn - Removed to make it easier to swap in a custom queryFn type + * definition with a custom context argument * - queryKey - Removed so that it can be replaced with the function form of * queryKey */ From d97706055b315d025268f281101cdc58a2ae913d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 20:23:40 +0000 Subject: [PATCH 24/63] refactor: link users queries to same queryKey implementation --- site/src/api/queries/users.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 712262499fb4a..f7b5d871fb485 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -34,7 +34,7 @@ export function paginatedUsers() { export const users = (req: UsersRequest): UseQueryOptions => { return { - queryKey: ["users", req], + queryKey: usersKey(req), queryFn: ({ signal }) => API.getUsers(req, signal), }; }; From 9224624b50502c2493324f7f50c5efee7e048856 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 20:26:21 +0000 Subject: [PATCH 25/63] docs: remove misleading comment --- site/src/components/PaginationWidget/usePaginatedQuery.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 7db9aec4315b6..892d2b4f117de 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -42,7 +42,7 @@ export type UsePaginatedQueryOptions< /** * A version of queryFn that is required and that exposes the pagination - * information through the pageParams context property + * information through its query function context argument */ queryFn: ( context: PaginatedQueryFnContext, From 540c779c731a4cda7c1b7eeef6be2af5cc5affb0 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 20:29:15 +0000 Subject: [PATCH 26/63] docs: more comments --- site/src/components/PaginationWidget/usePaginatedQuery.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 892d2b4f117de..b15bfc83fb719 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -52,8 +52,9 @@ export type UsePaginatedQueryOptions< * A custom, optional function for handling what happens if the user * navigates to a page that doesn't exist for the paginated data. * - * If this function is not defined/provided, usePaginatedQuery will navigate - * the user to the closest valid page. + * If this function is not defined/provided when an invalid page is + * encountered, usePaginatedQuery will default to navigating the user to the + * closest valid page. */ onInvalidPage?: (currentPage: number, totalPages: number) => void; }; From 4b42b6f3a7e9614eeb214d7feebea734f5454f19 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 20:39:33 +0000 Subject: [PATCH 27/63] chore: update onInvalidPage params for more flexibility --- .../PaginationWidget/usePaginatedQuery.ts | 38 +++++++++++++------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index b15bfc83fb719..3b0b442455941 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -1,6 +1,6 @@ import { useEffect } from "react"; import { useEffectEvent } from "hooks/hookPolyfills"; -import { useSearchParams } from "react-router-dom"; +import { type SetURLSearchParams, useSearchParams } from "react-router-dom"; import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; import { clamp } from "lodash"; @@ -22,7 +22,6 @@ const PAGE_NUMBER_PARAMS_KEY = "page"; * A more specialized version of UseQueryOptions built specifically for * paginated queries. */ -// All the type parameters just mirror the ones used by React Query export type UsePaginatedQueryOptions< TQueryFnData extends PaginatedData = PaginatedData, TQueryPayload = never, @@ -56,19 +55,19 @@ export type UsePaginatedQueryOptions< * encountered, usePaginatedQuery will default to navigating the user to the * closest valid page. */ - onInvalidPage?: (currentPage: number, totalPages: number) => void; + onInvalidPage?: (params: InvalidPageParams) => void; }; export function usePaginatedQuery< TQueryFnData extends PaginatedData = PaginatedData, + TQueryPayload = never, TError = unknown, TData extends PaginatedData = TQueryFnData, TQueryKey extends QueryKey = QueryKey, - TPayload = never, >( options: UsePaginatedQueryOptions< TQueryFnData, - TPayload, + TQueryPayload, TError, TData, TQueryKey @@ -88,20 +87,20 @@ export function usePaginatedQuery< const pageOffset = (currentPage - 1) * pageSize; const getQueryOptionsFromPage = (pageNumber: number) => { - const pageParam: QueryPageParams = { + const pageParams: QueryPageParams = { pageNumber, pageOffset, pageSize, - searchParams, + searchParams: searchParams, }; - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Have to do this because proving the type's soundness to the compiler will make the code so much more convoluted and harder to maintain - const payload = queryPayload?.(pageParam) as any; + // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Have to do this because proving the type's soundness to the compiler will make this file even more convoluted than it is now + const payload = queryPayload?.(pageParams) as any; return { - queryKey: queryKey({ ...pageParam, payload }), + queryKey: queryKey({ ...pageParams, payload }), queryFn: (context: QueryFunctionContext) => { - return outerQueryFn({ ...context, ...pageParam, payload }); + return outerQueryFn({ ...context, ...pageParams, payload }); }, } as const; }; @@ -149,7 +148,14 @@ export function usePaginatedQuery< } if (onInvalidPage !== undefined) { - onInvalidPage(currentPage, totalPages); + onInvalidPage({ + pageOffset, + pageSize, + totalPages, + setSearchParams, + pageNumber: currentPage, + searchParams: searchParams, + }); } else { searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); setSearchParams(searchParams); @@ -283,3 +289,11 @@ type BasePaginationOptions< UseQueryOptions, "keepPreviousData" | "queryKey" | "queryFn" >; + +/** + * The argument passed to a custom onInvalidPage callback. + */ +type InvalidPageParams = QueryPageParams & { + totalPages: number; + setSearchParams: SetURLSearchParams; +}; From 5bc43c9399b4a6f493d3ff39d21f054a32557506 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 21:08:24 +0000 Subject: [PATCH 28/63] fix: remove explicit any --- site/src/components/PaginationWidget/usePaginatedQuery.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 3b0b442455941..f3d3ca7d81e9f 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -94,8 +94,11 @@ export function usePaginatedQuery< searchParams: searchParams, }; - // eslint-disable-next-line @typescript-eslint/no-explicit-any -- Have to do this because proving the type's soundness to the compiler will make this file even more convoluted than it is now - const payload = queryPayload?.(pageParams) as any; + type RuntimePayload = [TQueryPayload] extends [never] + ? undefined + : TQueryPayload; + + const payload = queryPayload?.(pageParams) as RuntimePayload; return { queryKey: queryKey({ ...pageParams, payload }), From bd146a19422e9ca1c7563e0969c1a65bba9d1188 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 21:31:36 +0000 Subject: [PATCH 29/63] refactor: clean up type definitions --- .../PaginationWidget/usePaginatedQuery.ts | 51 ++++++++++++++----- 1 file changed, 38 insertions(+), 13 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index f3d3ca7d81e9f..719047484d8a7 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -11,6 +11,7 @@ import { type UseQueryOptions, useQueryClient, useQuery, + UseQueryResult, } from "react-query"; /** @@ -58,6 +59,23 @@ export type UsePaginatedQueryOptions< onInvalidPage?: (params: InvalidPageParams) => void; }; +export type UsePaginatedQueryResult = Omit< + UseQueryResult, + "isLoading" +> & { + isLoading: boolean; + hasNextPage: boolean; + hasPreviousPage: boolean; + + currentPage: number; + pageSize: number; + totalRecords: number; + + onPageChange: (newPage: number) => void; + goToPreviousPage: () => void; + goToNextPage: () => void; +}; + export function usePaginatedQuery< TQueryFnData extends PaginatedData = PaginatedData, TQueryPayload = never, @@ -72,7 +90,7 @@ export function usePaginatedQuery< TData, TQueryKey >, -) { +): UsePaginatedQueryResult { const { queryKey, queryPayload, @@ -94,11 +112,7 @@ export function usePaginatedQuery< searchParams: searchParams, }; - type RuntimePayload = [TQueryPayload] extends [never] - ? undefined - : TQueryPayload; - - const payload = queryPayload?.(pageParams) as RuntimePayload; + const payload = queryPayload?.(pageParams) as RuntimePayload; return { queryKey: queryKey({ ...pageParams, payload }), @@ -150,18 +164,20 @@ export function usePaginatedQuery< return; } - if (onInvalidPage !== undefined) { - onInvalidPage({ + if (onInvalidPage === undefined) { + searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); + setSearchParams(searchParams); + } else { + const params: InvalidPageParams = { pageOffset, pageSize, totalPages, setSearchParams, pageNumber: currentPage, searchParams: searchParams, - }); - } else { - searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); - setSearchParams(searchParams); + }; + + onInvalidPage(params); } }); @@ -244,13 +260,22 @@ type QueryPageParams = { searchParams: URLSearchParams; }; +/** + * Weird, hard-to-describe type definition, but it's necessary for making sure + * that the type information involving the queryPayload function narrows + * properly. + */ +type RuntimePayload = [TPayload] extends [never] + ? undefined + : TPayload; + /** * The query page params, appended with the result of the queryPayload function. * This type is passed to both queryKey and queryFn. If queryPayload is * undefined, payload will always be undefined */ type QueryPageParamsWithPayload = QueryPageParams & { - payload: [TPayload] extends [never] ? undefined : TPayload; + payload: RuntimePayload; }; /** From 983b83f1da225a70caefc5b2db8e64e540098c3a Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 21:44:38 +0000 Subject: [PATCH 30/63] refactor: rename query params for consistency --- site/src/api/queries/users.ts | 6 +- site/src/components/PaginationWidget/temp.ts | 112 ++++++++++++++++++ .../PaginationWidget/usePaginatedQuery.ts | 44 +++++-- 3 files changed, 147 insertions(+), 15 deletions(-) create mode 100644 site/src/components/PaginationWidget/temp.ts diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index f7b5d871fb485..3db52dab06cf8 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -19,11 +19,11 @@ export function usersKey(req: UsersRequest) { export function paginatedUsers() { return { - queryPayload: ({ pageSize, pageOffset, searchParams }) => { + queryPayload: ({ limit, offset, searchParams }) => { return { + limit, + offset, q: prepareQuery(searchParams.get("filter") ?? ""), - limit: pageSize, - offset: pageOffset, }; }, diff --git a/site/src/components/PaginationWidget/temp.ts b/site/src/components/PaginationWidget/temp.ts new file mode 100644 index 0000000000000..03025d8e2fe4b --- /dev/null +++ b/site/src/components/PaginationWidget/temp.ts @@ -0,0 +1,112 @@ +export function usePaginatedQuery(options: any) { + const { + queryKey, + queryPayload, + onInvalidPage, + queryFn: outerQueryFn, + ...extraOptions + } = options; + + const [searchParams, setSearchParams] = useSearchParams(); + const currentPage = parsePage(searchParams); + const pageSize = DEFAULT_RECORDS_PER_PAGE; + const pageOffset = (currentPage - 1) * pageSize; + + const getQueryOptionsFromPage = (pageNumber: number) => { + const pageParams = { + pageNumber, + offset: pageOffset, + limit: pageSize, + searchParams: searchParams, + }; + + const payload = queryPayload?.(pageParams); + + return { + queryKey: queryKey({ ...pageParams, payload }), + queryFn: (context) => { + return outerQueryFn({ ...context, ...pageParams, payload }); + }, + } as const; + }; + + const query = useQuery({ + ...extraOptions, + ...getQueryOptionsFromPage(currentPage), + keepPreviousData: true, + }); + + const totalRecords = query.data?.count ?? 0; + const totalPages = Math.ceil(totalRecords / pageSize); + const hasNextPage = pageSize * pageOffset < totalRecords; + const hasPreviousPage = currentPage > 1; + + const queryClient = useQueryClient(); + const prefetchPage = useEffectEvent((newPage: number) => { + return queryClient.prefetchQuery(getQueryOptionsFromPage(newPage)); + }); + + useEffect(() => { + if (hasNextPage) { + void prefetchPage(currentPage + 1); + } + }, [prefetchPage, currentPage, hasNextPage]); + + useEffect(() => { + if (hasPreviousPage) { + void prefetchPage(currentPage - 1); + } + }, [prefetchPage, currentPage, hasPreviousPage]); + + // Mainly here to catch user if they navigate to a page directly via URL + const updatePageIfInvalid = useEffectEvent(() => { + const clamped = clamp(currentPage, 1, totalPages); + if (currentPage === clamped) { + return; + } + + if (onInvalidPage === undefined) { + searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); + setSearchParams(searchParams); + } else { + const params = { + offset: pageOffset, + limit: pageSize, + totalPages, + setSearchParams, + pageNumber: currentPage, + searchParams: searchParams, + }; + + onInvalidPage(params); + } + }); + + useEffect(() => { + if (!query.isFetching) { + updatePageIfInvalid(); + } + }, [updatePageIfInvalid, query.isFetching]); + + const onPageChange = (newPage: number) => { + const safePage = Number.isInteger(newPage) + ? clamp(newPage, 1, totalPages) + : 1; + + searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(safePage)); + setSearchParams(searchParams); + }; + + return { + ...query, + onPageChange, + goToPreviousPage: () => onPageChange(currentPage - 1), + goToNextPage: () => onPageChange(currentPage + 1), + currentPage, + pageSize, + totalRecords, + hasNextPage, + hasPreviousPage, + isLoading: query.isLoading || query.isFetching, + } as const; +} diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 719047484d8a7..7b84be813b11a 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -101,15 +101,15 @@ export function usePaginatedQuery< const [searchParams, setSearchParams] = useSearchParams(); const currentPage = parsePage(searchParams); - const pageSize = DEFAULT_RECORDS_PER_PAGE; - const pageOffset = (currentPage - 1) * pageSize; + const limit = DEFAULT_RECORDS_PER_PAGE; + const offset = (currentPage - 1) * limit; const getQueryOptionsFromPage = (pageNumber: number) => { const pageParams: QueryPageParams = { pageNumber, - pageOffset, - pageSize, - searchParams: searchParams, + offset, + limit, + searchParams, }; const payload = queryPayload?.(pageParams) as RuntimePayload; @@ -132,8 +132,8 @@ export function usePaginatedQuery< }); const totalRecords = query.data?.count ?? 0; - const totalPages = Math.ceil(totalRecords / pageSize); - const hasNextPage = pageSize * pageOffset < totalRecords; + const totalPages = Math.ceil(totalRecords / limit); + const hasNextPage = limit * offset < totalRecords; const hasPreviousPage = currentPage > 1; const queryClient = useQueryClient(); @@ -169,8 +169,8 @@ export function usePaginatedQuery< setSearchParams(searchParams); } else { const params: InvalidPageParams = { - pageOffset, - pageSize, + offset: offset, + limit: limit, totalPages, setSearchParams, pageNumber: currentPage, @@ -202,7 +202,7 @@ export function usePaginatedQuery< goToPreviousPage: () => onPageChange(currentPage - 1), goToNextPage: () => onPageChange(currentPage + 1), currentPage, - pageSize, + pageSize: limit, totalRecords, hasNextPage, hasPreviousPage, @@ -254,9 +254,29 @@ type QueryPayloadExtender = [TQueryPayload] extends [ * queryPayload, queryKey, and queryFn properties of the hook. */ type QueryPageParams = { + /** + * The page number used when evaluating queryKey and queryFn. pageNumber will + * be the current page during rendering, but will be the next/previous pages + * for any prefetching. + */ pageNumber: number; - pageSize: number; - pageOffset: number; + + /** + * The number of data records to pull per query. Currently hard-coded based + * off the value from PaginationWidget's utils file + */ + limit: number; + + /** + * The page offset to use for querying. Just here for convenience; can also be + * derived from pageNumber and limit + */ + offset: number; + + /** + * The current URL search params. Useful for letting you grab certain search + * terms from the URL + */ searchParams: URLSearchParams; }; From a43e29456006bb3083f415653432eb74f45a59b6 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Mon, 20 Nov 2023 23:49:45 +0000 Subject: [PATCH 31/63] refactor: clean up input validation for page changes --- .../PaginationWidget/usePaginatedQuery.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 7b84be813b11a..9cdb09cc7a075 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -59,6 +59,10 @@ export type UsePaginatedQueryOptions< onInvalidPage?: (params: InvalidPageParams) => void; }; +/** + * The result of calling usePaginatedQuery. Mirrors the result of the base + * useQuery as closely as possible, while adding extra pagination properties + */ export type UsePaginatedQueryResult = Omit< UseQueryResult, "isLoading" @@ -188,11 +192,12 @@ export function usePaginatedQuery< }, [updatePageIfInvalid, query.isFetching]); const onPageChange = (newPage: number) => { - const safePage = Number.isInteger(newPage) - ? clamp(newPage, 1, totalPages) - : 1; + const cleanedInput = clamp(Math.trunc(newPage), 1, totalPages); + if (!Number.isInteger(cleanedInput) || cleanedInput <= 0) { + return; + } - searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(safePage)); + searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(cleanedInput)); setSearchParams(searchParams); }; From db03f3e870f2d807be3abff06930cf51ce73a367 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 01:03:19 +0000 Subject: [PATCH 32/63] refactor/fix: update hook to be aware of async data --- .../PaginationWidget/usePaginatedQuery.ts | 126 +++++++++++------- 1 file changed, 81 insertions(+), 45 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 9cdb09cc7a075..edae14dbd20e1 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -1,8 +1,6 @@ import { useEffect } from "react"; import { useEffectEvent } from "hooks/hookPolyfills"; import { type SetURLSearchParams, useSearchParams } from "react-router-dom"; - -import { DEFAULT_RECORDS_PER_PAGE } from "./utils"; import { clamp } from "lodash"; import { @@ -14,6 +12,8 @@ import { UseQueryResult, } from "react-query"; +const DEFAULT_RECORDS_PER_PAGE = 25; + /** * The key to use for getting/setting the page number from the search params */ @@ -56,29 +56,38 @@ export type UsePaginatedQueryOptions< * encountered, usePaginatedQuery will default to navigating the user to the * closest valid page. */ - onInvalidPage?: (params: InvalidPageParams) => void; + onInvalidPageChange?: (params: InvalidPageParams) => void; }; /** * The result of calling usePaginatedQuery. Mirrors the result of the base * useQuery as closely as possible, while adding extra pagination properties */ -export type UsePaginatedQueryResult = Omit< - UseQueryResult, - "isLoading" -> & { - isLoading: boolean; - hasNextPage: boolean; - hasPreviousPage: boolean; - +export type UsePaginatedQueryResult< + TData = unknown, + TError = unknown, +> = UseQueryResult & { currentPage: number; - pageSize: number; - totalRecords: number; - + limit: number; onPageChange: (newPage: number) => void; goToPreviousPage: () => void; goToNextPage: () => void; -}; +} & ( + | { + isSuccess: true; + hasNextPage: false; + hasPreviousPage: false; + totalRecords: undefined; + totalPages: undefined; + } + | { + isSuccess: false; + hasNextPage: boolean; + hasPreviousPage: boolean; + totalRecords: number; + totalPages: number; + } + ); export function usePaginatedQuery< TQueryFnData extends PaginatedData = PaginatedData, @@ -98,7 +107,7 @@ export function usePaginatedQuery< const { queryKey, queryPayload, - onInvalidPage, + onInvalidPageChange, queryFn: outerQueryFn, ...extraOptions } = options; @@ -135,10 +144,13 @@ export function usePaginatedQuery< keepPreviousData: true, }); - const totalRecords = query.data?.count ?? 0; - const totalPages = Math.ceil(totalRecords / limit); - const hasNextPage = limit * offset < totalRecords; - const hasPreviousPage = currentPage > 1; + const totalRecords = query.data?.count; + const totalPages = + totalRecords !== undefined ? Math.ceil(totalRecords / limit) : undefined; + + const hasPreviousPage = totalPages !== undefined && currentPage > 1; + const hasNextPage = + totalRecords !== undefined && limit * offset < totalRecords; const queryClient = useQueryClient(); const prefetchPage = useEffectEvent((newPage: number) => { @@ -162,36 +174,40 @@ export function usePaginatedQuery< }, [prefetchPage, currentPage, hasPreviousPage]); // Mainly here to catch user if they navigate to a page directly via URL - const updatePageIfInvalid = useEffectEvent(() => { + const updatePageIfInvalid = useEffectEvent((totalPages: number) => { const clamped = clamp(currentPage, 1, totalPages); if (currentPage === clamped) { return; } - if (onInvalidPage === undefined) { + if (onInvalidPageChange === undefined) { searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); setSearchParams(searchParams); } else { const params: InvalidPageParams = { - offset: offset, - limit: limit, + offset, + limit, totalPages, + searchParams, setSearchParams, pageNumber: currentPage, - searchParams: searchParams, }; - onInvalidPage(params); + onInvalidPageChange(params); } }); useEffect(() => { - if (!query.isFetching) { - updatePageIfInvalid(); + if (!query.isFetching && totalPages !== undefined) { + updatePageIfInvalid(totalPages); } - }, [updatePageIfInvalid, query.isFetching]); + }, [updatePageIfInvalid, query.isFetching, totalPages]); const onPageChange = (newPage: number) => { + if (totalPages === undefined) { + return; + } + const cleanedInput = clamp(Math.trunc(newPage), 1, totalPages); if (!Number.isInteger(cleanedInput) || cleanedInput <= 0) { return; @@ -201,24 +217,44 @@ export function usePaginatedQuery< setSearchParams(searchParams); }; + const goToPreviousPage = () => { + if (hasPreviousPage) { + onPageChange(currentPage - 1); + } + }; + + const goToNextPage = () => { + if (hasNextPage) { + onPageChange(currentPage + 1); + } + }; + return { ...query, - onPageChange, - goToPreviousPage: () => onPageChange(currentPage - 1), - goToNextPage: () => onPageChange(currentPage + 1), + limit, currentPage, - pageSize: limit, - totalRecords, - hasNextPage, - hasPreviousPage, - - // Hijacking the isLoading property slightly because keepPreviousData is - // true; by default, isLoading will always be false after the initial page - // loads, even if new pages are loading in. Especially since - // keepPreviousData is an implementation detail, simplifying the API felt - // like the better option, at the risk of it becoming more "magical" - isLoading: query.isLoading || query.isFetching, - } as const; + onPageChange, + goToPreviousPage, + goToNextPage, + + ...(query.isSuccess + ? { + hasNextPage, + hasPreviousPage, + totalRecords: totalRecords as number, + totalPages: totalPages as number, + } + : { + hasNextPage: false, + hasPreviousPage: false, + totalRecords: undefined, + totalPages: undefined, + }), + + // Have to do assertion to make TypeScript happy with React Query internal + // type, but this means that you won't get feedback from the compiler if you + // set up a property the wrong way + } as UsePaginatedQueryResult; } function parsePage(params: URLSearchParams): number { @@ -344,7 +380,7 @@ type BasePaginationOptions< >; /** - * The argument passed to a custom onInvalidPage callback. + * The argument passed to a custom onInvalidPageChange callback. */ type InvalidPageParams = QueryPageParams & { totalPages: number; From 1b825f1fea9f5edb07ffd11b64872999c16511c0 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 13:42:09 +0000 Subject: [PATCH 33/63] chore: add contravariance to dictionary --- .vscode/settings.json | 1 + 1 file changed, 1 insertion(+) diff --git a/.vscode/settings.json b/.vscode/settings.json index 6f726162d260a..8af33c71cac6a 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -18,6 +18,7 @@ "coderdenttest", "coderdtest", "codersdk", + "contravariance", "cronstrue", "databasefake", "dbmem", From 9631a27f16e3d173736e4ce19f0cd56d530e1374 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 13:42:43 +0000 Subject: [PATCH 34/63] refactor: increase type-safety of usePaginatedQuery --- .../PaginationWidget/usePaginatedQuery.ts | 61 ++++++++++--------- 1 file changed, 32 insertions(+), 29 deletions(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index edae14dbd20e1..4a821dc6883ab 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -66,28 +66,7 @@ export type UsePaginatedQueryOptions< export type UsePaginatedQueryResult< TData = unknown, TError = unknown, -> = UseQueryResult & { - currentPage: number; - limit: number; - onPageChange: (newPage: number) => void; - goToPreviousPage: () => void; - goToNextPage: () => void; -} & ( - | { - isSuccess: true; - hasNextPage: false; - hasPreviousPage: false; - totalRecords: undefined; - totalPages: undefined; - } - | { - isSuccess: false; - hasNextPage: boolean; - hasPreviousPage: boolean; - totalRecords: number; - totalPages: number; - } - ); +> = UseQueryResult & PaginationResultInfo; export function usePaginatedQuery< TQueryFnData extends PaginatedData = PaginatedData, @@ -229,32 +208,33 @@ export function usePaginatedQuery< } }; - return { - ...query, + // Have to do a type assertion at the end to make React Query's internal types + // happy; splitting type definitions up to limit risk of the type assertion + // silencing type warnings we actually want to pay attention to + const info: PaginationResultInfo = { limit, currentPage, onPageChange, goToPreviousPage, goToNextPage, - ...(query.isSuccess ? { + isSuccess: true, hasNextPage, hasPreviousPage, totalRecords: totalRecords as number, totalPages: totalPages as number, } : { + isSuccess: false, hasNextPage: false, hasPreviousPage: false, totalRecords: undefined, totalPages: undefined, }), + }; - // Have to do assertion to make TypeScript happy with React Query internal - // type, but this means that you won't get feedback from the compiler if you - // set up a property the wrong way - } as UsePaginatedQueryResult; + return { ...query, ...info } as UsePaginatedQueryResult; } function parsePage(params: URLSearchParams): number { @@ -262,6 +242,29 @@ function parsePage(params: URLSearchParams): number { return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } +type PaginationResultInfo = { + currentPage: number; + limit: number; + onPageChange: (newPage: number) => void; + goToPreviousPage: () => void; + goToNextPage: () => void; +} & ( + | { + isSuccess: false; + hasNextPage: false; + hasPreviousPage: false; + totalRecords: undefined; + totalPages: undefined; + } + | { + isSuccess: true; + hasNextPage: boolean; + hasPreviousPage: boolean; + totalRecords: number; + totalPages: number; + } +); + /** * Papers over how the queryPayload function is defined at the type level, so * that UsePaginatedQueryOptions doesn't look as scary. From 4ea0cba403f1c865a4a32b9f0b6b9441cd09a30e Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 14:03:33 +0000 Subject: [PATCH 35/63] docs: more comments --- site/src/components/PaginationWidget/usePaginatedQuery.ts | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/components/PaginationWidget/usePaginatedQuery.ts index 4a821dc6883ab..f2596e17d0f66 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/components/PaginationWidget/usePaginatedQuery.ts @@ -7,9 +7,9 @@ import { type QueryFunctionContext, type QueryKey, type UseQueryOptions, + type UseQueryResult, useQueryClient, useQuery, - UseQueryResult, } from "react-query"; const DEFAULT_RECORDS_PER_PAGE = 25; @@ -24,6 +24,8 @@ const PAGE_NUMBER_PARAMS_KEY = "page"; * paginated queries. */ export type UsePaginatedQueryOptions< + // Aside from TQueryPayload, all type parameters come from the base React + // Query type definition, and are here for compatibility TQueryFnData extends PaginatedData = PaginatedData, TQueryPayload = never, TError = unknown, @@ -242,6 +244,10 @@ function parsePage(params: URLSearchParams): number { return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } +/** + * All the pagination-properties for UsePaginatedQueryResult. Split up so that + * the types can be used separately in multiple spots. + */ type PaginationResultInfo = { currentPage: number; limit: number; From 3f63ec7065f53aa75c050a9e4fb88cdfdc620b74 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 14:06:12 +0000 Subject: [PATCH 36/63] chore: move usePaginatedQuery file --- .../{components/PaginationWidget => hooks}/usePaginatedQuery.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename site/src/{components/PaginationWidget => hooks}/usePaginatedQuery.ts (99%) diff --git a/site/src/components/PaginationWidget/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts similarity index 99% rename from site/src/components/PaginationWidget/usePaginatedQuery.ts rename to site/src/hooks/usePaginatedQuery.ts index f2596e17d0f66..46a1f2ffea4c1 100644 --- a/site/src/components/PaginationWidget/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -1,5 +1,5 @@ import { useEffect } from "react"; -import { useEffectEvent } from "hooks/hookPolyfills"; +import { useEffectEvent } from "./hookPolyfills"; import { type SetURLSearchParams, useSearchParams } from "react-router-dom"; import { clamp } from "lodash"; From 614e4ffa40ead22f18b06527330fd4e4a61cba56 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 14:07:49 +0000 Subject: [PATCH 37/63] fix: add back cacheTime --- site/src/api/queries/users.ts | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 3db52dab06cf8..2d490df2de38e 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -10,7 +10,7 @@ import type { } from "api/typesGenerated"; import { getAuthorizationKey } from "./authCheck"; import { getMetadataAsJSON } from "utils/metadata"; -import { UsePaginatedQueryOptions } from "components/PaginationWidget/usePaginatedQuery"; +import { type UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; import { prepareQuery } from "utils/filters"; export function usersKey(req: UsersRequest) { @@ -29,6 +29,7 @@ export function paginatedUsers() { queryKey: ({ payload }) => usersKey(payload), queryFn: ({ payload, signal }) => API.getUsers(payload, signal), + cacheTime: 5 * 1000 * 60, } as const satisfies UsePaginatedQueryOptions; } @@ -36,6 +37,7 @@ export const users = (req: UsersRequest): UseQueryOptions => { return { queryKey: usersKey(req), queryFn: ({ signal }) => API.getUsers(req, signal), + cacheTime: 5 * 1000 * 60, }; }; From e994532a038dac7a0050e058d15a4d8aaca10277 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 14:20:25 +0000 Subject: [PATCH 38/63] chore: swap in usePaginatedQuery for users table --- site/src/components/PaginationWidget/temp.ts | 112 ------------------- site/src/hooks/usePaginatedQuery.ts | 6 +- site/src/pages/UsersPage/UsersPage.tsx | 32 ++---- 3 files changed, 15 insertions(+), 135 deletions(-) delete mode 100644 site/src/components/PaginationWidget/temp.ts diff --git a/site/src/components/PaginationWidget/temp.ts b/site/src/components/PaginationWidget/temp.ts deleted file mode 100644 index 03025d8e2fe4b..0000000000000 --- a/site/src/components/PaginationWidget/temp.ts +++ /dev/null @@ -1,112 +0,0 @@ -export function usePaginatedQuery(options: any) { - const { - queryKey, - queryPayload, - onInvalidPage, - queryFn: outerQueryFn, - ...extraOptions - } = options; - - const [searchParams, setSearchParams] = useSearchParams(); - const currentPage = parsePage(searchParams); - const pageSize = DEFAULT_RECORDS_PER_PAGE; - const pageOffset = (currentPage - 1) * pageSize; - - const getQueryOptionsFromPage = (pageNumber: number) => { - const pageParams = { - pageNumber, - offset: pageOffset, - limit: pageSize, - searchParams: searchParams, - }; - - const payload = queryPayload?.(pageParams); - - return { - queryKey: queryKey({ ...pageParams, payload }), - queryFn: (context) => { - return outerQueryFn({ ...context, ...pageParams, payload }); - }, - } as const; - }; - - const query = useQuery({ - ...extraOptions, - ...getQueryOptionsFromPage(currentPage), - keepPreviousData: true, - }); - - const totalRecords = query.data?.count ?? 0; - const totalPages = Math.ceil(totalRecords / pageSize); - const hasNextPage = pageSize * pageOffset < totalRecords; - const hasPreviousPage = currentPage > 1; - - const queryClient = useQueryClient(); - const prefetchPage = useEffectEvent((newPage: number) => { - return queryClient.prefetchQuery(getQueryOptionsFromPage(newPage)); - }); - - useEffect(() => { - if (hasNextPage) { - void prefetchPage(currentPage + 1); - } - }, [prefetchPage, currentPage, hasNextPage]); - - useEffect(() => { - if (hasPreviousPage) { - void prefetchPage(currentPage - 1); - } - }, [prefetchPage, currentPage, hasPreviousPage]); - - // Mainly here to catch user if they navigate to a page directly via URL - const updatePageIfInvalid = useEffectEvent(() => { - const clamped = clamp(currentPage, 1, totalPages); - if (currentPage === clamped) { - return; - } - - if (onInvalidPage === undefined) { - searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); - setSearchParams(searchParams); - } else { - const params = { - offset: pageOffset, - limit: pageSize, - totalPages, - setSearchParams, - pageNumber: currentPage, - searchParams: searchParams, - }; - - onInvalidPage(params); - } - }); - - useEffect(() => { - if (!query.isFetching) { - updatePageIfInvalid(); - } - }, [updatePageIfInvalid, query.isFetching]); - - const onPageChange = (newPage: number) => { - const safePage = Number.isInteger(newPage) - ? clamp(newPage, 1, totalPages) - : 1; - - searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(safePage)); - setSearchParams(searchParams); - }; - - return { - ...query, - onPageChange, - goToPreviousPage: () => onPageChange(currentPage - 1), - goToNextPage: () => onPageChange(currentPage + 1), - currentPage, - pageSize, - totalRecords, - hasNextPage, - hasPreviousPage, - isLoading: query.isLoading || query.isFetching, - } as const; -} diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 46a1f2ffea4c1..304124f21e9b5 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -185,11 +185,13 @@ export function usePaginatedQuery< }, [updatePageIfInvalid, query.isFetching, totalPages]); const onPageChange = (newPage: number) => { - if (totalPages === undefined) { + // Page 1 is the only page that can be safely navigated to without knowing + // totalPages; no reliance on server data for math calculations + if (totalPages === undefined && newPage !== 1) { return; } - const cleanedInput = clamp(Math.trunc(newPage), 1, totalPages); + const cleanedInput = clamp(Math.trunc(newPage), 1, totalPages ?? 1); if (!Number.isInteger(cleanedInput) || cleanedInput <= 0) { return; } diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index 83f271cbe089b..48d3130915125 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -6,25 +6,24 @@ import { groupsByUserId } from "api/queries/groups"; import { getErrorMessage } from "api/errors"; import { deploymentConfig } from "api/queries/deployment"; import { - users, suspendUser, activateUser, deleteUser, updatePassword, updateRoles, authMethods, + paginatedUsers, } from "api/queries/users"; import { useMutation, useQuery, useQueryClient } from "react-query"; import { useSearchParams, useNavigate } from "react-router-dom"; -import { useOrganizationId, usePagination } from "hooks"; +import { useOrganizationId } from "hooks"; import { useMe } from "hooks/useMe"; import { usePermissions } from "hooks/usePermissions"; import { useStatusFilterMenu } from "./UsersFilter"; import { useFilter } from "components/Filter/filter"; import { useDashboard } from "components/Dashboard/DashboardProvider"; import { generateRandomString } from "utils/random"; -import { prepareQuery } from "utils/filters"; import { Helmet } from "react-helmet-async"; import { DeleteDialog } from "components/Dialogs/DeleteDialog/DeleteDialog"; @@ -34,6 +33,7 @@ import { ResetPasswordDialog } from "./ResetPasswordDialog"; import { pageTitle } from "utils/page"; import { UsersPageView } from "./UsersPageView"; import { displayError, displaySuccess } from "components/GlobalSnackbar/utils"; +import { usePaginatedQuery } from "hooks/usePaginatedQuery"; export const UsersPage: FC<{ children?: ReactNode }> = () => { const queryClient = useQueryClient(); @@ -43,20 +43,11 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { const { entitlements } = useDashboard(); const [searchParams] = searchParamsResult; - const pagination = usePagination({ searchParamsResult }); - const usersQuery = useQuery({ - ...users({ - q: prepareQuery(searchParams.get("filter") ?? ""), - limit: pagination.limit, - offset: pagination.offset, - }), - keepPreviousData: true, - }); - const organizationId = useOrganizationId(); const groupsByUserIdQuery = useQuery(groupsByUserId(organizationId)); const authMethodsQuery = useQuery(authMethods()); + const me = useMe(); const { updateUsers: canEditUsers, viewDeploymentValues } = usePermissions(); const rolesQuery = useQuery(roles()); const { data: deploymentValues } = useQuery({ @@ -64,13 +55,12 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { enabled: viewDeploymentValues, }); - const me = useMe(); + const usersQuery = usePaginatedQuery(paginatedUsers()); const useFilterResult = useFilter({ searchParamsResult, - onUpdate: () => { - pagination.goToPage(1); - }, + onUpdate: () => usersQuery.onPageChange(1), }); + const statusMenu = useStatusFilterMenu({ value: useFilterResult.values.status, onChange: (option) => @@ -165,10 +155,10 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { error: usersQuery.error, menus: { status: statusMenu }, }} - count={usersQuery.data?.count} - page={pagination.page} - limit={pagination.limit} - onPageChange={pagination.goToPage} + count={usersQuery.totalRecords} + page={usersQuery.currentPage} + limit={usersQuery.limit} + onPageChange={usersQuery.onPageChange} /> Date: Tue, 21 Nov 2023 14:47:24 +0000 Subject: [PATCH 39/63] chore: add goToFirstPage to usePaginatedQuery --- site/src/hooks/usePaginatedQuery.ts | 35 +++++++++++++------------- site/src/pages/UsersPage/UsersPage.tsx | 4 +-- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 304124f21e9b5..d2d9803a0eb5b 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -200,27 +200,27 @@ export function usePaginatedQuery< setSearchParams(searchParams); }; - const goToPreviousPage = () => { - if (hasPreviousPage) { - onPageChange(currentPage - 1); - } - }; - - const goToNextPage = () => { - if (hasNextPage) { - onPageChange(currentPage + 1); - } - }; - - // Have to do a type assertion at the end to make React Query's internal types - // happy; splitting type definitions up to limit risk of the type assertion - // silencing type warnings we actually want to pay attention to + // Have to do a type assertion for final return type to make React Query's + // internal types happy; splitting type definitions up to limit risk of the + // type assertion silencing type warnings we actually want to pay attention to const info: PaginationResultInfo = { limit, currentPage, onPageChange, - goToPreviousPage, - goToNextPage, + goToFirstPage: () => onPageChange(1), + + goToPreviousPage: () => { + if (hasPreviousPage) { + onPageChange(currentPage - 1); + } + }, + + goToNextPage: () => { + if (hasNextPage) { + onPageChange(currentPage + 1); + } + }, + ...(query.isSuccess ? { isSuccess: true, @@ -256,6 +256,7 @@ type PaginationResultInfo = { onPageChange: (newPage: number) => void; goToPreviousPage: () => void; goToNextPage: () => void; + goToFirstPage: () => void; } & ( | { isSuccess: false; diff --git a/site/src/pages/UsersPage/UsersPage.tsx b/site/src/pages/UsersPage/UsersPage.tsx index 48d3130915125..ef366b8787d5d 100644 --- a/site/src/pages/UsersPage/UsersPage.tsx +++ b/site/src/pages/UsersPage/UsersPage.tsx @@ -6,13 +6,13 @@ import { groupsByUserId } from "api/queries/groups"; import { getErrorMessage } from "api/errors"; import { deploymentConfig } from "api/queries/deployment"; import { + paginatedUsers, suspendUser, activateUser, deleteUser, updatePassword, updateRoles, authMethods, - paginatedUsers, } from "api/queries/users"; import { useMutation, useQuery, useQueryClient } from "react-query"; @@ -58,7 +58,7 @@ export const UsersPage: FC<{ children?: ReactNode }> = () => { const usersQuery = usePaginatedQuery(paginatedUsers()); const useFilterResult = useFilter({ searchParamsResult, - onUpdate: () => usersQuery.onPageChange(1), + onUpdate: usersQuery.goToFirstPage, }); const statusMenu = useStatusFilterMenu({ From 8dbbfd37e39ed414da4039d2b92bc98ef2f5a75b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 15:30:17 +0000 Subject: [PATCH 40/63] fix: make page redirects work properly --- site/src/hooks/usePaginatedQuery.ts | 41 ++++++++++++++++++++++------- 1 file changed, 32 insertions(+), 9 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index d2d9803a0eb5b..257e8d2bfa8f3 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -101,9 +101,9 @@ export function usePaginatedQuery< const getQueryOptionsFromPage = (pageNumber: number) => { const pageParams: QueryPageParams = { pageNumber, - offset, limit, - searchParams, + offset: (pageNumber - 1) * limit, + searchParams: getParamsWithoutPage(searchParams), }; const payload = queryPayload?.(pageParams) as RuntimePayload; @@ -155,22 +155,34 @@ export function usePaginatedQuery< }, [prefetchPage, currentPage, hasPreviousPage]); // Mainly here to catch user if they navigate to a page directly via URL - const updatePageIfInvalid = useEffectEvent((totalPages: number) => { - const clamped = clamp(currentPage, 1, totalPages); + const updatePageIfInvalid = useEffectEvent(async (totalPages: number) => { + // If totalPages is 0, that's a sign that the currentPage overshot, and the + // API returned a count of 0 because it didn't know how to process the query + let fixedTotalPages: number; + if (totalPages !== 0) { + fixedTotalPages = totalPages; + } else { + const firstPageOptions = getQueryOptionsFromPage(1); + const firstPageResult = await queryClient.fetchQuery(firstPageOptions); + fixedTotalPages = Math.ceil(firstPageResult.count / limit); + } + + const clamped = clamp(currentPage, 1, fixedTotalPages || 1); if (currentPage === clamped) { return; } + const withoutPage = getParamsWithoutPage(searchParams); if (onInvalidPageChange === undefined) { - searchParams.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); - setSearchParams(searchParams); + withoutPage.set(PAGE_NUMBER_PARAMS_KEY, String(clamped)); + setSearchParams(withoutPage); } else { const params: InvalidPageParams = { offset, limit, - totalPages, - searchParams, setSearchParams, + searchParams: withoutPage, + totalPages: fixedTotalPages, pageNumber: currentPage, }; @@ -180,7 +192,7 @@ export function usePaginatedQuery< useEffect(() => { if (!query.isFetching && totalPages !== undefined) { - updatePageIfInvalid(totalPages); + void updatePageIfInvalid(totalPages); } }, [updatePageIfInvalid, query.isFetching, totalPages]); @@ -246,6 +258,17 @@ function parsePage(params: URLSearchParams): number { return Number.isInteger(parsed) && parsed > 1 ? parsed : 1; } +/** + * Strips out the page number from a query so that there aren't mismatches + * between it and usePaginatedQuery's currentPage property (especially for + * prefetching) + */ +function getParamsWithoutPage(params: URLSearchParams): URLSearchParams { + const withoutPage = new URLSearchParams(params); + withoutPage.delete(PAGE_NUMBER_PARAMS_KEY); + return withoutPage; +} + /** * All the pagination-properties for UsePaginatedQueryResult. Split up so that * the types can be used separately in multiple spots. From 88d0a5fd342c83116c4187349961f074ecb62781 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 15:40:19 +0000 Subject: [PATCH 41/63] refactor: clean up clamp logic --- site/src/hooks/usePaginatedQuery.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 257e8d2bfa8f3..a21a44f69213b 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -154,7 +154,8 @@ export function usePaginatedQuery< } }, [prefetchPage, currentPage, hasPreviousPage]); - // Mainly here to catch user if they navigate to a page directly via URL + // Mainly here to catch user if they navigate to a page directly via URL; + // totalPages parameterized to insulate function from fetch status changes const updatePageIfInvalid = useEffectEvent(async (totalPages: number) => { // If totalPages is 0, that's a sign that the currentPage overshot, and the // API returned a count of 0 because it didn't know how to process the query @@ -167,7 +168,7 @@ export function usePaginatedQuery< fixedTotalPages = Math.ceil(firstPageResult.count / limit); } - const clamped = clamp(currentPage, 1, fixedTotalPages || 1); + const clamped = clamp(currentPage, 1, fixedTotalPages); if (currentPage === clamped) { return; } From 132f5b157447bd58e1fdb236c6c441b2f283b6ef Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 16:56:48 +0000 Subject: [PATCH 42/63] chore: swap in usePaginatedQuery for Audits table --- site/src/api/queries/audits.ts | 27 +++++++++++ site/src/components/Filter/filter.tsx | 2 +- site/src/hooks/usePaginatedQuery.ts | 13 ++++- site/src/pages/AuditPage/AuditPage.tsx | 67 ++++++++++++-------------- 4 files changed, 72 insertions(+), 37 deletions(-) create mode 100644 site/src/api/queries/audits.ts diff --git a/site/src/api/queries/audits.ts b/site/src/api/queries/audits.ts new file mode 100644 index 0000000000000..4590660996fc2 --- /dev/null +++ b/site/src/api/queries/audits.ts @@ -0,0 +1,27 @@ +import { getAuditLogs } from "api/api"; +import { type AuditLogResponse } from "api/typesGenerated"; +import { type UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; + +export function paginatedAudits( + searchParams: URLSearchParams, + filterParamsKey: string, +) { + return { + searchParams, + queryPayload: ({ searchParams }) => { + return searchParams.get(filterParamsKey) ?? ""; + }, + queryKey: ({ payload, pageNumber }) => { + return ["auditLogs", payload, pageNumber] as const; + }, + queryFn: ({ payload, limit, offset }) => { + return getAuditLogs({ + offset, + limit, + q: payload, + }); + }, + + cacheTime: 5 * 1000 * 60, + } as const satisfies UsePaginatedQueryOptions; +} diff --git a/site/src/components/Filter/filter.tsx b/site/src/components/Filter/filter.tsx index 1824554721b13..e0cee67d7241d 100644 --- a/site/src/components/Filter/filter.tsx +++ b/site/src/components/Filter/filter.tsx @@ -45,7 +45,7 @@ type UseFilterConfig = { onUpdate?: (newValue: string) => void; }; -const useFilterParamsKey = "filter"; +export const useFilterParamsKey = "filter"; export const useFilter = ({ fallbackFilter = "", diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index a21a44f69213b..e953b84bb5cb0 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -33,6 +33,14 @@ export type UsePaginatedQueryOptions< TQueryKey extends QueryKey = QueryKey, > = BasePaginationOptions & QueryPayloadExtender & { + /** + * An optional dependency for React Router's URLSearchParams. + * + * It's annoying that this is necessary, but this helps avoid searchParams + * from other parts of a component from de-syncing + */ + searchParams?: URLSearchParams; + /** * A function that takes pagination information and produces a full query * key. @@ -89,11 +97,14 @@ export function usePaginatedQuery< queryKey, queryPayload, onInvalidPageChange, + searchParams: outerSearchParams, queryFn: outerQueryFn, ...extraOptions } = options; - const [searchParams, setSearchParams] = useSearchParams(); + const [innerSearchParams, setSearchParams] = useSearchParams(); + const searchParams = outerSearchParams ?? innerSearchParams; + const currentPage = parsePage(searchParams); const limit = DEFAULT_RECORDS_PER_PAGE; const offset = (currentPage - 1) * limit; diff --git a/site/src/pages/AuditPage/AuditPage.tsx b/site/src/pages/AuditPage/AuditPage.tsx index 174bf517a480c..649b32553f545 100644 --- a/site/src/pages/AuditPage/AuditPage.tsx +++ b/site/src/pages/AuditPage/AuditPage.tsx @@ -1,7 +1,4 @@ -import { - DEFAULT_RECORDS_PER_PAGE, - isNonInitialPage, -} from "components/PaginationWidget/utils"; +import { isNonInitialPage } from "components/PaginationWidget/utils"; import { useFeatureVisibility } from "hooks/useFeatureVisibility"; import { FC } from "react"; import { Helmet } from "react-helmet-async"; @@ -9,21 +6,31 @@ import { useSearchParams } from "react-router-dom"; import { pageTitle } from "utils/page"; import { AuditPageView } from "./AuditPageView"; import { useUserFilterMenu } from "components/Filter/UserFilter"; -import { useFilter } from "components/Filter/filter"; -import { usePagination } from "hooks"; -import { useQuery } from "react-query"; -import { getAuditLogs } from "api/api"; +import { useFilter, useFilterParamsKey } from "components/Filter/filter"; import { useActionFilterMenu, useResourceTypeFilterMenu } from "./AuditFilter"; +import { usePaginatedQuery } from "hooks/usePaginatedQuery"; +import { paginatedAudits } from "api/queries/audits"; const AuditPage: FC = () => { - const searchParamsResult = useSearchParams(); - const pagination = usePagination({ searchParamsResult }); + const { audit_log: isAuditLogVisible } = useFeatureVisibility(); + + /** + * There is an implicit link between auditsQuery and filter via the + * searchParams object + * + * @todo Make link more explicit (probably by making it so that components + * and hooks can share the result of useSearchParams directly) + */ + const [searchParams, setSearchParams] = useSearchParams(); + const auditsQuery = usePaginatedQuery( + paginatedAudits(searchParams, useFilterParamsKey), + ); + const filter = useFilter({ - searchParamsResult, - onUpdate: () => { - pagination.goToPage(1); - }, + searchParamsResult: [searchParams, setSearchParams], + onUpdate: auditsQuery.goToFirstPage, }); + const userMenu = useUserFilterMenu({ value: filter.values.username, onChange: (option) => @@ -32,6 +39,7 @@ const AuditPage: FC = () => { username: option?.value, }), }); + const actionMenu = useActionFilterMenu({ value: filter.values.action, onChange: (option) => @@ -40,6 +48,7 @@ const AuditPage: FC = () => { action: option?.value, }), }); + const resourceTypeMenu = useResourceTypeFilterMenu({ value: filter.values["resource_type"], onChange: (option) => @@ -48,37 +57,25 @@ const AuditPage: FC = () => { resource_type: option?.value, }), }); - const { audit_log: isAuditLogVisible } = useFeatureVisibility(); - const { data, error } = useQuery({ - queryKey: ["auditLogs", filter.query, pagination.page], - queryFn: () => { - const limit = DEFAULT_RECORDS_PER_PAGE; - const page = pagination.page; - return getAuditLogs({ - offset: page <= 0 ? 0 : (page - 1) * limit, - limit: limit, - q: filter.query, - }); - }, - }); return ( <> {pageTitle("Audit")} + Date: Tue, 21 Nov 2023 17:00:29 +0000 Subject: [PATCH 43/63] refactor: move dependencies around --- site/src/api/queries/audits.ts | 8 +++----- site/src/pages/AuditPage/AuditPage.tsx | 7 ++----- 2 files changed, 5 insertions(+), 10 deletions(-) diff --git a/site/src/api/queries/audits.ts b/site/src/api/queries/audits.ts index 4590660996fc2..00d0f45e67abb 100644 --- a/site/src/api/queries/audits.ts +++ b/site/src/api/queries/audits.ts @@ -1,15 +1,13 @@ import { getAuditLogs } from "api/api"; import { type AuditLogResponse } from "api/typesGenerated"; +import { useFilterParamsKey } from "components/Filter/filter"; import { type UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; -export function paginatedAudits( - searchParams: URLSearchParams, - filterParamsKey: string, -) { +export function paginatedAudits(searchParams: URLSearchParams) { return { searchParams, queryPayload: ({ searchParams }) => { - return searchParams.get(filterParamsKey) ?? ""; + return searchParams.get(useFilterParamsKey) ?? ""; }, queryKey: ({ payload, pageNumber }) => { return ["auditLogs", payload, pageNumber] as const; diff --git a/site/src/pages/AuditPage/AuditPage.tsx b/site/src/pages/AuditPage/AuditPage.tsx index 649b32553f545..0be62a7e69f2b 100644 --- a/site/src/pages/AuditPage/AuditPage.tsx +++ b/site/src/pages/AuditPage/AuditPage.tsx @@ -6,7 +6,7 @@ import { useSearchParams } from "react-router-dom"; import { pageTitle } from "utils/page"; import { AuditPageView } from "./AuditPageView"; import { useUserFilterMenu } from "components/Filter/UserFilter"; -import { useFilter, useFilterParamsKey } from "components/Filter/filter"; +import { useFilter } from "components/Filter/filter"; import { useActionFilterMenu, useResourceTypeFilterMenu } from "./AuditFilter"; import { usePaginatedQuery } from "hooks/usePaginatedQuery"; import { paginatedAudits } from "api/queries/audits"; @@ -22,10 +22,7 @@ const AuditPage: FC = () => { * and hooks can share the result of useSearchParams directly) */ const [searchParams, setSearchParams] = useSearchParams(); - const auditsQuery = usePaginatedQuery( - paginatedAudits(searchParams, useFilterParamsKey), - ); - + const auditsQuery = usePaginatedQuery(paginatedAudits(searchParams)); const filter = useFilter({ searchParamsResult: [searchParams, setSearchParams], onUpdate: auditsQuery.goToFirstPage, From 218da68c66d6277449a7a24f5f0f931dfe26bd6d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 18:05:14 +0000 Subject: [PATCH 44/63] fix: remove deprecated properties from hook --- site/src/hooks/usePaginatedQuery.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index e953b84bb5cb0..3b257c3ab19c1 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -415,6 +415,7 @@ type PaginatedQueryFnContext< * definition with a custom context argument * - queryKey - Removed so that it can be replaced with the function form of * queryKey + * - onSuccess/onError - APIs are deprecated and removed in React Query v5 */ type BasePaginationOptions< TQueryFnData extends PaginatedData = PaginatedData, @@ -423,7 +424,7 @@ type BasePaginationOptions< TQueryKey extends QueryKey = QueryKey, > = Omit< UseQueryOptions, - "keepPreviousData" | "queryKey" | "queryFn" + "keepPreviousData" | "queryKey" | "queryFn" | "onSuccess" | "onError" >; /** From 54f01f1947dd87ef15dc2a534344d18bfc8eae8b Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 18:07:45 +0000 Subject: [PATCH 45/63] refactor: clean up code more --- site/src/api/queries/audits.ts | 4 +--- site/src/hooks/usePaginatedQuery.ts | 5 +++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/site/src/api/queries/audits.ts b/site/src/api/queries/audits.ts index 00d0f45e67abb..c0a758619f32b 100644 --- a/site/src/api/queries/audits.ts +++ b/site/src/api/queries/audits.ts @@ -6,9 +6,7 @@ import { type UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; export function paginatedAudits(searchParams: URLSearchParams) { return { searchParams, - queryPayload: ({ searchParams }) => { - return searchParams.get(useFilterParamsKey) ?? ""; - }, + queryPayload: () => searchParams.get(useFilterParamsKey) ?? "", queryKey: ({ payload, pageNumber }) => { return ["auditLogs", payload, pageNumber] as const; }, diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 3b257c3ab19c1..880ab1d870d59 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -36,8 +36,9 @@ export type UsePaginatedQueryOptions< /** * An optional dependency for React Router's URLSearchParams. * - * It's annoying that this is necessary, but this helps avoid searchParams - * from other parts of a component from de-syncing + * It's annoying that this is necessary, but it helps avoid URL de-syncs if + * useSearchParams is called multiple times in the same component (likely in + * multiple custom hooks) */ searchParams?: URLSearchParams; From ede2abc93110330349e259783b04c6dab3761cfb Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 18:14:04 +0000 Subject: [PATCH 46/63] docs: add todo comment --- site/src/hooks/usePaginatedQuery.ts | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 880ab1d870d59..362cf1aea7d39 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -39,6 +39,10 @@ export type UsePaginatedQueryOptions< * It's annoying that this is necessary, but it helps avoid URL de-syncs if * useSearchParams is called multiple times in the same component (likely in * multiple custom hooks) + * + * @todo Wrangle React Router's useSearchParams so that URL state can be + * shared between multiple components/hooks more directly without making you + * jump through so many hoops (it's affecting our filter logic, too) */ searchParams?: URLSearchParams; From 9ecab16573c70afce2c7ea6697bc70892083d43c Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 22:08:46 +0000 Subject: [PATCH 47/63] chore: update testing fixtures --- site/src/hooks/usePaginatedQuery.ts | 2 +- site/src/testHelpers/renderHelpers.tsx | 9 +++++---- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 362cf1aea7d39..317c885ff2be1 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -395,7 +395,7 @@ type QueryPageParamsWithPayload = QueryPageParams & { * Any JSON-serializable object returned by the API that exposes the total * number of records that match a query */ -type PaginatedData = { +export type PaginatedData = { count: number; }; diff --git a/site/src/testHelpers/renderHelpers.tsx b/site/src/testHelpers/renderHelpers.tsx index b07d4921bee0d..9b1f0cb27ef3f 100644 --- a/site/src/testHelpers/renderHelpers.tsx +++ b/site/src/testHelpers/renderHelpers.tsx @@ -125,6 +125,7 @@ export async function renderHookWithAuth( { initialProps, path = "/", + route = "/", extraRoutes = [], }: RenderHookWithAuthOptions = {}, ) { @@ -144,10 +145,10 @@ export async function renderHookWithAuth( */ // eslint-disable-next-line react-hooks/rules-of-hooks -- This is actually processed as a component; the linter just isn't aware of that const [readonlyStatefulRouter] = useState(() => { - return createMemoryRouter([ - { path, element: <>{children} }, - ...extraRoutes, - ]); + return createMemoryRouter( + [{ path, element: <>{children} }, ...extraRoutes], + { initialEntries: [route] }, + ); }); /** From 0c81d1c965c15c48284ce40191df86638a7bc25d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 22:40:17 +0000 Subject: [PATCH 48/63] wip: commit current progress for tests --- site/src/hooks/usePaginatedQuery.test.ts | 197 +++++++++++++++++++++++ 1 file changed, 197 insertions(+) create mode 100644 site/src/hooks/usePaginatedQuery.test.ts diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts new file mode 100644 index 0000000000000..14c7b27da9253 --- /dev/null +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -0,0 +1,197 @@ +import { renderHookWithAuth } from "testHelpers/renderHelpers"; +import { + type PaginatedData, + type UsePaginatedQueryOptions, + usePaginatedQuery, +} from "./usePaginatedQuery"; +import { waitFor } from "@testing-library/react"; + +beforeAll(() => { + jest.useFakeTimers(); +}); + +afterAll(() => { + jest.useRealTimers(); + jest.clearAllMocks(); +}); + +function render< + TQueryFnData extends PaginatedData = PaginatedData, + TQueryPayload = never, +>( + queryOptions: UsePaginatedQueryOptions, + route?: `/${string}`, +) { + type Props = { options: typeof queryOptions }; + + return renderHookWithAuth( + ({ options }: Props) => usePaginatedQuery(options), + { + route, + path: "/", + initialProps: { + options: queryOptions, + }, + }, + ); +} + +describe(usePaginatedQuery.name, () => { + describe("queryPayload method", () => { + const mockQueryFn = jest.fn(() => { + return { count: 0 }; + }); + + it("Passes along an undefined payload if queryPayload is not used", async () => { + const mockQueryKey = jest.fn(() => ["mockQuery"]); + + await render({ + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + const payloadValueMock = expect.objectContaining({ + payload: undefined, + }); + + expect(mockQueryKey).toHaveBeenCalledWith(payloadValueMock); + expect(mockQueryFn).toHaveBeenCalledWith(payloadValueMock); + }); + + it("Passes along type-safe payload if queryPayload is provided", async () => { + const mockQueryKey = jest.fn(({ payload }) => { + return ["mockQuery", payload]; + }); + + const testPayloadValues = [1, "Blah", { cool: true }]; + for (const payload of testPayloadValues) { + const { unmount } = await render({ + queryPayload: () => payload, + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + const matcher = expect.objectContaining({ payload }); + expect(mockQueryKey).toHaveBeenCalledWith(matcher); + expect(mockQueryFn).toHaveBeenCalledWith(matcher); + unmount(); + } + }); + }); + + describe("Querying for current page", () => { + const mockQueryKey = jest.fn(() => ["mock"]); + const mockQueryFn = jest.fn(() => Promise.resolve({ count: 50 })); + + it("Parses page number if it exists in URL params", async () => { + const pageNumbers = [1, 2, 7, 39, 743]; + + for (const num of pageNumbers) { + const { result, unmount } = await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${num}`, + ); + + expect(result.current.currentPage).toBe(num); + unmount(); + } + }); + + it("Defaults to page 1 if no page value can be parsed from params", async () => { + const { result } = await render({ + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + expect(result.current.currentPage).toBe(1); + }); + }); + + describe("Prefetching", () => { + const noPrefetchTimeout = 1000; + const mockQueryKey = jest.fn(({ pageNumber }) => ["query", pageNumber]); + const mockQueryFn = jest.fn(({ pageNumber, limit }) => { + return Promise.resolve({ + data: new Array(limit).fill(pageNumber), + count: 50, + }); + }); + + it("Prefetches the previous page if it exists", async () => { + const startingPage = 2; + await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${startingPage}`, + ); + + const pageMatcher = expect.objectContaining({ + pageNumber: 1, + }); + + await waitFor(() => expect(mockQueryFn).toBeCalledWith(pageMatcher)); + }); + + it("Prefetches the next page if it exists", async () => { + const startingPage = 1; + await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${startingPage}`, + ); + + const pageMatcher = expect.objectContaining({ + pageNumber: 2, + }); + + await waitFor(() => expect(mockQueryFn).toBeCalledWith(pageMatcher)); + }); + + it("Avoids prefetch for previous page if it doesn't exist", async () => { + const startingPage = 1; + await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${startingPage}`, + ); + + const pageMatcher = expect.objectContaining({ + pageNumber: 0, + }); + + // Can't use waitFor to test this, because the expect call will + // immediately succeed for the not case, even though queryFn needs to be + // called async via React Query + setTimeout(() => { + expect(mockQueryFn).not.toBeCalledWith(pageMatcher); + }, noPrefetchTimeout); + + jest.runAllTimers(); + }); + + it("Avoids prefetch for next page if it doesn't exist", async () => { + const startingPage = 2; + await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${startingPage}`, + ); + + const pageMatcher = expect.objectContaining({ + pageNumber: 3, + }); + + setTimeout(() => { + expect(mockQueryFn).not.toBeCalledWith(pageMatcher); + }, noPrefetchTimeout); + + jest.runAllTimers(); + }); + + it("Reuses the same queryKey and queryFn methods for the current page and all prefetching", async () => { + // + }); + }); + + describe("Invalid page safety nets/redirects", () => {}); + + describe("Returned properties", () => {}); + + describe("Passing outside value for URLSearchParams", () => {}); +}); From 3aa714c91c40b2dbdcd0080f1133788e6cbd6030 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 22:56:52 +0000 Subject: [PATCH 49/63] fix: update useEffectEvent to sync via layout effects --- site/src/hooks/hookPolyfills.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts index e5ba705296229..40ca8629c9d27 100644 --- a/site/src/hooks/hookPolyfills.ts +++ b/site/src/hooks/hookPolyfills.ts @@ -6,7 +6,7 @@ * 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"; +import { useCallback, useLayoutEffect, useRef } from "react"; /** * A DIY version of useEffectEvent. @@ -35,7 +35,10 @@ export function useEffectEvent( callback: (...args: TArgs) => TReturn, ) { const callbackRef = useRef(callback); - useEffect(() => { + + // useLayoutEffect should be overkill here 99% of the time, but it ensures it + // will run before any other layout effects that need this custom hook + useLayoutEffect(() => { callbackRef.current = callback; }, [callback]); From 289363050d99aec15d59a6974a764558bec9bafe Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 22:58:16 +0000 Subject: [PATCH 50/63] wip: commit more progress on tests --- site/src/hooks/usePaginatedQuery.test.ts | 101 +++++++++++------------ 1 file changed, 49 insertions(+), 52 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 14c7b27da9253..1652223552b0d 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -108,7 +108,6 @@ describe(usePaginatedQuery.name, () => { }); describe("Prefetching", () => { - const noPrefetchTimeout = 1000; const mockQueryKey = jest.fn(({ pageNumber }) => ["query", pageNumber]); const mockQueryFn = jest.fn(({ pageNumber, limit }) => { return Promise.resolve({ @@ -117,81 +116,79 @@ describe(usePaginatedQuery.name, () => { }); }); - it("Prefetches the previous page if it exists", async () => { - const startingPage = 2; + const testPrefetch = async ( + startingPage: number, + targetPage: number, + shouldMatch: boolean, + ) => { await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, `/?page=${startingPage}`, ); - const pageMatcher = expect.objectContaining({ - pageNumber: 1, - }); + const pageMatcher = expect.objectContaining({ pageNumber: targetPage }); + if (shouldMatch) { + await waitFor(() => expect(mockQueryFn).toBeCalledWith(pageMatcher)); + } else { + // Can't use waitFor to test this, because the expect call will + // immediately succeed for the not case, even though queryFn needs to be + // called async via React Query + setTimeout(() => { + expect(mockQueryFn).not.toBeCalledWith(pageMatcher); + }, 1000); + + jest.runAllTimers(); + } + }; - await waitFor(() => expect(mockQueryFn).toBeCalledWith(pageMatcher)); + it("Prefetches the previous page if it exists", async () => { + await testPrefetch(2, 1, true); }); it("Prefetches the next page if it exists", async () => { - const startingPage = 1; - await render( - { queryKey: mockQueryKey, queryFn: mockQueryFn }, - `/?page=${startingPage}`, - ); - - const pageMatcher = expect.objectContaining({ - pageNumber: 2, - }); - - await waitFor(() => expect(mockQueryFn).toBeCalledWith(pageMatcher)); + await testPrefetch(1, 2, true); }); it("Avoids prefetch for previous page if it doesn't exist", async () => { - const startingPage = 1; - await render( - { queryKey: mockQueryKey, queryFn: mockQueryFn }, - `/?page=${startingPage}`, - ); + await testPrefetch(1, 0, false); + }); - const pageMatcher = expect.objectContaining({ - pageNumber: 0, - }); + it("Avoids prefetch for next page if it doesn't exist", async () => { + await testPrefetch(2, 3, false); + }); - // Can't use waitFor to test this, because the expect call will - // immediately succeed for the not case, even though queryFn needs to be - // called async via React Query - setTimeout(() => { - expect(mockQueryFn).not.toBeCalledWith(pageMatcher); - }, noPrefetchTimeout); + it("Reuses the same queryKey and queryFn methods for the current page and all prefetching", async () => { + expect.hasAssertions(); + }); + }); - jest.runAllTimers(); + describe("Invalid page safety nets/redirects", () => { + it("Auto-redirects user to page 1 if params are corrupt/invalid", async () => { + expect.hasAssertions(); }); - it("Avoids prefetch for next page if it doesn't exist", async () => { - const startingPage = 2; - await render( - { queryKey: mockQueryKey, queryFn: mockQueryFn }, - `/?page=${startingPage}`, - ); + it("Auto-redirects user to closest page if request page overshoots", async () => { + expect.hasAssertions(); + }); - const pageMatcher = expect.objectContaining({ - pageNumber: 3, - }); + it("Auto-redirects user to first page if request page goes below 1", async () => { + expect.hasAssertions(); + }); - setTimeout(() => { - expect(mockQueryFn).not.toBeCalledWith(pageMatcher); - }, noPrefetchTimeout); + it("Calls the custom onInvalidPageChange callback if provided", async () => { + expect.hasAssertions(); + }); + }); - jest.runAllTimers(); + describe("Passing outside value for URLSearchParams", () => { + it("Reads from searchParams property if provided", async () => { + expect.hasAssertions(); }); - it("Reuses the same queryKey and queryFn methods for the current page and all prefetching", async () => { - // + it("Flushes state changes via provided searchParams property", async () => { + expect.hasAssertions(); }); }); - describe("Invalid page safety nets/redirects", () => {}); - describe("Returned properties", () => {}); - - describe("Passing outside value for URLSearchParams", () => {}); }); From ef900d4fa03cb8047934bc01880bbbd734995933 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Tue, 21 Nov 2023 23:19:01 +0000 Subject: [PATCH 51/63] wip: stub out all expected test cases --- site/src/hooks/usePaginatedQuery.test.ts | 51 ++++++++++++++++++++---- 1 file changed, 43 insertions(+), 8 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 1652223552b0d..4582faf38a607 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -1,10 +1,11 @@ import { renderHookWithAuth } from "testHelpers/renderHelpers"; +import { waitFor } from "@testing-library/react"; + import { type PaginatedData, type UsePaginatedQueryOptions, usePaginatedQuery, } from "./usePaginatedQuery"; -import { waitFor } from "@testing-library/react"; beforeAll(() => { jest.useFakeTimers(); @@ -36,11 +37,13 @@ function render< ); } -describe(usePaginatedQuery.name, () => { +/** + * There are a lot of test cases in this file. Scoping mocking to inner describe + * function calls to limit cognitive load of maintaining this file. + */ +describe(`${usePaginatedQuery.name} - Overall functionality`, () => { describe("queryPayload method", () => { - const mockQueryFn = jest.fn(() => { - return { count: 0 }; - }); + const mockQueryFn = jest.fn(() => Promise.resolve({ count: 0 })); it("Passes along an undefined payload if queryPayload is not used", async () => { const mockQueryKey = jest.fn(() => ["mockQuery"]); @@ -167,11 +170,11 @@ describe(usePaginatedQuery.name, () => { expect.hasAssertions(); }); - it("Auto-redirects user to closest page if request page overshoots", async () => { + it("Auto-redirects user to closest page if requested page overshoots", async () => { expect.hasAssertions(); }); - it("Auto-redirects user to first page if request page goes below 1", async () => { + it("Auto-redirects user to first page if requested page goes below 1", async () => { expect.hasAssertions(); }); @@ -189,6 +192,38 @@ describe(usePaginatedQuery.name, () => { expect.hasAssertions(); }); }); +}); + +describe(`${usePaginatedQuery.name} - Returned properties`, () => { + describe("Conditional render output", () => { + it("Always has select properties be defined regardless of fetch status", async () => { + expect.hasAssertions(); + }); + + it("Flips other properties to be defined after on-mount fetch succeeds", async () => { + expect.hasAssertions(); + }); + }); - describe("Returned properties", () => {}); + describe("Page change methods", () => { + test("goToFirstPage always succeeds regardless of fetch status", async () => { + expect.hasAssertions(); + }); + + test("goToNextPage works only if hasNextPage is true", async () => { + expect.hasAssertions(); + }); + + test("goToPreviousPage works only if hasPreviousPage is true", async () => { + expect.hasAssertions(); + }); + + test("onPageChange cleans 'corrupt' numeric values before navigating", async () => { + expect.hasAssertions(); + }); + + test("onPageChange rejects impossible numeric values and does nothing", async () => { + expect.hasAssertions(); + }); + }); }); From 23dc5830015a6b65ef9747558076d156087b0414 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 00:04:15 +0000 Subject: [PATCH 52/63] wip: more test progress --- site/src/hooks/usePaginatedQuery.test.ts | 93 ++++++++++++++++++------ 1 file changed, 70 insertions(+), 23 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 4582faf38a607..5555d3e9a8598 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -21,7 +21,7 @@ function render< TQueryPayload = never, >( queryOptions: UsePaginatedQueryOptions, - route?: `/${string}`, + route?: `/?page=${string}`, ) { type Props = { options: typeof queryOptions }; @@ -115,7 +115,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { const mockQueryFn = jest.fn(({ pageNumber, limit }) => { return Promise.resolve({ data: new Array(limit).fill(pageNumber), - count: 50, + count: 75, }); }); @@ -148,47 +148,94 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await testPrefetch(2, 1, true); }); - it("Prefetches the next page if it exists", async () => { - await testPrefetch(1, 2, true); + it.skip("Prefetches the next page if it exists", async () => { + await testPrefetch(2, 3, true); }); - it("Avoids prefetch for previous page if it doesn't exist", async () => { + it.skip("Avoids prefetch for previous page if it doesn't exist", async () => { await testPrefetch(1, 0, false); + await testPrefetch(6, 5, false); }); it("Avoids prefetch for next page if it doesn't exist", async () => { - await testPrefetch(2, 3, false); + await testPrefetch(3, 4, false); }); - it("Reuses the same queryKey and queryFn methods for the current page and all prefetching", async () => { - expect.hasAssertions(); + it.skip("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { + const startPage = 2; + await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${startPage}`, + ); + + const currentMatcher = expect.objectContaining({ pageNumber: startPage }); + expect(mockQueryKey).toBeCalledWith(currentMatcher); + expect(mockQueryFn).toBeCalledWith(currentMatcher); + + const prevPageMatcher = expect.objectContaining({ + pageNumber: startPage - 1, + }); + const nextPageMatcher = expect.objectContaining({ + pageNumber: startPage + 1, + }); + + await waitFor(() => expect(mockQueryKey).toBeCalledWith(prevPageMatcher)); + await waitFor(() => expect(mockQueryFn).toBeCalledWith(prevPageMatcher)); + await waitFor(() => expect(mockQueryKey).toBeCalledWith(nextPageMatcher)); + await waitFor(() => expect(mockQueryFn).toBeCalledWith(nextPageMatcher)); }); }); describe("Invalid page safety nets/redirects", () => { - it("Auto-redirects user to page 1 if params are corrupt/invalid", async () => { - expect.hasAssertions(); + const mockQueryKey = jest.fn(() => ["mock"]); + const mockQueryFn = jest.fn(({ pageNumber, limit }) => + Promise.resolve({ + data: new Array(limit).fill(pageNumber), + count: 100, + }), + ); + + it("Immediately/synchronously defaults to page 1 if params are corrupt/invalid", async () => { + const { result } = await render( + { + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }, + "/?page=Cat", + ); + + expect(result.current.currentPage).toBe(1); }); - it("Auto-redirects user to closest page if requested page overshoots", async () => { - expect.hasAssertions(); + it("Auto-redirects user to last page if requested page overshoots total pages", async () => { + const { result } = await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + "/?page=35", + ); + + await waitFor(() => expect(result.current.currentPage).toBe(4)); }); it("Auto-redirects user to first page if requested page goes below 1", async () => { - expect.hasAssertions(); + const { result } = await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + "/?page=-9999", + ); + + await waitFor(() => expect(result.current.currentPage).toBe(1)); }); - it("Calls the custom onInvalidPageChange callback if provided", async () => { + it.skip("Calls the custom onInvalidPageChange callback if provided", async () => { expect.hasAssertions(); }); }); describe("Passing outside value for URLSearchParams", () => { - it("Reads from searchParams property if provided", async () => { + it.skip("Reads from searchParams property if provided", async () => { expect.hasAssertions(); }); - it("Flushes state changes via provided searchParams property", async () => { + it.skip("Flushes state changes via provided searchParams property", async () => { expect.hasAssertions(); }); }); @@ -196,33 +243,33 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { describe(`${usePaginatedQuery.name} - Returned properties`, () => { describe("Conditional render output", () => { - it("Always has select properties be defined regardless of fetch status", async () => { + it.skip("Always has select properties be defined regardless of fetch status", async () => { expect.hasAssertions(); }); - it("Flips other properties to be defined after on-mount fetch succeeds", async () => { + it.skip("Flips other properties to be defined after on-mount fetch succeeds", async () => { expect.hasAssertions(); }); }); describe("Page change methods", () => { - test("goToFirstPage always succeeds regardless of fetch status", async () => { + test.skip("goToFirstPage always succeeds regardless of fetch status", async () => { expect.hasAssertions(); }); - test("goToNextPage works only if hasNextPage is true", async () => { + test.skip("goToNextPage works only if hasNextPage is true", async () => { expect.hasAssertions(); }); - test("goToPreviousPage works only if hasPreviousPage is true", async () => { + test.skip("goToPreviousPage works only if hasPreviousPage is true", async () => { expect.hasAssertions(); }); - test("onPageChange cleans 'corrupt' numeric values before navigating", async () => { + test.skip("onPageChange cleans 'corrupt' numeric values before navigating", async () => { expect.hasAssertions(); }); - test("onPageChange rejects impossible numeric values and does nothing", async () => { + test.skip("onPageChange rejects impossible numeric values and does nothing", async () => { expect.hasAssertions(); }); }); From 24361d14ebfe3ec6b0a2c37fb58bb8acc25f86aa Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 00:14:38 +0000 Subject: [PATCH 53/63] wip: more test progress --- site/src/hooks/usePaginatedQuery.test.ts | 35 +++++++++++++++++++----- 1 file changed, 28 insertions(+), 7 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 5555d3e9a8598..ba85a3e666e3d 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -110,7 +110,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }); }); - describe("Prefetching", () => { + describe.skip("Prefetching", () => { const mockQueryKey = jest.fn(({ pageNumber }) => ["query", pageNumber]); const mockQueryFn = jest.fn(({ pageNumber, limit }) => { return Promise.resolve({ @@ -148,11 +148,11 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await testPrefetch(2, 1, true); }); - it.skip("Prefetches the next page if it exists", async () => { + it("Prefetches the next page if it exists", async () => { await testPrefetch(2, 3, true); }); - it.skip("Avoids prefetch for previous page if it doesn't exist", async () => { + it("Avoids prefetch for previous page if it doesn't exist", async () => { await testPrefetch(1, 0, false); await testPrefetch(6, 5, false); }); @@ -161,7 +161,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await testPrefetch(3, 4, false); }); - it.skip("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { + it("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { const startPage = 2; await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, @@ -186,7 +186,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }); }); - describe("Invalid page safety nets/redirects", () => { + describe("Safety nets/redirects for invalid pages", () => { const mockQueryKey = jest.fn(() => ["mock"]); const mockQueryFn = jest.fn(({ pageNumber, limit }) => Promise.resolve({ @@ -225,8 +225,29 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await waitFor(() => expect(result.current.currentPage).toBe(1)); }); - it.skip("Calls the custom onInvalidPageChange callback if provided", async () => { - expect.hasAssertions(); + it("Calls the custom onInvalidPageChange callback if provided", async () => { + const onInvalidPageChange = jest.fn(); + await render( + { + onInvalidPageChange, + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }, + "/?page=900", + ); + + await waitFor(() => { + expect(onInvalidPageChange).toBeCalledWith( + expect.objectContaining({ + pageNumber: expect.any(Number), + limit: expect.any(Number), + offset: expect.any(Number), + totalPages: expect.any(Number), + searchParams: expect.any(URLSearchParams), + setSearchParams: expect.any(Function), + }), + ); + }); }); }); From 755f8c0c7295b81840d355c8383358c69cad4e30 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 00:26:58 +0000 Subject: [PATCH 54/63] wip: commit more test progress --- site/src/hooks/usePaginatedQuery.test.ts | 59 ++++++++++++++++++------ 1 file changed, 46 insertions(+), 13 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index ba85a3e666e3d..b8f51183549e4 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -225,16 +225,18 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await waitFor(() => expect(result.current.currentPage).toBe(1)); }); - it("Calls the custom onInvalidPageChange callback if provided", async () => { + it("Calls the custom onInvalidPageChange callback if provided (and does not update search params automatically)", async () => { + const testControl = new URLSearchParams({ + page: "1000", + }); + const onInvalidPageChange = jest.fn(); - await render( - { - onInvalidPageChange, - queryKey: mockQueryKey, - queryFn: mockQueryFn, - }, - "/?page=900", - ); + await render({ + onInvalidPageChange, + queryKey: mockQueryKey, + queryFn: mockQueryFn, + searchParams: testControl, + }); await waitFor(() => { expect(onInvalidPageChange).toBeCalledWith( @@ -248,16 +250,47 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }), ); }); + + expect(testControl.get("page")).toBe("1000"); }); }); describe("Passing outside value for URLSearchParams", () => { - it.skip("Reads from searchParams property if provided", async () => { - expect.hasAssertions(); + const mockQueryKey = jest.fn(() => ["mock"]); + const mockQueryFn = jest.fn(({ pageNumber, limit }) => + Promise.resolve({ + data: new Array(limit).fill(pageNumber), + count: 100, + }), + ); + + it("Reads from searchParams property if provided", async () => { + const searchParams = new URLSearchParams({ + page: "2", + }); + + const { result } = await render({ + searchParams, + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + expect(result.current.currentPage).toBe(2); }); - it.skip("Flushes state changes via provided searchParams property", async () => { - expect.hasAssertions(); + it("Flushes state changes via provided searchParams property", async () => { + const searchParams = new URLSearchParams({ + page: "2", + }); + + const { result } = await render({ + searchParams, + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + result.current.goToFirstPage(); + expect(searchParams.get("page")).toBe("1"); }); }); }); From 8bff0d38c177fea34546d1484e53133c6dd53eba Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 00:32:40 +0000 Subject: [PATCH 55/63] wip: AHHHHHHHH --- site/src/hooks/usePaginatedQuery.test.ts | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index b8f51183549e4..ac98363b8ba7a 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -195,7 +195,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }), ); - it("Immediately/synchronously defaults to page 1 if params are corrupt/invalid", async () => { + it("Synchronously defaults to page 1 if params are corrupt/invalid (no custom callback)", async () => { const { result } = await render( { queryKey: mockQueryKey, @@ -207,7 +207,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { expect(result.current.currentPage).toBe(1); }); - it("Auto-redirects user to last page if requested page overshoots total pages", async () => { + it("Auto-redirects user to last page if requested page overshoots total pages (no custom callback)", async () => { const { result } = await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, "/?page=35", @@ -216,7 +216,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await waitFor(() => expect(result.current.currentPage).toBe(4)); }); - it("Auto-redirects user to first page if requested page goes below 1", async () => { + it("Auto-redirects user to first page if requested page goes below 1 (no custom callback)", async () => { const { result } = await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, "/?page=-9999", @@ -296,16 +296,6 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }); describe(`${usePaginatedQuery.name} - Returned properties`, () => { - describe("Conditional render output", () => { - it.skip("Always has select properties be defined regardless of fetch status", async () => { - expect.hasAssertions(); - }); - - it.skip("Flips other properties to be defined after on-mount fetch succeeds", async () => { - expect.hasAssertions(); - }); - }); - describe("Page change methods", () => { test.skip("goToFirstPage always succeeds regardless of fetch status", async () => { expect.hasAssertions(); From 1fae773e1725028f86576a25b41e10bf7e5b1a61 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 14:52:33 +0000 Subject: [PATCH 56/63] chore: finish two more test cases --- site/src/hooks/usePaginatedQuery.test.ts | 75 +++++++++++++++++------- 1 file changed, 54 insertions(+), 21 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index ac98363b8ba7a..9ac22d6839463 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -20,26 +20,19 @@ function render< TQueryFnData extends PaginatedData = PaginatedData, TQueryPayload = never, >( - queryOptions: UsePaginatedQueryOptions, + options: UsePaginatedQueryOptions, route?: `/?page=${string}`, ) { - type Props = { options: typeof queryOptions }; - - return renderHookWithAuth( - ({ options }: Props) => usePaginatedQuery(options), - { - route, - path: "/", - initialProps: { - options: queryOptions, - }, - }, - ); + return renderHookWithAuth(({ options }) => usePaginatedQuery(options), { + route, + path: "/", + initialProps: { options }, + }); } /** * There are a lot of test cases in this file. Scoping mocking to inner describe - * function calls to limit cognitive load of maintaining this file. + * function calls to limit the cognitive load of maintaining all this stuff */ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { describe("queryPayload method", () => { @@ -225,7 +218,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await waitFor(() => expect(result.current.currentPage).toBe(1)); }); - it("Calls the custom onInvalidPageChange callback if provided (and does not update search params automatically)", async () => { + it("Calls the custom onInvalidPageChange callback if provided (instead of updating search params automatically)", async () => { const testControl = new URLSearchParams({ page: "1000", }); @@ -255,7 +248,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }); }); - describe("Passing outside value for URLSearchParams", () => { + describe("Passing in searchParams property", () => { const mockQueryKey = jest.fn(() => ["mock"]); const mockQueryFn = jest.fn(({ pageNumber, limit }) => Promise.resolve({ @@ -278,7 +271,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { expect(result.current.currentPage).toBe(2); }); - it("Flushes state changes via provided searchParams property", async () => { + it("Flushes state changes via provided searchParams property instead of internal searchParams", async () => { const searchParams = new URLSearchParams({ page: "2", }); @@ -297,12 +290,52 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { describe(`${usePaginatedQuery.name} - Returned properties`, () => { describe("Page change methods", () => { - test.skip("goToFirstPage always succeeds regardless of fetch status", async () => { - expect.hasAssertions(); + type Data = PaginatedData & { + data: readonly number[]; + }; + + const mockQueryKey = jest.fn(() => ["mock"]); + const mockQueryFn = jest.fn(({ pageNumber, limit }) => { + return new Promise((resolve) => { + setTimeout(() => { + resolve({ + data: new Array(limit).fill(pageNumber), + count: 100, + }); + }, 10_000); + }); }); - test.skip("goToNextPage works only if hasNextPage is true", async () => { - expect.hasAssertions(); + test("goToFirstPage always succeeds regardless of fetch status", async () => { + const queryFns = [mockQueryFn, jest.fn(() => Promise.reject("Too bad"))]; + + for (const queryFn of queryFns) { + const { result, unmount } = await render( + { queryFn, queryKey: mockQueryKey }, + "/?page=5", + ); + + expect(result.current.currentPage).toBe(5); + result.current.goToFirstPage(); + await waitFor(() => expect(result.current.currentPage).toBe(1)); + unmount(); + } + }); + + test("goToNextPage works only if hasNextPage is true", async () => { + const { result } = await render({ + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + expect(result.current.hasNextPage).toBe(false); + result.current.goToNextPage(); + expect(result.current.currentPage).toBe(1); + + await jest.runAllTimersAsync(); + await waitFor(() => expect(result.current.hasNextPage).toBe(true)); + result.current.goToNextPage(); + await waitFor(() => expect(result.current.currentPage).toBe(2)); }); test.skip("goToPreviousPage works only if hasPreviousPage is true", async () => { From be8272894cd99f5cabfcaed78c53e2751f1d5f4f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 15:08:20 +0000 Subject: [PATCH 57/63] wip: add in all tests (still need to investigate prefetching --- site/src/hooks/usePaginatedQuery.test.ts | 70 +++++++++++++++++++----- 1 file changed, 56 insertions(+), 14 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 9ac22d6839463..37655c90edd56 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -290,12 +290,11 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { describe(`${usePaginatedQuery.name} - Returned properties`, () => { describe("Page change methods", () => { - type Data = PaginatedData & { - data: readonly number[]; - }; - const mockQueryKey = jest.fn(() => ["mock"]); + const mockQueryFn = jest.fn(({ pageNumber, limit }) => { + type Data = PaginatedData & { data: readonly number[] }; + return new Promise((resolve) => { setTimeout(() => { resolve({ @@ -323,10 +322,13 @@ describe(`${usePaginatedQuery.name} - Returned properties`, () => { }); test("goToNextPage works only if hasNextPage is true", async () => { - const { result } = await render({ - queryKey: mockQueryKey, - queryFn: mockQueryFn, - }); + const { result } = await render( + { + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }, + "/?page=1", + ); expect(result.current.hasNextPage).toBe(false); result.current.goToNextPage(); @@ -338,16 +340,56 @@ describe(`${usePaginatedQuery.name} - Returned properties`, () => { await waitFor(() => expect(result.current.currentPage).toBe(2)); }); - test.skip("goToPreviousPage works only if hasPreviousPage is true", async () => { - expect.hasAssertions(); + test("goToPreviousPage works only if hasPreviousPage is true", async () => { + const { result } = await render( + { + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }, + "/?page=3", + ); + + expect(result.current.hasPreviousPage).toBe(false); + result.current.goToPreviousPage(); + expect(result.current.currentPage).toBe(3); + + await jest.runAllTimersAsync(); + await waitFor(() => expect(result.current.hasPreviousPage).toBe(true)); + result.current.goToPreviousPage(); + await waitFor(() => expect(result.current.currentPage).toBe(2)); }); - test.skip("onPageChange cleans 'corrupt' numeric values before navigating", async () => { - expect.hasAssertions(); + test("onPageChange cleans 'corrupt' numeric values before navigating", async () => { + const { result } = await render({ + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + await jest.runAllTimersAsync(); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + result.current.onPageChange(2.5); + + await waitFor(() => expect(result.current.currentPage).toBe(2)); }); - test.skip("onPageChange rejects impossible numeric values and does nothing", async () => { - expect.hasAssertions(); + test("onPageChange rejects impossible numeric values and does nothing", async () => { + const { result } = await render({ + queryKey: mockQueryKey, + queryFn: mockQueryFn, + }); + + await jest.runAllTimersAsync(); + await waitFor(() => expect(result.current.isSuccess).toBe(true)); + + result.current.onPageChange(NaN); + result.current.onPageChange(Infinity); + result.current.onPageChange(-Infinity); + + setTimeout(() => { + expect(result.current.currentPage).toBe(1); + }, 1000); + + jest.runAllTimers(); }); }); }); From 848fa0fc9f1715bd83580272c40b04c50cfbeba9 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 16:29:26 +0000 Subject: [PATCH 58/63] refactor: clean up code slightly --- site/src/hooks/usePaginatedQuery.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 317c885ff2be1..43c92952cb1f6 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -151,13 +151,14 @@ export function usePaginatedQuery< const queryClient = useQueryClient(); const prefetchPage = useEffectEvent((newPage: number) => { - return queryClient.prefetchQuery(getQueryOptionsFromPage(newPage)); + const options = getQueryOptionsFromPage(newPage); + return queryClient.prefetchQuery(options); }); // Have to split hairs and sync on both the current page and the hasXPage // variables, because the page can change immediately client-side, but the - // hasXPage values are derived from the server and won't be immediately ready - // on the initial render + // hasXPage values are derived from the server and won't always be immediately + // ready on the initial render useEffect(() => { if (hasNextPage) { void prefetchPage(currentPage + 1); @@ -181,7 +182,7 @@ export function usePaginatedQuery< } else { const firstPageOptions = getQueryOptionsFromPage(1); const firstPageResult = await queryClient.fetchQuery(firstPageOptions); - fixedTotalPages = Math.ceil(firstPageResult.count / limit); + fixedTotalPages = Math.ceil(firstPageResult.count / limit) || 1; } const clamped = clamp(currentPage, 1, fixedTotalPages); From c89e8e33f402ca4d5c5cdb154e0dbb9d7af8612d Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 17:25:12 +0000 Subject: [PATCH 59/63] fix: remove math bugs when calculating pages --- site/src/hooks/usePaginatedQuery.test.ts | 67 +++++++++++++----------- site/src/hooks/usePaginatedQuery.ts | 13 +++-- 2 files changed, 45 insertions(+), 35 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 37655c90edd56..9f7a2e3a32aed 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -103,27 +103,37 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }); }); - describe.skip("Prefetching", () => { + describe("Prefetching", () => { const mockQueryKey = jest.fn(({ pageNumber }) => ["query", pageNumber]); - const mockQueryFn = jest.fn(({ pageNumber, limit }) => { - return Promise.resolve({ - data: new Array(limit).fill(pageNumber), - count: 75, - }); - }); + + type Context = { pageNumber: number; limit: number }; + const mockQueryFnImplementation = ({ pageNumber, limit }: Context) => { + const data: { value: number }[] = []; + if (pageNumber * limit < 75) { + for (let i = 0; i < limit; i++) { + data.push({ value: i }); + } + } + + return Promise.resolve({ data, count: 75 }); + }; const testPrefetch = async ( startingPage: number, targetPage: number, shouldMatch: boolean, ) => { - await render( + // Have to reinitialize mock function every call to avoid false positives + // from shared mutable tracking state + const mockQueryFn = jest.fn(mockQueryFnImplementation); + const { result } = await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, `/?page=${startingPage}`, ); const pageMatcher = expect.objectContaining({ pageNumber: targetPage }); if (shouldMatch) { + await waitFor(() => expect(result.current.totalRecords).toBeDefined()); await waitFor(() => expect(mockQueryFn).toBeCalledWith(pageMatcher)); } else { // Can't use waitFor to test this, because the expect call will @@ -154,28 +164,25 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await testPrefetch(3, 4, false); }); - it("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { - const startPage = 2; - await render( - { queryKey: mockQueryKey, queryFn: mockQueryFn }, - `/?page=${startPage}`, - ); - - const currentMatcher = expect.objectContaining({ pageNumber: startPage }); - expect(mockQueryKey).toBeCalledWith(currentMatcher); - expect(mockQueryFn).toBeCalledWith(currentMatcher); - - const prevPageMatcher = expect.objectContaining({ - pageNumber: startPage - 1, - }); - const nextPageMatcher = expect.objectContaining({ - pageNumber: startPage + 1, - }); - - await waitFor(() => expect(mockQueryKey).toBeCalledWith(prevPageMatcher)); - await waitFor(() => expect(mockQueryFn).toBeCalledWith(prevPageMatcher)); - await waitFor(() => expect(mockQueryKey).toBeCalledWith(nextPageMatcher)); - await waitFor(() => expect(mockQueryFn).toBeCalledWith(nextPageMatcher)); + it.skip("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { + // const startPage = 2; + // await render( + // { queryKey: mockQueryKey, queryFn: mockQueryFn }, + // `/?page=${startPage}`, + // ); + // const currentMatcher = expect.objectContaining({ pageNumber: startPage }); + // expect(mockQueryKey).toBeCalledWith(currentMatcher); + // expect(mockQueryFn).toBeCalledWith(currentMatcher); + // const prevPageMatcher = expect.objectContaining({ + // pageNumber: startPage - 1, + // }); + // const nextPageMatcher = expect.objectContaining({ + // pageNumber: startPage + 1, + // }); + // await waitFor(() => expect(mockQueryKey).toBeCalledWith(prevPageMatcher)); + // await waitFor(() => expect(mockQueryFn).toBeCalledWith(prevPageMatcher)); + // await waitFor(() => expect(mockQueryKey).toBeCalledWith(nextPageMatcher)); + // await waitFor(() => expect(mockQueryFn).toBeCalledWith(nextPageMatcher)); }); }); diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 43c92952cb1f6..8045c4cb62077 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -110,9 +110,9 @@ export function usePaginatedQuery< const [innerSearchParams, setSearchParams] = useSearchParams(); const searchParams = outerSearchParams ?? innerSearchParams; - const currentPage = parsePage(searchParams); const limit = DEFAULT_RECORDS_PER_PAGE; - const offset = (currentPage - 1) * limit; + const currentPage = parsePage(searchParams); + const currentPageOffset = (currentPage - 1) * limit; const getQueryOptionsFromPage = (pageNumber: number) => { const pageParams: QueryPageParams = { @@ -145,9 +145,12 @@ export function usePaginatedQuery< const totalPages = totalRecords !== undefined ? Math.ceil(totalRecords / limit) : undefined; - const hasPreviousPage = totalPages !== undefined && currentPage > 1; const hasNextPage = - totalRecords !== undefined && limit * offset < totalRecords; + totalRecords !== undefined && limit + currentPageOffset < totalRecords; + const hasPreviousPage = + totalRecords !== undefined && + currentPage > 1 && + currentPageOffset - limit < totalRecords; const queryClient = useQueryClient(); const prefetchPage = useEffectEvent((newPage: number) => { @@ -196,7 +199,7 @@ export function usePaginatedQuery< setSearchParams(withoutPage); } else { const params: InvalidPageParams = { - offset, + offset: currentPageOffset, limit, setSearchParams, searchParams: withoutPage, From 3624a6d620dca8dc8fc539f81681203230196463 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 17:34:35 +0000 Subject: [PATCH 60/63] fix: wrap up all testing and clean up cases --- site/src/hooks/usePaginatedQuery.test.ts | 53 +++++++++++++----------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 9f7a2e3a32aed..0ccf5574a13b4 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -164,25 +164,30 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await testPrefetch(3, 4, false); }); - it.skip("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { - // const startPage = 2; - // await render( - // { queryKey: mockQueryKey, queryFn: mockQueryFn }, - // `/?page=${startPage}`, - // ); - // const currentMatcher = expect.objectContaining({ pageNumber: startPage }); - // expect(mockQueryKey).toBeCalledWith(currentMatcher); - // expect(mockQueryFn).toBeCalledWith(currentMatcher); - // const prevPageMatcher = expect.objectContaining({ - // pageNumber: startPage - 1, - // }); - // const nextPageMatcher = expect.objectContaining({ - // pageNumber: startPage + 1, - // }); - // await waitFor(() => expect(mockQueryKey).toBeCalledWith(prevPageMatcher)); - // await waitFor(() => expect(mockQueryFn).toBeCalledWith(prevPageMatcher)); - // await waitFor(() => expect(mockQueryKey).toBeCalledWith(nextPageMatcher)); - // await waitFor(() => expect(mockQueryFn).toBeCalledWith(nextPageMatcher)); + it("Reuses the same queryKey and queryFn methods for the current page and all prefetching (on a given render)", async () => { + const startPage = 2; + const mockQueryFn = jest.fn(mockQueryFnImplementation); + + await render( + { queryKey: mockQueryKey, queryFn: mockQueryFn }, + `/?page=${startPage}`, + ); + + const currentMatcher = expect.objectContaining({ pageNumber: startPage }); + expect(mockQueryKey).toBeCalledWith(currentMatcher); + expect(mockQueryFn).toBeCalledWith(currentMatcher); + + const prevPageMatcher = expect.objectContaining({ + pageNumber: startPage - 1, + }); + await waitFor(() => expect(mockQueryKey).toBeCalledWith(prevPageMatcher)); + await waitFor(() => expect(mockQueryFn).toBeCalledWith(prevPageMatcher)); + + const nextPageMatcher = expect.objectContaining({ + pageNumber: startPage + 1, + }); + await waitFor(() => expect(mockQueryKey).toBeCalledWith(nextPageMatcher)); + await waitFor(() => expect(mockQueryFn).toBeCalledWith(nextPageMatcher)); }); }); @@ -195,7 +200,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { }), ); - it("Synchronously defaults to page 1 if params are corrupt/invalid (no custom callback)", async () => { + it("No custom callback: synchronously defaults to page 1 if params are corrupt/invalid", async () => { const { result } = await render( { queryKey: mockQueryKey, @@ -207,7 +212,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { expect(result.current.currentPage).toBe(1); }); - it("Auto-redirects user to last page if requested page overshoots total pages (no custom callback)", async () => { + it("No custom callback: auto-redirects user to last page if requested page overshoots total pages", async () => { const { result } = await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, "/?page=35", @@ -216,7 +221,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await waitFor(() => expect(result.current.currentPage).toBe(4)); }); - it("Auto-redirects user to first page if requested page goes below 1 (no custom callback)", async () => { + it("No custom callback: auto-redirects user to first page if requested page goes below 1", async () => { const { result } = await render( { queryKey: mockQueryKey, queryFn: mockQueryFn }, "/?page=-9999", @@ -225,7 +230,7 @@ describe(`${usePaginatedQuery.name} - Overall functionality`, () => { await waitFor(() => expect(result.current.currentPage).toBe(1)); }); - it("Calls the custom onInvalidPageChange callback if provided (instead of updating search params automatically)", async () => { + it("With custom callback: Calls callback and does not update search params automatically", async () => { const testControl = new URLSearchParams({ page: "1000", }); @@ -366,7 +371,7 @@ describe(`${usePaginatedQuery.name} - Returned properties`, () => { await waitFor(() => expect(result.current.currentPage).toBe(2)); }); - test("onPageChange cleans 'corrupt' numeric values before navigating", async () => { + test("onPageChange accounts for floats and truncates numeric values before navigating", async () => { const { result } = await render({ queryKey: mockQueryKey, queryFn: mockQueryFn, From 13e2f30d8ff1c32c7fa720b68e7d2df832435f7f Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Wed, 22 Nov 2023 17:53:17 +0000 Subject: [PATCH 61/63] docs: update comments for clarity --- site/src/hooks/usePaginatedQuery.ts | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 8045c4cb62077..73e4562aab66e 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -34,15 +34,9 @@ export type UsePaginatedQueryOptions< > = BasePaginationOptions & QueryPayloadExtender & { /** - * An optional dependency for React Router's URLSearchParams. - * - * It's annoying that this is necessary, but it helps avoid URL de-syncs if - * useSearchParams is called multiple times in the same component (likely in - * multiple custom hooks) - * - * @todo Wrangle React Router's useSearchParams so that URL state can be - * shared between multiple components/hooks more directly without making you - * jump through so many hoops (it's affecting our filter logic, too) + * An optional dependency for React Router's URLSearchParams. If this is + * provided, all URL state changes will go through this object instead of + * an internal value. */ searchParams?: URLSearchParams; @@ -199,9 +193,9 @@ export function usePaginatedQuery< setSearchParams(withoutPage); } else { const params: InvalidPageParams = { - offset: currentPageOffset, limit, setSearchParams, + offset: currentPageOffset, searchParams: withoutPage, totalPages: fixedTotalPages, pageNumber: currentPage, From 8f83673faef1773ca39065873433c301ce6c1d12 Mon Sep 17 00:00:00 2001 From: Parkreiner Date: Fri, 24 Nov 2023 16:08:07 +0000 Subject: [PATCH 62/63] fix: update error-handling for invalid page handling --- site/src/hooks/usePaginatedQuery.ts | 8 ++++++-- site/src/pages/AuditPage/AuditPage.tsx | 2 +- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index 73e4562aab66e..aa16e1c92c448 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -178,8 +178,12 @@ export function usePaginatedQuery< fixedTotalPages = totalPages; } else { const firstPageOptions = getQueryOptionsFromPage(1); - const firstPageResult = await queryClient.fetchQuery(firstPageOptions); - fixedTotalPages = Math.ceil(firstPageResult.count / limit) || 1; + try { + const firstPageResult = await queryClient.fetchQuery(firstPageOptions); + fixedTotalPages = Math.ceil(firstPageResult?.count ?? 0 / limit) || 1; + } catch (err) { + fixedTotalPages = 1; + } } const clamped = clamp(currentPage, 1, fixedTotalPages); diff --git a/site/src/pages/AuditPage/AuditPage.tsx b/site/src/pages/AuditPage/AuditPage.tsx index 0be62a7e69f2b..63674b071629c 100644 --- a/site/src/pages/AuditPage/AuditPage.tsx +++ b/site/src/pages/AuditPage/AuditPage.tsx @@ -63,7 +63,7 @@ const AuditPage: FC = () => { Date: Thu, 30 Nov 2023 20:29:57 +0000 Subject: [PATCH 63/63] fix: apply suggestions --- site/src/api/queries/audits.ts | 8 ++++---- site/src/api/queries/users.ts | 8 +++++--- site/src/hooks/hookPolyfills.ts | 6 ++++-- site/src/hooks/usePaginatedQuery.test.ts | 2 +- site/src/hooks/usePaginatedQuery.ts | 7 ++++--- site/src/utils/filters.ts | 3 ++- 6 files changed, 20 insertions(+), 14 deletions(-) diff --git a/site/src/api/queries/audits.ts b/site/src/api/queries/audits.ts index c0a758619f32b..3479c183b1b5c 100644 --- a/site/src/api/queries/audits.ts +++ b/site/src/api/queries/audits.ts @@ -3,7 +3,9 @@ import { type AuditLogResponse } from "api/typesGenerated"; import { useFilterParamsKey } from "components/Filter/filter"; import { type UsePaginatedQueryOptions } from "hooks/usePaginatedQuery"; -export function paginatedAudits(searchParams: URLSearchParams) { +export function paginatedAudits( + searchParams: URLSearchParams, +): UsePaginatedQueryOptions { return { searchParams, queryPayload: () => searchParams.get(useFilterParamsKey) ?? "", @@ -17,7 +19,5 @@ export function paginatedAudits(searchParams: URLSearchParams) { q: payload, }); }, - - cacheTime: 5 * 1000 * 60, - } as const satisfies UsePaginatedQueryOptions; + }; } diff --git a/site/src/api/queries/users.ts b/site/src/api/queries/users.ts index 2d490df2de38e..6829cd1dd5fba 100644 --- a/site/src/api/queries/users.ts +++ b/site/src/api/queries/users.ts @@ -17,7 +17,10 @@ export function usersKey(req: UsersRequest) { return ["users", req] as const; } -export function paginatedUsers() { +export function paginatedUsers(): UsePaginatedQueryOptions< + GetUsersResponse, + UsersRequest +> { return { queryPayload: ({ limit, offset, searchParams }) => { return { @@ -29,8 +32,7 @@ export function paginatedUsers() { queryKey: ({ payload }) => usersKey(payload), queryFn: ({ payload, signal }) => API.getUsers(payload, signal), - cacheTime: 5 * 1000 * 60, - } as const satisfies UsePaginatedQueryOptions; + }; } export const users = (req: UsersRequest): UseQueryOptions => { diff --git a/site/src/hooks/hookPolyfills.ts b/site/src/hooks/hookPolyfills.ts index 40ca8629c9d27..94d8052b349b5 100644 --- a/site/src/hooks/hookPolyfills.ts +++ b/site/src/hooks/hookPolyfills.ts @@ -36,8 +36,10 @@ export function useEffectEvent( ) { const callbackRef = useRef(callback); - // useLayoutEffect should be overkill here 99% of the time, but it ensures it - // will run before any other layout effects that need this custom hook + // useLayoutEffect should be overkill here 99% of the time, but if this were + // defined as a regular effect, useEffectEvent would not be able to work with + // any layout effects at all; the callback sync here would fire *after* the + // layout effect that needs the useEffectEvent function useLayoutEffect(() => { callbackRef.current = callback; }, [callback]); diff --git a/site/src/hooks/usePaginatedQuery.test.ts b/site/src/hooks/usePaginatedQuery.test.ts index 0ccf5574a13b4..2ea3ea57a874f 100644 --- a/site/src/hooks/usePaginatedQuery.test.ts +++ b/site/src/hooks/usePaginatedQuery.test.ts @@ -34,7 +34,7 @@ function render< * There are a lot of test cases in this file. Scoping mocking to inner describe * function calls to limit the cognitive load of maintaining all this stuff */ -describe(`${usePaginatedQuery.name} - Overall functionality`, () => { +describe(`${usePaginatedQuery.name}`, () => { describe("queryPayload method", () => { const mockQueryFn = jest.fn(() => Promise.resolve({ count: 0 })); diff --git a/site/src/hooks/usePaginatedQuery.ts b/site/src/hooks/usePaginatedQuery.ts index aa16e1c92c448..46a4b9b487f43 100644 --- a/site/src/hooks/usePaginatedQuery.ts +++ b/site/src/hooks/usePaginatedQuery.ts @@ -180,8 +180,9 @@ export function usePaginatedQuery< const firstPageOptions = getQueryOptionsFromPage(1); try { const firstPageResult = await queryClient.fetchQuery(firstPageOptions); - fixedTotalPages = Math.ceil(firstPageResult?.count ?? 0 / limit) || 1; - } catch (err) { + const rounded = Math.ceil(firstPageResult?.count ?? 0 / limit); + fixedTotalPages = Math.max(rounded, 1); + } catch { fixedTotalPages = 1; } } @@ -223,7 +224,7 @@ export function usePaginatedQuery< } const cleanedInput = clamp(Math.trunc(newPage), 1, totalPages ?? 1); - if (!Number.isInteger(cleanedInput) || cleanedInput <= 0) { + if (Number.isNaN(cleanedInput)) { return; } diff --git a/site/src/utils/filters.ts b/site/src/utils/filters.ts index 389b866d0e111..164ef633b5244 100644 --- a/site/src/utils/filters.ts +++ b/site/src/utils/filters.ts @@ -1,5 +1,6 @@ -export function prepareQuery(query: undefined): undefined; export function prepareQuery(query: string): string; +export function prepareQuery(query: undefined): undefined; +export function prepareQuery(query: string | undefined): string | undefined; export function prepareQuery(query?: string): string | undefined { return query?.trim().replace(/ +/g, " "); }