diff --git a/.changeset/view-column-io-canonical-identity-5725.md b/.changeset/view-column-io-canonical-identity-5725.md new file mode 100644 index 0000000000..51eab73b65 --- /dev/null +++ b/.changeset/view-column-io-canonical-identity-5725.md @@ -0,0 +1,39 @@ +--- +'@object-ui/app-shell': minor +--- + +The metadata designer's View **column list** now reads a column's identity — +row label and bound field name — in the ObjectStack canonical spelling only: +`field` and `label`. The legacy TanStack aliases `accessorKey` / `header` are no +longer consulted (objectui#5725). This is the editor-side half of the read +objectui#5344 retired in the column inspector one file over; the two surfaces +render inside the **same panel** and until now gave the author two different +answers about what a column is called. + +Why this is a behaviour change and not a tidy-up: `ListColumn` refuses both +legacy keys by name (`unrecognized_keys`), so a column carrying them has no +field key and no label the spec recognises. Two consequences the list surface +had that the inspector did not: + +- **The label chain was inverted, not merely tolerant.** It read + `label ?? header ?? field ?? accessorKey`, preferring the undeclared `header` + **over** the declared `field`. A perfectly canonical column that also carried + a stray `header` key displayed the alias' value *instead of* its own declared + identity. +- **The field-name read backs more than a label.** It feeds `usedFieldNames()`, + which the Add-field picker consults, so a spec-refused column reserved a field + name and the picker tagged that field "Added" — for a column no accepted + document actually binds. + +**What an author sees:** a stored column shaped `{ accessorKey, header }` is now +named positionally in the list (`col 1`) rather than by its refused spelling, +and the row stays selectable exactly as before — the positional fallback already +existed at the end of that chain. The Add-field picker no longer reports such a +column's field name as taken. Canonical `{ field, label }` columns and +bare-string columns are untouched, and a declared `label` / `field` now outranks +any stray alias beside it. + +**What is deliberately NOT changed:** the write path. These helpers still +reorder and splice the raw `columns` array without normalising it, so no stored +document is rewritten by the act of viewing or editing it — the same fence +objectui#5344 held. diff --git a/packages/app-shell/src/views/metadata-admin/inspectors/ViewColumnInspector.identityRead.test.tsx b/packages/app-shell/src/views/metadata-admin/inspectors/ViewColumnInspector.identityRead.test.tsx index 925c105a71..c78f80086a 100644 --- a/packages/app-shell/src/views/metadata-admin/inspectors/ViewColumnInspector.identityRead.test.tsx +++ b/packages/app-shell/src/views/metadata-admin/inspectors/ViewColumnInspector.identityRead.test.tsx @@ -24,6 +24,12 @@ * - A legacy column therefore stays unsaveable before AND after an edit. * That closed loop is the RULED OUTCOME, not an oversight; what this change * removes is its invisibility. + * + * The last case in this suite was written by objectui#5344 as a pin DOCUMENTED + * TO FLIP: the column list rendered beside these controls read the same retired + * aliases through `previews/view-column-io.ts`, which was outside that card's + * granted surface. objectui#5725 retired that read, and the pin is flipped + * accordingly — the panel now gives one answer instead of two. */ import '@testing-library/jest-dom/vitest'; @@ -154,22 +160,33 @@ describe('ViewColumnInspector — identity is read in the canonical spelling onl expect(parses(['title'])).toBe(true); }); - it('records the residue this card is fenced out of: the column LIST still shows the legacy name', () => { + it('the whole panel agrees: no surface presents the refused spelling as an identity', () => { mount([{ accessorKey: 'name', header: 'Name' }]); - // `FieldsListEditor` renders inside this same panel and reads its row - // labels through `previews/view-column-io.ts`, which still carries the very + // FLIPPED, as objectui#5344 wrote this pin to be. It used to assert + // `toHaveLength(1)` and carried the residue objectui#5344 was fenced out + // of: `FieldsListEditor` renders inside this same panel and read its row + // labels through `previews/view-column-io.ts`, which still carried the // alias retired above (`o.label ?? o.header ?? o.field ?? o.accessorKey`). - // So the panel's identity controls stop presenting the refused spelling - // while the list one line above still does. That file is outside this - // card's granted surface; filed as objectui#5725. When that lands, this - // expectation flips — deliberately, so the boundary is visible rather than - // silently forgotten. + // So the panel's identity controls stopped presenting the refused spelling + // while the list one line above still did — one panel, two answers. + // objectui#5725 retired that read too, so the count goes 2 → 1 → 0 across + // the two cards and the disagreement is closed. // - // Counted, not merely asserted present: post-change EXACTLY ONE element - // says `Name`, the list row. Against the pre-change source there were two - // (the panel title read `header` as the column's label as well), which is - // why this reverse-verifies as a "found multiple elements" failure. - expect(screen.getAllByText('Name')).toHaveLength(1); + // This is the observable the objectui#5344 ruling required and did not get: + // asserted as a COUNT over the whole panel, so a surface re-acquiring the + // alias reverse-verifies as a "found N elements" failure rather than + // passing unnoticed. + expect(screen.queryAllByText('Name')).toHaveLength(0); + + // …and the list row is not nameless: it names the column positionally, so + // the author still has a row to click. Measured end-to-end (including that + // the row selects) in FieldsListEditor.retiredAliases.test.tsx. + expect(screen.getByText('col 1')).toBeInTheDocument(); + + // The identity controls are unchanged by objectui#5725 — pinned here so the + // "agree" claim is two-sided rather than a claim about the list alone. + expect(shownFieldKey()).toEqual({ widget: 'input', text: '' }); + expect(shownHeader()).toBe(''); }); }); diff --git a/packages/app-shell/src/views/metadata-admin/previews/FieldsListEditor.retiredAliases.test.tsx b/packages/app-shell/src/views/metadata-admin/previews/FieldsListEditor.retiredAliases.test.tsx new file mode 100644 index 0000000000..c244e513e5 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/FieldsListEditor.retiredAliases.test.tsx @@ -0,0 +1,145 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * What the column manager SHOWS and RESERVES after the identity read is + * retired to the canonical spelling (objectui#5725). + * + * The unit pins live in `view-column-io.retiredAliases.test.ts`; this suite + * mounts the real `FieldsListEditor` because two of the claims are only true + * end-to-end: + * + * - the positional fallback `col N` actually produces a row the author can + * CLICK. That was the open question objectui#5725 declined to take — an + * empty field-key box invites re-authoring, an empty list row would leave + * nothing to click — and it is answered here by measurement rather than by + * reading the fallback and assuming a row comes out of it. + * - `usedFieldNames()` reaches the Add-field picker. Measured through the + * real popover, so the reachability claim is not taken on trust. + * + * NOTE on the size of defect 2: the picker does NOT hide a used field, it tags + * it "Added" and still lets the author click it. So a spec-refused column + * mislabelled a field as taken; it never blocked adding it. Recorded here so + * the pin says what the mechanism does, not what it was assumed to do. + */ + +import '@testing-library/jest-dom/vitest'; +import * as React from 'react'; +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { render, screen, fireEvent, cleanup, within } from '@testing-library/react'; + +// `useObjectFields` short-circuits the fetch when handed a catalog, but still +// constructs the shared metadata client — stub it so no test reaches the +// network. Same mechanism as ViewColumnInspector.identityRead.test.tsx. +const state = vi.hoisted(() => ({ + metadataClient: { get: vi.fn(async () => undefined), list: vi.fn(async () => [] as unknown[]) }, +})); +vi.mock('../useMetadata', () => ({ + useMetadataClient: () => state.metadataClient, +})); + +import { FieldsListEditor } from './FieldsListEditor'; + +afterEach(cleanup); + +const CATALOG = [ + { name: 'name', label: 'Name', type: 'text', hidden: false }, + { name: 'amount', label: 'Amount', type: 'number', hidden: false }, +]; + +/** The stored shape the whole retirement family is about. */ +const LEGACY = { accessorKey: 'name', header: 'Name' }; + +function mount(columns: unknown[], onSelectionChange = vi.fn()) { + render( + , + ); + return onSelectionChange; +} + +/** + * Open the Add-field picker and return its content element. + * + * Scoped to the popover rather than the whole render: the column list rows are + * `role="button"` too, so an unscoped query for an option matches the row of a + * canonical column as well and the test dies on ambiguity instead of measuring + * anything. + */ +function openPicker(): HTMLElement { + fireEvent.click(screen.getByRole('button', { name: /Add field/i })); + return screen.getByRole('dialog'); +} + +/** One field's row inside the open picker. */ +const option = (picker: HTMLElement, label: string) => + within(picker).getByRole('button', { name: new RegExp(`^${label}`) }); + +describe('FieldsListEditor — a spec-refused column names itself positionally', () => { + it('renders a CLICKABLE row for a legacy column instead of its retired alias', () => { + const onSelectionChange = mount([LEGACY]); + + // The retired alias is gone from the list… + expect(screen.queryByText('Name')).not.toBeInTheDocument(); + // …and the row is not nameless: it says `col 1`. + expect(screen.getByText('col 1')).toBeInTheDocument(); + + // The measurement the open question turns on: the row still selects. + const row = screen.getByText('col 1').closest('[role="button"]') as HTMLElement; + fireEvent.click(row); + expect(onSelectionChange).toHaveBeenCalledWith({ + kind: 'column', + id: 'list.columns[0]', + label: 'col 1', + }); + }); + + it('counter-probe: a canonical column still shows its declared label', () => { + mount([{ field: 'amount', label: 'Amount' }]); + + expect(screen.getByText('Amount')).toBeInTheDocument(); + expect(screen.queryByText('col 1')).not.toBeInTheDocument(); + }); + + it('lets a declared identity outrank a stray undeclared alias', () => { + // The column carries NO `label`, which is the shape the inverted chain + // actually bit on: `label ?? header ?? field` reached `header` before + // `field`, so this row read `STRAY` instead of its own declared field. + // Measured: with `label` also present the old chain returns `label` too, + // so a `{field, label, header}` row cannot reverse-verify this leg at all + // — it is green against both sources and would have been a phantom pin. + mount([{ field: 'amount', header: 'STRAY' }]); + + expect(screen.getByText('amount')).toBeInTheDocument(); + expect(screen.queryByText('STRAY')).not.toBeInTheDocument(); + }); +}); + +describe('FieldsListEditor — a spec-refused column reserves no field name', () => { + it('does not tag `name` as Added for a legacy {accessorKey, header} column', () => { + mount([LEGACY]); + const picker = openPicker(); + + // Guard against a vacuous pass: the picker really opened and really lists + // the field, so the absent badge below is a measurement, not an empty DOM. + expect(option(picker, 'Name')).toBeInTheDocument(); + expect(within(picker).queryByText('Added')).not.toBeInTheDocument(); + }); + + it('counter-probe: a canonical column DOES still reserve its name', () => { + mount([{ field: 'name', label: 'Name' }]); + const picker = openPicker(); + + expect(within(option(picker, 'Name')).getByText('Added')).toBeInTheDocument(); + // Falsification: the badge is keyed to the bound name, not shown on every row. + expect(within(option(picker, 'Amount')).queryByText('Added')).not.toBeInTheDocument(); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/view-column-io.retiredAliases.test.ts b/packages/app-shell/src/views/metadata-admin/previews/view-column-io.retiredAliases.test.ts new file mode 100644 index 0000000000..8750f44ab8 --- /dev/null +++ b/packages/app-shell/src/views/metadata-admin/previews/view-column-io.retiredAliases.test.ts @@ -0,0 +1,78 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `view-column-io` reads a column's identity in the CANONICAL spelling only + * (objectui#5725) — the editor-side half of the retirement `ViewColumnInspector` + * landed one file over in objectui#5344. + * + * `ListColumn` refuses `accessorKey` / `header` by name (`unrecognized_keys`), + * so a column carrying them has no field key and no label the spec recognises. + * These helpers nevertheless read both, which produced two distinct defects — + * pinned separately below because they are separate legs, not one: + * + * 1. `colLabel`'s chain was `label ?? header ?? field ?? accessorKey`: + * INVERTED, not merely tolerant. `header` was preferred OVER `field`, so a + * canonical column carrying a stray `header` displayed the undeclared + * alias INSTEAD of its own declared identity. + * 2. `colFieldName` backs `usedFieldNames()`, which the Add-field picker + * consults, so a spec-refused column RESERVED a field name — a display + * alias leaking into a non-display decision. + * + * The counter-probe rows are load-bearing: "the aliases are gone" is otherwise + * satisfiable by breaking the label and the reservation outright. + */ + +import { describe, it, expect } from 'vitest'; +import { colFieldName, colLabel, usedFieldNames } from './view-column-io'; + +/** The stored shape the whole retirement family is about. */ +const LEGACY = { accessorKey: 'name', header: 'Name' }; + +describe('view-column-io · colLabel reads the canonical spelling only', () => { + it('names a spec-refused column positionally, never by its retired alias', () => { + // Not `''`: the positional fallback is what keeps the row clickable, and + // it is the only reason dropping the alias is safe for the LIST surface + // (the asymmetry objectui#5725 named — an empty field-key box invites + // re-authoring, an empty list row would leave nothing to click). + expect(colLabel(LEGACY, 0)).toBe('col 1'); + expect(colLabel({ header: 'Name' }, 3)).toBe('col 4'); + expect(colLabel({ accessorKey: 'name' }, 0)).toBe('col 1'); + }); + + it('lets a DECLARED identity outrank a stray undeclared alias', () => { + // The inverted-precedence leg. Pre-change these read 'STRAY'/'STRAY'. + expect(colLabel({ field: 'name', label: 'Name', header: 'STRAY' }, 0)).toBe('Name'); + expect(colLabel({ field: 'name', header: 'STRAY' }, 0)).toBe('name'); + }); + + it('counter-probe: canonical and bare-string columns are untouched', () => { + expect(colLabel({ field: 'amount', label: 'Amount' }, 0)).toBe('Amount'); + expect(colLabel({ field: 'amount' }, 0)).toBe('amount'); + expect(colLabel('amount', 0)).toBe('amount'); + expect(colLabel('', 0)).toBe('col 1'); + expect(colLabel(null, 1)).toBe('col 2'); + }); +}); + +describe('view-column-io · colFieldName / usedFieldNames bind the canonical key only', () => { + it('does not let a spec-refused column bind a field name', () => { + expect(colFieldName(LEGACY)).toBeUndefined(); + expect(colFieldName({ accessorKey: 'name' })).toBeUndefined(); + }); + + it('does not let a spec-refused column RESERVE a name in the picker', () => { + // The reachability claim: this Set is what the Add-field picker reads. + // Pre-change it contained 'name', so the picker reported the field as + // already taken for a column no accepted document actually binds. + expect([...usedFieldNames([LEGACY])]).toEqual([]); + }); + + it('counter-probe: canonical and bare-string columns still reserve their names', () => { + expect(colFieldName({ field: 'amount', label: 'Amount' })).toBe('amount'); + expect(colFieldName('amount')).toBe('amount'); + expect([...usedFieldNames([{ field: 'amount' }, 'status', LEGACY])].sort()).toEqual([ + 'amount', + 'status', + ]); + }); +}); diff --git a/packages/app-shell/src/views/metadata-admin/previews/view-column-io.ts b/packages/app-shell/src/views/metadata-admin/previews/view-column-io.ts index 14ef01fb4f..17e0a91e05 100644 --- a/packages/app-shell/src/views/metadata-admin/previews/view-column-io.ts +++ b/packages/app-shell/src/views/metadata-admin/previews/view-column-io.ts @@ -3,15 +3,25 @@ /** * Column-IO helpers for the View column configurator. * - * A View variant's `columns` array holds entries in one of two - * canonical shapes: - * • `string` — bare field name (kanban-style) - * • `{ field, label, ... }` — ObjectStack canonical shape - * • `{ accessorKey, header, ... }` — legacy/imported TanStack shape + * A View variant's `columns` array holds entries in one of two canonical + * shapes, and these helpers read a column's identity in those spellings ONLY: + * • `string` — bare field name (kanban-style) + * • `{ field, label, ... }` — ObjectStack canonical shape * - * These helpers read either shape without mutating it and build new - * entries that respect a variant's all-strings invariant so round-trips - * stay lossless. + * The legacy/imported TanStack spelling `{ accessorKey, header, ... }` is NOT + * an identity these helpers understand. `ListColumn` refuses both keys by name + * (`unrecognized_keys`), so a column carrying them has no field key and no + * label as far as the spec is concerned; reading them here would present a + * spec-refused spelling as a valid column identity — the same consumer-side + * tolerance alias `ObjectGrid` retired in objectui#5068, and that + * {@link ViewColumnInspector} retired one file over in objectui#5344. This is + * the editor-side half of that retirement (objectui#5725): the inspector's + * identity controls and the column list rendered beside them in the SAME panel + * now agree about what a column is called. + * + * These helpers still read every shape without mutating it, and build new + * entries that respect a variant's all-strings invariant so round-trips stay + * lossless — the WRITE path is untouched, so no stored document is rewritten. */ export interface VariantInfo { @@ -21,22 +31,42 @@ export interface VariantInfo { allStrings: boolean; } -/** Human label for a column entry (falls back to a positional label). */ +/** + * Human label for a column entry, read in the canonical spelling ONLY. + * + * `o.header` and `o.accessorKey` are deliberately not consulted. The retired + * chain was not merely tolerant, it was INVERTED: `header` was preferred over + * `field`, so a canonical column that also carried a stray `header` key + * displayed the undeclared alias INSTEAD of its own declared identity. A + * declared identity must outrank an undeclared one under every reading. + * + * A column the canonical keys cannot name falls through to the positional + * label `col N`, which is what the row shows and what the author clicks — the + * list never renders nameless. + */ export function colLabel(c: unknown, i: number): string { if (typeof c === 'string') return c || `col ${i + 1}`; if (c && typeof c === 'object') { const o = c as Record; - return String(o.label ?? o.header ?? o.field ?? o.accessorKey ?? `col ${i + 1}`); + return String(o.label ?? o.field ?? `col ${i + 1}`); } return `col ${i + 1}`; } -/** Bound field name for a column entry, if any. */ +/** + * Bound field name for a column entry, if any — canonical `field` only. + * + * This one backs more than a label: it feeds {@link usedFieldNames}, which is + * what the Add-field picker consults to mark a field as already taken. Reading + * `accessorKey` here let a spec-refused column reserve a field name the author + * is entitled to add, so the picker reported "Added" for a field no accepted + * column actually binds. + */ export function colFieldName(c: unknown): string | undefined { if (typeof c === 'string') return c || undefined; if (c && typeof c === 'object') { const o = c as Record; - const v = o.field ?? o.accessorKey; + const v = o.field; return typeof v === 'string' && v ? v : undefined; } return undefined;