codeant-ai-for-open-source[bot] commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r2771827378
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx:
##########
@@ -322,4 +322,127 @@ describe('RangeFilterPlugin', () => {
expect(sliders.length).toBeGreaterThan(0);
});
});
+
+ describe('Decimal value handling', () => {
+ it('should handle decimal ranges correctly (0.03 to 1.08)', () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.03, max: 1.08 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.5, 0.8] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs).toHaveLength(2);
+ expect(inputs[0]).toHaveValue('0.5');
+ expect(inputs[1]).toHaveValue('0.8');
+
+ // Verify the slider exists and can handle decimal values
+ const sliders = screen.getAllByRole('slider');
+ expect(sliders.length).toBeGreaterThan(0);
+ });
+
+ it('should calculate appropriate step size for small decimal ranges', ()
=> {
+ const smallRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0.001, max: 0.01 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [0.005, 0.008] },
+ };
+ getWrapper(smallRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('0.005');
+ expect(inputs[1]).toHaveValue('0.008');
+ });
+
+ it('should handle very large ranges with appropriate step size', () => {
+ const largeRangeProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 1000000 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [100000, 500000] },
+ };
+ getWrapper(largeRangeProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('100000');
+ expect(inputs[1]).toHaveValue('500000');
+ });
+
+ it('should handle negative decimal ranges', () => {
+ const negativeDecimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: -1.5, max: 2.5 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [-0.5, 1.5] },
+ };
+ getWrapper(negativeDecimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ expect(inputs[0]).toHaveValue('-0.5');
+ expect(inputs[1]).toHaveValue('1.5');
+ });
+
+ it('should allow decimal input via keyboard', async () => {
+ const decimalProps = {
+ queriesData: [
+ {
+ rowcount: 1,
+ colnames: ['min', 'max'],
+ coltypes: [GenericDataType.Numeric, GenericDataType.Numeric],
+ data: [{ min: 0, max: 10 }],
+ applied_filters: [],
+ rejected_filters: [],
+ },
+ ],
+ filterState: { value: [null, null] },
+ };
+ getWrapper(decimalProps);
+
+ const inputs = screen.getAllByRole('spinbutton');
+ const fromInput = inputs[0];
+
+ userEvent.clear(fromInput);
+ userEvent.type(fromInput, '2.5');
+ userEvent.tab();
Review Comment:
**Suggestion:** The test that types a decimal value via the keyboard calls
the asynchronous `userEvent.clear`, `userEvent.type`, and `userEvent.tab`
functions without `await`, so the expectation on `setDataMask` can run before
these interactions complete, making the test timing-dependent and potentially
flaky as `@testing-library/user-event` v12 APIs are promise-based. [possible
bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Flaky CI test failures for RangeFilterPlugin test.
- ⚠️ Slows development due to intermittent test reruns.
- ⚠️ Reduces confidence in decimal input handling tests.
```
</details>
```suggestion
await userEvent.clear(fromInput);
await userEvent.type(fromInput, '2.5');
await userEvent.tab();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file at
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx
and locate the
keyboard-input test starting at line 416 (`it('should allow decimal input via
keyboard'...)`). The test renders the component via `getWrapper()` and
selects the first
spinbutton at lines 426-433.
2. The test performs user interactions using @testing-library/user-event at
lines 434-436:
`userEvent.clear(fromInput);`, `userEvent.type(fromInput, '2.5');`, and
`userEvent.tab();`
— these calls return Promises in user-event v12+ but are not awaited in the
current code.
3. Immediately after the interactions, the test asserts that `setDataMask`
was called with
the decimal value (expectation at lines 438-445). Because the user-event
promises were not
awaited, the assertion can run before the asynchronous DOM events and
associated component
handlers complete, causing the test to intermittently fail in local runs or
CI.
4. Reproduce locally: run the single test (npm test --
RangeFilterPlugin.test.tsx)
repeatedly or under CI. On environments with scheduling variance, observe
intermittent
failures where the expectation that `setDataMask` was called with value
[2.5, null] is not
met because the typed input event handlers had not yet fired.
5. Confirm fix by changing the three userEvent calls to `await
userEvent.clear(...)`,
`await userEvent.type(...)`, and `await userEvent.tab()` (as shown in the
improved code).
Rerun the flaky test repeatedly; failures should disappear because the test
now waits for
the user-event promises to settle before asserting.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/filters/components/Range/RangeFilterPlugin.test.tsx
**Line:** 435:437
**Comment:**
*Possible Bug: The test that types a decimal value via the keyboard
calls the asynchronous `userEvent.clear`, `userEvent.type`, and `userEvent.tab`
functions without `await`, so the expectation on `setDataMask` can run before
these interactions complete, making the test timing-dependent and potentially
flaky as `@testing-library/user-event` v12 APIs are promise-based.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx:
##########
@@ -234,6 +234,40 @@ export default function RangeFilterPlugin(props:
PluginFilterRangeProps) {
const [row] = data;
// @ts-ignore
const { min, max }: { min: number; max: number } = row;
+
+ // Calculate appropriate step size for decimal values
+ const calculateStep = useCallback((minValue: number, maxValue: number) => {
+ const range = maxValue - minValue;
+
+ // If the range is very small (less than 1), use smaller steps
+ if (range < 1) {
+ // Find the number of decimal places needed
+ const rangeStr = range.toString();
+ const decimalMatch = rangeStr.match(/\.(\d+)/);
+ if (decimalMatch) {
+ const decimalPlaces = decimalMatch[1].length;
Review Comment:
**Suggestion:** The decimal step calculation for small ranges relies on
`range.toString()` and parsing the resulting string, which breaks for typical
floating-point cases (e.g., `0.07 - 0.05` → `0.020000000000000004`) and for
values rendered in scientific notation, producing either far too many steps or
falling back to `0.01`—both of which violate the "~100 steps" intent and can
make the slider effectively unusable for some decimal ranges. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Slider resolution incorrect for common float inputs.
- ⚠️ UX confusing for decimal-valued columns.
- ⚠️ Affects renderSlider and user interactions.
```
</details>
```suggestion
// Find the number of decimal places based on input values rather than
the range
const minDecimals = (minValue.toString().split('.')[1] || '').length;
const maxDecimals = (maxValue.toString().split('.')[1] || '').length;
const decimalPlaces = Math.max(minDecimals, maxDecimals);
if (decimalPlaces > 0) {
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use a dataset where min and max have decimal representations that produce
floating
point artifacts (e.g., min=0.05, max=0.07 -> range computed as
0.020000000000000004).
calculateStep at lines ~238-264 calls range.toString() and extracts decimals
(line 245),
so the code may count many decimals because of floating-point representation.
2. On render, this inflated decimalPlaces leads to a step like
10^-(decimalPlaces+1) (line
250), which can be much smaller than intended (excessively fine step) or if
parsing fails
falls back to 0.01 (line 252), both violating the "≈100 steps" objective.
3. Interact with slider (renderSlider uses step={sliderStep} at lines
~585/600). Observe
either near-infinite slider resolution (tiny steps) that is impractical, or
coarse
fallback (0.01) that cannot represent actual input values—both break
expected UX.
4. Verify by creating tests or manually logging sliderStep computed in
calculateStep to
confirm mismatch with intended ~100 increments.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx
**Line:** 244:248
**Comment:**
*Logic Error: The decimal step calculation for small ranges relies on
`range.toString()` and parsing the resulting string, which breaks for typical
floating-point cases (e.g., `0.07 - 0.05` → `0.020000000000000004`) and for
values rendered in scientific notation, producing either far too many steps or
falling back to `0.01`—both of which violate the "~100 steps" intent and can
make the slider effectively unusable for some decimal ranges.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
