codeant-ai-for-open-source[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2910499390
########## superset-frontend/plugins/plugin-chart-supercluster-map/src/components/ScatterPlotOverlay.tsx: ########## @@ -0,0 +1,373 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { memo, useCallback } from 'react'; +import CanvasOverlay, { type RedrawParams } from './CanvasOverlay'; +import { kmToPixels, MILES_PER_KM } from '../utils/geo'; +import roundDecimal from '../utils/roundDecimal'; +import luminanceFromRGB from '../utils/luminanceFromRGB'; + +interface GeoJSONLocation { + geometry: { + coordinates: [number, number]; + }; + properties: Record<string, number | string | boolean | null | undefined>; +} + +interface DrawTextOptions { + fontHeight?: number; + label?: string | number; + radius?: number; + rgb?: (string | number)[]; + shadow?: boolean; +} + +interface ScatterPlotOverlayProps { + aggregation?: string; + compositeOperation?: string; + dotRadius?: number; + globalOpacity?: number; + lngLatAccessor?: (location: GeoJSONLocation) => [number, number]; + locations: GeoJSONLocation[]; + pointRadiusUnit?: string; + renderWhileDragging?: boolean; + rgb?: (string | number)[]; + zoom?: number; +} + +const IS_DARK_THRESHOLD = 110; + +const defaultLngLatAccessor = (location: GeoJSONLocation): [number, number] => [ + location.geometry.coordinates[0], + location.geometry.coordinates[1], +]; + +const computeClusterLabel = ( + properties: Record<string, number | string | boolean | null | undefined>, + aggregation: string | undefined, +): number | string => { + const count = properties.point_count as number; + if (!aggregation) { + return count; + } + if (aggregation === 'sum' || aggregation === 'min' || aggregation === 'max') { + return properties[aggregation] as number; + } + const { sum } = properties as { sum: number }; + const mean = sum / count; + if (aggregation === 'mean') { + return Math.round(100 * mean) / 100; + } + const { squaredSum } = properties as { squaredSum: number }; + const variance = squaredSum / count - (sum / count) ** 2; + if (aggregation === 'var') { + return Math.round(100 * variance) / 100; + } + if (aggregation === 'stdev') { + return Math.round(100 * Math.sqrt(variance)) / 100; + } + + // fallback to point_count + return count; Review Comment: **Suggestion:** The cluster label aggregator option for standard deviation is exposed as `"std"` in the control panel, but `computeClusterLabel` only handles `"stdev"`, so selecting `"std"` silently falls back to showing the point count instead of the standard deviation; extend the condition to accept `"std"` as well so the behavior matches the UI. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Cluster labels ignore selected std aggregator. - ⚠️ Supercluster map shows counts instead of variability. - ⚠️ Users may misinterpret cluster summaries statistically. ``` </details> ```suggestion const { squaredSum } = properties as { squaredSum: number }; const variance = squaredSum / count - (sum / count) ** 2; if (aggregation === 'var') { return Math.round(100 * variance) / 100; } if (aggregation === 'std' || aggregation === 'stdev') { return Math.round(100 * Math.sqrt(variance)) / 100; } // fallback to point_count return count; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In Explore, create or open a chart using the "Supercluster"/MapLibre scatter map plugin, whose controls are defined in `superset-frontend/plugins/plugin-chart-supercluster-map/src/controlPanel.ts:39-189` and whose transform logic is in `src/transformProps.ts:104-219`. 2. In the "Labelling" section of the control panel (`controlPanel.ts:143-189`), pick a numeric column in `map_label` (so `hasCustomMetric` becomes true in `transformProps.ts:132-134`) and set "Cluster label aggregator" (`pandas_aggfunc`) to `std` (choice defined at `controlPanel.ts:167-179`). 3. Run the query; `transformProps` passes `pandas_aggfunc` through as `aggregatorName` in the returned props (`transformProps.ts:124, 191-195`), `MapLibre` forwards it as the `aggregation` prop into `ScatterPlotOverlay` when `hasCustomMetric` is true (`MapLibre.tsx:72-76, 165-174`). 4. On render, `ScatterPlotOverlay` computes cluster labels in `computeClusterLabel` (`ScatterPlotOverlay.tsx:60-87`); since `aggregation` is `'std'`, it does not match the `'stdev'` branch (`line 81`) and falls through to the default `return count` (`line 86`), so the UI-selected "std" option incorrectly shows point counts instead of standard deviation. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-supercluster-map/src/components/ScatterPlotOverlay.tsx **Line:** 76:86 **Comment:** *Logic Error: The cluster label aggregator option for standard deviation is exposed as `"std"` in the control panel, but `computeClusterLabel` only handles `"stdev"`, so selecting `"std"` silently falls back to showing the point count instead of the standard deviation; extend the condition to accept `"std"` as well so the behavior matches the UI. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=37cc6ae7eaeff682daf79d43702154f1783f029daaac4986634a857a7ee129dc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=37cc6ae7eaeff682daf79d43702154f1783f029daaac4986634a857a7ee129dc&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-supercluster-map/src/transformProps.ts: ########## @@ -0,0 +1,219 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import Supercluster, { + type Options as SuperclusterOptions, +} from 'supercluster'; +import { ChartProps } from '@superset-ui/core'; +import { DEFAULT_POINT_RADIUS, DEFAULT_MAX_ZOOM } from './MapLibre'; +import roundDecimal from './utils/roundDecimal'; + +const NOOP = () => {}; + +// Geo precision to limit decimal places (matching legacy backend behavior) +const GEO_PRECISION = 10; + +interface PointProperties { + metric: number | string | null; + radius: number | string | null; +} + +interface ClusterProperties { + metric: number; + sum: number; + squaredSum: number; + min: number; + max: number; +} + +interface DataRecord { + [key: string]: string | number | null | undefined; +} + +function buildGeoJSONFromRecords( + records: DataRecord[], + lonCol: string, + latCol: string, + labelCol: string | null, + pointRadiusCol: string | null, +) { + const features: GeoJSON.Feature<GeoJSON.Point, PointProperties>[] = []; + let minLon = Infinity; + let maxLon = -Infinity; + let minLat = Infinity; + let maxLat = -Infinity; + + for (const record of records) { + const lon = Number(record[lonCol]); + const lat = Number(record[latCol]); + if (Number.isNaN(lon) || Number.isNaN(lat)) { + continue; + } + + const roundedLon = roundDecimal(lon, GEO_PRECISION); + const roundedLat = roundDecimal(lat, GEO_PRECISION); + + minLon = Math.min(minLon, roundedLon); + maxLon = Math.max(maxLon, roundedLon); + minLat = Math.min(minLat, roundedLat); + maxLat = Math.max(maxLat, roundedLat); + + const metric = labelCol != null ? (record[labelCol] ?? null) : null; + const radius = + pointRadiusCol != null ? (record[pointRadiusCol] ?? null) : null; + + features.push({ + type: 'Feature', + properties: { metric, radius }, + geometry: { + type: 'Point', + coordinates: [roundedLon, roundedLat], + }, + }); + } + + const bounds = + features.length > 0 + ? [ + [minLon, minLat], + [maxLon, maxLat], + ] + : undefined; + + return { + geoJSON: { type: 'FeatureCollection' as const, features }, + bounds, + }; +} + +export default function transformProps(chartProps: ChartProps) { + const { + width, + height, + rawFormData: formData, + hooks, + queriesData, + } = chartProps; + const { onError = NOOP, setControlValue = NOOP } = hooks; + + const { + all_columns_x: allColumnsX, + all_columns_y: allColumnsY, + clustering_radius: clusteringRadius, + global_opacity: globalOpacity, + map_color: maplibreColor, + map_label: maplibreLabel, + map_renderer: mapProvider, + maplibre_style: maplibreStyle, + mapbox_style: mapboxStyle = '', + pandas_aggfunc: pandasAggfunc, + point_radius: pointRadius, + point_radius_unit: pointRadiusUnit, + render_while_dragging: renderWhileDragging, + } = formData; + + // Build GeoJSON from raw query records + const records: DataRecord[] = queriesData[0]?.data || []; + const hasCustomMetric = maplibreLabel != null && maplibreLabel.length > 0; + const labelCol = hasCustomMetric ? maplibreLabel[0] : null; + const pointRadiusCol = + pointRadius && pointRadius !== 'Auto' ? pointRadius : null; + + const { geoJSON, bounds } = buildGeoJSONFromRecords( + records, + allColumnsX, + allColumnsY, + labelCol, + pointRadiusCol, + ); + + // Validate color — supports hex (#rrggbb) and rgb(r, g, b) formats + let rgb: string[] | null = null; + const hexMatch = /^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/i.exec( + maplibreColor, + ); + if (hexMatch) { + rgb = [ + maplibreColor, + String(parseInt(hexMatch[1], 16)), + String(parseInt(hexMatch[2], 16)), + String(parseInt(hexMatch[3], 16)), + ]; + } else { + rgb = /^rgb\((\d{1,3}),\s*(\d{1,3}),\s*(\d{1,3})\)$/.exec(maplibreColor); + } + if (rgb === null) { + onError("Color field must be a hex color (#rrggbb) or 'rgb(r, g, b)'"); + + return {}; Review Comment: **Suggestion:** When the color string is invalid this function returns an empty object, but the MapLibre React component still expects a valid `clusterer` prop and will later call `clusterer.getClusters`, causing a runtime error; instead, report the error and fall back to a safe default color while still returning a complete props object. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Supercluster Map visualization crashes for invalid color inputs. - ⚠️ Users see blank chart and frontend TypeError exceptions. - ⚠️ Error handling hook runs but chart still breaks. ``` </details> ```suggestion let rgb: string[] | null = null; const hexMatch = /^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/i.exec( maplibreColor, ); if (hexMatch) { rgb = [ maplibreColor, String(parseInt(hexMatch[1], 16)), String(parseInt(hexMatch[2], 16)), String(parseInt(hexMatch[3], 16)), ]; } else { rgb = /^rgb\((\d{1,3}),\s*(\d{1,3}),\s*(\d{1,3})\)$/.exec(maplibreColor); } if (rgb === null) { onError("Color field must be a hex color (#rrggbb) or 'rgb(r, g, b)'"); // Fall back to a safe default color so the chart can still render. rgb = ['#000000', '0', '0', '0']; } ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Configure a Supercluster Map chart using the plugin defined in `superset-frontend/plugins/plugin-chart-supercluster-map/src/index.ts:49-57` (class `ScatterMapChartPlugin`, which wires `loadChart: () => import('./MapLibre')` and `loadTransformProps: () => import('./transformProps')`). 2. In the chart's control panel (backed by `superset-frontend/plugins/plugin-chart-supercluster-map/src/controlPanel.ts`, which defines color controls for this plugin), set the map color (`map_color` form field) to an invalid value that does not match `#rrggbb` or `rgb(r, g, b)` (e.g. `blue` or `#1234`), then run or refresh the chart in Explore. 3. The chart rendering pipeline calls `transformProps(chartProps)` in `superset-frontend/plugins/plugin-chart-supercluster-map/src/transformProps.ts:104-219`; inside it, the color parsing block at lines 145-160 fails to match the regex, leaving `rgb` as `null`, so the `if (rgb === null)` branch at lines 160-163 executes, calls `onError(...)`, and returns an empty object `{}` instead of a props object with a `clusterer`. 4. The React chart component `MapLibre` in `superset-frontend/plugins/plugin-chart-supercluster-map/src/MapLibre.tsx:69-182` is then rendered with props derived from the transform result, so `clusterer` is `undefined`; when `const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom));` runs at line 128, it throws a runtime error (`TypeError: Cannot read properties of undefined (reading 'getClusters')`), causing the Supercluster Map visualization to fail to render. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/plugins/plugin-chart-supercluster-map/src/transformProps.ts **Line:** 146:163 **Comment:** *Logic Error: When the color string is invalid this function returns an empty object, but the MapLibre React component still expects a valid `clusterer` prop and will later call `clusterer.getClusters`, causing a runtime error; instead, report the error and fall back to a safe default color while still returning a complete props object. 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> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=c8d1dddde5ffcba5d74f961c9f010b5e3c67b42bc94287b0d0b8bd9d4ca7932c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=c8d1dddde5ffcba5d74f961c9f010b5e3c67b42bc94287b0d0b8bd9d4ca7932c&reaction=dislike'>👎</a> -- 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]
