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/lucky-pugs-shake.md
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
---
"@object-ui/components": patch
---

`ui:dropdown-menu` now resolves a menu item's authored `icon` to a glyph instead of drawing the name as raw text.

Both arms of the item renderer — `DropdownMenuItem` and `DropdownMenuSubTrigger` — rendered `icon` straight into a text node, so an item authored as `{ "label": "Copy", "icon": "copy" }` drew the literal word `copy` beside its label. The name is now resolved through `resolveIcon`, the same lucide **record** surface `ui:button` 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.

The `components-overlay-dropdown-menu/with-icons` catalog fixture declared the retired lucide spelling `edit`, which is absent from lucide's runtime `icons` record and would therefore have drawn no glyph; it now declares `square-pen`, the live key the retired export resolves to by identity.
10 changes: 9 additions & 1 deletion content/docs/components/overlay/dropdown-menu.mdx
Original file line numberDiff line numberDiff line change
Expand Up@@ -13,13 +13,21 @@ The Dropdown Menu component displays a list of actions or options when triggered

<SchemaExample id="components-overlay-dropdown-menu/with-icons" />

## Icons

An item's `icon` is a **kebab-case Lucide icon name**, resolved against lucide's
runtime `icons` record — the same surface `ui:button` and the `action:*` family
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.

## Schema

```plaintext
interface DropdownMenuItem {
label?: string;
value?: string;
icon?: string;
icon?: string; // kebab-case Lucide icon name (e.g. "square-pen")
variant?: 'default' | 'destructive';
type?: 'separator';
disabled?: boolean;
Expand Down
Original file line numberDiff line numberDiff line change
Expand Up@@ -9,7 +9,7 @@
{
"label": "Edit",
"value": "edit",
"icon": "edit"
"icon": "square-pen"
},
{
"label": "Copy",
Expand Down
134 changes: 134 additions & 0 deletions packages/components/src/__tests__/dropdown-menu-item-icon.test.tsx
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
/**
* 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:dropdown-menu` resolves an item's authored `icon` to a glyph (objectui#5930).
*
* Before the fix both arms of `renderMenuItems` rendered the authored STRING
* into a text node — `{item.icon && <span className="mr-2">{item.icon}</span>}` —
* so the catalog fixture literally named `with-icons.json` drew the words
* `edit Edit`, `copy Copy`, `trash Delete`. The key parsed, published and was
* documented in the registration's own item-shape description; nothing was
* red, because no renderer test asserted a glyph here.
*
* ## Why the assertions are shaped this way
*
* The defect is a WRONG RENDER, not an absent one, so `queryByText(name)` is
* the load-bearing assertion: a test that only asserted "an svg exists" passes
* against the broken renderer too, since the pre-fix `<span>` sits inside the
* same item as the trigger's own glyph. Both directions are asserted — the
* glyph appears AND the bare name does not.
*
* ⚠️ `defaultOpen: true` is load-bearing. Radix mounts `DropdownMenuContent`
* lazily, so without it this suite renders an EMPTY container and proves
* nothing — the same trap recorded on `inline-locale-label-read-sites.test.tsx`.
*
* ## 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 here
*
* The contract under test is "the authored name is resolved against lucide's
* runtime `icons` RECORD", and the record surface is a synchronous object
* lookup with no chunk-loading path to flake on — unlike the dynamic
* (`LazyIcon`) surface, which sibling suites mock for exactly that reason.
* Mocking the record here would delete the half of the contract that matters:
* that a RETIRED spelling resolves to nothing. `edit` is that control — it is
* a deprecated lucide export whose key was dropped from the record, so it is
* the case that must render no glyph while `square-pen` (the same object under
* its live key) must render one.
*/

import { describe, it, expect, afterEach } from 'vitest';
import { render, screen, cleanup } 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());

/** `defaultOpen` is load-bearing — see the header note on Radix's lazy mount. */
function renderMenu(items: any[]) {
const C = ComponentRegistry.get('dropdown-menu') as React.ComponentType<any>;
return render(
<C schema={{ type: 'dropdown-menu', defaultOpen: true, items }} />,
);
}

/** The rendered glyph for an item, located via the item's own label. */
function glyphFor(label: string): SVGElement | null {
const item = screen.getByText(label).closest('[role="menuitem"], [role="menu"] div');
return item?.querySelector('svg') ?? null;
}

