diff --git a/.changeset/11574-grouping-switch-headers.md b/.changeset/11574-grouping-switch-headers.md new file mode 100644 index 0000000000..cf66db7f19 --- /dev/null +++ b/.changeset/11574-grouping-switch-headers.md @@ -0,0 +1,23 @@ +--- +'@object-ui/plugin-grid': patch +--- + +fix(plugin-grid): switching a server-grouped grid's grouping field shows exactly the new field's groups + +Switching a list view's grouping field (Title to Priority, say) used to leave +phantom `(empty)` group headers stuck on "Loading grid…" beside the real ones +until the page was reloaded, while the server answered only the real groups. +The grid kept the previous field's group headers while it asked for the new +field's, and read them under the new field's name: a header row carries only +the field it was grouped by, so every old header became an `(empty)` group on +the same key, and those duplicates outlived the real answer. + +The group headers and each group's page of rows are now held against the +question they answer (the grouping fields, the view's filter and search, and +the header's summary columns), and an answer is only ever read under that same +question. A change of grouping field, filter or search therefore shows the +grid's loading state until the server's groups for the new question arrive, +rather than the previous groups, and a group's rows still in flight when the +field changed never render under a new group that happens to share its key. A +refresh of the same view (after an edit, say) still keeps the groups and their +rows on screen while it reloads. diff --git a/packages/plugin-grid/src/__tests__/serverGroupingFieldSwitch-11574.test.tsx b/packages/plugin-grid/src/__tests__/serverGroupingFieldSwitch-11574.test.tsx new file mode 100644 index 0000000000..19af075148 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/serverGroupingFieldSwitch-11574.test.tsx @@ -0,0 +1,275 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#11574 — switching a server-grouped grid's grouping field paints + * exactly the server's groups for the NEW field, at every step. + * + * The defect, as the card measured it in the Console: Title (10 groups) → + * Priority showed 13 headers (9 phantom `(empty)` ones stuck on "Loading + * grid…" beside the 4 real ones), → Status 8 (3 phantom + 5), → Priority 8 + * (4 phantom + 4), while the server's grouped query answered 4, 5, 4. A + * reload cleared them. + * + * Triage ruling (comment 5973408350): the header state is keyed by the + * grouping field and its query, so a field change discards the previous + * field's headers and their pending row queries. Pinned here: the + * Title → Priority → Status → Priority sequence, and an in-flight row query + * for a discarded header that never renders. + * + * The double answers `queryGroupHeaders` by bucketing every stored row under + * the compiled query's own `groupBy`, and `find` by applying the compiled row + * query's `$filter` / `$top` / `$skip` — the two faces + * `serverGrouping-7189.test.tsx` pins the grid consuming. A query can be HELD + * by the test, so an answer lands exactly when the pin needs it to. + */ +import { describe, it, expect, vi, afterEach, beforeAll } from 'vitest'; +import { render, cleanup, within, act } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; + +import { ObjectGrid } from '../ObjectGrid'; +import { ActionProvider } from '@object-ui/react'; +import type { DataSource, ObjectGridSchema } from '@object-ui/types'; +import { registerAllFields } from '@object-ui/fields'; + +beforeAll(() => { + registerAllFields(); +}); + +const OBJECT = 'showcase_task'; + +const OBJECT_FIELDS = { + id: { type: 'text', label: 'Id' }, + title: { type: 'text', label: 'Title' }, + priority: { type: 'text', label: 'Priority' }, + status: { type: 'text', label: 'Status' }, + impact: { type: 'text', label: 'Impact' }, +}; + +interface Task { [field: string]: string; id: string; title: string; priority: string; status: string; impact: string } + +/** The card's shape: 10 titles, 4 priorities, 5 statuses. */ +const TASKS: Task[] = [ + { id: 't01', title: 'Draft the brief', priority: 'high', status: 'todo', impact: 'low' }, + { id: 't02', title: 'Book the venue', priority: 'low', status: 'todo', impact: 'high' }, + { id: 't03', title: 'Review the budget', priority: 'medium', status: 'in_progress', impact: 'medium' }, + { id: 't04', title: 'Hire the caterer', priority: 'urgent', status: 'in_progress', impact: 'high' }, + { id: 't05', title: 'Print the badges', priority: 'low', status: 'review', impact: 'low' }, + { id: 't06', title: 'Test the stream', priority: 'high', status: 'review', impact: 'medium' }, + { id: 't07', title: 'Send the invites', priority: 'medium', status: 'done', impact: 'high' }, + { id: 't08', title: 'Order the swag', priority: 'urgent', status: 'done', impact: 'low' }, + { id: 't09', title: 'Brief the speakers', priority: 'high', status: 'blocked', impact: 'high' }, + { id: 't10', title: 'Close the books', priority: 'medium', status: 'blocked', impact: 'medium' }, +]; + +/** The `FilterCondition` subset the compiled queries use: `$and`, `$eq`, `$null`, equality. */ +function matches(row: Record, cond: unknown): boolean { + if (cond === undefined || cond === null) return true; + return Object.entries(cond as Record).every(([key, value]) => { + if (key === '$and') return (value as unknown[]).every((c) => matches(row, c)); + if (value && typeof value === 'object' && !Array.isArray(value)) { + const op = value as Record; + if ('$eq' in op) return row[key] === op.$eq; + if ('$null' in op) return op.$null ? row[key] == null : row[key] != null; + throw new Error(`double: unsupported operator ${JSON.stringify(op)}`); + } + return row[key] === value; + }); +} + +/** The group predicates of one row query: `[field, value]`, `null` for `$null`. */ +function groupPredicatesOf(filter: unknown): Array<[string, unknown]> { + const and = (filter as { $and?: unknown[] } | undefined)?.$and ?? []; + return and.flatMap((c) => Object.entries(c as Record).map(([field, op]) => { + const o = op as Record; + return [field, o && typeof o === 'object' && '$null' in o ? null : (o?.$eq ?? op)] as [string, unknown]; + })); +} + +interface Gate { what: string; open: () => void } + +/** + * A data source serving both compiled queries over the whole store. A query + * the `hold` predicates accept waits on a gate the test opens. + */ +const makeServerDataSource = (rows: Task[]) => { + const gates: Gate[] = []; + const hold: { + headers?: (groupBy: string[]) => boolean; + rows?: (predicates: Array<[string, unknown]>) => boolean; + } = {}; + const wait = (what: string) => new Promise((resolve) => { gates.push({ what, open: resolve }); }); + const ds = { + gates, + hold, + queryGroupHeaders: vi.fn(async (_object: string, query: { where?: unknown; groupBy?: string[] }) => { + const groupBy = query.groupBy ?? []; + if (hold.headers?.(groupBy)) await wait(`headers ${groupBy.join(',')}`); + const buckets = new Map(); + for (const row of rows.filter((r) => matches(r, query.where))) { + const key = JSON.stringify(groupBy.map((f) => row[f] ?? null)); + buckets.set(key, (buckets.get(key) ?? 0) + 1); + } + return [...buckets.entries()].map(([key, count]) => { + const values = JSON.parse(key) as unknown[]; + return { ...Object.fromEntries(groupBy.map((f, i) => [f, values[i]])), count }; + }); + }), + find: vi.fn(async (_object: string, params: Record) => { + const predicates = groupPredicatesOf(params.$filter); + if (hold.rows?.(predicates)) await wait(`rows ${JSON.stringify(predicates)}`); + const matching = rows.filter((r) => matches(r, params.$filter)); + const skip = (params.$skip as number | undefined) ?? 0; + const top = (params.$top as number | undefined) ?? matching.length; + return { data: matching.slice(skip, skip + top), total: matching.length }; + }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn(async () => ({ name: OBJECT, fields: OBJECT_FIELDS })), + }; + return ds; +}; +type ServerDataSource = ReturnType; + +/** Open the one held gate whose description starts with `what`. */ +async function openGate(ds: ServerDataSource, what: string) { + const i = ds.gates.findIndex((g) => g.what.startsWith(what)); + if (i < 0) throw new Error(`no held query "${what}"; held: ${ds.gates.map((g) => g.what).join(' | ')}`); + const [gate] = ds.gates.splice(i, 1); + await act(async () => { gate.open(); }); +} + +/** The server's group set for one field: its distinct values, as labels. */ +const serverGroupsOf = (field: string) => [...new Set(TASKS.map((t) => String(t[field])))].sort(); + +const grid = (ds: ServerDataSource, field: string) => ( + + + +); + +const groupRows = () => [...document.querySelectorAll('[data-testid^="group-row-"]')]; +const headerLabels = () => groupRows().map((r) => r.querySelector('.group-label')?.textContent ?? ''); +const groupRowEl = (label: string) => groupRows().find((r) => r.querySelector('.group-label')?.textContent === label); +const stuckLoading = () => document.querySelectorAll('[data-testid^="group-rows-loading-"]'); + +/** Every header list the grid paints, sampled on each DOM mutation. */ +function recordPaintedHeaders() { + const painted: string[][] = []; + const observer = new MutationObserver(() => { + const labels = headerLabels(); + if (labels.length > 0) painted.push(labels); + }); + observer.observe(document.body, { childList: true, subtree: true, characterData: true }); + return { painted, stop: () => observer.disconnect() }; +} + +/** + * The grid paints exactly the server's groups for `field` — `expected` of + * them, the card's number — and every open group's rows arrive. + */ +async function expectServerGroups(field: string, expected: number) { + const answer = serverGroupsOf(field); + expect(answer).toHaveLength(expected); + await vi.waitFor(() => expect(headerLabels()).toEqual(expect.arrayContaining(answer))); + // Exactly the server's groups: none left over from the previous field. + expect([...headerLabels()].sort()).toEqual(answer); + expect(headerLabels()).not.toContain('(empty)'); + // …and no group is left on "Loading grid…". + await vi.waitFor(() => expect(stuckLoading()).toHaveLength(0)); +} + +afterEach(() => cleanup()); + +describe('switching the grouping field paints the server\'s groups for the new field (objectui#11574)', () => { + it('Title → Priority → Status → Priority: the header count equals the server\'s groups at every step', async () => { + const ds = makeServerDataSource(TASKS); + const recorder = recordPaintedHeaders(); + const view = render(grid(ds, 'title')); + await expectServerGroups('title', 10); + + for (const [field, expected] of [['priority', 4], ['status', 5], ['priority', 4]] as const) { + // The server answers the new field's header query after the grid has + // settled on the switch, as a network does. + ds.hold.headers = (groupBy) => groupBy.includes(field); + view.rerender(grid(ds, field)); + await vi.waitFor(() => expect(ds.gates.map((g) => g.what)).toContain(`headers ${field}`)); + await act(async () => {}); + ds.hold.headers = undefined; + await openGate(ds, `headers ${field}`); + await expectServerGroups(field, expected); + } + recorder.stop(); + + // Never painted on the way, either: every header list the grid showed is + // one field's whole server answer — no phantom `(empty)` header, and no + // previous field's group read under the next field's name. + const answers = (['title', 'priority', 'status'] as const).map((f) => JSON.stringify(serverGroupsOf(f))); + expect(recorder.painted.length).toBeGreaterThan(0); + for (const labels of recorder.painted) { + expect(answers).toContain(JSON.stringify([...labels].sort())); + } + + // No row query was asked for a group the server did not answer: every + // group predicate names a value the server grouped by. + for (const [, params] of ds.find.mock.calls) { + for (const [field, value] of groupPredicatesOf(params.$filter)) { + expect(serverGroupsOf(field)).toContain(value); + } + } + }); + + it('a row query in flight for a discarded header never renders, even under a new group with the same key', async () => { + // `priority` and `impact` share one value set, so the group `high` has + // the same composite key under both fields. + const ds = makeServerDataSource(TASKS); + // Held: the `high` page under either field, and the Impact header query. + ds.hold.rows = (predicates) => predicates.some(([f, v]) => (f === 'priority' || f === 'impact') && v === 'high'); + ds.hold.headers = (groupBy) => groupBy.includes('impact'); + + // Grouped by Priority, the `priority = high` page is in flight. + const view = render(grid(ds, 'priority')); + await vi.waitFor(() => expect([...headerLabels()].sort()).toEqual(serverGroupsOf('priority'))); + await vi.waitFor(() => expect(ds.gates.map((g) => g.what)).toContainEqual(expect.stringContaining('"priority","high"'))); + + // Switch to Impact while it is in flight. + view.rerender(grid(ds, 'impact')); + + // The discarded header's rows answer AFTER the switch… + await openGate(ds, 'rows [["priority","high"]]'); + // …then the server answers Impact's groups. + await openGate(ds, 'headers impact'); + await vi.waitFor(() => expect([...headerLabels()].sort()).toEqual(serverGroupsOf('impact'))); + + // Impact's `high` group is still waiting on its own page: it shows + // "Loading grid…", and none of Priority-high's rows. + const priorityHigh = TASKS.filter((t) => t.priority === 'high' && t.impact !== 'high').map((t) => t.title); + const impactHigh = TASKS.filter((t) => t.impact === 'high').map((t) => t.title); + const high = () => within(groupRowEl('high') as HTMLElement); + for (const title of priorityHigh) expect(high().queryByText(title)).toBeNull(); + expect(groupRowEl('high')!.querySelector('[data-testid^="group-rows-loading-"]')).not.toBeNull(); + + // Its own page lands: exactly Impact-high's rows. + await vi.waitFor(() => expect(ds.gates.map((g) => g.what)).toContainEqual(expect.stringContaining('"impact","high"'))); + await openGate(ds, 'rows [["impact","high"]]'); + for (const title of impactHigh) await vi.waitFor(() => expect(high().getByText(title)).toBeInTheDocument()); + for (const title of priorityHigh) expect(high().queryByText(title)).toBeNull(); + }); +}); diff --git a/packages/plugin-grid/src/useServerGrouping.ts b/packages/plugin-grid/src/useServerGrouping.ts index 1d7e9f5d34..42a6886160 100644 --- a/packages/plugin-grid/src/useServerGrouping.ts +++ b/packages/plugin-grid/src/useServerGrouping.ts @@ -57,6 +57,24 @@ * `ObjectChart` applies to a grouped series); a failed resolution keeps the id * rather than failing the grid. Ordering the groups is the consumer's too: * `useGroupedData` applies `GroupingField.order` over the header set. + * + * ## An answer is held against the question it answers (objectui#11574) + * + * Both hooks record what they hold WITH the question it answers — the object, + * the grouping fields, and the query they compile with (the composed filter, + * the search pair and, for the headers, the summary columns) — and read a + * held answer only under that same question, at once, in the render the + * question changes. A header row carries only the fields it was grouped by, + * so the previous field's header set read under the next field's name is + * every row keyed `null`: one `(empty)` group per old header, all on the same + * composite key, which React cannot reconcile (the stale duplicates outlive + * the real answer as phantom headers stuck on "Loading grid…"). A group row + * page is held by composite key, and two fields can share a key (`0:high` + * under `priority` and under `impact`), so the previous field's page — one + * answered after the switch included — would render under the next field's + * group. A refresh (`reloadKey`) re-asks the SAME question, so it keeps the + * answer in hand on screen while it reloads; any other change starts with + * none. */ import { useCallback, useEffect, useState } from 'react'; @@ -133,6 +151,17 @@ export interface ServerGroupHeaders { const IDLE: ServerGroupHeaders = { headers: undefined, keyLabels: {}, loading: false, error: null }; +/** Asked, with no answer to THIS question in hand yet. */ +const PENDING: ServerGroupHeaders = { headers: undefined, keyLabels: IDLE.keyLabels, loading: true, error: null }; + +/** The header state, recorded against the question it answers. */ +interface HeldHeaders extends ServerGroupHeaders { + /** The `question` this state answers; `''` when idle. */ + question: string; +} + +const IDLE_HELD: HeldHeaders = { ...IDLE, question: '' }; + /** * The search pair of a group row query, as the header query takes it. * @@ -159,7 +188,7 @@ export function groupSearchOf( */ export function useServerGroupHeaders(input: ServerGroupHeadersInput): ServerGroupHeaders { const { enabled, dataSource, objectName, fields, where, search, searchFields, aggregations, objectFields, reloadKey } = input; - const [state, setState] = useState(IDLE); + const [state, setState] = useState(IDLE_HELD); // Keyed on CONTENT, never on the identity of an object a host may rebuild // every render (AGENTS.md #10). @@ -180,16 +209,25 @@ export function useServerGroupHeaders(input: ServerGroupHeadersInput): ServerGro }), ); const canAsk = enabled && !!objectName && typeof dataSource?.queryGroupHeaders === 'function' && fields.length > 0; + // What the header set answers: the grouping fields and the header query + // they compile with. `reloadKey` is not in it — a refresh asks the same + // question again — and neither is `referenceKey`, which labels the keys of + // the same answer. + const question = JSON.stringify([objectName ?? null, fieldsKey, whereKey, searchKey, aggregationsKey]); useEffect(() => { if (!canAsk || !dataSource || !objectName) { // Leaving server grouping drops the last answer rather than keeping a // group set that no longer describes the view. - setState((prev) => (prev === IDLE ? prev : IDLE)); + setState((prev) => (prev === IDLE_HELD ? prev : IDLE_HELD)); return; } let cancelled = false; - setState((prev) => ({ ...prev, loading: true, error: null })); + // Re-asking the question in hand keeps its answer on screen while it + // reloads; a different question starts with none (objectui#11574). + setState((prev) => (prev.question === question + ? { ...prev, loading: true, error: null } + : { ...PENDING, question })); const fieldNames: string[] = JSON.parse(fieldsKey); const references: Array<[string, string | null] | null> = JSON.parse(referenceKey); const grouping = groupingOf(fieldNames.map((field) => ({ field }))); @@ -242,18 +280,22 @@ export function useServerGroupHeaders(input: ServerGroupHeadersInput): ServerGro }), ); - if (!cancelled) setState({ headers, keyLabels, loading: false, error: null }); + if (!cancelled) setState({ question, headers, keyLabels, loading: false, error: null }); } catch (err) { if (!cancelled) { - setState({ headers: undefined, keyLabels: {}, loading: false, error: err instanceof Error ? err : new Error(String(err)) }); + setState({ question, headers: undefined, keyLabels: {}, loading: false, error: err instanceof Error ? err : new Error(String(err)) }); } } })(); return () => { cancelled = true; }; - }, [canAsk, dataSource, objectName, fieldsKey, whereKey, searchKey, aggregationsKey, referenceKey, reloadKey]); + }, [canAsk, dataSource, objectName, question, fieldsKey, whereKey, searchKey, aggregationsKey, referenceKey, reloadKey]); - return state; + // Read in the render the question changes, not one render late: the + // state still holds the previous question's answer until the effect + // above has run, and that answer is never this question's. + if (!canAsk) return IDLE; + return state.question === question ? state : PENDING; } /** One leaf group whose rows the grid wants on screen. */ @@ -302,19 +344,35 @@ interface HeldPage extends ServerGroupRowsPage { signature: string; } +/** The pages held, recorded against the question they answer. */ +interface HeldPages { + question: string; + pages: Record; +} + +const NO_PAGES: Readonly> = {}; + /** * Page the rows INSIDE each visible, expanded leaf group — one compiled row * query per group, `limit` / `offset` per group. */ export function useServerGroupRows(input: ServerGroupRowsInput): ServerGroupRows { const { enabled, dataSource, objectName, fields, where, baseParams, pageSize, leaves, reloadKey } = input; - const [held, setHeld] = useState>({}); const fieldsKey = JSON.stringify(fields.map((f) => f.field)); const whereKey = JSON.stringify(where ?? null); const paramsKey = JSON.stringify(baseParams ?? null); const leavesKey = JSON.stringify(leaves.map((l) => [l.key, l.keyValues])); const queryKey = JSON.stringify([fieldsKey, whereKey, paramsKey, pageSize, reloadKey]); + // The question the group set answers, as far as a group's rows share it: + // the grouping fields, the composed filter and the search pair. A group's + // composite key means a different group under a different question (two + // fields can share one), so a page held under one is never read under + // another (objectui#11574). Order, projection, page size and a refresh are + // the same groups asked again: the page in hand stays on screen meanwhile. + const question = JSON.stringify([objectName ?? null, fieldsKey, whereKey, groupSearchOf(baseParams)]); + const [held, setHeld] = useState({ question, pages: {} }); + const heldPages = held.question === question ? held.pages : NO_PAGES; // The page each group is on, recorded AGAINST the question it was turned // under: a different question (filter, sort, page size, a refresh) makes @@ -339,12 +397,16 @@ export function useServerGroupRows(input: ServerGroupRowsInput): ServerGroupRows // Already asked (answered or in flight) for exactly the page on screen. // A response to any OTHER request for this group is dropped on arrival // by the same signature, so paging back and forth never shows a stale - // page under the pager's number. - if (held[key]?.signature === signature) continue; - setHeld((prev) => ({ - ...prev, - [key]: { rows: prev[key]?.rows ?? [], page, loading: true, error: null, signature }, - })); + // page under the pager's number — and one asked under another question + // by the question too, so it never lands in this question's pages. + if (heldPages[key]?.signature === signature) continue; + setHeld((prev) => { + const pages = prev.question === question ? prev.pages : {}; + return { + question, + pages: { ...pages, [key]: { rows: pages[key]?.rows ?? [], page, loading: true, error: null, signature } }, + }; + }); const compiled = compileListViewGroupRowsQuery({ grouping }, keyValues, { ...(composedWhere ? { where: composedWhere } : {}), @@ -364,23 +426,24 @@ export function useServerGroupRows(input: ServerGroupRowsInput): ServerGroupRows $skip: compiled.offset, }; + const answers = (prev: HeldPages) => prev.question === question && prev.pages[key]?.signature === signature; dataSource .find(objectName, params as QueryParams) .then((result) => { - setHeld((prev) => (prev[key]?.signature === signature - ? { ...prev, [key]: { rows: result?.data ?? [], page, loading: false, error: null, signature } } + setHeld((prev) => (answers(prev) + ? { question, pages: { ...prev.pages, [key]: { rows: result?.data ?? [], page, loading: false, error: null, signature } } } : prev)); }) .catch((err) => { - setHeld((prev) => (prev[key]?.signature === signature - ? { ...prev, [key]: { rows: [], page, loading: false, error: err instanceof Error ? err : new Error(String(err)), signature } } + setHeld((prev) => (answers(prev) + ? { question, pages: { ...prev.pages, [key]: { rows: [], page, loading: false, error: err instanceof Error ? err : new Error(String(err)), signature } } } : prev)); }); } - // `held` is read to skip a request already answered; naming it would + // `heldPages` is read to skip a request already answered; naming it would // re-run this effect on every answer, which asks nothing new. // eslint-disable-next-line react-hooks/exhaustive-deps - }, [canAsk, dataSource, objectName, fieldsKey, whereKey, leavesKey, pagesKey, queryKey, pageSize]); + }, [canAsk, dataSource, objectName, question, fieldsKey, whereKey, leavesKey, pagesKey, queryKey, pageSize]); const setPage = useCallback((key: string, page: number) => { setTurned((prev) => { @@ -391,8 +454,8 @@ export function useServerGroupRows(input: ServerGroupRowsInput): ServerGroupRows }); }, [queryKey]); - // The held STATE itself, not a projection memoised over it: a consumer - // keys an effect on it, and a state value's identity is a promise React - // keeps where a memo's is not (AGENTS.md #10). - return { pages: held, setPage }; + // The held STATE itself (or the one empty constant), not a projection + // memoised over it: a consumer keys an effect on it, and a state value's + // identity is a promise React keeps where a memo's is not (AGENTS.md #10). + return { pages: heldPages, setPage }; }