fix(charts): forward trendEnabled and thresholdZones, and pick the numeric KPI column (#1397)

CHANGELOG.md

@@ -16,6 +16,7 @@ Chart-experience release: chart authoring, editing, rule-based styling and click
 
 ### Fixed
 
+- Two options were advertised in the widget editor, implemented in the component library, and silently discarded in between: `trendEnabled` (single-value) and `thresholdZones` (gauge). Setting either did nothing, with no error, and the seeded reference dashboard shipped tiles demonstrating both — the `thresholdZones (3 bands)` tile was pixel-identical to `default`. The cause was not the Zod schema, which uses `.passthrough()` and preserved the keys intact; it was the plugin's explicit prop mapping, which never passed them to the chart. The worse half was the KPI: `transformToValueData` took the first column **positionally**, so following the editor's own "requires 2 rows" instruction on a `label, value` query rendered the date as the headline metric — `$2026-03`. It now picks the first numeric column and carries the previous row so a trend can be computed. A ratchet asserts that every option offered for a chart type is actually read by its plugin; it found the same bug in five more plugins, allowlisted and tracked in #1472 (#1397)
 - A long-format result — `category, series, value`, what `GROUP BY a, b` naturally produces — rendered a bar or line chart that looked plausible and was wrong. `resolveValueKeys` treats every non-label column as a value series, so the `series` column became a series of its own whose cells all coerced to null: 12 bars with duplicated x labels, a ghost legend swatch with no bars, and **no stacking at all** despite `stackMode` being set. Line was worse — a single revenue line drawn across duplicated x values, interleaving delivered -> shipped -> delivered, which reads as a violently spiky time series but is pure artifact. Nothing marked either as an error, and the seeded reference dashboard itself demonstrated two `stackMode` options with this exact query shape, which is how easy it is to fall into. Bar and line now reject a result whose plotted value columns contain no numeric values, naming the offending column and showing the pivot that fixes it. A column that is entirely null stays legal — that is a sparse series, what a LEFT JOIN produces. `validate` now receives the same column mapping as the transform, so a result whose text columns the user already mapped away is not wrongly rejected. The 11 affected reference tiles are rewritten to wide format, so the stacking options finally demonstrate stacking (#1400)
 - `numberFormat: "percent"` appended a `%` sign and scaled nothing, so a KPI computing `401 / 2000` rendered **`0.2%`** where the true share was **`20.05%`** — wrong by a factor of 100, rendered cleanly, with nothing in the UI to distinguish it from a real value. `0.2%` and `20.05%` are both believable share figures. The convention was never stated anywhere and the two option surfaces documented **opposite** contracts: the table's option was labelled `Percent (12%)`, asserting input was already 0–100, while the seeded reference tile was authored with ratio semantics — the clearest evidence the contract was not discoverable. `percent` now takes a **ratio** and scales it, via `Intl.NumberFormat`'s own `style: "percent"`, which also brings locale grouping (`12.5` → `1,250%`) that the hand-rolled branch never had. With no explicit decimal places it caps at 2 rather than Intl's default of 0, since rounding `20.05%` to `20%` is the same class of silent precision loss. **Breaking** for anyone who worked around the old behaviour by pre-multiplying by 100 in their query. Both option descriptions, both option labels, the single-value docs page and the demo tile now state the expected input range (#1396)
 - A client-side `groupBy` reported `count` as the number of **rows** in the group while `sum`, `avg`, `min` and `max` all skipped nulls, so the three contradicted each other: a group of `[100, 200, null]` returned `count: 3, sum: 300, avg: 150`, and `sum / count` came to 100. A reader saw "3 revenue values totalling 300" with no way to know only two rows had revenue at all — and because `count` is typically the denominator someone divides by, the error propagated into everything derived from it. Nulls in an aggregated column are not an edge case; they are what a `LEFT JOIN` produces. `count` now counts non-null values, matching SQL's `COUNT(col)` — the output key is `revenue_count`, named for the column, so the column form is the only one it could have meant. The existing test used sample data with no nulls and passed on both the wrong and the right behaviour; the new invariant test (`sum / count === avg` for every group) fails on the old code and cannot be silently re-broken (#1414)

app/src/lib/plugin/__tests__/chart-option-forwarding.test.ts

@@ -0,0 +1,149 @@
+import { describe, it, expect } from "vitest";
+import { readFileSync, existsSync, readdirSync } from "node:fs";
+import { join } from "node:path";
+
+/**
+ * Ratchet: every option offered in the widget editor must be read by the
+ * plugin that advertises it (#1397).
+ *
+ * Two options — `trendEnabled` (single-value) and `thresholdZones` (gauge) —
+ * shipped fully built at both ends and inert in between: present in the editor,
+ * implemented in the component library, and never forwarded by the plugin.
+ * Setting them did nothing, with no error, and the seeded reference dashboard
+ * shipped tiles demonstrating both. A control that lies is worse than a missing
+ * one.
+ *
+ * **Why source text and not the Zod schema.** #1397 proposed asserting that
+ * every option key exists in the plugin's `settingsSchema`. That would not have
+ * caught either bug: these schemas end in `.passthrough()`, so an unknown key
+ * survives the parse untouched — `gaugeSettingsSchema.parse({thresholdZones})`
+ * returns it intact. The value died at the plugin's explicit prop mapping,
+ * a seam the schema never sees. Conversely a key can be in the schema and still
+ * be dropped there. So the check has to be "does the plugin actually read this",
+ * and the cheapest honest proxy is a reference in its source.
+ *
+ * **Why not import `getChartOptions`.** It lives in `@neoboard/components`,
+ * whose barrel pulls in `@neo4j-nvl` — a browser-only WebGL dependency that
+ * cannot load in the node test environment. Reading the option definitions from
+ * source keeps this test fast and dependency-free; the shape it parses is
+ * guarded by `finds option keys for every chart type` below, so a refactor that
+ * changed it would fail loudly rather than silently pass.
+ *
+ * This catches "advertised but never wired". It cannot catch "wired but
+ * misused" — that needs a per-option behavioural test.
+ */
+
+const OPTIONS_DIR = join(
+  __dirname,
+  "../../../../../component/src/components/composed/chart-options",
+);
+const PLUGIN_DIR = join(__dirname, "../../../plugins");
+
+/** Files in chart-options/ that describe no chart type. */
+const NON_CHART_FILES = new Set(["index", "shared", "validate-iframe-url"]);
+
+function chartTypesWithOptions(): string[] {
+  return readdirSync(OPTIONS_DIR)
+    .filter((f) => f.endsWith(".ts") && !f.endsWith(".d.ts"))
+    .map((f) => f.slice(0, -3))
+    .filter((t) => !NON_CHART_FILES.has(t))
+    .filter((t) => existsSync(join(PLUGIN_DIR, t)))
+    .sort();
+}
+
+function optionKeys(type: string): string[] {
+  const src = readFileSync(join(OPTIONS_DIR, `${type}.ts`), "utf8");
+  return [...src.matchAll(/^\s*key:\s*"([^"]+)"/gm)].map((m) => m[1]);
+}
+
+const APP_SRC = join(__dirname, "../../..");
+
+function readIfPresent(path: string): string {
+  return existsSync(path) ? readFileSync(path, "utf8") : "";
+}
+
+/**
+ * A plugin's source, plus any `@/components/*` module it renders.
+ *
+ * Several plugins are thin wrappers that hand their settings straight to a
+ * shared renderer — `table` → `TableRenderer`, `form` → `FormWidgetRenderer` —
+ * and those renderers are where the options are actually read. Without
+ * following that one hop, every such plugin reports its entire option list as
+ * unforwarded, which is how this check first reported 29 problems where there
+ * were far fewer.
+ */
+function pluginSource(type: string): string {
+  const own = ["component.tsx", "settings.ts", "transform.ts"]
+    .map((f) => join(PLUGIN_DIR, type, f))
+    .map(readIfPresent)
+    .join("\n");
+
+  const delegated = [...own.matchAll(/from\s+"@\/components\/([\w./-]+)"/g)]
+    .flatMap((m) => [`${m[1]}.tsx`, `${m[1]}.ts`])
+    .map((rel) => join(APP_SRC, "components", rel))
+    .map(readIfPresent)
+    .join("\n");
+
+  return `${own}\n${delegated}`;
+}
+
+/**
+ * Options that are still not forwarded. Every entry is a live instance of the
+ * #1397 bug and is tracked in a follow-up issue. This list may only shrink —
+ * adding to it means shipping another control that lies.
+ *
+ * `parameter-select` is a different case and is listed for a different reason:
+ * its options are consumed by dashboard-level code (`lib/shared/url-params.ts`,
+ * `lib/parameter/apply-param-defaults.ts`) rather than by the plugin's render
+ * path, so this check cannot see them. `syncToUrl` genuinely works (#1388);
+ * `defaultValue` genuinely does not, because `extractParamDefaults` has zero
+ * callers — which is #1421, not this issue. Note the limit that exposes: a key
+ * referenced only from dead code would satisfy a text search. This ratchet
+ * catches "advertised but never wired", not "wired to nothing".
+ */
+const KNOWN_UNFORWARDED: Record<string, string[]> = {
+  graph: ["nodeSize", "showRelationshipLabels", "physics"],
+  json: ["fontSize", "showCopyButton", "theme"],
+  line: ["samplingThreshold", "samplingMethod"],
+  map: ["markerSize", "showPopup"],
+  "parameter-select": ["defaultValue", "syncToUrl"],
+};
+
+describe("chart option forwarding ratchet (#1397)", () => {
+  const types = chartTypesWithOptions();
+
+  it("finds chart types to check", () => {
+    expect(types.length).toBeGreaterThan(5);
+  });
+
+  it("finds option keys for every chart type", () => {
+    // Guards the source-parsing above: if the option-definition shape ever
+    // changes, every type would yield zero keys and the ratchet would pass
+    // vacuously. Fail loudly instead.
+    const empty = types.filter((t) => optionKeys(t).length === 0);
+    expect(empty).toEqual([]);
+  });
+
+  it.each(types)("%s reads every option it advertises", (type) => {
+    const src = pluginSource(type);
+    const allowed = new Set(KNOWN_UNFORWARDED[type] ?? []);
+    const unforwarded = optionKeys(type).filter(
+      (k) => !allowed.has(k) && !new RegExp(`\\b${k}\\b`).test(src),
+    );
+    expect(unforwarded).toEqual([]);
+  });
+
+  it("the allowlist names only options that are genuinely unforwarded", () => {
+    // Stops the list rotting: once an entry is fixed it must be removed, so the
+    // ratchet keeps tightening instead of quietly permitting a working key.
+    for (const [type, keys] of Object.entries(KNOWN_UNFORWARDED)) {
+      const src = pluginSource(type);
+      for (const key of keys) {
+        expect(
+          new RegExp(`\\b${key}\\b`).test(src),
+          `${type}.${key} is allowlisted but is now read — remove it`,
+        ).toBe(false);
+      }
+    }
+  });
+});

app/src/plugins/__tests__/option-forwarding.test.tsx

@@ -0,0 +1,127 @@
+import { describe, it, expect, vi } from "vitest";
+import { render, screen } from "@testing-library/react";
+import { gaugePlugin } from "../gauge";
+import { singleValuePlugin } from "../single-value";
+import { transformToValueData } from "../single-value/transform";
+
+/**
+ * #1397 — `thresholdZones` and `trendEnabled` were advertised in the widget
+ * editor, implemented in the component library, and never handed from the
+ * plugin to the chart. These assert the exact seam that was broken: the props
+ * the plugin actually passes.
+ */
+
+vi.mock("next/dynamic", () => ({
+  default: () => {
+    const Stub = (props: Record<string, unknown>) => (
+      <div
+        data-testid="chart"
+        data-threshold-zones={String(props.thresholdZones ?? "")}
+        data-trend-direction={
+          (props.trend as { direction?: string } | undefined)?.direction ?? ""
+        }
+        data-trend-label={
+          (props.trend as { label?: string } | undefined)?.label ?? ""
+        }
+        data-value={String(props.value ?? "")}
+      />
+    );
+    Stub.displayName = "ChartStub";
+    return Stub;
+  },
+}));
+
+vi.mock("@neoboard/components", () => ({
+  Skeleton: () => null,
+  getChartOptions: () => [],
+}));
+
+const GaugeComponent = gaugePlugin.component;
+const SingleValueComponent = singleValuePlugin.component;
+
+describe("gauge thresholdZones forwarding (#1397)", () => {
+  const zones =
+    '[{"value":30,"color":"#ef4444"},{"value":100,"color":"#22c55e"}]';
+
+  it("passes thresholdZones through to the chart", () => {
+    render(<GaugeComponent data={[]} settings={{ thresholdZones: zones }} />);
+    expect(
+      screen.getByTestId("chart").getAttribute("data-threshold-zones"),
+    ).toBe(zones);
+  });
+
+  it("passes nothing when the option is unset", () => {
+    render(<GaugeComponent data={[]} settings={{}} />);
+    expect(
+      screen.getByTestId("chart").getAttribute("data-threshold-zones"),
+    ).toBe("");
+  });
+});
+
+describe("single-value trendEnabled forwarding (#1397)", () => {
+  /** The shape the editor's own instruction produces: two rows, label + value. */
+  const twoRows = [
+    { label: "2026-03", value: 100 },
+    { label: "2026-02", value: 80 },
+  ];
+
+  it("renders an upward trend when enabled and a previous row exists", () => {
+    render(
+      <SingleValueComponent
+        data={transformToValueData(twoRows)}
+        settings={{ trendEnabled: true }}
+      />,
+    );
+    const el = screen.getByTestId("chart");
+    expect(el.getAttribute("data-trend-direction")).toBe("up");
+    expect(el.getAttribute("data-trend-label")).toBe("25.0%");
+  });
+
+  it("renders a downward trend when the value fell", () => {
+    render(
+      <SingleValueComponent
+        data={transformToValueData([...twoRows].reverse())}
+        settings={{ trendEnabled: true }}
+      />,
+    );
+    expect(
+      screen.getByTestId("chart").getAttribute("data-trend-direction"),
+    ).toBe("down");
+  });
+
+  it("renders no trend when the option is off", () => {
+    render(
+      <SingleValueComponent
+        data={transformToValueData(twoRows)}
+        settings={{ trendEnabled: false }}
+      />,
+    );
+    expect(
+      screen.getByTestId("chart").getAttribute("data-trend-direction"),
+    ).toBe("");
+  });
+
+  it("renders no trend when there is only one row to compare", () => {
+    render(
+      <SingleValueComponent
+        data={transformToValueData([{ label: "2026-03", value: 100 }])}
+        settings={{ trendEnabled: true }}
+      />,
+    );
+    expect(
+      screen.getByTestId("chart").getAttribute("data-trend-direction"),
+    ).toBe("");
+  });
+
+  // The headline symptom of #1397: following the editor's "requires 2 rows"
+  // instruction rendered the date column as the KPI, e.g. `$2026-03`.
+  it("shows the numeric column as the headline value, not the label", () => {
+    render(
+      <SingleValueComponent
+        data={transformToValueData(twoRows)}
+        settings={{ trendEnabled: true }}
+      />,
+    );
+    expect(screen.getByTestId("chart").getAttribute("data-value")).toBe("100");
+  });
+});

app/src/plugins/gauge/component.tsx

@@ -38,6 +38,7 @@ function GaugePluginComponent({
       startAngle={settings.startAngle}
       endAngle={settings.endAngle}
       colorPalette={settings.colorPalette}
+      thresholdZones={settings.thresholdZones}
       stylingRules={stylingRules as StylingRule[] | undefined}
       paramValues={paramValues}
       colorblindMode={settings.colorblindMode}

app/src/plugins/gauge/settings.ts

@@ -13,6 +13,10 @@ export const gaugeSettingsSchema = z
     endAngle: z.coerce.number().default(-45),
     colorPalette: z.string().optional(),
     colorblindMode: z.boolean().default(false),
+    // JSON array of {value, color} bands, parsed by the chart. Declared here
+    // for typing — `.passthrough()` never stripped it; the plugin simply did
+    // not forward it (#1397).
+    thresholdZones: z.string().optional(),
   })
   .passthrough();
 

app/src/plugins/single-value/component.tsx

@@ -11,7 +11,11 @@
 import type { StylingRule } from "@neoboard/components";
 import { normalizeValue } from "@/lib/shared/normalize-value";
 import { defineChartPlugin } from "../registry";
-import { transformToValueData, validateValueData } from "./transform";
+import {
+  transformToValueData,
+  validateValueData,
+  type SingleValueData,
+} from "./transform";
 import { type PluginProps } from "../utils";
 import { singleValueSettingsSchema } from "./settings";
 import { safeParseSettings } from "@/lib/plugin/safe-parse-settings";
@@ -24,6 +28,34 @@
   { ssr: false, loading: () => <Skeleton className="w-full h-full" /> },
 );
 
+/**
+ * Percentage change against the previous period, or undefined when the trend is
+ * off or there is nothing to compare. A previous value of 0 yields a direction
+ * without a percentage — dividing by it would render `Infinity%`.
+ */
+function buildTrend(
+  enabled: boolean,
+  value: string | number,
+  previous: number | undefined,
+): { direction: "up" | "down" | "neutral"; label?: string } | undefined {
+  if (!enabled || typeof value !== "number" || previous === undefined) {
+    return undefined;
+  }
+  const delta = value - previous;
+  const direction = delta > 0 ? "up" : delta < 0 ? "down" : "neutral";
+  if (previous === 0) {
+    return {
+      direction,
+      label: direction === "neutral" ? "no change" : undefined,
+    };
+  }
+  const pct = Math.abs(delta / previous) * 100;
+  return {
+    direction,
+    label: direction === "neutral" ? "no change" : `${pct.toFixed(1)}%`,
+  };
+}
+
 function SingleValuePluginComponent({
   data,
   settings: raw,
@@ -35,11 +67,24 @@
     raw,
     "single-value",
   );
-  const rawData = data ?? 0;
-  const val =
-    typeof rawData === "number" || typeof rawData === "string"
-      ? rawData
-      : (normalizeValue(rawData) ?? String(rawData));
+  // `transform` yields { value, previous }; older callers may still hand over a
+  // bare scalar, so both shapes are accepted.
+  const parsed: SingleValueData =
+    data !== null && typeof data === "object" && "value" in data
+      ? (data as SingleValueData)
+      : { value: (normalizeValue(data) ?? 0) as string | number };
+
+  const val = parsed.value;
+
+  // The chart takes a computed { direction, label }, not a boolean — so the
+  // option could never have been forwarded as-is. It needs the previous row,
+  // which is why the transform now carries it (#1397).
+  const trend = buildTrend(
+    settings.trendEnabled === true,
+    val,
+    parsed.previous,
+  );
+
   return (
     <SingleValueChart
       value={
@@ -51,6 +96,7 @@
       fontSize={settings.fontSize}
       numberFormat={settings.numberFormat}
       decimalPlaces={settings.decimalPlaces}
+      trend={trend}
       stylingRules={stylingRules as StylingRule[] | undefined}
       paramValues={paramValues}
     />