diff --git a/.changeset/6523-6346-menuitem-union-onclick.md b/.changeset/6523-6346-menuitem-union-onclick.md new file mode 100644 index 0000000000..fc8446dc9f --- /dev/null +++ b/.changeset/6523-6346-menuitem-union-onclick.md @@ -0,0 +1,49 @@ +--- +'@object-ui/types': minor +'@object-ui/components': minor +--- + +`MenuItem` is now a discriminated union, and all three menu renderers read the keys it +declares (objectui#6523, objectui#6346, maintainer ruling 2026-08-27 — "one answer for the +whole `MenuItem` family"). + +**The break, spelled out.** `MenuItem` (`@object-ui/types`, shared by `ui:dropdown-menu`, +`ui:context-menu` and `ui:menubar`) used to be a single object with `label: string` +required unconditionally. It is now `MenuCommandItem | MenuDividerItem`: a command item +(`label` required, plus `icon`/`disabled`/`onClick`/`shortcut`/`children`) or a divider +(`{ separator: true }`, nothing else). The union — not `label?: string` — is deliberate: it +is what the data actually is, and it keeps the command arm's label protection intact rather +than weakening it repo-wide to accommodate the divider. Both arms also tombstone `type` +(`type?: never` / `z.never().optional()`): the retired `{ type: 'separator' }` (and its +sibling `{ type: 'label' }`) is now a **declared refusal** at parse time, not a silent strip. +A consumer's own `MenuItem[]` authored with either retired spelling now fails +`MenuItemSchema.safeParse` and fails `tsc` under the published `.d.ts`; a consumer authoring +the declared `{ separator: true }` divider now **succeeds** for the first time — before this +change it failed a strict parse too, because `label` had no way to be omitted. + +**Renderer accept behaviour changes to match.** `dropdown-menu` and `context-menu` used to +branch on the undeclared `item.type === 'separator'`; an author who instead wrote the +DECLARED `{ separator: true }` got a value that validated, published, and rendered a blank +menu row (the divider fell through to the ordinary item branch with no `label`). Both +renderers now branch on `item.separator`, matching `menubar` — which had this right all +along and is the evidence the type, not those two renderers, was correct. Their registry +`defaultProps` and `description` strings stop teaching the retired dialect; the 4 places it +appeared in this repo (2 schema-catalog fixtures, 2 registry `defaultProps`) are migrated. + +**The item handler moves to the declared key (objectui#6346).** All three renderers now +fire `item.onClick` — the key `MenuItem` has always declared (TS source, built `.d.ts`, and +the Zod mirror all agreed) but that `dropdown-menu`/`context-menu` never read (they read an +undeclared `item.onSelect` instead) and that `menubar` wired nowhere at all. An author who +followed the published type and set `onClick` got a value that validated, published, and +never fired; that is fixed. `renderMenuItems`/`renderContextMenuItems` also tighten from +`items: any[]` to `items: MenuItem[]` — the widening that let the mismatch type-check in the +first place. Migration cost measured **zero** in this repo: no fixture, doc or test authored +`onSelect` on a menu item before this change. + +**Rider, recorded as parity not new capability.** `menubar` now also renders the declared +`shortcut` string — `dropdown-menu` and `context-menu` already drew it, so this aligns the +third container rather than expanding the surface. + +Everything that rendered correctly before this change still renders the same way; the +narrowing only refuses spellings that were already unprotected (silently stripped or never +read at all). diff --git a/content/docs/components/overlay/context-menu.mdx b/content/docs/components/overlay/context-menu.mdx index 2ca05f8d6f..8e37947729 100644 --- a/content/docs/components/overlay/context-menu.mdx +++ b/content/docs/components/overlay/context-menu.mdx @@ -18,22 +18,34 @@ submenu trigger. A name that is not a live key of that record (an unknown or a retired spelling such as `edit`) renders **no glyph**, never a fallback glyph and never the literal name as text. +## Dividers + +An item is either a command or a divider between groups of commands — never +both, and a divider never carries a `label` (objectui#6523). Author a divider +as `{ "separator": true }`; the retired `{ "type": "separator" }` spelling is +refused at parse time rather than silently ignored. + ## Schema ```plaintext -interface ContextMenuItem { - label?: string; +interface ContextMenuCommandItem { + label: string; value?: string; icon?: string; // kebab-case Lucide icon name (e.g. "trash") - type?: 'separator'; disabled?: boolean; + onClick?: () => void; // Click handler } +interface ContextMenuDividerItem { + separator: true; // renders as a divider — no label +} + +type ContextMenuItem = ContextMenuCommandItem | ContextMenuDividerItem; + interface ContextMenuSchema { type: 'context-menu'; trigger: ComponentSchema; // Trigger element items: ContextMenuItem[]; // Menu items - onSelect?: string | ActionConfig; className?: string; } ``` diff --git a/content/docs/components/overlay/dropdown-menu.mdx b/content/docs/components/overlay/dropdown-menu.mdx index 040e115b3d..206aa75688 100644 --- a/content/docs/components/overlay/dropdown-menu.mdx +++ b/content/docs/components/overlay/dropdown-menu.mdx @@ -21,26 +21,36 @@ resolve against. A name that is not a live key of that record (an unknown or a retired spelling such as `edit`) renders **no glyph**, never a fallback glyph and never the literal name as text. +## Dividers + +An item is either a command or a divider between groups of commands — never +both, and a divider never carries a `label` (objectui#6523). Author a divider +as `{ "separator": true }`; the retired `{ "type": "separator" }` spelling is +refused at parse time rather than silently ignored. + ## Schema ```plaintext -interface DropdownMenuItem { - label?: string; +interface DropdownMenuCommandItem { + label: string; value?: string; icon?: string; // kebab-case Lucide icon name (e.g. "square-pen") variant?: 'default' | 'destructive'; - type?: 'separator'; disabled?: boolean; + onClick?: () => void; // Click handler +} + +interface DropdownMenuDividerItem { + separator: true; // renders as a divider — no label } +type DropdownMenuItem = DropdownMenuCommandItem | DropdownMenuDividerItem; + interface DropdownMenuSchema { type: 'dropdown-menu'; trigger: ComponentSchema; // Trigger component items: DropdownMenuItem[]; // Menu items - // Events - onSelect?: string | ActionConfig; - // Styling className?: string; } diff --git a/content/docs/components/overlay/menubar.mdx b/content/docs/components/overlay/menubar.mdx index 3c53944aa4..951f197f5b 100644 --- a/content/docs/components/overlay/menubar.mdx +++ b/content/docs/components/overlay/menubar.mdx @@ -9,18 +9,30 @@ The Menubar component provides a horizontal menu bar similar to desktop applicat +## Dividers + +An item is either a command or a divider between groups of commands — never +both, and a divider never carries a `label`. Author a divider as +`{ "separator": true }`. + ## Schema ```plaintext -interface MenubarItem { - label?: string; +interface MenubarCommandItem { + label: string; value?: string; icon?: string; - shortcut?: string[]; - type?: 'separator'; + shortcut?: string; // e.g. "Ctrl+T" — a single string, not an array disabled?: boolean; + onClick?: () => void; // Click handler } +interface MenubarDividerItem { + separator: true; // renders as a divider — no label +} + +type MenubarItem = MenubarCommandItem | MenubarDividerItem; + interface MenubarMenu { label: string; items: MenubarItem[]; @@ -29,11 +41,6 @@ interface MenubarMenu { interface MenubarSchema { type: 'menubar'; menus: MenubarMenu[]; // Menu definitions - - // Events - onSelect?: string | ActionConfig; - - // Styling className?: string; } ``` diff --git a/examples/schema-catalog/src/schemas/components-overlay-context-menu/basic-context-menu.json b/examples/schema-catalog/src/schemas/components-overlay-context-menu/basic-context-menu.json index ab2c8eb93b..78ba83bd75 100644 --- a/examples/schema-catalog/src/schemas/components-overlay-context-menu/basic-context-menu.json +++ b/examples/schema-catalog/src/schemas/components-overlay-context-menu/basic-context-menu.json @@ -22,7 +22,7 @@ "icon": "clipboard" }, { - "type": "separator" + "separator": true }, { "label": "Delete", diff --git a/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/basic-dropdown-menu.json b/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/basic-dropdown-menu.json index 7217726651..feec00c5e3 100644 --- a/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/basic-dropdown-menu.json +++ b/examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/basic-dropdown-menu.json @@ -14,7 +14,7 @@ "value": "settings" }, { - "type": "separator" + "separator": true }, { "label": "Logout", diff --git a/examples/schema-catalog/test/component-fixture-declared-keys.test.ts b/examples/schema-catalog/test/component-fixture-declared-keys.test.ts index 619da41974..b4fc45f799 100644 --- a/examples/schema-catalog/test/component-fixture-declared-keys.test.ts +++ b/examples/schema-catalog/test/component-fixture-declared-keys.test.ts @@ -401,25 +401,66 @@ describe('catalog corpus: no toaster node carries a key ToasterSchema does not d * string[]` and teaching the renderer to draw it was ruled OUT as a capability * expansion, and is not what this block pins.) * - * ## Why this sweep is scoped to `menubar` nodes and not to the menu family + * ## UPDATE (objectui#6523, maintainer ruling 2026-08-27) — the split this + * section used to describe is now fixed, and the counter-probe below changed + * shape as a direct result * - * It would be one line to sweep every `MenuItem`-shaped container. That would - * be WRONG here. `dropdown-menu.tsx:46` and `context-menu.tsx:44` branch on - * `item.type === 'separator'` — the undeclared spelling — and both render - * `item.shortcut`, so their fixtures' `{ "type": "separator" }` entries draw - * real dividers today. The three renderers run two different dialects against - * one declared interface; that split is filed as objectui#6523 and is NOT this - * card's to rule. A family-wide sweep would either fail on fixtures this PR is fenced - * out of, or force the renderer change triage forbade. + * Both paragraphs immediately below are historical: they describe the corpus + * as it stood before objectui#6523 landed. `dropdown-menu.tsx` and + * `context-menu.tsx` now branch on the DECLARED `item.separator`, exactly + * like `menubar.tsx` always did, and `MenuItem` is a discriminated union (a + * command item with a required `label`, or `{ separator: true }` with none) — + * see `../../packages/types/src/overlay.ts`. The retired `{ type: 'separator' + * }` dialect is now a TOMBSTONE (`type?: never` / `z.never().optional()`), + * refused at parse time rather than silently stripped, on all three + * containers at once (they share the one `MenuItem` type). The two catalog + * fixtures that used to author it — + * `components-overlay-dropdown-menu/basic-dropdown-menu.json` and + * `components-overlay-context-menu/basic-context-menu.json` — were migrated + * to `{ "separator": true }` in the same change. * - * ## Why `.success` is not the probe, in BOTH directions + * ## Why this sweep is STILL scoped to `menubar` nodes, not widened to the family * - * `MenuItemSchema` is a bare `z.object` inside a `z.lazy`, so it strips an - * undeclared key and reports success — the class-2 blindness this file already - * records for `CommandItem`. And the declared separator spelling does not parse - * green either, because `MenuItem.label` is REQUIRED and a divider has no - * label. Both counter-probes below pin those facts, so this block cannot be - * "simplified" into a parse that would measure nothing. + * The renderer split that used to block a family-wide sweep is gone, but the + * sweep below stays `menubar`-only anyway: `collectMenubarItems` walks one + * node shape (`{ type: 'menubar', menus: [{ items }] }`), and widening it to + * also match `dropdown-menu`/`context-menu` nodes is a separate verification + * surface this fix does not need — the counter-probes further down already + * exercise the shared `MenuItemSchema` directly, which covers the retired + * dialect for all three containers without walking their corpora at all. The + * `dropdown-menu`/`context-menu` catalog fixtures ARE checked, just not by + * this walker: `context-menu-item-icon.test.tsx` renders + * `basic-context-menu.json`'s items directly, and the icon-resolution suites + * next to it exercise `basic-dropdown-menu.json`'s. + * + * (Historical, pre-#6523:) It would have been one line to sweep every + * `MenuItem`-shaped container. That would have been WRONG then: + * `dropdown-menu.tsx:46` and `context-menu.tsx:44` used to branch on + * `item.type === 'separator'` — the undeclared spelling — so their fixtures' + * `{ "type": "separator" }` entries drew real dividers on THOSE containers + * while failing the assertions below, which are `menubar`-shaped. A + * family-wide sweep would have either failed on fixtures outside that card's + * fence, or forced the renderer change objectui#6249's triage explicitly + * declined to make (that renderer change is what objectui#6523 later ruled). + * + * ## Why `.success` was not the probe before this fix, in BOTH directions — + * and is now the probe in both, because both directions changed + * + * (Historical, pre-#6523:) `MenuItemSchema` was a bare `z.object`, so it + * stripped an undeclared key and reported success — the class-2 blindness + * this file already records for `CommandItem`. And the declared separator + * spelling did not parse green either, because `MenuItem.label` was REQUIRED + * with no way to omit it for a divider. + * + * Both of those were the SAME defect (declared ≠ enforced) pointing opposite + * ways, and objectui#6523 corrected both ends at once: `MenuItem` became a + * union so the declared divider spelling parses green, and `type` became a + * declared `never` tombstone so the undeclared spelling is REFUSED rather + * than stripped. The counter-probe below now asserts `.success` in both + * directions on purpose — it is the fixed, not the "cannot be simplified", + * shape. What must not be re-simplified is the STRUCTURAL sweep above this + * block: it still walks the actual corpus, which a `.success` probe never + * substitutes for. */ describe('catalog corpus: every menubar item uses the declared MenuItem spellings (objectui#6249)', () => { type Located = { where: string; item: Json }; @@ -467,21 +508,34 @@ describe('catalog corpus: every menubar item uses the declared MenuItem spelling return acc; } - const declaredKeys = declaredKeysOf(MenuItemSchema, { + // Two calibration probes, not one combined object (objectui#6523): `MenuItem` + // is now a union of a command arm and a divider arm that DISAGREE on + // `separator` (`false | undefined` vs `true`), so a single object carrying + // both a `label` AND `separator: true` matches neither arm cleanly and is + // not a meaningful "declared keys" instrument any more. `type` is no longer + // part of this probe at all — see the dedicated refusal pins below, which + // is where it moved (a hard refusal, not a strip, can't be read back by + // `declaredKeysOf`'s "call `.parse` and see what survives" method, since a + // refusal never returns a value to read keys off). + const declaredCommandKeys = declaredKeysOf(MenuItemSchema, { label: 'probe', icon: 'file', disabled: false, shortcut: 'Ctrl+T', + value: 'probe', + }); + const declaredDividerKeys = declaredKeysOf(MenuItemSchema, { separator: true, - type: 'separator', value: 'probe', }); const items = allExamples().flatMap((e) => collectMenubarItems(e.schema, e.id)); - it('`separator` and `shortcut` survive the schema and `type` does not — the control', () => { - expect(declaredKeys).toContain('separator'); - expect(declaredKeys).toContain('shortcut'); - expect(declaredKeys).not.toContain('type'); + it('label/icon/disabled/shortcut survive on the command arm; the unknown `value` key does not — the control', () => { + expect(declaredCommandKeys.sort()).toEqual(['disabled', 'icon', 'label', 'shortcut'].sort()); + }); + + it('`separator` survives on the divider arm; the unknown `value` key does not — the control', () => { + expect(declaredDividerKeys).toEqual(['separator']); }); it('the sweep reaches the published demo and every one of its items — non-vacuity', () => { @@ -542,31 +596,66 @@ describe('catalog corpus: every menubar item uses the declared MenuItem spelling } }); + /** + * `MenuItem` being a UNION (objectui#6523) means a failed `.safeParse`'s + * top issue is `{ code: 'invalid_union', path: [], errors: [...] }` — one + * error array PER union member, not a single flat `path` any more. This + * reads the field name(s) named across every member's errors, so a + * counter-probe can still assert "the failure is about key X" without + * hard-coding which union member zod tried first. + */ + function unionFailurePathKeys(result: { success: boolean; error?: unknown }): string[] { + if (result.success) return []; + const issues = (result.error as { issues: Array> }).issues; + return issues.flatMap((issue) => { + const branches = issue.errors as Array> | undefined; + if (!branches) return [String((issue.path as unknown[])[0])]; + return branches.flatMap((branch) => branch.map((e) => String(e.path[0]))); + }); + } + it('counter-probe: the pre-#6249 array `shortcut` is REFUSED, so the sweep above bites', () => { const authored = { label: 'New Tab', shortcut: ['Ctrl', 'T'] }; const result = MenuItemSchema.safeParse(authored); expect(result.success).toBe(false); - expect(result.error?.issues[0]?.path).toEqual(['shortcut']); + expect(unionFailurePathKeys(result)).toContain('shortcut'); // ...and the corrected item, with the key gone, parses green. expect(MenuItemSchema.safeParse({ label: 'New Tab' }).success).toBe(true); }); - it('counter-probe: no parse can see the separator defect, in either direction', () => { - // The undeclared spelling is SILENTLY STRIPPED by the bare `z.object`, so a - // labelled item carrying it parses green and round-trips lossily — a - // `.success` probe is blind to exactly the defect this block exists to catch. + it('counter-probe: the separator defect is now visible to a plain `.safeParse` in BOTH directions (objectui#6523, fixed)', () => { + // Historical shape of this probe (pre-#6523): the undeclared `type` + // spelling was SILENTLY STRIPPED by the bare `z.object`, so a labelled + // item carrying it parsed green and round-tripped lossily — a `.success` + // probe was blind to exactly the defect this block exists to catch. That + // is why the STRUCTURAL sweep above this describe block exists at all: + // before this fix, no `.safeParse` call could have stood in for it. + // + // `type` is now a declared `z.never()` refusal, not an absence, so the + // same call fails outright instead of stripping and succeeding. const undeclared = MenuItemSchema.safeParse({ label: 'x', type: 'separator' }); - expect(undeclared.success).toBe(true); - expect(undeclared.data).toEqual({ label: 'x' }); - - // And the DECLARED spelling this PR writes does not parse green either: - // `MenuItem.label` is required and a divider has no label — the menubar - // renderer's own `defaultProps` write `{ separator: true }` with no label - // (`menubar.tsx:69`), so the shipped default does not satisfy the shipped - // type. That contract gap is objectui#6523; it is why the assertions above - // are structural rather than a `MenubarSchema.safeParse`. + expect(undeclared.success).toBe(false); + expect(unionFailurePathKeys(undeclared)).toContain('type'); + + // And the DECLARED spelling now parses green FOR THE FIRST TIME: + // `MenuItem` became a discriminated union (objectui#6523) specifically so + // a label-less divider is representable — before this fix, + // `MenuItem.label` was required with no way to omit it, so this EXACT + // value — the menubar renderer's own `defaultProps` divider + // (`menubar.tsx`) — failed a strict parse against the shipped type. The + // shipped default not satisfying the shipped type was the contract gap; + // this is the fix. const declared = MenuItemSchema.safeParse({ separator: true }); - expect(declared.success).toBe(false); - expect(declared.error?.issues[0]?.path).toEqual(['label']); + expect(declared.success).toBe(true); + expect(declared.success ? declared.data : undefined).toEqual({ separator: true }); + }); + + it('counter-probe: `type` refuses BOTH its retired values, not just the one this file had a fixture for', () => { + // `type` has no partial refusal — a `z.never()` tombstone cannot admit + // one string and reject another. `dropdown-menu`/`context-menu` also used + // to branch on `item.type === 'label'` (a section-heading spelling no + // fixture in this corpus ever authored), and retiring the declared + // divider's impostor necessarily retired this one too. + expect(MenuItemSchema.safeParse({ label: 'Section', type: 'label' }).success).toBe(false); }); }); diff --git a/packages/components/src/__tests__/context-menu-item-icon.test.tsx b/packages/components/src/__tests__/context-menu-item-icon.test.tsx index 6e078989ae..3571026447 100644 --- a/packages/components/src/__tests__/context-menu-item-icon.test.tsx +++ b/packages/components/src/__tests__/context-menu-item-icon.test.tsx @@ -169,7 +169,7 @@ describe('ui:context-menu item icon resolution (objectui#6278)', () => { { label: 'Copy', value: 'copy', icon: 'copy' }, { label: 'Cut', value: 'cut', icon: 'scissors' }, { label: 'Paste', value: 'paste', icon: 'clipboard' }, - { type: 'separator' }, + { separator: true }, { label: 'Delete', value: 'delete', icon: 'trash' }, ]); for (const [label, name] of [ diff --git a/packages/components/src/__tests__/menu-item-onclick-handler.test.tsx b/packages/components/src/__tests__/menu-item-onclick-handler.test.tsx new file mode 100644 index 0000000000..ed1d954db0 --- /dev/null +++ b/packages/components/src/__tests__/menu-item-onclick-handler.test.tsx @@ -0,0 +1,121 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * All three menu renderers fire the DECLARED `MenuItem.onClick` (objectui#6346, + * maintainer ruling 2026-08-27, "同意" on recommendation A): `dropdown-menu` + * and `context-menu` used to read an undeclared `item.onSelect` instead — an + * authored `onClick` validated, published, and never fired — and `menubar` + * wired no item handler at all, neither spelling. + * + * The measured-zero migration cost the ruling records (0 authored `onSelect` + * in-repo, positive controls firing) is what made this a renderer fix rather + * than a rename of the published type; it is not re-verified here, since that + * corpus measurement lives on the issue itself and would go stale silently + * if restated as a test assertion against a fixture population. + * + * `renderMenuItems`/`renderContextMenuItems` also TIGHTEN from `items: any[]` + * to `items: MenuItem[]` as part of this fix — the hole that let the + * undeclared `onSelect` (and `inset`) type-check in the first place. That is + * a compile-time property, pinned by the renderer files themselves compiling + * under `tsc` (`pnpm --filter @object-ui/components type-check`) — an + * `any`-typed regression would not show up as a runtime test failure here. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { ComponentRegistry } from '@object-ui/core'; +import '../renderers'; + +afterEach(() => cleanup()); + +describe('ui:dropdown-menu — fires the declared `onClick` (objectui#6346)', () => { + it('clicking a labelled item fires its authored `onClick`', async () => { + const onClick = vi.fn(); + const user = userEvent.setup(); + const C = ComponentRegistry.get('dropdown-menu') as React.ComponentType; + render( + , + ); + await user.click(screen.getByText('Save')); + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('an authored `onSelect` (the undeclared spelling) is never invoked', async () => { + // `onSelect` is not part of `MenuItem` at all — this authors it anyway to + // prove the renderer no longer reads it, mirroring how the pre-fix + // renderer ignored the DECLARED `onClick`. + const onSelect = vi.fn(); + const user = userEvent.setup(); + const C = ComponentRegistry.get('dropdown-menu') as React.ComponentType; + render( + , + ); + await user.click(screen.getByText('Save')); + expect(onSelect).not.toHaveBeenCalled(); + }); +}); + +describe('ui:context-menu — fires the declared `onClick` (objectui#6346)', () => { + function open(items: unknown[]) { + const C = ComponentRegistry.get('context-menu') as React.ComponentType; + const { container } = render( + , + ); + fireEvent.contextMenu(container.firstElementChild as HTMLElement); + } + + it('clicking a labelled item fires its authored `onClick`', async () => { + const onClick = vi.fn(); + const user = userEvent.setup(); + open([{ label: 'Copy', onClick }]); + await user.click(screen.getByText('Copy')); + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('an authored `onSelect` (the undeclared spelling) is never invoked', async () => { + const onSelect = vi.fn(); + const user = userEvent.setup(); + open([{ label: 'Copy', onSelect }]); + await user.click(screen.getByText('Copy')); + expect(onSelect).not.toHaveBeenCalled(); + }); +}); + +describe('ui:menubar — gains the wiring (objectui#6346 rider: menubar wired neither spelling before)', () => { + async function openFileMenu(items: unknown[]) { + const user = userEvent.setup(); + const C = ComponentRegistry.get('menubar') as React.ComponentType; + render(); + await user.click(screen.getByText('File')); + return user; + } + + it('clicking a labelled item fires its authored `onClick`', async () => { + const onClick = vi.fn(); + const user = await openFileMenu([{ label: 'New Tab', onClick }]); + await user.click(screen.getByText('New Tab')); + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('a submenu child item also fires its authored `onClick`', async () => { + const onClick = vi.fn(); + const user = await openFileMenu([ + { label: 'Recent', children: [{ label: 'report.csv', onClick }] }, + ]); + await user.click(screen.getByText('Recent')); + // `fireEvent.click`, not `userEvent.click`, on the nested item — Radix's + // synthetic pointer sequence for `userEvent` interacts with the Sub's + // hover-intent tracking across the trigger/content boundary in jsdom and + // never dispatches the `select`; a plain `click` event is what Radix's + // menu item listens for and is what a real click ultimately fires too. + fireEvent.click(await screen.findByText('report.csv')); + expect(onClick).toHaveBeenCalledTimes(1); + }); +}); diff --git a/packages/components/src/__tests__/menu-item-separator-dialect.test.tsx b/packages/components/src/__tests__/menu-item-separator-dialect.test.tsx new file mode 100644 index 0000000000..b5505e9a75 --- /dev/null +++ b/packages/components/src/__tests__/menu-item-separator-dialect.test.tsx @@ -0,0 +1,132 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `MenuItem`'s declared divider spelling (objectui#6523, maintainer ruling + * 2026-08-27): `dropdown-menu` and `context-menu` now branch on the declared + * `item.separator`, matching `menubar` — which already read it correctly and + * is what the ruling's issue used as evidence the declaration, not the + * renderer, was right. + * + * ## The blank-row regression this pins (ruling-required) + * + * Before this fix, an author who followed the SHIPPED TYPE and wrote the + * declared `{ separator: true }` in a dropdown or context menu got a value + * that validated, published, and rendered a BLANK MENU ROW: both renderers + * branched on an undeclared `item.type === 'separator'` instead, so + * `{ separator: true }` fell through to the ordinary item arm with no + * `label` to draw. That is the exact defect objectui#6249 hit and fixed for + * ONE fixture (menubar); this card is the renderer-level fix for the other + * two containers. `'the declared spelling now draws a real divider, not a + * blank row'` below is the regression pin — it is RED against the pre-fix + * `item.type === 'separator'` branch (verified by reverting the renderer + * change and re-running this file; see the PR body's ablation section). + * + * ## Why item COUNT is asserted, not just divider presence + * + * A renderer that draws a divider AND still emits a stray blank `menuitem` + * for the same entry would pass a "does a separator exist" check while + * shipping half of the regression. Asserting the exact `menuitem` count (no + * more, no fewer than the labelled items) closes that gap. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { ComponentRegistry } from '@object-ui/core'; +// Registers the renderers at module scope, NOT inside a `beforeAll` — there +// the cold transform is billed to `hookTimeout`. See +// object-ui/no-dynamic-import-in-test-hook (objectui#3010/#3021). +import '../renderers'; + +afterEach(() => cleanup()); + +/** `defaultOpen` is load-bearing — Radix mounts `DropdownMenuContent` lazily. */ +function renderDropdown(items: unknown[]) { + const C = ComponentRegistry.get('dropdown-menu') as React.ComponentType; + return render(); +} + +/** Context menu content mounts only after a `contextmenu` event on the trigger. */ +function renderContextMenu(items: unknown[]) { + const C = ComponentRegistry.get('context-menu') as React.ComponentType; + const { container } = render( + , + ); + fireEvent.contextMenu(container.firstElementChild as HTMLElement); + return container; +} + +describe('ui:dropdown-menu — the declared divider spelling (objectui#6523)', () => { + it('`{ separator: true }` renders a real divider row', () => { + renderDropdown([{ label: 'Profile' }, { separator: true }, { label: 'Logout' }]); + expect(screen.getAllByRole('separator')).toHaveLength(1); + }); + + it('the declared spelling draws no blank row alongside the divider — the regression pin', () => { + renderDropdown([{ label: 'Profile' }, { separator: true }, { label: 'Logout' }]); + // Exactly two labelled command items — not three, which is what the + // pre-fix renderer produced (the divider entry fell through to a blank + // `DropdownMenuItem` with no label). + const items = screen.getAllByRole('menuitem'); + expect(items).toHaveLength(2); + expect(items.map((el) => el.textContent)).toEqual(['Profile', 'Logout']); + }); + + it('the retired `{ type: "separator" }` spelling no longer draws a divider', () => { + // `type` is a declared refusal at the schema level (objectui#6523's zod + // pin, `menu-item-union.test.ts`); this is the RENDERER half — even a + // hand-built prop bypassing validation gets no special treatment, since + // the renderer no longer reads `item.type` at all. + renderDropdown([{ label: 'Profile' }, { type: 'separator' }, { label: 'Logout' }]); + expect(screen.queryAllByRole('separator')).toHaveLength(0); + }); +}); + +describe('ui:context-menu — the declared divider spelling (objectui#6523)', () => { + it('`{ separator: true }` renders a real divider row', () => { + renderContextMenu([{ label: 'Copy' }, { separator: true }, { label: 'Delete' }]); + expect(screen.getAllByRole('separator')).toHaveLength(1); + }); + + it('the declared spelling draws no blank row alongside the divider — the regression pin', () => { + renderContextMenu([{ label: 'Copy' }, { separator: true }, { label: 'Delete' }]); + const items = screen.getAllByRole('menuitem'); + expect(items).toHaveLength(2); + expect(items.map((el) => el.textContent)).toEqual(['Copy', 'Delete']); + }); + + it('the retired `{ type: "separator" }` spelling no longer draws a divider', () => { + renderContextMenu([{ label: 'Copy' }, { type: 'separator' }, { label: 'Delete' }]); + expect(screen.queryAllByRole('separator')).toHaveLength(0); + }); +}); + +describe('ui:menubar — `shortcut` rendering, parity not new capability (objectui#6523 rider)', () => { + /** Opens the first menu the way a user would — clicking its trigger. Radix + * Menubar opens on a full pointer sequence, not a bare `fireEvent.click`, + * so this drives it through `userEvent`. */ + async function renderMenubar(items: unknown[]) { + const user = userEvent.setup(); + const C = ComponentRegistry.get('menubar') as React.ComponentType; + render( + , + ); + await user.click(screen.getByText('File')); + } + + it('renders the declared `shortcut` string beside its item', async () => { + await renderMenubar([{ label: 'New Tab', shortcut: 'Ctrl+T' }]); + expect(screen.getByText('Ctrl+T')).toBeTruthy(); + }); + + it('an item without `shortcut` draws no shortcut text', async () => { + await renderMenubar([{ label: 'New Tab' }]); + expect(screen.queryByText('Ctrl+T')).toBeNull(); + }); +}); diff --git a/packages/components/src/renderers/overlay/context-menu.tsx b/packages/components/src/renderers/overlay/context-menu.tsx index 7aa466e63d..83e2f7084d 100644 --- a/packages/components/src/renderers/overlay/context-menu.tsx +++ b/packages/components/src/renderers/overlay/context-menu.tsx @@ -7,13 +7,12 @@ */ import { ComponentRegistry } from '@object-ui/core'; -import type { ContextMenuSchema } from '@object-ui/types'; +import type { ContextMenuSchema, MenuItem } from '@object-ui/types'; import { ContextMenu, ContextMenuTrigger, ContextMenuContent, - ContextMenuItem, - ContextMenuLabel, + ContextMenuItem, ContextMenuSeparator, ContextMenuSub, ContextMenuSubTrigger, @@ -37,12 +36,19 @@ import { renderChildren } from '../../lib/utils'; // re-affirmed for this exact shape by objectui#5930. import { resolveIcon } from '../action/resolve-icon'; -// Reuse helper for recursive menu items if I could share it, but for now duplicate concise logic -const renderContextMenuItems = (items: any[]) => { +// Reuse helper for recursive menu items if I could share it, but for now +// duplicate concise logic. `items` is the DECLARED `MenuItem[]` (objectui#6346 +// tightened this from `any[]`, which is what let a renderer that read an +// undeclared spelling type-check in the first place). +const renderContextMenuItems = (items: MenuItem[] | undefined) => { if (!items) return null; - return items.map((item: any, i: number) => { - if (item.type === 'separator') return ; - if (item.type === 'label') return {item.label}; + return items.map((item, i) => { + // The declared divider spelling (objectui#6523) — `context-menu` used to + // branch on an undeclared `item.type === 'separator'` instead, which is + // now a tombstoned key on `MenuItem` (`type?: never`) rather than a + // second accepted dialect. `item.separator` narrows `item` to the + // command arm for the remainder of this iteration. + if (item.separator) return ; // Resolved once per item and read by BOTH arms below. The submenu-trigger // arm carries the identical defect; repairing only the leaf would be a // narrower version of the same bug (objectui#5930, objectui#6278). @@ -50,7 +56,7 @@ const renderContextMenuItems = (items: any[]) => { if (item.children) { return ( - + {Icon && } {item.label} @@ -60,9 +66,13 @@ const renderContextMenuItems = (items: any[]) => { ) } - + return ( - + // `onSelect` is Radix's callback prop name on `ContextMenuItem`; it + // fires the DECLARED `item.onClick` (objectui#6346 — this renderer used + // to read an undeclared `item.onSelect` on the schema item instead, so + // an authored `onClick` validated, published, and never fired). + item.onClick?.()}> {Icon && } {item.label} {item.shortcut && {item.shortcut}} @@ -100,11 +110,11 @@ ComponentRegistry.register('context-menu', label: 'Trigger Area', }, { name: 'triggerClassName', type: 'string', label: 'Trigger Area Class' }, - { - name: 'items', - type: 'array', + { + name: 'items', + type: 'array', label: 'Items', - description: 'Recursive structure: { type?: "separator"|"label", label, icon, shortcut, children }. `icon` is a kebab-case Lucide icon name resolved against lucide\'s runtime `icons` record; an unknown or retired spelling renders no glyph.' + description: 'Recursive structure: a command item { label, icon, shortcut, disabled, onClick, children } or a divider { separator: true }. `icon` is a kebab-case Lucide icon name resolved against lucide\'s runtime `icons` record; an unknown or retired spelling renders no glyph.' }, { name: 'className', type: 'string', label: 'Content CSS Class' } ], @@ -112,7 +122,7 @@ ComponentRegistry.register('context-menu', items: [ { label: 'Action 1' }, { label: 'Action 2' }, - { type: 'separator' }, + { separator: true }, { label: 'Action 3' } ], trigger: [{ type: 'text', content: 'Right click here' }] diff --git a/packages/components/src/renderers/overlay/dropdown-menu.tsx b/packages/components/src/renderers/overlay/dropdown-menu.tsx index f8f3b942d2..d80a856f60 100644 --- a/packages/components/src/renderers/overlay/dropdown-menu.tsx +++ b/packages/components/src/renderers/overlay/dropdown-menu.tsx @@ -7,7 +7,7 @@ */ import { ComponentRegistry } from '@object-ui/core'; -import type { DropdownMenuSchema } from '@object-ui/types'; +import type { DropdownMenuSchema, MenuItem } from '@object-ui/types'; import { useDisplayLocale } from '@object-ui/i18n'; // Aliased on import, following PR #4169's convention: this repo has its OWN // `resolveKeyedI18nLabel` over a DIFFERENT vocabulary, and neither resolver @@ -39,12 +39,18 @@ import { renderChildren } from '../../lib/utils'; // as ruled out for authored icon fields by objectui#5622 and #5633. import { resolveIcon } from '../action/resolve-icon'; -// Helper for recursive menu items -const renderMenuItems = (items: any[]) => { +// Helper for recursive menu items. `items` is the DECLARED `MenuItem[]` +// (objectui#6346 tightened this from `any[]`, which is what let a renderer +// that read an undeclared spelling type-check in the first place). +const renderMenuItems = (items: MenuItem[] | undefined) => { if (!items) return null; - return items.map((item: any, i: number) => { - if (item.type === 'separator') return ; - if (item.type === 'label') return {item.label}; + return items.map((item, i) => { + // The declared divider spelling (objectui#6523) — `dropdown-menu` used to + // branch on an undeclared `item.type === 'separator'` instead, which is + // now a tombstoned key on `MenuItem` (`type?: never`) rather than a + // second accepted dialect. `item.separator` narrows `item` to the + // command arm for the remainder of this iteration. + if (item.separator) return ; // Resolved once per item and read by BOTH arms below. The submenu-trigger // arm carried the identical defect; repairing only the leaf would be a // narrower version of the same bug (objectui#5930). @@ -52,7 +58,7 @@ const renderMenuItems = (items: any[]) => { if (item.children) { return ( - + {Icon && } {item.label} @@ -62,9 +68,13 @@ const renderMenuItems = (items: any[]) => { ) } - + return ( - + // `onSelect` is Radix's callback prop name on `DropdownMenuItem`; it + // fires the DECLARED `item.onClick` (objectui#6346 — this renderer used + // to read an undeclared `item.onSelect` on the schema item instead, so + // an authored `onClick` validated, published, and never fired). + item.onClick?.()}> {Icon && } {item.label} {item.shortcut && {item.shortcut}} @@ -108,11 +118,11 @@ ComponentRegistry.register('dropdown-menu', type: 'slot', label: 'Trigger' }, - { - name: 'items', - type: 'array', + { + name: 'items', + type: 'array', label: 'Items', - description: 'Recursive structure: { type?: "separator"|"label", label, icon, shortcut, disabled, children: [] }. `icon` is a kebab-case Lucide icon name resolved against lucide\'s runtime `icons` record; an unknown or retired spelling renders no glyph.' + description: 'Recursive structure: a command item { label, icon, shortcut, disabled, onClick, children: [] } or a divider { separator: true }. `icon` is a kebab-case Lucide icon name resolved against lucide\'s runtime `icons` record; an unknown or retired spelling renders no glyph.' }, { name: 'className', type: 'string', label: 'Content CSS Class' } ], @@ -121,7 +131,7 @@ ComponentRegistry.register('dropdown-menu', items: [ { label: 'Item 1' }, { label: 'Item 2' }, - { type: 'separator' }, + { separator: true }, { label: 'Item 3' } ], align: 'start', diff --git a/packages/components/src/renderers/overlay/menubar.tsx b/packages/components/src/renderers/overlay/menubar.tsx index fb80acd410..ef50dbf74d 100644 --- a/packages/components/src/renderers/overlay/menubar.tsx +++ b/packages/components/src/renderers/overlay/menubar.tsx @@ -8,7 +8,7 @@ import { ComponentRegistry } from '@object-ui/core'; import type { MenubarSchema } from '@object-ui/types'; -import { Menubar, MenubarMenu, MenubarTrigger, MenubarContent, MenubarItem, MenubarSeparator, MenubarSub, MenubarSubTrigger, MenubarSubContent } from '../../ui/menubar'; +import { Menubar, MenubarMenu, MenubarTrigger, MenubarContent, MenubarItem, MenubarSeparator, MenubarSub, MenubarSubTrigger, MenubarSubContent, MenubarShortcut } from '../../ui/menubar'; ComponentRegistry.register('menubar', ({ schema, ...props }: { schema: MenubarSchema; [key: string]: any }) => { @@ -36,14 +36,42 @@ ComponentRegistry.register('menubar', {item.label} - {item.children.map((child, childIdx) => ( - {child.label} - ))} + {item.children.map((child, childIdx) => + // A submenu child is itself a `MenuItem` — the same + // union as the top-level item, so it can be a divider + // too (objectui#6523); narrowing on `child.separator` + // is what makes `child.label` below type-check. + child.separator ? ( + + ) : ( + child.onClick?.()} + > + {child.label} + {child.shortcut && {child.shortcut}} + + ) + )} ) : ( - + item.onClick?.()} + > {item.label} + {/* Parity, not new capability (objectui#6523 rider): the + declared `shortcut` string already has working + runtime in dropdown-menu and context-menu; menubar + read it nowhere. */} + {item.shortcut && {item.shortcut}} ) ))} diff --git a/packages/types/src/__tests__/menu-item-union.test.ts b/packages/types/src/__tests__/menu-item-union.test.ts new file mode 100644 index 0000000000..a5c6eb6538 --- /dev/null +++ b/packages/types/src/__tests__/menu-item-union.test.ts @@ -0,0 +1,185 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * `MenuItem` is a discriminated union (objectui#6523, maintainer ruling + * 2026-08-27, "同意" on the triage A+B recommendation): a command item + * (`label` required) or a divider (`separator: true`, no label). B — the + * union — is the ruling's PRECONDITION for A (the renderer fix landing in + * `packages/components`): a divider had to become representable before a + * renderer could be taught to read it. `label` stays REQUIRED on the command + * arm rather than becoming optional, which the ruling rejected because it + * would weaken every command item's label protection to solve a problem only + * the divider arm has. + * + * Both arms tombstone `type` (`?: never` on the TS side, `z.never().optional()` + * on the zod mirror in `../zod/overlay.zod.ts`) — the undeclared key + * `dropdown-menu.tsx`/`context-menu.tsx` used to branch on instead of the + * declared `separator` boolean (`'separator'` for a divider, `'label'` for a + * section heading). Retiring it needed a declared REFUSAL, not an absence: + * `MenuItemSchema` is a bare (non-strict) `z.object`, so an undeclared key is + * silently STRIPPED and the parse still reports success — which is why no + * gate ever caught either renderer reading `type`. `z.never()` makes + * authoring it fail loudly instead. + * + * ## Why the `@ts-expect-error` pins below route through a NAMED, non-fresh value + * + * A FRESH object literal assigned to a typed variable is checked twice by + * `tsc`: excess-property checking (is every key on the literal declared + * SOMEWHERE on the target type?) and ordinary assignability (does each + * declared key's value match its declared type?). `type` IS declared here — + * as `never` — so a fresh literal's rejection would not, by itself, prove the + * `never` tombstone is doing the work: deleting the `type` declaration + * entirely would produce the SAME visible failure (a different error code, + * TS2353 "object literal may only specify known properties", on the exact + * same literal). Assigning a NAMED variable of a structurally wider, + * independently-declared shape sidesteps excess-property checking (it only + * applies to fresh literals), so the failure below can only be the `never` + * assignability check on a key both shapes agree exists — proof the + * tombstone, not literal syntax, is what refuses it. + */ + +import { describe, it, expect } from 'vitest'; +import type { MenuItem, MenuCommandItem, MenuDividerItem } from '../overlay'; +import { MenuItemSchema } from '../zod/overlay.zod'; + +/* ── type-level pins ─────────────────────────────────────────────────────── */ + +type Equal = + (() => T extends A ? 1 : 2) extends (() => T extends B ? 1 : 2) ? true : false; +type Expect = T; + +// The divider arm declares exactly `separator` and the `type` tombstone — no +// command-only field (label, icon, onClick, shortcut, children, disabled) +// leaks in. If a future edit adds a field to `MenuDividerItem` without a +// deliberate decision, this line goes red rather than silently widening the +// "no label" arm the ruling specified. +type _DividerKeySet = Expect>; + +/* ── the union itself ────────────────────────────────────────────────────── */ + +describe('MenuItem — discriminated union (objectui#6523)', () => { + it('a command item requires `label`', () => { + const item: MenuCommandItem = { label: 'New Tab' }; + expect(item.label).toBe('New Tab'); + }); + + it('a divider is `{ separator: true }` alone — no label required', () => { + // This line is the union's whole point: before objectui#6523, + // `MenuItem.label` was required UNCONDITIONALLY (a single object shape), + // so a label-less divider did not compile against it at all — exactly + // the gap the ruling's "B is the precondition" step closed. + const divider: MenuDividerItem = { separator: true }; + expect(divider.separator).toBe(true); + }); + + it("a divider has no `label` field to write — the command arm's protection survives", () => { + const divider: MenuDividerItem = { + separator: true, + // @ts-expect-error a divider has no `label` — not an optional field + // relaxed for convenience, it does not exist on this arm at all. This + // is the "label-optional would weaken every command item" cost the + // ruling named and chose the union specifically to avoid paying. + label: 'not allowed', + }; + expect(divider.separator).toBe(true); + }); + + describe('`type` is a declared refusal, not an absence (both arms)', () => { + // Independently declared — NOT `MenuCommandItem & { type?: string }` — + // because intersecting with a type that already tombstones `type` as + // `never` would just collapse back to `never`. This shape shares the + // discriminant key with the union but keeps `type` genuinely widened, so + // the assignment below is a real test of the tombstone. See the file + // header for why it must also be a NAMED value, not a fresh literal. + interface LegacyCommandShape { + label: string; + type?: string; + } + interface LegacyDividerShape { + separator: true; + type?: string; + } + + it('refuses `type` on the command arm', () => { + const legacy: LegacyCommandShape = { label: 'New Tab', type: 'separator' }; + // @ts-expect-error `type` is TOMBSTONED (`?: never`) on `MenuCommandItem` + // (objectui#6523) — `dropdown-menu`/`context-menu` used to read this + // undeclared key instead of the declared `separator` boolean. + const item: MenuItem = legacy; + // Asserted off `legacy`, not the (union-typed) `item`: the point of + // this test is that the ASSIGNMENT above is refused, and `item`'s + // static type stays `MenuItem` regardless — reading a member-specific + // field off it would need its own narrowing, which is not what this + // test is pinning. + expect(legacy.label).toBe('New Tab'); + void item; + }); + + it('refuses `type` on the divider arm', () => { + const legacy: LegacyDividerShape = { separator: true, type: 'separator' }; + // @ts-expect-error same tombstone, the divider arm (objectui#6523). + const item: MenuItem = legacy; + expect(legacy.separator).toBe(true); + void item; + }); + }); +}); + +/* ── the zod mirror agrees ───────────────────────────────────────────────── */ + +describe('MenuItemSchema — the zod mirror agrees with the TS union (objectui#6523)', () => { + it('the declared divider spelling parses green — for the first time', () => { + // Before objectui#6523, `MenuItem.label` was required with no way to + // express a label-less divider, so this EXACT value — the menubar + // renderer's own `defaultProps` divider (`menubar.tsx`'s `{ separator: + // true }` entry) — failed a strict parse against the shipped type. The + // shipped default did not satisfy the shipped type; this is the fix. + const result = MenuItemSchema.safeParse({ separator: true }); + expect(result.success).toBe(true); + expect(result.data).toEqual({ separator: true }); + }); + + it('a bare object still fails — a command item requires `label`', () => { + const result = MenuItemSchema.safeParse({}); + expect(result.success).toBe(false); + }); + + it('`type: "separator"` is REFUSED, not silently stripped — the retired dialect', () => { + // Before objectui#6523 this call SUCCEEDED and silently dropped `type` + // from the parsed result — the "class-2 blindness" the card measured, + // and the reason no gate ever caught `dropdown-menu`/`context-menu` + // reading an undeclared key that the type never protected. + const result = MenuItemSchema.safeParse({ label: 'New Tab', type: 'separator' }); + expect(result.success).toBe(false); + }); + + it('`type: "label"` — the renderers\' OTHER undeclared spelling — is refused the same way', () => { + // `type` has no partial refusal: a `z.never()` tombstone cannot admit one + // string value and reject another, so retiring the declared spelling's + // impostor necessarily retires this one too (dropdown-menu/context-menu + // also branched on `item.type === 'label'`, equally undeclared). + const result = MenuItemSchema.safeParse({ label: 'Section', type: 'label' }); + expect(result.success).toBe(false); + }); + + it('the divider arm refuses `type` too', () => { + const result = MenuItemSchema.safeParse({ separator: true, type: 'separator' }); + expect(result.success).toBe(false); + }); + + it('a live command item — label, icon, shortcut, onClick — still parses green', () => { + const result = MenuItemSchema.safeParse({ + label: 'New Tab', + icon: 'plus', + shortcut: 'Ctrl+T', + onClick: () => {}, + }); + expect(result.success).toBe(true); + }); +}); diff --git a/packages/types/src/index.ts b/packages/types/src/index.ts index f4086c39c2..ab09c60291 100644 --- a/packages/types/src/index.ts +++ b/packages/types/src/index.ts @@ -253,6 +253,8 @@ export type { TooltipSchema, HoverCardSchema, MenuItem, + MenuCommandItem, + MenuDividerItem, MenubarSchema, DropdownMenuSchema, ContextMenuSchema, diff --git a/packages/types/src/overlay.ts b/packages/types/src/overlay.ts index 6b061768c2..9bc5738033 100644 --- a/packages/types/src/overlay.ts +++ b/packages/types/src/overlay.ts @@ -325,9 +325,31 @@ export interface HoverCardSchema extends BaseSchema { } /** - * Menu item + * Menu item — a clickable command, or a divider between groups of commands. + * + * A discriminated union (objectui#6523): a divider has no label and a command + * item always has one, so a single object with an optional `label` cannot + * express "this is a divider" without leaving the divider arm unrepresentable + * — the shipped renderers' own `defaultProps` for a divider + * (`{ separator: true }`, no `label`) failed a strict parse against the old + * shape. `label` stays REQUIRED on the command arm rather than becoming + * optional, which would have weakened every command item's label protection + * to solve a problem only the divider arm has. + * + * `type` is TOMBSTONED (`?: never`) on both arms. It is not merely + * undeclared: `dropdown-menu`/`context-menu` used to branch on an undeclared + * `item.type === 'separator'` (and `=== 'label'`) instead of the declared + * `separator` key, and a bare (non-strict) zod object silently stripped that + * key on parse, so no gate ever caught the two renderers reading a spelling + * the type never declared. Declaring `type?: never` makes authoring it a + * refusal at parse/type-check time instead of a silent no-op (ADR-0049). + */ +export type MenuItem = MenuCommandItem | MenuDividerItem; + +/** + * The command arm of {@link MenuItem} — a clickable, labelled entry. */ -export interface MenuItem { +export interface MenuCommandItem { /** * Menu item label */ @@ -353,9 +375,36 @@ export interface MenuItem { */ children?: MenuItem[]; /** - * Separator (renders as divider) + * Not a divider — present (typed `false`) only so the union can discriminate + * on this key without every command item needing to omit it. + */ + separator?: false; + /** + * RETIRED (objectui#6523) — dividers are spelled `{ separator: true }`. + * `dropdown-menu`/`context-menu` used to read an undeclared `type` key + * instead (`'separator'` for a divider, `'label'` for a section heading); + * neither spelling is part of the contract, so authoring `type` is refused + * rather than silently stripped. + */ + type?: never; +} + +/** + * The divider arm of {@link MenuItem} — renders as a separator between + * groups of commands. Deliberately carries no label and no other command + * fields: a divider is not a command with blank details, it is a different + * kind of row. + */ +export interface MenuDividerItem { + /** + * Renders as a divider (see {@link MenuItem}'s doc comment for why this is + * a separate arm rather than an optional flag on a single shape). + */ + separator: true; + /** + * RETIRED (objectui#6523) — see {@link MenuCommandItem.type}. */ - separator?: boolean; + type?: never; } /** diff --git a/packages/types/src/zod/overlay.zod.ts b/packages/types/src/zod/overlay.zod.ts index a8125bbfe8..16cc875656 100644 --- a/packages/types/src/zod/overlay.zod.ts +++ b/packages/types/src/zod/overlay.zod.ts @@ -126,18 +126,33 @@ export const HoverCardSchema = BaseSchema.extend({ }); /** - * Menu Item Schema + * Menu Item Schema — a discriminated union (objectui#6523): a command item + * (label required) or a divider (`separator: true`, no label). Mirrors the + * TS union `MenuItem = MenuCommandItem | MenuDividerItem` in `../overlay.ts`; + * see that file's doc comment for why this is a union rather than an + * optional `label`, and why `type` is tombstoned on both arms. */ export const MenuItemSchema: z.ZodType = z.lazy(() => - z.object({ - label: z.string().describe('Menu item label'), - icon: z.string().optional().describe('Menu item icon'), - disabled: z.boolean().optional().describe('Whether item is disabled'), - onClick: z.function().optional().describe('Click handler'), - shortcut: z.string().optional().describe('Keyboard shortcut'), - children: z.array(MenuItemSchema).optional().describe('Submenu items'), - separator: z.boolean().optional().describe('Whether this is a separator'), - }) + z.union([ + z.object({ + label: z.string().describe('Menu item label'), + icon: z.string().optional().describe('Menu item icon'), + disabled: z.boolean().optional().describe('Whether item is disabled'), + onClick: z.function().optional().describe('Click handler'), + shortcut: z.string().optional().describe('Keyboard shortcut'), + children: z.array(MenuItemSchema).optional().describe('Submenu items'), + separator: z.literal(false).optional().describe('Not a divider'), + type: z.never().optional().describe( + 'RETIRED (objectui#6523) — dividers are `{ separator: true }`; ' + + '`type` (\'separator\' or \'label\') was an undeclared spelling two ' + + 'renderers used to read and is now a declared refusal, not a strip.' + ), + }), + z.object({ + separator: z.literal(true).describe('Renders as a divider between items — no label'), + type: z.never().optional().describe('RETIRED (objectui#6523) — see the command-item arm above.'), + }), + ]) ); /**