Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
90 changes: 90 additions & 0 deletions app/e2e/param-defaults.spec.ts
Original file line number Diff line number Diff line change
@@ -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();
}
});
});
122 changes: 122 additions & 0 deletions app/src/components/__tests__/dashboard-workspace.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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(<DashboardWorkspace id="d1" editMode={false} />);
}

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");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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(<DashboardWorkspace id="d1" editMode={false} />);

expect(useParameterStore.getState().parameters.dimension).toBeUndefined();
Comment thread
coderabbitai[bot] marked this conversation as resolved.
});

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(<DashboardWorkspace id="d1" editMode={false} />);
expect(useParameterStore.getState().parameters.unset).toBeUndefined();
});
});

// ── Parameters ↔ URL ────────────────────────────────────────────────
it("applies param_ values from the URL on mount", () => {
searchParams = new URLSearchParams("param_year=1999");
Expand Down
28 changes: 28 additions & 0 deletions app/src/components/dashboard-workspace.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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<string | null>(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<string | null>(null);
Expand Down
72 changes: 72 additions & 0 deletions app/src/lib/__tests__/parameter/apply-param-defaults.test.ts
Original file line number Diff line number Diff line change
@@ -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";
Comment thread
coderabbitai[bot] marked this conversation as resolved.
import { extractParamDefaults } from "@/lib/parameter/apply-param-defaults";
import type { DashboardLayoutV2 } from "@/lib/db/schema";

Expand Down Expand Up @@ -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([]);
});
});
5 changes: 4 additions & 1 deletion app/src/stores/parameter-store.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading