From 9221c31c3d7869c6efd426df608d6af1f9cd9640 Mon Sep 17 00:00:00 2001 From: alfredorubin96 Date: Thu, 6 Aug 2026 12:19:11 +0200 Subject: [PATCH 1/2] fix(params): apply a parameter widget's configured Default value (#1421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit extractParamDefaults walked the layout correctly, had its own unit test, and had zero production callers. The editor's "Default value" field wrote into the saved layout and was read only by that test. The seeded Chart Playground carries 21 defaults across 8 pages and showed "Waiting for parameters..." on every chart until each knob was set by hand — one of the first things a new user opens. Seeded on load, filling only parameters not already set. That yields the required precedence with no ordering machinery: restore and URL are both applied before the layout finishes loading, so whatever they put in the store is already there when defaults run. URL param > restored session > widget default > unset A once-per-dashboard ref guard stops a cleared parameter snapping back; without it a knob would be impossible to clear. Adds "default" to ParameterSource rather than labelling these "selector-widget" — nothing branches on the field, but recording a value the user never picked as a user selection is false provenance. Third instance of this shape after #1234 and #1388, so a narrow guard asserts this helper has a production caller. The generalised version the issue asked for — any lib/ export reachable only from tests fails the build — measured at ~69 current matches, mostly legitimate (_reset* test hooks, Zod fragments composed in-file, Drizzle enums). That needs a real dead-export tool with a baseline and is filed as #1477. The E2E was verified in both directions: against the unfixed build it fails on both assertions ("Waiting for parameters" present, table absent); with the fix it passes. Closes #1421 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 1 + app/e2e/param-defaults.spec.ts | 90 +++++++++++++++ .../__tests__/dashboard-workspace.test.tsx | 109 ++++++++++++++++++ app/src/components/dashboard-workspace.tsx | 28 +++++ .../parameter/apply-param-defaults.test.ts | 49 ++++++++ app/src/stores/parameter-store.ts | 5 +- 6 files changed, 281 insertions(+), 1 deletion(-) create mode 100644 app/e2e/param-defaults.spec.ts 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..3564bc08 100644 --- a/app/src/components/__tests__/dashboard-workspace.test.tsx +++ b/app/src/components/__tests__/dashboard-workspace.test.tsx @@ -1124,6 +1124,115 @@ 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(); + expect(useParameterStore.getState().parameters.dimension?.value).toBe( + "category", + ); + }); + + 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"); + 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..78607107 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,6 @@ import { describe, it, expect } from "vitest"; +import { execFileSync } from "node:child_process"; +import { join } from "node:path"; import { extractParamDefaults } from "@/lib/parameter/apply-param-defaults"; import type { DashboardLayoutV2 } from "@/lib/db/schema"; @@ -104,3 +106,50 @@ 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 by non-test source", () => { + const appSrc = join(__dirname, "../../.."); + const hits = execFileSync( + "grep", + [ + "-rl", + "extractParamDefaults", + appSrc, + "--include=*.ts", + "--include=*.tsx", + ], + { encoding: "utf8" }, + ) + .trim() + .split("\n") + .filter(Boolean) + .filter((f) => !f.includes("apply-param-defaults.ts")) + .filter((f) => !f.includes("__tests__") && !f.includes(".test.")); + + expect( + hits, + "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; From b6f22a352d2da7004d3494b66a90942802275e2d Mon Sep 17 00:00:00 2001 From: alfredorubin96 Date: Fri, 7 Aug 2026 12:33:18 +0200 Subject: [PATCH 2/2] test(params): make two guards actually fail on broken code (#1421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit review on #1478 found two tests that passed against the very things they were meant to protect. Both verified, both now proven to fail when the protection is removed. 1. "does not re-apply the default after the user clears it" passed with the ref guard deleted. `rerender` reuses the same mocked dashboard object, so `serverLayout` keeps its identity, the effect's deps never change, and the effect never re-runs — nothing exercised the guard. It now hands back a fresh object so the effect fires again. 2. The production-caller guard matched raw text, and this feature's own explanatory comment names `extractParamDefaults` in prose. Deleting the import and the call left the comment behind and the guard still passed. It now strips comments and requires both an import binding and a call expression, and handles grep's exit-1-on-no-match so the custom failure message survives. Also asserts `sourceType === "default"` on the seeded value, so seeding cannot silently record it as a user selection. Verified by neutering each protection in turn: the ref-guard test fails with the guard removed, and the caller guard fails with the import and call deleted while the prose comment remains. Co-Authored-By: Claude Opus 5 --- .../__tests__/dashboard-workspace.test.tsx | 19 ++++++- .../parameter/apply-param-defaults.test.ts | 57 +++++++++++++------ 2 files changed, 56 insertions(+), 20 deletions(-) diff --git a/app/src/components/__tests__/dashboard-workspace.test.tsx b/app/src/components/__tests__/dashboard-workspace.test.tsx index 3564bc08..96702b0a 100644 --- a/app/src/components/__tests__/dashboard-workspace.test.tsx +++ b/app/src/components/__tests__/dashboard-workspace.test.tsx @@ -1160,9 +1160,11 @@ describe("DashboardWorkspace", () => { it("seeds the store from a widget's configured default", () => { renderWithDefaults(); - expect(useParameterStore.getState().parameters.dimension?.value).toBe( - "category", - ); + 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", () => { @@ -1208,6 +1210,17 @@ describe("DashboardWorkspace", () => { ); 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(); 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 78607107..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,5 +1,6 @@ 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"; @@ -128,27 +129,49 @@ describe("extractParamDefaults has a production caller (#1421)", () => { * 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 by non-test source", () => { + it("is imported and called by non-test source", () => { const appSrc = join(__dirname, "../../.."); - const hits = execFileSync( - "grep", - [ - "-rl", - "extractParamDefaults", - appSrc, - "--include=*.ts", - "--include=*.tsx", - ], - { encoding: "utf8" }, - ) - .trim() - .split("\n") - .filter(Boolean) + 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) => !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( - hits, + callers, "extractParamDefaults has no production caller — the Default value field would silently do nothing", ).not.toEqual([]); });