Uh oh!
There was an error while loading. Please reload this page.
fix(components): render a visible placeholder when a ui:icon glyph does not resolve - #5959
Merged
Merged
Conversation
…es not resolve
An unresolvable `ui:icon` used to `return null`, which failed silently in two
independent ways at once: invisible to a human, and clean-looking to a gate,
because a renderer that returns `null` spreads no attributes and an empty DOM
scan reports no findings.
It now renders a dashed-square placeholder on the same SVG host, keeping the
authored className/size/colour, with role="img", an accessible name that says
which icon failed, and a data-objectui-icon-unresolved marker. A node with no
`name` at all previously threw in toPascalCase(undefined) and had the throw
swallowed by the error boundary; it renders the placeholder too.
The placeholder glyph is a direct named import rather than a lookup in
lucide's runtime `icons` record: lucide retires a spelling by dropping it from
that record while keeping the deprecated named export, so a record-based
placeholder could itself resolve to nothing. Measured on lucide-react 1.31.0,
both CircleHelp and HelpCircle are absent from the record.
The widget-dom-leak-sweep entry for ui:icon no longer needs its forced
`schemaExtras: { name: 'check' }` workaround, so the renderer is swept on the
ordinary canary node. Its BARE_SPREAD_ON_SVG row was re-measured on that node
and is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019ZyKZejBWZoCSj1NP35wcpContributor
✅ 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-warren
marked this pull request as ready for review
August 24, 2026 07:21
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 24, 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 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.
Part of #5631
Part of, not a closing keyword. This PR lands items 3 and 4 of the 2026-08-22 ruling. Item 1 —ui:iconreadingschema.icon— is NOT here, blocked on the metadata sweep below, which is the ruling's own stop condition. #5631 must stay open when this merges.The sweep the ruling required, and its reading
The ruling put a sweep of real authored metadata first, and made a population that cannot be migrated cleanly a stop-and-report. It ran first. It stops the key change.
What was scanned
nameiconexamples/schema-catalog/src/schemas/**.jsonparsed and walked as a tree, matchingtype: 'icon'/'ui:icon'at any depthapps/site/app/playground/page.tsxpackages/components/src/__tests__/basic-renderers.test.tsxcontent/docs/**icon.mdx,block-schema.mdxnameobjectstack(sibling repo, working checkout).json/.ts/.tsx/.mdx/.ymlReading: 98 authored icon nodes, 100% of them naming the glyph with
name, and zero already onicon. Not a legacy tail — the entire corpus.Also measured: no code path synthesizes icon nodes. No bridge, defaults map or palette builds
{ type: 'icon' }at runtime, so the whole population is hand-authored metadata and there is no producer to fix at the producer.What the sweep could NOT see — stated, not rounded to zero
nameis what every doc and the type itself advertises, the prior for deployed content is high reliance, not low.cloudrepo is not checked out in this container. Not scanned.objectstackreading is from its working checkout at4c9780c, not a cleanmain.The contract blocks item 1 independently of the corpus
The published type is not merely silent about
icon— it rejects the ruling's authoring shape. Measured against the built@object-ui/typeszod mirror:IconSchemadeclaresname: z.string()required, documented verbatim as "Icon name (lucide-react icon name)". So under ruling A an author writing{ type: 'icon', icon: 'save' }is refused by the contract while the renderer reads a key the contract does not declare — contract-first exactly backwards (AGENTS.md #0 / #0.1).Item 1 therefore cannot land at the renderer alone. It needs, in one change:
packages/types/src/layout.ts+packages/types/src/zod/layout.zod.ts(declareicon, demotenameto identity), the 98-node corpus conversion,content/docs/components/basic/icon.mdx, and the renderer's registryinputsentry. That is ~29 files across four workspace entries including the protocol package — far outside this card's declared file surface, and a published-contract change in its own right.Per the ruling: stopping and reporting rather than shipping a break I cannot account for.
What this PR does land
1. An unresolvable icon renders a visible placeholder (ruling item 3, unconditional)
The ruling makes this hold "regardless of the key question", and triage recorded it as the half that need not wait on adjudication.
return nullis gone.It failed silently in two independent ways at once, which is why it survived so long:
nullspreads no attributes, so a DOM scan of it reports no findings. An empty scan and a clean scan are the same reading.Now: a dashed-square placeholder on the same SVG host, keeping the authored
className,sizeand colour so the gap sits exactly where the icon would have been, plusrole="img", an accessible name naming the icon that failed, and adata-objectui-icon-unresolvedmarker for gates. Theconsole.warnstays and now names the cause.Also fixed, found while implementing: a node with no
nameat all reachedtoPascalCase(undefined), threw onundefined.split, and had the throw swallowed bySchemaErrorBoundary— a third silent failure on this renderer. It renders the placeholder too.The placeholder glyph is a direct named import, not an
icons-record lookup. A placeholder that itself fails to resolve is the original bug one level up, and silent in the same way. Lucide retires a spelling by dropping it from the runtime record while keeping the deprecated named export — the #5622 mechanism. Measured on lucide-react 1.31.0:CircleHelpandHelpCircle, the two obvious "unknown" glyphs, are both absent from the record while both still resolve as named exports. Either one, looked up the usual way, would have rendered nothing. A named import is resolved at build time, so a future retirement fails the build loudly instead of silently returning tonull. Pinned by a test.2. The
BARE_SPREAD_ON_SVGledger row, re-measured (ruling item 4)The sweep entry for
ui:iconcarried a workaround —schemaExtras: { name: 'check' }— forcing a resolvable name because the renderer otherwise rendered nothing. With the placeholder, that workaround is no longer needed, soui:iconis now swept as an ordinary plain target on the node the sweep actually authors: identityname: 'canary_node'and nothing else.Re-measured on that node, the row is UNCHANGED — the placeholder is the same bare spread onto the same SVG host, so it leaks the same fourteen attributes. That the row did not move is the result: the reading no longer depends on a workaround that concealed whether the renderer rendered at all.
namestays in that row. The renderer still spreads the authored identity onto the SVG, and closing that is the #5632 burn-down — deliberately not folded in. Per the ruling's sequencing note, this lands before that batch (or that batch excludes this row); nothing here touches #5632's scope.Verification
All at final commit
c474bb04b.vitest run packages/components/Test Files 182 passed (182)/Tests 1669 passed (1669)vitest run …/widget-dom-leak-sweep.test.tsx+ new icon testTest Files 2 passed (2)/Tests 209 passed (209)type-check(@object-ui/components,@object-ui/app-shell)tsc --noEmit && tsc -p tsconfig.test.jsonechoedlint(same two packages)icon.tsxat 3 warnings, exact parity withorigin/maincheck:icon-record-namesOK lucide icon names: 64 authored/declared names … are live icons keyscheck:control-bytes✅ OK (scanned 4941 tracked text file(s))check:phantom-deps✅ Every in-scope import is declared by the package that publishes it.check:action-forward-paritycheck-changeset-presence.mjs✅ 2 source file(s) … declares 1 changeset(s)check-changeset-no-major.mjs✅ No changeset declares a major bump.Reverse verification
Direction predicted before running: RED in both. The fix was committed first, so the mutation had a real restore point; the mutation script carried a
trap … EXIT INT TERMrestore, and both legs were confirmed on disk by grepping the anchored text in both directions (injected present / removed absent) rather than trusting an editor exit code.Restoring
return null;in the placeholder branch:Tests 4 failed | 4 passed (8)— the four unresolvable cases fail; the four resolvable / named-import cases stay green, which is correct since they do not exercise that branch.Tests 1 failed | 200 passed (201), and the one failure isui:icon. That is what proves removing theschemaExtrasworkaround is load-bearing rather than cosmetic: without the placeholder,ui:iconcannot be swept on an ordinary authored node at all.Restore leg: marker absent, placeholder present,
git statusclean, both suites back to209 passed.The sweep's stderr under mutation also shows the card's defect out loud for the first time:
Warning assertions use an explicit
vi.spyOn. Vitest 4 runssilent: 'passed-only', soconsole.warnfrom a passing test is discarded — measured here directly: a debug probe printed nothing until its assertion was forced to fail. A test "checking" the warning by reading output would check nothing.On the declared
inputslist — checked, deliberately unchangedThe registry declares
{ name: 'name', label: 'Icon Name', defaultValue: 'face-slightly-smiling' }, so the renderer's declared authoring surface does itself advertisenameas the glyph key. Under ruling A it is owed a rename toicon. It is not renamed here: the resolver still readsname, and a declared input list advertising a key the resolver does not read is this same defect pointing the other way. The two must move together with the corpus migration. An in-file comment records this so it is not lost.Not folded in
icons-record resolver —editin DetailView's mobile Edit action,smileas theiconrenderer's own default — and only one of the four resolver copies is pinned #5622 /ViewSwitcherrenders NO icon forchartandganttviews: both icon names were dropped from lucide'siconsrecord #5586 — retired lucide spellings on this resolver (wrong name). Untouched.packages/components/src/renderers/**, grouped by mechanism #5632 — the SVG burn-down. Untouched; sequencing respected.Each is a different mechanism; none is closed by this PR.
Generated by Claude Code