diff --git a/.changeset/6278-context-menu-item-icon.md b/.changeset/6278-context-menu-item-icon.md new file mode 100644 index 0000000000..080e0bf78f --- /dev/null +++ b/.changeset/6278-context-menu-item-icon.md @@ -0,0 +1,9 @@ +--- +"@object-ui/components": patch +--- + +`ui:context-menu` now resolves a menu item's authored `icon` to a glyph. It previously never read the key at all. + +Both arms of `renderContextMenuItems` — the leaf `ContextMenuItem` and the `ContextMenuSubTrigger` — ignored `icon` entirely, so an item authored as `{ "label": "Copy", "icon": "copy" }` drew its label and nothing else. The name is now resolved through `resolveIcon`, the same lucide **record** surface `ui:button`, `ui:dropdown-menu` and the `action:*` family already resolve against: a live name draws its glyph, and an unknown or retired spelling draws nothing rather than degrading to a wrong glyph. This mirrors the repair `ui:dropdown-menu` received for the identical defect. + +The `components-overlay-context-menu/basic-context-menu` catalog fixture already declared four live names — `copy`, `scissors`, `clipboard`, `trash` — which drew nothing before this change and draw their glyphs now. Those names are also brought under `check:lucide-icon-record-names` by a new `context-menu` census entry, so a future retired spelling fails the gate instead of silently drawing nothing. diff --git a/content/docs/components/overlay/context-menu.mdx b/content/docs/components/overlay/context-menu.mdx index 56d3564692..2ca05f8d6f 100644 --- a/content/docs/components/overlay/context-menu.mdx +++ b/content/docs/components/overlay/context-menu.mdx @@ -9,13 +9,22 @@ The Context Menu component displays a menu when right-clicking on an element. +## Icons + +An item's `icon` is a **kebab-case Lucide icon name**, resolved against lucide's +runtime `icons` record — the same surface `ui:button`, `ui:dropdown-menu` and the +`action:*` family resolve against. It is read on **both** arms: a leaf item and a +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. + ## Schema ```plaintext interface ContextMenuItem { label?: string; value?: string; - icon?: string; + icon?: string; // kebab-case Lucide icon name (e.g. "trash") type?: 'separator'; disabled?: boolean; } diff --git a/packages/components/src/__tests__/context-menu-item-icon.test.tsx b/packages/components/src/__tests__/context-menu-item-icon.test.tsx new file mode 100644 index 0000000000..6e078989ae --- /dev/null +++ b/packages/components/src/__tests__/context-menu-item-icon.test.tsx @@ -0,0 +1,188 @@ +/** + * 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. + */ + +/** + * `ui:context-menu` resolves an item's authored `icon` to a glyph (objectui#6278). + * + * The twin repair is objectui#5930 on `renderers/overlay/dropdown-menu.tsx`; + * this file is that suite ported to the file next door, with the two + * differences below made explicit rather than copied over silently. + * + * ## Difference 1 — the defect here is an ABSENT render, not a WRONG one + * + * `dropdown-menu` rendered the authored string into a text node, so its + * load-bearing assertion could be `queryByText('trash')` — the word was on + * screen. `context-menu` never referenced `icon` at all (0 occurrences in the + * file at `090927f4f`, against 2 for `shortcut` and 12 for `label` on the same + * pathspec), so it drew NOTHING. `queryByText(name)` is therefore a GHOST here: + * it is null before the fix and null after it. The discriminating assertion is + * the presence of the RESOLVED GLYPH, and that is what every measurement row + * below asserts. + * + * ## Difference 2 — the submenu trigger already contains an svg + * + * `ContextMenuSubTrigger` renders its own `ChevronRight` unconditionally (see + * `src/ui/context-menu.tsx`). A bare `querySelector('svg')` on that arm is + * therefore GREEN IN BOTH WORLDS — the blind instrument this suite must not + * use. Each assertion names the glyph by the identity lucide gives it, + * `svg.lucide-`, which is derived from the AUTHORED name (the + * independent input) and not from the renderer under test. The chevron is + * asserted alongside it as a positive control ON THE INSTRUMENT: if the + * `lucide-trash` row is red while the chevron row is green, the query works and + * the authored icon is genuinely missing. + * + * ## Why Radix's lazy mount is handled by an EVENT here, not `defaultOpen` + * + * The twin passes `defaultOpen: true`. `@radix-ui/react-context-menu`'s root + * has no such prop — its `ContextMenuProps` is `{ children, open, onOpenChange, + * dir, modal }` — and a context menu opens on the trigger's `contextmenu` + * event. Without firing it this suite would render an EMPTY container and prove + * nothing, which is why `expectsMenuOpen` is asserted as a harness control + * before any glyph is read. + * + * ## Why the renderer is invoked DIRECTLY + * + * `ComponentRegistry.get(name)` returns the component the registry actually + * renders; driving through `SchemaRenderer` injects its own props around it and + * can be green in both directions (PR #4603's toggle case, restated by #4580). + * + * ## Why lucide is NOT mocked + * + * The contract under test is "the authored name is resolved against lucide's + * runtime `icons` RECORD". Mocking the record would delete the half that + * matters: that a RETIRED spelling resolves to NOTHING rather than degrading to + * a wrong glyph. `edit` is that control — a deprecated lucide export whose key + * is absent from the runtime record (measured on lucide-react 1.31.0, 1767 + * keys), so it must draw no glyph while `copy`/`trash` must draw one. That row + * is what rules out the `LazyIcon` surface, which would degrade `edit` to the + * `Database` glyph. + * + * ## What this file does NOT own + * + * Fixture drift. The catalog's own four names live in + * `examples/schema-catalog/src/schemas/components-overlay-context-menu/basic-context-menu.json` + * and are judged on every run by `scripts/check-lucide-icon-record-names.mjs`, + * whose census this card's PR extends with a `'context-menu'` entry. This suite + * pins the RENDERER; the gate pins the SPELLINGS. + */ + +import { describe, it, expect, afterEach } from 'vitest'; +import { render, screen, cleanup, fireEvent } from '@testing-library/react'; +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()); + +/** + * Render the menu and OPEN it. Radix mounts `ContextMenuContent` only after the + * trigger sees a `contextmenu` event — see the header note. + */ +function renderMenu(items: any[]) { + const C = ComponentRegistry.get('context-menu') as React.ComponentType; + const { container } = render( + , + ); + fireEvent.contextMenu(container.firstElementChild as HTMLElement); + return container; +} + +/** The menu item element carrying `label`. Both arms render `role="menuitem"`. */ +function itemFor(label: string): HTMLElement { + const el = screen.getByText(label).closest('[role="menuitem"]'); + if (!el) throw new Error(`no [role="menuitem"] ancestor for ${label}`); + return el as HTMLElement; +} + +describe('ui:context-menu item icon resolution (objectui#6278)', () => { + describe('harness control — the menu actually opens', () => { + // Without this every "renders no glyph" row below would pass vacuously + // against a container that rendered nothing at all. + it('mounts the content and its items after the contextmenu event', () => { + renderMenu([{ label: 'Copy', icon: 'copy' }, { label: 'Plain' }]); + expect(screen.getByRole('menu')).toBeTruthy(); + expect(itemFor('Copy')).toBeTruthy(); + expect(itemFor('Plain')).toBeTruthy(); + }); + }); + + describe('ContextMenuItem arm (leaf)', () => { + it('renders the resolved glyph for a live icon name', () => { + renderMenu([{ label: 'Copy', icon: 'copy' }]); + // RED before the repair: the leaf item contained no svg whatsoever. + expect(itemFor('Copy').querySelector('svg.lucide-copy')).not.toBeNull(); + }); + + it('renders no glyph for a RETIRED spelling — the RECORD surface, not a fallback', () => { + // Rules out `LazyIcon`, which degrades an unknown name to `Database`. + renderMenu([{ label: 'Edit', icon: 'edit' }]); + expect(itemFor('Edit').querySelector('svg')).toBeNull(); + }); + + it('renders no glyph when no icon is authored', () => { + renderMenu([{ label: 'Plain' }]); + expect(itemFor('Plain').querySelector('svg')).toBeNull(); + }); + }); + + describe('ContextMenuSubTrigger arm (submenu)', () => { + // Repairing only the leaf arm would be "a narrower version of the same + // bug" (objectui#5930), so this arm is measured as its own row. + const submenu = [{ label: 'More', icon: 'trash', children: [{ label: 'Nested' }] }]; + + it('positive control on the instrument — the arm DOES contain a queryable svg', () => { + // Green in both worlds BY DESIGN: `ContextMenuSubTrigger` always draws a + // chevron. It exists so a red `lucide-trash` row cannot be misread as a + // broken query. + renderMenu(submenu); + expect(itemFor('More').querySelector('svg.lucide-chevron-right')).not.toBeNull(); + }); + + it('renders the resolved glyph for a live icon name', () => { + renderMenu(submenu); + // RED before the repair: the chevron was the arm's ONLY svg. + expect(itemFor('More').querySelector('svg.lucide-trash')).not.toBeNull(); + }); + + it('renders no glyph beside the chevron for a RETIRED spelling', () => { + renderMenu([{ label: 'More', icon: 'edit', children: [{ label: 'Nested' }] }]); + const svgs = Array.from(itemFor('More').querySelectorAll('svg')); + expect(svgs.map((s) => s.getAttribute('class'))).toHaveLength(1); + expect(itemFor('More').querySelector('svg.lucide-chevron-right')).not.toBeNull(); + }); + }); + + describe('the basic-context-menu.json catalog fixture', () => { + // The four names the catalog actually authors. The fixture is a live + // specimen AND a declared AI few-shot retrieval source, so every name it + // ships must draw. Spelling drift is the gate's job (see the header); + // this row is the renderer's half of that contract. + it('draws a distinct glyph for each of the four authored names', () => { + renderMenu([ + { label: 'Copy', value: 'copy', icon: 'copy' }, + { label: 'Cut', value: 'cut', icon: 'scissors' }, + { label: 'Paste', value: 'paste', icon: 'clipboard' }, + { type: 'separator' }, + { label: 'Delete', value: 'delete', icon: 'trash' }, + ]); + for (const [label, name] of [ + ['Copy', 'copy'], + ['Cut', 'scissors'], + ['Paste', 'clipboard'], + ['Delete', 'trash'], + ]) { + expect( + itemFor(label).querySelector(`svg.lucide-${name}`), + `${label} should draw the ${name} glyph`, + ).not.toBeNull(); + } + }); + }); +}); diff --git a/packages/components/src/renderers/overlay/context-menu.tsx b/packages/components/src/renderers/overlay/context-menu.tsx index 943f184aaf..7aa466e63d 100644 --- a/packages/components/src/renderers/overlay/context-menu.tsx +++ b/packages/components/src/renderers/overlay/context-menu.tsx @@ -21,6 +21,21 @@ import { ContextMenuShortcut } from '../../ui'; import { renderChildren } from '../../lib/utils'; +// Same-package sibling import, the path `renderers/complex/data-table.tsx` +// already uses. `icon` on a menu item is an authored lucide NAME. The twin +// `dropdown-menu.tsx` rendered it into a text node; THIS renderer never +// referenced the key at all, so the catalog fixture +// `basic-context-menu.json` shipped four live names — `copy`, `scissors`, +// `clipboard`, `trash` — that drew NOTHING (objectui#6278). +// +// Routed through the RECORD surface (`icons` from 'lucide-react'), which is +// what `action:*` and `ui:button` next door resolve against, so a retired +// spelling renders NOTHING rather than a word. The dynamic surface +// (`LazyIcon`) is deliberately NOT used: it degrades an unknown name to the +// `Database` glyph, trading a no-icon failure for a WRONG-icon one, recorded +// as ruled out for authored icon fields by objectui#5622 and #5633 and +// 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[]) => { @@ -28,10 +43,15 @@ const renderContextMenuItems = (items: any[]) => { return items.map((item: any, i: number) => { if (item.type === 'separator') return ; if (item.type === 'label') return {item.label}; + // 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). + const Icon = resolveIcon(item.icon); if (item.children) { return ( + {Icon && } {item.label} @@ -43,6 +63,7 @@ const renderContextMenuItems = (items: any[]) => { return ( + {Icon && } {item.label} {item.shortcut && {item.shortcut}} @@ -83,7 +104,7 @@ ComponentRegistry.register('context-menu', name: 'items', type: 'array', label: 'Items', - description: 'Recursive structure: { type?: "separator"|"label", label, shortcut, children }' + 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.' }, { name: 'className', type: 'string', label: 'Content CSS Class' } ], diff --git a/scripts/__tests__/check-lucide-icon-record-names.test.ts b/scripts/__tests__/check-lucide-icon-record-names.test.ts index d046927279..e542e4977d 100644 --- a/scripts/__tests__/check-lucide-icon-record-names.test.ts +++ b/scripts/__tests__/check-lucide-icon-record-names.test.ts @@ -586,6 +586,11 @@ describe('this repository', () => { .filter(([, spec]) => 'descendants' in spec && spec.descendants) .map(([type]) => type); expect(declaring).toContain('dropdown-menu'); + // objectui#6278 — the same shape in `renderers/overlay/context-menu.tsx`. + // Its four catalog names (`copy`, `scissors`, `clipboard`, `trash`) are + // judged only through this declaration; drop the entry and the fixture goes + // back to being unjudged while the repo stays green. + expect(declaring).toContain('context-menu'); }); it('did NOT grow part 1 in the process — descent is a part-2 rule', () => { @@ -596,6 +601,10 @@ describe('this repository', () => { expect(repoResult.discovered.record).toHaveLength(8); expect(repoResult.discovered.record).not.toContain('packages/components/src/renderers/overlay/dropdown-menu.tsx'); expect(RECORD_READING_TYPES['dropdown-menu'].resolver).toContain('renderers/action/resolve-icon.ts'); + // objectui#6278 routes the twin the same way, so it must not move the + // part-1 count above either. + expect(repoResult.discovered.record).not.toContain('packages/components/src/renderers/overlay/context-menu.tsx'); + expect(RECORD_READING_TYPES['context-menu'].resolver).toContain('renderers/action/resolve-icon.ts'); }); it('really judges the `ui:icon` nodes objectui#6009 opened up', () => { diff --git a/scripts/check-lucide-icon-record-names.mjs b/scripts/check-lucide-icon-record-names.mjs index 53a9bf451b..92f827a242 100644 --- a/scripts/check-lucide-icon-record-names.mjs +++ b/scripts/check-lucide-icon-record-names.mjs @@ -205,6 +205,20 @@ export const RECORD_READING_TYPES = { 'action:group': { paths: ['icon', 'actions[].icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' }, 'action:icon': { paths: ['icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' }, 'action:menu': { paths: ['icon', 'actions[].icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' }, + // The twin of the `dropdown-menu` entry below, and it earns its own line for + // the same reason: the container's OWN `icon` is never read (`paths: []`), + // while its item icons sit on untyped children, recursively, and every one of + // them goes through the single `resolveIcon(item.icon)` call that serves BOTH + // the leaf arm and the submenu-trigger arm. The renderer did not read the key + // at all until objectui#6278, which is why this entry could not have been + // added by objectui#5992 — a census entry declares that a type's names REACH + // a vocabulary, and until the repair landed they reached nothing. + 'context-menu': { + paths: [], + descendants: true, + min: 1, + resolver: 'packages/components/src/renderers/action/resolve-icon.ts (via renderers/overlay/context-menu.tsx)', + }, 'data-table': { paths: ['rowActionDefs[].icon'], resolver: 'packages/components/src/renderers/complex/data-table.tsx' }, // The container's OWN `icon` is never read (`paths: []`); its item icons sit // on untyped children, recursively, and every one of them goes through the