fix(components): resolve ui:breadcrumb and ui:command child item icons through the record resolver - #6644
Merged
Merged
Conversation
…ugh the shared record resolver
…n the lucide record gate
This was referenced Aug 28, 2026
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
|
os-sales
marked this pull request as ready for review
August 28, 2026 08:29
This was referenced Aug 28, 2026
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Part of #5931 — deliberately not a closing keyword. Two of the card's three components are wired here; the third,
ui:button-group, is routed for a decision below and is untouched by this branch. Merging this should not close the card.Mirrors the pattern PR #5994 landed for
ui:dropdown-menuand PR #6324 landed forui:context-menu, rather than re-deriving one.What was wrong
BreadcrumbItem.iconandCommandItem.iconare declared, published keys of the protocol — present in@object-ui/types, mirrored in the Zod schemas, and documented on both components' pages — and neither renderer contained the substringiconat all. The catalog fixture literally namedcomponents-data-display-breadcrumb/with-iconsrendered no icons; the nine names the twocommandfixtures declare all drew nothing.What changed
resolveIcon(renderers/action/resolve-icon.ts) — the lucide record surfaceui:button,action:*,ui:dropdown-menuandui:context-menualready resolve against. A live name draws its glyph; an unknown or retired spelling draws nothing — never a fallback glyph, never the literal name as text.LazyIcon/getLazyIcon: the dynamic surface degrades an unknown name to theDatabaseglyph, trading a no-icon failure for a wrong-icon one. Ruled out for authored icon fields by Two more retired lucide spellings reach theicons-record resolver —editin DetailView's mobile Edit action,smileas theiconrenderer's own default — and only one of the four resolver copies is pinned #5622 and Nothing checks that anicon:literal reaching a record-reading lucide resolver is a liveiconskey — four hand-copied resolvers, four local pins, no gate over the population #5633, and restated in each renderer's code comment.breadcrumbhas two arms — the last crumb renders asBreadcrumbPage, every earlier one asBreadcrumbLink. The icon is resolved once per item and rendered above that split, insideBreadcrumbItem, so neither arm can be forgotten. Both are measured as their own test rows.commandhas exactly one arm, and that was checked rather than assumed:CommandGrouptakesheadingas a plain string and declares noicon, sogroups[].items[].iconis the only icon key in the shape.Fixture spellings — two were already dead
A fixture must not declare a name that draws nothing. Every name the three components' fixtures declare was checked against lucide's runtime
iconsrecord (lucide-react 1.31.0, 1767 keys). Two were retired spellings that are absent from the record, corrected to the live key the retired export resolves to by identity:components-data-display-breadcrumb/with-iconslayoutpanels-top-leftLayout === PanelsTopLeftcomponents-form-command/command-menusmileface-slightly-smilingSmile === FaceSlightlySmilingThe other ten names (
home,book,calendar,calculator,user,credit-card,settings,file-plus,folder-open,save) are live.homeis the one whose glyph class is not its own spelling —resolveIcon'siconNameMapsends it to lucide'sHouse, and the fixture test assertslucide-houseso that indirection is pinned from the outside.ui:button-groupis NOT wired here — it needs a decisionIts three fixtures author
icononbuttons[]and its docs page documentsicon?: stringonButtonGroupButton, butButtonGroupButtondeclares noiconkey — not inpackages/types/src/navigation.tsand not in its Zod mirrorpackages/types/src/zod/navigation.zod.ts. Wiring it therefore means first widening the published item contract, which is the card's "stop and report, do not choose" branch. Measured, not inferred: renderingcomponents-basic-button-group/icon-toolbar.jsonthrough the registry today yields 3 buttons, all with emptytextContent, 0 svg — the "Toolbar Actions" example on the published docs page is three blank buttons. So stripping the key is not the honest branch either; it would empty that fixture. Full options and a recommendation are in the developer report on #5931.The same divergence covers
value,selectionMode,onValueChangeanddisabled, all documented on that page and all absent from the type — which is why this is one decision, not four.The lucide gate's census row for
button-groupis updated to say this explicitly, so the next reader does not have to re-derive why it is the one container in that table still left undeclared.Gate census (
scripts/check-lucide-icon-record-names.mjs)Extended the same way #6324 extended it when
context-menu's repair landed: a census entry declares that a type's names now reach a vocabulary, and it could not have been added before the repair.breadcrumbandcommandare declared withdescendants: trueand a measuredmin(3 and 9) rather than the weakermin: 1its siblings carry — a descent that reached one of nine would be as blind as one that reached none.command's names sit atgroups[].items[].icon, a depth the single-levelARRAY_PATHcannot express at all, so descent is the only non-vacuous form available. Judged names: 167 → 179 (+12), exactly the measured population.⛔ This does not teach the gate to guess at untyped child nodes; that blind spot stays filed as #5992 and is untouched. The gate's stale header rows for
breadcrumb,commandandcontext-menu(the last falsified by #6324) are corrected, and two comments in its own test suite that citedbreadcrumb/commandas examples of containers reaching no resolver now citebutton-group, which still is one.Verification — at
1352da1fpackages/components/src/__tests__/breadcrumb-item-icon.test.tsxandcommand-item-icon.test.tsx— 20 tests, green. Renderers invoked viaComponentRegistry.get(name), notSchemaRenderer(fix(components): resolve ui:dropdown-menu item icons instead of rendering the name as text #5994's stated reason).svg.lucide-KEY; the "authored name is not on screen as text" direction is asserted once each and labelled honestly as a regression guard, not a discriminator — unlikedropdown-menu, these renderers never printed the name, soqueryByTextis null in both worlds.BreadcrumbSeparator's chevron andCommandInput's search glyph are asserted present at container level while the item-scoped query is red pre-fix, so a red row cannot be misread as a broken query. Each row scopes to the crumb's ownli/ the item's own cmdk element, so neither sibling glyph leaks in.ui:commandrenders cmdk inline (CommandDialogis a separate export this renderer does not use). A harness control asserts the items really mounted, so no "renders no glyph" row can pass vacuously.origin/mainfrom a committed state, confirmed the mutation on disk (0resolveIconoccurrences, blob hash differs from the HEAD blob), re-ran: 8 failed / 12 passed. The 8 are exactly the glyph-presence rows across both breadcrumb arms and command's arm plus the fixture rows; the 12 that stay green are the harness controls and the retired/unknown/no-icon rows, which are correctly null in both worlds. Restored and proved the restore:git diff HEADempty and the on-disk hash equal to the HEAD blob.layoutandsmileback turnscheck:icon-record-namesred on both, naming the exact JSON pointers — and the gate independently derived the same two replacements this PR chose. Restored and re-proved.{item|button|crumb.icon}as a bare JSX text render — 0 hits across all three renderer files, while the same query shape finds thelabelrenders in two of them (breadcrumb's third isresolveKeyedI18nLabel(item.label), shown separately).packages/components/199 files / 1810 tests green;examples/schema-catalog/+ the gate's own suite 15 files / 1874 tests green; the six adjacent suites naming these types 6 files / 70 tests green.type-checkgreen, and proved to be a real measurement —tsc --listFilesshows both edited renderers and both new test files inside the compiled programs.eslint .over the repo: 3875 files, 0 errors. The touched files carry 0 errors and only pre-existing warning classes.1352da1f:check:icon-record-names,check:control-bytes,check:doc-types,check:doc-fences,check:doc-snippets(after building its prerequisites),check-changeset-presence,check-changeset-no-major.The changeset scores
patchfor@object-ui/components; the presence gate ruled it owed and it was not pre-scored in either direction.Generated by Claude Code