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
8 changes: 8 additions & 0 deletions app/src/components/widget-editor-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React, {
useRef,
} from "react";
import { useQueryExecution } from "@/hooks/use-query-execution";
import { allReferencedParamsReady } from "@/hooks/use-widget-query";
import type {
DashboardWidget,
DashboardLayoutV2,
Expand Down Expand Up @@ -284,6 +285,12 @@ export function WidgetEditorModal({

const previewQuery = useQueryExecution();
const allParamValues = useParameterValues();
// Query references $param_x tokens that aren't all bound — the preview shows
// a waiting state instead of running the literal token and erroring (#1055).
const previewWaitingForParams = !allReferencedParamsReady(
query,
allParamValues,
);

// Derive the selected connection object so we can read its type
const selectedConnection = useMemo(
Expand Down Expand Up @@ -890,6 +897,7 @@ export function WidgetEditorModal({
}}
initialPreviewData={initialPreviewData}
onRunPreview={handlePreview}
waitingForParams={previewWaitingForParams}
/>
</div>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,26 @@ vi.mock("@/hooks/use-widget-query", () => ({
return result;
},
),
// Mirrors the real helper: a query is ready unless it references a
// $param_<name> that is missing/empty in allParams (#1055).
allReferencedParamsReady: vi.fn(
(q: string, allParams: Record<string, unknown>) => {
const regex = /\$param_(\w+)/g;
let match;
while ((match = regex.exec(q)) !== null) {
const v = allParams[match[1]];
if (
v === undefined ||
v === null ||
v === "" ||
(Array.isArray(v) && v.length === 0)
) {
return false;
}
}
return true;
},
),
}));

vi.mock("@/lib/query/wrap-with-preview-limit", () => ({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,22 @@ describe("WidgetPreviewPanel", () => {
expect(screen.queryByText("Run")).not.toBeInTheDocument();
});

it("shows a waiting state instead of running an unbound-param query (#1055)", () => {
render(
<WidgetPreviewPanel
{...makeProps({
query: "SELECT * FROM t WHERE s = $param_status",
waitingForParams: true,
})}
/>,
);
expect(screen.getByTestId("preview-waiting-params")).toHaveTextContent(
/Waiting for parameters/i,
);
// The chart preview (card container) must not render while waiting.
expect(screen.queryByTestId("card-container")).not.toBeInTheDocument();
});

it("renders MarkdownWidget when isMarkdown", () => {
render(
<WidgetPreviewPanel
Expand Down
13 changes: 8 additions & 5 deletions app/src/components/widget-editor/parameter-config-section.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

import React, { useState, useEffect, useRef } from "react";
import { useWidgetEditorStore } from "@/stores/widget-editor-store";
import { normalizeParamName } from "@/lib/parameter/normalize-param-name";
import {
Calendar,
Type,
Expand Down Expand Up @@ -166,6 +167,8 @@ export function ParameterConfigSection({
const onParamWidgetNameChange = useWidgetEditorStore(
(s) => s.setParamWidgetName,
);
// The consumed token strips a leading param_ so it isn't doubled (#1055).
const displayParamName = normalizeParamName(paramWidgetName);
const chartOptions = useWidgetEditorStore((s) => s.chartOptions);
const onChartOptionsChange = useWidgetEditorStore((s) => s.setChartOptions);
const connectionId = useWidgetEditorStore((s) => s.connectionId);
Expand Down Expand Up @@ -387,31 +390,31 @@ export function ParameterConfigSection({
<p>
Other widgets can use this parameter as:{" "}
<code className="bg-muted px-1 py-0.5 rounded text-foreground">
$param_{paramWidgetName}
$param_{displayParamName}
</code>
</p>
{paramUIType === "date" &&
(dateSub === "range" || dateSub === "relative") && (
<p>
Date range sub-parameters:{" "}
<code className="bg-muted px-1 py-0.5 rounded text-foreground">
$param_{paramWidgetName}_from
$param_{displayParamName}_from
</code>
,{" "}
<code className="bg-muted px-1 py-0.5 rounded text-foreground">
$param_{paramWidgetName}_to
$param_{displayParamName}_to
</code>
</p>
)}
{paramUIType === "number-range" && (
<p>
Number range sub-parameters:{" "}
<code className="bg-muted px-1 py-0.5 rounded text-foreground">
$param_{paramWidgetName}_min
$param_{displayParamName}_min
</code>
,{" "}
<code className="bg-muted px-1 py-0.5 rounded text-foreground">
$param_{paramWidgetName}_max
$param_{displayParamName}_max
</code>
</p>
)}
Expand Down
5 changes: 4 additions & 1 deletion app/src/components/widget-editor/parameter-preview.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
CascadingSelector,
} from "@neoboard/components";
import type { ParamUIType, DateSubType } from "./parameter-config-section";
import { normalizeParamName } from "@/lib/parameter/normalize-param-name";

const DEFAULT_PREVIEW_OPTIONS = [
{ value: "option-1", label: "Option 1" },
Expand Down Expand Up @@ -47,7 +48,9 @@ export function ParameterPreview({
>
<div className="w-full max-w-xs space-y-3">
<Label className="text-xs text-muted-foreground block">
{paramWidgetName ? `$param_${paramWidgetName}` : "Parameter preview"}
{paramWidgetName
? `$param_${normalizeParamName(paramWidgetName)}`
: "Parameter preview"}
</Label>
{seedQueryError && (
<p className="text-xs text-destructive">{seedQueryError}</p>
Expand Down
10 changes: 9 additions & 1 deletion app/src/components/widget-editor/use-auto-preview.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ import {
useState,
} from "react";
import type { ConnectionListItem } from "@/hooks/use-connections";
import { extractReferencedParams } from "@/hooks/use-widget-query";
import {
extractReferencedParams,
allReferencedParamsReady,
} from "@/hooks/use-widget-query";
import { wrapWithPreviewLimit } from "@/lib/query/wrap-with-preview-limit";
import type { DashboardWidget } from "@/lib/db/schema";

Expand Down Expand Up @@ -78,6 +81,11 @@ export function useAutoPreview({
const cId = connectionIdRef.current;
const q = queryRef.current;
if (cId && q.trim()) {
// Don't run a query that still has unbound $param_x tokens — the literal
// token would surface a raw `syntax error at or near "$"` in the editor
// preview. Mirror the dashboard's "Waiting for parameters…" state by
// skipping the run (#1055).
if (!allReferencedParamsReady(q, allParamValuesRef.current)) return;
const referenced = extractReferencedParams(q, allParamValuesRef.current);
const params =
Object.keys(referenced).length > 0 ? referenced : undefined;
Expand Down
4 changes: 3 additions & 1 deletion app/src/components/widget-editor/use-widget-save.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { useCallback } from "react";
import { useWidgetEditorStore } from "@/stores/widget-editor-store";
import type { DashboardWidget, DashboardLayoutV2 } from "@/lib/db/schema";
import { resolveInternalParamType } from "./parameter-config-section";
import { normalizeParamName } from "@/lib/parameter/normalize-param-name";

/**
* Builds a DashboardWidget object from the current widget editor store state.
Expand Down Expand Up @@ -59,7 +60,8 @@ export function useBuildWidgetForSave(
dateSub,
multiSelect,
),
parameterName: paramWidgetName,
// Strip a leading param_ so the consumed token isn't doubled (#1055).
parameterName: normalizeParamName(paramWidgetName),
// Seed query is only meaningful for the option-backed types.
seedQuery:
paramUIType === "select" || paramUIType === "cascading"
Expand Down
18 changes: 18 additions & 0 deletions app/src/components/widget-editor/widget-preview-panel.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,8 @@ type WidgetPreviewPanelProps = Readonly<{
};
initialPreviewData: PreviewData | undefined;
onRunPreview: () => void;
/** Query references $param_x tokens that aren't all bound yet (#1055). */
waitingForParams?: boolean;
}>;

function renderMarkdown(chartOptions: Record<string, unknown>) {
Expand Down Expand Up @@ -253,6 +255,7 @@ export function WidgetPreviewPanel({
previewQuery,
initialPreviewData,
onRunPreview,
waitingForParams,
}: WidgetPreviewPanelProps) {
Comment thread
coderabbitai[bot] marked this conversation as resolved.
function renderPreviewContent() {
if (isMarkdown) return renderMarkdown(chartOptions);
Expand All @@ -270,6 +273,20 @@ export function WidgetPreviewPanel({
});
}
if (isForm) return renderForm(formFields, chartOptions);
if (waitingForParams) {
// Mirror the dashboard's waiting state instead of running the literal
// $param_x token and surfacing a raw DB syntax error (#1055).
return (
<div className="flex h-full items-center justify-center p-6">
<p
className="text-sm text-muted-foreground"
data-testid="preview-waiting-params"
>
Waiting for parameters…
</p>
</div>
);
}
return renderChart({
chartType,
connectionId,
Expand Down Expand Up @@ -311,6 +328,7 @@ export function WidgetPreviewPanel({
{!isParamSelect &&
!isForm &&
!isContentOnly &&
!waitingForParams &&
previewQuery.isError && (
<Tooltip>
<TooltipTrigger asChild>
Expand Down
19 changes: 19 additions & 0 deletions app/src/lib/parameter/__tests__/normalize-param-name.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { describe, it, expect } from "vitest";
import { normalizeParamName } from "../normalize-param-name";

describe("normalizeParamName (#1055)", () => {
it("strips a leading param_ prefix so the token isn't doubled", () => {
expect(normalizeParamName("param_status")).toBe("status");
expect(normalizeParamName("PARAM_status")).toBe("status");
});

it("leaves a clean name untouched", () => {
expect(normalizeParamName("status")).toBe("status");
expect(normalizeParamName("country")).toBe("country");
});

it("only strips one leading prefix (not internal occurrences)", () => {
expect(normalizeParamName("param_param_status")).toBe("param_status");
expect(normalizeParamName("status_param")).toBe("status_param");
});
});
11 changes: 11 additions & 0 deletions app/src/lib/parameter/normalize-param-name.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/**
* Normalize a user-supplied parameter name (#1055).
*
* Parameters are consumed as `$param_<name>`, so a user who names a parameter
* `param_status` would otherwise produce a doubled `$param_param_status` token
* and a `PARAM_STATUS` label. Strip a single leading `param_` (case-insensitive)
* so the consumed token and label are clean.
*/
export function normalizeParamName(name: string): string {
return name.replace(/^param_/i, "");
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,19 +27,32 @@ describe("DataGridPagination", () => {
data={data}
pageSize={10}
pagination={(table) => <DataGridPagination table={table} />}
/>
/>,
);
expect(screen.getByText("Rows per page")).toBeInTheDocument();
});

it("shows the current page size in the selector trigger (#1055)", () => {
render(
<DataGrid
columns={columns}
data={data}
pageSize={10}
pagination={(table) => <DataGridPagination table={table} />}
/>,
);
// The trigger (combobox) displays the active page size, not just a chevron.
expect(screen.getByRole("combobox")).toHaveTextContent("10");
});

it("renders page info", () => {
render(
<DataGrid
columns={columns}
data={data}
pageSize={10}
pagination={(table) => <DataGridPagination table={table} />}
/>
/>,
);
expect(screen.getByText("Page 1 of 3")).toBeInTheDocument();
});
Expand All @@ -52,7 +65,7 @@ describe("DataGridPagination", () => {
data={data}
pageSize={10}
pagination={(table) => <DataGridPagination table={table} />}
/>
/>,
);
await user.click(screen.getByRole("button", { name: "Go to next page" }));
expect(screen.getByText("Page 2 of 3")).toBeInTheDocument();
Expand All @@ -66,10 +79,12 @@ describe("DataGridPagination", () => {
data={data}
pageSize={10}
pagination={(table) => <DataGridPagination table={table} />}
/>
/>,
);
await user.click(screen.getByRole("button", { name: "Go to next page" }));
await user.click(screen.getByRole("button", { name: "Go to previous page" }));
await user.click(
screen.getByRole("button", { name: "Go to previous page" }),
);
expect(screen.getByText("Page 1 of 3")).toBeInTheDocument();
});

Expand All @@ -80,8 +95,10 @@ describe("DataGridPagination", () => {
data={data}
pageSize={10}
pagination={(table) => <DataGridPagination table={table} />}
/>
/>,
);
expect(screen.getByRole("button", { name: "Go to previous page" })).toBeDisabled();
expect(
screen.getByRole("button", { name: "Go to previous page" }),
).toBeDisabled();
});
});
Original file line number Diff line number Diff line change
@@ -1,20 +1,25 @@
import { render, screen } from "@testing-library/react";
import { describe, it, expect } from "vitest";
import userEvent from "@testing-library/user-event";
import type { ColumnDef } from "@tanstack/react-table";
import { DataGrid } from "../data-grid";
import { DataGridViewOptions } from "../data-grid-view-options";

interface TestRow {
name: string;
email: string;
total_spend: number;
}

const columns: ColumnDef<TestRow, unknown>[] = [
{ accessorKey: "name", header: "Name" },
{ accessorKey: "email", header: "Email" },
{ accessorKey: "total_spend", header: "Total Spend" },
];

const data: TestRow[] = [{ name: "Alice", email: "alice@example.com" }];
const data: TestRow[] = [
{ name: "Alice", email: "alice@example.com", total_spend: 100 },
];

describe("DataGridViewOptions", () => {
it("renders icon-only button with sr-only text and title", () => {
Expand All @@ -35,6 +40,23 @@ describe("DataGridViewOptions", () => {
expect(button).toHaveAttribute("title", "Hide columns");
});

it("humanizes snake_case column labels in the hide-columns menu (#1055)", async () => {
const user = userEvent.setup();
render(
<DataGrid
columns={columns}
data={data}
toolbar={(table) => <DataGridViewOptions table={table} />}
/>,
);
await user.click(screen.getByRole("button", { name: /hide columns/i }));
// total_spend → "Total Spend", not "total_spend".
expect(
screen.getByRole("menuitemcheckbox", { name: "Total Spend" }),
).toBeInTheDocument();
expect(screen.queryByText("total_spend")).not.toBeInTheDocument();
});

it("does not render visible 'View' label text", () => {
render(
<DataGrid
Expand Down
Loading
Loading