From 6020347a04db4ecc11a07502ad58db2062307e23 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 11:14:40 +0000 Subject: [PATCH] fix(core,list): a column has one identity, stamped at ingestion (#3104) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Column field identity was resolved twice with two different precedences over the same `schema.columns` array, and the two halves disagreed: request path ListView's `$expand` / `$select` builders and ObjectGrid.getSelectFields read `f?.field` and ONLY that. render path the FLS gate, the hidden-field filter, `fieldOrder`, both export branches and the hide-fields popover read `f.name || f.fieldName || f.field` — name FIRST. So `{field:'account', name:'account_name'}` fetched `account` while the renderer keyed off `account_name`, and `{name:'account'}` rendered a column the request dropped entirely. That is the mechanism behind the "relation column shows a bare id / column is empty / sort does nothing / export is missing a column" defect class. Per AGENTS.md #0.1 the fix is not another `?? name` at the read sites. Legacy acceptance moves to the boundary that already folds this view's vocabulary: `normalizeListViewSchema` now canonicalizes each column's identity too, running after the `fields` -> `columns` rename so a doubly-legacy view is fixed on both axes in one pass. New in @object-ui/core (`utils/column-identity.ts`): columnIdentity() the single reader, canonical-first normalizeColumnIdentity(-ies)() the fold hasConflictingColumnIdentity() true when a column's keys disagree The fold MIRRORS rather than deletes the legacy key, unlike the other folds in normalizeListViewSchema. Deleting works in-repo (every name-first read falls through to `field`), but `columns` entries cross the package boundary into host renderers and dropping `name` from under them is a breaking change with no inventory. An absent legacy key is never invented, and an already-canonical column is returned by reference so ListView's useMemo deps stay stable. `accessorKey` is deliberately untouched: it is TanStack Table's column key (`TableColumn.accessorKey`), not ObjectStack metadata identity. Tests: - ListView.columnIdentity.test.tsx — the repro through the real ListView. `checkField` is the observable: BOTH paths call it, so a column whose keys disagree checks two different field names. Now exactly one. - column-identity.ratchet.test.ts — the 34-site inventory with a per-site precedence and verdict; a new dual read, or an extra one in a listed file, fails. Shrinking fails too, so the tree and the record cannot drift. The inventory corrects the issue's framing in two places: resolveActionParams (3) and dashboard-filters (1) carry BOTH orders legitimately — `name` and `field` are separately declared there and mean different things, so those are two layers, not two spellings. The real pollution is worse than "two orders coexist": ListView and ObjectGrid each disagree with themselves. Behaviour is unchanged for any column carrying a single identity key. The entries whose resolution moves are exactly the ones where two sites already disagreed. Refs: objectstack#4115, #3090 (playbook), #2598 (chokepoint precedent) Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01C2pdPmf2yZSd4wFDs1NHY5 --- ...column-identity-single-key-at-ingestion.md | 53 ++++ content/docs/guide/troubleshooting.md | 44 +++ packages/core/src/index.ts | 1 + .../__tests__/column-identity.ratchet.test.ts | 292 ++++++++++++++++++ .../utils/__tests__/column-identity.test.ts | 216 +++++++++++++ packages/core/src/utils/column-identity.ts | 164 ++++++++++ .../core/src/utils/normalize-list-view.ts | 27 +- .../ListView.columnIdentity.test.tsx | 174 +++++++++++ 8 files changed, 970 insertions(+), 1 deletion(-) create mode 100644 .changeset/column-identity-single-key-at-ingestion.md create mode 100644 packages/core/src/utils/__tests__/column-identity.ratchet.test.ts create mode 100644 packages/core/src/utils/__tests__/column-identity.test.ts create mode 100644 packages/core/src/utils/column-identity.ts create mode 100644 packages/plugin-list/src/__tests__/ListView.columnIdentity.test.tsx diff --git a/.changeset/column-identity-single-key-at-ingestion.md b/.changeset/column-identity-single-key-at-ingestion.md new file mode 100644 index 0000000000..0deeaeca2e --- /dev/null +++ b/.changeset/column-identity-single-key-at-ingestion.md @@ -0,0 +1,53 @@ +--- +"@object-ui/core": minor +--- + +feat(core): one column identity per column — `field` stamped at ingestion (#3104) + +A column's field identity was resolved twice, with two different precedences +over the same `schema.columns` array, and the two halves disagreed: + +- **request path** — `ListView`'s `$expand` and `$select` builders, and + `ObjectGrid.getSelectFields`, read `f?.field` and only `f?.field`. +- **render path** — the FLS gate, the hidden-field filter, `fieldOrder`, both + export branches and the hide-fields popover read + `f.name || f.fieldName || f.field` — name FIRST. + +So `{ field: 'account', name: 'account_name' }` fetched `account` while the +renderer keyed off `account_name`, and `{ name: 'account' }` rendered a column +the request dropped entirely — neither `$select` nor `$expand` carried it. That +is the mechanism behind the "relation column shows a bare id / column is empty +/ sort does nothing / export is missing a column" defect class. + +Per AGENTS.md #0.1 the fix is not another `?? name` at the read sites. Legacy +acceptance moves to the one boundary that already folds this view's vocabulary, +`normalizeListViewSchema`, which now also canonicalizes each column's identity. + +New in `@object-ui/core`: + +- `columnIdentity(entry)` — the single reader. Resolves `field` → `name` → + `fieldName`, canonical-first, so it agrees with `buildExpandFields` instead + of racing it. Handles bare-string columns. +- `normalizeColumnIdentity(entry)` / `normalizeColumnIdentities(columns)` — the + fold. Stamps `field`; a legacy key that is **already present** is mirrored + onto the same identity so name-first readers resolve what the request asked + for; a legacy key that is **absent is never invented**, and an + already-canonical column is returned by reference. +- `hasConflictingColumnIdentity(entry)` — true when a column's keys disagree. +- `CANONICAL_COLUMN_IDENTITY_KEY`, `LEGACY_COLUMN_IDENTITY_KEYS`, + `TABLE_ADAPTER_COLUMN_KEY`. + +The fold **mirrors** rather than deleting the legacy key, unlike the other +folds in `normalizeListViewSchema`. Deleting would work inside this repo (every +name-first read falls through to `field`), but `columns` entries cross the +package boundary into host renderers and dropping `name` from under them is a +breaking change with no inventory. Deletion is a later call, once the in-repo +consumers read `columnIdentity()`. + +Behaviour is unchanged for any column carrying a single identity key — every +read site resolves the same string it did before. The entries whose resolution +moves are exactly the ones where two sites already disagreed. + +`accessorKey` is deliberately untouched: it is TanStack Table's own column key +(`TableColumn.accessorKey`), not ObjectStack metadata identity, and folding +across that boundary would fossilize the merge. diff --git a/content/docs/guide/troubleshooting.md b/content/docs/guide/troubleshooting.md index eac39f38a6..b26575c3ee 100644 --- a/content/docs/guide/troubleshooting.md +++ b/content/docs/guide/troubleshooting.md @@ -282,6 +282,50 @@ This reports duplicate registrations and namespace collisions. The components shipped in `@object-ui/components` and `@object-ui/plugin-calendar` already use the v9 API; this note exists so downstream apps with their own `` wrappers can apply the same migration. +## 11. A list column is empty, or a relation column shows a raw id + +**Symptom:** A column appears in a list/grid but every cell is blank, or a +`lookup` / `master_detail` / `user` column shows a record id (`8UY9zHWBfjYjYor4`) +instead of the related record's name. Sorting by that column does nothing, and +exports come out missing it. + +**Cause:** The column object names its field with more than one key. The +canonical key is `field` — the only identity key `@objectstack/spec`'s +`ListColumnSchema` declares — but stored objectui metadata also carries the +legacy `name` (and, in older imports, `fieldName`). When a column carries two +of them with different values, the renderer and the data request can resolve +two different fields: the row fetch asks the server for one field while the +grid renders another, so the cell has nothing behind it and the relation is +never expanded. + +```jsonc +// ✗ two identities on one column +{ "field": "account", "name": "account_name" } + +// ✗ legacy-only: the renderer shows it, the request used to drop it +{ "name": "account" } + +// ✓ canonical +{ "field": "account" } +``` + +**Fix:** Author columns with `field`. Metadata reaching a `list-view` is +canonicalized at the component boundary by `normalizeListViewSchema`, which +stamps `field` from whichever spelling is present and makes any legacy key it +already carries agree — so mixed metadata resolves to one field everywhere +instead of two. Legacy keys are still accepted, but they are a migration +bridge, not a second contract: fix the producer. + +If you read column identity in your own code, use the one reader rather than +spelling out a fallback chain — it resolves canonical-first, so it agrees with +what the data layer requested: + +```typescript +import { columnIdentity } from '@object-ui/core'; + +const fieldName = columnIdentity(column); // string | undefined +``` + ## Getting Help If none of the above resolves your issue: diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index b64fcbb3dc..ff97ff331c 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -18,6 +18,7 @@ export * from './utils/filter-converter.js'; export * from './utils/managedBy.js'; export * from './utils/extract-records.js'; export * from './utils/expand-fields.js'; +export * from './utils/column-identity.js'; export * from './utils/sort-values.js'; export * from './evaluator/index.js'; export * from './actions/index.js'; diff --git a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts new file mode 100644 index 0000000000..8d5328cde4 --- /dev/null +++ b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts @@ -0,0 +1,292 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * Ratchet — the column-identity dual read (`field` ?? `name`) may only shrink. + * + * objectui#3104. A column's identity is read in two incompatible precedences + * across the repo, and the two halves land on different fields for the same + * column (see `ListView.columnIdentity.test.tsx` for the live repro). This file + * freezes the inventory so the family cannot grow while it is being retired: + * a new dual read in a new file fails, and an extra one in a listed file fails. + * + * Shrinking also fails, deliberately — lower the number here in the same commit + * that removes the read, so the count in the tree and the count on record never + * drift apart. + * + * If this fails because you added a read: don't. Call `columnIdentity()` from + * `@object-ui/core` — it resolves canonical-first, so it agrees with the data + * request instead of racing it. If you genuinely need a new one, add it here + * WITH a verdict and say why in the PR. + * + * The scanner is a line-level heuristic, not a parser: an alternation chain + * (`||` / `??`) mentioning two or more distinct identity keys as property + * accesses. That is precise enough to ratchet and loose enough to catch + * spellings nobody has written yet; it is NOT a judgement that every hit is a + * defect, which is what `verdict` below records. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +/** packages/core/src/utils/__tests__ → packages */ +const packagesRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../../..'); + +/** + * Keys whose presence in an alternation chain makes it a column-identity read. + * Two or more DISTINCT ones must appear for a line to count. + */ +const IDENTITY_KEYS = ['field', 'name', 'fieldName'] as const; + +/** + * Keys that count as further members of a chain that already qualifies, but + * never qualify one on their own. `accessorKey` is TanStack Table's column key + * and `key` is a generic entry key — neither is ObjectStack metadata identity, + * and a chain built only from them is not this family. + */ +const COMPANION_KEYS = ['key', 'accessorKey'] as const; + +const ACCESS = new RegExp( + String.raw`(?:\?\.|\.)(${[...IDENTITY_KEYS, ...COMPANION_KEYS].join('|')})\b`, + 'g', +); +const ALTERNATION = /\|\||\?\?/; + +type Verdict = + /** Same concept, two spellings — the family this battle retires (PR2). */ + | 'column-identity' + /** + * Two LAYERS, not two spellings: both keys are declared, mean different + * things, and both precedences are correct. Counted so the inventory is + * honest and so these files cannot quietly grow a real dual read, but they + * are NOT convergence targets. + */ + | 'two-layer' + /** The form-field cluster — a separate battle, settled the other way (#3090). */ + | 'form-cluster' + /** Not an identity read at all (a join, or an unrelated fallback). */ + | 'unrelated'; + +interface Entry { + count: number; + /** Which key wins, for the reads that are a column identity. */ + order?: 'field-first' | 'name-first' | 'adapter-first'; + verdict: Verdict; + why: string; +} + +/** + * The inventory, as of objectui#3104 PR1. + * + * The headline is not that two precedences exist — it is that ListView and + * ObjectGrid each disagree with THEMSELVES. Both build their `$expand` / + * `$select` from `f?.field` alone (a single-key read the scanner cannot see, + * because it is not an alternation) while every render-side read in the same + * file is name-first. That is the mechanism behind "relation column shows a + * bare id": the request asks for one field, the renderer shows another. + */ +const INVENTORY: Record = { + // ── the column-identity family (24) ────────────────────────────────────── + 'plugin-list/src/ListView.tsx': { + count: 9, + order: 'name-first', + verdict: 'column-identity', + why: 'FLS gate, hidden-field filter, fieldOrder, export ×2, hide-fields list — all name-first, while $expand/$select in the same file read `f?.field` only.', + }, + 'plugin-detail/src/RelatedList.tsx': { + count: 8, + order: 'adapter-first', + verdict: 'column-identity', + why: '`accessorKey || field || name` — merges TanStack\'s column key with metadata identity. The adapter key is out of scope; the `field || name` tail is not.', + }, + 'core/src/utils/expand-fields.ts': { + count: 1, + order: 'field-first', + verdict: 'column-identity', + why: 'The request path. Canonical-first, so it is the side the renderers must converge ONTO.', + }, + 'plugin-grid/src/ObjectGrid.tsx': { + count: 1, + order: 'name-first', + verdict: 'column-identity', + why: 'The `ensureId` probe is name-first while `getSelectFields` right below it reads `c.field` only — the same self-disagreement as ListView.', + }, + 'plugin-tree/src/ObjectTree.tsx': { + count: 1, + order: 'name-first', + verdict: 'column-identity', + why: 'Tree column identity.', + }, + 'plugin-detail/src/renderers/record-details.tsx': { + count: 1, + order: 'field-first', + verdict: 'column-identity', + why: 'Detail field entry identity.', + }, + 'plugin-detail/src/renderers/record-related-list.tsx': { + count: 1, + order: 'field-first', + verdict: 'column-identity', + why: 'Related-list column identity.', + }, + 'app-shell/src/views/metadata-admin/previews/ViewPreview.tsx': { + count: 1, + order: 'field-first', + verdict: 'column-identity', + why: 'Studio preview column identity.', + }, + 'app-shell/src/views/metadata-admin/SchemaForm.tsx': { + count: 1, + order: 'field-first', + verdict: 'column-identity', + why: 'Renders a column array into a summary string.', + }, + + // ── two layers, not two spellings (6) ──────────────────────────────────── + 'app-shell/src/utils/resolveActionParams.ts': { + count: 3, + verdict: 'two-layer', + why: 'BOTH orders appear here and both are right: `field ?? name` picks the key to read off the ROW (row data is keyed by object field), `name ?? field` names the PARAM in the action payload, defaulting to the field it binds. Two concepts, not two spellings.', + }, + 'core/src/utils/dashboard-filters.ts': { + count: 1, + verdict: 'two-layer', + why: '`DashboardFilterDef` declares both: `name` is the filter variable\'s handle, `field` is the object field it targets. `name || field` defaults an unnamed filter\'s handle to its field — a derivation, not a dual read.', + }, + 'app-shell/src/views/metadata-admin/previews/ActionPreview.tsx': { + count: 1, + verdict: 'two-layer', + why: 'Action param name, same layering as resolveActionParams.', + }, + 'plugin-grid/src/resolveBulkActions.ts': { + count: 1, + verdict: 'two-layer', + why: 'Action param name, same layering as resolveActionParams.', + }, + + // ── the form cluster — settled the other way (#3090) (2) ───────────────── + 'plugin-form/src/ObjectForm.tsx': { + count: 1, + verdict: 'form-cluster', + why: '#3090 established that spec FormField (`field`) and runtime FormField (`name`) are two LAYERS; the join is deliberate and lives in `normalizeSectionField`.', + }, + 'plugin-form/src/sectionFields.ts': { + count: 1, + verdict: 'form-cluster', + why: 'The #3090 hub itself.', + }, + + // ── not identity reads (2) ─────────────────────────────────────────────── + 'app-shell/src/views/metadata-admin/widgets.tsx': { + count: 1, + verdict: 'unrelated', + why: '`fields.find(f => f.name === r.field)` — a join between a field list and a rule\'s field reference, matched only because both keys share a line.', + }, + 'plugin-chatbot/src/ChatbotEnhanced.tsx': { + count: 1, + verdict: 'unrelated', + why: '`c.object ?? c.name` — a citation label fallback that happens to sit on a line mentioning `c.field`.', + }, +}; + +function collectSourceFiles(): string[] { + const out: string[] = []; + const walk = (dir: string) => { + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const name = entry.name; + if (name === 'node_modules' || name === 'dist' || name === '__tests__') continue; + const full = path.join(dir, name); + if (entry.isDirectory()) walk(full); + else if (/\.tsx?$/.test(name) && !/\.(test|spec|d)\.tsx?$/.test(name)) out.push(full); + } + }; + for (const pkg of readdirSync(packagesRoot, { withFileTypes: true })) { + if (!pkg.isDirectory()) continue; + const src = path.join(packagesRoot, pkg.name, 'src'); + try { + walk(src); + } catch { + // A package without a `src` directory — nothing to scan. + } + } + return out; +} + +/** `{ 'pkg/src/File.tsx': ['pkg/src/File.tsx:12 :: '] }` */ +function scan(): Map { + const found = new Map(); + for (const file of collectSourceFiles()) { + const rel = path.relative(packagesRoot, file).split(path.sep).join('/'); + const src = readFileSync(file, 'utf8'); + src.split('\n').forEach((line, i) => { + if (!ALTERNATION.test(line)) return; + const keys = [...line.matchAll(ACCESS)].map((m) => m[1] as string); + const distinct = new Set(keys.filter((k) => (IDENTITY_KEYS as readonly string[]).includes(k))); + if (distinct.size < 2) return; + const list = found.get(rel) ?? []; + list.push(`${rel}:${i + 1} :: ${line.trim().slice(0, 120)}`); + found.set(rel, list); + }); + } + return found; +} + +const sum = (pick: (e: Entry) => boolean) => + Object.values(INVENTORY).filter(pick).reduce((n, e) => n + e.count, 0); + +describe('column identity dual-read ratchet (#3104)', () => { + it('scans a plausible number of package source files (guards a broken scan path)', () => { + expect(collectSourceFiles().length).toBeGreaterThan(500); + }); + + it('finds no dual read in a file the inventory does not list', () => { + const unlisted = [...scan().entries()] + .filter(([file]) => !(file in INVENTORY)) + .flatMap(([, lines]) => lines); + // If this fails: read `columnIdentity()` from `@object-ui/core` instead of + // spelling out another `field ?? name`. That is the whole point of #3104. + expect(unlisted).toEqual([]); + }); + + it('matches the recorded count in every listed file', () => { + const found = scan(); + const drift: string[] = []; + for (const [file, entry] of Object.entries(INVENTORY)) { + const actual = found.get(file)?.length ?? 0; + if (actual === entry.count) continue; + drift.push( + actual > entry.count + ? `${file}: ${actual} dual reads, inventory says ${entry.count} — converge it onto columnIdentity() instead of adding another.` + : `${file}: ${actual} dual reads, inventory says ${entry.count} — good news; lower the count in this file to match.`, + ); + } + expect(drift).toEqual([]); + }); + + it('keeps the family total from growing', () => { + // The number that has to reach zero. The other verdicts are inventory, not + // worklist: `two-layer` and `form-cluster` are settled decisions, and + // `unrelated` is scanner noise recorded so it cannot hide a real one. + expect(sum((e) => e.verdict === 'column-identity')).toBe(24); + expect(sum(() => true)).toBe(34); + }); + + it('records a precedence for every read in the family', () => { + const missing = Object.entries(INVENTORY) + .filter(([, e]) => e.verdict === 'column-identity' && !e.order) + .map(([file]) => file); + expect(missing).toEqual([]); + }); + + it('still shows both precedences in the family — the pollution this closes', () => { + const orders = new Set( + Object.values(INVENTORY) + .filter((e) => e.verdict === 'column-identity') + .map((e) => e.order), + ); + // When PR2 lands this becomes a single order and this assertion inverts. + expect(orders.size).toBeGreaterThan(1); + }); +}); diff --git a/packages/core/src/utils/__tests__/column-identity.test.ts b/packages/core/src/utils/__tests__/column-identity.test.ts new file mode 100644 index 0000000000..0302c481d9 --- /dev/null +++ b/packages/core/src/utils/__tests__/column-identity.test.ts @@ -0,0 +1,216 @@ +/** + * 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. + */ + +import { describe, it, expect } from 'vitest'; +import { + CANONICAL_COLUMN_IDENTITY_KEY, + LEGACY_COLUMN_IDENTITY_KEYS, + TABLE_ADAPTER_COLUMN_KEY, + columnIdentity, + hasConflictingColumnIdentity, + normalizeColumnIdentity, + normalizeColumnIdentities, +} from '../column-identity'; +import { normalizeListViewSchema } from '../normalize-list-view'; +import { buildExpandFields } from '../expand-fields'; + +describe('columnIdentity (#3104)', () => { + it('reads the spec-canonical `field`', () => { + expect(columnIdentity({ field: 'stage' })).toBe('stage'); + }); + + it('reads a bare string column', () => { + expect(columnIdentity('stage')).toBe('stage'); + }); + + it('falls back to the legacy `name`, then `fieldName`', () => { + expect(columnIdentity({ name: 'stage' })).toBe('stage'); + expect(columnIdentity({ fieldName: 'stage' })).toBe('stage'); + expect(columnIdentity({ name: 'a', fieldName: 'b' })).toBe('a'); + }); + + it('lets the canonical key win over every legacy spelling', () => { + expect(columnIdentity({ field: 'account', name: 'account_name' })).toBe('account'); + expect(columnIdentity({ field: 'a', name: 'b', fieldName: 'c' })).toBe('a'); + }); + + it('agrees with what `buildExpandFields` already resolves', () => { + // The two must not drift: `buildExpandFields` IS the request path, and the + // whole defect is the render path resolving something else. + const fields = { account: { type: 'lookup', reference: 'accounts' } }; + const column = { field: 'account', name: 'account_name' }; + expect(buildExpandFields(fields, [column])).toEqual([columnIdentity(column)]); + }); + + it('returns undefined — not an empty string — when nothing is resolvable', () => { + expect(columnIdentity({})).toBeUndefined(); + expect(columnIdentity({ field: '' })).toBeUndefined(); + expect(columnIdentity('')).toBeUndefined(); + expect(columnIdentity(null)).toBeUndefined(); + expect(columnIdentity(undefined)).toBeUndefined(); + expect(columnIdentity(42)).toBeUndefined(); + }); + + it('skips an empty canonical key rather than resolving to it', () => { + expect(columnIdentity({ field: '', name: 'stage' })).toBe('stage'); + }); + + it('does NOT read the TanStack adapter key', () => { + // `accessorKey` names a slot in the table library's column model, not a + // field in ObjectStack metadata. Reading it here would merge the two + // vocabularies — the mistake this battle exists to undo. + expect(TABLE_ADAPTER_COLUMN_KEY).toBe('accessorKey'); + expect(columnIdentity({ accessorKey: 'stage' })).toBeUndefined(); + }); +}); + +describe('hasConflictingColumnIdentity (#3104)', () => { + it('flags a column whose keys disagree', () => { + expect(hasConflictingColumnIdentity({ field: 'account', name: 'account_name' })).toBe(true); + expect(hasConflictingColumnIdentity({ name: 'a', fieldName: 'b' })).toBe(true); + }); + + it('does not flag agreeing or single-key columns', () => { + expect(hasConflictingColumnIdentity({ field: 'account', name: 'account' })).toBe(false); + expect(hasConflictingColumnIdentity({ field: 'account' })).toBe(false); + expect(hasConflictingColumnIdentity({ name: 'account' })).toBe(false); + expect(hasConflictingColumnIdentity({})).toBe(false); + }); + + it('ignores the adapter key when judging conflict', () => { + expect(hasConflictingColumnIdentity({ accessorKey: 'x', field: 'account' })).toBe(false); + }); +}); + +describe('normalizeColumnIdentity (#3104)', () => { + it('stamps the canonical key from a legacy-only column', () => { + expect(normalizeColumnIdentity({ name: 'stage' })).toEqual({ field: 'stage', name: 'stage' }); + expect(normalizeColumnIdentity({ fieldName: 'stage' })) + .toEqual({ field: 'stage', fieldName: 'stage' }); + }); + + it('mirrors a disagreeing legacy key onto the canonical identity', () => { + expect(normalizeColumnIdentity({ field: 'account', name: 'account_name' })) + .toEqual({ field: 'account', name: 'account' }); + }); + + it('never invents a legacy key that was not already there', () => { + // The fold retires the legacy vocabulary; manufacturing it would be the + // opposite of the point. A spec-clean column comes back BY REFERENCE. + const column = { field: 'stage', label: 'Stage' }; + expect(normalizeColumnIdentity(column)).toBe(column); + expect(normalizeColumnIdentity(column)).not.toHaveProperty('name'); + }); + + it('preserves every other key on the column', () => { + expect(normalizeColumnIdentity({ name: 'stage', label: 'Stage', width: 120, sortable: true })) + .toEqual({ field: 'stage', name: 'stage', label: 'Stage', width: 120, sortable: true }); + }); + + it('leaves the TanStack adapter key untouched', () => { + expect(normalizeColumnIdentity({ accessorKey: 'stage', name: 'other' })) + .toEqual({ accessorKey: 'stage', field: 'other', name: 'other' }); + }); + + it('passes strings and unresolvable entries through by reference', () => { + expect(normalizeColumnIdentity('stage')).toBe('stage'); + const empty = {}; + expect(normalizeColumnIdentity(empty)).toBe(empty); + }); + + it('does not mutate its input', () => { + const column = { field: 'account', name: 'account_name' }; + normalizeColumnIdentity(column); + expect(column).toEqual({ field: 'account', name: 'account_name' }); + }); + + it('is idempotent', () => { + const once = normalizeColumnIdentity({ field: 'account', name: 'account_name' }); + expect(normalizeColumnIdentity(once)).toBe(once); + }); +}); + +describe('normalizeColumnIdentities (#3104)', () => { + it('folds every entry that needs it', () => { + expect(normalizeColumnIdentities([ + 'stage', + { field: 'account', name: 'account_name' }, + { name: 'owner' }, + ])).toEqual([ + 'stage', + { field: 'account', name: 'account' }, + { field: 'owner', name: 'owner' }, + ]); + }); + + it('returns the input array by reference when nothing needs folding', () => { + // ListView's downstream `useMemo`s depend on this: a new array identity on + // the already-canonical path would re-render the whole grid every pass. + const columns = ['stage', { field: 'account' }]; + expect(normalizeColumnIdentities(columns)).toBe(columns); + }); + + it('passes non-arrays through', () => { + expect(normalizeColumnIdentities(undefined)).toBeUndefined(); + expect(normalizeColumnIdentities('stage')).toBe('stage'); + }); + + it('exposes the key vocabulary it folds', () => { + expect(CANONICAL_COLUMN_IDENTITY_KEY).toBe('field'); + expect(LEGACY_COLUMN_IDENTITY_KEYS).toEqual(['name', 'fieldName']); + }); +}); + +describe('normalizeListViewSchema — column identity fold (#3104)', () => { + const columnsOf = (schema: unknown) => + (schema as Record).columns as unknown[]; + + it('canonicalizes the identities of `columns`', () => { + const out = normalizeListViewSchema({ + viewType: 'grid', + columns: [{ field: 'account', name: 'account_name' }, { name: 'owner' }], + }); + expect(columnsOf(out)).toEqual([ + { field: 'account', name: 'account' }, + { field: 'owner', name: 'owner' }, + ]); + }); + + it('runs AFTER the legacy `fields` → `columns` rename', () => { + // A view can be legacy on both axes at once: legacy list key AND legacy + // entry key. One pass has to fix both or the entries stay unresolvable. + const out = normalizeListViewSchema({ viewType: 'grid', fields: [{ name: 'owner' }] }); + expect(columnsOf(out)).toEqual([{ field: 'owner', name: 'owner' }]); + expect(out).not.toHaveProperty('fields'); + }); + + it('still returns the schema by reference when there is nothing to fold', () => { + const schema = { type: 'list-view', viewType: 'grid', columns: [{ field: 'account' }] }; + expect(normalizeListViewSchema(schema)).toBe(schema); + }); + + it('does not mutate the input schema', () => { + const columns = [{ field: 'account', name: 'account_name' }]; + const schema = { viewType: 'grid', columns }; + normalizeListViewSchema(schema); + expect(columns).toEqual([{ field: 'account', name: 'account_name' }]); + }); + + it('is idempotent', () => { + const once = normalizeListViewSchema({ + viewType: 'grid', + columns: [{ field: 'account', name: 'account_name' }], + }); + expect(normalizeListViewSchema(once)).toBe(once); + }); + + it('leaves string columns alone', () => { + const schema = { viewType: 'grid', columns: ['name', 'stage'] }; + expect(normalizeListViewSchema(schema)).toBe(schema); + }); +}); diff --git a/packages/core/src/utils/column-identity.ts b/packages/core/src/utils/column-identity.ts new file mode 100644 index 0000000000..f537528f9c --- /dev/null +++ b/packages/core/src/utils/column-identity.ts @@ -0,0 +1,164 @@ +/** + * ObjectUI — column identity canonicalization + * 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. + */ + +/** + * The spec's canonical key for "which object field this column shows". + * `ListColumnSchema` (`@objectstack/spec/view`) declares `field: z.ZodString` + * as its ONLY required key — there is no `name`, no `fieldName`, no + * `accessorKey` on it. So `field` is not one dialect among several: it is the + * contract, and everything else is objectui-side legacy (AGENTS.md #0). + * + * Note this is the exact opposite of the FORM layer, where the runtime key is + * `name` (objectui#3090). The two surfaces sit next to each other and mean the + * opposite thing by the same pair of key names, which is why copy-paste between + * them keeps producing the defect this module exists to close. + */ +export const CANONICAL_COLUMN_IDENTITY_KEY = 'field'; + +/** + * Legacy identity spellings, in the order a canonicalizing read falls back + * through them. `field` wins over both, always. + * + * `fieldName` is here for defence, not because objectui emits it: as of + * objectui#3104 a repo-wide scan finds NO writer of a column-level `fieldName` + * — every `fieldName:` occurrence in the tree is a function parameter or an + * unrelated interface member (`AIFieldSuggestion.fieldName`). It survives + * purely in READERS that were written defensively, and stored host metadata + * could still carry it, so the fold accepts it and the ratchet counts it. + */ +export const LEGACY_COLUMN_IDENTITY_KEYS = ['name', 'fieldName'] as const; + +/** + * NOT an identity key — deliberately excluded from everything in this module. + * + * `accessorKey` is TanStack Table's own column key (`TableColumn.accessorKey`, + * `packages/types/src/data-display.ts`, "Data accessor key"). It names a slot in + * the table LIBRARY's column model, not a field in ObjectStack metadata. Reads + * like `RelatedList`'s `accessorKey || field || name` therefore merge two + * different vocabularies in one expression; canonicalizing across that boundary + * would fossilize the merge, which is precisely the mistake this battle exists + * to undo. The fold neither reads nor writes it. + */ +export const TABLE_ADAPTER_COLUMN_KEY = 'accessorKey'; + +const isRecord = (v: unknown): v is Record => + !!v && typeof v === 'object' && !Array.isArray(v); + +const asIdentity = (v: unknown): string | undefined => + typeof v === 'string' && v.length > 0 ? v : undefined; + +/** + * Read a column entry's field identity — the ONE reader every consumer should + * converge onto (objectui#3104 PR2). + * + * Accepts the three shapes a `columns` array actually holds: a bare string + * (`'stage'`), a spec column (`{ field: 'stage' }`), and the legacy objectui + * shapes (`{ name }` / `{ fieldName }`). Resolution is canonical-first, so it + * agrees with `buildExpandFields` and with the `$select` projection rather than + * with whichever half of a mixed-key column happened to be read. + * + * Returns `undefined` — not `''` — when nothing is resolvable, so callers can + * tell "no identity" from "identity is the empty string" and drop the entry + * instead of requesting a nameless column. + */ +export function columnIdentity(entry: unknown): string | undefined { + if (typeof entry === 'string') return asIdentity(entry); + if (!isRecord(entry)) return undefined; + const canonical = asIdentity(entry[CANONICAL_COLUMN_IDENTITY_KEY]); + if (canonical) return canonical; + for (const key of LEGACY_COLUMN_IDENTITY_KEYS) { + const legacy = asIdentity(entry[key]); + if (legacy) return legacy; + } + return undefined; +} + +/** + * True when a column entry carries more than one identity key AND they + * disagree — the shape that makes the renderer and the data request resolve + * two different fields. Exported for the "say who wins, loudly" step + * (objectui#3104 PR3) and used by the tests to name the defect precisely. + */ +export function hasConflictingColumnIdentity(entry: unknown): boolean { + if (!isRecord(entry)) return false; + const resolved = columnIdentity(entry); + if (!resolved) return false; + for (const key of [CANONICAL_COLUMN_IDENTITY_KEY, ...LEGACY_COLUMN_IDENTITY_KEYS]) { + const present = asIdentity(entry[key]); + if (present && present !== resolved) return true; + } + return false; +} + +/** + * Stamp the canonical identity onto one column entry. + * + * The transitional strategy (objectui#3104 PR1) is **mirror, don't delete**: + * + * - `field` is written with the resolved identity, so the request path — which + * reads `field` and ONLY `field` in `ListView`'s `$expand`/`$select` + * builders and in `ObjectGrid.getSelectFields` — always finds one. + * - a legacy key that is ALREADY PRESENT is overwritten with that same + * identity, so the name-first read sites still standing (`ListView` ×8, + * `ObjectGrid`, `ObjectTree`, the detail renderers) resolve to exactly what + * the request asked for. + * - a legacy key that is ABSENT is never invented. A spec-clean + * `{ field: 'stage' }` comes back untouched, by reference; the fold does not + * manufacture the legacy vocabulary it is trying to retire. + * + * This differs on purpose from `normalizeListViewSchema`'s other folds, which + * DELETE the legacy key so a missed read-site fails loudly. Deleting works + * inside this repo — every name-first read here falls through to `field` — but + * `columns` entries cross the package boundary into host applications and + * third-party renderers, and dropping `name` from under them is a breaking + * change we have no inventory for. Mirroring is behaviour-preserving for every + * reader, in or out of the repo; the deletion is PR3's call to make once the + * in-repo consumers have converged onto {@link columnIdentity}. + * + * The result: for any single-key column the resolved identity is unchanged at + * every read site. The only entries whose behaviour moves are the ones where + * two sites already disagreed — which is the defect, not a regression. + */ +export function normalizeColumnIdentity(entry: T): T { + if (typeof entry === 'string' || !isRecord(entry)) return entry; + const identity = columnIdentity(entry); + if (!identity) return entry; + + const needsCanonical = entry[CANONICAL_COLUMN_IDENTITY_KEY] !== identity; + const staleLegacy = LEGACY_COLUMN_IDENTITY_KEYS.filter( + (key) => entry[key] !== undefined && entry[key] !== identity, + ); + if (!needsCanonical && staleLegacy.length === 0) return entry; + + const next: Record = { ...entry }; + next[CANONICAL_COLUMN_IDENTITY_KEY] = identity; + for (const key of staleLegacy) next[key] = identity; + return next as T; +} + +/** + * Stamp the canonical identity across a `columns` array. + * + * Non-mutating and allocation-frugal in the same way as + * `normalizeListViewSchema`: when every entry is already canonical the INPUT + * array is returned by reference, so `ListView`'s downstream `useMemo`s keep a + * stable dependency identity on the common path and nothing re-renders. + * + * String entries pass through untouched — a bare `'stage'` is unambiguous and + * every consumer already special-cases it. + */ +export function normalizeColumnIdentities(columns: T): T { + if (!Array.isArray(columns)) return columns; + let changed = false; + const next = columns.map((entry) => { + const folded = normalizeColumnIdentity(entry); + if (folded !== entry) changed = true; + return folded; + }); + return (changed ? next : columns) as T; +} diff --git a/packages/core/src/utils/normalize-list-view.ts b/packages/core/src/utils/normalize-list-view.ts index 879422449b..645f1b93a9 100644 --- a/packages/core/src/utils/normalize-list-view.ts +++ b/packages/core/src/utils/normalize-list-view.ts @@ -6,6 +6,8 @@ * LICENSE file in the root directory of this source tree. */ +import { normalizeColumnIdentities } from './column-identity.js'; + /** ListView's toolbar density vocabulary — three steps, not the spec's five. */ export type DensityMode = 'compact' | 'comfortable' | 'spacious'; @@ -149,6 +151,13 @@ const ARIA_KEY_ALIASES: Record = { * metadata stores and hosts forward verbatim, becomes the renderable `'grid'` * — otherwise it reaches the renderer's typeless default branch and shows as * a red "Unknown component type" box. + * - each `columns` entry's IDENTITY — `name` / `fieldName` → the spec's `field` + * (#3104). This one MIRRORS instead of deleting, for the reason given on + * {@link normalizeColumnIdentities}: `columns` entries cross the package + * boundary into host renderers, so dropping `name` from under them is a + * breaking change with no inventory. Runs AFTER the `fields` → `columns` fold + * above, so a view that spells its column list the legacy way still gets its + * entries canonicalized. */ export function normalizeListViewSchema(schema: T): T { if (!schema || typeof schema !== 'object') return schema; @@ -168,9 +177,22 @@ export function normalizeListViewSchema(schema: T): T { const foldSharing = !!sharing && (sharing.visibility !== undefined || sharing.enabled !== undefined); const viewType = s.viewType; const defaultViewKind = !viewType || viewType === 'list'; + // The columns array the identity fold will see — mirroring the `foldColumns` + // precedence below (canonical `columns` wins; otherwise the legacy `fields` + // that is about to become it). `normalizeColumnIdentities` returns its input + // by reference when every entry is already canonical, so this comparison is + // also the "is there anything to do" test. + const columnsSource = Array.isArray(s.columns) + ? s.columns + : foldColumns + ? (legacyFields as unknown[]) + : undefined; + const foldedColumns = normalizeColumnIdentities(columnsSource); + const foldColumnIdentity = foldedColumns !== columnsSource; if ( !foldColumns && !foldRowHeight && !foldFilter && !legacyFlags.length && - !foldDescription && !foldAria && !foldSharing && !defaultViewKind + !foldDescription && !foldAria && !foldSharing && !defaultViewKind && + !foldColumnIdentity ) { return schema; } @@ -180,6 +202,9 @@ export function normalizeListViewSchema(schema: T): T { if (!Array.isArray(next.columns)) next.columns = legacyFields; delete next.fields; } + // After the `fields` → `columns` rename, so a legacy-spelled column LIST gets + // its per-entry identities canonicalized in the same pass. + if (foldColumnIdentity) next.columns = foldedColumns; if (foldRowHeight) { if (typeof next.rowHeight !== 'string') { next.rowHeight = DENSITY_MODE_TO_ROW_HEIGHT[legacyDensity as DensityMode]; diff --git a/packages/plugin-list/src/__tests__/ListView.columnIdentity.test.tsx b/packages/plugin-list/src/__tests__/ListView.columnIdentity.test.tsx new file mode 100644 index 0000000000..c47b94d60a --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.columnIdentity.test.tsx @@ -0,0 +1,174 @@ +/** + * 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. + * + * Repro + regression pin for the column-identity dual read (objectui#3104). + * + * ListView resolves "which field is this column" TWICE, with two different + * precedences over the same `schema.columns` array: + * + * request path — `$expand` and the `$select` projection read `f?.field` + * and ONLY `f?.field` (ListView.tsx, the `expandFields` and + * `selectFields` memos). + * render path — the FLS gate, the hidden-field filter, `fieldOrder`, the + * export column list and the hide-fields popover all read + * `f.name || f.fieldName || f.field` — name FIRST. + * + * So one column object can be two different fields at once, and the failure is + * silent in both directions: + * + * `{ field: 'account', name: 'account_name' }` + * → the row fetch asks the server for `account` (+ `$expand=account`) + * while the field-level permission check, the export and the + * hide-fields list all key off `account_name`. + * `{ name: 'account' }` + * → the render path shows an `account` column; the request path + * resolves `undefined` and DROPS it, so neither `$select` nor + * `$expand` carries it. The column renders with nothing behind it — + * this is the "relation column shows a bare id" defect class. + * + * `checkField` is the sharpest observable available: BOTH paths call it (the + * render path from `effectiveFields`, the request path from `selectFields`), + * so a column whose two keys disagree makes the same column check two + * different field names. One column must produce exactly one identity. + * + * If this fails: do not add another `?? name` at the failing read site. The + * fold lives in `normalizeColumnIdentity` (`@object-ui/core`), wired into + * `normalizeListViewSchema` at ListView's component boundary. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render } from '@testing-library/react'; +import type { ListViewSchema } from '@object-ui/types'; +import { SchemaRendererProvider } from '@object-ui/react'; +import { ListView } from '../ListView'; + +// `vi.mock` is hoisted above this file's imports, so the spy it closes over has +// to be hoisted too. +const { checkField } = vi.hoisted(() => ({ checkField: vi.fn(() => true) })); + +// ListView's FLS gate is skipped entirely unless permissions report `isLoaded`, +// and the real provider needs role metadata we have no reason to model here. +// Mocking just `usePermissions` turns the gate on and makes every identity it +// resolves observable, while leaving the rest of the package intact. +vi.mock('@object-ui/permissions', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + usePermissions: () => ({ + check: () => ({ allowed: true }), + checkField, + getFieldPermissions: () => [], + getRowFilter: () => undefined, + getObjectApiOperations: () => undefined, + roles: [], + systemPermissions: [], + hasCapabilities: () => true, + isLoaded: true, + can: () => true, + cannot: () => false, + }), + }; +}); + +/** `account` is a lookup, so a correctly-resolved column also drives `$expand`. */ +const makeDataSource = () => ({ + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), + getObjectSchema: vi.fn().mockResolvedValue({ + name: 'contacts', + fields: { + name: { type: 'text' }, + account: { type: 'lookup', reference: 'accounts' }, + }, + }), +}); + +const renderList = async (columns: unknown[]) => { + const ds = makeDataSource(); + const schema = { + type: 'list-view', + objectName: 'contacts', + viewType: 'grid', + columns, + } as unknown as ListViewSchema; + + render( + + + , + ); + + await vi.waitFor(() => expect(ds.find).toHaveBeenCalled()); + const params = ds.find.mock.calls.at(-1)?.[1] ?? {}; + return { + select: (params.$select ?? []) as string[], + expand: (params.$expand ?? []) as string[], + /** Every distinct field name either path resolved for the account column. */ + checkedIdentities: new Set( + checkField.mock.calls + .map((call) => (call as unknown as unknown[])[1] as string) + .filter((f) => typeof f === 'string' && f.startsWith('account')), + ), + }; +}; + +describe('ListView — column identity is resolved once (#3104)', () => { + beforeEach(() => { + checkField.mockClear(); + }); + + it('resolves ONE identity when a column carries both `field` and `name`', async () => { + // The spec's canonical key is `field` (`ListColumnSchema` declares it as the + // only required key and has no `name` at all), so `account` must win and + // `account_name` must appear nowhere. + const { select, expand, checkedIdentities } = await renderList([ + { field: 'account', name: 'account_name', label: 'Account' }, + ]); + + expect(select).toContain('account'); + expect(expand).toContain('account'); + // The render path used to key off `account_name` here while the request + // asked for `account` — one column, two fields. + expect(checkedIdentities).toEqual(new Set(['account'])); + expect(select).not.toContain('account_name'); + expect(expand).not.toContain('account_name'); + }); + + it('requests a legacy `name`-only column instead of silently dropping it', async () => { + // The bare-id defect: the render path shows the column, the request path + // resolved `undefined` and filtered it out, so the row arrived without the + // field and without the expanded relation. + const { select, expand, checkedIdentities } = await renderList([ + { name: 'account', label: 'Account' }, + ]); + + expect(select).toContain('account'); + expect(expand).toContain('account'); + expect(checkedIdentities).toEqual(new Set(['account'])); + }); + + it('leaves a spec-canonical column exactly as it was', async () => { + const { select, expand, checkedIdentities } = await renderList([ + { field: 'account', label: 'Account' }, + ]); + + expect(select).toContain('account'); + expect(expand).toContain('account'); + expect(checkedIdentities).toEqual(new Set(['account'])); + }); + + it('leaves a bare string column exactly as it was', async () => { + const { select, expand, checkedIdentities } = await renderList(['account']); + + expect(select).toContain('account'); + expect(expand).toContain('account'); + expect(checkedIdentities).toEqual(new Set(['account'])); + }); +});