diff --git a/.changeset/tidy-donkeys-shave.md b/.changeset/tidy-donkeys-shave.md new file mode 100644 index 0000000000..28ff741df2 --- /dev/null +++ b/.changeset/tidy-donkeys-shave.md @@ -0,0 +1,13 @@ +--- +"@object-ui/components": patch +--- + +`ui:breadcrumb` and `ui:command` now resolve a child item's authored `icon` to a glyph instead of drawing nothing. + +`BreadcrumbItem.icon` and `CommandItem.icon` are declared keys of the protocol — present in `@object-ui/types`, mirrored in the Zod schemas, and documented on both components' pages — and neither renderer referenced `icon` at all. The catalog fixture literally named `components-data-display-breadcrumb/with-icons` rendered no icons, and the nine names the two `command` fixtures declare all drew nothing. Both names are now resolved through `resolveIcon`, the same lucide **record** surface `ui:button`, `action:*`, `ui:dropdown-menu` and `ui:context-menu` resolve against: a live name draws its glyph, and an unknown or retired spelling draws nothing rather than degrading to a wrong glyph. + +`breadcrumb` resolves once per item and draws the glyph above the page/link split, so the last crumb (rendered as the current page) carries it as well as the linked crumbs before it. + +Two fixtures declared retired lucide spellings that are absent from the runtime `icons` record and would therefore have drawn nothing: `components-data-display-breadcrumb/with-icons` declared `layout`, now `panels-top-left`, and `components-form-command/command-menu` declared `smile`, now `face-slightly-smiling` — in both cases the live key the retired export resolves to by identity. + +`ui:button-group` is NOT included: its fixtures author `icon` on `buttons[]`, but `ButtonGroupButton` declares no such key in `@object-ui/types` or its Zod mirror, so wiring it would first mean widening the published item contract. Routed for a decision rather than chosen (objectui#5931). diff --git a/content/docs/components/data-display/breadcrumb.mdx b/content/docs/components/data-display/breadcrumb.mdx index 13f2e1e208..344eddf309 100644 --- a/content/docs/components/data-display/breadcrumb.mdx +++ b/content/docs/components/data-display/breadcrumb.mdx @@ -13,13 +13,22 @@ The Breadcrumb component shows the current page's location within the site hiera +## 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 `layout`) renders **no glyph**, never a fallback glyph +and never the literal name as text. The glyph is drawn for every crumb: the +final one (rendered as the current page) as well as the linked ones before it. + ## Schema ```plaintext interface BreadcrumbItem { label: string; href?: string; - icon?: string; + icon?: string; // kebab-case Lucide icon name (e.g. "panels-top-left") } interface BreadcrumbSchema { diff --git a/content/docs/components/form/command.mdx b/content/docs/components/form/command.mdx index 460f2ccc62..abab21a968 100644 --- a/content/docs/components/form/command.mdx +++ b/content/docs/components/form/command.mdx @@ -9,13 +9,21 @@ The Command component provides a fast, searchable command menu with keyboard nav +## 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 `smile`) renders **no glyph**, never a fallback glyph +and never the literal name as text. + ## Schema ```plaintext interface CommandItem { value: string; label: string; - icon?: string; + icon?: string; // kebab-case Lucide icon name (e.g. "face-slightly-smiling") } interface CommandGroup { diff --git a/examples/schema-catalog/src/schemas/components-data-display-breadcrumb/with-icons.json b/examples/schema-catalog/src/schemas/components-data-display-breadcrumb/with-icons.json index c9e9434a0a..e22721015f 100644 --- a/examples/schema-catalog/src/schemas/components-data-display-breadcrumb/with-icons.json +++ b/examples/schema-catalog/src/schemas/components-data-display-breadcrumb/with-icons.json @@ -13,7 +13,7 @@ }, { "label": "Components", - "icon": "layout" + "icon": "panels-top-left" } ] } diff --git a/examples/schema-catalog/src/schemas/components-form-command/command-menu.json b/examples/schema-catalog/src/schemas/components-form-command/command-menu.json index b0b326c801..3d3575cf25 100644 --- a/examples/schema-catalog/src/schemas/components-form-command/command-menu.json +++ b/examples/schema-catalog/src/schemas/components-form-command/command-menu.json @@ -13,7 +13,7 @@ { "value": "search", "label": "Search Emoji", - "icon": "smile" + "icon": "face-slightly-smiling" }, { "value": "calculator", diff --git a/packages/components/src/__tests__/breadcrumb-item-icon.test.tsx b/packages/components/src/__tests__/breadcrumb-item-icon.test.tsx new file mode 100644 index 0000000000..93ac2f294d --- /dev/null +++ b/packages/components/src/__tests__/breadcrumb-item-icon.test.tsx @@ -0,0 +1,221 @@ +/** + * 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:breadcrumb` resolves an item's authored `icon` to a glyph (objectui#5931). + * + * `BreadcrumbItem.icon` has been a declared, published key of the protocol + * (`packages/types/src/navigation.ts`, mirrored in `zod/navigation.zod.ts`, + * documented in `content/docs/components/data-display/breadcrumb.mdx`) and is + * authored by the catalog fixture literally named `with-icons.json` — while + * `renderers/data-display/breadcrumb.tsx` contained ZERO occurrences of `icon`. + * The page named "With Icons" rendered none. + * + * The sibling repairs are objectui#5930 (`dropdown-menu`) and objectui#6278 + * (`context-menu`); this suite is that shape ported, with the differences below + * made explicit rather than copied over silently. + * + * ## Difference 1 — the defect 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('copy')` — the word was on + * screen. This renderer never referenced `icon` at all, so it drew NOTHING and + * `queryByText(name)` is null both before and after the repair. The + * DISCRIMINATING assertion here is therefore the presence of the RESOLVED + * GLYPH, and that is what every measurement row asserts. The text direction is + * still asserted, once, as a REGRESSION guard: it is the exact defect + * `dropdown-menu` shipped, and a future "repair" that printed the name instead + * of resolving it would be caught by it and by nothing else here. + * + * ## Difference 2 — nothing here mounts lazily, and that is a MEASUREMENT + * + * The twins pass `defaultOpen: true` / fire a `contextmenu` event because Radix + * mounts their content lazily; without it those suites render an empty + * container and prove nothing. `ui/breadcrumb.tsx` is plain `nav`/`ol`/`li` + * markup with no Radix root and no open state at all, so its items are in the + * tree on first render. The harness control below asserts that rather than + * assuming it — a vacuous container would make every "renders no glyph" row + * pass for the wrong reason. + * + * ## Difference 3 — BOTH arms, and where the icon sits relative to them + * + * The renderer splits on position: the LAST crumb is a `BreadcrumbPage` + * (``), every earlier one a + * `BreadcrumbLink` (``). Repairing only one would be "a narrower version of + * the same bug" (objectui#5930), so both are measured as their own rows. The + * fix resolves once per item and renders the glyph ABOVE that split, inside + * `BreadcrumbItem`, so neither arm can be forgotten. + * + * ## The instrument's positive control + * + * `BreadcrumbSeparator` always draws a `ChevronRight`, in a SIBLING `
  • `. A + * bare `container.querySelector('svg')` would therefore be green in both worlds + * — the blind instrument this suite must not use. Every row scopes to the + * crumb's own `
  • ` and names the glyph by the class lucide derives from the + * icon's own identity (`svg.lucide-`), while the chevron is asserted at + * container level as a control ON THE INSTRUMENT: if a `lucide-book` row is red + * while the chevron row is green, the query works and the authored icon is + * genuinely absent. + * + * ## 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. `layout` is that control — a deprecated lucide export whose + * key is absent from the runtime record (measured on lucide-react 1.31.0, 1767 + * keys; `Layout === PanelsTopLeft` is TRUE, the retired alias is the very same + * object under a dead name). It is also the spelling this fixture shipped, now + * corrected to `panels-top-left`. That row is what rules out the `LazyIcon` + * surface, which would degrade `layout` to the `Database` glyph. + * + * ## 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). + * + * ## What this file does NOT own + * + * Fixture spelling drift. `with-icons.json`'s names are judged on every run by + * `scripts/check-lucide-icon-record-names.mjs`, whose census this card's PR + * extends with a `'breadcrumb'` entry. This suite pins the RENDERER; the gate + * pins the SPELLINGS. + */ + +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()); + +function renderCrumbs(items: any[]) { + const C = ComponentRegistry.get('breadcrumb') as React.ComponentType; + return render(); +} + +/** The crumb's own `
  • ` (`BreadcrumbItem`). The separator is a DIFFERENT `
  • `. */ +function crumbFor(label: string): HTMLElement { + const el = screen.getByText(label).closest('li'); + if (!el) throw new Error(`no
  • ancestor for ${label}`); + return el as HTMLElement; +} + +/** Two crumbs, so the link arm and the page arm are both exercised. */ +const TWO = (icon?: string) => [ + { label: 'Home', href: '/', ...(icon ? { icon } : {}) }, + { label: 'Docs', ...(icon ? { icon } : {}) }, +]; + +describe('ui:breadcrumb item icon resolution (objectui#5931)', () => { + describe('harness controls', () => { + it('mounts every crumb on first render — nothing here is lazy', () => { + renderCrumbs(TWO()); + expect(crumbFor('Home')).toBeTruthy(); + expect(crumbFor('Docs')).toBeTruthy(); + }); + + it('both arms are the ones the renderer is documented to produce', () => { + renderCrumbs(TWO()); + // Non-last -> BreadcrumbLink (); last -> BreadcrumbPage (aria-current). + expect(screen.getByText('Home').closest('a')).not.toBeNull(); + expect(screen.getByText('Docs').closest('[aria-current="page"]')).not.toBeNull(); + }); + + it('positive control on the instrument — a queryable svg IS present', () => { + // Green in both worlds BY DESIGN: `BreadcrumbSeparator` always draws a + // chevron. It exists so a red `lucide-*` row cannot be misread as a + // broken query. + const { container } = renderCrumbs(TWO()); + expect(container.querySelector('svg.lucide-chevron-right')).not.toBeNull(); + // …and it is NOT inside either crumb, which is why the rows below can + // scope to the crumb's own
  • and stay discriminating. + expect(crumbFor('Home').querySelector('svg')).toBeNull(); + }); + }); + + describe('BreadcrumbLink arm (every crumb but the last)', () => { + it('renders the resolved glyph for a live icon name', () => { + renderCrumbs([{ label: 'Docs', href: '/docs', icon: 'book' }, { label: 'Components' }]); + // RED before the repair: the crumb contained no svg whatsoever. + expect(crumbFor('Docs').querySelector('svg.lucide-book')).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`. + renderCrumbs([{ label: 'Docs', href: '/docs', icon: 'layout' }, { label: 'Components' }]); + expect(crumbFor('Docs').querySelector('svg')).toBeNull(); + }); + + it('renders no glyph for an UNKNOWN name', () => { + renderCrumbs([{ label: 'Docs', href: '/docs', icon: 'not-a-real-icon' }, { label: 'Components' }]); + expect(crumbFor('Docs').querySelector('svg')).toBeNull(); + }); + }); + + describe('BreadcrumbPage arm (the last crumb)', () => { + it('renders the resolved glyph for a live icon name', () => { + renderCrumbs([{ label: 'Docs', href: '/docs' }, { label: 'Components', icon: 'panels-top-left' }]); + // RED before the repair, and red again if only the link arm were fixed. + expect(crumbFor('Components').querySelector('svg.lucide-panels-top-left')).not.toBeNull(); + }); + + it('renders no glyph for a RETIRED spelling', () => { + renderCrumbs([{ label: 'Docs', href: '/docs' }, { label: 'Components', icon: 'layout' }]); + expect(crumbFor('Components').querySelector('svg')).toBeNull(); + }); + + it('renders no glyph when no icon is authored', () => { + renderCrumbs(TWO()); + expect(crumbFor('Docs').querySelector('svg')).toBeNull(); + }); + }); + + describe('the authored NAME never reaches the DOM as text', () => { + // Not discriminating here — this renderer never printed the name, so this + // is null in both worlds. It is the regression guard against acquiring + // `dropdown-menu`'s defect (objectui#5930), which is the one failure mode a + // glyph-presence assertion cannot see. + it('draws the glyph and not the word', () => { + renderCrumbs([{ label: 'Docs', href: '/docs', icon: 'book' }, { label: 'Components', icon: 'panels-top-left' }]); + expect(crumbFor('Docs').querySelector('svg.lucide-book')).not.toBeNull(); + expect(screen.queryByText('book')).toBeNull(); + expect(screen.queryByText('panels-top-left')).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. `panels-top-left` is + // the identity-derived live key for the retired `layout` it carried. + it('draws a glyph for every icon name it declares', () => { + renderCrumbs([ + { label: 'Home', href: '/', icon: 'home' }, + { label: 'Docs', href: '/docs', icon: 'book' }, + { label: 'Components', icon: 'panels-top-left' }, + ]); + for (const [label, glyph] of [ + // `home` is the ONE authored name whose glyph class is not its own + // spelling: `resolveIcon`'s `iconNameMap` sends it to lucide's `House`. + // Asserting the resolved class pins that indirection from the outside. + ['Home', 'lucide-house'], + ['Docs', 'lucide-book'], + ['Components', 'lucide-panels-top-left'], + ]) { + expect( + crumbFor(label).querySelector(`svg.${glyph}`), + `${label} should draw ${glyph}`, + ).not.toBeNull(); + } + }); + }); +}); diff --git a/packages/components/src/__tests__/command-item-icon.test.tsx b/packages/components/src/__tests__/command-item-icon.test.tsx new file mode 100644 index 0000000000..84ae988294 --- /dev/null +++ b/packages/components/src/__tests__/command-item-icon.test.tsx @@ -0,0 +1,219 @@ +/** + * 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:command` resolves an item's authored `icon` to a glyph (objectui#5931). + * + * `CommandItem.icon` has been a declared, published key of the protocol + * (`packages/types/src/form.ts`, mirrored in `zod/form.zod.ts`, documented in + * `content/docs/components/form/command.mdx`) and is authored NINE times across + * the two catalog fixtures — while `renderers/form/command.tsx` contained ZERO + * occurrences of `icon`. A command palette whose every row was drawn without + * its glyph. + * + * The sibling repairs are objectui#5930 (`dropdown-menu`) and objectui#6278 + * (`context-menu`); this suite is that shape ported, with the differences below + * made explicit rather than copied over silently. + * + * ## Difference 1 — the defect 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('copy')` — the word was on + * screen. This renderer never referenced `icon` at all, so it drew NOTHING and + * `queryByText(name)` is null both before and after the repair. The + * DISCRIMINATING assertion here is therefore the presence of the RESOLVED + * GLYPH. The text direction is still asserted, once, as a REGRESSION guard: it + * is the exact defect `dropdown-menu` shipped, and a future "repair" that + * printed the name instead of resolving it would be caught by it and by + * nothing else here. + * + * ## Difference 2 — the lazy-mount question, answered by measurement + * + * The twins pass `defaultOpen: true` / fire a `contextmenu` event because Radix + * mounts their content lazily; without it those suites render an empty + * container and prove nothing. `ui:command` renders `cmdk` INLINE — there is no + * open state on the component at all (`CommandDialog` is a separate export this + * renderer does not use), so its list is in the tree on first render. cmdk does + * FILTER, though, and an item filtered out is an item that is not in the DOM: + * with an empty query every item matches, which is the state every row here + * renders in. The harness control asserts the items really mounted rather than + * assuming either fact. + * + * ## Difference 3 — this component has exactly ONE arm, and that was checked + * + * The twins carry a second arm (the submenu trigger) that had the identical + * defect, and repairing only the leaf would have been "a narrower version of + * the same bug" (objectui#5930). `ui:command` has no second arm to miss: + * `CommandGroup` takes `heading` as a plain string and the `CommandGroup` type + * declares no `icon` of its own, so `groups[].items[].icon` is the only icon + * key in the shape. Recorded as a measurement, because "there is no second + * arm" is exactly the claim that is worth being wrong about. + * + * ## The instrument's positive control + * + * `CommandInput` always draws a `Search` glyph, OUTSIDE the list. A bare + * `container.querySelector('svg')` would therefore be green in both worlds — + * the blind instrument this suite must not use. Every row scopes to the item's + * own `[cmdk-item]` element and names the glyph by the class lucide derives + * from the icon's own identity (`svg.lucide-`), while the search glyph is + * asserted at container level as a control ON THE INSTRUMENT. + * + * ## 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. `smile` is that control — a deprecated lucide export whose key + * is absent from the runtime record (measured on lucide-react 1.31.0, 1767 + * keys; `Smile === FaceSlightlySmiling` is TRUE, the retired alias is the very + * same object under a dead name). It is also the spelling `command-menu.json` + * shipped, now corrected to `face-slightly-smiling`. That row is what rules out + * the `LazyIcon` surface, which would degrade `smile` to the `Database` glyph. + * + * ## 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). + * + * ## What this file does NOT own + * + * Fixture spelling drift. The catalog's nine names are judged on every run by + * `scripts/check-lucide-icon-record-names.mjs`, whose census this card's PR + * extends with a `'command'` entry. This suite pins the RENDERER; the gate pins + * the SPELLINGS. + */ + +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()); + +function renderCommand(items: any[], heading = 'Suggestions') { + const C = ComponentRegistry.get('command') as React.ComponentType; + return render(); +} + +/** The item's own cmdk element. */ +function itemFor(label: string): HTMLElement { + const el = screen.getByText(label).closest('[cmdk-item], [role="option"]'); + if (!el) throw new Error(`no cmdk item ancestor for ${label}`); + return el as HTMLElement; +} + +describe('ui:command item icon resolution (objectui#5931)', () => { + describe('harness controls', () => { + it('mounts the list and its items on first render — nothing is deferred', () => { + // Without this every "renders no glyph" row below could pass vacuously + // against a container that rendered no items at all. + renderCommand([{ value: 'calendar', label: 'Calendar', icon: 'calendar' }, { value: 'plain', label: 'Plain' }]); + expect(itemFor('Calendar')).toBeTruthy(); + expect(itemFor('Plain')).toBeTruthy(); + }); + + it('positive control on the instrument — a queryable svg IS present', () => { + // Green in both worlds BY DESIGN: `CommandInput` always draws a search + // glyph. It exists so a red `lucide-*` row cannot be misread as a broken + // query. + const { container } = renderCommand([{ value: 'plain', label: 'Plain' }]); + expect(container.querySelector('svg.lucide-search')).not.toBeNull(); + // …and it is NOT inside the item, which is why the rows below can scope + // to the item and stay discriminating. + expect(itemFor('Plain').querySelector('svg')).toBeNull(); + }); + }); + + describe('CommandItem arm — this component\'s only icon arm', () => { + it('renders the resolved glyph for a live icon name', () => { + renderCommand([{ value: 'calendar', label: 'Calendar', icon: 'calendar' }]); + // RED before the repair: the item contained no svg whatsoever. + expect(itemFor('Calendar').querySelector('svg.lucide-calendar')).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`. + renderCommand([{ value: 'search', label: 'Search Emoji', icon: 'smile' }]); + expect(itemFor('Search Emoji').querySelector('svg')).toBeNull(); + }); + + it('renders no glyph for an UNKNOWN name', () => { + renderCommand([{ value: 'x', label: 'Unknown', icon: 'not-a-real-icon' }]); + expect(itemFor('Unknown').querySelector('svg')).toBeNull(); + }); + + it('renders no glyph when no icon is authored', () => { + renderCommand([{ value: 'plain', label: 'Plain' }]); + expect(itemFor('Plain').querySelector('svg')).toBeNull(); + }); + }); + + describe('the authored NAME never reaches the DOM as text', () => { + // Not discriminating here — this renderer never printed the name, so this + // is null in both worlds. It is the regression guard against acquiring + // `dropdown-menu`'s defect (objectui#5930), which is the one failure mode a + // glyph-presence assertion cannot see. + it('draws the glyph and not the word', () => { + renderCommand([{ value: 'calendar', label: 'Calendar', icon: 'calendar' }]); + expect(itemFor('Calendar').querySelector('svg.lucide-calendar')).not.toBeNull(); + expect(screen.queryByText('calendar')).toBeNull(); + }); + }); + + describe('the catalog fixtures', () => { + // Both fixtures are live specimens AND declared AI few-shot retrieval + // sources, so every name they ship must actually draw. + // `face-slightly-smiling` is the identity-derived live key for the retired + // `smile` that `command-menu.json` carried. + it('command-menu.json — draws a glyph for all six names it declares', () => { + renderCommand([ + { value: 'calendar', label: 'Calendar', icon: 'calendar' }, + { value: 'search', label: 'Search Emoji', icon: 'face-slightly-smiling' }, + { value: 'calculator', label: 'Calculator', icon: 'calculator' }, + { value: 'profile', label: 'Profile', icon: 'user' }, + { value: 'billing', label: 'Billing', icon: 'credit-card' }, + { value: 'settings', label: 'Settings', icon: 'settings' }, + ]); + for (const [label, name] of [ + ['Calendar', 'calendar'], + ['Search Emoji', 'face-slightly-smiling'], + ['Calculator', 'calculator'], + ['Profile', 'user'], + ['Billing', 'credit-card'], + ['Settings', 'settings'], + ]) { + expect( + itemFor(label).querySelector(`svg.lucide-${name}`), + `${label} should draw the ${name} glyph`, + ).not.toBeNull(); + } + }); + + it('command-palette-with-shortcuts.json — draws a glyph for all three names it declares', () => { + renderCommand([ + { value: 'new', label: 'New File', icon: 'file-plus' }, + { value: 'open', label: 'Open File', icon: 'folder-open' }, + { value: 'save', label: 'Save', icon: 'save' }, + ], 'File'); + for (const [label, name] of [ + ['New File', 'file-plus'], + ['Open File', 'folder-open'], + ['Save', 'save'], + ]) { + expect( + itemFor(label).querySelector(`svg.lucide-${name}`), + `${label} should draw the ${name} glyph`, + ).not.toBeNull(); + } + }); + }); +}); diff --git a/packages/components/src/renderers/data-display/breadcrumb.tsx b/packages/components/src/renderers/data-display/breadcrumb.tsx index ae910a8121..b219891b08 100644 --- a/packages/components/src/renderers/data-display/breadcrumb.tsx +++ b/packages/components/src/renderers/data-display/breadcrumb.tsx @@ -11,6 +11,18 @@ import type { BreadcrumbSchema } from '@object-ui/types'; import { Breadcrumb, BreadcrumbList, BreadcrumbItem, BreadcrumbLink, BreadcrumbPage, BreadcrumbSeparator } from '../../ui/breadcrumb'; import { renderChildren } from '../../lib/utils'; import { resolveKeyedI18nLabel } from '@object-ui/react'; +// `BreadcrumbItem.icon` is an authored lucide NAME that this renderer never +// read — the catalog fixture literally named `with-icons.json` drew no icons +// at all (objectui#5931). +// +// Routed through the RECORD surface (`icons` from 'lucide-react', reached via +// the shared `resolveIcon`), which is what `ui:button`, `action:*`, +// `ui:dropdown-menu` and `ui:context-menu` resolve against, so an unknown or +// RETIRED spelling renders NOTHING. 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'; ComponentRegistry.register('breadcrumb', ({ schema, ...props }: { schema: BreadcrumbSchema; [key: string]: any }) => { @@ -28,18 +40,28 @@ ComponentRegistry.register('breadcrumb', {...{ 'data-obj-id': dataObjId, 'data-obj-type': dataObjType, style }} > - {schema.items?.map((item, idx) => ( -
    - - {idx === (schema.items?.length || 0) - 1 ? ( - {resolveKeyedI18nLabel(item.label) ?? ''} - ) : ( - {resolveKeyedI18nLabel(item.label) ?? ''} - )} - - {idx < (schema.items?.length || 0) - 1 && } -
    - ))} + {schema.items?.map((item, idx) => { + const isLast = idx === (schema.items?.length || 0) - 1; + // Resolved ONCE per item and rendered ABOVE the page/link split, so + // BOTH arms carry it by construction. Repairing only the leaf arm + // would be "a narrower version of the same bug" (objectui#5930) — + // here the last crumb is a `BreadcrumbPage` and every earlier one a + // `BreadcrumbLink`, and the fixture exercises both. + const Icon = resolveIcon(item.icon); + return ( +
    + + {Icon && } + {isLast ? ( + {resolveKeyedI18nLabel(item.label) ?? ''} + ) : ( + {resolveKeyedI18nLabel(item.label) ?? ''} + )} + + {!isLast && } +
    + ); + })}
    ); diff --git a/packages/components/src/renderers/form/command.tsx b/packages/components/src/renderers/form/command.tsx index 82e8e5f3e3..6f5cbb8126 100644 --- a/packages/components/src/renderers/form/command.tsx +++ b/packages/components/src/renderers/form/command.tsx @@ -9,6 +9,18 @@ import { ComponentRegistry } from '@object-ui/core'; import type { CommandSchema } from '@object-ui/types'; import { Command, CommandInput, CommandList, CommandEmpty, CommandGroup, CommandItem } from '../../ui/command'; +// `CommandItem.icon` is an authored lucide NAME that this renderer never read — +// `command-menu.json` and `command-palette-with-shortcuts.json` declare nine of +// them between them and drew none (objectui#5931). +// +// Routed through the RECORD surface (`icons` from 'lucide-react', reached via +// the shared `resolveIcon`), which is what `ui:button`, `action:*`, +// `ui:dropdown-menu` and `ui:context-menu` resolve against, so an unknown or +// RETIRED spelling renders NOTHING. 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'; ComponentRegistry.register('command', ({ schema, ...props }: { schema: CommandSchema; [key: string]: any }) => { @@ -29,12 +41,20 @@ ComponentRegistry.register('command', {schema.emptyText || 'No results found.'} {schema.groups?.map((group, idx) => ( + // `CommandGroup` renders `heading` as a plain string and `CommandGroup` + // declares no `icon` of its own, so the item arm below is this + // component's ONLY arm that can carry one — read off the type and the + // renderer, not assumed. - {group.items?.map((item, itemIdx) => ( - - {item.label} - - ))} + {group.items?.map((item, itemIdx) => { + const Icon = resolveIcon(item.icon); + return ( + + {Icon && } + {item.label} + + ); + })} ))} diff --git a/scripts/__tests__/check-lucide-icon-record-names.test.ts b/scripts/__tests__/check-lucide-icon-record-names.test.ts index 088de104d4..b48ed9f730 100644 --- a/scripts/__tests__/check-lucide-icon-record-names.test.ts +++ b/scripts/__tests__/check-lucide-icon-record-names.test.ts @@ -304,15 +304,24 @@ describe('the `ui:icon` node type', () => { describe('a name whose resolver this gate cannot identify is declined, not flagged', () => { it('leaves the same retired spelling alone on an untyped and a non-censused node', () => { // Both shapes are live in this repository: Tailwind tone maps keyed `icon`, - // and catalog child items under `button-group`/`breadcrumb`/`command` - // (three of which never read `icon`, and a fourth that renders it as text). - // A gate that flagged these would be suppressed on day one, and then it - // would catch nothing at all. + // and catalog child items under a container that reaches no resolver — + // `button-group`, whose renderer still never reads `button.icon` and whose + // item type declares no such key (objectui#5931 routed that one for a + // decision rather than censusing it). A gate that flagged these would be + // suppressed on day one, and then it would catch nothing at all. + // + // ⚠️ The container in the JSON fixture below is deliberately one this + // repository's census does NOT declare descent for. `breadcrumb` and + // `command` used to serve here and no longer can: objectui#5931 wired both + // renderers through `resolveIcon`, so their child icons are now JUDGED. + // `judge`'s default table strips `descendants`, so this row would still be + // green with either — which is exactly why the name is chosen for what it + // MEANS and not for what currently passes. const result = judge('declined', { files: { 'packages/app/src/tones.ts': "export const tone = { icon: 'text-amber-500' };", 'packages/app/src/other.ts': "export const node = { type: 'text', icon: 'filter' };", - 'examples/catalog/items.json': JSON.stringify({ type: 'breadcrumb', items: [{ icon: 'layout' }] }, null, 2), + 'examples/catalog/items.json': JSON.stringify({ type: 'button-group', buttons: [{ icon: 'layout' }] }, null, 2), }, }); @@ -421,10 +430,13 @@ describe('an icon on an UNTYPED child of a container that declares descent', () it('does NOT leak descent into a container that never declared it', () => { // The reason `descendants` is opt-in per container rather than a blanket - // "nearest censused ancestor" rule: `breadcrumb`/`button-group`/`command` - // items were measured and none of their renderers reads `icon` at all. + // "nearest censused ancestor" rule: `button-group` items were measured and + // its renderer reads no `icon` at all, so a blanket rule would judge names + // that reach nothing. `breadcrumb`/`command` stood beside it here until + // objectui#5931 wired both through `resolveIcon` — a verdict is a fact + // about a renderer, and it expires when that renderer is repaired. const result = judge('descend-undeclared', { - files: { 'examples/catalog/crumbs.json': JSON.stringify({ type: 'breadcrumb', items: [{ icon: 'layout' }] }, null, 2) }, + files: { 'examples/catalog/buttons.json': JSON.stringify({ type: 'button-group', buttons: [{ icon: 'layout' }] }, null, 2) }, recordReadingTypes: DESCENT_TYPES, }); diff --git a/scripts/check-lucide-icon-record-names.mjs b/scripts/check-lucide-icon-record-names.mjs index 1a591a76c8..567000b120 100644 --- a/scripts/check-lucide-icon-record-names.mjs +++ b/scripts/check-lucide-icon-record-names.mjs @@ -72,7 +72,10 @@ * was low by 53. Re-measured, by reading each renderer (objectui#5992): * * 61 untyped `icon` names, across SEVEN containers. Exactly ONE of them - * reaches a record-reading resolver: + * reached a record-reading resolver AT THAT COMMIT — four do today, and + * each line below carries the card that moved it. A row's verdict is a + * fact about a renderer, so it expires when that renderer is repaired; + * "which containers reach the record" is re-read here, not remembered. * * dropdown-menu 3 RECORD — `resolveIcon(item.icon)` in * `renderers/overlay/dropdown-menu.tsx` (objectui#5930). @@ -82,11 +85,23 @@ * same call — a depth no single-level `items[].icon` * path can express. * button-group 8 `renderers/basic/button-group.tsx` never reads - * `button.icon` at all; the names render nothing - * breadcrumb 3 renderer never reads `icon` - * command 9 renderer never reads `icon` - * context-menu 4 renderer never reads `icon` — dropdown-menu's twin, - * and NOT routed by objectui#5930 + * `button.icon` at all; the names render nothing. + * STILL TRUE at objectui#5931, and deliberately so: + * `icon` is absent from `ButtonGroupButton` in + * `packages/types` AND from its zod mirror, so + * declaring it here would be censusing a key that is + * not authorable. Routed for a decision, not guessed. + * breadcrumb 3 RECORD, since objectui#5931 — `resolveIcon(item.icon)` + * in `renderers/data-display/breadcrumb.tsx`, resolved + * once per item ABOVE the page/link split so BOTH arms + * read it. JUDGED HERE. + * command 9 RECORD, since objectui#5931 — `resolveIcon(item.icon)` + * in `renderers/form/command.tsx`. JUDGED HERE, and + * necessarily by descent: the names sit at + * `groups[].items[].icon`, a depth no single-level + * `paths` entry can express. + * context-menu 4 RECORD, since objectui#6278 — dropdown-menu's twin, + * and NOT routed by objectui#5930. JUDGED HERE. * timeline 4 rendered as RAW TEXT, `{item.icon}`; * the four authored names are emoji, not lucide names * tree-view 30 read, but as a TWO-VALUED literal switch @@ -96,6 +111,9 @@ * imports `resolveIcon`, not `icons`, so the record read happens in * `renderers/action/resolve-icon.ts`, which is already declared. Part 1 * stays at eight resolvers; this is a part-2 rule, not a census change. + * `context-menu.tsx`, `breadcrumb.tsx` and `command.tsx` route the same way + * and are absent from part 1 for the same reason — part 1 knowing a MODULE + * and part 2 knowing a `type` are two independent facts. * * ── Re-taken at objectui@e3784607f, and UNCHANGED ────────────────────── * objectui#6009 censused the `icon` TYPE, which looks like it should move @@ -212,6 +230,35 @@ 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' }, + // objectui#5931. The container's OWN `icon` is never read (`paths: []`); its + // item icons sit on untyped children at `items[].icon`, and the renderer + // resolves each ONCE and renders the glyph ABOVE the `BreadcrumbPage` / + // `BreadcrumbLink` split, so both arms read the same call. Declared by + // `descendants` rather than `paths: ['items[].icon']`, which this walk can + // also express: only the descent form carries `min`, and a declaration that + // silently stops reaching its nodes is the failure this gate exists for. + // `min` is the MEASURED count (the three names `with-icons.json` authors), + // not the weaker `1` its two siblings below carry — a descent that reached + // only one of three would be as blind as one that reached none. + 'breadcrumb': { + paths: [], + descendants: true, + min: 3, + resolver: 'packages/components/src/renderers/action/resolve-icon.ts (via renderers/data-display/breadcrumb.tsx)', + }, + // objectui#5931, and the same shape one level deeper: the names sit at + // `groups[].items[].icon`, which `ARRAY_PATH` cannot express at all — it + // matches ONE level (`/^(\w+)\[\]\.icon$/`), so a `paths` entry here would + // extract nothing, report no violations and read exactly like a clean tree. + // Descent reaches them because an untyped node passes descent through to its + // own children. `min` is the measured count: six names in `command-menu.json` + // plus three in `command-palette-with-shortcuts.json`. + 'command': { + paths: [], + descendants: true, + min: 9, + resolver: 'packages/components/src/renderers/action/resolve-icon.ts (via renderers/form/command.tsx)', + }, // 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