bito-code-review[bot] commented on code in PR #38035: URL: https://github.com/apache/superset/pull/38035#discussion_r2911299103
########## superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py: ########## @@ -0,0 +1,172 @@ +# 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. +"""migrate mapbox and deckgl charts to point_cluster_map + +Revision ID: ce6bd21901ab +Revises: 4b2a8c9d3e1f +Create Date: 2026-03-02 00:00:00.000000 + + +""" + +import copy +import logging +from typing import Any + +from alembic import op +from sqlalchemy.orm import Session + +from superset import db +from superset.migrations.shared.migrate_viz.base import ( + FORM_DATA_BAK_FIELD_NAME, + MigrateViz, + QUERIES_BAK_FIELD_NAME, + Slice, +) +from superset.migrations.shared.utils import try_load_json +from superset.utils import json + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "ce6bd21901ab" +down_revision = "a1b2c3d4e5f6" + +DECKGL_VIZ_TYPES = [ + "deck_arc", + "deck_contour", + "deck_geojson", + "deck_grid", + "deck_heatmap", + "deck_hex", + "deck_multi", + "deck_path", + "deck_polygon", + "deck_scatter", + "deck_screengrid", +] + + +class MigrateMapBox(MigrateViz): + """Migrate the legacy standalone Mapbox scatter chart to point_cluster_map.""" + + has_x_axis_control = False + source_viz_type = "mapbox" + target_viz_type = "point_cluster_map" + rename_keys = { + "mapbox_label": "map_label", + "mapbox_color": "map_color", + } + remove_keys = set() + + def _post_action(self) -> None: + # If the style URL is a mapbox:// URL, the chart was using Mapbox GL. + # Set map_renderer so the new chart continues to use the Mapbox renderer, + # which will pick up MAPBOX_API_KEY from the server config. + mapbox_style = self.data.get("mapbox_style", "") + if isinstance(mapbox_style, str) and mapbox_style.startswith("mapbox://"): + self.data["map_renderer"] = "mapbox" + + @classmethod + def upgrade_slice(cls, slc: Slice) -> None: + try: + clz = cls(slc.params) + form_data_bak = copy.deepcopy(clz.data) + + clz._pre_action() + clz._migrate() + clz._post_action() + + slc.viz_type = clz.target_viz_type + + backup: Any | dict[str, Any] = {FORM_DATA_BAK_FIELD_NAME: form_data_bak} + + query_context = try_load_json(slc.query_context) + queries_bak = None + + if query_context: + if "form_data" in query_context: + query_context["form_data"] = clz.data + queries_bak = copy.deepcopy(query_context.get("queries")) + else: + query_context = {} + + slc.query_context = json.dumps(query_context) + if queries_bak is not None: + backup[QUERIES_BAK_FIELD_NAME] = queries_bak + + slc.params = json.dumps({**clz.data, **backup}) + + except Exception as e: + logger.warning("Failed to migrate slice %s: %s", slc.id, e) + + +def _migrate_deckgl_slice(slc: Slice) -> bool: + """Set map_renderer='mapbox' for all existing deck.gl slices. + + This ensures full backwards compatibility: existing charts keep using the + Mapbox renderer. Users can later switch to MapLibre in the chart controls. + Only new charts will default to MapLibre. + + Returns True if the slice was modified. + """ + params = try_load_json(slc.params) + if not params: + return False + + if "map_renderer" in params: + return False + + params["map_renderer"] = "mapbox" + slc.params = json.dumps(params) + return True + + +def _downgrade_deckgl_slice(slc: Slice) -> bool: + """Reverse _migrate_deckgl_slice. Returns True if the slice was modified.""" + params = try_load_json(slc.params) + if not params or "map_renderer" not in params: + return False + + params.pop("map_renderer", None) + slc.params = json.dumps(params) + return True + + +def _migrate_deckgl_slices(session: Session, *, upgrade: bool) -> None: + fn = _migrate_deckgl_slice if upgrade else _downgrade_deckgl_slice + slices = session.query(Slice).filter(Slice.viz_type.in_(DECKGL_VIZ_TYPES)).all() + for slc in slices: + try: + fn(slc) + except Exception as e: Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Blind exception catch in upgrade</b></div> <div id="fix"> Catching bare `Exception` on line 114 is too broad. Catch specific exceptions like `ValueError`, `KeyError`, or `json.JSONDecodeError` to handle only expected errors. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion except (ValueError, KeyError, TypeError) as e: ```` </div> </details> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py: ########## @@ -0,0 +1,172 @@ +# 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. +"""migrate mapbox and deckgl charts to point_cluster_map + +Revision ID: ce6bd21901ab +Revises: 4b2a8c9d3e1f +Create Date: 2026-03-02 00:00:00.000000 + + +""" + +import copy +import logging +from typing import Any + +from alembic import op +from sqlalchemy.orm import Session + +from superset import db +from superset.migrations.shared.migrate_viz.base import ( + FORM_DATA_BAK_FIELD_NAME, + MigrateViz, + QUERIES_BAK_FIELD_NAME, + Slice, +) +from superset.migrations.shared.utils import try_load_json +from superset.utils import json + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "ce6bd21901ab" +down_revision = "a1b2c3d4e5f6" + +DECKGL_VIZ_TYPES = [ + "deck_arc", + "deck_contour", + "deck_geojson", + "deck_grid", + "deck_heatmap", + "deck_hex", + "deck_multi", + "deck_path", + "deck_polygon", + "deck_scatter", + "deck_screengrid", +] + + +class MigrateMapBox(MigrateViz): + """Migrate the legacy standalone Mapbox scatter chart to point_cluster_map.""" + + has_x_axis_control = False + source_viz_type = "mapbox" + target_viz_type = "point_cluster_map" + rename_keys = { + "mapbox_label": "map_label", + "mapbox_color": "map_color", + } + remove_keys = set() + + def _post_action(self) -> None: + # If the style URL is a mapbox:// URL, the chart was using Mapbox GL. + # Set map_renderer so the new chart continues to use the Mapbox renderer, + # which will pick up MAPBOX_API_KEY from the server config. + mapbox_style = self.data.get("mapbox_style", "") + if isinstance(mapbox_style, str) and mapbox_style.startswith("mapbox://"): + self.data["map_renderer"] = "mapbox" + + @classmethod + def upgrade_slice(cls, slc: Slice) -> None: + try: + clz = cls(slc.params) + form_data_bak = copy.deepcopy(clz.data) + + clz._pre_action() + clz._migrate() + clz._post_action() + + slc.viz_type = clz.target_viz_type + + backup: Any | dict[str, Any] = {FORM_DATA_BAK_FIELD_NAME: form_data_bak} + + query_context = try_load_json(slc.query_context) + queries_bak = None + + if query_context: + if "form_data" in query_context: + query_context["form_data"] = clz.data + queries_bak = copy.deepcopy(query_context.get("queries")) + else: + query_context = {} + + slc.query_context = json.dumps(query_context) + if queries_bak is not None: + backup[QUERIES_BAK_FIELD_NAME] = queries_bak + + slc.params = json.dumps({**clz.data, **backup}) + + except Exception as e: + logger.warning("Failed to migrate slice %s: %s", slc.id, e) + + +def _migrate_deckgl_slice(slc: Slice) -> bool: + """Set map_renderer='mapbox' for all existing deck.gl slices. + + This ensures full backwards compatibility: existing charts keep using the + Mapbox renderer. Users can later switch to MapLibre in the chart controls. + Only new charts will default to MapLibre. + + Returns True if the slice was modified. + """ + params = try_load_json(slc.params) + if not params: + return False + + if "map_renderer" in params: + return False + + params["map_renderer"] = "mapbox" + slc.params = json.dumps(params) + return True + + +def _downgrade_deckgl_slice(slc: Slice) -> bool: + """Reverse _migrate_deckgl_slice. Returns True if the slice was modified.""" + params = try_load_json(slc.params) + if not params or "map_renderer" not in params: + return False + + params.pop("map_renderer", None) + slc.params = json.dumps(params) + return True + + +def _migrate_deckgl_slices(session: Session, *, upgrade: bool) -> None: + fn = _migrate_deckgl_slice if upgrade else _downgrade_deckgl_slice + slices = session.query(Slice).filter(Slice.viz_type.in_(DECKGL_VIZ_TYPES)).all() + for slc in slices: + try: + fn(slc) + except Exception as e: + logger.warning("Failed to migrate deck.gl slice %s: %s", slc.id, e) + session.commit() Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Try-except in loop with blind exception</b></div> <div id="fix"> Try-except block inside the loop (lines 154-157) incurs performance overhead and catches bare `Exception`. Move exception handling outside the loop or catch specific exceptions like `ValueError` or `KeyError`. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion def _migrate_deckgl_slices(session: Session, *, upgrade: bool) -> None: fn = _migrate_deckgl_slice if upgrade else _downgrade_deckgl_slice slices = session.query(Slice).filter(Slice.viz_type.in_(DECKGL_VIZ_TYPES)).all() for slc in slices: try: fn(slc) except (ValueError, KeyError, TypeError) as e: logger.warning("Failed to migrate deck.gl slice %s: %s", slc.id, e) session.commit() ```` </div> </details> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/buildQuery.ts: ########## @@ -0,0 +1,86 @@ +/** + * 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 { + buildQueryContext, + ensureIsArray, + QueryFormColumn, + QueryObject, + QueryObjectFilterClause, + SqlaFormData, +} from '@superset-ui/core'; +import { + addJsColumnsToColumns, + addTooltipColumnsToQuery, +} from '../buildQueryUtils'; + +export interface DeckGeoJsonFormData extends SqlaFormData { + geojson?: string; + filter_nulls?: boolean; + js_columns?: string[]; + tooltip_contents?: unknown[]; +} + +export default function buildQuery(formData: DeckGeoJsonFormData) { + const { + geojson, + filter_nulls = true, + js_columns, + tooltip_contents, + } = formData; + + if (!geojson) { + throw new Error('GeoJSON column is required for GeoJSON charts'); + } + + return buildQueryContext(formData, (baseQueryObject: QueryObject) => { + let columns: QueryFormColumn[] = [ + ...ensureIsArray(baseQueryObject.columns || []), + geojson, + ]; + + // Add js_columns + const columnStrings = columns.map(col => + typeof col === 'string' ? col : '', + ); + const withJsColumns = addJsColumnsToColumns(columnStrings, js_columns); + columns = withJsColumns as QueryFormColumn[]; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>AdhocColumn handling bug in js_columns addition</b></div> <div id="fix"> The mapping of columns to strings incorrectly converts AdhocColumn objects to empty strings, causing data loss when adding js_columns. This could lead to incorrect query generation if AdhocColumn instances are present in baseQueryObject.columns. Consider using getColumnLabel for labels and modifying addJsColumnsToColumns to work with QueryFormColumn[] or reconstructing the columns array properly. </div> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/MapLibre.tsx: ########## @@ -0,0 +1,184 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import { memo, useCallback, useMemo, useState } from 'react'; +import { Map as MapLibreMap } from 'react-map-gl/maplibre'; +import { Map as MapboxMap } from 'react-map-gl/mapbox'; +import { WebMercatorViewport } from '@math.gl/web-mercator'; +import { useTheme } from '@apache-superset/core/theme'; +import { t } from '@apache-superset/core/translation'; +import ScatterPlotOverlay from './components/ScatterPlotOverlay'; +import { getMapboxApiKey } from './utils/mapbox'; +import 'maplibre-gl/dist/maplibre-gl.css'; +import './MapLibre.css'; + +const DEFAULT_MAP_STYLE = 'https://tiles.openfreemap.org/styles/liberty'; + +export const DEFAULT_MAX_ZOOM = 16; +export const DEFAULT_POINT_RADIUS = 60; + +interface Viewport { + longitude: number; + latitude: number; + zoom: number; +} + +interface Clusterer { + getClusters(bbox: number[], zoom: number): GeoJSONLocation[]; +} + +interface GeoJSONLocation { + geometry: { + coordinates: [number, number]; + }; + properties: Record<string, number | string | boolean | null | undefined>; +} + +interface MapLibreProps { + width?: number; + height?: number; + aggregatorName?: string; + clusterer: Clusterer; // Required - used for getClusters() + globalOpacity?: number; + hasCustomMetric?: boolean; + mapProvider?: string; + mapStyle?: string; + onViewportChange?: (viewport: Viewport) => void; + pointRadius?: number; + pointRadiusUnit?: string; + renderWhileDragging?: boolean; + rgb?: (string | number)[]; + bounds?: [[number, number], [number, number]]; // May be undefined for empty datasets +} + +function MapLibre({ + width = 400, + height = 400, + aggregatorName, + clusterer, + globalOpacity = 1, + hasCustomMetric, + mapProvider, + mapStyle, + onViewportChange, + pointRadius = DEFAULT_POINT_RADIUS, + pointRadiusUnit = 'Pixels', + renderWhileDragging = true, + rgb, + bounds, +}: MapLibreProps) { + const initialViewport = useMemo(() => { + let latitude = 0; + let longitude = 0; + let zoom = 1; + + if (bounds && bounds[0] && bounds[1]) { + const mercator = new WebMercatorViewport({ + width, + height, + }).fitBounds(bounds); + ({ latitude, longitude, zoom } = mercator); + } + + return { longitude, latitude, zoom }; + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + + const [viewport, setViewport] = useState<Viewport>(initialViewport); + + const handleMove = useCallback( + (evt: { viewState: { longitude: number; latitude: number; zoom: number } }) => { + const { longitude, latitude, zoom } = evt.viewState; + const newViewport = { longitude, latitude, zoom }; + setViewport(newViewport); + onViewportChange?.(newViewport); + }, + [onViewportChange], + ); + + // add this variable to widen the visible area + const offsetHorizontal = (width * 0.5) / 100; + const offsetVertical = (height * 0.5) / 100; + + const bbox = + bounds && bounds[0] && bounds[1] + ? [ + bounds[0][0] - offsetHorizontal, + bounds[0][1] - offsetVertical, + bounds[1][0] + offsetHorizontal, + bounds[1][1] + offsetVertical, + ] + : [-180, -90, 180, 90]; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect bbox offset units</b></div> <div id="fix"> The bbox calculation incorrectly subtracts pixel-based offsets (derived from width/height) from degree-based longitude and latitude coordinates. This unit mismatch causes incorrect bounding boxes to be passed to clusterer.getClusters(), potentially leading to missing or wrong data points on the map. The fix removes the offsets and uses bounds directly. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion const bbox = bounds && bounds[0] && bounds[1] ? [bounds[0][0], bounds[0][1], bounds[1][0], bounds[1][1]] : [-180, -90, 180, 90]; ```` </div> </details> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py: ########## @@ -0,0 +1,172 @@ +# 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. +"""migrate mapbox and deckgl charts to point_cluster_map + +Revision ID: ce6bd21901ab +Revises: 4b2a8c9d3e1f +Create Date: 2026-03-02 00:00:00.000000 + + +""" + +import copy +import logging +from typing import Any + +from alembic import op +from sqlalchemy.orm import Session + +from superset import db +from superset.migrations.shared.migrate_viz.base import ( + FORM_DATA_BAK_FIELD_NAME, + MigrateViz, + QUERIES_BAK_FIELD_NAME, + Slice, +) +from superset.migrations.shared.utils import try_load_json +from superset.utils import json + +logger = logging.getLogger("alembic.env") + +# revision identifiers, used by Alembic. +revision = "ce6bd21901ab" +down_revision = "a1b2c3d4e5f6" Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect down_revision identifier</b></div> <div id="fix"> The down_revision is set to 'a1b2c3d4e5f6', but the actual previous migration has revision 'f5b5f88d8526'. Alembic uses this for ordering, so incorrect values could break migrations. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion down_revision = "f5b5f88d8526" ```` </div> </details> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset/migrations/versions/2026-03-02_00-00_ce6bd21901ab_migrate_deckgl_and_mapbox.py: ########## @@ -0,0 +1,172 @@ +# 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. +"""migrate mapbox and deckgl charts to point_cluster_map + +Revision ID: ce6bd21901ab +Revises: 4b2a8c9d3e1f Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Incorrect docstring revision reference</b></div> <div id="fix"> The migration docstring indicates it revises '4b2a8c9d3e1f', but the actual previous migration has revision 'f5b5f88d8526'. Update this to ensure correct migration ordering. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ````suggestion Revises: f5b5f88d8526 ```` </div> </details> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them ########## superset-frontend/plugins/plugin-chart-point-cluster-map/src/transformProps.ts: ########## @@ -0,0 +1,237 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +import Supercluster, { + type Options as SuperclusterOptions, +} from 'supercluster'; +import { ChartProps } from '@superset-ui/core'; +import { DEFAULT_POINT_RADIUS, DEFAULT_MAX_ZOOM } from './MapLibre'; +import roundDecimal from './utils/roundDecimal'; + +const NOOP = () => {}; + +// Geo precision to limit decimal places (matching legacy backend behavior) +const GEO_PRECISION = 10; + +interface PointProperties { + metric: number | string | null; + radius: number | string | null; +} + +interface ClusterProperties { + metric: number; + sum: number; + squaredSum: number; + min: number; + max: number; +} + +interface DataRecord { + [key: string]: string | number | null | undefined; +} + +function buildGeoJSONFromRecords( + records: DataRecord[], + lonCol: string, + latCol: string, + labelCol: string | null, + pointRadiusCol: string | null, +) { + const features: GeoJSON.Feature<GeoJSON.Point, PointProperties>[] = []; + let minLon = Infinity; + let maxLon = -Infinity; + let minLat = Infinity; + let maxLat = -Infinity; + + for (const record of records) { + const lon = Number(record[lonCol]); + const lat = Number(record[latCol]); + if (Number.isNaN(lon) || Number.isNaN(lat)) { + continue; + } + + const roundedLon = roundDecimal(lon, GEO_PRECISION); + const roundedLat = roundDecimal(lat, GEO_PRECISION); + + minLon = Math.min(minLon, roundedLon); + maxLon = Math.max(maxLon, roundedLon); + minLat = Math.min(minLat, roundedLat); + maxLat = Math.max(maxLat, roundedLat); + + const metric = labelCol != null ? (record[labelCol] ?? null) : null; + const radius = + pointRadiusCol != null ? (record[pointRadiusCol] ?? null) : null; + + features.push({ + type: 'Feature', + properties: { metric, radius }, + geometry: { + type: 'Point', + coordinates: [roundedLon, roundedLat], + }, + }); + } + + const bounds: [[number, number], [number, number]] | undefined = + features.length > 0 + ? [ + [minLon, minLat], + [maxLon, maxLat], + ] + : undefined; + + return { + geoJSON: { type: 'FeatureCollection' as const, features }, + bounds, + }; +} + +export default function transformProps(chartProps: ChartProps) { + const { + width, + height, + rawFormData: formData, + hooks, + queriesData, + } = chartProps; + const { onError = NOOP, setControlValue = NOOP } = hooks; + + const { + all_columns_x: allColumnsX, + all_columns_y: allColumnsY, + clustering_radius: clusteringRadius, + global_opacity: globalOpacity, + map_color: maplibreColor, + map_label: maplibreLabel, + map_renderer: mapProvider, + maplibre_style: maplibreStyle, + mapbox_style: mapboxStyle = '', + pandas_aggfunc: pandasAggfunc, + point_radius: pointRadius, + point_radius_unit: pointRadiusUnit, + render_while_dragging: renderWhileDragging, + } = formData; + + // Support two data formats: + // 1. Legacy/GeoJSON: queriesData[0].data is an object with { geoJSON, bounds, hasCustomMetric } + // 2. Tabular records: queriesData[0].data is an array of flat records from a SQL query + const rawData = queriesData[0]?.data; + const isLegacyFormat = rawData && !Array.isArray(rawData) && rawData.geoJSON; + + let geoJSON: { type: 'FeatureCollection'; features: any[] }; + let bounds: [[number, number], [number, number]] | undefined; + let hasCustomMetric: boolean; + + if (isLegacyFormat) { + const legacy = rawData as any; + geoJSON = legacy.geoJSON; + bounds = legacy.bounds; + hasCustomMetric = legacy.hasCustomMetric ?? false; + } else { + const records: DataRecord[] = (rawData as DataRecord[]) || []; + hasCustomMetric = maplibreLabel != null && maplibreLabel.length > 0; + const labelCol = hasCustomMetric ? maplibreLabel[0] : null; + const pointRadiusCol = + pointRadius && pointRadius !== 'Auto' ? pointRadius : null; + + const built = buildGeoJSONFromRecords( + records, + allColumnsX, + allColumnsY, + labelCol, + pointRadiusCol, + ); + geoJSON = built.geoJSON; + bounds = built.bounds; + } + + // Validate color — supports hex (#rrggbb) and rgb(r, g, b) formats + let rgb: string[] | null = null; + const hexMatch = /^#([0-9a-f]{2})([0-9a-f]{2})([0-9a-f]{2})$/i.exec( + maplibreColor, + ); + if (hexMatch) { + rgb = [ + maplibreColor, + String(parseInt(hexMatch[1], 16)), + String(parseInt(hexMatch[2], 16)), + String(parseInt(hexMatch[3], 16)), + ]; + } else { + rgb = /^rgb\((\d{1,3}),\s*(\d{1,3}),\s*(\d{1,3})\)$/.exec(maplibreColor); + } + if (rgb === null) { + onError("Color field must be a hex color (#rrggbb) or 'rgb(r, g, b)'"); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing translation for user-facing error</b></div> <div id="fix"> The error message shown to users when color validation fails is hardcoded without translation. This violates the requirement to use translation functions for all user-facing text labels, potentially affecting internationalization. Consider importing `t` from '@apache-superset/core/translation' and wrapping the string. </div> </div> <small><i>Code Review Run #ea39a5</i></small> </div> --- Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>) - [ ] Yes, avoid them -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
