From a3920c36e4c4a897fb90c798c125d3ace42d872c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 19 Aug 2026 18:25:07 +0000 Subject: [PATCH] fix(plugin-dashboard): resolve column identity at the producer, before data-table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ObjectDataTable.normalizeColumns` returned object columns raw, so a column authored in the spec-canonical spelling (`{ field: 'stage' }`) reached the data-table adapter with no `accessorKey` and rendered a header over `row[undefined]`. `computeLookupExpand` missed the same column for the same reason, dropping `field`-spelled lookups out of `$expand`. Identity is now resolved once, here, via the shared `columnIdentity` reader and stamped onto `accessorKey` — the adapter's own key, which `@object-ui/core` deliberately holds outside the metadata identity fold. Metadata vocabulary in, adapter vocabulary out; one translation, one place (objectui#5022, #5068). An author-supplied `accessorKey` is never overwritten, an already-canonical entry is returned by reference, and an entry with no resolvable identity is returned untouched. Part of #5120 — the consumer half (retiring data-table's undeclared `col.name` alias) is held pending the maintainer's ruling; the census found real authorized usage of that spelling in the published skills guides. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RV6yuVCxymHYE16PL9vQkE --- ...ard-table-producer-column-identity-5120.md | 38 +++ .../plugin-dashboard/src/ObjectDataTable.tsx | 49 +++- .../ObjectDataTable.columnIdentity.test.tsx | 227 ++++++++++++++++++ 3 files changed, 310 insertions(+), 4 deletions(-) create mode 100644 .changeset/dashboard-table-producer-column-identity-5120.md create mode 100644 packages/plugin-dashboard/src/__tests__/ObjectDataTable.columnIdentity.test.tsx diff --git a/.changeset/dashboard-table-producer-column-identity-5120.md b/.changeset/dashboard-table-producer-column-identity-5120.md new file mode 100644 index 0000000000..78e977474c --- /dev/null +++ b/.changeset/dashboard-table-producer-column-identity-5120.md @@ -0,0 +1,38 @@ +--- +"@object-ui/plugin-dashboard": minor +--- + +fix(plugin-dashboard): `ObjectDataTable` resolves column identity before it hands columns to the table + +`normalizeColumns` converted the `string[]` shorthand and returned every object +column **raw**. `data-table` is an adapter, and its column key is `accessorKey` +(`TableColumn.accessorKey`) — a key `@object-ui/core` deliberately holds outside +the metadata identity fold, where `column-identity.ts` names it +`TABLE_ADAPTER_COLUMN_KEY`. So a column authored in the spec-canonical spelling, +`{ field: 'stage' }`, reached the adapter carrying no `accessorKey` at all: the +widget rendered a header over `row[undefined]` — every cell blank, nothing said +— and `computeLookupExpand`'s `$expand` whitelist, which resolved +`c.accessorKey || c.name`, missed the same column, so a `field`-spelled lookup +also lost its related record and showed a raw FK id. + +Identity is now resolved once, here, through the shared `columnIdentity` reader +and stamped onto the adapter's key. This is the move objectui#5022 made in +`RelatedList` and objectui#5068 generalized in `ObjectGrid`: metadata vocabulary +in, adapter vocabulary out, one translation in one place. + +**Affected input.** A column authored `{ field: … }` on an `object-data-table` +now renders its cells and, when the field is relational, enters `$expand`. Both +were previously empty. Columns authored `{ accessorKey: … }` are untouched, by +reference. An author-supplied `accessorKey` is never overwritten — a deliberate +divergence between the table slot and the metadata key belongs to the author — +and an entry whose identity resolves to nothing is returned untouched, so +nothing is invented for it. + +The other half of objectui#5120 — retiring `data-table`'s undeclared `col.name` +alias — is **not** in this change. The card's census-first fork clause tripped: +`skills/objectui/guides/data-integration.md` and +`skills/objectui/guides/schema-expressions.md` both instruct authors to spell a +`data-table` column `{ "name": …, "label": … }`, so the limb has real authorized +usage and the deletion went back to the maintainer. This change is a +prerequisite for that deletion rather than a substitute: it is what stops +`object-data-table` from depending on the alias. diff --git a/packages/plugin-dashboard/src/ObjectDataTable.tsx b/packages/plugin-dashboard/src/ObjectDataTable.tsx index 7865b83121..2280a8d6a1 100644 --- a/packages/plugin-dashboard/src/ObjectDataTable.tsx +++ b/packages/plugin-dashboard/src/ObjectDataTable.tsx @@ -8,7 +8,7 @@ import React, { useState, useEffect, useContext, useMemo, useCallback } from 'react'; import { useDataScope, SchemaRendererContext, SchemaRenderer, useFilterScope } from '@object-ui/react'; -import { extractRecords, isDrillEnabled } from '@object-ui/core'; +import { extractRecords, isDrillEnabled, columnIdentity } from '@object-ui/core'; import type { DrillDownConfig } from '@object-ui/types'; import { Skeleton, RefreshIndicator, cn } from '@object-ui/components'; import { useSafeFieldLabel, useObjectTranslation, useLocalization, useDisplayLocale } from '@object-ui/i18n'; @@ -51,7 +51,38 @@ interface NormalizedColumn { * * - `string[]` entries are converted to `{ header, accessorKey }` objects, * handling both snake_case and camelCase for header generation. - * - Object entries are returned as-is. + * - Object entries have their field identity RESOLVED here, at the producer, + * and stamped onto the data-table adapter's own key. + * + * Object entries used to be returned raw (objectui#5120). `accessorKey` is the + * table LIBRARY's column key — `column-identity.ts` names it + * `TABLE_ADAPTER_COLUMN_KEY` and deliberately holds the metadata-identity fold + * away from it — so a column authored in the spec-canonical spelling + * (`{ field: 'stage' }`) reached the adapter carrying no `accessorKey` at all + * and rendered a header over `row[undefined]`: blank cells, no warning. The + * `$expand` whitelist in `computeLookupExpand` missed it for the same reason, + * so a `field`-spelled lookup column also lost its related record. + * + * Resolving it HERE is the move objectui#5022 made in `RelatedList` and + * objectui#5068 generalized in `ObjectGrid`: metadata vocabulary in, adapter + * vocabulary out, one translation in one place. The adapter stays monolingual; + * the producer owns the translation. + * + * Mirror, don't move — the same three rules `RelatedList` states: + * - an author-supplied `accessorKey` is NEVER overwritten; a deliberate + * divergence between the table slot and the metadata key belongs to the + * author; + * - the authored spelling is left in place, so a host reading `field` / `name` + * back off these columns keeps working; + * - an entry with no resolvable identity is returned UNTOUCHED — nothing is + * invented for it. It behaves exactly as it does today: a header (from + * `header` / `label`) over empty cells, silently. Whether that silence + * deserves a dev-time diagnostic is objectui#5349's question, and is + * deliberately NOT answered here. + * + * Returning the INPUT entry by reference when there is nothing to add is load + * bearing: data-table re-seeds its column state whenever the list is a new + * object (objectui#4618), and this widget rebuilds its node on every render. */ export function normalizeColumns(columns: (string | Record)[]): NormalizedColumn[] { return columns.map((col) => { @@ -60,7 +91,10 @@ export function normalizeColumns(columns: (string | Record)[]): Nor // widget family spell a header the same way (objectui#4618). return { header: humanizeFieldKey(col), accessorKey: col }; } - return col as NormalizedColumn; + if (!col || col.accessorKey) return col as NormalizedColumn; + const key = columnIdentity(col); + if (!key) return col as NormalizedColumn; + return { ...col, accessorKey: key } as NormalizedColumn; }); } @@ -106,8 +140,15 @@ export function computeLookupExpand( if (cols.length > 0) { // Explicit columns whitelist: only expand the relations the user asked for. + // One reader for identity, the same one `normalizeColumns` stamps with + // (objectui#5120). This used to be `c.accessorKey || c.name` — name-first, + // and blind to the spec-canonical `field` — so a `field`-spelled lookup + // column was left out of `$expand` and its cell showed a raw FK id while + // the whitelist claimed the author had not asked for it. The adapter key + // still wins when the author supplied one, exactly as it does in + // `normalizeColumns`, so both halves resolve the same column. const accessors = cols - .map((c: any) => (typeof c === 'string' ? c : (c.accessorKey || c.name))) + .map((c: any) => (typeof c === 'string' ? c : (c?.accessorKey || columnIdentity(c)))) .filter(Boolean); for (const acc of accessors) { const def = fieldsByName[acc]; diff --git a/packages/plugin-dashboard/src/__tests__/ObjectDataTable.columnIdentity.test.tsx b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.columnIdentity.test.tsx new file mode 100644 index 0000000000..00e28873c2 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/ObjectDataTable.columnIdentity.test.tsx @@ -0,0 +1,227 @@ +/** + * 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. + */ + +/** + * `ObjectDataTable` resolves column identity at the PRODUCER (objectui#5120). + * + * `data-table` is an adapter: its column key is `accessorKey` + * (`TableColumn.accessorKey`, `@object-ui/types`), which `@object-ui/core` + * deliberately holds OUTSIDE the metadata identity fold — `column-identity.ts` + * names it `TABLE_ADAPTER_COLUMN_KEY` for exactly that reason. `normalizeColumns` + * used to hand object columns to that adapter raw, so a column authored in the + * spec-canonical spelling (`{ field: 'stage' }`) arrived with no `accessorKey` + * at all: the widget rendered a header over `row[undefined]` — blank cells, no + * warning — and `computeLookupExpand`'s `$expand` whitelist missed the same + * column, so a lookup cell showed a raw FK id. + * + * The fix is the one objectui#5022 made in `RelatedList` and objectui#5068 + * generalized in `ObjectGrid`, stated there in one line: *metadata vocabulary + * in, adapter vocabulary out; one translation, one place*. This file pins the + * producer half of it for the dashboard's table widget. + * + * SCOPE. objectui#5120 also rules that `data-table`'s undeclared `col.name` + * alias (`data-table.tsx:777` / `:786`) retires. That half is NOT implemented + * here and is NOT pinned here: the card's census-first fork clause tripped — + * `skills/objectui/guides/data-integration.md` and + * `skills/objectui/guides/schema-expressions.md` both instruct authors to spell + * a `data-table` column `{ "name": …, "label": … }`, which is real authorized + * usage of the limb, so the deletion went back to the maintainer. Everything + * below is true with the alias in place and stays true after it goes: the + * producer stamps `accessorKey`, which is the one key the adapter declares. + */ +import { describe, it, expect, vi, beforeAll } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; +import { ComponentRegistry } from '@object-ui/core'; + +// The REAL renderers, imported at module scope (never behind a lazy boundary +// inside a bounded test window — AGENTS.md §测试纪律). `@object-ui/components` +// registers `data-table` as an import side effect, which is what the seam +// tests below resolve through. +import '@object-ui/components'; + +vi.mock('@object-ui/react', async () => { + const actual: any = await vi.importActual('@object-ui/react'); + return { + ...actual, + // Delegate to the REAL registered renderer for the node the widget emits. + // A hand-written stand-in would re-implement the adapter's accessor rule, + // and then the test would pin the stand-in rather than the seam. + SchemaRenderer: ({ schema }: any) => { + const Cmp = ComponentRegistry.get(schema.type) as any; + if (!Cmp) throw new Error(`${schema.type} not registered`); + return ; + }, + useDataScope: () => undefined, + SchemaRendererContext: actual.SchemaRendererContext, + }; +}); + +import { ObjectDataTable, normalizeColumns, computeLookupExpand } from '../ObjectDataTable'; + +const ROWS = [ + { id: '1', stage: 'Won', amount: 100 }, + { id: '2', stage: 'Lost', amount: 200 }, +]; + +function renderWidget(columns: unknown[]) { + return render( + , + ); +} + +/** Every rendered body cell's text, row-major. */ +function bodyCells(): string[] { + return Array.from(document.querySelectorAll('tbody td')).map((td) => (td.textContent ?? '').trim()); +} + +describe('ObjectDataTable — normalizeColumns resolves identity at the producer (#5120)', () => { + it('stamps the adapter key from the spec-canonical `field`', () => { + // THE FIX. `field` is `ListColumnSchema`'s only required key and the one + // `columnIdentity` calls canonical; before this card it was invisible here. + expect(normalizeColumns([{ field: 'stage', header: 'Stage' }])).toEqual([ + { field: 'stage', header: 'Stage', accessorKey: 'stage' }, + ]); + }); + + it('stamps the adapter key from the legacy `name` spelling too', () => { + // Not a second contract: `name` is a LEGACY_COLUMN_IDENTITY_KEY that the + // shared `columnIdentity` reader owns (objectui#3104). Reading it HERE is + // what lets the adapter stop reading it — the translation happens once, in + // the producer, instead of twice in two vocabularies. + expect(normalizeColumns([{ name: 'stage', header: 'Stage' }])).toEqual([ + { name: 'stage', header: 'Stage', accessorKey: 'stage' }, + ]); + }); + + it('never overwrites an author-supplied accessorKey, even against a divergent field', () => { + // Mirror, don't move (`RelatedList`'s rule): a deliberate divergence + // between the table slot and the metadata key belongs to the author. + const authored = { accessorKey: 'slot', field: 'stage', header: 'Stage' }; + const [out] = normalizeColumns([authored]); + expect(out).toBe(authored); + expect(out.accessorKey).toBe('slot'); + }); + + it('returns an already-canonical entry BY REFERENCE', () => { + // Load bearing, not a micro-optimisation: data-table re-seeds its column + // state whenever the list is a new object (objectui#4618) and this widget + // rebuilds its node on every render. + const authored = { accessorKey: 'stage', header: 'Stage' }; + expect(normalizeColumns([authored])[0]).toBe(authored); + }); + + it('returns an entry with NO resolvable identity untouched — nothing is invented', () => { + const orphan = { header: 'Mystery' }; + const [out] = normalizeColumns([orphan]); + expect(out).toBe(orphan); + expect(out).not.toHaveProperty('accessorKey'); + }); + + it('still expands the bare string shorthand into header + accessorKey', () => { + expect(normalizeColumns(['stage_name'])).toEqual([{ header: 'Stage Name', accessorKey: 'stage_name' }]); + }); +}); + +describe('ObjectDataTable — the $expand whitelist reads the same identity (#5120)', () => { + const objectSchema = { + fields: { + account: { type: 'lookup', reference_to: 'accounts' }, + stage: { type: 'text' }, + }, + }; + + it('expands a `field`-spelled lookup column', () => { + // Was `[]`: the whitelist resolved `c.accessorKey || c.name`, so a + // canonical column never entered it and its cell showed a raw FK id. + expect(computeLookupExpand({ columns: [{ field: 'account' }], objectName: 'opp' }, objectSchema)).toEqual([ + 'account', + ]); + }); + + it('still expands `name`-spelled and `accessorKey`-spelled lookup columns', () => { + expect(computeLookupExpand({ columns: [{ name: 'account' }] }, objectSchema)).toEqual(['account']); + expect(computeLookupExpand({ columns: [{ accessorKey: 'account' }] }, objectSchema)).toEqual(['account']); + expect(computeLookupExpand({ columns: ['account'] }, objectSchema)).toEqual(['account']); + }); + + it('leaves a non-relational column out of $expand', () => { + expect(computeLookupExpand({ columns: [{ field: 'stage' }] }, objectSchema)).toEqual([]); + }); +}); + +describe('ObjectDataTable to data-table — the producer→adapter seam (#5120)', () => { + beforeAll(() => { + expect(ComponentRegistry.has('data-table')).toBe(true); + }); + + it('renders the cells of a `field`-spelled column', () => { + // The half of this card that FIXES rather than removes. Before the producer + // resolved identity, this rendered two headers over `row[undefined]`. + renderWidget([ + { field: 'stage', header: 'Stage' }, + { field: 'amount', header: 'Amount' }, + ]); + + expect(screen.getByText('Won')).toBeInTheDocument(); + expect(screen.getByText('Lost')).toBeInTheDocument(); + expect(bodyCells()).toEqual(['Won', '100', 'Lost', '200']); + }); + + it('renders a declared `accessorKey` column exactly as before', () => { + renderWidget([ + { accessorKey: 'stage', header: 'Stage' }, + { accessorKey: 'amount', header: 'Amount' }, + ]); + + expect(bodyCells()).toEqual(['Won', '100', 'Lost', '200']); + }); + + it('leaves an unresolvable column standing — and illegible', () => { + // LEGIBILITY, pinned as behaviour rather than left as folklore, and pinned + // as MEASURED rather than as assumed. A column whose identity resolves to + // nothing is not dropped and does not throw: its header renders over empty + // cells and its neighbour is unaffected. + // + // It is not quite silent, and the noise is the interesting part. The + // adapter keys each cell by the accessor — `key={col.accessorKey}` at + // `data-table.tsx:1829` — so an unresolved column hands React `undefined` + // and React emits its generic missing-key warning. That warning names + // `tr` and `DataTableRenderer`; it names neither the column nor the + // metadata that produced it, so it points an author at React's docs rather + // than at the key they mis-spelled. Nothing in ObjectUI's own voice is + // said at all. This is unchanged by this card — such a column carried no + // `accessorKey` before it either — and it is the same silence + // objectui#5349 is weighing for `ObjectGrid`; deliberately NOT answered + // here. + const errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); + const warnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + renderWidget([ + { field: 'stage', header: 'Stage' }, + { header: 'Mystery' }, + ]); + + expect(screen.getByText('Mystery')).toBeInTheDocument(); + expect(screen.getByText('Won')).toBeInTheDocument(); + expect(screen.queryByRole('alert')).not.toBeInTheDocument(); + + const said = [...errorSpy.mock.calls, ...warnSpy.mock.calls].map((c) => c.join(' ')); + // Not one word about the column, the key, or ObjectUI. + expect(said.some((line) => /Mystery|accessorKey|column|ObjectUI/i.test(line))).toBe(false); + // The only thing said at all is React's generic missing-key warning. + expect(said).toHaveLength(1); + expect(said[0]).toMatch(/unique "key" prop/); + + errorSpy.mockRestore(); + warnSpy.mockRestore(); + }); +});