fix(overlay): MenuItem discriminated union + declared onClick across dropdown-menu/context-menu/menubar - #6693
Merged
Conversation
…renderers read the declared onClick
MenuItem (dropdown-menu/context-menu/menubar) is now MenuCommandItem | MenuDividerItem:
a command item with a required label, or `{ separator: true }` with none. `type` is
tombstoned (`?: never` / `z.never().optional()`) on both arms, retiring the undeclared
`{ type: 'separator' }` / `{ type: 'label' }` dialect dropdown-menu and context-menu used
to read instead of the declared `separator` boolean.
dropdown-menu and context-menu now branch on `item.separator` (matching menubar, which
always had this right) instead of the undeclared `item.type`; their registry defaultProps
and description strings stop teaching the retired spelling. All three renderers now fire
the declared `item.onClick` — dropdown-menu/context-menu used to read an undeclared
`item.onSelect` instead, and menubar wired no handler at all.
`renderMenuItems`/`renderContextMenuItems` tighten from `items: any[]` to `MenuItem[]`,
closing the hole that let the mismatch type-check. menubar also gains `shortcut` rendering
(parity with the other two containers, not new capability).
Fixes #6523
Fixes #6346
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_8ca04858-ea8e-5b85-9182-de59aa49e00c
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6523
Fixes #6346
Summary
MenuItem(shared byui:dropdown-menu,ui:context-menu,ui:menubar) is now adiscriminated union, and all three menu renderers read exactly the keys it declares — no
more, no less. This lands both cards' rulings together (maintainer ruling 2026-08-27, "one
answer for the whole
MenuItemfamily"), each keeping its ownFixesline and pins below.#6523 — B (the union) then A (the renderer fix) then the rider
B —
MenuItembecomesMenuCommandItem | MenuDividerItem(packages/types/src/overlay.ts,mirrored in
packages/types/src/zod/overlay.zod.ts). A command item keepslabel: stringrequired; a divider is
{ separator: true }alone. The union, notlabel?: string: it iswhat the data actually is, and it keeps every command item's label protection intact rather
than weakening it repo-wide to accommodate the divider arm. Both arms tombstone
type(
type?: never/z.never().optional()) — the retired{ type: 'separator' }(and itssibling
{ type: 'label' }, the same key) is now a declared refusal, not a silent strip.Before/after, measured directly:
A —
dropdown-menuandcontext-menubranch on the declareditem.separator, matchingmenubar(which had this right all along — the evidence the type was correct). Theirregistry
defaultPropsanddescriptionstrings stop teaching{ type: 'separator' }.Re-derived count: 4 occurrences of
{ type: 'separator' }in this repo, now migrated:examples/schema-catalog/src/schemas/components-overlay-dropdown-menu/basic-dropdown-menu.jsonexamples/schema-catalog/src/schemas/components-overlay-context-menu/basic-context-menu.jsonpackages/components/src/renderers/overlay/dropdown-menu.tsx(registrydefaultProps)packages/components/src/renderers/overlay/context-menu.tsx(registrydefaultProps)(2 schema-catalog fixtures + 2 registry
defaultProps= 4, matching the ruling's count. Noother
dropdown-menu/context-menucatalog fixture authors it — only 3 files in the wholecatalog use either
typeat all, and only these 2 carry a separator.) A matching test-fixtureliteral in
context-menu-item-icon.test.tsx(mirroringbasic-context-menu.json) is migratedtoo.
⛔ Stop condition checked, not triggered. No
{ type: 'separator' }(ortype: 'label')exists anywhere outside this repo that this session can measure — this repo's own corpus is
the only evidence available, and it is fully migrated. C (retiring
separatorin favor oftype) is not the reconsideration here.Counter-probe flipped, as the ruling asked.
examples/schema-catalog/test/component-fixture-declared-keys.test.ts's"no parse can see the separator defect, in either direction" probe used to assert
.successwas blind (stripped
type, rejected the declared divider). It now asserts the fixedbehaviour in both directions —
.safeParseis no longer blind, and the file's headerexplains why the structural, corpus-walking sweep above it is still the right instrument for
what a
.successprobe can never replace (auditing what a fixture actually AUTHORS, not whata hand-built probe object parses to).
Rider —
menubarrendersshortcut(parity, not new capability). The declaredstringkey already had working runtime in the other two containers; menubar read it nowhere. Aligns
the third container.
#6346 — the item handler moves to the declared key
All three renderers now fire
item.onClick— declared in TS source, the built.d.ts, andthe Zod mirror all along, but never read.
dropdown-menu/context-menuused to read anundeclared
item.onSelectinstead (Radix's own callback prop name, now wired to invokeitem.onClick);menubarwired no handler at all, neither spelling.renderMenuItems/renderContextMenuItemstighten fromitems: any[]toitems: MenuItem[]— the wideningthat let the mismatch type-check in the first place.
Migration cost: measured zero in this repo — no fixture, doc or test authored
onSelecton a menu item before this change (functions aren't JSON-authorable in the firstplace, so this is a pure renderer/test-literal concern, not a catalog-migration one).
Consequential cleanup (forced by the
any[]→MenuItem[]tightening, not a separate decision)item.type === 'label'(a third, equally undeclared dialect on the sametypekey,never authored anywhere in this repo's docs or fixtures) is removed from both renderers —
once
typeis a declarednevertombstone,item.type === 'label'no longer type-checks(
undefinedhas no overlap with the string literal'label'), so the branch became deadcode the moment
typeretired.DropdownMenuLabel/ContextMenuLabel's section-headingaffordance is unaffected — those components are still used for
schema.labelon the wholemenu, just not per-item any more.
item.inset(an undeclared key, never taught in any doc or fixture, never authored) isdropped from both renderers'
DropdownMenuItem/DropdownMenuSubTriggerandContextMenuItem/ContextMenuSubTriggerprops — it doesn't exist onMenuItemand had nolive author anywhere to preserve.
Docs
content/docs/components/overlay/{dropdown-menu,context-menu,menubar}.mdx's## Schemablocks are corrected for exactly what this PR changes: the
type?: 'separator'line becomesthe union shape,
onClickis added to the command item (menubar's array-valuedshortcut?: string[]is corrected to the declaredstring), and the bogus schema-levelonSelect?: string | ActionConfigline (never a real field on any of the three schemas, and directlyconfusable with the #6346 finding) is removed. Pre-existing inaccuracies these blocks also
carry (
value/variantfieldsMenuItemnever declared,MenubarSchema.menustaught asrequired when it's optional) are left untouched — that is open issue #6521's scope
(21
valueoccurrences across 3 fixture families), not this PR's; #6521 itself says to"align with whichever way #6523 is ruled," which this PR's
menubar.mdxedit now is.Tests (all new, all pass)
packages/types/src/__tests__/menu-item-union.test.ts— the union and itstypetombstoneat both the TS level (
@ts-expect-errorpins routed through a named, non-fresh value sothe failure is provably the
neverassignability check and not excess-property checking —see the file header) and the zod level (
MenuItemSchema.safeParsepins for both directions).packages/components/src/__tests__/menu-item-separator-dialect.test.tsx— the dividerrenders in both
dropdown-menuandcontext-menu(the ruling's requiredabsent-on-declared-spelling / blank-row-regression pins), the retired spelling no longer
draws anything, and menubar's
shortcutrider.packages/components/src/__tests__/menu-item-onclick-handler.test.tsx—onClickfires inall three containers (including a menubar submenu child), and the undeclared
onSelectnolonger does.
examples/schema-catalog/test/component-fixture-declared-keys.test.ts— flipped counter-probe(above) plus a recalibrated "declared keys survive the schema" control (split into a
command-arm probe and a divider-arm probe, since one combined object can no longer match a
union with two mutually-exclusive
separatorvalues).Ablation (each leg predicted before it ran, mutation and restoration both proven on disk)
Leg A — revert only the divider-dialect fix (
item.separatorbranch → old undeclareditem.type === 'separator'/'label'branches) indropdown-menu.tsx/context-menu.tsx.Predicted: the 6 divider/blank-row tests in
menu-item-separator-dialect.test.tsxgo red (3per container — divider-renders, no-blank-row, retired-spelling-no-longer-draws — the last one
red because the OLD code still draws it), the 2 menubar
shortcuttests stay green (menubaruntouched by this leg). Observed: exactly that — 6 failed, 2 passed. Mutation confirmed on
disk by anchored
grep -cbefore/after; restoration confirmed bygit diff HEADempty aftergit checkout HEAD -- PATH.Leg B — revert only the onClick-wiring fix (
onSelect={() => item.onClick?.()}→ the oldonSelect={item.onSelect}passthrough for dropdown-menu/context-menu; the onClick wiringremoved entirely — both the top-level item and the submenu child — for menubar). Predicted:
all 6 tests in
menu-item-onclick-handler.test.tsxgo red (authoredonClicknever fires;authored
onSelect, which the old dropdown/context code wired directly, DOES fire, invertingthe "never invoked" assertion). Observed: exactly that — 6 failed, 0 passed, after fixing
one script bug on the first attempt (the menubar submenu-child mutation regex initially missed
because that comment spans 3 lines rather than 1, leaving 1 stale wiring call and 1 green test
where I'd predicted red — a bug in the ablation harness, not a finding about the renderer;
corrected and re-run to full 6/6 red with anchored counts confirming 0 remaining occurrences).
Restoration confirmed by
git diff HEADempty.Gate table (all at
c75b928e6)pnpm --filter @object-ui/types type-checktsc --noEmit && tsc -p tsconfig.examples.json && tsc -p tsconfig.test.json, exit 0)pnpm --filter @object-ui/components type-checktsc --noEmit && tsc -p tsconfig.test.json, exit 0)pnpm exec vitest run packages/types/pnpm exec vitest run packages/components/pnpm exec vitest run examples/schema-catalog/test/component-fixture-declared-keys.test.tsnode scripts/check-control-bytes.mjsnode scripts/check-doc-component-types.mjsnode scripts/check-doc-fence-languages.mjsnode scripts/check-lucide-icon-record-names.mjsresolveIcon)node scripts/check-changeset-presence.mjsnode scripts/check-changeset-no-major.mjspnpm --filter @object-ui/typeseslint (touched files)z.ZodType(any-parameterized), unchanged annotation style)pnpm --filter @object-ui/componentseslint (touched files)no-explicit-anywarnings, all on pre-existing[key: string]: anyprop-spread signatures or the sameReact.ComponentType(any-parameterized) test-helper pattern already used by sibling test filesnode scripts/check-doc-snippet-types.mjsPRECONDITION NOT MET(16 unrelated packages, e.g.@object-ui/app-shell, not built in this fresh worktree). This gate only compilests/tsxfences; every block this PR touches isplaintext, unchanged from before, so this diff cannot be implicated. Pre-existing, not attempted to force-build the whole plugin family for an unaffected gate.Out-of-scope, for the record
overlay/menubar.mdxpublishes an inventedMenubarIteminterface — it is what taught the fixture its wrong spellings #6521 (open, unassigned,pm:blocked) covers thevalue/variantfields these threedocs pages invent and
MenubarSchema.menus's taught-as-required optionality — deliberatelyuntouched here (see "Docs" above).
core/app-schema.mdxdocs page has an unrelated, pre-existing namecollision: its
## Global Actionstssnippet doesimport type { MenuItem } from '@object-ui/types'(resolving to the overlayMenuItemthis PR changes, via the barrel)to type
AppAction.items, but the REALAppAction.items(packages/types/src/app.ts:728)resolves to
app.ts's own, differently-shaped, LEGACYMenuItem(re-exported asAppMenuItemin the barrel to avoid exactly this collision) — the snippet is referencing thewrong same-named type and always has been. This does not break under my change (the snippet
is a bare type reference, not an object literal, so it compiles under either shape) and is
unrelated to either card's ruling; filed as finding(docs): core/app-schema.mdx's "Global Actions" snippet imports the wrong same-named
MenuItem#6692 rather than fixed here.Generated by Claude Code