Re: [PR] chore: udpate react-map-gl from v6 to v8 and refactor MapBox chart to TypeScript [superset]

[via GitHub]

codeant-ai-for-open-source[bot] commented on code in PR #35525:
URL: https://github.com/apache/superset/pull/35525#discussion_r2658836548


##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -0,0 +1,227 @@
+/**
+ * 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 { Component } from 'react';
+import Map, { ViewStateChangeEvent } from 'react-map-gl/mapbox';
+import { WebMercatorViewport } from '@math.gl/web-mercator';
+import ScatterPlotGlowOverlay, {
+  type AggregationType,
+  type Location,
+} from './ScatterPlotGlowOverlay';
+import './MapBox.css';
+
+export interface Clusterer {
+  getClusters(bbox: [number, number, number, number], zoom: number): 
Location[];
+}
+
+const NOOP = () => {};
+export const DEFAULT_MAX_ZOOM = 16;
+export const DEFAULT_POINT_RADIUS = 60;
+const DEFAULT_DIMENSION = 400;
+const VIEWPORT_BUFFER_MULTIPLIER = 0.5;
+const VIEWPORT_BUFFER_DIVISOR = 100;
+
+interface Viewport {
+  longitude: number;
+  latitude: number;
+  zoom: number;
+  isDragging?: boolean;
+}
+
+interface MapBoxProps {
+  aggregatorName?: AggregationType;
+  bounds: [[number, number], [number, number]];
+  clusterer: Clusterer;
+  globalOpacity?: number;
+  hasCustomMetric?: boolean;
+  height?: number;
+  mapStyle?: string;
+  mapboxApiKey: string;
+  onViewportChange?: (viewport: Viewport) => void;
+  pointRadius?: number;
+  pointRadiusUnit?: string;
+  renderWhileDragging?: boolean;
+  rgb?: [number, number, number, number];
+  width?: number;
+}
+
+interface MapBoxState {
+  viewport: Viewport;
+  clusters: Location[];
+  isMapLoaded: boolean;
+}
+
+const defaultProps: Partial<MapBoxProps> = {
+  width: 400,
+  height: 400,
+  globalOpacity: 1,
+  onViewportChange: NOOP,
+  pointRadius: DEFAULT_POINT_RADIUS,
+  pointRadiusUnit: 'Pixels',
+};
+
+class MapBox extends Component<MapBoxProps, MapBoxState> {
+  static defaultProps = defaultProps;
+
+  private lastZoom: number;
+
+  constructor(props: MapBoxProps) {
+    super(props);
+
+    const { width, height, bounds, clusterer } = this.props;
+    const mercator = new WebMercatorViewport({
+      width: width || DEFAULT_DIMENSION,
+      height: height || DEFAULT_DIMENSION,
+    }).fitBounds(bounds);
+    const { latitude, longitude, zoom } = mercator;
+
+    const viewport = { longitude, latitude, zoom };
+    this.lastZoom = Math.round(zoom);
+
+    this.state = {
+      viewport,
+      clusters: this.computeClusters(clusterer, bounds, width, height, zoom),
+      isMapLoaded: false,
+    };
+    this.handleViewportChange = this.handleViewportChange.bind(this);
+    this.handleMapLoad = this.handleMapLoad.bind(this);
+  }
+
+  componentDidUpdate(prevProps: MapBoxProps, prevState: MapBoxState) {
+    const { viewport } = this.state;
+    const { clusterer, bounds, width, height } = this.props;
+    const roundedZoom = Math.round(viewport.zoom);
+
+    const shouldRecompute =
+      prevProps.clusterer !== clusterer || this.lastZoom !== roundedZoom;
+
+    if (shouldRecompute) {
+      this.lastZoom = roundedZoom;
+      this.setState({
+        clusters: this.computeClusters(
+          clusterer,
+          bounds,
+          width,
+          height,
+          viewport.zoom,
+        ),
+      });
+    }
+  }
+
+  computeClusters(
+    clusterer: Clusterer,
+    bounds: [[number, number], [number, number]],
+    width: number | undefined,
+    height: number | undefined,
+    zoom: number,
+  ): Location[] {
+    const offsetHorizontal =
+      ((width || DEFAULT_DIMENSION) * VIEWPORT_BUFFER_MULTIPLIER) /
+      VIEWPORT_BUFFER_DIVISOR;
+    const offsetVertical =
+      ((height || DEFAULT_DIMENSION) * VIEWPORT_BUFFER_MULTIPLIER) /
+      VIEWPORT_BUFFER_DIVISOR;
+    const bbox: [number, number, number, number] = [
+      bounds[0][0] - offsetHorizontal,
+      bounds[0][1] - offsetVertical,
+      bounds[1][0] + offsetHorizontal,
+      bounds[1][1] + offsetVertical,
+    ];
+    return clusterer.getClusters(bbox, Math.round(zoom));
+  }
+
+  handleViewportChange(evt: ViewStateChangeEvent) {
+    const { latitude, longitude, zoom } = evt.viewState;
+
+    if (
+      latitude === undefined ||
+      longitude === undefined ||
+      zoom === undefined
+    ) {
+      console.warn('MapBox: Invalid viewport change event', evt);
+      return;
+    }
+
+    const viewport: Viewport = { latitude, longitude, zoom };

Review Comment:
   **Suggestion:** handleViewportChange drops the `isDragging` flag from the 
incoming `evt.viewState` and stores a viewport object without it; as a result 
`viewport.isDragging` never becomes true while the map is being dragged and 
child overlay logic that relies on `isDragging` (e.g. render optimizations) 
will not behave correctly. Preserve and store `isDragging` from `evt.viewState` 
when updating the component state and forwarding the viewport. [logic error]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
       const { latitude, longitude, zoom, isDragging } = evt.viewState;
   
       if (
         latitude === undefined ||
         longitude === undefined ||
         zoom === undefined
       ) {
         console.warn('MapBox: Invalid viewport change event', evt);
         return;
       }
   
       const viewport: Viewport = { latitude, longitude, zoom, isDragging };
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Accurate and actionable. The current code never preserves isDragging from 
the Map's viewState, so consumers relying on viewport.isDragging (or the 
component's own derived isDragging) will always see false. Including isDragging 
in the stored viewport fixes a real logic bug and is a minimal, safe change 
that matches the existing Viewport type and onViewportChange contract.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx
   **Line:** 150:161
   **Comment:**
        *Logic Error: handleViewportChange drops the `isDragging` flag from the 
incoming `evt.viewState` and stores a viewport object without it; as a result 
`viewport.isDragging` never becomes true while the map is being dragged and 
child overlay logic that relies on `isDragging` (e.g. render optimizations) 
will not behave correctly. Preserve and store `isDragging` from `evt.viewState` 
when updating the component state and forwarding the viewport.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -0,0 +1,460 @@
+/**
+ * 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 { PureComponent, createRef, RefObject } from 'react';
+import { useMap } from 'react-map-gl/mapbox';
+import type { MapRef } from 'react-map-gl/mapbox';
+import { kmToPixels, MILES_PER_KM } from './utils/geo';
+import roundDecimal from './utils/roundDecimal';
+import luminanceFromRGB from './utils/luminanceFromRGB';
+import 'mapbox-gl/dist/mapbox-gl.css';
+
+const LUMINANCE_THRESHOLD_DARK = 110;
+const TEXT_WIDTH_RATIO = 1.8;
+const DEFAULT_RGB_COLOR: [number, number, number, number] = [0, 0, 0, 0];
+const DEFAULT_DOT_RADIUS = 4;
+const DEFAULT_RADIUS_DIVISOR = 6;
+const CLUSTER_LABEL_THRESHOLD_LARGE = 10000;
+const CLUSTER_LABEL_THRESHOLD_MEDIUM = 1000;
+
+export type AggregationType = 'sum' | 'min' | 'max' | 'mean' | 'var' | 'stdev';
+
+export interface ClusterProperties {
+  cluster: true;
+  cluster_id?: number;
+  point_count: number;
+  sum?: number;
+  squaredSum?: number;
+  min?: number;
+  max?: number;
+}
+
+export interface PointProperties {
+  cluster?: false;
+  radius?: number | null;
+  metric?: number | string | null;
+  cat_color?: string | null;
+}
+
+export type LocationProperties = ClusterProperties | PointProperties;
+
+export interface Geometry {
+  coordinates: [number, number];
+  type: string;
+}
+
+export interface Location {
+  type?: 'Feature';
+  geometry: Geometry;
+  properties: LocationProperties;
+}
+
+interface ScatterPlotGlowOverlayProps {
+  aggregation?: AggregationType;
+  compositeOperation?: GlobalCompositeOperation;
+  dotRadius?: number;
+  isDragging?: boolean;
+  lngLatAccessor?: (location: Location) => [number, number];
+  locations: Location[];
+  mapRef?: { current: MapRef | null | undefined };
+  pointRadiusUnit?: string;
+  renderWhileDragging?: boolean;
+  rgb?: [number, number, number, number];
+  zoom?: number;
+}
+
+interface DrawTextOptions {
+  fontHeight?: number;
+  label?: string | number;
+  radius?: number;
+  rgb?: [number, number, number, number];
+  shadow?: boolean;
+}
+
+const defaultProps: Partial<ScatterPlotGlowOverlayProps> = {
+  // Same as browser default.
+  compositeOperation: 'source-over',
+  dotRadius: 4,
+  lngLatAccessor: (location: Location) => {
+    const { coordinates } = location.geometry;
+    return [coordinates[0], coordinates[1]];
+  },
+  renderWhileDragging: true,
+};
+
+const isClusterProperties = (
+  props: LocationProperties,
+): props is ClusterProperties => props.cluster === true;
+
+const computeClusterLabel = (
+  properties: LocationProperties,
+  aggregation?: AggregationType,
+): number => {
+  if (!isClusterProperties(properties)) {
+    return 0;
+  }
+
+  const count = properties.point_count ?? 0;
+  if (!aggregation) {
+    return count;
+  }
+  if (aggregation === 'sum' || aggregation === 'min' || aggregation === 'max') 
{
+    const value = properties[aggregation];
+    if (typeof value === 'number') {
+      return value;
+    }
+    return 0;
+  }
+  const sum = properties.sum ?? 0;
+  const mean = count > 0 ? sum / count : 0;
+  if (aggregation === 'mean') {
+    return Math.round(100 * mean) / 100;
+  }
+  const squaredSum = properties.squaredSum ?? 0;
+  const variance = count > 0 ? squaredSum / count - mean ** 2 : 0;
+  if (aggregation === 'var') {
+    return Math.round(100 * variance) / 100;
+  }
+  if (aggregation === 'stdev') {
+    return Math.round(100 * Math.sqrt(Math.max(0, variance))) / 100;
+  }
+
+  return count;
+};
+
+class ScatterPlotGlowOverlay extends 
PureComponent<ScatterPlotGlowOverlayProps> {
+  static defaultProps = defaultProps;
+
+  private canvasRef: RefObject<HTMLCanvasElement>;
+
+  private containerRef: RefObject<HTMLDivElement>;
+
+  constructor(props: ScatterPlotGlowOverlayProps) {
+    super(props);
+    this.canvasRef = createRef<HTMLCanvasElement>();
+    this.containerRef = createRef<HTMLDivElement>();
+    this.redraw = this.redraw.bind(this);
+    this.updateCanvasSize = this.updateCanvasSize.bind(this);
+  }
+
+  componentDidMount() {
+    this.updateCanvasSize();
+    this.redraw();
+    if (this.props.mapRef?.current) {
+      this.props.mapRef.current.on('move', this.redraw);
+      this.props.mapRef.current.on('moveend', this.redraw);
+    }
+  }
+
+  componentDidUpdate(prevProps: ScatterPlotGlowOverlayProps) {
+    const shouldUpdateSize = prevProps.isDragging !== this.props.isDragging;
+    if (shouldUpdateSize) {
+      this.updateCanvasSize();
+    }
+
+    const shouldRedraw =
+      prevProps.locations !== this.props.locations ||
+      prevProps.dotRadius !== this.props.dotRadius ||
+      prevProps.rgb !== this.props.rgb ||
+      prevProps.aggregation !== this.props.aggregation ||
+      prevProps.compositeOperation !== this.props.compositeOperation ||
+      prevProps.pointRadiusUnit !== this.props.pointRadiusUnit ||
+      prevProps.zoom !== this.props.zoom;
+
+    if (shouldRedraw) {
+      this.redraw();
+    }
+  }
+
+  componentWillUnmount() {
+    if (this.props.mapRef?.current) {
+      this.props.mapRef.current.off('move', this.redraw);
+      this.props.mapRef.current.off('moveend', this.redraw);
+    }
+  }
+
+  updateCanvasSize() {
+    const canvas = this.canvasRef.current;
+    const container = this.containerRef.current;
+    if (!canvas || !container) return;
+
+    const { width, height } = container.getBoundingClientRect();
+    if (canvas.width !== width || canvas.height !== height) {
+      canvas.width = width;
+      canvas.height = height;

Review Comment:
   **Suggestion:** Canvas size is set using floating bounding-box dimensions 
directly which can cause frequent unnecessary resizes and rendering issues; 
round sizes and also set the element's CSS size to avoid mismatches (and 
optionally account for devicePixelRatio elsewhere). [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
       const w = Math.round(width);
       const h = Math.round(height);
       // Keep canvas internal pixel size in whole pixels and keep the CSS size 
in sync.
       if (canvas.width !== w || canvas.height !== h) {
         canvas.width = w;
         canvas.height = h;
         canvas.style.width = `${w}px`;
         canvas.style.height = `${h}px`;
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Rounding bounding-box dimensions and keeping the canvas element's CSS size 
in sync reduces pointless resize churn when getBoundingClientRect returns 
fractional pixels and avoids layout vs. bitmap-size mismatches. The suggested 
change is a sensible, low-risk improvement over the current logic.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx
   **Line:** 197:199
   **Comment:**
        *Possible Bug: Canvas size is set using floating bounding-box 
dimensions directly which can cause frequent unnecessary resizes and rendering 
issues; round sizes and also set the element's CSS size to avoid mismatches 
(and optionally account for devicePixelRatio elsewhere).
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -0,0 +1,227 @@
+/**
+ * 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 { Component } from 'react';
+import Map, { ViewStateChangeEvent } from 'react-map-gl/mapbox';
+import { WebMercatorViewport } from '@math.gl/web-mercator';
+import ScatterPlotGlowOverlay, {
+  type AggregationType,
+  type Location,
+} from './ScatterPlotGlowOverlay';
+import './MapBox.css';
+
+export interface Clusterer {
+  getClusters(bbox: [number, number, number, number], zoom: number): 
Location[];
+}
+
+const NOOP = () => {};
+export const DEFAULT_MAX_ZOOM = 16;
+export const DEFAULT_POINT_RADIUS = 60;
+const DEFAULT_DIMENSION = 400;
+const VIEWPORT_BUFFER_MULTIPLIER = 0.5;
+const VIEWPORT_BUFFER_DIVISOR = 100;
+
+interface Viewport {
+  longitude: number;
+  latitude: number;
+  zoom: number;
+  isDragging?: boolean;
+}
+
+interface MapBoxProps {
+  aggregatorName?: AggregationType;
+  bounds: [[number, number], [number, number]];
+  clusterer: Clusterer;
+  globalOpacity?: number;
+  hasCustomMetric?: boolean;
+  height?: number;
+  mapStyle?: string;
+  mapboxApiKey: string;
+  onViewportChange?: (viewport: Viewport) => void;
+  pointRadius?: number;
+  pointRadiusUnit?: string;
+  renderWhileDragging?: boolean;
+  rgb?: [number, number, number, number];
+  width?: number;
+}
+
+interface MapBoxState {
+  viewport: Viewport;
+  clusters: Location[];
+  isMapLoaded: boolean;
+}
+
+const defaultProps: Partial<MapBoxProps> = {
+  width: 400,
+  height: 400,
+  globalOpacity: 1,
+  onViewportChange: NOOP,
+  pointRadius: DEFAULT_POINT_RADIUS,
+  pointRadiusUnit: 'Pixels',
+};
+
+class MapBox extends Component<MapBoxProps, MapBoxState> {
+  static defaultProps = defaultProps;
+
+  private lastZoom: number;
+
+  constructor(props: MapBoxProps) {
+    super(props);
+
+    const { width, height, bounds, clusterer } = this.props;
+    const mercator = new WebMercatorViewport({
+      width: width || DEFAULT_DIMENSION,
+      height: height || DEFAULT_DIMENSION,
+    }).fitBounds(bounds);
+    const { latitude, longitude, zoom } = mercator;
+
+    const viewport = { longitude, latitude, zoom };
+    this.lastZoom = Math.round(zoom);
+
+    this.state = {
+      viewport,
+      clusters: this.computeClusters(clusterer, bounds, width, height, zoom),
+      isMapLoaded: false,
+    };
+    this.handleViewportChange = this.handleViewportChange.bind(this);
+    this.handleMapLoad = this.handleMapLoad.bind(this);
+  }
+
+  componentDidUpdate(prevProps: MapBoxProps, prevState: MapBoxState) {
+    const { viewport } = this.state;
+    const { clusterer, bounds, width, height } = this.props;
+    const roundedZoom = Math.round(viewport.zoom);
+
+    const shouldRecompute =
+      prevProps.clusterer !== clusterer || this.lastZoom !== roundedZoom;

Review Comment:
   **Suggestion:** componentDidUpdate only recomputes clusters when the 
`clusterer` prop changes or zoom crosses a rounded threshold; it ignores 
changes to `bounds`, `width`, or `height`. If the bounding box or component 
size changes the clusters will become stale. Add checks for bounds and size 
changes (deep-compare bounds) so clusters are recomputed when those inputs 
change. [logic error]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
       const boundsChanged =
         JSON.stringify(prevProps.bounds) !== JSON.stringify(bounds);
       const sizeChanged = prevProps.width !== width || prevProps.height !== 
height;
   
       const shouldRecompute =
         prevProps.clusterer !== clusterer ||
         this.lastZoom !== roundedZoom ||
         boundsChanged ||
         sizeChanged;
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Correct and relevant. computeClusters depends on bounds, width and height; 
componentDidUpdate currently ignores changes to those props, which can produce 
stale clusters after resize or bounds update. Adding bounds/size comparisons 
ensures clusters are recomputed when their inputs change. The suggested 
JSON.stringify check is pragmatic for a shallow deep-compare of the bounds 
tuple.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx
   **Line:** 110:111
   **Comment:**
        *Logic Error: componentDidUpdate only recomputes clusters when the 
`clusterer` prop changes or zoom crosses a rounded threshold; it ignores 
changes to `bounds`, `width`, or `height`. If the bounding box or component 
size changes the clusters will become stale. Add checks for bounds and size 
changes (deep-compare bounds) so clusters are recomputed when those inputs 
change.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/index.ts:
##########
@@ -24,14 +24,15 @@ import example1Dark from './images/MapBox-dark.jpg';
 import example2 from './images/MapBox2.jpg';
 import example2Dark from './images/MapBox2-dark.jpg';
 import controlPanel from './controlPanel';
+import transformProps from './transformProps';
 
 const metadata = new ChartMetadata({
   category: t('Map'),
   credits: ['https://www.mapbox.com/mapbox-gl-js/api/'],
   description: '',
   exampleGallery: [
-    { url: example1, urlDark: example1Dark, description: t('Light mode') },
-    { url: example2, urlDark: example2Dark, description: t('Dark mode') },
+    { url: example1, urlDark: example1Dark },
+    { url: example2, urlDark: example2Dark },

Review Comment:
   **Suggestion:** Performance: importing large example images at module 
top-level forces bundlers to include them in the initial bundle; adding 
multiple large images will bloat the main bundle. Use the already-imported 
small `thumbnail`/`thumbnailDark` for the metadata gallery or lazy-load the 
heavy images to keep initial bundle size small. [performance]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
       { url: thumbnail, urlDark: thumbnailDark },
       { url: thumbnail, urlDark: thumbnailDark },
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Valid performance concern: importing heavy images at module top-level can 
bloat the initial bundle. Replacing gallery entries with smaller thumbnails or 
lazy-loading the large examples will reduce bundle size. The suggested 
replacement (using thumbnail) is reasonable, though duplicating the same 
thumbnail twice is odd — better to use distinct smaller assets or lazy-load the 
originals when the gallery is opened.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/plugins/legacy-plugin-chart-map-box/src/index.ts
   **Line:** 34:35
   **Comment:**
        *Performance: Performance: importing large example images at module 
top-level forces bundlers to include them in the initial bundle; adding 
multiple large images will bloat the main bundle. Use the already-imported 
small `thumbnail`/`thumbnailDark` for the metadata gallery or lazy-load the 
heavy images to keep initial bundle size small.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -0,0 +1,460 @@
+/**
+ * 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 { PureComponent, createRef, RefObject } from 'react';
+import { useMap } from 'react-map-gl/mapbox';
+import type { MapRef } from 'react-map-gl/mapbox';
+import { kmToPixels, MILES_PER_KM } from './utils/geo';
+import roundDecimal from './utils/roundDecimal';
+import luminanceFromRGB from './utils/luminanceFromRGB';
+import 'mapbox-gl/dist/mapbox-gl.css';
+
+const LUMINANCE_THRESHOLD_DARK = 110;
+const TEXT_WIDTH_RATIO = 1.8;
+const DEFAULT_RGB_COLOR: [number, number, number, number] = [0, 0, 0, 0];
+const DEFAULT_DOT_RADIUS = 4;
+const DEFAULT_RADIUS_DIVISOR = 6;
+const CLUSTER_LABEL_THRESHOLD_LARGE = 10000;
+const CLUSTER_LABEL_THRESHOLD_MEDIUM = 1000;
+
+export type AggregationType = 'sum' | 'min' | 'max' | 'mean' | 'var' | 'stdev';
+
+export interface ClusterProperties {
+  cluster: true;
+  cluster_id?: number;
+  point_count: number;
+  sum?: number;
+  squaredSum?: number;
+  min?: number;
+  max?: number;
+}
+
+export interface PointProperties {
+  cluster?: false;
+  radius?: number | null;
+  metric?: number | string | null;
+  cat_color?: string | null;
+}
+
+export type LocationProperties = ClusterProperties | PointProperties;
+
+export interface Geometry {
+  coordinates: [number, number];
+  type: string;
+}
+
+export interface Location {
+  type?: 'Feature';
+  geometry: Geometry;
+  properties: LocationProperties;
+}
+
+interface ScatterPlotGlowOverlayProps {
+  aggregation?: AggregationType;
+  compositeOperation?: GlobalCompositeOperation;
+  dotRadius?: number;
+  isDragging?: boolean;
+  lngLatAccessor?: (location: Location) => [number, number];
+  locations: Location[];
+  mapRef?: { current: MapRef | null | undefined };
+  pointRadiusUnit?: string;
+  renderWhileDragging?: boolean;
+  rgb?: [number, number, number, number];
+  zoom?: number;
+}
+
+interface DrawTextOptions {
+  fontHeight?: number;
+  label?: string | number;
+  radius?: number;
+  rgb?: [number, number, number, number];
+  shadow?: boolean;
+}
+
+const defaultProps: Partial<ScatterPlotGlowOverlayProps> = {
+  // Same as browser default.
+  compositeOperation: 'source-over',
+  dotRadius: 4,
+  lngLatAccessor: (location: Location) => {
+    const { coordinates } = location.geometry;
+    return [coordinates[0], coordinates[1]];
+  },
+  renderWhileDragging: true,
+};
+
+const isClusterProperties = (
+  props: LocationProperties,
+): props is ClusterProperties => props.cluster === true;
+
+const computeClusterLabel = (
+  properties: LocationProperties,
+  aggregation?: AggregationType,
+): number => {
+  if (!isClusterProperties(properties)) {
+    return 0;
+  }
+
+  const count = properties.point_count ?? 0;
+  if (!aggregation) {
+    return count;
+  }
+  if (aggregation === 'sum' || aggregation === 'min' || aggregation === 'max') 
{
+    const value = properties[aggregation];
+    if (typeof value === 'number') {
+      return value;
+    }
+    return 0;
+  }
+  const sum = properties.sum ?? 0;
+  const mean = count > 0 ? sum / count : 0;
+  if (aggregation === 'mean') {
+    return Math.round(100 * mean) / 100;
+  }
+  const squaredSum = properties.squaredSum ?? 0;
+  const variance = count > 0 ? squaredSum / count - mean ** 2 : 0;
+  if (aggregation === 'var') {
+    return Math.round(100 * variance) / 100;
+  }
+  if (aggregation === 'stdev') {
+    return Math.round(100 * Math.sqrt(Math.max(0, variance))) / 100;
+  }
+
+  return count;
+};
+
+class ScatterPlotGlowOverlay extends 
PureComponent<ScatterPlotGlowOverlayProps> {
+  static defaultProps = defaultProps;
+
+  private canvasRef: RefObject<HTMLCanvasElement>;
+
+  private containerRef: RefObject<HTMLDivElement>;
+
+  constructor(props: ScatterPlotGlowOverlayProps) {
+    super(props);
+    this.canvasRef = createRef<HTMLCanvasElement>();
+    this.containerRef = createRef<HTMLDivElement>();
+    this.redraw = this.redraw.bind(this);
+    this.updateCanvasSize = this.updateCanvasSize.bind(this);
+  }
+
+  componentDidMount() {
+    this.updateCanvasSize();
+    this.redraw();
+    if (this.props.mapRef?.current) {
+      this.props.mapRef.current.on('move', this.redraw);
+      this.props.mapRef.current.on('moveend', this.redraw);
+    }
+  }
+
+  componentDidUpdate(prevProps: ScatterPlotGlowOverlayProps) {
+    const shouldUpdateSize = prevProps.isDragging !== this.props.isDragging;
+    if (shouldUpdateSize) {
+      this.updateCanvasSize();
+    }
+
+    const shouldRedraw =
+      prevProps.locations !== this.props.locations ||
+      prevProps.dotRadius !== this.props.dotRadius ||
+      prevProps.rgb !== this.props.rgb ||
+      prevProps.aggregation !== this.props.aggregation ||
+      prevProps.compositeOperation !== this.props.compositeOperation ||
+      prevProps.pointRadiusUnit !== this.props.pointRadiusUnit ||
+      prevProps.zoom !== this.props.zoom;
+
+    if (shouldRedraw) {
+      this.redraw();
+    }
+  }
+
+  componentWillUnmount() {
+    if (this.props.mapRef?.current) {
+      this.props.mapRef.current.off('move', this.redraw);
+      this.props.mapRef.current.off('moveend', this.redraw);
+    }
+  }
+
+  updateCanvasSize() {
+    const canvas = this.canvasRef.current;
+    const container = this.containerRef.current;
+    if (!canvas || !container) return;
+
+    const { width, height } = container.getBoundingClientRect();
+    if (canvas.width !== width || canvas.height !== height) {
+      canvas.width = width;
+      canvas.height = height;
+    }
+  }
+
+  drawText(
+    ctx: CanvasRenderingContext2D,
+    pixel: [number, number],
+    options: DrawTextOptions = {},
+  ) {
+    const {
+      fontHeight = 0,
+      label = '',
+      radius = 0,
+      rgb = DEFAULT_RGB_COLOR,
+      shadow = false,
+    } = options;
+    const maxWidth = radius * TEXT_WIDTH_RATIO;
+    const luminance = luminanceFromRGB(rgb[0], rgb[1], rgb[2]);
+
+    ctx.globalCompositeOperation = 'source-over';
+    ctx.fillStyle = luminance <= LUMINANCE_THRESHOLD_DARK ? 'white' : 'black';
+    ctx.font = `${fontHeight}px sans-serif`;
+    ctx.textAlign = 'center';
+    ctx.textBaseline = 'middle';
+    if (shadow) {
+      ctx.shadowBlur = 15;
+      ctx.shadowColor = luminance <= LUMINANCE_THRESHOLD_DARK ? 'black' : '';
+    }
+
+    const textWidth = ctx.measureText(String(label)).width;
+    if (textWidth > maxWidth) {
+      const scale = fontHeight / textWidth;
+      ctx.font = `${scale * maxWidth}px sans-serif`;
+    }
+
+    const { compositeOperation } = this.props;
+
+    ctx.fillText(String(label), pixel[0], pixel[1]);
+    ctx.globalCompositeOperation = compositeOperation || 'source-over';
+    ctx.shadowBlur = 0;
+    ctx.shadowColor = '';

Review Comment:
   **Suggestion:** Using an empty string for canvas shadow colors is not a 
valid CSS color and may be treated inconsistently by browsers; use a valid 
color like 'transparent' to clear the shadow color and avoid relying on 
empty-string behavior. [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
         ctx.shadowColor = luminance <= LUMINANCE_THRESHOLD_DARK ? 'black' : 
'transparent';
       }
   
       const textWidth = ctx.measureText(String(label)).width;
       if (textWidth > maxWidth) {
         const scale = fontHeight / textWidth;
         ctx.font = `${scale * maxWidth}px sans-serif`;
       }
   
       const { compositeOperation } = this.props;
   
       ctx.fillText(String(label), pixel[0], pixel[1]);
       ctx.globalCompositeOperation = compositeOperation || 'source-over';
       ctx.shadowBlur = 0;
       ctx.shadowColor = 'transparent';
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   canvasRenderingContext2D.shadowColor expects a CSS color; using an empty 
string is ambiguous across engines. Replacing '' with 'transparent' (or 
resetting to a definite color) is a small, correct defensive change that avoids 
inconsistent behavior when clearing shadows.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx
   **Line:** 225:239
   **Comment:**
        *Possible Bug: Using an empty string for canvas shadow colors is not a 
valid CSS color and may be treated inconsistently by browsers; use a valid 
color like 'transparent' to clear the shadow color and avoid relying on 
empty-string behavior.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/MapBox.test.tsx:
##########
@@ -0,0 +1,466 @@
+/**
+ * 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 { render, waitFor } from 'spec/helpers/testing-library';
+import MapBox, {
+  Clusterer,
+  DEFAULT_MAX_ZOOM,
+  DEFAULT_POINT_RADIUS,
+} from '../src/MapBox';
+import { type Location } from '../src/ScatterPlotGlowOverlay';
+
+type BBox = [number, number, number, number];
+
+const mockMap = {
+  on: jest.fn(),
+  off: jest.fn(),
+  project: jest.fn((coords: [number, number]) => ({
+    x: coords[0],
+    y: coords[1],
+  })),
+  getCanvas: jest.fn(() => ({
+    style: {},
+  })),
+  getContainer: jest.fn(() => document.createElement('div')),
+};
+
+const MockMap = ({ children, onMove, onIdle }: any) => {
+  const { useEffect } = require('react');
+  useEffect(() => {
+    onIdle?.();
+  }, [onIdle]);
+  return (
+    <div
+      data-test="mapbox-mock"
+      onClick={() =>

Review Comment:
   **Suggestion:** The test helper `MockMap` only sets 
`data-test="mapbox-mock"` while some test queries use `getByTestId` (which 
commonly looks for `data-testid`); this mismatch can cause tests to fail 
intermittently depending on test-library configuration. Add 
`data-testid="mapbox-mock"` to the mocked element to ensure both query styles 
succeed. [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
         data-testid="mapbox-mock"
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   The test file calls getByTestId in multiple places while the mock only 
provides data-test.
   getByTestId looks for data-testid by default, so adding 
data-testid="mapbox-mock" makes the mock robust
   across different testing-library configurations. It's a small, safe change 
that resolves a real mismatch.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/test/MapBox.test.tsx
   **Line:** 51:51
   **Comment:**
        *Possible Bug: The test helper `MockMap` only sets 
`data-test="mapbox-mock"` while some test queries use `getByTestId` (which 
commonly looks for `data-testid`); this mismatch can cause tests to fail 
intermittently depending on test-library configuration. Add 
`data-testid="mapbox-mock"` to the mocked element to ensure both query styles 
succeed.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/utils/geo.ts:
##########
@@ -22,7 +22,11 @@ import roundDecimal from './roundDecimal';
 export const EARTH_CIRCUMFERENCE_KM = 40075.16;
 export const MILES_PER_KM = 1.60934;
 
-export function kmToPixels(kilometers, latitude, zoomLevel) {
+export function kmToPixels(
+  kilometers: number,
+  latitude: number,
+  zoomLevel: number,
+): number {
   // Algorithm from: http://wiki.openstreetmap.org/wiki/Zoom_levels
   const latitudeRad = latitude * (Math.PI / 180);
   // Seems like the zoomLevel is off by one

Review Comment:
   **Suggestion:** Off-by-one in the denominator exponent: the map tile math 
uses a base tile size of 256 pixels (2^8), so dividing by 2 ** (zoomLevel + 9) 
makes the resolution one power-of-two too small and returns incorrect pixel 
counts; change the +9 offset to +8 to match the standard formula. [logic error]
   
   **Severity Level:** Minor ⚠️
   



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/types/external.d.ts:
##########
@@ -0,0 +1,48 @@
+/**
+ * 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.
+ */
+
+declare module '*.png' {
+  const value: string;
+  export default value;
+}
+
+declare module '*.jpg' {
+  const value: string;
+  export default value;
+}
+
+declare module '*.jpeg' {
+  const value: string;
+  export default value;
+}
+
+declare module '*.svg' {
+  const value: string;
+  export default value;

Review Comment:
   **Suggestion:** SVG imports are commonly used both as a URL string and as a 
React component (via SVGR). The current declaration only exports a default 
`string`, so importing an SVG as `ReactComponent` will produce a type error at 
compile time and break code that expects the named `ReactComponent` export. Add 
a named `ReactComponent` export with appropriate React SVG props in the `.svg` 
module declaration. [type error]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
     import * as React from 'react';
     const src: string;
     export default src;
     export const ReactComponent: React.FunctionComponent<
       React.SVGProps<SVGSVGElement> & { title?: string }
     >;
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   This is a real, practical TypeScript issue in projects that use SVGR: 
importing { ReactComponent as Icon } from './icon.svg' will be a type error 
with the current declaration. Adding a typed ReactComponent export fixes 
compile-time errors and aligns with common bundler setups. The proposed 
improved code is correct and safe for codebases that use SVGR.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/types/external.d.ts
   **Line:** 36:37
   **Comment:**
        *Type Error: SVG imports are commonly used both as a URL string and as 
a React component (via SVGR). The current declaration only exports a default 
`string`, so importing an SVG as `ReactComponent` will produce a type error at 
compile time and break code that expects the named `ReactComponent` export. Add 
a named `ReactComponent` export with appropriate React SVG props in the `.svg` 
module declaration.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/transformProps.ts:
##########
@@ -16,12 +16,80 @@
  * specific language governing permissions and limitations
  * under the License.
  */
+import { ChartProps } from '@superset-ui/core';
 import Supercluster from 'supercluster';
 import { DEFAULT_POINT_RADIUS, DEFAULT_MAX_ZOOM } from './MapBox';
 
 const NOOP = () => {};
 
-export default function transformProps(chartProps) {
+export interface FormData {
+  clusteringRadius: number;
+  globalOpacity: number;
+  mapboxColor: string;
+  mapboxStyle: string;
+  pandasAggfunc: string;
+  pointRadius: number | string;
+  pointRadiusUnit: string;
+  renderWhileDragging: boolean;
+}
+
+interface ViewportChangeParams {
+  latitude: number;
+  longitude: number;
+  zoom: number;
+}
+
+interface ClusterOptions {
+  maxZoom: number;
+  radius: number;
+  initial?: () => {
+    sum: number;
+    squaredSum: number;
+    min: number;
+    max: number;
+  };
+  map?: (prop: { metric: number }) => {
+    sum: number;
+    squaredSum: number;
+    min: number;
+    max: number;
+  };
+  reduce?: (
+    accu: {
+      sum: number;
+      squaredSum: number;
+      min: number;
+      max: number;
+    },
+    prop: {
+      sum: number;
+      squaredSum: number;
+      min: number;
+      max: number;
+    },
+  ) => void;
+}
+
+interface TransformedProps {
+  width?: number;
+  height?: number;
+  aggregatorName?: string;
+  bounds?: [[number, number], [number, number]];
+  clusterer?: Supercluster;
+  globalOpacity?: number;
+  hasCustomMetric?: boolean;
+  mapboxApiKey?: string;
+  mapStyle?: string;
+  onViewportChange?: (params: ViewportChangeParams) => void;
+  pointRadius?: number;
+  pointRadiusUnit?: string;
+  renderWhileDragging?: boolean;
+  rgb?: [number, number, number, number];
+}
+
+export default function transformProps(
+  chartProps: ChartProps<FormData>,
+): TransformedProps {
   const { width, height, formData, hooks, queriesData } = chartProps;
   const { onError = NOOP, setControlValue = NOOP } = hooks;

Review Comment:
   **Suggestion:** Unsafe access of query results: `queriesData[0].data` is 
used without checking that `queriesData` exists, that it has at least one 
element, or that `.data` is defined — this can raise a runtime TypeError when 
charts are rendered with missing/empty query results. Guard the access and 
provide sensible defaults. [null pointer]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
     const queryData = queriesData?.[0]?.data ?? {};
     const { bounds, geoJSON, hasCustomMetric = false, mapboxApiKey } = 
queryData;
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   The PR currently assumes queriesData[0].data is always present — that's a 
real runtime footgun.
   Guarding the access (optional chaining / fallback) prevents a TypeError when 
charts are rendered
   with missing/empty query results. The suggested change is correct and 
actionable, but note that
   downstream code still uses geoJSON and other fields, so simply defaulting to 
{} may require
   additional defensive checks elsewhere.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/src/transformProps.ts
   **Line:** 93:94
   **Comment:**
        *Null Pointer: Unsafe access of query results: `queriesData[0].data` is 
used without checking that `queriesData` exists, that it has at least one 
element, or that `.data` is defined — this can raise a runtime TypeError when 
charts are rendered with missing/empty query results. Guard the access and 
provide sensible defaults.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/transformProps.ts:
##########
@@ -85,12 +175,13 @@ export default function transformProps(chartProps) {
     hasCustomMetric,
     mapboxApiKey,
     mapStyle: mapboxStyle,
-    onViewportChange({ latitude, longitude, zoom }) {
+    onViewportChange({ latitude, longitude, zoom }: ViewportChangeParams) {
       setControlValue('viewport_longitude', longitude);
       setControlValue('viewport_latitude', latitude);
       setControlValue('viewport_zoom', zoom);
     },
-    pointRadius: pointRadius === 'Auto' ? DEFAULT_POINT_RADIUS : pointRadius,
+    pointRadius:
+      pointRadius === 'Auto' ? DEFAULT_POINT_RADIUS : Number(pointRadius),
     pointRadiusUnit,

Review Comment:
   **Suggestion:** `pointRadius` is coerced with `Number(pointRadius)` without 
validating the result; non-numeric strings produce NaN which may break 
consumers — coerce and validate to a finite number, falling back to 
DEFAULT_POINT_RADIUS when invalid. [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
         (() => {
           const pr =
             pointRadius === 'Auto' ? DEFAULT_POINT_RADIUS : 
Number(pointRadius);
           return Number.isFinite(pr) ? pr : DEFAULT_POINT_RADIUS;
         })(),
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Coercing a non-numeric string with Number(...) yields NaN which will likely 
break consumers. The IIFE
   that falls back to DEFAULT_POINT_RADIUS when Number.isFinite is false is a 
safe, localized improvement.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/src/transformProps.ts
   **Line:** 184:185
   **Comment:**
        *Possible Bug: `pointRadius` is coerced with `Number(pointRadius)` 
without validating the result; non-numeric strings produce NaN which may break 
consumers — coerce and validate to a finite number, falling back to 
DEFAULT_POINT_RADIUS when invalid.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/common.tsx:
##########
@@ -37,7 +37,7 @@ import {
   QueryFormData,
   SetDataMaskHook,
 } from '@superset-ui/core';
-import { Layer, PickingInfo, Color } from '@deck.gl/core';
+import { PickingInfo, Color } from '@deck.gl/core';

Review Comment:
   **Suggestion:** The code imports `PickingInfo` and `Color` as runtime values 
from '@deck.gl/core' even though they are only used as TypeScript types; if 
those symbols are not exported as runtime values this will throw at runtime. 
Use a type-only import to ensure the import is erased at runtime. [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
   import type { PickingInfo, Color } from '@deck.gl/core';
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Both PickingInfo and Color are only used as types in this file. Using a 
type-only import (import type ...) prevents generating a runtime binding that 
might not exist at runtime and avoids potential import errors. This is a 
correct, minimal, and safe change.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/common.tsx
   **Line:** 40:40
   **Comment:**
        *Possible Bug: The code imports `PickingInfo` and `Color` as runtime 
values from '@deck.gl/core' even though they are only used as TypeScript types; 
if those symbols are not exported as runtime values this will throw at runtime. 
Use a type-only import to ensure the import is erased at runtime.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/ScatterPlotGlowOverlay.test.tsx:
##########
@@ -0,0 +1,586 @@
+/**
+ * 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 { render } from 'spec/helpers/testing-library';
+import ScatterPlotGlowOverlay, {
+  AggregationType,
+} from '../src/ScatterPlotGlowOverlay';
+
+jest.mock('react-map-gl/mapbox', () => ({
+  useMap: () => ({
+    current: {
+      project: (coords: [number, number]) => ({
+        x: coords[0] * 10,
+        y: coords[1] * 10,
+      }),
+      on: jest.fn(),
+      off: jest.fn(),
+    },
+  }),
+}));
+
+const mockLocation = {
+  geometry: {
+    coordinates: [10, 20] as [number, number],
+    type: 'Point',
+  },
+  properties: {},
+};
+
+const mockClusterLocation = {
+  geometry: {
+    coordinates: [10, 20] as [number, number],
+    type: 'Point',
+  },
+  properties: {
+    cluster: true,
+    point_count: 10,
+    sum: 100,
+    squaredSum: 1200,
+    min: 5,
+    max: 20,
+  },
+};
+
+test('renders without crashing', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders canvas element', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 128, 64, 255]}
+    />,
+  );
+
+  const canvas = container.querySelector('canvas');
+  expect(canvas).toBeInTheDocument();
+  expect(canvas?.tagName).toBe('CANVAS');
+});
+
+test('renders with default props', () => {
+  const { container } = render(<ScatterPlotGlowOverlay locations={[]} />);
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with empty locations array', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with single location', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with multiple locations', () => {
+  const locations = [mockLocation, { ...mockLocation }];
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={locations} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with cluster location', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label as point_count when no aggregation specified', () 
=> {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using sum aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      ...mockClusterLocation.properties,
+      sum: 150,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using min aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      ...mockClusterLocation.properties,
+      min: 5,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="min"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using max aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      ...mockClusterLocation.properties,
+      max: 20,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="max"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using mean aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+      sum: 100,
+      squaredSum: 1200,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="mean"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using variance aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+      sum: 100,
+      squaredSum: 1200,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="var"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using stdev aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+      sum: 100,
+      squaredSum: 1200,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="stdev"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses white text color on dark background (low luminance)', () => {
+  const darkRgb: [number, number, number, number] = [255, 10, 10, 10];
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={darkRgb}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses black text color on light background (high luminance)', () => {
+  const lightRgb: [number, number, number, number] = [255, 240, 240, 240];
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={lightRgb}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders point with custom radius', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: 50,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      dotRadius={100}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders point with metric label', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      metric: 42.5,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders point with null radius (uses default)', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: null,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles kilometers pointRadiusUnit', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: 10,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      pointRadiusUnit="Kilometers"
+      zoom={10}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles miles pointRadiusUnit', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: 10,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      pointRadiusUnit="Miles"
+      zoom={10}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses custom lngLatAccessor', () => {
+  const customAccessor = () => [50, 60] as [number, number];
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      lngLatAccessor={customAccessor}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles isDragging with renderWhileDragging true', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      isDragging
+      renderWhileDragging
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles isDragging with renderWhileDragging false', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      isDragging
+      renderWhileDragging={false}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses custom composite operation', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      compositeOperation="screen"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with all aggregation types', () => {
+  const aggregations: AggregationType[] = [
+    'sum',
+    'min',
+    'max',
+    'mean',
+    'var',
+    'stdev',
+  ];
+
+  aggregations.forEach(aggregation => {
+    const { container } = render(
+      <ScatterPlotGlowOverlay
+        locations={[mockClusterLocation]}
+        rgb={[255, 0, 0, 255]}
+        aggregation={aggregation}
+      />,
+    );
+
+    expect(container.querySelector('canvas')).toBeInTheDocument();
+  });
+});
+
+test('formats large cluster labels with "k" suffix (>= 10000)', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 15000,
+      sum: 15000,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('formats cluster labels between 1000-9999 with one decimal "k" suffix', 
() => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 2500,
+      sum: 2500,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('does not format cluster labels below 1000', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 999,
+      sum: 999,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles missing aggregation values gracefully', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles non-numeric metric values', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      metric: 'N/A',
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('overlay container has correct positioning styles', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  const overlayDiv = container.firstChild as HTMLElement;

Review Comment:
   **Suggestion:** The test asserts styles on the overlay element but casts 
`container.firstChild` directly to `HTMLElement` without checking it exists; if 
the component failed to render the container, the test will throw a runtime 
error. First assert the node is present with `toBeInTheDocument()` and only 
then cast and assert styles. [null pointer]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
     expect(container.firstChild).toBeInTheDocument();
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Good, defensive change: asserting the node exists before casting avoids a 
potential null dereference and yields a clearer test failure. The rest of the 
test suite already uses toBeInTheDocument(), so this is consistent and safe.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/test/ScatterPlotGlowOverlay.test.tsx
   **Line:** 560:560
   **Comment:**
        *Null Pointer: The test asserts styles on the overlay element but casts 
`container.firstChild` directly to `HTMLElement` without checking it exists; if 
the component failed to render the container, the test will throw a runtime 
error. First assert the node is present with `toBeInTheDocument()` and only 
then cast and assert styles.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/ScatterPlotGlowOverlay.test.tsx:
##########
@@ -0,0 +1,586 @@
+/**
+ * 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 { render } from 'spec/helpers/testing-library';
+import ScatterPlotGlowOverlay, {
+  AggregationType,
+} from '../src/ScatterPlotGlowOverlay';
+
+jest.mock('react-map-gl/mapbox', () => ({
+  useMap: () => ({
+    current: {
+      project: (coords: [number, number]) => ({
+        x: coords[0] * 10,
+        y: coords[1] * 10,
+      }),
+      on: jest.fn(),
+      off: jest.fn(),
+    },
+  }),
+}));
+
+const mockLocation = {
+  geometry: {
+    coordinates: [10, 20] as [number, number],
+    type: 'Point',
+  },
+  properties: {},
+};
+
+const mockClusterLocation = {
+  geometry: {
+    coordinates: [10, 20] as [number, number],
+    type: 'Point',
+  },
+  properties: {
+    cluster: true,
+    point_count: 10,
+    sum: 100,
+    squaredSum: 1200,
+    min: 5,
+    max: 20,
+  },
+};
+
+test('renders without crashing', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders canvas element', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 128, 64, 255]}
+    />,
+  );
+
+  const canvas = container.querySelector('canvas');
+  expect(canvas).toBeInTheDocument();
+  expect(canvas?.tagName).toBe('CANVAS');
+});
+
+test('renders with default props', () => {
+  const { container } = render(<ScatterPlotGlowOverlay locations={[]} />);
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with empty locations array', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with single location', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with multiple locations', () => {
+  const locations = [mockLocation, { ...mockLocation }];
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={locations} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with cluster location', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label as point_count when no aggregation specified', () 
=> {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using sum aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      ...mockClusterLocation.properties,
+      sum: 150,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using min aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      ...mockClusterLocation.properties,
+      min: 5,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="min"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using max aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      ...mockClusterLocation.properties,
+      max: 20,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="max"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using mean aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+      sum: 100,
+      squaredSum: 1200,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="mean"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using variance aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+      sum: 100,
+      squaredSum: 1200,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="var"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('computes cluster label using stdev aggregation', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+      sum: 100,
+      squaredSum: 1200,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="stdev"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses white text color on dark background (low luminance)', () => {
+  const darkRgb: [number, number, number, number] = [255, 10, 10, 10];
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={darkRgb}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses black text color on light background (high luminance)', () => {
+  const lightRgb: [number, number, number, number] = [255, 240, 240, 240];
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockClusterLocation]}
+      rgb={lightRgb}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders point with custom radius', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: 50,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      dotRadius={100}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders point with metric label', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      metric: 42.5,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders point with null radius (uses default)', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: null,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles kilometers pointRadiusUnit', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: 10,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      pointRadiusUnit="Kilometers"
+      zoom={10}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles miles pointRadiusUnit', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      radius: 10,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      pointRadiusUnit="Miles"
+      zoom={10}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses custom lngLatAccessor', () => {
+  const customAccessor = () => [50, 60] as [number, number];
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      lngLatAccessor={customAccessor}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles isDragging with renderWhileDragging true', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      isDragging
+      renderWhileDragging
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles isDragging with renderWhileDragging false', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      isDragging
+      renderWhileDragging={false}
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('uses custom composite operation', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+      compositeOperation="screen"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('renders with all aggregation types', () => {
+  const aggregations: AggregationType[] = [
+    'sum',
+    'min',
+    'max',
+    'mean',
+    'var',
+    'stdev',
+  ];
+
+  aggregations.forEach(aggregation => {
+    const { container } = render(
+      <ScatterPlotGlowOverlay
+        locations={[mockClusterLocation]}
+        rgb={[255, 0, 0, 255]}
+        aggregation={aggregation}
+      />,
+    );
+
+    expect(container.querySelector('canvas')).toBeInTheDocument();
+  });
+});
+
+test('formats large cluster labels with "k" suffix (>= 10000)', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 15000,
+      sum: 15000,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('formats cluster labels between 1000-9999 with one decimal "k" suffix', 
() => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 2500,
+      sum: 2500,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('does not format cluster labels below 1000', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 999,
+      sum: 999,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles missing aggregation values gracefully', () => {
+  const location = {
+    ...mockClusterLocation,
+    properties: {
+      cluster: true,
+      point_count: 10,
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[location]}
+      rgb={[255, 0, 0, 255]}
+      aggregation="sum"
+    />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('handles non-numeric metric values', () => {
+  const location = {
+    ...mockLocation,
+    properties: {
+      metric: 'N/A',
+    },
+  };
+
+  const { container } = render(
+    <ScatterPlotGlowOverlay locations={[location]} rgb={[255, 0, 0, 255]} />,
+  );
+
+  expect(container.querySelector('canvas')).toBeInTheDocument();
+});
+
+test('overlay container has correct positioning styles', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  const overlayDiv = container.firstChild as HTMLElement;
+  expect(overlayDiv).toHaveStyle({
+    position: 'absolute',
+    left: '0px',
+    top: '0px',
+    width: '100%',
+    height: '100%',
+    pointerEvents: 'none',
+  });
+});
+
+test('canvas has correct positioning styles', () => {
+  const { container } = render(
+    <ScatterPlotGlowOverlay
+      locations={[mockLocation]}
+      rgb={[255, 0, 0, 255]}
+    />,
+  );
+
+  const canvas = container.querySelector('canvas') as HTMLCanvasElement;

Review Comment:
   **Suggestion:** The test queries the canvas and immediately calls 
`toHaveStyle` after casting to `HTMLCanvasElement`, but if 
`querySelector('canvas')` returns null the assertion call will throw; ensure 
the canvas exists first by asserting `toBeInTheDocument()` on the query result, 
then retrieve and assert styles. [null pointer]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
     expect(container.querySelector('canvas')).toBeInTheDocument();
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Also a sensible defensive improvement: verify the canvas is present before 
casting and checking styles to avoid throwing on null. The project already 
includes toBeInTheDocument() usage elsewhere, so this change is consistent and 
low risk.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/test/ScatterPlotGlowOverlay.test.tsx
   **Line:** 579:579
   **Comment:**
        *Null Pointer: The test queries the canvas and immediately calls 
`toHaveStyle` after casting to `HTMLCanvasElement`, but if 
`querySelector('canvas')` returns null the assertion call will throw; ensure 
the canvas exists first by asserting `toBeInTheDocument()` on the query result, 
then retrieve and assert styles.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/utils/geo.test.ts:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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 {
+  kmToPixels,
+  EARTH_CIRCUMFERENCE_KM,
+  MILES_PER_KM,
+} from '../../src/utils/geo';
+
+test('converts kilometers to pixels at equator (0 degrees) at zoom level 0', 
() => {
+  const result = kmToPixels(100, 0, 0);
+  expect(result).toBeGreaterThan(0);
+  expect(typeof result).toBe('number');
+});
+
+test('converts kilometers to pixels at equator (0 degrees) at zoom level 10', 
() => {
+  const km = 10;
+  const result = kmToPixels(km, 0, 10);
+  expect(result).toBeGreaterThan(0);
+  expect(typeof result).toBe('number');
+});
+
+test('higher zoom levels result in more pixels per kilometer', () => {
+  const km = 10;
+  const latitude = 0;
+  const lowZoom = kmToPixels(km, latitude, 5);
+  const highZoom = kmToPixels(km, latitude, 10);
+
+  expect(highZoom).toBeGreaterThan(lowZoom);
+});
+
+test('same distance at higher latitude results in more pixels (Mercator 
distortion)', () => {
+  const km = 10;
+  const zoom = 10;
+  const equatorPixels = kmToPixels(km, 0, zoom);
+  const midLatPixels = kmToPixels(km, 45, zoom);
+
+  expect(midLatPixels).toBeGreaterThan(equatorPixels);
+});
+
+test('handles negative latitudes correctly', () => {
+  const km = 10;
+  const zoom = 10;
+  const northPixels = kmToPixels(km, 45, zoom);
+  const southPixels = kmToPixels(km, -45, zoom);
+
+  expect(northPixels).toBeCloseTo(southPixels, 2);
+});
+
+test('uses correct Earth circumference constant', () => {
+  expect(EARTH_CIRCUMFERENCE_KM).toBe(40075.16);
+});
+
+test('uses correct miles to kilometers conversion factor', () => {
+  expect(MILES_PER_KM).toBe(1.60934);
+});
+
+test('returns rounded result with 2 decimal places', () => {
+  const result = kmToPixels(10, 45, 10);
+  const decimals = (result.toString().split('.')[1] || '').length;
+  expect(decimals).toBeLessThanOrEqual(2);
+});
+
+test('handles zero kilometers', () => {
+  const result = kmToPixels(0, 0, 10);
+  expect(result).toBe(0);
+});
+
+test('handles very small distances', () => {
+  const result = kmToPixels(0.001, 0, 10);
+  expect(result).toBeGreaterThanOrEqual(0);
+  expect(typeof result).toBe('number');
+});
+
+test('handles large distances', () => {
+  const result = kmToPixels(10000, 0, 5);
+  expect(result).toBeGreaterThan(0);
+  expect(typeof result).toBe('number');
+});
+
+test('handles poles (90 degrees latitude)', () => {
+  const result = kmToPixels(10, 90, 10);
+  expect(result).toBeGreaterThanOrEqual(0);
+  expect(typeof result).toBe('number');
+});
+
+test('handles poles (-90 degrees latitude)', () => {
+  const result = kmToPixels(10, -90, 10);
+  expect(result).toBeGreaterThanOrEqual(0);
+  expect(typeof result).toBe('number');

Review Comment:
   **Suggestion:** The Mercator-based formula used by `kmToPixels` can produce 
division-by-zero (cos(±90°) == 0) resulting in Infinity or NaN; current tests 
only check `typeof` and non-negativity which permit Infinity. Replace both pole 
tests with checks that the function returns a finite number (or explicitly 
handle/clamp poles in the implementation). [logic error]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
   test('handles poles (±90 degrees latitude) returns finite numbers', () => {
     const northResult = kmToPixels(10, 90, 10);
     const southResult = kmToPixels(10, -90, 10);
     expect(Number.isFinite(northResult)).toBe(true);
     expect(Number.isFinite(southResult)).toBe(true);
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   The current assertions (>= 0 and typeof 'number') will pass for Infinity, 
which is not useful. Asserting Number.isFinite(...) is a stronger, meaningful 
check and will catch cases where the implementation divides by zero and returns 
Infinity/NaN. This is a valid improvement to test robustness.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/test/utils/geo.test.ts
   **Line:** 97:106
   **Comment:**
        *Logic Error: The Mercator-based formula used by `kmToPixels` can 
produce division-by-zero (cos(±90°) == 0) resulting in Infinity or NaN; current 
tests only check `typeof` and non-negativity which permit Infinity. Replace 
both pole tests with checks that the function returns a finite number (or 
explicitly handle/clamp poles in the implementation).
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/spec/__mocks__/supercluster.ts:
##########
@@ -0,0 +1,33 @@
+/**
+ * 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.
+ */
+
+// Mock for supercluster package
+export default class Supercluster {
+  load() {

Review Comment:
   **Suggestion:** The mock `load` method has no parameter signature; when the 
real code calls `load(points)` TypeScript callers will get a type/signature 
mismatch (or tooling may complain) and any passed arguments are ignored—add a 
parameter with a permissive type and an explicit return type of `this` to match 
the real API. [type error]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
     load(points: any[] = []): this {
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   The mock currently omits the real API signature. Adding an optional points 
parameter and an explicit `this` return type matches upstream Supercluster, 
prevents TypeScript signature mismatches, and is low-risk (the suggested 
default preserves existing tests).
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/spec/__mocks__/supercluster.ts
   **Line:** 22:22
   **Comment:**
        *Type Error: The mock `load` method has no parameter signature; when 
the real code calls `load(points)` TypeScript callers will get a type/signature 
mismatch (or tooling may complain) and any passed arguments are ignored—add a 
parameter with a permissive type and an explicit return type of `this` to match 
the real API.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/spec/__mocks__/supercluster.ts:
##########
@@ -0,0 +1,33 @@
+/**
+ * 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.
+ */
+
+// Mock for supercluster package
+export default class Supercluster {
+  load() {
+    return this;
+  }
+
+  getClusters() {

Review Comment:
   **Suggestion:** The mock `getClusters` method has no parameters; callers 
that pass `bbox` and `zoom` will either trigger TypeScript signature errors or 
have their arguments ignored at runtime—add parameters with permissive types 
and an explicit return type to mirror the real API. [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
     getClusters(bbox: [number, number, number, number] = [0, 0, 0, 0], zoom: 
number = 0): any[] {
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Mirroring the real API (bbox and zoom) improves type accuracy and avoids 
ignored arguments. The suggested defaults keep existing tests working while 
providing correct typings for callers.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** superset-frontend/spec/__mocks__/supercluster.ts
   **Line:** 26:26
   **Comment:**
        *Possible Bug: The mock `getClusters` method has no parameters; callers 
that pass `bbox` and `zoom` will either trigger TypeScript signature errors or 
have their arguments ignored at runtime—add parameters with permissive types 
and an explicit return type to mirror the real API.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/transformProps.test.ts:
##########
@@ -0,0 +1,464 @@
+/**
+ * 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 { ChartProps, DatasourceType } from '@superset-ui/core';
+import { supersetTheme } from '@apache-superset/core/ui';
+import transformProps, { FormData } from '../src/transformProps';
+
+jest.mock('supercluster');
+
+const createMockChartProps = (
+  overrides: Partial<ChartProps<FormData>> = {},
+): ChartProps<FormData> => {
+  const {
+    formData: formDataOverrides,
+    queriesData: queriesDataOverrides,
+    hooks: hooksOverrides,
+    ...restOverrides
+  } = overrides;
+  return {
+    annotationData: {},
+    initialValues: {},
+    ownState: {},
+    filterState: {},
+    behaviors: [],
+    theme: supersetTheme,
+    width: 800,
+    height: 600,
+    datasource: {
+      id: 1,
+      name: 'test_table',
+      type: DatasourceType.Table,
+      columns: [],
+      metrics: [],
+      columnFormats: {},
+      verboseMap: {},
+    },
+    rawDatasource: {},
+    rawFormData: {
+      clusteringRadius: 60,
+      globalOpacity: 1,
+      mapboxColor: 'rgb(0, 0, 0)',
+      mapboxStyle: 'mapbox://styles/mapbox/streets-v11',
+      pandasAggfunc: 'sum',
+      pointRadius: 60,
+      pointRadiusUnit: 'Pixels',
+      renderWhileDragging: true,
+    },
+    hooks: {
+      onError: jest.fn(),
+      setControlValue: jest.fn(),
+      ...hooksOverrides,
+    },
+    formData: {
+      clusteringRadius: 60,
+      globalOpacity: 1,
+      mapboxColor: 'rgb(0, 0, 0)',
+      mapboxStyle: 'mapbox://styles/mapbox/streets-v11',
+      pandasAggfunc: 'sum',
+      pointRadius: 60,
+      pointRadiusUnit: 'Pixels',
+      renderWhileDragging: true,
+      ...formDataOverrides,
+    },
+    queriesData: [
+      {
+        data: {
+          bounds: [
+            [-122.5, 37.5],
+            [-122.0, 38.0],
+          ] as [[number, number], [number, number]],
+          geoJSON: {
+            type: 'FeatureCollection',
+            features: [],
+          },
+          hasCustomMetric: false,
+          mapboxApiKey: 'test-api-key',
+          ...queriesDataOverrides?.[0]?.data,
+        },
+        colnames: [],
+        coltypes: [],
+        rowcount: 0,
+        ...queriesDataOverrides?.[0],

Review Comment:
   **Suggestion:** Logic bug: `queriesDataOverrides` only uses the first 
element (`[0]`) of any provided overrides array, silently ignoring additional 
query entries; when callers pass a full `queriesData` array it will not be 
preserved or merged correctly. Return the provided array if present (optionally 
merging defaults per entry) instead of only applying the first element. [logic 
error]
   
   **Severity Level:** Minor ⚠️
   ```suggestion
       queriesData: queriesDataOverrides?.length
         ? queriesDataOverrides
         : [
             {
               data: {
                 bounds: [
                   [-122.5, 37.5],
                   [-122.0, 38.0],
                 ] as [[number, number], [number, number]],
                 geoJSON: {
                   type: 'FeatureCollection',
                   features: [],
                 },
                 hasCustomMetric: false,
                 mapboxApiKey: 'test-api-key',
               },
               colnames: [],
               coltypes: [],
               rowcount: 0,
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   This is a legitimate logic bug: the helper only merges the first element of 
a provided queriesData array (queriesDataOverrides?.[0]) and ignores any 
additional entries. The improved code returns the provided array when present 
which preserves callers' intent and prevents surprises when tests or callers 
pass multiple query objects. This directly fixes a real correctness issue in 
the factory.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/test/transformProps.test.ts
   **Line:** 80:98
   **Comment:**
        *Logic Error: Logic bug: `queriesDataOverrides` only uses the first 
element (`[0]`) of any provided overrides array, silently ignoring additional 
query entries; when callers pass a full `queriesData` array it will not be 
preserved or merged correctly. Return the provided array if present (optionally 
merging defaults per entry) instead of only applying the first element.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/types/external.d.ts:
##########
@@ -0,0 +1,48 @@
+/**
+ * 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.
+ */
+
+declare module '*.png' {
+  const value: string;

Review Comment:
   **Suggestion:** Some bundlers (or frameworks like Next.js) expose image 
imports as an object (with `src`, `width`, `height`, etc.) instead of a plain 
string; typing image modules strictly as `string` can cause type mismatches at 
compile time. Broaden the image module export type to include the common object 
shape (or `any`) so both styles are accepted. [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
     const value: string | { src: string; width?: number; height?: number; 
blurDataURL?: string };
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Many frameworks/bundlers (Next.js, some webpack loaders) provide an object 
with src/width/height instead of a bare string. Widening the type to a union 
covers both behaviors and prevents spurious type errors in consumers. This 
addresses a tangible compatibility issue visible in similar projects.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/types/external.d.ts
   **Line:** 21:21
   **Comment:**
        *Possible Bug: Some bundlers (or frameworks like Next.js) expose image 
imports as an object (with `src`, `width`, `height`, etc.) instead of a plain 
string; typing image modules strictly as `string` can cause type mismatches at 
compile time. Broaden the image module export type to include the common object 
shape (or `any`) so both styles are accepted.
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/utils/geo.test.ts:
##########
@@ -0,0 +1,121 @@
+/**
+ * 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 {
+  kmToPixels,
+  EARTH_CIRCUMFERENCE_KM,
+  MILES_PER_KM,
+} from '../../src/utils/geo';
+
+test('converts kilometers to pixels at equator (0 degrees) at zoom level 0', 
() => {
+  const result = kmToPixels(100, 0, 0);
+  expect(result).toBeGreaterThan(0);
+  expect(typeof result).toBe('number');
+});
+
+test('converts kilometers to pixels at equator (0 degrees) at zoom level 10', 
() => {
+  const km = 10;
+  const result = kmToPixels(km, 0, 10);
+  expect(result).toBeGreaterThan(0);
+  expect(typeof result).toBe('number');
+});
+
+test('higher zoom levels result in more pixels per kilometer', () => {
+  const km = 10;
+  const latitude = 0;
+  const lowZoom = kmToPixels(km, latitude, 5);
+  const highZoom = kmToPixels(km, latitude, 10);
+
+  expect(highZoom).toBeGreaterThan(lowZoom);
+});
+
+test('same distance at higher latitude results in more pixels (Mercator 
distortion)', () => {
+  const km = 10;
+  const zoom = 10;
+  const equatorPixels = kmToPixels(km, 0, zoom);
+  const midLatPixels = kmToPixels(km, 45, zoom);
+
+  expect(midLatPixels).toBeGreaterThan(equatorPixels);
+});
+
+test('handles negative latitudes correctly', () => {
+  const km = 10;
+  const zoom = 10;
+  const northPixels = kmToPixels(km, 45, zoom);
+  const southPixels = kmToPixels(km, -45, zoom);
+
+  expect(northPixels).toBeCloseTo(southPixels, 2);
+});
+
+test('uses correct Earth circumference constant', () => {
+  expect(EARTH_CIRCUMFERENCE_KM).toBe(40075.16);
+});
+
+test('uses correct miles to kilometers conversion factor', () => {
+  expect(MILES_PER_KM).toBe(1.60934);
+});
+
+test('returns rounded result with 2 decimal places', () => {
+  const result = kmToPixels(10, 45, 10);
+  const decimals = (result.toString().split('.')[1] || '').length;
+  expect(decimals).toBeLessThanOrEqual(2);

Review Comment:
   **Suggestion:** The decimal-place check uses string formatting and can be 
brittle (exponential notation or edge cases); validate rounding numerically by 
asserting that rounding to 2 decimals reproduces the value (and ensure the 
result is finite). [possible bug]
   
   **Severity Level:** Critical 🚨
   ```suggestion
     expect(Number.isFinite(result)).toBe(true);
     expect(Math.round(result * 100) / 100).toBeCloseTo(result, 10);
   ```
   <details>
   <summary><b>Why it matters? ⭐ </b></summary>
   
   Counting decimal digits by converting to string can break for numbers 
formatted in exponential notation or edge cases. Verifying numerically that 
rounding to 2 decimals reproduces the value (or that the value equals the 
rounded value within a tiny epsilon) is more robust and directly verifies the 
intended behavior of roundDecimal.
   </details>
   <details>
   <summary><b>Prompt for AI Agent 🤖 </b></summary>
   
   ```mdx
   This is a comment left during a code review.
   
   **Path:** 
superset-frontend/plugins/legacy-plugin-chart-map-box/test/utils/geo.test.ts
   **Line:** 76:77
   **Comment:**
        *Possible Bug: The decimal-place check uses string formatting and can 
be brittle (exponential notation or edge cases); validate rounding numerically 
by asserting that rounding to 2 decimals reproduces the value (and ensure the 
result is finite).
   
   Validate the correctness of the flagged issue. If correct, How can I resolve 
this? If you propose a fix, implement it and please make it concise.
   ```
   </details>



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org