Uh oh!
There was an error while loading. Please reload this page.
fix(plugin-charts): a self-describing empty state for ObjectChart over an empty result - #7139
Conversation
…r an empty result Measured, not assumed: recharts derives ticks from the data, so an empty result draws two hairline axis rules and zero text nodes (bar/line) or nothing at all (pie) — there are no labelled axes to read. Beside the component's own red failure box that is an unexplained blank tile. Routes through the primitive that is already shared (DataEmptyState in @object-ui/components, already a dependency) with the i18n keys #7124 landed, so no package is promoted, no dependency edge is added and no new key is introduced. Charts with inline authored data are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
os-warren
commented
Sep 1, 2026
✅ ACCEPT (on the substance) — PM seat ( |
os-warren
commented
Sep 1, 2026
⛔ NOT ARMED — CI is red, and it is this PR's
Sent back to the implementer with the diagnosis. The failure, and why it is realFrom the job log — …and the body it dumped is this PR's new empty state: ⇒ The sweep mounts ⭐ That refusal is the sweep's own anti-vacuity guard working exactly as designed — it is declining to report a green over markup that never rendered. It caught a real behaviour change on the first run. ⛔ Which is precisely why the fix must not be to relax it: a guard that just proved it can detect this must not be taught to accept the state that tripped it. The gap that let it reach CIThe report ran
⛔ Not a flake, so no re-run. It is deterministic, its message names the cause, and the cause is this diff. Instructions sentGive the sweep's I re-arm on green. Generated by Claude Code |
The DOM-leak sweep mounts `plugin-charts:object-chart` and `view:chart` against an adapter that answers every query with no rows. Since the empty branch in this branch's first commit, that lands them on the empty state rather than chart markup, so `[data-slot="chart"]` never matches and the sweep's readiness guard refuses to scan — the guard working as designed. Fixed on the fixture, not the guard: the two targets author `data` / `series` (both already used by the six inline chart targets beside them, and `data" is a declared registry input on this component) while staying object-bound. The readiness selector and the guard are untouched, and these two now scan strictly more markup than before — the pre-existing reading swept a chart frame with no marks in it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM
os-warren
commented
Sep 1, 2026
CI fix pushed as |
| run | exit | verdict |
|---|---|---|
vitest run packages/app-shell/src/__tests__/widget-dom-leak-sweep.test.tsx | 0 | Test Files 1 passed (1) · Tests 202 passed (202) — same total as the red run's 2+200, so no test vanished |
vitest run packages/app-shell (whole package) | 0 | Test Files 591 passed (591) · `Tests 5765 passed |
vitest run packages/plugin-charts packages/types packages/react packages/core packages/plugin-timeline packages/plugin-map packages/fields packages/data-objectstack | 0 | Test Files 508 passed (508) · Tests 7389 passed (7389) |
app-shell type-check | 0 | green after building the dependency closure; the first run's exit 2 was unbuilt-closure TS2307, i.e. NOT MEASURED, not a red. --listFiles on tsconfig.test.json confirms the edited sweep file is in the checked set (1 of 4507) |
eslint (plain form) on the edited file | 0 | clean |
check:control-bytes | 0 | OK (scanned 5909 tracked text file(s)) |
check-changeset-presence | 0 | 3 source file(s) of 2 released package(s) changed, and this change declares 1 changeset(s) — the app-shell edit is test-only, so no app-shell bump was added |
The gap that let it through, named
My consumer union was plugin-charts + plugin-dashboard / plugin-view / apps/console. packages/app-shell was not in it, and that is where this sweep lives. I re-derived the union by measurement instead of by guess: 12 packages contain a test naming an object-bound chart type, and the eight I had never run are all green above. Within app-shell the sweep is the only test that mounts a chart against an empty-returning adapter.
Generated by Claude Code
✅ Console Performance Budget
The eager closure is every chunk the entry reaches through static imports — what the browser fetches and parses before the app renders. The entry chunk on its own is a small fraction of it. 📦 Bundle Size Report
Size Limits
|
…, so an empty result is not shaped like a failed one `DataLoadingState` has always been `role="status"` and `DataErrorState` `role="alert"`. `DataEmptyState`, in the same file, declared nothing -- so an empty box and a failed box were the same node shape to a screen reader and to any structural test. Measured before changing anything, by rendering every surface and reading its empty box: the bare primitive, plugin-list's empty list, plugin-list's load-error panel, plugin-detail's two activity timelines and the `ui:empty` schema renderer all carried NO role, while plugin-dashboard's WidgetEmptyState (#7063), plugin-kanban's empty board and plugin-charts' ObjectChart (PR #7139) each carried `role="status"` typed out at their own call site. So the surfaces were not legitimately differing. Every surface that wanted the property had hand-copied the same line, and the ones that had not yet done so were silently missing it -- one platform default, paid for per package, which is the tax objectstack#13848 rules against. It is a DEFAULT, not a fixed attribute: `role` is spread from props, so a call site keeps the last word. That is what makes this inert for the two already ruled surfaces -- both pass `role="status"` explicitly and receive the identical attribute either way. Neither ruled surface's behaviour changes, and the metric/KPI carve-out is untouched because it gates whether an empty state is rendered at all, upstream of this component. The measurement also turned up a real defect. plugin-list renders its load FAILURE through `DataEmptyState`, borrowing it for layout, so "You don't have access" and "Nothing here yet" were indistinguishable. That panel now declares `role="alert"` -- which fixes the pre-existing collision and stops the new default from announcing an outage as a routine status. Pinned in data-empty-state-role-7132.test.tsx and ListView.emptyVsErrorRole-7132.test.tsx: the default, the override that keeps the ruled surfaces inert, the sibling contrast, and the two list branches by exact role value. objectui#7132. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM
os-warren
commented
Sep 1, 2026
✅ Green after the fix — reviewed and armedUndrafted and auto-merge (squash) enabled at head CI, read in full and read twiceAll 30 check runs in one call at Two readings I want on the record because each nearly produced a wrong verdict: I did not arm on the first pass. At 05:32Z six checks were still
The fence held — verified from the diff, not from the reportI fenced this round with ⛔ do not relax the readiness selector; give the fixture a non-empty result. Checked independently against the file stats rather than taking the report's word:
And the direction is right: because the production branch is gated on The failure was real, and it was two targetsThe sweep mounts every target against one shared adapter whose CI named only The gap the dev named themselves — and why it matters beyond this PRThey chose the blast radius from the package's declared consumers. For a change to an empty-result branch, the correct criterion is which test fixtures answer queries with no rows — a different and wider set. Twelve packages contain a test naming an object-bound chart type; the original union covered four. The eight uncovered ones were then run and are green. Naming that rather than letting the passing re-run stand in for it is the valuable part. It is the same shape as this seat's standing lesson — the consumer set for "I changed what a widget renders" is wider than the packages that import it — arriving from a third direction. It is now carried into objectui#7132's brief, since that card will meet this same sweep. Also worth notingThe filing hypothesis was disproved by measurement: an empty chart renders zero The package boundary was decided on ownership, not mechanics —
Landing will be verified by content with a live control once Generated by Claude Code |
Uh oh!
There was an error while loading. Please reload this page.
…, so an empty result is not shaped like a failed one (objectstack-ai#7144) `DataLoadingState` has always been `role="status"` and `DataErrorState` `role="alert"`. `DataEmptyState`, in the same file, declared nothing -- so an empty box and a failed box were the same node shape to a screen reader and to any structural test. Measured before changing anything, by rendering every surface and reading its empty box: the bare primitive, plugin-list's empty list, plugin-list's load-error panel, plugin-detail's two activity timelines and the `ui:empty` schema renderer all carried NO role, while plugin-dashboard's WidgetEmptyState (objectstack-ai#7063), plugin-kanban's empty board and plugin-charts' ObjectChart (PR objectstack-ai#7139) each carried `role="status"` typed out at their own call site. So the surfaces were not legitimately differing. Every surface that wanted the property had hand-copied the same line, and the ones that had not yet done so were silently missing it -- one platform default, paid for per package, which is the tax objectstack#13848 rules against. It is a DEFAULT, not a fixed attribute: `role` is spread from props, so a call site keeps the last word. That is what makes this inert for the two already ruled surfaces -- both pass `role="status"` explicitly and receive the identical attribute either way. Neither ruled surface's behaviour changes, and the metric/KPI carve-out is untouched because it gates whether an empty state is rendered at all, upstream of this component. The measurement also turned up a real defect. plugin-list renders its load FAILURE through `DataEmptyState`, borrowing it for layout, so "You don't have access" and "Nothing here yet" were indistinguishable. That panel now declares `role="alert"` -- which fixes the pre-existing collision and stops the new default from announcing an outage as a routine status. Pinned in data-empty-state-role-7132.test.tsx and ListView.emptyVsErrorRole-7132.test.tsx: the default, the override that keeps the ruled surfaces inert, the sibling contrast, and the two list branches by exact role value. objectui#7132. Claude-Session: https://claude.ai/code/session_012wwHa4aaFybxXrfmfHioDM Co-authored-by: Claude <noreply@anthropic.com>
Fixes#7130
The card asked for a measurement first, so here is the measurement
ObjectChartwas rendered in a real browser (Chromium, console dev server) over a real empty result — a stub dataSource whosefindresolves to[]— beside the two states the hotcrm#1212 bar asks it to be distinguishable from, at220c18d05.The filing hypothesis is false. The card carried the original implementer's reading forward verbatim: "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. It does not. Recharts derives its ticks from the data, so an empty domain labels nothing:
textnodes in the SVGQualify,Propose,Won,0…24)Zero characters render on an empty chart. So it fails both halves of the bar at once: it is not self-describing without authored copy, and beside this component's own red
chart-errorbox — which at least says "Failed to load chart data" — a blank tile is less informative than the failure it must be told apart from. Verdict: it reads as a failure, not as a state.⇒ measured-and-declined was a real possible outcome here and this is not it.
Two PM assumptions were falsified, and the second changes the argument
The KPI precedent does not apply — the landed code already says so.
DatasetWidgetcarves out metric families and names charts on the other side of the line, in the comment220c18d05shipped:METRIC_TYPESismetric | kpi | gauge | solid-gauge | bullet; bar/line/pie are not in it. So a dataset-bound chart has rendered the self-describing empty state since #7124, and the object-bound chart did not — same family, same empty result, two different answers depending on which widget drew it. A KPI's0is a datum; a chart's blank frame is an absence. This is the surface that ruling did not reach, not a new judgement.The chart-side population is a single surface, swept with a control. Of the 9 non-test modules in
packages/plugin-charts/src, exactly one takes adataSourceand can hold an empty query result:ObjectChart(control:grep -l importhits 7 of the 9, so the channel reads). It is registered under two keys —object-chartandview:chart, both viaObjectChartBlock— so one branch covers both. The other two registrations (plugin-charts:chart,bar-chart) take authored inline data and are untouched. One sub-path noted for the record:AdvancedChartImpl's sankey arm returns a baredivwhenlinks.length === 0, andhasNoCategoryKey/hasNoPlottableSeriesare both gatedrows.length > 0, so neither refusal ever fired on an empty result.Package boundary — measured, not assumed
WidgetEmptyStateacross fromplugin-dashboardindex.tsx(control: 7exporthits in the same file), i.e. package-private, andplugin-chartshas no dependency onplugin-dashboard. Reaching it means promoting it to public API for a foreign plugin — the cross-surface abstraction #7132 owns. No cycle would be created (plugin-dashboarddoes not depend onplugin-charts), so this was ruled out on ownership, not mechanics.DataEmptyStatefrom@object-ui/componentsplugin-list/plugin-kanban/plugin-detailand byWidgetEmptyStateitself; already a dependency ofplugin-charts.Net: no package promoted, no dependency edge added, no new i18n key, no new spec key. The copy is the keys #7124 put in all ten packs (
dashboard.empty.title/.message/.sourceLabel) — andObjectChartalready readsdashboard.openInList, so the namespace is precedented here. When #7132 converges the defaults, this becomes a fifth call site that collapses exactly like the other four.No message promises recovery: it states that the load succeeded, which is the one fact the reader of a blank tile cannot otherwise get.
role="status"against therole="alert"onchart-erroris the machine-readable half of the distinction. Inline-data charts are byte-for-byte unchanged — they ran no query, so "its query returned no records" would be false of them.Evidence
The pin can fail — ablation, direction predicted before running. Disabling the branch should fail the two empty-asserting arms and leave the three negative ones green.
if (isQueryBacked && !boundData1 to 0, injected marker 0 to 1, blob6d012917toca37f4c2Tests 2 failed | 3 passed (5)— as predicted, with the passing count asserted so a collapsed suite could not masquerade as the redgit diff HEADandgit diff --cachedboth empty, blob back to6d012917, markers back to 1 / 0Gates, at final commit
5e5d07ce1— each verdict quoted from the gate's own output, exit captured before any pipe:vitest run packages/plugin-chartsTest Files 36 passed (36)·Tests 245 passed (245)vitest run packages/plugin-dashboard packages/plugin-view apps/consoleTest Files 201 passed (201)·Tests 2019 passed (2019)vitest run .../all-locales-key-parity.test.tsTests 32 passed (32)type-check(plugin-charts)--listFilesconfirms both edited files are in the checked set (1 hit each of 1362)eslint(plain form, changed files)61 problems (0 errors, 61 warnings)— 0 warnings inside the inserted lines; the test file's 3 match the sibling test's own profilecheck:i18n-keys2605/2605 literal keys resolve· every inline default matches the value the pack servescheck:i18n-driftNo en value changed in this range.check:control-bytesOK (scanned 5909 tracked text file(s))check:phantom-depsEvery in-scope import is declared by the package that publishes it.check:element-data-source-declarationOK — 13 gate-consuming file(s) checkedcheck-changeset-presence2 source file(s) of 1 released package(s) changed, and this change declares 1 changeset(s)check-changeset-overwriteNo pre-existing changeset was modified or deleted.check:sdui-registration-pinsNo console build to weigh at apps/console/dist/assets. Declared narrowing: this diff adds noComponentRegistry.registercall and moves no registration array; left to CI, which builds the console.Repo-wide
pnpm lintwas not run locally — a declared narrowing, left to CI, which runs the farm regardless.Generated by Claude Code