feat(v1.0): number-range integer/float + slider fix + Chart Playground/Reference demos - #853
Closed
alfredo1996 wants to merge 10 commits into
Closed
feat(v1.0): number-range integer/float + slider fix + Chart Playground/Reference demos#853alfredo1996 wants to merge 10 commits into
alfredo1996 wants to merge 10 commits into
Conversation
The `number-range` parameter widget type is fully supported by the runtime
(`ParamNumberRange` dual-handle slider, parameter-store, plugin settings
schema), but the widget editor never exposed it. As a result:
* Users could not create a number-range parameter widget from the UI.
* Existing number-range widgets (e.g. those produced by the Chart
Playground demo) reopened in the editor as type "Select" with no
min/max/step controls — saving over them silently changed their type.
Root cause: the parameter-type dropdown in `parameter-config-section.tsx`
only listed select/freetext/date, and `reverseParamTypeMapping` had no
case for `number-range` (falling through to the `select` default). The
forward mapping in `resolveInternalParamType` was also missing the
`number-range` branch.
Changes:
* Add `"number-range"` to the `ParamUIType` union and to both the
forward (`resolveInternalParamType`) and reverse
(`reverseParamTypeMapping`) mappings in
`parameter-config-section.tsx` and `stores/widget-editor-store.ts`.
* Add a "Number Range" entry (SlidersHorizontal icon) to the parameter
type dropdown.
* Add an editor UI block with min/max/step number inputs that read
from / write to `chartOptions.rangeMin`/`rangeMax`/`rangeStep` — the
same keys the runtime renderer (`parameter-widget-renderer.tsx`) and
the Zod settings schema (`plugins/parameter-select/settings.ts`)
already use.
* Add a `NumberRangeSlider` branch to the editor's parameter preview
panel so the configured slider renders inline.
* Unit tests cover: mapping round-trip for number-range, loading an
existing number-range widget, building one in the save path, and
rendering the editor UI inputs.
How to test:
1. Open any number-range widget produced by the Chart Playground
showcase (PR #850) — it now opens as type "Number Range" with the
min/max/step inputs populated and editable.
2. Create a new parameter widget, pick "Number Range", set
min/max/step + a parameter name, save. The widget renders a slider
and sets `$param_<name>`, `$param_<name>_min`, `$param_<name>_max`.
Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
A fifth demo showcase, co-existing with the existing four (Chart Gallery, Click Actions, Transformations, Rule-Based Styling). 8 pages cover every chart type, each with query-level knobs (parameter-select widgets that flow into $param_* substitution) and side-by-side variant tiles for chart options that don't go through the substitution path. Pages: Overview (3 KPIs), Categorical (bar + pie), Time series (line + gantt), Distribution & Hierarchy (treemap/sunburst/circle), Geographic (map + choropleth), Network & Flow (graph + sankey), Single-value/Tabular/Specialty (single-value/gauge/table/json/radar), Text & Interactive (parameter-select + form + markdown + iframe). Numeric knobs use the `_max` companion of number-range parameters so the query substitutes a scalar instead of a tuple. Each page includes a markdown intro reminding users to refresh to reset parameters.
…eric knobs The numeric knobs in the Chart Playground (limit / window / count / target sliders) were saved with `parameterType: "number-range"`. That type stores a `[min, max]` tuple and writes companion `$param_X_min` / `$param_X_max` parameters, which is why every query referenced `$param_X_max`. Two problems with that choice: 1. The widget editor's parameter-type selector only exposes `select`, `freetext`, and `date` (see `parameter-config-section.tsx`). `number-range` exists in the runtime renderer but has no editor UI — opening these widgets fell back to "select" with no min/max controls, so the user could not change min/max/step/default. 2. For a single-knob "limit / window" use case we want a scalar substitution (`LIMIT 25`), not a tuple's upper bound used as a single value via a `_max` companion hack. Switched every standalone numeric parameter widget to `parameterType: "select"` with a small constant `seedQuery` returning the preset numeric values. This type is fully editable from the editor and substitutes the chosen value as a plain scalar, so queries now reference `$param_X` directly (the `_max` companion hack is gone). The pg_rating form-field keeps `number-range` — form fields have full editor support for number-range via `form-fields-editor.tsx`. 10 parameter widgets switched, 14 query references updated. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Reverts 983601d which incorrectly swapped number-range widgets to select-with-preset-options as a workaround for the widget editor missing number-range support. The right fix is making the editor support number-range (see fix/parameter-config-number-range-editor / separate PR). Continuous numeric sliders are the correct UX for these knobs; discrete dropdowns of presets were a downgrade. After both PRs merge, the Chart Playground number-range widgets will be fully editable from the UI.
…ation reference A new static reference dashboard with one page per chart type. Each tile shows the same dataset with one chartOption changed, so users can see exactly what every setting does side by side. No parameter knobs — this is a reference, not a playground. Co-exists with the four existing showcases (Chart Gallery, Click Actions, Transformations, Rule-Based Styling) and the Chart Playground (#850). Different job per artefact: - Gallery = one-look tour of supported widgets - Playground = interactive knobs over a single chart - Reference = static side-by-side option comparison (this PR) 20 pages, 183 variant tiles + 20 markdown headers (203 widgets total). Every option in component/src/components/composed/chart-options/* is demonstrated where it has a meaningful visual effect. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
- Add Integer/Float type selector to number-range param editor (parameter selector + form fields editor). Stored as chartOptions.rangeNumberType, defaults to "integer" for backward compat. - Coerce typed/slider values to whole numbers in integer mode at three layers: editor inputs, ParamNumberRange runtime, NumberRangeSlider. - Fix shadcn Slider: render one Thumb per value so dual-handle (range) sliders show handles at BOTH ends, not just the start. - Thread rangeNumberType through parameter-select plugin and form widget renderer so JSON-seeded dashboards can opt into float widgets. - Demo: convert "Gauge — minimum target %" knob in chart-playground to float (step 0.5) as a worked example.
WalkthroughAdds ChangesNumber-range integer/float parameter support
Chart Playground demo dashboard
Sequence Diagram(s)sequenceDiagram
participant User
participant ParameterPreview
participant NumberRangeSlider
participant Coerce
participant ParamNumberRange
participant Store
User->>ParameterPreview: open preview / interact
ParameterPreview->>NumberRangeSlider: render(min,max,step,numberType)
User->>NumberRangeSlider: edit inputs / move thumbs
NumberRangeSlider->>Coerce: [min,max] with numberType
Coerce-->>NumberRangeSlider: coerced tuple
NumberRangeSlider->>ParamNumberRange: onChange(coerced tuple)
ParamNumberRange->>Store: set(range tuple) + mirror min/max fields
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced May 18, 2026
Adds unit tests for the rangeNumberType branches in both parameter-config-section and form-fields-editor: switching between integer/float adjusts the step default (1 ↔ 0.1), and min/max/step inputs round when integer and preserve decimals when float. Switches the Input mock in parameter-config-section-ui.test.tsx to defaultValue+key so React treats it as uncontrolled — its value tracker otherwise suppresses fireEvent.change for numeric values, making fractional coercion impossible to assert. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
… branch Covers the new number-range preview branch (rangeMin/Max/Step fallbacks) plus the existing label / error / date-range branches that previously had no test file at all. Pushes SonarCloud new_coverage on PR over the 80% gate. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com>
Brings new-code coverage on form-fields-editor.tsx from 50% to ~95% to clear SonarCloud 80% gate.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/src/components/widget-editor/form-fields-editor.tsx`:
- Around line 267-276: The update path in the widget editor (the block that
computes nextStep from e.target.value and field.rangeStep and calls onUpdate
with rangeNumberType and rangeStep) must validate and sanitize number-range
invariants before calling onUpdate: ensure rangeStep is a finite number > 0
(clamp or set a sensible default like 1 or 0.1 depending on rangeNumberType),
coerce values with Number(...) and Number.isFinite, and ensure min <= max (swap
or adjust them if not). Locate the code that reads e.target.value as "integer" |
"float", uses field.rangeStep/currentStep and nextStep, and update it so
validations (finite check, step>0, and min/max ordering) run and corrected
values are passed into onUpdate rather than persisting raw inputs.
In `@app/src/components/widget-editor/parameter-config-section.tsx`:
- Around line 272-289: The code is allowing invalid rangeStep values (0,
negative, NaN) to be persisted; when updating chartOptions (e.g., in the
onValueChange handler that sets rangeNumberType and rangeStep) validate and
sanitize rangeStep: coerce to a number, use Number.isFinite to reject
NaN/Infinity, and clamp to a safe minimum (for "integer" use Math.max(1,
Math.round(value)) and for "float" use Math.max(0.0001, value) or a sensible
default like 0.1 if the input is <=0 or not finite); apply this check wherever
rangeStep is written so chartOptions.rangeStep is never 0, negative, or NaN.
- Around line 320-355: The onChartOptionsChange handlers for updating rangeMin
and rangeMax can persist an invalid state where rangeMin > rangeMax; update both
handlers (the onChange lambdas that compute rangeMin and rangeMax) to enforce
min<=max by computing the new numeric value (respecting
chartOptions.rangeNumberType rounding) and then, when setting rangeMin, if
newMin > prev.rangeMax set rangeMax = newMin, and when setting rangeMax, if
newMax < prev.rangeMin set rangeMin = newMax; keep other prev fields unchanged
and return the combined object so the range stays valid.
In `@app/src/components/widget-editor/parameter-preview.tsx`:
- Around line 96-106: The NumberRangeSlider preview is missing the numberType
prop so float-configured parameters get integer behavior; update the JSX for
NumberRangeSlider (rendered when paramUIType === "number-range") to pass through
chartOptions.numberType (e.g., numberType={(chartOptions.numberType as any) ??
'float'}) so the slider honors the parameter's numberType setting; ensure you
reference NumberRangeSlider and paramUIType and use chartOptions.numberType as
the source for the prop.
In `@component/src/components/composed/parameter-widgets/number-range-slider.tsx`:
- Around line 52-64: The handlers handleMinInput and handleMaxInput currently
only coerce the edited bound before calling onChange, which allows the untouched
bound to remain un-coerced (e.g., a decimal in integer mode); update both
handlers so that before emitting onChange you apply coerce to both elements of
the tuple (call coerce on current[1] in handleMinInput and on current[0] in
handleMaxInput) and then call onChange with the fully coerced pair.
In `@scripts/demo/chart-playground.json`:
- Around line 1113-1128: Replace the non-embeddable seed URLs used for the
iframe demo: update the seedQuery and defaultValue that populate the
"$param_txt_iframe_url" option (see "seedQuery", "defaultValue") so they point
to known embeddable pages (or pages you control) instead of Wikipedia/GitHub,
and ensure the iframe widget with id "txt-iframe" and chartType "iframe" will
receive one of those embeddable URLs as its default and selectable options.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac97bdb2-1f1e-4ddc-826c-f3713129641b
📒 Files selected for processing (21)
app/src/components/form-widget-renderer.tsxapp/src/components/parameter-widget-renderer.tsxapp/src/components/parameters/param-number-range.tsxapp/src/components/widget-editor/__tests__/form-fields-editor.test.tsxapp/src/components/widget-editor/__tests__/parameter-config-section-ui.test.tsxapp/src/components/widget-editor/__tests__/parameter-config-section.test.tsxapp/src/components/widget-editor/__tests__/parameter-preview.test.tsxapp/src/components/widget-editor/__tests__/use-widget-save.test.tsxapp/src/components/widget-editor/form-fields-editor.tsxapp/src/components/widget-editor/parameter-config-section.tsxapp/src/components/widget-editor/parameter-preview.tsxapp/src/lib/widget/form-field-def.tsapp/src/plugins/parameter-select/component.tsxapp/src/plugins/parameter-select/settings.tsapp/src/stores/__tests__/widget-editor-store.test.tsapp/src/stores/widget-editor-store.tscomponent/src/components/composed/parameter-widgets/number-range-slider.tsxcomponent/src/components/ui/slider.tsxscripts/demo/chart-playground.jsonscripts/demo/chart-reference.jsonscripts/demo/showcases.mjs
Comment on lines
+272
to
+289
| onValueChange={(v) => { | ||
| const next = v as "integer" | "float"; | ||
| onChartOptionsChange((prev) => { | ||
| // When switching to float, drop integer-only step=1 default; | ||
| // when switching to integer, snap step to >=1 whole number. | ||
| const currentStep = | ||
| (prev.rangeStep as number | undefined) ?? 1; | ||
| let nextStep = currentStep; | ||
| if (next === "float" && currentStep === 1) { | ||
| nextStep = 0.1; | ||
| } else if (next === "integer") { | ||
| nextStep = Math.max(1, Math.round(currentStep)); | ||
| } | ||
| return { | ||
| ...prev, | ||
| rangeNumberType: next, | ||
| rangeStep: nextStep, | ||
| }; |
There was a problem hiding this comment.
Sanitize rangeStep before persisting config.
On Line 379 and Line 385, float mode writes raw input directly. This allows 0, negative, or NaN step values into chartOptions.
Suggested fix
+const normalizeStep = (raw: number, type: "integer" | "float") => {
+ if (!Number.isFinite(raw)) return type === "integer" ? 1 : 0.1;
+ return type === "integer" ? Math.max(1, Math.round(raw)) : Math.max(0.000001, raw);
+};
onValueChange={(v) => {
const next = v as "integer" | "float";
onChartOptionsChange((prev) => {
const currentStep =
(prev.rangeStep as number | undefined) ?? 1;
- let nextStep = currentStep;
- if (next === "float" && currentStep === 1) {
- nextStep = 0.1;
- } else if (next === "integer") {
- nextStep = Math.max(1, Math.round(currentStep));
- }
+ const nextStep =
+ next === "float" && currentStep === 1
+ ? 0.1
+ : normalizeStep(currentStep, next);
return {
...prev,
rangeNumberType: next,
rangeStep: nextStep,
};
});
}}
onChange={(e) =>
onChartOptionsChange((prev) => {
const numType =
(prev.rangeNumberType as string | undefined) ?? "integer";
const raw = Number(e.target.value);
return {
...prev,
- rangeStep:
- numType === "integer"
- ? Math.max(1, Math.round(raw))
- : raw,
+ rangeStep: normalizeStep(raw, numType as "integer" | "float"),
};
})
}Also applies to: 375-386
🤖 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
272 - 289, The code is allowing invalid rangeStep values (0, negative, NaN) to
be persisted; when updating chartOptions (e.g., in the onValueChange handler
that sets rangeNumberType and rangeStep) validate and sanitize rangeStep: coerce
to a number, use Number.isFinite to reject NaN/Infinity, and clamp to a safe
minimum (for "integer" use Math.max(1, Math.round(value)) and for "float" use
Math.max(0.0001, value) or a sensible default like 0.1 if the input is <=0 or
not finite); apply this check wherever rangeStep is written so
chartOptions.rangeStep is never 0, negative, or NaN.
Comment on lines
+320
to
+355
| onChange={(e) => | ||
| onChartOptionsChange((prev) => { | ||
| const numType = | ||
| (prev.rangeNumberType as string | undefined) ?? "integer"; | ||
| const raw = Number(e.target.value); | ||
| return { | ||
| ...prev, | ||
| rangeMin: numType === "integer" ? Math.round(raw) : raw, | ||
| }; | ||
| }) | ||
| } | ||
| /> | ||
| </div> | ||
| <div className="space-y-1"> | ||
| <Label htmlFor="param-range-max" className="text-xs"> | ||
| Max | ||
| </Label> | ||
| <Input | ||
| id="param-range-max" | ||
| type="number" | ||
| step={ | ||
| ((chartOptions.rangeNumberType as string | undefined) ?? | ||
| "integer") === "integer" | ||
| ? 1 | ||
| : "any" | ||
| } | ||
| value={(chartOptions.rangeMax as number | undefined) ?? 100} | ||
| onChange={(e) => | ||
| onChartOptionsChange((prev) => { | ||
| const numType = | ||
| (prev.rangeNumberType as string | undefined) ?? "integer"; | ||
| const raw = Number(e.target.value); | ||
| return { | ||
| ...prev, | ||
| rangeMax: numType === "integer" ? Math.round(raw) : raw, | ||
| }; |
There was a problem hiding this comment.
Prevent saving inverted range bounds (min > max).
On Line 327 and Line 354, bounds are updated independently. This can persist rangeMin > rangeMax, which makes the resulting range config invalid.
Suggested fix
onChange={(e) =>
onChartOptionsChange((prev) => {
const numType =
(prev.rangeNumberType as string | undefined) ?? "integer";
const raw = Number(e.target.value);
+ const nextMin =
+ numType === "integer" ? Math.round(raw) : raw;
+ const currentMax = (prev.rangeMax as number | undefined) ?? 100;
return {
...prev,
- rangeMin: numType === "integer" ? Math.round(raw) : raw,
+ rangeMin: Math.min(nextMin, currentMax),
};
})
}
onChange={(e) =>
onChartOptionsChange((prev) => {
const numType =
(prev.rangeNumberType as string | undefined) ?? "integer";
const raw = Number(e.target.value);
+ const nextMax =
+ numType === "integer" ? Math.round(raw) : raw;
+ const currentMin = (prev.rangeMin as number | undefined) ?? 0;
return {
...prev,
- rangeMax: numType === "integer" ? Math.round(raw) : raw,
+ rangeMax: Math.max(nextMax, currentMin),
};
})
}🤖 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
320 - 355, The onChartOptionsChange handlers for updating rangeMin and rangeMax
can persist an invalid state where rangeMin > rangeMax; update both handlers
(the onChange lambdas that compute rangeMin and rangeMax) to enforce min<=max by
computing the new numeric value (respecting chartOptions.rangeNumberType
rounding) and then, when setting rangeMin, if newMin > prev.rangeMax set
rangeMax = newMin, and when setting rangeMax, if newMax < prev.rangeMin set
rangeMin = newMax; keep other prev fields unchanged and return the combined
object so the range stays valid.
Comment on lines
+96
to
+106
| {paramUIType === "number-range" && ( | ||
| <NumberRangeSlider | ||
| parameterName={paramWidgetName || "preview"} | ||
| min={(chartOptions.rangeMin as number | undefined) ?? 0} | ||
| max={(chartOptions.rangeMax as number | undefined) ?? 100} | ||
| step={(chartOptions.rangeStep as number | undefined) ?? 1} | ||
| value={null} | ||
| onChange={() => {}} | ||
| onClear={() => {}} | ||
| showInputs | ||
| /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify NumberRangeSlider exposes numberType and whether ParameterPreview forwards it.
rg -n --type=tsx -C3 '\bnumberType\b' component/src/components/composed/parameter-widgets/number-range-slider.tsx app/src/components/widget-editor/parameter-preview.tsx
rg -n --type=tsx -C3 'rangeNumberType|NumberRangeSlider' app/src/components/widget-editor/parameter-preview.tsxRepository: alfredo1996/neoboard
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Find files and check content with corrected syntax
echo "=== Looking for NumberRangeSlider component ==="
find . -name "*number-range-slider*" -type f | head -20
echo "=== Checking ParameterPreview component ==="
find . -name "*parameter-preview*" -type f | head -20Repository: alfredo1996/neoboard
Length of output: 363
🏁 Script executed:
#!/bin/bash
# Search for numberType in the codebase without type flag
echo "=== Searching for numberType prop usage ==="
rg -n 'numberType' --max-count=50
echo "=== Searching for rangeNumberType ==="
rg -n 'rangeNumberType' --max-count=50Repository: alfredo1996/neoboard
Length of output: 8492
🏁 Script executed:
#!/bin/bash
# Get more details about NumberRangeSlider props
echo "=== Inspecting NumberRangeSlider component ==="
rg -B5 -A10 'interface.*NumberRangeSliderProps|export.*NumberRangeSlider' | head -100Repository: alfredo1996/neoboard
Length of output: 3374
🏁 Script executed:
#!/bin/bash
# Inspect parameter-preview.tsx to check chartOptions type and current implementation
echo "=== Reading parameter-preview.tsx (full file) ==="
cat -n app/src/components/widget-editor/parameter-preview.tsxRepository: alfredo1996/neoboard
Length of output: 4704
Add numberType passthrough to NumberRangeSlider preview.
The number-range preview omits the numberType prop, causing float-configured parameters to preview with integer rounding instead of preserving float behavior.
Proposed fix
{paramUIType === "number-range" && (
<NumberRangeSlider
parameterName={paramWidgetName || "preview"}
min={(chartOptions.rangeMin as number | undefined) ?? 0}
max={(chartOptions.rangeMax as number | undefined) ?? 100}
step={(chartOptions.rangeStep as number | undefined) ?? 1}
+ numberType={
+ (chartOptions.rangeNumberType as "integer" | "float" | undefined) ??
+ "integer"
+ }
value={null}
onChange={() => {}}
onClear={() => {}}
showInputs
/>
)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {paramUIType === "number-range" && ( | |
| <NumberRangeSlider | |
| parameterName={paramWidgetName || "preview"} | |
| min={(chartOptions.rangeMin as number | undefined) ?? 0} | |
| max={(chartOptions.rangeMax as number | undefined) ?? 100} | |
| step={(chartOptions.rangeStep as number | undefined) ?? 1} | |
| value={null} | |
| onChange={() => {}} | |
| onClear={() => {}} | |
| showInputs | |
| /> | |
| {paramUIType === "number-range" && ( | |
| <NumberRangeSlider | |
| parameterName={paramWidgetName || "preview"} | |
| min={(chartOptions.rangeMin as number | undefined) ?? 0} | |
| max={(chartOptions.rangeMax as number | undefined) ?? 100} | |
| step={(chartOptions.rangeStep as number | undefined) ?? 1} | |
| numberType={ | |
| (chartOptions.rangeNumberType as "integer" | "float" | undefined) ?? | |
| "integer" | |
| } | |
| value={null} | |
| onChange={() => {}} | |
| onClear={() => {}} | |
| showInputs | |
| /> |
🤖 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-preview.tsx` around lines 96 -
106, The NumberRangeSlider preview is missing the numberType prop so
float-configured parameters get integer behavior; update the JSX for
NumberRangeSlider (rendered when paramUIType === "number-range") to pass through
chartOptions.numberType (e.g., numberType={(chartOptions.numberType as any) ??
'float'}) so the slider honors the parameter's numberType setting; ensure you
reference NumberRangeSlider and paramUIType and use chartOptions.numberType as
the source for the prop.
Comment on lines
52
to
64
| const handleMinInput = (raw: string) => { | ||
| const num = Number(raw); | ||
| if (isNaN(num)) return; | ||
| const clamped = Math.min(Math.max(num, min), current[1]); | ||
| onChange([clamped, current[1]]); | ||
| onChange([coerce(clamped), current[1]]); | ||
| }; | ||
|
|
||
| const handleMaxInput = (raw: string) => { | ||
| const num = Number(raw); | ||
| if (isNaN(num)) return; | ||
| const clamped = Math.max(Math.min(num, max), current[0]); | ||
| onChange([current[0], clamped]); | ||
| onChange([current[0], coerce(clamped)]); | ||
| }; |
There was a problem hiding this comment.
Coerce both tuple bounds before emitting in integer mode.
On Line 56 and Line 63, only the edited bound is coerced. In integer mode, the untouched bound can stay decimal and leak through onChange.
Suggested fix
const handleMinInput = (raw: string) => {
const num = Number(raw);
if (isNaN(num)) return;
const clamped = Math.min(Math.max(num, min), current[1]);
- onChange([coerce(clamped), current[1]]);
+ onChange([coerce(clamped), coerce(current[1])]);
};
const handleMaxInput = (raw: string) => {
const num = Number(raw);
if (isNaN(num)) return;
const clamped = Math.max(Math.min(num, max), current[0]);
- onChange([current[0], coerce(clamped)]);
+ onChange([coerce(current[0]), coerce(clamped)]);
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleMinInput = (raw: string) => { | |
| const num = Number(raw); | |
| if (isNaN(num)) return; | |
| const clamped = Math.min(Math.max(num, min), current[1]); | |
| onChange([clamped, current[1]]); | |
| onChange([coerce(clamped), current[1]]); | |
| }; | |
| const handleMaxInput = (raw: string) => { | |
| const num = Number(raw); | |
| if (isNaN(num)) return; | |
| const clamped = Math.max(Math.min(num, max), current[0]); | |
| onChange([current[0], clamped]); | |
| onChange([current[0], coerce(clamped)]); | |
| }; | |
| const handleMinInput = (raw: string) => { | |
| const num = Number(raw); | |
| if (isNaN(num)) return; | |
| const clamped = Math.min(Math.max(num, min), current[1]); | |
| onChange([coerce(clamped), coerce(current[1])]); | |
| }; | |
| const handleMaxInput = (raw: string) => { | |
| const num = Number(raw); | |
| if (isNaN(num)) return; | |
| const clamped = Math.max(Math.min(num, max), current[0]); | |
| onChange([coerce(current[0]), coerce(clamped)]); | |
| }; |
🤖 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 `@component/src/components/composed/parameter-widgets/number-range-slider.tsx`
around lines 52 - 64, The handlers handleMinInput and handleMaxInput currently
only coerce the edited bound before calling onChange, which allows the untouched
bound to remain un-coerced (e.g., a decimal in integer mode); update both
handlers so that before emitting onChange you apply coerce to both elements of
the tuple (call coerce on current[1] in handleMinInput and on current[0] in
handleMaxInput) and then call onChange with the fully coerced pair.
Comment on lines
+1113
to
+1128
| "seedQuery": "SELECT 'https://example.com' AS value, 'example.com' AS label UNION ALL SELECT 'https://en.wikipedia.org/wiki/Data_visualization', 'Wikipedia — Data visualization' UNION ALL SELECT 'https://github.com', 'github.com'", | ||
| "defaultValue": "https://example.com", | ||
| "searchable": false | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| "id": "txt-iframe", | ||
| "chartType": "iframe", | ||
| "connectionId": "conn_postgres_read", | ||
| "query": "", | ||
| "settings": { | ||
| "title": "Embedded content", | ||
| "chartOptions": { | ||
| "url": "$param_txt_iframe_url", | ||
| "iframeTitle": "Playground iframe" |
There was a problem hiding this comment.
Use iframe URLs that are actually embeddable.
Line 1113 includes Wikipedia/GitHub URLs, and Line 1127 renders them in an iframe. Those domains typically block framing, so this demo path will look broken for users. Please switch the seed options to known embeddable URLs (or hosted pages you control).
🤖 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 `@scripts/demo/chart-playground.json` around lines 1113 - 1128, Replace the
non-embeddable seed URLs used for the iframe demo: update the seedQuery and
defaultValue that populate the "$param_txt_iframe_url" option (see "seedQuery",
"defaultValue") so they point to known embeddable pages (or pages you control)
instead of Wikipedia/GitHub, and ensure the iframe widget with id "txt-iframe"
and chartType "iframe" will receive one of those embeddable URLs as its default
and selectable options.
* fix(number-range): draft inputs, NaN guards, validation (#857) The number-range slider added in #853 had several rough edges in the input + value pipeline that surfaced once the dual-thumb primitive landed. Fixes: - NumberRangeSlider min/max inputs now track a draft string and commit on blur/Enter. Previously `Number("-")` was NaN and silently no-op'd, leaving the field dead while typing a negative. - handleSliderChange falls back to a sane second value when Radix emits a single-element array (min===max edge case), so the tuple is always [number, number]. - param-number-range.tsx now drops non-finite tuples from the restore path instead of producing [NaN, NaN] slider state. - form-fields-editor number-range section refactored into its own component with draft state. Empty clear no longer silently zeros rangeMin via `Number("") === 0`. step <= 0 keeps the prior value. - Inline validation error when min >= max or step <= 0. Tests: - New draft-and-blur behavior coverage on the slider + editor. - NaN-guard coverage on the restore path. - Min/max validation message coverage. * test(e2e): adapt number-range E2E to draft inputs NumberRangeSlider now uses text inputs (inputMode=numeric) with commit-on-blur, so: - accessible role is `textbox`, not `spinbutton` — target inputs by their aria-label (`<param> minimum` / `<param> maximum`) - `fill()` only updates the draft; commit requires blur — add an explicit `press("Tab")` after each fill so the parameter store actually updates Co-Authored-By: Claude Opus 4 <noreply@anthropic.com> * test(number-range): cover float coerce, NaN revert, draft resync (#857) Brings Sonar new-code coverage for number-range-slider.tsx above the 80% gate. Targets previously uncovered branches: - float numberType (preserves decimals; no Math.round) - integer numberType (rounds 3.7 → 4) - NaN draft revert on blur for both min and max inputs - Enter key commits on the max input (already covered for min) - draft inputs resync when the value prop changes externally (e.g. slider drag while inputs are unfocused) - showInputs defaults to true when prop is omitted Co-Authored-By: Claude Opus 4 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
alfredo1996
added a commit
that referenced
this pull request
May 19, 2026
* feat(editor): expose number-range + cascading-select in parameter editor (#861) ParameterConfigSection was capped at 3 of the 8 supported parameter types (date / freetext / select). Number-range shipped in PR #853 and cascading-select went into the param-substitute/seed-query work, but neither was reachable through the main editor — they could only be configured by editing dashboard JSON or via FormFieldsEditor. Adds two new top-level entries to the editor's ParamUIType: - "number-range" — exposes min / max / step inputs that map to the existing rangeMin / rangeMax / rangeStep chart-options. The preview renders a live NumberRangeSlider using those bounds (with a guard so a partially-typed max <= min still renders without crashing). - "cascading" — exposes a parentParameterName input plus the same seed query block used by select. Maps to/from the runtime cascading-select type. The preview renders a live CascadingSelector showing the "depends on" label. Round-trip mappings for both types are covered in parameter-config-section.test.ts so the local copy of ParamUIType in widget-editor-store stays in sync. UI rendering of each new section is covered in parameter-config-section-ui.test.ts. Also widens the connection-required check in modal-footer / -modal so the save button and connection picker include cascading (which needs a DB for its seed query). Drive-by: SeedQueryInput's sync-prop-to-draft effect was tripping the react-hooks/set-state-in-effect rule; rewrote it as adjust-state-in- render per the React docs. Co-Authored-By: Claude Opus 4 <noreply@anthropic.com> * test: cover new parameter-preview + store branches (#861) Brings Sonar new-code coverage from 52.5% to >=80% so the quality gate passes. No production changes: - parameter-preview.test.tsx: new file covering all 8 preview branches including number-range (min/max guards, NaN handling) and cascading (parent param, placeholder propagation) - widget-editor-store.test.ts: round-trip tests for the new reverseParamTypeMapping cases (number-range, cascading-select) Co-Authored-By: Claude Opus 4 <noreply@anthropic.com> --------- Co-authored-by: alfredorubin96 <alfredo.rubin@neotechnology.com> Co-authored-by: Claude Opus 4 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
component/src/components/composed/__tests__/parameter-widgets.test.tsx (1)
471-478:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAssert calendar day lookup before clicking.
The current
if (dayBtn)makes this test pass even when no day button is found.Proposed fix
- const dayBtn = document.querySelector("button[data-day]"); - if (dayBtn) { - fireEvent.click(dayBtn); - expect(onChange).toHaveBeenCalledWith( - expect.stringMatching(/^\d{4}-\d{2}-\d{2}$/), - ); - } + const dayBtn = document.querySelector("button[data-day]"); + expect(dayBtn).not.toBeNull(); + fireEvent.click(dayBtn as Element); + expect(onChange).toHaveBeenCalledWith( + expect.stringMatching(/^\d{4}-\d{2}-\d{2}$/), + );🤖 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 `@component/src/components/composed/__tests__/parameter-widgets.test.tsx` around lines 471 - 478, The test currently guards the day button check with "if (dayBtn)" which lets the test silently pass when the element is missing; replace that guard with an explicit assertion that the button exists (e.g., expect(dayBtn).not.toBeNull() / expect(dayBtn).toBeInstanceOf(HTMLElement)) immediately after querying it, then proceed to call fireEvent.click(dayBtn) and assert onChange was called with the date regex; update the block around the "render(<div />)" / "const dayBtn = document.querySelector('button[data-day]')" to assert presence before clicking so the test fails loudly if the selector returns null.
♻️ Duplicate comments (2)
app/src/components/widget-editor/form-fields-editor.tsx (1)
126-141:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlock invalid number-range values before persisting.
commit/type-switch still allows non-finite values and can persist invalid min/max relationships; the UI shows an error but writes bad config state.Proposed fix
- const parsed = Number(raw); + const parsed = Number(raw); // Empty / NaN: revert draft to prior committed value. - if (raw.trim() === "" || isNaN(parsed)) { + if (raw.trim() === "" || !Number.isFinite(parsed)) { if (key === "rangeMin") setMinDraft(String(min)); else if (key === "rangeMax") setMaxDraft(String(max)); else setStepDraft(String(step)); return; } if (key === "rangeStep") { const next = numType === "integer" ? Math.max(1, Math.round(parsed)) : parsed; - onUpdate(field.id, { rangeStep: next > 0 ? next : step }); + onUpdate(field.id, { rangeStep: Number.isFinite(next) && next > 0 ? next : step }); } else { - onUpdate(field.id, { [key]: coerce(parsed) }); + const next = coerce(parsed); + if (key === "rangeMin") onUpdate(field.id, { rangeMin: Math.min(next, max) }); + else onUpdate(field.id, { rangeMax: Math.max(next, min) }); }Also applies to: 158-168
🤖 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/form-fields-editor.tsx` around lines 126 - 141, The commit function allows non-finite numbers and invalid min/max relationships to be persisted; update commit (and the similar block at lines ~158–168) to validate parsed values before calling onUpdate: reject non-finite values (Number.isFinite(parsed)), ensure rangeStep > 0 (for integer and float types) and enforce rangeMin < rangeMax when updating either rangeMin or rangeMax (if the new value would violate the relationship, revert the draft using setMinDraft/setMaxDraft/setStepDraft and do not call onUpdate), and for rangeStep compute and validate next (using numType and Math.round for integers) before persisting; keep using coerce(parsed) for valid min/max updates and always call onUpdate(field.id, {...}) only when all validations pass.component/src/components/composed/parameter-widgets/number-range-slider.tsx (1)
85-99:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCoerce both bounds before emitting in integer mode.
Only the edited bound is coerced; the untouched bound can stay fractional and break integer-mode guarantees.
Proposed fix
- onChange([coerce(clamped), current[1]]); + onChange([coerce(clamped), coerce(current[1])]); ... - onChange([current[0], coerce(clamped)]); + onChange([coerce(current[0]), coerce(clamped)]);🤖 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 `@component/src/components/composed/parameter-widgets/number-range-slider.tsx` around lines 85 - 99, The bug is that only the edited bound is passed through coerce before calling onChange, allowing the untouched bound (current[0] or current[1]) to remain fractional and violate integer-mode guarantees; update commitMax and the analogous commitMin to apply coerce to both bounds when emitting (i.e., call onChange with coerce(current[0]) and coerce(clamped) in commitMax, and coerce(clamped) and coerce(current[1]) in commitMin), using the existing coerce, current and onChange symbols so integer-mode invariants hold.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@app/src/components/widget-editor/__tests__/form-fields-editor.test.tsx`:
- Around line 630-636: The test currently guards the main assertion with "if
(next)" so it may silently skip validation; change it to assert unconditionally
by removing the conditional check and directly assigning/expecting the last
call: grab the last argument from mockSetFormFields.mock.calls (as the variable
next or similar) and then immediately assert expect(next!.find(f => f.id ===
"f1")!.rangeStep).toBe(0.5); so the test always fails if the mock wasn’t called
or the value isn’t correct; update references to mockSetFormFields and next in
form-fields-editor.test.tsx accordingly.
---
Outside diff comments:
In `@component/src/components/composed/__tests__/parameter-widgets.test.tsx`:
- Around line 471-478: The test currently guards the day button check with "if
(dayBtn)" which lets the test silently pass when the element is missing; replace
that guard with an explicit assertion that the button exists (e.g.,
expect(dayBtn).not.toBeNull() / expect(dayBtn).toBeInstanceOf(HTMLElement))
immediately after querying it, then proceed to call fireEvent.click(dayBtn) and
assert onChange was called with the date regex; update the block around the
"render(<div />)" / "const dayBtn = document.querySelector('button[data-day]')"
to assert presence before clicking so the test fails loudly if the selector
returns null.
---
Duplicate comments:
In `@app/src/components/widget-editor/form-fields-editor.tsx`:
- Around line 126-141: The commit function allows non-finite numbers and invalid
min/max relationships to be persisted; update commit (and the similar block at
lines ~158–168) to validate parsed values before calling onUpdate: reject
non-finite values (Number.isFinite(parsed)), ensure rangeStep > 0 (for integer
and float types) and enforce rangeMin < rangeMax when updating either rangeMin
or rangeMax (if the new value would violate the relationship, revert the draft
using setMinDraft/setMaxDraft/setStepDraft and do not call onUpdate), and for
rangeStep compute and validate next (using numType and Math.round for integers)
before persisting; keep using coerce(parsed) for valid min/max updates and
always call onUpdate(field.id, {...}) only when all validations pass.
In `@component/src/components/composed/parameter-widgets/number-range-slider.tsx`:
- Around line 85-99: The bug is that only the edited bound is passed through
coerce before calling onChange, allowing the untouched bound (current[0] or
current[1]) to remain fractional and violate integer-mode guarantees; update
commitMax and the analogous commitMin to apply coerce to both bounds when
emitting (i.e., call onChange with coerce(current[0]) and coerce(clamped) in
commitMax, and coerce(clamped) and coerce(current[1]) in commitMin), using the
existing coerce, current and onChange symbols so integer-mode invariants hold.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b08c7a96-aafb-4ca1-8691-d2e4c9188afd
📒 Files selected for processing (8)
app/e2e/parameter-types.spec.tsapp/e2e/parameters.spec.tsapp/src/components/parameters/__tests__/param-number-range.test.tsapp/src/components/parameters/param-number-range.tsxapp/src/components/widget-editor/__tests__/form-fields-editor.test.tsxapp/src/components/widget-editor/form-fields-editor.tsxcomponent/src/components/composed/__tests__/parameter-widgets.test.tsxcomponent/src/components/composed/parameter-widgets/number-range-slider.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/src/components/parameters/param-number-range.tsx
Comment on lines
+630
to
+636
| const next = mockSetFormFields.mock.calls.at(-1)?.[0] as | ||
| | FormFieldDef[] | ||
| | undefined; | ||
| if (next) { | ||
| // If commit did fire, step must have fallen back to the prior value. | ||
| expect(next.find((f) => f.id === "f1")!.rangeStep).toBe(0.5); | ||
| } |
There was a problem hiding this comment.
Make the step=0 test assert unconditionally.
This test can pass without validating behavior because the core assertion is inside if (next).
Proposed fix
- const next = mockSetFormFields.mock.calls.at(-1)?.[0] as
- | FormFieldDef[]
- | undefined;
- if (next) {
- // If commit did fire, step must have fallen back to the prior value.
- expect(next.find((f) => f.id === "f1")!.rangeStep).toBe(0.5);
- }
+ const next = mockSetFormFields.mock.calls.at(-1)?.[0] as
+ | FormFieldDef[]
+ | undefined;
+ expect(next).toBeDefined();
+ expect(next!.find((f) => f.id === "f1")!.rangeStep).toBe(0.5);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const next = mockSetFormFields.mock.calls.at(-1)?.[0] as | |
| | FormFieldDef[] | |
| | undefined; | |
| if (next) { | |
| // If commit did fire, step must have fallen back to the prior value. | |
| expect(next.find((f) => f.id === "f1")!.rangeStep).toBe(0.5); | |
| } | |
| const next = mockSetFormFields.mock.calls.at(-1)?.[0] as | |
| | FormFieldDef[] | |
| | undefined; | |
| expect(next).toBeDefined(); | |
| expect(next!.find((f) => f.id === "f1")!.rangeStep).toBe(0.5); |
🤖 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/__tests__/form-fields-editor.test.tsx`
around lines 630 - 636, The test currently guards the main assertion with "if
(next)" so it may silently skip validation; change it to assert unconditionally
by removing the conditional check and directly assigning/expecting the last
call: grab the last argument from mockSetFormFields.mock.calls (as the variable
next or similar) and then immediately assert expect(next!.find(f => f.id ===
"f1")!.rangeStep).toBe(0.5); so the test always fails if the mock wasn’t called
or the value isn’t correct; update references to mockSetFormFields and next in
form-fields-editor.test.tsx accordingly.
3 tasks
Owner
Author
|
Superseded by #884 — cherry-picked the 4 demo commits onto current release/1.0. The editor-side commits from this PR (number-range type / integer-float toggle) were obsoleted by #858 / #861 / #863, which shipped equivalent functionality. The companion-typing change would have broken the #858 invariant. Closing in favor of the focused demo-only PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Consolidates #850, #851, #852 into a single PR so the new number-range editor, the runtime slider fix, and the two new demo dashboards can be tested in one pass.
What's in this PR
Editor — number-range parameter type (was #851)
parameter-config-section.tsx: Number Range now appears as a Parameter Type with Min/Max/Step inputs (root-cause fix for "I can't edit number-range widgets from the UI").widget-editor-store.ts+ reverse-mapping: existing widgets open back into the new Number Range tab.form-fields-editor.tsx: same Min/Max/Step inputs in the form-widget field editor.Editor — integer/float type option (new)
chartOptions.rangeNumberType: "integer" | "float"— defaults to"integer"so existing dashboards keep current behaviour.0.1, integer → step snapped to>=1whole number.<input step>follows the type (1vs"any"), so the browser's spinner does the right thing.Runtime — value coercion
ParamNumberRange(app/src/components/parameters/) acceptsrangeNumberTypeandMath.rounds values in integer mode before writing to the parameter store.NumberRangeSlider(component/src/components/composed/parameter-widgets/) gains anumberTypeprop and coerces slider drags + manual input in integer mode.ParameterWidgetRendererand theparameter-selectplugin schema thread the new prop through. Form-widget renderer does the same.12.5.Bugfix — Slider second handle (the "small point at the end")
component/src/components/ui/slider.tsxwas the default shadcn template that hard-codes a single<SliderPrimitive.Thumb />. With a dual-handle Radix slider that means only the start has a visible/draggable circle; the end of the range bar shows nothing.props.value ?? props.defaultValueand rendering one Thumb per value. Single-handle sliders still get one thumb; range sliders now get a thumb at both ends.Demos (were #850 + #852)
scripts/demo/showcases.mjs.rangeNumberType: "float"withstep: 0.5as a worked example.Companion params unchanged
Companion parameters
$param_X_min/$param_X_maxstill flow into queries verbatim. In integer mode they're whole numbers (safe everywhere). In float mode they're decimals — fine for Cypher'stoFloat()and Postgresnumeric/float, but will fail or auto-cast against PostgresINTEGERcolumns. Worth knowing if you wire a float widget to a strictly-integer column.Backwards compatibility
rangeNumberTypefield. Default reads as"integer", which matches the runtime behaviour they had before (Number(raw)was effectively integer for any sane existing widget withstep: 1).Supersedes
Test plan
neoboard demoboots, both new dashboards appear in the sidebar🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores