diff --git a/app/api/client.ts b/app/api/client.ts index cdd3c08875..2d8da89976 100644 --- a/app/api/client.ts +++ b/app/api/client.ts @@ -9,6 +9,8 @@ import { QueryClient } from '@tanstack/react-query' import { Api } from './__generated__/Api' import { + getApiQueryOptions, + getListQueryOptionsFn, getUseApiMutation, getUseApiQueries, getUseApiQuery, @@ -17,6 +19,8 @@ import { wrapQueryClient, } 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 host: process.env.NODE_ENV === 'test' ? 'http://testhost' : '', @@ -24,6 +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..9f267bf6ed 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' @@ -28,6 +29,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 +125,66 @@ 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 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) => + < + 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> = {} + ): PaginatedQuery> => { + // 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 { + optionsFn: (pageToken?: string) => { + const newParams = { ...params, query: { ...params.query, limit, pageToken } } + return getApiQueryOptions(api)(method, newParams, { + ...options, + // identity function so current page sticks around while next loads + placeholderData: (x) => x, + }) + }, + pageSize: limit, + } + } + export const getUseApiQuery = (api: A) => ( @@ -140,7 +202,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 +214,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/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/pages/project/snapshots/SnapshotsPage.tsx b/app/pages/project/snapshots/SnapshotsPage.tsx index d2b0e28ab1..828f849eb0 100644 --- a/app/pages/project/snapshots/SnapshotsPage.tsx +++ b/app/pages/project/snapshots/SnapshotsPage.tsx @@ -11,6 +11,8 @@ import { Outlet, useNavigate, type LoaderFunctionArgs } from 'react-router-dom' import { apiQueryClient, + getListQFn, + 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 { useQueryTable } from '~/table/QueryTable2' import { Badge } from '~/ui/lib/Badge' import { CreateLink } from '~/ui/lib/CreateButton' import { EmptyMessage } from '~/ui/lib/EmptyMessage' @@ -52,12 +54,12 @@ const EmptyState = () => ( /> ) +const snapshotList = (project: string) => getListQFn('snapshotList', { query: { project } }) + SnapshotsPage.loader = async ({ params }: LoaderFunctionArgs) => { const { project } = getProjectSelector(params) await Promise.all([ - apiQueryClient.prefetchQuery('snapshotList', { - query: { project, limit: PAGE_SIZE }, - }), + 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 @@ -100,7 +102,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 +133,11 @@ export function SnapshotsPage() { [deleteSnapshot, navigate, project] ) const columns = useColsWithActions(staticCols, makeActions) + const { table } = useQueryTable({ + query: snapshotList(project), + columns, + emptyState: , + }) return ( <> @@ -146,7 +152,7 @@ export function SnapshotsPage() { New snapshot - } /> + {table} ) diff --git a/app/pages/system/inventory/DisksTab.tsx b/app/pages/system/inventory/DisksTab.tsx index e029e66f9e..3e9a3fb16f 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, + getListQFn, + 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 { useQueryTable } from '~/table/QueryTable2' import { Badge, type BadgeColor } from '~/ui/lib/Badge' import { EmptyMessage } from '~/ui/lib/EmptyMessage' @@ -37,8 +38,10 @@ const EmptyState = () => ( /> ) +const diskList = getListQFn('physicalDiskList', {}) + export async function loader() { - await apiQueryClient.prefetchQuery('physicalDiskList', { query: { limit: PAGE_SIZE } }) + await queryClient.prefetchQuery(diskList.optionsFn()) return null } @@ -68,6 +71,7 @@ const staticCols = [ Component.displayName = 'DisksTab' export function Component() { - const { Table } = useQueryTable('physicalDiskList', {}) - return
} columns={staticCols} /> + 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 94b8da375f..9a7a5346ad 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 { apiQueryClient, 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/QueryTable' +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,10 +42,10 @@ const EmptyState = () => { ) } +const sledList = getListQFn('sledList', {}) + export async function loader() { - await apiQueryClient.prefetchQuery('sledList', { - query: { limit: PAGE_SIZE }, - }) + await queryClient.prefetchQuery(sledList.optionsFn()) return null } @@ -69,6 +75,7 @@ const staticCols = [ Component.displayName = 'SledsTab' export function Component() { - const { Table } = useQueryTable('sledList', {}, { placeholderData: (x) => x }) - return
} columns={staticCols} /> + const emptyState = + const { table } = useQueryTable({ query: sledList, columns: staticCols, emptyState }) + return table } 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 new file mode 100644 index 0000000000..55dd7b623b --- /dev/null +++ b/app/table/QueryTable2.tsx @@ -0,0 +1,103 @@ +/* + * 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 } from '@tanstack/react-query' +import { getCoreRowModel, useReactTable, type ColumnDef } from '@tanstack/react-table' +import React, { useEffect, useMemo, useRef } from 'react' + +import { ensurePrefetched, type PaginatedQuery, type ResultsPage } 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' + +type QueryTableProps = { + query: PaginatedQuery> + 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[] +} + +/** + * 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, + rowHeight = 'small', + emptyState, + columns, +}: QueryTableProps) { + const { currentPage, goToNextPage, goToPrevPage, hasPrev } = usePagination() + 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) + const { data, isPlaceholderData } = queryResult + const tableData = useMemo(() => data?.items || [], [data]) + + // 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, + data: tableData, + getRowId: (row) => row.id, + getCoreRowModel: getCoreRowModel(), + manualPagination: true, + }) + + const isEmpty = tableData.length === 0 && !hasPrev + + const tableElement = isEmpty ? ( + {emptyState || } + ) : ( + <> +
+ { + requestScrollReset() + goToNextPage(p) + }} + onPrev={() => { + requestScrollReset() + 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} + /> + + ) + + return { table: tableElement, query: queryResult } +} diff --git a/app/ui/lib/Pagination.tsx b/app/ui/lib/Pagination.tsx index 1257e10e8c..76157befa8 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,10 +46,12 @@ export const Pagination = ({ onNext, onPrev, className, + loading, }: PaginationProps) => { return ( <> -
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 2013675b36..30322a7901 100644 --- a/test/e2e/pagination.e2e.ts +++ b/test/e2e/pagination.e2e.ts @@ -5,32 +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 expectCell(page, 'snapshot-1') + await expectCell(page, 'disk-1-snapshot-25') + await expect(rows).toHaveCount(25) - await expect(rows).toHaveCount(26) - await expectRowVisible(table, { name: 'snapshot-1' }) - await expectRowVisible(table, { name: 'disk-1-snapshot-24' }) + 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 + + 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/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 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) +}