diff --git a/.changeset/listview-shared-datasource-gate-4038.md b/.changeset/listview-shared-datasource-gate-4038.md new file mode 100644 index 0000000000..368a2d19e4 --- /dev/null +++ b/.changeset/listview-shared-datasource-gate-4038.md @@ -0,0 +1,47 @@ +--- +"@object-ui/plugin-list": patch +--- + +`list-view` now reads its `dataSource` binding through the shared `ElementDataSourceGate` instead of a private copy of the precedence table + +objectstack#5576 landed the per-element `dataSource` binding on `list-view` by +writing the precedence table — binding keys override the component's, a `view` is +only a baseline, `filter` AND-combines rather than replaces, an authored-but-empty +`columns` counts as unauthored, the row cap lands on `pagination.pageSize`, and +`viewType` is taken only when the component declared none — inline in +`ListViewBlock`. objectstack#6953 then needed the same table for the other eight +object-bound blocks and lifted it into `@object-ui/react` +(`useElementDataSourceSchema` / `ElementDataSourceGate`), deliberately not +touching `ListViewBlock`: refactoring already-merged code inside a wiring PR +would have been an out-of-scope regression surface. + +That left one table with two implementations — `list-view` on the private copy, +every other block on the shared one. Nothing was wrong for a user today; the risk +is the next person to change the rules changing one side, which is how the spec's +"*additional* filter criteria" becomes two dialects and a per-element filter +quietly starts replacing a saved view's instead of narrowing it. + +`ListViewBlock` now contributes only what is genuinely its own — the names of the +keys `ListView` reads: + +```ts +const LIST_VIEW_DATA_SOURCE: ElementDataSourceMapping = { + columns: true, filter: true, sort: true, + limit: 'pagination.pageSize', viewType: true, +}; +``` + +and the ~45-line `useMemo` mapping block is deleted, along with the block's +hand-rolled error and loading panels (the shared +`ElementDataSourceErrorPanel` / `ElementDataSourceLoadingPanel` render with the +`list-view` testId prefix, so `list-view-datasource-error` and +`list-view-resolving-view` are unchanged down to the byte, and the error heading +is passed through as `errorTitle`). + +**No behaviour changes in either direction**, and that is the acceptance +criterion rather than a hoped-for outcome: objectstack#5576's entire suite passes +untouched, with no assertion edited — had any single case needed adapting, the +two implementations would have been proven to disagree, which is a defect to +re-grade rather than a refactor detail to absorb. New pins cover the mapping +table key by key at the block/gate seam, plus the shared loading panel, which +neither implementation had ever asserted. diff --git a/packages/plugin-list/src/ListViewBlock.tsx b/packages/plugin-list/src/ListViewBlock.tsx index ba1533e881..9c7be7fcbb 100644 --- a/packages/plugin-list/src/ListViewBlock.tsx +++ b/packages/plugin-list/src/ListViewBlock.tsx @@ -6,11 +6,34 @@ * LICENSE file in the root directory of this source tree. */ -import React, { useContext, useMemo } from 'react'; -import { isElementDataSourceConfig, mergeFilterNodes } from '@object-ui/core'; -import { SchemaRendererContext, useElementDataSource } from '@object-ui/react'; +import React, { useContext } from 'react'; +import { isElementDataSourceConfig } from '@object-ui/core'; +import { + ElementDataSourceGate, + SchemaRendererContext, + type ElementDataSourceMapping, +} from '@object-ui/react'; import { ListView, type ListViewHandle, type ListViewProps } from './ListView'; +/** + * Which schema keys `ListView` actually reads, so the composed binding lands on + * those and nothing else. + * + * `list-view` is the block that reads all five: `columns` is a FIELD projection + * here (so a saved view's column list belongs on it), `filter` and `sort` go to + * the query, the row cap is read as `pagination.pageSize`, and `viewType` picks + * which view kind renders — its registry `inputs` enumerate grid/kanban/gallery, + * so naming a saved kanban view and rendering a grid would be a silently wrong + * answer. + */ +const LIST_VIEW_DATA_SOURCE: ElementDataSourceMapping = { + columns: true, + filter: true, + sort: true, + limit: 'pagination.pageSize', + viewType: true, +}; + /** * Registry entry point for `` — two bridges in one component. * @@ -52,125 +75,53 @@ import { ListView, type ListViewHandle, type ListViewProps } from './ListView'; * `SchemaRenderer` no longer spreads the schema's `dataSource` as a prop, which * removes the collision at its source. This component completes the other half: * it maps the binding onto the props `ListView` actually reads, resolving a named - * saved view through {@link useElementDataSource}. - * - * ### Precedence + * saved view through `ElementDataSourceGate`. * - * Two kinds of value arrive, and they do not have the same standing: + * ### Where the precedence table lives (objectui#4038) * - * - **`dataSource.*` keys are authoritative.** The author wrote them on THIS - * placement, and the spec says the binding "overrides page-level object - * context". They beat the component's own same-named key. - * - **View-sourced values are a baseline.** A `view` is a *reference*; a key - * written on the component itself is more specific than the view it points at, - * so the component's key wins. ("view provides the baseline, explicit keys - * override".) - * - **`filter` never overrides — it combines.** The spec describes the - * binding's filter as "*Additional* filter criteria", so component filter, - * view filter and binding filter all AND together through - * `mergeFilterNodes`. A binding can only narrow what the view already - * restricts, never widen it: a mistyped per-element filter cannot expose rows - * the saved view excluded. (`ListView` then ANDs the user's own toolbar - * filters onto this, unchanged.) + * It is NOT here. objectstack#5576 landed this block's own ~45-line copy of + * "binding overrides the component key / a view is only a baseline / `filter` + * AND-combines / an empty `columns` counts as unauthored / the row cap lands on + * `pagination.pageSize` / `viewType` only when undeclared", and objectstack#6953 + * then lifted that same table into `@object-ui/react` when the other eight + * object-bound blocks needed it. Two copies of one table is how the spec's + * "*additional* filter criteria" quietly becomes two dialects — the next person + * to change the rules changes one side. So the table now lives ONCE, in + * {@link ElementDataSourceGate}, and this block contributes only the part that is + * genuinely its own: {@link LIST_VIEW_DATA_SOURCE}, the names of the keys it + * reads. The behaviour is unchanged in both directions — that objectstack#5576's + * whole suite passes here untouched is the acceptance criterion of the + * convergence, not a happy accident. * - * An authored-but-EMPTY `columns` counts as "not authored": `[]` is what the - * designer emits for an unconfigured column list, and supplying the columns is - * exactly why a view was named. Any non-empty `columns` on the component wins. - * - * ### An unresolvable `view` fails loudly - * - * When the named view does not exist we render a configuration error instead of - * falling back to the object's default view. Silently widening a named view to - * "all records" is the failure class the binding exists to remove, and it is the - * one an AI-authored page hides best: the page looks like it works. Same posture - * (and same shape of panel) as `SchemaRenderer`'s "Unknown component type" — - * authored metadata pointing at something that is not there. + * An unresolvable `view` still fails loudly (the gate renders the configuration + * error rather than falling back to the object's default view): silently widening + * a named view to "all records" is the failure class the binding exists to + * remove, and the one an AI-authored page hides best. */ const ListViewBlock = React.forwardRef((props, ref) => { const context = useContext(SchemaRendererContext as React.Context); // Defence in depth for the collision above: even though SchemaRenderer no // longer spreads it, a host (or an older cached bundle) handing us the spec - // BINDING under this prop name must never be mistaken for an adapter. + // BINDING under this prop name must never be mistaken for an adapter. The gate + // carries the same guard for its own resolution; this one also decides what + // `ListView` itself receives as its adapter, which the gate cannot do for us. const explicitAdapter = isElementDataSourceConfig(props.dataSource) ? undefined : props.dataSource; const adapter = explicitAdapter ?? context?.dataSource; - const binding = useElementDataSource(props.schema, adapter); - - const schema = useMemo(() => { - const composed = binding.composed; - if (!composed) return props.schema; - - const base = props.schema as Record; - const next: Record = { ...base }; - - next.objectName = composed.object; - - const authoredColumns = Array.isArray(base.columns) && base.columns.length > 0 - ? base.columns - : undefined; - if (authoredColumns === undefined && composed.columns !== undefined) { - next.columns = composed.columns; - } - - // Component filter AND (view filter AND binding filter). `composed.filter` - // already carries the latter pair; a single surviving source comes back - // unwrapped, so the common "only the view filters" case stays flat. - const filter = mergeFilterNodes(base.filter, composed.filter); - if (filter !== undefined) next.filter = filter; - else delete next.filter; - - // `composed.sort` is the binding's sort when it declared one, else the - // view's — so the component's own `sort` may only win over the latter. - if (composed.sort !== undefined) { - const viewSuppliedSort = binding.config?.sort === undefined; - if (!viewSuppliedSort || base.sort === undefined) next.sort = composed.sort; - } - - if (composed.limit !== undefined) { - const viewSuppliedLimit = binding.config?.limit === undefined; - const authoredPageSize = base.pagination?.pageSize; - if (!viewSuppliedLimit || authoredPageSize === undefined) { - next.pagination = { ...(base.pagination ?? {}), pageSize: composed.limit }; - } - } - - if (composed.viewType !== undefined && base.viewType === undefined) { - next.viewType = composed.viewType; - } - - return next as ListViewProps['schema']; - }, [props.schema, binding.composed, binding.config]); - - if (binding.status === 'missing') { - return ( -
-

