From 8a6c3dc507c91f02d209a05bf2ef8586f4f38e9e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 09:45:45 +0000 Subject: [PATCH] refactor(list): converge ListViewBlock onto the shared ElementDataSourceGate (#4038) ListViewBlock carried objectstack#5576's original inline copy of the dataSource precedence table; objectstack#6953 lifted the same table into @object-ui/react for the other eight object-bound blocks, leaving one table with two implementations. The private ~45-line useMemo mapping block and the hand-rolled status panels are deleted; the block now contributes only LIST_VIEW_DATA_SOURCE, the key names ListView reads. Semantics are unchanged in both directions: objectstack#5576's suite passes untouched, zero assertion edits, which is the card's acceptance criterion. New seam pins cover the mapping table key by key and the shared loading panel (never asserted by either implementation). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3 --- .../listview-shared-datasource-gate-4038.md | 47 +++ packages/plugin-list/src/ListViewBlock.tsx | 167 ++++------ .../__tests__/ListView.sharedGate.test.tsx | 306 ++++++++++++++++++ .../ElementDataSourceGate.tsx | 10 +- 4 files changed, 418 insertions(+), 112 deletions(-) create mode 100644 .changeset/listview-shared-datasource-gate-4038.md create mode 100644 packages/plugin-list/src/__tests__/ListView.sharedGate.test.tsx 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 *