codeant-ai-for-open-source[bot] commented on code in PR #36962:
URL: https://github.com/apache/superset/pull/36962#discussion_r2728817138
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx:
##########
@@ -232,6 +252,50 @@ 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;
Review Comment:
**Suggestion:** Logic bug: MIN_POINT_RADIUS is a fixed 5px while
MAX_POINT_RADIUS is computed as `radius / 3`. When `dotRadius` (i.e. `radius`)
is small (the default `dotRadius` is 4), `MAX_POINT_RADIUS` becomes smaller
than `MIN_POINT_RADIUS` which produces a negative range and incorrect point
radii. Ensure `MIN_POINT_RADIUS` does not exceed `MAX_POINT_RADIUS` (or compute
MIN relative to MAX) so normalization/clamping uses a valid range. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Map point sizes incorrect in Pixels mode.
- ⚠️ Point Radius UI (Pixels) yields misleading visualizations.
```
</details>
```suggestion
const MAX_POINT_RADIUS = Math.max(radius / 3, 1);
const MIN_POINT_RADIUS = Math.min(5, MAX_POINT_RADIUS);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open a MapBox chart that uses the ScatterPlotGlowOverlay component:
- File:
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.jsx
- Function: redraw, context lines approx. 134-166 (min/max computation
occurs at
148-166).
2. Ensure Point Radius Unit = "Pixels" in the chart controls and choose a
numeric column
for the Point Radius field so that location.properties.radius is
populated.
3. Use the default dotRadius (defaultProps dotRadius = 4 found at
ScatterPlotGlowOverlay.jsx:... defaultProps),
which sets radius = 4 (see redraw usage at line ~134). During redraw the
code sets:
MIN_POINT_RADIUS = 5 (line 258) and MAX_POINT_RADIUS = radius / 3 =
1.333... (line
259).
4. Observe rendering results:
- Because MIN_POINT_RADIUS > MAX_POINT_RADIUS the "all-equal" branch that
computes
(MIN_POINT_RADIUS + MAX_POINT_RADIUS) / 2 yields a size smaller than
MIN_POINT_RADIUS,
producing inconsistent/surprisingly small point radii (points can
appear tiny or
disappear).
- The mis-sized points are produced during Canvas drawing at the arc call
(roundDecimal(pointRadius, 1)).
```
</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:** 258:259
**Comment:**
*Logic Error: Logic bug: MIN_POINT_RADIUS is a fixed 5px while
MAX_POINT_RADIUS is computed as `radius / 3`. When `dotRadius` (i.e. `radius`)
is small (the default `dotRadius` is 4), `MAX_POINT_RADIUS` becomes smaller
than `MIN_POINT_RADIUS` which produces a negative range and incorrect point
radii. Ensure `MIN_POINT_RADIUS` does not exceed `MAX_POINT_RADIUS` (or compute
MIN relative to MAX) so normalization/clamping uses a valid range.
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]
