diff --git a/.changeset/conversion-walk-nested-page-components.md b/.changeset/conversion-walk-nested-page-components.md new file mode 100644 index 0000000000..17da7e99b6 --- /dev/null +++ b/.changeset/conversion-walk-nested-page-components.md @@ -0,0 +1,59 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): a page-component conversion reaches components nested inside a container, not only region- and slot-level ones (#6775) + +`mapPageComponents` — the walker every page-component conversion is built on — +visited `pages[].regions[].components[]` and `pages[].slots.` and stopped +there. A component nested inside another component's `properties` (a card's +`children` / `body` / `footer`, a `page:tabs` or `page:accordion` panel's +`items[].children`) was never visited, so **no** page-component conversion +rewrote it. `walkPageComponents` in `@objectstack/lint` has descended into +those containers from the start, which means every conversion reached strictly +less than the lint rule that judges its result. + +The walker now descends into the same containers lint does, to any depth, with +the same path spelling — so a conversion notice and a lint finding name one +site with one string. Copy-on-write is unchanged: an untouched sub-tree keeps +its reference, and a stack where nothing converts is still returned by +identity. + +**Why this mattered on the load path.** The usual answer for a site a +conversion cannot reach is the tombstone: the key is typed `never`, so `tsc` +refuses it at the authoring site and the parse refuses it at load, wherever it +sits. That answer does not hold for a key that stays live elsewhere on the +surface. `page-header-subtitle-alias` retires `description` on page-header +components, and `description` remains a declared prop on other components (an +`element:text_input`'s helper text), so it cannot be tombstoned — +`properties.description` parses green at *any* position. A header authored in +a card or inside a `kind: 'slotted'` record page therefore got no rewrite and +no diagnostic from any of the three layers: the conversion did not fire, the +page schema was satisfied (`properties` is an open bag nothing validates by +`type` on the load path), and the props check is advisory, CLI-only, and runs +on already-converted metadata. Retiring the consumer-side +`subtitle ?? description` fallback would have dropped those pages' second line +silently. + +Every page-component conversion rides the widened walk and its fixture now +pins the nested and slotted positions alongside the region-level one: +`page-header-subtitle-alias`, `record-picker-display-field-to-label-field`, +`record-picker-inert-keys-removed`, `page-card-body-to-children`, +`inline-action-api-params-to-body-extra`, `page-tabs-type-to-tab-style`, and +`page-component-visibility-to-visibleWhen`. + +`page-card-body-to-children` is the one interaction worth naming: it MOVES a +container key (`properties.body` → `properties.children`). The descent reads +the mapped component, so a nested sub-tree is walked exactly once — under the +canonical key, not once per spelling. + +Two differences from the lint walk remain, both deliberate and both pinned by +a cross-walker parity test: source-authored pages (`kind: 'html' | 'react' | +'jsx'`) are skipped by lint and still converted here (their regions are a +derived cache that must be normalized, or a stored page rehydrates in a shape +the runtime no longer serves), and the conversion walk keeps a depth ceiling of +32 containers, which lint has no counterpart for because it never runs on +hand-built `defineStack` objects. + +No conversion was added or removed, and no already-converted metadata changes +shape: this widens which authoring positions the existing rewrites reach. diff --git a/packages/lint/src/page-walk-conversion-parity.test.ts b/packages/lint/src/page-walk-conversion-parity.test.ts new file mode 100644 index 0000000000..2054118d35 --- /dev/null +++ b/packages/lint/src/page-walk-conversion-parity.test.ts @@ -0,0 +1,145 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The two page-component walkers must reach the same components (#6775). + * + * There are two of them and there has to be: `walkPageComponents` (here) yields + * nodes for the lint rules to judge, and `mapPageComponents` + * (`@objectstack/spec`'s conversion layer) rewrites them copy-on-write. What + * must NOT differ is which components each one arrives at — a conversion that + * reaches less than the rule judging its result normalizes part of a corpus and + * leaves the rest looking converted, which is exactly what #6775 measured: + * `page-header-subtitle-alias` rewrote a header in a region and skipped the + * identical header in a slot or inside a card, with no diagnostic from any + * layer (the props bag is unvalidated on the load path, and that key has no + * tombstone to fall back on). + * + * This file is the only place that can see both, since `@objectstack/lint` + * depends on `@objectstack/spec` and not the other way round. The parity is + * asserted BEHAVIOURALLY — every position this walk yields is a position a + * conversion notice names — rather than by comparing implementations, so it + * keeps holding if either walk is rewritten. + * + * One difference is deliberate and pinned below: source-authored pages + * (`kind: 'html' | 'react' | 'jsx'`) are skipped here and visited there. Lint + * skips them so it does not report findings about a DERIVED region cache the + * author never wrote; a conversion still has to normalize that cache, or a + * stored page rehydrates in a shape the runtime no longer serves. + */ + +import { applyConversions } from '@objectstack/spec'; +import { describe, expect, it } from 'vitest'; + +import { walkPageComponents } from './page-walk.js'; + +/** + * A page-header authored with the retired `description` spelling — the probe. + * `page-header-subtitle-alias` rewrites it to `subtitle` and emits a notice + * whose path names the site, so "did the conversion reach here?" is answerable + * for any position without exporting the walker itself. + */ +const probe = (title: string) => ({ type: 'page:header', properties: { title, description: 'Second line' } }); + +/** + * Every authoring position in one page: both region slots, a single-component + * slot and an array slot, and each container a component nests a sub-tree in + * (`children`, `items[].children`, `body`, `footer`), including two levels of + * nesting. + */ +const page = { + name: 'parity', + kind: 'slotted', + object: 'account', + regions: [ + { + name: 'main', + components: [ + probe('region'), + { type: 'page:section', properties: { children: [probe('children')] } }, + { + type: 'page:tabs', + properties: { tabStyle: 'line', items: [{ label: 'T', children: [probe('tab panel')] }] }, + }, + { + type: 'page:card', + properties: { + body: [probe('card body')], + footer: [{ type: 'page:section', properties: { children: [probe('two deep')] } }], + }, + }, + ], + }, + ], + slots: { + header: probe('single slot'), + details: [probe('array slot 0'), probe('array slot 1')], + }, +}; + +/** The positions the lint walk yields that carry the probe. */ +const walkedProbePaths = () => + walkPageComponents(page as unknown as Record, 'pages[0]') + .filter((w) => w.component.type === 'page:header') + .map((w) => w.path); + +/** + * The positions the conversion layer actually rewrote the probe at. + * + * Filtered to this one entry: the fixture page also carries a `page:card` with + * a `body`, which `page-card-body-to-children` rewrites — a real notice about a + * different key, and not a position the probe sits at. + */ +const convertedProbePaths = () => { + const paths: string[] = []; + applyConversions( + { pages: [structuredClone(page)] }, + { + includeRetired: true, + onNotice: (n) => { if (n.conversionId === 'page-header-subtitle-alias') paths.push(n.path); }, + }, + ); + // The notice names the rewritten KEY; the component is its parent. + return paths.map((p) => p.replace(/\.properties\.subtitle$/, '')); +}; + +describe('#6775 — walkPageComponents and the conversion walk reach the same components', () => { + it('the probe sits at every position the lint walk knows about', () => { + // Guards the fixture itself: if a container shape is added to the lint walk + // and not to this page, the parity assertion below would pass vacuously. + expect(walkedProbePaths()).toEqual([ + 'pages[0].regions[0].components[0]', + 'pages[0].regions[0].components[1].properties.children[0]', + 'pages[0].regions[0].components[2].properties.items[0].children[0]', + 'pages[0].regions[0].components[3].properties.body[0]', + 'pages[0].regions[0].components[3].properties.footer[0].properties.children[0]', + 'pages[0].slots.header', + 'pages[0].slots.details[0]', + 'pages[0].slots.details[1]', + ]); + }); + + it('a conversion rewrites the probe at every one of them, spelling the same paths', () => { + // Order-insensitive: the two walks are free to visit in different orders, + // but neither may reach a component the other cannot. + expect(new Set(convertedProbePaths())).toEqual(new Set(walkedProbePaths())); + }); + + it('source-authored pages are the one deliberate difference', () => { + // Lint yields nothing for them (the regions are a derived cache, not + // authored metadata); the conversion still normalizes that cache. + const jsxPage = { name: 'j', kind: 'jsx', source: '
', regions: [{ name: 'main', components: [probe('cached')] }] }; + expect(walkPageComponents(jsxPage as unknown as Record, 'pages[0]')).toEqual([]); + + const notices: string[] = []; + applyConversions( + { pages: [structuredClone(jsxPage)] }, + { + includeRetired: true, + // `kind: 'jsx'` itself converts (`page-kind-jsx-to-html`, protocol 11); + // what this pins is the component inside the derived cache. + onNotice: (n) => { if (n.conversionId === 'page-header-subtitle-alias') notices.push(n.path); }, + }, + ); + expect(notices).toEqual(['pages[0].regions[0].components[0].properties.subtitle']); + }); +}); diff --git a/packages/spec/src/conversions/page-component-walk.test.ts b/packages/spec/src/conversions/page-component-walk.test.ts new file mode 100644 index 0000000000..c2740cf77a --- /dev/null +++ b/packages/spec/src/conversions/page-component-walk.test.ts @@ -0,0 +1,217 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The conversion pass reaches every page component, wherever it is authored + * (#6775, after #6776). + * + * `mapPageComponents` used to walk `pages[].regions[].components[]` and stop. + * #6776 added the named `slots`; this file covers the rest of the asymmetry — + * the sub-trees a component nests inside its own free-form `properties` + * (`children`, `items[].children`, `body`, `footer`), which `walkPageComponents` + * in `packages/lint` has always descended into and the conversion layer did not. + * + * The assertion made over and over here is the *parity* one, the same one + * `region-walk.test.ts` makes for flow nodes: whatever the table does to a + * component at region level, it must do to the identical component in a slot or + * one (or three) containers in. Position must not decide meaning — the schema + * cannot see the difference (`properties` is an open bag that nothing validates + * by `type` on the load path), so a walk that stops early normalizes half a + * corpus and leaves the other half looking converted. + */ + +import { describe, expect, it } from 'vitest'; + +import { applyConversions, collectConversionNotices } from './apply.js'; + +/** A page-header authored with the retired `description` spelling. */ +const staleHeader = (title: string) => ({ + type: 'page:header', + properties: { title, description: 'Second line' }, +}); +/** The converted form of {@link staleHeader}. */ +const canonicalHeader = (title: string) => ({ + type: 'page:header', + properties: { title, subtitle: 'Second line' }, +}); + +/** A page whose single region holds `components`. */ +const regionPage = (...components: unknown[]) => ({ + pages: [{ name: 'p', regions: [{ name: 'main', components }] }], +}); + +const componentAt = (stack: Record, ...steps: (string | number)[]) => + steps.reduce((node, step) => node[step], (stack.pages as any[])[0]); + +describe('#6775 — a conversion reaches a component nested in another component', () => { + /** + * The four container shapes, spelled exactly as `packages/lint`'s + * `walkPageComponents` spells them. A fifth added there has to be added here + * too, or the two walkers drift apart again — which is the whole defect. + */ + const containers: { label: string; properties: (child: unknown) => Record; path: string }[] = [ + { + label: 'properties.children[] — the generic layout nesting', + properties: (child) => ({ children: [child] }), + path: 'properties.children[0]', + }, + { + label: 'properties.items[].children[] — page:tabs / page:accordion panels', + properties: (child) => ({ tabStyle: 'line', items: [{ label: 'Tab', children: [child] }] }), + path: 'properties.items[0].children[0]', + }, + { + label: 'properties.body[] — a page:card body', + properties: (child) => ({ body: [child] }), + path: 'properties.body[0]', + }, + { + label: 'properties.footer[] — a page:card footer', + properties: (child) => ({ footer: [child] }), + path: 'properties.footer[0]', + }, + ]; + + for (const { label, properties, path } of containers) { + it(`converts a header under ${label}, exactly as at region level`, () => { + // `page:section` rather than `page:card`, so the container itself is not a + // conversion target and the only notice is the nested header's. + const container = { type: 'page:section', properties: properties(staleHeader('Nested')) }; + const { stack, notices } = collectConversionNotices(regionPage(staleHeader('Top'), container), { + includeRetired: true, + }); + + expect(componentAt(stack, 'regions', 0, 'components', 0)).toEqual(canonicalHeader('Top')); + const nested = path + .replace(/\]/g, '') + .split(/[.[]/) + .reduce((node, step) => node[/^\d+$/.test(step) ? Number(step) : step], + componentAt(stack, 'regions', 0, 'components', 1)); + expect(nested).toEqual(canonicalHeader('Nested')); + + expect(notices.map((n) => n.path)).toEqual([ + 'pages[0].regions[0].components[0].properties.subtitle', + `pages[0].regions[0].components[1].${path}.properties.subtitle`, + ]); + }); + } + + it('recurses — a header three containers down converts too', () => { + const deep = { + type: 'page:section', + properties: { + children: [ + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [{ label: 'T', children: [{ type: 'page:section', properties: { children: [staleHeader('Deep')] } }] }], + }, + }, + ], + }, + }; + const { notices } = collectConversionNotices(regionPage(deep), { includeRetired: true }); + expect(notices.map((n) => n.path)).toEqual([ + 'pages[0].regions[0].components[0].properties.children[0]' + + '.properties.items[0].children[0].properties.children[0].properties.subtitle', + ]); + }); + + it('reaches a component nested inside a named SLOT, not only inside a region', () => { + // The two widenings compose: the slotted record page is the shape objectui's + // guide prescribes, and a card inside it is ordinary composition. + const { stack, notices } = collectConversionNotices( + { + pages: [{ + name: 'account_detail', + kind: 'slotted', + regions: [], + slots: { details: { type: 'page:section', properties: { children: [staleHeader('In a slot')] } } }, + }], + }, + { includeRetired: true }, + ); + expect(componentAt(stack, 'slots', 'details', 'properties', 'children', 0)) + .toEqual(canonicalHeader('In a slot')); + expect(notices.map((n) => n.path)).toEqual([ + 'pages[0].slots.details.properties.children[0].properties.subtitle', + ]); + }); + + it('visits a moved sub-tree ONCE — the descent reads the mapped component', () => { + // `page-card-body-to-children` renames `properties.body` → `children` on the + // outer card. Because the descent happens after the mapper, the inner card + // is walked under the canonical key and emits one notice, not one per + // spelling — and its own path is spelled with `children`, not `body`. + const { stack, notices } = collectConversionNotices( + regionPage({ + type: 'page:card', + properties: { title: 'Outer', body: [{ type: 'page:card', properties: { title: 'Inner', body: [] } }] }, + }), + { includeRetired: true }, + ); + expect(notices.map((n) => n.path)).toEqual([ + 'pages[0].regions[0].components[0].properties.children', + 'pages[0].regions[0].components[0].properties.children[0].properties.children', + ]); + expect(componentAt(stack, 'regions', 0, 'components', 0)).toEqual({ + type: 'page:card', + properties: { title: 'Outer', children: [{ type: 'page:card', properties: { title: 'Inner', children: [] } }] }, + }); + }); +}); + +describe('#6775 — the page-component walk stays copy-on-write and shape-gated', () => { + it('returns the identical reference when nothing nested converts', () => { + const clean = regionPage({ + type: 'page:section', + properties: { children: [canonicalHeader('Already canonical')] }, + }); + expect(applyConversions(clean, { includeRetired: true })).toBe(clean); + }); + + it('shares untouched branches — only the changed path is copied', () => { + const untouched = { type: 'page:section', properties: { children: [canonicalHeader('Fine')] } }; + const stack = regionPage( + { type: 'page:section', properties: { children: [staleHeader('Stale')] } }, + untouched, + ); + const out = applyConversions(stack, { includeRetired: true }) as any; + expect(out).not.toBe(stack); + expect(out.pages[0].regions[0].components[1]).toBe(untouched); + }); + + it('leaves a container key that is not a component list alone', () => { + // `body` is an ordinary key elsewhere on this surface — a `record:alert` + // carries prose there, and a `page:tabs` item is a tab record, not a + // component. The walk is gated on the SHAPE (an array, of dicts), so + // neither is descended and neither is mistaken for a slot. + const stack = regionPage( + { type: 'record:alert', properties: { body: 'Confirm the work.' } }, + { type: 'page:tabs', properties: { tabStyle: 'line', items: [{ label: 'Holders' }] } }, + { type: 'page:section', properties: { children: ['not-a-component', 42, null] } }, + ); + expect(applyConversions(stack, { includeRetired: true })).toBe(stack); + }); + + it('terminates on a self-referential component instead of recursing forever', () => { + // A stack handed to `defineStack` is hand-built objects, not parsed JSON, + // so a cycle is reachable on the load path — the ceiling `mapFlowNodes` has + // for regions, here for containers. + const cyclic: Record = { type: 'page:section', properties: {} }; + (cyclic.properties as Record).children = [cyclic]; + expect(() => applyConversions(regionPage(cyclic), { includeRetired: true })).not.toThrow(); + }); + + it('converts at the depth ceiling but not past it', () => { + const nest = (depth: number) => { + let node: Record = staleHeader('Bottom'); + for (let i = 0; i < depth; i++) node = { type: 'page:section', properties: { children: [node] } }; + return node; + }; + // 32 containers above the header — the last hop the ceiling allows. + expect(collectConversionNotices(regionPage(nest(32)), { includeRetired: true }).notices).toHaveLength(1); + // One deeper: not reached, and no throw — the ceiling is a stop, not a crash. + expect(collectConversionNotices(regionPage(nest(33)), { includeRetired: true }).notices).toHaveLength(0); + }); +}); diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index d2fa25f45f..136f7e5030 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -540,8 +540,10 @@ const viewVisibleOnToVisibleWhen: MetadataConversion = { /** * Page component `visibility` → `visibleWhen` (protocol 15, ADR-0089 D2). * - * The page-component spelling of the same predicate. Applies to - * `pages[].regions[].components[]`. **Live window**, same terms as + * The page-component spelling of the same predicate. Applies to every page + * component {@link mapPageComponents} reaches — `regions[].components[]`, + * `slots.`, and the containers a component nests under its `properties` + * (#6776, #6775). **Live window**, same terms as * {@link viewVisibleOnToVisibleWhen}. (An AI agent's `visibility` property is * a different, unrelated surface and is not touched.) */ @@ -570,10 +572,29 @@ const pageComponentVisibilityToVisibleWhen: MetadataConversion = { components: [ { type: 'record:list', visibility: "page.selectedId != ''" }, { type: 'element:divider' }, + // Nested one container down (#6775): the predicate means the + // same thing inside a card as it does at region level, so the + // walk reaches it there too. + { + type: 'page:card', + properties: { + title: 'Selected', + children: [{ type: 'record:detail', visibility: "page.selectedId != ''" }], + }, + }, ], }, ], }, + // …and in a named slot on a slotted record page (#6776). + { + name: 'crm_lead_record', + kind: 'slotted', + regions: [], + slots: { + highlights: { type: 'record:detail', visibility: "record.stage == 'won'" }, + }, + }, ], }, after: { @@ -586,13 +607,28 @@ const pageComponentVisibilityToVisibleWhen: MetadataConversion = { components: [ { type: 'record:list', visibleWhen: "page.selectedId != ''" }, { type: 'element:divider' }, + { + type: 'page:card', + properties: { + title: 'Selected', + children: [{ type: 'record:detail', visibleWhen: "page.selectedId != ''" }], + }, + }, ], }, ], }, + { + name: 'crm_lead_record', + kind: 'slotted', + regions: [], + slots: { + highlights: { type: 'record:detail', visibleWhen: "record.stage == 'won'" }, + }, + }, ], }, - expectedNotices: 1, + expectedNotices: 3, }, }; @@ -4694,6 +4730,17 @@ const PAGE_HEADER_COMPONENT_TYPES = new Set(['page:header', 'page-header']); * is a live declared prop elsewhere on the same surface (`element:text_input` * helper text), and those components are not this entry's business. * + * **This entry is why the walker's reach had to grow (#6775).** Every other + * page-component entry leans on a tombstone to cover the sites a conversion + * cannot reach; this one has none to lean on, because `description` stays a + * live declared prop on other components, so `properties.description` parses + * green at ANY position. Until the walker descended into `slots.*` (#6776) and + * into container `properties` (#6775), a header on a `kind: 'slotted'` record + * page or inside a card/tab panel was rewritten by nobody and reported by + * nobody — and objectui's `subtitle ?? description` fallback could not retire + * without those pages silently losing their second line, the exact failure + * shape this entry exists to prevent. The fixture pins all three positions. + * * **Live window**; retires at 18. */ const pageHeaderSubtitleAlias: MetadataConversion = { @@ -4737,6 +4784,66 @@ const pageHeaderSubtitleAlias: MetadataConversion = { }, ], }, + // The slotted record page — `regions: []`, header in a named slot. This + // is the shape objectui's own guide prescribes for a customized record + // header, and a region-only walk visited none of it (#6776). + { + name: 'crm_account_detail', + kind: 'slotted', + regions: [], + slots: { + header: { type: 'page:header', properties: { title: '{name}', description: 'Account overview' } }, + }, + }, + // Container nesting (#6775): a header inside a card's `children`, one in + // its `footer`, one inside a tab panel, and one two levels down. All are + // spec-valid (`properties` is an open bag) and all were invisible to the + // region-only walk — with no tombstone to catch them at parse time. + { + name: 'crm_pipeline_dashboard', + regions: [ + { + name: 'main', + components: [ + { + type: 'page:card', + properties: { + title: 'Pipeline', + children: [ + { type: 'page-header', properties: { title: 'Open', description: 'This quarter' } }, + ], + footer: [ + { type: 'page:header', properties: { title: 'Closed', description: 'Last quarter' } }, + ], + }, + }, + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [ + { + label: 'Activity', + children: [ + { type: 'page:header', properties: { title: 'Recent', description: 'Last 7 days' } }, + // Two levels down — the recursion, not just one hop. + { + type: 'page:card', + properties: { + children: [ + { type: 'page:header', properties: { title: 'Nested', description: 'Deep' } }, + ], + }, + }, + ], + }, + ], + }, + }, + ], + }, + ], + }, ], }, after: { @@ -4755,9 +4862,61 @@ const pageHeaderSubtitleAlias: MetadataConversion = { }, ], }, + { + name: 'crm_account_detail', + kind: 'slotted', + regions: [], + slots: { + header: { type: 'page:header', properties: { title: '{name}', subtitle: 'Account overview' } }, + }, + }, + { + name: 'crm_pipeline_dashboard', + regions: [ + { + name: 'main', + components: [ + { + type: 'page:card', + properties: { + title: 'Pipeline', + children: [ + { type: 'page-header', properties: { title: 'Open', subtitle: 'This quarter' } }, + ], + footer: [ + { type: 'page:header', properties: { title: 'Closed', subtitle: 'Last quarter' } }, + ], + }, + }, + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [ + { + label: 'Activity', + children: [ + { type: 'page:header', properties: { title: 'Recent', subtitle: 'Last 7 days' } }, + { + type: 'page:card', + properties: { + children: [ + { type: 'page:header', properties: { title: 'Nested', subtitle: 'Deep' } }, + ], + }, + }, + ], + }, + ], + }, + }, + ], + }, + ], + }, ], }, - expectedNotices: 2, + expectedNotices: 7, }, }; @@ -4774,14 +4933,15 @@ const pageHeaderSubtitleAlias: MetadataConversion = { * contract" — so the honoured keys were declared and the unread ones retire * here. * - * **Region level is the reach, deliberately.** {@link mapPageComponents} walks - * `pages[].regions[].components[]` and stops: `PageComponentSchema` declares no - * children key, so a picker nested inside a card's `children` sits in another - * component's free-form `properties` and is not typed page-component shape. - * Same boundary as {@link pageHeaderSubtitleAlias}, drawn for the same reason. - * The tombstones are what cover the rest: they type the key `never`, so a - * nested authoring site fails `tsc` and carries its own prescription at parse - * time whether or not a conversion could reach it. + * **The reach is every position a picker can be authored in** (#6775). + * {@link mapPageComponents} walks `regions[].components[]`, `slots.` and + * the containers a component nests under its `properties` — the same set + * `walkPageComponents` lints — so a picker inside a card's `children` or a tab + * panel is rewritten where it sits. The tombstones still carry the refusal at + * parse time for anything a conversion declines to touch (a disagreeing pair + * under {@link renameKey}'s house rule, or a source no migration ran over); + * what changed is that "run `os migrate meta`" is now a promise the rewrite can + * keep at a nested site, not only at region level. * * All three are **retired from the load path**: each key is tombstoned in * `ui/component.zod.ts`, so the loader rejects it loudly with the prescription @@ -4842,10 +5002,32 @@ const recordPickerDisplayFieldToLabelField: MetadataConversion = { // `displayField` is a live LOOKUP-FIELD key elsewhere on the // surface — a different component's business, untouched here. { type: 'element:form', properties: { object: 'c', displayField: 'title' } }, + // Nested one container down (#6775) — a picker inside a card is + // where a form-shaped page actually puts one. + { + type: 'page:card', + properties: { + title: 'Link a project', + children: [ + { type: 'element:record_picker', properties: { object: 'd', displayField: 'code' } }, + ], + }, + }, ], }, ], }, + // A slotted page's named slot — same component, other authoring shape. + { + name: 'showcase_project_detail', + kind: 'slotted', + regions: [], + slots: { + details: [ + { type: 'element:record_picker', properties: { object: 'e', displayField: 'label' } }, + ], + }, + }, ], }, after: { @@ -4860,13 +5042,32 @@ const recordPickerDisplayFieldToLabelField: MetadataConversion = { { type: 'element:record_picker', properties: { object: 'a', labelField: 'name' } }, { type: 'element:record_picker', properties: { object: 'b', labelField: 'name', displayField: 'title' } }, { type: 'element:form', properties: { object: 'c', displayField: 'title' } }, + { + type: 'page:card', + properties: { + title: 'Link a project', + children: [ + { type: 'element:record_picker', properties: { object: 'd', labelField: 'code' } }, + ], + }, + }, ], }, ], }, + { + name: 'showcase_project_detail', + kind: 'slotted', + regions: [], + slots: { + details: [ + { type: 'element:record_picker', properties: { object: 'e', labelField: 'label' } }, + ], + }, + }, ], }, - expectedNotices: 2, + expectedNotices: 4, }, }; @@ -4916,10 +5117,35 @@ const recordPickerInertKeysRemoved: MetadataConversion = { // `multiple` is a live FIELD key (lookup fields) — a different // surface entirely, and not this entry's business. { type: 'element:form', properties: { object: 'a', multiple: true } }, + // Inside a tab panel (#6775): `page:tabs` hangs its sub-tree off + // `properties.items[].children`, which the walk now descends. + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [ + { + label: 'Pick one', + children: [ + { type: 'element:record_picker', properties: { object: 'b', multiple: true } }, + ], + }, + ], + }, + }, ], }, ], }, + // The named-slot shape, on a slotted record page. + { + name: 'picker_detail', + kind: 'slotted', + regions: [], + slots: { + details: { type: 'element:record_picker', properties: { object: 'c', searchFields: ['name'] } }, + }, + }, ], }, after: { @@ -4932,13 +5158,35 @@ const recordPickerInertKeysRemoved: MetadataConversion = { components: [ { type: 'element:record_picker', properties: { object: 'showcase_project' } }, { type: 'element:form', properties: { object: 'a', multiple: true } }, + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [ + { + label: 'Pick one', + children: [ + { type: 'element:record_picker', properties: { object: 'b' } }, + ], + }, + ], + }, + }, ], }, ], }, + { + name: 'picker_detail', + kind: 'slotted', + regions: [], + slots: { + details: { type: 'element:record_picker', properties: { object: 'c' } }, + }, + }, ], }, - expectedNotices: 2, + expectedNotices: 4, }, }; @@ -4998,10 +5246,32 @@ const pageCardBodyToChildren: MetadataConversion = { }, // `body` on a component that is not a card — not this entry's key. { type: 'record:alert', properties: { body: 'Confirm the work before marking it done.' } }, + // A card nested in a card (#6775). The OUTER rename moves the + // sub-tree from `body` to `children`, and the descent reads the + // MAPPED component, so the inner card is visited exactly once — + // under the canonical key, not once per spelling. + { + type: 'page:card', + properties: { + title: 'Outer', + body: [ + { type: 'page:card', properties: { title: 'Inner', body: [{ type: 'element:text' }] } }, + ], + }, + }, ], }, ], }, + // The named-slot shape: a card authored into a slotted page's `details`. + { + name: 'my_work_detail', + kind: 'slotted', + regions: [], + slots: { + details: { type: 'page:card', properties: { title: 'Detail', body: [{ type: 'element:text' }] } }, + }, + }, ], }, after: { @@ -5021,13 +5291,30 @@ const pageCardBodyToChildren: MetadataConversion = { properties: { children: [{ type: 'element:text' }], body: [{ type: 'element:image' }] }, }, { type: 'record:alert', properties: { body: 'Confirm the work before marking it done.' } }, + { + type: 'page:card', + properties: { + title: 'Outer', + children: [ + { type: 'page:card', properties: { title: 'Inner', children: [{ type: 'element:text' }] } }, + ], + }, + }, ], }, ], }, + { + name: 'my_work_detail', + kind: 'slotted', + regions: [], + slots: { + details: { type: 'page:card', properties: { title: 'Detail', children: [{ type: 'element:text' }] } }, + }, + }, ], }, - expectedNotices: 1, + expectedNotices: 4, }, }; @@ -5079,12 +5366,13 @@ const pageCardBodyToChildren: MetadataConversion = { * already-present `bodyExtra` WINS and a differing object-form `params` is left * exactly where it sits, for the author to reconcile (#4923). * - * Region level is the reach, as for {@link pageHeaderSubtitleAlias} and - * {@link pageCardBodyToChildren}: `PageComponentSchema` declares no children - * key, so a button nested inside another component's free-form `properties` is - * not typed page-component shape. The array-only field is what covers the rest - * — it refuses the object form with a message naming `bodyExtra`, whether or - * not a conversion could reach the site. + * The reach is every position a button can be authored in, as for + * {@link pageHeaderSubtitleAlias} and {@link pageCardBodyToChildren} (#6775): + * `regions[].components[]`, `slots.`, and the containers a component + * nests under its `properties` — which is where a submit button most often + * sits, inside the card that holds the form. The array-only field still covers + * anything the rewrite declines to touch: it refuses the object form with a + * message naming `bodyExtra`, at any position. * * **Live window**; retires at 18. */ @@ -5170,10 +5458,44 @@ const inlineActionApiParamsToBodyExtra: MetadataConversion = { action: { type: 'url', target: '/x?id=${param.id}', params: { id: 'abc' } }, }, }, + // The shape a pure-SDUI form is actually built in (#6775): the + // submit button sits in the card's `footer`, one container down. + { + type: 'page:card', + properties: { + title: 'Contact us', + footer: [ + { + type: 'element:button', + properties: { + label: 'Send', + action: { type: 'api', target: '/api/v1/forms/send', params: { note: '{{page.note}}' } }, + }, + }, + ], + }, + }, ], }, ], }, + // The named-slot shape: an action button in a slotted page's `actions`. + { + name: 'showcase_contact_detail', + kind: 'slotted', + regions: [], + slots: { + actions: [ + { + type: 'element:button', + properties: { + label: 'Resend', + action: { type: 'api', target: '/api/v1/forms/resend', params: { id: '{{record.id}}' } }, + }, + }, + ], + }, + }, ], }, after: { @@ -5226,13 +5548,44 @@ const inlineActionApiParamsToBodyExtra: MetadataConversion = { action: { type: 'url', target: '/x?id=${param.id}', params: { id: 'abc' } }, }, }, + { + type: 'page:card', + properties: { + title: 'Contact us', + footer: [ + { + type: 'element:button', + properties: { + label: 'Send', + action: { type: 'api', target: '/api/v1/forms/send', bodyExtra: { note: '{{page.note}}' } }, + }, + }, + ], + }, + }, ], }, ], }, + { + name: 'showcase_contact_detail', + kind: 'slotted', + regions: [], + slots: { + actions: [ + { + type: 'element:button', + properties: { + label: 'Resend', + action: { type: 'api', target: '/api/v1/forms/resend', bodyExtra: { id: '{{record.id}}' } }, + }, + }, + ], + }, + }, ], }, - expectedNotices: 1, + expectedNotices: 3, }, }; @@ -5272,11 +5625,15 @@ const inlineActionApiParamsToBodyExtra: MetadataConversion = { * DISAGREEING pair is left for the author to reconcile rather than the loader * picking a look. * - * Region level is the reach, as for {@link pageCardBodyToChildren}: - * `PageComponentSchema` declares no children key, so a `page:tabs` nested inside - * another component's free-form `properties` is not typed page-component shape. - * The tombstone covers the rest — `tsc` at the authoring site and the parse at - * load, both carrying the prescription whether or not the walk reaches there. + * The reach is every position, as for {@link pageCardBodyToChildren} (#6775): + * a `page:tabs` nested inside another component's `properties` is rewritten + * where it sits. The discriminator matters more here than anywhere else, since + * the key being renamed shares a name with the node's dispatch key — so the + * fixture pins that descending into a props bag does NOT turn some other + * component's inner `type` (an action's `type: 'url'`, a tab item's fields) + * into a rewrite target: only `properties.type` on a node whose own `type` is + * `page:tabs` moves. The tombstone still carries the refusal at the authoring + * site (`tsc`) and at load (the parse), whatever the walk reached. * * `retiredFromLoadPath: true`: no alias window, deliberately. The tombstone owns * the refusal; this entry exists so `spec-changes.json`, the upgrade guide and @@ -5316,12 +5673,40 @@ const pageTabsTypeToTabStyle: MetadataConversion = { // rather than the loader picking for them. { type: 'page:tabs', properties: { tabStyle: 'pill', type: 'card', items: [] } }, // A `type` one level down inside another component's properties - // is a different key entirely — the walk is region-level and - // never descends into a props bag. + // is a different key entirely: `action.type` is the action's + // discriminator, and the walk descends into CONTAINER keys + // (`children` / `items[].children` / `body` / `footer`), never + // into an arbitrary props value. { type: 'element:button', properties: { label: 'Open', action: { type: 'url', target: '/x' } }, }, + // Tabs nested in a card, and tabs inside a tab panel (#6775): + // the rewrite reaches both, and the outer strip's own `items` + // stay ordinary tab records, not components. + { + type: 'page:card', + properties: { + title: 'Related', + children: [ + { type: 'page:tabs', properties: { type: 'pill', items: [{ label: 'Notes' }] } }, + ], + }, + }, + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [ + { + label: 'Nested', + children: [ + { type: 'page:tabs', properties: { type: 'card', items: [] } }, + ], + }, + ], + }, + }, ], }, ], @@ -5354,6 +5739,29 @@ const pageTabsTypeToTabStyle: MetadataConversion = { type: 'element:button', properties: { label: 'Open', action: { type: 'url', target: '/x' } }, }, + { + type: 'page:card', + properties: { + title: 'Related', + children: [ + { type: 'page:tabs', properties: { tabStyle: 'pill', items: [{ label: 'Notes' }] } }, + ], + }, + }, + { + type: 'page:tabs', + properties: { + tabStyle: 'line', + items: [ + { + label: 'Nested', + children: [ + { type: 'page:tabs', properties: { tabStyle: 'card', items: [] } }, + ], + }, + ], + }, + }, ], }, ], @@ -5367,7 +5775,7 @@ const pageTabsTypeToTabStyle: MetadataConversion = { }, ], }, - expectedNotices: 3, + expectedNotices: 5, }, }; diff --git a/packages/spec/src/conversions/walk.ts b/packages/spec/src/conversions/walk.ts index 82f5a1d3a4..2dc3bdddd8 100644 --- a/packages/spec/src/conversions/walk.ts +++ b/packages/spec/src/conversions/walk.ts @@ -175,36 +175,163 @@ export function mapPages(stack: Dict, mapper: (page: Dict, path: string) => Dict } /** - * Immutably map every **declared-shape** page component — the two places a - * `PageComponentSchema` actually lives: `stack.pages[].regions[].components[]` - * and `stack.pages[].slots.` (which is `PageComponent | PageComponent[]`). + * Depth ceiling for the page-component recursion, mirroring + * {@link MAX_REGION_DEPTH} and for the same reason: a stack handed to + * `defineStack` is hand-built objects rather than parsed JSON, so a component + * whose `properties.children` contains itself is reachable and would otherwise + * be an unbounded recursion on the load path. + */ +const MAX_COMPONENT_DEPTH = 32; + +/** + * The container keys a page component nests its sub-tree under, listed exactly + * as `walkPageComponents` (`packages/lint/src/page-walk.ts`) lists them: + * `page:card` → `body` / `footer`, every layout container → `children`. + * `page:tabs` / `page:accordion` hang theirs off `items[].children`, handled + * separately below because of the extra index. + * + * Recognised by SHAPE — an array — and deliberately NOT keyed by component + * `type`, which is the same rule lint applies: `properties` is an open bag that + * nothing validates per-type on the load path, and layout containers compose + * `children` without ever declaring it in a props schema. A `body: 'Confirm the + * work'` string on a `record:alert` is not an array and so is never mistaken + * for a slot; a non-dict element inside one is passed through untouched. + */ +const COMPONENT_CHILD_KEYS = ['children', 'body', 'footer'] as const; + +/** + * Map a list of page components, immutably. Returns the SAME array reference + * when nothing under it changed, so the copy-on-write contract survives the + * descent. + */ +function mapComponentList( + list: unknown[], + basePath: string, + mapper: (component: Dict, path: string) => Dict, + depth: number, +): unknown[] { + let changed = false; + const next = list.map((child, i) => { + if (!isDict(child)) return child; + const mapped = mapComponentTree(child, `${basePath}[${i}]`, mapper, depth); + if (mapped !== child) changed = true; + return mapped; + }); + return changed ? next : list; +} + +/** + * Map one page component **and everything nested under it** — the component + * itself, then the components inside its `properties` containers, recursively + * (a card inside a tab panel inside a card). + * + * The mapper runs on the container FIRST and the descent then reads the + * *mapped* component's `properties`, the same ordering {@link mapNodeTree} uses + * for flow regions. That is what keeps the walk single-visit under a conversion + * that MOVES a container key: `page-card-body-to-children` renames + * `properties.body` → `properties.children`, and because the descent happens + * after the rename, the sub-tree is walked once under the canonical key instead + * of once per spelling. + */ +function mapComponentTree( + component: Dict, + path: string, + mapper: (component: Dict, path: string) => Dict, + depth: number, +): Dict { + const mapped = mapper(component, path); + if (depth >= MAX_COMPONENT_DEPTH) return mapped; + const properties = mapped.properties; + if (!isDict(properties)) return mapped; + + let nextProps = properties; + + // `page:tabs` / `page:accordion` — `items[].children[]`. + const items = nextProps.items; + if (Array.isArray(items)) { + let itemsChanged = false; + const nextItems = items.map((item, i) => { + if (!isDict(item) || !Array.isArray(item.children)) return item; + const nextChildren = mapComponentList( + item.children, + `${path}.properties.items[${i}].children`, + mapper, + depth + 1, + ); + if (nextChildren === item.children) return item; + itemsChanged = true; + return { ...item, children: nextChildren }; + }); + if (itemsChanged) nextProps = { ...nextProps, items: nextItems }; + } + + for (const key of COMPONENT_CHILD_KEYS) { + const list = nextProps[key]; + if (!Array.isArray(list)) continue; + const next = mapComponentList(list, `${path}.properties.${key}`, mapper, depth + 1); + if (next !== list) nextProps = { ...nextProps, [key]: next }; + } + + return nextProps === properties ? mapped : { ...mapped, properties: nextProps }; +} + +/** + * Immutably map every page component — the two places a `PageComponentSchema` + * lives (`stack.pages[].regions[].components[]` and `stack.pages[].slots.`, + * which is `PageComponent | PageComponent[]`) **plus everything nested inside a + * component's `properties` containers**, to any depth. * * `mapper` receives each component dict and its path * (`pages[i].regions[j].components[k]`, `pages[i].slots.tabs`, - * `pages[i].slots.tabs[0]`) and returns the same reference (no change) or a new - * dict. Every container on the way — the stack, `pages`, a page, its `regions`, - * a region, its `components`, its `slots` — is copied only when a descendant - * actually changed: {@link mapPages}' contract, one level deeper. + * `pages[i].slots.tabs[0]`, + * `pages[i].regions[j].components[k].properties.items[0].children[1]`) and + * returns the same reference (no change) or a new dict. Every container on the + * way — the stack, `pages`, a page, its `regions`, a region, its `components`, + * its `slots`, and each `properties` bag on the way down — is copied only when + * a descendant actually changed: {@link mapPages}' contract, all the way down. + * + * **The reach is `walkPageComponents`'s reach (#6775), and getting there took + * two goes.** This walker's comment used to call region level "the whole + * surface", reasoning that a conversion can only reach what the type declares + * and that everything else sits in a free-form bag which the tombstone (`tsc` + + * the parse) covers instead. Both halves failed where it counted: + * + * - **`slots` (#6776)** is as typed as a region component — `PageSchema.slots` + * is a closed map of seven named slots, each + * `z.union([PageComponentSchema, z.array(PageComponentSchema)])` — and is + * the canonical authoring shape for a `kind: 'slotted'` record page. All + * four in-repo `page:tabs` sites are `slots.tabs`, so a region-only rewrite + * left `os migrate meta` unable to touch the only shape that key is written + * in, while the tombstone's prescription promised it would. + * - **Container nesting (#6775)** is where "the tombstone covers it" fails: a + * tombstone only fires for a key some props schema judges, and `properties` + * is an open bag nothing validates by `type` on the load path. So + * `page-header-subtitle-alias`, whose retired `description` is tombstoned + * nowhere (`description` is a live declared prop on other components), got + * no diagnostic at a nested site from any layer: the conversion did not + * fire, `PageSchema` stayed green, and the advisory props gate runs CLI-only + * and on already-converted metadata. A slotted record page — the shape + * objectui's own guide recommends — could carry a header the rewrite never + * saw, so objectui's `subtitle ?? description` fallback could not retire + * without silently dropping those subtitles. + * + * `walkPageComponents` in `packages/lint` has visited all three surfaces from + * the start, so until now every conversion here reached strictly less than the + * lint rule that judges its result — "same key, different meaning depending on + * where you put it", the position-dependence the flow-node region recursion + * (#4347) was built to abolish. * - * That is the whole surface, and it is bounded by the type rather than by the - * shape of any one page: `PageComponentSchema` declares no children key, so - * anything nested (tab panels, card bodies) sits inside another component's - * free-form `properties` and is NOT typed page-component shape — the tombstone - * (`tsc` + the parse) covers those, as every retirement entry's doc says. + * Two differences from the lint walk remain, both deliberate: * - * **`slots` was missing until #6776, and the gap was load-bearing.** This - * walker's own comment used to call region level "the whole surface", on the - * reasoning that everything else is inside a free-form bag. `slots` is the - * counter-example: `PageSchema.slots` is a closed map of seven named slots, - * each declared `z.union([PageComponentSchema, z.array(PageComponentSchema)])` - * — exactly as typed as a region component, and the canonical authoring shape - * for a `kind: 'slotted'` record page. `walkPageComponents` in `packages/lint` - * has always visited both, so every conversion here reached strictly less than - * the lint rule that judges the result. #6776 is where that cost something - * real: all four in-repo `page:tabs` authoring sites are `slots.tabs`, so a - * region-only rewrite would have left `os migrate meta` unable to touch the - * only shape that key is written in, while the tombstone's prescription - * promised it would. + * 1. **Source-authored pages** (`kind: 'html' | 'react' | 'jsx'`) are visited + * here and skipped there. For lint that skip prevents findings about a + * DERIVED region cache the author never wrote; a conversion still has to + * normalize that cache, or a stored page rehydrates in a shape the runtime + * no longer serves. Skipping them here would REMOVE reach conversions have + * had since #5509. + * 2. **The {@link MAX_COMPONENT_DEPTH} ceiling** has no counterpart in lint, + * which walks parsed JSON only. This walker also runs on hand-built + * `defineStack` objects, where a self-referencing `children` is reachable. */ export function mapPageComponents( stack: Dict, @@ -221,15 +348,14 @@ export function mapPageComponents( const components = region.components; if (!Array.isArray(components)) return region; - let componentsChanged = false; - const nextComponents = components.map((component, ci) => { - if (!isDict(component)) return component; - const mapped = mapper(component, `${pagePath}.regions[${ri}].components[${ci}]`); - if (mapped !== component) componentsChanged = true; - return mapped; - }); + const nextComponents = mapComponentList( + components, + `${pagePath}.regions[${ri}].components`, + mapper, + 0, + ); - if (!componentsChanged) return region; + if (nextComponents === components) return region; regionsChanged = true; return { ...region, components: nextComponents }; }); @@ -247,19 +373,13 @@ export function mapPageComponents( // matches it so a conversion notice and a lint finding name one site // with one string. if (Array.isArray(value)) { - let listChanged = false; - const nextList = value.map((component, i) => { - if (!isDict(component)) return component; - const mapped = mapper(component, `${pagePath}.slots.${slot}[${i}]`); - if (mapped !== component) listChanged = true; - return mapped; - }); - if (!listChanged) continue; + const nextList = mapComponentList(value, `${pagePath}.slots.${slot}`, mapper, 0); + if (nextList === value) continue; nextSlots[slot] = nextList; slotsChanged = true; } else { if (!isDict(value)) continue; - const mapped = mapper(value, `${pagePath}.slots.${slot}`); + const mapped = mapComponentTree(value, `${pagePath}.slots.${slot}`, mapper, 0); if (mapped === value) continue; nextSlots[slot] = mapped; slotsChanged = true;