korbit-ai[bot] commented on code in PR #34858:
URL: https://github.com/apache/superset/pull/34858#discussion_r2301505043
##########
superset-frontend/src/explore/components/controls/SelectControl.jsx:
##########
@@ -87,6 +87,56 @@ const defaultProps = {
valueKey: 'value',
};
+const numberComparator = (a, b) => {
+ const aVal = typeof a.value === 'number' ? a.value : parseFloat(a.value);
Review Comment:
### Unsafe numeric conversion in sorting comparator <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The numberComparator doesn't handle invalid numeric strings or NaN values,
which could lead to incorrect sorting or runtime errors.
###### Why this matters
If a.value or b.value contains a string that cannot be converted to a number
(e.g., 'abc'), parseFloat will return NaN, causing unpredictable sorting
behavior.
###### Suggested change ∙ *Feature Preview*
Add validation to handle invalid numeric values:
```javascript
const numberComparator = (a, b) => {
const aVal = typeof a.value === 'number' ? a.value : Number(a.value);
const bVal = typeof b.value === 'number' ? b.value : Number(b.value);
if (Number.isNaN(aVal) || Number.isNaN(bVal)) return 0;
return aVal - bVal;
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b4867f77-62c9-40c8-aabb-b9348d52d49d/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b4867f77-62c9-40c8-aabb-b9348d52d49d?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b4867f77-62c9-40c8-aabb-b9348d52d49d?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b4867f77-62c9-40c8-aabb-b9348d52d49d?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/b4867f77-62c9-40c8-aabb-b9348d52d49d)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:b2a10306-582f-40ca-83c2-3019db98e174 -->
[](b2a10306-582f-40ca-83c2-3019db98e174)
##########
superset-frontend/src/explore/components/controls/SelectControl.jsx:
##########
@@ -87,6 +87,56 @@
valueKey: 'value',
};
+const numberComparator = (a, b) => {
+ const aVal = typeof a.value === 'number' ? a.value : parseFloat(a.value);
+ const bVal = typeof b.value === 'number' ? b.value : parseFloat(b.value);
+ return aVal - bVal;
+};
+
+export const areAllChoiceValuesNumbers = (choices, valueKey = 'value') => {
+ if (!choices || choices.length === 0) {
+ return false;
+ }
+ return choices.every(c => {
+ if (Array.isArray(c)) {
+ const [value] = c;
+ return typeof value === 'number';
+ }
+ if (typeof c === 'object' && c !== null) {
+ return typeof c[valueKey] === 'number';
+ }
+ return typeof c === 'number';
+ });
+};
+
+export const areAllOptionValuesNumbers = (options, valueKey = 'value') => {
+ if (!options || options.length === 0) {
+ return false;
+ }
+ return options.every(o => typeof o[valueKey] === 'number');
Review Comment:
### Missing null check in options validation <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The areAllOptionValuesNumbers function assumes the option object exists and
has the valueKey property, which could cause runtime errors.
###### Why this matters
If any option in the array is null, undefined, or doesn't have the specified
valueKey, accessing o[valueKey] will throw a TypeError.
###### Suggested change ∙ *Feature Preview*
Add null checks to prevent runtime errors:
```javascript
export const areAllOptionValuesNumbers = (options, valueKey = 'value') => {
if (!options || options.length === 0) {
return false;
}
return options.every(o => o && typeof o[valueKey] === 'number');
};
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2273c984-0d24-4d1f-91cd-9c357a98a5a9/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2273c984-0d24-4d1f-91cd-9c357a98a5a9?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2273c984-0d24-4d1f-91cd-9c357a98a5a9?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2273c984-0d24-4d1f-91cd-9c357a98a5a9?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2273c984-0d24-4d1f-91cd-9c357a98a5a9)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:d463c7b4-43b7-405a-980b-d0f55d7a2c71 -->
[](d463c7b4-43b7-405a-980b-d0f55d7a2c71)
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
