From 84051b467729cffad2919dd16a2d64362bd54879 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 22:23:45 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(layout):=20navigation-renderer=20?= =?UTF-8?q?=E7=9A=84=20items=20=E5=A3=B0=E6=98=8E=E4=B8=BA=20required=20?= =?UTF-8?q?=E2=80=94=E2=80=94=20=E6=A0=A1=E9=AA=8C=E5=99=A8=E4=B8=8D?= =?UTF-8?q?=E5=86=8D=E6=94=BE=E8=BF=87=E5=BF=85=E5=B4=A9=E7=9A=84=E8=8A=82?= =?UTF-8?q?=E7=82=B9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `NavigationRendererProps.items` 是非可选的 `NavigationItem[]`,渲染器不给默认值, 而注册声明一直没写 `required`。`sdui-parser` 只在 `input.required` 为真时报 `missing-required-prop`(validate.ts:55-64),于是 `{ "type": "navigation-renderer" }` 校验零诊断、渲染直接抛。 运行期实测(非静态读出):第一处无守卫的读点是 `pinnedItems` memo 里的 `collectPinnedItems(filteredItems)`(NavigationRenderer.tsx:1242 → :1410 的 `for (const item of items)`),抛 `TypeError: items is not iterable`。 `resolveActiveNavItem` memo 挡得住(`visit` 首行 `if (!nodes) return`), :1247 的 `filteredItems.slice()` 同样会抛但走不到。 这是 #3972(键的存在 × 类型)的第四面:可选性。与 #3900/#3972 相反,它是收紧 —— 新增一条 error 级诊断,而被它拦下的形状恰好等于渲染必然崩溃的形状。 钉子两方向 + 对照: - 注册级(packages/layout):`items.required === true`(两个 namespace); `basePath.required` 保持 falsy(渲染器真给了 `basePath = ''` 默认值); 编译期镜像 `undefined extends items` 为假;运行期崩溃见证。 - 端到端(examples/schema-catalog,与 #3972 的 manifest 门钉同处):省略 items 报 error 级 `missing-required-prop`;提供数组零诊断;可选属性省略不报。 - #3900 的 `not-a-container` 对照节点补 `items: []`,恢复单一事实(注释同步)。 Fixes #3987 Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .changeset/navigation-items-required-3987.md | 30 +++++ .../test/pageheader-with-actions.test.tsx | 95 ++++++++++++-- ...gation-renderer-items-declaration.test.tsx | 121 +++++++++++++++++- packages/layout/src/index.ts | 21 ++- 4 files changed, 256 insertions(+), 11 deletions(-) create mode 100644 .changeset/navigation-items-required-3987.md diff --git a/.changeset/navigation-items-required-3987.md b/.changeset/navigation-items-required-3987.md new file mode 100644 index 0000000000..6281a5e876 --- /dev/null +++ b/.changeset/navigation-items-required-3987.md @@ -0,0 +1,30 @@ +--- +"@object-ui/layout": patch +--- + +`navigation-renderer` 的 `items` 声明为 `required: true` —— 校验器不再放过必崩的节点 + +`items` 在组件侧是**非可选**的 `NavigationItem[]`(`NavigationRendererProps.items`,无 `?`), +渲染器也不给默认值,而注册声明一直没写 `required`。`sdui-parser` 只在 `input.required` 为真时 +报 `missing-required-prop`(`validate.ts:55-64`),于是 `{ "type": "navigation-renderer" }` +这个节点**校验零诊断、渲染直接抛**:第一处无守卫的读点是 `pinnedItems` memo 里的 +`collectPinnedItems(filteredItems)`(`NavigationRenderer.tsx:1242` → `:1410` 的 +`for (const item of items)`),实测 `TypeError: items is not iterable`。 +(`resolveActiveNavItem` memo 挡得住 —— 它的 `visit` 首行是 `if (!nodes) return`; +`:1247` 的 `filteredItems.slice()` 同样会抛,但根本走不到。) + +这是 objectui#3972(键的**存在**与**类型**三面对齐)的第四面:**可选性**。#3972 与 +objectui#3900 都是删除假诊断,这一条相反——它是**收紧**。 + +**blast radius:** 今天省略 `items` 写 `navigation-renderer` 的 schema,会新增一条 +**error** 级 `missing-required-prop`。受影响面是仓外按 `inputs` / `packages/layout/README.md` +做 schema 驱动的消费者(仓内没有任何 JSON 元数据把它当 schema 节点写,React 调用侧的必填由 +TS 兜住;`examples/schema-catalog` 的 `not-a-container` 对照节点补了 `items: []`,使它只剩 +那一处故意植入的缺陷)。而这条诊断新拦下的形状,**恰好等于渲染必然崩溃的形状** —— 让作者 +(尤其是 AI 作者)在发布期就听到运行期注定要发生的失败,正是 `missing-required-prop` 存在 +的理由。若某个消费者确实想要"缺 items 就渲染空导航",那是给组件加 `= []` 默认值的另一条路 +(objectui#3987 里记了,与本改动不互斥),而不是让校验器继续沉默。 + +`basePath` **保持可选**并被钉成对照:渲染器真的给了它默认值(`basePath = ''`)。`required` +是逐个属性从组件读出的事实,不是一刀切——否则这道门会开始拒绝完全能渲染的 schema,作者就会 +学着无视 `missing-required-prop`,正如 #3972 里他们被教着无视 `type-mismatch`。 diff --git a/examples/schema-catalog/test/pageheader-with-actions.test.tsx b/examples/schema-catalog/test/pageheader-with-actions.test.tsx index 2bebc81b2e..51cd3b9747 100644 --- a/examples/schema-catalog/test/pageheader-with-actions.test.tsx +++ b/examples/schema-catalog/test/pageheader-with-actions.test.tsx @@ -10,7 +10,7 @@ * rendering, and it held a third, already-drifted copy of the component's * spacing numbers (objectui#3786 fixed the second copy, in the prose). * - * Four things are pinned here, and they are different facts: + * Five things are pinned here, and they are different facts: * * 1. SHAPE — the example's root node is a `page-header`, and it contains none * of the class strings that only exist inside `PageHeader.tsx`. This is the @@ -23,6 +23,10 @@ * 4. VALIDATE, PROPS — the same JSON draws no `unknown-prop` either, and an * array-valued `navigation-renderer.items` draws no `type-mismatch` (#3972). * Same lie as (3) on the prop face instead of the containment face. + * 5. VALIDATE, OPTIONALITY — a `navigation-renderer` WITHOUT `items` now draws + * an error-level `missing-required-prop` (#3987), and the props the renderer + * defaults still draw nothing. Unlike (3) and (4) this one is a tightening, + * not a false-diagnostic fix; see that describe's header. * * (2) and (3) are two halves of one contradiction that used to be live. The * render path never consults `isContainer` — `SchemaRenderer` strips `children` @@ -188,15 +192,20 @@ describe('the documented demo validates clean of `not-a-container` (#3900)', () // author must still hear about it. If this component ever legitimately // becomes a container, this assertion goes red — move the control to // another childless registration rather than deleting it. - // `items` is deliberately omitted (it is not `required`, so its absence - // draws nothing): this control is about containment only. It used to have a - // second reason — writing `items: []` ALSO drew a `type-mismatch`, because the - // registration declared that prop `type: 'object'` while - // `NavigationRendererProps.items` is `NavigationItem[]`. That defect is fixed - // (#3972) and pinned in the describe below; the omission here is now about - // keeping this control single-fact, nothing more. + // `items: []` is written for the same single-fact reason the two earlier + // versions of this comment gave, arrived at from the opposite direction each + // time. It used to be OMITTED because writing it drew a `type-mismatch` (the + // registration said `type: 'object'` while `NavigationRendererProps.items` is + // `NavigationItem[]` — fixed by #3972, pinned in the describe below), and + // because its absence drew nothing at all. Both halves have since moved: the + // array form now validates clean, and the absence draws + // `missing-required-prop` (#3987 — omitting `items` crashes the renderer, so + // the declaration says `required: true`). Supplying the empty array keeps the + // planted defect — children under a childless component — the only thing + // wrong with this node. const codes = diagnose({ type: 'navigation-renderer', + items: [], children: [{ type: 'button', label: 'Nope' }], }).map((d) => d.code); @@ -282,3 +291,73 @@ describe('the declaration face matches what the renderers read (#3972)', () => { ); }); }); + +/** + * The optionality face of the same key (#3987), through the same manifest. + * + * `navigation-renderer.items` is non-optional in TS and has no default, so a node + * that omits it throws `TypeError: items is not iterable` on the first thing the + * render does with it (`NavigationRenderer.tsx:1242` → `:1410`; measured in + * `packages/layout/src/__tests__/navigation-renderer-items-declaration.test.tsx`). + * The declaration used to leave `required` unset, and `validate.ts:55-64` reports + * `missing-required-prop` only when it is set — so the ONE node shape guaranteed + * to crash was also the one shape the validator had nothing to say about. + * + * This is a TIGHTENING, not a false-diagnostic fix like #3900/#3972: it adds an + * error-level diagnostic to schemas that validated clean yesterday. That is the + * point — the schemas it newly rejects are exactly the ones that cannot render — + * but it is also why the control below matters more than usual. "Required" has to + * stay a per-prop fact read off the component: if it ever becomes a blanket, this + * gate starts rejecting perfectly renderable schemas and authors learn to ignore + * `missing-required-prop` the way #3972's authors were being taught to ignore + * `type-mismatch`. + */ +describe('the validator now reports the node shape that is guaranteed to crash (#3987)', () => { + const REQUIRED = 'missing-required-prop'; + + it('reports `missing-required-prop` for a `navigation-renderer` without `items`', () => { + const diagnostics = diagnose({ type: 'navigation-renderer' }); + + // Reachability before the presence assertion: an `unknown-component` return + // never reaches the required-prop loop at all, so this would otherwise pass + // on a manifest that lost the tag. + expect(diagnostics.filter((d) => d.code === 'unknown-component')).toEqual([]); + + const missing = diagnostics.find((d) => d.code === REQUIRED); + expect(missing, 'omitting `items` drew no `missing-required-prop`').toBeTruthy(); + // Error, not warning — the render cannot recover, so neither should the gate. + expect(missing?.severity).toBe('error'); + expect(missing?.message).toContain('"items"'); + }); + + it('reports nothing once `items` is supplied', () => { + // The array form the renderer actually consumes: no `missing-required-prop`, + // and no `type-mismatch` either (that pair is #3972's, asserted above). + expect(diagnose({ type: 'navigation-renderer', items: [] }).map((d) => d.code)).toEqual([]); + expect( + diagnose({ + type: 'navigation-renderer', + items: [{ id: 'home', type: 'object', label: 'Home', objectName: 'home' }], + basePath: '/apps/crm', + }).map((d) => d.code), + ).toEqual([]); + }); + + it('does not report the optional props the renderer defaults', () => { + // The control that keeps `required` a per-prop fact. `basePath` is omitted in + // both nodes above; the renderer defaults it (`basePath = ''`), the + // declaration leaves `required` unset, and the gate must stay silent about + // it. If this goes red, the tightening has spread from "the prop whose + // absence crashes" to "every declared prop", and the assertions above stop + // meaning what they say. + const codes = diagnose({ type: 'navigation-renderer', items: [] }).map((d) => d.code); + expect(codes).not.toContain(REQUIRED); + + // …and the same for a second component in the same registration file, so the + // control is not one prop's accident: `page-header` renders with only a + // title, and every key it declares is optional. + expect(diagnose({ type: 'page-header', title: 'Users' }).map((d) => d.code)).not.toContain( + REQUIRED, + ); + }); +}); diff --git a/packages/layout/src/__tests__/navigation-renderer-items-declaration.test.tsx b/packages/layout/src/__tests__/navigation-renderer-items-declaration.test.tsx index e426a06755..11bcb25ccb 100644 --- a/packages/layout/src/__tests__/navigation-renderer-items-declaration.test.tsx +++ b/packages/layout/src/__tests__/navigation-renderer-items-declaration.test.tsx @@ -36,14 +36,23 @@ * `examples/schema-catalog/test/pageheader-with-actions.test.tsx`, next to the * `diagnose()` helper that builds the manifest the app really validates against. * + * objectui#3987 added the fourth face of the same key — `required: true`, plus + * the render crash that makes it a crash-stopper rather than documentation — in + * the two describes at the bottom of this file, and its manifest-gate half sits + * beside #3972's in that same schema-catalog file. + * * Module-scope import of the barrel, not `beforeAll` (AGENTS.md §测试纪律): the * registration is a load-time side effect of `../index`, and its transform cost * belongs to the import phase rather than a hook's 10s budget. */ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi } from 'vitest'; +import React from 'react'; +import { render, screen } from '@testing-library/react'; +import { MemoryRouter } from 'react-router-dom'; +import { SidebarProvider } from '@object-ui/components'; import { ComponentRegistry } from '@object-ui/core'; -import { registerLayout, type NavigationRendererProps } from '../index'; +import { registerLayout, NavigationRenderer, type NavigationRendererProps } from '../index'; registerLayout(); @@ -71,3 +80,111 @@ describe('the `navigation-renderer` registration declares `items` as an array (o expect(ITEMS_ARE_AN_ARRAY).toBe(true); }); }); + +/** + * The FOURTH face of the same agreement: `items` is declared REQUIRED + * (objectui#3987). + * + * #3972 above aligned the key's existence and its type. Optionality is a + * separate fact and it was declared the opposite of what the component + * enforces: `NavigationRendererProps.items` has no `?`, the renderer supplies + * no default, and `sdui-parser` reports `missing-required-prop` only when + * `input.required` is set (`validate.ts:55-64`). So a bare + * `{ "type": "navigation-renderer" }` node passed validation in complete + * silence and then threw on render — the crash is witnessed by the describe + * below, not inferred. + * + * `basePath` is the control, and a real one rather than a nonsense key: the + * renderer genuinely defaults it (`basePath = ''`), so it must stay optional. + * "Everything is required" would be as wrong as "nothing is" — the flag is a + * per-prop fact about whether the component can proceed without it. + */ +type ItemsIsRequiredProp = undefined extends NavigationRendererProps['items'] ? false : true; +const ITEMS_IS_REQUIRED_PROP: ItemsIsRequiredProp = true; + +describe('the `navigation-renderer` registration declares `items` required (objectui#3987)', () => { + it.each([undefined, 'layout'])('declares `required: true` (namespace: %s)', (namespace) => { + const config = ComponentRegistry.getConfig('navigation-renderer', namespace); + expect(config, 'navigation-renderer is not registered').toBeTruthy(); + + const items = (config?.inputs ?? []).find((input) => input.name === 'items'); + expect(items, 'navigation-renderer no longer declares `items` at all').toBeTruthy(); + expect(items?.required).toBe(true); + }); + + it.each([undefined, 'layout'])( + 'leaves `basePath` optional — the renderer defaults it (namespace: %s)', + (namespace) => { + const config = ComponentRegistry.getConfig('navigation-renderer', namespace); + const basePath = (config?.inputs ?? []).find((input) => input.name === 'basePath'); + expect(basePath, 'navigation-renderer no longer declares `basePath` at all').toBeTruthy(); + // Not `toBe(false)`: the declaration omits the flag entirely, and + // `validate.ts` reads it as a truthiness test. What must never happen is + // this control turning truthy, which would mean the `required` assertion + // above is measuring a blanket rather than a per-prop decision. + expect(basePath?.required).toBeFalsy(); + }, + ); + + it('and the prop it describes is still non-optional in TS', () => { + // Compile-time half: if `items` ever becomes `items?: NavigationItem[]`, + // `ItemsIsRequiredProp` resolves to `false` and this file stops + // type-checking — the declaration cannot silently drift back. + expect(ITEMS_IS_REQUIRED_PROP).toBe(true); + }); +}); + +/** + * Why the declaration above is a crash-stopper and not documentation + * (objectui#3987). + * + * objectui#3987 read the crash statically (no default + unguarded + * dereferences). This is the measured version, and it corrects the reading in + * one place: the FIRST unguarded read is `collectPinnedItems(filteredItems)` in + * the `pinnedItems` memo (`NavigationRenderer.tsx:1242` → `:1410`), which does + * `for (const item of items)`. The `resolveActiveNavItem` memo above it survives + * an absent list (its `visit` starts with `if (!nodes) return`), and + * `filteredItems.slice()` at `:1247` would throw too but is never reached. + * + * If the alternative route in objectui#3987 is ever taken — giving `items` a + * `= []` default so a node without it renders an empty navigation — REPLACE this + * describe with the empty-render assertion instead of deleting it, and keep + * `required: true`: an author who forgot `items` wants a diagnostic, and a + * silently empty sidebar is not one. + */ +describe('a `navigation-renderer` authored without `items` crashes on render (objectui#3987)', () => { + const renderNav = (props: Partial) => + render( + + + + + , + ); + + it('throws a TypeError instead of rendering an empty navigation', () => { + // React logs the render error on its own; the assertion is about the throw, + // so keep the captured output clean. + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}); + try { + // `basePath` is supplied so the ONLY missing prop is the required one. + expect(() => renderNav({ basePath: '/apps/crm' })).toThrow(TypeError); + // The wording is V8's for `for…of` over `undefined`; the load-bearing part + // is the throw, but pinning the text catches a swap to some other failure + // mode that would need this whole describe re-read. + expect(() => renderNav({ basePath: '/apps/crm' })).toThrow(/items is not iterable/); + } finally { + consoleError.mockRestore(); + } + }); + + it('renders fine when `items` is supplied and the optional `basePath` is not', () => { + // The other half of the control pair: the props the declaration calls + // optional really are optional, so the throw above is about `items` and not + // about rendering this component bare in a test. + expect(() => + renderNav({ items: [{ id: 'nav_accounts', type: 'object', label: 'Accounts', objectName: 'account' }] }), + ).not.toThrow(); + expect(screen.getByText('Accounts')).toBeTruthy(); + }); +}); diff --git a/packages/layout/src/index.ts b/packages/layout/src/index.ts index e7353a67e3..7c88da6ad2 100644 --- a/packages/layout/src/index.ts +++ b/packages/layout/src/index.ts @@ -160,12 +160,31 @@ export function registerLayout() { // This is not objectui#3832 (`ComponentInput.type` cannot spell a spec union): // `ManifestInputType` has `'array'`, so the declaration was simply wrong about // a type it could express exactly. + // + // `required: true` is the fourth face of the same agreement (objectui#3987). + // #3972 aligned the key's EXISTENCE and TYPE; optionality was still declared + // the opposite of what the component enforces. `NavigationRendererProps.items` + // has no `?` and the renderer supplies no default (`NavigationRenderer.tsx:1204`), + // so `{ "type": "navigation-renderer" }` — a node the validator passed in + // silence, because `validate.ts:55-64` only reports `missing-required-prop` + // when `input.required` is set — crashes on the first thing the render does + // with the prop: `collectPinnedItems(filteredItems)` at `:1242` does + // `for (const item of items)` (`:1410`) and throws + // `TypeError: items is not iterable`. (The `resolveActiveNavItem` memo above + // it survives, its `visit` guards `if (!nodes) return`; `filteredItems.slice()` + // at `:1247` would throw too but is never reached.) + // + // So this is not a stylistic "document it as required" — it is the one + // diagnostic that exists precisely to stop a node whose render is a + // guaranteed crash from shipping. `basePath` below stays optional because the + // renderer really does default it (`basePath = ''`); the two are declared + // differently because the component treats them differently. ComponentRegistry.register('navigation-renderer', NavigationRenderer, { namespace: 'layout', label: 'Navigation Renderer', category: 'Layout', inputs: [ - { name: 'items', type: 'array' }, + { name: 'items', type: 'array', required: true }, { name: 'basePath', type: 'string' }, ], }); From 1364d5c2c0f8abca8aa230f4108c30c7786aaed9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 9 Aug 2026 22:28:19 +0000 Subject: [PATCH 2/2] =?UTF-8?q?docs(test):=20=E4=BF=AE=E6=AD=A3=20#3987=20?= =?UTF-8?q?=E5=AF=B9=E7=85=A7=E6=B3=A8=E9=87=8A=E9=87=8C=20basePath=20?= =?UTF-8?q?=E7=9C=81=E7=95=A5=E8=8C=83=E5=9B=B4=E7=9A=84=E8=AF=B4=E6=B3=95?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 上一版写成"两个节点都省略了 basePath",而上一条测试的第二个节点其实供了它 (两者都零诊断)。注释真实性是这条改动本身在管的事,顺手改准。 Co-authored-by: Claude Claude-Session: https://claude.ai/code/session_01GTRjn8xBqp75dk7kFupVRt --- .../test/pageheader-with-actions.test.tsx | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/examples/schema-catalog/test/pageheader-with-actions.test.tsx b/examples/schema-catalog/test/pageheader-with-actions.test.tsx index 51cd3b9747..1e00187631 100644 --- a/examples/schema-catalog/test/pageheader-with-actions.test.tsx +++ b/examples/schema-catalog/test/pageheader-with-actions.test.tsx @@ -344,12 +344,13 @@ describe('the validator now reports the node shape that is guaranteed to crash ( }); it('does not report the optional props the renderer defaults', () => { - // The control that keeps `required` a per-prop fact. `basePath` is omitted in - // both nodes above; the renderer defaults it (`basePath = ''`), the - // declaration leaves `required` unset, and the gate must stay silent about - // it. If this goes red, the tightening has spread from "the prop whose - // absence crashes" to "every declared prop", and the assertions above stop - // meaning what they say. + // The control that keeps `required` a per-prop fact. `basePath` is omitted + // here (as it is in the first node of the test above, while the second one + // supplies it — both validate clean): the renderer defaults it + // (`basePath = ''`), the declaration leaves `required` unset, and the gate + // must stay silent about it. If this goes red, the tightening has spread + // from "the prop whose absence crashes" to "every declared prop", and the + // assertions above stop meaning what they say. const codes = diagnose({ type: 'navigation-renderer', items: [] }).map((d) => d.code); expect(codes).not.toContain(REQUIRED);