From 76a0a4a699dc7edac439f85e952ea27de8eb2dd1 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 17:03:00 +0000 Subject: [PATCH] fix(plugin-grid): give ObjectGrid one owner for "declared type + format hint -> renderer" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@object-ui/fields` publishes a two-step resolve — `getCellRenderer(` applied to `resolveCellRendererType(...)` — because a textual base type carrying a `format` hint maps to a richer renderer than its declared type does. ObjectGrid spelled that resolve six times with three conventions: one two-step, four raw declared types, and one fixed registry key. On the four raw sites a `text` + `format: 'phone'` column silently resolved to `TextCellRenderer`, which reads only `value` — the hint was dropped with no error, no warning and no `tel:` link, in the flagship grid. Patching four sites to match one would have left the divergence in place, so the resolution itself moves into `./cellRendererResolution`: five sites take its field entry point, the compound-cell prefix badge takes its named fixed-key entry point (a different contract, documented rather than left silent), and ObjectGrid no longer imports either published resolver. The module names the two answers the word "resolve" used to collide on — `declaredType` (which type the field has) and `rendererType` (which renderer that type maps to). A local called `resolvedType` holding only the first is what made four broken sites read as correct. Alignment and the header type icon now follow `rendererType` on every path, as the object-columns path already did; the type forwarded to the inline editor stays `declaredType`, so a hinted text column still edits as text. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01MPaVWWMuWeT5LgB1qoXjVB --- ...8920-grid-format-hint-shared-resolution.md | 9 + packages/plugin-grid/src/ObjectGrid.tsx | 130 ++++++--- ...ellRendererResolutionBoundary-8920.test.ts | 165 +++++++++++ .../formatHintedColumnRenderer-8920.test.tsx | 264 ++++++++++++++++++ .../plugin-grid/src/cellRendererResolution.ts | 146 ++++++++++ 5 files changed, 673 insertions(+), 41 deletions(-) create mode 100644 .changeset/8920-grid-format-hint-shared-resolution.md create mode 100644 packages/plugin-grid/src/__tests__/cellRendererResolutionBoundary-8920.test.ts create mode 100644 packages/plugin-grid/src/__tests__/formatHintedColumnRenderer-8920.test.tsx create mode 100644 packages/plugin-grid/src/cellRendererResolution.ts diff --git a/.changeset/8920-grid-format-hint-shared-resolution.md b/.changeset/8920-grid-format-hint-shared-resolution.md new file mode 100644 index 0000000000..59dd9f6421 --- /dev/null +++ b/.changeset/8920-grid-format-hint-shared-resolution.md @@ -0,0 +1,9 @@ +--- +"@object-ui/plugin-grid": patch +--- + +fix(plugin-grid): honour a column's `format` hint on every `ObjectGrid` render path + +`@object-ui/fields` publishes a two-step resolve — `getCellRenderer(resolveCellRendererType(field))` — because a textual base type carrying a `format` hint (`phone`, `email`, `url`, `currency`, `percent`) maps to a richer renderer than its declared type does. `ObjectGrid` spelled that resolve six times with three conventions, and four of them passed the declared type straight to `getCellRenderer`. On those paths a `text` + `format: 'phone'` column silently fell back to plain truncated text: no error, no warning, no `tel:` link. + +All six sites now route through one module that owns "declared type + format hint -> renderer", so a hinted column renders the same way whether its columns were declared as objects, as strings, derived from an authored `fields` projection, derived from the object schema, or read in the record-detail panel. Numeric alignment and the header type icon follow the same resolved renderer key on every path, while the type forwarded to the inline editor stays the declared one — a hinted text column still edits as text. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 064b293dcd..55031aa3d0 100644 --- a/packages/plugin-grid/src/ObjectGrid.tsx +++ b/packages/plugin-grid/src/ObjectGrid.tsx @@ -27,7 +27,13 @@ import { isSystemManagedField, normalizeTableColumnType } from '@object-ui/types import type { I18nLabel } from '@objectstack/spec/ui'; import { SchemaRenderer, useDataScope, useNavigationOverlay, useAction, useSafeFieldLabel, usePredicateScope, useRelatedRecordActions } from '@object-ui/react'; import { createSafeTranslation } from '@object-ui/i18n'; -import { getCellRenderer, resolveCellRendererType, formatCurrency, formatCompactCurrency, formatDate, formatPercent, humanizeLabel, getBadgeColorClasses, getBadgeHexAppearance, FieldEditWidget, hasFieldEditWidget, DISCRETE_EDIT_TYPES, coerceToSafeValue } from '@object-ui/fields'; +// objectui#8920 — the grid reaches a cell renderer through THIS module and +// nowhere else. `getCellRenderer` / `resolveCellRendererType` are deliberately +// NOT imported here: six sites spelling the resolve three different ways is +// what dropped a `format`-hinted column's renderer, and one shared owner is +// what stops a seventh site picking a convention of its own. +import { resolveGridCellRendering, gridCellRendererForFixedKey, BADGE_PREFIX_RENDERER_KEY } from './cellRendererResolution'; +import { formatCurrency, formatCompactCurrency, formatDate, formatPercent, humanizeLabel, getBadgeColorClasses, getBadgeHexAppearance, FieldEditWidget, hasFieldEditWidget, DISCRETE_EDIT_TYPES, coerceToSafeValue } from '@object-ui/fields'; import { useLocalization, useDisplayLocale, resolveFieldCurrency } from '@object-ui/i18n'; import { stateMachineNextValues, isFieldInlineEditable } from './inline-edit-options'; import { @@ -2483,7 +2489,8 @@ export const ObjectGrid: React.FC = ({ // Type-based cell renderer: explicit col type > objectDef type > heuristic inference. // Format hints (e.g. `text` + `format: 'phone'`) promote to the - // richer renderer (PhoneCellRenderer) via resolveCellRendererType. + // richer renderer (PhoneCellRenderer) via the grid's one shared + // resolve, `./cellRendererResolution` (objectui#8920). const objectDefField = objectSchema?.fields?.[col.field]; // ⭐ ANNOTATED, and the annotation is load-bearing (objectui#6004). // `objectSchema` is `useState`, so `objectDefField?.type` is @@ -2497,10 +2504,11 @@ export const ObjectGrid: React.FC = ({ // object-field fallback below is now the only road, which is what // every measured author already used. const formatHint = objectDefField?.format; - const inferredType: string | null = baseInferredType - ? resolveCellRendererType({ type: baseInferredType, format: formatHint }) - : null; - const CellRenderer = inferredType ? getCellRenderer(inferredType) : null; + // Both answers, from the one shared resolve (objectui#8920): + // `baseInferredType` is the DECLARED type the inline editor reads, + // `inferredType` the renderer key it promotes to. + const { rendererType: inferredType, Renderer } = resolveGridCellRendering({ type: baseInferredType, format: formatHint }); + const CellRenderer = inferredType ? Renderer : null; // Build field metadata for cell renderers with objectDef enrichment const fieldMeta: Record = { name: col.field, type: inferredType || 'text' }; @@ -2636,12 +2644,17 @@ export const ObjectGrid: React.FC = ({ const prefixConfig = col.prefix; if (prefixConfig?.field) { const baseCellRenderer = cellRenderer; - const PrefixRenderer = prefixConfig.type === 'badge' ? getCellRenderer('select') : null; + // ⭐ The one site whose contract is NOT "declared type + format + // hint": a FIXED registry key for the badge, owned by this + // component rather than by the prefixed field (objectui#8920). + // Named and routed through the same module so it reads as the + // declared exception it is, not as a fifth silent convention. + const PrefixRenderer = prefixConfig.type === 'badge' ? gridCellRendererForFixedKey(BADGE_PREFIX_RENDERER_KEY) : null; cellRenderer = (value: any, row: any) => { const prefixValue = row[prefixConfig.field]; const prefixEl = prefixValue != null && prefixValue !== '' ? PrefixRenderer - ? + ? : {String(prefixValue)} : null; return ( @@ -2698,16 +2711,29 @@ export const ObjectGrid: React.FC = ({ const rawHeader = rawFieldLabel || fieldName.charAt(0).toUpperCase() + fieldName.slice(1).replace(/_/g, ' '); const header = schema.objectName ? resolveFieldLabel(schema.objectName, fieldName, rawHeader) : rawHeader; - // Resolve type: objectDef type > heuristic inference (consistent with ListColumn path) - // Annotated for the same reason as path A's `baseInferredType` - // above: `fieldDef` is `any`, and an `any` reaching the `...(resolvedType - // && { type: resolvedType })` spread below collapses the emit literal - // to `any` (objectui#6004). - const resolvedType: string | null = fieldDef?.type || inferColumnType({ field: fieldName }) || null; - const CellRenderer = resolvedType ? getCellRenderer(resolvedType) : null; + // TWO resolves, two names (objectui#8920). "Resolve type" here means + // objectDef type > heuristic inference — WHICH TYPE THE FIELD HAS, + // and that is `declaredType`. The published second step, WHICH + // RENDERER THE TYPE MAPS TO, is `rendererType`; this path used to + // skip it entirely, so a `text` + `format: 'phone'` column got + // `TextCellRenderer` and the hint vanished with no diagnostic. + // A local called `resolvedType` holding only the FIRST answer is the + // trap that hid that for four of the six sites. + // + // The `string | null` annotation path A's `baseInferredType` needs + // (objectui#6004: `fieldDef` is `any`, and an `any` reaching the + // `...(declaredType && { type: declaredType })` spread below + // collapses the emit literal) now lives on `GridCellRendering`'s + // members — it moved into the helper's return type, it did not go + // away. + const { declaredType, rendererType, Renderer } = resolveGridCellRendering({ + type: fieldDef?.type || inferColumnType({ field: fieldName }), + format: fieldDef?.format, + }); + const CellRenderer = rendererType ? Renderer : null; // Build field metadata with objectDef enrichment - const fieldMeta: Record = { name: fieldName, type: resolvedType || 'text' }; + const fieldMeta: Record = { name: fieldName, type: rendererType || 'text' }; if (fieldDef) { if (fieldDef.label) fieldMeta.label = fieldDef.label; if (fieldDef.currency) fieldMeta.currency = fieldDef.currency; @@ -2721,16 +2747,16 @@ export const ObjectGrid: React.FC = ({ // reads the schema def directly, see `renderCellEditor` (objectui#7154). applyRelationalMeta(fieldMeta, fieldDef as any); // Auto-generate select options from data when no options defined - if (resolvedType === 'select' && !fieldMeta.options) { + if (rendererType === 'select' && !fieldMeta.options) { const uniqueValues = Array.from(new Set(data.map(row => row[fieldName]).filter(Boolean))); fieldMeta.options = uniqueValues.map((v: any) => ({ value: v, label: humanizeLabel(String(v)) })); } - if ((resolvedType === 'select' || resolvedType === 'status') && (fieldDef as any)?.appearance != null) { + if ((rendererType === 'select' || rendererType === 'status') && (fieldDef as any)?.appearance != null) { fieldMeta.appearance = (fieldDef as any).appearance; } const numericTypes = ['number', 'currency', 'percent']; - const inferredAlign = resolvedType && numericTypes.includes(resolvedType) ? 'right' as const : undefined; + const inferredAlign = rendererType && numericTypes.includes(rendererType) ? 'right' as const : undefined; // Auto-link primary field (first column) to record detail const isPrimaryField = colIndex === 0; @@ -2768,9 +2794,12 @@ export const ObjectGrid: React.FC = ({ return { header, accessorKey: fieldName, - // Forward the resolved field type for the type-aware inline editor. - ...(resolvedType && { type: resolvedType }), - ...(schema.showColumnTypeIcons && resolvedType && { headerIcon: getTypeIcon(resolvedType) }), + // Forward the DECLARED type for the type-aware inline editor — the + // renderer key would make a `format`-hinted text column edit as a + // phone/currency control it never declared. Path A forwards + // `baseInferredType` for exactly this reason (objectui#8920). + ...(declaredType && { type: declaredType }), + ...(schema.showColumnTypeIcons && rendererType && { headerIcon: getTypeIcon(rendererType) }), ...(inferredAlign && { align: inferredAlign }), ...(cellRenderer && { cell: cellRenderer }), sortable: fieldDef?.sortable !== false, @@ -2859,13 +2888,18 @@ export const ObjectGrid: React.FC = ({ }); return fieldsToShow.map((fieldName) => { const fieldDef = objectSchema?.fields?.[fieldName]; - // Annotated for the same reason as paths A and B (objectui#6004). - const resolvedType: string | null = fieldDef?.type || inferColumnType({ field: fieldName }) || null; - const CellRenderer = resolvedType ? getCellRenderer(resolvedType) : null; + // The same two resolves as path B, through the same shared owner + // (objectui#8920) — and the same objectui#6004 annotation, now + // carried by `GridCellRendering`'s `string | null` members. + const { declaredType, rendererType, Renderer } = resolveGridCellRendering({ + type: fieldDef?.type || inferColumnType({ field: fieldName }), + format: fieldDef?.format, + }); + const CellRenderer = rendererType ? Renderer : null; const header = fieldDef?.label || fieldName.charAt(0).toUpperCase() + fieldName.slice(1).replace(/_/g, ' '); // Build field metadata with objectDef enrichment - const fieldMeta: Record = { name: fieldName, type: resolvedType || 'text' }; + const fieldMeta: Record = { name: fieldName, type: rendererType || 'text' }; if (fieldDef) { if (fieldDef.label) fieldMeta.label = fieldDef.label; if (fieldDef.currency) fieldMeta.currency = fieldDef.currency; @@ -2879,23 +2913,26 @@ export const ObjectGrid: React.FC = ({ // reads the schema def directly, see `renderCellEditor` (objectui#7154). applyRelationalMeta(fieldMeta, fieldDef as any); // Auto-generate select options from data when no options defined - if (resolvedType === 'select' && !fieldMeta.options) { + if (rendererType === 'select' && !fieldMeta.options) { const uniqueValues = Array.from(new Set(data.map(row => row[fieldName]).filter(Boolean))); fieldMeta.options = uniqueValues.map((v: any) => ({ value: v, label: humanizeLabel(String(v)) })); } - if ((resolvedType === 'select' || resolvedType === 'status') && (fieldDef as any)?.appearance != null) { + if ((rendererType === 'select' || rendererType === 'status') && (fieldDef as any)?.appearance != null) { fieldMeta.appearance = (fieldDef as any).appearance; } const numericTypes = ['number', 'currency', 'percent']; - const inferredAlign = resolvedType && numericTypes.includes(resolvedType) ? 'right' as const : undefined; + const inferredAlign = rendererType && numericTypes.includes(rendererType) ? 'right' as const : undefined; return { header, accessorKey: fieldName, - // Forward the resolved field type for the type-aware inline editor. - ...(resolvedType && { type: resolvedType }), - ...(schema.showColumnTypeIcons && resolvedType && { headerIcon: getTypeIcon(resolvedType) }), + // Forward the DECLARED type for the type-aware inline editor — the + // renderer key would make a `format`-hinted text column edit as a + // phone/currency control it never declared. Path A forwards + // `baseInferredType` for exactly this reason (objectui#8920). + ...(declaredType && { type: declaredType }), + ...(schema.showColumnTypeIcons && rendererType && { headerIcon: getTypeIcon(rendererType) }), ...(inferredAlign && { align: inferredAlign }), ...(CellRenderer && { cell: (value: any) => }), sortable: fieldDef?.sortable !== false, @@ -2980,9 +3017,12 @@ export const ObjectGrid: React.FC = ({ && !perms.checkField(schema.objectName, fieldName, 'read')) return; // Annotated for the same reason as paths A-C (objectui#6004): `field` is - // `any`, so this value has to be named before it reaches a spread below. - const fieldType: string | undefined = field.type; - const CellRenderer = getCellRenderer(field.type); + // `any`, so these values have to be named before they reach a spread + // below — the naming now lives on `GridCellRendering`'s `string | null` + // members. `fieldType` is the DECLARED type the emit forwards to the + // inline editor; the renderer comes from the `format`-promoted key, which + // this path used to skip (objectui#8920). + const { declaredType: fieldType, rendererType, Renderer: CellRenderer } = resolveGridCellRendering(field); const numericTypes = ['number', 'currency', 'percent']; const translatedField = field.options ? { ...field, options: translateOptions(schema.objectName, fieldName, field.options) } @@ -2993,7 +3033,9 @@ export const ObjectGrid: React.FC = ({ accessorKey: fieldName, // Forward the field type for the type-aware inline editor. ...(fieldType && { type: fieldType }), - ...(numericTypes.includes(field.type) && { align: 'right' as const }), + // Aligned on the RENDERER key, like paths A-C: a `text` column with + // `format: 'currency'` renders as currency, so it aligns as currency. + ...(!!rendererType && numericTypes.includes(rendererType) && { align: 'right' as const }), cell: (value: any) => , sortable: field.sortable !== false, }); @@ -4433,11 +4475,17 @@ export const ObjectGrid: React.FC = ({ // Use objectSchema field type for type-aware rendering const fieldDef = objectSchema?.fields?.[key]; - if (fieldDef?.type) { - const CellRenderer = getCellRenderer(fieldDef.type); - if (CellRenderer) { - return ; - } + // Through the shared resolve, so the panel honours a `format` hint the + // same way the row above it does (objectui#8920). `rendererType` is null + // exactly when the key has no declared type, which is the guard this + // used to spell as `if (fieldDef?.type)`. The old inner + // `if (CellRenderer)` was DEAD — `getCellRenderer` ends in + // `standardMap[key] || TextCellRenderer` and never returns anything + // falsy — and `GridCellRendering.Renderer` states that totality in the + // type, so the dead branch goes with it. + const { rendererType, Renderer } = resolveGridCellRendering(fieldDef); + if (rendererType) { + return ; } // Fallback: infer from value and key name diff --git a/packages/plugin-grid/src/__tests__/cellRendererResolutionBoundary-8920.test.ts b/packages/plugin-grid/src/__tests__/cellRendererResolutionBoundary-8920.test.ts new file mode 100644 index 0000000000..4fcb66cc32 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/cellRendererResolutionBoundary-8920.test.ts @@ -0,0 +1,165 @@ +/** + * 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. + */ + +/** + * objectui#8920 — `plugin-grid` reaches a cell renderer through ONE module. + * + * ## Why a source scan rather than a rendering test + * + * `formatHintedColumnRenderer-8920.test.tsx` renders every path and proves the + * hint is honoured today. It cannot see the property this file guards: a + * SEVENTH call site added tomorrow, spelled `getCellRenderer(field.type)`, + * renders perfectly for every type that carries no `format` hint — which is + * most of them — and nothing observable distinguishes it from a correct one + * until an author writes the hint. That is exactly how four sites survived: the + * defect is *which resolve was spelled*, and the source is where that is + * measurable. + * + * The bound is therefore structural: `packages/plugin-grid/src` calls + * `getCellRenderer` / `resolveCellRendererType` from `cellRendererResolution.ts` + * and nowhere else. Adding a site is still easy; adding one that picks its own + * convention is not. + * + * ## Anti-vacuity — the expected answer OUTSIDE the helper is ZERO + * + * A zero and a broken scanner render identically, so three controls guard it, + * each able to fail on its own: + * + * 1. the population is enumerated and asserted non-trivial BY COUNT, and the + * helper module and `ObjectGrid.tsx` are both asserted present in it — + * a scan over an empty or wrong directory fails here, loudly; + * 2. the matcher is proved able to find a direct call on a synthetic input + * whose answer is known, in both directions; + * 3. the matcher is proved able to find direct calls in a REAL file at real + * scale — necessarily `cellRendererResolution.ts`, since it is now the + * only place they may appear. ⛔ If that module ever stops calling them, + * do not delete this control: the bound it guards has gone with it and + * this whole file needs rewriting, which is what its failure will say. + */ + +import { describe, it, expect } from 'vitest'; +import { readFileSync, readdirSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const here = path.dirname(fileURLToPath(import.meta.url)); +// packages/plugin-grid/src/__tests__ -> packages/plugin-grid/src +const SRC = path.resolve(here, '..'); +const HELPER = 'cellRendererResolution.ts'; +const GRID = 'ObjectGrid.tsx'; + +/** + * Every non-test source file under `packages/plugin-grid/src`, as repo-relative + * paths from `src`. Tooling directories are excluded the way this repo spells + * that exclusion everywhere else — by DIRECTORY (`__tests__`, `__mocks__`), not + * by filename pattern — plus the `*.test.*` files that sit beside their + * subjects in this package. + */ +function sourceFiles(dir = SRC, prefix = ''): string[] { + const out: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const rel = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + if (entry.name === '__tests__' || entry.name === '__mocks__' || entry.name === '__benchmarks__') continue; + out.push(...sourceFiles(path.join(dir, entry.name), rel)); + continue; + } + if (!/\.(ts|tsx)$/.test(entry.name)) continue; + if (/\.test\.tsx?$/.test(entry.name)) continue; + out.push(rel); + } + return out; +} + +/** + * The symbol immediately followed by `(` — a CALL SHAPE, wherever it appears. + * + * ⚠️ Deliberately NOT comment-aware, and the bound is stated to match: prose + * inside the guarded files may NAME either resolver but must not spell it with + * a trailing `(`. Two reasons this is the right trade rather than a shortcut. + * A comment stripper written for TypeScript has to tokenise strings, templates + * and regex literals to be correct, and every wrong version of it fails toward + * a FALSE NEGATIVE — a real call swallowed by a mis-parsed line, reported as a + * clean zero, which is precisely the reading this file exists to make + * trustworthy. And the rule it costs is one this card wants anyway: a comment + * that spells `getCellRenderer(field.type)` inside the grid is re-teaching the + * convention that dropped the hint. Name it without the parenthesis. + * + * The helper module is exempt by construction — it is scanned only by control + * 3, which reads the SET of symbols found, so its prose may quote the calls it + * is documenting. + */ +const DIRECT_CALL_RE = /\b(getCellRenderer|resolveCellRendererType)\s*\(/g; + +function directCalls(source: string): string[] { + return [...source.matchAll(DIRECT_CALL_RE)].map((m) => m[1]); +} + +const FILES = sourceFiles(); +const read = (rel: string) => readFileSync(path.join(SRC, rel), 'utf8'); + +describe('objectui#8920 — one owner for the grid\'s cell-renderer resolution', () => { + it('the population is enumerated and non-trivial (anti-vacuity control 1)', () => { + // Every "no such call" claim below is worthless if this list is empty or + // points at the wrong tree, so it is asserted first and BY COUNT. + expect(FILES.length).toBeGreaterThan(20); + expect(FILES).toContain(HELPER); + expect(FILES).toContain(GRID); + expect(read(GRID).length).toBeGreaterThan(100_000); + }); + + it('the matcher finds a direct call, and only a call (anti-vacuity control 2)', () => { + expect(directCalls('const R = getCellRenderer(t);')).toEqual(['getCellRenderer']); + expect(directCalls('resolveCellRendererType({ type, format })')).toEqual(['resolveCellRendererType']); + // A bare mention is not a call shape — this is the direction that keeps the + // bound satisfiable while the guarded source still explains itself in prose. + expect(directCalls('// getCellRenderer is deliberately not imported here')).toEqual([]); + expect(directCalls('import { getCellRenderer } from "@object-ui/fields";')).toEqual([]); + // …and the documented cost, asserted so nobody has to rediscover it from a + // confusing failure: parenthesised prose DOES count. + expect(directCalls('// was: getCellRenderer(field.type)')).toEqual(['getCellRenderer']); + }); + + it('the matcher finds real calls at real scale (anti-vacuity control 3)', () => { + // The helper is now the only place these calls may appear, so it is the + // only real-file control available — and that is not a weakness: if it + // stops calling them, the bound below has nothing left to mean. + // The SET, not the multiset: the helper's own documentation quotes the two + // calls it wraps, and counting those quotations would pin prose. + expect( + [...new Set(directCalls(read(HELPER)))].sort(), + `Anti-vacuity control 3 has lost its anchor: \`${HELPER}\` no longer calls ` + + 'either published resolver, so the matcher is not proved able to find ' + + 'anything at real scale and every "no direct call" claim in this file ' + + 'is a green no-op. ⛔ Do not delete this control — rewrite the bound ' + + 'it guards (objectui#8920).', + ).toEqual(['getCellRenderer', 'resolveCellRendererType']); + }); + + it('⭐ ObjectGrid.tsx makes no direct resolver call', () => { + // The card's own subject: six sites in this one file, three conventions. + expect( + directCalls(read(GRID)), + 'ObjectGrid.tsx called a published resolver directly. That is how a ' + + '`format`-hinted textual column lost its renderer in four of six ' + + 'sites (objectui#8920): `getCellRenderer(field.type)` type-checks and ' + + 'renders correctly for every unhinted type, so nothing else can catch ' + + 'it. Route through `./cellRendererResolution` instead.', + ).toEqual([]); + }); + + it('⭐ no file under plugin-grid/src outside the helper makes one either', () => { + const offenders = FILES.filter((f) => f !== HELPER).filter((f) => directCalls(read(f)).length > 0); + expect( + offenders, + 'These files resolve a cell renderer without going through ' + + `\`${HELPER}\`. One owner is what stops a seventh site from picking a ` + + 'convention of its own (objectui#8920).', + ).toEqual([]); + }); +}); diff --git a/packages/plugin-grid/src/__tests__/formatHintedColumnRenderer-8920.test.tsx b/packages/plugin-grid/src/__tests__/formatHintedColumnRenderer-8920.test.tsx new file mode 100644 index 0000000000..7b13667898 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/formatHintedColumnRenderer-8920.test.tsx @@ -0,0 +1,264 @@ +/** + * 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. + */ + +/** + * objectui#8920 — a `format`-hinted textual column must reach its renderer + * through EVERY path `ObjectGrid` can pick, not just the one that happened to + * apply the published two-step. + * + * ## The defect + * + * `@object-ui/fields` publishes `getCellRenderer(resolveCellRendererType(f))`. + * Skip the second step and a `text` + `format: 'phone'` column resolves to + * `TextCellRenderer`, which destructures only `value` and therefore cannot read + * `field.format`. Nothing throws, nothing warns: the cell just renders plain + * truncated text where a `tel:` anchor belongs. Six sites in `ObjectGrid.tsx` + * resolved a renderer with three conventions and only ONE of them ran the + * promotion, so which of the grid's paths honoured an author's `format` hint + * depended on how the columns had been declared. + * + * ## Why a RENDERING test, and why one case per path + * + * `getCellRenderer(field.type)` type-checks — `field.type` is a `string` and + * the parameter is a `string` — so every type-level and doc-level gate reads + * the broken sites green. **A green that cannot go red on this defect is not + * evidence.** The only oracle that can fail is rendered output: which renderer + * drew the cell, observed as whether a `tel:` anchor exists. + * + * And one case is not enough. A single case through one path proves the shared + * helper works; it says nothing about whether the divergence is gone. The + * divergence IS the defect, so the population — every path — is the subject, + * and each path gets its own case. + * + * ## The control, and why it is phone-SHAPED + * + * Every case carries an unhinted control column, `note`, whose value is a + * second, DIFFERENT phone number. So: + * + * - the hinted assertion is `a[href="tel:"]` — present; + * - the control assertion is `a[href="tel:"]` — absent, while the + * control's text still renders. + * + * A control holding a non-phone string would only show that the fix did not + * reach it. This one shows the promotion is driven by the DECLARED `format` + * and not by the value's shape, which is the other way this could have been + * "fixed" and would have been worse. + * + * ## Reaching each path (measured, not presumed) + * + * `generateColumns()` picks its source with `normalizeColumns(schema.columns)` + * first, then an inline-data branch, then the object-schema branch: + * + * A `columns` as objects — `columns: [{ field }]` + * B `columns` as strings — `columns: ['work_phone']` + * C inline-data projection — no `columns`, `fields` present, `data` inline + * (`fields` is what keeps `rowKeysWouldOutrank + * SchemaPolicy` false once the schema lands) + * D object-schema policy — no `columns`, no `fields`, rows handed down + * E record-detail panel — `renderRecordDetail` -> `renderFieldValue` + * P compound-cell prefix — `col.prefix.type === 'badge'` + * + * ## PREDICTED before running, on the pre-fix tree: 4 red, 2 green-both-sides + * + * A and P were ALREADY correct — A is the one site that ran the promotion, and + * P asks for a fixed registry key with no field type to promote. Their cases + * are PINS (must-not-change), not evidence that anything was fixed; they are + * here because a shared helper that silently changed either of them would be a + * regression this file has to catch. B, C, D and E are the genuinely red ones. + */ + +import React from 'react'; +import { describe, it, expect, vi, beforeAll, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { registerAllFields } from '@object-ui/fields'; +import { ActionProvider } from '@object-ui/react'; +import { ObjectGrid } from '../ObjectGrid'; + +registerAllFields(); + +beforeAll(() => { + if (!Element.prototype.hasPointerCapture) { + Element.prototype.hasPointerCapture = vi.fn(() => false) as any; + } + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +afterEach(() => cleanup()); + +/** The hinted column's value — the one that MUST become a `tel:` anchor. */ +const HINTED_PHONE = '+15551234567'; +/** The control column's value — phone-SHAPED, declared without a hint. */ +const CONTROL_PHONE = '+15559876543'; + +/** + * `work_phone` is the subject: a TEXTUAL base type carrying a `format` hint, + * which is exactly the pair `resolveCellRendererType` exists to promote. + * `note` is its control — same base type, same value shape, no hint. + */ +const CONTACT_SCHEMA = { + name: 'contacts', + label: 'Contact', + fields: { + name: { type: 'text', label: 'Name' }, + work_phone: { type: 'text', format: 'phone', label: 'Work Phone' }, + note: { type: 'text', label: 'Note' }, + stage: { + type: 'select', + label: 'Stage', + options: [{ value: 'new', label: 'New' }], + }, + }, +}; + +const ROWS = [ + { + id: 'c-1', + name: 'Alice', + work_phone: HINTED_PHONE, + note: CONTROL_PHONE, + stage: 'new', + }, +]; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: ROWS, total: ROWS.length })), + getObjectSchema: vi.fn(async () => CONTACT_SCHEMA), + } as any; +} + +/** Every `tel:` href currently in the tree, in DOM order. */ +function telHrefs(root: HTMLElement): string[] { + return Array.from(root.querySelectorAll('a[href^="tel:"]')).map( + (a) => a.getAttribute('href') ?? '', + ); +} + +function renderGrid(schemaOverrides: Record, props: Record = {}) { + return render( + + + , + ); +} + +/** + * The assertion every path shares: the hinted column drew a `tel:` anchor, the + * unhinted control did not, and the control's value is still on screen (so the + * absence half cannot be satisfied by the column having vanished). + */ +async function expectHintHonoured(root: HTMLElement) { + await waitFor(() => expect(telHrefs(root)).toContain(`tel:${HINTED_PHONE}`)); + expect(telHrefs(root)).not.toContain(`tel:${CONTROL_PHONE}`); + expect(root.textContent).toContain(CONTROL_PHONE); +} + +describe('objectui#8920 — every ObjectGrid path honours a `format` hint', () => { + /** PATH A — object `columns`. PIN: already correct before the fix. */ + it('A: object `columns` (must-not-change pin — this path already resolved)', async () => { + const { container } = renderGrid({ + columns: [{ field: 'name' }, { field: 'work_phone' }, { field: 'note' }], + data: { provider: 'value', items: ROWS }, + }); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + await expectHintHonoured(container); + }); + + /** PATH B — string `columns`. */ + it('B: string `columns`', async () => { + const { container } = renderGrid({ + columns: ['name', 'work_phone', 'note'], + data: { provider: 'value', items: ROWS }, + }); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + await expectHintHonoured(container); + }); + + /** + * PATH C — the inline-data projection. + * + * `fields` (not `columns`) is what selects this branch and keeps it selected: + * `rowKeysWouldOutrankSchemaPolicy` is `!schemaFields && objectName && + * objectSchema`, so an authored projection is exactly the condition under + * which the inline path survives the schema landing. + */ + it('C: inline-data projection (`fields`, no `columns`)', async () => { + const { container } = renderGrid({ + fields: ['name', 'work_phone', 'note'], + data: { provider: 'value', items: ROWS }, + }); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + await expectHintHonoured(container); + }); + + /** + * PATH D — the object-schema default-columns policy. + * + * Rows handed down as a prop with NO authored projection: that is + * `rowKeysWouldOutrankSchemaPolicy === true`, which skips path C and lands + * here (objectui#6677's ordering). + */ + it('D: object-schema default columns (rows handed down, no projection)', async () => { + const { container } = renderGrid({}, { data: ROWS }); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + await expectHintHonoured(container); + }); + + /** + * PATH E — the record-detail panel. + * + * `work_phone` and `note` are deliberately NOT columns here, so the only + * place either value can reach the DOM is the panel: no table cell can + * satisfy these assertions on the panel's behalf. The panel renders through + * a portal, so it is read by test id rather than from `container`. + */ + it('E: record-detail panel', async () => { + renderGrid({ + columns: [{ field: 'name' }], + data: { provider: 'value', items: ROWS }, + navigation: { mode: 'drawer' }, + }); + + fireEvent.click(await screen.findByText('Alice')); + await waitFor(() => expect(screen.getByTestId('record-detail-panel')).toBeInTheDocument()); + await expectHintHonoured(screen.getByTestId('record-detail-panel')); + }); + + /** + * PATH P — the compound-cell prefix badge. PIN: this site asks for a FIXED + * registry key (`select`), not for a field's renderer, so there is nothing to + * promote and its output must be byte-identical across the fix. It is here + * because routing it through the shared module is what keeps it from reading + * as a convention nobody chose — and a shared module that quietly changed it + * would be a regression. + */ + it('P: compound-cell prefix badge still draws the select renderer (must-not-change pin)', async () => { + const { container } = renderGrid({ + columns: [{ field: 'name', prefix: { field: 'stage', type: 'badge' } }, { field: 'work_phone' }], + data: { provider: 'value', items: ROWS }, + }); + + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + // The badge renderer translates the option value to its label; a plain + // text prefix would print the raw `new`. + await waitFor(() => expect(container.textContent).toContain('New')); + // …and the hinted column in the same grid is still promoted. + expect(telHrefs(container)).toContain(`tel:${HINTED_PHONE}`); + }); +}); diff --git a/packages/plugin-grid/src/cellRendererResolution.ts b/packages/plugin-grid/src/cellRendererResolution.ts new file mode 100644 index 0000000000..5d8596238a --- /dev/null +++ b/packages/plugin-grid/src/cellRendererResolution.ts @@ -0,0 +1,146 @@ +/** + * 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. + */ + +/** + * THE GRID'S SINGLE ANSWER TO "WHICH CELL RENDERER?" (objectui#8920). + * + * ## The defect this module exists to close + * + * `@object-ui/fields` publishes a TWO-STEP resolve: + * `getCellRenderer(resolveCellRendererType(field))`. The second step exists + * because a field's DECLARED type is not always the renderer's key — a textual + * base type carrying a `format` hint (`phone`, `email`, `url`, `currency`, + * `percent`) maps to the richer renderer. Skip it and the cell gets + * `TextCellRenderer`, which destructures only `value` and therefore never reads + * `field.format`: the hint is dropped, nothing throws, nothing warns, and the + * column silently renders as plain truncated text instead of a `tel:` / + * `mailto:` / formatted value. + * + * `ObjectGrid` used to spell that resolve SIX times with THREE conventions — + * one two-step, four raw declared types, one fixed registry key. Patching the + * four to match the one leaves the divergence in place and lets a seventh site + * pick either convention, so the resolution itself lives here instead: every + * `getCellRenderer` call the grid makes is made from this module, and + * `ObjectGrid.tsx` no longer imports `getCellRenderer` at all. That invariant + * is pinned by `__tests__/cellRendererResolutionBoundary-8920.test.ts`. + * + * ## Two resolutions, two names — the trap that hid the defect + * + * The word "resolve" named two different things in this file's caller, and the + * collision is most of why four sites read as correct: + * + * - "resolve which TYPE the field has" — `objectDef type > heuristic + * inference`. That answer is `declaredType`, and it is what the inline + * editor and the emitted `type:` key read. + * - "resolve which RENDERER the type maps to" — the published two-step. + * That answer is `rendererType`, and it is what `getCellRenderer`, + * `fieldMeta.type`, the header icon and the numeric alignment read. + * + * A local called `resolvedType` that held only the first one reads exactly like + * a value the second step had already been applied to. Both are named here so + * no call site has to hold the distinction in its head. + */ + +import type React from 'react'; +import { + getCellRenderer, + resolveCellRendererType, + type CellRendererProps, +} from '@object-ui/fields'; + +/** + * A field-shaped input. Deliberately narrower than `FieldMetadata`: the two + * members below are the whole of what "declared type + format hint → renderer" + * reads, and naming them stops the `any` that every caller's `objectSchema` + * read carries (`useState`) from collapsing the emit literals downstream + * (objectui#6004). + */ +export interface GridCellFieldLike { + /** The field's DECLARED type, already through any heuristic inference. */ + type?: string | null; + /** The field's `format` hint, as the object definition declares it. */ + format?: string | null; +} + +/** Everything the grid needs to know about one column's renderer. */ +export interface GridCellRendering { + /** + * The DECLARED type, unpromoted — `null` when the column has none. This is + * what the inline editor reads, so a `text` + `format: 'phone'` column keeps + * editing as text. + */ + declaredType: string | null; + /** + * The RENDERER key — the declared type promoted by its `format` hint. + * `null` exactly when `declaredType` is `null`. + */ + rendererType: string | null; + /** + * The cell renderer for `rendererType`. + * + * TOTAL, mirroring `getCellRenderer` itself, which ends in + * `standardMap[key] || TextCellRenderer` and therefore never returns a falsy + * renderer. A caller that wants "no declared type ⇒ no type-aware cell" + * asks `rendererType`, not this — the two questions are different and the + * grid's paths answer them differently on purpose. + */ + Renderer: React.FC; +} + +/** + * Resolve one column's renderer from its declared type and `format` hint. + * + * ⚠️ A field with NO declared type does not promote, even when it carries a + * `format`. `resolveCellRendererType` would promote it — `''` is a member of + * its textual base set — but every grid path already treats "no type at all" + * as "no type-aware renderer", and reversing that is a behaviour change wider + * than the one objectui#8920 rules on. The bound is deliberate, not an + * oversight; the unresolvable key still lands on the text renderer below, + * byte-identical to the `getCellRenderer(field.type)` this replaced when + * `field.type` was absent. + */ +export function resolveGridCellRendering( + field: GridCellFieldLike | null | undefined, +): GridCellRendering { + const declaredType: string | null = field?.type || null; + const rendererType: string | null = + declaredType === null + ? null + : resolveCellRendererType({ type: declaredType, format: field?.format ?? undefined }); + return { + declaredType, + rendererType, + Renderer: getCellRenderer(rendererType ?? ''), + }; +} + +/** + * ⭐ THE ONE SITE WITH A DIFFERENT CONTRACT, NAMED SO IT IS NOT A FIFTH SILENT + * CONVENTION. + * + * The compound-cell prefix badge asks for a FIXED registry key, not for a + * field's renderer: `prefix.type === 'badge'` is the author saying "draw this + * neighbouring column's value as a badge", and the key it needs is a constant + * of this component, not anything the prefixed field declares. There is no + * declared type to promote and no `format` to read, so + * `resolveGridCellRendering` cannot own it without misrepresenting the call as + * a field resolution. + * + * It routes through this module anyway, and that is the point: the grid has + * exactly two ways to reach a cell renderer, both of them here, both of them + * documented — rather than one documented way plus a bare `getCellRenderer` + * call that reads like a fifth convention nobody chose. + */ +export function gridCellRendererForFixedKey( + rendererType: string, +): React.FC { + return getCellRenderer(rendererType); +} + +/** The registry key the compound-cell prefix badge draws with. */ +export const BADGE_PREFIX_RENDERER_KEY = 'select';