fix(plugin-dashboard): declare the DOM pass-through the metric props accept (#4426) - #4435
Merged
Merged
Conversation
…accept (#4426) `MetricWidgetProps` / `MetricCardProps` end their prop list with a `...domProps` spread onto the Shadcn `Card`, kept deliberately by #4357, but declared none of it — so `id` / `role` / `aria-label` were a TS error for a direct consumer while working at runtime. `MetricWidgetProps` now extends `React.HTMLAttributes<HTMLDivElement>` and `MetricCardProps` extends the same minus `title` (its heading, an `I18nLabel`, which never reached the DOM). The seven schema-shaped keys `SchemaRenderer` injects stay undeclared in `SchemaHostProps`. Zero runtime change. 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
|
…ow typetests project CI caught the first attempt: `scripts/__tests__/check-type-check-coverage.test.ts` pins objectui#4291's ratchet as a repository-state test — a `tsconfig.typetests.json` reappearing ANYWHERE turns it red. The gate script permits the shape; the ratchet forbids a new user. The narrow project is removed and `type-check` goes back to plain `tsc --noEmit`. The compile-time assertions move to `src/domPassthroughPins.ts`, which the package's own `tsc --noEmit` already compiles — `widgets/toDomProps.ts`'s shape in @object-ui/fields. They are `type`-only, so they emit zero runtime bytes. Reverting either `extends` turns them red under the ordinary type-check. The test file keeps only its RUNTIME assertions; its `@ts-expect-error` cases are dropped rather than left in a file nothing compiles, which would have read as coverage while checking nothing (objectui#3181). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Qqyix2QcnpUC9XeYVDzx3
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
Collaborator
Author
|
ACCEPT — PM 复核 (session
Flipping ready + arming auto-merge. Generated by Claude Code |
yinlianghui
marked this pull request as ready for review
August 12, 2026 07:10
This was referenced Aug 12, 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.
Fixes #4426
MetricWidgetandMetricCardend their prop list with a...domPropsspread onto the ShadcnCard. PR #4428 kept that spread deliberately — it is the components' only accessibility pass-through, and removing it would delete the only way a host can put anid, aroleor anaria-labelon a KPI card. Neither props interface declared any of it, so the type refused what the runtime accepted. This is the mirror image of #4357: that issue was renderer metadata reaching the DOM because the spread is open; this is DOM attributes being unreachable because the type was closed.Zero runtime change — no component body was touched, and the added pins are
type-only, so they emit zero runtime bytes.The convention, measured
The ruling asked for the repo's own spelling, not a third one. Counted over
packages/*/src, an exported*Propsinterface that spreads onto a host element extendsReact.HTMLAttributes< HTMLDivElement >— 7 occurrences, 6 first-party plus shadcn's own:PageHeaderComponentPropslayoutChatbotPropsplugin-chatbotChatbotEnhancedPropsplugin-chatbotTypingIndicatorPropsplugin-chatbotRefreshIndicatorPropscomponents/customFieldPropscomponents/customBadgePropscomponents/ui(upstream shadcn, no-touch zone)PageHeaderComponentPropsis the closest analogue and states the convention in its own doc comment: "it extendsHTMLAttributesso every DOM prop rides along."The alternative the issue named — an explicit block mirroring
FieldWidgetDomProps(#3221) — is 1 occurrence, and it is not a competing style: it exists to be bound key-by-key to a runtime whitelist (toDomProps) in both directions, so that a declared key which is never forwarded is a compile error. These components have no such whitelist. Writing half of one here would declare a set nothing enforces — the exact "declared but not delivered" failure that type was built to prevent. Whether plugin widgets should get that whitelist is #4425, and if they do, the declared DOM set becomes that whitelist and the two are one change.So:
MetricWidgetProps extends React.HTMLAttributes< HTMLDivElement >MetricCardProps extends Omit< React.HTMLAttributes< HTMLDivElement >, 'title' >The one carve-out, and why it is the accurate contract rather than a workaround
MetricCard.titleis the card's heading, in theI18nLabelvocabulary; HTML'stitleis a tooltip string. The types are incompatible, so extending whole does not merely mis-describe the component — it does not compile. ButOmitis right on the merits regardless: the component destructurestitleout and renders it intoCardTitle, so notitleattribute has ever reached this element. Declaring the inherited DOMtitlewould type-check, read as a supported tooltip, and silently do nothing — objectui#3290 / objectui#3222's first-class failure mode. Pinned from the runtime side by case (c), which asserts the attribute is absent after a render.MetricWidgethas no such collision (its heading islabel) and extends the DOM attributes whole.The repo's spelling for this carve-out is
ComboboxProps—extends Omit< React.ButtonHTMLAttributes< HTMLButtonElement >, "value" | "onChange" >— omitted there for the same reason: the component's own contract owns the name.Two smaller notes, both documented at the declaration:
MetricWidget.onClickstays zero-arg (narrower than the inheritedMouseEventHandler< HTMLDivElement >, because the same handler is wired to Enter/Space where there is no mouse event to hand over; a zero-arg function is assignable to the inherited signature, so existing callers keep compiling), andprefixkeeps its value-prefix meaning over the inherited RDFa attribute of the same name and samestringtype.What is deliberately NOT declared
The seven schema-shaped keys
SchemaRendererinjects —schema/bind/events/props/ariaLabel/ariaDescribedBy/dataSource. None is an HTML attribute name, all seven are destructured out before the spread, and declaring them would re-assert as public contract exactly what #4428 stripped from the DOM. They stay inSchemaHostProps, intersected in at each component's own signature — #4428's shape, kept.A compile-time pin holds this from the type side:
Extract< keyof MetricWidgetProps, keyof SchemaHostProps >must benever, and likewise for the card. Written againstkeyof SchemaHostPropsrather than a copied list, so a key added there is covered automatically.Correction 1: accepted is not declared
My first draft of the probe put a
@ts-expect-erroron a consumer passingschema={…}directly. That directive is unused (TS2578), and the probe was wrong. Both components are declaredMetricWidgetProps & SchemaHostPropsat their own signature, so the renderer's keys are accepted by the component — deliberately, becauseSchemaRendererhas to be able to inject them. The true and narrower claim is theExtract/neverpin above: they are not on the exported props interface, so they never become documented authoring surface, and they are still stripped before the spread. Accepted-and-dropped, not declared.Correction 2: where compile-time pins are allowed to live — CI caught this one
The defect was type-only (
id/role/aria-labelreached the card the whole time), so nothing vitest runs can observe the fix's direction. The assertions had to be compiled by something, and this package's tests are compiled by nothing:tsconfig.jsonexcludes**/*.test.tsx,@object-ui/plugin-dashboardis the sole remainingTEST_DEBTentry inscripts/check-type-check-coverage.mjs(6 errors, #4118), and vitest erases types. That is objectui#3181 — assertions in an uncompiled test file read as coverage and are decoration.My first push reached for the narrow
tsconfig.typetests.jsonrescue hatch, whose gate rules explicitly describe it as the escape for a package still inTEST_DEBT. That was wrong, and CI said so —scripts/__tests__/check-type-check-coverage.test.tspins #4291's ratchet as a repository-state test:The gate script permits the shape; the ratchet forbids a new user. It is stated there in as many words: "a
tsconfig.typetests.jsonreappearing ANYWHERE turns this red."The narrow project is gone and
type-checkis back to plaintsc --noEmit. The assertions moved tosrc/domPassthroughPins.ts, a source module the package's owntsc --noEmitalready compiles —widgets/toDomProps.ts's shape in@object-ui/fields, which binds its DOM whitelist to its declaration the same way. This is strictly better than the project I first added: no extra config, no ratchet to violate, and the pins are enforced by the default type-check that CI's Type Check job runs. UnliketoDomProps.tsthey aretype-only, so they emit nothing at all — which matters, because this change must stay types-only.The test file correspondingly keeps only its runtime assertions. Its
@ts-expect-errorcases were deleted rather than left behind in a file nothing compiles, where they would have read as a negative pin while checking nothing.Verification
pnpm exec vitest run scripts/__tests__/check-type-check-coverage.test.ts packages/plugin-dashboard/ --maxWorkers=2— 45 files, 393 tests passed, including the test that failed on the first push. fix(plugin-dashboard): keep schema-shaped props off the KPI card DOM (#4357) #4428'sMetricWidget.domProps.test.tsxis untouched and green.tsc --noEmitinpackages/plugin-dashboard— exit 0, and it now compiles the pins.--filter '@object-ui/plugin-dashboard^...' build), then a package rebuild, before judging any type.turbo run type-check --concurrency=2— 78 successful, 78 total (run against the first push; the second push only moves assertions between files that the same run compiles). This is the no-downstream-red proof for an exported-type widening; the 5 downstream consumers (console,site,console-starter,app-shell,byo-backend-console) were enumerated with the prefix filter...@object-ui/plugin-dashboard(the downstream direction — the suffix form walks upstream instead) and all are inside that run.eslinton the four touched source files: 0 errors. The 11 warnings areno-unused-varson the compile-time assertion aliases — the same shape as the established idiom inflow-designer-edge.types.test.ts, andlint.ymldeliberately does not set--max-warnings. Package-widelintis exit 0.check:control-bytes,check:phantom-deps,changeset:check,check-changeset-presence,type-check:coverage: all green — the last now back to reporting0 with a narrow type-assertion project. Plus a self-scan of every touched file for control bytes beyond the gate's surface: no hits.Reverse verification — direction predicted before each run
Method:
git checkout origin/main --the two component files (nevergit stash), rebuilddist, re-measure, restore. Consumer probes compiled against the rebuiltdist/*.d.tsfrom a real consumer package (apps/console), never siblingsrc/, per #4428's method.id/role/aria-label/aria-describedby/tabIndexdata-*onlysrc/domPassthroughPins.tsunder plaintsc --noEmitThe before-state positive probe reproduces the issue's own report:
The negative probe is red on both sides with byte-identical errors —
bogusProprejected on both components, andcolorVariant: "chartreuse"still rejected against the closed vocabulary. That is the assertion that the widening opened no[key: string]: any; its committed counterpart is theRejectsBoguspin, green on both sides for the same reason.The pins file failed in exactly the two predicted places and nowhere else, under the package's ordinary type-check:
The negative pins and the
Extract/neverrenderer-key pins reported nothing in either state, as predicted. That is what makes the pins load-bearing rather than decorative.One measured correction to the fix's framing
The ruling named
id/role/aria-label/data-*as what must type-check.data-*was never blocked. The control probe —data-testidanddata-obj-idwith no other DOM prop — is green before the change as well as after, because TypeScript does not type-check a JSX attribute whose name is not a valid identifier. So the widening genuinely fixes the first three;data-*is reported as already working rather than claimed as newly fixed. It is still exercised in the runtime test, since it is part of the pass-through a consumer will write.Changeset
minor, per the #4403 precedent — two exported interfaces widen. Never major, per the fixed-group rule. The widening is purely additive for existing callers: every prop that compiled before still compiles, nothing narrows, and no source change is required to upgrade. Both semantic decisions (titlestays the heading,onClickstays zero-arg) are written into the changeset body rather than left to the diff.Generated by Claude Code