refactor(components): resolve ui:button icons through the shared resolver

.changeset/5993-button-shared-icon-resolver.md

@@ -0,0 +1,44 @@
+---
+'@object-ui/components': patch
+---
+
+`ui:button` resolves its authored `icon` through the shared `resolveIcon` instead of a
+byte-equivalent copy of it (objectui#5993).
+
+`renderers/form/button.tsx` carried its own `toPascalCase`, its own `iconNameMap` holding
+the single `Home -> House` entry, and its own index into lucide's runtime `icons` record —
+the same algorithm as `renderers/action/resolve-icon.ts`, but not the same function. The
+`action:*` family, `complex/data-table.tsx` and both menu renderers already import the
+shared one. The hazard was drift, not rendering: an alias added to `resolve-icon.ts` to
+absorb a lucide retirement (the objectui#5586 / #5622 mechanism) reached every one of those
+sites and silently missed `ui:button`, which would have gone on resolving the retired
+spelling to nothing while the rest of the repo resolved it correctly.
+
+**No behaviour changes, and that is measured rather than asserted.** The two
+implementations were compared over 3547 names — every one of lucide's 1767 record keys in
+both spellings, plus kebab-case probes (`arrow-right`, `dollar-sign`, `user-plus`), the
+`Home` alias, retired spellings and `undefined`: 3539 identical by object identity, 8
+differing only in the nullish flavour returned for a miss (the copy indexed the record and
+got `undefined`; the shared resolver `?? null`s it), zero genuine forks. That one
+difference cannot reach the DOM — `Icon` is consumed at exactly two sites, both
+`{!isLoading && Icon && <Icon .../>}` truthiness tests, and React renders nothing for
+`null` and `undefined` alike. Icon identity, `h-4 w-4` sizing, `iconPosition`, the loading
+state and the `Loader2` spinner are unchanged, and are pinned by
+`renderers/form/__tests__/button-shared-icon-resolver.test.tsx`.
+
+Because behaviour is unchanged, the usual red-before ablation does not exist for this
+change and none was manufactured. The one row in that suite that discriminates is
+structural: it spies on the shared module and fails when the glyph does not come out of it,
+which is red on the copy and green on the import.
+
+`scripts/check-lucide-icon-record-names.mjs` drops `form/button.tsx` from
+`DECLARED_RECORD_READERS` in the same commit — that gate rediscovers record readers from
+source on every run and fails on drift in both directions, so the removal is verified by
+the gate rather than declared. It is also what now guards the dedupe: a re-inlined copy
+would be discovered as an undeclared record reader and fail. The census entry for the
+`button` *type* stays, its resolver re-pointed at `resolve-icon.ts`, so `ui:button`'s
+authored icon names are still judged against the live record.
+
+`renderers/basic/icon.tsx` keeps its own copy deliberately and is untouched: `ui:icon`
+draws a `SquareDashed` placeholder and warns on an unresolvable name (objectui#5631), which
+the shared resolver does not do.

packages/components/src/renderers/form/__tests__/button-shared-icon-resolver.test.tsx

@@ -0,0 +1,196 @@
+/**
+ * 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:button` resolves its authored `icon` through the SHARED resolver
+ * (objectui#5993).
+ *
+ * ## What was on the tree, and what this suite can and cannot prove
+ *
+ * `renderers/form/button.tsx` carried a byte-equivalent reimplementation of
+ * `renderers/action/resolve-icon.ts` — its own `toPascalCase`, its own
+ * `iconNameMap` with the single `Home -> House` entry, its own index into
+ * lucide's runtime `icons` record. Same algorithm, not the same function.
+ *
+ * ⚠️ The two implementations were behaviourally EQUIVALENT, so this suite must
+ * not pretend otherwise. Measured over 3547 names before the dedupe (every one
+ * of lucide's 1767 record keys in both spellings, plus kebab-case probes, the
+ * `Home` alias, retired spellings and `undefined`): 3539 identical by object
+ * identity, 8 differing ONLY in the nullish flavour returned for a miss (the
+ * copy indexed the record and got `undefined`; the shared resolver `?? null`s
+ * it), and ZERO genuine forks. `Icon` is consumed at exactly two sites, both
+ * `{!isLoading && Icon && <Icon .../>}` truthiness tests, so that one
+ * difference cannot reach the DOM.
+ *
+ * That has a consequence for how these rows read, and it is stated here rather
+ * than left for a reviewer to discover:
+ *
+ *   - The BEHAVIOUR rows below are GREEN IN BOTH WORLDS. They are not evidence
+ *     that the dedupe is correct — they are the guard that it changed nothing
+ *     (icon identity, size, `iconPosition`, the loading state). A green run
+ *     here proves NO-CHANGE, which is the whole contract of this card.
+ *   - Exactly one row DISCRIMINATES, and it is a structural one: the ROUTING
+ *     row. Before the dedupe `resolveIcon` was never called by this renderer,
+ *     so the spy below records zero calls and that row is RED. It is the only
+ *     honest red-before this card has, and it pins the thing that actually
+ *     changed: which function the glyph came out of.
+ *
+ * ## Why the shared module is spied rather than replaced
+ *
+ * The factory delegates to `importOriginal`, so every behaviour row still runs
+ * the REAL resolver against the REAL lucide record. A stub returning a fixed
+ * component would have deleted 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). Only the CALL is observed.
+ *
+ * ## Why the renderer is invoked DIRECTLY
+ *
+ * `ComponentRegistry.get('button')` 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).
+ */
+
+import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
+import { render, screen, cleanup } from '@testing-library/react';
+import { ComponentRegistry } from '@object-ui/core';
+
+vi.mock('../../action/resolve-icon', async (importOriginal) => {
+  const actual = await importOriginal<typeof import('../../action/resolve-icon')>();
+  return { resolveIcon: vi.fn(actual.resolveIcon) };
+});
+
+import { resolveIcon } from '../../action/resolve-icon';
+// Module scope, not `beforeAll` (objectui#3010/#3021).
+import '../../../renderers';
+
+const shared = vi.mocked(resolveIcon);
+
+beforeEach(() => shared.mockClear());
+afterEach(() => cleanup());
+
+/**
+ * DOM ORDER inside the button, read off `childNodes`.
+ *
+ * ⚠️ NOT `compareDocumentPosition` against `screen.getByText('Go')`: the label
+ * is a bare TEXT NODE, so that query returns its closest ELEMENT — the
+ * `<button>` itself — and the comparison then answers `CONTAINS`, which is
+ * true whichever side the glyph sits on. Measured: it returned 10
+ * (`PRECEDING|CONTAINS`) for BOTH positions.
+ */
+function indexIn(button: HTMLElement, node: Node): number {
+  return Array.prototype.indexOf.call(button.childNodes, node);
+}
+
+function labelIndexIn(button: HTMLElement): number {
+  const at = Array.prototype.findIndex.call(
+    button.childNodes,
+    (n: Node) => n.nodeType === Node.TEXT_NODE && n.textContent === 'Go',
+  );
+  if (at < 0) throw new Error('the label text node is not a direct child of the button');
+  return at;
+}
+
+function renderButton(schema: Record<string, unknown>): HTMLElement {
+  const B = ComponentRegistry.get('button') as React.ComponentType<{ schema: unknown }>;
+  const { container } = render(<B schema={{ type: 'button', label: 'Go', ...schema }} />);
+  return container.querySelector('button') as HTMLElement;
+}
+
+describe('ui:button icon resolution (objectui#5993)', () => {
+  describe('harness control', () => {
+    // Without this, every "renders no glyph" row could pass against a button
+    // that failed to render at all.
+    it('renders the button and its label', () => {
+      const button = renderButton({ icon: 'arrow-right' });
+      expect(button).not.toBeNull();
+      expect(screen.getByText('Go')).toBeTruthy();
+    });
+  });
+
+  describe('routing — the only row that discriminates', () => {
+    it('resolves the authored name through the SHARED resolver', () => {
+      // RED before the dedupe: this renderer carried its own copy and never
+      // called this function, so the spy recorded nothing.
+      renderButton({ icon: 'arrow-right' });
+      expect(shared).toHaveBeenCalledWith('arrow-right');
+    });
+
+    it('renders the glyph the shared resolver returned, not one of its own', () => {
+      const button = renderButton({ icon: 'arrow-right' });
+      // `toHaveBeenCalledTimes` FIRST, deliberately. Reading
+      // `mock.results[0]?.value` straight away is a BLIND instrument: with zero
+      // calls it is `undefined`, and `expect(undefined).not.toBeNull()` passes
+      // — measured green against the restored copy before this line was added.
+      expect(shared).toHaveBeenCalledTimes(1);
+      expect(shared.mock.results[0].value).not.toBeNull();
+      expect(button.querySelector('svg.lucide-arrow-right')).not.toBeNull();
+    });
+  });
+
+  describe('behaviour — GREEN IN BOTH WORLDS, pinning that nothing moved', () => {
+    it('resolves a kebab-case name to its PascalCase glyph', () => {
+      expect(renderButton({ icon: 'arrow-right' }).querySelector('svg.lucide-arrow-right')).not.toBeNull();
+    });
+
+    it('resolves `home` through the RENAME alias to House, not to a dead `home`', () => {
+      // The single entry both maps carried. It is the one input whose answer
+      // would have changed had the dedupe dropped the alias on the floor.
+      const button = renderButton({ icon: 'home' });
+      expect(button.querySelector('svg.lucide-house')).not.toBeNull();
+      expect(button.querySelector('svg.lucide-home')).toBeNull();
+    });
+
+    it('renders NO glyph for a retired spelling — the RECORD surface, not a fallback', () => {
+      // `edit` still imports and still renders as a COMPONENT; its key is gone
+      // from the runtime record. Rules out `LazyIcon`, which degrades an
+      // unknown name to `Database`.
+      const button = renderButton({ icon: 'edit' });
+      expect(button.querySelector('svg')).toBeNull();
+      expect(screen.getByText('Go')).toBeTruthy();
+    });
+
+    it('renders NO glyph for an unresolvable name, and no placeholder either', () => {
+      // ⚠️ NOT the `ui:icon` contract. That renderer draws a `SquareDashed`
+      // placeholder and warns (objectui#5631, pinned by
+      // `basic/__tests__/icon-unresolvable-placeholder.test.tsx`).
+      // `ui:button` draws nothing, before and after this card.
+      const button = renderButton({ icon: 'definitely-not-a-lucide-icon' });
+      expect(button.querySelector('svg')).toBeNull();
+    });
+
+    it('renders no glyph when no icon is authored', () => {
+      expect(renderButton({}).querySelector('svg')).toBeNull();
+    });
+
+    it('places the glyph BEFORE the label by default, at h-4 w-4', () => {
+      const button = renderButton({ icon: 'arrow-right' });
+      const glyph = button.querySelector('svg.lucide-arrow-right')!;
+      expect(glyph.getAttribute('class')).toContain('mr-2');
+      expect(glyph.getAttribute('class')).toContain('h-4');
+      expect(glyph.getAttribute('class')).toContain('w-4');
+      expect(indexIn(button, glyph)).toBeLessThan(labelIndexIn(button));
+    });
+
+    it('places the glyph AFTER the label when iconPosition is right', () => {
+      const button = renderButton({ icon: 'arrow-right', iconPosition: 'right' });
+      const glyph = button.querySelector('svg.lucide-arrow-right')!;
+      expect(glyph.getAttribute('class')).toContain('ml-2');
+      expect(glyph.getAttribute('class')).toContain('h-4');
+      expect(glyph.getAttribute('class')).toContain('w-4');
+      expect(indexIn(button, glyph)).toBeGreaterThan(labelIndexIn(button));
+    });
+
+    it('swaps the glyph for the spinner while loading', () => {
+      const button = renderButton({ icon: 'arrow-right', loading: true });
+      expect(button.querySelector('svg.animate-spin')).not.toBeNull();
+      expect(button.querySelector('svg.lucide-arrow-right')).toBeNull();
+      expect(button.hasAttribute('disabled')).toBe(true);
+    });
+  });
+});

packages/components/src/renderers/form/button.tsx

@@ -11,21 +11,9 @@ import type { ButtonSchema } from '@object-ui/types';
 import { Button } from '../../ui';
 import { renderChildren } from '../../lib/utils';
 import { forwardRef } from 'react';
-import { Loader2, icons, type LucideIcon } from 'lucide-react';
+import { Loader2 } from 'lucide-react';
 import { toFormControlDomProps } from '../../lib/form-control-dom-props';
-
-// Helper to convert icon names to PascalCase (e.g., "arrow-right" -> "ArrowRight")
-function toPascalCase(str: string): string {
-  return str
-    .split('-')
-    .map(word => word.charAt(0).toUpperCase() + word.slice(1))
-    .join('');
-}
-
-// Map of renamed icons in lucide-react
-const iconNameMap: Record<string, string> = {
-  'Home': 'House',
-};
+import { resolveIcon } from '../action/resolve-icon';
 
 // Index signature on the parameter annotation, not on the `forwardRef` type
 // argument — mechanism note on `action:bar` (objectui#4422), pinned by
@@ -40,13 +28,12 @@ const ButtonRenderer = forwardRef<HTMLButtonElement, { schema: ButtonSchema }>(
         ...buttonProps 
     } = props;
 
-    // Resolve icon
-    let Icon: LucideIcon | null = null;
-    if (schema.icon) {
-      const iconName = toPascalCase(schema.icon);
-      const mappedIconName = iconNameMap[iconName] || iconName;
-      Icon = (icons as any)[mappedIconName] as LucideIcon;
-    }
+    // Resolve the icon through the SHARED resolver (objectui#5993). This file
+    // used to carry its own `toPascalCase` + `iconNameMap` + `icons` index — the
+    // same algorithm, but not the same function, so an alias added to
+    // `resolve-icon.ts` to absorb a lucide retirement (objectui#5586, #5622)
+    // reached every `action:*` site and silently missed `ui:button`.
+    const Icon = resolveIcon(schema.icon);
 
     // Determine loading state
     const isLoading = schema.loading || props.loading;

scripts/__tests__/check-lucide-icon-record-names.test.ts

@@ -598,7 +598,13 @@ describe('this repository', () => {
     // fresh `icons` import, so `renderers/overlay/dropdown-menu.tsx` correctly
     // stays OUT of the record-reader census. A descent declaration that moved
     // this number would have altered the wrong part of the gate.
-    expect(repoResult.discovered.record).toHaveLength(8);
+    //
+    // The figure was 8 until objectui#5993 deduped `renderers/form/button.tsx`
+    // onto the shared `resolveIcon`. That is the ONE way this number is allowed
+    // to move: a site stopped reading the record. It did not stop resolving
+    // icons — `RECORD_READING_TYPES['button']` still judges its authored names,
+    // one indirection away, exactly like the two menu entries this row is about.
+    expect(repoResult.discovered.record).toHaveLength(7);
     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
@@ -623,17 +629,29 @@ describe('this repository', () => {
     // objectui#5633's own discovery run. objectui#6009 supplies only the part-2
     // fact — which `type` sends names there — so this count must not move.
     expect(repoResult.discovered.record).toContain('packages/components/src/renderers/basic/icon.tsx');
-    expect(repoResult.discovered.record).toHaveLength(8);
+    // 7 since objectui#5993, for the reason the row above spells out. `ui:icon`
+    // keeps its own resolver deliberately — it draws a `SquareDashed`
+    // placeholder and warns where the shared one returns `null` (objectui#5631,
+    // pinned by `basic/__tests__/icon-unresolvable-placeholder.test.tsx`), so it
+    // is NOT a dedupe candidate and stays in this census.
+    expect(repoResult.discovered.record).toHaveLength(7);
   });
 
   it('carries more record-reading resolvers than objectui#5633 catalogued by hand', () => {
     // The card's table listed four. Discovery found eight, which is the whole
     // argument for measuring the population instead of maintaining a list: the
     // four it missed each resolve authored strings through the same record.
+    //
+    // Three of those four are still here. The fourth — `renderers/form/button.tsx`
+    // — was a hand-copied reimplementation of `resolve-icon.ts`, and objectui#5993
+    // deduped it onto the shared function, so it left this census by being FIXED
+    // rather than by being forgotten. Discovery is what proves that: the equality
+    // below fails if it comes back undeclared, and the census-drift check in the
+    // gate fails if the declaration outlives the read.
     expect(repoResult.discovered.record).toEqual([...DECLARED_RECORD_READERS].sort());
-    expect(repoResult.discovered.record.length).toBeGreaterThanOrEqual(8);
+    expect(repoResult.discovered.record.length).toBeGreaterThanOrEqual(7);
+    expect(repoResult.discovered.record).not.toContain('packages/components/src/renderers/form/button.tsx');
     for (const late of [
-      'packages/components/src/renderers/form/button.tsx',
       'packages/plugin-list/src/ListView.tsx',
       'packages/plugin-detail/src/RelatedList.tsx',
       'packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx',

scripts/check-lucide-icon-record-names.mjs

@@ -158,7 +158,6 @@ export const DECLARED_RECORD_READERS = [
   'packages/app-shell/src/views/metadata-admin/previews/ActionPreview.tsx',
   'packages/components/src/renderers/action/resolve-icon.ts',
   'packages/components/src/renderers/basic/icon.tsx',
-  'packages/components/src/renderers/form/button.tsx',
   'packages/plugin-detail/src/RelatedList.tsx',
   'packages/plugin-list/src/ListView.tsx',
   'packages/plugin-list/src/components/TabBar.tsx',
@@ -199,7 +198,15 @@ export const SCAN_ROOTS = ['packages', 'apps', 'examples'];
 // descent that reaches NOTHING reports no violations and reads exactly like a
 // clean tree.
 export const RECORD_READING_TYPES = {
-  'button': { paths: ['icon'], resolver: 'packages/components/src/renderers/form/button.tsx' },
+  // `form/button.tsx` no longer reads the record itself: it imports the shared
+  // `resolveIcon` (objectui#5993), which is why it left part 1's census in the
+  // same commit. The `type` stays judged — its names still reach a
+  // record-reading resolver, just one indirection away, exactly like the
+  // `context-menu` and `dropdown-menu` entries below.
+  'button': {
+    paths: ['icon'],
+    resolver: 'packages/components/src/renderers/action/resolve-icon.ts (via renderers/form/button.tsx)',
+  },
   'action:bar': { paths: ['actions[].icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' },
   'action:button': { paths: ['icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' },
   'action:group': { paths: ['icon', 'actions[].icon'], resolver: 'packages/components/src/renderers/action/resolve-icon.ts' },