korbit-ai[bot] commented on code in PR #35525:
URL: https://github.com/apache/superset/pull/35525#discussion_r2406279961
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/types/external.d.ts:
##########
@@ -0,0 +1,9 @@
+declare module '*.png' {
+ const value: any;
Review Comment:
### Overly permissive 'any' type for image imports <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The type declaration uses 'any' type for image module declarations, which
defeats the purpose of TypeScript's type safety.
###### Why this matters
Using 'any' eliminates compile-time type checking and IntelliSense support
for imported image assets, potentially leading to runtime errors if the
imported values are used incorrectly.
###### Suggested change ∙ *Feature Preview*
Replace 'any' with 'string' since image imports typically resolve to URL
strings:
```typescript
declare module '*.png' {
const value: string;
export default value;
}
declare module '*.jpg' {
const value: string;
export default value;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/804e434f-bc5b-4eae-be42-4a9f09593cba/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/804e434f-bc5b-4eae-be42-4a9f09593cba?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/804e434f-bc5b-4eae-be42-4a9f09593cba?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/804e434f-bc5b-4eae-be42-4a9f09593cba?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/804e434f-bc5b-4eae-be42-4a9f09593cba)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:15d35dc5-646d-4045-aefa-58d30b570886 -->
[](15d35dc5-646d-4045-aefa-58d30b570886)
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -109,8 +140,8 @@ class MapBox extends Component {
// to an area outside of the original bounds, no additional queries are
made to the backend to
// retrieve additional data.
// add this variable to widen the visible area
- const offsetHorizontal = (width * 0.5) / 100;
- const offsetVertical = (height * 0.5) / 100;
+ const offsetHorizontal = ((width || 400) * 0.5) / 100;
+ const offsetVertical = ((height || 400) * 0.5) / 100;
Review Comment:
### Magic Numbers in Viewport Calculations <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
Magic numbers (0.5 and 100) are used in the viewport offset calculations
without clear explanation or abstraction.
###### Why this matters
Using magic numbers makes the code less maintainable and harder to
understand the intent behind these specific values.
###### Suggested change ∙ *Feature Preview*
Extract magic numbers into named constants with clear intent:
```typescript
const VIEWPORT_PADDING_PERCENTAGE = 0.5;
const PERCENTAGE_TO_DECIMAL = 100;
const offsetHorizontal = (width * VIEWPORT_PADDING_PERCENTAGE) /
PERCENTAGE_TO_DECIMAL;
const offsetVertical = (height * VIEWPORT_PADDING_PERCENTAGE) /
PERCENTAGE_TO_DECIMAL;
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00a21e4a-76e3-47a8-89ec-f72bb4335c43/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00a21e4a-76e3-47a8-89ec-f72bb4335c43?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00a21e4a-76e3-47a8-89ec-f72bb4335c43?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00a21e4a-76e3-47a8-89ec-f72bb4335c43?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/00a21e4a-76e3-47a8-89ec-f72bb4335c43)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:a3dd34ea-bf0d-42a3-b09d-cdcdbf7d3a8c -->
[](a3dd34ea-bf0d-42a3-b09d-cdcdbf7d3a8c)
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -73,19 +103,65 @@ const computeClusterLabel = (properties, aggregation) => {
return count;
};
-class ScatterPlotGlowOverlay extends PureComponent {
- constructor(props) {
+class ScatterPlotGlowOverlay extends
PureComponent<ScatterPlotGlowOverlayProps> {
+ static defaultProps = defaultProps;
+
+ private canvasRef: RefObject<HTMLCanvasElement>;
+
+ private containerRef: RefObject<HTMLDivElement>;
+
+ constructor(props: ScatterPlotGlowOverlayProps) {
super(props);
+ this.canvasRef = createRef<HTMLCanvasElement>();
+ this.containerRef = createRef<HTMLDivElement>();
this.redraw = this.redraw.bind(this);
+ this.updateCanvasSize = this.updateCanvasSize.bind(this);
+ }
+
+ componentDidMount() {
+ this.updateCanvasSize();
+ this.redraw();
+ if (this.props.mapRef?.current) {
+ this.props.mapRef.current.on('move', this.redraw);
+ this.props.mapRef.current.on('moveend', this.redraw);
+ }
}
- drawText(ctx, pixel, options = {}) {
+ componentDidUpdate() {
+ this.updateCanvasSize();
+ this.redraw();
+ }
Review Comment:
### Unconditional canvas operations in componentDidUpdate <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The componentDidUpdate method unconditionally calls updateCanvasSize() and
redraw() on every prop or state change, causing unnecessary canvas operations
and redraws.
###### Why this matters
This leads to performance degradation as the canvas is resized and redrawn
even when the changes don't affect the visual output, such as when unrelated
props change or when the canvas size hasn't actually changed.
###### Suggested change ∙ *Feature Preview*
Add prop comparison to only update canvas size and redraw when relevant
props change:
```typescript
componentDidUpdate(prevProps: ScatterPlotGlowOverlayProps) {
const shouldRedraw =
prevProps.locations !== this.props.locations ||
prevProps.zoom !== this.props.zoom ||
prevProps.isDragging !== this.props.isDragging ||
prevProps.dotRadius !== this.props.dotRadius ||
prevProps.rgb !== this.props.rgb ||
prevProps.aggregation !== this.props.aggregation;
if (shouldRedraw) {
this.updateCanvasSize();
this.redraw();
}
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a2fb626c-026f-4649-bb07-48417f36c795/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a2fb626c-026f-4649-bb07-48417f36c795?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a2fb626c-026f-4649-bb07-48417f36c795?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a2fb626c-026f-4649-bb07-48417f36c795?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/a2fb626c-026f-4649-bb07-48417f36c795)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:50418465-7de7-45ba-8410-f13ec0a1c5a4 -->
[](50418465-7de7-45ba-8410-f13ec0a1c5a4)
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -16,51 +16,81 @@
* specific language governing permissions and limitations
* under the License.
*/
-/* eslint-disable react/require-default-props */
-import PropTypes from 'prop-types';
-import { PureComponent } from 'react';
-import { CanvasOverlay } from 'react-map-gl';
+import { PureComponent, createRef, RefObject } from 'react';
+import { useMap } from 'react-map-gl/mapbox';
+import type { MapRef } from 'react-map-gl/mapbox';
import { kmToPixels, MILES_PER_KM } from './utils/geo';
import roundDecimal from './utils/roundDecimal';
import luminanceFromRGB from './utils/luminanceFromRGB';
import 'mapbox-gl/dist/mapbox-gl.css';
-const propTypes = {
- aggregation: PropTypes.string,
- compositeOperation: PropTypes.string,
- dotRadius: PropTypes.number,
- lngLatAccessor: PropTypes.func,
- locations: PropTypes.arrayOf(PropTypes.object).isRequired,
- pointRadiusUnit: PropTypes.string,
- renderWhileDragging: PropTypes.bool,
- rgb: PropTypes.arrayOf(
- PropTypes.oneOfType([PropTypes.string, PropTypes.number]),
- ),
- zoom: PropTypes.number,
-};
+type AggregationType = 'sum' | 'min' | 'max' | 'mean' | 'var' | 'stdev';
+
+interface LocationProperties {
+ cluster?: boolean;
+ point_count?: number;
+ sum?: number;
+ squaredSum?: number;
+ radius?: number | null;
+ metric?: number | string | null;
+ [key: string]: unknown;
+}
+
+interface Location {
+ properties: LocationProperties;
+ [key: string]: unknown;
+}
+
+interface ScatterPlotGlowOverlayProps {
+ aggregation?: AggregationType;
+ compositeOperation?: GlobalCompositeOperation;
+ dotRadius?: number;
+ isDragging?: boolean;
+ lngLatAccessor?: (location: Location) => [number, number];
+ locations: Location[];
+ mapRef?: { current: MapRef | null | undefined };
+ pointRadiusUnit?: string;
+ renderWhileDragging?: boolean;
+ rgb?: [number, number, number, number];
+ zoom?: number;
+}
-const defaultProps = {
+interface DrawTextOptions {
+ fontHeight?: number;
+ label?: string | number;
+ radius?: number;
+ rgb?: [number, number, number, number];
+ shadow?: boolean;
+}
+
+const defaultProps: Partial<ScatterPlotGlowOverlayProps> = {
// Same as browser default.
compositeOperation: 'source-over',
dotRadius: 4,
- lngLatAccessor: location => [location[0], location[1]],
+ lngLatAccessor: (location: Location) => [
+ location[0] as number,
+ location[1] as number,
+ ],
Review Comment:
### Invalid default lngLatAccessor assumes array-like location structure
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The default lngLatAccessor function assumes location has numeric indices [0]
and [1], but the Location interface defines location as having properties and
[key: string]: unknown, which doesn't guarantee numeric array access.
###### Why this matters
This will cause runtime errors when the default accessor tries to access
location[0] and location[1] on objects that don't have these numeric indices,
leading to undefined values being passed to map projection functions.
###### Suggested change ∙ *Feature Preview*
Update the default accessor to handle the actual location data structure.
Based on the usage pattern, it should likely access coordinate properties:
```typescript
lngLatAccessor: (location: Location) => {
// Assuming coordinates are stored in a specific property
// This needs to match the actual data structure
const coords = location.coordinates || location.geometry?.coordinates;
return coords ? [coords[0], coords[1]] : [0, 0];
},
```
Or if locations are expected to have lng/lat properties:
```typescript
lngLatAccessor: (location: Location) => [
(location as any).lng || (location as any).longitude || 0,
(location as any).lat || (location as any).latitude || 0,
],
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a00e58d-fef7-4c83-be94-246189b81789/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a00e58d-fef7-4c83-be94-246189b81789?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a00e58d-fef7-4c83-be94-246189b81789?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a00e58d-fef7-4c83-be94-246189b81789?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/2a00e58d-fef7-4c83-be94-246189b81789)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:23597c17-79c0-40c8-9cac-b290e65ddf47 -->
[](23597c17-79c0-40c8-9cac-b290e65ddf47)
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -120,38 +151,36 @@ class MapBox extends Component {
const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom));
Review Comment:
### Uncached clustering computation on every render <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The getClusters method is called on every render without any memoization or
caching mechanism.
###### Why this matters
This causes expensive clustering calculations to be performed repeatedly
even when the viewport hasn't changed significantly, leading to poor rendering
performance and unnecessary CPU usage during map interactions.
###### Suggested change ∙ *Feature Preview*
Implement memoization using useMemo or cache the clustering results based on
viewport changes. Only recalculate clusters when zoom level or bounds change
significantly:
```typescript
const clusters = useMemo(() => {
return clusterer.getClusters(bbox, Math.round(viewport.zoom));
}, [clusterer, bbox, Math.round(viewport.zoom)]);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67339ea3-64d7-422b-a161-e529167fc303/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67339ea3-64d7-422b-a161-e529167fc303?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67339ea3-64d7-422b-a161-e529167fc303?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67339ea3-64d7-422b-a161-e529167fc303?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/67339ea3-64d7-422b-a161-e529167fc303)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:965e589c-7eb7-47eb-89b6-5c1c6cb89b40 -->
[](965e589c-7eb7-47eb-89b6-5c1c6cb89b40)
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -29,24 +26,53 @@ const NOOP = () => {};
export const DEFAULT_MAX_ZOOM = 16;
export const DEFAULT_POINT_RADIUS = 60;
-const propTypes = {
- width: PropTypes.number,
- height: PropTypes.number,
- aggregatorName: PropTypes.string,
- clusterer: PropTypes.object,
- globalOpacity: PropTypes.number,
- hasCustomMetric: PropTypes.bool,
- mapStyle: PropTypes.string,
- mapboxApiKey: PropTypes.string.isRequired,
- onViewportChange: PropTypes.func,
- pointRadius: PropTypes.number,
- pointRadiusUnit: PropTypes.string,
- renderWhileDragging: PropTypes.bool,
- rgb: PropTypes.array,
- bounds: PropTypes.array,
-};
+interface Clusterer {
+ getClusters(bbox: number[], zoom: number): Location[];
+}
-const defaultProps = {
+interface Geometry {
+ coordinates: [number, number];
+ type: string;
+}
+
+interface Location {
+ geometry: Geometry;
+ properties: {
+ cluster?: boolean;
+ [key: string]: unknown;
+ };
+ [key: string]: unknown;
+}
Review Comment:
### Over-permissive Type Definition <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The Location interface uses excessive index signatures ([key: string]:
unknown) which makes the type too permissive and reduces type safety.
###### Why this matters
Overly permissive types can hide potential bugs and make it harder to catch
errors at compile time.
###### Suggested change ∙ *Feature Preview*
Define a more specific interface with only the required properties:
```typescript
interface Location {
geometry: Geometry;
properties: {
cluster?: boolean;
// Define specific properties needed
point_count?: number;
metric?: number;
};
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1e84686-cd58-46ba-8348-80ea04d82a03/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1e84686-cd58-46ba-8348-80ea04d82a03?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1e84686-cd58-46ba-8348-80ea04d82a03?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1e84686-cd58-46ba-8348-80ea04d82a03?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/e1e84686-cd58-46ba-8348-80ea04d82a03)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:98734657-a647-4edd-bf4b-76b68a96da8f -->
[](98734657-a647-4edd-bf4b-76b68a96da8f)
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx:
##########
@@ -216,27 +312,27 @@ class ScatterPlotGlowOverlay extends PureComponent {
const pointMetric = location.properties.metric;
let pointRadius =
radiusProperty === null ? defaultRadius : radiusProperty;
- let pointLabel;
+ let pointLabel: string | number | undefined;
- if (radiusProperty !== null) {
+ if (radiusProperty !== null && lngLatAccessor) {
const pointLatitude = lngLatAccessor(location)[1];
Review Comment:
### Unnecessary null check for lngLatAccessor with default value
<sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The code checks if lngLatAccessor exists before using it, but lngLatAccessor
is guaranteed to exist due to the default props, making this check unnecessary
and potentially confusing.
###### Why this matters
This creates dead code paths and suggests uncertainty about the accessor's
availability, which could lead to maintenance issues and confusion about when
the latitude-based radius calculations are performed.
###### Suggested change ∙ *Feature Preview*
Remove the unnecessary null check since lngLatAccessor always has a default
value:
```typescript
if (radiusProperty !== null) {
const pointLatitude = lngLatAccessor!(location)[1];
// ... rest of the logic
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8956fc5e-f089-4fde-bc5d-6f7cf157aabc/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8956fc5e-f089-4fde-bc5d-6f7cf157aabc?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8956fc5e-f089-4fde-bc5d-6f7cf157aabc?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8956fc5e-f089-4fde-bc5d-6f7cf157aabc?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/8956fc5e-f089-4fde-bc5d-6f7cf157aabc)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:0f3f22bd-71a4-4477-b924-0ca2154052a0 -->
[](0f3f22bd-71a4-4477-b924-0ca2154052a0)
--
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]
