fix: chart review HIGH + MEDIUM bugs (bar, line, table)

app/src/components/table-renderer.tsx

@@ -11,6 +11,7 @@
   resolveThresholdColor,
   resolveStylingRuleColor,
   interpolateColor,
+  contrastTextColor,
 } from "@neoboard/components";
 import type { StylingRule, ColorScaleConfig } from "@neoboard/components";
 import type { ColumnDef } from "@tanstack/react-table";
@@ -25,20 +26,6 @@
   max: "max",
 };
 
-/** Return black or white text based on background luminance for readability. */
-function contrastTextColor(hex: string): string {
-  const c = hex.replace("#", "");
-  const r = parseInt(c.substring(0, 2), 16) / 255;
-  const g = parseInt(c.substring(2, 4), 16) / 255;
-  const b = parseInt(c.substring(4, 6), 16) / 255;
-  // Relative luminance (WCAG formula)
-  const lum =
-    0.2126 * (r <= 0.03928 ? r / 12.92 : ((r + 0.055) / 1.055) ** 2.4) +
-    0.7152 * (g <= 0.03928 ? g / 12.92 : ((g + 0.055) / 1.055) ** 2.4) +
-    0.0722 * (b <= 0.03928 ? b / 12.92 : ((b + 0.055) / 1.055) ** 2.4);
-  return lum > 0.179 ? "#000000" : "#ffffff";
-}
-
 export interface TableRendererProps {
   data: unknown;
   settings?: Record<string, unknown>;
@@ -299,35 +286,57 @@
     return <EmptyState title={emptyMessage} className="py-6" />;
   }
 
+  // Avoid the pagination "flash" on first render: when pagination is enabled
+  // we depend on the measured container height to compute the page size. If
+  // we render before the ResizeObserver fires, DataGrid mounts with the
+  // default `pageSize=10`, then immediately re-renders with the dynamic size —
+  // which visibly snaps the row count. Render an empty wrapper on the first
+  // tick instead so the observer can measure, then commit a single DataGrid.
+  const awaitingHeight = enablePagination && containerHeight === undefined;
+
+  // Derive a screen-reader description from data shape — the underlying
+  // <table> has no top-level label and the wrapper is otherwise just a
+  // scroll container, so AT users hit it with no context.
+  const columnCount = records.length ? Object.keys(records[0]).length : 0;
+  const ariaLabel =
+    (settings.ariaLabel as string | undefined) ??
+    `Table with ${records.length} rows and ${columnCount} columns`;
+
   return (
-    <div ref={containerRef} className="h-full overflow-y-auto">
-      <DataGrid
-        key={enableGrouping ? `grp-${aggregationFn}` : undefined}
-        columns={columns}
-        data={records as Record<string, unknown>[]}
-        enableSorting={enableSorting}
-        enableColumnResizing={enableColumnResizing}
-        enableSelection={settings.enableSelection as boolean | undefined}
-        enableGlobalFilter={settings.enableGlobalFilter !== false}
-        enableColumnFilters={settings.enableColumnFilters !== false}
-        enablePagination={enablePagination}
-        pageSize={(settings.pageSize as number) ?? 10}
-        containerHeight={enablePagination ? containerHeight : undefined}
-        onCellClick={onCellClick}
-        clickableColumns={clickableColumns}
-        getRowStyle={getRowStyle}
-        getCellStyle={getCellStyle}
-        enableGrouping={enableGrouping}
-        initialGrouping={initialGrouping}
-        pagination={(table) => (
-          <div className="flex items-center gap-2">
-            <DataGridViewOptions table={table} />
-            <div className="flex-1">
-              <DataGridPagination table={table} />
+    <div
+      ref={containerRef}
+      className="h-full overflow-y-auto"
+      role="region"
+      aria-label={ariaLabel}
+    >
+      {awaitingHeight ? null : (
+        <DataGrid
+          key={enableGrouping ? `grp-${aggregationFn}` : undefined}
+          columns={columns}
+          data={records as Record<string, unknown>[]}
+          enableSorting={enableSorting}
+          enableColumnResizing={enableColumnResizing}
+          enableSelection={settings.enableSelection as boolean | undefined}
+          enableColumnFilters={settings.enableColumnFilters !== false}
+          enablePagination={enablePagination}
+          pageSize={(settings.pageSize as number) ?? 10}
+          containerHeight={enablePagination ? containerHeight : undefined}
+          onCellClick={onCellClick}
+          clickableColumns={clickableColumns}
+          getRowStyle={getRowStyle}
+          getCellStyle={getCellStyle}
+          enableGrouping={enableGrouping}
+          initialGrouping={initialGrouping}
+          pagination={(table) => (
+            <div className="flex items-center gap-2">
+              <DataGridViewOptions table={table} />
+              <div className="flex-1">
+                <DataGridPagination table={table} />
+              </div>
             </div>
-          </div>
-        )}
-      />
+          )}
+        />
+      )}
     </div>
   );
 }

