diff --git a/packages/app-elements/src/ui/resources/useResourceFilters/adaptSdkToMetrics.test.ts b/packages/app-elements/src/ui/resources/useResourceFilters/adaptSdkToMetrics.test.ts index afbdb43d8..e78be5ce6 100644 --- a/packages/app-elements/src/ui/resources/useResourceFilters/adaptSdkToMetrics.test.ts +++ b/packages/app-elements/src/ui/resources/useResourceFilters/adaptSdkToMetrics.test.ts @@ -87,6 +87,26 @@ describe("adaptSdkToMetrics", () => { expect(metricsFilters.order?.date_field).toBe("updated_at") }) + // The default range is anchored to the current time, so two calls one second + // apart disagree. `useResourceList` deep-compares `metricsQuery` and refetches + // from page 1 when it differs, which is why `useResourceFilters` computes this + // filter once per mount instead of on every render. + test("Should move the default date range as the clock advances", () => { + const args = { + sdkFilters: {}, + resourceType: "orders", + instructions, + } as const + + const before = adaptSdkToMetrics(args) + vi.advanceTimersByTime(1000) + const after = adaptSdkToMetrics(args) + + expect(before.order?.date_to).toBe("2023-04-05T15:20:00Z") + expect(after.order?.date_to).toBe("2023-04-05T15:20:01Z") + expect(after).not.toStrictEqual(before) + }) + test("Should set a default 5-year date range when text search is defined", () => { const metricsFilters = adaptSdkToMetrics({ sdkFilters: { diff --git a/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.test.tsx b/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.test.tsx new file mode 100644 index 000000000..ecfe852a0 --- /dev/null +++ b/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.test.tsx @@ -0,0 +1,89 @@ +import { render, waitFor } from "@testing-library/react" +import { act, type FC } from "react" +import { CoreSdkProvider } from "#providers/CoreSdkProvider" +import { MockTokenProvider as TokenProvider } from "#providers/TokenProvider/MockTokenProvider" +import { instructions } from "./mockedInstructions" +import { useResourceFilters } from "./useResourceFilters" + +/** + * `FilteredList` and `FilteredTable` are memoized on the `sdkFilters` object, so + * a fresh identity is a fresh component type and React remounts the whole list. + * The query string carries more than filters, and an unrelated parameter must + * not cost a remount. + */ +describe("useResourceFilters", () => { + let renders: Array<{ + sdkFilters: unknown + FilteredList: unknown + FilteredTable: unknown + }> = [] + + const Harness: FC = () => { + const { sdkFilters, FilteredList, FilteredTable } = useResourceFilters({ + instructions, + }) + renders.push({ sdkFilters, FilteredList, FilteredTable }) + return
{sdkFilters == null ? "pending" : "ready"}
+ } + + // wouter patches `history.pushState` and dispatches an event, so navigating + // this way is what a real url change looks like to the hook + const navigate = (search: string): void => { + act(() => { + window.history.pushState({}, "", search) + }) + } + + const renderHarness = async (search: string): Promise => { + window.history.pushState({}, "", search) + render( + + + + + , + ) + await waitFor(() => { + expect(renders.at(-1)?.sdkFilters).not.toBeUndefined() + }) + } + + beforeEach(() => { + renders = [] + // jsdom keeps a single location per test file, so a test that navigated + // would otherwise leak its query string into the next one + window.history.pushState({}, "", "/") + }) + + test("keeps the memoized list components when an unrelated query param changes", async () => { + await renderHarness("/?status_in=placed") + const before = renders.at(-1) + const renderCountBefore = renders.length + + navigate("/?status_in=placed&page=2") + + await waitFor(() => { + expect(renders.length).toBeGreaterThan(renderCountBefore) + }) + + const after = renders.at(-1) + expect(after?.sdkFilters).toBe(before?.sdkFilters) + expect(after?.FilteredList).toBe(before?.FilteredList) + expect(after?.FilteredTable).toBe(before?.FilteredTable) + }) + + test("rebuilds the memoized list components when the filters really change", async () => { + await renderHarness("/?status_in=placed") + const before = renders.at(-1) + + navigate("/?status_in=approved") + + await waitFor(() => { + expect(renders.at(-1)?.sdkFilters).not.toBe(before?.sdkFilters) + }) + + const after = renders.at(-1) + expect(after?.FilteredList).not.toBe(before?.FilteredList) + expect(after?.FilteredTable).not.toBe(before?.FilteredTable) + }) +}) diff --git a/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.tsx b/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.tsx index 0e871f405..751c1af75 100644 --- a/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.tsx +++ b/packages/app-elements/src/ui/resources/useResourceFilters/useResourceFilters.tsx @@ -1,4 +1,5 @@ import type { ListableResourceType, QueryFilter } from "@commercelayer/sdk" +import isEqual from "lodash-es/isEqual" import { type JSX, useCallback, @@ -290,11 +291,19 @@ export function useResourceFilters({ useEffect( function updateSdkQueryFilterOnSearchChange() { - setSdkFilters( - adapters.adaptUrlQueryToSdk({ - queryString, - timezone: user?.timezone, - }), + const nextSdkFilters = adapters.adaptUrlQueryToSdk({ + queryString, + timezone: user?.timezone, + }) + // `FilteredList` and `FilteredTable` are memoized on this object, so a new + // identity is a new component type and React remounts the whole list. + // The query string carries more than filters (a view title, a page), and + // those must not blank the rows: keep the previous object whenever the + // filters it encodes are unchanged, which makes React skip the update. + setSdkFilters((currentSdkFilters) => + isEqual(currentSdkFilters, nextSdkFilters) + ? currentSdkFilters + : nextSdkFilters, ) }, [queryString], @@ -361,6 +370,38 @@ function ResourceListComponent({ ) } +/** + * Metrics filter for the current `sdkFilters`, computed once per mount. + * + * It must not be rebuilt on every render: with no explicit date filter, + * `adaptSdkToMetrics` falls back to a range anchored to `new Date()` (truncated + * to the second). `useResourceList` deep-compares `{ query, metricsQuery }` and + * refetches from page 1 when it differs, so an unmemoized filter turns any + * re-render landing in a later second — opening the filters drawer, switching + * tab — into a full reload of the list. + */ +function useMetricsFilter({ + adapters, + sdkFilters, + type, +}: { + adapters: ReturnType + sdkFilters: QueryFilter | undefined + type: TResource +}): ReturnType["adaptSdkToMetrics"]> { + // Both call sites bail out before rendering when `sdkFilters` is undefined, so + // the value computed for that case is never sent; an empty object keeps the + // hook unconditional and its return type free of a null nobody has to handle. + return useMemo( + () => + adapters.adaptSdkToMetrics({ + sdkFilters: sdkFilters ?? {}, + resourceType: type, + }), + [sdkFilters, type], + ) +} + const makeFilteredList: (options: { sdkFilters: QueryFilter | undefined adapters: ReturnType @@ -368,6 +409,7 @@ const makeFilteredList: (options: { ({ sdkFilters, adapters }) => ({ type, query, metricsQuery, hideTitle, ...resourceListProps }) => { const { t } = useTranslation() + const metricsFilter = useMetricsFilter({ adapters, sdkFilters, type }) if (resourceListProps == null) { return
resourceListProps not defined
@@ -394,10 +436,7 @@ const makeFilteredList: (options: { ? undefined : { ...metricsQuery, - filter: adapters.adaptSdkToMetrics({ - sdkFilters, - resourceType: type, - }), + filter: metricsFilter, } } /> @@ -450,6 +489,7 @@ const makeFilteredTable: (options: { ({ sdkFilters, adapters }) => ({ type, query, metricsQuery, hideTitle, ...tableProps }) => { const { t } = useTranslation() + const metricsFilter = useMetricsFilter({ adapters, sdkFilters, type }) if (sdkFilters == null) { return null @@ -471,10 +511,7 @@ const makeFilteredTable: (options: { ? undefined : { ...metricsQuery, - filter: adapters.adaptSdkToMetrics({ - sdkFilters, - resourceType: type, - }), + filter: metricsFilter, } } /> diff --git a/packages/app-elements/src/ui/resources/useResourceList/useMetricsCursorTrail.ts b/packages/app-elements/src/ui/resources/useResourceList/useMetricsCursorTrail.ts new file mode 100644 index 000000000..c390830dc --- /dev/null +++ b/packages/app-elements/src/ui/resources/useResourceList/useMetricsCursorTrail.ts @@ -0,0 +1,201 @@ +import { useCallback, useRef } from "react" +import { useTokenProvider } from "#providers/TokenProvider" + +const storageVersion = 1 + +interface StoredTrail { + version: number + fingerprint: string + cursors: Array +} + +export interface MetricsCursorTrail { + /** + * The cursor that opens `page`, or `undefined` when the trail does not hold + * it — in which case that page cannot be opened at all. + */ + cursorFor: (page: number) => string | null | undefined + /** + * Record, after loading `page`, the cursor that opens the page after it. + */ + record: (page: number, cursor: string | null) => void + /** Forget every cursor but page 1's, which is always "no cursor". */ + reset: () => void +} + +/** + * The cursor trail of a metrics-backed list, kept in `sessionStorage`. + * + * The metrics api only moves forward: the cursor that opens page N is handed + * back when page N-1 is fetched. Holding those cursors is what lets the pager + * step backwards, and persisting them is what lets a list reopen on the page it + * was left on, since going to a details page unmounts the list and would + * otherwise take the trail with it. + * + * `sessionStorage` is the right shelf for it: it is per tab, it survives a + * reload, and it is gone when the tab is. A trail is private to the session + * that walked it, and a link someone shares carries no cursor. + * + * One entry per list, so nothing accumulates. The entry records the query it + * was walked against, and a trail measured against a different one is dropped + * rather than trusted: a cursor is only meaningful for the exact filters and + * sort that produced it. + */ +export function useMetricsCursorTrail({ + enabled, + type, + metricsQuery, +}: { + /** Only metrics-backed lists in `pagination` mode keep a trail. */ + enabled: boolean + type: string + metricsQuery: unknown +}): MetricsCursorTrail { + const { + settings: { mode, organizationSlug, appSlug, domain }, + } = useTokenProvider() + + // Scope in clear so entries stay readable in devtools and can be evicted by + // prefix; without it, switching organization or moving from test to live + // would resume paging against another scope's trail. + const storageKey = `cl.metrics.trail.${mode}.${organizationSlug}.${appSlug}.${type}` + + const fingerprint = fingerprintOf(`${domain}|${queryIdentity(metricsQuery)}`) + + const trailRef = useRef<{ + fingerprint: string + cursors: Array + } | null>(null) + + const cursors = useCallback((): Array => { + if (trailRef.current?.fingerprint === fingerprint) { + return trailRef.current.cursors + } + // first use, or the query changed under us: whatever was remembered was + // measured against a different list + const loaded = enabled ? readTrail(storageKey, fingerprint) : [null] + trailRef.current = { fingerprint, cursors: loaded } + return loaded + }, [enabled, storageKey, fingerprint]) + + const persist = useCallback(() => { + if (!enabled || trailRef.current == null) { + return + } + writeTrail(storageKey, { + version: storageVersion, + fingerprint, + cursors: trailRef.current.cursors, + }) + }, [enabled, storageKey, fingerprint]) + + const cursorFor = useCallback( + (page: number) => cursors()[page - 1], + [cursors], + ) + + const record = useCallback( + (page: number, cursor: string | null) => { + cursors()[page] = cursor + persist() + }, + [cursors, persist], + ) + + const reset = useCallback(() => { + trailRef.current = { fingerprint, cursors: [null] } + persist() + }, [fingerprint, persist]) + + return { cursorFor, record, reset } +} + +function readTrail(key: string, fingerprint: string): Array { + if (typeof window === "undefined") { + return [null] + } + + try { + const raw = window.sessionStorage.getItem(key) + if (raw == null) { + return [null] + } + + const stored = JSON.parse(raw) as StoredTrail + if ( + stored.version !== storageVersion || + stored.fingerprint !== fingerprint || + !Array.isArray(stored.cursors) || + stored.cursors[0] !== null + ) { + return [null] + } + + return stored.cursors + } catch { + // unreadable or unparseable: a lost trail only costs a walk from page 1 + return [null] + } +} + +function writeTrail(key: string, trail: StoredTrail): void { + if (typeof window === "undefined") { + return + } + + try { + window.sessionStorage.setItem(key, JSON.stringify(trail)) + } catch { + // storage can be full or unavailable (private browsing); the trail is a + // convenience, so losing it must never break paging + } +} + +/** + * What makes two metrics queries "the same list" for the purpose of reusing a + * cursor trail. + * + * The query is hashed rather than stored: filter values reach the metrics api + * as `aggregated_details`, which is where a text search puts customer names and + * emails, and those have no business sitting in web storage. + * + * Timestamps are compared by day rather than verbatim. With no date filter set, + * the metrics filter defaults to "the last year, ending now", recomputed on + * every mount down to the second — hashing that as-is gives every mount its own + * fingerprint and the trail never survives the trip to a details page, which is + * the one thing it exists for. An end that moved by seconds does not + * meaningfully change which records are in the set, and the boundary drift that + * comes with it is the same one cursor pagination has anyway. A range the user + * actually changed resets the trail through `isQueryChanged`, well before this + * fingerprint is consulted. + * + * Key order out of `JSON.stringify` is insertion order, stable here because the + * same code builds the query every time. Were it ever to vary, the fingerprint + * would stop matching and the trail would be dropped — the safe way to fail. + */ +function queryIdentity(metricsQuery: unknown): string { + return JSON.stringify(metricsQuery ?? null).replace( + /\d{4}-\d{2}-\d{2}T[\d:.]+Z?/g, + (timestamp) => timestamp.slice(0, 10), + ) +} + +/** + * 64 bits of FNV-1a-style hashing, as two 32-bit halves in base 36. + * + * Not a cryptographic hash and not meant to be: it only has to make an + * accidental match between two different queries implausible, and it exists so + * that the query itself never has to be written down. + */ +function fingerprintOf(value: string): string { + let hashA = 0x811c9dc5 + let hashB = 0x01000193 + + for (let index = 0; index < value.length; index++) { + const charCode = value.charCodeAt(index) + hashA = Math.imul(hashA ^ charCode, 0x01000193) + hashB = Math.imul(hashB ^ charCode, 0x85ebca6b) + } + + return `${(hashA >>> 0).toString(36)}${(hashB >>> 0).toString(36)}` +} diff --git a/packages/app-elements/src/ui/resources/useResourceList/usePageInUrl.ts b/packages/app-elements/src/ui/resources/useResourceList/usePageInUrl.ts new file mode 100644 index 000000000..dce9fd907 --- /dev/null +++ b/packages/app-elements/src/ui/resources/useResourceList/usePageInUrl.ts @@ -0,0 +1,94 @@ +import { useCallback } from "react" +import { useSearch } from "wouter/use-browser-location" + +const pageParam = "page" + +export interface UsePageInUrlReturn { + /** + * The page the url is asking for. `1` when the parameter is absent, or when + * its value is not a positive integer. + */ + requestedPage: number + /** + * Write `page` to the url as a new history entry, so the browser's back + * button walks back through the pages the user visited. + */ + pushPage: (page: number) => void + /** + * Write `page` to the url in place, without a history entry. Use it to + * correct a page the list could not open: pushing would leave the unservable + * page in history, so going back would return to it and correct it again, + * trapping the user on the pager. + */ + replacePage: (page: number) => void +} + +/** + * Keeps the current page of a paginated list in the query string. + * + * This is what makes the page survive leaving the list and coming back, either + * with the browser's back button or with `goBack` from `useAppLinking`, which + * restores the whole url including its query string. + * + * Page 1 carries no parameter: it is the default, and a bare url is the one + * worth sharing. + * + * The parameter is not namespaced, so two paginated lists rendered on the same + * route would share it. No page does that today. + */ +export function usePageInUrl(): UsePageInUrlReturn { + const search = useSearch() + const requestedPage = parsePage(search) + + // Neither setter closes over `search`: they read the live url when called, so + // their identity stays stable across navigations and effects depending on + // them do not re-run on every url change. + const writePage = useCallback((page: number, mode: "push" | "replace") => { + const url = new URL(window.location.href) + + if (page <= 1) { + url.searchParams.delete(pageParam) + } else { + url.searchParams.set(pageParam, String(page)) + } + + // writing the url it already has would add a history entry for nothing, and + // wake up every wouter subscriber to announce that nothing changed + if (url.href === window.location.href) { + return + } + + window.history[mode === "push" ? "pushState" : "replaceState"]( + {}, + "", + url.href, + ) + }, []) + + const pushPage = useCallback( + (page: number) => { + writePage(page, "push") + }, + [writePage], + ) + + const replacePage = useCallback( + (page: number) => { + writePage(page, "replace") + }, + [writePage], + ) + + return { requestedPage, pushPage, replacePage } +} + +function parsePage(search: string): number { + const raw = new URLSearchParams(search).get(pageParam) + + if (raw == null) { + return 1 + } + + const parsed = Number(raw) + return Number.isInteger(parsed) && parsed >= 1 ? parsed : 1 +} diff --git a/packages/app-elements/src/ui/resources/useResourceList/useResourceList.test.tsx b/packages/app-elements/src/ui/resources/useResourceList/useResourceList.test.tsx index 4c2ad476e..4018160f8 100644 --- a/packages/app-elements/src/ui/resources/useResourceList/useResourceList.test.tsx +++ b/packages/app-elements/src/ui/resources/useResourceList/useResourceList.test.tsx @@ -1,8 +1,10 @@ import type { Order } from "@commercelayer/sdk" import { render, waitFor } from "@testing-library/react" -import { act, type FC } from "react" +import { HttpResponse, http } from "msw" +import { act, type FC, type JSX } from "react" import { CoreSdkProvider } from "#providers/CoreSdkProvider" import { MockTokenProvider as TokenProvider } from "#providers/TokenProvider/MockTokenProvider" +import { server } from "../../../mocks/server" import { type UseResourceListConfig, useResourceList } from "./useResourceList" const mockedOrder: Order = { @@ -149,7 +151,373 @@ describe("useResourceList", () => { }) }) +describe("useResourceList - the page in the url", () => { + const renderList = (): ReturnType => + render( + + + + + , + ) + + const currentPageParam = (): string | null => + new URLSearchParams(window.location.search).get("page") + + beforeEach(() => { + window.history.pushState({}, "", "/") + }) + + test("Should write the page to the url, and drop the parameter on page 1", async () => { + const { findByRole, findAllByTestId } = renderList() + await findAllByTestId("orderItem-ready") + expect(currentPageParam()).toBeNull() + + await act(async () => { + ;(await findByRole("button", { name: "Next page" })).click() + }) + await waitFor(() => { + expect(currentPageParam()).toBe("2") + }) + + await act(async () => { + ;(await findByRole("button", { name: "Previous page" })).click() + }) + await waitFor(() => { + expect(currentPageParam()).toBeNull() + }) + }) + + test("Should open the page the url asks for, on a core api list", async () => { + window.history.pushState({}, "", "/?page=2") + const { findAllByTestId } = renderList() + + await waitFor(async () => { + const items = await findAllByTestId("orderItem-ready") + expect(items[0]?.dataset.page).toBe("page2") + }) + // the request was honoured, so the url keeps it + expect(currentPageParam()).toBe("2") + }) + + test("Should walk the pages with the browser back button", async () => { + const { findByRole, findAllByTestId } = renderList() + await findAllByTestId("orderItem-ready") + + await act(async () => { + ;(await findByRole("button", { name: "Next page" })).click() + }) + await waitFor(() => { + expect(currentPageParam()).toBe("2") + }) + + await act(async () => { + window.history.back() + }) + await waitFor(async () => { + expect(currentPageParam()).toBeNull() + const items = await findAllByTestId("orderItem-ready") + expect(items[0]?.dataset.page).toBe("page1") + }) + }) + + test("Should open a restored page with a single request", async () => { + // `initialFetch` reads the page from the url, so a list restored on page 2 + // must not fetch page 1 first and then correct itself + const listRequests: string[] = [] + const countRequest = ({ request }: { request: Request }): void => { + const url = new URL(request.url) + if (url.pathname.endsWith("/orders")) { + listRequests.push(url.searchParams.get("page[number]") ?? "1") + } + } + server.events.on("request:start", countRequest) + + try { + window.history.pushState({}, "", "/?page=2") + const { findAllByTestId } = renderList() + + await waitFor(async () => { + const items = await findAllByTestId("orderItem-ready") + expect(items[0]?.dataset.page).toBe("page2") + }) + + expect(listRequests).toStrictEqual(["2"]) + } finally { + server.events.removeListener("request:start", countRequest) + } + }) + + test("Should ignore a page parameter that is not a positive integer", async () => { + window.history.pushState({}, "", "/?page=nope") + const { findAllByTestId } = renderList() + + await waitFor(async () => { + const items = await findAllByTestId("orderItem-ready") + expect(items[0]?.dataset.page).toBe("page1") + }) + }) +}) + +describe("useResourceList - the page in the url, on a metrics-backed list", () => { + const metricsSearchUrl = "https://mock.localhost/metrics/orders/search" + + const MetricsListImplementation: FC<{ status?: string; dateTo?: string }> = ({ + status, + dateTo, + }) => { + const { ResourceList, Pagination } = useResourceList({ + type: "orders", + paginationType: "pagination", + metricsQuery: { + search: { limit: 2 }, + filter: { order: { status, date_to: dateTo } }, + }, + }) + + return ( + <> + ( +
+ Order #{resource.number} +
+ )} + /> + + + ) + } + + /** Records the cursor each search was asked to open. */ + let sentCursors: Array = [] + + const renderMetricsList = ( + props: { status?: string; dateTo?: string } = {}, + ): ReturnType => render(metricsTree(props)) + + const metricsTree = (props: { + status?: string + dateTo?: string + }): JSX.Element => ( + + + + + + ) + + const pageParam = (): string | null => + new URLSearchParams(window.location.search).get("page") + + const storedTrail = (): Array | null => { + for (let index = 0; index < window.sessionStorage.length; index++) { + const key = window.sessionStorage.key(index) + if (key?.startsWith("cl.metrics.trail.") === true) { + return JSON.parse(window.sessionStorage.getItem(key) ?? "{}").cursors + } + } + return null + } + + const goNext = async (view: ReturnType): Promise => { + await act(async () => { + ;(await view.findByRole("button", { name: "Next page" })).click() + }) + } + + const goPrevious = async (view: ReturnType): Promise => { + await act(async () => { + ;(await view.findByRole("button", { name: "Previous page" })).click() + }) + } + + beforeEach(() => { + sentCursors = [] + window.sessionStorage.clear() + window.history.pushState({}, "", "/") + server.use( + http.post(metricsSearchUrl, async ({ request }) => { + const body = (await request.json()) as { + search?: { cursor?: string | null } + } + sentCursors.push(body.search?.cursor ?? null) + + return HttpResponse.json({ + data: [ + { id: "metrics-1", type: "orders", number: 1 }, + { id: "metrics-2", type: "orders", number: 2 }, + ], + meta: { + pagination: { record_count: 6, cursor: "cursor-for-page-2" }, + }, + }) + }), + ) + }) + + test("Should decline a page it holds no cursor for, and correct the url", async () => { + window.history.pushState({}, "", "/?page=3") + const { findAllByTestId, findByRole } = render( + + + + + , + ) + + await findAllByTestId("orderItem-ready") + + // no cursor for page 3 on a fresh mount, so page 1 is the honest answer + expect(sentCursors).toStrictEqual([null]) + await waitFor(() => { + expect(new URLSearchParams(window.location.search).get("page")).toBeNull() + }) + // and the pager agrees it is on the first page + expect(await findByRole("button", { name: "Previous page" })).toBeDisabled() + }) + + test("Should honour a page whose cursor the trail already holds", async () => { + const { findAllByTestId, findByRole } = render( + + + + + , + ) + await findAllByTestId("orderItem-ready") + + await act(async () => { + ;(await findByRole("button", { name: "Next page" })).click() + }) + + await waitFor(() => { + expect(new URLSearchParams(window.location.search).get("page")).toBe("2") + }) + // page 1 opened with no cursor, page 2 with the one page 1 handed back + expect(sentCursors).toStrictEqual([null, "cursor-for-page-2"]) + }) + + test("Should reopen the requested page from the persisted trail, after a remount", async () => { + const first = renderMetricsList() + await first.findAllByTestId("orderItem-ready") + await goNext(first) + await waitFor(() => { + expect(pageParam()).toBe("2") + }) + + // leaving for a details page unmounts the list; the trail must outlive it + first.unmount() + sentCursors = [] + + const second = renderMetricsList() + await second.findAllByTestId("orderItem-ready") + + expect(pageParam()).toBe("2") + expect(sentCursors).toStrictEqual(["cursor-for-page-2"]) + }) + + test("Should not reuse a trail walked against a different query", async () => { + const first = renderMetricsList({ status: "placed" }) + await first.findAllByTestId("orderItem-ready") + await goNext(first) + await waitFor(() => { + expect(pageParam()).toBe("2") + }) + first.unmount() + sentCursors = [] + + // same url, different filters: those cursors mean nothing here + const second = renderMetricsList({ status: "approved" }) + await second.findAllByTestId("orderItem-ready") + + expect(sentCursors).toStrictEqual([null]) + await waitFor(() => { + expect(pageParam()).toBeNull() + }) + }) + + test("Should survive the default date range moving between mounts", async () => { + // With no date filter of its own, the metrics filter defaults to a range + // ending "now", rebuilt on every mount down to the second. The trail has to + // outlive that, or it never survives a trip to a details page. + const first = renderMetricsList({ dateTo: "2026-08-24T10:00:00Z" }) + await first.findAllByTestId("orderItem-ready") + await goNext(first) + await waitFor(() => { + expect(pageParam()).toBe("2") + }) + first.unmount() + sentCursors = [] + + const second = renderMetricsList({ dateTo: "2026-08-24T10:00:37Z" }) + await second.findAllByTestId("orderItem-ready") + + expect(pageParam()).toBe("2") + expect(sentCursors).toStrictEqual(["cursor-for-page-2"]) + }) + + test("Should reset the page, the url and the stored trail when filters or tab change", async () => { + const view = renderMetricsList({ status: "placed" }) + await view.findAllByTestId("orderItem-ready") + await goNext(view) + await waitFor(() => { + expect(pageParam()).toBe("2") + }) + expect(storedTrail()).toHaveLength(3) + + // switching tab or editing a filter reaches the list as a changed query + view.rerender(metricsTree({ status: "approved" })) + + await waitFor(() => { + // back to the first page, with the url no longer asking for the second + expect(pageParam()).toBeNull() + // and the cursors walked against the previous filters are gone + expect(storedTrail()).toStrictEqual([null, "cursor-for-page-2"]) + }) + }) + + test("Should forget the trail when page 1 is reached again", async () => { + const view = renderMetricsList() + await view.findAllByTestId("orderItem-ready") + + await goNext(view) + await waitFor(() => { + expect(pageParam()).toBe("2") + }) + await goNext(view) + await waitFor(() => { + expect(pageParam()).toBe("3") + }) + expect(storedTrail()).toHaveLength(4) + + await goPrevious(view) + await goPrevious(view) + await waitFor(() => { + expect(pageParam()).toBeNull() + }) + + // page 1 is fetched with no cursor, so it re-anchors the list and the + // cursors measured against the older snapshot are dropped + await waitFor(() => { + expect(storedTrail()).toStrictEqual([null, "cursor-for-page-2"]) + }) + }) +}) + describe("useResourceList - pagination mode", () => { + // The page now lives in the query string, and jsdom keeps a single location + // per test file: without this, a test that paged forward leaves `?page=2` + // behind and the next one mounts already restored to page 2. + beforeEach(() => { + window.history.pushState({}, "", "/") + }) + test("Should replace items (not accumulate) when navigating pages", async () => { const { findAllByTestId, findByRole } = render( diff --git a/packages/app-elements/src/ui/resources/useResourceList/useResourceList.tsx b/packages/app-elements/src/ui/resources/useResourceList/useResourceList.tsx index 65699177e..7b90c6fd0 100644 --- a/packages/app-elements/src/ui/resources/useResourceList/useResourceList.tsx +++ b/packages/app-elements/src/ui/resources/useResourceList/useResourceList.tsx @@ -37,6 +37,8 @@ import { useMetricsSdkProvider } from "./metricsApiClient" import { PaginationInfo } from "./PaginationInfo" import { initialState, reducer } from "./reducer" import { subscribeToResourceLists } from "./resourceListSignals" +import { useMetricsCursorTrail } from "./useMetricsCursorTrail" +import { usePageInUrl } from "./usePageInUrl" import { computeTitleWithTotalCount } from "./utils" export interface ResourceListItemTemplateProps< @@ -218,19 +220,52 @@ export function useResourceList({ reducer, initialState, ) - const [currentPage, setCurrentPage] = React.useState(1) + const { requestedPage, pushPage, replacePage } = usePageInUrl() const listRef = React.useRef(null) /** * Metrics API + `pagination` mode only: the cursor that opens each page. * Index 0 is page 1 (no cursor); after loading page N we learn the cursor for * page N+1. The metrics API can only move forward, so remembering the cursors - * we have seen is what makes "previous page" possible. + * we have seen is what makes "previous page" possible — and persisting them + * is what lets the list reopen on the page it was left on. */ - const metricsCursorsRef = React.useRef>([null]) + const metricsTrail = useMetricsCursorTrail({ + enabled: metricsQuery != null && paginationType === "pagination", + type, + metricsQuery, + }) - const resetMetricsCursors = useCallback(() => { - metricsCursorsRef.current = [null] - }, []) + /** + * Turn the page the url asks for into the page that can actually be served. + * + * This is the seam between "what the url wants" and "what the api can do". + * The core api addresses any page directly, so a request always stands. The + * metrics api can only open a page whose cursor is in the trail, and + * answering a request it cannot meet would return the first page of rows + * under the requested page's label — see `listFetcher`, which stamps + * `meta.currentPage` from the requested number. Declining is the only honest + * answer, and page 1 is always servable. + */ + const resolveRequestedPage = useCallback( + (page: number): number => { + if (page <= 1) { + return 1 + } + if (metricsQuery == null) { + return page + } + return metricsTrail.cursorFor(page) != null ? page : 1 + }, + [metricsQuery, metricsTrail], + ) + + // Resolved once, on mount, so that a restored list opens the page the url + // asks for with a single request. A metrics-backed list can answer here too, + // when its trail was persisted by an earlier mount; `followRequestedPage` + // below corrects the url when the request cannot be met after all. + const [currentPage, setCurrentPage] = React.useState(() => + paginationType === "pagination" ? resolveRequestedPage(requestedPage) : 1, + ) // Both queries are watched: for metrics-backed lists `metricsQuery` is the one // that actually drives the request (deep-compared, so inline objects are safe). @@ -238,8 +273,12 @@ export function useResourceList({ value: { query, metricsQuery }, onChange: () => { setCurrentPage(1) - resetMetricsCursors() + // the url must follow, or it would keep asking for a page this list has + // just left — in place, since the filter change already pushed an entry + replacePage(1) dispatch({ type: "reset" }) + // no need to clear the cursor trail here: the page 1 fetch below + // re-anchors it, and a changed query no longer matches its fingerprint void fetchMore({ query, pageNumber: 1 }) }, }) @@ -265,7 +304,7 @@ export function useResourceList({ metricsQuery != null && paginationType === "pagination" && pageNumber != null - ? (metricsCursorsRef.current[pageNumber - 1] ?? null) + ? (metricsTrail.cursorFor(pageNumber) ?? null) : undefined, ...(metricsQuery != null ? { @@ -286,8 +325,18 @@ export function useResourceList({ paginationType === "pagination" && pageNumber != null ) { - metricsCursorsRef.current[pageNumber] = - listResponse.meta.cursor ?? null + // Page 1 is fetched with no cursor, so it re-anchors the list against + // the data as it is now. Everything the trail remembered about later + // pages was measured against an older snapshot and would place their + // boundaries in the wrong spot, so it goes. + // + // This is the *only* place the trail is cleared, and it is enough: + // every path that abandons the current page — a filter or tab change, + // `refresh`, a page the resolver declined — goes on to fetch page 1. + if (pageNumber === 1) { + metricsTrail.reset() + } + metricsTrail.record(pageNumber, listResponse.meta.cursor ?? null) } dispatch({ type: "loaded", payload: listResponse }) } catch (err) { @@ -301,12 +350,53 @@ export function useResourceList({ function initialFetch() { void fetchMore({ query, - pageNumber: paginationType === "pagination" ? 1 : undefined, + // `currentPage` is already the page the url asked for, when that page + // could be served, so a restored list opens it with a single request + pageNumber: paginationType === "pagination" ? currentPage : undefined, }) }, [sdkClient], ) + /** + * The url is the single source of truth for the page: a pager click, the + * browser's back button and a shared link all arrive here the same way. + * + * Deliberately keyed on `requestedPage` alone. `fetchMore` changes identity + * with `data`, so depending on it would refetch on every response; the values + * read here come from the render in which the page changed, which is the + * render whose values are wanted. + */ + useEffect( + function followRequestedPage() { + if (paginationType !== "pagination") { + return + } + + const servablePage = resolveRequestedPage(requestedPage) + + if (servablePage !== requestedPage) { + // correcting the url re-runs this effect with a page that can be served + replacePage(servablePage) + return + } + + if (servablePage === currentPage) { + return + } + + setCurrentPage(servablePage) + void fetchMore({ query, pageNumber: servablePage }).then(() => { + if (paginationScrollTo === "top") { + window.scrollTo({ top: 0 }) + } else if (paginationScrollTo === "list") { + listRef.current?.scrollIntoView() + } + }) + }, + [requestedPage], + ) + const isApiError = data != null && error != null const displayList = useMemo( () => @@ -343,13 +433,15 @@ export function useResourceList({ const refresh = useCallback(() => { setCurrentPage(1) - resetMetricsCursors() + if (paginationType === "pagination") { + replacePage(1) + } dispatch({ type: "reset" }) void fetchMore({ query, pageNumber: paginationType === "pagination" ? 1 : undefined, }) - }, [query, paginationType, fetchMore, resetMetricsCursors]) + }, [query, paginationType, fetchMore, replacePage]) // A component that mutates a resource is often not the one rendering the list: // a details drawer is a sibling of the list, which stays mounted underneath, so @@ -371,18 +463,13 @@ export function useResourceList({ [type], ) + // The pager only writes the url; `followRequestedPage` above does the rest, so + // that clicking Next and pressing the browser's back button take one path. const handlePageChange = useCallback( (newPage: number) => { - setCurrentPage(newPage) - void fetchMore({ query, pageNumber: newPage }).then(() => { - if (paginationScrollTo === "top") { - window.scrollTo({ top: 0 }) - } else if (paginationScrollTo === "list") { - listRef.current?.scrollIntoView() - } - }) + pushPage(newPage) }, - [query, fetchMore, paginationScrollTo], + [pushPage], ) const ResourceList = useCallback>>(