refactor(charts): single chart-style spec + resolve field metadata once

CHANGELOG.md

@@ -9,6 +9,16 @@ auto-generated per-PR notes; this file is the curated, human-readable history.
 
 ## [Unreleased]
 
+### Changed
+- **Chart style internals consolidated (post-#258 review).** The per-chart-type
+  style surface (which fields each type owns, their accepted values, and their
+  defaults) now lives in one `CHART_STYLE_SPEC` table that both
+  `normalizeChartStyle` and `chartStylePreset` read, replacing the parallel
+  field lists that could drift. Chart rendering resolves visible-measure field
+  metadata once per repaint (threaded from `chart-render` through
+  `chartJsConfig` into `buildChartData`) instead of three times. No user-facing
+  behavior or schema change.
+
 ### Added
 - **Bar, Column, Area, and Pie now share type-specific presentation presets**
   (#258) through the same compact Style selector. Bar/Column add Grouped,

src/core/chart-data.js

@@ -146,46 +146,39 @@ const CHART_SCALES = new Set(['auto', 'zero', 'data']);
 const CHART_VISIBILITY = new Set(['auto', 'show', 'hide']);
 const CHART_AXES = new Set(['show', 'hide']);
 const CHART_FRAMES = new Set(['normal', 'compact']);
-const CHART_STYLE_FIELDS = {
-  hbar: { mode: CHART_BAR_MODES, density: CHART_BAR_DENSITIES, scale: CHART_SCALES, legend: CHART_VISIBILITY, grid: CHART_VISIBILITY, axes: CHART_AXES },
-  bar: { mode: CHART_BAR_MODES, density: CHART_BAR_DENSITIES, scale: CHART_SCALES, legend: CHART_VISIBILITY, grid: CHART_VISIBILITY, axes: CHART_AXES },
-  line: { curve: CHART_CURVES, points: CHART_POINTS, scale: CHART_SCALES, legend: CHART_VISIBILITY, grid: CHART_VISIBILITY, axes: CHART_AXES },
-  area: { curve: CHART_CURVES, points: CHART_POINTS, stack: CHART_STACKS, scale: CHART_SCALES, legend: CHART_VISIBILITY, grid: CHART_VISIBILITY, axes: CHART_AXES },
-  pie: { shape: CHART_PIE_SHAPES, legend: CHART_VISIBILITY, frame: CHART_FRAMES },
+
+// The single source of truth for a chart type's style surface: which fields it
+// owns, the set of accepted values, and the default — in the canonical field
+// order. `normalizeChartStyle` (fill defaults + drop unsupported values) and
+// `chartStylePreset` (flag an unsupported value as Custom) both read this one
+// table, so the accepted-value set and the field list can't drift between the
+// two the way parallel per-type branches would. The value Sets stay aligned
+// with the schema enums (`schemas/query-spec-v1.schema.json`) and the preset
+// tables above by the round-trip tests in tests/unit/chart-data.test.js.
+const CHART_STYLE_SPEC = {
+  hbar: { mode: [CHART_BAR_MODES, 'grouped'], density: [CHART_BAR_DENSITIES, 'normal'], scale: [CHART_SCALES, 'zero'], legend: [CHART_VISIBILITY, 'auto'], grid: [CHART_VISIBILITY, 'auto'], axes: [CHART_AXES, 'show'] },
+  bar: { mode: [CHART_BAR_MODES, 'grouped'], density: [CHART_BAR_DENSITIES, 'normal'], scale: [CHART_SCALES, 'zero'], legend: [CHART_VISIBILITY, 'auto'], grid: [CHART_VISIBILITY, 'auto'], axes: [CHART_AXES, 'show'] },
+  line: { curve: [CHART_CURVES, 'linear'], points: [CHART_POINTS, 'auto'], scale: [CHART_SCALES, 'data'], legend: [CHART_VISIBILITY, 'auto'], grid: [CHART_VISIBILITY, 'auto'], axes: [CHART_AXES, 'show'] },
+  area: { curve: [CHART_CURVES, 'linear'], points: [CHART_POINTS, 'auto'], stack: [CHART_STACKS, 'overlay'], scale: [CHART_SCALES, 'data'], legend: [CHART_VISIBILITY, 'auto'], grid: [CHART_VISIBILITY, 'auto'], axes: [CHART_AXES, 'show'] },
+  pie: { shape: [CHART_PIE_SHAPES, 'pie'], legend: [CHART_VISIBILITY, 'show'], frame: [CHART_FRAMES, 'normal'] },
 };
 
 /** Resolve renderer-independent, type-specific style without mutating imported data. */
 export function normalizeChartStyle(style, type = 'line') {
   const value = style && typeof style === 'object' && !Array.isArray(style) ? style : {};
-  if (type === 'hbar' || type === 'bar') return {
-    mode: CHART_BAR_MODES.has(value.mode) ? value.mode : 'grouped',
-    density: CHART_BAR_DENSITIES.has(value.density) ? value.density : 'normal',
-    scale: CHART_SCALES.has(value.scale) ? value.scale : 'zero',
-    legend: CHART_VISIBILITY.has(value.legend) ? value.legend : 'auto',
-    grid: CHART_VISIBILITY.has(value.grid) ? value.grid : 'auto',
-    axes: CHART_AXES.has(value.axes) ? value.axes : 'show',
-  };
-  if (type === 'pie') return {
-    shape: CHART_PIE_SHAPES.has(value.shape) ? value.shape : 'pie',
-    legend: CHART_VISIBILITY.has(value.legend) ? value.legend : 'show',
-    frame: CHART_FRAMES.has(value.frame) ? value.frame : 'normal',
-  };
-  return {
-    curve: CHART_CURVES.has(value.curve) ? value.curve : 'linear',
-    points: CHART_POINTS.has(value.points) ? value.points : 'auto',
-    ...(type === 'area' ? { stack: CHART_STACKS.has(value.stack) ? value.stack : 'overlay' } : {}),
-    scale: CHART_SCALES.has(value.scale) ? value.scale : 'data',
-    legend: CHART_VISIBILITY.has(value.legend) ? value.legend : 'auto',
-    grid: CHART_VISIBILITY.has(value.grid) ? value.grid : 'auto',
-    axes: CHART_AXES.has(value.axes) ? value.axes : 'show',
-  };
+  const spec = CHART_STYLE_SPEC[type] || CHART_STYLE_SPEC.line;
+  const out = {};
+  for (const [field, [supported, fallback]] of Object.entries(spec)) {
+    out[field] = supported.has(value[field]) ? value[field] : fallback;
+  }
+  return out;
 }
 
 /** Match every preset-owned field exactly; unusual advanced combinations stay Custom. */
 export function chartStylePreset(style, type) {
   const styleSource = style && typeof style === 'object' && !Array.isArray(style) ? style : {};
-  if (Object.entries(CHART_STYLE_FIELDS[type] || {})
-    .some(([field, supported]) => field in styleSource && !supported.has(styleSource[field]))) return 'custom';
+  if (Object.entries(CHART_STYLE_SPEC[type] || {})
+    .some(([field, [supported]]) => field in styleSource && !supported.has(styleSource[field]))) return 'custom';
   const normalizedStyle = normalizeChartStyle(style, type);
   const matched = chartStylePresets(type).find((preset) => (
     Object.keys(preset.style).every((field) => normalizedStyle[field] === preset.style[field])
@@ -396,10 +389,12 @@ export function visibleChartMeasures(columns, cfg, fieldConfig) {
  * - group-by (cfg.series set): one dataset per series value, aligned to the
  *   union of X categories, missing cell → null.
  * - otherwise: one dataset per visible measure in `cfg.y`.
+ * `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.
  */
-export function buildChartData(columns, rows, cfg, fieldConfig = {}) {
+export function buildChartData(columns, rows, cfg, fieldConfig = {}, measures = visibleChartMeasures(columns, cfg, fieldConfig)) {
   const slice = rows.slice(0, chartRowCap(cfg.type));
-  const measures = visibleChartMeasures(columns, cfg, fieldConfig);
   const num = (v) => {
     if (v == null || v === '') return null;
     const parsed = Number(v);
@@ -459,12 +454,18 @@ const withAlpha = (hex, frac) => {
  * `colors` is a resolved token bundle from `chartColors`. `opts.hideGrid`
  * supplies the surface default for `style.grid:'auto'` (dashboard tiles draw
  * on the panel background where a light gridline reads as noise — #149);
- * explicit `show`/`hide` style values override it.
+ * explicit `show`/`hide` style values override it. `opts.measures` (optional)
+ * is a pre-resolved `visibleChartMeasures` array — the caller passes it to
+ * avoid re-resolving field metadata it already computed.
  */
 export function chartJsConfig(columns, rows, cfg, colors, opts = {}) {
   const fieldConfig = opts.fieldConfig || {};
-  const measures = visibleChartMeasures(columns, cfg, fieldConfig);
-  const { labels, datasets } = buildChartData(columns, rows, cfg, fieldConfig);
+  // Resolve field metadata once per render: the caller (chart-render) may pass
+  // the measures it already computed for its empty-state guard, and we hand the
+  // 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);
   const pal = colors.palette;
   const horizontal = cfg.type === 'hbar';
   const isPie = cfg.type === 'pie';

src/ui/chart-render.js

@@ -116,7 +116,11 @@ export function renderChart(app, r, opts = {}) {
     }
   }
 
-  if (visibleChartMeasures(r.columns, cfg, opts.fieldConfig).length === 0) {
+  // 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) {
     return h('div', { class: 'chart-view' }, bar,
       chartEmpty(Icon.chart(), 'All selected chart fields are hidden by panel.fieldConfig.'));
   }
@@ -130,6 +134,7 @@ export function renderChart(app, r, opts = {}) {
   const chart = new app.Chart(canvas, chartJsConfig(r.columns, r.rows, cfg, chartColors(app.cssVar), {
     fieldConfig: opts.fieldConfig,
     hideGrid: opts.hideGrid,
+    measures,
   }));
   setChart(chart);
   // Chart.js's own responsive sizing reads layout through APIs (getComputedStyle,

tests/unit/chart-data.test.js

@@ -246,6 +246,11 @@ describe('chart style', () => {
   ])('accepts every supported presentation %s value', (field, values) => {
     for (const value of values) expect(normalizeChartStyle({ [field]: value }, 'line')[field]).toBe(value);
   });
+  it('falls back to the line surface for an unknown/missing chart type', () => {
+    const lineDefaults = { curve: 'linear', points: 'auto', scale: 'data', legend: 'auto', grid: 'auto', axes: 'show' };
+    expect(normalizeChartStyle({}, 'mystery')).toEqual(lineDefaults);
+    expect(normalizeChartStyle({})).toEqual(lineDefaults); // default type argument
+  });
   it('accepts Pie frame values and independently defaults invalid fields without mutation', () => {
     expect(normalizeChartStyle({ frame: 'compact' }, 'pie').frame).toBe('compact');
     const style = {
@@ -327,6 +332,15 @@ describe('buildChartData', () => {
       { label: 'W', data: [30, 20] }, // W has B6(30) and AA(20)
     ]);
   });
+  it('reuses a caller-supplied measures array verbatim (threaded from chartJsConfig)', () => {
+    // A pre-resolved measures array selects `delay` (index 2) with a custom
+    // label; buildChartData must plot it directly rather than re-resolving from
+    // cfg.y/fieldConfig — the single-resolution path chartJsConfig relies on.
+    const rows = [['B6', '10', '5.5', 'E']];
+    const measures = [{ index: 2, presentation: { displayName: 'Avg delay' }, authoredValueFormat: false }];
+    const out = buildChartData(cols, rows, { type: 'bar', x: 0, y: [1], series: null }, {}, measures);
+    expect(out.datasets).toEqual([{ label: 'Avg delay', data: [5.5] }]);
+  });
   it('caps at the row cap for the config type', () => {
     const bigCols = [{ name: 'c', type: 'String' }, { name: 'n', type: 'UInt64' }];
     const big = Array.from({ length: 600 }, (_, i) => ['c' + i, String(i)]);
@@ -445,6 +459,17 @@ describe('chartJsConfig', () => {
       datasetIndex: 0, dataset: cfg.data.datasets[0], raw: 1500, formattedValue: '1,500',
     })).toBe('flights: 1,500');
   });
+  it('reuses opts.measures instead of re-resolving field metadata', () => {
+    // chart-render resolves visibleChartMeasures once for its empty-state guard
+    // and threads it in; chartJsConfig must honor that array (label proves it is
+    // used, not recomputed from fieldConfig).
+    const measures = [{
+      index: 1, authoredValueFormat: false,
+      presentation: { displayName: 'Flights!', unit: '', decimals: null, description: null, hidden: false },
+    }];
+    const out = chartJsConfig(cols, rows, { type: 'bar', x: 0, y: [1], series: null }, colors, { measures });
+    expect(out.data.datasets[0].label).toBe('Flights!');
+  });
   it('line is not filled; area fills with an alpha-blended hex', () => {
     const line = chartJsConfig(cols, rows, { type: 'line', x: 0, y: [1], series: null }, colors);
     expect(line.type).toBe('line');