Uh oh!
There was an error while loading. Please reload this page.
fix(components): resolve ui:dropdown-menu item icons instead of rendering the name as text - #5994
Merged
yinlianghui merged 2 commits intoAug 24, 2026
Merged
Conversation
…ring the name as text Both arms of `renderMenuItems` rendered an item's authored `icon` string into a text node, so the catalog fixture named `with-icons.json` drew the words "edit"/"copy"/"trash" beside its labels. Route both arms through `resolveIcon` — the lucide RECORD surface `ui:button` and `action:*` already use — so an unknown or retired spelling renders nothing rather than a word or a wrong glyph. The fixture's `edit` is a retired spelling absent from lucide's runtime `icons` record; replaced with `square-pen`, the live key it resolves to by identity. Part of #5930 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L
`react-hooks/static-components` does not fire inside `renderMenuItems` — it is a plain helper, not a component — so both directives reported as unused. The sibling `action:menu` sites keep theirs because there the resolution happens inside a component body. Part of #5930 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CSoz9uGhaaSgiq3hshtN7L
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
|
yinlianghui
marked this pull request as ready for review
August 24, 2026 11:32
Uh oh!
There was an error while loading. Please reload this page.
yinlianghui
deleted the
claude/issue-5930-dropdown-menu-icon-resolution
branch
August 24, 2026 11:43
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
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.
Fixes#5930
Both arms of
renderMenuItemsinpackages/components/src/renderers/overlay/dropdown-menu.tsxrendered a menu item's authorediconstring straight into a text node, so the catalog fixture literally namedwith-icons.jsondrewedit Edit,copy Copy,trash Delete.Both arms now resolve the name through
resolveIcon.Route taken
Per the dispatch ruling, the record surface (
resolveIconfromrenderers/action/resolve-icon.ts) — whataction:*andui:buttonnext door already resolve against. A retired or unknown spelling renders nothing. The dynamic surface (LazyIcon) was not used: it degrades an unknown name to theDatabaseglyph, trading a no-icon failure for a wrong-icon one, recorded as ruled out for authored icon fields by #5622 and #5633. Retiring the key was likewise not taken.Premise verified before any edit.
resolveIconlives in the same package, one directory over;renderers/complex/data-table.tsx:12already imports it as'../action/resolve-icon', so this is an established sibling import, no new cross-package dependency and no layering inversion.One nuance worth recording:
ui:buttonis the same resolver by behaviour, not by identity —renderers/form/button.tsxcarries a byte-equivalent inlined copy (its owntoPascalCase, its ownHome -> Housemap, its own index intoicons) rather than importing the shared function. Same population, same casing, same outcome, so the ruling holds exactly. Filed as #5993.Line numbers re-derived
The card's
~40/~52were days old. Re-derived from the tree: exactly 40 and 52, with a control probe (a deliberately nonsensicalitem.iconXYZZYreturned exit 1) so a zero-hit would have read as real rather than as a silent miss.The fixture carried a RETIRED spelling
editis not a live key of lucide's runtimeiconsrecord (measured against the installed lucide-react 1.31.0: 1767 keys,Editabsent,CopyandTrashpresent). It survives only as a deprecated export. So after this fix the first item ofwith-icons.jsonwould have drawn no glyph at all — a fixture named for its icons, silently showing two of three.Corrected to
square-pen, derived by identity rather than by preference — the method the gate itself documents for naming a replacement:This matches the repo's own established repair:
packages/plugin-detail/src/DetailView.tsx:792already spells this same glyphicon: 'square-pen', from #5622's fix of the same retired name.Does this make the #5633 gate newly relevant here?
No — and that is itself a finding. Measured both directions:
iconsrecord directly. This PR importsresolveIconinstead, sodropdown-menu.tsxcorrectly does not enterDECLARED_RECORD_READERS; the gate still reports 8 record-reading resolvers.type, and menu items are untyped child objects. Restoringeditto the fixture on top of this fix and re-running the gate gave exit 0, same 64 names — a dead spelling in a published fixture passes silently.Filed as #5992, which also notes that this PR falsifies a measured parenthetical in the gate's header ("the fourth renders it as raw text"), left untouched here as out of fence.
Verification
Union re-run after the final commit, at
368f1c510:dropdown-menu-item-icon.test.tsx(new)check:icon-record-namesOK lucide icon names: 64 authored/declared names ... are livecheck:control-bytes✅ check-control-bytes: OK (scanned 4958 tracked text file(s))check:changeset-presence✅ 2 source file(s) ... declares 1 changeset(s)check:doc-types✅ Every documented component type is registered.check:doc-snippetsEvery covered documentation snippet compiles against the built types.@object-ui/componentstype-checkcheck:doc-snippetsfirst refused as a blind instrument (unbuilt sibling packages); the full workspace was built (43/43 tasks) and it was re-run rather than declared narrowed.Reverse-verification. With the fix committed, both
IconJSX sites were reverted to the pre-fix<span>form under atrap ... EXIT INT TERMrestore. Mutation confirmed on disk against the exact text targeted (injected pre-fix span present ×2, post-fix JSX absent ×0) — not by an editor exit code. Predicted direction red, observed 5 failed | 2 passed; the two survivors are the no-icon case and the retired-spelling-renders-nothing half, neither of which depends on the fix in the failing direction. Restore leg verified in both directions with a cleangit status.The suite asserts both directions — the glyph appears and the bare name does not — because an svg-only assertion passes against the broken renderer too.
Scope
Fence respected.
context-menu.tsxandmenubar.tsxwere checked and readiconnowhere at all, so they carry no narrower version of this defect. Nothing underexamples/schema-catalog/src/schemas/plugin-dashboard/was touched, and no catalog-wide gate was added.Generated by Claude Code
Generated by Claude Code