fix(plugin-dashboard): the widget grid passes only whitelisted DOM props to its container (#4432) - #4491
Merged
Conversation
…ops to its container (#4432) `view:dashboard` resolves to `DashboardRenderer`, so `SchemaRenderer` hands it the dashboard node's own keys, the contents of the node's `props` container, the resolved ARIA and the host's trailing props — and every key the component did not destructure was spread raw onto the widget-grid container. Measured through the real SDUI path: 13 non-DOM attributes. Migration step 2 of the #4425 phase-2 ruling (option 1 — the `toDomProps` whitelist is the SDUI widget contract), following #4431 / PR #4485's shape and consuming the `toDomProps` executor that PR lifted to `@object-ui/core`. Both layout branches convert; the spread goes FIRST so the component's own computed attributes stay authoritative. `onClick` — a whitelisted key the container also computes — now has one carrier instead of being replaced by the trailing spread. The `view:dashboard` ledger row is deleted and the sweep gate's header reading table is brought to current truth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — step-7 复核 by PM session
Flipping ready + arming auto-merge. #4434 (judge unification) unlocks on this landing — dispatching next with a poll-gate. Generated by Claude Code Generated by Claude Code |
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.
Closes #4432
Migration step 2 of the #4425 phase-2 ruling (comment): option 1 — promote the whitelist to the SDUI widget contract. It follows #4431 / PR #4485 exactly and consumes the shared
toDomPropsexecutor that PR lifted to@object-ui/core. No new dependency:plugin-dashboardalready declares@object-ui/core.The defect
view:dashboardresolves toDashboardRenderer, whose widget-grid container ended in a bare{...props}spread (packages/plugin-dashboard/src/DashboardRenderer.tsx).SchemaRendererhands a registered component the authored node's own keys, the contents of itspropscontainer, the ARIA it resolved, the evaluateddisabledverdict and the host's trailing props — so all of it became attributes, because React passes unknown lowercase attributes through in silence and stringifies object values. Measured through the real SDUI path with an adapter attached: 13 non-DOM attributes.datasourceis not among them — this component destructures the adapter and hands it to its ownSchemaRenderercalls, so it never reached the spread. That is the one key PR #4428 shipped a pass without, and it is pinned here anyway (see below).Both layout branches spread onto the same host element, so both convert: the responsive desktop grid and the mobile stack.
Red-first, three directions
1. The defect — pre-fix, with the ledger row temporarily removed so the leak is visible (
git checkout --to restore, nevergit stash):(the tag name is unbracketed above only because the stored body strips a bracket followed by a letter)
Baseline first, on the unmodified tree with all rows present:
Test Files 1 passed (1) · Tests 39 passed (39).2. The stale-row ratchet — post-fix, with the
view:dashboardrow still in place. This is the direction that makes the row expire with the defect:Row deleted in this PR →
Test Files 1 passed (1) · Tests 39 passed (39).3. The exact-set pin.
packages/plugin-dashboard/src/__tests__/DashboardRenderer.domProps.test.tsxrenders the dashboard throughSchemaRendererProviderwith a data-source adapter attached — a schema-only fixture cannot seedatasource, which is how #4428 shipped a six-key first pass — and asserts the container's attribute set exactly, so a key that stops being delivered is as red as a key that leaks. Reverse-verified by removing the fix and re-running: all 5 cases go red, the set case reporting the full 13 back on the element.The delivered set, asserted with values:
aria-describedby,aria-label,class,data-obj-id,data-obj-type,id,role,style,tabindex. Plus the negative sweep of the 13 named keys,schema/datasourceby name, a generic "no attribute value contains[object Object]" assertion, and the ARIA pair — camelCasearialabel/ariadescribedbygone while the resolvedaria-label/aria-describedbyremain with their values.Behaviour edges, stated exactly (the #4485 disabled-precedent check)
The old spread was trailing, so it silently overrode every computed prop on this container. Three things move, none of them silently:
onClicknow has ONE carrier. It is a declared DOM pass-through key of the contract and this container computes its own design-mode background handler (handleBackgroundClick, which deselects on a background click). The trailing spread resolved that collision by letting the incoming handler replace the computed one, so a host that passedonClicksilently lost background deselection. Dropping the incoming handler instead would have been the mirror failure — a whitelisted key that type-checks, reads as supported, and never arrives. Both run now, container affordance first, and both directions are pinned. A non-functiononClick(SDUI spells click behaviourevents: { onClick: [ActionDef] }, which is DATA and is dropped by the whitelist) is ignored rather than handed to React, which used to throwExpected 'onClick' listener to be a function, instead got a value of 'string' type.— also pinned.styleno longer replaces the computed grid layout.styleis not in the SDUI pass-through set, and this container computes its owngridTemplateColumns/gridAutoRows/gap; an authoredstyleused to overwrite all of it and collapse the grid. It is now dropped.data-user-actionsno longer overrides the value computed from theuserActionsprop. Thedata-*family still passes the whitelist; only this one collision with a computed attribute resolves the other way now.The injected
disabledverdict is dropped rather than forwarded. Unlike #4485's chatbot case there is no verdict to consume here: nothing in this component ever read it, so it only ever became adisabledattribute on a container element that has no such attribute. That is the leak, not a behaviour.Header truth-up (routed here from PR #4485's review)
The sweep file's docblock still carried the phase-1 reading (
plugin-chatbot | 3 | 2 | 14 each,plugin-dashboard | 8 | 3 | 7 / 9 / 13) and the sentence "5 of 23 targets leak" — both false after #4485 and after this PR. The table now reads:2 of 23 targets leak, and both are exactly what the ledger holds after this PR:
plugin-dashboard:metricandplugin-dashboard:metric-card, the open tail #4425 owns directly. Their rows are untouched. The header also now records that phase 2 is ruled (so the gate is the migration's ratchet rather than the measurement it was waiting for), which two steps closed which rows, and that the three packages reading 0 are not clean for the same reason — charts never spread, calendar takes a declared prop list, chatbot and this grid reach zero by filtering. The "divergence" section says plainly thatplugin-dashboardis now MIXED by design: this container filters throughtoDomPropswhile the two KPI components still run theschemaHostPropsdeny-list, and the two surviving rows are precisely that difference.Verification
pnpm exec vitest run packages/plugin-dashboard/ packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx—Test Files 46 passed (46) · Tests 410 passed (410).type-checkfor@object-ui/plugin-dashboardand@object-ui/app-shell(both tsc projects —tsc --noEmitandtsc -p tsconfig.test.json, which is what compiles the sweep file) — all Done, after building each package's dependency closure first.lintfor the same two — 0 errors. The first draft'suseCallbackon the composed handler drew twoCompilation Skipped: Existing memoization could not be preservedReact Compiler warnings; it closes overprops.onClickread out of the rest object, which the compiler cannot prove stable, so it is a plain handler now (0 new warnings; the file lands on a plain element with no memoized child).check-control-bytes(4179 files),check-changeset-presence,check-changeset-no-major,check-phantom-dependencies,check-lint-coverage,check-type-check-coverage— all green.Changeset:
@object-ui/plugin-dashboardpatch, graded by measurement rather than assumption — the package was built with and without the fix and the emitteddist/*.d.tsare byte-identical in both directions, so this is behaviour-only. No public type or export moved.Generated by Claude Code