diff --git a/.changeset/column-identity-consumers-read-one-key.md b/.changeset/column-identity-consumers-read-one-key.md new file mode 100644 index 0000000000..5870efe76c --- /dev/null +++ b/.changeset/column-identity-consumers-read-one-key.md @@ -0,0 +1,59 @@ +--- +"@object-ui/core": patch +"@object-ui/plugin-list": patch +"@object-ui/plugin-grid": patch +"@object-ui/plugin-detail": patch +"@object-ui/plugin-tree": patch +--- + +fix(list,grid,detail,tree,core): every column resolver reads one key (#3104 PR2) + +PR1 (#3119) put a canonicalizing fold at ListView's ingestion boundary. This +converges the 22 read sites themselves onto `columnIdentity()` from +`@object-ui/core`, so a surface that is NOT downstream of that fold resolves +the same identity anyway. + +That distinction is the user-visible part. A standalone `object-grid` node — +authored directly on a page, with no `list-view` above it — never passed +through `normalizeListViewSchema`. Its `getSelectFields` read `c.field` alone +while the `ensureId` probe one line above read `f?.name || f?.field`, so a +legacy `{ name: 'account' }` column reached `$select` as a literal `undefined` +hole: the server never returned the field and every cell in that column came +back empty. Same for `ObjectTree`, `RelatedList` and the `record:details` / +`record:related_list` renderers. + +Converged: + +| Surface | Was | Now | +|---|---|---| +| `ListView` ×9 + its 2 request builders | `name \|\| fieldName \|\| field` vs `f?.field` | `columnIdentity()` | +| `RelatedList` ×8 | `accessorKey \|\| field \|\| name` | `accessorKey \|\| columnIdentity()` | +| `ObjectGrid` | name-first probe vs `c.field` projection | `columnIdentity()` | +| `ObjectTree` | `name \|\| fieldName \|\| field \|\| key` | `columnIdentity() \|\| key` | +| `buildExpandFields` | `field ?? name ?? fieldName` | `columnIdentity()` | +| `record-details` / `record-related-list` | `field \|\| name (\|\| key)` | `columnIdentity() (\|\| key)` | + +`accessorKey` keeps its precedence in `RelatedList` — it is TanStack Table's +column key, not ObjectStack metadata identity, and only the `field || name` +tail was converged. `key` stays a tail fallback in `ObjectTree` and +`record-related-list` for the same reason: it is a generic entry key. + +Two incidental fixes that TypeScript surfaced once the resolver stopped +returning `any`: ListView's filter-field options and its hide-fields popover +both built entries keyed `undefined` for a column with no resolvable identity. +Those entries could never match a column; they are now dropped. + +**Inventory re-triage.** PR1 recorded 24 family members. Two were mis-classified +and are reclassified here rather than converged — reading what they actually +feed shows they are not column reads at all: + +- `ViewPreview.tsx` adapts a ViewItem **form** section to what `object-form` + selects by (`field` → `name`) — the #3090 two-layer join. +- `SchemaForm.tsx` renders an arbitrary metadata **array** into a popover + summary and guesses at a display key; the entries are validations, actions, + or whatever the JSON schema declares. + +So the family was 22, and it is now **0**. The ratchet asserts that, asserts +each converged surface actually routes through the shared reader (a surface +that dropped identity resolution instead of converging it goes red), and pins +`accessorKey`'s precedence in `RelatedList`. diff --git a/content/docs/guide/troubleshooting.md b/content/docs/guide/troubleshooting.md index b26575c3ee..cf1be0151e 100644 --- a/content/docs/guide/troubleshooting.md +++ b/content/docs/guide/troubleshooting.md @@ -309,12 +309,19 @@ never expanded. { "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. +**Fix:** Author columns with `field`. Two mechanisms keep legacy metadata +working, and both resolve the canonical key first: + +- 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. +- Every renderer that resolves a column — list, grid, tree, related lists, the + `$expand` / `$select` builders — reads that identity through one shared + function, so a surface rendered outside the fold (a standalone `object-grid` + node, for instance) still resolves the same field the request asked for. + +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 diff --git a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts index 8d5328cde4..ac110cb2c9 100644 --- a/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts +++ b/packages/core/src/utils/__tests__/column-identity.ratchet.test.ts @@ -4,11 +4,17 @@ * * 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. + * objectui#3104. A column's identity used to be read in two incompatible + * precedences across the repo, and the two halves landed on different fields + * for the same column (see `ListView.columnIdentity.test.tsx` for the repro). + * + * **The family is now at zero** (PR2): every column-identity read goes through + * `columnIdentity()`. What remains in the inventory below is the residue the + * scanner still matches — reads that share the key names but not the concept. + * They are listed, not converged, and each says why. + * + * This file freezes that state: 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 @@ -78,71 +84,25 @@ interface Entry { } /** - * The inventory, as of objectui#3104 PR1. + * The inventory, as of objectui#3104 PR2. + * + * The column-identity family is EMPTY: all 22 reads now go through + * `columnIdentity()`. What is left below matched the scanner but was never the + * same concept — two-layer joins where both precedences are correct, the form + * cluster that #3090 settled the other way, and generic display fallbacks that + * merely share the key names. They stay listed so the scanner cannot quietly + * grow a real dual read inside a file we already decided about. * - * 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. + * PR1 recorded 24 family members; two of those were mis-triaged and are + * reclassified here after reading what they actually feed: + * - `ViewPreview.tsx` converts a ViewItem FORM section into `object-form`'s + * runtime shape (`field` -> `name`) — the #3090 two-layer join, not a column. + * - `SchemaForm.tsx` renders an arbitrary metadata ARRAY into a summary string + * and guesses at a display key; the entries are validations/actions/whatever + * the JSON schema declares, so there is no column vocabulary to converge. */ 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.', - }, + // ── the column-identity family: EMPTY (was 24 in PR1; 22 after re-triage) ── // ── two layers, not two spellings (6) ──────────────────────────────────── 'app-shell/src/utils/resolveActionParams.ts': { @@ -166,7 +126,7 @@ const INVENTORY: Record = { why: 'Action param name, same layering as resolveActionParams.', }, - // ── the form cluster — settled the other way (#3090) (2) ───────────────── + // ── the form cluster — settled the other way (#3090) (3) ───────────────── 'plugin-form/src/ObjectForm.tsx': { count: 1, verdict: 'form-cluster', @@ -177,8 +137,13 @@ const INVENTORY: Record = { verdict: 'form-cluster', why: 'The #3090 hub itself.', }, + 'app-shell/src/views/metadata-admin/previews/ViewPreview.tsx': { + count: 1, + verdict: 'form-cluster', + why: 'Re-triaged in PR2: `toFormFieldEntry` adapts a ViewItem FORM section to what `object-form` selects by (`name`), which is the #3090 join — not a list column. Converging it onto `columnIdentity` would be a category error.', + }, - // ── not identity reads (2) ─────────────────────────────────────────────── + // ── not identity reads (3) ─────────────────────────────────────────────── 'app-shell/src/views/metadata-admin/widgets.tsx': { count: 1, verdict: 'unrelated', @@ -189,6 +154,11 @@ const INVENTORY: Record = { verdict: 'unrelated', why: '`c.object ?? c.name` — a citation label fallback that happens to sit on a line mentioning `c.field`.', }, + 'app-shell/src/views/metadata-admin/SchemaForm.tsx': { + count: 1, + verdict: 'unrelated', + why: 'Re-triaged in PR2: `summariseComposite` renders ANY composite/array metadata value into a popover summary and guesses at a display key. The array items are whatever the JSON schema declares, so this is a display fallback over arbitrary objects, not a column read.', + }, }; function collectSourceFiles(): string[] { @@ -265,28 +235,55 @@ describe('column identity dual-read ratchet (#3104)', () => { 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('holds the family at zero', () => { + // This is the number the battle was about, and it is done. The remaining + // verdicts are inventory, not worklist: `two-layer` and `form-cluster` are + // settled decisions, and `unrelated` is scanner residue recorded so it + // cannot hide a real one. + expect(sum((e) => e.verdict === 'column-identity')).toBe(0); + expect(sum(() => true)).toBe(12); }); - it('records a precedence for every read in the family', () => { + it('records a precedence for every read left 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), + it('routes every converged surface through the one reader', () => { + // The counterpart to "the family is zero": the reads did not vanish, they + // moved onto `columnIdentity`. If a surface silently dropped its identity + // resolution instead of converging it, this goes red. + const CONVERGED = [ + 'core/src/utils/expand-fields.ts', + 'plugin-list/src/ListView.tsx', + 'plugin-grid/src/ObjectGrid.tsx', + 'plugin-detail/src/RelatedList.tsx', + 'plugin-detail/src/renderers/record-details.tsx', + 'plugin-detail/src/renderers/record-related-list.tsx', + 'plugin-tree/src/ObjectTree.tsx', + ]; + const missing = CONVERGED.filter((rel) => { + const src = readFileSync(path.join(packagesRoot, rel), 'utf8'); + return !/\bcolumnIdentity\s*\(/.test(src); + }); + expect(missing).toEqual([]); + }); + + it('keeps the table-adapter key ahead of metadata identity in RelatedList', () => { + // A source-level pin, and labelled as one: RelatedList's columns can arrive + // in TanStack's shape (`accessorKey`) or ObjectStack's (`field`). The + // convergence replaced only the `field || name` tail — if `accessorKey` + // ever loses its precedence here, imported TanStack columns stop resolving + // and every cell in them goes blank. + const src = readFileSync( + path.join(packagesRoot, 'plugin-detail/src/RelatedList.tsx'), + 'utf8', ); - // When PR2 lands this becomes a single order and this assertion inverts. - expect(orders.size).toBeGreaterThan(1); + const identityReads = [...src.matchAll(/\bcolumnIdentity\s*\(/g)].length; + const adapterFirst = [...src.matchAll(/accessorKey\s*\|\|\s*columnIdentity\s*\(/g)].length; + expect(identityReads).toBeGreaterThan(0); + expect(adapterFirst).toBe(identityReads); }); }); diff --git a/packages/core/src/utils/expand-fields.ts b/packages/core/src/utils/expand-fields.ts index 7ba16841a7..6f4d61b008 100644 --- a/packages/core/src/utils/expand-fields.ts +++ b/packages/core/src/utils/expand-fields.ts @@ -6,6 +6,8 @@ * LICENSE file in the root directory of this source tree. */ +import { columnIdentity } from './column-identity.js'; + /** * Relational ("reference-bearing") field types whose stored value is a foreign * key into another object — and which therefore benefit from `$expand` so a @@ -101,12 +103,12 @@ export function buildExpandFields( if (columns && Array.isArray(columns) && columns.length > 0) { const columnFieldNames = new Set(); for (const col of columns) { - if (typeof col === 'string') { - columnFieldNames.add(col); - } else if (col && typeof col === 'object') { - const name = col.field ?? col.name ?? col.fieldName; - if (name) columnFieldNames.add(name); - } + // `columnIdentity` handles all three entry shapes (bare string, spec + // `{field}`, legacy `{name}`/`{fieldName}`) and resolves canonical-first + // — the same precedence every renderer now uses, so what gets expanded + // and what gets rendered can no longer name two different fields (#3104). + const name = columnIdentity(col); + if (name) columnFieldNames.add(name); } return referenceFieldNames.filter((f) => columnFieldNames.has(f)); } diff --git a/packages/plugin-detail/src/RelatedList.tsx b/packages/plugin-detail/src/RelatedList.tsx index 52c947fd56..a8951bd950 100644 --- a/packages/plugin-detail/src/RelatedList.tsx +++ b/packages/plugin-detail/src/RelatedList.tsx @@ -41,6 +41,7 @@ import type { LucideIcon } from 'lucide-react'; import type { DataSource, FieldMetadata } from '@object-ui/types'; import { getCellRenderer, resolveCellRendererType, RecordPickerDialog } from '@object-ui/fields'; import { + columnIdentity, compareSortValues, getSortValue, isExpandableFieldType, @@ -695,7 +696,7 @@ export const RelatedList: React.FC = ({ const filterFLS = (cols: any[]): any[] => { if (!perms?.isLoaded || !relatedObjectName) return cols; return cols.filter((c) => { - const key = c?.accessorKey || c?.field || c?.name; + const key = c?.accessorKey || columnIdentity(c); if (!key) return true; return perms.checkField(relatedObjectName, String(key), 'read'); }); @@ -703,7 +704,7 @@ export const RelatedList: React.FC = ({ const filterFK = (cols: any[]): any[] => referenceField ? cols.filter((c) => { - const key = c?.accessorKey || c?.field || c?.name; + const key = c?.accessorKey || columnIdentity(c); return key !== referenceField; }) : cols; @@ -717,7 +718,7 @@ export const RelatedList: React.FC = ({ const pruneEmpty = (cols: any[]): any[] => { if (!relatedData.length) return cols; return cols.filter((c) => { - const key = c?.accessorKey || c?.field || c?.name; + const key = c?.accessorKey || columnIdentity(c); if (!key) return true; return relatedData.some((r) => !isValueEmpty(r?.[key])); }); @@ -768,7 +769,7 @@ export const RelatedList: React.FC = ({ if (typeof c !== 'string') { // Object column: attach a cell renderer when it lacks one and we can // resolve the field def — preserves any author-supplied cell/render. - const key = c?.accessorKey || c?.field || c?.name; + const key = c?.accessorKey || columnIdentity(c); if (c && !c.cell && !c.render && key) { const def = (objectSchema?.fields as any)?.[key]; const cell = def ? makeCell(String(key), def) : undefined; @@ -895,7 +896,7 @@ export const RelatedList: React.FC = ({ const sortableColumns = React.useMemo(() => { if (!windowed) return effectiveColumns; return effectiveColumns.map((col: any) => { - const field = col.accessorKey || col.field || col.name; + const field = col.accessorKey || columnIdentity(col); return field && isExpandableFieldType((objectSchema?.fields as any)?.[field]) ? { ...col, sortable: false } : col; @@ -921,10 +922,10 @@ export const RelatedList: React.FC = ({ // duplicate renderer. Falls back to the data-table on desktop and // when explicit `type='list'` is requested (legacy path). if (isMobile && (type === 'grid' || type === 'table')) { - const titleField = effectiveColumns[0]?.accessorKey || effectiveColumns[0]?.field || effectiveColumns[0]?.name; + const titleField = effectiveColumns[0]?.accessorKey || columnIdentity(effectiveColumns[0]); const visibleFields = effectiveColumns .slice(1, 4) - .map((c: any) => c.accessorKey || c.field || c.name) + .map((c: any) => c.accessorKey || columnIdentity(c)) .filter(Boolean); return { type: 'object-gallery', @@ -1104,7 +1105,7 @@ export const RelatedList: React.FC = ({ {sortable && !hasSortableHeaders && effectiveColumns && effectiveColumns.length > 0 && relatedData.length > 0 && (
{effectiveColumns.map((col: any) => { - const field = col.accessorKey || col.field || col.name; + const field = col.accessorKey || columnIdentity(col); if (!field) return null; // A windowed sort goes out as a server `$orderby` on the flat // field name, so a relational column would order the collection by diff --git a/packages/plugin-detail/src/renderers/record-details.tsx b/packages/plugin-detail/src/renderers/record-details.tsx index 77e8ecbe5b..05a818020c 100644 --- a/packages/plugin-detail/src/renderers/record-details.tsx +++ b/packages/plugin-detail/src/renderers/record-details.tsx @@ -15,15 +15,11 @@ import { useRecordContext, useHighlightFieldNames, useSafeFieldLabel } from '@ob import { useFieldPermissions, usePermissions } from '@object-ui/permissions'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; import type { RecordDetailsComponentProps } from '@object-ui/types'; -import { isObjectInlineEditable } from '@object-ui/core'; +import { columnIdentity, isObjectInlineEditable } from '@object-ui/core'; import { DetailView } from '../DetailView'; /** Normalize a field entry (string | {field} | {name}) to its machine name. */ -const fieldName = (entry: any): string | null => { - if (typeof entry === 'string') return entry; - if (entry && typeof entry === 'object') return entry.field || entry.name || null; - return null; -}; +const fieldName = (entry: any): string | null => columnIdentity(entry) ?? null; const splitDesigner = (props: Record) => { const { 'data-obj-id': id, 'data-obj-type': type, style, ...rest } = props || {}; diff --git a/packages/plugin-detail/src/renderers/record-related-list.tsx b/packages/plugin-detail/src/renderers/record-related-list.tsx index cd5570b26e..7b0e2cc572 100644 --- a/packages/plugin-detail/src/renderers/record-related-list.tsx +++ b/packages/plugin-detail/src/renderers/record-related-list.tsx @@ -16,15 +16,17 @@ import { useRecordContext, useSafeFieldLabel, useRelatedRecordActions } from '@o import { useFieldPermissions, usePermissions } from '@object-ui/permissions'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; import { humanizeLabel } from '@object-ui/fields'; +import { columnIdentity } from '@object-ui/core'; import type { RecordRelatedListComponentProps } from '@object-ui/types'; import { RelatedList } from '../RelatedList'; -/** Normalize a column entry (string | {field} | {name} | {key}) to its name. */ -const colName = (entry: any): string | null => { - if (typeof entry === 'string') return entry; - if (entry && typeof entry === 'object') return entry.field || entry.name || entry.key || null; - return null; -}; +/** + * Normalize a column entry (string | {field} | {name} | {key}) to its name. + * `key` is kept as a tail fallback rather than folded into `columnIdentity`: + * it is a generic entry key, not ObjectStack metadata identity (#3104). + */ +const colName = (entry: any): string | null => + columnIdentity(entry) || (entry && typeof entry === 'object' ? entry.key : null) || null; /** Extract a record's primary key, tolerating the `id` / `_id` split. */ const rowId = (row: any): string | number | null => row?.id ?? row?._id ?? null; diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 2988082443..fd4541f26f 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -36,7 +36,7 @@ import { RefreshIndicator, } from '@object-ui/components'; import { usePullToRefresh } from '@object-ui/mobile'; -import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, isExpandableFieldType } from '@object-ui/core'; +import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, columnIdentity, isExpandableFieldType } from '@object-ui/core'; import { usePermissions } from '@object-ui/permissions'; import { ChevronRight, ChevronDown, ChevronLeft, ChevronsLeft, ChevronsRight, Download, Rows2, Rows3, Rows4, AlignJustify, Type, Hash, Calendar, CheckSquare, User, Tag, Clock, Loader2 } from 'lucide-react'; import { useRowColor } from './useRowColor'; @@ -542,13 +542,19 @@ export const ObjectGrid: React.FC = ({ // Always include 'id' so row click / navigation handlers can resolve // the record key — without it `record.id` is undefined and the // primary-field link silently no-ops. + // Both halves used to resolve identity differently — the probe was + // name-first while the projection below read `c.field` alone — so a + // legacy `{name}` column was projected as `undefined` while the + // probe happily saw its name (#3104). One reader now, both halves. const ensureId = (list: any[]): any[] => { - const names = list.map((f: any) => typeof f === 'string' ? f : (f?.name || f?.field)); + const names = list.map((f: any) => columnIdentity(f)); return names.includes('id') ? list : ['id', ...list]; }; if (schemaFields) return ensureId(schemaFields as any[]); if (schemaColumns && Array.isArray(schemaColumns)) { - const fields = schemaColumns.map((c: any) => typeof c === 'string' ? c : c.field); + const fields = schemaColumns + .map((c: any) => columnIdentity(c)) + .filter((v): v is string => !!v); return ensureId(fields); } return undefined; diff --git a/packages/plugin-grid/src/__tests__/columnIdentity.test.tsx b/packages/plugin-grid/src/__tests__/columnIdentity.test.tsx new file mode 100644 index 0000000000..a4abca2d56 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/columnIdentity.test.tsx @@ -0,0 +1,88 @@ +/** + * 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. + * + * ObjectGrid column identity (objectui#3104 PR2). + * + * ObjectGrid is NOT downstream of `normalizeListViewSchema` when it is rendered + * on its own — a page can author an `object-grid` node directly, and then no + * ingestion fold has run over its `columns`. So the chokepoint from PR1 does + * not cover this surface; converging the reads onto `columnIdentity()` is what + * does. + * + * The bug: `getSelectFields` resolved `c.field` alone while the `ensureId` + * probe one line above resolved `f?.name || f?.field`. A legacy `{ name }` + * column was therefore projected as `undefined` — it reached `$select` as a + * hole, so the server never returned the field and every cell in that column + * came back empty. + */ + +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render } from '@testing-library/react'; +import React from 'react'; + +import { ObjectGrid } from '../ObjectGrid'; +import { ActionProvider } from '@object-ui/react'; + +const makeDataSource = () => ({ + find: vi.fn().mockResolvedValue({ value: [], '@odata.count': 0 }), + 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 selectFor = async (columns: unknown[]) => { + const ds = makeDataSource(); + const schema: any = { + type: 'object-grid', + objectName: 'contacts', + columns, + }; + render( + + + , + ); + await vi.waitFor(() => expect(ds.find).toHaveBeenCalled()); + return (ds.find.mock.calls.at(-1)?.[1]?.$select ?? []) as string[]; +}; + +describe('ObjectGrid — $select resolves one column identity (#3104)', () => { + beforeEach(() => vi.clearAllMocks()); + + it('projects a spec-canonical `field` column', async () => { + expect(await selectFor([{ field: 'account' }])).toContain('account'); + }); + + it('projects a legacy `name`-only column instead of a hole', async () => { + // Before the convergence this landed in `$select` as `undefined`. + const select = await selectFor([{ name: 'account' }]); + expect(select).toContain('account'); + expect(select).not.toContain(undefined); + }); + + it('projects the canonical key when a column carries both', async () => { + const select = await selectFor([{ field: 'account', name: 'account_name' }]); + expect(select).toContain('account'); + expect(select).not.toContain('account_name'); + }); + + it('still projects bare string columns', async () => { + expect(await selectFor(['account'])).toContain('account'); + }); + + it('always includes `id` so row navigation can resolve the record key', async () => { + expect(await selectFor([{ name: 'account' }])).toContain('id'); + }); +}); diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index 15fba7c4e4..60de744656 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -19,7 +19,7 @@ import { useDensityMode } from '@object-ui/react'; import type { ListViewSchema } from '@object-ui/types'; import { detectStatusField } from '@object-ui/types'; import { usePullToRefresh } from '@object-ui/mobile'; -import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveCrudAffordances, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes, EXPANDABLE_FIELD_TYPES } from '@object-ui/core'; +import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, resolveCrudAffordances, normalizeListViewSchema, rowHeightToDensityMode, mergeFilterNodes, columnIdentity, EXPANDABLE_FIELD_TYPES } from '@object-ui/core'; import { useObjectTranslation, useObjectLabel, useSafeFieldLabel, createSafeTranslation } from '@object-ui/i18n'; import { usePermissions } from '@object-ui/permissions'; @@ -954,7 +954,7 @@ export const ListView = React.forwardRef(({ const expandFields = React.useMemo(() => { const baseColumns = Array.isArray(schema.columns) ? (schema.columns as any[]) - .map((f) => (typeof f === 'string' ? f : f?.field)) + .map((f) => columnIdentity(f)) .filter((v): v is string => typeof v === 'string' && v.length > 0) : []; const collected = new Set(baseColumns); @@ -1107,7 +1107,7 @@ export const ListView = React.forwardRef(({ const selectFields = (() => { const rawCols = Array.isArray(schema.columns) ? (schema.columns as any[]) - .map(f => (typeof f === 'string' ? f : f?.field)) + .map(f => columnIdentity(f)) .filter((v): v is string => typeof v === 'string' && v.length > 0) : []; const cols = (perms?.isLoaded && schema.objectName) @@ -1422,7 +1422,7 @@ export const ListView = React.forwardRef(({ // FLS: drop columns the current user cannot read. if (perms?.isLoaded && schema.objectName) { fields = fields.filter((f: any) => { - const fieldName = typeof f === 'string' ? f : (f?.name || f?.fieldName || f?.field); + const fieldName = columnIdentity(f); if (!fieldName) return true; return perms.checkField(schema.objectName!, fieldName, 'read'); }); @@ -1431,19 +1431,17 @@ export const ListView = React.forwardRef(({ // Remove hidden fields if (hiddenFields.size > 0) { fields = fields.filter((f: any) => { - const fieldName = typeof f === 'string' ? f : (f?.name || f?.fieldName || f?.field); + const fieldName = columnIdentity(f); return fieldName != null && !hiddenFields.has(fieldName); }); } - + // Apply field order if (schema.fieldOrder && schema.fieldOrder.length > 0) { const orderMap = new Map(schema.fieldOrder.map((f: any, i: number) => [f as string, i])); fields = [...fields].sort((a: any, b: any) => { - const nameA = typeof a === 'string' ? a : (a?.name || a?.fieldName || a?.field); - const nameB = typeof b === 'string' ? b : (b?.name || b?.fieldName || b?.field); - const orderA: number = orderMap.get(nameA) ?? Infinity; - const orderB: number = orderMap.get(nameB) ?? Infinity; + const orderA: number = orderMap.get(columnIdentity(a) as string) ?? Infinity; + const orderB: number = orderMap.get(columnIdentity(b) as string) ?? Infinity; return orderA - orderB; }); } @@ -1710,20 +1708,25 @@ export const ListView = React.forwardRef(({ if (!objectDef?.fields) { // Fallback to the declared columns if objectDef not loaded yet - fields = (Array.isArray(schema.columns) ? (schema.columns as any[]) : []).map((f: any) => { - if (typeof f === 'string') return { value: f, label: f, type: 'text' }; - // `field` is the spec's ListColumn key; `name`/`fieldName` are the - // shapes hosts pass through from stored metadata. - const fieldName = f.name || f.fieldName || f.field; - return { + fields = (Array.isArray(schema.columns) ? (schema.columns as any[]) : []) + .flatMap((f: any) => { + if (typeof f === 'string') return [{ value: f, label: f, type: 'text' }]; + // A column with no resolvable identity cannot be filtered or sorted + // on — it used to become an option keyed `undefined`. Drop it. + const fieldName = columnIdentity(f); + if (!fieldName) return []; + return [{ value: fieldName, - label: tFieldLabel(fieldName, f.label || f.name), + // The label falls back to the identity, not to the raw `name`: + // after #3104 a legacy `name` mirrors the identity anyway, and + // reading it here would resurrect the second spelling. + label: tFieldLabel(fieldName, f.label || fieldName), type: f.type || 'text', options: buildOptions(fieldName, f.options), referenceTo: f.reference_to || f.reference, displayField: f.display_field || f.reference_field, idField: f.id_field, - }; + }]; }); } else { fields = Object.entries(objectDef.fields).map(([key, field]: [string, any]) => ({ @@ -1843,8 +1846,8 @@ export const ListView = React.forwardRef(({ && (exportConfig as any)?.streaming !== false; if (serverEligible) { const fields = effectiveFields - .map((f: any) => typeof f === 'string' ? f : (f.name || f.fieldName || f.field)) - .filter(Boolean); + .map((f: any) => columnIdentity(f)) + .filter(Boolean) as string[]; // The same three filter sources as the data fetch, from the same function. const finalFilter = buildEffectiveFilter(schema.filter, currentFilters, userFilterConditions); @@ -1903,7 +1906,7 @@ export const ListView = React.forwardRef(({ const exportData = maxRecords > 0 ? data.slice(0, maxRecords) : data; if (format === 'csv') { - const fields = effectiveFields.map((f: any) => typeof f === 'string' ? f : (f.name || f.fieldName || f.field)); + const fields = effectiveFields.map((f: any) => columnIdentity(f)).filter(Boolean) as string[]; const rows: string[] = []; if (includeHeaders) { rows.push(fields.join(',')); @@ -1953,13 +1956,18 @@ export const ListView = React.forwardRef(({ // All available fields for hide/show (with i18n) const allFields = React.useMemo(() => { - return (Array.isArray(schema.columns) ? (schema.columns as any[]) : []).map((f: any) => { + return (Array.isArray(schema.columns) ? (schema.columns as any[]) : []).flatMap((f: any) => { if (typeof f === 'string') { - return { name: f, label: tFieldLabel(f, f) }; + return [{ name: f, label: tFieldLabel(f, f) }]; } - const name = f.name || f.fieldName || f.field; - const rawLabel = f.label || f.name || f.field; - return { name, label: tFieldLabel(name, rawLabel) }; + // `name` here is this popover's OWN key (it drives `hiddenFields`), which + // is why it keeps that shape — but the value is the column identity, so + // hiding a column and projecting it now agree (#3104). + const name = columnIdentity(f); + // No resolvable identity → nothing to hide or show; it used to render a + // checkbox keyed `undefined` that could never match a column. + if (!name) return []; + return [{ name, label: tFieldLabel(name, f.label || name) }]; }); }, [schema.columns, tFieldLabel]); diff --git a/packages/plugin-tree/src/ObjectTree.tsx b/packages/plugin-tree/src/ObjectTree.tsx index c451262f1a..333f87e071 100644 --- a/packages/plugin-tree/src/ObjectTree.tsx +++ b/packages/plugin-tree/src/ObjectTree.tsx @@ -23,7 +23,7 @@ import React, { useEffect, useMemo, useState } from 'react'; import type { DataSource, ViewData } from '@object-ui/types'; import { useNavigationOverlay, useSafeFieldLabel } from '@object-ui/react'; import { NavigationOverlay, cn } from '@object-ui/components'; -import { extractRecords, buildExpandFields } from '@object-ui/core'; +import { extractRecords, buildExpandFields, columnIdentity } from '@object-ui/core'; import { ChevronRight, ChevronDown } from 'lucide-react'; export interface ObjectTreeProps { @@ -65,9 +65,9 @@ function getDataConfig(schema: any): ViewData | null { * resilient regardless of caller. */ function fieldKey(f: any): string | undefined { - if (typeof f === 'string') return f; - if (f && typeof f === 'object') return f.name || f.fieldName || f.field || f.key; - return undefined; + // `key` stays a tail fallback — it is a generic entry key, not ObjectStack + // metadata identity, so it is not part of `columnIdentity` (#3104). + return columnIdentity(f) || (f && typeof f === 'object' ? f.key : undefined) || undefined; } function getTreeConfig(schema: any): TreeConfig {