diff --git a/.changeset/5631-ui-icon-visible-placeholder.md b/.changeset/5631-ui-icon-visible-placeholder.md new file mode 100644 index 0000000000..0e64171ad8 --- /dev/null +++ b/.changeset/5631-ui-icon-visible-placeholder.md @@ -0,0 +1,25 @@ +--- +'@object-ui/components': minor +--- + +`ui:icon`: an unresolvable glyph now renders a visible placeholder instead of nothing + +An icon whose name does not resolve to a lucide glyph used to `return null`. +That failed silently in two independent ways at once: invisible to a human (no +gap, no error boundary — just an absent glyph), and clean-looking to a gate (a +renderer that returns `null` spreads no attributes, so a DOM scan of it reports +no findings). + +It now renders a dashed-square placeholder on the same SVG host, keeping the +authored `className`, `size` and colour so the gap sits exactly where the icon +would have been, with `role="img"`, an accessible name identifying the icon +that failed, and a `data-objectui-icon-unresolved` marker. The `console.warn` +stays and now names the cause. + +Also fixed: a node with no `name` at all reached `toPascalCase(undefined)` and +threw, which the error boundary then swallowed — a third silent failure. It +renders the placeholder too. + +Not included: `ui:icon` still reads the SDUI identity key `name` as its glyph +name. Moving it to `schema.icon` is ruled but blocked on an authored-metadata +migration — see objectui#5631. diff --git a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx index 141d6ac708..a93b37de81 100644 --- a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx +++ b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx @@ -149,7 +149,19 @@ * * - 12 rendered no element at all: the overlays are closed until * `defaultOpen`, `action:*` return `null` with no actions, and `ui:icon` - * returns `null` because the canary node's `name` is not a lucide icon. + * returned `null` because the canary node's `name` is not a lucide icon. + * + * ⚠️ `ui:icon` is the one of those twelve that has since been FIXED at the + * renderer rather than worked around here (objectui#5631). It used to need + * a forced `schemaExtras: { name: 'check' }` to render at all; it now + * renders a visible placeholder for an unresolvable glyph, so it is swept + * as an ordinary plain target on the node this file actually authors — + * identity `name: 'canary_node'` and nothing else. Its + * {@link BARE_SPREAD_ON_SVG} row was re-measured on that node as the + * ruling required and is UNCHANGED: the placeholder is the same bare + * spread onto the same SVG host, so it leaks the same fourteen. That the + * row did not move is the point — the reading no longer depends on a + * workaround that hid whether the renderer rendered. * - 4 threw `useSidebar must be used within a SidebarProvider` and were * caught by `SchemaErrorBoundary`, whose markup is attribute-clean. * @@ -649,7 +661,8 @@ const COMPONENTS_PLAIN_TYPES: readonly string[] = [ 'ui:date-picker', 'ui:dd', 'ui:del', 'ui:div', 'ui:dl', 'ui:dt', 'ui:em', 'ui:email', 'ui:empty', 'ui:figcaption', 'ui:figure', 'ui:file-upload', 'ui:filter-builder', 'ui:flex', 'ui:footer', 'ui:form', 'ui:grid', 'ui:h1', 'ui:h2', 'ui:h3', 'ui:h4', 'ui:h5', 'ui:h6', - 'ui:header', 'ui:home', 'ui:hr', 'ui:html', 'ui:i', 'ui:image', 'ui:img', 'ui:input', + 'ui:header', 'ui:home', 'ui:hr', 'ui:html', 'ui:i', 'ui:icon', 'ui:image', 'ui:img', + 'ui:input', 'ui:input-otp', 'ui:ins', 'ui:kbd', 'ui:label', 'ui:li', 'ui:list', 'ui:loading', 'ui:main', 'ui:mark', 'ui:menubar', 'ui:nav', 'ui:navigation-menu', 'ui:ol', 'ui:p', 'ui:page', 'ui:pagination', 'ui:password', 'ui:pre', 'ui:progress', 'ui:q', @@ -673,11 +686,6 @@ const COMPONENTS_SPECIAL_TARGETS: readonly Target[] = [ componentsTarget('ui:dropdown-menu', { ...OPEN_OVERLAY, items: [{ label: 'a', value: 'a' }] }), componentsTarget('ui:hover-card', OPEN_OVERLAY), componentsTarget('ui:tooltip', { trigger: CLEAN_SLOT, content: 'tip' }, '[data-state="closed"]'), - // `IconRenderer` returns `null` when `schema.name` is not a lucide icon, and - // the canary node's `name` is `canary_node` — so the default node rendered - // NOTHING and read clean. (That collision is itself worth knowing: this - // renderer reads the SDUI identity key `name` as an icon name.) - componentsTarget('ui:icon', { name: 'check' }), // `action:*` return `null` with no actions (see CANARY_ACTIONS). componentsTarget('action:bar', { actions: CANARY_ACTIONS }), componentsTarget('action:group', { actions: CANARY_ACTIONS }), @@ -977,6 +985,13 @@ const BARE_SPREAD_MINUS_NAME: readonly string[] = [ * camelCase canaries survive exactly as authored (`ariaLabel`, not * `arialabel`). A ledger keyed on the lowercased spelling would have silently * failed to match these two. + * + * `ui:icon`'s membership here was re-measured under objectui#5631, on the + * ordinary canary node rather than the forced-resolvable one the old entry + * needed, and came back identical — see the `ui:icon` note in this file's + * "four phantom cleans" section. `name` stays in this list: the renderer still + * spreads the authored identity onto the SVG, and closing that is the + * objectui#5632 burn-down, deliberately NOT folded in here. */ const BARE_SPREAD_ON_SVG: readonly string[] = [ 'ariaDescribedBy', 'ariaLabel', 'bind', 'colorVariant', 'dataSource', 'events', 'name', diff --git a/packages/components/src/renderers/basic/__tests__/icon-unresolvable-placeholder.test.tsx b/packages/components/src/renderers/basic/__tests__/icon-unresolvable-placeholder.test.tsx new file mode 100644 index 0000000000..26a92531e7 --- /dev/null +++ b/packages/components/src/renderers/basic/__tests__/icon-unresolvable-placeholder.test.tsx @@ -0,0 +1,176 @@ +/** + * 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:icon` — an unresolvable glyph renders a VISIBLE placeholder, never + * nothing (objectui#5631, maintainer ruling 2026-08-22 comment 5380754137: + * "an unresolvable icon renders a visible placeholder instead of `null`, + * **regardless** of the key question"). + * + * ## What this file pins, and what it deliberately does not + * + * It pins the END OF THE SILENCE — item 3 of the ruling, which is unconditional + * and independent of which schema key names the glyph. It does NOT pin + * `schema.icon` as the glyph key (item 1): that migration is blocked on a + * measured corpus, and the PR body carries the reading. So every case below + * still authors `name`, exactly as the renderer still reads it. + * + * ## Why the warning is asserted through an explicit spy + * + * Vitest 4 runs with `silent: 'passed-only'`, so `console.warn` output from a + * PASSING test is discarded — it never reaches stderr for a human to notice. + * Observing the warning by eye is therefore not available here, and a test + * that "checked" it by reading output would check nothing. `vi.spyOn` captures + * the call regardless of what the reporter prints. That property is not + * incidental: an unread `console.warn` is half of why objectui#5631 stayed + * invisible, so the warning is pinned as behaviour rather than trusted. + * + * ## Why the assertions are on the RENDERED RESULT, not on `!== null` + * + * The pre-fix branch returned `null`. A test asserting `container.firstChild` + * is non-null would pass against a placeholder that rendered an empty + * `` — invisible to a human and attribute-clean to the DOM-leak sweep, + * i.e. the same defect wearing a different shape. Each case below resolves + * something a person or a gate could actually see: a real `svg` host, the + * authored box (`className`/size) still applied to it, an accessible name, and + * the marker attribute. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, screen, cleanup } from '@testing-library/react'; +import { SchemaRenderer } from '@object-ui/react'; +// Registers `ui:icon` at module scope, not in a hook +// (object-ui/no-dynamic-import-in-test-hook, objectui#3010). +import '../../../renderers'; + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +function renderIcon(schema: Record) { + const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); + const { container } = render(); + return { container, warn }; +} + +/** The marker the placeholder branch puts on its host, for gates and for this file. */ +const MARKER = '[data-objectui-icon-unresolved]'; + +describe('ui:icon — unresolvable glyph', () => { + it('renders a visible SVG placeholder instead of nothing', () => { + const { container } = renderIcon({ name: 'definitely-not-a-lucide-icon' }); + + const placeholder = container.querySelector(MARKER); + expect(placeholder).not.toBeNull(); + // The host is a real SVG, the same element kind a resolved glyph renders, + // so the gap occupies the layout slot the icon would have occupied. + expect(placeholder?.tagName.toLowerCase()).toBe('svg'); + expect(placeholder?.getAttribute('data-objectui-icon-unresolved')).toBe( + 'definitely-not-a-lucide-icon', + ); + }); + + it('names the unresolved icon in its accessible name', () => { + renderIcon({ name: 'definitely-not-a-lucide-icon' }); + + // `role="img"` + `aria-label`: the placeholder is perceivable, and it says + // WHICH icon failed rather than being an anonymous box. + expect( + screen.getByRole('img', { name: 'Unresolved icon: definitely-not-a-lucide-icon' }), + ).toBeTruthy(); + }); + + it('warns, naming the identity-key collision that is objectui#5631', () => { + const { warn } = renderIcon({ name: 'save_icon' }); + + // Not a pinned call COUNT: React invokes the render function more than + // once here (measured: two calls for one `render`), and pinning the number + // would make this file fail on a StrictMode change rather than on the + // behaviour it is about. What matters is that the warning happens and what + // it says. + expect(warn).toHaveBeenCalled(); + const message = String(warn.mock.calls[0]?.[0]); + expect(message).toContain('save_icon'); + expect(message).toContain('objectui#5631'); + // The warning must say why an ordinary authored identity lands here — the + // reader of this warning is an author who wrote `name: 'save_icon'` and is + // looking at a placeholder. + expect(message).toContain('identity key'); + }); + + it('keeps the authored box: className and size still reach the placeholder', () => { + const { container } = renderIcon({ + name: 'definitely-not-a-lucide-icon', + className: 'text-red-500', + size: 48, + }); + + const placeholder = container.querySelector(MARKER) as SVGElement | null; + expect(placeholder).not.toBeNull(); + // Without this the placeholder would collapse to lucide's default box and + // the gap would not sit where the author put the icon. + expect(placeholder?.getAttribute('class')).toContain('text-red-500'); + expect(placeholder?.getAttribute('style')).toContain('48px'); + }); + + it('renders the placeholder — not a thrown error — when `name` is absent entirely', () => { + // Pre-fix this reached `toPascalCase(undefined)` and threw on + // `undefined.split`, which the SchemaErrorBoundary then swallowed: a THIRD + // way for this renderer to fail without saying so. `name` is typed + // `string` on `IconSchema`, but it arrives from authored JSON. + const { container, warn } = renderIcon({ id: 'no_name_node' }); + + const placeholder = container.querySelector(MARKER); + expect(placeholder).not.toBeNull(); + expect(placeholder?.getAttribute('data-objectui-icon-unresolved')).toBe('(none)'); + expect(String(warn.mock.calls[0]?.[0])).toContain('an absent icon name'); + }); +}); + +describe('ui:icon — resolvable glyph is untouched by objectui#5631', () => { + it('renders the real lucide glyph with no placeholder and no warning', () => { + const { container, warn } = renderIcon({ name: 'check', className: 'text-green-500' }); + + expect(container.querySelector(MARKER)).toBeNull(); + const svg = container.querySelector('svg'); + expect(svg).not.toBeNull(); + expect(svg?.getAttribute('class')).toContain('text-green-500'); + expect(warn).not.toHaveBeenCalled(); + }); + + it('still resolves the kebab-case and renamed-icon paths', () => { + // `home` -> `Home` -> mapped to `House`: the `iconNameMap` hop, which the + // placeholder branch must not have short-circuited. + const { container, warn } = renderIcon({ name: 'home' }); + + expect(container.querySelector(MARKER)).toBeNull(); + expect(container.querySelector('svg')).not.toBeNull(); + expect(warn).not.toHaveBeenCalled(); + }); +}); + +describe('the placeholder glyph itself resolves (objectui#5622 mechanism)', () => { + it('is imported by name, so it cannot silently become another `null`', async () => { + // The failure this guards: lucide retires a spelling by dropping it from + // the runtime `icons` record while KEEPING the deprecated named export. + // A placeholder looked up in that record could therefore resolve to + // `undefined` and render nothing — objectui#5631 again, one level up. + // Measured on lucide-react 1.31.0: `CircleHelp` and `HelpCircle` are both + // ABSENT from the record while both resolve as named exports, so this is + // not a hypothetical hazard. + const lucide = await import('lucide-react'); + + expect(typeof (lucide as Record).SquareDashed).not.toBe('undefined'); + // The record-based lookup this file's renderer uses for AUTHORED names is + // exactly what the placeholder must not depend on. Pinned so that a future + // edit swapping the named import for `icons[...]` has to face this case. + expect( + Object.prototype.hasOwnProperty.call(lucide.icons, 'CircleHelp') + || Object.prototype.hasOwnProperty.call(lucide.icons, 'HelpCircle'), + ).toBe(false); + }); +}); diff --git a/packages/components/src/renderers/basic/icon.tsx b/packages/components/src/renderers/basic/icon.tsx index 0b029d7fd9..1aeb518591 100644 --- a/packages/components/src/renderers/basic/icon.tsx +++ b/packages/components/src/renderers/basic/icon.tsx @@ -8,7 +8,7 @@ import { ComponentRegistry } from '@object-ui/core'; import type { IconSchema } from '@object-ui/types'; -import { icons } from 'lucide-react'; +import { icons, SquareDashed } from 'lucide-react'; import React, { forwardRef } from 'react'; import { cn } from '../../lib/utils'; @@ -26,6 +26,49 @@ const iconNameMap: Record = { 'Home': 'House', // "Home" was renamed to "House" in lucide-react's icons object }; +/** + * The glyph rendered when the requested one does not resolve (objectui#5631). + * + * ## Why a placeholder at all + * + * This branch used to `return null`. That made an unresolvable icon invisible + * in two independent ways at once, which is the whole reason objectui#5631 + * existed long enough to be found by accident: + * + * - **Invisible to a human.** Nothing rendered. No error boundary, no gap + * that reads as broken — just an absent glyph a reviewer's eye completes. + * - **Invisible to a gate.** A renderer that returns `null` spreads no + * attributes, so the DOM-leak sweep in + * `packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx` scanned + * an empty tree and reported no findings. An empty scan and a clean scan + * are the same reading. `ui:icon` was one of twelve targets that read clean + * for exactly that reason, and it only started reporting its fourteen + * leaked attributes once the sweep forced a resolvable name onto it. + * + * The maintainer ruling of 2026-08-22 (issue #5631, comment 5380754137) makes + * ending that silence unconditional — it holds "regardless of the key + * question", i.e. independent of *which* schema key names the glyph. + * + * ## Why a NAMED IMPORT and not a lookup in the `icons` record + * + * A placeholder that itself fails to resolve is the original bug again, one + * level up, and silent in the same way. Every other lookup in this file goes + * through lucide's runtime `icons` record, and lucide retires a spelling by + * dropping it from that record while keeping the deprecated named export — + * the objectui#5622 mechanism. Measured on the installed lucide-react 1.31.0: + * `CircleHelp` and `HelpCircle`, the two obvious "unknown" glyphs, are BOTH + * absent from the `icons` record while both still resolve as named exports. + * Either one looked up the usual way would have rendered nothing. + * + * A direct named import removes the failure mode instead of dodging it: it is + * resolved at build time, so if lucide ever retires this spelling the build + * fails loudly rather than the placeholder silently becoming another `null`. + * `SquareDashed` is currently both a named export and present in the record, + * and it is used DIRECTLY in the placeholder branch below rather than through a + * module-scope alias — an alias reads to `react-refresh/only-export-components` + * as a second component declaration in a file that exports none. + */ + // Index signature on the parameter annotation, not on the `forwardRef` type // argument — mechanism note on `action:bar` (objectui#4422), pinned by // `__tests__/forwardref-props-annotation.guard.test.ts`. @@ -38,28 +81,65 @@ const IconRenderer = forwardRef "${mappedIconName}"` : ''}) not found in lucide-react`); - return null; - } - + // Build size style const sizeStyle = schema.size ? { width: schema.size, height: schema.size } : undefined; - + // Merge classNames: schema color, schema className, prop className const mergedClassName = cn( schema.color, schema.className, className ); - + + // ⚠️ This renderer still reads the SDUI IDENTITY key `name` as its glyph + // name. That collision IS objectui#5631, and the ruling's answer to it is + // `schema.icon` — but the migration is not landed here; see the + // `inputs` note on the registration below for what is still owed and why. + // + // `schema.name` is typed `string` but arrives from authored JSON, so it can + // be absent at runtime. It used to reach `toPascalCase` unguarded, where + // `undefined.split` threw and the SchemaErrorBoundary swallowed it — a + // third way for this renderer to fail without saying so. + const requested = typeof schema.name === 'string' ? schema.name : ''; + // Convert icon name to PascalCase for Lucide lookup + const iconName = toPascalCase(requested); + // Apply icon name mapping for renamed icons + const mappedIconName = iconNameMap[iconName] || iconName; + const Icon = requested ? (icons as any)[mappedIconName] : undefined; + + if (!Icon) { + console.warn( + `ui:icon: no lucide glyph resolves for ${requested ? `"${requested}"` : 'an absent icon name'}` + + `${requested ? ` (lookup: "${iconName}"${mappedIconName !== iconName ? ` -> "${mappedIconName}"` : ''})` : ''}. ` + + `Rendering a visible placeholder instead of nothing (objectui#5631). ` + + `Note: this renderer reads the SDUI identity key \`name\` as its glyph name, ` + + `so an ordinary authored identity such as "save_icon" lands here.` + ); + + // Same host element and the same authored box as a resolved icon, so the + // gap is visible exactly where the glyph would have been. `role`/ + // `aria-label` sit BEFORE the spread so an author can still override + // them; the marker attribute sits AFTER it so nothing can clobber the + // one hook a gate uses to find this branch. + return ( + + ); + } + return (