Skip to content

chore: udpate react-map-gl from v6 to v8 and refactor MapBox chart to TypeScript #35525

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+1,198 −719
Open

chore: udpate react-map-gl from v6 to v8 and refactor MapBox chart to TypeScript #35525

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2e10aaf
Upgrade react-map-gl from v6 to v8
DamianPendrak Oct 6, 2025
f9f69cb
Refactor MapBox chart to TypeScript
DamianPendrak Oct 6, 2025
67ea990
Code improvements
DamianPendrak Oct 6, 2025
2e53d9a
Mock Supercluster
DamianPendrak Oct 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
872 changes: 513 additions & 359 deletions superset-frontend/package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 3 additions & 2 deletions superset-frontend/plugins/legacy-plugin-chart-map-box/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,15 @@
"dependencies": {
"@math.gl/web-mercator": "^4.1.0",
"prop-types": "^15.8.1",
"react-map-gl": "^6.1.19",
"react-map-gl": "^8.0.4",
"supercluster": "^8.0.1"
},
"peerDependencies": {
"@superset-ui/chart-controls": "*",
"@superset-ui/core": "*",
"mapbox-gl": "*",
"react": "^17.0.2"
"react": "^17.0.2",
"react-dom": "^17.0.2"
},
"publishConfig": {
"access": "public"
Expand Down
150 changes: 90 additions & 60 deletions ...egacy-plugin-chart-map-box/src/MapBox.jsx → ...egacy-plugin-chart-map-box/src/MapBox.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,37 +16,65 @@
* specific language governing permissions and limitations
* under the License.
*/
/* eslint-disable react/jsx-sort-default-props, react/sort-prop-types */
/* eslint-disable react/forbid-prop-types, react/require-default-props */
import { Component } from 'react';
import PropTypes from 'prop-types';
import MapGL from 'react-map-gl';
import Map, { ViewStateChangeEvent } from 'react-map-gl/mapbox';
import { WebMercatorViewport } from '@math.gl/web-mercator';
import ScatterPlotGlowOverlay from './ScatterPlotGlowOverlay';
import ScatterPlotGlowOverlay, {
AggregationType,
} from './ScatterPlotGlowOverlay';
import './MapBox.css';

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;
}
Comment on lines +40 to +47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Over-permissive Type Definition category Design

Tell me more
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:

interface Location {
  geometry: Geometry;
  properties: {
    cluster?: boolean;
    // Define specific properties needed
    point_count?: number;
    metric?: number;
  };
}
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


interface Viewport {
longitude: number;
latitude: number;
zoom: number;
isDragging?: boolean;
}

interface MapBoxProps {
aggregatorName?: AggregationType;
bounds: [[number, number], [number, number]];
clusterer: Clusterer;
globalOpacity?: number;
hasCustomMetric?: boolean;
height?: number;
mapStyle?: string;
mapboxApiKey: string;
onViewportChange?: (viewport: Viewport) => void;
pointRadius?: number;
pointRadiusUnit?: string;
renderWhileDragging?: boolean;
rgb?: [number, number, number, number];
width?: number;
}

interface MapBoxState {
viewport: Viewport;
}

const defaultProps: Partial<MapBoxProps> = {
width: 400,
height: 400,
globalOpacity: 1,
Expand All @@ -55,17 +83,19 @@ const defaultProps = {
pointRadiusUnit: 'Pixels',
};

class MapBox extends Component {
constructor(props) {
class MapBox extends Component<MapBoxProps, MapBoxState> {
static defaultProps = defaultProps;

constructor(props: MapBoxProps) {
super(props);

const { width, height, bounds } = this.props;
// Get a viewport that fits the given bounds, which all marks to be clustered.
// Derive lat, lon and zoom from this viewport. This is only done on initial
// render as the bounds don't update as we pan/zoom in the current design.
const mercator = new WebMercatorViewport({
width,
height,
width: width || 400,
height: height || 400,
}).fitBounds(bounds);
const { latitude, longitude, zoom } = mercator;

Expand All @@ -79,10 +109,14 @@ class MapBox extends Component {
this.handleViewportChange = this.handleViewportChange.bind(this);
}

handleViewportChange(viewport) {
handleViewportChange(evt: ViewStateChangeEvent) {
const { latitude, longitude, zoom } = evt.viewState;
const viewport: Viewport = { latitude, longitude, zoom };
this.setState({ viewport });
const { onViewportChange } = this.props;
onViewportChange(viewport);
if (onViewportChange) {
onViewportChange(viewport);
}
}

render() {
Expand All @@ -91,7 +125,6 @@ class MapBox extends Component {
height,
aggregatorName,
clusterer,
globalOpacity,
mapStyle,
mapboxApiKey,
pointRadius,
Expand All @@ -109,8 +142,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;
Comment on lines +145 to +146
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic Numbers in Viewport Calculations category Design

Tell me more
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:

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

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.

const bbox = [
bounds[0][0] - offsetHorizontal,
bounds[0][1] - offsetVertical,
Expand All @@ -120,38 +153,35 @@ class MapBox extends Component {
const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uncached clustering computation on every render category Performance

Tell me more
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:

const clusters = useMemo(() => {
  return clusterer.getClusters(bbox, Math.round(viewport.zoom));
}, [clusterer, bbox, Math.round(viewport.zoom)]);
Provide feedback to improve future suggestions

Nice Catch Incorrect Not in Scope Not in coding standard Other

💬 Looking for more details? Reply to this comment to chat with Korbit.


return (
<MapGL
{...viewport}
mapStyle={mapStyle}
width={width}
height={height}
mapboxApiAccessToken={mapboxApiKey}
onViewportChange={this.handleViewportChange}
preserveDrawingBuffer
>
<ScatterPlotGlowOverlay
<div style={{ width, height }}>
<Map
{...viewport}
isDragging={isDragging}
locations={clusters}
dotRadius={pointRadius}
pointRadiusUnit={pointRadiusUnit}
rgb={rgb}
globalOpacity={globalOpacity}
compositeOperation="screen"
renderWhileDragging={renderWhileDragging}
aggregation={hasCustomMetric ? aggregatorName : null}
lngLatAccessor={location => {
const { coordinates } = location.geometry;

return [coordinates[0], coordinates[1]];
}}
/>
</MapGL>
mapStyle={mapStyle}
mapboxAccessToken={mapboxApiKey}
onMove={this.handleViewportChange}
preserveDrawingBuffer
style={{ width: '100%', height: '100%' }}
>
<ScatterPlotGlowOverlay
{...viewport}
isDragging={isDragging}
locations={clusters}
dotRadius={pointRadius}
pointRadiusUnit={pointRadiusUnit}
rgb={rgb}
compositeOperation="screen"
renderWhileDragging={renderWhileDragging}
aggregation={hasCustomMetric ? aggregatorName : undefined}
lngLatAccessor={location => {
const { coordinates } = location.geometry;

return [coordinates[0], coordinates[1]];
}}
/>
</Map>
</div>
);
}
}

MapBox.propTypes = propTypes;
MapBox.defaultProps = defaultProps;

export default MapBox;
Loading
Loading