Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
5 changes: 4 additions & 1 deletion app/src/components/widget-editor-modal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -654,7 +654,10 @@ export function WidgetEditorModal({
connections={connections}
showConnection={
!isContentOnly &&
(isForm || !isParamSelect || paramUIType === "select")
(isForm ||
!isParamSelect ||
paramUIType === "select" ||
paramUIType === "cascading")
}
/>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,13 @@ vi.mock("@neoboard/components", () => ({

vi.mock("lucide-react", () => {
const Icon = () => <span />;
return { Calendar: Icon, Type: Icon, ListFilter: Icon };
return {
Calendar: Icon,
Type: Icon,
ListFilter: Icon,
SlidersHorizontal: Icon,
GitBranch: Icon,
};
});

const mockSetParamUIType = vi.fn();
Expand Down Expand Up @@ -338,4 +344,83 @@ describe("ParameterConfigSection", () => {
expect(screen.getByText("$param_period_from")).toBeInTheDocument();
expect(screen.getByText("$param_period_to")).toBeInTheDocument();
});

// ── number-range editor (regression: #861) ────────────────────────
it("shows range-bounds inputs for number-range type", () => {
mockStoreState.paramUIType = "number-range";
render(
<ParameterConfigSection
seedQueryExecution={baseSeedExecution}
seedPreviewOptions={null}
/>,
);
expect(screen.getByLabelText("Range minimum")).toBeInTheDocument();
expect(screen.getByLabelText("Range maximum")).toBeInTheDocument();
expect(screen.getByLabelText("Range step")).toBeInTheDocument();
});

it("shows number-range sub-parameters in reference hint", () => {
mockStoreState.paramUIType = "number-range";
mockStoreState.paramWidgetName = "year";
render(
<ParameterConfigSection
seedQueryExecution={baseSeedExecution}
seedPreviewOptions={null}
/>,
);
expect(screen.getByText("$param_year_min")).toBeInTheDocument();
expect(screen.getByText("$param_year_max")).toBeInTheDocument();
});

it("writes rangeMax into chartOptions when user changes max input", () => {
mockStoreState.paramUIType = "number-range";
mockStoreState.chartOptions = { rangeMin: 0, rangeMax: 100, rangeStep: 1 };
render(
<ParameterConfigSection
seedQueryExecution={baseSeedExecution}
seedPreviewOptions={null}
/>,
);
fireEvent.change(screen.getByLabelText("Range maximum"), {
target: { value: "250" },
});
// Either a direct object or a functional updater is acceptable —
// we only need to confirm chartOptions was updated for the max field.
expect(mockSetChartOptions).toHaveBeenCalled();
});

// ── cascading editor (regression: #861) ────────────────────────────
it("shows parent-parameter input for cascading type", () => {
mockStoreState.paramUIType = "cascading";
render(
<ParameterConfigSection
seedQueryExecution={baseSeedExecution}
seedPreviewOptions={null}
/>,
);
expect(screen.getByText("Parent Parameter Name")).toBeInTheDocument();
});

it("still shows seed query input for cascading type", () => {
mockStoreState.paramUIType = "cascading";
render(
<ParameterConfigSection
seedQueryExecution={baseSeedExecution}
seedPreviewOptions={null}
/>,
);
// The same seed-query block used by `select` should also render here.
expect(screen.getByText("Seed Query")).toBeInTheDocument();
});

it("hides seed query input for number-range type", () => {
mockStoreState.paramUIType = "number-range";
render(
<ParameterConfigSection
seedQueryExecution={baseSeedExecution}
seedPreviewOptions={null}
/>,
);
expect(screen.queryByText("Seed Query")).not.toBeInTheDocument();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,13 @@ import { describe, it, expect, vi } from "vitest";
vi.mock("@neoboard/components", () => ({}));
vi.mock("lucide-react", () => {
const Icon = () => null;
return { Calendar: Icon, Type: Icon, ListFilter: Icon };
return {
Calendar: Icon,
Type: Icon,
ListFilter: Icon,
SlidersHorizontal: Icon,
GitBranch: Icon,
};
});
vi.mock("@/stores/widget-editor-store", () => ({
useWidgetEditorStore: () => ({}),
Expand Down Expand Up @@ -43,6 +49,18 @@ describe("resolveInternalParamType", () => {
);
});

it("maps number-range to number-range (regression: #861)", () => {
expect(resolveInternalParamType("number-range", "single", false)).toBe(
"number-range",
);
});

it("maps cascading to cascading-select (regression: #861)", () => {
expect(resolveInternalParamType("cascading", "single", false)).toBe(
"cascading-select",
);
});

it("ignores dateSub for non-date types", () => {
expect(resolveInternalParamType("freetext", "range", false)).toBe("text");
});
Expand Down Expand Up @@ -109,6 +127,22 @@ describe("reverseParamTypeMapping", () => {
});
});

it("maps number-range back (regression: #861)", () => {
expect(reverseParamTypeMapping("number-range")).toEqual({
uiType: "number-range",
dateSub: "single",
multi: false,
});
});

it("maps cascading-select back (regression: #861)", () => {
expect(reverseParamTypeMapping("cascading-select")).toEqual({
uiType: "cascading",
dateSub: "single",
multi: false,
});
});

it("roundtrips with resolveInternalParamType", () => {
const cases = [
{ ui: "date" as const, sub: "single" as const, multi: false },
Expand All @@ -117,6 +151,8 @@ describe("reverseParamTypeMapping", () => {
{ ui: "freetext" as const, sub: "single" as const, multi: false },
{ ui: "select" as const, sub: "single" as const, multi: false },
{ ui: "select" as const, sub: "single" as const, multi: true },
{ ui: "number-range" as const, sub: "single" as const, multi: false },
{ ui: "cascading" as const, sub: "single" as const, multi: false },
];
for (const { ui, sub, multi } of cases) {
const internal = resolveInternalParamType(ui, sub, multi);
Expand Down
2 changes: 1 addition & 1 deletion app/src/components/widget-editor/modal-footer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export function ModalFooter({
disabled={
isParamSelect
? !paramWidgetName.trim() ||
(paramUIType === "select" &&
((paramUIType === "select" || paramUIType === "cascading") &&
(!connectionId ||
!String(chartOptions.seedQuery ?? "").trim()))
: isContentOnly
Expand Down
139 changes: 131 additions & 8 deletions app/src/components/widget-editor/parameter-config-section.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,13 @@

import React, { useState, useEffect, useRef } from "react";
import { useWidgetEditorStore } from "@/stores/widget-editor-store";
import { Calendar, Type, ListFilter } from "lucide-react";
import {
Calendar,
Type,
ListFilter,
SlidersHorizontal,
GitBranch,
} from "lucide-react";
import type { LucideIcon } from "lucide-react";
import {
Button,
Expand All @@ -18,7 +24,19 @@
} from "@neoboard/components";

// ── Parameter type mapping helpers ──────────────────────────────────
export type ParamUIType = "date" | "freetext" | "select";
//
// `ParamUIType` is the editor's UX-facing taxonomy: each value corresponds
// to a top-level dropdown choice. It's intentionally narrower than the
// runtime `parameterType` (8 values) — `"date"` collapses 3 sub-modes
// into one selector + a sub-radio, and `"select"` collapses single vs
// multi via a checkbox. The remaining runtime types (`number-range`,
// `cascading-select`) get their own top-level entries.
export type ParamUIType =
| "date"
| "freetext"
| "select"
| "number-range"
| "cascading";
export type DateSubType = "single" | "range" | "relative";

export function resolveInternalParamType(
Expand All @@ -34,6 +52,8 @@
: "date";
}
if (ui === "freetext") return "text";
if (ui === "number-range") return "number-range";
if (ui === "cascading") return "cascading-select";
return multi ? "multi-select" : "select";
}

Expand All @@ -53,6 +73,10 @@
return { uiType: "freetext", dateSub: "single", multi: false };
case "multi-select":
return { uiType: "select", dateSub: "single", multi: true };
case "number-range":
return { uiType: "number-range", dateSub: "single", multi: false };
case "cascading-select":
return { uiType: "cascading", dateSub: "single", multi: false };
default:
return { uiType: "select", dateSub: "single", multi: false };
}
Expand All @@ -63,6 +87,8 @@
date: { label: "Date Picker", Icon: Calendar },
freetext: { label: "Freetext", Icon: Type },
select: { label: "Select", Icon: ListFilter },
"number-range": { label: "Number Range", Icon: SlidersHorizontal },
cascading: { label: "Cascading Select", Icon: GitBranch },
};

const paramTypes = Object.keys(paramTypeMeta) as ParamUIType[];
Expand All @@ -81,15 +107,19 @@
placeholder: string;
}) {
const [draft, setDraft] = useState(value);
// Track the prop value so we can detect external updates (e.g. a parent
// resetting it) and resync the draft *during render*, per
// https://react.dev/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes
const [prevValue, setPrevValue] = useState(value);
if (value !== prevValue) {
setPrevValue(value);
setDraft(value);
}
const onChangeRef = useRef(onChange);
useEffect(() => {
onChangeRef.current = onChange;
}, [onChange]);

useEffect(() => {
setDraft(value);
}, [value]);

useEffect(() => {
if (draft === value) return;
const timer = setTimeout(() => {
Expand Down Expand Up @@ -202,8 +232,89 @@
</div>
)}

{/* Seed Query (only for select type) */}
{paramUIType === "select" && (
{/* Number-range bounds (only for number-range) */}
{paramUIType === "number-range" && (
<div className="space-y-1.5" data-testid="param-number-range-config">
<Label>Range Bounds</Label>
<div className="flex items-center gap-2">
<Input
id="range-min"
type="number"
aria-label="Range minimum"
value={(chartOptions.rangeMin as number | undefined) ?? 0}
onChange={(e) =>
onChartOptionsChange((prev) => ({
...prev,
rangeMin: Number(e.target.value),
}))
}
className="w-24"
/>
<span className="text-xs text-muted-foreground">to</span>
<Input
id="range-max"
type="number"
aria-label="Range maximum"
value={(chartOptions.rangeMax as number | undefined) ?? 100}
onChange={(e) =>
onChartOptionsChange((prev) => ({
...prev,
rangeMax: Number(e.target.value),
}))
}
className="w-24"
/>
<span className="text-xs text-muted-foreground">step</span>
<Input
id="range-step"
type="number"
aria-label="Range step"
min={0}
value={(chartOptions.rangeStep as number | undefined) ?? 1}
onChange={(e) =>
onChartOptionsChange((prev) => ({
...prev,
rangeStep: Number(e.target.value),
}))
Comment on lines +245 to +278

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Prevent invalid numeric values from being stored for range bounds/step.

Using Number(e.target.value) here can write NaN during normal typing states, and Line 272 allows 0 step. That can propagate invalid range config and break number-range behavior.

Proposed fix
             <Input
               id="range-min"
               type="number"
               aria-label="Range minimum"
               value={(chartOptions.rangeMin as number | undefined) ?? 0}
-              onChange={(e) =>
-                onChartOptionsChange((prev) => ({
-                  ...prev,
-                  rangeMin: Number(e.target.value),
-                }))
-              }
+              onChange={(e) => {
+                const n = e.target.valueAsNumber;
+                onChartOptionsChange((prev) => ({
+                  ...prev,
+                  rangeMin: Number.isFinite(n) ? n : undefined,
+                }));
+              }}
               className="w-24"
             />
@@
             <Input
               id="range-max"
               type="number"
               aria-label="Range maximum"
               value={(chartOptions.rangeMax as number | undefined) ?? 100}
-              onChange={(e) =>
-                onChartOptionsChange((prev) => ({
-                  ...prev,
-                  rangeMax: Number(e.target.value),
-                }))
-              }
+              onChange={(e) => {
+                const n = e.target.valueAsNumber;
+                onChartOptionsChange((prev) => ({
+                  ...prev,
+                  rangeMax: Number.isFinite(n) ? n : undefined,
+                }));
+              }}
               className="w-24"
             />
@@
             <Input
               id="range-step"
               type="number"
               aria-label="Range step"
-              min={0}
+              min={0.000001}
               value={(chartOptions.rangeStep as number | undefined) ?? 1}
-              onChange={(e) =>
-                onChartOptionsChange((prev) => ({
-                  ...prev,
-                  rangeStep: Number(e.target.value),
-                }))
-              }
+              onChange={(e) => {
+                const n = e.target.valueAsNumber;
+                onChartOptionsChange((prev) => ({
+                  ...prev,
+                  rangeStep: Number.isFinite(n) && n > 0 ? n : undefined,
+                }));
+              }}
               className="w-20"
             />
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/src/components/widget-editor/parameter-config-section.tsx` around lines
245 - 278, The onChange handlers for the numeric inputs (ids "range-min",
"range-max", "range-step") currently use Number(e.target.value) which can
produce NaN during typing and allows an invalid 0 for rangeStep; update the
handlers in onChartOptionsChange to: parse the input (parseFloat or Number),
check Number.isFinite(value) and only set rangeMin/rangeMax when finite
(otherwise leave previous value), and for rangeStep accept only a finite value >
0 (or fallback to a sensible default like 1) to prevent storing 0 or NaN; apply
these guards in the onChange callbacks referenced by onChartOptionsChange and
validate against chartOptions.rangeStep when computing defaults.

}
className="w-20"
/>
</div>
<p className="text-xs text-muted-foreground">
Use <code className="bg-muted px-1 rounded">step</code> ≥ 1 for
integers, or a fractional value (e.g. 0.1) for floats.
</p>
</div>
)}

{/* Cascading parent (only for cascading) */}
{paramUIType === "cascading" && (
<div className="space-y-1.5" data-testid="param-cascading-config">
<Label htmlFor="parent-param-name">Parent Parameter Name</Label>
<Input
id="parent-param-name"
value={(chartOptions.parentParameterName as string) ?? ""}
onChange={(e) =>
onChartOptionsChange((prev) => ({
...prev,
parentParameterName: e.target.value,
}))
}
placeholder="e.g. country"
/>
<p className="text-xs text-muted-foreground">
The seed query below can reference the parent via{" "}
<code className="bg-muted px-1 rounded">
$param_
{(chartOptions.parentParameterName as string) || "parent"}
</code>

Check warning on line 310 in app/src/components/widget-editor/parameter-config-section.tsx

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Ambiguous spacing after previous element code

See more on https://sonarcloud.io/project/issues?id=alfredo1996_neoboard&issues=AZ493UEcnydhvRWj96Pz&open=AZ493UEcnydhvRWj96Pz&pullRequest=868
. The cascade re-runs whenever the parent value changes.
</p>
</div>
)}

{/* Seed Query (for select and cascading) */}
{(paramUIType === "select" || paramUIType === "cascading") && (
<div className="space-y-1.5">
<Label htmlFor="seed-query">
Seed Query <span className="text-destructive">*</span>
Expand Down Expand Up @@ -292,6 +403,18 @@
</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
</code>
,{" "}
<code className="bg-muted px-1 py-0.5 rounded text-foreground">
$param_{paramWidgetName}_max
</code>
</p>
)}
</div>
</div>
)}
Expand Down
Loading
Loading