diff --git a/CHANGELOG.md b/CHANGELOG.md index cc8a9982..cd7660dd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ Chart-experience release: chart authoring, editing, rule-based styling and click ### Fixed +- A parameter widget's **Default value** was never applied, so every dashboard relying on defaults rendered empty on arrival. `extractParamDefaults` walked the layout correctly, had its own unit test, and had **zero production callers** — the editor's field wrote a value into the saved layout that was read only by a test. The seeded Chart Playground carries 21 configured defaults across 8 pages and showed `Waiting for parameters… $param_cat_dimension $param_cat_metric …` on every chart until the user set each knob by hand. Defaults are now seeded on load, filling only parameters that are not already set — which gives the precedence `URL > restored session > default` without any ordering machinery, since both of those are applied before the layout finishes loading. A once-per-dashboard guard stops a cleared parameter snapping straight back. This was the third instance of the same shape after #1234 and #1388, so a guard now asserts this helper has a production caller; the general form, a dead-export check across `lib/`, is #1477 (#1421) - Every dashboard page you had ever visited kept auto-refreshing, forever, so query load scaled with browsing history rather than with what was on screen. Measured on Chart Reference sitting on the same page with the same 18 visible widgets: 16 `/api/query` POSTs in 40s having opened only page 1, against 77 after touring six pages and returning — **4.81x the database traffic for an identical view**, ~68 hidden widgets polling for pages nobody was looking at. Tour all 20 pages and leave the tab open and roughly 230 widgets keep polling. All of it is refresh-tier work against the customer's database, which is exactly the load the query scheduler exists to shed, so one user exploring a large dashboard could crowd out interactive queries for everyone on that connector. `isActive` was already computed and already used to hide the page and to gate `onLayoutChange` — it just never gated the refresh. Visited pages still stay mounted, so tab switching remains instant and does not re-query; they simply stop polling while hidden (#1419) - 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) diff --git a/app/e2e/param-defaults.spec.ts b/app/e2e/param-defaults.spec.ts new file mode 100644 index 00000000..c86fd1b6 --- /dev/null +++ b/app/e2e/param-defaults.spec.ts @@ -0,0 +1,90 @@ +import { test, expect, ALICE, createTestDashboard } from "./fixtures"; + +/** + * #1421 — a parameter widget's **Default value** was never applied, because + * `extractParamDefaults` had zero production callers. Every dashboard relying + * on defaults rendered empty on arrival: the seeded Chart Playground (8 pages, + * 21 configured defaults) showed "Waiting for parameters…" on every chart until + * the user set each knob by hand. + * + * Built as a fixture rather than driving the seeded Playground, which is a demo + * showcase and is not present in the E2E database. + */ +test.describe("Parameter defaults are applied on load (#1421)", () => { + test.beforeEach(async ({ authPage }) => { + await authPage.login(ALICE.email, ALICE.password); + }); + + test("a configured default renders the chart instead of 'Waiting for parameters'", async ({ + page, + }) => { + test.setTimeout(90_000); + + const { id, cleanup } = await createTestDashboard( + page.request, + `Param defaults ${Date.now()}`, + ); + + try { + await page.request.put(`/api/dashboards/${id}`, { + data: { + layoutJson: { + version: 2, + pages: [ + { + id: "p1", + title: "Page 1", + widgets: [ + { + id: "sel", + chartType: "parameter-select", + connectionId: "conn-neo4j-001", + query: "", + settings: { + title: "Title filter", + chartOptions: { + parameterName: "title_filter", + parameterType: "text", + defaultValue: "The", + }, + }, + }, + { + // Consumes the parameter. Neo4j binds `$param_` natively, + // so this is a real bound parameter, not string splicing. + id: "tbl", + chartType: "table", + connectionId: "conn-neo4j-001", + query: + "MATCH (m:Movie) WHERE m.title CONTAINS $param_title_filter RETURN m.title AS title LIMIT 3", + settings: { title: "Filtered movies" }, + }, + ], + gridLayout: [ + { i: "sel", x: 0, y: 0, w: 4, h: 3 }, + { i: "tbl", x: 4, y: 0, w: 8, h: 4 }, + ], + }, + ], + }, + }, + }); + + await page.goto(`/${id}`); + + // The symptom: the consuming widget stalls, naming the token it lacks. + await expect(page.getByText(/Waiting for parameters/)).toHaveCount(0, { + timeout: 20_000, + }); + await expect(page.getByText("$param_title_filter")).toHaveCount(0); + + // And the chart actually renders, which only happens once the parameter + // resolves and the query runs. + await expect(page.locator("table").first()).toBeVisible({ + timeout: 20_000, + }); + } finally { + await cleanup(); + } + }); +}); diff --git a/app/src/components/__tests__/dashboard-workspace.test.tsx b/app/src/components/__tests__/dashboard-workspace.test.tsx index 115c9e85..96702b0a 100644 --- a/app/src/components/__tests__/dashboard-workspace.test.tsx +++ b/app/src/components/__tests__/dashboard-workspace.test.tsx @@ -1124,6 +1124,128 @@ describe("DashboardWorkspace", () => { expect(useDashboardStore.getState().layout.pages).toHaveLength(3); }); + // ── Parameter defaults (#1421) ────────────────────────────────────── + // `extractParamDefaults` walked the layout correctly and had its own unit + // test, but zero production callers — so the editor's "Default value" field + // wrote into the saved layout and was read only by a test. The seeded Chart + // Playground carries 21 defaults and rendered "Waiting for parameters…" on + // every chart until the user configured each knob by hand. + describe("parameter defaults are applied on load (#1421)", () => { + function withDefaults() { + const d = makeDashboard(1); + d.layoutJson.pages[0].widgets.push({ + id: "pw1", + chartType: "parameter-select", + connectionId: "c1", + query: "", + settings: { + chartOptions: { + parameterName: "dimension", + parameterType: "select", + defaultValue: "category", + }, + }, + } as unknown as (typeof d.layoutJson.pages)[0]["widgets"][0]); + return d; + } + + function renderWithDefaults() { + mockUseDashboard.mockReturnValue({ + data: withDefaults(), + isLoading: false, + isFetching: false, + }); + return render(); + } + + it("seeds the store from a widget's configured default", () => { + renderWithDefaults(); + const entry = useParameterStore.getState().parameters.dimension; + expect(entry?.value).toBe("category"); + // Provenance matters: without this, seeding could label the value as a + // user selection and the assertion above would not notice. + expect(entry?.sourceType).toBe("default"); + }); + + it("a URL parameter beats the default", () => { + searchParams = new URLSearchParams("param_dimension=revenue"); + renderWithDefaults(); + expect(useParameterStore.getState().parameters.dimension?.value).toBe( + "revenue", + ); + }); + + it("a restored session value beats the default", () => { + // Must go through localStorage, not the store: `restoreFromDashboard` + // runs on mount and *replaces* the store wholesale, so anything set + // beforehand is wiped before the defaults effect ever runs. + // Shape matters: `restoreFromDashboard` drops any entry missing a string + // `source`, `field` or `type`, so a near-miss fixture would restore + // nothing and this test would "pass" against a broken implementation. + localStorage.setItem( + "nb-params:d1", + JSON.stringify({ + dimension: { + value: "region", + source: "Dimension", + field: "dimension", + type: "text", + sourceType: "selector-widget", + sourceWidgetId: "pw1", + }, + }), + ); + renderWithDefaults(); + expect(useParameterStore.getState().parameters.dimension?.value).toBe( + "region", + ); + }); + + // The dangerous regression: re-seeding on every render would make a + // parameter impossible to clear — it would snap back instantly. + it("does not re-apply the default after the user clears it", () => { + const { rerender } = renderWithDefaults(); + expect(useParameterStore.getState().parameters.dimension?.value).toBe( + "category", + ); + + useParameterStore.getState().clearParameter("dimension"); + + // A plain rerender is not enough: it reuses the same dashboard object, so + // `serverLayout` keeps its identity, the effect's deps never change, and + // the effect does not re-run — the test would pass with the ref guard + // deleted. Handing back a fresh object forces the effect to fire again, + // which is the only thing that actually exercises the guard. + mockUseDashboard.mockReturnValue({ + data: withDefaults(), + isLoading: false, + isFetching: false, + }); + rerender(); + + expect(useParameterStore.getState().parameters.dimension).toBeUndefined(); + }); + + it("leaves a parameter-select with no default alone", () => { + const d = makeDashboard(1); + d.layoutJson.pages[0].widgets.push({ + id: "pw2", + chartType: "parameter-select", + connectionId: "c1", + query: "", + settings: { chartOptions: { parameterName: "unset" } }, + } as unknown as (typeof d.layoutJson.pages)[0]["widgets"][0]); + mockUseDashboard.mockReturnValue({ + data: d, + isLoading: false, + isFetching: false, + }); + + render(); + expect(useParameterStore.getState().parameters.unset).toBeUndefined(); + }); + }); + // ── Parameters ↔ URL ──────────────────────────────────────────────── it("applies param_ values from the URL on mount", () => { searchParams = new URLSearchParams("param_year=1999"); diff --git a/app/src/components/dashboard-workspace.tsx b/app/src/components/dashboard-workspace.tsx index d35cde6d..7fb5c0b3 100644 --- a/app/src/components/dashboard-workspace.tsx +++ b/app/src/components/dashboard-workspace.tsx @@ -28,6 +28,7 @@ import { buildParamsUrl, extractSyncParams, } from "@/lib/shared/url-params"; +import { extractParamDefaults } from "@/lib/parameter/apply-param-defaults"; import { migrateLayout } from "@/lib/dashboard/migrate-layout"; import { getRefetchInterval } from "@/lib/dashboard/dashboard-settings"; import { classifySaveError } from "@/lib/dashboard/save-error"; @@ -186,6 +187,33 @@ export function DashboardWorkspace({ [serverLayout], ); + // Widget "Default value" settings, applied once per dashboard (#1421). + // + // `extractParamDefaults` was written, tested, and never called — so the + // editor's Default value field wrote into the saved layout and was read only + // by its own unit test. The seeded Chart Playground carries 21 defaults and + // showed "Waiting for parameters…" on every chart until each knob was set by + // hand. + // + // Only fills parameters that are not already set, which is what gives the + // precedence `URL > restored session > default` without any ordering + // machinery: the restore and URL effects above both run before the layout has + // loaded, so whatever they put in the store is already there by the time this + // can run. The ref guard is what stops a cleared parameter snapping back — + // without it, clearing a knob would be impossible. + const defaultsAppliedFor = useRef(null); + useEffect(() => { + if (!serverLayout || defaultsAppliedFor.current === id) return; + defaultsAppliedFor.current = id; + + const defaults = extractParamDefaults(serverLayout); + const store = useParameterStore.getState(); + for (const [name, value] of Object.entries(defaults)) { + if (store.parameters[name] !== undefined) continue; + store.setParameter(name, value, value, "", "text", "default", ""); + } + }, [id, serverLayout]); + // Seeded from the URL we arrived on, so the first sync is a no-op unless it // actually has something to strip. const lastSyncedUrlRef = useRef(null); diff --git a/app/src/lib/__tests__/parameter/apply-param-defaults.test.ts b/app/src/lib/__tests__/parameter/apply-param-defaults.test.ts index 61d2f786..eb5e6df0 100644 --- a/app/src/lib/__tests__/parameter/apply-param-defaults.test.ts +++ b/app/src/lib/__tests__/parameter/apply-param-defaults.test.ts @@ -1,4 +1,7 @@ import { describe, it, expect } from "vitest"; +import { execFileSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; import { extractParamDefaults } from "@/lib/parameter/apply-param-defaults"; import type { DashboardLayoutV2 } from "@/lib/db/schema"; @@ -104,3 +107,72 @@ describe("extractParamDefaults", () => { }); }); }); + +// --------------------------------------------------------------------------- +// #1421 — the helper below was written, unit-tested, and never called. +// --------------------------------------------------------------------------- + +describe("extractParamDefaults has a production caller (#1421)", () => { + /** + * This function was correct and fully covered while doing nothing, because + * nothing invoked it: the editor's "Default value" field wrote into the saved + * layout and was read only by this test file. The seeded Chart Playground + * carries 21 defaults and showed "Waiting for parameters…" on every chart. + * + * A unit test proving a function works is not evidence that anything calls + * it. This is the third instance of that shape — #1388 (`extractNoSyncParams`) + * and #1234 (the audit trail) were the first two. + * + * Deliberately narrow. The general form — "any `lib/` export reachable only + * from `__tests__` fails the build" — was measured at ~69 current matches, + * the great majority legitimate (`_reset*` test hooks, Zod fragments composed + * in-file, Drizzle enums). Shipping that would mean shipping an allowlist + * bigger than the signal, so it is filed separately as a tooling change. + */ + it("is imported and called by non-test source", () => { + const appSrc = join(__dirname, "../../.."); + let matched: string[]; + try { + matched = execFileSync( + "grep", + [ + "-rl", + "extractParamDefaults", + appSrc, + "--include=*.ts", + "--include=*.tsx", + ], + { encoding: "utf8" }, + ) + .trim() + .split("\n") + .filter(Boolean); + } catch { + // grep exits 1 on no matches, which would otherwise throw before the + // assertion and hide the message explaining what broke. + matched = []; + } + + // A raw text match is not enough: this file's own explanatory comment names + // the helper, so a comment alone would satisfy it even with the import and + // the call deleted. Strip comments, then require both. + const callers = matched + .filter((f) => !f.includes("apply-param-defaults.ts")) + .filter((f) => !f.includes("__tests__") && !f.includes(".test.")) + .filter((f) => { + const code = readFileSync(f, "utf8") + .replace(/\/\*[\s\S]*?\*\//g, "") + .replace(/^\s*\/\/.*$/gm, ""); + const imported = /import\s*\{[^}]*\bextractParamDefaults\b[^}]*\}/.test( + code, + ); + const called = /\bextractParamDefaults\s*\(/.test(code); + return imported && called; + }); + + expect( + callers, + "extractParamDefaults has no production caller — the Default value field would silently do nothing", + ).not.toEqual([]); + }); +}); diff --git a/app/src/stores/parameter-store.ts b/app/src/stores/parameter-store.ts index bf41b072..33c33e55 100644 --- a/app/src/stores/parameter-store.ts +++ b/app/src/stores/parameter-store.ts @@ -29,7 +29,10 @@ export type ParameterSource = | "click-action" | "selector-widget" | "url" - | "cross-dashboard"; + | "cross-dashboard" + // Seeded from a parameter widget's configured Default value on load (#1421). + // Distinct from "selector-widget": the user never picked this. + | "default"; export interface ParameterEntry { value: unknown;