app/src/plugins/bar/transform.ts

@@ -7,21 +7,27 @@ import {
   resolveLabelKey,
   resolveValueKeys,
   normalizeValue,
+  collectAllKeys,
+  toSeriesNumber,
   type ColumnMapping,
 } from "../transforms/shared-utils";
 
 /**
  * Transform to bar chart format: [{ label, series1, series2 }]
  * When mapping is provided, uses mapped columns; otherwise uses positional defaults.
  * Always applies normalizeValue to labels for consistent type handling.
+ *
+ * Series keys are collected from the *union* of all rows so sparse data
+ * (where some series only appear in later rows) isn't silently dropped.
+ * Cell values use `toSeriesNumber` to preserve missing-vs-zero distinction.
  */
 export function transformToBarData(
   data: unknown,
   mapping?: ColumnMapping,
 ): unknown {
   const records = toRecords(data);
   if (!records.length) return [];
-  const keys = Object.keys(records[0]);
+  const keys = collectAllKeys(records);
   if (keys.length < 2) return [];
 
   const labelKey = resolveLabelKey(keys, mapping);
@@ -32,7 +38,7 @@ export function transformToBarData(
       label: String(normalizeValue(r[labelKey]) ?? ""),
     };
     for (const k of valueKeys) {
-      point[k] = Number(r[k]) || 0;
+      point[k] = toSeriesNumber(r[k]);
     }
     return point;
   });