describe('ui:dropdown-menu item icon resolution (objectui#5930)', () => {
describe('DropdownMenuItem arm', () => {
it('renders a glyph for a live icon name', () => {
renderMenu([{ label: 'Copy', icon: 'copy' }]);
expect(glyphFor('Copy')).not.toBeNull();
});

it('does NOT render the authored icon name as text — the defect itself', () => {
renderMenu([{ label: 'Copy', icon: 'copy' }]);
// Pre-fix this found the literal word beside the label.
expect(screen.queryByText('copy')).toBeNull();
});

it('renders no glyph and no text for a RETIRED spelling', () => {
// `edit` is a deprecated lucide export dropped from the runtime `icons`
// record. The record surface is chosen precisely so this renders nothing
// rather than degrading to a wrong glyph.
renderMenu([{ label: 'Edit', icon: 'edit' }]);
expect(glyphFor('Edit')).toBeNull();
expect(screen.queryByText('edit')).toBeNull();
});

it('renders no glyph when no icon is authored', () => {
renderMenu([{ label: 'Plain' }]);
expect(glyphFor('Plain')).toBeNull();
});
});

describe('DropdownMenuSubTrigger arm', () => {
// The submenu TRIGGER is mounted with the parent content; only
// `DropdownMenuSubContent` needs the submenu opened, and it is not read here.
it('renders a glyph for a live icon name', () => {
renderMenu([{ label: 'More', icon: 'trash', children: [{ label: 'Nested' }] }]);
expect(glyphFor('More')).not.toBeNull();
});

it('does NOT render the authored icon name as text — the defect itself', () => {
renderMenu([{ label: 'More', icon: 'trash', children: [{ label: 'Nested' }] }]);
expect(screen.queryByText('trash')).toBeNull();
});
});

describe('the with-icons.json catalog fixture', () => {
// The fixture is a live specimen AND a declared AI few-shot retrieval
// source, so every name it ships must actually draw. `square-pen` is the
// identity-derived live key for the retired `edit` this fixture carried.
it('draws a glyph for every icon name it declares', () => {
renderMenu([
{ label: 'Edit', value: 'edit', icon: 'square-pen' },
{ label: 'Copy', value: 'copy', icon: 'copy' },
{ label: 'Delete', value: 'delete', icon: 'trash' },
]);
for (const label of ['Edit', 'Copy', 'Delete']) {
expect(glyphFor(label), `${label} should draw a glyph`).not.toBeNull();
}
for (const name of ['square-pen', 'copy', 'trash']) {
expect(screen.queryByText(name)).toBeNull();
}
});
});
});
22 changes: 19 additions & 3 deletions packages/components/src/renderers/overlay/dropdown-menu.tsx
Original file line numberDiff line numberDiff line change
Expand Up@@ -26,18 +26,34 @@ import {
DropdownMenuSubContent
} 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, and it was
// rendered as a raw text node here — the fixture named `with-icons.json` drew
// the words "edit"/"copy"/"trash" beside its labels (objectui#5930).
//
// 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.
import { resolveIcon } from '../action/resolve-icon';

// Helper for recursive menu items
const renderMenuItems = (items: any[]) => {
if (!items) return null;
return items.map((item: any, i: number) => {
if (item.type === 'separator') return <DropdownMenuSeparator key={i} />;
if (item.type === 'label') return <DropdownMenuLabel key={i}>{item.label}</DropdownMenuLabel>;
// 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).
const Icon = resolveIcon(item.icon);
if (item.children) {
return (
<DropdownMenuSub key={i}>
<DropdownMenuSubTrigger inset={item.inset}>
{item.icon && <span className="mr-2">{item.icon}</span>}
{Icon && <Icon className="mr-2 h-4 w-4" />}
{item.label}
</DropdownMenuSubTrigger>
<DropdownMenuSubContent>
Expand All@@ -49,7 +65,7 @@ const renderMenuItems = (items: any[]) => {

return (
<DropdownMenuItem key={i} disabled={item.disabled} inset={item.inset} onSelect={item.onSelect}>
{item.icon && <span className="mr-2">{item.icon}</span>}
{Icon && <Icon className="mr-2 h-4 w-4" />}
{item.label}
{item.shortcut && <span className="ml-auto text-xs tracking-widest opacity-60">{item.shortcut}</span>}
</DropdownMenuItem>
Expand DownExpand Up@@ -96,7 +112,7 @@ ComponentRegistry.register('dropdown-menu',
name: 'items',
type: 'array',
label: 'Items',
description: 'Recursive structure: { type?: "separator"|"label", label, icon, shortcut, disabled, children: [] }'
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.'
},
{ name: 'className', type: 'string', label: 'Content CSS Class' }
],
Expand Down
Loading