Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-dashboard): keep schema-shaped props off the KPI card DOM (#4357) - #4428
Merged
Conversation
…4357) MetricWidget and MetricCard ended their prop lists with `...props` and spread it onto the Card. Reached through SchemaRenderer — every dashboard KPI tile is — that spread also received the node's own metadata, and React writes unknown lowercase attributes straight to the DOM, stringifying objects. Every KPI card carried `schema="[object Object]"`. Measured at the SchemaRenderer call site: six props arrive that are not HTML attribute names — schema, events, props, bind, ariaLabel, ariaDescribedBy. They are destructured out in both components; the spread survives for everything that IS one (id, name, role, disabled, aria-*, data-*, className), which is the components' only accessibility passthrough. The list and its measurement live once, in schemaHostProps.ts. The cost was never visible — it was that the defect poisoned the assertion this area attracts. #4032 had to assert on the card heading because the container pin was red for a reason unrelated to labels. That workaround is replaced by the container assertion it was standing in for.
The first pass measured only what a SCHEMA can carry, and every fixture in this package renders a dashboard without a data source — so `dataSource` read `undefined` and wrote nothing, in every test. Measured again with an adapter attached, the KPI card carried `datasource="[object Object]"`: the adapter is not a schema key at all (SchemaRenderer strips the schema's own `dataSource` binding by name, objectstack#5576) but the object DashboardRenderer hands its SchemaRenderer call, arriving through the renderer's trailing props. That is the only one of the seven that leaked on a production dashboard rather than an authored edge case, and the pin could not see it. Case (g) now renders the dashboard WITH an adapter, so the shape only deployments had is a test.
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
yinlianghui
commented
Aug 12, 2026
CollaboratorAuthor
ACCEPT — PM 复核 (session
Flipping ready + arming auto-merge. Generated by Claude Code |
yinlianghui
marked this pull request as ready for review
August 12, 2026 05:42
Uh oh!
There was an error while loading. Please reload this page.
This was referenced Aug 12, 2026
Merged
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#4357
MetricWidgetandMetricCardended their prop lists with...propsand spread the whole thing onto the ShadcnCard. Reached throughSchemaRenderer— which every dashboard KPI tile is — the renderer hands a component its widget schema plus the schema's own keys, so SDUI metadata landed on the DOM: React passes unknown lowercase attributes straight through and stringifies object values.Measured, not assumed
One render carrying every SDUI key at once, through the real
SchemaRendererpath (packages/react/src/SchemaRenderer.tsx, theReact.createElement(Component, …)block). The props a widget receives that are not HTML attribute names, and the attribute each emitted before this change:schemaschema="[object Object]"eventsevents="[object Object]"propsprops="[object Object]"bindbind="data.revenue"ariaLabelarialabel="…"aria-labelariaDescribedByariadescribedby="…"aria-describedbydataSourcedatasource="[object Object]"Raw pre-fix DOM,
type: 'metric'throughSchemaRenderer:dataSourceis the one a schema-only measurement misses, and the only one that leaked on a production dashboard rather than an authored edge case. It is not a schema key —SchemaRendererstrips the schema's owndataSourcebinding by name (objectstack#5576) — it is the adapterDashboardRendererhands itsSchemaRenderercall (DashboardRenderer.tsx:735/:764), arriving through the renderer's trailing props. Every fixture in this package renders a dashboard without an adapter, so it readundefinedand wrote nothing in every test, while every deployment that actually loads data putdatasource="[object Object]"on the card. My first pass enumerated six keys and shipped that gap; the second measurement, with an adapter attached, found it. Case (g) of the pin now renders the dashboard with an adapter, so the shape only production had is a test.All seven are destructured out in both components. The spread survives: everything that is a DOM attribute still reaches the element unchanged —
id,name,role,disabled,aria-*,data-*,className. Removing the spread would have deleted the components' only accessibility passthrough. The list and its measurement live in one place,packages/plugin-dashboard/src/schemaHostProps.ts, rather than being copied into two components. The line drawn is "is the key an HTML attribute name".The pin, and the workaround it replaces
Pre-fix RED, both components at
origin/main(MetricWidget.domProps.test.tsx):(e) and (f) are the acceptance boundary and were predicted to pass on both sides: nothing about the render moves except the bogus attributes disappearing. Post-fix the whole package is green — 43 files, 363 tests.
DashboardRenderer.metricI18n.test.tsx(#4032) asserted on the card heading because the container assertion was unwritable: the card carriedschema="[object Object]"before and after any i18n fix, so the natural pin was red for a reason unrelated to labels and the tempting repair was to loosen it. That workaround is removed — cases (b) and (b2) now assertnot.toContain('[object Object]')on the container, the comment points at this fix, and the now-unusedcardHeading()helper goes with it.Reverse verification
Committed first, then took one component's destructuring out with
git checkout origin/main -- FILE(nevergit stash), predicting the split beforehand: (a)/(c)/(d)/(g) areMetricWidget's, (b) isMetricCard's.MetricWidgetrevertedMetricCardrevertedThe i18n cases going red only in the first run is the direct evidence that replacing #4032's workaround is load-bearing rather than cosmetic — that assertion now fails if this fix regresses.
Type surface, against the rebuilt
dist/index.d.tsfrom a consumer package: a probe using both components normally, passing the injected metadata, and assigningMetricCardto a narrowerReact.FCcompiles clean; a negative probe stays red on a badlabel, acolorVariantoutside the vocabulary, a badtrendand an undeclared prop — so tsc read the new declaration, not a cached one.One thing the reviewer should weigh: this repo already has a decided answer for this class
objectui#3291 / PR #3313 closed the same defect in
packages/fieldswith a whitelist (toDomProps), and its docstring argues explicitly against the deny-list shape — "a blacklist enumerating today's renderer-only keys … would not stop the next authored key either" — while naming this very path as the harder one, "SchemaRenderer… has no strip layer at all, so on the SDUI path a widget's own spread is the ONLY line of defence".This PR is a deny-list because that is what the ruling on #4357 specified (destructuring enumeration; allow-list only if enumeration is impossible). It is correct and verified for the seven measured props, and the
dataSourcemiss above is a live miniature of the weakness that doc predicted. What remains uncovered is the open tail: an authored key a component does not declare still reaches the DOM (measured —props: { colorVariant }onmetric-cardlands ascolorvariant="success", sinceMetricCardhas no such prop). Filed as #4425 with both options and the sweep-gate question, rather than changed unilaterally here.Also filed: #4426 — the mirror image, pre-existing and untouched. Neither props interface declares the DOM passthrough the spread accepts, so
id/role/aria-labelare a type error for a TS consumer while working at runtime.Changeset
patch. The exportedMetricWidgetProps/MetricCardPropsinterfaces are byte-identical — the components' accepted props widen only by the optional, ignoredSchemaHostPropskeys, which is additive and narrows nothing, so the #4403 precedent (minor for a props-type change) does not apply. Never major, per the fixed-group rule.Verification
pnpm exec vitest run packages/plugin-dashboard/from the repo root — 43 files, 363 tests, greentsc --noEmitinpackages/plugin-dashboard(notsconfig.test.jsonexists here — confirmed, matching PR fix(dashboard,report): localize a LOCAL select dimension on table/pivot and the dataset report block (#4330) #4388's report)--filter '@object-ui/plugin-dashboard^...' build), then a package rebuild before judging any typeeslinton the five touched files: 0 errorscheck-control-bytes,check-phantom-dependencies,check-changeset-presence,check-changeset-fixed,check-changeset-no-major: greenConsumer sweep, honest about its limit:
--filter '...@object-ui/plugin-dashboard'(prefix = the five downstream consumers, not the suffix form that walks upstream) type-checks red — but all 204 errors areCannot find module '@object-ui/auth' | '@object-ui/layout' | '@object-ui/plugin-*', i.e. packages whosedista dependency-scoped build never produces, and not one names a metric symbol. It is a missing-artifact red, so it neither confirms nor denies anything about this change; the type evidence that does count is the consumer-side probe against the rebuilt.d.tsabove.Generated by Claude Code