diff --git a/.changeset/objectgrid-rowheight-boundary-4443.md b/.changeset/objectgrid-rowheight-boundary-4443.md new file mode 100644 index 000000000..3eedf47c0 --- /dev/null +++ b/.changeset/objectgrid-rowheight-boundary-4443.md @@ -0,0 +1,18 @@ +--- +'@object-ui/plugin-grid': patch +--- + +standalone ObjectGrid resolves off-spec `rowHeight` to compact, matching ListView and the spec bridge, instead of silently styling it as medium + +One component answered one question two ways. `ObjectGrid` seeded its density state with `schema.rowHeight ?? 'compact'`, so an ABSENT `rowHeight` landed on `compact` while an OFF-SPEC one skipped every arm of the density ternaries and came out at their terminal `else` — the `medium` styling. That is the absent-vs-off-spec split objectui#4440 removed from `ListView`, and it made a standalone grid the third answer to a question the rest of the system had already settled: `@object-ui/core`'s `rowHeightToDensityMode` abstains for an off-spec value, the `@object-ui/react` spec bridge abstains, and `ListView` defaults the abstention to `compact`. Off-spec now renders exactly like absent, everywhere. + +Only a standalone grid was affected. When `ListView` owns the grid it overwrites the prop with a value derived from `density.mode`, so nothing off-spec survives that hop. + +The narrowing happens at the state boundary, not in the ternaries. `medium` is still a real row height with its own styling arm, and a leaf renderer's terminal `else` is still legitimate styling — what changes is that nothing unrecognized can reach it. Membership is tested against `ROW_HEIGHT_TO_DENSITY_MODE`, so the admitted values keep one definition in the repo and the build fails if the spec grows a sixth row height without teaching the resolver about it. Both entry points go through the resolver: the initial state and the effect that re-syncs when the `rowHeight` prop changes. + +Two off-spec spellings behaved differently before this, which the report of the defect did not distinguish, and the boundary fix covers both: + +- A plain off-spec value (`'garbage'`) was not a key of the toolbar's row-height icon map either. That map is looked up by the same unvalidated state, so `rowHeightIcons[mode]` was `undefined` and rendering `` threw `Element type is invalid` — a standalone grid with an off-spec `rowHeight` did not render at all, rather than rendering as `medium`. The toolbar is shown precisely when `schema.rowHeight` is defined, so the crash and the off-spec case coincide exactly. +- A prototype member (`'toString'`) WAS reachable through that map's prototype chain, resolving to `Object.prototype.toString` — a function, which React accepts as a component — so it survived to the ternaries and rendered as `medium`, the defect as filed. The resolver uses `hasOwnProperty` rather than `in` for this reason, the same reason `@object-ui/core` does. + +Both are now inert: the state can only ever hold one of the five admitted row heights, so the icon lookup is total and the ternaries never fall through. diff --git a/packages/plugin-grid/src/ObjectGrid.tsx b/packages/plugin-grid/src/ObjectGrid.tsx index 9495837da..934ba2010 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, columnIdentity, collectPredicateFieldRefs, listViewPredicates, isProjectableField, isExpandableFieldType, toFilterNode } from '@object-ui/core'; +import { resolveConditionalFormatting, buildExpandFields, buildExportFileName, columnIdentity, collectPredicateFieldRefs, listViewPredicates, isProjectableField, isExpandableFieldType, toFilterNode, ROW_HEIGHT_TO_DENSITY_MODE } 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'; @@ -325,6 +325,43 @@ function normalizeColumns( return columns as string[]; } +/** The row heights this grid styles — the five `RowHeight` values the spec admits. */ +type RowHeightMode = 'compact' | 'short' | 'medium' | 'tall' | 'extra_tall'; + +/** + * The ONE answer this component gives for a `rowHeight` it does not recognize + * (objectui#4443). + * + * The seed used to be `schema.rowHeight ?? 'compact'`, which made the component + * answer the same question two ways: an ABSENT `rowHeight` landed on `compact`, + * an OFF-SPEC one fell through the density ternaries below to their terminal + * `else` — the `medium` styling. That is the absent-vs-off-spec split #4440 + * removed from `ListView`, and a third answer to a question `@object-ui/core` + * (`rowHeightToDensityMode`, which abstains) and the `@object-ui/react` spec + * bridge (#4352, which abstains) had already settled. One metadata-driven + * system, one answer: off-spec renders exactly like absent. + * + * The ternary chains are deliberately NOT touched — `medium` is a real value + * with its own arm, and a leaf renderer's terminal `else` is legitimate styling. + * Narrowing happens here, at the boundary, so nothing off-spec ever reaches it. + * + * Membership is tested against `ROW_HEIGHT_TO_DENSITY_MODE` rather than a local + * list so the admitted values have one definition in the repo; that table is + * typed `Record`, so the build fails if the spec grows a + * sixth row height and this resolver is not taught about it. + * + * `hasOwnProperty`, not `in`: `in` walks the prototype chain, so `'toString'` + * would come back admitted. That is not hypothetical here — the toolbar's icon + * map is looked up by the same key, and a prototype member reached + * `Object.prototype.toString` and rendered it as a React component, while a + * plain off-spec value produced `undefined` and threw outright. + */ +function resolveRowHeightMode(rowHeight: unknown): RowHeightMode { + if (typeof rowHeight !== 'string') return 'compact'; + if (!Object.prototype.hasOwnProperty.call(ROW_HEIGHT_TO_DENSITY_MODE, rowHeight)) return 'compact'; + return rowHeight as RowHeightMode; +} + export const ObjectGrid: React.FC = ({ schema, dataSource, @@ -368,7 +405,7 @@ export const ObjectGrid: React.FC = ({ const [showExport, setShowExport] = useState(false); const [exportBusy, setExportBusy] = useState(false); const [exportError, setExportError] = useState(null); - const [rowHeightMode, setRowHeightMode] = useState<'compact' | 'short' | 'medium' | 'tall' | 'extra_tall'>(schema.rowHeight ?? 'compact'); + const [rowHeightMode, setRowHeightMode] = useState(resolveRowHeightMode(schema.rowHeight)); const [selectedRows, setSelectedRows] = useState([]); const [selectAllMatching, setSelectAllMatching] = useState(false); // Bumped to tell the underlying table to drop its internal checkbox selection. @@ -389,10 +426,15 @@ export const ObjectGrid: React.FC = ({ (schema.pagination as any)?.pageSize ?? schema.pageSize ?? 10, ); - // Sync internal rowHeightMode when schema.rowHeight prop changes (e.g., parent ListView density toggle) + // Sync internal rowHeightMode when schema.rowHeight prop changes (e.g., parent ListView density toggle). + // Routed through the same resolver as the seed above: this is the component's + // second entry point for an author-supplied `rowHeight`, and one resolver at + // every entry is what keeps the answer single (objectui#4443). React.useEffect(() => { - if (schema.rowHeight && schema.rowHeight !== rowHeightMode) { - setRowHeightMode(schema.rowHeight); + if (!schema.rowHeight) return; + const next = resolveRowHeightMode(schema.rowHeight); + if (next !== rowHeightMode) { + setRowHeightMode(next); } // eslint-disable-next-line react-hooks/exhaustive-deps }, [schema.rowHeight]); diff --git a/packages/plugin-grid/src/__tests__/rowHeightOffSpecBoundary.test.tsx b/packages/plugin-grid/src/__tests__/rowHeightOffSpecBoundary.test.tsx new file mode 100644 index 000000000..aa026fa1b --- /dev/null +++ b/packages/plugin-grid/src/__tests__/rowHeightOffSpecBoundary.test.tsx @@ -0,0 +1,213 @@ +/** + * Off-spec `rowHeight` resolves at the state boundary (#4443). + * + * `ObjectGrid` seeds `rowHeightMode` from `schema.rowHeight`. Before #4443 the + * seed was `schema.rowHeight ?? 'compact'`, so the component answered the same + * question two different ways: an ABSENT `rowHeight` landed on `compact`, while + * an OFF-SPEC one fell through the rendering ternaries to their terminal + * `else` — the `medium` styling. That is the absent-vs-off-spec split #4440 + * removed from `ListView`, and the third answer to a question `@object-ui/core` + * (`rowHeightToDensityMode` abstains) and the `@object-ui/react` spec bridge had + * already agreed on. + * + * These tests pin the boundary, not the ternaries: `medium` stays a real value + * with its own styling arm, and the terminal `else` stays that arm. What must + * hold is that nothing off-spec ever reaches it. + * + * Two off-spec spellings are covered because they FAIL DIFFERENTLY before the + * fix, which the issue text did not distinguish: + * - a plain off-spec value (`'garbage'`) is not a key of the toolbar's icon + * map either, so `rowHeightIcons[mode]` is `undefined` and rendering + * `` throws — the standalone grid does not render AT ALL; + * - a prototype member (`'toString'`) IS reachable through the icon map's + * prototype chain, so it survives to the ternary and renders as `medium` — + * the defect exactly as filed. Same reason `core` uses `hasOwnProperty` + * rather than `in` (see `normalize-list-view.ts`). + */ +import { describe, it, expect } from 'vitest'; +import { render, screen, waitFor } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import React from 'react'; + +import { ObjectGrid } from '../ObjectGrid'; +import { registerAllFields } from '@object-ui/fields'; +import { ActionProvider } from '@object-ui/react'; + +registerAllFields(); + +const rows = [ + { id: '1', name: 'Alice' }, + { id: '2', name: 'Bob' }, +]; + +function makeSchema(opts?: Record) { + return { + type: 'object-grid', + objectName: 'test_object', + columns: [{ field: 'name', label: 'Name' }], + data: { provider: 'value', items: rows }, + ...opts, + } as any; +} + +function renderGrid(opts?: Record) { + return render( + + + , + ); +} + +/** + * The two independent copies of the density ternary, as they reach the DOM: + * + * - COLUMN_COPY (`ObjectGrid.tsx` ~1833, `rowHeightCellClass`) is attached per + * column and is the only copy carrying the `h-*` row-height floor. It lands + * on the data columns. + * - TABLE_COPY (`ObjectGrid.tsx` ~2370, `dataTableSchema.cellClassName`) is the + * table-level default for columns that declare none — the narrow row-number + * column — and carries padding/leading only, no `h-*`. + * + * They are therefore separately observable, and each case below asserts both. + */ +const COLUMN_COPY: Record = { + compact: ['px-3', 'py-1', 'h-9', 'text-[13px]', 'leading-tight'], + short: ['px-3', 'py-1', 'h-9', 'text-[13px]', 'leading-normal'], + medium: ['px-3', 'py-1.5', 'h-11', 'text-[13px]', 'leading-normal'], + tall: ['px-3', 'py-2.5', 'h-14', 'text-sm'], + extra_tall: ['px-3', 'py-3.5', 'h-16', 'text-sm', 'leading-relaxed'], +}; + +const TABLE_COPY: Record = { + compact: ['px-3', 'py-1', 'text-[13px]', 'leading-tight'], + short: ['px-3', 'py-1', 'text-[13px]', 'leading-normal'], + medium: ['px-3', 'py-1.5', 'text-[13px]', 'leading-normal'], + tall: ['px-3', 'py-2.5', 'text-sm'], + extra_tall: ['px-3', 'py-3.5', 'text-sm', 'leading-relaxed'], +}; + +/** Every `h-*` floor the column copy can emit — exactly one may be present. */ +const ALL_HEIGHTS = ['h-9', 'h-11', 'h-14', 'h-16']; +/** Every vertical padding the table copy can emit — exactly one may be present. */ +const ALL_PADDINGS = ['py-1', 'py-1.5', 'py-2.5', 'py-3.5']; + +async function renderAndSettle(opts?: Record) { + const utils = renderGrid(opts); + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + return utils; +} + +function cellsOf(container: HTMLElement) { + const row = container.querySelector('tbody tr'); + expect(row, 'expected a rendered body row').toBeTruthy(); + const tds = Array.from(row!.querySelectorAll('td')); + const dataCell = tds.find((td) => !td.classList.contains('w-10')); + const plainCell = tds.find((td) => td.classList.contains('w-10')); + expect(dataCell, 'expected a data column cell (column-level cellClassName)').toBeTruthy(); + expect(plainCell, 'expected a row-number cell (table-level cellClassName)').toBeTruthy(); + return { dataCell: dataCell!, plainCell: plainCell! }; +} + +/** Assert BOTH ternary copies resolved to `mode`. */ +function expectDensity(container: HTMLElement, mode: keyof typeof COLUMN_COPY) { + const { dataCell, plainCell } = cellsOf(container); + + // Copy 1 — per-column class, the one with the `h-*` floor. + for (const token of COLUMN_COPY[mode]) { + expect( + dataCell.classList.contains(token), + `data cell should carry ${mode} token "${token}" (column-level copy), got "${dataCell.className}"`, + ).toBe(true); + } + const heights = ALL_HEIGHTS.filter((h) => dataCell.classList.contains(h)); + expect(heights, `data cell should carry exactly the ${mode} row-height floor`).toEqual( + ALL_HEIGHTS.filter((h) => COLUMN_COPY[mode].includes(h)), + ); + + // Copy 2 — table-level class on the column that declares none. + for (const token of TABLE_COPY[mode]) { + expect( + plainCell.classList.contains(token), + `row-number cell should carry ${mode} token "${token}" (table-level copy), got "${plainCell.className}"`, + ).toBe(true); + } + const paddings = ALL_PADDINGS.filter((p) => plainCell.classList.contains(p)); + expect(paddings, `row-number cell should carry exactly the ${mode} padding`).toEqual( + ALL_PADDINGS.filter((p) => TABLE_COPY[mode].includes(p)), + ); +} + +describe('ObjectGrid rowHeight — off-spec resolves at the state boundary (#4443)', () => { + // --------------------------------------------------------------------- + // The direction that must NOT change. + // --------------------------------------------------------------------- + it('absent rowHeight stays compact', async () => { + const { container } = await renderAndSettle(); + expectDensity(container, 'compact'); + }); + + // --------------------------------------------------------------------- + // The five admitted values keep their own styling — the ternary chain is + // untouched and `medium` remains a real value, not just a fallback. + // --------------------------------------------------------------------- + it.each(['compact', 'short', 'medium', 'tall', 'extra_tall'] as const)( + 'in-spec rowHeight %s maps to its own styling', + async (mode) => { + const { container } = await renderAndSettle({ rowHeight: mode }); + expectDensity(container, mode); + }, + ); + + // --------------------------------------------------------------------- + // Off-spec input. Before the fix these fall through to the terminal else. + // --------------------------------------------------------------------- + it('off-spec rowHeight renders as compact, not medium (toolbar suppressed)', async () => { + // `hideRowHeightToggle` keeps the icon-map crash (below) out of the way so + // this case observes the STYLING ternaries for a plain off-spec value. + const { container } = await renderAndSettle({ rowHeight: 'garbage', hideRowHeightToggle: true }); + expectDensity(container, 'compact'); + }); + + it('off-spec rowHeight leaves a standalone grid renderable, and compact', async () => { + // Standalone: `showRowHeightToggle` is true whenever `schema.rowHeight` is + // defined, so the toolbar looks the off-spec value up in its icon map. + const { container } = await renderAndSettle({ rowHeight: 'garbage' }); + expectDensity(container, 'compact'); + expect(screen.getByTitle('Row height: compact')).toBeInTheDocument(); + }); + + it('a prototype-member spelling resolves to compact, not through the prototype chain', async () => { + const { container } = await renderAndSettle({ rowHeight: 'toString' }); + expectDensity(container, 'compact'); + // Pins the resolved STATE, not just the styling: before the fix this read + // "Row height: toString". + expect(screen.getByTitle('Row height: compact')).toBeInTheDocument(); + }); + + it('a non-string rowHeight resolves to compact', async () => { + const { container } = await renderAndSettle({ rowHeight: 42, hideRowHeightToggle: true }); + expectDensity(container, 'compact'); + }); + + // --------------------------------------------------------------------- + // The second entry point: the effect that re-syncs state when the prop + // changes (e.g. a parent ListView's density toggle). One resolver, one + // answer, at every entry. + // --------------------------------------------------------------------- + it('re-syncing to an off-spec rowHeight resolves to compact too', async () => { + const { container, rerender } = render( + + + , + ); + await waitFor(() => expect(screen.getByText('Alice')).toBeInTheDocument()); + expectDensity(container, 'tall'); + + rerender( + + + , + ); + await waitFor(() => expectDensity(container, 'compact')); + }); +});