From 83f6c8d2e49d905bb4bb723b73ddd80ad0656ba1 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 20 Nov 2024 12:44:55 -0600 Subject: [PATCH 01/10] redo useQueryTable so that the types are legit and it returns the data --- app/api/client.ts | 4 + app/pages/project/snapshots/SnapshotsPage.tsx | 19 +++-- app/table/QueryTable2.tsx | 84 +++++++++++++++++++ 3 files changed, 101 insertions(+), 6 deletions(-) create mode 100644 app/table/QueryTable2.tsx diff --git a/app/api/client.ts b/app/api/client.ts index cdd3c08875..2985d800b7 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -9,6 +9,7 @@ import { QueryClient } from '@tanstack/react-query' import { Api } from './__generated__/Api' import { + getApiQueryOptions, getUseApiMutation, getUseApiQueries, getUseApiQuery, @@ -17,6 +18,8 @@ import { wrapQueryClient, } from './hooks' +export { ensure } from './hooks' + export const api = new Api({ // unit tests run in Node, whose fetch implementation requires a full URL host: process.env.NODE_ENV === 'test' ? 'http://testhost' : '', @@ -24,6 +27,7 @@ export const api = new Api({ export type ApiMethods = typeof api.methods +export const apiq = getApiQueryOptions(api.methods) export const useApiQuery = getUseApiQuery(api.methods) export const useApiQueries = getUseApiQueries(api.methods) /** diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index d2b0e28ab1..405b8e00e9 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -10,7 +10,9 @@ import { useCallback } from 'react' import { Outlet, useNavigate, type LoaderFunctionArgs } from 'react-router-dom' import { + apiq, apiQueryClient, + queryClient, useApiMutation, useApiQueryClient, useApiQueryErrorsAllowed, @@ -25,7 +27,7 @@ import { confirmDelete } from '~/stores/confirm-delete' import { SkeletonCell } from '~/table/cells/EmptyCell' import { useColsWithActions, type MenuAction } from '~/table/columns/action-col' import { Columns } from '~/table/columns/common' -import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable' +import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable2' import { Badge } from '~/ui/lib/Badge' import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' @@ -51,13 +53,14 @@ const EmptyState = () => ( buttonTo={pb.snapshotsNew(useProjectSelector())} /> ) +// clearly we need to shorten this +const snapshotListOptions = (project: string) => (limit: number, pageToken?: string) => + apiq('snapshotList', { query: { project, pageToken, limit } }) SnapshotsPage.loader = async ({ params }: LoaderFunctionArgs) => { const { project } = getProjectSelector(params) await Promise.all([ - apiQueryClient.prefetchQuery('snapshotList', { - query: { project, limit: PAGE_SIZE }, - }), + queryClient.prefetchQuery(snapshotListOptions(project)(PAGE_SIZE)), // Fetch disks and preload into RQ cache so fetches by ID in DiskNameFromId // can be mostly instant yet gracefully fall back to fetching individually @@ -100,7 +103,6 @@ const staticCols = [ export function SnapshotsPage() { const queryClient = useApiQueryClient() const { project } = useProjectSelector() - const { Table } = useQueryTable('snapshotList', { query: { project } }) const navigate = useNavigate() const { mutateAsync: deleteSnapshot } = useApiMutation('snapshotDelete', { @@ -132,6 +134,11 @@ export function SnapshotsPage() { [deleteSnapshot, navigate, project] ) const columns = useColsWithActions(staticCols, makeActions) + const { table } = useQueryTable({ + optionsFn: snapshotListOptions(project), + columns, + emptyState: , + }) return ( <> @@ -146,7 +153,7 @@ export function SnapshotsPage() { New snapshot - } /> + {table} ) diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx new file mode 100644 index 0000000000..0815f2fb4f --- /dev/null +++ b/app/table/QueryTable2.tsx @@ -0,0 +1,84 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ +import { useQuery, type QueryKey, type QueryOptions } from '@tanstack/react-query' +import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' +import React, { useCallback, useMemo } from 'react' + +import { ensure, type ApiError } from '@oxide/api' + +import { Pagination } from '~/components/Pagination' +import { usePagination } from '~/hooks/use-pagination' +import { EmptyMessage } from '~/ui/lib/EmptyMessage' +import { TableEmptyBox } from '~/ui/lib/Table' + +import { Table } from './Table' + +export const PAGE_SIZE = 25 + +type QueryTableProps = { + optionsFn: ( + limit: number, + page_token?: string + ) => QueryOptions<{ items: TItem[]; nextPage?: string }, ApiError> & { + queryKey: QueryKey + } + pageSize?: number + rowHeight?: 'small' | 'large' + emptyState: React.ReactElement + // React Table does the same in the type of `columns` on `useReactTable` + // eslint-disable-next-line @typescript-eslint/no-explicit-any + columns: ColumnDef[] +} + +export function useQueryTable({ + optionsFn, + pageSize = PAGE_SIZE, + rowHeight = 'small', + emptyState, + columns, +}: QueryTableProps) { + const { currentPage, goToNextPage, goToPrevPage, hasPrev } = usePagination() + const queryResult = useQuery(optionsFn(pageSize, currentPage)) + // only ensure prefetched if we're on the first page + if (currentPage === undefined) ensure(queryResult) + const { data, isLoading } = queryResult + const tableData = useMemo(() => data?.items || [], [data]) + + // TODO: need a better function that takes name or ID. sleds in the sleds + // table have no name, for example + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const getRowId = useCallback((row: any) => row.name, []) + + const table = useReactTable({ + columns, + data: tableData, + getRowId, + getCoreRowModel: getCoreRowModel(), + manualPagination: true, + }) + + const isEmpty = tableData.length === 0 && !hasPrev + + const tableElement = isLoading ? null : isEmpty ? ( + {emptyState || } + ) : ( + <> +
+ + + ) + + return { table: tableElement, query: queryResult } +} From 4de0bb06a73d0398987eb1965e202941382e4eb8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 20 Nov 2024 15:45:32 -0600 Subject: [PATCH 02/10] fix junky getRowId and use new QueryTable on sleds and physical disks --- app/pages/system/inventory/DisksTab.tsx | 18 +++++++++++++----- app/pages/system/inventory/SledsTab.tsx | 19 ++++++++++++------- app/table/QueryTable2.tsx | 12 ++++-------- 3 files changed, 29 insertions(+), 20 deletions(-) diff --git a/app/pages/system/inventory/DisksTab.tsx b/app/pages/system/inventory/DisksTab.tsx index e029e66f9e..961054b46e 100644 --- a/app/pages/system/inventory/DisksTab.tsx +++ b/app/pages/system/inventory/DisksTab.tsx @@ -8,14 +8,15 @@ import { createColumnHelper } from '@tanstack/react-table' import { - apiQueryClient, + apiq, + queryClient, type PhysicalDisk, type PhysicalDiskPolicy, type PhysicalDiskState, } from '@oxide/api' import { Servers24Icon } from '@oxide/design-system/icons/react' -import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable' +import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable2' import { Badge, type BadgeColor } from '~/ui/lib/Badge' import { EmptyMessage } from '~/ui/lib/EmptyMessage' @@ -37,8 +38,11 @@ const EmptyState = () => ( /> ) +const diskList = (limit: number, pageToken?: string) => + apiq('physicalDiskList', { query: { limit, pageToken } }, { placeholderData: (x) => x }) + export async function loader() { - await apiQueryClient.prefetchQuery('physicalDiskList', { query: { limit: PAGE_SIZE } }) + await queryClient.prefetchQuery(diskList(PAGE_SIZE)) return null } @@ -68,6 +72,10 @@ const staticCols = [ Component.displayName = 'DisksTab' export function Component() { - const { Table } = useQueryTable('physicalDiskList', {}) - return
} columns={staticCols} /> + const { table } = useQueryTable({ + optionsFn: diskList, + columns: staticCols, + emptyState: , + }) + return table } diff --git a/app/pages/system/inventory/SledsTab.tsx b/app/pages/system/inventory/SledsTab.tsx index 94b8da375f..ac8c9ca599 100644 --- a/app/pages/system/inventory/SledsTab.tsx +++ b/app/pages/system/inventory/SledsTab.tsx @@ -7,11 +7,11 @@ */ import { createColumnHelper } from '@tanstack/react-table' -import { apiQueryClient, type Sled, type SledPolicy, type SledState } from '@oxide/api' +import { apiq, queryClient, type Sled, type SledPolicy, type SledState } from '@oxide/api' import { Servers24Icon } from '@oxide/design-system/icons/react' import { makeLinkCell } from '~/table/cells/LinkCell' -import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable' +import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable2' import { Badge, type BadgeColor } from '~/ui/lib/Badge' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { pb } from '~/util/path-builder' @@ -36,10 +36,11 @@ const EmptyState = () => { ) } +const sledList = (limit: number, pageToken?: string) => + apiq('sledList', { query: { limit, pageToken } }, { placeholderData: (x) => x }) + export async function loader() { - await apiQueryClient.prefetchQuery('sledList', { - query: { limit: PAGE_SIZE }, - }) + await queryClient.prefetchQuery(sledList(PAGE_SIZE)) return null } @@ -69,6 +70,10 @@ const staticCols = [ Component.displayName = 'SledsTab' export function Component() { - const { Table } = useQueryTable('sledList', {}, { placeholderData: (x) => x }) - return
} columns={staticCols} /> + const { table } = useQueryTable({ + optionsFn: sledList, + columns: staticCols, + emptyState: , + }) + return table } diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index 0815f2fb4f..fb1e2ea580 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -7,7 +7,7 @@ */ import { useQuery, type QueryKey, type QueryOptions } from '@tanstack/react-query' import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' -import React, { useCallback, useMemo } from 'react' +import React, { useMemo } from 'react' import { ensure, type ApiError } from '@oxide/api' @@ -35,7 +35,8 @@ type QueryTableProps = { columns: ColumnDef[] } -export function useQueryTable({ +// require ID only so we can use it in getRowId +export function useQueryTable({ optionsFn, pageSize = PAGE_SIZE, rowHeight = 'small', @@ -49,15 +50,10 @@ export function useQueryTable({ const { data, isLoading } = queryResult const tableData = useMemo(() => data?.items || [], [data]) - // TODO: need a better function that takes name or ID. sleds in the sleds - // table have no name, for example - // eslint-disable-next-line @typescript-eslint/no-explicit-any - const getRowId = useCallback((row: any) => row.name, []) - const table = useReactTable({ columns, data: tableData, - getRowId, + getRowId: (row) => row.id, getCoreRowModel: getCoreRowModel(), manualPagination: true, }) From 94064441b6ada9c2ed1c30de387e9813ef2016d3 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 21 Nov 2024 10:22:27 -0600 Subject: [PATCH 03/10] get wild with a list-specific helper to make the call sites clean --- app/api/client.ts | 10 ++++- app/api/hooks.ts | 40 ++++++++++++++++++- app/api/util.ts | 2 + app/pages/project/snapshots/SnapshotsPage.tsx | 13 +++--- app/pages/system/inventory/DisksTab.tsx | 9 ++--- app/pages/system/inventory/SledsTab.tsx | 15 ++++--- app/table/QueryTable.tsx | 5 ++- app/table/QueryTable2.tsx | 11 +++-- 8 files changed, 77 insertions(+), 28 deletions(-) diff --git a/app/api/client.ts b/app/api/client.ts index 2985d800b7..1e9715214a 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -10,6 +10,7 @@ import { QueryClient } from '@tanstack/react-query' import { Api } from './__generated__/Api' import { getApiQueryOptions, + getListQueryOptionsFn, getUseApiMutation, getUseApiQueries, getUseApiQuery, @@ -18,7 +19,7 @@ import { wrapQueryClient, } from './hooks' -export { ensure } from './hooks' +export { ensurePrefetched, PAGE_SIZE } from './hooks' export const api = new Api({ // unit tests run in Node, whose fetch implementation requires a full URL @@ -27,7 +28,14 @@ export const api = new Api({ export type ApiMethods = typeof api.methods +/** API-specific query options helper. */ export const apiq = getApiQueryOptions(api.methods) +/** + * Query options helper that only supports list endpoints. Returns + * a function `(limit, pageToken) => QueryOptions` for use with + * `useQueryTable`. + */ +export const getListQFn = getListQueryOptionsFn(api.methods) export const useApiQuery = getUseApiQuery(api.methods) export const useApiQueries = getUseApiQueries(api.methods) /** diff --git a/app/api/hooks.ts b/app/api/hooks.ts index ebb9dcf8fc..ddac2f6efb 100644 --- a/app/api/hooks.ts +++ b/app/api/hooks.ts @@ -28,6 +28,7 @@ import { invariant } from '~/util/invariant' import type { ApiResult } from './__generated__/Api' import { processServerError, type ApiError } from './errors' import { navToLogin } from './nav-to-login' +import { type ResultsPage } from './util' /* eslint-disable @typescript-eslint/no-explicit-any */ export type Params = F extends (p: infer P) => any ? P : never @@ -123,6 +124,37 @@ export const getApiQueryOptions = ...options, }) +export const PAGE_SIZE = 25 + +/** + * This is the same as getApiQueryOptions except for two things: + * + * 1. We use a type constraint on the method key to ensure it can + * only be used with endpoints that return a `ResultsPage`. + * 2. Instead of returning the options directly, it returns a function that + * takes `limit` and `pageToken` and merges them into the query params so + * that these can be passed in by `QueryTable`. + */ +export const getListQueryOptionsFn = + (api: A) => + < + M extends string & + { + // this helper can only be used with endpoints that return ResultsPage + [K in keyof A]: Result extends ResultsPage ? K : never + }[keyof A], + >( + method: M, + params: Params, + options: UseQueryOtherOptions, ApiError> = {} + ) => + (limit: number = PAGE_SIZE, pageToken?: string) => + getApiQueryOptions(api)( + method, + { ...params, query: { ...params.query, limit, pageToken } }, + options + ) + export const getUseApiQuery = (api: A) => ( @@ -140,7 +172,7 @@ export const getUsePrefetchedApiQuery = options: UseQueryOtherOptions, ApiError> = {} ) => { const qOptions = getApiQueryOptions(api)(method, params, options) - return ensure(useQuery(qOptions), qOptions.queryKey) + return ensurePrefetched(useQuery(qOptions), qOptions.queryKey) } const prefetchError = (key?: QueryKey) => @@ -152,7 +184,11 @@ Ensure the following: • request isn't erroring-out server-side (check the Networking tab) • mock API endpoint is implemented in handlers.ts` -export function ensure( +/** + * Ensure a query result came from the cache by blowing up if `data` comes + * back undefined. + */ +export function ensurePrefetched( result: UseQueryResult, /** * Optional because if we call this manually from a component like diff --git a/app/api/util.ts b/app/api/util.ts index 954cd77b0e..d552aa4f36 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -23,6 +23,8 @@ import type { VpcFirewallRuleUpdate, } from './__generated__/Api' +export type ResultsPage = { items: TItem[]; nextPage?: string } + // API limits encoded in https://github.com/oxidecomputer/omicron/blob/main/nexus/src/app/mod.rs export const MAX_NICS_PER_INSTANCE = 8 diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index 405b8e00e9..4f33132774 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -10,8 +10,8 @@ import { useCallback } from 'react' import { Outlet, useNavigate, type LoaderFunctionArgs } from 'react-router-dom' import { - apiq, apiQueryClient, + getListQFn, queryClient, useApiMutation, useApiQueryClient, @@ -27,7 +27,7 @@ import { confirmDelete } from '~/stores/confirm-delete' import { SkeletonCell } from '~/table/cells/EmptyCell' import { useColsWithActions, type MenuAction } from '~/table/columns/action-col' import { Columns } from '~/table/columns/common' -import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable2' +import { useQueryTable } from '~/table/QueryTable2' import { Badge } from '~/ui/lib/Badge' import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' @@ -53,14 +53,13 @@ const EmptyState = () => ( buttonTo={pb.snapshotsNew(useProjectSelector())} /> ) -// clearly we need to shorten this -const snapshotListOptions = (project: string) => (limit: number, pageToken?: string) => - apiq('snapshotList', { query: { project, pageToken, limit } }) + +const snapshotList = (project: string) => getListQFn('snapshotList', { query: { project } }) SnapshotsPage.loader = async ({ params }: LoaderFunctionArgs) => { const { project } = getProjectSelector(params) await Promise.all([ - queryClient.prefetchQuery(snapshotListOptions(project)(PAGE_SIZE)), + queryClient.prefetchQuery(snapshotList(project)()), // Fetch disks and preload into RQ cache so fetches by ID in DiskNameFromId // can be mostly instant yet gracefully fall back to fetching individually @@ -135,7 +134,7 @@ export function SnapshotsPage() { ) const columns = useColsWithActions(staticCols, makeActions) const { table } = useQueryTable({ - optionsFn: snapshotListOptions(project), + optionsFn: snapshotList(project), columns, emptyState: , }) diff --git a/app/pages/system/inventory/DisksTab.tsx b/app/pages/system/inventory/DisksTab.tsx index 961054b46e..64fbe2989a 100644 --- a/app/pages/system/inventory/DisksTab.tsx +++ b/app/pages/system/inventory/DisksTab.tsx @@ -8,7 +8,7 @@ import { createColumnHelper } from '@tanstack/react-table' import { - apiq, + getListQFn, queryClient, type PhysicalDisk, type PhysicalDiskPolicy, @@ -16,7 +16,7 @@ import { } from '@oxide/api' import { Servers24Icon } from '@oxide/design-system/icons/react' -import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable2' +import { useQueryTable } from '~/table/QueryTable2' import { Badge, type BadgeColor } from '~/ui/lib/Badge' import { EmptyMessage } from '~/ui/lib/EmptyMessage' @@ -38,11 +38,10 @@ const EmptyState = () => ( /> ) -const diskList = (limit: number, pageToken?: string) => - apiq('physicalDiskList', { query: { limit, pageToken } }, { placeholderData: (x) => x }) +const diskList = getListQFn('physicalDiskList', {}, { placeholderData: (x) => x }) export async function loader() { - await queryClient.prefetchQuery(diskList(PAGE_SIZE)) + await queryClient.prefetchQuery(diskList()) return null } diff --git a/app/pages/system/inventory/SledsTab.tsx b/app/pages/system/inventory/SledsTab.tsx index ac8c9ca599..82e92ed934 100644 --- a/app/pages/system/inventory/SledsTab.tsx +++ b/app/pages/system/inventory/SledsTab.tsx @@ -7,11 +7,17 @@ */ import { createColumnHelper } from '@tanstack/react-table' -import { apiq, queryClient, type Sled, type SledPolicy, type SledState } from '@oxide/api' +import { + getListQFn, + queryClient, + type Sled, + type SledPolicy, + type SledState, +} from '@oxide/api' import { Servers24Icon } from '@oxide/design-system/icons/react' import { makeLinkCell } from '~/table/cells/LinkCell' -import { PAGE_SIZE, useQueryTable } from '~/table/QueryTable2' +import { useQueryTable } from '~/table/QueryTable2' import { Badge, type BadgeColor } from '~/ui/lib/Badge' import { EmptyMessage } from '~/ui/lib/EmptyMessage' import { pb } from '~/util/path-builder' @@ -36,11 +42,10 @@ const EmptyState = () => { ) } -const sledList = (limit: number, pageToken?: string) => - apiq('sledList', { query: { limit, pageToken } }, { placeholderData: (x) => x }) +const sledList = getListQFn('sledList', {}, { placeholderData: (x) => x }) export async function loader() { - await queryClient.prefetchQuery(sledList(PAGE_SIZE)) + await queryClient.prefetchQuery(sledList()) return null } diff --git a/app/table/QueryTable.tsx b/app/table/QueryTable.tsx index 4c78da2cef..c73f7a25cd 100644 --- a/app/table/QueryTable.tsx +++ b/app/table/QueryTable.tsx @@ -12,6 +12,7 @@ import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react- import React, { useCallback, useMemo, type ComponentType } from 'react' import { + PAGE_SIZE, useApiQuery, type ApiError, type ApiListMethods, @@ -27,6 +28,8 @@ import { TableEmptyBox } from '~/ui/lib/Table' import { Table } from './Table' +export { PAGE_SIZE } + interface UseQueryTableResult> { Table: ComponentType> } @@ -59,8 +62,6 @@ type QueryTableProps = { columns: ColumnDef[] } -export const PAGE_SIZE = 25 - // eslint-disable-next-line @typescript-eslint/no-explicit-any const makeQueryTable = >( query: any, diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index fb1e2ea580..4985bd2d7a 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -9,7 +9,7 @@ import { useQuery, type QueryKey, type QueryOptions } from '@tanstack/react-quer import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' import React, { useMemo } from 'react' -import { ensure, type ApiError } from '@oxide/api' +import { ensurePrefetched, PAGE_SIZE, type ApiError, type ResultsPage } from '@oxide/api' import { Pagination } from '~/components/Pagination' import { usePagination } from '~/hooks/use-pagination' @@ -18,13 +18,11 @@ import { TableEmptyBox } from '~/ui/lib/Table' import { Table } from './Table' -export const PAGE_SIZE = 25 - type QueryTableProps = { optionsFn: ( limit: number, page_token?: string - ) => QueryOptions<{ items: TItem[]; nextPage?: string }, ApiError> & { + ) => QueryOptions, ApiError> & { queryKey: QueryKey } pageSize?: number @@ -44,9 +42,10 @@ export function useQueryTable({ columns, }: QueryTableProps) { const { currentPage, goToNextPage, goToPrevPage, hasPrev } = usePagination() - const queryResult = useQuery(optionsFn(pageSize, currentPage)) + const queryOptions = optionsFn(pageSize, currentPage) + const queryResult = useQuery(queryOptions) // only ensure prefetched if we're on the first page - if (currentPage === undefined) ensure(queryResult) + if (currentPage === undefined) ensurePrefetched(queryResult, queryOptions.queryKey) const { data, isLoading } = queryResult const tableData = useMemo(() => data?.items || [], [data]) From 8cab460ba3fab7641edb14b4199cb3ffa6832c81 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 21 Nov 2024 15:31:31 -0600 Subject: [PATCH 04/10] encapsulate pageSize in the query config so it is defined in one place --- app/api/client.ts | 2 +- app/api/hooks.ts | 42 ++++++++++++++----- app/pages/project/snapshots/SnapshotsPage.tsx | 4 +- app/pages/system/inventory/DisksTab.tsx | 9 ++-- app/pages/system/inventory/SledsTab.tsx | 9 ++-- app/table/QueryTable2.tsx | 21 ++++------ 6 files changed, 48 insertions(+), 39 deletions(-) diff --git a/app/api/client.ts b/app/api/client.ts index 1e9715214a..2d8da89976 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -19,7 +19,7 @@ import { wrapQueryClient, } from './hooks' -export { ensurePrefetched, PAGE_SIZE } from './hooks' +export { ensurePrefetched, PAGE_SIZE, type PaginatedQuery } from './hooks' export const api = new Api({ // unit tests run in Node, whose fetch implementation requires a full URL diff --git a/app/api/hooks.ts b/app/api/hooks.ts index ddac2f6efb..8ee11ea95e 100644 --- a/app/api/hooks.ts +++ b/app/api/hooks.ts @@ -21,6 +21,7 @@ import { type UseQueryOptions, type UseQueryResult, } from '@tanstack/react-query' +import * as R from 'remeda' import { type SetNonNullable } from 'type-fest' import { invariant } from '~/util/invariant' @@ -124,16 +125,32 @@ export const getApiQueryOptions = ...options, }) +// Managed here instead of at the display layer so it can be built into the +// query options and shared between loader prefetch and QueryTable export const PAGE_SIZE = 25 +/** + * This primarily exists so we can have an object that encapsulates everything + * useQueryTable needs to know about a query. In particular, it needs the page + * size, and you can't pull that out of the query options object unless you + * stick it in `meta`, and then we don't have type safety. + */ +export type PaginatedQuery = { + optionsFn: ( + pageToken?: string + ) => UseQueryOptions & { queryKey: QueryKey } + pageSize: number +} + /** * This is the same as getApiQueryOptions except for two things: * * 1. We use a type constraint on the method key to ensure it can * only be used with endpoints that return a `ResultsPage`. - * 2. Instead of returning the options directly, it returns a function that - * takes `limit` and `pageToken` and merges them into the query params so - * that these can be passed in by `QueryTable`. + * 2. Instead of returning the options directly, it returns a paginated + * query config object containing the page size and a function that + * takes `limit` and `pageToken` and merges them into the query params + * so that these can be passed in by `QueryTable`. */ export const getListQueryOptionsFn = (api: A) => @@ -147,13 +164,18 @@ export const getListQueryOptionsFn = method: M, params: Params, options: UseQueryOtherOptions, ApiError> = {} - ) => - (limit: number = PAGE_SIZE, pageToken?: string) => - getApiQueryOptions(api)( - method, - { ...params, query: { ...params.query, limit, pageToken } }, - options - ) + ): PaginatedQuery> => { + // pathOr plays nice when the properties don't exist + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const limit = R.pathOr(params as any, ['query', 'limit'], PAGE_SIZE) + return { + optionsFn: (pageToken?: string) => { + const newParams = { ...params, query: { ...params.query, limit, pageToken } } + return getApiQueryOptions(api)(method, newParams, options) + }, + pageSize: limit, + } + } export const getUseApiQuery = (api: A) => diff --git a/app/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index 4f33132774..828f849eb0 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -59,7 +59,7 @@ const snapshotList = (project: string) => getListQFn('snapshotList', { query: { SnapshotsPage.loader = async ({ params }: LoaderFunctionArgs) => { const { project } = getProjectSelector(params) await Promise.all([ - queryClient.prefetchQuery(snapshotList(project)()), + queryClient.prefetchQuery(snapshotList(project).optionsFn()), // Fetch disks and preload into RQ cache so fetches by ID in DiskNameFromId // can be mostly instant yet gracefully fall back to fetching individually @@ -134,7 +134,7 @@ export function SnapshotsPage() { ) const columns = useColsWithActions(staticCols, makeActions) const { table } = useQueryTable({ - optionsFn: snapshotList(project), + query: snapshotList(project), columns, emptyState: , }) diff --git a/app/pages/system/inventory/DisksTab.tsx b/app/pages/system/inventory/DisksTab.tsx index 64fbe2989a..dc8f057dc0 100644 --- a/app/pages/system/inventory/DisksTab.tsx +++ b/app/pages/system/inventory/DisksTab.tsx @@ -41,7 +41,7 @@ const EmptyState = () => ( const diskList = getListQFn('physicalDiskList', {}, { placeholderData: (x) => x }) export async function loader() { - await queryClient.prefetchQuery(diskList()) + await queryClient.prefetchQuery(diskList.optionsFn()) return null } @@ -71,10 +71,7 @@ const staticCols = [ Component.displayName = 'DisksTab' export function Component() { - const { table } = useQueryTable({ - optionsFn: diskList, - columns: staticCols, - emptyState: , - }) + const emptyState = + const { table } = useQueryTable({ query: diskList, columns: staticCols, emptyState }) return table } diff --git a/app/pages/system/inventory/SledsTab.tsx b/app/pages/system/inventory/SledsTab.tsx index 82e92ed934..1ac5c55548 100644 --- a/app/pages/system/inventory/SledsTab.tsx +++ b/app/pages/system/inventory/SledsTab.tsx @@ -45,7 +45,7 @@ const EmptyState = () => { const sledList = getListQFn('sledList', {}, { placeholderData: (x) => x }) export async function loader() { - await queryClient.prefetchQuery(sledList()) + await queryClient.prefetchQuery(sledList.optionsFn()) return null } @@ -75,10 +75,7 @@ const staticCols = [ Component.displayName = 'SledsTab' export function Component() { - const { table } = useQueryTable({ - optionsFn: sledList, - columns: staticCols, - emptyState: , - }) + const emptyState = + const { table } = useQueryTable({ query: sledList, columns: staticCols, emptyState }) return table } diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index 4985bd2d7a..9a38bcc568 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -5,11 +5,11 @@ * * Copyright Oxide Computer Company */ -import { useQuery, type QueryKey, type QueryOptions } from '@tanstack/react-query' +import { useQuery } from '@tanstack/react-query' import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' import React, { useMemo } from 'react' -import { ensurePrefetched, PAGE_SIZE, type ApiError, type ResultsPage } from '@oxide/api' +import { ensurePrefetched, type PaginatedQuery, type ResultsPage } from '@oxide/api' import { Pagination } from '~/components/Pagination' import { usePagination } from '~/hooks/use-pagination' @@ -19,13 +19,7 @@ import { TableEmptyBox } from '~/ui/lib/Table' import { Table } from './Table' type QueryTableProps = { - optionsFn: ( - limit: number, - page_token?: string - ) => QueryOptions, ApiError> & { - queryKey: QueryKey - } - pageSize?: number + query: PaginatedQuery> rowHeight?: 'small' | 'large' emptyState: React.ReactElement // React Table does the same in the type of `columns` on `useReactTable` @@ -35,14 +29,13 @@ type QueryTableProps = { // require ID only so we can use it in getRowId export function useQueryTable({ - optionsFn, - pageSize = PAGE_SIZE, + query, rowHeight = 'small', emptyState, columns, }: QueryTableProps) { const { currentPage, goToNextPage, goToPrevPage, hasPrev } = usePagination() - const queryOptions = optionsFn(pageSize, currentPage) + const queryOptions = query.optionsFn(currentPage) const queryResult = useQuery(queryOptions) // only ensure prefetched if we're on the first page if (currentPage === undefined) ensurePrefetched(queryResult, queryOptions.queryKey) @@ -65,8 +58,8 @@ export function useQueryTable({ <>
Date: Thu, 21 Nov 2024 16:26:25 -0600 Subject: [PATCH 05/10] do the placeholderData thing for all lists --- app/api/hooks.ts | 6 +++++- app/pages/system/inventory/DisksTab.tsx | 2 +- app/pages/system/inventory/SledsTab.tsx | 2 +- 3 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/api/hooks.ts b/app/api/hooks.ts index 8ee11ea95e..af176bbf1a 100644 --- a/app/api/hooks.ts +++ b/app/api/hooks.ts @@ -171,7 +171,11 @@ export const getListQueryOptionsFn = return { optionsFn: (pageToken?: string) => { const newParams = { ...params, query: { ...params.query, limit, pageToken } } - return getApiQueryOptions(api)(method, newParams, options) + return getApiQueryOptions(api)(method, newParams, { + ...options, + // identity function so current page sticks around while next loads + placeholderData: (x) => x, + }) }, pageSize: limit, } diff --git a/app/pages/system/inventory/DisksTab.tsx b/app/pages/system/inventory/DisksTab.tsx index dc8f057dc0..3e9a3fb16f 100644 --- a/app/pages/system/inventory/DisksTab.tsx +++ b/app/pages/system/inventory/DisksTab.tsx @@ -38,7 +38,7 @@ const EmptyState = () => ( /> ) -const diskList = getListQFn('physicalDiskList', {}, { placeholderData: (x) => x }) +const diskList = getListQFn('physicalDiskList', {}) export async function loader() { await queryClient.prefetchQuery(diskList.optionsFn()) diff --git a/app/pages/system/inventory/SledsTab.tsx b/app/pages/system/inventory/SledsTab.tsx index 1ac5c55548..9a7a5346ad 100644 --- a/app/pages/system/inventory/SledsTab.tsx +++ b/app/pages/system/inventory/SledsTab.tsx @@ -42,7 +42,7 @@ const EmptyState = () => { ) } -const sledList = getListQFn('sledList', {}, { placeholderData: (x) => x }) +const sledList = getListQFn('sledList', {}) export async function loader() { await queryClient.prefetchQuery(sledList.optionsFn()) From ca6f43032af530897c891b2e8ddefac86f92038c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 21 Nov 2024 18:02:28 -0600 Subject: [PATCH 06/10] scroll to top when page changes --- app/layouts/helpers.tsx | 7 ++++++- app/table/QueryTable2.tsx | 11 ++++++++--- test/e2e/pagination.e2e.ts | 2 ++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/app/layouts/helpers.tsx b/app/layouts/helpers.tsx index 1aef3d1865..d4769685d7 100644 --- a/app/layouts/helpers.tsx +++ b/app/layouts/helpers.tsx @@ -20,7 +20,12 @@ export function ContentPane() { const ref = useRef(null) useScrollRestoration(ref) return ( -
+
diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index 9a38bcc568..7a2b65e56f 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -7,7 +7,7 @@ */ import { useQuery } from '@tanstack/react-query' import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' -import React, { useMemo } from 'react' +import React, { useEffect, useMemo } from 'react' import { ensurePrefetched, type PaginatedQuery, type ResultsPage } from '@oxide/api' @@ -39,9 +39,14 @@ export function useQueryTable({ const queryResult = useQuery(queryOptions) // only ensure prefetched if we're on the first page if (currentPage === undefined) ensurePrefetched(queryResult, queryOptions.queryKey) - const { data, isLoading } = queryResult + const { data } = queryResult const tableData = useMemo(() => data?.items || [], [data]) + const firstItemId = tableData?.[0].id + useEffect(() => { + document.querySelector('#scroll-container')?.scrollTo(0, 0) + }, [firstItemId]) + const table = useReactTable({ columns, data: tableData, @@ -52,7 +57,7 @@ export function useQueryTable({ const isEmpty = tableData.length === 0 && !hasPrev - const tableElement = isLoading ? null : isEmpty ? ( + const tableElement = isEmpty ? ( {emptyState || } ) : ( <> diff --git a/test/e2e/pagination.e2e.ts b/test/e2e/pagination.e2e.ts index 2013675b36..4523966ab0 100644 --- a/test/e2e/pagination.e2e.ts +++ b/test/e2e/pagination.e2e.ts @@ -34,3 +34,5 @@ test('pagination', async ({ page }) => { await nextButton.click() await expect(nextButton).toBeDisabled() // no more pages }) + +// TODO: test scroll to top on page change From 65bb5fd9446b74d5e8fcf6d72a71149bea3fb557 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 21 Nov 2024 20:03:14 -0600 Subject: [PATCH 07/10] loading spinner on page changes! --- app/table/QueryTable2.tsx | 5 ++++- app/ui/lib/Pagination.tsx | 7 ++++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index 7a2b65e56f..06774cb9d4 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -39,7 +39,7 @@ export function useQueryTable({ const queryResult = useQuery(queryOptions) // only ensure prefetched if we're on the first page if (currentPage === undefined) ensurePrefetched(queryResult, queryOptions.queryKey) - const { data } = queryResult + const { data, isPlaceholderData } = queryResult const tableData = useMemo(() => data?.items || [], [data]) const firstItemId = tableData?.[0].id @@ -69,6 +69,9 @@ export function useQueryTable({ nextPage={data?.nextPage} onNext={goToNextPage} onPrev={goToPrevPage} + // I can't believe how well this works, but it exactly matches when + // we want to show the spinner. Cached page changes don't need it. + loading={isPlaceholderData} /> ) diff --git a/app/ui/lib/Pagination.tsx b/app/ui/lib/Pagination.tsx index 1257e10e8c..005161a635 100644 --- a/app/ui/lib/Pagination.tsx +++ b/app/ui/lib/Pagination.tsx @@ -9,6 +9,8 @@ import cn from 'classnames' import { DirectionLeftIcon, DirectionRightIcon } from '@oxide/design-system/icons/react' +import { Spinner } from './Spinner' + interface PageInputProps { number: number className?: string @@ -34,6 +36,7 @@ export interface PaginationProps { onNext: (nextPage: string) => void onPrev: () => void className?: string + loading?: boolean } export const Pagination = ({ pageSize, @@ -43,6 +46,7 @@ export const Pagination = ({ onNext, onPrev, className, + loading, }: PaginationProps) => { return ( <> @@ -55,7 +59,8 @@ export const Pagination = ({ rows per page - + + {loading && } -
+ ) } diff --git a/mock-api/snapshot.ts b/mock-api/snapshot.ts index 4a70380f9a..33effdb4c6 100644 --- a/mock-api/snapshot.ts +++ b/mock-api/snapshot.ts @@ -14,7 +14,7 @@ import { disks } from './disk' import type { Json } from './json-type' import { project } from './project' -const generatedSnapshots: Json[] = Array.from({ length: 25 }, (_, i) => +const generatedSnapshots: Json[] = Array.from({ length: 80 }, (_, i) => generateSnapshot(i) ) @@ -91,7 +91,7 @@ export const snapshots: Json[] = [ function generateSnapshot(index: number): Json { return { id: uuid(), - name: `disk-1-snapshot-${index + 6}`, + name: `disk-1-snapshot-${index + 7}`, description: '', project_id: project.id, time_created: new Date().toISOString(), diff --git a/test/e2e/pagination.e2e.ts b/test/e2e/pagination.e2e.ts index 4523966ab0..30322a7901 100644 --- a/test/e2e/pagination.e2e.ts +++ b/test/e2e/pagination.e2e.ts @@ -5,34 +5,65 @@ * * Copyright Oxide Computer Company */ -import { expect, test } from '@playwright/test' +import { expect, test, type Page } from '@playwright/test' -import { expectRowVisible } from './utils' +import { expectScrollTop, scrollTo } from './utils' + +// expectRowVisible is too have for all this +const expectCell = (page: Page, name: string) => + expect(page.getByRole('cell', { name, exact: true })).toBeVisible() test('pagination', async ({ page }) => { await page.goto('/projects/mock-project/snapshots') const table = page.getByRole('table') - const rows = page.getByRole('row') + const rows = table.getByRole('rowgroup').last().getByRole('row') const nextButton = page.getByRole('button', { name: 'next' }) const prevButton = page.getByRole('button', { name: 'prev', exact: true }) + const spinner = page.getByLabel('Pagination').getByLabel('Spinner') + + await expect(spinner).toBeHidden() + await expect(prevButton).toBeDisabled() // we're on the first page - await expect(rows).toHaveCount(26) - await expectRowVisible(table, { name: 'snapshot-1' }) - await expectRowVisible(table, { name: 'disk-1-snapshot-24' }) + await expectCell(page, 'snapshot-1') + await expectCell(page, 'disk-1-snapshot-25') + await expect(rows).toHaveCount(25) + + await scrollTo(page, 100) await nextButton.click() - await expect(rows).toHaveCount(7) - await expectRowVisible(table, { name: 'disk-1-snapshot-25' }) - await expectRowVisible(table, { name: 'disk-1-snapshot-30' }) - await prevButton.click() - await expect(rows).toHaveCount(26) - await expectRowVisible(table, { name: 'snapshot-1' }) - await expectRowVisible(table, { name: 'disk-1-snapshot-24' }) + // spinner goes while the data is fetching... + await expect(spinner).toBeVisible() + await expectScrollTop(page, 100) // scroll resets to top on page change + // ...and goes away roughly when scroll resets + await expect(spinner).toBeHidden() + await expectScrollTop(page, 0) // scroll resets to top on page change + + await expectCell(page, 'disk-1-snapshot-26') + await expectCell(page, 'disk-1-snapshot-50') + await expect(rows).toHaveCount(25) + + await nextButton.click() + await expectCell(page, 'disk-1-snapshot-51') + await expectCell(page, 'disk-1-snapshot-75') + await expect(rows).toHaveCount(25) await nextButton.click() + await expectCell(page, 'disk-1-snapshot-76') + await expectCell(page, 'disk-1-snapshot-86') + await expect(rows).toHaveCount(11) await expect(nextButton).toBeDisabled() // no more pages -}) -// TODO: test scroll to top on page change + await scrollTo(page, 250) + + await prevButton.click() + await expect(spinner).toBeHidden({ timeout: 10 }) // no spinner, cached page + await expect(rows).toHaveCount(25) + await expectCell(page, 'disk-1-snapshot-51') + await expectCell(page, 'disk-1-snapshot-75') + await expectScrollTop(page, 0) // scroll resets to top on prev too + + await nextButton.click() + await expect(spinner).toBeHidden({ timeout: 10 }) // no spinner, cached page +}) diff --git a/test/e2e/scroll-restore.e2e.ts b/test/e2e/scroll-restore.e2e.ts index 318b2e8a59..483c0e9e82 100644 --- a/test/e2e/scroll-restore.e2e.ts +++ b/test/e2e/scroll-restore.e2e.ts @@ -5,18 +5,9 @@ * * Copyright Oxide Computer Company */ -import { expect, test, type Page } from './utils' +import { expect, test } from '@playwright/test' -async function expectScrollTop(page: Page, expected: number) { - const container = page.getByTestId('scroll-container') - const getScrollTop = () => container.evaluate((el: HTMLElement) => el.scrollTop) - await expect.poll(getScrollTop).toBe(expected) -} - -async function scrollTo(page: Page, to: number) { - const container = page.getByTestId('scroll-container') - await container.evaluate((el: HTMLElement, to) => el.scrollTo(0, to), to) -} +import { expectScrollTop, scrollTo } from './utils' test('scroll restore', async ({ page }) => { // open small window to make scrolling easier diff --git a/test/e2e/utils.ts b/test/e2e/utils.ts index 2f8437d0cd..38e32a5ab1 100644 --- a/test/e2e/utils.ts +++ b/test/e2e/utils.ts @@ -232,3 +232,14 @@ export async function chooseFile( buffer: size === 'large' ? bigFile : smallFile, }) } + +export async function expectScrollTop(page: Page, expected: number) { + const container = page.getByTestId('scroll-container') + const getScrollTop = () => container.evaluate((el: HTMLElement) => el.scrollTop) + await expect.poll(getScrollTop).toBe(expected) +} + +export async function scrollTo(page: Page, to: number) { + const container = page.getByTestId('scroll-container') + await container.evaluate((el: HTMLElement, to) => el.scrollTo(0, to), to) +} From 5756daec26f32c78c178d999bb55f187d9ab259c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 21 Nov 2024 22:18:43 -0600 Subject: [PATCH 09/10] fix other e2es, don't scroll reset on browser forward/back --- app/table/QueryTable2.tsx | 28 ++++++++++++++++++++++------ test/e2e/snapshots.e2e.ts | 2 +- 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index 72125339fb..8f0c7b28ce 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -7,7 +7,7 @@ */ import { useQuery } from '@tanstack/react-query' import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' -import React, { useEffect, useMemo } from 'react' +import React, { useEffect, useMemo, useRef } from 'react' import { ensurePrefetched, type PaginatedQuery, type ResultsPage } from '@oxide/api' @@ -27,8 +27,6 @@ type QueryTableProps = { columns: ColumnDef[] } -const resetScroll = () => document.querySelector('#scroll-container')?.scrollTo(0, 0) - // require ID only so we can use it in getRowId export function useQueryTable({ query, @@ -44,8 +42,20 @@ export function useQueryTable({ const { data, isPlaceholderData } = queryResult const tableData = useMemo(() => data?.items || [], [data]) + // this is annoying, but basically we only want this to happen when clicking + // next/prev to change page, not, for example, on initial pageload after + // browser forward/back. + const needScrollReset = useRef(false) const firstItemId = tableData?.[0].id - useEffect(resetScroll, [firstItemId]) + useEffect(() => { + if (needScrollReset.current) { + document.querySelector('#scroll-container')?.scrollTo(0, 0) + needScrollReset.current = false + } + // trigger by first item ID and not, e.g., currentPage because currentPage changes + // as soon as you click Next, while the item ID doesn't change until the page + // actually changes. + }, [firstItemId]) const table = useReactTable({ columns, @@ -67,8 +77,14 @@ export function useQueryTable({ hasNext={tableData.length === query.pageSize} hasPrev={hasPrev} nextPage={data?.nextPage} - onNext={goToNextPage} - onPrev={goToPrevPage} + onNext={(p) => { + needScrollReset.current = true + goToNextPage(p) + }} + onPrev={() => { + needScrollReset.current = true + goToPrevPage() + }} // I can't believe how well this works, but it exactly matches when // we want to show the spinner. Cached page changes don't need it. loading={isPlaceholderData} diff --git a/test/e2e/snapshots.e2e.ts b/test/e2e/snapshots.e2e.ts index 2e211b4d6e..4467a50ad5 100644 --- a/test/e2e/snapshots.e2e.ts +++ b/test/e2e/snapshots.e2e.ts @@ -28,7 +28,7 @@ test('Click through snapshots', async ({ page }) => { test('Confirm delete snapshot', async ({ page }) => { await page.goto('/projects/mock-project/snapshots') - const row = page.getByRole('row', { name: 'disk-1-snapshot-6' }) + const row = page.getByRole('row', { name: 'disk-1-snapshot-7' }) // scroll a little so the dropdown menu isn't behind the pagination bar await page.getByRole('table').click() // focus the content pane From 31ddca8ded9e424408a830c329b8ba5ca9b9a55b Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 21 Nov 2024 23:23:27 -0600 Subject: [PATCH 10/10] fix bug found while converting other tables, extract useScrollReset --- app/api/hooks.ts | 6 +++++- app/table/QueryTable2.tsx | 39 +++++++++++++++++++++++---------------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/app/api/hooks.ts b/app/api/hooks.ts index af176bbf1a..9f267bf6ed 100644 --- a/app/api/hooks.ts +++ b/app/api/hooks.ts @@ -165,7 +165,11 @@ export const getListQueryOptionsFn = params: Params, options: UseQueryOtherOptions, ApiError> = {} ): PaginatedQuery> => { - // pathOr plays nice when the properties don't exist + // We pull limit out of the query params rather than passing it in some + // other way so that there is exactly one way of specifying it. If we had + // some other way of doing it, and then you also passed it in as a query + // param, it would be hard to guess which takes precedence. (pathOr plays + // nice when the properties don't exist.) // eslint-disable-next-line @typescript-eslint/no-explicit-any const limit = R.pathOr(params as any, ['query', 'limit'], PAGE_SIZE) return { diff --git a/app/table/QueryTable2.tsx b/app/table/QueryTable2.tsx index 8f0c7b28ce..55dd7b623b 100644 --- a/app/table/QueryTable2.tsx +++ b/app/table/QueryTable2.tsx @@ -27,6 +27,23 @@ type QueryTableProps = { columns: ColumnDef[] } +/** + * Reset scroll to top when clicking * next/prev to change page but not, + * for example, on initial pageload after browser forward/back. + */ +function useScrollReset(triggerDep: string | undefined) { + const resetRequested = useRef(false) + useEffect(() => { + if (resetRequested.current) { + document.querySelector('#scroll-container')?.scrollTo(0, 0) + resetRequested.current = false + } + }, [triggerDep]) + return () => { + resetRequested.current = true + } +} + // require ID only so we can use it in getRowId export function useQueryTable({ query, @@ -42,20 +59,10 @@ export function useQueryTable({ const { data, isPlaceholderData } = queryResult const tableData = useMemo(() => data?.items || [], [data]) - // this is annoying, but basically we only want this to happen when clicking - // next/prev to change page, not, for example, on initial pageload after - // browser forward/back. - const needScrollReset = useRef(false) - const firstItemId = tableData?.[0].id - useEffect(() => { - if (needScrollReset.current) { - document.querySelector('#scroll-container')?.scrollTo(0, 0) - needScrollReset.current = false - } - // trigger by first item ID and not, e.g., currentPage because currentPage changes - // as soon as you click Next, while the item ID doesn't change until the page - // actually changes. - }, [firstItemId]) + // trigger by first item ID and not, e.g., currentPage because currentPage + // changes as soon as you click Next, while the item ID doesn't change until + // the page actually changes. + const requestScrollReset = useScrollReset(tableData.at(0)?.id) const table = useReactTable({ columns, @@ -78,11 +85,11 @@ export function useQueryTable({ hasPrev={hasPrev} nextPage={data?.nextPage} onNext={(p) => { - needScrollReset.current = true + requestScrollReset() goToNextPage(p) }} onPrev={() => { - needScrollReset.current = true + requestScrollReset() goToPrevPage() }} // I can't believe how well this works, but it exactly matches when