diff --git a/.changeset/wild-charts-explain-themselves.md b/.changeset/wild-charts-explain-themselves.md new file mode 100644 index 0000000000..e44172901c --- /dev/null +++ b/.changeset/wild-charts-explain-themselves.md @@ -0,0 +1,19 @@ +--- +'@object-ui/plugin-charts': patch +--- + +`ObjectChart` now renders a self-describing empty state when its query succeeds +and returns no rows, instead of falling through to a bare chart frame. + +The frame was measured in a browser rather than assumed: recharts derives its +ticks from the data, so with an empty result the bar and line families emit two +hairline axis rules and no `text` nodes at all, and pie/donut emit nothing — +there are no labelled axes to tell the reader what would have been plotted. +Beside the component's own red "Failed to load chart data" box, a blank tile +gives the reader nothing to distinguish a young chart from a broken one. + +The copy is the one `plugin-dashboard` already shows on the dataset-bound path +("No data yet" / the load succeeded / the source name), so the same chart over +the same empty result no longer reads two different ways depending on which +widget drew it. Charts with inline authored data are unchanged — they ran no +query to report on. diff --git a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx index 1e7a52d72e..b66c0138bb 100644 --- a/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx +++ b/packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx @@ -431,6 +431,14 @@ const AUTHORED_PROPS = { * #4428 shipped a six-key first pass because a schema-only measurement cannot * see it. Answering with empty results keeps every data-bound target on its * real render path rather than an error state. + * + * ⚠️ That last sentence has ONE measured exception, and it is not an error + * state: since objectui#7130 an empty result is a real render path for + * `ObjectChart` that is not its CHART markup — it renders a self-describing + * empty state. The two object-bound chart targets therefore author their own + * rows; see {@link OBJECT_CHART_EXTRAS}. Read that before widening this + * adapter: handing rows to all ~200 targets would move many of them off the + * branch they are pinned on. */ const FAKE_ADAPTER = { find: async () => [], @@ -473,11 +481,36 @@ const CHART_DATA = [ { name: 'Feb', sales: 300, revenue: 139, value: 300 }, ]; const CHART_SERIES = [{ dataKey: 'sales' }, { dataKey: 'revenue' }]; +/** + * The two OBJECT-BOUND chart targets (`plugin-charts:object-chart`, + * `view:chart`). They stay object-bound — `objectName` is what makes them a + * different registry path from the six inline chart targets above, which reach + * `ObjectChart` through `ObjectChartBlock` and its `ElementDataSourceGate`. + * + * `data` / `series` are authored on TOP of that binding (`data` is a declared + * registry input on this component: "Optional static data") because + * {@link FAKE_ADAPTER} answers every query with no rows, and since objectui#7130 + * `ObjectChart` renders a self-describing empty state on an empty result + * instead of an empty chart frame. Without rows these two targets stop at that + * empty state, `[data-slot="chart"]` never matches, and the readiness guard + * below refuses to scan — correctly, because an empty state is not the chart + * markup this sweep exists to scan. + * + * This is objectui#5630's lesson in a third dress: a data-bound target whose + * clean reading would otherwise cover only its empty-state placeholder. That + * card answered it with a populated host where a host was needed, and with a + * plain `schemaExtras` change where the populated branch was reachable from + * pure schema (`element:definition-list`'s `items`). This is the latter case, + * and it also makes these two scan STRICTLY MORE markup than before: the + * pre-#7130 reading swept a chart frame with no marks in it. + */ const OBJECT_CHART_EXTRAS = { objectName: 'accounts', chartType: 'bar', categoryField: 'name', valueField: 'amount', + data: CHART_DATA, + series: CHART_SERIES, }; const CALENDAR_OBJECT_EXTRAS = { objectName: 'accounts', diff --git a/packages/plugin-charts/src/ObjectChart.emptyResult.test.tsx b/packages/plugin-charts/src/ObjectChart.emptyResult.test.tsx new file mode 100644 index 0000000000..d1e6a4fdc9 --- /dev/null +++ b/packages/plugin-charts/src/ObjectChart.emptyResult.test.tsx @@ -0,0 +1,122 @@ +/** + * 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#7130 — `ObjectChart` over an empty result renders a self-describing + * empty state instead of a bare frame. + * + * ## What the bare frame was, measured + * + * Before this branch the component fell through to `ChartRenderer` with + * `data: []`. Rendered in a real browser at 220c18d05, recharts emitted an SVG + * with TWO hairline axis rules and ZERO `` nodes for bar/line, and + * nothing at all for pie — ticks are derived from the data, so an empty domain + * labels nothing. The filing hypothesis ("a chart frame with axes is arguably + * self-describing") is false: there are no labels on an empty chart. + * + * ## Why the assertions are shaped this way + * + * The maintainer's bar (hotcrm#1212) is *distinguishable from a load failure at + * a glance*, so the pin is not "an empty state exists" — it is that the empty + * state and the failure state are DIFFERENT, checked on the ARIA roles that + * carry that difference (`status` vs `alert`), plus the three directions this + * branch could be wrong in: firing over real rows, firing before the fetch + * resolves, and swallowing an error. Each is a separate `it` so an ablation + * reports which arm moved. + */ +import React from 'react'; +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { render, screen, cleanup, waitFor } from '@testing-library/react'; + +vi.mock('recharts', async () => { + const actual = await vi.importActual('recharts'); + return { + ...actual, + ResponsiveContainer: ({ children }: any) => + React.cloneElement(children, { width: 480, height: 320 }), + }; +}); + +import { ObjectChart } from './ObjectChart'; + +beforeEach(() => { + vi.stubGlobal('fetch', vi.fn(async () => ({ ok: true, json: async () => ({}) }))); +}); +afterEach(() => { + vi.unstubAllGlobals(); + cleanup(); +}); + +const schema = { + type: 'object-chart', + chartType: 'bar' as const, + objectName: 'crm_opportunity', + xAxisKey: 'stage', + series: [{ dataKey: 'amount', label: 'Amount' }], + isAnimationActive: false, +}; + +const renderWith = (dataSource: any, overrides: Record = {}) => + render(); + +describe('ObjectChart — empty result (objectui#7130)', () => { + it('renders a self-describing empty state when the query returns no rows', async () => { + renderWith({ find: vi.fn().mockResolvedValue([]) }); + + const box = await screen.findByTestId('chart-empty-state'); + // The copy states that the load SUCCEEDED — the fact a blank tile cannot + // give the reader — and promises no recovery (no "loading", no "retry"). + expect(box).toHaveTextContent('No data yet'); + expect(box).toHaveTextContent('loaded successfully'); + expect(box.textContent).not.toMatch(/try again|retry|loading/i); + // Names WHAT is empty, so the tile is self-describing without authored copy. + expect(screen.getByTestId('chart-empty-source')).toHaveTextContent('crm_opportunity'); + }); + + it('marks the empty state `status`, distinct from the failure box `alert`', async () => { + // The whole point of the card: the reader must be able to tell an empty + // result from a load failure. Asserted on the two roles rather than on + // copy, because that is the machine-readable half of the distinction. + const { unmount } = renderWith({ find: vi.fn().mockResolvedValue([]) }); + expect(await screen.findByTestId('chart-empty-state')).toHaveAttribute('role', 'status'); + unmount(); + + renderWith({ find: vi.fn().mockRejectedValue(new Error('Network request failed')) }); + const failure = await screen.findByTestId('chart-error'); + expect(failure).toHaveAttribute('role', 'alert'); + // …and the empty branch must not have swallowed the error. + expect(screen.queryByTestId('chart-empty-state')).toBeNull(); + }); + + it('does NOT fire over a populated result', async () => { + renderWith({ + find: vi.fn().mockResolvedValue([ + { stage: 'Qualify', amount: 12 }, + { stage: 'Won', amount: 9 }, + ]), + }); + // Wait for the fetch to settle before asserting the absence, so this cannot + // pass merely by running before the data arrives. + await waitFor(() => expect(screen.queryByTestId('chart-loading')).toBeNull()); + expect(screen.queryByTestId('chart-empty-state')).toBeNull(); + }); + + it('does NOT flash before the fetch resolves — loading still wins', async () => { + renderWith({ find: vi.fn(() => new Promise(() => {})) }); + expect(await screen.findByTestId('chart-loading')).toBeTruthy(); + expect(screen.queryByTestId('chart-empty-state')).toBeNull(); + }); + + it('leaves an inline-data chart alone — it ran no query to report on', async () => { + // `data: []` is an authoring choice, not an empty query result, so the copy + // ("its query returned no records") would be false of it. + renderWith({ find: vi.fn().mockResolvedValue([]) }, { data: [] }); + await waitFor(() => expect(screen.queryByTestId('chart-loading')).toBeNull()); + expect(screen.queryByTestId('chart-empty-state')).toBeNull(); + }); +}); diff --git a/packages/plugin-charts/src/ObjectChart.tsx b/packages/plugin-charts/src/ObjectChart.tsx index 122ec10b5e..6d01291796 100644 --- a/packages/plugin-charts/src/ObjectChart.tsx +++ b/packages/plugin-charts/src/ObjectChart.tsx @@ -3,8 +3,8 @@ import React, { useState, useEffect, useContext, useCallback, useMemo } from 're import { useDataScope, SchemaRendererContext, SchemaRenderer, useDrillNavigation, useFilterScope, ElementDataSourceGate, type ElementDataSourceMapping } from '@object-ui/react'; import { ChartRenderer } from './ChartRenderer'; import { ComponentRegistry, humanizeLabel, extractRecords, computeDrillFilter, isDrillEnabled, resolveDrillTitle, resolveFilterPlaceholders, resolveContextTokens, shiftFilterByCompareTo, compareToTrendLabelKey, buildChartSeries, buildOptionColorMap, deriveDimensionLabelMaps, dimensionOptionTranslator, loadDimensionFieldMeta, relabelDimensions, localizeFieldOptions, elementDataSourceBlock, type DimensionFieldMeta, type CompareToConfig, type DrillEvent, type ChartResultField, type ChartSegmentClickEvent } from '@object-ui/core'; -import { Sheet, SheetContent, SheetHeader, SheetTitle, Dialog, DialogContent, DialogHeader, DialogTitle, RefreshIndicator, Button, ChartSkeleton } from '@object-ui/components'; -import { AlertCircle, ArrowUpRight } from 'lucide-react'; +import { Sheet, SheetContent, SheetHeader, SheetTitle, Dialog, DialogContent, DialogHeader, DialogTitle, RefreshIndicator, Button, ChartSkeleton, DataEmptyState } from '@object-ui/components'; +import { AlertCircle, ArrowUpRight, Inbox } from 'lucide-react'; import { useSafeFieldLabel, useSafeTranslate } from '@object-ui/i18n'; import type { DrillDownConfig } from '@object-ui/types'; @@ -919,6 +919,80 @@ export const ObjectChart = (props: any) => { return
No data source available for “{schema.objectName}”
; } + // Query succeeded and returned nothing → a self-describing empty state, NOT + // the bare frame this used to fall through to (objectui#7130). + // + // ## What the bare frame actually rendered — measured, not assumed + // + // The card was filed on the hypothesis that "a chart frame with axes is + // arguably self-describing": an empty table is a blank rectangle, but an + // empty chart still draws labelled axes telling the reader what WOULD have + // been plotted. Rendered in a real browser at 220c18d05, that is false. + // Recharts derives its ticks FROM the data, so with `data: []` there is no + // domain and no tick to label: the bar/line families emit an SVG containing + // two hairline axis rules and ZERO `` nodes, and pie/donut emit an + // empty `` with no marks at all. Measured against a populated control + // in the same render, which emitted eight `` nodes. So the frame is + // not self-describing — it is a blank tile beside a `chart-error` box that + // at least says something, which is precisely the hotcrm#1212 failure: + // nothing on screen says whether the chart failed or is simply young. + // + // ## Why this is not the KPI carve-out + // + // `DatasetWidget` deliberately exempts metric families — "a metric (single + // value) over an empty dataset is 0, not an empty state" — and names charts + // on the OTHER side of that line in the same comment: "Charts and tables + // keep the empty state (there is genuinely nothing to plot)." A KPI's `0` is + // a datum; a chart's blank frame is an absence. So a dataset-bound chart has + // rendered this state since #7124 and the object-bound one did not: same + // family, same empty result, two answers. This is the surface that ruling + // did not reach, not a new judgement. + // + // ## Why `DataEmptyState` and not `WidgetEmptyState` + // + // The seam #7124 built is `plugin-dashboard`-local and unexported (absent + // from that package's `index.tsx`), and `plugin-charts` does not depend on + // `plugin-dashboard`. Reaching it would mean promoting it to public API for + // a foreign plugin — the cross-surface abstraction objectui#7132 owns. + // `DataEmptyState` is instead the primitive that is ALREADY shared and + // already consumed by plugin-list / plugin-kanban / plugin-detail and by + // `WidgetEmptyState` itself, out of `@object-ui/components`, which this + // package already depends on. Nothing is promoted, no dependency edge is + // added, and when #7132 converges the defaults this call site collapses the + // same way the other four do. + // + // The copy is the keys #7124 landed in all ten packs — no new key, and no + // promise of recovery: it states that the load SUCCEEDED, which is the one + // fact the reader of a blank tile cannot otherwise get. `role="status"` + // against the `role="alert"` on the `chart-error` box above is the machine + // check that the two states are distinct. + // + // Gated on a QUERY-backed chart: a chart handed inline `data: []` by its + // author never ran a query, so "its query returned no records" would be + // false of it, and those charts render byte-for-byte as before. + const isQueryBacked = !!(schema.objectName || schema.dataset); + if (isQueryBacked && !boundData && !schema.data && finalData.length === 0) { + return ( + h3]:text-sm [&>h3]:font-medium [&>p]:text-xs " + (schema.className || '')} + icon={} + iconWrapperClassName="flex size-9 items-center justify-center rounded-lg bg-muted" + title={tt('dashboard.empty.title', 'No data yet')} + description={tt( + 'dashboard.empty.message', + 'This widget loaded successfully and its query returned no records yet.', + )} + > +

+ {tt('dashboard.empty.sourceLabel', 'Source:')}{' '} + {schema.dataset || schema.objectName} +

+ + ); + } + const internalChartClick = isDrillEnabled(drillDown) ? (ev: ChartSegmentClickEvent) => { const labelCategory = ev.category;