bito-code-review[bot] commented on code in PR #34859: URL: https://github.com/apache/superset/pull/34859#discussion_r2321473181
########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/spatialUtils.test.ts: ########## @@ -0,0 +1,602 @@ +/** + * 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, + QueryObjectFilterClause, + SupersetTheme, +} from '@superset-ui/core'; +import { decode } from 'ngeohash'; + +import { + getSpatialColumns, + addSpatialNullFilters, + buildSpatialQuery, + processSpatialData, + transformSpatialProps, + SpatialFormData, +} from './spatialUtils'; + +jest.mock('ngeohash', () => ({ + decode: jest.fn(), +})); + +jest.mock('@superset-ui/core', () => ({ + ...jest.requireActual('@superset-ui/core'), + buildQueryContext: jest.fn(), + getMetricLabel: jest.fn(), + ensureIsArray: jest.fn(arr => arr || []), + normalizeOrderBy: jest.fn(({ orderby }) => ({ orderby })), +})); + +// Mock DOM element for bootstrap data +const mockBootstrapData = { + common: { + conf: { + MAPBOX_API_KEY: 'test_api_key', + }, + }, +}; + +Object.defineProperty(document, 'getElementById', { + value: jest.fn().mockReturnValue({ + getAttribute: jest.fn().mockReturnValue(JSON.stringify(mockBootstrapData)), + }), + writable: true, +}); + +const mockDecode = decode as jest.MockedFunction<typeof decode>; + +describe('spatialUtils', () => { + test('getSpatialColumns returns correct columns for latlong type', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'latlong', + lonCol: 'longitude', + latCol: 'latitude', + }; + + const result = getSpatialColumns(spatial); + expect(result).toEqual(['longitude', 'latitude']); + }); + + test('getSpatialColumns returns correct columns for delimited type', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'delimited', + lonlatCol: 'coordinates', + }; + + const result = getSpatialColumns(spatial); + expect(result).toEqual(['coordinates']); + }); + + test('getSpatialColumns returns correct columns for geohash type', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'geohash', + geohashCol: 'geohash_code', + }; + + const result = getSpatialColumns(spatial); + expect(result).toEqual(['geohash_code']); + }); + + test('getSpatialColumns throws error when spatial is null', () => { + expect(() => getSpatialColumns(null as any)).toThrow('Bad spatial key'); + }); + + test('getSpatialColumns throws error when spatial type is missing', () => { + const spatial = {} as SpatialFormData['spatial']; + expect(() => getSpatialColumns(spatial)).toThrow('Bad spatial key'); + }); + + test('getSpatialColumns throws error when latlong columns are missing', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'latlong', + }; + expect(() => getSpatialColumns(spatial)).toThrow( + 'Longitude and latitude columns are required for latlong type', + ); + }); + + test('getSpatialColumns throws error when delimited column is missing', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'delimited', + }; + expect(() => getSpatialColumns(spatial)).toThrow( + 'Longitude/latitude column is required for delimited type', + ); + }); + + test('getSpatialColumns throws error when geohash column is missing', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'geohash', + }; + expect(() => getSpatialColumns(spatial)).toThrow( + 'Geohash column is required for geohash type', + ); + }); + + test('getSpatialColumns throws error for unknown spatial type', () => { + const spatial = { + type: 'unknown', + } as any; + expect(() => getSpatialColumns(spatial)).toThrow( + 'Unknown spatial type: unknown', + ); + }); + + test('addSpatialNullFilters adds null filters for spatial columns', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'latlong', + lonCol: 'longitude', + latCol: 'latitude', + }; + const existingFilters: QueryObjectFilterClause[] = [ + { col: 'other_col', op: '==', val: 'test' }, + ]; + + const result = addSpatialNullFilters(spatial, existingFilters); + + expect(result).toEqual([ + { col: 'other_col', op: '==', val: 'test' }, + { col: 'longitude', op: 'IS NOT NULL', val: null }, + { col: 'latitude', op: 'IS NOT NULL', val: null }, + ]); + }); + + test('addSpatialNullFilters returns original filters when spatial is null', () => { + const existingFilters: QueryObjectFilterClause[] = [ + { col: 'test_col', op: '==', val: 'test' }, + ]; + + const result = addSpatialNullFilters(null as any, existingFilters); + expect(result).toBe(existingFilters); + }); + + test('addSpatialNullFilters works with empty filters array', () => { + const spatial: SpatialFormData['spatial'] = { + type: 'delimited', + lonlatCol: 'coordinates', + }; + + const result = addSpatialNullFilters(spatial, []); + + expect(result).toEqual([ + { col: 'coordinates', op: 'IS NOT NULL', val: null }, + ]); + }); + + test('buildSpatialQuery throws error when spatial is missing', () => { + const formData = {} as SpatialFormData; + + expect(() => buildSpatialQuery(formData)).toThrow( + 'Spatial configuration is required for this chart', + ); + }); + + test('buildSpatialQuery calls buildQueryContext with correct parameters', () => { + const mockBuildQueryContext = + jest.requireMock('@superset-ui/core').buildQueryContext; + const formData: SpatialFormData = { + spatial: { + type: 'latlong', + lonCol: 'longitude', + latCol: 'latitude', + }, + size: 'count', + js_columns: ['extra_col'], + } as SpatialFormData; + + buildSpatialQuery(formData); + + expect(mockBuildQueryContext).toHaveBeenCalledWith(formData, { + buildQuery: expect.any(Function), + }); + }); + + test('processSpatialData processes latlong data correctly', () => { + const records = [ + { longitude: -122.4, latitude: 37.8, count: 10, extra: 'test1' }, + { longitude: -122.5, latitude: 37.9, count: 20, extra: 'test2' }, + ]; + const spatial: SpatialFormData['spatial'] = { + type: 'latlong', + lonCol: 'longitude', + latCol: 'latitude', + }; + const metricLabel = 'count'; + const jsColumns = ['extra']; + + const result = processSpatialData(records, spatial, metricLabel, jsColumns); + + expect(result).toHaveLength(2); + expect(result[0]).toEqual({ + position: [-122.4, 37.8], + weight: 10, + extraProps: { extra: 'test1' }, + }); + expect(result[1]).toEqual({ + position: [-122.5, 37.9], + weight: 20, + extraProps: { extra: 'test2' }, + }); + }); + + test('processSpatialData processes delimited data correctly', () => { + const records = [ + { coordinates: '-122.4,37.8', count: 15 }, + { coordinates: '-122.5,37.9', count: 25 }, + ]; + const spatial: SpatialFormData['spatial'] = { + type: 'delimited', + lonlatCol: 'coordinates', + }; + + const result = processSpatialData(records, spatial, 'count'); + + expect(result).toHaveLength(2); + expect(result[0]).toEqual({ + position: [-122.4, 37.8], + weight: 15, + extraProps: {}, + }); + }); + + test('processSpatialData processes geohash data correctly', () => { + mockDecode.mockReturnValue({ + latitude: 37.8, + longitude: -122.4, + error: { + latitude: 0, + longitude: 0, + }, + }); + + const records = [{ geohash: 'dr5regw3p', count: 30 }]; + const spatial: SpatialFormData['spatial'] = { + type: 'geohash', + geohashCol: 'geohash', + }; + + const result = processSpatialData(records, spatial, 'count'); + + expect(result).toHaveLength(1); + expect(result[0]).toEqual({ + position: [-122.4, 37.8], + weight: 30, + extraProps: {}, + }); + expect(mockDecode).toHaveBeenCalledWith('dr5regw3p'); + }); + + test('processSpatialData reverses coordinates when reverseCheckbox is true', () => { + const records = [{ longitude: -122.4, latitude: 37.8, count: 10 }]; + const spatial: SpatialFormData['spatial'] = { + type: 'latlong', + lonCol: 'longitude', + latCol: 'latitude', + reverseCheckbox: true, + }; + + const result = processSpatialData(records, spatial, 'count'); + + expect(result[0].position).toEqual([37.8, -122.4]); + }); + + test('processSpatialData handles invalid coordinates', () => { + const records = [ + { longitude: 'invalid', latitude: 37.8, count: 10 }, + { longitude: -122.4, latitude: NaN, count: 20 }, + { coordinates: 'invalid,coords', count: 30 }, + { coordinates: '-122.4,invalid', count: 40 }, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Invalid test logic for coordinate types</b></div> <div id="fix"> Test logic error: Using `latlong` spatial type but testing `coordinates` field. The `coordinates` field is only used by `delimited` type, so these test records will be ignored. Either change spatial type to `delimited` or remove the `coordinates` test records. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/34859#issuecomment-3252828460>#e3b0d6</a></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/legacy-preset-chart-deckgl/src/layers/Scatter/transformProps.ts: ########## @@ -0,0 +1,163 @@ +/** + * 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, getMetricLabel } from '@superset-ui/core'; +import { + processSpatialData, + getMapboxApiKey, + addJsColumnsToExtraProps, + DataRecord, +} from '../spatialUtils'; +import { DeckScatterFormData } from './buildQuery'; + +const NOOP = () => {}; + +interface ScatterPoint { + position: [number, number]; + radius?: number; + color?: [number, number, number, number]; + cat_color?: string; + metric?: number; + extraProps?: Record<string, unknown>; + [key: string]: unknown; +} + +function processScatterData( + records: DataRecord[], + spatial: DeckScatterFormData['spatial'], + radiusMetricLabel?: string, + categoryColumn?: string, + jsColumns?: string[], +): ScatterPoint[] { + if (!spatial || !records.length) { + return []; + } + + const spatialFeatures = processSpatialData(records, spatial); + + return spatialFeatures.map((feature, index) => { + const record = records[index]; Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Critical array index mismatch issue</b></div> <div id="fix"> Critical array index mismatch: `spatialFeatures.map((feature, index) => { const record = records[index]; })` assumes both arrays have the same length and corresponding indices. However, `processSpatialData` filters out records with invalid spatial data, causing index misalignment. This will result in accessing wrong record data or undefined values, breaking scatter plot functionality. Fix: Use the feature object directly since `processSpatialData` already copies all record properties to the spatial feature objects. </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion return spatialFeatures.map((feature) => { const record = feature; ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/34859#issuecomment-3252828460>#e3b0d6</a></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/legacy-preset-chart-deckgl/src/layers/Path/transformProps.ts: ########## @@ -0,0 +1,212 @@ +/** + * 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, getMetricLabel, DTTM_ALIAS } from '@superset-ui/core'; +import { + getMapboxApiKey, + addJsColumnsToExtraProps, + DataRecord, +} from '../spatialUtils'; +import { DeckPathFormData } from './buildQuery'; + +declare global { + interface Window { + polyline?: { + decode: (data: string) => [number, number][]; + }; + geohash?: { + decode: (data: string) => { longitude: number; latitude: number }; + }; + } +} + +export interface DeckPathTransformPropsFormData extends DeckPathFormData { + js_data_mutator?: string; + js_tooltip?: string; + js_onclick_href?: string; +} + +const NOOP = () => {}; + +interface PathFeature { + path: [number, number][]; + metric?: number; + timestamp?: unknown; + extraProps?: Record<string, unknown>; + [key: string]: unknown; +} + +const decoders = { + json: (data: string): [number, number][] => { + try { + const parsed = JSON.parse(data); + return Array.isArray(parsed) ? parsed : []; + } catch (error) { + return []; + } + }, + polyline: (data: string): [number, number][] => { + try { + if (typeof window !== 'undefined' && window.polyline) { + return window.polyline.decode(data); + } + return []; + } catch (error) { + return []; + } + }, + geohash: (data: string): [number, number][] => { + try { + if (typeof window !== 'undefined' && window.geohash) { + const decoded = window.geohash.decode(data); + return [[decoded.longitude, decoded.latitude]]; + } + return []; + } catch (error) { + return []; + } + }, +}; + +function processPathData( + records: DataRecord[], + lineColumn: string, + lineType: 'polyline' | 'json' | 'geohash' = 'json', + reverseLongLat: boolean = false, + metricLabel?: string, + jsColumns?: string[], +): PathFeature[] { + if (!records.length || !lineColumn) { + return []; + } + + const decoder = decoders[lineType] || decoders.json; + + return records.map(record => { + const lineData = record[lineColumn]; + let path: [number, number][] = []; + + if (lineData) { + path = decoder(String(lineData)); + + if (reverseLongLat && path.length > 0) { + path = path.map(([lng, lat]) => [lat, lng]); + } + } + + let feature: PathFeature = { + path, + timestamp: record[DTTM_ALIAS], + extraProps: {}, + }; + + if (metricLabel && record[metricLabel] != null) { + const metricValue = parseFloat(String(record[metricLabel])); + if (!Number.isNaN(metricValue)) { + feature.metric = metricValue; + } + } + + feature = addJsColumnsToExtraProps(feature, record, jsColumns); + Object.keys(record).forEach(key => { + if (key === lineColumn && lineType !== 'geohash') { + return; + } + + if (key === 'timestamp' || key === DTTM_ALIAS || key === metricLabel) { + return; + } + + if (jsColumns?.includes(key)) { + return; + } + + feature[key] = record[key]; + }); + + return feature; + }); +} + +export default function transformProps(chartProps: ChartProps) { + const { + datasource, + height, + hooks, + queriesData, + rawFormData: formData, + width, + filterState, + emitCrossFilters, + } = chartProps; + + const { + onAddFilter = NOOP, + onContextMenu = NOOP, + setControlValue = NOOP, + setDataMask = NOOP, + } = hooks; + + const { + line_column, + line_type = 'json', + metric, + reverse_long_lat = false, + js_columns, + } = formData as DeckPathTransformPropsFormData; + + const metricLabel = metric ? getMetricLabel(metric) : undefined; + + const queryData = queriesData[0]; + const records = queryData?.data || []; + const features = processPathData( + records, + line_column || '', + line_type, + reverse_long_lat, + metricLabel, + js_columns, + ).reverse(); + + return { + datasource, + emitCrossFilters, + formData, + height, + onAddFilter, + onContextMenu, + payload: { + ...queryData, + data: { + features, + mapboxApiKey: getMapboxApiKey(), + metricLabels: metricLabel ? [metricLabel] : [], + }, + }, + setControlValue, + filterState, + viewport: { + ...formData.viewport, + height, Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Missing null check for viewport property</b></div> <div id="fix"> The `formData.viewport` property is accessed without null checking in the viewport object spread. This could cause a runtime error if `viewport` is undefined. Add null check: `...(formData.viewport || {})` </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion setControlValue, filterState, viewport: { ...(formData.viewport || {}), height, ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/34859#issuecomment-3252828460>#e3b0d6</a></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/legacy-preset-chart-deckgl/src/layers/spatialUtils.ts: ########## @@ -0,0 +1,396 @@ +/** + * 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, + getMetricLabel, + QueryFormData, + QueryObjectFilterClause, + ensureIsArray, + ChartProps, + normalizeOrderBy, +} from '@superset-ui/core'; +import { decode } from 'ngeohash'; + +export interface SpatialConfiguration { + type: 'latlong' | 'delimited' | 'geohash'; + lonCol?: string; + latCol?: string; + lonlatCol?: string; + geohashCol?: string; + reverseCheckbox?: boolean; +} + +export interface DataRecord { + [key: string]: string | number | null | undefined; +} + +export interface BootstrapData { + common?: { + conf?: { + MAPBOX_API_KEY?: string; + }; + }; +} + +export interface SpatialFormData extends QueryFormData { + spatial: SpatialConfiguration; + size?: string; + grid_size?: number; + js_data_mutator?: string; + js_agg_function?: string; + js_columns?: string[]; + color_scheme?: string; + color_scheme_type?: string; + color_breakpoints?: number[]; + default_breakpoint_color?: string; + color_picker?: string; +} + +export interface SpatialPoint { + position: [number, number]; + weight: number; + extraProps?: Record<string, unknown>; + [key: string]: unknown; +} + +export function getSpatialColumns(spatial: SpatialConfiguration): string[] { + if (!spatial || !spatial.type) { + throw new Error('Bad spatial key'); + } + + switch (spatial.type) { + case 'latlong': + if (!spatial.lonCol || !spatial.latCol) { + throw new Error( + 'Longitude and latitude columns are required for latlong type', + ); + } + return [spatial.lonCol, spatial.latCol]; + case 'delimited': + if (!spatial.lonlatCol) { + throw new Error( + 'Longitude/latitude column is required for delimited type', + ); + } + return [spatial.lonlatCol]; + case 'geohash': + if (!spatial.geohashCol) { + throw new Error('Geohash column is required for geohash type'); + } + return [spatial.geohashCol]; + default: + throw new Error(`Unknown spatial type: ${spatial.type}`); + } +} + +export function addSpatialNullFilters( + spatial: SpatialConfiguration, + filters: QueryObjectFilterClause[], +): QueryObjectFilterClause[] { + if (!spatial) return filters; + + const spatialColumns = getSpatialColumns(spatial); + const nullFilters: QueryObjectFilterClause[] = spatialColumns.map(column => ({ + col: column, + op: 'IS NOT NULL', + val: null, + })); + + return [...filters, ...nullFilters]; +} + +export function buildSpatialQuery(formData: SpatialFormData) { + const { spatial, size: metric, js_columns } = formData; + + if (!spatial) { + throw new Error(`Spatial configuration is required for this chart`); + } + return buildQueryContext(formData, { + buildQuery: baseQueryObject => { + const spatialColumns = getSpatialColumns(spatial); + const columns = [...(baseQueryObject.columns || []), ...spatialColumns]; + const metrics = metric ? [metric] : []; + + if (js_columns?.length) { + js_columns.forEach(col => { + if (!columns.includes(col)) { + columns.push(col); + } + }); + } + + const filters = addSpatialNullFilters( + spatial, + ensureIsArray(baseQueryObject.filters || []), + ); + + const orderby = metric + ? normalizeOrderBy({ orderby: [[metric, false]] }).orderby + : baseQueryObject.orderby; + + return [ + { + ...baseQueryObject, + columns, + metrics, + filters, + orderby, + is_timeseries: false, + row_limit: baseQueryObject.row_limit, + }, + ]; + }, + }); +} + +function parseCoordinates(latlong: string): [number, number] | null { + if (!latlong || typeof latlong !== 'string') { + return null; + } + + try { + const coords = latlong.split(',').map(coord => parseFloat(coord.trim())); + if ( + coords.length === 2 && + !Number.isNaN(coords[0]) && + !Number.isNaN(coords[1]) + ) { + return [coords[0], coords[1]]; + } + return null; + } catch (error) { + return null; + } +} + +function reverseGeohashDecode(geohashCode: string): [number, number] | null { + if (!geohashCode || typeof geohashCode !== 'string') { + return null; + } + + try { + const { latitude: lat, longitude: lng } = decode(geohashCode); + if ( + Number.isNaN(lat) || + Number.isNaN(lng) || + lat < -90 || + lat > 90 || + lng < -180 || + lng > 180 + ) { + return null; + } + return [lng, lat]; + } catch (error) { + return null; + } +} + +export function addJsColumnsToExtraProps< + T extends { extraProps?: Record<string, unknown> }, +>(feature: T, record: DataRecord, jsColumns?: string[]): T { + if (!jsColumns?.length) { + return feature; + } + + const extraProps: Record<string, unknown> = { ...(feature.extraProps ?? {}) }; + + jsColumns.forEach(col => { + if (record[col] !== undefined) { + extraProps[col] = record[col]; + } + }); + + return { ...feature, extraProps }; +} + +export function processSpatialData( + records: DataRecord[], + spatial: SpatialConfiguration, + metricLabel?: string, + jsColumns?: string[], +): SpatialPoint[] { + if (!spatial || !records.length) { + return []; + } + + const features: SpatialPoint[] = []; + const spatialColumns = getSpatialColumns(spatial); + const jsColumnsSet = jsColumns ? new Set(jsColumns) : null; + const spatialColumnsSet = new Set(spatialColumns); + + for (const record of records) { + let position: [number, number] | null = null; + + switch (spatial.type) { + case 'latlong': + if (spatial.lonCol && spatial.latCol) { + const lon = parseFloat(String(record[spatial.lonCol] ?? '')); + const lat = parseFloat(String(record[spatial.latCol] ?? '')); + if (!Number.isNaN(lon) && !Number.isNaN(lat)) { + position = [lon, lat]; + } + } + break; + case 'delimited': + if (spatial.lonlatCol) { + position = parseCoordinates(String(record[spatial.lonlatCol] ?? '')); + } + break; + case 'geohash': + if (spatial.geohashCol) { + const geohashValue = record[spatial.geohashCol]; + if (geohashValue) { + position = reverseGeohashDecode(String(geohashValue)); + } + } + break; + default: + continue; + } + + if (!position) { + continue; + } + + if (spatial.reverseCheckbox) { + position = [position[1], position[0]]; + } + + let weight = 1; + if (metricLabel && record[metricLabel] != null) { + const metricValue = parseFloat(String(record[metricLabel])); + if (!Number.isNaN(metricValue)) { + weight = metricValue; + } + } + + let spatialPoint: SpatialPoint = { + position, + weight, + extraProps: {}, + }; + + spatialPoint = addJsColumnsToExtraProps(spatialPoint, record, jsColumns); + Object.keys(record).forEach(key => { + if (spatialColumnsSet.has(key)) { + return; + } + + if (key === metricLabel) { + return; + } + + if (jsColumnsSet?.has(key)) { + return; + } + + spatialPoint[key] = record[key]; + }); + + features.push(spatialPoint); + } + + return features; +} + +const NOOP = () => {}; + +export function getMapboxApiKey(mapboxApiKey?: string): string { + if (mapboxApiKey) { + return mapboxApiKey; + } + + if (typeof document !== 'undefined') { + try { + const appContainer = document.getElementById('app'); + const dataBootstrap = appContainer?.getAttribute('data-bootstrap'); + if (dataBootstrap) { + const bootstrapData: BootstrapData = JSON.parse(dataBootstrap); + return bootstrapData?.common?.conf?.MAPBOX_API_KEY || ''; + } + } catch (error) { + throw new Error( + 'Failed to read MAPBOX_API_KEY from bootstrap data:', + error, + ); Review Comment: <div> <div id="suggestion"> <div id="issue"><b>Invalid Error constructor usage</b></div> <div id="fix"> The `Error` constructor is being called with two arguments (message and error object), but it only accepts a single message parameter. This will cause the original error information to be lost, making debugging difficult. Fix by concatenating the error message: `throw new Error(`Failed to read MAPBOX_API_KEY from bootstrap data: ${error}`);` </div> <details> <summary> <b>Code suggestion</b> </summary> <blockquote>Check the AI-generated fix before applying</blockquote> <div id="code"> ```suggestion throw new Error( `Failed to read MAPBOX_API_KEY from bootstrap data: ${error}`, ); ``` </div> </details> </div> <small><i>Code Review Run <a href=https://github.com/apache/superset/pull/34859#issuecomment-3252828460>#e3b0d6</a></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]
