fix(components): route the SVG-hosted renderers' spread through toDomProps - #7564
Merged
Merged
Conversation
…mProps` `ui:icon` and `ui:spinner` forwarded their whole prop bag to the SVG they render, so every authored SDUI key on the node became an attribute — the `BARE_SPREAD_ON_SVG` shape of objectui#5574, 14 per target, plus `icon="check"` on all 71 icon nodes in the schema catalog. `IconSchema` and `SpinnerSchema` declare only `icon` / `size` / `color`, and both renderers already consume all three by name, so the element-agnostic SDUI pass-through list withholds nothing they need. That is why this group takes the bare `toDomProps` rather than the third declaration the form-control group needed for `name` and `disabled`. Two behaviours change that the sweep gate structurally cannot see, because its judge counts `stroke` / `width` / `height` as legitimate on an SVG host: - `ui:spinner` now spins. Its computed class (`animate-spin` plus the size class) was being overwritten by the `className` carried in the spread. - A sized `ui:spinner` no longer emits `width="lg" height="lg"`: `size` is an enum and the spread handed the string to lucide's numeric `size` prop. An icon's `color` is a Tailwind class, as `IconSchema.color` declares and as every authored value in the catalog uses; it also reached lucide's `color` prop and emitted an invalid `stroke="text-red-500"`. Only the class path remains. Both ledger rows are DELETED in this change, as the two-way exact-set assertion requires — 97 targets in six shapes to 95 in five. Part of #5632 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
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-project-manager
marked this pull request as ready for review
September 3, 2026 22:07
os-project-manager
enabled auto-merge
September 3, 2026 22:07
This was referenced Sep 8, 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 #5632
Clause-②: no
Determined from this diff, not from the expectation: the changed files are two renderer bodies and two test files.
packages/typesandpackages/specare untouched, and no registryinputs/defaultProps/namespace/labelline moved — so no published accept surface moves.The slice, and why this one
One mechanism group, per the triage seat's decomposition ruling (comment 5380586286):
BARE_SPREAD_ON_SVG—ui:iconandui:spinner.Exclusion list: EMPTY. Stated rather than omitted. Both members land, so the group entry is DELETED from
COMPONENTS_LEAK_GROUPSoutright and the grouping is five mechanisms now, not six.Why this group and not another, re-measured on
origin/main3e01cb55frather than inherited (six groups, 97 targets):ui:iconreads the SDUI identity keynameas its lucide icon name, so any icon node that authorsnamerenders nothing at all #5631, "or exclude that row".ui:iconreads the SDUI identity keynameas its lucide icon name, so any icon node that authorsnamerenders nothing at all #5631 closed 2026-08-24 (PR feat(types,components)!:ui:iconnames its glyph withicon, not the identity keyname#6012,ui:iconnames its glyph withicon, not the identity keyname), so the entanglement no longer exists and the group lands whole with no exclusion.BARE_SPREAD(91) is sub-batched by triage's own ruling and would force a partial group plus an exclusion list of ~89 rows; the four remaining groups are one target each.BARE_SPREAD.Ledger: 97 targets in six shapes to 95 in five.
The mechanism: the bare
toDomProps, and why NOT a third declarationThe sibling slice (PR #5858) could not take the bare executor: its hosts are form controls, where
nameanddisabledare legal HTML, so it needed a declaration of its own. That question has to be asked per host, and here the answer is the opposite one — measured, not assumed.IconSchemadeclaresicon,size,color;SpinnerSchemadeclaressize. Both renderers already consume every one of them by name (the glyph lookup,sizeStyle, thecn()class list,sizeClasses). Nothing they declare needs to reach the element through a spread, so nothing legitimate is withheld and no third declaration is warranted.The half a leak gate cannot see — which on an SVG host is most of it
This is the part worth reviewing. A leak gate reports attributes that ARRIVE illegitimately; it has no case for one that STOPS arriving. On an SVG host
@object-ui/test-support's judge countsstroke,width,height,fillandcoloras legitimate, so everything lucide emits sits in the half the gate never reports, in either direction. Three real behaviours lived in that blind spot. All three were read off the DOM directly, and all three are now pinned by full-attribute-set assertions in the new probe:ui:spinnerdid not spin.classNameis on the SDUI pass-through list, so it stayed in the prop bag and overrode the computed class that followed it: aui:spinnerrendered throughSchemaRenderercarriedclass="lucide lucide-loader-circle"and neitheranimate-spinnor its size class. Filtering the spread does not fix this — the key survives the filter — so it is destructured and MERGED here, the way the siblingbasic/icon.tsxin this same group already did it and the way the migration's worked examplelayout/grid.tsx([finding] grid renderer 把 schema 键原样漏成 DOM 属性(columns="[object Object]"、mdcolumns="2" 等无效 HTML 属性) #4787 / PR fix(components): whitelist ui:grid's DOM passthrough so schema keys stop leaking as attributes #5573) does by ordering. Named here rather than ridden along: it is the other half of the harm of the same bare spread, on the same line, and routing the spread without it would leave the filter forwarding a key that destroys a legitimate computed value.ui:spinneremitted invalid dimensions.sizeis an enum (sm/md/lg/xl) consumed throughsizeClasses; the spread also handed the string to lucide's numericsizeprop, so every sized spinner carriedwidth="lg" height="lg".colorproduced an invalid paint value.IconSchema.colordeclares a Tailwind CLASS ("Color Class") andcn()applies it. The spread also reached lucide'scolorprop, emittingstroke="text-red-500"beside the class doing the real work. All threecolorvalues authored in the catalog are classes, so nothing depended on the raw-CSS-colour accident; an authored raw colour no longer tints the glyph through it, which is the contract-first direction (AGENTS.md #0.1).Evidence
"The tests are green" is not evidence on this card, and this run re-confirms why:
packages/components' full 2155 tests are green in BOTH ablation directions — no existing test asserted any of these attributes, nor noticed that the spinner was not spinning.Catalog-scale DOM measurement, new probe
examples/schema-catalog/test/svg-host-dom-leak-5632.test.tsx, through the realSchemaRenderer:icon: 71 nodes, 71 illegitimate attributes before, 0 after (icon[icon], the declared glyph key the renderer consumes and forwarded as well).spinner: 6 nodes, 0 before and after at catalog scale — the catalog authors no undeclared keys on them, so the sweep's canary node is what covers this member. Said plainly rather than presented as a result.ui:grid: 26 nodes, 0 in both runs, in the same run asicon's 71.noElement0 everywhere), so the after-zero is a reading and not a walk that stopped finding nodes — fix(components): route flex/stack/container/text throughtoDomProps(#5574) #5839's guard. Each node renders withchildren/bodyremoved, so the control is not a mixture of other groups.['label','zzcanary']on an svg host and['color','label','width','zzcanary']on a div host. A judge that allowedcolor/widtheverywhere — or that lowercased SVG names — would pass everything here while seeing none of this group.Two-way expiry observed. With the renderers fixed and the rows still present, the sweep failed on EXACTLY the two group members and no others:
2 failed | 200 passed, directionexpected [] to deeply equal [ 'ariaDescribedBy', …(13) ]. The rows were then DELETED — not edited, noit.skip, no exemption — and the gate is green at202 passed.Reverse-verification on commit
5d023aaa2, reverting ONLY the two renderer sources to3e01cb55f. Predicted before running; observed as predicted. Mutation proven ON DISK in both directions, not by an editor exit code: blob hashes moved to exactly theorigin/mainblobs, injected anchorstoDomProps(iconProps)/toDomProps(spinnerProps)2/1 to 0/0 and the deleted bare spreads counted back in at 2/1. Observed4 failed | 201 passed: the two sweep targets (ui:icon leaked 14 non-DOM attribute(s), same forui:spinner) plus both probe readings (expected [ …(71) ] to deeply equal [], and the legitimate-set pin at 12 attributes vs 11). The census did NOT move under mutation, so the reds are leaks and not a broken walk. Restore proven BY STATE —git diff HEADempty,git statusclean, worktree blobs byte-equal to the HEAD blobs — never by an exit code; the script carriestrapon EXIT/INT/TERM with absolute paths.Legs that do NOT discriminate, named so the discriminating ones mean something: the judge element-awareness self-check (green both legs — it renders raw markup, no renderer in the path); the
ui:gridcontrol (0 both legs by construction); the node census (identical both legs — it discriminates a broken walk, never the fix); andpackages/components' 2155 tests (green both legs, which is the finding above rather than a reassurance).Verification, and every narrowing declared
All vitest runs from the repo root; no
pnpm --filterfor vitest and no run from a package cwd (the #3378 guard). Union re-run at the final commit5d023aaa2.pnpm exec vitest run packages/app-shell/src/__tests__ packages/fields/src/__tests__/widget-dom-leak-e2e.test.tsx examples/schema-catalog/test—Test Files 33 passed (33)/Tests 2536 passed (2536).pnpm exec vitest run packages/components—Test Files 230 passed (230)/Tests 2155 passed (2155).pnpm --filter @object-ui/components --filter @object-ui/app-shell type-check— bothDone.--listFilesconfirms the edited sweep test is really in thetsconfig.test.jsonprogram (1 hit), so "type-check clean" actually covers it.pnpm --filter @object-ui/example-schema-catalog type-check— clean, and--listFilesconfirms the new probe is in that program (1 hit). It first reported fiveTS2882errors in files this PR does not touch; those were an unbuilt dependency closure and cleared afterpnpm --filter '@object-ui/example-schema-catalog^...' build, which reads exactly like a real break.pnpm exec eslint . --no-inline-config -f json— the FULL repo, not a narrowing: 4264 files linted (population read from eslint's own config, count read from the JSON output), 0 errors in all four files this PR touches. The repo carries 93 pre-existing errors across 77 other files on this tree; none is a file this PR touches.node scripts/check-changeset-presence.mjs—3 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s).check:control-bytesOK (6224 tracked text files) ·check:icon-record-namesOK (182 names, and this PR touchesicon.tsx) ·check:self-importOK ·check:phantom-depsOK ·check:side-effects-arrayOK.check:sdui-registration-pinsNOT RUN, declared: it exits 2 withNo console build to weigh at apps/console/dist/assets— a prerequisite, not a failed measurement. It guards registrations being dropped by a wrongsideEffectsarray at bundle time; this PR changes nopackage.json, andcheck:side-effects-array(which reads that array directly) is green. CI owns the bundle-level run.Exit codes were captured before any pipe, and every gate result above quotes the gate's own printed line rather than a bare
$?.🤖 Generated with Claude Code
https://claude.ai/code/session_01EMrWaQw3XS5DxTHxp4yRyC
Generated by Claude Code