diff --git a/.changeset/8481-empty-array-is-not-a-cell-value.md b/.changeset/8481-empty-array-is-not-a-cell-value.md new file mode 100644 index 0000000000..26c4a595c6 --- /dev/null +++ b/.changeset/8481-empty-array-is-not-a-cell-value.md @@ -0,0 +1,35 @@ +--- +'@object-ui/fields': minor +--- + +An empty array is no longer a cell value in the shared read renderers (objectui#8481). + +Three renderers in `@object-ui/fields` open a multi-value container and map their +entries into it. Their opening guards tested only `null` / `undefined` / `''`, so `[]` +passed and each mapped over zero entries — the renderer's entire output was a +**childless container**: no glyph, no `aria-label`, a visually blank cell. + +| field types | renderer | output for `[]` before | +|---|---|---| +| `select`, `status`, `multiselect`, `radio`, `checkboxes`, `tags` | `SelectCellRenderer` | a flex-wrap DIV with no children | +| `lookup`, `master_detail`, `tree` | `LookupCellRenderer` | a flex-wrap DIV with no children | +| `user` | `UserCellRenderer` | an avatar-stack DIV with no children | + +All ten types now render the shared `EmptyValue` affordance — the muted em-dash with a +`No value` accessible name — exactly as they already did for `null`. + +**Why this is user-visible on multiple surfaces.** `@object-ui/plugin-detail` already +carried two private upstream pre-checks against this (objectui#8474's `hasCellValue` +and `RelatedList`'s `isValueEmpty` from objectui#8459), so the record page was already +protected. Every consumer that does not pre-check reached the renderer directly: +`ObjectGrid` (both the desktop table and the sub-768px card layout), `ObjectGallery` +and `ObjectKanban` were each verified by rendering to paint the blank cell before this +change and the affordance after it. + +**Deliberately narrow.** This is not a package-wide emptiness predicate and the helper +is not exported. Measured by rendering every registered field type against `[]`, these +renderers hold at least seven different private answers to "is this empty", and several +of the disagreements are intentional: `JsonCellRenderer` draws the two-character array +literal (measured and kept by objectui#8474) and `FileCellRenderer` states `0 files`. +Neither moves, and both are pinned as the declared boundary of this change. `{}` is +untouched everywhere — the test is `Array.isArray`, so an object cannot reach it. diff --git a/packages/fields/src/__tests__/cellRenderers.emptyArray-8481.test.tsx b/packages/fields/src/__tests__/cellRenderers.emptyArray-8481.test.tsx new file mode 100644 index 0000000000..926ddd623e --- /dev/null +++ b/packages/fields/src/__tests__/cellRenderers.emptyArray-8481.test.tsx @@ -0,0 +1,229 @@ +/** + * 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#8481 — an empty array is not a cell value, and the SHARED renderer + * is where that has to be said. + * + * Three renderers in `@object-ui/fields` open a multi-value container and map + * their entries into it. Their opening guards tested only `null`/`undefined`/ + * `''`, so `[]` passed and each mapped over zero entries — the renderer's + * whole output was a childless container: + * + * | field types | renderer | output for `[]` on `7cf6f38fb` | + * |------------------------------------------------------|----------------------|--------------------------------| + * | select, status, multiselect, radio, checkboxes, tags | `SelectCellRenderer` | a DIV classed `flex flex-wrap gap-1`, no children | + * | lookup, master_detail, tree | `LookupCellRenderer` | a DIV classed `flex flex-wrap gap-1`, no children | + * | user | `UserCellRenderer` | a DIV classed `flex -space-x-2`, no children | + * + * `@object-ui/plugin-detail` had already grown two private upstream + * pre-checks against this (objectui#8474, objectui#8459). Every consumer that + * does not pre-check reached the renderer directly. The consumer half of the + * evidence lives in `plugin-grid`'s `emptyArrayCell-8481.test.tsx`. + * + * ── Why the POPULATED cases are the load-bearing ones ───────────────────── + * An implementation that answered "empty" for EVERY value would satisfy every + * `[]`-renders-the-affordance case in this file, including the ones that read + * most convincingly. What refuses it is the four NON-REGRESSION cases below — + * a populated multiselect drawing its badges, a populated lookup its chips, a + * populated user field its avatars, and a scalar select value its single + * badge. That is objectui#8474's measured lesson applied here: the vivid + * assertion is rarely the discriminating one. + * + * ── Why the fix is NOT a package-wide emptiness predicate ───────────────── + * Measured, by rendering every registered field type against `[]`: the + * renderers in this package hold at least seven different private answers to + * "is this empty", and several of the disagreements are deliberate. + * `JsonCellRenderer` draws the two-character literal for `[]` (objectui#8474 + * measured that and kept it) and `FileCellRenderer` states "0 files" — both + * pinned below as the declared boundary of this change, so widening it later + * has to delete an assertion rather than merely forget a consideration. + */ + +import React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { getCellRenderer, resolveCellRendererType } from '../index'; + +afterEach(() => cleanup()); + +const OPTIONS = [ + { value: 'alpha', label: 'Alpha' }, + { value: 'beta', label: 'Beta' }, +]; + +/** Resolve + render exactly the way a consumer builds a read-mode cell. */ +function renderCell(type: string, value: unknown, field: Record = {}) { + const Renderer = getCellRenderer(resolveCellRendererType({ type }) || type); + return render( + , + ); +} + +/** The shared "No value" affordance — a muted glyph carrying an aria-label. */ +const affordance = (root: HTMLElement) => + root.querySelector('[data-slot="empty-value"]'); + +/** + * The defect's signature: an element that exists, occupies the cell and has + * nothing inside it. Asserted structurally rather than by text, because its + * whole problem is that it has no text to look for. + */ +function childlessContainers(root: HTMLElement): HTMLElement[] { + return Array.from(root.querySelectorAll('div')).filter( + (el) => el.childElementCount === 0 && (el.textContent ?? '') === '', + ); +} + +/** Every multi-value type that reaches one of the three fixed renderers. */ +const MULTI_VALUE_TYPES = [ + 'select', 'status', 'multiselect', 'radio', 'checkboxes', 'tags', + 'lookup', 'master_detail', 'tree', + 'user', +] as const; + +describe('objectui#8481 — an empty array is not a cell value', () => { + describe('THE DEFECT — a multi-value container with no entries says so', () => { + for (const type of MULTI_VALUE_TYPES) { + it(`THE DEFECT — \`${type}\` holding [] renders the No-value affordance, not a childless container`, () => { + const { container } = renderCell(type, [], { options: OPTIONS }); + + const empty = affordance(container); + expect(empty, `${type}: expected the shared EmptyValue affordance for []`).not.toBeNull(); + expect( + empty?.getAttribute('aria-label'), + `${type}: the affordance must carry its accessible name`, + ).toBe('No value'); + + expect( + childlessContainers(container).length, + `${type}: [] must not render a childless container (the objectui#8481 defect)`, + ).toBe(0); + }); + } + }); + + describe('NON-REGRESSION — these refuse an EMPTY-for-everything implementation', () => { + it('NON-REGRESSION — a POPULATED multiselect still renders one badge per entry', () => { + const { container } = renderCell('multiselect', ['alpha', 'beta'], { options: OPTIONS }); + + expect( + within(container).queryByText('Alpha'), + 'a populated multiselect must still draw the first option badge', + ).not.toBeNull(); + expect( + within(container).queryByText('Beta'), + 'a populated multiselect must still draw the second option badge', + ).not.toBeNull(); + expect( + affordance(container), + 'a populated multiselect must NOT render the No-value affordance', + ).toBeNull(); + }); + + it('NON-REGRESSION — a POPULATED lookup still renders one chip per referenced record', () => { + const { container } = renderCell('lookup', ['alpha', 'beta'], { reference_to: 'other' }); + + expect( + within(container).queryByText('alpha'), + 'a populated lookup must still draw the first record chip', + ).not.toBeNull(); + expect( + within(container).queryByText('beta'), + 'a populated lookup must still draw the second record chip', + ).not.toBeNull(); + expect( + affordance(container), + 'a populated lookup must NOT render the No-value affordance', + ).toBeNull(); + }); + + it('NON-REGRESSION — a POPULATED user field still renders its avatar stack', () => { + const { container } = renderCell('user', [{ name: 'Ada Lovelace' }, { name: 'Grace Hopper' }]); + + const stack = container.querySelector('div.flex'); + expect(stack, 'a populated user field must still render its stack container').not.toBeNull(); + expect( + stack!.childElementCount, + 'a populated user field must still render one avatar per user', + ).toBe(2); + expect( + affordance(container), + 'a populated user field must NOT render the No-value affordance', + ).toBeNull(); + }); + + it('NON-REGRESSION — a SCALAR select value still renders its badge', () => { + const { container } = renderCell('select', 'alpha', { options: OPTIONS }); + + expect( + within(container).queryByText('Alpha'), + 'a scalar select value must still draw its badge', + ).not.toBeNull(); + expect( + affordance(container), + 'a scalar select value must NOT render the No-value affordance', + ).toBeNull(); + }); + + it('NON-REGRESSION — a ONE-entry array is a value, even when the entry itself is falsy', () => { + // Refuses a widening spelled as "every entry is falsy" rather than + // "there are no entries": `[0]` has something to draw and draws it. + const { container } = renderCell('multiselect', [0], { options: OPTIONS }); + + expect( + affordance(container), + 'a one-entry array must NOT render the No-value affordance', + ).toBeNull(); + const row = container.querySelector('div.flex.flex-wrap'); + expect(row, 'a one-entry array must still open its multi-value row').not.toBeNull(); + expect(row!.childElementCount, 'a one-entry array renders exactly one entry').toBe(1); + }); + }); + + describe('THE BOUNDARY — the renderers this change deliberately did not move', () => { + it('THE BOUNDARY — `json` still draws the two-character literal for [] and for {}', () => { + // objectui#8474 measured this and kept it: a json-family cell holding + // `[]` was never blank, it printed two characters of punctuation, and + // turning that into a placeholder is a taste change, not a bug fix. + const arr = renderCell('json', []); + expect( + within(arr.container).queryByText('[]'), + '`json` holding [] must still print the array literal', + ).not.toBeNull(); + cleanup(); + + const obj = renderCell('json', {}); + expect( + within(obj.container).queryByText('{}'), + '`json` holding {} must still print the object literal', + ).not.toBeNull(); + }); + + it('THE BOUNDARY — `file` still states its count rather than the affordance', () => { + const { container } = renderCell('file', []); + expect( + within(container).queryByText('0 files'), + '`file` holding [] states a count; it was never blank, so it did not move', + ).not.toBeNull(); + }); + + it('THE BOUNDARY — `{}` is untouched on the three renderers this change DOES move', () => { + // The widening that would have swept `{}` in — `Object.keys(v).length + // === 0` — is separately unsafe (a Date, a populated Map/Set and a + // getter-backed class instance all report zero own keys while + // rendering). This change tests `Array.isArray`, so `{}` cannot reach it. + const { container } = renderCell('multiselect', {}, { options: OPTIONS }); + expect( + affordance(container), + '{} must NOT be swept into the empty-array fix', + ).toBeNull(); + }); + }); +}); diff --git a/packages/fields/src/index.tsx b/packages/fields/src/index.tsx index c16ba226ee..b37e2c6215 100644 --- a/packages/fields/src/index.tsx +++ b/packages/fields/src/index.tsx @@ -1423,6 +1423,40 @@ export function getSemanticHex(name?: string, fallback: string = '#3b82f6'): str return COLOR_NAME_HEX[name] ?? fallback; } +/** + * An array with zero entries is not a cell value (objectui#8481). + * + * Three renderers below open a MULTI-VALUE container and map their entries + * into it — `SelectCellRenderer` (a flex-wrap row of badges/dots), + * `LookupCellRenderer` (a flex-wrap row of record chips) and + * `UserCellRenderer` (an overlapping avatar stack). Each one's opening guard + * tested only `null`/`undefined`/`''`, so `[]` reached the array branch and + * mapped over zero entries: the renderer's whole output was a CHILDLESS + * container — no glyph, no `aria-label`, a visually blank cell. + * + * That blindness lived in the SHARED renderer, so it was the same blank cell + * on every surface. `@object-ui/plugin-detail` had already grown two private + * upstream pre-checks against it (objectui#8474's `hasCellValue`, and + * `RelatedList`'s `isValueEmpty` from objectui#8459); every consumer that does + * NOT pre-check — `ObjectGrid`, `ObjectGallery`, `ObjectKanban`, + * `ObjectDataTable` — reached the renderer directly and painted the blank. + * A renderer with nothing to draw says so itself rather than depending on + * every caller remembering to ask first. + * + * ⛔ Deliberately NOT the package's general emptiness predicate, and + * deliberately not exported. The renderers in this file do NOT agree on what + * "empty" means, and that disagreement is measured and in several places + * intentional: `JsonCellRenderer` draws the two-character literal for `[]` + * (objectui#8474 measured and kept that), `FileCellRenderer` states "0 files", + * `BooleanCellRenderer` treats `false` as a value while `DateCellRenderer`'s + * `!value` treats the epoch as empty. This helper answers ONE question — "is + * this a multi-value container with no entries to draw?" — for the three + * renderers that ask it. Unifying the rest is a separate, contested change. + */ +function isEmptyMultiValue(value: unknown): boolean { + return Array.isArray(value) && value.length === 0; +} + /** * Select field cell renderer. * @@ -1441,7 +1475,10 @@ export function SelectCellRenderer({ value, field }: CellRendererProps): React.R const options: SelectOptionMetadata[] = selectField.options || []; const appearance: 'badge' | 'dot' = selectField.appearance === 'dot' ? 'dot' : 'badge'; - if (value == null || value === '') return ; + // `[]` is handled HERE rather than in the array branch below, because this + // is the statement the renderer makes about having nothing to draw + // (objectui#8481). + if (value == null || value === '' || isEmptyMultiValue(value)) return ; // Match a stored value to a configured option, falling back to a // case-insensitive comparison so seed data with mixed case @@ -1897,7 +1934,10 @@ export function LookupCellRenderer({ value, field }: CellRendererProps): React.R // Always call the hook (rules of hooks). It safely no-ops when inputs are missing. const resolvedName = useLookupName(referenceTo, primaryPrimitiveId, displayField); - if (value == null || value === '') return ; + // Same childless-container defect as `SelectCellRenderer` above: the array + // branch further down opens a flex-wrap row of chips and maps zero entries + // into it (objectui#8481). + if (value == null || value === '' || isEmptyMultiValue(value)) return ; // A reference can arrive as a JSON-encoded object string — e.g. an // unresolved external-id reference '{"externalId":"Website Relaunch"}'. @@ -2065,7 +2105,9 @@ export function FormulaCellRenderer({ value }: CellRendererProps): React.ReactEl * User/Owner field cell renderer (with avatars) */ export function UserCellRenderer({ value }: CellRendererProps): React.ReactElement { - if (!value) return ; + // `!value` never saw `[]` — a truthy empty array reached the avatar-stack + // branch below and rendered an empty stack (objectui#8481). + if (!value || isEmptyMultiValue(value)) return ; // Primitive value: just display the ID/username as text if (typeof value !== 'object') { diff --git a/packages/plugin-grid/src/__tests__/emptyArrayCell-8481.test.tsx b/packages/plugin-grid/src/__tests__/emptyArrayCell-8481.test.tsx new file mode 100644 index 0000000000..6aa49b5666 --- /dev/null +++ b/packages/plugin-grid/src/__tests__/emptyArrayCell-8481.test.tsx @@ -0,0 +1,194 @@ +/** + * 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#8481, consumer half — the surfaces that do NOT pre-check. + * + * `@object-ui/plugin-detail` guards the shared renderers with two private + * upstream predicates (objectui#8474's `hasCellValue`, objectui#8459's + * `RelatedList.isValueEmpty`). `ObjectGrid` has neither: it resolves a + * renderer through `getCellRenderer(...)` and calls it with the RAW value at + * five sites, and its one `EmptyValue` fallback is the no-renderer default + * path, whose guard (`value != null && value !== ''`) carries the very same + * hole one branch over. So a `multiselect` column holding `[]` painted a + * childless flex-wrap DIV — a visually blank cell — in a real grid. + * + * ── Why this file renders BOTH widths ───────────────────────────────────── + * `ObjectGrid` switches to a card layout below 768px (`window.innerWidth < + * 768`), and that layout resolves its own cell renderers. Two independent + * read paths, so both are pinned and the width is set explicitly in each — + * never left at whatever the DOM environment happens to default to. + * + * ── Which of these cases discriminates ──────────────────────────────────── + * ⚠️ Not the blank-cell ones. An emptiness test answering EMPTY for every + * value would satisfy every "[] renders the affordance" case here. The case + * that refuses it is the POPULATED row rendered in the SAME grid: it is the + * only assertion in this file that an over-correction fails. + */ + +import React from 'react'; +import { describe, it, expect, afterEach, beforeAll, vi } from 'vitest'; +import { render, screen, waitFor, cleanup, within } from '@testing-library/react'; +import '@testing-library/jest-dom'; +import { ActionProvider, SchemaRendererProvider } from '@object-ui/react'; +import { registerAllFields } from '@object-ui/fields'; +import { ObjectGrid } from '../ObjectGrid'; + +registerAllFields(); + +const OPTIONS = [ + { value: 'alpha', label: 'Alpha' }, + { value: 'beta', label: 'Beta' }, +]; + +/** One row whose multiselect is `[]`, one whose multiselect is populated. */ +const ROWS = [ + { id: 'r1', name: 'Row one', tags: [] as string[] }, + { id: 'r2', name: 'Row two', tags: ['alpha', 'beta'] }, +]; + +function makeDataSource() { + return { + find: vi.fn(async () => ({ data: ROWS, total: ROWS.length, hasMore: false, pageSize: 50 })), + getObjectSchema: vi.fn(async (name: string) => ({ + name, + fields: { + id: { type: 'text' }, + name: { type: 'text', label: 'Name' }, + tags: { type: 'multiselect', label: 'Tags', options: OPTIONS }, + }, + })), + } as any; +} + +const ORIGINAL_INNER_WIDTH = window.innerWidth; + +function setWidth(px: number) { + Object.defineProperty(window, 'innerWidth', { writable: true, configurable: true, value: px }); +} + +beforeAll(() => { + if (!Element.prototype.scrollIntoView) { + Element.prototype.scrollIntoView = vi.fn() as any; + } +}); + +afterEach(() => { + setWidth(ORIGINAL_INNER_WIDTH); + cleanup(); +}); + +function renderGrid() { + const ds = makeDataSource(); + return render( + + + + + , + ); +} + +/** + * ⚠️ Every lookup below is SCOPED TO ONE ROW, and that is load-bearing. + * Measured: the grid's own toolbar renders a `div.flex.flex-wrap` that is + * legitimately empty when no filter chips are active, so an unscoped + * "no childless flex-wrap anywhere" assertion reads the CHROME and fails + * against a correct grid — an instrument pointed at the wrong thing. + */ +function rowOf(label: string): HTMLElement { + const cell = screen.getByText(label); + const row = cell.closest('tr') ?? cell.closest('[role="row"]') ?? cell.parentElement; + if (!row) throw new Error(`no row found for ${label}`); + return row as HTMLElement; +} + +/** The defect's signature: an element occupying the cell with nothing in it. */ +function childlessFlexWrap(root: HTMLElement): HTMLElement[] { + return Array.from(root.querySelectorAll('div.flex.flex-wrap')).filter( + (el) => el.childElementCount === 0, + ); +} + +const affordances = (root: HTMLElement) => + Array.from(root.querySelectorAll('[data-slot="empty-value"]')); + +describe('objectui#8481 — ObjectGrid paints no blank cell for an empty array', () => { + it('DESKTOP — a multiselect column holding [] renders the No-value affordance', async () => { + setWidth(1280); + renderGrid(); + await waitFor(() => expect(screen.queryByText('Row one')).not.toBeNull()); + const emptyRow = rowOf('Row one'); + + expect( + childlessFlexWrap(emptyRow).length, + 'desktop grid: the [] row must not paint a childless flex-wrap cell (the objectui#8481 defect)', + ).toBe(0); + expect( + affordances(emptyRow).length, + 'desktop grid: the [] cell must carry exactly one No-value affordance', + ).toBe(1); + expect( + affordances(emptyRow)[0]?.getAttribute('aria-label'), + 'desktop grid: the affordance must carry its accessible name', + ).toBe('No value'); + }); + + it('⚠️ DISCRIMINATING — the POPULATED row in the SAME desktop grid still draws its badges', async () => { + // The one case here an EMPTY-for-everything implementation fails. + setWidth(1280); + renderGrid(); + await waitFor(() => expect(screen.queryByText('Row two')).not.toBeNull()); + const filledRow = rowOf('Row two'); + + expect( + within(filledRow).queryByText('Alpha'), + 'the populated row must still draw its first badge', + ).not.toBeNull(); + expect( + within(filledRow).queryByText('Beta'), + 'the populated row must still draw its second badge', + ).not.toBeNull(); + expect( + affordances(filledRow).length, + 'the populated row has nothing empty in it, so it carries no affordance', + ).toBe(0); + }); + + it('MOBILE CARD VIEW — the same column below the 768px breakpoint also renders the affordance', async () => { + setWidth(390); + renderGrid(); + await waitFor(() => expect(screen.queryByText('Row one')).not.toBeNull()); + const emptyCard = rowOf('Row one'); + const filledCard = rowOf('Row two'); + + expect( + childlessFlexWrap(emptyCard).length, + 'mobile card view: the [] card must not paint a childless flex-wrap cell', + ).toBe(0); + expect( + affordances(emptyCard).length, + 'mobile card view: the [] card must carry a No-value affordance', + ).toBeGreaterThan(0); + expect( + within(filledCard).queryByText('Alpha'), + 'mobile card view: the populated card must still draw its badge', + ).not.toBeNull(); + }); +});