Uh oh!
There was an error while loading. Please reload this page.
fix(components): whitelist ui:grid's DOM passthrough so schema keys stop leaking as attributes - #5573
Merged
Merged
Conversation
…top leaking as attributes (#4787) `grid.tsx` ended in a bare `{...gridProps}` spread that removed only `data-obj-*` and `style`, so every other key `SchemaRenderer` hands a registered component reached the rendered `<div>`. Measured on a canary node, eight invalid HTML attributes leaked, including `columns="[object Object]"` and `mdcolumns="2"`. The spread now goes through `toDomProps` from `@object-ui/core` — the whitelist objectui#3291 established and objectui#4425 phase 2 promoted to the SDUI widget contract — rather than enumerating today's schema keys to strip, which re-rots on the next schema addition and can never reach author-supplied keys. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
…4787) Written against the shared canary, case 3 PASSED against the leaking renderer: React latches its unknown-prop warning per prop name per module instance, so cases 1/1b consume the latch first and the spy sees nothing. Reverse-verification caught it green on code that leaked ten attributes. It now authors its own camelCase key (`zzCanaryCamel`), used nowhere else in the file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012u2pRjcqAYtoEjgr3wwhnK
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 21, 2026 14:32
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 21, 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
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#4787
The defect, measured
grid.tsxended in a bare{...gridProps}spread onto its< div >, removing onlydata-obj-*andstyle. Everything elseSchemaRendererhands a registered component — the authored node's own keys, the contents of itspropscontainer, and any extra key the author wrote — landed on the element as invalid HTML attributes. Measured onorigin/mainwith the canary node the new test uses:Eight illegitimate attributes:
columns,gap,mdcolumns,smcolumns,name,props,zzcanary, andcolorvariant(the flattenedpropscontainer). A responsivecolumnsobject renders ascolumns="[object Object]". Layout is unaffected, which is why every catalog grid example has been rendering with them.The fix — whitelist, per triage's ruling
The spread now goes through
toDomPropsfrom@object-ui/core: the whitelist #3291 established inpackages/fieldsand #4425 phase 2 promoted to the SDUI widget contract. This is the existing mechanism, not a second spelling of it —grid.tsxis now one more caller of the same executor thatplugin-chatbotandDashboardRendereralready use.Keys that are declared DOM-safe survive (
id,className,role,tabIndex,autoFocus, the React synthetic handlers) plus the opendata-*/aria-*families — which is how the designer'sdata-obj-id/data-obj-typestill arrive, so the two hand-forwarded lines they used to need are gone.stylecontinues to be forwarded by name (the #4435 route): it is this container's designer sizing channel, but the shared set is deliberately element-agnostic and nothing element-specific belongs in it.Enumerating today's
GridSchemakeys to strip would have re-rotted on the next schema addition, and could never have reached the open tail —zzcanaryand thepropscontainer are author-supplied, so no finite list names them.Grid's own vocabulary was always read off
schema, never off these props, so no authored input and no rendered layout changes.Why React's unknown-attribute warning never turned a test red
Triage asked for this, and it has four independent answers, each sufficient on its own. The card's premise — that there is an "unknown-lowercase-attribute warning" being swallowed — turns out to be the least of it.
1. For most of the leak, including both attributes the card's title names, React emits no warning at all. Since React 16, unknown all-lowercase attributes are passed straight to the DOM by design, silently, with object values stringified. So
columns,gap,name,props,zzcanaryproduce zero console output. Measured: eight leaked attributes, threeconsole.errorcalls. There is no lowercase warning to swallow — React does not have one.2. The warning that does fire is the camelCase one, for
mdColumns/smColumns/lgColumns/xlColumns/colorVariant:Note the trap in React's own remedy: spelling it lowercase silences the warning while keeping the leak — it converts case 2 into case 1.
3. Vitest 4's reporter defaults to
silent: 'passed-only', so console output from a passing test is discarded. Measured directly — two tests in one file, identicalconsole.error, only the failing one's output survives:MARKER-FROM-PASSING-TESTappears nowhere in the run output. This makes the warning structurally incapable of turning a test red: to be seen at all, something else must have already failed. A grid that renders fine while leaking attributes is precisely the passing case.4. React latches the warning per prop name per module instance, so it fires only on the first render carrying a given key. This is not theory — it bit this PR. Case 3 of the new test, written against the shared canary, passed against the leaking renderer, because cases 1 and 1b render first and consume the latch. Reverse-verification caught it green on code leaking ten attributes; it now authors its own
zzCanaryCamelkey, used nowhere else in the file, and goes red as it should.There is also no
console.error-as-failure pin anywhere invitest.setup.base.ts/vitest.setup.dom-light.tsx/vitest.setup.dom.tsx, so even a printed warning would only be log noise.Recommendation: a warning-as-error pin is not the right guard here
I recommend against it as the answer to this class, for one decisive reason: it is blind to the majority of this very defect. It can only ever catch case 2 — five of the ten leaked attributes here, and neither of the two the card's title names. A green warning-as-error pin would read as "this class is closed" while
columns="[object Object]"sailed past it untouched. That is a phantom check, and reasons 3 and 4 above mean it would also be order- and reporter-sensitive.The mechanism that actually closes the class is the one this repo already built: read the DOM, not the console.
packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsxchecks every rendered attribute against what HTML defines, and catches lowercase and camelCase alike. Case 1 of the new test applies that technique to this renderer.The real gap is that the sweep's targets are four plugin packages and it does not reach the
packages/componentsrenderers at all — which is whyui:gridleaked unobserved through #4008's ratchet and #4011's implementation. Extending it there is the pin worth having, it would have caught this, and it lands outside this card's file surface, so it is filed rather than built: #5574 (sub-issue of #4425), which also records thatflex.tsx,stack.tsx,container.tsxandtext.tsxcarry the identical unfixed spread.Tests
packages/components/src/__tests__/grid-dom-attribute-whitelist.test.tsx, four cases.Case 1 sweeps every attribute on the element against the declared DOM-safe set rather than naming today's bad keys — so a key added to
GridSchematomorrow is covered without editing the test, and the open tail is covered at all. Case 1b covers the responsivecolumnsobject. Case 2 is a positive control: the computed grid classes, the authoredclassNamemerged with them,id,role,aria-*,data-*includingdata-obj-id/data-obj-type, the forwardedstyle, and the children. Case 3 is the React-warning ratchet described above, kept explicitly labelled as the weaker half.Reverse-verification (fix reverted to
origin/main, mutation confirmed on disk bygrep -c toDomProps= 0 /gridProps= 2, restored via anEXITtrap):Case 2 staying green against the leaking renderer is the point: it proves it is a control, not the detector — a fix that stripped everything would fail it while cases 1/1b passed.
Gates
All run at
020140073(the final commit), from the worktree root:pnpm --filter @object-ui/components type-checktsc --noEmit && tsc -p tsconfig.test.jsonpnpm --filter @object-ui/components lint896 problems (0 errors, 896 warnings), all pre-existingreact-refresh/only-export-componentspnpm exec vitest run packages/components/Test Files 174 passed (174),Tests 1576 passed (1576)check:control-bytes✅ check-control-bytes: OK (scanned 4630 tracked text file(s); skipped 85 binary)check:doc-types✅ Every documented component type is registered.check-changeset-presence✅ 2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-no-major✅ No changeset declares a major bump.check-changeset-fixed✅ All workspace packages are in the changeset fixed group.Declared narrowing, two items — read these as narrowing, not as passes:
check:doc-snippetswas NOT run. It refuses to run on this worktree:@object-ui/plugin-view … is not on disk — run the build first, 11 packages unbuilt, and it says so explicitly (The snippet program was NOT run). Building their full closures is effectively a whole-workspace build, which CI does anyway. It cannot observe this diff regardless: it is a type-level gate over doc snippets compiled against built package types, andgrid.tsxhas zeroexportstatements — it is a pure side-effect registration module (import './grid'), contributing nothing to any package's.d.ts.check:doc-types, the gate that actually checks documented component types against the registry, did run and is green.pnpm lint/pnpm type-check(turbo run …) were scoped to@object-ui/components. The package's own lint ran over the whole package, so there is no file-level narrowing within it; the narrowing is at package level, and it is sound because the diff touches only thepackages/componentstree plus.changeset/, andeslint.config.jsenables no type-aware linting (noparserOptions.project, noprojectService), so this diff cannot move any untouched package's verdict.Also swept: no snapshot or fixture anywhere in the repo pinned the leaked spellings (
grep -rn "mdcolumns|smcolumns|lgcolumns|xlcolumns"outside the changed files returns nothing), so no fixture triage was needed.Generated by Claude Code