codeant-ai-for-open-source[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2974939306
########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/MapLibre.tsx: ########## @@ -0,0 +1,216 @@ +/** + * 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, useEffect, useState } from 'react'; +import { Map as MapLibreMap } from 'react-map-gl/maplibre'; +import { Map as MapboxMap } from 'react-map-gl/mapbox'; +import { WebMercatorViewport } from '@math.gl/web-mercator'; +import { useTheme } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core/translation'; +import ScatterPlotOverlay from './components/ScatterPlotOverlay'; +import { getMapboxApiKey } from './utils/mapbox'; +import 'maplibre-gl/dist/maplibre-gl.css'; +import './MapLibre.css'; + +const DEFAULT_MAP_STYLE = 'https://tiles.openfreemap.org/styles/liberty'; + +export const DEFAULT_MAX_ZOOM = 16; +export const DEFAULT_POINT_RADIUS = 60; + +interface Viewport { + longitude: number; + latitude: number; + zoom: number; +} + +interface Clusterer { + getClusters(bbox: number[], zoom: number): GeoJSONLocation[]; +} + +interface GeoJSONLocation { + geometry: { + coordinates: [number, number]; + }; + properties: Record<string, number | string | boolean | null | undefined>; +} + +interface MapLibreProps { + width?: number; + height?: number; + aggregatorName?: string; + clusterer: Clusterer; // Required - used for getClusters() + globalOpacity?: number; + hasCustomMetric?: boolean; + mapProvider?: string; + mapStyle?: string; + onViewportChange?: (viewport: Viewport) => void; + pointRadius?: number; + pointRadiusUnit?: string; + renderWhileDragging?: boolean; + rgb?: (string | number)[]; + bounds?: [[number, number], [number, number]]; // May be undefined for empty datasets + viewportLongitude?: number; + viewportLatitude?: number; + viewportZoom?: number; +} + +function MapLibre({ + width = 400, + height = 400, + aggregatorName, + clusterer, + globalOpacity = 1, + hasCustomMetric, + mapProvider, + mapStyle, + onViewportChange, + pointRadius = DEFAULT_POINT_RADIUS, + pointRadiusUnit = 'Pixels', + renderWhileDragging = true, + rgb, + bounds, + viewportLongitude, + viewportLatitude, + viewportZoom, +}: MapLibreProps) { + const computeFitBounds = useCallback((): Viewport => { + if (bounds && bounds[0] && bounds[1]) { + const mercator = new WebMercatorViewport({ width, height }).fitBounds( + bounds, + ); + return { + latitude: mercator.latitude, + longitude: mercator.longitude, + zoom: mercator.zoom, + }; + } + return { latitude: 0, longitude: 0, zoom: 1 }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + const mergeViewportWithProps = useCallback( + (fitBounds: Viewport, base: Viewport = fitBounds): Viewport => ({ + ...base, + longitude: viewportLongitude ?? fitBounds.longitude, + latitude: viewportLatitude ?? fitBounds.latitude, + zoom: viewportZoom ?? fitBounds.zoom, + }), + [viewportLongitude, viewportLatitude, viewportZoom], + ); + + const [viewport, setViewport] = useState<Viewport>(() => + mergeViewportWithProps(computeFitBounds()), + ); + + useEffect(() => { + const fitBounds = computeFitBounds(); + const next = mergeViewportWithProps(fitBounds, viewport); + if ( + next.longitude !== viewport.longitude || + next.latitude !== viewport.latitude || + next.zoom !== viewport.zoom + ) { + setViewport(next); + } + // Only re-run when the viewport-override props change + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [viewportLongitude, viewportLatitude, viewportZoom]); Review Comment: **Suggestion:** The viewport-fit logic is memoized with empty dependencies and the sync effect only listens to explicit viewport override props, so changes to `bounds`, `width`, or `height` do not recenter/rezoom the map. This causes stale viewport state after data/filter/resize updates. Recompute fit bounds when bounds/size change and rerun the viewport sync effect based on those dependencies. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Filtered datasets may not recenter map automatically. - ⚠️ Resize can keep outdated zoom/center values. - ❌ Viewport and data extent can become visually inconsistent. ``` </details> ```suggestion }, [bounds, width, height]); const mergeViewportWithProps = useCallback( (fitBounds: Viewport, base: Viewport = fitBounds): Viewport => ({ ...base, longitude: viewportLongitude ?? fitBounds.longitude, latitude: viewportLatitude ?? fitBounds.latitude, zoom: viewportZoom ?? fitBounds.zoom, }), [viewportLongitude, viewportLatitude, viewportZoom], ); const [viewport, setViewport] = useState<Viewport>(() => mergeViewportWithProps(computeFitBounds()), ); useEffect(() => { const fitBounds = computeFitBounds(); setViewport(current => { const next = mergeViewportWithProps(fitBounds, current); if ( next.longitude === current.longitude && next.latitude === current.latitude && next.zoom === current.zoom ) { return current; } return next; }); }, [computeFitBounds, mergeViewportWithProps]); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Load the chart through plugin entrypoint `superset-frontend/plugins/plugin-chart-point-cluster-map/src/index.ts:52-54`, which wires `MapLibre.tsx` and `transformProps.ts`. 2. Trigger a data refresh scenario (e.g., chart filters) so `transformProps()` recomputes and returns new `bounds` from `queriesData` at `transformProps.ts:167-198` and passes `bounds` at `transformProps.ts:250`. 3. In `MapLibre.tsx`, `computeFitBounds` is memoized with empty deps at `MapLibre.tsx:91-104`, and the sync effect runs only on viewport override props at `MapLibre.tsx:120-132`. 4. Re-render with changed `bounds` (or `width/height`) but unchanged `viewportLongitude/viewportLatitude/viewportZoom`; map center/zoom remains stale because fit-bounds logic is not re-applied. ``` </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-point-cluster-map/src/MapLibre.tsx **Line:** 103:132 **Comment:** *Logic Error: The viewport-fit logic is memoized with empty dependencies and the sync effect only listens to explicit viewport override props, so changes to `bounds`, `width`, or `height` do not recenter/rezoom the map. This causes stale viewport state after data/filter/resize updates. Recompute fit bounds when bounds/size change and rerun the viewport sync effect based on those dependencies. 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=9d14dfd93dbccb598d3f395a154c56640ed7f6bb9fcd8eb9770428579d73ff0a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=9d14dfd93dbccb598d3f395a154c56640ed7f6bb9fcd8eb9770428579d73ff0a&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts: ########## @@ -0,0 +1,292 @@ +/** + * 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 { t } from '@apache-superset/core/translation'; +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; + +const MIN_LONGITUDE = -180; +const MAX_LONGITUDE = 180; +const MIN_LATITUDE = -90; +const MAX_LATITUDE = 90; +const MIN_ZOOM = 0; + +function toFiniteNumber( + value: string | number | null | undefined, +): number | undefined { + if (value === null || value === undefined) return undefined; + const normalizedValue = typeof value === 'string' ? value.trim() : value; + if (normalizedValue === '') return undefined; + const num = Number(normalizedValue); + return Number.isFinite(num) ? num : undefined; +} + +function clampNumber( + value: number | undefined, + min: number, + max: number, +): number | undefined { + if (value === undefined) return undefined; + return Math.min(max, Math.max(min, value)); +} + +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 rawLon = record[lonCol]; + const rawLat = record[latCol]; + if (rawLon == null || rawLat == null) { + continue; + } + const lon = Number(rawLon); + const lat = Number(rawLat); + if (!Number.isFinite(lon) || !Number.isFinite(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: [[number, number], [number, number]] | undefined = + 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, + viewport_longitude: viewportLongitude, + viewport_latitude: viewportLatitude, + viewport_zoom: viewportZoom, + } = formData; + + // Support two data formats: + // 1. Legacy/GeoJSON: queriesData[0].data is an object with { geoJSON, bounds, hasCustomMetric } + // 2. Tabular records: queriesData[0].data is an array of flat records from a SQL query + const rawData = queriesData[0]?.data; + const isLegacyFormat = rawData && !Array.isArray(rawData) && rawData.geoJSON; + + let geoJSON: { type: 'FeatureCollection'; features: any[] }; + let bounds: [[number, number], [number, number]] | undefined; + let hasCustomMetric: boolean; + + if (isLegacyFormat) { + const legacy = rawData as any; + ({ geoJSON } = legacy); + ({ bounds } = legacy); + hasCustomMetric = legacy.hasCustomMetric ?? false; + } else { + const records: DataRecord[] = (rawData as DataRecord[]) || []; + hasCustomMetric = + maplibreLabel != null && + maplibreLabel.length > 0 && + maplibreLabel[0] !== 'count'; Review Comment: **Suggestion:** `count` is currently treated as "no custom metric", which changes legacy behavior for grouped datasets: cluster labels fall back to point-count instead of aggregating the selected label metric. This breaks compatibility for charts that rely on `map_label = ['count']`. Treat any non-empty label selection as a custom metric so cluster aggregation stays consistent. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Point cluster labels wrong for grouped `count` charts. - ❌ Migration parity with legacy map behavior regresses. ``` </details> ```suggestion hasCustomMetric = maplibreLabel != null && maplibreLabel.length > 0; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open Explore with viz type `point_cluster_map` (registered in `superset-frontend/src/visualizations/presets/MainPreset.ts:134`) and set a grouped query. 2. In chart controls, pick label `count` (documented semantics at `superset-frontend/plugins/plugin-chart-point-cluster-map/src/controlPanel.ts:154-157`). 3. `transformProps()` (`.../transformProps.ts:181-185`) marks `hasCustomMetric` false when `map_label[0] === 'count'`. 4. Rendering path `MapLibre.tsx:205` sends `aggregation=undefined`, so `computeClusterLabel()` in `components/ScatterPlotOverlay.tsx:69-70` falls back to `point_count` instead of aggregating the selected metric; cluster labels differ from legacy backend behavior (`superset/superset/viz.py:1458-1510`, where non-empty label sets `has_custom_metric=True`). ``` </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-point-cluster-map/src/transformProps.ts **Line:** 181:184 **Comment:** *Logic Error: `count` is currently treated as "no custom metric", which changes legacy behavior for grouped datasets: cluster labels fall back to point-count instead of aggregating the selected label metric. This breaks compatibility for charts that rely on `map_label = ['count']`. Treat any non-empty label selection as a custom metric so cluster aggregation stays consistent. 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=c229ed3f5bce5115e94c9f0dbe4815012b0f1d4b739cbed7dadd2bdade35026c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=c229ed3f5bce5115e94c9f0dbe4815012b0f1d4b739cbed7dadd2bdade35026c&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/components/ScatterPlotOverlay.tsx: ########## @@ -0,0 +1,400 @@ +/** + * 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'; + +// Shared radius bounds keep cluster and point sizing in sync. +export const MIN_CLUSTER_RADIUS_RATIO = 1 / 6; +export const MAX_POINT_RADIUS_RATIO = 1 / 3; + +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 === 'std' || aggregation === 'stdev') { + return Math.round(100 * Math.sqrt(variance)) / 100; + } + + // fallback to point_count + return count; +}; + +function drawText( + ctx: CanvasRenderingContext2D, + pixel: [number, number], + compositeOperation: string, + options: DrawTextOptions = {}, +) { + const { + fontHeight = 0, + label = '', + radius = 0, + rgb = [0, 0, 0], + shadow = false, + } = options; + const maxWidth = radius * 1.8; + const luminance = luminanceFromRGB( + rgb[1] as number, + rgb[2] as number, + rgb[3] as number, + ); + + ctx.globalCompositeOperation = 'source-over'; + ctx.fillStyle = luminance <= IS_DARK_THRESHOLD ? 'white' : 'black'; + ctx.font = `${fontHeight}px sans-serif`; + ctx.textAlign = 'center'; + ctx.textBaseline = 'middle'; + if (shadow) { + ctx.shadowBlur = 15; + ctx.shadowColor = luminance <= IS_DARK_THRESHOLD ? 'black' : ''; + } + + const textWidth = ctx.measureText(String(label)).width; + if (textWidth > maxWidth) { + const scale = fontHeight / textWidth; + ctx.font = `${scale * maxWidth}px sans-serif`; + } + + ctx.fillText(String(label), pixel[0], pixel[1]); + ctx.globalCompositeOperation = compositeOperation as GlobalCompositeOperation; + ctx.shadowBlur = 0; + ctx.shadowColor = ''; +} + +function ScatterPlotOverlay({ + aggregation, + compositeOperation = 'source-over', + dotRadius = 4, + globalOpacity = 1, + lngLatAccessor = defaultLngLatAccessor, + locations, + pointRadiusUnit, + renderWhileDragging = true, + rgb, + zoom, +}: ScatterPlotOverlayProps) { + const redraw = useCallback( + ({ width, height, ctx, isDragging, project }: RedrawParams) => { + const radius = dotRadius; + const clusterLabelMap: (number | string)[] = []; + + locations.forEach((location, i) => { + if (location.properties.cluster) { + clusterLabelMap[i] = computeClusterLabel( + location.properties, + aggregation, + ); + } + }); + + const finiteClusterLabels = clusterLabelMap + .map(value => Number(value)) + .filter(value => Number.isFinite(value)); + const safeMaxAbsLabel = + finiteClusterLabels.length > 0 + ? Math.max( + Math.max(...finiteClusterLabels.map(value => Math.abs(value))), + 1, + ) + : 1; + + // 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 radiusValueRaw = location.properties.radius; + const radiusValue = + typeof radiusValueRaw === 'string' && radiusValueRaw.trim() === '' + ? null + : Number(radiusValueRaw); + if (radiusValue != null && Number.isFinite(radiusValue)) { + minRadiusValue = Math.min(minRadiusValue, radiusValue); + maxRadiusValue = Math.max(maxRadiusValue, radiusValue); + } + } + }); + } + + ctx.clearRect(0, 0, width, height); + ctx.globalCompositeOperation = + compositeOperation as GlobalCompositeOperation; + + if ((renderWhileDragging || !isDragging) && locations) { + locations.forEach((location: GeoJSONLocation, i: number) => { + const pixel = project(lngLatAccessor(location)); + const pixelRounded: [number, number] = [ + roundDecimal(pixel[0], 1), + roundDecimal(pixel[1], 1), + ]; + + if ( + pixelRounded[0] + radius >= 0 && + pixelRounded[0] - radius < width && + pixelRounded[1] + radius >= 0 && + pixelRounded[1] - radius < height Review Comment: **Suggestion:** Visibility culling always uses the base `dotRadius`, but actual rendered radii in kilometer/mile units can be much larger, so partially visible large circles near edges are incorrectly skipped. Disable this small-radius culling for geographic radius units (or use actual computed radius) to avoid dropping visible points. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Edge-of-map large circles disappear unexpectedly. - ⚠️ Spatial density appears lower near viewport boundaries. ``` </details> ```suggestion const visibilityRadius = pointRadiusUnit === 'Kilometers' || pointRadiusUnit === 'Miles' ? Infinity : radius; if ( pixelRounded[0] + visibilityRadius >= 0 && pixelRounded[0] - visibilityRadius < width && pixelRounded[1] + visibilityRadius >= 0 && pixelRounded[1] - visibilityRadius < height ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Use `point_cluster_map` (registered in `MainPreset.ts:134`) with `point_radius_unit` set to `Kilometers`/`Miles` (`controlPanel.ts:124-133`) and a large radius column. 2. Render path: `MapLibre.tsx:197-207` passes locations to `ScatterPlotOverlay` with redraw from `CanvasOverlay.tsx:72-78` on move/drag. 3. `ScatterPlotOverlay.tsx:206-211` culls points using fixed `radius` (`dotRadius`, default `60` from `MapLibre.tsx:33,82`) before true geo-radius conversion is applied. 4. For points whose center is just outside viewport but converted radius is large (`ScatterPlotOverlay.tsx:293-306`), culling skips drawing entirely, so circles that should be partially visible never 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-point-cluster-map/src/components/ScatterPlotOverlay.tsx **Line:** 206:210 **Comment:** *Logic Error: Visibility culling always uses the base `dotRadius`, but actual rendered radii in kilometer/mile units can be much larger, so partially visible large circles near edges are incorrectly skipped. Disable this small-radius culling for geographic radius units (or use actual computed radius) to avoid dropping visible points. 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=230869d195e4b2e04a3521397adfe0f0e5e17d82f2d00f09a018ee2d05709cb3&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=230869d195e4b2e04a3521397adfe0f0e5e17d82f2d00f09a018ee2d05709cb3&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/components/ScatterPlotOverlay.tsx: ########## @@ -0,0 +1,400 @@ +/** + * 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'; + +// Shared radius bounds keep cluster and point sizing in sync. +export const MIN_CLUSTER_RADIUS_RATIO = 1 / 6; +export const MAX_POINT_RADIUS_RATIO = 1 / 3; + +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 === 'std' || aggregation === 'stdev') { + return Math.round(100 * Math.sqrt(variance)) / 100; + } + + // fallback to point_count + return count; +}; + +function drawText( + ctx: CanvasRenderingContext2D, + pixel: [number, number], + compositeOperation: string, + options: DrawTextOptions = {}, +) { + const { + fontHeight = 0, + label = '', + radius = 0, + rgb = [0, 0, 0], + shadow = false, + } = options; + const maxWidth = radius * 1.8; + const luminance = luminanceFromRGB( + rgb[1] as number, + rgb[2] as number, + rgb[3] as number, + ); + + ctx.globalCompositeOperation = 'source-over'; + ctx.fillStyle = luminance <= IS_DARK_THRESHOLD ? 'white' : 'black'; + ctx.font = `${fontHeight}px sans-serif`; + ctx.textAlign = 'center'; + ctx.textBaseline = 'middle'; + if (shadow) { + ctx.shadowBlur = 15; + ctx.shadowColor = luminance <= IS_DARK_THRESHOLD ? 'black' : ''; + } + + const textWidth = ctx.measureText(String(label)).width; + if (textWidth > maxWidth) { + const scale = fontHeight / textWidth; + ctx.font = `${scale * maxWidth}px sans-serif`; + } + + ctx.fillText(String(label), pixel[0], pixel[1]); + ctx.globalCompositeOperation = compositeOperation as GlobalCompositeOperation; + ctx.shadowBlur = 0; + ctx.shadowColor = ''; +} + +function ScatterPlotOverlay({ + aggregation, + compositeOperation = 'source-over', + dotRadius = 4, + globalOpacity = 1, + lngLatAccessor = defaultLngLatAccessor, + locations, + pointRadiusUnit, + renderWhileDragging = true, + rgb, + zoom, +}: ScatterPlotOverlayProps) { + const redraw = useCallback( + ({ width, height, ctx, isDragging, project }: RedrawParams) => { + const radius = dotRadius; + const clusterLabelMap: (number | string)[] = []; + + locations.forEach((location, i) => { + if (location.properties.cluster) { + clusterLabelMap[i] = computeClusterLabel( + location.properties, + aggregation, + ); + } + }); + + const finiteClusterLabels = clusterLabelMap + .map(value => Number(value)) + .filter(value => Number.isFinite(value)); + const safeMaxAbsLabel = + finiteClusterLabels.length > 0 + ? Math.max( + Math.max(...finiteClusterLabels.map(value => Math.abs(value))), + 1, + ) + : 1; + + // 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 radiusValueRaw = location.properties.radius; + const radiusValue = + typeof radiusValueRaw === 'string' && radiusValueRaw.trim() === '' + ? null + : Number(radiusValueRaw); + if (radiusValue != null && Number.isFinite(radiusValue)) { + minRadiusValue = Math.min(minRadiusValue, radiusValue); + maxRadiusValue = Math.max(maxRadiusValue, radiusValue); + } + } + }); + } + + ctx.clearRect(0, 0, width, height); + ctx.globalCompositeOperation = + compositeOperation as GlobalCompositeOperation; + + if ((renderWhileDragging || !isDragging) && locations) { + locations.forEach((location: GeoJSONLocation, i: number) => { + const pixel = project(lngLatAccessor(location)); + const pixelRounded: [number, number] = [ + roundDecimal(pixel[0], 1), + roundDecimal(pixel[1], 1), + ]; + + if ( + pixelRounded[0] + radius >= 0 && + pixelRounded[0] - radius < width && + pixelRounded[1] + radius >= 0 && + pixelRounded[1] - radius < height + ) { + ctx.beginPath(); + if (location.properties.cluster) { + const clusterLabel = clusterLabelMap[i]; + const numericLabel = Number(clusterLabel); + const safeNumericLabel = Number.isFinite(numericLabel) + ? numericLabel + : 0; + const minClusterRadius = + pointRadiusUnit === 'Pixels' + ? radius * MAX_POINT_RADIUS_RATIO + : radius * MIN_CLUSTER_RADIUS_RATIO; + const ratio = Math.abs(safeNumericLabel) / safeMaxAbsLabel; + const scaledRadius = roundDecimal( + minClusterRadius + ratio ** 0.5 * (radius - minClusterRadius), + 1, + ); + const fontHeight = roundDecimal(scaledRadius * 0.5, 1); + const [x, y] = pixelRounded; + const gradient = ctx.createRadialGradient( + x, + y, + scaledRadius, + x, + y, + 0, + ); + + gradient.addColorStop( + 1, + `rgba(${rgb![1]}, ${rgb![2]}, ${rgb![3]}, ${0.8 * globalOpacity})`, + ); + gradient.addColorStop( + 0, + `rgba(${rgb![1]}, ${rgb![2]}, ${rgb![3]}, 0)`, + ); + ctx.arc( + pixelRounded[0], + pixelRounded[1], + scaledRadius, + 0, + Math.PI * 2, + ); + ctx.fillStyle = gradient; + ctx.fill(); + + if (Number.isFinite(safeNumericLabel)) { + let label: string | number = clusterLabel; + const absLabel = Math.abs(safeNumericLabel); + const sign = safeNumericLabel < 0 ? '-' : ''; Review Comment: **Suggestion:** The finite-check is performed on `safeNumericLabel`, which is forced to `0` for invalid values, so non-finite cluster labels still render and can show `"NaN"` text. Check `numericLabel` directly before rendering labels to avoid displaying invalid cluster values. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Cluster labels can show invalid text values. - ⚠️ Custom-metric map readability degrades for affected clusters. ``` </details> ```suggestion if (Number.isFinite(numericLabel)) { let label: string | number = clusterLabel; const absLabel = Math.abs(numericLabel); const sign = numericLabel < 0 ? '-' : ''; ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Render `point_cluster_map` (registered in `superset-frontend/src/visualizations/presets/MainPreset.ts:134`, loaded via `plugin-chart-point-cluster-map/src/index.ts:52-54`). 2. Chart flow reaches `MapLibre.tsx:197-207`, which passes cluster data and `aggregation` into `ScatterPlotOverlay`. 3. In `ScatterPlotOverlay.tsx:85-86`, `computeClusterLabel()` can produce non-finite values for cluster labels (`std/stdev` path uses `Math.sqrt(variance)`), and `numericLabel` becomes non-finite at `:215`. 4. Current guard at `ScatterPlotOverlay.tsx:257` checks `safeNumericLabel` (forced to `0` at `:216-218`), so label rendering still runs and can draw invalid text (e.g., `"NaN"`/invalid label string) via `drawText()` at `:266-272`. ``` </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-point-cluster-map/src/components/ScatterPlotOverlay.tsx **Line:** 257:260 **Comment:** *Logic Error: The finite-check is performed on `safeNumericLabel`, which is forced to `0` for invalid values, so non-finite cluster labels still render and can show `"NaN"` text. Check `numericLabel` directly before rendering labels to avoid displaying invalid cluster values. 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=4089eac1971ab3386992a95ec62403f812f98331d91dabe63fb716db8d3a9e33&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=4089eac1971ab3386992a95ec62403f812f98331d91dabe63fb716db8d3a9e33&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts: ########## @@ -0,0 +1,292 @@ +/** + * 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 { t } from '@apache-superset/core/translation'; +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; + +const MIN_LONGITUDE = -180; +const MAX_LONGITUDE = 180; +const MIN_LATITUDE = -90; +const MAX_LATITUDE = 90; +const MIN_ZOOM = 0; + +function toFiniteNumber( + value: string | number | null | undefined, +): number | undefined { + if (value === null || value === undefined) return undefined; + const normalizedValue = typeof value === 'string' ? value.trim() : value; + if (normalizedValue === '') return undefined; + const num = Number(normalizedValue); + return Number.isFinite(num) ? num : undefined; +} + +function clampNumber( + value: number | undefined, + min: number, + max: number, +): number | undefined { + if (value === undefined) return undefined; + return Math.min(max, Math.max(min, value)); +} + +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 rawLon = record[lonCol]; + const rawLat = record[latCol]; + if (rawLon == null || rawLat == null) { + continue; + } + const lon = Number(rawLon); + const lat = Number(rawLat); + if (!Number.isFinite(lon) || !Number.isFinite(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: [[number, number], [number, number]] | undefined = + 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, + viewport_longitude: viewportLongitude, + viewport_latitude: viewportLatitude, + viewport_zoom: viewportZoom, + } = formData; + + // Support two data formats: + // 1. Legacy/GeoJSON: queriesData[0].data is an object with { geoJSON, bounds, hasCustomMetric } + // 2. Tabular records: queriesData[0].data is an array of flat records from a SQL query + const rawData = queriesData[0]?.data; + const isLegacyFormat = rawData && !Array.isArray(rawData) && rawData.geoJSON; + + let geoJSON: { type: 'FeatureCollection'; features: any[] }; + let bounds: [[number, number], [number, number]] | undefined; + let hasCustomMetric: boolean; + + if (isLegacyFormat) { + const legacy = rawData as any; + ({ geoJSON } = legacy); + ({ bounds } = legacy); + hasCustomMetric = legacy.hasCustomMetric ?? false; + } else { + const records: DataRecord[] = (rawData as DataRecord[]) || []; + hasCustomMetric = + maplibreLabel != null && + maplibreLabel.length > 0 && + maplibreLabel[0] !== 'count'; + const labelCol = hasCustomMetric ? maplibreLabel[0] : null; + const pointRadiusCol = + pointRadius && pointRadius !== 'Auto' ? pointRadius : null; + + const built = buildGeoJSONFromRecords( + records, + allColumnsX, + allColumnsY, + labelCol, + pointRadiusCol, + ); + ({ geoJSON } = built); + ({ bounds } = built); + } + + // 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(t("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 = ['', '0', '0', '0']; + } + + const opts: SuperclusterOptions<PointProperties, ClusterProperties> = { + maxZoom: DEFAULT_MAX_ZOOM, + radius: clusteringRadius, Review Comment: **Suggestion:** `clustering_radius` comes from a free-form control and can be a non-numeric string, but it is passed directly to Supercluster `radius`. Invalid values become `NaN` in clustering math and can produce broken or empty clustering behavior. Normalize and bound the radius before constructing Supercluster options. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Point cluster map can render incorrect clustering. - ⚠️ Free-form control accepts values runtime code cannot safely use. ``` </details> ```suggestion const normalizedClusteringRadius = toFiniteNumber(clusteringRadius) ?? 60; const opts: SuperclusterOptions<PointProperties, ClusterProperties> = { maxZoom: DEFAULT_MAX_ZOOM, radius: Math.max(0, normalizedClusteringRadius), ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `point_cluster_map` controls, enter a non-numeric free-form clustering radius (control is `SelectControl` with `freeForm: true` at `.../controlPanel.ts:67-70`). 2. Save/run chart so `rawFormData.clustering_radius` reaches `transformProps()` (`.../transformProps.ts:148`). 3. Current code passes that raw value directly into Supercluster options (`.../transformProps.ts:221-224`) and constructs clusterer (`.../transformProps.ts:242`). 4. Map rendering calls `clusterer.getClusters(...)` (`.../MapLibre.tsx:160`); invalid radius values can produce broken/empty clustering behavior instead of predictable radius handling. ``` </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-point-cluster-map/src/transformProps.ts **Line:** 221:223 **Comment:** *Possible Bug: `clustering_radius` comes from a free-form control and can be a non-numeric string, but it is passed directly to Supercluster `radius`. Invalid values become `NaN` in clustering math and can produce broken or empty clustering behavior. Normalize and bound the radius before constructing Supercluster options. 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=63a771cf6f77a3094a135a48e239a55e3dfc7bd3a698808ebf265515d7dad74c&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=63a771cf6f77a3094a135a48e239a55e3dfc7bd3a698808ebf265515d7dad74c&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/components/ScatterPlotOverlay.tsx: ########## @@ -0,0 +1,400 @@ +/** + * 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'; + +// Shared radius bounds keep cluster and point sizing in sync. +export const MIN_CLUSTER_RADIUS_RATIO = 1 / 6; +export const MAX_POINT_RADIUS_RATIO = 1 / 3; + +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 === 'std' || aggregation === 'stdev') { + return Math.round(100 * Math.sqrt(variance)) / 100; + } + + // fallback to point_count + return count; +}; + +function drawText( + ctx: CanvasRenderingContext2D, + pixel: [number, number], + compositeOperation: string, + options: DrawTextOptions = {}, +) { + const { + fontHeight = 0, + label = '', + radius = 0, + rgb = [0, 0, 0], + shadow = false, + } = options; + const maxWidth = radius * 1.8; + const luminance = luminanceFromRGB( + rgb[1] as number, + rgb[2] as number, + rgb[3] as number, + ); + + ctx.globalCompositeOperation = 'source-over'; + ctx.fillStyle = luminance <= IS_DARK_THRESHOLD ? 'white' : 'black'; + ctx.font = `${fontHeight}px sans-serif`; + ctx.textAlign = 'center'; + ctx.textBaseline = 'middle'; + if (shadow) { + ctx.shadowBlur = 15; + ctx.shadowColor = luminance <= IS_DARK_THRESHOLD ? 'black' : ''; + } + + const textWidth = ctx.measureText(String(label)).width; + if (textWidth > maxWidth) { + const scale = fontHeight / textWidth; + ctx.font = `${scale * maxWidth}px sans-serif`; + } + + ctx.fillText(String(label), pixel[0], pixel[1]); + ctx.globalCompositeOperation = compositeOperation as GlobalCompositeOperation; + ctx.shadowBlur = 0; + ctx.shadowColor = ''; +} + +function ScatterPlotOverlay({ + aggregation, + compositeOperation = 'source-over', + dotRadius = 4, + globalOpacity = 1, + lngLatAccessor = defaultLngLatAccessor, + locations, + pointRadiusUnit, + renderWhileDragging = true, + rgb, + zoom, +}: ScatterPlotOverlayProps) { + const redraw = useCallback( + ({ width, height, ctx, isDragging, project }: RedrawParams) => { + const radius = dotRadius; + const clusterLabelMap: (number | string)[] = []; + + locations.forEach((location, i) => { + if (location.properties.cluster) { + clusterLabelMap[i] = computeClusterLabel( + location.properties, + aggregation, + ); + } + }); + + const finiteClusterLabels = clusterLabelMap + .map(value => Number(value)) + .filter(value => Number.isFinite(value)); + const safeMaxAbsLabel = + finiteClusterLabels.length > 0 + ? Math.max( + Math.max(...finiteClusterLabels.map(value => Math.abs(value))), + 1, + ) + : 1; + + // 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 radiusValueRaw = location.properties.radius; + const radiusValue = + typeof radiusValueRaw === 'string' && radiusValueRaw.trim() === '' + ? null + : Number(radiusValueRaw); + if (radiusValue != null && Number.isFinite(radiusValue)) { + minRadiusValue = Math.min(minRadiusValue, radiusValue); + maxRadiusValue = Math.max(maxRadiusValue, radiusValue); + } + } + }); + } + + ctx.clearRect(0, 0, width, height); + ctx.globalCompositeOperation = + compositeOperation as GlobalCompositeOperation; + + if ((renderWhileDragging || !isDragging) && locations) { + locations.forEach((location: GeoJSONLocation, i: number) => { + const pixel = project(lngLatAccessor(location)); + const pixelRounded: [number, number] = [ + roundDecimal(pixel[0], 1), + roundDecimal(pixel[1], 1), + ]; + + if ( + pixelRounded[0] + radius >= 0 && + pixelRounded[0] - radius < width && + pixelRounded[1] + radius >= 0 && + pixelRounded[1] - radius < height + ) { + ctx.beginPath(); + if (location.properties.cluster) { + const clusterLabel = clusterLabelMap[i]; + const numericLabel = Number(clusterLabel); + const safeNumericLabel = Number.isFinite(numericLabel) + ? numericLabel + : 0; + const minClusterRadius = + pointRadiusUnit === 'Pixels' + ? radius * MAX_POINT_RADIUS_RATIO + : radius * MIN_CLUSTER_RADIUS_RATIO; + const ratio = Math.abs(safeNumericLabel) / safeMaxAbsLabel; + const scaledRadius = roundDecimal( + minClusterRadius + ratio ** 0.5 * (radius - minClusterRadius), + 1, + ); + const fontHeight = roundDecimal(scaledRadius * 0.5, 1); + const [x, y] = pixelRounded; + const gradient = ctx.createRadialGradient( + x, + y, + scaledRadius, + x, + y, + 0, + ); + + gradient.addColorStop( + 1, + `rgba(${rgb![1]}, ${rgb![2]}, ${rgb![3]}, ${0.8 * globalOpacity})`, + ); + gradient.addColorStop( + 0, + `rgba(${rgb![1]}, ${rgb![2]}, ${rgb![3]}, 0)`, + ); + ctx.arc( + pixelRounded[0], + pixelRounded[1], + scaledRadius, + 0, + Math.PI * 2, + ); + ctx.fillStyle = gradient; + ctx.fill(); + + if (Number.isFinite(safeNumericLabel)) { + let label: string | number = clusterLabel; + const absLabel = Math.abs(safeNumericLabel); + const sign = safeNumericLabel < 0 ? '-' : ''; + if (absLabel >= 10000) { + label = `${sign}${Math.round(absLabel / 1000)}k`; + } else if (absLabel >= 1000) { + label = `${sign}${Math.round(absLabel / 100) / 10}k`; + } + drawText(ctx, pixelRounded, compositeOperation, { + fontHeight, + label, + radius: scaledRadius, + rgb, + shadow: true, + }); + } + } else { + const defaultRadius = radius * MIN_CLUSTER_RADIUS_RATIO; + const rawRadius = location.properties.radius; + const numericRadiusProperty = + rawRadius != null && + !(typeof rawRadius === 'string' && rawRadius.trim() === '') + ? Number(rawRadius) + : null; + const radiusProperty = + numericRadiusProperty != null && + Number.isFinite(numericRadiusProperty) + ? numericRadiusProperty + : null; + const pointMetric = location.properties.metric ?? null; + let pointRadius: number = radiusProperty ?? defaultRadius; + let pointLabel: string | number | undefined; + + if (radiusProperty != null) { + const pointLatitude = lngLatAccessor(location)[1]; + if (pointRadiusUnit === 'Kilometers') { + pointLabel = `${roundDecimal(pointRadius, 2)}km`; + pointRadius = kmToPixels( + pointRadius, + pointLatitude, + zoom ?? 0, + ); + } else if (pointRadiusUnit === 'Miles') { + pointLabel = `${roundDecimal(pointRadius, 2)}mi`; + pointRadius = kmToPixels( + pointRadius * MILES_PER_KM, + pointLatitude, + zoom ?? 0, + ); + } else if (pointRadiusUnit === 'Pixels') { + const MIN_POINT_RADIUS = radius * MIN_CLUSTER_RADIUS_RATIO; + const MAX_POINT_RADIUS = radius * MAX_POINT_RADIUS_RATIO; + + if ( + Number.isFinite(minRadiusValue) && + Number.isFinite(maxRadiusValue) && + maxRadiusValue > minRadiusValue + ) { + const numericPointRadius = Number(pointRadius); + if (!Number.isFinite(numericPointRadius)) { + 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); + } + pointLabel = `${roundDecimal(radiusProperty, 2)}`; + } else if ( + Number.isFinite(minRadiusValue) && + minRadiusValue === maxRadiusValue + ) { + pointRadius = (MIN_POINT_RADIUS + MAX_POINT_RADIUS) / 2; + pointLabel = `${roundDecimal(radiusProperty, 2)}`; + } else { + pointRadius = Math.max( + MIN_POINT_RADIUS, + Math.min(pointRadius, MAX_POINT_RADIUS), + ); + pointLabel = `${roundDecimal(radiusProperty, 2)}`; + } + } + } + + if (pointMetric !== null) { + const numericMetric = parseFloat(String(pointMetric)); + pointLabel = Number.isFinite(numericMetric) + ? roundDecimal(numericMetric, 2) + : String(pointMetric); + } + + if (!pointRadius) { Review Comment: **Suggestion:** Negative or non-finite radii can still reach `ctx.arc` in kilometer/mile modes, which can throw `IndexSizeError` at runtime for negative radius values. Normalize radius to a positive finite fallback before drawing. [possible bug] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ Kilometer/Mile point rendering can throw during redraw. - ⚠️ Map overlay may stop drawing after error. ``` </details> ```suggestion if (!Number.isFinite(pointRadius) || pointRadius <= 0) { ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In Explore for `point_cluster_map` (plugin active in `MainPreset.ts:134`), choose a radius column containing negative values and set `point_radius_unit` to `Kilometers` or `Miles` (`controlPanel.ts:124-133`). 2. Query builder includes that radius column without numeric sign validation (`buildQuery.ts:64-67`), and `transformProps.ts:108-113` forwards raw radius values into feature properties. 3. During draw, kilometer/mile conversion uses raw `pointRadius` (`ScatterPlotOverlay.tsx:293-306`), so negative values remain negative after `kmToPixels` (`utils/geo.ts:25-37`). 4. Guard `if (!pointRadius)` at `ScatterPlotOverlay.tsx:355` does not catch negative finite radii, then `ctx.arc(..., radius, ...)` at `:359-365` receives negative radius and can throw runtime canvas errors. ``` </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-point-cluster-map/src/components/ScatterPlotOverlay.tsx **Line:** 355:355 **Comment:** *Possible Bug: Negative or non-finite radii can still reach `ctx.arc` in kilometer/mile modes, which can throw `IndexSizeError` at runtime for negative radius values. Normalize radius to a positive finite fallback before drawing. 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=dea275ca5c053d200bca5f16133c63dc62f737a050d3e6f4b239fb8b51d0d465&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=dea275ca5c053d200bca5f16133c63dc62f737a050d3e6f4b239fb8b51d0d465&reaction=dislike'>👎</a> ########## superset-frontend/plugins/plugin-chart-point-cluster-map/test/ScatterPlotOverlay.test.tsx: ########## @@ -67,22 +71,20 @@ declare global { var mockRedraw: unknown; } -// Mock react-map-gl's CanvasOverlay -jest.mock('react-map-gl', () => ({ - CanvasOverlay: ({ redraw }: { redraw: unknown }) => { - // Store the redraw function so tests can call it +// Mock the CanvasOverlay component to capture the redraw function +jest.mock('../src/components/CanvasOverlay', () => ({ + __esModule: true, + default: ({ redraw }: { redraw: unknown }) => { global.mockRedraw = redraw; return <div data-testid="canvas-overlay" />; }, })); -// Mock utility functions jest.mock('../src/utils/luminanceFromRGB', () => ({ __esModule: true, - default: jest.fn(() => 150), // Return a value above the dark threshold + default: jest.fn(() => 150), })); Review Comment: **Suggestion:** The shared `global.mockRedraw` callback is never reset between tests, so a stale callback from a previous test can be reused and make later tests pass even when `ScatterPlotOverlay` fails to register a new redraw handler. Reset the global callback and mocks before each test to keep tests isolated and prevent false positives. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ ScatterPlotOverlay tests can pass with broken redraw wiring. - ⚠️ Regressions in CanvasOverlay integration may be hidden. ``` </details> ```suggestion beforeEach(() => { global.mockRedraw = undefined; jest.clearAllMocks(); }); ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Run this Jest file `superset-frontend/plugins/plugin-chart-point-cluster-map/test/ScatterPlotOverlay.test.tsx`; first test render stores `global.mockRedraw` in mocked `CanvasOverlay` (`lines 75-80`). 2. `triggerRedraw()` only checks `typeof global.mockRedraw === 'function'` (`lines 143-146`) and then invokes it, without verifying it was set by the current test render. 3. There is no local reset hook in this file, and project Jest config (`superset-frontend/jest.config.js:21-93`) does not enable `clearMocks/resetMocks`, so `global.mockRedraw` persists across tests. 4. If a later regression prevents `ScatterPlotOverlay` from registering redraw (current registration path is `ScatterPlotOverlay.tsx:397` passing to `CanvasOverlay`), `triggerRedraw()` can still call stale callback from previous test and falsely pass. ``` </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-point-cluster-map/test/ScatterPlotOverlay.test.tsx **Line:** 87:87 **Comment:** *Logic Error: The shared `global.mockRedraw` callback is never reset between tests, so a stale callback from a previous test can be reused and make later tests pass even when `ScatterPlotOverlay` fails to register a new redraw handler. Reset the global callback and mocks before each test to keep tests isolated and prevent false positives. 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=cee0e2731d02fd19b3139e7602879e62309ef1289f53b29cafa6b0125473dc6e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38035&comment_hash=cee0e2731d02fd19b3139e7602879e62309ef1289f53b29cafa6b0125473dc6e&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]
