Uh oh!
There was an error while loading. Please reload this page.
refactor(components): resolve ui:button icons through the shared resolver - #6350
Merged
os-support-ai merged 1 commit intoAug 25, 2026
Merged
Conversation
…lver `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, so an alias added to the shared resolver to absorb a lucide retirement reached every `action:*` site and silently missed `ui:button` (objectui#5993). Behaviour is unchanged, and measured rather than assumed: over 3547 names (lucide's 1767 record keys in both spellings, plus kebab-case probes, the `Home` alias, retired spellings and `undefined`) the two implementations agree by object identity on 3539, differ on 8 only in the nullish flavour returned for a miss (`undefined` from the record index vs `null` from the shared resolver), and fork on none. `Icon` is consumed at exactly two truthiness sites, so that difference cannot reach the DOM. `scripts/check-lucide-icon-record-names.mjs` drops the file from `DECLARED_RECORD_READERS` in the same commit: that census is rediscovered from source on every run and fails on drift in both directions, which both verifies the removal and makes a re-inlined copy fail the gate. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011SfZeFWrhGLHmfq61xbz4q
Contributor
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
This was referenced Aug 25, 2026
os-support-ai
marked this pull request as ready for review
August 25, 2026 14:59
Uh oh!
There was an error while loading. Please reload this page.
os-support-ai
deleted the
claude/issue-5993-button-inlined-icon-resolver
branch
August 25, 2026 15:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes#5993
ui:buttonresolved its authorediconwith a hand-copied reimplementation of the sharedresolver. It now imports
resolveIconfromrenderers/action/resolve-icon.ts, and thecopy is gone.
This is a duplication repair, not a rendering fix. Nothing a user sees changes — that
is the contract, and the verification below is built around proving it rather than around
a defect that does not exist.
1. Premise, re-derived on
origin/main@79ebf30d1Line numbers read off the tree on this branch's merge base rather than inherited from the
card (two rows of the dispatched table were off by one; the substance was exact).
renderers/action/resolve-icon.tsrenderers/form/button.tsxfunction toPascalCase(:14-19)function toPascalCase(:18-23)const iconNameMap = { Home: 'House' }(:22-24)const iconNameMap = { 'Home': 'House' }(:26-28)export function resolveIcon(:30-35):44-49Both split on
-, both PascalCase each segment, both consult the same single-entry renamemap, both index lucide's runtime
iconsrecord. Same algorithm, not the same function —so an alias added to
resolve-icon.tsto absorb a lucide retirement (the objectui#5586 /objectui#5622 mechanism) reached every
action:*site,complex/data-table.tsxand bothmenu renderers, and silently missed this one.
Helper census before deleting anything.
toPascalCaseis referenced exactly twice inthe file — its definition (
:18) and the icon lookup (:46).iconNameMaplikewise(
:26,:47). Neither served anything but the icon lookup, so both go with it.2. Equivalence — measured, not assumed
The deleted copy was transcribed verbatim and run beside the real shared resolver over
3547 names: every one of lucide's 1767 record keys in both its PascalCase and its
kebab-case spelling, plus kebab-case probes, the rename alias, retired spellings, the empty
string and
undefined.Zero genuine forks. Agreement is by object identity, not by name — the same component
object comes back from both.
The one real difference, and why it is inert — measured, not assumed
The copy ended in
(icons as any)[mapped] as LucideIcon, which yieldsundefinedfora miss;
resolveIconends in?? null. The card flagged this as the difference to check,so it was checked at the consumption site rather than reasoned about:
Iconoccurs at exactly four places in the old file — the declaration (:44), theassignment (
:48), and two render sites:Both are truthiness tests, and React renders nothing for
nullandundefinedalike,so the flavour of the falsy value never reaches the DOM. Pinned behaviourally by three rows
in the new suite (a retired spelling, an unresolvable name, and no icon authored — all
three assert
button.querySelector('svg')is null while the label still renders).One consequence worth stating: the new code calls
resolveIcon(schema.icon)unconditionally, where the copy skipped the lookup when
schema.iconwas falsy.resolveIconreturnsnullforundefinedand for''by its ownif (!name)guard, sothe outcome is identical; the guard simply moved inside the function.
3. Removal proof
Counts taken with
git show 79ebf30d1:<path>against the working file:^function toPascalCase^const iconNameMapiconsimported fromlucide-react(icons as any)[…]index'../action/resolve-icon'resolveIcon(callNo second definition of either helper survives anywhere in the file. The two textual
occurrences that remain are inside the explanatory comment naming what was removed, and
they are prose — the census in
scripts/check-lucide-icon-record-names.mjsparses importsfrom the TS AST, so a comment cannot be mistaken for a read.
4. Why there is no red-before ablation, and what red-before there is
Behaviour is unchanged by construction, so the usual "revert the fix, watch the test go
red" leg does not exist for the behavioural rows: they are green on the copy and green on
the import. No fake behavioural difference was manufactured to produce one. Presenting
those green rows as evidence that the change is correct would be circular; they are the
guard that it changed nothing.
What is checkable was checked, in two reverse-verification legs. Each restored the
pre-change state on disk, proved the mutation landed by blob hash and by grep before
reading anything, and restored via
git checkout HEAD -- <abs path>under a trap, withgit diff HEADempty and the blob hash back toHEAD's afterwards.Leg A — restore the inlined copy, keep the new suite (
79ebf30d1'sbutton.tsx; blobd1d0a278→e6377fe9,copy defs on disk: 2,shared-resolver imports: 0):The two routing rows are the only ones that discriminate, and they discriminate on
exactly the thing this card changes: which function the glyph came out of. The nine
behaviour rows stayed green in both worlds, as they must.
That leg also caught a blind instrument in the suite before it shipped: the second routing
row originally read
expect(shared.mock.results[0]?.value).not.toBeNull(), which passeswith zero calls (
undefinedis notnull) and was measured green against the restoredcopy. It now asserts
toHaveBeenCalledTimes(1)first, and the reason is written beside it.Leg B — restore the census declaration while the source no longer reads the record
(blob
1a591a76→6bd2ec8d, injected declaration lines: 1):5. The icon-name gate — what its verdict means here
scripts/check-lucide-icon-record-names.mjsdoes cover this file, and it is not abystander: part 1 of that gate rediscovers every record-reading module from source on each
run and fails when the discovered set differs from
DECLARED_RECORD_READERSin eitherdirection. Dropping
form/button.tsxfrom that list is therefore verified rather thanasserted — leg B above is that verification, and the run on this branch reports the
population as 7 resolvers, down from 8.
Two things it says, and one it does not:
undeclared record reader and fail the census in the opposite direction — which is a
stronger guard against re-divergence than any single test.
the copy in place. Both modules were declared legitimate record readers; nothing in the
gate compares two resolvers to each other. That blindness is exactly what the card was
filed about, and it is disclosed in objectui#5935.
The part-2 census entry for the
buttontype stays — its authored icon names stillreach a record-reading resolver, one indirection away — with its
resolverfieldre-pointed at
resolve-icon.ts, matching thecontext-menu/dropdown-menuidiomalready in that table.
Three rows of
scripts/__tests__/check-lucide-icon-record-names.test.tshard-coded thepopulation at 8 and enumerated
form/button.tsxamong the four resolvers objectui#5633'shand-kept table missed. They move to 7, with the reason recorded in place: the site left
the census by being fixed, not by being forgotten, and the row that listed it now asserts
its absence so the direction of the change is pinned rather than merely edited away.
6. Out of scope — reported, not touched
renderers/basic/icon.tsx(ui:icon) keeps its own copy, deliberately. It draws aSquareDashedplaceholder and warns on an unresolvable name (objectui#5631, maintainerruling 2026-08-22), where the shared
resolveIconreturnsnull. Folding it in woulddelete a deliberate behaviour and break
renderers/basic/__tests__/icon-unresolvable-placeholder.test.tsx. Untouched here, and thereason is now recorded beside the census pin that counts it.
A fourth production copy exists that the dispatched census did not name:
packages/plugin-view/src/ViewSwitcher.tsx(:149-165) carries its owntoPascalCase,its own
iconNameMapand its own function also calledresolveIcon. It is the sameduplication class as this card — and
plugin-viewalready depends on@object-ui/components,so it has no structural obstacle to importing the shared one.
Both sites are already inside the scope of the open umbrella card objectui#5935
("Consolidate the eight lucide record-reading icon resolvers into one seam"), which
enumerates
form/button.tsxamong its eight and rules on the tokeniser and aliasdivergences the wider consolidation has to settle. No new issue was filed, because that
search found one. This PR is the first slice of that population to land, and it is the
easy slice precisely because
ui:button's tokeniser and alias map were already identicalto the shared resolver's — which is why it could be done without changing behaviour.
objectui#5935 stays open.
7. Gates
Gate set derived by reading the step lists under
.github/workflows/(ci.yml,lint.yml,changeset-*.yml,control-bytes.yml,vi-mock-specifiers.yml), not from memory. Exitcodes captured before any pipe; each verdict line is the gate's own output.
Run at
f72ac931— the final commit, working tree clean.pnpm check:icon-record-namesOK lucide icon names: 167 authored/declared names reaching 7 record-reading resolvers are live 'icons' keyspnpm lintTasks: 47 successful, 47 total(0 errors; warnings pre-existing)pnpm --filter @object-ui/components run type-checktsc --noEmit && tsc -p tsconfig.test.json— no diagnosticspnpm type-check:scriptstsc -p tsconfig.scripts.json— no diagnosticspnpm exec vitest run packages/components/Test Files 190 passed (190)/Tests 1731 passed (1731)pnpm exec vitest run scripts/__tests__/check-lucide-icon-record-names.test.tsTest Files 1 passed (1)/Tests 39 passed (39)node scripts/check-changeset-presence.mjs✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)node scripts/check-changeset-no-major.mjs✅ No changeset declares a 'major' bump.node scripts/check-changeset-fixed.mjs✅ All workspace packages are in the changeset fixed group.node scripts/check-vi-mock-specifiers.mjs✅ check-vi-mock-specifiers: OK (… 685 relative specifier(s) resolved …)node scripts/check-control-bytes.mjs✅ check-control-bytes: OK (scanned 5226 tracked text file(s); skipped 85 binary).node scripts/check-entry-guard.mjs --self-test✓ check-entry-guard self-test: 63 cases passnode scripts/check-entry-guard.mjs✓ check:entry-guard: 47 scripts/ file(s) — no entry guard outside the baselinenode scripts/check-lint-coverage.mjs✅ lint coverage: 46/46 packages linted, 0 with outstanding errors (0 total).node scripts/check-type-check-coverage.mjs✅ type-check coverage: 45/46 via 'type-check' … 41/41 packages compile their testspnpm check:phantom-deps✅ Every in-scope import is declared by the package that publishes it.pnpm check:self-import✅ No package names itself inside its own src/.Two "ran but did not measure" traps ruled out by measurement, not by assumption:
type-checkcompiles tests through a second project(
tsc -p tsconfig.test.json).--listFilesconfirms both edited files are in thatprogram —
renderers/form/button.tsxandrenderers/form/__tests__/button-shared-icon-resolver.test.tsx— so "type-check is clean"is a statement about the new test file too, not merely next to it.
pnpm lintis the repo-wide scan (eslint .in 47 tasks) and it was run in full, notnarrowed:
Tasks: 47 successful, 47 totalin 4m6s. No narrowing needed and none claimed.Left to CI, declared rather than skipped silently:
check:esm-specifiers,check:spec-symbols,check:action-forward-parity,check:designer-field-key-parity,check:i18n-keys,check:i18n-drift,check:node-esm-load,check:published-dist,type-check:vitest-setup, the repo-wide shardedpnpm test, the coverage merge, the CLIself-check and Build & E2E.
half-state-patrolis a scheduled backlog sweep, not a gate onthis diff.
8. Files
packages/components/src/renderers/form/button.tsx— imports the shared resolver; localtoPascalCase,iconNameMapand theiconsimport deleted.packages/components/src/renderers/form/__tests__/button-shared-icon-resolver.test.tsx—new. 11 rows: 1 harness control, 2 routing (the discriminating pair), 8 behaviour.
scripts/check-lucide-icon-record-names.mjs— census: reader dropped,buttontype'sresolver re-pointed.
scripts/__tests__/check-lucide-icon-record-names.test.ts— population pins 8 → 7..changeset/5993-button-shared-icon-resolver.md—patchon@object-ui/components(
node scripts/check-changeset-presence.mjsasked for one: published source changed).Never
major, per AGENTS.md §版本号策略.Generated by Claude Code