Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .changeset/6278-context-menu-item-icon.md
Original file line numberDiff line numberDiff line change
@@ -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.
11 changes: 10 additions & 1 deletion content/docs/components/overlay/context-menu.mdx
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,13 +9,22 @@ The Context Menu component displays a menu when right-clicking on an element.

<SchemaExample id="components-overlay-context-menu/basic-context-menu" />

## 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;
}
Expand Down
188 changes: 188 additions & 0 deletions packages/components/src/__tests__/context-menu-item-icon.test.tsx
Original file line numberDiff line numberDiff line change
@@ -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-<authored-name>`, 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<any>;
const { container } = render(
<C schema={{ type: 'context-menu', trigger: { type: 'text', content: 'AREA' }, items }} />,
);
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();
}
});
});
});
23 changes: 22 additions & 1 deletion packages/components/src/renderers/overlay/context-menu.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -21,17 +21,37 @@ 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[]) => {
if (!items) return null;
return items.map((item: any, i: number) => {
if (item.type === 'separator') return <ContextMenuSeparator key={i} />;
if (item.type === 'label') return <ContextMenuLabel key={i}>{item.label}</ContextMenuLabel>;
// 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 (
<ContextMenuSub key={i}>
<ContextMenuSubTrigger inset={item.inset}>
{Icon && <Icon className="mr-2 h-4 w-4" />}
{item.label}
</ContextMenuSubTrigger>
<ContextMenuSubContent>
Expand All@@ -43,6 +63,7 @@ const renderContextMenuItems = (items: any[]) => {

return (
<ContextMenuItem key={i} disabled={item.disabled} inset={item.inset} onSelect={item.onSelect}>
{Icon && <Icon className="mr-2 h-4 w-4" />}
{item.label}
{item.shortcut && <ContextMenuShortcut>{item.shortcut}</ContextMenuShortcut>}
</ContextMenuItem>
Expand DownExpand Up@@ -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' }
],
Expand Down
9 changes: 9 additions & 0 deletions scripts/__tests__/check-lucide-icon-record-names.test.ts
Original file line numberDiff line numberDiff line change
Expand Up@@ -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', () => {
Expand All@@ -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', () => {
Expand Down
14 changes: 14 additions & 0 deletions scripts/check-lucide-icon-record-names.mjs
Original file line numberDiff line numberDiff line change
Expand Up@@ -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
Expand Down
Loading