diff --git a/.changeset/metric-props-dom-passthrough-4426.md b/.changeset/metric-props-dom-passthrough-4426.md new file mode 100644 index 000000000..5f58bd6fa --- /dev/null +++ b/.changeset/metric-props-dom-passthrough-4426.md @@ -0,0 +1,20 @@ +--- +'@object-ui/plugin-dashboard': minor +--- + +`MetricWidgetProps` / `MetricCardProps` declare the DOM pass-through their spread has always accepted + +Both KPI components end their prop list with a `...domProps` spread onto the Shadcn `Card`, and objectui#4357 (PR #4428) kept that spread deliberately — it is their only accessibility pass-through, and removing it would delete the only way a host can put an `id`, a `role` or an `aria-label` on a KPI card. Neither props interface declared any of it. So the type refused what the runtime accepted: a JS consumer, and every SDUI author going through `SchemaRenderer` (untyped at that boundary), got the pass-through, while a TypeScript consumer importing the component directly got `error TS2322` on `id` / `role` / `aria-label` and needed a cast. + +`MetricWidgetProps` now extends `React.HTMLAttributes`, and `MetricCardProps` extends the same minus `title`. That is the repo's measured convention for an exported props interface that spreads onto a host element (`PageHeaderComponentProps`, `ChatbotProps`, `ChatbotEnhancedProps`, `TypingIndicatorProps`, `RefreshIndicatorProps`, `FieldProps`, and shadcn's `BadgeProps`), and the `Omit` carve-out is `ComboboxProps`'s spelling for a name the component's own contract owns. + +Graded `minor` rather than `patch` per the objectui#4403 precedent: two exported interfaces widen. The widening is purely additive for existing callers — every prop that compiled before still compiles, and nothing narrows — so no source change is required to upgrade. + +Semantics worth knowing, because both are contract statements rather than incidental: + +- **`MetricCard.title` stays the heading.** HTML's `title` is a tooltip; this card's `title` is its heading, in the `I18nLabel` vocabulary, destructured out and rendered into `CardTitle`. No `title` attribute has ever reached this element, so the inherited DOM `title` is omitted rather than declared and silently dropped — the "declared but not delivered" failure this repo treats as first-class (objectui#3290, objectui#3222). `MetricWidget` has no such collision (its heading is `label`) and extends the DOM attributes whole. +- **`MetricWidget.onClick` stays zero-arg**, narrower than the inherited `MouseEventHandler`, 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 callers already passing `(e) => …` keep compiling. + +Not declared, deliberately: the schema-shaped keys `SchemaRenderer` injects (`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 PR #4428 stripped from the DOM. They stay in `SchemaHostProps`, intersected in at each component's own signature — accepted so the renderer can inject them, never part of the documented authoring surface. + +Zero runtime change: no component body was touched, and PR #4428's pins pass untouched. diff --git a/packages/plugin-dashboard/src/MetricCard.tsx b/packages/plugin-dashboard/src/MetricCard.tsx index b2e36c64c..9b366b57c 100644 --- a/packages/plugin-dashboard/src/MetricCard.tsx +++ b/packages/plugin-dashboard/src/MetricCard.tsx @@ -14,7 +14,25 @@ import type { I18nLabel } from '@object-ui/types'; import { ArrowDownIcon, ArrowUpIcon, MinusIcon, AlertCircle, Loader2 } from 'lucide-react'; import type { SchemaHostProps } from './schemaHostProps'; -export interface MetricCardProps { +/** + * DOM PASS-THROUGH (objectui#4426) — see `MetricWidget.tsx`'s interface header + * for the full argument; this is the same widening on the same spread, onto the + * same Shadcn `Card` (`div`). + * + * `title` is the one key `Omit`-ed, and the omission is the accurate contract + * rather than a workaround. HTML's `title` is a tooltip string; this card's + * `title` is its HEADING, in the `I18nLabel` vocabulary — an incompatible type, + * and one the component destructures out and renders into `CardTitle`, so no + * `title` attribute has ever reached this element. Declaring the inherited DOM + * `title` here would be the "declared but not delivered" failure this repo + * treats as first-class (objectui#3290, objectui#3222): it would type-check, + * read as a supported tooltip, and silently do nothing. `MetricWidget` has no + * such collision — its heading is `label` — so it extends the DOM attributes + * whole. The repo's spelling for this carve-out is `ComboboxProps` + * (`extends Omit, "value" | "onChange">`), + * omitted there for the same reason: the component's own contract owns the name. + */ +export interface MetricCardProps extends Omit, 'title'> { /** * Card heading, in @objectstack/spec's `I18nLabel` vocabulary — a plain * string or an inline per-locale map. See `MetricWidget.label` for why the diff --git a/packages/plugin-dashboard/src/MetricWidget.tsx b/packages/plugin-dashboard/src/MetricWidget.tsx index 79716c153..4c67a6c58 100644 --- a/packages/plugin-dashboard/src/MetricWidget.tsx +++ b/packages/plugin-dashboard/src/MetricWidget.tsx @@ -149,7 +149,44 @@ function formatMetricValue( */ export type { MetricColorVariant }; -export interface MetricWidgetProps { +/** + * DOM PASS-THROUGH (objectui#4426). `MetricWidget` ends its prop list with a + * `...domProps` spread onto the Shadcn `Card` — a `div` — so `id`, `role`, + * `aria-*`, `data-*`, `tabIndex`, `title` and the DOM event handlers reach the + * element. objectui#4357 (PR #4428) deliberately KEPT that spread: it is the + * component's only accessibility passthrough, and removing it would delete the + * only way a host can put an `id` or an `aria-label` on a KPI card. + * + * Until now the type refused what the runtime accepted. A TypeScript consumer + * importing the component directly could not write `` without a cast, while a JS + * consumer and every SDUI author going through `SchemaRenderer` (untyped at + * that boundary) got the passthrough for free. This `extends` is that + * declaration — the mirror image of #4357, closing the same question from the + * other side: renderer metadata was reaching the DOM because the spread is + * open; DOM attributes were unreachable because the type was closed. + * + * Measured convention, not an invented spelling: an exported `*Props` interface + * that spreads onto a host element extends `React.HTMLAttributes` + * here — `PageHeaderComponentProps` (whose own doc says "it extends + * `HTMLAttributes` so every DOM prop rides along"), `ChatbotProps`, + * `ChatbotEnhancedProps`, `TypingIndicatorProps`, `RefreshIndicatorProps`, + * `FieldProps`, and shadcn's own `BadgeProps`. + * + * What this does NOT declare, deliberately: the schema-shaped keys + * `SchemaRenderer` injects (`schema` / `bind` / `events` / `props` / + * `ariaLabel` / `ariaDescribedBy` / `dataSource`). None of them is an HTML + * attribute name, all seven are destructured out and never reach the DOM, and + * declaring them here would re-assert as a public contract exactly what #4357 + * stripped. They stay in `SchemaHostProps`, intersected in at the component's + * own signature — the renderer's private door, not the consumer's front door. + * The narrower `FieldWidgetDomProps` shape (objectui#3221) is the other spelling + * in this repo, but it exists to be bound key-by-key to a runtime whitelist + * (`toDomProps`) in both directions; this component has no such whitelist, and + * inventing a half of one here would declare a set the spread does not enforce. + * Whether plugin widgets should get that whitelist is objectui#4425. + */ +export interface MetricWidgetProps extends React.HTMLAttributes { /** * The KPI's heading, in @objectstack/spec's `I18nLabel` vocabulary — a plain * string or an inline per-locale map (`{ en: 'Revenue', 'zh-CN': '收入' }`). @@ -181,11 +218,27 @@ export interface MetricWidgetProps { format?: string; /** ISO currency code (e.g. `'USD'`); enables currency formatting on numeric values. */ currency?: string; - /** Static prefix appended in front of the formatted value (e.g. `'$'`, `'¥'`). */ + /** + * Static prefix appended in front of the formatted value (e.g. `'$'`, `'¥'`). + * + * Same name, same `string` type as the inherited RDFa `prefix` attribute, and + * this declaration wins: the component reads it and destructures it out, so + * it never reaches the DOM. That is the accurate contract — it was already + * true before this interface declared its passthrough. + */ prefix?: string; /** Static suffix appended after the formatted value (e.g. `' /mo'`). */ suffix?: string; - /** When set, the entire card becomes clickable and emits this handler. */ + /** + * When set, the entire card becomes clickable and emits this handler. + * + * Deliberately NARROWER than the inherited `MouseEventHandler`: + * the KPI card treats a click as "the tile was activated" and wires the same + * handler to Enter/Space, where there is no mouse event to hand over. A + * zero-arg function is assignable to the inherited signature, so consumers + * already passing `(e) => …` keep compiling; this only stops the component + * from promising an event object its keyboard path cannot produce. + */ onClick?: () => void; /** * Layout variant. `'card'` (default) is the bordered KPI card; `'bare'` drops diff --git a/packages/plugin-dashboard/src/__tests__/MetricWidget.domPassthrough.test.tsx b/packages/plugin-dashboard/src/__tests__/MetricWidget.domPassthrough.test.tsx new file mode 100644 index 000000000..dda92dbb6 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/MetricWidget.domPassthrough.test.tsx @@ -0,0 +1,138 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * objectui#4426 — the RUNTIME half of the KPI cards' DOM pass-through contract. + * + * Both components end their prop list with a `...domProps` spread onto the + * Shadcn `Card`, and objectui#4357 (PR #4428) kept that spread deliberately: it + * is their only accessibility pass-through. But neither props interface extended + * `React.HTMLAttributes`, so what the runtime accepted the TYPE refused: + * + * ``` + * probe.tsx(11,5): error TS2322: Type '{ label: string; value: number; id: string; + * role: string; "aria-label": string; … }' is not assignable to type + * 'IntrinsicAttributes & MetricWidgetProps & SchemaHostProps'. + * Property 'id' does not exist on type '…'. + * ``` + * + * A JS consumer, and every SDUI author going through `SchemaRenderer` (untyped + * at that boundary), got the pass-through; a TypeScript consumer importing the + * component directly needed a cast. + * + * ## What is asserted WHERE, and why the split is not arbitrary + * + * The defect was type-only — `id` / `role` / `aria-label` reached the card the + * whole time — so the fix's own direction cannot be observed by anything vitest + * runs. The compile-time assertions therefore live in `../domPassthroughPins.ts`, + * a SOURCE module the package's `tsc --noEmit` actually compiles. They are not + * here, and deliberately so: this package's tests are compiled by nothing + * (`tsconfig.json` excludes `**\/*.test.tsx`, the package is the sole remaining + * `TEST_DEBT` entry in `scripts/check-type-check-coverage.mjs`, and vitest erases + * types), which is objectui#3181 — assertions in an uncompiled test file read as + * coverage and are decoration. A `@ts-expect-error` written here would be + * especially dishonest: nothing would ever check that the error it expects still + * happens. + * + * What IS real here, and only here: that those attributes actually land on the + * element, and that `title` does NOT. Case (c) is the one that would catch the + * tempting bad fix for the `title` collision — declaring the inherited DOM + * `title` instead of omitting it would type-check, read as a supported tooltip, + * and silently do nothing (objectui#3290 / objectui#3222's first-class failure). + * + * DIRECTIONS, written before the run: all three cases are GREEN on both sides of + * #4426 at RUNTIME — the spread already forwarded these attributes, and this + * change adds no runtime behaviour at all. They are a regression floor for the + * spread `MetricWidget.domProps.test.tsx` case (e) protects from the other side, + * not evidence of the type fix. The type fix's before/after direction is the + * pins file, and is recorded in the PR. + */ + +import * as React from 'react'; +import { describe, it, expect, afterEach } from 'vitest'; +import { render, cleanup } from '@testing-library/react'; +import { I18nProvider } from '@object-ui/i18n'; +import { MetricWidget } from '../MetricWidget'; +import { MetricCard } from '../MetricCard'; + +afterEach(cleanup); + +function wrap(ui: React.ReactElement) { + return render( + + {ui} + , + ); +} + +describe('MetricWidget / MetricCard — the declared DOM pass-through reaches the element (#4426)', () => { + it('(a) MetricWidget carries a direct consumer\'s DOM identity and ARIA', () => { + const { container } = wrap( + , + ); + + const el = container.firstElementChild as HTMLElement; + expect(el.getAttribute('id')).toBe('revenue'); + expect(el.getAttribute('role')).toBe('region'); + expect(el.getAttribute('aria-label')).toBe('Revenue KPI'); + expect(el.getAttribute('aria-describedby')).toBe('revenue-desc'); + expect(el.getAttribute('tabindex')).toBe('0'); + expect(el.getAttribute('data-testid')).toBe('revenue-kpi'); + expect(el.className).toContain('kpi-tile'); + // Zero runtime change: the KPI still renders exactly as before. + expect(container.textContent).toContain('Total Revenue'); + expect(container.textContent).toContain('1,930,000'); + }); + + it('(b) MetricCard does the same through its own props', () => { + const { container } = wrap( + , + ); + + const el = container.firstElementChild as HTMLElement; + expect(el.getAttribute('id')).toBe('revenue-card'); + expect(el.getAttribute('role')).toBe('region'); + expect(el.getAttribute('aria-label')).toBe('Revenue KPI'); + expect(el.getAttribute('aria-describedby')).toBe('revenue-desc'); + expect(el.getAttribute('tabindex')).toBe('0'); + expect(el.getAttribute('data-testid')).toBe('revenue-card'); + expect(container.textContent).toContain('Total Revenue'); + }); + + it('(c) MetricCard\'s `title` is the heading and never becomes a DOM tooltip', () => { + // The reason `MetricCardProps` OMITS the inherited `title` rather than + // declaring it. If a later edit "aligns" the interface by declaring the DOM + // `title`, this stays green — but the type would then promise a tooltip the + // component drops on the floor, which is what the pins file forbids and what + // this case documents from the runtime side. + const { container } = wrap(); + + const el = container.firstElementChild as HTMLElement; + expect(el.getAttribute('title')).toBeNull(); + expect(container.textContent).toContain('Total Revenue'); + }); +}); diff --git a/packages/plugin-dashboard/src/domPassthroughPins.ts b/packages/plugin-dashboard/src/domPassthroughPins.ts new file mode 100644 index 000000000..820e97889 --- /dev/null +++ b/packages/plugin-dashboard/src/domPassthroughPins.ts @@ -0,0 +1,125 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +/** + * Compile-time pins for the KPI components' DOM pass-through contract + * (objectui#4426). + * + * ## Why these live in `src/` and not in a test file + * + * They are TYPE assertions, and this package's tests are compiled by nothing: + * `tsconfig.json` is the build and excludes `**\/*.test.tsx`, + * `@object-ui/plugin-dashboard` is the sole remaining `TEST_DEBT` entry in + * `scripts/check-type-check-coverage.mjs` (6 errors, objectui#4118), and vitest + * erases types before running. Putting them in a test file is objectui#3181 + * exactly — a provably-false `Assert>` there passes + * `pnpm type-check` at exit 0. + * + * The narrow `tsconfig.typetests.json` rescue hatch is NOT the answer either, + * however much the gate script's own comments read like an invitation: + * objectui#4291 retired the last six, and + * `scripts/__tests__/check-type-check-coverage.test.ts` now pins the terminal + * state as a repository-state test — "a `tsconfig.typetests.json` reappearing + * ANYWHERE turns this red". A first attempt at this change added one and CI said + * so. The gate permits the SHAPE; the ratchet forbids a new USER. + * + * So the assertions go where this repo already puts load-bearing compile-time + * assertions: in source, next to the contract, compiled by the package's own + * `tsc --noEmit`. That is `widgets/toDomProps.ts`'s shape in `@object-ui/fields`, + * which binds its DOM whitelist to its declaration in both directions the same + * way. Unlike that file these are `type`-only, so they emit ZERO runtime bytes — + * which matters here, because #4426 is a types-only change and must stay one. + * + * ## What each direction catches + * + * The defect being closed was invisible to every runtime test: `id` / `role` / + * `aria-label` reached the card the whole time, so nothing vitest runs could + * observe either the bug or its fix. The runtime half — that those attributes + * really do land, and that `title` really does not — is + * `__tests__/MetricWidget.domPassthrough.test.tsx`. This file is the other half. + */ + +import type { MetricWidgetProps } from './MetricWidget'; +import type { MetricCardProps } from './MetricCard'; +import type { SchemaHostProps } from './schemaHostProps'; + +/* The repo idiom — see + * `packages/app-shell/src/views/metadata-admin/previews/flow-designer-edge.types.test.ts`. + */ +type Assert = T; +type Extends = [A] extends [B] ? true : false; +type IsAny = 0 extends 1 & T ? true : false; +type Equal = (() => T extends A ? 1 : 2) extends () => T extends B ? 1 : 2 + ? true + : false; + +/** + * Guard against the pins lying: were either side `any`, every assertion below + * would pass while proving nothing (objectstack#4171's failure mode, and the + * reason `flow-designer-edge.types.test.ts` opens the same way). + */ +type _WidgetNotAny = Assert, false>>; +type _CardNotAny = Assert, false>>; + +/** + * POSITIVE — the declaration this issue exists for. These are the keys PR #4428 + * measured as still flowing through the spread after it stripped the seven + * schema-shaped ones. Reverting either `extends` turns both of these red. + */ +type DomIdentity = 'id' | 'role' | 'aria-label' | 'aria-describedby' | 'tabIndex'; +type _WidgetAcceptsDomIdentity = Assert>; +type _CardAcceptsDomIdentity = Assert>; + +/** + * NEGATIVE — the widening is not an open door. An `[key: string]: any` would + * collapse `keyof` to `string | number` and flip both of these to `true`. + * + * Green on BOTH sides of #4426 by design: the bogus key was rejected before the + * widening and must still be rejected after it. That is the assertion, and it is + * the one a "just make the complaint go away" edit would break. + */ +type _WidgetRejectsBogus = Assert, false>>; +type _CardRejectsBogus = Assert, false>>; + +/** + * NEGATIVE — objectui#4357's half, restated from the type side. + * + * The schema-shaped keys `SchemaRenderer` injects are stripped, not passed + * through. They stay in `SchemaHostProps` and are intersected in at each + * component's own signature, so the renderer can still inject them — but they + * must never reach the EXPORTED props interface, or this widening would + * re-assert as public authoring surface exactly what PR #4428 removed from the + * DOM. Written against `keyof SchemaHostProps` rather than a copied list, so a + * key added there is covered here automatically. + */ +type RendererKeys = keyof SchemaHostProps; +type _WidgetKeepsRendererKeysPrivate = + Assert, never>>; +type _CardKeepsRendererKeysPrivate = + Assert, never>>; + +/** + * The two carve-outs, pinned so a future edit cannot quietly undo the reasoning. + * + * `MetricCard.title` stays the HEADING (`I18nLabel` — a string or an inline + * per-locale map), not HTML's tooltip string. Dropping the `Omit` does not + * merely change this type, it stops the interface compiling; this pin is here so + * the next reader learns which `title` won and why. + */ +type _CardTitleTakesAnI18nMap = + Assert>>; + +/** + * `MetricWidget.onClick` narrows the inherited `MouseEventHandler` to a zero-arg + * handler, because the same handler is wired to Enter/Space where there is no + * mouse event to hand over. + */ +type _WidgetOnClickIsZeroArg = + Assert, () => void>>; + +export {};