This list view’s data source could not be resolved

-

{binding.error}

-
- ); - } - - if (binding.status === 'loading') { - return ( -
- Loading view… -
- ); - } - - return ; + return ( + + {(schema) => } + + ); }); ListViewBlock.displayName = 'ListViewBlock'; diff --git a/packages/plugin-list/src/__tests__/ListView.sharedGate.test.tsx b/packages/plugin-list/src/__tests__/ListView.sharedGate.test.tsx new file mode 100644 index 0000000000..7df5133b7a --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.sharedGate.test.tsx @@ -0,0 +1,306 @@ +/** + * 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. + * + * `list-view` reads its per-element `dataSource` binding through the SHARED + * `ElementDataSourceGate` (objectui#4038). + * + * objectstack#5576 wrote the precedence table INSIDE `ListViewBlock`; + * objectstack#6953 lifted the same table into `@object-ui/react` when the other + * eight object-bound blocks needed it, leaving two copies of one table. Two + * copies is how the next person to change the rules changes one side, and the + * spec's "*additional* filter criteria" quietly becomes two dialects. + * objectui#4038 deleted the private copy; what `ListViewBlock` still owns is the + * five KEY NAMES it reads, and that is what this file pins. + * + * ## What each half pins — and what can actually turn it red + * + * The convergence is a refactor between two implementations that AGREED, so + * objectstack#5576's suite passing untouched is its acceptance criterion + * (`ListView.elementDataSource.test.tsx` — not one assertion edited). It follows + * that the behavioural pins below cannot, even in principle, tell the private + * copy from the shared gate: nothing can, that is the premise. What they guard is + * the part the refactor left behind as a five-line constant, where a mistake is + * now cheap to make and invisible to review — drop `viewType`, spell the row cap + * `limit` instead of `pagination.pageSize`, or stop mapping `columns`, and they + * go red naming the key that moved. + * + * The two `seam` tests are the other half, and the only ones that go red if the + * block re-forks its own copy: they pin that the mapping is handed to the shared + * gate at all. Together: a re-fork fails fast, and a mistyped mapping fails + * precisely. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, waitFor } from '@testing-library/react'; +import * as React from 'react'; +import { SchemaRendererProvider } from '@object-ui/react'; +import { ListViewBlock } from '../ListViewBlock'; + +const h = vi.hoisted(() => ({ captured: null as any, gate: [] as any[] })); + +// Capture the BOUND schema `ListView` actually receives — the question asked of +// the block, not merely that a prop was threaded somewhere. Same idiom as +// `plugin-detail/src/__tests__/RecordRelatedListRenderer.elementDataSource.test.tsx`. +vi.mock('../ListView', () => ({ + ListView: (props: any) => { + h.captured = props; + return
; + }, +})); + +// Partial mock — house idiom (`plugin-kanban/src/registration.test.tsx`): keep +// every real export and wrap ONLY the gate, so the REAL gate still does all the +// work while this file records what the block hands it. A whole-module +// replacement would hand back a second `SchemaRendererContext` instance and the +// provider below would stop reaching the block at all. +vi.mock(import('@object-ui/react'), async (importOriginal) => { + const actual = await importOriginal(); + const RealGate = actual.ElementDataSourceGate; + return { + ...actual, + ElementDataSourceGate: ((props: any) => { + h.gate.push(props); + return ; + }) as typeof actual.ElementDataSourceGate, + }; +}); + +/** A saved view that supplies every key the mapping can route. */ +const HOT_VIEW = { + name: 'hot', + label: 'Hot accounts', + // The view's render KIND (the spec's `type`), not a field list. + type: 'kanban', + columns: ['name', 'rating'], + filter: [['rating', '=', 'hot']], + sort: [{ field: 'name', order: 'desc' }], + pagination: { pageSize: 5 }, +}; + +const makeAdapter = (listViews: Record = { hot: HOT_VIEW }) => ({ + find: vi.fn().mockResolvedValue({ data: [], total: 0 }), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue({ name: 'account', fields: [], listViews }), +}); + +function renderBlock(schema: Record, adapter: any = makeAdapter()) { + const utils = render( + + + , + ); + return { ...utils, adapter }; +} + +/** The schema the gate composed and handed down to `ListView`. */ +async function boundSchema(): Promise> { + await waitFor(() => expect(h.captured).toBeTruthy()); + return h.captured.schema as Record; +} + +beforeEach(() => { + h.captured = null; + h.gate.length = 0; +}); + +describe('list-view — the seam: the binding is read through the SHARED gate (objectui#4038)', () => { + it('hands the shared gate exactly the keys ListView reads', async () => { + renderBlock({ type: 'list-view', dataSource: { object: 'account' } }); + await boundSchema(); + + // `list-view` is the block that reads all five. This is the whole of what + // the convergence left in `ListViewBlock` — the ~45-line table itself now + // lives once, in `@object-ui/react`. + expect(h.gate[0].mapping).toEqual({ + columns: true, + filter: true, + sort: true, + limit: 'pagination.pageSize', + viewType: true, + }); + }); + + it('names itself to the shared status panels with objectstack#5576’s testIds', async () => { + const { container } = renderBlock({ + type: 'list-view', + dataSource: { object: 'account', view: 'lukewarm' }, + }); + + await waitFor(() => + expect(container.querySelector('[data-testid="list-view-datasource-error"]')).not.toBeNull(), + ); + // The prefix is what keeps the shared panels byte-compatible with the + // assertions objectstack#5576 already carries. + expect(h.gate[0].testId).toBe('list-view'); + expect(h.gate[0].errorTitle).toBe('This list view’s data source could not be resolved'); + expect(container.textContent).toContain('This list view’s data source could not be resolved'); + }); + + it('gives the gate the same adapter it gives ListView', async () => { + // Both bridges in one assertion: the adapter resolved off the renderer + // context (#3144) is the one the gate resolves saved views with AND the one + // `ListView` queries through. + const adapter = makeAdapter(); + renderBlock({ type: 'list-view', dataSource: { object: 'account' } }, adapter); + await boundSchema(); + + expect(h.gate[0].dataSource).toBe(adapter); + expect(h.captured.dataSource).toBe(adapter); + }); + + it('renders the shared LOADING panel while a named view is still resolving', async () => { + // Untested before objectui#4038 in either implementation: `-resolving-view` + // appeared only in the block's own source. A component that treated "not + // resolved yet" as "does not exist" would flash a configuration error on + // every mount, so the two states get two panels. + const pending = { + ...makeAdapter(), + getObjectSchema: vi.fn(() => new Promise(() => {})), + }; + const { container } = renderBlock( + { type: 'list-view', dataSource: { object: 'account', view: 'hot' } }, + pending, + ); + + const panel = container.querySelector('[data-testid="list-view-resolving-view"]'); + expect(panel).not.toBeNull(); + expect(panel!.getAttribute('role')).toBe('status'); + // The block must not be mounted against a half-resolved query — that is how + // a page shows a wider answer than the one that was authored. + expect(h.captured).toBeNull(); + }); +}); + +describe('list-view — the mapping table, key by key, through the shared gate', () => { + it('routes `object` onto the objectName ListView queries', async () => { + renderBlock({ type: 'list-view', dataSource: { object: 'account' } }); + expect((await boundSchema()).objectName).toBe('account'); + }); + + it('takes the view’s columns when the component authored none', async () => { + renderBlock({ type: 'list-view', dataSource: { object: 'account', view: 'hot' } }); + expect((await boundSchema()).columns).toEqual(['name', 'rating']); + }); + + it('counts an authored-but-EMPTY columns as unauthored', async () => { + // `[]` is what the designer emits for an unconfigured column list, and + // supplying the columns is exactly why a view was named. + renderBlock({ type: 'list-view', columns: [], dataSource: { object: 'account', view: 'hot' } }); + expect((await boundSchema()).columns).toEqual(['name', 'rating']); + }); + + it('lets a non-empty authored columns win over the view’s', async () => { + renderBlock({ + type: 'list-view', + columns: ['name'], + dataSource: { object: 'account', view: 'hot' }, + }); + expect((await boundSchema()).columns).toEqual(['name']); + }); + + it('AND-combines the component’s own filter with the view’s and the binding’s', async () => { + // "Additional filter criteria" taken literally: all three sources stay in + // force, so a per-element filter can only narrow what the saved view already + // restricts. `and` is associative, so the nesting below is the composer's + // output shape rather than a semantic claim — it is pinned because a change + // that DROPS a source would show up here first. + renderBlock({ + type: 'list-view', + filter: [['stage', '=', 'won']], + dataSource: { object: 'account', view: 'hot', filter: { owner: 'me' } }, + }); + + expect((await boundSchema()).filter).toEqual([ + 'and', + [['stage', '=', 'won']], + ['and', [['rating', '=', 'hot']], ['owner', '=', 'me']], + ]); + }); + + it('lets the component’s own sort win over one the VIEW supplied', async () => { + // A view is a reference; a key written on the component is more specific. + renderBlock({ + type: 'list-view', + sort: [{ field: 'created', order: 'asc' }], + dataSource: { object: 'account', view: 'hot' }, + }); + expect((await boundSchema()).sort).toEqual([{ field: 'created', order: 'asc' }]); + }); + + it('lets the BINDING’s own sort win over the component’s', async () => { + // …but a sort authored on this placement is authoritative over both. + renderBlock({ + type: 'list-view', + sort: [{ field: 'created', order: 'asc' }], + dataSource: { object: 'account', view: 'hot', sort: [{ field: 'rating', order: 'asc' }] }, + }); + expect((await boundSchema()).sort).toEqual([{ field: 'rating', order: 'asc' }]); + }); + + it('lands the row cap on pagination.pageSize, keeping the other pagination keys', async () => { + renderBlock({ + type: 'list-view', + pagination: { mode: 'server' }, + dataSource: { object: 'account', limit: 3 }, + }); + + const schema = await boundSchema(); + expect(schema.pagination).toEqual({ mode: 'server', pageSize: 3 }); + // The mapping names only keys this block READS. A cap parked on a flat + // `limit`/`pageSize` would be accepted and dropped — the very defect the + // wiring removes, one layer further in. + expect(schema.limit).toBeUndefined(); + expect(schema.pageSize).toBeUndefined(); + }); + + it('lets an authored pagination.pageSize win over a cap the VIEW supplied', async () => { + renderBlock({ + type: 'list-view', + pagination: { pageSize: 25 }, + dataSource: { object: 'account', view: 'hot' }, + }); + expect((await boundSchema()).pagination).toEqual({ pageSize: 25 }); + }); + + it('still takes the BINDING’s cap over an authored pagination.pageSize', async () => { + renderBlock({ + type: 'list-view', + pagination: { pageSize: 25 }, + dataSource: { object: 'account', view: 'hot', limit: 3 }, + }); + expect((await boundSchema()).pagination).toEqual({ pageSize: 3 }); + }); + + it('takes the view’s KIND only when the component declared no viewType', async () => { + // `list-view` is the only container that renders several kinds off + // `schema.viewType`, so naming a saved kanban view and rendering a grid + // would be a silently wrong answer. + renderBlock({ type: 'list-view', dataSource: { object: 'account', view: 'hot' } }); + expect((await boundSchema()).viewType).toBe('kanban'); + }); + + it('leaves an authored viewType alone', async () => { + renderBlock({ + type: 'list-view', + viewType: 'gallery', + dataSource: { object: 'account', view: 'hot' }, + }); + expect((await boundSchema()).viewType).toBe('gallery'); + }); + + it('passes a schema with NO binding through BY REFERENCE', async () => { + // Identity, not equality: a new schema object on every render would remount + // `ListView` and refetch. The unbound path must stay exactly as it was. + const authored = { type: 'list-view', objectName: 'account', columns: ['name'] }; + renderBlock(authored); + await waitFor(() => expect(h.captured).toBeTruthy()); + expect(h.captured.schema).toBe(authored); + }); +}); diff --git a/packages/react/src/element-data-source/ElementDataSourceGate.tsx b/packages/react/src/element-data-source/ElementDataSourceGate.tsx index 406adf4d81..b9f43a87e9 100644 --- a/packages/react/src/element-data-source/ElementDataSourceGate.tsx +++ b/packages/react/src/element-data-source/ElementDataSourceGate.tsx @@ -22,10 +22,12 @@ * halves drift: one block ANDs the filters and the next replaces them, and the * spec's "additional filter criteria" quietly becomes two dialects. * - * `plugin-list`'s `ListViewBlock` predates this module and still carries its own - * copy of the table (they agree today — objectstack#5576's whole suite passes - * against this one unchanged). Collapsing it onto this module is objectstack#7120; - * until then, a change to the rules below belongs in both places. + * `plugin-list`'s `ListViewBlock` carried the ORIGINAL copy of the table + * (objectstack#5576, which predates this module) and was collapsed onto this one + * by objectui#4038 — on the acceptance criterion that objectstack#5576's whole + * suite passes against this implementation untouched, which it does. Every + * object-bound block now reads the rules below from here, so this is the one + * place a change to them belongs. * * ## Precedence — one table, applied everywhere *