From 95075dd1c2a657e0bc607ded920af4babf29b238 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 10:39:10 +0000 Subject: [PATCH] fix(spec): the page-component conversion walk reaches nested containers, not only regions and slots (#6775) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `mapPageComponents` visited `pages[].regions[].components[]` and `pages[].slots.` and stopped. A component nested inside another component's `properties` — a card's `children` / `body` / `footer`, a `page:tabs` / `page:accordion` panel's `items[].children` — was visited by nobody, so no page-component conversion rewrote it, while `walkPageComponents` in `packages/lint` has descended into those containers from the start. Every conversion therefore reached strictly less than the lint rule that judges its result. The walk 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. The mapper still runs on the container first and the descent reads the MAPPED component, so `page-card-body-to-children` — which moves `properties.body` to `properties.children` — walks its sub-tree exactly once, under the canonical key. Copy-on-write is unchanged: an untouched sub-tree keeps its reference, and a stack where nothing converts is returned by identity. The load-path cost this fixes belongs to `page-header-subtitle-alias`. Every other entry leans on a tombstone for the sites a conversion cannot reach; this one has none, because `description` stays a live declared prop on other components, so `properties.description` parses green at any position. A header authored in a card or on a `kind: 'slotted'` record page got no rewrite and no diagnostic from any layer. Fixtures for all seven walker-backed conversions now pin the slotted and nested positions beside the region-level one, and a cross-walker parity test in `packages/lint` — the only place that can see both walkers — asserts the two reachable sets agree, pinning the two deliberate differences: lint skips source-authored pages (the conversion must still normalize their derived cache) and the conversion walk keeps a 32-container depth ceiling for hand-built `defineStack` objects. No conversion registry entry added or removed. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Cd32yJ2omZsgUXxiAjeBcE --- .../conversion-walk-nested-page-components.md | 59 +++ .../src/page-walk-conversion-parity.test.ts | 145 ++++++ .../conversions/page-component-walk.test.ts | 217 ++++++++ packages/spec/src/conversions/registry.ts | 468 ++++++++++++++++-- packages/spec/src/conversions/walk.ts | 204 ++++++-- 5 files changed, 1021 insertions(+), 72 deletions(-) create mode 100644 .changeset/conversion-walk-nested-page-components.md create mode 100644 packages/lint/src/page-walk-conversion-parity.test.ts create mode 100644 packages/spec/src/conversions/page-component-walk.test.ts 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;