diff --git a/.changeset/metric-widget-dom-spread-4357.md b/.changeset/metric-widget-dom-spread-4357.md new file mode 100644 index 000000000..a56632a8f --- /dev/null +++ b/.changeset/metric-widget-dom-spread-4357.md @@ -0,0 +1,47 @@ +--- +'@object-ui/plugin-dashboard': patch +--- + +KPI cards no longer write their own schema onto the DOM — `MetricWidget` and +`MetricCard` keep `SchemaRenderer`'s schema-shaped props out of the `...props` +spread (objectui#4357). + +Both components are two things at once: an SDUI block reached through +`SchemaRenderer`, and a plain React component a host may render directly. The +React half wants a `...props` spread on its root so callers can pass `aria-*`, +`data-*`, `id`, `role`. The SDUI half means that spread also received the node's +own metadata — and React writes unknown lowercase attributes straight to the DOM, +stringifying object values. Every KPI card therefore carried +`schema="[object Object]"`, and a widget authored with events, a binding or a +props container carried `events="[object Object]"`, `bind="data.revenue"` and +`props="[object Object]"` beside it. + +Seven props were measured arriving at the call site that are not HTML attribute +names — `schema`, `events`, `props`, `bind`, `ariaLabel`, `ariaDescribedBy` (the +last two are the camelCase authored forms of ARIA the renderer already emits in +their dashed spelling) and `dataSource`. They are destructured out; the spread +survives untouched for everything that IS a DOM attribute: `id`, `name`, `role`, +`disabled`, `aria-*`, `data-*`, `className`. Nothing else about the render moves +— no text, no class, no element. + +`dataSource` is the one that only a live dashboard shows. It is not a schema key +(the renderer strips the schema's own `dataSource` binding by name); it is the +injected adapter `DashboardRenderer` hands its `SchemaRenderer` call, which +arrives through the renderer's trailing props. Every fixture in this package +renders without an adapter, so it read `undefined` and wrote nothing — while +every deployment that actually loads data put `datasource="[object Object]"` on +the card. The pin renders a dashboard with an adapter so the case that only +production had is now a test. + +The cost of this was never visible; it was that the defect poisoned the +assertion this area attracts. objectui#4163 pins +`not.toContain('[object Object]')` on the dashboard grid, and objectui#4032 +wanted the same pin on the metric path but could not write it: the card carried +the attribute before and after any i18n fix, so the container assertion was red +for a reason unrelated to labels and the tempting repair was to loosen it. That +suite asserted on the card heading instead, with a comment. The workaround is +now removed and the container assertion is back. + +The exported `MetricWidgetProps` / `MetricCardProps` interfaces are unchanged — +the components' accepted props widen only by the optional, ignored +`SchemaHostProps` keys, so no consumer type narrows. diff --git a/packages/plugin-dashboard/src/MetricCard.tsx b/packages/plugin-dashboard/src/MetricCard.tsx index a9878f4d5..b2e36c64c 100644 --- a/packages/plugin-dashboard/src/MetricCard.tsx +++ b/packages/plugin-dashboard/src/MetricCard.tsx @@ -12,6 +12,7 @@ import { cn } from '@object-ui/components'; import { useObjectTranslation, pickLocalized } from '@object-ui/i18n'; import type { I18nLabel } from '@object-ui/types'; import { ArrowDownIcon, ArrowUpIcon, MinusIcon, AlertCircle, Loader2 } from 'lucide-react'; +import type { SchemaHostProps } from './schemaHostProps'; export interface MetricCardProps { /** @@ -36,7 +37,7 @@ export interface MetricCardProps { * MetricCard - Standalone metric card component for dashboard KPIs * Displays a metric value with optional icon, trend indicator, and description */ -export const MetricCard: React.FC = ({ +export const MetricCard: React.FC = ({ title, value, icon, @@ -46,7 +47,19 @@ export const MetricCard: React.FC = ({ className, loading, error, - ...props + // Schema-shaped props `SchemaRenderer` injects, destructured out so the + // spread below cannot write them to the DOM (objectui#4357). Named and + // measured in `./schemaHostProps`; `schema` alone put a + // `schema="[object Object]"` attribute on every card. The rest spread + // survives — it is the component's genuine DOM/aria passthrough. + schema: _schema, + bind: _bind, + events: _events, + props: _propsBag, + ariaLabel: _ariaLabel, + ariaDescribedBy: _ariaDescribedBy, + dataSource: _dataSource, + ...domProps }) => { // Resolve icon via lazy resolver — each icon ships as its own micro-chunk const IconComponent = icon ? getLazyIcon(icon) : null; @@ -54,7 +67,7 @@ export const MetricCard: React.FC = ({ const { language } = useObjectTranslation(); return ( - + {pickLocalized(title, language)} diff --git a/packages/plugin-dashboard/src/MetricWidget.tsx b/packages/plugin-dashboard/src/MetricWidget.tsx index 794098178..79716c153 100644 --- a/packages/plugin-dashboard/src/MetricWidget.tsx +++ b/packages/plugin-dashboard/src/MetricWidget.tsx @@ -12,6 +12,7 @@ import { import type { I18nLabel } from '@object-ui/types'; import { ArrowDownIcon, ArrowUpIcon, MinusIcon, AlertCircle, Loader2 } from 'lucide-react'; import { VARIANT_ICON_CLASSES, VARIANT_TEXT_CLASSES, type MetricColorVariant } from './colorVariants'; +import type { SchemaHostProps } from './schemaHostProps'; const TREND_LABEL_DEFAULTS: Record = { 'dashboard.trend.vsLastQuarter': 'vs last quarter', @@ -210,8 +211,20 @@ export const MetricWidget = ({ suffix, onClick, variant = 'card', - ...props -}: MetricWidgetProps) => { + // Schema-shaped props `SchemaRenderer` injects, destructured out so the + // spread below cannot write them to the DOM (objectui#4357). Named and + // measured in `./schemaHostProps`; `schema` alone put a + // `schema="[object Object]"` attribute on every KPI card. The rest spread + // survives — it is the component's genuine DOM/aria passthrough. + schema: _schema, + bind: _bind, + events: _events, + props: _propsBag, + ariaLabel: _ariaLabel, + ariaDescribedBy: _ariaDescribedBy, + dataSource: _dataSource, + ...domProps +}: MetricWidgetProps & SchemaHostProps) => { const iconClasses = VARIANT_ICON_CLASSES[colorVariant] || VARIANT_ICON_CLASSES.default; const { t: tTrend } = useTrendT(); // Two locale channels, deliberately distinct. `useDisplayLocale` is the @@ -298,7 +311,7 @@ export const MetricWidget = ({ onClick(); } } : undefined} - {...props} + {...domProps} > diff --git a/packages/plugin-dashboard/src/__tests__/DashboardRenderer.metricI18n.test.tsx b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.metricI18n.test.tsx index 490213b82..8c94a0703 100644 --- a/packages/plugin-dashboard/src/__tests__/DashboardRenderer.metricI18n.test.tsx +++ b/packages/plugin-dashboard/src/__tests__/DashboardRenderer.metricI18n.test.tsx @@ -86,13 +86,6 @@ function dashboard(widget: Record): DashboardComponentSchema { } as unknown as DashboardComponentSchema; } -/** The KPI card's heading row — the slot every assertion below is about. */ -function cardHeading(): HTMLElement { - const el = document.querySelector('.tracking-tight.text-sm.font-medium'); - if (!el) throw new Error('metric card heading not rendered'); - return el as HTMLElement; -} - function renderIn(language: string, schema: DashboardComponentSchema) { return render( @@ -116,7 +109,7 @@ describe('DashboardRenderer — KPI cards translate from the widget convention k }); it('(b) resolves an inline per-locale title map, and never leaks the widget type', () => { - renderIn('zh', dashboard({ + const { container } = renderIn('zh', dashboard({ id: 'unkeyed', type: 'metric', title: { en: 'Total Revenue', 'zh-CN': '总收入' }, @@ -128,17 +121,19 @@ describe('DashboardRenderer — KPI cards translate from the widget convention k // so `|| widgetType` rendered the literal string "metric". expect(screen.queryByText('metric')).toBeNull(); // And the sibling surface's failure mode, which this path never had. - // Asserted on the HEADING, not on `container.innerHTML`: the card also - // carries an unrelated `schema="[object Object]"` DOM attribute, present on - // this path before and after this change and on plain-string titles too — - // `SchemaRenderer` hands the widget schema down and `MetricWidget` spreads - // `...props` onto the `Card`. Filed separately rather than widened into - // this pin, which would have made the assertion pass for the wrong reason. - expect(cardHeading().innerHTML).not.toContain('[object Object]'); + // Asserted on the CONTAINER, which is where this pin belongs and where + // #4163 puts the sibling one. It used to be scoped to the card heading: + // the card carried an unrelated `schema="[object Object]"` attribute on + // every render — `SchemaRenderer` hands the widget schema down and + // `MetricWidget` spread `...props` onto the `Card` — so the container + // assertion was red for a reason that had nothing to do with labels, and + // the tempting repair was to delete it. objectui#4357 removed the leak at + // its source (`src/schemaHostProps.ts`), so the assertion is writable now. + expect(container.innerHTML).not.toContain('[object Object]'); }); it('(b2) resolves the inline map for `en` too — the authored en text, not the raw map', () => { - renderIn('en', dashboard({ + const { container } = renderIn('en', dashboard({ id: 'unkeyed', type: 'metric', title: { en: 'Total Revenue', 'zh-CN': '总收入' }, @@ -146,7 +141,7 @@ describe('DashboardRenderer — KPI cards translate from the widget convention k })); expect(screen.getByText('Total Revenue')).toBeTruthy(); - expect(cardHeading().innerHTML).not.toContain('[object Object]'); + expect(container.innerHTML).not.toContain('[object Object]'); }); it('(b3) prefers the bundle over the inline map when both exist', () => { diff --git a/packages/plugin-dashboard/src/__tests__/MetricWidget.domProps.test.tsx b/packages/plugin-dashboard/src/__tests__/MetricWidget.domProps.test.tsx new file mode 100644 index 000000000..3cb8e675e --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/MetricWidget.domProps.test.tsx @@ -0,0 +1,230 @@ +/** + * 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#4357 — `MetricWidget` / `MetricCard` ended their prop lists with + * `...props` and spread the whole thing onto the Shadcn `Card`. Reached through + * `SchemaRenderer` (every 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 at the `SchemaRenderer` call site (`packages/react/src/SchemaRenderer.tsx`, + * the `React.createElement(Component, …)` block) — the props a widget receives + * that are NOT DOM attributes, and the attribute each one emitted before the fix: + * + * | prop | emitted as | what it is | + * |------------------|-------------------------------|-------------------------------------| + * | `schema` | `schema="[object Object]"` | the node, injected on EVERY render | + * | `events` | `events="[object Object]"` | SDUI action metadata | + * | `props` | `props="[object Object]"` | the props container (spread already)| + * | `bind` | `bind="data.revenue"` | SDUI data-binding path | + * | `ariaLabel` | `arialabel="…"` | camelCase twin of `aria-label` | + * | `ariaDescribedBy`| `ariadescribedby="…"` | camelCase twin of `aria-describedby`| + * | `dataSource` | `datasource="[object Object]"`| the injected data-source ADAPTER | + * + * The line the fix draws is "is this an HTML attribute name": the seven above are + * not (the renderer already emits the two dashed `aria-*` forms itself), so they + * are destructured out. Everything that IS one keeps flowing through the spread — + * `id`, `name`, `role`, `disabled`, `aria-*`, `data-*`, `className` — because the + * spread is genuine DOM/aria passthrough and stays. + * + * `dataSource` is why case (g) exists and why it renders the dashboard WITH an + * adapter. It is the only one of the seven that never appears in a schema — + * `SchemaRenderer` strips the schema's own `dataSource` binding by name + * (objectstack#5576) and this is the ADAPTER `DashboardRenderer` hands its + * `SchemaRenderer` call, arriving through the renderer's trailing props. Every + * fixture in this package renders without one, so it reads `undefined` and + * writes nothing; every live dashboard has one, so the attribute was there in + * production and in no test. A pin that only ever renders dataless dashboards + * cannot see it. + * + * WHY THE ASSERTION IS `container.innerHTML` AND NOT THE HEADING. This defect's + * cost was never a visible one; it was that it POISONED the assertion this area + * attracts. #4163 pinned `not.toContain('[object Object]')` on + * `DashboardGridLayout`, and #4032 wanted the same pin on the metric path but + * could not write it — the card carried `schema="[object Object]"` before and + * after any i18n fix, so the pin would have been red for a reason unrelated to + * labels, and the tempting repair (loosen it) throws the real pin away. #4032 + * asserted on the card heading instead, with a comment pointing here. That + * workaround is now removed: the container assertion below is the one that could + * not be written, and `DashboardRenderer.metricI18n.test.tsx` asserts on the + * container again. + * + * DIRECTIONS, written before the run: (a)–(d) and (g) RED before the fix — + * (a)/(c)/(d)/(g) by `MetricWidget`, (b) by `MetricCard`; (e)/(f) GREEN on both + * sides, they are the acceptance boundary (nothing else about the render may + * move). + */ + +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 type { DashboardComponentSchema } from '@object-ui/types'; +import { SchemaRenderer } from '@object-ui/react'; +// Both widgets are resolved from the registry, populated as a side effect of +// the package barrel. Imported at MODULE scope (never in a hook) per AGENTS.md's +// flaky-test rule: the cost lands in the import phase, unbounded by any timeout. +import { DashboardRenderer } from '../index'; + +afterEach(cleanup); + +/** Every SDUI key at once, so one render measures the whole surface. */ +const SCHEMA_METADATA = { + id: 'revenue', + name: 'revenue_kpi', + bind: 'data.revenue', + events: { onClick: [{ action: 'navigate', params: { url: '/deals' } }] }, + ariaLabel: 'Revenue KPI', + ariaDescribedBy: 'desc-1', + role: 'group', +} as const; + +/** Attribute names that must never appear — none of them is an HTML attribute. */ +const LEAKED_ATTRIBUTES = ['schema', 'events', 'props', 'bind', 'arialabel', 'ariadescribedby', 'datasource']; + +/** + * A data-source adapter, shaped like the one a live dashboard injects. Only its + * identity matters here: it must reach the widget and not the DOM. + */ +const ADAPTER = { name: 'fake', find: async () => ({ items: [] }), findOne: async () => null }; + +function renderSchema(schema: Record) { + return render( + + + , + ); +} + +function card(container: HTMLElement): HTMLElement { + const el = container.firstElementChild; + if (!el) throw new Error('nothing rendered'); + return el as HTMLElement; +} + +function attributeNames(el: HTMLElement): string[] { + return Array.from(el.attributes).map((a) => a.name); +} + +describe('MetricWidget / MetricCard — schema-shaped props stay off the DOM (#4357)', () => { + it('(a) MetricWidget: the metric container carries no stringified schema', () => { + const { container } = renderSchema({ + type: 'metric', + label: 'Total Revenue', + value: 1930000, + ...SCHEMA_METADATA, + props: { colorVariant: 'success' }, + }); + + // THE pin — the assertion this defect used to block, on the container. + expect(container.innerHTML).not.toContain('[object Object]'); + expect(attributeNames(card(container))).not.toContain('schema'); + }); + + it('(b) MetricCard: same, through its own registry type', () => { + const { container } = renderSchema({ + type: 'metric-card', + title: 'Total Revenue', + value: 1930000, + ...SCHEMA_METADATA, + }); + + expect(container.innerHTML).not.toContain('[object Object]'); + expect(attributeNames(card(container))).not.toContain('schema'); + }); + + it('(c) neither component emits any of the seven measured non-DOM props', () => { + for (const schema of [ + { type: 'metric', label: 'Total Revenue', value: 1930000, ...SCHEMA_METADATA, props: { colorVariant: 'success' } }, + { type: 'metric-card', title: 'Total Revenue', value: 1930000, ...SCHEMA_METADATA, props: {} }, + ]) { + const { container } = renderSchema(schema); + const names = attributeNames(card(container)); + for (const leaked of LEAKED_ATTRIBUTES) { + expect(names, `${schema.type} emitted ${leaked}=`).not.toContain(leaked); + } + cleanup(); + } + }); + + it('(d) the dashboard KPI path — the container assertion #4032 could not write', () => { + const schema = { + type: 'dashboard', + name: 'sales', + widgets: [{ id: 'revenue', type: 'metric', title: 'Total Revenue', options: { value: 1930000 } }], + } as unknown as DashboardComponentSchema; + + const { container } = render( + + + , + ); + + expect(container.innerHTML).not.toContain('[object Object]'); + }); + + it('(g) the dashboard KPI path WITH a data source — the production shape', () => { + const schema = { + type: 'dashboard', + name: 'sales', + widgets: [{ id: 'revenue', type: 'metric', title: 'Total Revenue', options: { value: 1930000 } }], + } as unknown as DashboardComponentSchema; + + const { container } = render( + + + , + ); + + const tile = container.querySelector('[data-obj-type="metric"]'); + if (!tile) throw new Error('metric tile not rendered'); + + // Case (d) renders the same dashboard with no adapter and cannot see this: + // `dataSource` is `undefined` there, so React writes nothing. + expect(container.innerHTML).not.toContain('[object Object]'); + expect(attributeNames(tile as HTMLElement)).not.toContain('datasource'); + // The KPI still renders its number — the adapter is dropped, not the widget. + expect(container.textContent).toContain('1,930,000'); + }); + + it('(e) genuine DOM / aria passthrough survives — the spread is not removed', () => { + const { container } = renderSchema({ + type: 'metric', + label: 'Total Revenue', + value: 1930000, + ...SCHEMA_METADATA, + className: 'kpi-tile', + }); + + const el = card(container); + // Authored DOM identity and ARIA, plus the renderer's own data attributes. + expect(el.getAttribute('id')).toBe('revenue'); + expect(el.getAttribute('name')).toBe('revenue_kpi'); + expect(el.getAttribute('role')).toBe('group'); + expect(el.getAttribute('aria-label')).toBe('Revenue KPI'); + expect(el.getAttribute('aria-describedby')).toBe('desc-1'); + expect(el.getAttribute('data-obj-id')).toBe('revenue'); + expect(el.getAttribute('data-obj-type')).toBe('metric'); + expect(el.className).toContain('kpi-tile'); + }); + + it('(f) rendered output is otherwise byte-identical — label and formatted value', () => { + const { container } = renderSchema({ + type: 'metric', + label: 'Total Revenue', + value: 1930000, + ...SCHEMA_METADATA, + }); + + expect(container.textContent).toContain('Total Revenue'); + // The KPI formatter's own contract: thousands separators, no decimals. + expect(container.textContent).toContain('1,930,000'); + }); +}); diff --git a/packages/plugin-dashboard/src/schemaHostProps.ts b/packages/plugin-dashboard/src/schemaHostProps.ts new file mode 100644 index 000000000..0327e1c38 --- /dev/null +++ b/packages/plugin-dashboard/src/schemaHostProps.ts @@ -0,0 +1,80 @@ +/** + * 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. + */ + +/** + * The props `SchemaRenderer` hands a widget that are NOT DOM attributes + * (objectui#4357). + * + * A dashboard widget is two things at once: an SDUI block reached through + * `SchemaRenderer`, and a plain React component a host may render directly. + * The React half wants a `...props` spread onto its root element so callers can + * pass `aria-*`, `data-*`, `id`, `role`, `className`. The SDUI half means that + * spread also receives the node's own metadata — and React writes unknown + * lowercase attributes straight to the DOM, stringifying object values. That is + * how every KPI card ended up with `schema="[object Object]"`. + * + * MEASURED at the call site (`packages/react/src/SchemaRenderer.tsx`, the + * `React.createElement(Component, …)` block), one render carrying every SDUI key + * at once. What a widget receives, and the attribute each one emitted: + * + * | prop | emitted as | what it is | + * |-------------------|------------------------------|--------------------------------------| + * | `schema` | `schema="[object Object]"` | the node itself, injected EVERY render | + * | `events` | `events="[object Object]"` | SDUI action metadata (`onClick: […]`) | + * | `props` | `props="[object Object]"` | the props container — the renderer already spreads its CONTENTS separately, so the container is pure metadata | + * | `bind` | `bind="data.revenue"` | SDUI data-binding path | + * | `ariaLabel` | `arialabel="…"` | camelCase authored form; the renderer already emits the resolved `aria-label` | + * | `ariaDescribedBy` | `ariadescribedby="…"` | ditto for `aria-describedby` | + * | `dataSource` | `datasource="[object Object]"` | the injected data-source ADAPTER | + * + * `dataSource` is the one a schema-only measurement misses, and the only one + * that leaks on a *production* dashboard rather than an authored edge case. + * It does not come from the node: `SchemaRenderer` strips the schema's own + * `dataSource` BINDING by name (objectstack#5576) — this is the adapter object + * `DashboardRenderer` hands its `SchemaRenderer` call, which arrives through the + * renderer's trailing `...props` and lands on whatever the widget spreads onto. + * A dashboard rendered without a data source (every test fixture) leaves it + * `undefined` and nothing shows; a dashboard rendered with one — every live + * deployment — put `datasource="[object Object]"` on the card. + * + * The line this type draws is **"is the key an HTML attribute name"**. None of + * the seven is (the two dashed `aria-*` forms the renderer emits itself are, and + * they keep flowing). Everything that IS one stays in the spread and reaches the + * DOM exactly as before: `id`, `name`, `role`, `disabled`, `aria-*`, `data-*`, + * `className`. Removing the spread instead would have been the wrong fix — it is + * the component's only accessibility passthrough. + * + * The renderer strips its own schema metadata (`type` / `children` / `visible` / + * the schema's `dataSource` binding / …) before spreading, so those never + * arrive; this type covers only what survives that strip — including the + * adapter, which arrives by a different door. Declared here rather than in each component + * because two copies of one key list is how a list becomes two disagreeing + * lists — the same reason `colorVariants` was extracted. The pin that keeps the + * declaration honest for BOTH components is + * `__tests__/MetricWidget.domProps.test.tsx`. + */ +export interface SchemaHostProps { + /** The widget's own schema node, injected by `SchemaRenderer` on every render. */ + schema?: unknown; + /** SDUI data-binding path (`'user.address.city'`). */ + bind?: unknown; + /** SDUI event handlers (`{ onClick: [ActionDef, …] }`) — data, not DOM. */ + events?: unknown; + /** The schema's props container; its contents are spread separately. */ + props?: unknown; + /** Authored camelCase ARIA; the renderer emits the resolved `aria-label`. */ + ariaLabel?: unknown; + /** Authored camelCase ARIA; the renderer emits `aria-describedby`. */ + ariaDescribedBy?: unknown; + /** + * The injected data-source adapter, forwarded by `DashboardRenderer` through + * `SchemaRenderer`'s trailing props. Neither metric component reads it — the + * async, object-aware KPI is `ObjectMetricWidget`. + */ + dataSource?: unknown; +}