diff --git a/.changeset/rowheight-density-one-answer-4440.md b/.changeset/rowheight-density-one-answer-4440.md new file mode 100644 index 000000000..c04d7e2fc --- /dev/null +++ b/.changeset/rowheight-density-one-answer-4440.md @@ -0,0 +1,42 @@ +--- +'@object-ui/core': minor +'@object-ui/plugin-list': minor +--- + +`rowHeightToDensityMode` answers only for the five spec row heights — the coerce-to-`comfortable` fallback is gone + +Two surfaces narrow a list view's `rowHeight` onto the renderer's three-step +density vocabulary, and since objectui#4352 they answered differently for the +same off-spec input: `@object-ui/react`'s spec bridge declined to answer, while +`@object-ui/core`'s `rowHeightToDensityMode` rehabilitated anything unknown into +`comfortable`. One metadata-driven system, two answers for one input +(objectui#4440). + +The strict answer wins, per AGENTS.md #0.1: a renderer-side rehabilitation of +off-spec metadata is a second de-facto contract, and one strict contract beats N +dialects — a bad `rowHeight` gets fixed at the producer, where the schema already +rejects it. The five mappings themselves are untouched (`compact`/`short` → +`compact`, `medium` → `comfortable`, `tall`/`extra_tall` → `spacious`), and the +table keeps its `Record< RowHeight, … >` typing, so a row height added upstream +still fails the build here. + +**Breaking semantics, deliberately graded `minor`** (this repo never publishes +`major` — its major tracks `@objectstack`). Two things change: + +- **Published type.** `rowHeightToDensityMode` is exported from + `@object-ui/core`, and its return widens from `DensityMode` to + `DensityMode | undefined`. A host assigning the result straight into a + `DensityMode` now has to say what an off-spec row height should mean to it. +- **Rendered output, for input the spec already rejects.** `ListView` — the one + in-repo caller — used to render an off-spec `rowHeight` one step looser than an + ABSENT one (`comfortable`, 40px rows, vs `compact`, 32px). It now renders it + exactly like an absent one, `compact`, which is also `ObjectGrid`'s own default. + A sweep of this repo, the `objectstack` example apps and one downstream app + found zero authored off-spec values, and the legacy `densityMode` alias cannot + produce one (`DENSITY_MODE_TO_ROW_HEIGHT` is typed + `Record< DensityMode, RowHeight >`). + +Also closed while retiring the branch: the lookup guarded membership with `in`, +which walks the prototype chain, so `rowHeight: 'toString'` returned +`Object.prototype.toString` — a function — from something typed `DensityMode`. It +is an own-property check now. diff --git a/packages/core/src/utils/__tests__/normalize-list-view.test.ts b/packages/core/src/utils/__tests__/normalize-list-view.test.ts index f48da85a2..844b95e1a 100644 --- a/packages/core/src/utils/__tests__/normalize-list-view.test.ts +++ b/packages/core/src/utils/__tests__/normalize-list-view.test.ts @@ -9,6 +9,7 @@ import { describe, it, expect } from 'vitest'; import { normalizeListViewSchema, + rowHeightToDensityMode, DENSITY_MODE_TO_ROW_HEIGHT, ROW_HEIGHT_TO_DENSITY_MODE, } from '../normalize-list-view.js'; @@ -290,3 +291,48 @@ describe('normalizeListViewSchema (#2890)', () => { }); }); }); + +describe('rowHeightToDensityMode (#4440)', () => { + describe('the five spec row heights — the mapping itself, unchanged', () => { + it.each([ + ['compact', 'compact'], + ['short', 'compact'], + ['medium', 'comfortable'], + ['tall', 'spacious'], + ['extra_tall', 'spacious'], + ] as const)('maps the spec row height %s to %s', (rowHeight, density) => { + expect(rowHeightToDensityMode(rowHeight)).toBe(density); + }); + }); + + describe('off-spec input — abstain, do not rehabilitate', () => { + // The retired branch answered `'comfortable'` here, while + // `@object-ui/react`'s spec bridge answered "no density at all" for the + // same string after #4352 (PR #4439). One metadata-driven system, two + // answers for one input, was #4440; this is the surviving answer. + it.each([ + 'comfortable', // the OUTPUT vocabulary, never a spec row height + 'spacious', + 'small', + 'large', + 'gargantuan', // a string in neither vocabulary + '', + ])('gives no density for the off-spec rowHeight %j', (rowHeight) => { + expect(rowHeightToDensityMode(rowHeight)).toBeUndefined(); + }); + + it('gives no density for non-string metadata', () => { + // Stored view definitions reach this function from a database that + // TypeScript never saw, so the runtime guard is load-bearing, not + // decoration: `ListViewSchema.rowHeight` is statically `RowHeight`. + for (const value of [undefined, null, 42, true, {}, ['medium']]) { + expect(rowHeightToDensityMode(value)).toBeUndefined(); + } + }); + + it('does not answer for an inherited Object.prototype key', () => { + expect(rowHeightToDensityMode('toString')).toBeUndefined(); + expect(rowHeightToDensityMode('constructor')).toBeUndefined(); + }); + }); +}); diff --git a/packages/core/src/utils/normalize-list-view.ts b/packages/core/src/utils/normalize-list-view.ts index f20e65e39..0f3193d45 100644 --- a/packages/core/src/utils/normalize-list-view.ts +++ b/packages/core/src/utils/normalize-list-view.ts @@ -53,17 +53,38 @@ export const ROW_HEIGHT_TO_DENSITY_MODE: Record = { }; /** - * Tolerant reader for {@link ROW_HEIGHT_TO_DENSITY_MODE}. View metadata is - * user-authored, so `rowHeight` is not guaranteed to be one of the five spec - * values — an unknown one lands on `comfortable` rather than rendering an - * undefined density. Keeping the fallback here means every surface collapses - * the same way. + * Runtime reader for {@link ROW_HEIGHT_TO_DENSITY_MODE}: the spec's five row + * heights narrowed onto the renderer's three densities, and **nothing else**. + * + * The parameter stays `unknown` because this is the boundary user-authored view + * metadata actually crosses — `ListViewSchema.rowHeight` is statically a + * `RowHeight`, but the value arrives from stored view definitions TypeScript + * never saw (`ObjectView`: `viewDef.rowHeight ?? listSchema.rowHeight`). The + * type-level half of the guarantee is the `Record` on the table + * above, which fails the build when the spec grows a row height. + * + * An off-spec value gets NO density (objectui#4440). It used to be coerced to + * `comfortable`, which is the opposite of what `@object-ui/react`'s spec bridge + * answers for the same string after objectui#4352 — one metadata-driven system + * holding two answers for one input. AGENTS.md #0.1 decides which one survives: + * a renderer-side rehabilitation of off-spec metadata is a second de-facto + * contract, and one strict contract beats N. The producer is where a bad + * `rowHeight` gets fixed. + * + * Callers apply their own "nothing was said" default to `undefined`, so an + * off-spec row height now renders exactly like an absent one — `'compact'` in + * both `ListView` and `ObjectGrid`. + * + * `hasOwnProperty`, not `in`: `in` walks the prototype chain, so `'toString'` + * used to come back as `Object.prototype.toString` — a FUNCTION returned from + * something typed `DensityMode`. */ -export function rowHeightToDensityMode(rowHeight: unknown): DensityMode { - if (typeof rowHeight === 'string' && rowHeight in ROW_HEIGHT_TO_DENSITY_MODE) { - return ROW_HEIGHT_TO_DENSITY_MODE[rowHeight as RowHeight]; +export function rowHeightToDensityMode(rowHeight: unknown): DensityMode | undefined { + if (typeof rowHeight !== 'string') return undefined; + if (!Object.prototype.hasOwnProperty.call(ROW_HEIGHT_TO_DENSITY_MODE, rowHeight)) { + return undefined; } - return 'comfortable'; + return ROW_HEIGHT_TO_DENSITY_MODE[rowHeight as RowHeight]; } const isRecord = (v: unknown): v is Record => diff --git a/packages/plugin-list/src/ListView.tsx b/packages/plugin-list/src/ListView.tsx index b155bd7a9..1bdd3710b 100644 --- a/packages/plugin-list/src/ListView.tsx +++ b/packages/plugin-list/src/ListView.tsx @@ -1034,10 +1034,18 @@ export const ListView = React.forwardRef(({ // legacy `densityMode` is folded into it by `normalizeListViewSchema` above — // it used to be read FIRST here, so a view carrying both rendered the legacy // value, backwards from every other pair's canonical-wins precedence. - const resolvedDensity = React.useMemo(() => { - if (schema.rowHeight) return rowHeightToDensityMode(schema.rowHeight); - return 'compact'; - }, [schema.rowHeight]); + // + // `rowHeightToDensityMode` answers only for the five spec row heights and + // abstains for anything else (#4440), so an off-spec value lands on the same + // `'compact'` an ABSENT `rowHeight` has always landed on — which is also + // `ObjectGrid`'s own default (`schema.rowHeight ?? 'compact'`). Do NOT drop + // the `??` and let `undefined` reach `useDensityMode`: its parameter default + // is `'comfortable'`, so the coercion #4440 retired would simply reappear one + // frame lower, and the two surfaces would disagree again. + const resolvedDensity = React.useMemo( + () => rowHeightToDensityMode(schema.rowHeight) ?? 'compact', + [schema.rowHeight], + ); const density = useDensityMode(resolvedDensity, { onChange: schema.onDensityChange, }); diff --git a/packages/plugin-list/src/__tests__/ListView.density.offspec.test.tsx b/packages/plugin-list/src/__tests__/ListView.density.offspec.test.tsx new file mode 100644 index 000000000..6e7975e4f --- /dev/null +++ b/packages/plugin-list/src/__tests__/ListView.density.offspec.test.tsx @@ -0,0 +1,110 @@ +/** + * 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, vi } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import { SchemaRendererProvider } from '@object-ui/react'; +import type { ListViewSchema } from '@object-ui/types'; +import { ListView } from '../ListView'; + +/** + * What an off-spec `rowHeight` actually RENDERS — the empirical half of + * objectui#4440, pinned on the live path rather than argued from the mapping. + * + * `ListView` is the only in-repo caller of `@object-ui/core`'s + * `rowHeightToDensityMode`. Before #4440 that function coerced an unknown + * `rowHeight` to `'comfortable'`, so a garbage value rendered the toolbar (and + * the rows it forwards its density to) one step looser than an ABSENT + * `rowHeight` did — absent has always resolved to `'compact'` here, and + * `ObjectGrid` independently defaults `schema.rowHeight ?? 'compact'` too. + * + * After #4440 the mapping abstains and the caller's own "nothing was said" + * default applies, so off-spec renders exactly like absent. That is the + * measured consequence of aligning with `@object-ui/react`'s spec bridge, and + * this file is where it is visible: the density button's `aria-label` is + * `density.mode` verbatim. + * + * Note the abstain deliberately does NOT fall through to `useDensityMode`'s own + * parameter default, which is `'comfortable'` — landing there would have + * re-created the retired coercion one stack frame further down and left the two + * surfaces disagreeing exactly as before, while looking like a fix. + */ + +const mockDataSource = { + find: vi.fn().mockResolvedValue([]), + findOne: vi.fn(), + create: vi.fn(), + update: vi.fn(), + delete: vi.fn(), +}; + +const renderListView = (rowHeight?: unknown) => { + const schema = { + type: 'list-view', + objectName: 'contacts', + viewType: 'grid', + columns: ['name', 'email'], + ...(rowHeight === undefined ? {} : { rowHeight }), + } as ListViewSchema; + + return render( + + + , + ); +}; + +/** The toolbar's density button labels itself `Density: `. */ +const renderedDensity = () => + screen + .getByRole('button', { name: /^Density: / }) + .getAttribute('aria-label') + ?.replace('Density: ', ''); + +describe('ListView density for an off-spec rowHeight (#4440)', () => { + describe('spec row heights — controls, unaffected by the change', () => { + it.each([ + ['compact', 'Compact'], + ['short', 'Compact'], + ['medium', 'Comfortable'], + ['tall', 'Spacious'], + ['extra_tall', 'Spacious'], + ])('renders %s as %s', (rowHeight, expected) => { + renderListView(rowHeight); + expect(renderedDensity()).toBe(expected); + }); + + it('renders an absent rowHeight as Compact', () => { + renderListView(undefined); + expect(renderedDensity()).toBe('Compact'); + }); + }); + + describe('off-spec rowHeight renders exactly like an absent one', () => { + // Every one of these rendered `Comfortable` before #4440. No authored + // metadata in objectui, objectstack or the surveyed downstream app spells + // any of them (the PR #4439 sweep, re-run for objectui and objectstack), + // so this is the rendered consequence of a value that does not exist — + // documented because it was measured, not because it was reachable. + it.each(['comfortable', 'spacious', 'small', 'large', 'gargantuan'])( + 'renders the off-spec rowHeight %s as Compact', + (rowHeight) => { + renderListView(rowHeight); + expect(renderedDensity()).toBe('Compact'); + }, + ); + + it('renders a non-string rowHeight as Compact', () => { + // Stored view definitions cross an untyped boundary + // (`ObjectView`: `viewDef.rowHeight ?? listSchema.rowHeight`), so the + // value is only statically a `RowHeight`. + renderListView(42); + expect(renderedDensity()).toBe('Compact'); + }); + }); +}); diff --git a/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts b/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts new file mode 100644 index 000000000..ca8c850d4 --- /dev/null +++ b/packages/react/src/spec-bridge/__tests__/RowHeightDensityAgreement.test.ts @@ -0,0 +1,90 @@ +/** + * 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 { rowHeightToDensityMode, type DensityMode } from '@object-ui/core'; +import { SpecBridge } from '../SpecBridge'; + +/** + * The agreement pin for objectui#4440. + * + * Two surfaces narrow a list view's `rowHeight` onto the renderer's three-step + * density vocabulary, and for a while they answered differently for the same + * off-spec input: `@object-ui/core`'s `rowHeightToDensityMode` coerced anything + * unknown to `'comfortable'`, while this package's `mapDensity` — after #4352 + * (PR #4439) — declined to answer at all. One metadata-driven system, two + * answers for one input. #4440 retired the coercion; this test is what stops + * the disagreement regrowing silently on either side. + * + * It lives HERE, not in `@object-ui/core`, because the dependency direction + * decides: `@object-ui/react` depends on `@object-ui/core`, so this package can + * see both surfaces, and core cannot import react without inverting the graph. + * The pin therefore imports core's function by its PUBLISHED specifier + * (`@object-ui/core`, a declared dependency of this package) and reaches the + * bridge through `SpecBridge.transformListView`, whose parameter is `any` — + * the untyped boundary a host's stored JSON actually crosses, and after #4352 + * the only way an off-spec `rowHeight` can enter the bridge at all. + */ + +/** What the bridge answers for a `rowHeight`, in core's return shape. */ +function bridgeDensityFor(rowHeight: unknown): DensityMode | undefined { + const node = new SpecBridge().transformListView({ + name: 'row_height_agreement', + rowHeight, + }); + // The bridge writes the key only when it has an answer, so an absent key and + // an explicit `undefined` are the same statement: "no density, use yours". + return 'density' in node ? (node.density as DensityMode | undefined) : undefined; +} + +describe('rowHeight → density: core and the spec bridge give one answer (#4440)', () => { + describe('the five spec row heights — controls, green on both sides', () => { + it.each([ + ['compact', 'compact'], + ['short', 'compact'], + ['medium', 'comfortable'], + ['tall', 'spacious'], + ['extra_tall', 'spacious'], + ] as const)('both surfaces map %s to %s', (rowHeight, expected) => { + expect(rowHeightToDensityMode(rowHeight)).toBe(expected); + expect(bridgeDensityFor(rowHeight)).toBe(expected); + }); + }); + + describe('off-spec row heights — both abstain', () => { + // `comfortable` / `spacious` / `small` / `large` are the four spellings + // #4352 deleted from the bridge; `gargantuan` is a string in neither + // vocabulary. Before #4440 core answered `'comfortable'` for every one of + // them while the bridge answered nothing. + it.each(['comfortable', 'spacious', 'small', 'large', 'gargantuan'])( + 'neither surface invents a density for the off-spec rowHeight %s', + (rowHeight) => { + expect(rowHeightToDensityMode(rowHeight)).toBeUndefined(); + expect(bridgeDensityFor(rowHeight)).toBeUndefined(); + }, + ); + + // NOT pinned here, deliberately: `Object.prototype` member names + // (`toString`, `constructor`, …). Core abstains for them since #4440, but + // the bridge still indexes its table with an unguarded key and hands back + // `Object.prototype.toString` — a FUNCTION — as the density. That is a + // different defect from the coercion this file is about, it lives in source + // outside #4440's surface, and it is filed as #4442. Extending the two + // `it.each` lists above with those keys is the assertion that fails until + // #4442 lands, and is the natural test half of its fix. + + it('agrees for every off-spec input without either side being read first', () => { + // Same assertion phrased as the invariant itself: whatever the answer is, + // it is ONE answer. A future edit that re-adds a fallback to either + // surface breaks this even if it re-adds it to both differently. + for (const rowHeight of ['comfortable', 'spacious', 'small', 'large', 'gargantuan', '']) { + expect(rowHeightToDensityMode(rowHeight)).toBe(bridgeDensityFor(rowHeight)); + } + }); + }); +});