From e8d2c334e6838016bac57fa3ba74d2e282aeb0c3 Mon Sep 17 00:00:00 2001 From: Boris Tyshkevich Date: Fri, 17 Jul 2026 07:13:57 +0000 Subject: [PATCH] fix(#111): cap chart X categories, not raw rows; honest transform note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildChartData() previously sliced the raw rows to chartRowCap(type) BEFORE grouping, so a row past the cap was dropped from its category's sum and a category could vanish only because its first row arrived late. Now it is two-pass: pass 1 discovers unique raw X keys across ALL fetched rows in first-seen order and retains the first `cap`; pass 2 aggregates every fetched row whose X is a retained category. Categories are never value-ranked, sorted, or folded into an Other bucket. The result carries typed ChartDataMeta (totalRows/totalCategories/ shownCategories/categoriesTruncated/duplicateCellsSummed/groupKey) and a new pure chartDataNote(meta) renders "first N of M categories" and/or "duplicate X[/series] rows summed in the browser" — describing only the browser-side transform, never whether the SQL pre-aggregated. Duplicate detection keys on X (or (X, series)), so a normal multi-series result is not reported as summing, and a duplicate confined to an omitted category never flags the visible chart. renderChart aggregates once (chartJsConfig accepts the precomputed result) and renders the note on every surface: inline in the config bar on the workbench, and as a standalone block above the canvas on Dashboard / read-only / detached tiles (controls:false), which previously capped categories silently. Tests: buildChartData category-cap + metadata cases, chartDataNote formatter, renderChart note text/placement across control states + the single-aggregation contract, and a Dashboard-surface panels regression. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01XP1KN94N969SVMNFPdVtGg --- CHANGELOG.md | 19 +++++ src/core/chart-data.ts | 134 +++++++++++++++++++++++------ src/styles.css | 6 ++ src/ui/chart-render.ts | 50 +++++++---- tests/unit/chart-data.test.ts | 146 +++++++++++++++++++++++++++++++- tests/unit/chart-render.test.ts | 78 +++++++++++++++-- tests/unit/panels.test.ts | 14 +++ 7 files changed, 394 insertions(+), 53 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8437d92..4501c15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -61,6 +61,25 @@ auto-generated per-PR notes; this file is the curated, human-readable history. behavior or schema change. ### Fixed +- **Chart aggregation sums the whole fetched result, and the note is honest** + (#111, follow-up to #109). `buildChartData()` no longer slices the raw rows + before grouping — it discovers unique X categories across *all* fetched rows + (first-seen order), retains the first `chartRowCap(type)` of them, and + aggregates every fetched row whose X falls in a retained category. A row past + the old raw-row boundary now still contributes to its category, and a category + is no longer dropped merely because its first row arrived late. The result + carries typed `meta` (`totalRows`/`totalCategories`/`shownCategories`/ + `categoriesTruncated`/`duplicateCellsSummed`/`groupKey`); a new pure + `chartDataNote(meta)` renders `first N of M categories` and/or + `duplicate X[/series] rows summed in the browser` — describing only the + browser-side transform, never whether the SQL pre-aggregated. Duplicate + detection keys on X (or `(X, series)`), so a normal multi-series result with + repeated X values is not reported as summing, and a duplicate confined to an + omitted category never flags the visible chart. The note now renders on every + surface — Dashboard and read-only/detached tiles (`controls:false`) disclose + category truncation and duplicate summing as a standalone block above the + canvas, instead of silently capping. Chart data is aggregated once per render + (`chartJsConfig` accepts the precomputed result). - **Share-link result view is validated before use** (#266). The share-link bootstrap (`src/main.ts`) now narrows `spec.view` against the `resultView` enum before assigning it — the v2 tagged decode passes `spec.view` through diff --git a/src/core/chart-data.ts b/src/core/chart-data.ts index ecc3487..1a68240 100644 --- a/src/core/chart-data.ts +++ b/src/core/chart-data.ts @@ -4,7 +4,7 @@ // pivot/scale layer is unit-testable at 100% and the DOM glue in // `ui/results.js` stays a thin wrapper around `new Chart(canvas, config)`. -import { isNumericType } from './format.js'; +import { isNumericType, formatRows } from './format.js'; import { hasFieldValueFormat, resolveFieldConfig } from './field-config.js'; import type { FieldPresentation } from './field-config.js'; import type { @@ -522,23 +522,59 @@ export function visibleChartMeasures(columns: Column[], cfg: ChartConfig, fieldC }).filter((measure) => !measure.presentation.hidden); } +/** The browser-side aggregation identity a chart grouped its fetched rows on: + * raw `X` alone (`'x'`), or raw `(X, Series)` when a group-by is set. */ +export type ChartGroupKey = 'x' | 'x+series'; + +/** + * Typed, explicit metadata about the browser-side transform `buildChartData` + * applied — how many rows/categories were fetched vs. displayed, and whether + * the visible chart combined duplicate aggregation cells. It describes only + * what the chart did; it makes no claim about whether the SQL pre-aggregated. + */ +export interface ChartDataMeta { + /** `rows.length` — every fetched row, before the category cap. */ + totalRows: number; + /** Unique raw X values across the full fetched result. */ + totalCategories: number; + /** Retained (displayed) X categories — `min(totalCategories, cap)`. */ + shownCategories: number; + /** `shownCategories < totalCategories`. */ + categoriesTruncated: boolean; + /** At least two fetched rows mapped to the same *displayed* aggregation + * cell (X without Series, `(X, Series)` with Series). A duplicate that + * occurs only in an omitted category never sets this. */ + duplicateCellsSummed: boolean; + /** The aggregation identity — `'x'` when `cfg.series == null`, else `'x+series'`. */ + groupKey: ChartGroupKey; +} + /** `buildChartData`'s library-agnostic result: labels + one dataset per - * measure or series value. */ + * measure or series value, plus explicit transform metadata. */ export interface ChartDataResult { labels: string[]; datasets: { label: string; data: (number | null)[] }[]; + meta: ChartDataMeta; } /** - * Transform `rows` (capped) + columns into a library-agnostic - * { labels, datasets:[{label, data}] } per the encoding in `cfg`. Rows are - * grouped on the *raw* X cell value (first-seen order) and the measure is - * SUM-aggregated per cell, so multiple rows sharing an X bucket combine rather - * than the last one silently winning. `chartLabel` is applied only to the - * final tick text, never to the grouping identity. - * - group-by (cfg.series set): one dataset per series value, aligned to the - * union of X categories, missing cell → null. + * Transform the *full* fetched `rows` + columns into a library-agnostic + * { labels, datasets:[{label, data}], meta } per the encoding in `cfg`. + * + * The row cap (`chartRowCap(cfg.type)`) limits the number of X *categories*, + * never the raw rows aggregated: pass 1 discovers unique raw X keys in + * first-seen order and retains the first `cap` of them; pass 2 aggregates + * every fetched row whose X is one of the retained categories, so a row that + * appears after the cap-th input row still contributes to its displayed + * category. Categories are neither value-ranked, sorted, nor folded into an + * `Other` bucket. Grouping identity is the *raw* X cell value (`String(...)`); + * `chartLabel` is applied only to the final tick text. + * - group-by (cfg.series set): one dataset per series value *encountered in a + * displayed category*, aligned to the retained X categories, missing → null; * - otherwise: one dataset per visible measure in `cfg.y`. + * The measure is SUM-aggregated per cell, so multiple rows sharing a cell + * combine rather than the last one silently winning. `meta.duplicateCellsSummed` + * records whether that happened for a *displayed* cell. * `measures` defaults to `visibleChartMeasures(...)` but the caller * (`chartJsConfig`) passes the value it already resolved so a single render * doesn't resolve field metadata twice; direct/test callers may omit it. @@ -550,7 +586,6 @@ export function buildChartData( fieldConfig: FieldConfig = {}, measures: ChartMeasure[] = visibleChartMeasures(columns, cfg, fieldConfig), ): ChartDataResult { - const slice = rows.slice(0, chartRowCap(cfg.type)); const num = (v: unknown): number | null => { if (v == null || v === '') return null; const parsed = Number(v); @@ -560,40 +595,84 @@ export function buildChartData( const parsed = num(value); if (parsed != null) map.set(key, (map.get(key) || 0) + parsed); }; - const cats: string[] = []; // raw X keys, first-seen order - const seen = new Set(); - const noteCat = (xk: string): void => { if (!seen.has(xk)) { seen.add(xk); cats.push(xk); } }; + // Pass 1: discover unique raw X categories in first-seen order across the + // full fetched result, then retain the first `cap` for display. + const allCats: string[] = []; + const seenCat = new Set(); + for (const row of rows) { + const xk = String(row[cfg.x]); + if (!seenCat.has(xk)) { seenCat.add(xk); allCats.push(xk); } + } + const cats = allCats.slice(0, chartRowCap(cfg.type)); // displayed, first-seen order + const displayed = new Set(cats); + const groupKey: ChartGroupKey = cfg.series != null ? 'x+series' : 'x'; + const meta = (duplicateCellsSummed: boolean): ChartDataMeta => ({ + totalRows: rows.length, + totalCategories: allCats.length, + shownCategories: cats.length, + categoriesTruncated: cats.length < allCats.length, + duplicateCellsSummed, + groupKey, + }); + + // Pass 2: aggregate only rows whose X belongs to a displayed category. if (cfg.series != null) { const yi = measures[0]?.index; - if (yi == null) return { labels: [], datasets: [] }; + if (yi == null) return { labels: [], datasets: [], meta: meta(false) }; const groups = new Map>(); // seriesValue -> Map(xKey -> summed y) - for (const row of slice) { + const seenCells = new Map>(); // seriesValue -> Set(xKey) already seen + let dup = false; + for (const row of rows) { const xk = String(row[cfg.x]); - noteCat(xk); + if (!displayed.has(xk)) continue; const sk = String(row[cfg.series]); + let cellSet = seenCells.get(sk); + if (!cellSet) { cellSet = new Set(); seenCells.set(sk, cellSet); } + if (cellSet.has(xk)) dup = true; else cellSet.add(xk); if (!groups.has(sk)) groups.set(sk, new Map()); - const byCat = groups.get(sk)!; - add(byCat, xk, row[yi]); + add(groups.get(sk)!, xk, row[yi]); } const datasets = [...groups.entries()].map(([name, byCat]) => ({ label: name, data: cats.map((xk) => byCat.has(xk) ? byCat.get(xk)! : null), })); - return { labels: cats.map(chartLabel), datasets }; + return { labels: cats.map(chartLabel), datasets, meta: meta(dup) }; } const sums: Map[] = measures.map(() => new Map()); // per measure: xKey -> summed y - for (const row of slice) { + const seenX = new Set(); + let dup = false; + for (const row of rows) { const xk = String(row[cfg.x]); - noteCat(xk); + if (!displayed.has(xk)) continue; + if (seenX.has(xk)) dup = true; else seenX.add(xk); measures.forEach(({ index }, mi) => add(sums[mi], xk, row[index])); } const datasets = measures.map(({ presentation }, mi) => ({ label: presentation.displayName, data: cats.map((xk) => sums[mi].has(xk) ? sums[mi].get(xk)! : null), })); - return { labels: cats.map(chartLabel), datasets }; + return { labels: cats.map(chartLabel), datasets, meta: meta(dup) }; +} + +/** + * The user-facing disclosure of the browser-side transform, or `null` when the + * chart displays every fetched category and summed no duplicate cell. Describes + * only what the chart did — never whether the SQL was pre-aggregated. Truncation + * and duplicate summing are independent facts, joined with `; ` when both hold. + */ +export function chartDataNote(meta: ChartDataMeta): string | null { + const clauses: string[] = []; + if (meta.categoriesTruncated) { + clauses.push('first ' + formatRows(meta.shownCategories) + ' of ' + formatRows(meta.totalCategories) + ' categories'); + } + if (meta.duplicateCellsSummed) { + clauses.push(meta.groupKey === 'x+series' + ? 'duplicate X/series rows summed in the browser' + : 'duplicate X rows summed in the browser'); + } + return clauses.length ? clauses.join('; ') : null; } const withAlpha = (hex: string, frac: number): string => { @@ -609,6 +688,11 @@ export interface ChartJsConfigOptions { fieldConfig?: FieldConfig; hideGrid?: boolean; measures?: ChartMeasure[]; + /** A pre-aggregated `buildChartData` result — the renderer computes it once + * (it needs `.meta` for the disclosure note) and threads it here so a single + * render aggregates the fetched rows exactly once. Omitted by direct/test + * callers, which fall back to aggregating inline. */ + data?: ChartDataResult; } /** One Chart.js dataset — the fields this module sets, plus whatever @@ -705,7 +789,9 @@ export function chartJsConfig( // same array to buildChartData so a single config build resolves the metadata // once instead of three times (#254 review finding). const measures = opts.measures ?? visibleChartMeasures(columns, cfg, fieldConfig); - const { labels, datasets } = buildChartData(columns, rows, cfg, fieldConfig, measures); + // `opts.data` (when the renderer already aggregated for its note) avoids a + // second full aggregation per render; direct/test callers fall back inline. + const { labels, datasets } = opts.data ?? buildChartData(columns, rows, cfg, fieldConfig, measures); const pal = colors.palette; const horizontal = cfg.type === 'hbar'; const isPie = cfg.type === 'pie'; diff --git a/src/styles.css b/src/styles.css index fc697b0..6cefd52 100644 --- a/src/styles.css +++ b/src/styles.css @@ -1063,6 +1063,12 @@ body.detached-tab .graph-overlay-panel { margin-left: auto; font-family: var(--mono); font-size: 10.5px; color: var(--fg-faint); white-space: nowrap; } +/* On surfaces with no config bar to host it inline, the note is a standalone + block above the canvas — left-aligned, padded to match the canvas gutter. */ +.chart-cap-note--standalone { + margin-left: 0; flex-shrink: 0; + padding: 6px 14px 0; white-space: normal; +} /* The canvas fills the remaining space; Chart.js is responsive within it. The min-height floor keeps it visible (and lets .chart-view scroll) when a tall wrapped config bar would otherwise leave it 0px. */ diff --git a/src/ui/chart-render.ts b/src/ui/chart-render.ts index cb0431a..bbd8f97 100644 --- a/src/ui/chart-render.ts +++ b/src/ui/chart-render.ts @@ -8,10 +8,10 @@ import { h } from './dom.js'; import { Icon } from './icons.js'; -import { formatRows } from '../core/format.js'; import { - chartFieldOptions, chartColors, chartJsConfig, chartCfgValid, normalizeChartCfg, chartRowCap, + chartFieldOptions, chartColors, chartJsConfig, chartCfgValid, normalizeChartCfg, chartStylePresets, chartStylePreset, applyChartStylePreset, normalizeChartStyle, visibleChartMeasures, + buildChartData, chartDataNote, } from '../core/chart-data.js'; import type { ChartConfig, ChartFamilyType, ChartFieldOption, ChartStyle } from '../core/chart-data.js'; import type { Column } from '../core/panel-cfg.js'; @@ -123,6 +123,21 @@ export function renderChart( const cfg = chartCfgValid(opts.cfg, r.columns) ? normalizeChartCfg(opts.cfg as ChartConfig) : null; if (!cfg) return chartEmpty(Icon.chart(), 'These results aren’t chartable — add a numeric column to plot them.'); + // Resolve the visible measures and aggregate the chart data ONCE per render: + // the aggregation's `.meta` drives the disclosure note (below) and the same + // result is threaded into `chartJsConfig` (via `data`), so a render never + // aggregates the fetched rows twice. When every selected field is hidden + // there's nothing to aggregate — the empty-state guard handles it below. + const measures = visibleChartMeasures(r.columns, cfg, opts.fieldConfig); + const chartData = measures.length + ? buildChartData(r.columns, r.rows, cfg, opts.fieldConfig || {}, measures) + : null; + // The disclosure note describes only the browser-side transform (category + // truncation and/or duplicate-cell summing) — never whether the SQL + // pre-aggregated. It must render on every surface, not only where the + // interactive controls do (Dashboard / read-only tiles pass controls:false). + const note = chartData ? chartDataNote(chartData.meta) : null; + // `opts.controls === false` omits the interactive Type/X/Y config bar entirely // (the read-only dashboard tile — #149): the chart draws, but no field controls // are built, rather than building them and hiding them with CSS. @@ -174,22 +189,16 @@ export function renderChart( changed(cfg); })); } - // The chart plots at most cap points for the current type; say so when the - // result is bigger (the table still shows everything) — no silent - // truncation. Recomputed on every rerender (the Type select's onChange), - // so switching type re-slices and updates the note in lockstep. - const cap = chartRowCap(cfg.type); - if (r.rows.length > cap) { - bar.appendChild(h('span', { class: 'chart-cap-note' }, - 'first ' + cap + ' of ' + formatRows(r.rows.length) + ' rows')); - } + // Disclose the browser-side transform inline in the config row when there + // is one (category truncation and/or duplicate-cell summing). Recomputed on + // every rerender (e.g. the Type select's onChange changes the category cap), + // so switching type updates the note in lockstep. + if (note) bar.appendChild(h('span', { class: 'chart-cap-note' }, note)); } - // Resolve the visible measures once and reuse them for the chart config - // below (threaded through `chartJsConfig`), rather than re-resolving field - // metadata for the empty-state guard and again inside the renderer. - const measures = visibleChartMeasures(r.columns, cfg, opts.fieldConfig); - if (measures.length === 0) { + // `chartData` is null iff every selected field is hidden (no measures to + // aggregate) — the empty state, with no chart and no note. + if (!chartData) { return h('div', { class: 'chart-view' }, bar, chartEmpty(Icon.chart(), 'All selected chart fields are hidden by panel.fieldConfig.')); } @@ -205,6 +214,7 @@ export function renderChart( fieldConfig: opts.fieldConfig, hideGrid: opts.hideGrid, measures, + data: chartData, // aggregated once above — don't re-aggregate for the config })); setChart(chart); // Chart.js's own responsive sizing reads layout through APIs (getComputedStyle, @@ -226,6 +236,12 @@ export function renderChart( }); const compactFrame = cfg.type === 'pie' && normalizeChartStyle(cfg.style, cfg.type).frame === 'compact'; - return h('div', { class: 'chart-view' }, bar, + // Surfaces without the config bar (Dashboard / read-only / detached tiles, + // controls:false) still disclose the transform — as a small standalone block + // above the canvas, since there's no config row to host it inline. + const standaloneNote = opts.controls === false && note + ? h('div', { class: 'chart-cap-note chart-cap-note--standalone' }, note) + : null; + return h('div', { class: 'chart-view' }, bar, standaloneNote, h('div', { class: 'chart-canvas-wrap' + (compactFrame ? ' is-compact' : '') }, canvas)); } diff --git a/tests/unit/chart-data.test.ts b/tests/unit/chart-data.test.ts index e486437..c4e3ba5 100644 --- a/tests/unit/chart-data.test.ts +++ b/tests/unit/chart-data.test.ts @@ -5,9 +5,9 @@ import { cloneChartCfg, chartCfgValid, normalizeChartCfg, chartRowCap, normalizeChartStyle, chartStylePresets, chartStylePreset, applyChartStylePreset, shouldShowChartPoints, formatChartValue, visibleChartMeasures, - CHART_STYLE_PRESETS, + CHART_STYLE_PRESETS, chartDataNote, } from '../../src/core/chart-data.js'; -import type { ChartConfig, ChartFamilyType, ChartMeasure, ChartStyle } from '../../src/core/chart-data.js'; +import type { ChartConfig, ChartFamilyType, ChartMeasure, ChartStyle, ChartDataMeta } from '../../src/core/chart-data.js'; import type { Column } from '../../src/core/panel-cfg.js'; // A chart cfg literal, typed loosely enough for this suite's fixtures (many @@ -397,11 +397,134 @@ describe('buildChartData', () => { const cfg = cc({ type: 'line', x: 0, y: [1], series: 3 }); const fields = { columns: { flights: { displayName: 'Flight count' } } }; expect(buildChartData(cols, [['B6', 10, 2, 'East']], cfg, fields).datasets[0].label).toBe('East'); + // A hidden measure leaves the series path with no plottable Y — labels and + // datasets are empty, but the metadata still reflects the discovered category. + const emptyMeta: ChartDataMeta = { + totalRows: 1, totalCategories: 1, shownCategories: 1, + categoriesTruncated: false, duplicateCellsSummed: false, groupKey: 'x+series', + }; expect(buildChartData(cols, [['B6', 10, 2, 'East']], cfg, { columns: { flights: { hidden: true } } })) - .toEqual({ labels: [], datasets: [] }); + .toEqual({ labels: [], datasets: [], meta: emptyMeta }); expect(buildChartData(cols, [['B6', 10, 2, 'East']], cc({ type: 'line', x: 0, y: [1, 2], series: 3, - }), { columns: { flights: { hidden: true } } })).toEqual({ labels: [], datasets: [] }); + }), { columns: { flights: { hidden: true } } })).toEqual({ labels: [], datasets: [], meta: emptyMeta }); + }); + + // --- #111: the cap limits X categories (not raw rows), and typed metadata --- + const capCols = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + it('a row after the old raw-row boundary still contributes to its displayed category', () => { + // Pie's cap is 30. Fill 30 distinct categories with one row each, then place + // a SECOND row for the first category ('k0') far past the 30-row boundary. + const rows: unknown[][] = Array.from({ length: 30 }, (_, i) => ['k' + i, '1']); + for (let i = 0; i < 40; i++) rows.push(['filler' + i, '1']); // never displayed (cap already hit) + rows.push(['k0', '100']); // late row for an early, displayed category + const out = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(out.labels[0]).toBe('k0'); + expect(out.datasets[0].data[0]).toBe(101); // 1 + 100, not truncated away + expect(out.meta.totalCategories).toBe(70); // 30 real + 40 filler + expect(out.meta.shownCategories).toBe(30); + }); + it('retains the first `cap` X categories in first-seen order', () => { + const rows: unknown[][] = Array.from({ length: 35 }, (_, i) => ['k' + i, '1']); + const out = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(out.labels).toEqual(Array.from({ length: 30 }, (_, i) => 'k' + i)); // first-seen, first 30 + }); + it('omits post-cap categories from labels and datasets', () => { + const rows: unknown[][] = Array.from({ length: 32 }, (_, i) => ['k' + i, '1']); + const out = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(out.labels).not.toContain('k30'); + expect(out.labels).not.toContain('k31'); + expect(out.datasets[0].data).toHaveLength(30); + }); + it('metadata: truncation only (unique cells, more categories than the cap)', () => { + const rows: unknown[][] = Array.from({ length: 35 }, (_, i) => ['k' + i, '1']); + const { meta } = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(meta).toEqual({ + totalRows: 35, totalCategories: 35, shownCategories: 30, + categoriesTruncated: true, duplicateCellsSummed: false, groupKey: 'x', + }); + }); + it('metadata: duplicate only (a repeated displayed X, category count at/below the cap)', () => { + const rows: unknown[][] = [['a', '1'], ['b', '2'], ['a', '3']]; + const { meta } = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(meta.categoriesTruncated).toBe(false); + expect(meta.duplicateCellsSummed).toBe(true); + expect(meta.totalCategories).toBe(2); + }); + it('metadata: both truncation and a duplicate displayed cell', () => { + const rows: unknown[][] = Array.from({ length: 35 }, (_, i) => ['k' + i, '1']); + rows.push(['k0', '9']); // duplicate of a displayed category + const { meta } = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(meta.categoriesTruncated).toBe(true); + expect(meta.duplicateCellsSummed).toBe(true); + }); + it('unique multi-series cells (repeated X, distinct Series) are not summing', () => { + const rows = [ + ['Jan', 'EU', '1'], ['Jan', 'US', '2'], + ['Feb', 'EU', '3'], ['Feb', 'US', '4'], + ]; + const c = [{ name: 'month', type: 'String' }, { name: 'region', type: 'String' }, { name: 'rev', type: 'UInt64' }]; + const { meta } = buildChartData(c, rows, { type: 'bar', x: 0, y: [2], series: 1 }); + expect(meta.groupKey).toBe('x+series'); + expect(meta.duplicateCellsSummed).toBe(false); + }); + it('a repeated (X, Series) cell is summed and flagged', () => { + const rows = [['Jan', 'EU', '1'], ['Jan', 'EU', '5']]; // same (month, region) + const c = [{ name: 'month', type: 'String' }, { name: 'region', type: 'String' }, { name: 'rev', type: 'UInt64' }]; + const out = buildChartData(c, rows, { type: 'bar', x: 0, y: [2], series: 1 }); + expect(out.datasets).toEqual([{ label: 'EU', data: [6] }]); // 1 + 5 + expect(out.meta.duplicateCellsSummed).toBe(true); + }); + it('a duplicate confined to an omitted category does not flag the visible chart', () => { + // 30 unique displayed categories, then a duplicated pair in a 31st (omitted) one. + const rows: unknown[][] = Array.from({ length: 30 }, (_, i) => ['k' + i, '1']); + rows.push(['zz', '1'], ['zz', '2']); // 'zz' is category #31 → omitted by pie's cap + const { meta } = buildChartData(capCols, rows, { type: 'pie', x: 0, y: [1], series: null }); + expect(meta.categoriesTruncated).toBe(true); + expect(meta.duplicateCellsSummed).toBe(false); // the duplicate is never displayed + }); + it('a Series present only in omitted categories produces no all-null dataset', () => { + // 'US' appears only in the omitted category 'z'; it must not become a dataset. + const rows: unknown[][] = []; + for (let i = 0; i < 30; i++) rows.push(['k' + i, 'EU', '1']); + rows.push(['z', 'US', '9']); // category #31 → omitted + const c = [{ name: 'k', type: 'String' }, { name: 'region', type: 'String' }, { name: 'v', type: 'UInt64' }]; + const out = buildChartData(c, rows, { type: 'pie', x: 0, y: [2], series: 1 }); + expect(out.datasets.map((d) => d.label)).toEqual(['EU']); // no 'US' dataset + expect(out.datasets.every((d) => d.data.some((v) => v != null))).toBe(true); + }); +}); + +describe('chartDataNote', () => { + const base: ChartDataMeta = { + totalRows: 10, totalCategories: 5, shownCategories: 5, + categoriesTruncated: false, duplicateCellsSummed: false, groupKey: 'x', + }; + it('returns null when neither condition holds', () => { + expect(chartDataNote(base)).toBeNull(); + }); + it('truncation only → "first N of M categories"', () => { + expect(chartDataNote({ ...base, shownCategories: 30, totalCategories: 600, categoriesTruncated: true })) + .toBe('first 30 of 600 categories'); + }); + it('duplicate X only → browser-summed X note', () => { + expect(chartDataNote({ ...base, duplicateCellsSummed: true })) + .toBe('duplicate X rows summed in the browser'); + }); + it('duplicate X/series only → browser-summed X/series note', () => { + expect(chartDataNote({ ...base, groupKey: 'x+series', duplicateCellsSummed: true })) + .toBe('duplicate X/series rows summed in the browser'); + }); + it('both conditions → two clauses joined with "; "', () => { + expect(chartDataNote({ + ...base, shownCategories: 30, totalCategories: 600, categoriesTruncated: true, duplicateCellsSummed: true, + })).toBe('first 30 of 600 categories; duplicate X rows summed in the browser'); + }); + it('both conditions with Series → the X/series clause', () => { + expect(chartDataNote({ + ...base, groupKey: 'x+series', shownCategories: 30, totalCategories: 600, + categoriesTruncated: true, duplicateCellsSummed: true, + })).toBe('first 30 of 600 categories; duplicate X/series rows summed in the browser'); }); }); @@ -432,6 +555,21 @@ describe('chartJsConfig', () => { expect(cfg.options.scales!.y.grid.display).toBe(false); // category axis expect(cfg.data.datasets[0].backgroundColor).toBe(colors.palette[0]); }); + it('uses a precomputed opts.data verbatim instead of re-aggregating (#111 single-pass)', () => { + // A sentinel data result unrelated to `rows`: chartJsConfig must draw it as-is, + // proving the renderer's one aggregation is reused, not recomputed here. + const data = { + labels: ['only'], datasets: [{ label: 'precomputed', data: [42] }], + meta: { + totalRows: 2, totalCategories: 1, shownCategories: 1, + categoriesTruncated: false, duplicateCellsSummed: false, groupKey: 'x' as const, + }, + }; + const cfg = chartJsConfig(cols, rows, { type: 'bar', x: 0, y: [1], series: null }, colors, { data }); + expect(cfg.data.labels).toEqual(['only']); + expect(cfg.data.datasets[0].label).toBe('precomputed'); + expect(cfg.data.datasets[0].data).toEqual([42]); + }); it('shows value-axis gridlines by default, hides them with opts.hideGrid (#149)', () => { const shown = chartJsConfig(cols, rows, { type: 'bar', x: 0, y: [1], series: null }, colors); expect(shown.options.scales!.y.grid.display).toBe(true); diff --git a/tests/unit/chart-render.test.ts b/tests/unit/chart-render.test.ts index d2e1a26..e69421d 100644 --- a/tests/unit/chart-render.test.ts +++ b/tests/unit/chart-render.test.ts @@ -301,7 +301,7 @@ describe('renderChart', () => { change(fieldSel(app.dom.resultsRegion!, 'Series'), ''); expect(app.activeTab().panelCfg!.series).toBeNull(); }); - it('notes the row cap when the result is larger than the chart shows', () => { + it('notes the category cap when more categories exist than the chart shows', () => { const r = newResult('Table'); r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; r.rows = Array.from({ length: 600 }, (_, i) => ['k' + i, String(i)]); @@ -310,33 +310,95 @@ describe('renderChart', () => { paintChart(app); const note = app.dom.resultsRegion!.querySelector('.chart-cap-note'); expect(note).not.toBeNull(); - expect(note!.textContent).toContain('first 500 of'); + expect(note!.textContent).toBe('first 500 of 600 categories'); // a small result shows no cap note const small = appWithResult(tableResult(), { resultView: 'chart' }); paintChart(small); expect(small.dom.resultsRegion!.querySelector('.chart-cap-note')).toBeNull(); }); - it('switching chart type re-slices to the new type\'s cap and updates the note', () => { + it('switching chart type recalculates the category cap and updates the note', () => { const r = newResult('Table'); r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; r.rows = Array.from({ length: 600 }, (_, i) => ['k' + i, String(i)]); r.progress = { rows: 600, bytes: 100, elapsed_ns: 5e6 }; const app = appWithResult(r, { resultView: 'chart' }); paintChart(app); - // default (hbar, autoChart's categorical pick) cap is 500 < 600 rows + // default (hbar, autoChart's categorical pick) cap is 500 < 600 categories expect(app.activeTab().panelCfg!.type).toBe('hbar'); expect(app.dom.resultsRegion!.querySelector('.chart-cap-note')!.textContent) - .toBe('first ' + chartRowCap('hbar') + ' of 600 rows'); + .toBe('first ' + chartRowCap('hbar') + ' of 600 categories'); expect(app.chart!.config.data.labels).toHaveLength(chartRowCap('hbar')); - // switch to pie: a much tighter legibility cap — re-slices and the note shrinks with it + // switch to pie: a much tighter legibility cap — re-caps and the note shrinks with it change(fieldSel(app.dom.resultsRegion!, 'Type'), 'pie'); - expect(app.dom.resultsRegion!.querySelector('.chart-cap-note')!.textContent).toContain('first ' + chartRowCap('pie') + ' of'); + expect(app.dom.resultsRegion!.querySelector('.chart-cap-note')!.textContent) + .toBe('first ' + chartRowCap('pie') + ' of 600 categories'); expect(app.chart!.config.data.labels).toHaveLength(chartRowCap('pie')); - // switch to line: its cap (5000) exceeds the row count — no truncation, no note at all + // switch to line: its cap (5000) exceeds the category count — no truncation, no note at all change(fieldSel(app.dom.resultsRegion!, 'Type'), 'line'); expect(app.dom.resultsRegion!.querySelector('.chart-cap-note')).toBeNull(); expect(app.chart!.config.data.labels).toHaveLength(600); }); + it('shows the duplicate-X note when a displayed X repeats and nothing is truncated', () => { + const r = newResult('Table'); + r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + r.rows = [['a', '1'], ['a', '2'], ['b', '3']]; // 'a' repeats → summed in the browser + const app = appWithResult(r, { resultView: 'chart' }); + paintChart(app); + const note = app.dom.resultsRegion!.querySelector('.chart-cap-note'); + expect(note!.textContent).toBe('duplicate X rows summed in the browser'); + }); + it('shows the duplicate-X/series note for a repeated (X, series) cell', () => { + const r = newResult('Table'); + r.columns = [{ name: 'carrier', type: 'String' }, { name: 'region', type: 'String' }, { name: 'flights', type: 'UInt64' }]; + r.rows = [['B6', 'E', '10'], ['B6', 'E', '30']]; // same (carrier, region) → summed + const app = appWithResult(r); + const el = renderChart(app, r, { cfg: { type: 'bar', x: 0, y: [2], series: 1 } as ChartConfig, rerender: vi.fn() }); + expect(el.querySelector('.chart-cap-note')!.textContent).toBe('duplicate X/series rows summed in the browser'); + }); + it('combines truncation and duplicate clauses when both hold', () => { + const r = newResult('Table'); + r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + r.rows = Array.from({ length: 30 }, (_, i) => ['k' + i, '1']); + r.rows.push(['k0', '9']); // duplicate of a displayed category + r.rows.push(['k30', '1']); // a 31st category → pie's cap of 30 truncates it + const app = appWithResult(r); + const el = renderChart(app, r, { cfg: { type: 'pie', x: 0, y: [1], series: null } as ChartConfig, rerender: vi.fn() }); + expect(el.querySelector('.chart-cap-note')!.textContent) + .toBe('first 30 of 31 categories; duplicate X rows summed in the browser'); + }); + it('renders the note as a standalone block when the config bar is hidden (controls:false)', () => { + const r = newResult('Table'); + r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + r.rows = Array.from({ length: 32 }, (_, i) => ['k' + i, '1']); // 32 categories > pie cap 30 + const app = appWithResult(r); + const el = renderChart(app, r, { cfg: { type: 'pie', x: 0, y: [1], series: null } as ChartConfig, controls: false }); + expect(el.querySelector('.chart-config')).toBeNull(); // no config bar + const note = el.querySelector('.chart-cap-note.chart-cap-note--standalone'); + expect(note).not.toBeNull(); + expect(note!.textContent).toBe('first 30 of 32 categories'); + // and it sits above the canvas + expect(note!.nextElementSibling!.classList.contains('chart-canvas-wrap')).toBe(true); + }); + it('renders no note on a controls:false surface with no truncation or duplicate', () => { + const app = appWithResult(chartResult()); + const r = app.activeTab().result as StreamResult; + const el = renderChart(app, r, { cfg: { type: 'bar', x: 0, y: [2], series: null } as ChartConfig, controls: false }); + expect(el.querySelector('.chart-cap-note')).toBeNull(); + }); + it('aggregates the chart data once per render (note + Chart.js share one pass)', async () => { + const chartData = await import('../../src/core/chart-data.js'); + const spy = vi.spyOn(chartData, 'buildChartData'); + try { + const r = newResult('Table'); + r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + r.rows = Array.from({ length: 40 }, (_, i) => ['k' + i, '1']); // truncates → forces a note + const app = appWithResult(r); + renderChart(app, r, { cfg: { type: 'pie', x: 0, y: [1], series: null } as ChartConfig, rerender: vi.fn() }); + expect(spy).toHaveBeenCalledTimes(1); + } finally { + spy.mockRestore(); + } + }); it('destroys the previous Chart instance on re-render, and re-derives config on a new schema', () => { const app = appWithResult(chartResult(), { resultView: 'chart' }); paintChart(app); diff --git a/tests/unit/panels.test.ts b/tests/unit/panels.test.ts index b5fedff..9933277 100644 --- a/tests/unit/panels.test.ts +++ b/tests/unit/panels.test.ts @@ -676,6 +676,20 @@ describe('renderResolvedPanel', () => { const out = PANEL_TYPES.logs.renderPanel({ app: app, result: r, cfg: { type: 'logs' }, cap: 10 }); expect(out.node.textContent).toContain('No time + message columns'); }); + it('a Dashboard chart panel discloses the category cap despite hidden controls (#111)', () => { + const app = makeApp(); + const r = newResult('Table'); + r.columns = [{ name: 'k', type: 'String' }, { name: 'v', type: 'UInt64' }]; + r.rows = Array.from({ length: 40 }, (_, i) => ['k' + i, '1']); // 40 categories > pie cap 30 + const out = PANEL_TYPES.pie.renderPanel({ + app, result: r, cfg: { type: 'pie', x: 0, y: [1], series: null }, + surface: 'dashboard', rerender: () => {}, readonly: true, + }); + expect(qs(out.node, '.chart-config')).toBeNull(); // dashboard hides the config bar + const note = qs(out.node, '.chart-cap-note'); + expect(note).not.toBeNull(); + expect(note.textContent).toBe('first 30 of 40 categories'); // still disclosed + }); it("the chart arm's destroy tears down its instance exactly once", () => { const app = makeApp(); const r = chartResult();