diff --git a/.changeset/record-details-retire-layout-input-3818.md b/.changeset/record-details-retire-layout-input-3818.md new file mode 100644 index 000000000..049335d15 --- /dev/null +++ b/.changeset/record-details-retire-layout-input-3818.md @@ -0,0 +1,15 @@ +--- +'@object-ui/plugin-detail': patch +--- + +`record:details` stops publishing a `layout` key the spec removed and the renderer never honoured + +`record:details` declared `layout: enum ['auto','custom']` with `defaultValue: 'auto'` and the description "auto uses the object highlightFields; custom uses explicit sections". None of that was ever implemented. The renderer's only `schema.layout` read tested `'inline'` | `'compact'` — two values the schema never permitted — so both legal values fell through the same ternary and the key selected nothing. `auto` and `custom` have behaved identically for as long as both have existed. + +Two directions were wrong with zero diagnostics: `layout: 'auto'` plus explicit `sections` still rendered the sections, and `layout: 'custom'` with no sections silently fell back to the flat body rather than reporting the missing groups. Because the input carried a `defaultValue`, this was not stale documentation — it was the manifest, the generated `sdui-intrinsics.d.ts` and the designer panel actively offering the key. An AI author writing `layout: 'custom'` believed it took effect. + +`@objectstack/spec` 17.0.0 removed the property (objectstack#6946, ADR-0087 D2); `17.0.0-rc.6` is pinned here, so the key is already rejected on parse with a named migration message pointing at `os migrate meta --from 16`. This release completes the objectui half of that retirement: the input declaration is gone, and so is the dead `inline`/`compact` branch — the synthesized layout is now the constant it always resolved to. + +Nothing that worked stops working. The body-source contract is unchanged and is now the only one declared: **`sections` renders the explicit groups; omitting it falls back to the flat body derived from the object's fields.** That is pinned in both directions, plus the empty-array boundary between them, in `recordDetailsBodySource.test.tsx`. + +One gate got sharper on the way through. The parity test's "declares no top-level input the spec does not accept" check read raw `.shape` keys — but an ADR-0087 D2 tombstone stays *in* the shape as a `z.never()`, so a retired key still answers "is this declared?" with yes. That is precisely why this input survived the rc.6 pin bump with every derived gate green. The check now filters tombstoned members out, so it catches the next D2 retirement instead of waving it through. diff --git a/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts b/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts index a1b355c6d..e1c815bb5 100644 --- a/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts +++ b/apps/console/src/__tests__/registry-inputs-spec-parity.test.ts @@ -434,6 +434,32 @@ const UNPUBLISHED_EXEMPTIONS: Record = { 'page:card.body': 'Retired upstream by objectstack#5775 / PR objectstack#6281 (ADR-0087 D2 tombstone, converging on the `children` this renderer reads and now publishes); declaring it would publish a key the spec rejects by name — objectui#4027. Listed here only because the pinned @objectstack/spec@17.0.0-rc.5 predates the retirement. Resolves via objectui#3809, not via the pin bump.', + // ── record:details.layout — retired upstream AND withdrawn here (1 key) ─── + // The fifth D2 tombstone, and the first one whose objectui half has actually + // landed — so it is here for a DIFFERENT reason than the four above, and the + // difference is worth reading before treating it as more of the same. + // + // Those four are stale-pin cover: the key is still published in this repo and + // the entry says "the pin predates the retirement". This one is the opposite. + // objectui#3818 DELETED the `record:details` `layout` input (the spec's + // `auto` | `custom` semantics were never implemented — the renderer's only + // read tested `inline` | `compact`, values the schema never permitted, so both + // legal values took the same branch and the key selected nothing), which is + // exactly what this gate's forward direction wants. The entry exists because + // the REVERSE direction then demands the key back: `specTopLevelKeys` reads + // raw `Object.keys(shape)`, the ADR-0087 D2 tombstone is still an entry in + // that shape, and so a key the spec rejects by name reads as "declared, and + // you failed to publish it". + // + // That is objectui#3809's blind spot seen from the other side — it predicted a + // false GREEN in the forward direction, and this is the same root cause + // producing a false RED in the reverse one. Both vanish together when #3809 + // narrows `specTopLevelKeys` to non-tombstone members; this entry then goes + // stale and `carries no stale unpublished-key exemption` will name it, along + // with the four above. Do not resolve it by re-adding the input. + 'record:details.layout': + 'Retired upstream by objectstack#6946 (ADR-0087 D2 tombstone) and withdrawn here by objectui#3818 — its published `auto` | `custom` semantics were never implemented, and the spec now rejects the key by name, so publishing it again would teach a key the contract refuses. Unlike the stale-pin entries above this one is live at @objectstack/spec@17.0.0-rc.6: the tombstone stays in `Object.keys(shape)`, so the reverse direction demands a key the forward direction forbids. Resolves via objectui#3809 tombstone recognition, not by declaring the input.', + // `element:record_picker.filter` was the ninth entry here — a real A-class gap // that fell out of objectui#3808's three-class triage, exempted only because it // was outside that PR's dispatched scope. objectui#3830 declared the input, so diff --git a/packages/plugin-detail/src/__tests__/recordDetailsBodySource.test.tsx b/packages/plugin-detail/src/__tests__/recordDetailsBodySource.test.tsx new file mode 100644 index 000000000..da24afa63 --- /dev/null +++ b/packages/plugin-detail/src/__tests__/recordDetailsBodySource.test.tsx @@ -0,0 +1,114 @@ +/** + * 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. + */ + +/** + * `record:details` — what chooses the detail body (objectui#3818). + * + * The positive control for the `layout` retirement. `record:details` published + * `layout: enum ['auto','custom']` describing itself as the body selector + * ("auto uses the object highlightFields; custom uses explicit sections"), but + * the renderer's only read tested `'inline'` | `'compact'` — values the schema + * never permitted — so both legal values took the same branch and the key + * selected nothing. @objectstack/spec 17.0.0 removed it (objectstack#6946, + * ADR-0087 D2) and objectui#3818 removes the input and the dead branch here. + * + * Deleting a selector is only safe if the REAL selector is pinned, so this + * file pins the contract that was doing the work all along and is now the + * documented one: **what you author decides the body.** `sections` renders the + * explicit groups (the old `custom`); omitting it falls back to the flat body + * (the old `auto`). Both directions, because the failure this guards is a body + * that silently comes up wrong — or blank — with no diagnostic anywhere. + * + * WHY THERE IS NO "authoring `layout` changes nothing" ASSERTION HERE. It + * would be vacuous, and the measurement that says so is worth recording: the + * deleted branch fed `synthesized.layout`, and `DetailView` never reads + * `layout` at all (no read site in the file, and it does not spread the schema + * onward). So the branch was dead TWICE — retired vocabulary AND ignored + * consumer — and restoring it changes no rendered output. A DOM-equality pin + * would therefore stay green through the exact regression it appears to guard. + * The read point is pinned where it can actually go red: the source-text and + * published-surface assertions in `recordDetailsInputs.spec-parity.test.ts`. + */ + +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import * as React from 'react'; +import { RecordContextProvider } from '@object-ui/react'; +import { RecordDetailsRenderer } from '../renderers/record-details'; + +/** + * No `name` / `title` / `display_name` in the data on purpose: the renderer + * drops the page-H1 title field from the body (`titleCandidates`), and a + * fixture that tripped that would make an absence assertion below pass for the + * wrong reason. + */ +const objectSchema = { + fields: { + phone: { type: 'text', label: 'Phone' }, + email: { type: 'text', label: 'Email' }, + industry: { type: 'text', label: 'Industry' }, + }, +}; + +const data = { + phone: '555-0100', + email: 'ops@acme.test', + industry: 'Manufacturing', +}; + +const renderDetails = (schema: Record) => + render( + + + , + ); + +describe('record:details — `sections` presence decides the body (#3818)', () => { + it('renders the authored groups when `sections` is present (the old `custom`)', () => { + renderDetails({ + sections: [ + { name: 'contact_info', label: 'Contact Info', fields: ['phone', 'email'] }, + ], + }); + + // The group heading and its fields are on screen... + expect(screen.getByText('Contact Info')).toBeInTheDocument(); + expect(screen.getByText('555-0100')).toBeInTheDocument(); + expect(screen.getByText('ops@acme.test')).toBeInTheDocument(); + + // ...and a field OUTSIDE every section is not, which is the half that + // matters: once `sections` is authored it is the ONLY source of the body. + // `industry` is present in both the object schema and the data, so its + // absence is a decision by `sections`, not missing input. + expect(screen.queryByText('Manufacturing')).not.toBeInTheDocument(); + }); + + it('falls back to the flat field body when `sections` is absent (the old `auto`)', () => { + renderDetails({ fields: ['industry'] }); + + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + // No section chrome: the flat arm is a single unlabelled group, so the + // heading from the other direction must not appear. + expect(screen.queryByText('Contact Info')).not.toBeInTheDocument(); + }); + + it('an empty `sections` array is not "sections authored" — the flat body still wins', () => { + // The boundary between the two arms. `DetailView` gates the section arm on + // `sections.length > 0` and the flat arm on `!sections?.length`, so the + // empty array must behave as absence rather than blanking the body — the + // blank-page failure mode this block exists to keep closed. + renderDetails({ sections: [], fields: ['industry'] }); + + expect(screen.getByText('Manufacturing')).toBeInTheDocument(); + }); +}); diff --git a/packages/plugin-detail/src/__tests__/recordDetailsInputs.spec-parity.test.ts b/packages/plugin-detail/src/__tests__/recordDetailsInputs.spec-parity.test.ts index a02ef2e7a..efd2a4ff4 100644 --- a/packages/plugin-detail/src/__tests__/recordDetailsInputs.spec-parity.test.ts +++ b/packages/plugin-detail/src/__tests__/recordDetailsInputs.spec-parity.test.ts @@ -43,10 +43,15 @@ */ import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; import { ComponentRegistry } from '@object-ui/core'; import { RecordDetailsProps } from '@objectstack/spec/ui'; import '../index'; +const SRC_DIR = join(dirname(fileURLToPath(import.meta.url)), '..'); + type ShapeCarrier = { shape?: unknown; _def?: { shape?: unknown } }; /** Resolve a Zod object's `.shape` through both spellings, lazy or plain. */ @@ -78,9 +83,34 @@ function arrayElement(schema: unknown): unknown { return arr?.element ?? arr?.def?.element ?? arr?._def?.element ?? arr?._def?.type; } -/** Top-level keys of the spec's `RecordDetailsProps`. */ +/** Top-level keys of the spec's `RecordDetailsProps`, INCLUDING tombstones. */ const specTopLevelKeys = (): string[] => shapeKeys(RecordDetailsProps); +/** + * Is this top-level key an ADR-0087 tombstone — declared, but typed `never` so + * every value is rejected with a named migration message? + * + * This distinction is load-bearing, and objectui#3818 is what proved it. A D2 + * retirement does NOT delete the key from the shape; it REPLACES the member + * with `z.never()` carrying the "removed in 17.0.0, run `os migrate meta`" + * text. So a retired key still answers `Object.keys(shape)` — which is why + * `layout` survived the 17.0.0-rc.6 pin bump on the published surface below + * with every derived gate green: the gate asked "is this key in the shape", + * the tombstone said yes, and the manifest kept offering an input the spec + * rejects on parse. Filtering tombstones out is what makes the gate mean what + * its name says. + */ +const isTombstoned = (key: string): boolean => { + const member = shapeMember(RecordDetailsProps, key) as + | { _def?: { type?: string }; def?: { type?: string } } + | undefined; + return (member?._def?.type ?? member?.def?.type) === 'never'; +}; + +/** Top-level keys the spec actually ACCEPTS — tombstones removed. */ +const specAcceptedTopLevelKeys = (): string[] => + specTopLevelKeys().filter((key) => !isTombstoned(key)); + /** Member keys of one `sections[]` entry, per the spec. */ const specSectionKeys = (): string[] => shapeKeys(arrayElement(shapeMember(RecordDetailsProps, 'sections'))); @@ -191,7 +221,11 @@ describe('record:details — registry inputs vs @objectstack/spec', () => { }); it('declares no top-level input the spec does not accept', () => { - const allowed = new Set(specTopLevelKeys()); + // ACCEPTED keys, not merely DECLARED ones — a tombstone is present in the + // shape and rejects every value (see `isTombstoned`). Reading raw shape + // keys here is what let the retired `layout` input stay published and green + // through the 17.0.0-rc.6 bump (objectui#3818). + const allowed = new Set(specAcceptedTopLevelKeys()); const offSpec = inputs().map((i) => i.name).filter((name) => !allowed.has(name)); expect(offSpec).toEqual([]); }); @@ -262,3 +296,80 @@ describe('record:details — registry inputs vs @objectstack/spec', () => { expect(description).not.toContain('{'); }); }); + +/** + * objectui#3818 — the retired `layout` key, pinned on the published surface. + * + * `record:details` published `layout: enum ['auto','custom']` with + * `defaultValue: 'auto'` and the description "auto uses the object + * highlightFields; custom uses explicit sections". None of that was ever + * implemented: the renderer's ONLY `schema.layout` read tested `'inline'` | + * `'compact'` — two values the schema never permitted — so both legal values + * fell through the same ternary to `'vertical'` and the key selected nothing. + * A two-value enum whose values do the same thing is a false affordance with + * zero diagnostics: an author writing `layout: 'custom'` believed it took + * effect. @objectstack/spec 17.0.0 removed the key (objectstack#6946, + * ADR-0087 D2) and the maintainer ruling of 2026-08-09 on objectui#3818 is + * REMOVAL on this side too. + * + * Why these pins are not redundant with the derived gate above: a D2 tombstone + * stays IN the shape (see `isTombstoned`), so every key-presence check kept + * passing while the input was still published. The gate is now + * tombstone-aware, and the assertions below name this key so the repair stays + * legible if that helper is ever loosened. + */ +describe('record:details — `layout` is retired, not merely undocumented (#3818)', () => { + it('the spec REJECTS `layout`, with the ADR-0087 migration message', () => { + // The premise, checked first: if upstream ever un-retires the key, this + // fails before the negative pins below start guarding a dead rule. + expect(specTopLevelKeys()).toContain('layout'); // tombstone is still declared + expect(isTombstoned('layout')).toBe(true); + expect(specAcceptedTopLevelKeys()).not.toContain('layout'); + + // A rejection case, so the assertion set is the envelope and not a bare + // "it failed": path + code + the named migration text. Both formerly + // published values AND both values the dead renderer branch tested — all + // four are unauthorable now, which is the whole point. + for (const value of ['auto', 'custom', 'inline', 'compact']) { + const parsed = RecordDetailsProps.safeParse({ layout: value }); + expect(parsed.success).toBe(false); + expect(parsed.error?.issues.map((i) => i.path.join('.'))).toContain('layout'); + expect(parsed.error?.issues.map((i) => i.code)).toContain('invalid_type'); + expect(parsed.error?.issues[0]?.message).toMatch(/removed in @objectstack\/spec 17\.0\.0/); + } + }); + + it('the published surface offers no `layout` input', () => { + // Non-empty FIRST — `inputs()` returning `[]` would satisfy every + // absence assertion below for the wrong reason (the vacuous-green trap + // the `hideFields` block above documents from objectui#3808's reverse run). + expect(inputs().length).toBeGreaterThan(0); + expect(inputs().map((i) => i.name)).toContain('sections'); + + expect(inputs().map((i) => i.name)).not.toContain('layout'); + expect(input('layout')).toBeUndefined(); + }); + + it('no input description still teaches `layout` as an authorable key', () => { + // The declaration and its prose died together: `sections` used to end + // "Required when layout is 'custom'", which would have kept teaching the + // key from a neighbouring description after the input itself was gone. + const descriptions = inputs().map((i) => i.description ?? ''); + expect(descriptions.join('')).not.toBe(''); + for (const description of descriptions) { + expect(description).not.toMatch(/\blayout is\b/i); + expect(description).not.toMatch(/layout:\s*['"]?(auto|custom)/i); + } + }); + + it('the renderer contains no `schema.layout` read', () => { + // Source-text half. The behavioural twin lives in + // `recordDetailsBodySource.test.tsx` (authoring the key changes no output); + // this one catches a re-added branch that a render assertion could miss if + // the branch were re-introduced behind a condition the fixtures never hit. + const src = readFileSync(join(SRC_DIR, 'renderers/record-details.tsx'), 'utf8'); + expect(src).toContain('RecordDetailsRenderer'); // the file really is the renderer + expect(src).not.toMatch(/schema\s*\.\s*layout/); + expect(src).not.toMatch(/['"]compact['"]/); + }); +}); diff --git a/packages/plugin-detail/src/index.tsx b/packages/plugin-detail/src/index.tsx index ad9b7d783..76a8bdac6 100644 --- a/packages/plugin-detail/src/index.tsx +++ b/packages/plugin-detail/src/index.tsx @@ -259,8 +259,18 @@ ComponentRegistry.register('details', RecordDetailsRenderer, { // `array`), `validateComponentProps` upstream is advisory, and // `RecordDetailsRenderer` maps every entry as an object (`s.name` / // `s.label` / `s.fields`), so a string entry contributes no fields at all. - // With `layout: 'custom'` sections are the ONLY source of the body, so the - // author who trusted the old text got a blank detail page. objectui#3807. + // Once `sections` is authored at all it is the ONLY source of the body, so + // the author who trusted the old text got a blank detail page. objectui#3807. + // + // There is deliberately NO `layout` input. `record:details` published one + // (`enum ['auto','custom']`, `defaultValue: 'auto'`) until objectui#3818; + // @objectstack/spec 17.0.0 removed the key outright (objectstack#6946, + // ADR-0087 D2) because those semantics were never implemented — the + // renderer's only read tested `inline` | `compact`, values the schema never + // permitted, so `auto` and `custom` took the same branch and the key + // selected nothing. Re-adding it here would republish a key the spec now + // rejects on parse; the body-source contract is `sections`-presence, stated + // in the `sections` description below. // // Documented member keys are exactly the spec's four (`name`, `label`, // `columns`, `fields`) — deliberately NOT the extras `RecordDetailsRenderer` @@ -271,8 +281,7 @@ ComponentRegistry.register('details', RecordDetailsRenderer, { // below. The renderer tolerating them is not a licence to teach them. inputs: [ { name: 'columns', type: 'enum', label: 'Columns', enum: ['1', '2', '3', '4'], defaultValue: '2', description: 'Number of columns for field layout (1-4)' }, - { name: 'layout', type: 'enum', label: 'Layout', enum: ['auto', 'custom'], defaultValue: 'auto', description: 'auto uses the object highlightFields; custom uses explicit sections' }, - { name: 'sections', type: 'array', label: 'Sections', description: 'Field groups rendered as the detail body, in order. Every entry is an OBJECT — `{ name?, label?, columns?, fields }` — a bare section-id string is NOT accepted (the spec retired that spelling in objectstack#5611, and the renderer reads name/label/fields off each entry, so a string entry renders no fields at all). `fields` (required) are the field names shown in this section, in order. `label` is the section heading; omit it for an untitled, borderless section. `name` is a stable snake_case identifier and the i18n anchor — the heading resolves through objects.._sections..label, so a section without a name shows its authored label in every locale. `columns` (1-4) is THIS section\'s field-grid width; omit it and the renderer derives the width. Required when layout is "custom", where sections are the only source of the detail body.' }, + { name: 'sections', type: 'array', label: 'Sections', description: 'Field groups rendered as the detail body, in order. Every entry is an OBJECT — `{ name?, label?, columns?, fields }` — a bare section-id string is NOT accepted (the spec retired that spelling in objectstack#5611, and the renderer reads name/label/fields off each entry, so a string entry renders no fields at all). `fields` (required) are the field names shown in this section, in order. `label` is the section heading; omit it for an untitled, borderless section. `name` is a stable snake_case identifier and the i18n anchor — the heading resolves through objects.._sections..label, so a section without a name shows its authored label in every locale. `columns` (1-4) is THIS section\'s field-grid width; omit it and the renderer derives the width. Authoring `sections` at all makes it the only source of the detail body; omit it and the body falls back to the object\'s highlightFields.' }, { name: 'fields', type: 'array', label: 'Fields', description: 'Explicit field list (overrides highlightFields)' }, // `hideFields` is DECLARED, not merely honoured (objectui#3808). The spec // declares it (objectstack#5611) and `RecordDetailsRenderer` has read it diff --git a/packages/plugin-detail/src/renderers/record-details.tsx b/packages/plugin-detail/src/renderers/record-details.tsx index 05a818020..83f46137c 100644 --- a/packages/plugin-detail/src/renderers/record-details.tsx +++ b/packages/plugin-detail/src/renderers/record-details.tsx @@ -100,9 +100,6 @@ export const RecordDetailsRenderer: React.FC = ({ } } - const layout: 'vertical' | 'horizontal' = - schema.layout === 'inline' || schema.layout === 'compact' ? 'horizontal' : 'vertical'; - const enforceFLS = (schema as any).enforceFieldSecurity === true; const redact: string[] = Array.isArray((schema as any).redactFields) ? (schema as any).redactFields @@ -241,7 +238,15 @@ export const RecordDetailsRenderer: React.FC = ({ objectName: ctx.objectName, resourceId: ctx.recordId as any, data: ctx.data, - layout, + // Constant by contract, not by omission. `record:details` HAD a `layout` + // key; @objectstack/spec 17.0.0 removed it (objectstack#6946, ADR-0087 D2) + // because its published `auto` | `custom` values were never implemented — + // the only read here tested `inline` | `compact`, two values the schema + // never permitted, so both legal values took this same branch. What + // actually chooses the body is what you author: `sections` renders the + // explicit groups, omitting it falls back to the object's highlightFields + // (see `filteredSections` / `filteredFields` above). objectui#3818. + layout: 'vertical', columns: schema.columns, sections: filteredSections, fields: filteredFields,