From f6f72aaf5a485507d7b95de07c48db6cb1bb2a59 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 7 Sep 2026 23:43:20 +0000 Subject: [PATCH] fix(plugin-detail): one definition of emptiness for the whole record page (#8394) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit objectui#8350 gave the `record:details` dedupe ladder the page H1's authority and objectui#8376 converged `DetailSection`'s three raw `null | undefined | ''` tests onto `hasCellValue`. Four more raw spellings were left on the same page, none trimming, so a whitespace-only value read EMPTY at the H1 and the body grid and FILLED at every band between and around them — each painting a blank. `hasCellValue` moves out of `DetailSection.tsx` into `./emptiness`, and the highlight strip, the summary chips, the audit timeline and the record footer read it. Objects stay values: they go to type-aware renderers that draw them, which is why the scalar half delegates to `recordDisplayValueAt` and the object half does not. Two sites are not a plain predicate swap: * `DetailView` decides this TWICE — `autoSummaryFields`' picker as well as the render. Raw, a whitespace-only `status` won the single status slot and the render then dropped the chip, so a render-only fix turns a blank chip into a missing one. * `RecordMetaFooter` asks it four times over one value and only the last of them reaches `UserRef`; converging that alone leaves `Created by · 5m ago` standing over an actor that is not there. Normalized at the read instead. Pinned on the DOM in `detailPage.emptinessAuthority-8394.test.tsx`, whose non-regression cases are red for a wholesale delegation and red for an emptiness test that answers EMPTY for everything. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01YBWFb5YgMU5dw8p2VKj16S --- .../8394-record-page-emptiness-authority.md | 58 +++ packages/plugin-detail/src/DetailSection.tsx | 70 +-- packages/plugin-detail/src/DetailView.tsx | 15 +- .../plugin-detail/src/HeaderHighlight.tsx | 11 +- .../plugin-detail/src/HistoryTimeline.tsx | 13 +- .../plugin-detail/src/RecordMetaFooter.tsx | 37 +- ...etailPage.emptinessAuthority-8394.test.tsx | 423 ++++++++++++++++++ packages/plugin-detail/src/emptiness.ts | 100 +++++ 8 files changed, 654 insertions(+), 73 deletions(-) create mode 100644 .changeset/8394-record-page-emptiness-authority.md create mode 100644 packages/plugin-detail/src/__tests__/detailPage.emptinessAuthority-8394.test.tsx create mode 100644 packages/plugin-detail/src/emptiness.ts diff --git a/.changeset/8394-record-page-emptiness-authority.md b/.changeset/8394-record-page-emptiness-authority.md new file mode 100644 index 000000000..341e19800 --- /dev/null +++ b/.changeset/8394-record-page-emptiness-authority.md @@ -0,0 +1,58 @@ +--- +'@object-ui/plugin-detail': minor +--- + +The whole record page now shares ONE definition of emptiness, and it **trims** +(objectui#8394). + +objectui#8350 gave `record:details`' dedupe ladder the page H1's authority; +objectui#8376 converged `DetailSection`'s three spellings onto it. Four raw +`null | undefined | ''` tests were left on the same page, none of them trimming. +So for a whitespace-only value the H1 said "empty", the body grid said "empty", +and the bands between and around them said "filled" and painted nothing — a +contradiction visible in a single screenful. + +**What a reader saw, per band.** + +- **The highlight strip** (`HeaderHighlight`, ADR-0085) sits between the H1 and + the body grid, and after objectui#8376 it was the last band on the page still + calling a whitespace-only value FILLED: it painted a **blank chip** where the + em-dash affordance belongs. +- **The summary chips beside the H1** (`DetailView`) rendered a **blank Badge**. + ⭐ And this surface decides emptiness **twice**: the auto-detection that picks + which field becomes a chip asked the same raw question one rung earlier, so a + whitespace-only `status` won the single status slot — and then the render + dropped it, leaving **no status chip at all** where a genuinely filled `stage` + would have shown one. Fixing only the render site would have turned a blank + chip into a missing chip. +- **The audit timeline** (`HistoryTimeline`) printed the spaces instead of the + `—` it uses to mean "nothing". +- **The record footer** (`RecordMetaFooter`) rendered a **blank actor**. ⭐ Here + the fix is at the READ, not at the renderer: four consumers ask "is there an + actor?" about one value — the presence gate, the `sameUser` suppression, the + choice between the `Created by` and the "by"-less `Created` label, and the + gate that actually mounts the renderer. Only the last reaches the renderer, so + converging it alone would have removed the blank and left `Created by · 5m + ago` standing over an actor that is not there — the dangling phrase that label + branch exists to prevent. Normalized once at the read, all four agree. + +**The change.** `hasCellValue` — the predicate objectui#8376 measured into +existence — moves out of `DetailSection.tsx` into a small shared module, and +every band above reads it. Its scalar answer is `@object-ui/core`'s +`recordDisplayValueAt`, the same authority the H1 uses, rather than a fifth +hand-written test. + +**Objects are still values, deliberately.** `recordDisplayValueAt` answers "does +this resolve to a NAME", so an object goes through the Salesforce-style display +chain and is empty when that yields nothing. Right for a title, wrong for a +cell: on these surfaces an object is handed to a type-aware renderer that knows +how to draw it — `{ latitude, longitude }` as coordinates, an option array as +badges, an expanded `{ id, name }` reference through the lookup renderer's own +display chain, anything else as JSON. Delegating that half would have replaced +populated chips, cells and actors with placeholders. This applies to the record +footer too, which the filing card guessed might want the title predicate: it +does not, because its renderer draws objects. + +The only values whose rendering changes are strings that contain nothing but +whitespace. `0`, `false`, `''`, `null`, `undefined` and every object value are +classified exactly as they were. diff --git a/packages/plugin-detail/src/DetailSection.tsx b/packages/plugin-detail/src/DetailSection.tsx index c8f72e5ed..8cacb31b5 100644 --- a/packages/plugin-detail/src/DetailSection.tsx +++ b/packages/plugin-detail/src/DetailSection.tsx @@ -26,7 +26,6 @@ import { } from '@object-ui/components'; import { ChevronDown, ChevronRight, Copy, Check, Eye, EyeOff, Pencil } from 'lucide-react'; import { SchemaRenderer, toRenderableSchema, useInlineEdit } from '@object-ui/react'; -import { recordDisplayValueAt } from '@object-ui/core'; import { getCellRenderer, resolveCellRendererType } from '@object-ui/fields'; import type { DetailViewSection as DetailViewSectionType, DetailViewField, FieldMetadata } from '@object-ui/types'; import { applyDetailAutoLayout } from './autoLayout'; @@ -36,75 +35,13 @@ import { PermissionFacetLink } from './renderers/PermissionFacetLink'; import { NON_EDITABLE_SYSTEM_FIELDS } from './systemFields'; import { InlineFieldInput } from './InlineFieldInput'; import { headerColorClass } from './headerColor'; +import { hasCellValue } from './emptiness'; import { enrichDetailField, isComputedFieldType, isInlineExcludedDetailFieldType, } from './fieldEnrichment'; -/** - * Does this cell have anything to render? **THE** definition of emptiness on - * this surface (objectui#8376) — read by every one of the three places that - * used to spell it out for itself: - * - * - `isEmptyValue`, the row filter behind `emptyCount`, the reader's - * "Show N empty fields" toggle and the auto-hide heuristic; - * - the `isEmpty` branch of `displayValue`, which draws the muted em-dash + - * `No value` affordance; - * - `canCopy`, which offers the copy affordance on the row. - * - * The three MUST agree. "Show N empty fields" means "N rows show the em-dash", - * the skeleton rule ("a section that is entirely empty keeps its labels") means - * "every row shows the em-dash", and a row that says `No value` must not also - * offer to copy that value. Three raw `null | undefined | ''` tests happened to - * agree; one definition cannot stop agreeing. - * - * ## Why the scalar half DELEGATES (objectui#8350's authority) - * - * All three tests were raw and none TRIMMED, so `' '` counted as FILLED - * while `@object-ui/core`'s `recordDisplayValueAt` — the definition the page H1 - * and the `record:details` dedupe ladder both read — calls it EMPTY. The - * consequences were not cosmetic: the row painted a visually blank cell (the - * exact UI the em-dash exists to prevent), it escaped `emptyCount` so the - * toggle read one too low, and because `shouldAutoHideEmpty` only needs - * `filledCount > 0` ONE such value suppressed the all-empty skeleton and hid - * every genuinely empty row in its section. So the scalar answer is not - * re-spelled here: it is the authority's, and a `.trim()` written at this call - * site would be the second implementation that drifts next. - * - * ## Why the OBJECT half does NOT delegate — measured, not assumed - * - * `recordDisplayValueAt` answers "does this resolve to a NAME", so an object - * value goes through `displayNameOfEmbeddedObject` and is EMPTY whenever that - * Salesforce-style chain yields nothing. That is right for a title and WRONG - * for a cell: here an object value is handed to a TYPE-AWARE cell renderer that - * knows how to draw it. `{ latitude, longitude }` renders as coordinates - * (`LocationCellRenderer`), `{ street, city, … }` as a formatted postal address - * (`AddressCellRenderer`, objectui#4037), `['alpha','beta']` as select badges, - * any other object as JSON — none of which carries a name-ish key, so - * delegating this half would replace populated cells with `No value`, drop them - * out of `filledCount`, and let auto-hide bury them. An object is therefore a - * VALUE here, exactly as it was before this change: this function moves - * whitespace-only strings and nothing else. - * - * Pinned end-to-end (DOM, not predicate) in - * `__tests__/DetailSection.emptinessAuthority-8376.test.tsx`, whose - * NON-REGRESSION cases are red for a wholesale delegation. - */ -function hasCellValue(value: unknown): boolean { - // Object/array values belong to the cell renderers, not to the display-name - // chain — see the docblock above. `typeof null === 'object'`, so null is - // excluded here and answered by the authority below. - if (value !== null && typeof value === 'object') return true; - // A one-key synthetic record is how a VALUE asks the authority its question: - // `recordDisplayValueAt` is keyed `(record, field)` because its callers read - // a field off a record, while this site's value has two sources (the record, - // then the authored `field.value` fallback) and is already resolved by the - // time emptiness is asked. Re-typing the test to take a value is precisely - // the extra implementation this function exists to remove. - return recordDisplayValueAt({ value }, 'value') !== undefined; -} - /** * Section-header icon. `fieldGroups[].icon` declares a Lucide name (spec), * so ASCII-identifier-ish values render as the real icon; anything else @@ -225,8 +162,9 @@ export const DetailSection: React.FC = ({ }, []); // Identify empty fields once for both filtering and the toggle counter — - // through `hasCellValue`, the ONE definition this file now shares with the - // em-dash affordance and the copy affordance (objectui#8376). + // through `hasCellValue` in `./emptiness`, the ONE definition this file + // shares with the em-dash affordance, the copy affordance (objectui#8376) + // and, since objectui#8394, every other band of the record page. const isEmptyValue = React.useCallback((field: DetailViewField) => { return !hasCellValue(data?.[field.name] ?? field.value); }, [data]); diff --git a/packages/plugin-detail/src/DetailView.tsx b/packages/plugin-detail/src/DetailView.tsx index 49028b983..d838646f9 100644 --- a/packages/plugin-detail/src/DetailView.tsx +++ b/packages/plugin-detail/src/DetailView.tsx @@ -51,6 +51,7 @@ import { useLocalization, resolveFieldCurrency } from '@object-ui/i18n'; import type { DetailViewSchema, DataSource, ActionSchema, SchemaNode } from '@object-ui/types'; import { useDetailTranslation } from './useDetailTranslation'; import { useRecordEditable } from './useRecordEditable'; +import { hasCellValue } from './emptiness'; /** Default page size for related lists in the detail view */ const DEFAULT_RELATED_PAGE_SIZE = 5; @@ -435,7 +436,12 @@ export const DetailView: React.FC = ({ fieldDefMap[name] = { ...(fieldDefMap[name] || {}), ...def, name }; } } - const has = (n: string) => data?.[n] !== undefined && data?.[n] !== null && data?.[n] !== ''; + // The picker and the chip renderer below MUST ask the same question. This + // spelling is the same defect one rung earlier (objectui#8394): a + // whitespace-only `status` satisfied a raw test, so it won the single + // status slot — and then the render dropped it for being empty, leaving no + // status chip at all where a genuinely filled `stage` would have shown one. + const has = (n: string) => hasCellValue(data?.[n]); const picks: string[] = []; // 1) status / stage / state / select with options const statusKeys = ['status', 'stage', 'state', 'phase']; @@ -976,7 +982,12 @@ export const DetailView: React.FC = ({ {effectiveSummaryFields.map((fieldName) => { const val = data?.[fieldName]; - if (val === null || val === undefined || val === '') return null; + // Same definition as `has` above and as every other band of + // the page (`./emptiness`, objectui#8376/#8394). A raw test + // here rendered a whitespace-only value as a visually blank + // Badge beside the H1 — while the H1's own authority called + // that field empty. + if (!hasCellValue(val)) return null; // Format value based on field type from schema or objectSchema. // Best-effort: currency → localized currency, date/datetime → // localized date string, others → String(val). diff --git a/packages/plugin-detail/src/HeaderHighlight.tsx b/packages/plugin-detail/src/HeaderHighlight.tsx index 5858f00ac..8273834c9 100644 --- a/packages/plugin-detail/src/HeaderHighlight.tsx +++ b/packages/plugin-detail/src/HeaderHighlight.tsx @@ -28,6 +28,7 @@ import { } from './fieldEnrichment'; import { NON_EDITABLE_SYSTEM_FIELDS } from './systemFields'; import { useDetailTranslation } from './useDetailTranslation'; +import { hasCellValue } from './emptiness'; export interface HeaderHighlightProps { fields: HighlightField[]; @@ -168,7 +169,15 @@ export const HeaderHighlight: React.FC = ({ resolvedType === 'textarea' || (!!resolvedType && EXPANDABLE_FIELD_TYPES.has(resolvedType)); const isBoolean = resolvedType === 'boolean'; - const isEmpty = value === null || value === undefined || value === ''; + // The SAME definition the body grid draws its em-dash from + // (`./emptiness`, objectui#8376/#8394). This strip sits between the + // page H1 and the body grid, and the H1's own authority + // (`recordDisplayValueAt`) trims — so a raw test here made this the + // one band on the page still calling a whitespace-only value FILLED + // and painting a blank chip where the em-dash belongs. Objects stay + // values: they go to `CellRenderer` below, which knows how to draw + // them. + const isEmpty = !hasCellValue(value); // Compact-layout UX: an editor (select / date / lookup) needs more // room than a KPI number, so an actively-edited column widens to the diff --git a/packages/plugin-detail/src/HistoryTimeline.tsx b/packages/plugin-detail/src/HistoryTimeline.tsx index eb0a8773b..6cef6a42a 100644 --- a/packages/plugin-detail/src/HistoryTimeline.tsx +++ b/packages/plugin-detail/src/HistoryTimeline.tsx @@ -27,6 +27,7 @@ import { TooltipTrigger, cn, } from '@object-ui/components'; +import { hasCellValue } from './emptiness'; export interface HistoryChange { /** Raw field name from the schema (e.g. "industry"). */ @@ -130,8 +131,18 @@ function initialsFromName(name?: string | null): string { .join(''); } +/** + * An audit value as the timeline shows it, or the `'—'` that means "nothing". + * + * Emptiness is the record page's ONE definition (`./emptiness`, + * objectui#8376/#8394), not a raw test: a whitespace-only stored value used to + * fall through to the `typeof value === 'string'` branch below and print its + * spaces, so the cell the timeline means to read as "nothing" rendered blank + * instead of the em-dash. Objects are still values here for the same reason + * they are in a cell — the `JSON.stringify` branch below draws them. + */ function formatDiffValue(value: unknown): string { - if (value === null || value === undefined || value === '') return '—'; + if (!hasCellValue(value)) return '—'; if (typeof value === 'string') return value; if (typeof value === 'number' || typeof value === 'boolean') return String(value); try { diff --git a/packages/plugin-detail/src/RecordMetaFooter.tsx b/packages/plugin-detail/src/RecordMetaFooter.tsx index 2c963e987..4e80a845a 100644 --- a/packages/plugin-detail/src/RecordMetaFooter.tsx +++ b/packages/plugin-detail/src/RecordMetaFooter.tsx @@ -12,6 +12,7 @@ import { getCellRenderer, resolveCellRendererType } from '@object-ui/fields'; import { AUDIT_FIELD_BY_ROLE } from '@object-ui/types'; import type { FieldMetadata } from '@object-ui/types'; import { useDetailTranslation } from './useDetailTranslation'; +import { hasCellValue } from './emptiness'; /** * Audit field names auto-injected by the framework's `applySystemFields` — @@ -44,6 +45,34 @@ function toDate(value: unknown): Date | null { return null; } +/** + * The actor a `created_by` / `updated_by` column names, or `undefined` when the + * record names none. + * + * ⚠️ Normalized HERE, at the read, and not inside `UserRef` — this is the one + * site on the record page where converging the emptiness predicate at the + * renderer would have been the wrong fix (objectui#8394). FOUR consumers below + * ask "is there an actor?" about the same value: `hasCreated` / `hasUpdated`, + * the `sameUser` comparison that suppresses a redundant "Updated" segment, the + * `label` choice between `detail.createdBy` and the "by"-less `detail.created`, + * and `MetaEntry`'s own `{user ? … }` gate — and only that last one ever + * reaches `UserRef`. So a whitespace-only `created_by` fixed at the renderer + * alone would still pick the "Created by" label and still draw the `·` + * separator, rendering "Created by · 5m ago" with nothing in between: exactly + * the dangling phrase the label branch exists to prevent, just with the blank + * moved. Answered once, all four agree. + * + * Emptiness is the record page's ONE definition (`./emptiness`), so an EXPANDED + * reference payload (`{ id, name }`) stays an actor — `UserRef` hands objects + * to `LookupCellRenderer`, which resolves them through its own display chain. + * The card guessed this site might want the TITLE predicate instead; it does + * not, and that is why: a bare `{ id }` payload has no display name, yet the + * renderer still draws it. + */ +function actorOrNone(value: T): T | undefined { + return hasCellValue(value) ? value : undefined; +} + function formatRelativeTime(date: Date, t: TFn): string { const ms = Date.now() - date.getTime(); // Future timestamps (clock skew, scheduled records) — fall through to "just now". @@ -79,7 +108,9 @@ interface UserRefProps { * as DetailSection so reference resolution (ID → display name) is consistent. */ const UserRef: React.FC = ({ value, objectSchema, fieldName }) => { - if (value === null || value === undefined || value === '') return null; + // Defensive floor only: `RecordMetaFooter` already normalizes through + // `actorOrNone`, so this agrees with the caller rather than deciding alone. + if (!hasCellValue(value)) return null; const fieldDef = objectSchema?.fields?.[fieldName]; // created_by / updated_by are ALWAYS user references on ObjectStack, but many // fetched schemas omit the audit system fields from `fields`. Without a @@ -180,8 +211,8 @@ export const RecordMetaFooter: React.FC = ({ const createdAt = toDate(data[AUDIT_FIELDS.createdAt]); const updatedAt = toDate(data[AUDIT_FIELDS.updatedAt]); - const createdBy = data[AUDIT_FIELDS.createdBy]; - const updatedBy = data[AUDIT_FIELDS.updatedBy]; + const createdBy = actorOrNone(data[AUDIT_FIELDS.createdBy]); + const updatedBy = actorOrNone(data[AUDIT_FIELDS.updatedBy]); const hasCreated = !!(createdAt || createdBy); // Treat updated_at within ~2s of created_at as "never touched" — covers diff --git a/packages/plugin-detail/src/__tests__/detailPage.emptinessAuthority-8394.test.tsx b/packages/plugin-detail/src/__tests__/detailPage.emptinessAuthority-8394.test.tsx new file mode 100644 index 000000000..c8f5cf98b --- /dev/null +++ b/packages/plugin-detail/src/__tests__/detailPage.emptinessAuthority-8394.test.tsx @@ -0,0 +1,423 @@ +/** + * 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 WHOLE record page has one definition of emptiness, and it TRIMS + * (objectui#8394, widening objectui#8376 past `DetailSection`). + * + * ## The defect + * + * objectui#8350 gave the `record:details` dedupe ladder the H1's authority; + * objectui#8376 converged `DetailSection`'s three raw spellings onto + * `hasCellValue`. Four more raw `null | undefined | ''` tests were left on the + * SAME page, none of them trimming — so for a whitespace-only value the H1 said + * "empty", the body grid said "empty", and the bands between and around them + * said "filled" and painted nothing: + * + * - `HeaderHighlight` — the ADR-0085 strip, one band above the body grid and + * directly under the H1: a blank chip where the em-dash belongs; + * - `DetailView` — the `summaryFields` chips beside the H1: a blank Badge; + * - `HistoryTimeline` — a blank audit cell where `'—'` means "nothing"; + * - `RecordMetaFooter` — a blank actor slot. + * + * ## Two sites are NOT a simple predicate swap, and the cases say so + * + * ⭐ `DetailView` decides this TWICE. `autoSummaryFields`' `has()` picks which + * field becomes a chip, and the render decides whether that chip draws. They + * must give the same answer: with `status` holding only spaces, the raw picker + * spent the single status slot on it, and a render-only fix then dropped the + * chip — leaving NO status chip where a genuinely filled `stage` would have + * shown one. `PICKER AND RENDER AGREE` is red for a fix applied only at the + * render site (the line the card names). + * + * ⭐ `RecordMetaFooter` asks it FOUR times over one value — `hasCreated`, the + * `sameUser` suppression, the `label` choice, and `MetaEntry`'s `{user ? … }` + * gate — and only the last reaches `UserRef` (the line the card names). Fixing + * `UserRef` alone leaves the "Created by" label and the `·` separator in place + * over an actor that is not there. `THE "BY"-LESS LABEL` is red for that + * partial fix, so the footer is normalized at the READ instead. + * + * ## Controls + * + * "The blank is gone" is trivially true of a document that rendered nothing, so + * every case asserts a sibling that DID render, by value. `NON-REGRESSION — an + * emptiness test that answers EMPTY for everything` is the case that is red for + * deleting the feature, which every other case here would otherwise accept, and + * `NON-REGRESSION — object-valued highlights` is the case that is red for + * delegating the object half to `recordDisplayValueAt` wholesale. + * + * ## Reading the affordances + * + * Two different placeholders can carry `aria-label="No value"` on this page: + * `HeaderHighlight`'s own span, and `@object-ui/components`' `EmptyValue`, which + * the field cell renderers draw. Only the latter carries + * `data-slot="empty-value"`, so the strip's own affordance is read as + * `[aria-label="No value"]:not([data-slot="empty-value"])` and cannot pick up a + * cell renderer's placeholder by accident. The summary chips are read through + * the `aria-label=": "` each `Badge` already carries, which + * names the field — so "which chip" is asserted, not merely "how many". + */ + +import { describe, it, expect, beforeAll, afterEach } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import * as React from 'react'; +import { recordDisplayValueAt } from '@object-ui/core'; +import { HeaderHighlight } from '../HeaderHighlight'; +import { DetailView } from '../DetailView'; +import { HistoryTimeline, type HistoryEntry } from '../HistoryTimeline'; +import { RecordMetaFooter } from '../RecordMetaFooter'; +import { hasCellValue } from '../emptiness'; +import type { DetailViewSchema } from '@object-ui/types'; + +/** + * Desktop. `DetailView` reads `useIsMobile()` for its own layout, and the + * `DetailSection`s it renders switch auto-hide thresholds on it — pinned rather + * than inherited from happy-dom's default so no assertion here is green only + * because of an unpinned viewport. + */ +beforeAll(() => { + Object.defineProperty(window, 'innerWidth', { configurable: true, value: 1280 }); +}); + +afterEach(cleanup); + +/** Presence as a VALUE — `getByText` would throw before `expect` ran and the + * CI summary would carry none of the reason. */ +const shown = (text: string) => screen.queryByText(text) !== null; + +/** `HeaderHighlight`'s OWN em-dash affordance — see the docblock. */ +const stripAffordances = (c: HTMLElement) => + c.querySelectorAll('[aria-label="No value"]:not([data-slot="empty-value"])'); + +/** The summary chip for `field`, read by the `aria-label` the Badge carries. */ +const chipFor = (c: HTMLElement, field: string) => + c.querySelector(`[aria-label^="${field}: "]`); + +const highlightSchema = { + fields: { + industry: { type: 'text', label: 'Industry' }, + notes: { type: 'text', label: 'Notes' }, + amount: { type: 'number', label: 'Amount' }, + office_location: { type: 'location', label: 'Office Location' }, + tags: { + type: 'multiselect', + label: 'Tags', + options: [ + { value: 'alpha', label: 'Alpha' }, + { value: 'beta', label: 'Beta' }, + ], + }, + }, +}; + +const highlights = (names: string[]) => + names.map((name) => ({ + name, + // The strip labels a chip from `HighlightField.label`; declared here so the + // controls below can find a chip by name without depending on how an + // i18n-less test environment renders a missing label. + label: (highlightSchema.fields as Record)[name].label, + })) as any; + +describe('HeaderHighlight — the ADR-0085 strip trims (#8394)', () => { + it('AFFORDANCE — a whitespace-only highlight draws the `No value` em-dash, not a blank chip', () => { + // The card's headline site: after objectui#8376 this was the ONLY band on + // the page still calling a whitespace-only value FILLED, and it sits + // between the H1 (which calls it empty) and the body grid (which now does + // too) — the contradiction is visible in one screenful. + const { container } = render( + , + ); + + // CONTROLS — the strip rendered, and both ordinary chips rendered BY VALUE. + expect(shown('Manufacturing'), 'CONTROL: the filled text chip rendered').toBe(true); + expect(shown('42'), 'CONTROL: the filled number chip rendered').toBe(true); + // CONTROL — the whitespace chip's own slot exists, so the assertion below + // is about what that chip DRAWS and not about whether it is on screen. + expect(shown('Notes'), 'CONTROL: the whitespace-only chip is on screen at all').toBe(true); + + expect( + stripAffordances(container), + 'exactly the whitespace-only chip draws the strip\'s `No value` em-dash', + ).toHaveLength(1); + expect( + container.textContent, + 'the raw whitespace must not reach the DOM as a rendered value', + ).not.toMatch(/ {3}/); + }); + + it('NON-REGRESSION — object-valued highlights are still FILLED (the half that must NOT delegate)', () => { + // `recordDisplayValueAt` calls both of these EMPTY — neither carries a + // name-ish key — yet both render real content through their type-aware cell + // renderer. This case is RED for a wholesale delegation, which is what the + // card warns the fix must not be. + const { container } = render( + , + ); + + expect(shown('Manufacturing'), 'CONTROL: the ordinary filled chip rendered').toBe(true); + expect( + container.textContent, + 'a geolocation object renders as coordinates, not as the em-dash', + ).toContain('30.2741, 120.1551'); + expect(shown('Alpha'), 'a multiselect array renders its options').toBe(true); + expect(shown('Beta'), 'a multiselect array renders its options').toBe(true); + expect( + stripAffordances(container), + 'NO chip here is empty — an object value is a value on this surface', + ).toHaveLength(0); + }); + + it('NON-REGRESSION — an emptiness test that answers EMPTY for everything is refused (`0` and a string)', () => { + // Every whitespace case above is ALSO satisfied by deleting the feature. + // This one is not: `0` is the value a careless `!value` rewrite loses. + const { container } = render( + , + ); + + expect(shown('Manufacturing'), 'a plain string is a value').toBe(true); + expect(shown('0'), '`0` is a value, not a blank').toBe(true); + expect( + stripAffordances(container), + 'no chip draws the em-dash — nothing here is empty', + ).toHaveLength(0); + }); +}); + +describe('DetailView — the summary chips beside the H1 trim (#8394)', () => { + const render8394 = (schema: Partial) => + render(); + + it('CHIP — a whitespace-only summary field renders no chip, and its neighbour still does', () => { + const { container } = render8394({ + data: { id: 'D1', name: 'Acme', notes: ' ', amount: 42 }, + summaryFields: ['notes', 'amount'] as any, + fields: [{ name: 'notes', label: 'Notes' }, { name: 'amount', label: 'Amount' }] as any, + }); + + // CONTROL — the page rendered its H1 at all. + expect(container.querySelector('h1'), 'CONTROL: the header rendered').not.toBeNull(); + // CONTROL — the sibling chip IS there, so the absence below is a decision + // about this value and not about the chip row. + expect( + chipFor(container, 'amount'), + 'CONTROL: the filled summary field still renders its chip', + ).not.toBeNull(); + + expect( + chipFor(container, 'notes'), + 'a whitespace-only summary field must render no chip at all, not a blank one', + ).toBeNull(); + }); + + it('⭐ PICKER AND RENDER AGREE — a whitespace-only `status` must not consume the auto-detected status slot', () => { + // `autoSummaryFields` decides emptiness a SECOND time, one rung before the + // render. Raw, it picked `status` (spaces satisfy `!== ''`), and a + // render-only fix then dropped that chip — so the header showed NO status + // chip, where the genuinely filled `stage` should have taken the slot. + const { container } = render8394({ + data: { id: 'D2', name: 'Acme', status: ' ', stage: 'Won' }, + fields: [{ name: 'status', label: 'Status' }, { name: 'stage', label: 'Stage' }] as any, + }); + + expect(container.querySelector('h1'), 'CONTROL: the header rendered').not.toBeNull(); + + expect( + chipFor(container, 'status'), + 'the whitespace-only `status` is not a chip', + ).toBeNull(); + expect( + chipFor(container, 'stage'), + 'the auto-detected status slot goes to `stage`, which actually has a value — a render-only fix leaves this slot EMPTY', + ).not.toBeNull(); + expect( + chipFor(container, 'stage')!.getAttribute('aria-label'), + 'and the chip that took the slot reads its value', + ).toBe('stage: Won'); + }); +}); + +describe('HistoryTimeline — the audit placeholder trims (#8394)', () => { + const entry = (changes: HistoryEntry['changes']): HistoryEntry => ({ + id: 'h1', + action: 'update', + user_name: 'Jane Doe', + created_at: '2024-06-01T00:00:00Z', + changes, + }); + + it('PLACEHOLDER — a whitespace-only audit value reads as `—`, not as a blank cell', () => { + const { container } = render( + , + ); + + // CONTROLS — the entry rendered, and the sibling row kept both its values. + expect(shown('Jane Doe'), 'CONTROL: the timeline entry rendered').toBe(true); + expect(shown('Acme Corp'), 'CONTROL: the sibling change row renders its new value').toBe(true); + // CONTROL — this row's OLD value is untouched, so the assertion below is + // about the new value only and not about the row disappearing. + expect(shown('finance'), 'CONTROL: the `from` value of the same row still renders').toBe(true); + + expect( + container.textContent, + 'the whitespace-only `to` value reads as the em-dash placeholder', + ).toContain('finance → —'); + expect( + container.textContent, + 'the raw whitespace must not reach the DOM as a rendered value', + ).not.toMatch(/ {3}/); + }); + + it('NON-REGRESSION — object and falsy audit values are still values', () => { + const { container } = render( + , + ); + + expect(shown('Jane Doe'), 'CONTROL: the timeline entry rendered').toBe(true); + expect( + container.textContent, + 'an object audit value is still JSON, not the placeholder', + ).toContain('{"lat":1,"lng":2}'); + expect( + container.textContent, + '`false` is a value — an emptiness test answering EMPTY for everything fails here', + ).toContain('true → false'); + }); +}); + +describe('RecordMetaFooter — an actor is normalized at the READ (#8394)', () => { + const footer = (data: Record) => + render(); + + it('⭐ THE "BY"-LESS LABEL — a whitespace-only `created_by` is no actor at all', () => { + // Fixing only `UserRef` (the line the card names) removes the blank but + // leaves BOTH the "Created by" label and the `·` separator standing over + // an actor that is not there — "Created by · 5m ago", the dangling phrase + // the label branch exists to prevent. That is why the footer normalizes at + // the read: this case is red for the renderer-only fix. + const { container } = footer({ + created_at: '2024-06-01T00:00:00Z', + created_by: ' ', + }); + + // CONTROL — the footer rendered at all. + expect(screen.getByTestId('record-meta-footer'), 'CONTROL: the footer rendered').toBeTruthy(); + + expect(shown('Created'), 'the "by"-less label is used when there is no actor').toBe(true); + expect(shown('Created by'), 'the "Created by" label must not stand over a missing actor').toBe( + false, + ); + expect( + container.textContent, + 'no actor means no `·` separator either', + ).not.toContain('·'); + expect( + container.textContent, + 'the raw whitespace must not reach the DOM as a rendered actor', + ).not.toMatch(/ {3}/); + }); + + it('NON-REGRESSION — a real actor still labels, separates and renders (id and expanded object)', () => { + // The control for the case above AND the object half: an expanded + // `{ id, name }` reference is a value here because `UserRef` hands objects + // to `LookupCellRenderer`, which resolves them through its own display + // chain. This case is red for an emptiness test that answers EMPTY for + // everything. + const { container } = footer({ + created_at: '2024-06-01T00:00:00Z', + created_by: { id: 'u1', name: 'Jane Doe' }, + }); + + expect(screen.getByTestId('record-meta-footer'), 'CONTROL: the footer rendered').toBeTruthy(); + expect(shown('Created by'), 'a real actor takes the "by" label').toBe(true); + expect(shown('Jane Doe'), 'an expanded reference object still renders its name').toBe(true); + expect(container.textContent, 'a real actor keeps the `·` separator').toContain('·'); + }); + it('NON-REGRESSION — a bare `{ id }` actor still renders (the footer\'s object half must NOT delegate)', () => { + // MEASURED on this branch, not assumed: `LookupCellRenderer` draws `u1` for + // this payload (an opaque 32-char id is the case it hides behind its own + // placeholder — objectui#2688 — and this is not that). The TITLE authority + // calls a bare `{ id }` payload EMPTY, so delegating the object half here + // would drop the actor entirely: the label would fall back to the + // "by"-less `Created` and `u1` would leave the screen. This is the case + // that makes the footer\'s object half load-bearing. + const { container } = footer({ + created_at: '2024-06-01T00:00:00Z', + created_by: { id: 'u1' }, + }); + + expect(screen.getByTestId('record-meta-footer'), 'CONTROL: the footer rendered').toBeTruthy(); + expect(shown('Created by'), 'a bare `{ id }` payload is still an actor').toBe(true); + expect( + container.textContent, + 'the reference renderer draws it — a wholesale delegation would blank it', + ).toContain('u1'); + }); +}); + +describe('The one authority, asserted rather than cited (#8394)', () => { + /** + * An ADDITION to the DOM cases above, never a substitute. It measures the + * shared predicate directly so the split the whole page depends on is stated + * once, in one place, instead of being inferred from five renders. + */ + it('MEASUREMENT — `hasCellValue` trims scalars and keeps objects, and the authority does not', () => { + expect(hasCellValue(' '), 'whitespace-only is EMPTY').toBe(false); + expect(hasCellValue(''), 'empty string is EMPTY').toBe(false); + expect(hasCellValue(null), 'null is EMPTY').toBe(false); + expect(hasCellValue(undefined), 'undefined is EMPTY').toBe(false); + expect(hasCellValue(0), '`0` is a value').toBe(true); + expect(hasCellValue(false), '`false` is a value').toBe(true); + expect(hasCellValue('Acme'), 'a plain string is a value').toBe(true); + + // The object half — a value HERE, and empty to the title authority. These + // two expectations are the measurement behind the split; they disagree on + // purpose. + const geo = { latitude: 30.2741, longitude: 120.1551 }; + expect(hasCellValue(geo), 'a geolocation object is a cell VALUE').toBe(true); + expect( + recordDisplayValueAt({ value: geo }, 'value'), + 'the same object has no display NAME — a wholesale delegation would blank it', + ).toBeUndefined(); + expect(hasCellValue(['alpha', 'beta']), 'an option array is a cell VALUE').toBe(true); + expect( + recordDisplayValueAt({ value: ['alpha', 'beta'] }, 'value'), + 'the same array has no display NAME', + ).toBeUndefined(); + }); +}); diff --git a/packages/plugin-detail/src/emptiness.ts b/packages/plugin-detail/src/emptiness.ts new file mode 100644 index 000000000..d765171ff --- /dev/null +++ b/packages/plugin-detail/src/emptiness.ts @@ -0,0 +1,100 @@ +/** + * 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 { recordDisplayValueAt } from '@object-ui/core'; + +/** + * Does this cell have anything to render? **THE** definition of emptiness on + * the `record:details` page (objectui#8376, widened to the whole package by + * objectui#8394). + * + * ## Readers + * + * Every band of the record page that draws a placeholder instead of a value, or + * omits a slot because there is no value, asks THIS — never its own + * `null | undefined | ''`: + * + * - `DetailSection` — `isEmptyValue` (the row filter behind `emptyCount`, the + * "Show N empty fields" toggle and the auto-hide heuristic), the `isEmpty` + * branch of `displayValue` (the muted em-dash + `No value` affordance), and + * `canCopy` (a row that says `No value` must not offer to copy it); + * - `HeaderHighlight` — the ADR-0085 highlight strip's `isEmpty`, the same + * em-dash affordance one band higher on the same screen; + * - `DetailView` — the `summaryFields` chips beside the page H1, BOTH the + * auto-detection that picks which field becomes a chip and the render that + * skips a valueless one. Those two must agree or the derivation spends the + * one status slot on a field the render then drops, and no chip appears; + * - `HistoryTimeline` — `formatDiffValue`'s `'—'` placeholder for an audit + * value; + * - `RecordMetaFooter` — whether `created_by` / `updated_by` is an actor at + * all (see the note at its call site: the footer normalizes at the READ, + * because the label, the `·` separator and the renderer are three consumers + * of one answer). + * + * They MUST agree. Before objectui#8350 / #8376 / #8394 each spelled the test + * out for itself, none trimmed, and the page contradicted itself in one + * screenful: the H1 called a whitespace-only field empty, the body grid painted + * a blank cell, the strip between them painted a blank chip. Raw tests that + * happen to agree can stop agreeing; one definition cannot. + * + * ## Why the scalar half DELEGATES (objectui#8350's authority) + * + * None of those tests TRIMMED, so `' '` counted as FILLED while + * `@object-ui/core`'s `recordDisplayValueAt` — the definition the page H1 and + * the `record:details` dedupe ladder both read — calls it EMPTY. The + * consequences were not cosmetic: a row painted a visually blank cell (the exact + * UI the em-dash exists to prevent), it escaped `emptyCount` so the toggle read + * one too low, and because `shouldAutoHideEmpty` only needs `filledCount > 0` + * ONE such value suppressed the all-empty skeleton and hid every genuinely empty + * row in its section. So the scalar answer is not re-spelled here: it is the + * authority's, and a `.trim()` written at a call site would be the second + * implementation that drifts next. + * + * ## Why the OBJECT half does NOT delegate — measured, not assumed + * + * `recordDisplayValueAt` answers "does this resolve to a NAME", so an object + * value goes through `displayNameOfEmbeddedObject` and is EMPTY whenever that + * Salesforce-style chain yields nothing. That is right for a title and WRONG for + * a cell: here an object value is handed to a TYPE-AWARE renderer that knows how + * to draw it. `{ latitude, longitude }` renders as coordinates + * (`LocationCellRenderer`), `{ street, city, … }` as a formatted postal address + * (`AddressCellRenderer`, objectui#4037), `['alpha','beta']` as select badges, + * an expanded `{ id, name }` reference through `LookupCellRenderer`'s own + * display chain, any other object as JSON — none of which carries a name-ish key + * in the general case, so delegating this half would replace populated cells + * with `No value`, drop them out of `filledCount`, and let auto-hide bury them. + * An object is therefore a VALUE here: this function moves whitespace-only + * strings and nothing else. + * + * ## Not every emptiness question on this page is THIS question + * + * ⛔ Do not widen the reader list by pattern-matching on the shape of a test. + * `ConcurrentUpdateDialog`'s `formatValue` deliberately renders `''` as `""` + * rather than as a placeholder — a conflict dialog is reporting what is STORED, + * where "empty string" and "absent" are different facts the reader must be able + * to tell apart. Converging it would delete information rather than add it. + * + * Pinned end-to-end (DOM, not predicate) in + * `__tests__/DetailSection.emptinessAuthority-8376.test.tsx` and + * `__tests__/detailPage.emptinessAuthority-8394.test.tsx`, whose NON-REGRESSION + * cases are red for a wholesale delegation and red for an emptiness test that + * answers EMPTY for everything. + */ +export function hasCellValue(value: unknown): boolean { + // Object/array values belong to the cell renderers, not to the display-name + // chain — see the docblock above. `typeof null === 'object'`, so null is + // excluded here and answered by the authority below. + if (value !== null && typeof value === 'object') return true; + // A one-key synthetic record is how a VALUE asks the authority its question: + // `recordDisplayValueAt` is keyed `(record, field)` because its callers read + // a field off a record, while a call site's value has several sources (the + // record, then an authored `field.value` fallback) and is already resolved by + // the time emptiness is asked. Re-typing the test to take a value is precisely + // the extra implementation this function exists to remove. + return recordDisplayValueAt({ value }, 'value') !== undefined; +}