korbit-ai[bot] commented on code in PR #34859: URL: https://github.com/apache/superset/pull/34859#discussion_r2321391419
########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/transformProps.ts: ########## @@ -0,0 +1,183 @@ +/** + * 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 { + getMapboxApiKey, + addJsColumnsToExtraProps, + DataRecord, +} from '../spatialUtils'; +import { DeckPolygonFormData } from './buildQuery'; + +const NOOP = () => {}; + +interface PolygonFeature { + polygon?: number[][]; + name?: string; + elevation?: number; + extraProps?: Record<string, unknown>; + [key: string]: unknown; +} Review Comment: ### Overly Permissive Interface Definition <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The PolygonFeature interface uses an index signature [key: string]: unknown which makes the type system less effective by allowing any property. ###### Why this matters Using index signatures reduces type safety and makes it harder to catch errors at compile time. It bypasses TypeScript's property checking mechanisms. ###### Suggested change ∙ *Feature Preview* Define a more specific interface with known properties: ```typescript interface PolygonFeature { polygon?: number[][]; name?: string; elevation?: number; extraProps: Record<string, unknown>; metrics?: Record<string, number | string>; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d012239e-13cf-4f02-aa8f-67971e94a7e1/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d012239e-13cf-4f02-aa8f-67971e94a7e1?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d012239e-13cf-4f02-aa8f-67971e94a7e1?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d012239e-13cf-4f02-aa8f-67971e94a7e1?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d012239e-13cf-4f02-aa8f-67971e94a7e1) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:30a220ea-5d83-4af1-b163-e4cf855b6248 --> [](30a220ea-5d83-4af1-b163-e4cf855b6248) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/transformProps.ts: ########## @@ -0,0 +1,183 @@ +/** + * 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 { + getMapboxApiKey, + addJsColumnsToExtraProps, + DataRecord, +} from '../spatialUtils'; +import { DeckPolygonFormData } from './buildQuery'; + +const NOOP = () => {}; + +interface PolygonFeature { + polygon?: number[][]; + name?: string; + elevation?: number; + extraProps?: Record<string, unknown>; + [key: string]: unknown; +} + +function processPolygonData( + records: DataRecord[], + formData: DeckPolygonFormData, +): PolygonFeature[] { + const { + line_column, + line_type, + metric, + point_radius_fixed, + reverse_long_lat, + js_columns, + } = formData; + + if (!line_column || !records.length) { + return []; + } + + const metricLabel = metric ? getMetricLabel(metric) : null; + const elevationLabel = point_radius_fixed?.value + ? getMetricLabel(point_radius_fixed.value) + : null; + + return records + .map(record => { + let feature: PolygonFeature = { + extraProps: {}, + }; + + feature = addJsColumnsToExtraProps(feature, record, js_columns); + Object.keys(record).forEach(key => { + if (key === line_column) { + return; + } + + if (js_columns?.includes(key)) { + return; + } + + feature[key] = record[key]; + }); Review Comment: ### Inefficient Object Property Copy <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Unnecessary iteration through all record keys to copy properties, when Object.assign or spread operator could be used with property filtering. ###### Why this matters This implementation has O(n) complexity and creates unnecessary function calls for each property. It also makes the code more verbose and harder to maintain. ###### Suggested change ∙ *Feature Preview* Use Object.fromEntries with Object.entries filtering or spread with property exclusion: ```typescript const filteredKeys = Object.entries(record) .filter(([key]) => key !== line_column && !js_columns?.includes(key)); feature = { ...feature, ...Object.fromEntries(filteredKeys) }; ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/114cad12-8b3e-4b3d-a54a-cb15d497ee9c/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/114cad12-8b3e-4b3d-a54a-cb15d497ee9c?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/114cad12-8b3e-4b3d-a54a-cb15d497ee9c?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/114cad12-8b3e-4b3d-a54a-cb15d497ee9c?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/114cad12-8b3e-4b3d-a54a-cb15d497ee9c) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ab167dcc-a6bf-4c39-a1d7-6b4acf3143bd --> [](ab167dcc-a6bf-4c39-a1d7-6b4acf3143bd) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/transformProps.ts: ########## @@ -0,0 +1,22 @@ +/** + * 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 { transformSpatialProps } from '../spatialUtils'; + +// Use the generic spatial transform function directly +export default transformSpatialProps; Review Comment: ### Missing Contour-Specific Property Transformations <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Direct reuse of generic spatial transform function for Contour layer without specific contour-related property transformations. ###### Why this matters Contour layers typically require specific property handling for contour-specific features like cellSize, contours, and getWeight. Using a generic transform may miss these essential contour-specific transformations. ###### Suggested change ∙ *Feature Preview* Create a contour-specific transform function that handles contour-specific properties before applying generic spatial transforms: ```typescript import { transformSpatialProps } from '../spatialUtils'; export default function transformContourProps(props) { const contourProps = { ...props, cellSize: props.cellSize || 1000, contours: props.contours || [], getWeight: d => d.weight }; return transformSpatialProps(contourProps); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea604121-470a-4941-8fcc-5e6485910b17/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea604121-470a-4941-8fcc-5e6485910b17?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea604121-470a-4941-8fcc-5e6485910b17?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea604121-470a-4941-8fcc-5e6485910b17?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/ea604121-470a-4941-8fcc-5e6485910b17) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6cb2f2e7-b4af-44eb-8f68-76cd343ac3c7 --> [](6cb2f2e7-b4af-44eb-8f68-76cd343ac3c7) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/buildQuery.ts: ########## @@ -0,0 +1,34 @@ +/** + * 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 { SpatialFormData, buildSpatialQuery } from '../spatialUtils'; + +export interface DeckContourFormData extends SpatialFormData { + cellSize?: string; Review Comment: ### Incorrect Type for Cell Size Property <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The cellSize property is typed as string when it should be a number for deck.gl contour layer calculations. ###### Why this matters Using a string type for cellSize could lead to runtime errors or incorrect contour calculations in deck.gl, as the contour layer expects a numerical value for cell size computations. ###### Suggested change ∙ *Feature Preview* Change the type of cellSize from string to number in the DeckContourFormData interface: ```typescript export interface DeckContourFormData extends SpatialFormData { cellSize?: number; aggregation?: string; contours?: Array<{ color: { r: number; g: number; b: number }; lowerThreshold: number; upperThreshold?: number; strokeWidth?: number; }>; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e102be8-f36e-4756-9967-e46f04c7c50f/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e102be8-f36e-4756-9967-e46f04c7c50f?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e102be8-f36e-4756-9967-e46f04c7c50f?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e102be8-f36e-4756-9967-e46f04c7c50f?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/6e102be8-f36e-4756-9967-e46f04c7c50f) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:4d0b0b6a-07a1-4995-8e07-837c242e0c42 --> [](4d0b0b6a-07a1-4995-8e07-837c242e0c42) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Polygon/buildQuery.ts: ########## @@ -0,0 +1,102 @@ +/** + * 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, + SqlaFormData, + getMetricLabel, + QueryObjectFilterClause, +} from '@superset-ui/core'; + +export interface DeckPolygonFormData extends SqlaFormData { + line_column?: string; + line_type?: string; + metric?: string; + point_radius_fixed?: { + value?: string; + }; + reverse_long_lat?: boolean; + filter_nulls?: boolean; + js_columns?: string[]; +} + +export default function buildQuery(formData: DeckPolygonFormData) { + const { + line_column, + metric, + point_radius_fixed, + filter_nulls = true, + js_columns, + } = formData; + + if (!line_column) { + throw new Error('Polygon column is required for Polygon charts'); + } + + return buildQueryContext(formData, baseQueryObject => { + const columns = [...(baseQueryObject.columns || []), line_column]; + + // Add js_columns to ensure they're available for JavaScript functions + const jsColumns = ensureIsArray(js_columns || []); + jsColumns.forEach(col => { + if (!columns.includes(col)) { + columns.push(col); + } + }); + + const metrics = []; Review Comment: ### Missing metrics validation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The metrics array is initialized as empty and conditionally populated, but the code doesn't validate if any metrics are actually added when required. ###### Why this matters If both metric and point_radius_fixed.value are undefined, an empty metrics array will be sent to the query, which might cause unexpected behavior in the polygon chart visualization. ###### Suggested change ∙ *Feature Preview* Add validation to ensure at least one metric is provided when required for the polygon chart: ```typescript const metrics = []; if (metric) { metrics.push(metric); } if (point_radius_fixed?.value) { metrics.push(point_radius_fixed.value); } if (metrics.length === 0 && requiresMetric) { throw new Error('At least one metric is required for Polygon charts'); } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3158c375-5ee3-4aec-af2b-1ec8bcb33717/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3158c375-5ee3-4aec-af2b-1ec8bcb33717?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3158c375-5ee3-4aec-af2b-1ec8bcb33717?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3158c375-5ee3-4aec-af2b-1ec8bcb33717?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/3158c375-5ee3-4aec-af2b-1ec8bcb33717) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:af505806-a3f6-499e-97e7-f3c02cef8afa --> [](af505806-a3f6-499e-97e7-f3c02cef8afa) ########## 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 => { Review Comment: ### Suboptimal Object Key Iteration <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Using Object.keys().forEach() creates an unnecessary array of keys and is less performant than a for...in loop for object iteration. ###### Why this matters This approach allocates memory for the keys array and has additional function call overhead for each iteration. ###### Suggested change ∙ *Feature Preview* Use for...in loop for better performance: ```typescript for (const key in record) { if (key === lineColumn && lineType !== 'geohash') continue; if (key === 'timestamp' || key === DTTM_ALIAS || key === metricLabel) continue; if (jsColumns?.includes(key)) continue; feature[key] = record[key]; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2211d2e-e00f-47dd-ac00-ca8d35d3ff3a/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2211d2e-e00f-47dd-ac00-ca8d35d3ff3a?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2211d2e-e00f-47dd-ac00-ca8d35d3ff3a?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2211d2e-e00f-47dd-ac00-ca8d35d3ff3a?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/d2211d2e-e00f-47dd-ac00-ca8d35d3ff3a) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:9babe2b4-e627-497b-ae02-25f657d2aa70 --> [](9babe2b4-e627-497b-ae02-25f657d2aa70) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Contour/buildQuery.ts: ########## @@ -0,0 +1,34 @@ +/** + * 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 { SpatialFormData, buildSpatialQuery } from '../spatialUtils'; + +export interface DeckContourFormData extends SpatialFormData { + cellSize?: string; + aggregation?: string; + contours?: Array<{ + color: { r: number; g: number; b: number }; + lowerThreshold: number; + upperThreshold?: number; + strokeWidth?: number; + }>; +} + +export default function buildQuery(formData: DeckContourFormData) { + return buildSpatialQuery(formData); +} Review Comment: ### Unnecessary Abstraction Layer <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The buildQuery function is a simple pass-through wrapper that doesn't add any value or specific contour-related logic. ###### Why this matters This abstraction layer adds unnecessary complexity without providing any benefits. If no contour-specific query logic is needed, the spatial query builder could be used directly. ###### Suggested change ∙ *Feature Preview* Either remove the wrapper and use buildSpatialQuery directly where needed, or add contour-specific query logic: ```typescript export default function buildQuery(formData: DeckContourFormData) { // Add contour-specific query modifications here if needed const spatialQuery = buildSpatialQuery(formData); return spatialQuery; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a014810d-9b10-4c96-a4d0-68d508563217/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a014810d-9b10-4c96-a4d0-68d508563217?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a014810d-9b10-4c96-a4d0-68d508563217?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a014810d-9b10-4c96-a4d0-68d508563217?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a014810d-9b10-4c96-a4d0-68d508563217) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:cf3c83f2-2ec1-4bfe-8391-b664535124a3 --> [](cf3c83f2-2ec1-4bfe-8391-b664535124a3) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Arc/transformProps.ts: ########## @@ -0,0 +1,151 @@ +/** + * 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 } from '@superset-ui/core'; +import { + processSpatialData, + getMapboxApiKey, + addJsColumnsToExtraProps, + DataRecord, +} from '../spatialUtils'; +import { DeckArcFormData } from './buildQuery'; + +const NOOP = () => {}; + +interface ArcPoint { + sourcePosition: [number, number]; + targetPosition: [number, number]; + cat_color?: string; + __timestamp?: number; + extraProps?: Record<string, unknown>; + [key: string]: unknown; +} + +function processArcData( + records: DataRecord[], + startSpatial: DeckArcFormData['start_spatial'], + endSpatial: DeckArcFormData['end_spatial'], + dimension?: string, + jsColumns?: string[], +): ArcPoint[] { + if (!startSpatial || !endSpatial || !records.length) { + return []; + } + + const startFeatures = processSpatialData(records, startSpatial); + const endFeatures = processSpatialData(records, endSpatial); + + return records + .map((record, index) => { + const startFeature = startFeatures[index]; + const endFeature = endFeatures[index]; + + if (!startFeature || !endFeature) { + return null; + } + + let arcPoint: ArcPoint = { + sourcePosition: startFeature.position, + targetPosition: endFeature.position, + extraProps: {}, + }; + + arcPoint = addJsColumnsToExtraProps(arcPoint, record, jsColumns); + + if (dimension && record[dimension] != null) { + arcPoint.cat_color = String(record[dimension]); + } + + // eslint-disable-next-line no-underscore-dangle + if (record.__timestamp != null) { + // eslint-disable-next-line no-underscore-dangle + arcPoint.__timestamp = Number(record.__timestamp); + } + Object.keys(record).forEach(key => { + if ( + key !== '__timestamp' && + key !== dimension && + !(jsColumns || []).includes(key) + ) { + arcPoint[key] = record[key]; + } + }); + + return arcPoint; + }) + .filter((point): point is ArcPoint => point !== null); +} + +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 { start_spatial, end_spatial, dimension, js_columns } = + formData as DeckArcFormData; + + const queryData = queriesData[0]; + const records = queryData?.data || []; + const features = processArcData( + records, + start_spatial, + end_spatial, + dimension, + js_columns, + ); + + return { + datasource, + emitCrossFilters, + formData, + height, + onAddFilter, + onContextMenu, + payload: { + ...queryData, + data: { + features, + mapboxApiKey: getMapboxApiKey(), + }, + }, + setControlValue, + filterState, + viewport: { + ...formData.viewport, + height, + width, + }, + width, + setDataMask, + setTooltip: () => {}, Review Comment: ### Non-functional tooltip implementation <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The setTooltip function is implemented as an empty function, which means tooltips won't work in the Arc layer visualization. ###### Why this matters Users won't see any tooltips when hovering over arcs in the visualization, making it difficult to understand the data points and their properties. ###### Suggested change ∙ *Feature Preview* Implement proper tooltip functionality to display relevant arc information. Example implementation: ```typescript setTooltip: object => { if (!object) return null; const { sourcePosition, targetPosition, ...properties } = object; return { html: Object.entries(properties) .map(([key, value]) => `<div><strong>${key}:</strong> ${value}</div>`) .join(''), }; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5469e1b3-ee2f-48db-b341-0de82a96cd45/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5469e1b3-ee2f-48db-b341-0de82a96cd45?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5469e1b3-ee2f-48db-b341-0de82a96cd45?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5469e1b3-ee2f-48db-b341-0de82a96cd45?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5469e1b3-ee2f-48db-b341-0de82a96cd45) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:6d2be3ce-689a-4650-b26d-87666b412375 --> [](6d2be3ce-689a-4650-b26d-87666b412375) ########## superset-frontend/plugins/legacy-preset-chart-deckgl/src/layers/Heatmap/buildQuery.ts: ########## @@ -0,0 +1,27 @@ +/** + * 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 { SpatialFormData, buildSpatialQuery } from '../spatialUtils'; + +export interface DeckHeatmapFormData extends SpatialFormData { + // Heatmap-specific properties can be added here if needed +} Review Comment: ### Missing Required Heatmap Properties <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? The DeckHeatmapFormData interface is missing required heatmap-specific properties that are essential for the functionality of a heatmap visualization, such as weight metrics and aggregation settings. ###### Why this matters Without the necessary heatmap-specific properties, the visualization may fail to render correctly or might not represent the data intensity as expected in a heatmap. ###### Suggested change ∙ *Feature Preview* Add the required heatmap-specific properties to the interface: ```typescript export interface DeckHeatmapFormData extends SpatialFormData { weight_metric?: string; aggregation?: 'SUM' | 'AVG' | 'COUNT'; intensity?: number; radius_pixels?: number; } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3a7015e-4887-48b0-a334-6a787ab2aa89/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3a7015e-4887-48b0-a334-6a787ab2aa89?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3a7015e-4887-48b0-a334-6a787ab2aa89?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3a7015e-4887-48b0-a334-6a787ab2aa89?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e3a7015e-4887-48b0-a334-6a787ab2aa89) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:ea8faeb4-e0df-4683-83c0-ffd35325590f --> [](ea8faeb4-e0df-4683-83c0-ffd35325590f) ########## 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); Review Comment: ### Unsafe JSON parsing of potentially untrusted data <sub></sub> <details> <summary>Tell me more</summary> ###### What is the issue? Direct JSON parsing of untrusted data without validation in the json decoder function. ###### Why this matters Maliciously crafted JSON input could lead to memory exhaustion through deeply nested structures or extremely large payloads. ###### Suggested change ∙ *Feature Preview* ```typescript json: (data: string): [number, number][] => { try { // Add size validation before parsing if (data.length > MAX_JSON_LENGTH) { return []; } const parsed = JSON.parse(data); // Add depth validation after parsing if (!isValidPathArray(parsed)) { return []; } return Array.isArray(parsed) ? parsed : []; } catch (error) { return []; } } ``` ###### Provide feedback to improve future suggestions [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2189615-3978-498d-b225-adafc610bb22/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2189615-3978-498d-b225-adafc610bb22?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2189615-3978-498d-b225-adafc610bb22?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2189615-3978-498d-b225-adafc610bb22?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/f2189615-3978-498d-b225-adafc610bb22) </details> <sub> 💬 Looking for more details? Reply to this comment to chat with Korbit. </sub> <!--- korbi internal id:0a149cda-02cb-428a-87c7-b8bcf5d654aa --> [](0a149cda-02cb-428a-87c7-b8bcf5d654aa) -- 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]
