feat(editor): expose number-range + cascading-select (#861) - #868
Merged
Conversation
…tor (#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>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR extends the widget editor parameter taxonomy from 3 UI types to 5, adding ChangesParameter Type System and UI Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/components/widget-editor/__tests__/parameter-config-section-ui.test.tsx (1)
375-390: ⚡ Quick winAssert the
rangeMaxvalue, not only call occurrence.This test currently passes even if the wrong field is updated. Validate that the
setChartOptionsargument actually setsrangeMaxto250(for both object and functional-updater forms).Proposed assertion hardening
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(); + expect(mockSetChartOptions).toHaveBeenCalled(); + const arg = mockSetChartOptions.mock.calls.at(-1)?.[0]; + if (typeof arg === "function") { + const next = arg({ rangeMin: 0, rangeMax: 100, rangeStep: 1 }); + expect(next).toMatchObject({ rangeMax: 250 }); + } else { + expect(arg).toMatchObject({ rangeMax: 250 }); + }As per coding guidelines,
app/**/*.test.tsx: Use Vitest with jsdom environment for React component render tests in app/ package with branch coverage.🤖 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__/parameter-config-section-ui.test.tsx` around lines 375 - 390, Update the test that fires a change on the "Range maximum" input (in the ParameterConfigSection test where mockSetChartOptions is used) to assert the actual rangeMax was set to 250: after firing the event, grab the argument passed to mockSetChartOptions (first call), detect whether it is an object or a function, and in the object case assert arg.rangeMax === 250, and in the function case call the function with a representative previous chartOptions (e.g. {rangeMin:0, rangeMax:100, rangeStep:1}) and assert the returned object's rangeMax === 250; keep using the existing mockSetChartOptions and ParameterConfigSection references so the test verifies the updated value instead of only call occurrence.
🤖 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/parameter-config-section.tsx`:
- Around line 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.
---
Nitpick comments:
In
`@app/src/components/widget-editor/__tests__/parameter-config-section-ui.test.tsx`:
- Around line 375-390: Update the test that fires a change on the "Range
maximum" input (in the ParameterConfigSection test where mockSetChartOptions is
used) to assert the actual rangeMax was set to 250: after firing the event, grab
the argument passed to mockSetChartOptions (first call), detect whether it is an
object or a function, and in the object case assert arg.rangeMax === 250, and in
the function case call the function with a representative previous chartOptions
(e.g. {rangeMin:0, rangeMax:100, rangeStep:1}) and assert the returned object's
rangeMax === 250; keep using the existing mockSetChartOptions and
ParameterConfigSection references so the test verifies the updated value instead
of only call occurrence.
🪄 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: 79aa0510-d016-4423-a32f-10cc2e0568f5
📒 Files selected for processing (8)
app/src/components/widget-editor-modal.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/modal-footer.tsxapp/src/components/widget-editor/parameter-config-section.tsxapp/src/components/widget-editor/parameter-preview.tsxapp/src/components/widget-editor/use-widget-save.tsapp/src/stores/widget-editor-store.ts
Comment on lines
+245
to
+278
| 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), | ||
| })) |
There was a problem hiding this comment.
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.
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>
|
6 tasks
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.
Summary
Closes #861 — the parameter editor was capped at 3 of the 8 runtime types.
Adds two top-level entries to the editor's `ParamUIType`:
The connection picker and save-button gating in `widget-editor-modal` / `modal-footer` also learn about `cascading` since it needs a DB connection for its seed query.
Drive-by
`SeedQueryInput`'s `setDraft(value)` inside `useEffect` was tripping `react-hooks/set-state-in-effect`; rewrote as the React "adjust state in render" pattern (same fix as #859's `useSeedQueryOptions`).
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes