bito-code-review[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2955795136
########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts: ########## @@ -0,0 +1,287 @@ +/** + * 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 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: [[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, + }; + if (hasCustomMetric) { + opts.map = (prop: PointProperties) => ({ + metric: Number(prop.metric) || 0, + sum: Number(prop.metric) || 0, + squaredSum: (Number(prop.metric) || 0) ** 2, + min: Number(prop.metric) || 0, + max: Number(prop.metric) || 0, + }); + opts.reduce = (accu: ClusterProperties, prop: ClusterProperties) => { + /* eslint-disable no-param-reassign */ + accu.sum += prop.sum; + accu.squaredSum += prop.squaredSum; + accu.min = Math.min(accu.min, prop.min); + accu.max = Math.max(accu.max, prop.max); + /* eslint-enable no-param-reassign */ + }; + } + const clusterer = new Supercluster<PointProperties, ClusterProperties>(opts); + // Disable strict typecheck on load since Supercluster typings have namespace issues with esModuleInterop + clusterer.load(geoJSON.features as any); + + return { + width, + height, + aggregatorName: pandasAggfunc, + bounds, + clusterer, + globalOpacity: Math.min(1, Math.max(0, toFiniteNumber(globalOpacity) ?? 1)), + hasCustomMetric, + mapProvider, + mapStyle: + mapProvider === 'mapbox' + ? (mapboxStyle as string) + : (maplibreStyle as string), + onViewportChange({ + latitude, + longitude, + zoom, + }: { + latitude: number; + longitude: number; + zoom: number; + }) { + setControlValue('viewport_longitude', longitude); + setControlValue('viewport_latitude', latitude); + setControlValue('viewport_zoom', zoom); + }, + pointRadius: DEFAULT_POINT_RADIUS, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Ignored user pointRadius setting</b></div> <div id="fix"> User-set pointRadius in formData is ignored, always defaulting to 60. This prevents customization of point sizes. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - setControlValue('viewport_zoom', zoom); - }, - pointRadius: DEFAULT_POINT_RADIUS, - pointRadiusUnit, + setControlValue('viewport_zoom', zoom); - }, + pointRadius: toFiniteNumber(pointRadius) ?? DEFAULT_POINT_RADIUS, + pointRadiusUnit, ``` </div> </details> </div> <small><i>Code Review Run #32788d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts: ########## @@ -0,0 +1,287 @@ +/** + * 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 lon = Number(record[lonCol]); + const lat = Number(record[latCol]); + if (Number.isNaN(lon) || Number.isNaN(lat)) { + continue; + } Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect null handling in geo data parsing</b></div> <div id="fix"> Records with null lon/lat values are incorrectly included as points at (0,0) since Number(null) returns 0, not NaN. This changes map output by adding spurious points. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion for (const record of records) { if (record[lonCol] == null || record[latCol] == null) { continue; } const lon = Number(record[lonCol]); const lat = Number(record[latCol]); if (Number.isNaN(lon) || Number.isNaN(lat)) { continue; } ```` </div> </details> </div> <small><i>Code Review Run #32788d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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)`, + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>RGB indexing bug in gradient</b></div> <div id="fix"> The RGB array is indexed incorrectly in the gradient color stops, using rgb[1], rgb[2], rgb[3] instead of rgb[0], rgb[1], rgb[2]. This causes wrong cluster colors. Fix by using 0-based indexing. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion gradient.addColorStop( 1, `rgba(${rgb![0]}, ${rgb![1]}, ${rgb![2]}, ${0.8 * globalOpacity})`, ); gradient.addColorStop( 0, `rgba(${rgb![0]}, ${rgb![1]}, ${rgb![2]}, 0)`, ); ```` </div> </details> </div> <small><i>Code Review Run #32788d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/config.py: ########## @@ -2142,7 +2142,15 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument # noq "https://events.mapbox.com";, "https://tile.openstreetmap.org";, "https://tile.osm.ch";, - "https://a.basemaps.cartocdn.com";, + "https://basemaps.cartocdn.com";, + "https://*.basemaps.cartocdn.com";, + "https://tiles.openfreemap.org";, + "https://*.maptiler.com";, + "https://tiles.stadiamaps.com";, + "https://tiles.versatiles.org";, + "https://*.protomaps.com";, + "https://*.maplibre.org";, + "https://raw.githubusercontent.com";, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>CSP img-src missing map domains</b></div> <div id="fix"> The CSP img-src does not include the new map tile domains, which will prevent raster tiles (e.g., PNG images from tiles.openfreemap.org in the default style) and sprites from loading in maps. This could result in incomplete or broken map rendering. Update img-src to match the connect-src additions. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ``` - "https://cdn.document360.io";, - ], + "https://cdn.document360.io";, + "https://basemaps.cartocdn.com";, + "https://*.basemaps.cartocdn.com";, + "https://tiles.openfreemap.org";, + "https://*.maptiler.com";, + "https://tiles.stadiamaps.com";, + "https://tiles.versatiles.org";, + "https://*.protomaps.com";, + "https://*.maplibre.org";, + "https://raw.githubusercontent.com";, + ], @@ -2184,2 +2184,11 @@ - "https://cdn.document360.io";, - ], + "https://cdn.document360.io";, + "https://basemaps.cartocdn.com";, + "https://*.basemaps.cartocdn.com";, + "https://tiles.openfreemap.org";, + "https://*.maptiler.com";, + "https://tiles.stadiamaps.com";, + "https://tiles.versatiles.org";, + "https://*.protomaps.com";, + "https://*.maplibre.org";, + "https://raw.githubusercontent.com";, + ], ``` </div> </details> </div> <small><i>Code Review Run #32788d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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, + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>RGB indexing bug in luminance</b></div> <div id="fix"> The RGB array is indexed incorrectly in the luminance calculation, using rgb[1], rgb[2], rgb[3] instead of rgb[0], rgb[1], rgb[2]. This causes wrong text color determination. Fix by using 0-based indexing. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const luminance = luminanceFromRGB( rgb[0] as number, rgb[1] as number, rgb[2] as number, ); ```` </div> </details> </div> <small><i>Code Review Run #32788d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## 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) { + pointRadius = defaultRadius; + } + + ctx.arc( + pixelRounded[0], + pixelRounded[1], + roundDecimal(pointRadius, 1), + 0, + Math.PI * 2, + ); + ctx.fillStyle = `rgba(${rgb![1]}, ${rgb![2]}, ${rgb![3]}, ${globalOpacity})`; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>RGB indexing bug in fillStyle</b></div> <div id="fix"> The RGB array is indexed incorrectly in the point fill style, using rgb[1], rgb[2], rgb[3] instead of rgb[0], rgb[1], rgb[2]. This causes wrong point colors. Fix by using 0-based indexing. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion ctx.fillStyle = `rgba(${rgb![0]}, ${rgb![1]}, ${rgb![2]}, ${globalOpacity})`; ```` </div> </details> </div> <small><i>Code Review Run #32788d</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- 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]
