diff --git a/.changeset/7140-sankey-no-positive-flow.md b/.changeset/7140-sankey-no-positive-flow.md new file mode 100644 index 0000000000..ff8b43ca82 --- /dev/null +++ b/.changeset/7140-sankey-no-positive-flow.md @@ -0,0 +1,36 @@ +--- +'@object-ui/plugin-charts': minor +--- + +A sankey with no positive flow says so, instead of rendering an empty div +(objectui#7140). + +`AdvancedChartImpl`'s sankey arm keeps only strictly positive measures, so a +chart handed **real rows** whose measure is all `0`, all `null`, all negative, +or unparseable built no links and returned a bare `
`. Measured in Chromium +against a populated control: the control drew 1 `` / 7 `` / +26 descendants; each of those four tiles rendered `descendantCount: 1`, +`svgCount: 0`, `textContent: ''`, and their screenshots hashed identical to one +another. No marks, no text, no `role` — a tile indistinguishable from a widget +that had crashed, which is the one distinction the file's other refusals exist +to make. + +It now renders through the `ChartRefusal` shell those refusals already use — +same box, same `role="status"`, and a new `data-chart-error="no-positive-flow"` +— reading *"This chart has no flow to draw: no row's `` is above +zero."* + +Two boundaries are deliberate and pinned: + +- **No rows at all is untouched.** That is the empty-result question, answered + upstream in `ObjectChart` where the query outcome is known; a sentence about + what the rows contain would be false about a dataset with no rows in it. +- **One positive row among zeros still draws.** The refusal fires on an empty + link set, never on a thin one. + +One code and one sentence for three causes (a genuinely all-zero flow, values a +flow cannot represent because they are negative, and measures `Number(…) || 0` +folds to zero): naming any single cause would be false for the other two, so +the copy names the predicate the filter actually applies, which is true for all +three. No recovery is promised. Every other chart family is byte-identical — +eight of the twelve tiles in the browser sweep hashed unchanged. diff --git a/packages/plugin-charts/src/AdvancedChartImpl.sankeyNoPositiveFlow.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.sankeyNoPositiveFlow.test.tsx new file mode 100644 index 0000000000..9c0a069c3a --- /dev/null +++ b/packages/plugin-charts/src/AdvancedChartImpl.sankeyNoPositiveFlow.test.tsx @@ -0,0 +1,146 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * Sankey — rows arrived, none of them is a positive number (objectui#7140). + * + * The sankey arm keeps only strictly positive measures + * (`data.filter((r) => (Number(r?.[dataKey]) || 0) > 0)`), so a chart handed + * REAL rows whose measure is all `0`, all `null`, all negative, or unparseable + * built no links and returned a bare `
`. + * + * Measured in Chromium against a populated control before the fix, at + * `origin/main` e8e4c4df5: the control drew 1 `` / 7 `` / + * 26 descendants; each of the four blank tiles rendered `descendantCount: 1`, + * `svgCount: 0`, `textContent: ''`, and their screenshots hashed identical to + * one another. Nothing was on the page — no marks, no text, no `role` — so the + * tile was indistinguishable from a widget that had crashed, which is the one + * distinction every other refusal in that file exists to make. + * + * The two boundaries this pins are the ones that make the message TRUE rather + * than merely present: + * + * - **no rows at all still returns the bare div.** That is the empty-RESULT + * question (objectui#7130), answered upstream in `ObjectChart` where the + * query outcome is known. "No row's measure is above zero" would be a false + * sentence about a dataset with no rows in it. + * - **one positive row among zeros still DRAWS.** The refusal fires on an + * empty link set, never on a thin one; a sankey that can draw anything is + * never replaced by prose. + */ +import React from 'react'; +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup } 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 AdvancedChartImpl from './AdvancedChartImpl'; + +afterEach(cleanup); + +const SERIES = [{ dataKey: 'amount', label: 'Amount' }]; + +const renderSankey = (data: Array>) => + render( + , + ); + +const refusalOf = (container: HTMLElement) => + container.querySelector('[data-chart-error="no-positive-flow"]'); + +/** + * The four row shapes that reach the empty link set. They are NOT the same + * situation — a genuinely all-zero flow, values a flow cannot represent because + * they are negative, and measures `Number(…) || 0` folds to zero — but the + * predicate the filter applies is the one thing true of all of them, so one + * message serves all four without saying anything false about any of them. + */ +const NO_POSITIVE_ROWS: Array<[string, Array>]> = [ + ['every measure is 0', [{ stage: 'Prospecting', amount: 0 }, { stage: 'Won', amount: 0 }]], + ['every measure is null', [{ stage: 'Prospecting', amount: null }, { stage: 'Won', amount: null }]], + ['every measure is negative', [{ stage: 'Refunds', amount: -40 }, { stage: 'Credits', amount: -12 }]], + ['every measure is unparseable', [{ stage: 'A', amount: 'n/a' }, { stage: 'B', amount: 'n/a' }]], +]; + +describe('AdvancedChartImpl — sankey with no positive flow (objectui#7140)', () => { + it.each(NO_POSITIVE_ROWS)('says so instead of rendering nothing when %s', (_label, rows) => { + const { container } = renderSankey(rows); + + const refusal = refusalOf(container); + expect(refusal, 'renders the explanatory placeholder').not.toBeNull(); + // A refusal is a STATE, not an alert — the shell the other two refusals in + // this file render through. + expect(refusal?.getAttribute('role')).toBe('status'); + // Names the measure it tested, so the author knows WHICH column was all + // zero rather than being told the chart is empty. + expect(refusal?.textContent).toContain('amount'); + expect(refusal?.textContent).toContain('above zero'); + // The old behaviour was a bare `
` with nothing in it. Anything that + // paints a plot here would be a sankey drawn from links that do not exist. + expect(container.querySelector('svg')).toBeNull(); + }); + + it('leaves the no-rows case alone — that is the empty-result question, answered upstream', () => { + const { container } = renderSankey([]); + + expect(refusalOf(container), 'no refusal without rows to refuse over').toBeNull(); + expect(container.querySelector('svg')).toBeNull(); + // Byte-for-byte what this arm returned before objectui#7140: an empty div, + // carrying only the className it was handed. + expect(container.textContent).toBe(''); + }); + + it('still draws when ONE row is positive among zeros', () => { + const { container } = renderSankey([ + { stage: 'Prospecting', amount: 0 }, + { stage: 'Proposal', amount: 7 }, + { stage: 'Won', amount: 0 }, + ]); + + expect(refusalOf(container), 'a drawable sankey is never replaced by prose').toBeNull(); + expect(container.querySelector('svg')).not.toBeNull(); + }); + + it('CONTROL — an all-positive sankey draws, and carries no refusal', () => { + const { container } = renderSankey([ + { stage: 'Prospecting', amount: 40 }, + { stage: 'Proposal', amount: 25 }, + { stage: 'Won', amount: 12 }, + ]); + + expect(refusalOf(container)).toBeNull(); + expect(container.querySelector('svg')).not.toBeNull(); + }); + + it('does not fire for other chart families handed the same all-zero rows', () => { + // The guard lives inside the sankey arm and reads the sankey filter's own + // result, so it cannot reach a family that has no such filter. Pinned + // because a hoisted copy of the predicate is the obvious refactor and would + // blank four working charts: bar/pie/funnel/treemap all render an all-zero + // dataset today (measured in Chromium — axes, labels and legend). + for (const chartType of ['bar', 'pie', 'funnel', 'treemap'] as const) { + const { container } = render( + , + ); + expect(refusalOf(container), `${chartType} must be untouched`).toBeNull(); + cleanup(); + } + }); +}); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index baf47ce1ee..3105af33af 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -1048,7 +1048,45 @@ function AdvancedChartImplInner({ const nodes = [{ name: rootName }, ...rows.map((r) => ({ name: String(r?.[xAxisKey] ?? '') }))]; const links = rows.map((r, i) => ({ source: 0, target: i + 1, value: Number(r?.[dataKey]) || 0 })); if (links.length === 0) { - return
; + // Rows ARRIVED and the filter above kept none of them, so there is no + // flow to draw. This used to return a bare `
` — objectui#7140. + // + // Measured in Chromium before it was changed, against a populated + // control that drew 1 `` / 7 ``: the all-zero, all-null, + // all-negative and unparseable-measure tiles each rendered ONE element + // and nothing else (`descendantCount: 1`, `svg: 0`, `textContent: ''`), + // and their screenshots were byte-identical to each other. No marks, no + // text, no `role` — the one path in this file that put nothing at all on + // the page, and pixel-identical to a render that crashed. A reader could + // not tell a genuinely all-zero flow from a broken widget, which is the + // distinction every other refusal here exists to make. + // + // Gated on rows being present for the same reason `hasNoCategoryKey` and + // `hasNoPlottableSeries` are: handed NO rows the sentence below would be + // false — there is no row whose measure could be anything. That is the + // empty-RESULT question (objectui#7130), answered upstream in + // `ObjectChart` where the query outcome is known, so this arm leaves the + // no-rows case byte-for-byte as it was. + // + // ONE code and ONE sentence, for the reason `hasNoPlottableSeries`' + // docstring gives: three causes reach here — a genuinely all-zero flow, + // values a flow cannot represent because they are negative, and + // unparseable measures that `Number(…) || 0` folds to zero — and naming + // any ONE of them is a sentence that is false for the other two. The + // predicate the filter actually applies is true for all three, so the + // copy names THAT. No console warning either, unlike the two refusals + // below: those carry a diagnostic pair that does not fit on screen, + // whereas this message already names the key and the exact test it + // failed. + if (data.length === 0) { + return
; + } + return ( + + This chart has no flow to draw: no row's{' '} + {dataKey} is above zero. + + ); } return (