bito-code-review[bot] commented on code in PR #34742:
URL: https://github.com/apache/superset/pull/34742#discussion_r2771970622
##########
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;
+ // Use a step that gives approximately 100 steps across the range
+ return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
+ }
+ return 0.01;
+ }
+
+ // For larger ranges, calculate step to give approximately 100-1000 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent Step Calculation</b></div>
<div id="fix">
The calculateStep function has inconsistent logic for small vs large ranges,
leading to incorrect step sizes for small decimal ranges. The special case for
range < 1 does not achieve the stated goal of ~100 steps.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
- 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;
- // Use a step that gives approximately 100 steps across the range
- return Math.pow(10, -Math.min(decimalPlaces + 1, 6));
- }
- return 0.01;
- }
-
- // For larger ranges, calculate step to give approximately 100-1000
steps
- const idealSteps = 100;
- let step = range / idealSteps;
-
- // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
- const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
- step = Math.round(step / magnitude) * magnitude;
-
- return step;
- }, []);
+ const calculateStep = useCallback((minValue: number, maxValue: number)
=> {
+ const range = maxValue - minValue;
+ if (range <= 0) return 0.01;
+
+ // For all ranges, calculate step to give approximately 100 steps
+ const idealSteps = 100;
+ let step = range / idealSteps;
+
+ // Round step to a nice value (0.001, 0.01, 0.1, 1, 10, etc.)
+ const magnitude = Math.pow(10, Math.floor(Math.log10(step)));
+ step = Math.round(step / magnitude) * magnitude;
+
+ return step;
+ }, []);
```
</div>
</details>
</div>
<small><i>Code Review Run #654c72</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
