codeant-ai-for-open-source[bot] commented on code in PR #36962:
URL: https://github.com/apache/superset/pull/36962#discussion_r2686696628
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx:
##########
@@ -143,6 +143,21 @@ class ScatterPlotGlowOverlay extends PureComponent {
const maxLabel = Math.max(...clusterLabelMap.filter(v =>
!Number.isNaN(v)));
+ // Calculate min/max radius values for Pixels mode scaling
+ let minRadiusValue = Infinity;
+ let maxRadiusValue = -Infinity;
+ if (pointRadiusUnit === 'Pixels') {
+ locations.forEach(location => {
+ if (!location.properties.cluster && location.properties.radius !==
null) {
+ const radiusValue = location.properties.radius;
Review Comment:
**Suggestion:** The pre-pass that computes min/max radius for Pixels mode
only accepts non-null numeric values and uses Number.isFinite directly on the
raw property; this will ignore numeric strings and will also skip undefined
values (because the code checks !== null). Coerce the property to Number and
treat both null and undefined as absent by checking != null, so numeric strings
like "5" are handled and undefined doesn't throw. [type error]
**Severity Level:** Minor ⚠️
```suggestion
// Accept both null and undefined as "no value" and coerce potential
numeric strings
if (!location.properties.cluster && location.properties.radius !=
null) {
const radiusValueRaw = location.properties.radius;
const radiusValue = Number(radiusValueRaw);
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing pre-pass ignores undefined (checks !== null) and will skip
numeric strings since Number.isFinite on a string returns false. Coercing with
Number(...) and checking != null is a sensible, minimal fix that prevents
incorrect min/max when values come as numeric strings from geoJSON and avoids
silently skipping undefined. This directly fixes a functional bug (wrong
scaling range).
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx
**Line:** 151:152
**Comment:**
*Type Error: The pre-pass that computes min/max radius for Pixels mode
only accepts non-null numeric values and uses Number.isFinite directly on the
raw property; this will ignore numeric strings and will also skip undefined
values (because the code checks !== null). Coerce the property to Number and
treat both null and undefined as absent by checking != null, so numeric strings
like "5" are handled and undefined doesn't throw.
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/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx:
##########
@@ -230,6 +245,36 @@ class ScatterPlotGlowOverlay extends PureComponent {
pointLatitude,
zoom,
);
+ } else if (pointRadiusUnit === 'Pixels') {
+ // Scale pixel values to a reasonable range (5px to radius/3)
+ // This ensures points are visible and proportional to their
values
+ const MIN_POINT_RADIUS = 5;
+ const MAX_POINT_RADIUS = radius / 3;
+
+ if (
+ Number.isFinite(minRadiusValue) &&
+ Number.isFinite(maxRadiusValue) &&
+ maxRadiusValue > minRadiusValue
+ ) {
+ // Normalize the value to 0-1 range, then scale to pixel
range
+ const normalizedValue =
+ (pointRadius - minRadiusValue) / (maxRadiusValue -
minRadiusValue);
+ pointRadius =
+ MIN_POINT_RADIUS +
+ normalizedValue * (MAX_POINT_RADIUS - MIN_POINT_RADIUS);
Review Comment:
**Suggestion:** Normalization calculation does not explicitly coerce
`pointRadius` to a Number before arithmetic; if `pointRadius` is a non-numeric
string or other non-finite value, the normalization will produce NaN and
propagate to final radius. Coerce to Number and fall back to a sensible default
when non-finite. [type error]
**Severity Level:** Minor ⚠️
```suggestion
const numericPointRadius = Number(pointRadius);
if (!Number.isFinite(numericPointRadius)) {
// fallback to minimum visible size when the value is
not a finite number
pointRadius = MIN_POINT_RADIUS;
} else {
const normalizedValueRaw =
(numericPointRadius - minRadiusValue) /
(maxRadiusValue - minRadiusValue);
const normalizedValue = Math.max(0, Math.min(1,
normalizedValueRaw));
pointRadius =
MIN_POINT_RADIUS +
normalizedValue * (MAX_POINT_RADIUS -
MIN_POINT_RADIUS);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Coercing pointRadius to Number and guarding when it's non-finite avoids NaN
leaking into canvas draw sizes. This is a focused, low-risk fix that prevents
runtime math errors when radius values are strings or otherwise malformed.
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx
**Line:** 261:264
**Comment:**
*Type Error: Normalization calculation does not explicitly coerce
`pointRadius` to a Number before arithmetic; if `pointRadius` is a non-numeric
string or other non-finite value, the normalization will produce NaN and
propagate to final radius. Coerce to Number and fall back to a sensible default
when non-finite.
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]
