Uh oh!
There was an error while loading. Please reload this page.
fix(components): resolve ui:context-menu item icons on both arms - #6324
Merged
Conversation
`renderContextMenuItems` never referenced `icon` at all — 0 occurrences in the file, against 2 for `shortcut` and 12 for `label` — so the catalog fixture `basic-context-menu.json` shipped four live lucide names (`copy`, `scissors`, `clipboard`, `trash`) that drew nothing beside their labels. Route the authored name through `resolveIcon`, the lucide RECORD surface `ui:button` and the `action:*` family already resolve against, on BOTH arms: the leaf `ContextMenuItem` and the `ContextMenuSubTrigger`. Repairing only the leaf would be a narrower version of the same bug. The dynamic `LazyIcon` surface stays ruled out — it degrades an unknown name to the `Database` glyph, trading a no-icon failure for a wrong-icon one. This is objectui#5930's diff ported to the file next door, with one difference: the twin rendered the name as raw TEXT, so its load-bearing assertion could be `queryByText(name)`. Here the defect was an ABSENT render, which makes that assertion a ghost — every measurement row asserts the resolved glyph by the identity lucide gives it instead. Also adds the `'context-menu'` census entry to `scripts/check-lucide-icon-record-names.mjs`, so the fixture's four names come under the gate's judgement as the card's own note asks. 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
|
os-support-ai
marked this pull request as ready for review
August 25, 2026 13:07
Uh oh!
There was an error while loading. Please reload this page.
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#6278
renderContextMenuItemsnever referencediconat all, so the catalog fixturebasic-context-menu.jsonshipped four live lucide names —copy,scissors,clipboard,trash— that drew nothing beside their labels. This ports the repair #5930 landed on this file's twin (renderers/overlay/dropdown-menu.tsx, PR #5994) to the file next door.The repair
The authored name is routed through
resolveIcon— the lucide record surfaceui:button,ui:dropdown-menuand theaction:*family already resolve against — on both arms: the leafContextMenuItemand theContextMenuSubTrigger.resolveIconis called once per item, above the arm split, exactly as the twin does it. The dynamicLazyIconsurface stays ruled out: it degrades an unknown name to theDatabaseglyph, trading a no-icon failure for a wrong-icon one.Also adds the
'context-menu'census entry toscripts/check-lucide-icon-record-names.mjs, as the card's own note asks, so the fixture's four names come under the gate's judgement.Verified premises (the card measured at
ef2a3bd8d; this branch is off090927f4f)context-menu.tsxstill has no reference toiconshortcut2 andlabel12 on the same pathspeceditis null, giving a live retired-spelling controlresolveIconis genuinely the twin's seampackages/components/src/renderers/action/resolve-icon.ts— readsiconsfromlucide-react, returnsnullfor an unknown nameThe issue body was also checked for sanitizer truncation: intact — 3966 bytes, balanced fences, all 11 angle-bracket fragments present, footer present.
Two honest differences from the twin — neither was copied over blind
1. The defect here is an ABSENT render, not a WRONG one.
dropdown-menudrew the name as raw text, so its load-bearing assertion could bequeryByText('trash'). Here nothing was drawn at all, which makes that assertion a ghost — null before the fix and null after. Every measurement row in this suite asserts the resolved glyph instead, identified assvg.lucide-<authored-name>— derived from the authored name (the independent input), never from the renderer under test.2. The submenu trigger already contains an svg.
ContextMenuSubTriggerrenders its ownChevronRightunconditionally, so a barequerySelector('svg')on that arm is green in both worlds — the blind instrument this suite must not use. The chevron is asserted alongside the subject as a positive control on the instrument, so a redlucide-trashrow cannot be misread as a broken query.A third difference: Radix's
ContextMenuPropsis{ children, open, onOpenChange, dir, modal }— there is nodefaultOpen, so the twin's open-trick does not port. The suite fires acontextmenuevent on the trigger and asserts the menu actually mounted before reading any glyph.Red before, green after — both arms as separate rows
Measured at the parent commit with
grep -c iconon the renderer reading 0:After the repair:
Tests 8 passed (8).Ablation — each arm reverted alone
Each mutation is proved on disk in both directions (injected marker grepped, removed text grepped, working blob hash compared against the HEAD blob) and restored under
trap … EXIT INT TERMwith cwd-independent paths;git diff HEAD --statwas empty afterwards and both blobs matched HEAD.The submenu row is the one that matters: it is why a leaf-only repair cannot read as complete.
A first ablation round was discarded as invalid and is reported rather than quietly re-run. Injecting a bare
/* … */marker into JSX children renders it as a text node, which brokegetByTextand took unrelated rows down with it (5 red and 3 red instead of 2 and 1); and renaming the census key tripped the gate'smin: 1non-vacuity rule rather than removing the entry. Both were artifacts of the instrument, not readings. Re-run with a JSX-valid no-op ({/* … */}) and a true deletion, the numbers above are what the corrected mutations give.Gates run locally, all on
ad54f7587(the final commit)check:icon-record-names(0 errors, 167 names / 8 resolvers) ·check:control-bytes·check:doc-types·check:doc-fences·check:doc-snippets(267/267 blocks, after building the 32 packages it needs — its first run was aPRECONDITION NOT METexit 2, i.e. not measured, not red) ·check:vi-mock-specifiers·check-changeset-presence·type-checkfor@object-ui/components(9 tasks) andtype-check:scripts·lint(@object-ui/components402 files judged,lint:root196 files judged — 0 errors in both; both editedscripts/files and both editedpackages/components/files confirmed present in the judged populations via--format json).Tests: the full
packages/componentssuite — 188 files, 1716 tests, all passed — plusscripts/__tests__/check-lucide-icon-record-names.test.ts. The twin'sdropdown-menu-item-icon.test.tsxis green, so the neighbour did not regress.Note on
type-check:packages/components/tsconfig.jsonexcludessrc/__tests__and**/*.test.tsx, so the first program does not see the new test file. The package script istsc --noEmit && tsc -p tsconfig.test.json, and--listFilesconfirms the second program does cover it — checked rather than assumed.Scope
Deliberately untouched:
button-group,breadcrumbandcommand(#5931 owns those and ispm:blockedon whether the key should exist at all);timelineandtree-view(deliberately different per the card's table); and the resolver consolidation queued as #5935, which would rewrite the very seam this routes into. This card was separable only because its twin was already answered — none of those are addressed here and all of them stay open.Generated by Claude Code
Generated by Claude Code