@@ -45,7 +51,7 @@ export function transformToBarData(
 export function validateBarData(data: unknown): string | null {
   const records = toRecords(data);
   if (!records.length) return null;
-  const cols = Object.keys(records[0]).length;
+  const cols = collectAllKeys(records).length;
   if (cols < 2)
     return `Bar chart requires at least 2 columns: first column for category labels (x-axis) and one or more columns for numeric values (y-axis). Your query returned only ${cols} column(s). Example: \`SELECT category, count FROM ...\``;
   return null;

app/src/plugins/line/transform.ts

@@ -7,21 +7,28 @@ import {
   resolveLabelKey,
   resolveValueKeys,
   normalizeValue,
+  collectAllKeys,
+  toSeriesNumber,
   type ColumnMapping,
 } from "../transforms/shared-utils";
 
 /**
  * Transform to line chart format: [{ x, series1, series2 }]
  * When mapping is provided, uses mapped columns; otherwise uses positional defaults.
  * Always applies normalizeValue to x-axis values for consistent type handling.
+ *
+ * Series keys are collected from the *union* of all rows so sparse data
+ * (where some series only appear in later rows) isn't silently dropped.
+ * Cell values use `toSeriesNumber` to preserve missing-vs-zero distinction;
+ * downstream `connectNulls` controls whether gaps are bridged.
  */
 export function transformToLineData(
   data: unknown,
   mapping?: ColumnMapping,
 ): unknown {
   const records = toRecords(data);
   if (!records.length) return [];
-  const keys = Object.keys(records[0]);
+  const keys = collectAllKeys(records);
   if (keys.length < 2) return [];
 
   const xKey = resolveLabelKey(keys, mapping);
@@ -30,7 +37,7 @@ export function transformToLineData(
   return records.map((r) => {
     const point: Record<string, unknown> = { x: normalizeValue(r[xKey]) };
     for (const k of seriesKeys) {
-      point[k] = Number(r[k]) || 0;
+      point[k] = toSeriesNumber(r[k]);
     }
     return point;
   });
@@ -43,7 +50,7 @@ export function transformToLineData(
 export function validateLineData(data: unknown): string | null {
   const records = toRecords(data);
   if (!records.length) return null;
-  const cols = Object.keys(records[0]).length;
+  const cols = collectAllKeys(records).length;
   if (cols < 2)
     return `Line chart requires at least 2 columns: first column for x-axis values (dates, numbers, or labels) and one or more columns for numeric series. Your query returned only ${cols} column(s). Example: \`SELECT date, revenue FROM ...\``;
   return null;

app/src/plugins/transforms/__tests__/bar.test.ts

@@ -37,12 +37,43 @@ describe("transformToBarData", () => {
     expect(result[0].s2).toBe(2);
   });
 
-  it("coerces non-numeric values to 0", () => {
+  it("maps non-numeric values to null (preserved as gap, not 0)", () => {
+    // Previously coerced to 0 — that hid bad data. Returning null lets
+    // ECharts render a gap and keeps the missing-vs-zero distinction.
     const data = [{ cat: "X", value: "not-a-number" }];
-    const result = transformToBarData(data) as Array<{ value: number }>;
+    const result = transformToBarData(data) as Array<{ value: number | null }>;
+    expect(result[0].value).toBeNull();
+  });
+
+  it("preserves numeric zero (regression: zero must not become null)", () => {
+    const data = [{ cat: "X", value: 0 }];
+    const result = transformToBarData(data) as Array<{ value: number | null }>;
     expect(result[0].value).toBe(0);
   });
 
+  it("maps null/undefined cells to null (missing data, not 0)", () => {
+    const data = [
+      { cat: "A", value: null },
+      { cat: "B", value: undefined },
+    ];
+    const result = transformToBarData(data) as Array<{ value: number | null }>;
+    expect(result[0].value).toBeNull();
+    expect(result[1].value).toBeNull();
+  });
+
+  it("unions series keys across rows so sparse series are not dropped", () => {
+    // s2 is absent from the first row — before the fix, transformToBarData
+    // would only emit { label, s1 } and silently lose the s2 series.
+    const data = [
+      { cat: "X", s1: 1 },
+      { cat: "Y", s1: 2, s2: 9 },
+    ];
+    const result = transformToBarData(data) as Array<Record<string, unknown>>;
+    expect(result[0].s1).toBe(1);
+    expect(result[0].s2).toBeNull();
+    expect(result[1].s2).toBe(9);
+  });
+
   it("respects column mapping", () => {
     const data = [{ a: 1, b: 2, c: 3 }];
     const result = transformToBarData(data, {

app/src/plugins/transforms/__tests__/line.test.ts

@@ -24,12 +24,43 @@ describe("transformToLineData", () => {
     expect(transformToLineData([{ x: 1 }])).toEqual([]);
   });
 
-  it("coerces non-numeric series values to 0", () => {
+  it("maps non-numeric series values to null (preserved as gap, not 0)", () => {
+    // Previously coerced to 0 — that hid bad data. Returning null lets
+    // ECharts render a gap and keeps the missing-vs-zero distinction.
     const data = [{ x: "Jan", y: "bad" }];
-    const result = transformToLineData(data) as Array<{ y: number }>;
+    const result = transformToLineData(data) as Array<{ y: number | null }>;
+    expect(result[0].y).toBeNull();
+  });
+
+  it("preserves numeric zero (regression: zero must not become null)", () => {
+    const data = [{ x: "Jan", y: 0 }];
+    const result = transformToLineData(data) as Array<{ y: number | null }>;
     expect(result[0].y).toBe(0);
   });
 
+  it("maps null/undefined cells to null (missing data, not 0)", () => {
+    const data = [
+      { x: "Jan", y: null },
+      { x: "Feb", y: undefined },
+    ];
+    const result = transformToLineData(data) as Array<{ y: number | null }>;
+    expect(result[0].y).toBeNull();
+    expect(result[1].y).toBeNull();
+  });
+
+  it("unions series keys across rows so sparse series are not dropped", () => {
+    // y2 only appears in the second row — before the fix, transformToLineData
+    // would only emit { x, y1 } and silently lose the y2 series.
+    const data = [
+      { x: "Jan", y1: 1 },
+      { x: "Feb", y1: 2, y2: 9 },
+    ];
+    const result = transformToLineData(data) as Array<Record<string, unknown>>;
+    expect(result[0].y1).toBe(1);
+    expect(result[0].y2).toBeNull();
+    expect(result[1].y2).toBe(9);
+  });
+
   it("converts Date objects in x-axis", () => {
     const data = [{ date: new Date("2024-06-01T00:00:00Z"), revenue: 100 }];
     const result = transformToLineData(data) as Array<{ x: unknown }>;

app/src/plugins/transforms/__tests__/shared.test.ts

@@ -1,5 +1,11 @@
 import { describe, it, expect } from "vitest";
-import { toRecords, resolveLabelKey, resolveValueKeys } from "../shared-utils";
+import {
+  toRecords,
+  resolveLabelKey,
+  resolveValueKeys,
+  collectAllKeys,
+  toSeriesNumber,
+} from "../shared-utils";
 
 describe("toRecords", () => {
   it("returns array data unchanged", () => {
@@ -62,3 +68,53 @@ describe("resolveValueKeys", () => {
     expect(resolveValueKeys(["a", "b"], "a", { yAxis: [] })).toEqual(["b"]);
   });
 });
+
+describe("collectAllKeys", () => {
+  it("returns the union of keys across all rows in first-seen order", () => {
+    expect(
+      collectAllKeys([
+        { a: 1, b: 2 },
+        { b: 3, c: 4 },
+        { a: 5, d: 6 },
+      ]),
+    ).toEqual(["a", "b", "c", "d"]);
+  });
+
+  it("returns an empty array for an empty record list", () => {
+    expect(collectAllKeys([])).toEqual([]);
+  });
+
+  it("does not duplicate keys that appear in multiple rows", () => {
+    expect(collectAllKeys([{ a: 1 }, { a: 2 }, { a: 3 }])).toEqual(["a"]);
+  });
+});
+
+describe("toSeriesNumber", () => {
+  it("preserves finite numbers including zero and negatives", () => {
+    expect(toSeriesNumber(0)).toBe(0);
+    expect(toSeriesNumber(42)).toBe(42);
+    expect(toSeriesNumber(-3.14)).toBe(-3.14);
+  });
+
+  it("parses numeric strings", () => {
+    expect(toSeriesNumber("10")).toBe(10);
+    expect(toSeriesNumber("0")).toBe(0);
+    expect(toSeriesNumber("-2.5")).toBe(-2.5);
+  });
+
+  it("returns null for null, undefined and empty string (missing data)", () => {
+    expect(toSeriesNumber(null)).toBeNull();
+    expect(toSeriesNumber(undefined)).toBeNull();
+    expect(toSeriesNumber("")).toBeNull();
+  });
+
+  it("returns null for non-numeric strings instead of silently giving 0", () => {
+    expect(toSeriesNumber("not-a-number")).toBeNull();
+    expect(toSeriesNumber("NaN")).toBeNull();
+  });
+
+  it("returns null for Infinity / NaN", () => {
+    expect(toSeriesNumber(Number.POSITIVE_INFINITY)).toBeNull();
+    expect(toSeriesNumber(Number.NaN)).toBeNull();
+  });
+});