diff --git a/.changeset/quiet-moons-listen.md b/.changeset/quiet-moons-listen.md new file mode 100644 index 000000000..28fceb43f --- /dev/null +++ b/.changeset/quiet-moons-listen.md @@ -0,0 +1,63 @@ +--- +'@object-ui/core': minor +'@object-ui/plugin-charts': patch +'@object-ui/plugin-dashboard': patch +--- + +A clicked cartesian mark names its own series, and the drill title reads its label + +objectui#4672, objectui#4682. + +**The dead pivoted drill.** objectui#4680 fixed what a cartesian click could +read out of recharts 3's `MouseHandlerDataParam`, and measured the wall it could +not get past: a chart-level click is an AXIS interaction, and recharts +dispatches those with `activeDataKey` hard-coded `undefined`, because the shared +cursor spans every series at that tick. A pivoted dataset chart — 2 dimensions, +1 measure, the shape ADR-0021 introduced — needs the series to resolve its drill +row, so every segment of every such dashboard chart stayed a dead click. The +series was left unresolved rather than guessed, and the card carried the rest. + +The answer is the mark itself. This renderer draws the `Bar` / `Line` / `Area`, +so an item-level `onClick` closes over the very `dataKey` it was rendered with — +the series is statically known, not inferred from tooltip state. + +Both handlers fire for one gesture (measured: item first, chart second, sharing +one `nativeEvent` object), so the item handler does not emit. It RECORDS its +series, stamped with that gesture, and the chart-level handler composes the one +event. That is the double-fire answer and the additive property together: + +- **one click, one drill event**, because there is one emit site — not a second + event suppressed after the fact; +- **a click that lands on no mark is untouched**: it records nothing and falls + through to the objectui#4680 axis answer exactly as shipped — category, bucket + identity, and the series only where one series is plotted. Empty plot area + stays category-only, and "drill the whole category" was rejected as a + different product question. Nothing that resolved before stops resolving; a + line's `dot={false}` stroke simply GAINS the exact series where it is hit; +- pairing on the shared DOM event rather than on a flag means a record left by + one gesture can never be adopted by a later click. + +The clicked key is forwarded exactly as rendered, `''` included: the +empty-string second-dimension group draws its own bar since objectui#4673, and +`''` is falsy, so a truthiness test on the way out would send no series at all +and leave that bar's drill standing on the reader's coercion instead of on what +was clicked. + +**The opaque drill title.** `ChartSegmentClickEvent` gains `seriesLabel`, and +`DatasetWidget`'s drill drawer titles itself from `seriesLabel ?? series`. +`ev.series` stays the LOOKUP key — `findChartSeriesRow` resolves it through the +same assignment `buildChartSeries` made — and only the title reads the label. + +The two strings are equal for every ordinary group, which is why reading the key +as a title went unnoticed. They part company when a group's label cannot name +it: the null bucket beside a record whose stored value literally spells +`(None)`, which is objectui#4508's collision on the series axis, reachable since +objectui#4673. Both groups then key by `chartBucketId`, and the drawer opened on +the right records under the title `Backlog / [null]`. An internal id where a +label belongs reads as broken DATA rather than as a broken title. + +Neither string can do the other's job, which is why this is a second field +rather than a change to the first: the label is not resolvable (it is exactly +what the colliding groups share) and the key is not showable. `seriesLabel` is +optional and absent wherever a renderer resolved no label, so every other +chart's title is byte-identical. diff --git a/packages/core/src/utils/chart-series.nullCategory.test.ts b/packages/core/src/utils/chart-series.nullCategory.test.ts index d04cbeded..1514114ec 100644 --- a/packages/core/src/utils/chart-series.nullCategory.test.ts +++ b/packages/core/src/utils/chart-series.nullCategory.test.ts @@ -61,6 +61,7 @@ import { chartRowBucketId, CHART_BUCKET_ID_KEY, NULL_CATEGORY_LABEL, + type ChartSegmentClickEvent, } from './chart-series'; import { pivotBucketId, pivotDimensionValue } from './dataset-pivot'; @@ -1004,3 +1005,64 @@ describe('findChartSeriesRow — the null second-dimension segment keeps its dri ).toBe(1); }); }); + +/** + * objectui#4682 — the click event carries the group's LABEL beside its key. + * + * `ChartSegmentClickEvent.series` is a `dataKey` {@link buildChartSeries} + * assigned, and {@link findChartSeriesRow} resolves it back through that same + * assignment. A consumer that TITLES itself from that key is reading an + * internal id: for every ordinary group the key is the label, but the groups + * whose label cannot name them key by identity, and the drawer then announced + * `[null]` over a segment the user saw labelled `(None)`. + * + * `seriesLabel` is a second field rather than a change to the first, and this + * block is why: the two strings answer different questions, and neither can do + * the other's job. The label is not resolvable — it is precisely what the + * colliding groups SHARE — and the key is not showable. + */ +describe('ChartSegmentClickEvent — the series key and its label (objectui#4682)', () => { + const RAW = [ + { status: 'Backlog', priority: NULL_CATEGORY_LABEL, est_hours: 1 }, + { status: 'Backlog', priority: null, est_hours: 2 }, + ]; + const DIMS = ['status', 'priority']; + const VALS = ['est_hours']; + + it('the LABEL cannot do the lookup’s job — which is why the key stays the key', () => { + const { series } = buildChartSeries(RAW, DIMS, VALS); + // One label over two groups: resolving by it is not merely lossy, it is + // undefined — there is no row it names. + expect(series.map((s) => s.label)).toEqual([NULL_CATEGORY_LABEL, NULL_CATEGORY_LABEL]); + expect(findChartSeriesRow(RAW, DIMS, VALS, 'Backlog', NULL_CATEGORY_LABEL)).toBe(-1); + // The keys resolve, each to its own row. + expect(findChartSeriesRow(RAW, DIMS, VALS, 'Backlog', series[0].dataKey)).toBe(0); + expect(findChartSeriesRow(RAW, DIMS, VALS, 'Backlog', series[1].dataKey)).toBe(1); + }); + + it('carries a label a consumer may show, alongside the key it must not', () => { + const { series } = buildChartSeries(RAW, DIMS, VALS); + // The shape a renderer composes: `series` for `findChartSeriesRow`, + // `seriesLabel` for the title. Typed here so the field cannot be dropped + // from the interface without this file going red. + const ev: ChartSegmentClickEvent = { + category: 'Backlog', + series: series[1].dataKey, + seriesLabel: series[1].label, + }; + expect(ev.series).toBe('[null]'); + expect(ev.seriesLabel).toBe(NULL_CATEGORY_LABEL); + expect(findChartSeriesRow(RAW, DIMS, VALS, ev.category, ev.series)).toBe(1); + // The title's read, as the consumer performs it. + expect(ev.seriesLabel ?? ev.series).toBe(NULL_CATEGORY_LABEL); + }); + + it('is OPTIONAL — an event without one still titles from the key', () => { + // Renderers that resolve a series they have no label for (the scatter / + // treemap / sankey mappers) omit it, and their titles must not lose the + // series half. + const ev: ChartSegmentClickEvent = { category: 'Backlog', series: 'High' }; + expect(ev.seriesLabel).toBeUndefined(); + expect(ev.seriesLabel ?? ev.series).toBe('High'); + }); +}); diff --git a/packages/core/src/utils/chart-series.ts b/packages/core/src/utils/chart-series.ts index 6cf1f6382..a9c1e9143 100644 --- a/packages/core/src/utils/chart-series.ts +++ b/packages/core/src/utils/chart-series.ts @@ -197,6 +197,29 @@ export interface ChartSegmentClickEvent { categoryId?: string; /** The clicked series' key — a measure, or a pivoted second-dimension value. */ series?: string; + /** + * The clicked series' DISPLAY LABEL, when the renderer knew one + * (objectui#4682) — `series[].label` as {@link buildChartSeries} assigned it. + * + * {@link ChartSegmentClickEvent.series} stays the LOOKUP key and this never + * substitutes for it: {@link findChartSeriesRow} resolves a key through the + * same assignment `buildChartSeries` made, and a label cannot be resolved + * that way — it is not unique (that is precisely why the colliding groups key + * by identity instead). This field exists for what a key cannot do: be SHOWN. + * + * The two strings are equal for every ordinary group, which is why reading + * the key as a title went unnoticed. They part company when a group's label + * cannot name it — objectui#4508's collision on the series axis — and the key + * becomes an opaque `chartBucketId`. A drawer titled `Backlog / [null]` over + * a segment the user saw labelled `(None)` reads as broken DATA rather than + * as a broken title, which is why the label travels with the click rather + * than being re-derived by the consumer. + * + * Absent when the renderer resolved no series, or when the series carries no + * label distinct from its key; a consumer titles itself from + * `seriesLabel ?? series` and so is unchanged wherever it is absent. + */ + seriesLabel?: string; /** The measure value at the click point. */ value?: number; } diff --git a/packages/plugin-charts/src/AdvancedChartImpl.cartesianClickPayload.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.cartesianClickPayload.test.tsx index c25c1da7b..ae953a1c9 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.cartesianClickPayload.test.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.cartesianClickPayload.test.tsx @@ -203,13 +203,35 @@ describe('AdvancedChartImpl — the cartesian handler over the recharts 3 payloa expect(clicks[0].value).toBe(11); }); - it('leaves the series unresolved when the shared cursor names none — it does not guess one', () => { + /** + * RESTATED, not relaxed, by objectui#4672's ruled half. + * + * This assertion was written when NO cartesian click could resolve a series + * under the shared cursor, and it pinned the non-guessing contract for all of + * them. The mark-level handler has since answered the case it was standing in + * for: a click that lands ON a segment now resolves its series exactly + * (`AdvancedChartImpl.itemSeriesClick.test.tsx`). + * + * What it pins is therefore NARROWER now, and still load-bearing — it is + * objectui#4672's sub-decision 3 verbatim. Re-measured rather than assumed: + * this case drives the chart-level handler DIRECTLY, which is precisely the + * shape of a click that reached no mark (empty plot area, an axis label, a + * gap between bars), so the assertion holds unchanged and now says the thing + * the ruling requires — such a click stays category-only, and "drill the + * whole category" was rejected as a different product question. + * + * The end-to-end form of the same contract, through a real DOM click on the + * plot surface, is in the sibling file; both are kept because they fail for + * different reasons — that one if a mark handler ever fires for a non-mark + * target, this one if the handler itself starts guessing. + */ + it('leaves the series unresolved when a click reached no mark — it does not guess one', () => { const clicks: ChartSegmentClickEvent[] = []; renderPivoted((ev) => clicks.push(ev)); - // The same click under the DEFAULT (shared/axis) cursor these charts - // render: recharts dispatches axis interactions with `activeDataKey` - // hard-coded `undefined`, so the payload names no series at all. + // The DEFAULT (shared/axis) cursor these charts render: recharts dispatches + // axis interactions with `activeDataKey` hard-coded `undefined`, so the + // payload names no series at all — and no mark claimed this gesture. seam.onClick!({ activeCoordinate: { x: 372.5, y: 200 }, activeDataKey: undefined, @@ -224,9 +246,9 @@ describe('AdvancedChartImpl — the cartesian handler over the recharts 3 payloa expect(clicks[0].category).toBe('Done'); // ...and the series is left open rather than filled with series[0], which // would drill to another group's records — a WRONG drill, worse than the - // dead one. Resolving this needs the clicked mark, not this payload: - // objectui#4672's open half. + // dead one. expect(clicks[0].series).toBeUndefined(); + expect(clicks[0].seriesLabel).toBeUndefined(); expect(clicks[0].value).toBeUndefined(); }); @@ -287,6 +309,13 @@ describe('AdvancedChartImpl — the cartesian handler over the recharts 3 payloa * label but NO `activeDataKey`. That is why the pivoted drill cannot be * resolved from this payload, and it is the premise to re-measure when recharts * is upgraded. + * + * Still true and still the reason the mark-level handler exists + * (objectui#4672's ruled Option A): the series is resolved from the mark that + * was clicked, never from this payload. If a future recharts starts populating + * `activeDataKey` for axis interactions, this test goes red and the fallback + * arm above becomes reachable again — which is the outcome to notice, not to + * paper over. */ describe('recharts 3 — what a shared-cursor cartesian click actually reports', () => { it('reports an index and a label, and no series key', async () => { diff --git a/packages/plugin-charts/src/AdvancedChartImpl.itemSeriesClick.test.tsx b/packages/plugin-charts/src/AdvancedChartImpl.itemSeriesClick.test.tsx new file mode 100644 index 000000000..6f35e281d --- /dev/null +++ b/packages/plugin-charts/src/AdvancedChartImpl.itemSeriesClick.test.tsx @@ -0,0 +1,353 @@ +/** + * 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#4672's ruled half — the clicked MARK identifies the series. + * + * The sibling file `AdvancedChartImpl.cartesianClickPayload.test.tsx` pins what + * the chart-level (AXIS) click can answer, and measured the wall this file + * exists to get past: recharts 3 dispatches an axis interaction with + * `activeDataKey` hard-coded `undefined`, because the shared cursor spans every + * series at that tick. So a PIVOTED chart — two dimensions, one measure, the + * shape ADR-0021 introduced — could not resolve the series its drill lookup + * requires, and every segment of every such dashboard chart was a dead click. + * + * The answer is the mark itself: this component renders the `Bar` / `Line` / + * `Area`, so an item-level handler closes over the very `dataKey` it drew. + * + * WHAT THIS FILE ASSERTS, and why each case is here rather than in the sibling: + * + * 1. the headline harm, end to end — a real DOM click on a real segment of a + * real pivoted chart resolves through `findChartSeriesRow` to THAT group's + * row. Not the click event alone: the card's harm is a dead drill, so the + * lookup is part of the assertion; + * 2. ONE CLICK, ONE EVENT — both handlers fire for one gesture, so the count + * is the contract, not an implementation detail; + * 3. ADDITIVE — a click that lands on no mark keeps the objectui#4680 axis + * contract exactly, and nothing that resolved before stops resolving; + * 4. the series key is forwarded EXACTLY, `''` included (objectui#4673's + * empty-string group); + * 5. the label rides along for the drill title (objectui#4682). + * + * Real DOM clicks throughout, with the sibling's rAF idiom: recharts throttles + * pointer moves through `requestAnimationFrame` and the click reads the tooltip + * state that move left behind. + */ + +import React from 'react'; +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { render, cleanup, fireEvent, act } from '@testing-library/react'; +import { + buildChartSeries, + findChartSeriesRow, + NULL_CATEGORY_LABEL, + type ChartSegmentClickEvent, +} from '@object-ui/core'; + +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 flushFrame = async () => { + await act(async () => { + await new Promise((r) => requestAnimationFrame(() => r())); + await new Promise((r) => setTimeout(r, 0)); + }); +}; + +/** A real pointer gesture: the move recharts throttles, then the click. */ +const clickAt = async (el: Element, clientX: number, clientY: number) => { + fireEvent.mouseMove(el, { clientX, clientY }); + await flushFrame(); + fireEvent.click(el, { clientX, clientY }); + await flushFrame(); +}; + +/** Click the middle of an SVG rect, addressed as the DOM element it is. */ +const clickRect = async (rect: Element) => { + const x = Number(rect.getAttribute('x')) + Number(rect.getAttribute('width')) / 2; + const y = Number(rect.getAttribute('y')) + Number(rect.getAttribute('height')) / 2; + await clickAt(rect, x, y); +}; + +/** + * The rectangle series `si` drew over bucket `bi`. recharts groups marks per + * series (`.recharts-bar`), so this addresses a segment the way a user does: + * "the Low bar of the Done column". + */ +const segment = (container: HTMLElement, si: number, bi: number): Element => { + const group = container.querySelectorAll('.recharts-bar')[si]; + expect(group, `series group ${si}`).toBeTruthy(); + const rect = group.querySelectorAll('.recharts-rectangle')[bi]; + expect(rect, `bucket ${bi} of series ${si}`).toBeTruthy(); + return rect; +}; + +/** The pivot the card reports dead: 2 dimensions, 1 measure. */ +const PIVOT_RAW = [ + { status: 'Open', priority: 'High', est_hours: 3 }, + { status: 'Open', priority: 'Low', est_hours: 5 }, + { status: 'Done', priority: 'High', est_hours: 7 }, + { status: 'Done', priority: 'Low', est_hours: 11 }, +]; +const DIMS = ['status', 'priority']; +const VALS = ['est_hours']; + +function renderChart( + raw: Array>, + onChartClick: (ev: ChartSegmentClickEvent) => void, + chartType: 'bar' | 'line' | 'area' = 'bar', +) { + const { data, xAxisKey, series } = buildChartSeries(raw, DIMS, VALS); + const { container } = render( + , + ); + return { container, data, series }; +} + +describe('AdvancedChartImpl — a clicked MARK identifies its series (objectui#4672)', () => { + it('resolves the pivoted drill end to end — the card’s dead click, alive', async () => { + const clicks: ChartSegmentClickEvent[] = []; + const { container, series } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev)); + // Stated, not assumed: the second dimension became the series axis. + expect(series.map((s) => s.dataKey)).toEqual(['High', 'Low']); + + // The Low segment of the Done column — 11 hours. + await clickRect(segment(container, 1, 1)); + + expect(clicks).toHaveLength(1); + expect(clicks[0].category).toBe('Done'); + // Pre-fix this was `undefined` for EVERY multi-series cartesian click: the + // shared cursor names no series, so the payload could not answer. + expect(clicks[0].series).toBe('Low'); + expect(clicks[0].value).toBe(11); + + // The harm the card actually reports is the DRILL, so the lookup is part + // of the assertion. Pre-fix this returned -1 and the drawer never opened. + const idx = findChartSeriesRow( + PIVOT_RAW, + DIMS, + VALS, + clicks[0].category, + clicks[0].series, + { bucketId: clicks[0].categoryId }, + ); + expect(idx).toBe(3); + expect(PIVOT_RAW[idx]).toEqual({ status: 'Done', priority: 'Low', est_hours: 11 }); + }); + + it('resolves each segment to ITS OWN group, not to a shared answer', async () => { + // Four segments, four different rows. A fix that guessed (series[0], say) + // would pass the case above and fail here — which is the point of it. + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev)); + + for (const [si, bi] of [[0, 0], [1, 0], [0, 1], [1, 1]] as const) { + await clickRect(segment(container, si, bi)); + } + + expect(clicks.map((c) => [c.category, c.series, c.value])).toEqual([ + ['Open', 'High', 3], + ['Open', 'Low', 5], + ['Done', 'High', 7], + ['Done', 'Low', 11], + ]); + expect( + clicks.map((c) => findChartSeriesRow(PIVOT_RAW, DIMS, VALS, c.category, c.series, { bucketId: c.categoryId })), + ).toEqual([0, 1, 2, 3]); + }); + + it('emits exactly ONE event for one gesture — the item does not double-fire', async () => { + // Both handlers run for a mark click (measured: item first, chart second), + // so "one click, one drill event" is a contract this pins rather than an + // incidental property. A second event here would open the drawer twice, or + // open it on the series-less axis answer that arrives after the good one. + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev)); + + await clickRect(segment(container, 1, 1)); + + expect(clicks).toHaveLength(1); + }); + + it('carries the series through a line chart’s dot={false} stroke', async () => { + // Premise C, measured: these lines render no dots, so the stroke is the + // only mark there is. Clicking it now names the series exactly — a gain + // over the axis answer, which for a multi-series line named none. + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev), 'line'); + + const curves = container.querySelectorAll('.recharts-line-curve'); + expect(curves).toHaveLength(2); + await clickAt(curves[1], 300, 120); + + expect(clicks).toHaveLength(1); + expect(clicks[0].series).toBe('Low'); + }); + + it('carries the series through an area chart’s filled mark', async () => { + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev), 'area'); + + const areas = container.querySelectorAll('.recharts-area-area'); + expect(areas).toHaveLength(2); + await clickAt(areas[1], 300, 200); + + expect(clicks).toHaveLength(1); + expect(clicks[0].series).toBe('Low'); + }); +}); + +describe('AdvancedChartImpl — the mark handler is ADDITIVE (objectui#4672, sub-decision 2/3)', () => { + it('leaves a click on NO mark at objectui#4680’s contract — series unresolved', async () => { + // The empty plot area, addressed as the DOM does it: the event's target is + // the plot surface, not a mark, so no item handler runs. objectui#4680's + // ruling is kept VERBATIM here — category-only, series open, and no + // "drill the whole category" semantics invented. + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev)); + const surface = container.querySelector('.recharts-surface')!; + + await clickAt(surface, 200, 30); + + expect(clicks).toHaveLength(1); + expect(clicks[0].series).toBeUndefined(); + expect(clicks[0].seriesLabel).toBeUndefined(); + }); + + it('does not let a mark’s answer leak into the NEXT click', async () => { + // The staleness question a flag-and-timeout mechanism gets wrong: the two + // handlers are paired by the DOM event object they share, so a record left + // by one gesture can never be adopted by another. Click a segment, then + // click empty space — the second click must resolve nothing. + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev)); + + await clickRect(segment(container, 1, 1)); + await clickAt(container.querySelector('.recharts-surface')!, 200, 30); + + expect(clicks).toHaveLength(2); + expect(clicks[0].series).toBe('Low'); + expect(clicks[1].series).toBeUndefined(); + }); + + it('keeps the single-series chart resolving from the axis, mark or no mark', async () => { + // objectui#4680's other arm: one plotted series, so the clicked column can + // belong to nothing else. It must keep answering for clicks that reach no + // mark at all — "nothing that resolves today stops resolving". + const clicks: ChartSegmentClickEvent[] = []; + const { data, xAxisKey, series } = buildChartSeries( + [ + { status: 'Open', est_hours: 10 }, + { status: 'Done', est_hours: 30 }, + ], + ['status'], + ['est_hours'], + ); + expect(series.map((s) => s.dataKey)).toEqual(['est_hours']); + const { container } = render( + clicks.push(ev)} + />, + ); + + await clickAt(container.querySelector('.recharts-surface')!, 380, 40); + + expect(clicks).toHaveLength(1); + expect(clicks[0].series).toBe('est_hours'); + }); +}); + +describe('AdvancedChartImpl — the series key is forwarded exactly (objectui#4673)', () => { + it('sends the empty-string group’s own key, not an absent one', async () => { + // `''` is a real group with a real bar since objectui#4673, and it is + // FALSY. A truthiness test on the way out would send `series: undefined` + // and leave this bar's drill standing on the reader's coercion rather than + // on what was clicked. Forwarded as itself, it resolves by design. + const raw = [ + { status: 'Backlog', priority: '', est_hours: 40 }, + { status: 'Backlog', priority: 'High', est_hours: 5 }, + ]; + const clicks: ChartSegmentClickEvent[] = []; + const { container, series } = renderChart(raw, (ev) => clicks.push(ev)); + expect(series.map((s) => s.dataKey)).toEqual(['', 'High']); + + await clickRect(segment(container, 0, 0)); + + expect(clicks).toHaveLength(1); + expect(clicks[0].series).toBe(''); + expect(clicks[0].value).toBe(40); + // …and it lands on the empty-string group's OWN row. + expect( + findChartSeriesRow(raw, DIMS, VALS, clicks[0].category, clicks[0].series, { + bucketId: clicks[0].categoryId, + }), + ).toBe(0); + }); +}); + +describe('AdvancedChartImpl — the clicked series carries its LABEL (objectui#4682)', () => { + it('sends the identity KEY for the lookup and the readable LABEL beside it', async () => { + // objectui#4508's collision on the series axis: a stored value spelling the + // null bucket's label, beside a genuine null. Neither group's label can + // name it, so both key by identity — and the key is exactly what must NOT + // be shown to a user. + const raw = [ + { status: 'Backlog', priority: NULL_CATEGORY_LABEL, est_hours: 1 }, + { status: 'Backlog', priority: null, est_hours: 2 }, + ]; + const clicks: ChartSegmentClickEvent[] = []; + const { container, series } = renderChart(raw, (ev) => clicks.push(ev)); + expect(series).toEqual([ + { dataKey: '["(None)"]', label: NULL_CATEGORY_LABEL }, + { dataKey: '[null]', label: NULL_CATEGORY_LABEL }, + ]); + + await clickRect(segment(container, 1, 0)); + + expect(clicks).toHaveLength(1); + // The KEY stays the lookup's answer — it is what resolves the right rows… + expect(clicks[0].series).toBe('[null]'); + expect( + findChartSeriesRow(raw, DIMS, VALS, clicks[0].category, clicks[0].series, { + bucketId: clicks[0].categoryId, + }), + ).toBe(1); + // …and the LABEL travels beside it, which is all a title may read. + expect(clicks[0].seriesLabel).toBe(NULL_CATEGORY_LABEL); + }); + + it('mirrors the key for an ordinary group, so ordinary titles are unchanged', async () => { + const clicks: ChartSegmentClickEvent[] = []; + const { container } = renderChart(PIVOT_RAW, (ev) => clicks.push(ev)); + + await clickRect(segment(container, 1, 1)); + + expect(clicks[0].series).toBe('Low'); + expect(clicks[0].seriesLabel).toBe('Low'); + }); +}); diff --git a/packages/plugin-charts/src/AdvancedChartImpl.tsx b/packages/plugin-charts/src/AdvancedChartImpl.tsx index fcd80e63f..23671a23b 100644 --- a/packages/plugin-charts/src/AdvancedChartImpl.tsx +++ b/packages/plugin-charts/src/AdvancedChartImpl.tsx @@ -133,6 +133,75 @@ const resolveClickedSeriesKey = ( return undefined; }; +/** + * The DOM event behind a React synthetic one — the IDENTITY of a single user + * gesture (objectui#4672). + * + * Load-bearing rather than incidental: a click on a mark reaches the item-level + * handler and the chart-level handler as two separate callbacks, and the only + * thing that says they are ONE click is that both were handed the same + * `nativeEvent` object. Measured on recharts 3.10.1 for bar, line and area + * alike: identical object, item first, chart second. Matching on that object + * rather than on a flag-and-timeout means a stale record can never be adopted + * by a later click — a different gesture is a different object, always. + */ +const gestureIdOf = (value: unknown): unknown => { + if (value && typeof value === 'object' && 'nativeEvent' in value) { + return (value as { nativeEvent: unknown }).nativeEvent; + } + return undefined; +}; + +/** + * The gesture identity out of an ITEM handler's arguments, whose shape differs + * per mark family — measured, because recharts does not type two of the three. + * + * - `Bar`: `(item: BarRectangleItem, index: number, event)` — three args, and + * the item carries the row and the value. + * - `Line` / `Area`: `(curveProps, event)` — TWO args, and the first is the + * rendered curve's props, not a datum. A line/area mark click therefore + * knows WHICH SERIES it is (this component rendered it) and nothing about + * which category, which is exactly why the series is recorded here and the + * event is still composed by the chart-level handler, which does know. + * + * Scanning from the end takes the event in both shapes without branching on the + * mark family, and returns `undefined` for any shape carrying no event at all — + * an argument list this code has never seen cannot be mistaken for a gesture. + */ +const gestureIdOfArgs = (args: unknown[]): unknown => { + for (let i = args.length - 1; i >= 0; i -= 1) { + const id = gestureIdOf(args[i]); + if (id !== undefined) return id; + } + return undefined; +}; + +/** + * The DISPLAY LABEL of a plotted series, given the key a click resolved to + * (objectui#4682). + * + * A series key is a `dataKey` — what the renderer binds and what the drill + * lookup resolves back to a group. Its label is what the legend paints. For + * every ordinary group those are the same string, which is why reading the key + * as a title has passed unnoticed; they part company exactly when a group keys + * by its IDENTITY because its label cannot name it (objectui#4673's + * `pivotSeriesBuckets`, over objectui#4508's collision) — and then the drawer + * title reads `[null]` at a segment the user saw labelled `(None)`. + * + * Keys are unique within a chart's series (`buildChartSeries` assigns them + * injectively; the measure branch keys by measure name), so this lookup is + * unambiguous and both arms — the clicked mark and the axis fallback — resolve + * their label through this one path. + */ +const seriesLabelForKey = ( + key: string | undefined, + plotted: NormalizedSeries[], +): string | undefined => { + if (key == null) return undefined; + const found = plotted.find((s) => String(s.dataKey) === key); + return typeof found?.label === 'string' ? found.label : undefined; +}; + export interface AdvancedChartImplProps { /** * Chart family. `combo` is renderer-local and rarely needs to be passed: @@ -352,8 +421,55 @@ function AdvancedChartImplInner({ // its series and its value.) // // WHICH SERIES was clicked is the one thing the payload cannot always answer - // — see the resolver below. - const handleCartesianClick = React.useCallback((payload: any) => { + // — see `resolveClickedSeriesKey`, and the mark handler below it. + // + // ── The clicked mark (objectui#4672's ruled Option A) ────────────────────── + // A chart-level cartesian click is an AXIS interaction, and recharts names no + // series in one: several series sit under one shared cursor at a tick, so the + // payload cannot say which of them the pointer was over. That left every + // PIVOTED drill dead, because its lookup matches on the second dimension. + // + // The mark itself knows. This component renders the series, so a `Bar` / + // `Line` / `Area` item handler closes over the very `dataKey` it was rendered + // with — the answer is statically known, not inferred from tooltip state. + // + // Both handlers fire for one click (item first, chart second — measured), so + // the item handler does NOT emit: it RECORDS its series, stamped with the + // gesture, and the chart-level handler below emits the single event. That is + // the double-fire answer and the additive property at once — + // + // - exactly one `onChartClick` per gesture, because there is exactly one + // emit site, rather than a second event suppressed after the fact; + // - a click that lands on NO mark records nothing, so it falls through to + // the axis answer this handler already gave (category + identity, series + // when unambiguous) with not one byte changed. Nothing that resolved + // before stops resolving; a line's `dot={false}` stroke simply gains the + // exact series where the stroke itself is hit. + // + // It also has to be this way round for line and area, whose item handlers are + // handed the curve's props and no datum: the mark knows its series and only + // the chart-level payload knows the category. Each contributes what it has. + const clickedMark = React.useRef<{ gesture: unknown; dataKey: string } | null>(null); + + const handleMarkClick = React.useCallback( + (s: NormalizedSeries) => (...args: unknown[]) => { + const gesture = gestureIdOfArgs(args); + if (gesture === undefined) return; + // Recorded EXACTLY as rendered, `''` included. The empty string is a real + // second-dimension group as of objectui#4673 — it draws its own bar and + // its key is its own label — and `''` is falsy, so any truthiness test on + // the way out would send `series: undefined` and hand that bar's click to + // whichever group an absent key happens to coerce to. + clickedMark.current = { gesture, dataKey: String(s.dataKey) }; + }, + [], + ); + + const markClickProps = onChartClick + ? (s: NormalizedSeries) => ({ onClick: handleMarkClick(s) }) + : () => ({}); + + const handleCartesianClick = React.useCallback((payload: any, event?: any) => { if (!onChartClick || !payload) return; // A click with no active tick (the plot margins, an axis label) reports a // NULL index, not an absent one — and `Number(null)` is 0, which would @@ -362,12 +478,24 @@ function AdvancedChartImplInner({ const rawIdx = payload.activeTooltipIndex ?? payload.activeIndex; const idx = rawIdx == null ? Number.NaN : Number(rawIdx); const row = Number.isInteger(idx) && idx >= 0 ? data[idx] : undefined; - const clickedKey = resolveClickedSeriesKey(payload.activeDataKey, series); + // The mark handler runs first and only for THIS gesture; anything left over + // from a click that never reached here belongs to a different DOM event and + // can never be adopted by this one. + const mark = clickedMark.current; + clickedMark.current = null; + const gesture = gestureIdOf(event); + const onMark = mark != null && gesture !== undefined && mark.gesture === gesture; + const clickedKey = onMark + ? mark!.dataKey + : resolveClickedSeriesKey(payload.activeDataKey, series); const cell = clickedKey != null && row ? (row as Record)[clickedKey] : undefined; onChartClick({ category: payload.activeLabel != null ? String(payload.activeLabel) : undefined, categoryId: chartRowBucketId(row), series: clickedKey, + // The KEY stays the lookup's answer; the LABEL rides alongside it so the + // drill title can read what the segment actually said (objectui#4682). + seriesLabel: seriesLabelForKey(clickedKey, series), value: typeof cell === 'number' ? cell : undefined, }); }, [onChartClick, data, series]); @@ -383,6 +511,10 @@ function AdvancedChartImplInner({ category: cat != null ? String(cat) : undefined, categoryId: chartRowBucketId(entry.payload), series: dk, + // Measured for objectui#4682: this path sends only the KEY, so a pie over + // a pivot whose first group keys by identity titles its drawer with that + // identity. The funnel path is untouched — it sends no series at all. + seriesLabel: seriesLabelForKey(dk, series), value: typeof entry.payload?.[dk] === 'number' ? entry.payload[dk] : undefined, }); }, [onChartClick, xAxisKey, series]); @@ -1145,7 +1277,7 @@ function AdvancedChartImplInner({ const colorPerCategory = primaryCount === 1 && !isComparison && series.length === 1 && data.length > 1; const cmp = comparisonStyle(s, 'bar'); return ( - + {colorPerCategory && data.map((entry, idx) => ( ))} @@ -1156,7 +1288,7 @@ function AdvancedChartImplInner({ if (chartType === 'line') { const cmp = comparisonStyle(s, 'line'); return ( - + {dataLabel(valueFormatter)} ); @@ -1164,7 +1296,7 @@ function AdvancedChartImplInner({ if (chartType === 'area') { const cmp = comparisonStyle(s, 'area'); return ( - + {dataLabel(valueFormatter)} ); diff --git a/packages/plugin-dashboard/src/DatasetWidget.tsx b/packages/plugin-dashboard/src/DatasetWidget.tsx index f21bb0d15..144876771 100644 --- a/packages/plugin-dashboard/src/DatasetWidget.tsx +++ b/packages/plugin-dashboard/src/DatasetWidget.tsx @@ -1447,8 +1447,19 @@ export function DatasetWidget({ widget, dataSource }: { widget: any; dataSource: if (idx < 0) return; // `series` is a real (second) dimension value only when pivoted; otherwise // it's the measure name — omit it from the drawer title. + // + // The title reads the LABEL, not the key (objectui#4682). `ev.series` is a + // renderer `dataKey`, and the lookup above is the only thing that should + // consult it: for every ordinary group the key IS the label, but a group + // whose label cannot name it keys by its `chartBucketId` instead + // (objectui#4508's collision on the series axis, reachable since + // objectui#4673), and the drawer then announced `Backlog / [null]` over a + // segment the user saw labelled `(None)` — the right records under a title + // that reads as broken data. `seriesLabel` is absent wherever the renderer + // knew no label, so every other chart's title is byte-identical. const pivoted = dimensions.length >= 2 && values.length === 1; - const title = [ev?.category, pivoted ? ev?.series : undefined].filter(Boolean).map(String).join(' / '); + const seriesTitle = ev?.seriesLabel ?? ev?.series; + const title = [ev?.category, pivoted ? seriesTitle : undefined].filter(Boolean).map(String).join(' / '); openDrill(idx, title); }; diff --git a/packages/plugin-dashboard/src/__tests__/DatasetWidget.drillTitleLabel.test.tsx b/packages/plugin-dashboard/src/__tests__/DatasetWidget.drillTitleLabel.test.tsx new file mode 100644 index 000000000..45431e774 --- /dev/null +++ b/packages/plugin-dashboard/src/__tests__/DatasetWidget.drillTitleLabel.test.tsx @@ -0,0 +1,344 @@ +/** + * 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#4682 — the drill drawer's TITLE, over a group that keys by identity. + * + * `handleChartDrill` composed its title out of `ev.series`, which is a renderer + * `dataKey` and not a display label. For every ordinary group those are the + * same string, which is why this read correctly for so long. They part company + * exactly when a group's label cannot NAME it — objectui#4508's collision on + * the series axis, reachable since objectui#4673 — and the group keys its + * column by its `chartBucketId` instead. + * + * The user then clicks a segment labelled `(None)` and the drawer opens on the + * right records under the title `Backlog / [null]`. Right data, opaque title: + * an internal id where a label belongs reads as broken DATA rather than as a + * broken title, which is the whole reason this was filed. + * + * The KEY still does the lookup — `findChartSeriesRow` resolves it through the + * same assignment `buildChartSeries` made, and a label cannot be resolved that + * way because a label is exactly what these two groups SHARE. Only the title + * reads `seriesLabel`. + * + * DIRECTIONS, written before the reverse verification was run: the identity + * case is RED before the change (the title carries `[null]`), and every + * ordinary case is GREEN on both sides — an ordinary group's label IS its key, + * so its title cannot move. + */ + +import * as React from 'react'; +import { describe, it, expect, vi, beforeAll, afterEach } from 'vitest'; +import { render, cleanup, waitFor } from '@testing-library/react'; +import { + ComponentRegistry, + NULL_CATEGORY_LABEL, + buildChartSeries, + chartRowBucketId, + type ChartSegmentClickEvent, +} from '@object-ui/core'; +import { I18nProvider } from '@object-ui/i18n'; +import { DatasetWidget } from '../DatasetWidget'; + +/** Capture what the widget hands the chart renderer (jsdom lays out no SVG). */ +let capturedChartProps: any = null; +beforeAll(() => { + ComponentRegistry.register('chart', (props: any) => { + capturedChartProps = props; + return null; + }); +}); + +/** Observe the drawer's TITLE — the field under test — and its filter. */ +const drawerProps: Array<{ title: string; filter: Record }> = []; +vi.mock('../DrillDownDrawer', () => ({ + DrillDownDrawer: ({ title, filter }: { title: string; filter: Record }) => { + drawerProps.push({ title, filter }); + return null; + }, +})); + +afterEach(() => { + cleanup(); + capturedChartProps = null; + drawerProps.length = 0; + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +/** Plain TEXT dimensions: no option list can relabel anything under the test. */ +const TASK = { + name: 'proj_task', + fields: { status_id: { type: 'text' }, priority_id: { type: 'text' } }, +}; + +function installMetaRouter() { + vi.stubGlobal( + 'fetch', + vi.fn(async () => ({ ok: true, json: async () => ({ item: TASK }) })), + ); +} + +const DIMS = ['status', 'priority']; +const VALS = ['est_hours']; + +const datasetOf = ( + rows: Array>, + rawRows: Array>, +) => ({ + queryDataset: vi.fn(async () => ({ + rows, + fields: [ + { name: 'status', type: 'text', label: 'Status' }, + { name: 'priority', type: 'text', label: 'Priority' }, + { name: 'est_hours', type: 'number', label: 'Hours' }, + ], + object: 'proj_task', + // Both dimensions drill, so the filter names the clicked GROUP as well as + // the clicked bucket — which is what makes a wrong series visible. + dimensionFields: { status: 'status_id', priority: 'priority_id' }, + drillRawRows: rawRows, + })), +}); + +function renderWidget(dataSource: { queryDataset: unknown }) { + return render( + + + , + ); +} + +/** The series the widget handed the renderer — one per drawn group. */ +const chartSeries = (): Array<{ dataKey: string; label?: string }> => + capturedChartProps?.schema?.series ?? []; +const chartData = (): Array> => capturedChartProps?.schema?.data ?? []; + +/** + * Click the segment series `si` drew over bucket `bi`, composing exactly what + * `AdvancedChartImpl`'s mark handler composes: the key for the lookup, the + * label for the title, and the bucket's identity. + */ +const clickSegment = (si: number, bi: number) => { + const s = chartSeries()[si]; + const row = chartData()[bi]; + const ev: ChartSegmentClickEvent = { + category: row[capturedChartProps.schema.xAxisKey] == null + ? undefined + : String(row[capturedChartProps.schema.xAxisKey]), + categoryId: chartRowBucketId(row), + series: s.dataKey, + seriesLabel: s.label, + value: typeof row[s.dataKey] === 'number' ? (row[s.dataKey] as number) : undefined, + }; + capturedChartProps.onSegmentClick(ev); + return ev; +}; + +const lastDrawer = () => drawerProps[drawerProps.length - 1]; + +describe('DatasetWidget — the drill title reads the series LABEL (objectui#4682)', () => { + it('titles an identity-keyed group with what its segment said, not with its key', async () => { + installMetaRouter(); + renderWidget( + datasetOf( + [ + // Two groups that PAINT the same text: one stores it literally, one + // has no value at all. objectui#4508's collision, on the series axis. + { status: 'Backlog', priority: NULL_CATEGORY_LABEL, est_hours: 1 }, + { status: 'Backlog', priority: null, est_hours: 2 }, + ], + [ + { status: 'st-backlog', priority: 'pri-literal' }, + { status: 'st-backlog', priority: null }, + ], + ), + ); + + await waitFor(() => expect(chartSeries().length).toBe(2)); + // The precondition, stated rather than assumed: neither group's label can + // name it, so both key by identity and the key is NOT a readable string. + expect(chartSeries()).toEqual([ + { dataKey: '["(None)"]', label: NULL_CATEGORY_LABEL }, + { dataKey: '[null]', label: NULL_CATEGORY_LABEL }, + ]); + + const ev = clickSegment(1, 0); + expect(ev.series).toBe('[null]'); + + await waitFor(() => expect(drawerProps.length).toBe(1)); + // The KEY did the lookup — the right records, which is what objectui#4673 + // already guaranteed and what must not move… + expect(lastDrawer().filter).toEqual({ status_id: 'st-backlog', priority_id: null }); + // …and the TITLE reads the label. Pre-fix: 'Backlog / [null]'. + expect(lastDrawer().title).toBe(`Backlog / ${NULL_CATEGORY_LABEL}`); + expect(lastDrawer().title).not.toContain('[null]'); + }); + + it('titles the OTHER colliding group with the same text, and drills it apart', async () => { + // Both segments read `(None)` to the user, so both titles must — while the + // filters stay different. Label and identity answer different questions. + installMetaRouter(); + renderWidget( + datasetOf( + [ + { status: 'Backlog', priority: NULL_CATEGORY_LABEL, est_hours: 1 }, + { status: 'Backlog', priority: null, est_hours: 2 }, + ], + [ + { status: 'st-backlog', priority: 'pri-literal' }, + { status: 'st-backlog', priority: null }, + ], + ), + ); + await waitFor(() => expect(chartSeries().length).toBe(2)); + + clickSegment(0, 0); + await waitFor(() => expect(drawerProps.length).toBe(1)); + expect(lastDrawer().title).toBe(`Backlog / ${NULL_CATEGORY_LABEL}`); + expect(lastDrawer().filter).toEqual({ status_id: 'st-backlog', priority_id: 'pri-literal' }); + }); + + it('BOUNDARY — an ordinary group’s title is byte-identical to before', async () => { + // Green on both sides: an ordinary group keys by its own label, so + // `seriesLabel ?? series` cannot change what this title says. + installMetaRouter(); + renderWidget( + datasetOf( + [ + { status: 'Backlog', priority: 'High', est_hours: 5 }, + { status: 'Backlog', priority: 'Low', est_hours: 3 }, + ], + [ + { status: 'st-backlog', priority: 'pri-high' }, + { status: 'st-backlog', priority: 'pri-low' }, + ], + ), + ); + await waitFor(() => expect(chartSeries().length).toBe(2)); + expect(chartSeries()).toEqual([ + { dataKey: 'High', label: 'High' }, + { dataKey: 'Low', label: 'Low' }, + ]); + + clickSegment(1, 0); + await waitFor(() => expect(drawerProps.length).toBe(1)); + expect(lastDrawer().title).toBe('Backlog / Low'); + expect(lastDrawer().filter).toEqual({ status_id: 'st-backlog', priority_id: 'pri-low' }); + }); + + it('BOUNDARY — a click carrying no label still titles from the key', async () => { + // Every renderer that resolves a series does send the label now, but the + // field is OPTIONAL on the event (scatter / treemap / sankey map their own + // events and know no series label). Those clicks must keep the title they + // have always had rather than losing the series half of it. + installMetaRouter(); + renderWidget( + datasetOf( + [{ status: 'Backlog', priority: 'High', est_hours: 5 }], + [{ status: 'st-backlog', priority: 'pri-high' }], + ), + ); + await waitFor(() => expect(chartSeries().length).toBe(1)); + + capturedChartProps.onSegmentClick({ + category: 'Backlog', + categoryId: chartRowBucketId(chartData()[0]), + series: 'High', + }); + + await waitFor(() => expect(drawerProps.length).toBe(1)); + expect(lastDrawer().title).toBe('Backlog / High'); + }); + + it('BOUNDARY — a NON-pivoted widget still omits the series from its title', async () => { + // `seriesLabel` must not smuggle the measure name into a title the pivot + // check deliberately keeps out of it. + installMetaRouter(); + render( + + ({ + rows: [{ status: 'Backlog', est_hours: 5 }], + fields: [ + { name: 'status', type: 'text', label: 'Status' }, + { name: 'est_hours', type: 'number', label: 'Hours' }, + ], + object: 'proj_task', + dimensionFields: { status: 'status_id' }, + drillRawRows: [{ status: 'st-backlog' }], + })), + } as any} + /> + , + ); + + await waitFor(() => expect(chartData().length).toBe(1)); + capturedChartProps.onSegmentClick({ + category: 'Backlog', + categoryId: chartRowBucketId(chartData()[0]), + series: 'est_hours', + seriesLabel: 'Hours', + }); + + await waitFor(() => expect(drawerProps.length).toBe(1)); + expect(lastDrawer().title).toBe('Backlog'); + }); +}); + +/** + * The transform's own statement of the fact this card rests on, kept beside the + * consumer that reads it: a group's key and its label are DIFFERENT strings + * exactly when the label cannot name the group. + */ +describe('buildChartSeries — key and label diverge only on collision (objectui#4682)', () => { + it('gives the colliding groups one label and two keys', () => { + const { series } = buildChartSeries( + [ + { status: 'Backlog', priority: NULL_CATEGORY_LABEL, est_hours: 1 }, + { status: 'Backlog', priority: null, est_hours: 2 }, + ], + DIMS, + VALS, + ); + expect(series.map((s) => s.label)).toEqual([NULL_CATEGORY_LABEL, NULL_CATEGORY_LABEL]); + expect(new Set(series.map((s) => s.dataKey)).size).toBe(2); + // Which is the whole reason the title cannot be composed from the key, and + // the lookup cannot be performed from the label. + expect(series.every((s) => s.dataKey === s.label)).toBe(false); + }); + + it('keeps them identical for an ordinary pivot', () => { + const { series } = buildChartSeries( + [ + { status: 'Backlog', priority: 'High', est_hours: 5 }, + { status: 'Backlog', priority: 'Low', est_hours: 3 }, + ], + DIMS, + VALS, + ); + expect(series.every((s) => s.dataKey === s.label)).toBe(true); + }); +});