chore(lint): convert legacy plugin chart components to function components#39452
Open
rusackas wants to merge 5 commits intochore/lint-cleanup-function-componentsfrom
Open
chore(lint): convert legacy plugin chart components to function components#39452rusackas wants to merge 5 commits intochore/lint-cleanup-function-componentsfrom
rusackas wants to merge 5 commits intochore/lint-cleanup-function-componentsfrom
Conversation
…nents Converts HorizonChart, HorizonRow, MapBox, ScatterPlotGlowOverlay, PairedTTest, TTestTable, and WordCloud from class components to function components. Includes updated TTestTable test. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
Bito Automatic Review Skipped - Branch Excluded |
dosubot
bot
added
change:frontend
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+81
to
+99
| const initialViewport = useMemo((): Viewport => { | ||
| let latitude = 0; | ||
| let longitude = 0; | ||
| let zoom = 1; | ||
|
|
||
| const defaultProps: Partial<MapBoxProps> = { | ||
| width: 400, | ||
| height: 400, | ||
| globalOpacity: 1, | ||
| onViewportChange: NOOP, | ||
| pointRadius: DEFAULT_POINT_RADIUS, | ||
| pointRadiusUnit: 'Pixels', | ||
| }; | ||
|
|
||
| class MapBox extends Component<MapBoxProps, MapBoxState> { | ||
| static defaultProps = defaultProps; | ||
|
|
||
| constructor(props: MapBoxProps) { | ||
| super(props); | ||
|
|
||
| const fitBounds = this.computeFitBoundsViewport(); | ||
|
|
||
| this.state = { | ||
| viewport: this.mergeViewportWithProps(fitBounds), | ||
| }; | ||
| this.handleViewportChange = this.handleViewportChange.bind(this); | ||
| } | ||
|
|
||
| handleViewportChange(viewport: Viewport) { | ||
| this.setState({ viewport }); | ||
| const { onViewportChange } = this.props; | ||
| onViewportChange!(viewport); | ||
| } | ||
|
|
||
| mergeViewportWithProps( | ||
| fitBounds: Viewport, | ||
| viewport: Viewport = fitBounds, | ||
| props: MapBoxProps = this.props, | ||
| useFitBoundsForUnset = true, | ||
| ): Viewport { | ||
| const { viewportLongitude, viewportLatitude, viewportZoom } = props; | ||
|
|
||
| return { | ||
| ...viewport, | ||
| longitude: | ||
| viewportLongitude ?? | ||
| (useFitBoundsForUnset ? fitBounds.longitude : viewport.longitude), | ||
| latitude: | ||
| viewportLatitude ?? | ||
| (useFitBoundsForUnset ? fitBounds.latitude : viewport.latitude), | ||
| zoom: | ||
| viewportZoom ?? (useFitBoundsForUnset ? fitBounds.zoom : viewport.zoom), | ||
| }; | ||
| } | ||
|
|
||
| computeFitBoundsViewport(): Viewport { | ||
| const { width = 400, height = 400, bounds } = this.props; | ||
| // Guard against empty datasets where bounds may be undefined | ||
| if (bounds && bounds[0] && bounds[1]) { | ||
| const mercator = new WebMercatorViewport({ width, height }).fitBounds( | ||
| bounds, | ||
| ); | ||
| return { | ||
| latitude: mercator.latitude, | ||
| longitude: mercator.longitude, | ||
| zoom: mercator.zoom, | ||
| }; | ||
| } | ||
| return { latitude: 0, longitude: 0, zoom: 1 }; | ||
| } | ||
|
|
||
| componentDidUpdate(prevProps: MapBoxProps) { | ||
| const { viewport } = this.state; | ||
| const fitBoundsInputsChanged = | ||
| prevProps.width !== this.props.width || | ||
| prevProps.height !== this.props.height || | ||
| prevProps.bounds !== this.props.bounds; | ||
| const viewportPropsChanged = | ||
| prevProps.viewportLongitude !== this.props.viewportLongitude || | ||
| prevProps.viewportLatitude !== this.props.viewportLatitude || | ||
| prevProps.viewportZoom !== this.props.viewportZoom; | ||
|
|
||
| if (!fitBoundsInputsChanged && !viewportPropsChanged) { | ||
| return; | ||
| const mercator = new WebMercatorViewport({ | ||
| width, | ||
| height, | ||
| }).fitBounds(bounds); | ||
| ({ latitude, longitude, zoom } = mercator); | ||
| } | ||
|
|
||
| const fitBounds = this.computeFitBoundsViewport(); | ||
| const nextViewport = this.mergeViewportWithProps( | ||
| fitBounds, | ||
| viewport, | ||
| this.props, | ||
| fitBoundsInputsChanged || viewportPropsChanged, | ||
| ); | ||
|
|
||
| const viewportChanged = | ||
| nextViewport.longitude !== viewport.longitude || | ||
| nextViewport.latitude !== viewport.latitude || | ||
| nextViewport.zoom !== viewport.zoom; | ||
|
|
||
| if (viewportChanged) { | ||
| this.setState({ viewport: nextViewport }); | ||
| } | ||
| } | ||
|
|
||
| render() { | ||
| const { | ||
| width, | ||
| height, | ||
| aggregatorName, | ||
| clusterer, | ||
| globalOpacity, | ||
| mapStyle, | ||
| mapboxApiKey, | ||
| pointRadius, | ||
| pointRadiusUnit, | ||
| renderWhileDragging, | ||
| rgb, | ||
| hasCustomMetric, | ||
| bounds, | ||
| } = this.props; | ||
| const { viewport } = this.state; | ||
| const isDragging = | ||
| viewport.isDragging === undefined ? false : viewport.isDragging; | ||
|
|
||
| // Compute the clusters based on the original bounds and current zoom level. Note when zoom/pan | ||
| // 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 ?? 400) * 0.5) / 100; | ||
| const offsetVertical = ((height ?? 400) * 0.5) / 100; | ||
|
|
||
| // Guard against empty datasets where bounds may be undefined | ||
| const bbox = | ||
| bounds && bounds[0] && bounds[1] | ||
| ? [ | ||
| bounds[0][0] - offsetHorizontal, | ||
| bounds[0][1] - offsetVertical, | ||
| bounds[1][0] + offsetHorizontal, | ||
| bounds[1][1] + offsetVertical, | ||
| ] | ||
| : [-180, -90, 180, 90]; // Default to world bounds | ||
|
|
||
| const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom)); | ||
|
|
||
| return ( | ||
| <MapGL | ||
| return { longitude, latitude, zoom }; | ||
| }, []); // Only compute once on mount - bounds don't update as we pan/zoom | ||
|
|
||
| const [viewport, setViewport] = useState<Viewport>(initialViewport); | ||
|
|
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
MapBox now computes the viewport only once on mount and ignores the viewportLongitude/viewportLatitude/viewportZoom props from transformProps, so it no longer reacts to updated bounds/size or externally controlled viewport values (e.g. restoring saved viewport or refitting after control changes).
Suggestion: Reintroduce controlled viewport reconciliation: accept viewportLongitude/viewportLatitude/viewportZoom props, merge them with fitBounds defaults, and recompute the viewport in an effect when bounds, width/height, or viewport control props change, keeping onViewportChange wired through.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx
**Line:** 81:99
**Comment:**
*HIGH: MapBox now computes the viewport only once on mount and ignores the viewportLongitude/viewportLatitude/viewportZoom props from transformProps, so it no longer reacts to updated bounds/size or externally controlled viewport values (e.g. restoring saved viewport or refitting after control changes).
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
Pre-existing behavior — the map-box plugin was deleted on master (c496415) and this PR restores it to its prior class-component state. This behavior is not introduced by this PR; it matches the pre-deletion master version. Leaving as-is.
Comment on lines
+169
to
+225
| const filteredLabels = clusterLabelMap.filter( | ||
| v => !Number.isNaN(v), | ||
| ) as number[]; | ||
| // Guard against empty array or zero max to prevent NaN from division | ||
| const maxLabel = | ||
| filteredLabels.length > 0 ? Math.max(...filteredLabels) : 1; | ||
| const safeMaxLabel = maxLabel > 0 ? maxLabel : 1; | ||
|
|
||
| // Calculate min/max radius values for Pixels mode scaling | ||
| let minRadiusValue = Infinity; | ||
| let maxRadiusValue = -Infinity; | ||
| if (pointRadiusUnit === 'Pixels') { | ||
| locations.forEach(location => { | ||
| // Accept both null and undefined as "no value" and coerce potential numeric strings | ||
| if ( | ||
| !location.properties.cluster && | ||
| location.properties.radius != null | ||
| ) { | ||
| const radiusValueRaw = location.properties.radius; | ||
| const radiusValue = Number(radiusValueRaw); | ||
| if (Number.isFinite(radiusValue)) { | ||
| minRadiusValue = Math.min(minRadiusValue, radiusValue); | ||
| maxRadiusValue = Math.max(maxRadiusValue, radiusValue); | ||
| } | ||
| } else { | ||
| const defaultRadius = radius * MIN_CLUSTER_RADIUS_RATIO; | ||
| const rawRadius = location.properties.radius; | ||
| const numericRadiusProperty = | ||
| rawRadius != null && | ||
| !(typeof rawRadius === 'string' && rawRadius.trim() === '') | ||
| ? Number(rawRadius) | ||
| : null; | ||
| const radiusProperty = | ||
| numericRadiusProperty != null && | ||
| Number.isFinite(numericRadiusProperty) | ||
| ? numericRadiusProperty | ||
| : null; | ||
| const pointMetric = location.properties.metric ?? null; | ||
| let pointRadius: number = radiusProperty ?? defaultRadius; | ||
| let pointLabel: string | number | undefined; | ||
|
|
||
| if (radiusProperty != null) { | ||
| const pointLatitude = lngLatAccessor!(location)[1]; | ||
| if (pointRadiusUnit === 'Kilometers') { | ||
| pointLabel = `${roundDecimal(pointRadius, 2)}km`; | ||
| pointRadius = kmToPixels(pointRadius, pointLatitude, zoom ?? 0); | ||
| } else if (pointRadiusUnit === 'Miles') { | ||
| pointLabel = `${roundDecimal(pointRadius, 2)}mi`; | ||
| pointRadius = kmToPixels( | ||
| pointRadius * MILES_PER_KM, | ||
| pointLatitude, | ||
| zoom ?? 0, | ||
| ); | ||
| } else if (pointRadiusUnit === 'Pixels') { | ||
| // Scale pixel values to a reasonable range (radius/6 to radius/3) | ||
| // This ensures points are visible and proportional to their values | ||
| const MIN_POINT_RADIUS = radius * MIN_CLUSTER_RADIUS_RATIO; | ||
| const MAX_POINT_RADIUS = radius * MAX_POINT_RADIUS_RATIO; | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| if ( | ||
| Number.isFinite(minRadiusValue) && | ||
| Number.isFinite(maxRadiusValue) && | ||
| maxRadiusValue > minRadiusValue | ||
| ) { | ||
| // Normalize the value to 0-1 range, then scale to pixel range | ||
| const numericPointRadius = Number(pointRadius); | ||
| if (!Number.isFinite(numericPointRadius)) { | ||
| // fallback to minimum visible size when the value is not a finite number | ||
| pointRadius = MIN_POINT_RADIUS; | ||
| ctx.clearRect(0, 0, width, height); | ||
| ctx.globalCompositeOperation = (compositeOperation ?? | ||
| 'source-over') as GlobalCompositeOperation; | ||
|
|
||
| if ((renderWhileDragging || !isDragging) && locations) { | ||
| locations.forEach((location: GeoJSONLocation, i: number) => { | ||
| const pixel = project(lngLatAccessor(location)) as [number, number]; | ||
| const pixelRounded: [number, number] = [ | ||
| roundDecimal(pixel[0], 1), | ||
| roundDecimal(pixel[1], 1), | ||
| ]; | ||
|
|
||
| if ( | ||
| pixelRounded[0] + radius >= 0 && | ||
| pixelRounded[0] - radius < width && | ||
| pixelRounded[1] + radius >= 0 && | ||
| pixelRounded[1] - radius < height | ||
| ) { | ||
| ctx.beginPath(); | ||
| if (location.properties.cluster) { | ||
| const clusterLabel = clusterLabelMap[i]; | ||
| // Validate clusterLabel is a finite number before using it for radius calculation | ||
| const numericLabel = Number(clusterLabel); | ||
| const safeNumericLabel = Number.isFinite(numericLabel) | ||
| ? numericLabel | ||
| : 0; | ||
| const scaledRadius = roundDecimal( | ||
| (safeNumericLabel / safeMaxLabel) ** 0.5 * radius, | ||
| 1, |
Contributor
There was a problem hiding this comment.
🟠 Architect Review — HIGH
Cluster radius scaling now uses the signed cluster label directly in (label / safeMaxLabel) ** 0.5, so negative aggregated labels (e.g. sum/min/mean < 0) produce NaN radii and effectively invisible clusters instead of being sized by the magnitude of the value.
Suggestion: When computing cluster radii, base the radius on the absolute, finite label magnitude (e.g. sqrt(|label| / max|label|) * dotRadius with a minimum radius), and keep the sign only for label text formatting to avoid NaN and ensure negative clusters render correctly.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx
**Line:** 169:225
**Comment:**
*HIGH: Cluster radius scaling now uses the signed cluster label directly in (label / safeMaxLabel) ** 0.5, so negative aggregated labels (e.g. sum/min/mean < 0) produce NaN radii and effectively invisible clusters instead of being sized by the magnitude of the value.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
Pre-existing behavior — the map-box plugin was deleted on master (c496415) and this PR restores it to its prior class-component state. This behavior is not introduced by this PR; it matches the pre-deletion master version. Leaving as-is.
Comment on lines
+19
to
+98
| @@ -61,183 +59,105 @@ interface MapBoxProps { | |||
| renderWhileDragging?: boolean; | |||
| rgb?: (string | number)[]; | |||
| bounds?: [[number, number], [number, number]]; // May be undefined for empty datasets | |||
| viewportLongitude?: number; | |||
| viewportLatitude?: number; | |||
| viewportZoom?: number; | |||
| } | |||
|
|
|||
| interface MapBoxState { | |||
| viewport: Viewport; | |||
| } | |||
| function MapBox({ | |||
| width = 400, | |||
| height = 400, | |||
| aggregatorName, | |||
| clusterer, | |||
| globalOpacity = 1, | |||
| hasCustomMetric, | |||
| mapStyle, | |||
| mapboxApiKey, | |||
| onViewportChange = NOOP, | |||
| pointRadius = DEFAULT_POINT_RADIUS, | |||
| pointRadiusUnit = 'Pixels', | |||
| renderWhileDragging, | |||
| rgb, | |||
| bounds, | |||
| }: MapBoxProps) { | |||
| // Compute initial viewport from bounds | |||
| const initialViewport = useMemo((): Viewport => { | |||
| let latitude = 0; | |||
| let longitude = 0; | |||
| let zoom = 1; | |||
|
|
|||
| const defaultProps: Partial<MapBoxProps> = { | |||
| width: 400, | |||
| height: 400, | |||
| globalOpacity: 1, | |||
| onViewportChange: NOOP, | |||
| pointRadius: DEFAULT_POINT_RADIUS, | |||
| pointRadiusUnit: 'Pixels', | |||
| }; | |||
|
|
|||
| class MapBox extends Component<MapBoxProps, MapBoxState> { | |||
| static defaultProps = defaultProps; | |||
|
|
|||
| constructor(props: MapBoxProps) { | |||
| super(props); | |||
|
|
|||
| const fitBounds = this.computeFitBoundsViewport(); | |||
|
|
|||
| this.state = { | |||
| viewport: this.mergeViewportWithProps(fitBounds), | |||
| }; | |||
| this.handleViewportChange = this.handleViewportChange.bind(this); | |||
| } | |||
|
|
|||
| handleViewportChange(viewport: Viewport) { | |||
| this.setState({ viewport }); | |||
| const { onViewportChange } = this.props; | |||
| onViewportChange!(viewport); | |||
| } | |||
|
|
|||
| mergeViewportWithProps( | |||
| fitBounds: Viewport, | |||
| viewport: Viewport = fitBounds, | |||
| props: MapBoxProps = this.props, | |||
| useFitBoundsForUnset = true, | |||
| ): Viewport { | |||
| const { viewportLongitude, viewportLatitude, viewportZoom } = props; | |||
|
|
|||
| return { | |||
| ...viewport, | |||
| longitude: | |||
| viewportLongitude ?? | |||
| (useFitBoundsForUnset ? fitBounds.longitude : viewport.longitude), | |||
| latitude: | |||
| viewportLatitude ?? | |||
| (useFitBoundsForUnset ? fitBounds.latitude : viewport.latitude), | |||
| zoom: | |||
| viewportZoom ?? (useFitBoundsForUnset ? fitBounds.zoom : viewport.zoom), | |||
| }; | |||
| } | |||
|
|
|||
| computeFitBoundsViewport(): Viewport { | |||
| const { width = 400, height = 400, bounds } = this.props; | |||
| // Guard against empty datasets where bounds may be undefined | |||
| if (bounds && bounds[0] && bounds[1]) { | |||
| const mercator = new WebMercatorViewport({ width, height }).fitBounds( | |||
| bounds, | |||
| ); | |||
| return { | |||
| latitude: mercator.latitude, | |||
| longitude: mercator.longitude, | |||
| zoom: mercator.zoom, | |||
| }; | |||
| } | |||
| return { latitude: 0, longitude: 0, zoom: 1 }; | |||
| } | |||
|
|
|||
| componentDidUpdate(prevProps: MapBoxProps) { | |||
| const { viewport } = this.state; | |||
| const fitBoundsInputsChanged = | |||
| prevProps.width !== this.props.width || | |||
| prevProps.height !== this.props.height || | |||
| prevProps.bounds !== this.props.bounds; | |||
| const viewportPropsChanged = | |||
| prevProps.viewportLongitude !== this.props.viewportLongitude || | |||
| prevProps.viewportLatitude !== this.props.viewportLatitude || | |||
| prevProps.viewportZoom !== this.props.viewportZoom; | |||
|
|
|||
| if (!fitBoundsInputsChanged && !viewportPropsChanged) { | |||
| return; | |||
| const mercator = new WebMercatorViewport({ | |||
| width, | |||
| height, | |||
| }).fitBounds(bounds); | |||
| ({ latitude, longitude, zoom } = mercator); | |||
| } | |||
|
|
|||
| const fitBounds = this.computeFitBoundsViewport(); | |||
| const nextViewport = this.mergeViewportWithProps( | |||
| fitBounds, | |||
| viewport, | |||
| this.props, | |||
| fitBoundsInputsChanged || viewportPropsChanged, | |||
| ); | |||
|
|
|||
| const viewportChanged = | |||
| nextViewport.longitude !== viewport.longitude || | |||
| nextViewport.latitude !== viewport.latitude || | |||
| nextViewport.zoom !== viewport.zoom; | |||
|
|
|||
| if (viewportChanged) { | |||
| this.setState({ viewport: nextViewport }); | |||
| } | |||
| } | |||
|
|
|||
| render() { | |||
| const { | |||
| width, | |||
| height, | |||
| aggregatorName, | |||
| clusterer, | |||
| globalOpacity, | |||
| mapStyle, | |||
| mapboxApiKey, | |||
| pointRadius, | |||
| pointRadiusUnit, | |||
| renderWhileDragging, | |||
| rgb, | |||
| hasCustomMetric, | |||
| bounds, | |||
| } = this.props; | |||
| const { viewport } = this.state; | |||
| const isDragging = | |||
| viewport.isDragging === undefined ? false : viewport.isDragging; | |||
|
|
|||
| // Compute the clusters based on the original bounds and current zoom level. Note when zoom/pan | |||
| // 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 ?? 400) * 0.5) / 100; | |||
| const offsetVertical = ((height ?? 400) * 0.5) / 100; | |||
|
|
|||
| // Guard against empty datasets where bounds may be undefined | |||
| const bbox = | |||
| bounds && bounds[0] && bounds[1] | |||
| ? [ | |||
| bounds[0][0] - offsetHorizontal, | |||
| bounds[0][1] - offsetVertical, | |||
| bounds[1][0] + offsetHorizontal, | |||
| bounds[1][1] + offsetVertical, | |||
| ] | |||
| : [-180, -90, 180, 90]; // Default to world bounds | |||
|
|
|||
| const clusters = clusterer.getClusters(bbox, Math.round(viewport.zoom)); | |||
|
|
|||
| return ( | |||
| <MapGL | |||
| return { longitude, latitude, zoom }; | |||
| }, []); // Only compute once on mount - bounds don't update as we pan/zoom | |||
|
|
|||
| const [viewport, setViewport] = useState<Viewport>(initialViewport); | |||
Contributor
There was a problem hiding this comment.
Suggestion: The component dropped support for externally controlled viewport fields and only initializes state once, so control panel values (viewportLongitude, viewportLatitude, viewportZoom) and later prop updates are ignored. Wire these props back into viewport computation and synchronize internal state when computed viewport inputs change. [logic error]
Severity Level: Critical 🚨
- ❌ MapBox chart viewport controls have no visible effect.
- ❌ MapBox charts ignore new data bounds after filter changes.
- ⚠️ Users cannot persist or share intended map zoom/center.
Suggested change
| import { useState, useCallback, useMemo, useEffect, memo } from 'react'; | |
| import MapGL from 'react-map-gl'; | |
| import { WebMercatorViewport } from '@math.gl/web-mercator'; | |
| import ScatterPlotGlowOverlay from './ScatterPlotGlowOverlay'; | |
| import './MapBox.css'; | |
| const NOOP = () => {}; | |
| export const DEFAULT_MAX_ZOOM = 16; | |
| export const DEFAULT_POINT_RADIUS = 60; | |
| interface Viewport { | |
| longitude: number; | |
| latitude: number; | |
| zoom: number; | |
| isDragging?: boolean; | |
| } | |
| interface Clusterer { | |
| getClusters(bbox: number[], zoom: number): GeoJSONLocation[]; | |
| } | |
| interface GeoJSONLocation { | |
| geometry: { | |
| coordinates: [number, number]; | |
| }; | |
| properties: Record<string, number | string | boolean | null | undefined>; | |
| } | |
| interface MapBoxProps { | |
| width?: number; | |
| height?: number; | |
| aggregatorName?: string; | |
| clusterer: Clusterer; // Required - used for getClusters() | |
| globalOpacity?: number; | |
| hasCustomMetric?: boolean; | |
| mapStyle?: string; | |
| mapboxApiKey: string; | |
| onViewportChange?: (viewport: Viewport) => void; | |
| pointRadius?: number; | |
| pointRadiusUnit?: string; | |
| renderWhileDragging?: boolean; | |
| rgb?: (string | number)[]; | |
| bounds?: [[number, number], [number, number]]; // May be undefined for empty datasets | |
| viewportLongitude?: number; | |
| viewportLatitude?: number; | |
| viewportZoom?: number; | |
| } | |
| function MapBox({ | |
| width = 400, | |
| height = 400, | |
| aggregatorName, | |
| clusterer, | |
| globalOpacity = 1, | |
| hasCustomMetric, | |
| mapStyle, | |
| mapboxApiKey, | |
| onViewportChange = NOOP, | |
| pointRadius = DEFAULT_POINT_RADIUS, | |
| pointRadiusUnit = 'Pixels', | |
| renderWhileDragging, | |
| rgb, | |
| bounds, | |
| viewportLongitude, | |
| viewportLatitude, | |
| viewportZoom, | |
| }: MapBoxProps) { | |
| // Compute initial viewport from bounds | |
| const initialViewport = useMemo((): Viewport => { | |
| let latitude = 0; | |
| let longitude = 0; | |
| let zoom = 1; | |
| // Guard against empty datasets where bounds may be undefined | |
| if (bounds && bounds[0] && bounds[1]) { | |
| const mercator = new WebMercatorViewport({ | |
| width, | |
| height, | |
| }).fitBounds(bounds); | |
| ({ latitude, longitude, zoom } = mercator); | |
| } | |
| return { | |
| longitude: viewportLongitude ?? longitude, | |
| latitude: viewportLatitude ?? latitude, | |
| zoom: viewportZoom ?? zoom, | |
| }; | |
| }, [ | |
| bounds, | |
| width, | |
| height, | |
| viewportLongitude, | |
| viewportLatitude, | |
| viewportZoom, | |
| ]); | |
| const [viewport, setViewport] = useState<Viewport>(initialViewport); | |
| useEffect(() => { | |
| setViewport(initialViewport); | |
| }, [initialViewport]); |
Steps of Reproduction ✅
1. Render a MapBox visualization in Superset, which uses `MapBoxChartPlugin`
(`superset-frontend/plugins/legacy-plugin-chart-map-box/src/index.ts:12-19`) to load
`MapBox` and `transformProps`, so the React chart ultimately renders `<MapBox … />` with
props produced by `transformProps`.
2. In the chart's control panel, under the "Viewport" section
(`superset-frontend/plugins/legacy-plugin-chart-map-box/src/controlPanel.ts:8-55`), set
`Default longitude`, `Default latitude`, and/or `Zoom` (controls `viewport_longitude`,
`viewport_latitude`, `viewport_zoom`) and apply the changes so they are stored in
`formData`.
3. When the chart re-renders, `transformProps`
(`superset-frontend/plugins/legacy-plugin-chart-map-box/src/transformProps.ts:5-16,
58-102`) reads these control values, clamps them, and returns them as `viewportLongitude`,
`viewportLatitude`, and `viewportZoom` props along with `bounds`, `width`, and `height` to
`<MapBox>`.
4. In `MapBox.tsx`, the `MapBoxProps` interface and component destructuring
(`superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:47-79`) do not
include `viewportLongitude`, `viewportLatitude`, or `viewportZoom`, and the initial
viewport is computed once with `useMemo` using an empty dependency array
(`MapBox.tsx:80-96`) and stored in state via `useState` (`MapBox.tsx:98`). As a result,
the rendered `<MapGL>` (`MapBox.tsx:136-145`) always uses the original fitBounds-derived
or default `{longitude, latitude, zoom}` and ignores both the control-panel viewport
values and later prop updates (including new `bounds` or size), so user-configured
viewport settings and data-driven recentering never take effect.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx
**Line:** 19:98
**Comment:**
*Logic Error: The component dropped support for externally controlled viewport fields and only initializes state once, so control panel values (`viewportLongitude`, `viewportLatitude`, `viewportZoom`) and later prop updates are ignored. Wire these props back into viewport computation and synchronize internal state when computed viewport inputs change.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
Pre-existing behavior — the map-box plugin was deleted on master (c496415) and this PR restores it to its prior class-component state. This behavior is not introduced by this PR; it matches the pre-deletion master version. Leaving as-is.
| }: PairedTTestProps) { | ||
| return ( | ||
| <StyledDiv> | ||
| <div className={`superset-legacy-chart-paired-t-test ${className}`}> |
Contributor
There was a problem hiding this comment.
Suggestion: The rendered root class name no longer matches the scoped selector used in this component's styles (paired_ttest), so chart-specific styles tied to that container stop applying. Keep the rendered class aligned with the selector. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Paired t-test chart loses intended overflow scroll behavior.
- ⚠️ Text-based PairedTTest reports may render without proper scrolling.
Suggested change
| <div className={`superset-legacy-chart-paired-t-test ${className}`}> | |
| <div className={`superset-legacy-chart-paired_ttest ${className}`}> |
Steps of Reproduction ✅
1. From the Superset UI, create or open a chart using visualization type
`VizType.PairedTTest`, which is registered in
`superset-frontend/src/visualizations/presets/MainPreset.ts:16` via `new
PairedTTestChartPlugin().configure({ key: VizType.PairedTTest })`.
2. The `PairedTTestChartPlugin` defined in
`superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/index.ts:41-48`
lazy-loads `./PairedTTest`, causing React to render the `PairedTTest` component when this
chart is viewed.
3. In `PairedTTest.tsx:33-37`, styles are scoped inside `StyledDiv` with a selector
`.superset-legacy-chart-paired_ttest .scrollbar-container { overflow: auto; }`, which
expects a root element with class `superset-legacy-chart-paired_ttest`.
4. The rendered root container at `PairedTTest.tsx:119-121` uses
`className={`superset-legacy-chart-paired-t-test ${className}`}`, so no element ever has
class `superset-legacy-chart-paired_ttest`; as a result, the
`.superset-legacy-chart-paired_ttest .scrollbar-container` rule never matches, and any
`scrollbar-container` wrapper around the table will not receive the intended `overflow:
auto` styling when viewing a Paired T-Test chart.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-paired-t-test/src/PairedTTest.tsx
**Line:** 120:120
**Comment:**
*Possible Bug: The rendered root class name no longer matches the scoped selector used in this component's styles (`paired_ttest`), so chart-specific styles tied to that container stop applying. Keep the rendered class aligned with the selector.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
Pre-existing behavior — the map-box plugin was deleted on master (c496415) and this PR restores it to its prior class-component state. This behavior is not introduced by this PR; it matches the pre-deletion master version. Leaving as-is.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Comment on lines
+274
to
+275
| const radiusProperty = | ||
| typeof rawRadius === 'number' ? rawRadius : null; |
Contributor
There was a problem hiding this comment.
Suggestion: Radius parsing now accepts numeric strings for range calculation, but point rendering only accepts number-typed values, so string radii are silently dropped and rendered with default size. Parse and validate string values when assigning per-point radius so numeric string inputs are rendered correctly. [type error]
Severity Level: Major ⚠️
- ⚠️ MapBox scatterplot glow overlay shows incorrect point sizes.
- ⚠️ Numeric radius columns stored as strings misrender in Pixels mode.
Suggested change
| const radiusProperty = | |
| typeof rawRadius === 'number' ? rawRadius : null; | |
| const parsedRadius = | |
| typeof rawRadius === 'string' ? Number(rawRadius.trim()) : Number(rawRadius); | |
| const radiusProperty = | |
| rawRadius == null || | |
| (typeof rawRadius === 'string' && rawRadius.trim() === '') || | |
| !Number.isFinite(parsedRadius) | |
| ? null | |
| : parsedRadius; |
Steps of Reproduction ✅
1. Configure a MapBox chart that renders `ScatterPlotGlowOverlay` with `pointRadiusUnit
=== 'Pixels'` and pass `locations` whose `properties.radius` are numeric strings (e.g.
`'10'`), as consumed by `ScatterPlotGlowOverlay` in
`superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx`.
2. In the normalization phase inside `redraw` (same file, lines ~181–193), note that
`radiusValueRaw = location.properties.radius` is converted with `Number(radiusValueRaw)`
and only finite numeric values are used to compute `minRadiusValue` and `maxRadiusValue`,
so string values like `'10'` correctly influence the global range.
3. In the per-point rendering block (same file, lines ~273–281), observe that `rawRadius`
is the same string value, but `radiusProperty = typeof rawRadius === 'number' ? rawRadius
: null;` treats all string radii as `null`, causing `pointRadius` to fall back to
`defaultRadius` instead of using the actual numeric value.
4. When the chart is rendered, points whose `radius` was provided as a numeric string are
drawn at the default small size, even though they were included in the min/max range
calculation, resulting in incorrect and inconsistent point radius rendering for those data
points.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-map-box/src/ScatterPlotGlowOverlay.tsx
**Line:** 274:275
**Comment:**
*Type Error: Radius parsing now accepts numeric strings for range calculation, but point rendering only accepts `number`-typed values, so string radii are silently dropped and rendered with default size. Parse and validate string values when assigning per-point radius so numeric string inputs are rendered correctly.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
Pre-existing behavior — the map-box plugin was deleted on master (c496415) and this PR restores it to its prior class-component state. This behavior is not introduced by this PR; it matches the pre-deletion master version. Leaving as-is.
Comment on lines
+199
to
+246
| const [scaleFactor] = useState(1); | ||
| const isMountedRef = useRef(true); | ||
|
|
||
| // Store previous props for comparison | ||
| const prevPropsRef = useRef<{ | ||
| data: PlainObject[]; | ||
| encoding: Partial<WordCloudEncoding>; | ||
| width: number; | ||
| height: number; | ||
| rotation: RotationType; | ||
| } | null>(null); | ||
|
|
||
| const createEncoder = useCallback( | ||
| (enc?: Partial<WordCloudEncoding>): SimpleEncoder => | ||
| new SimpleEncoder(enc ?? {}, { | ||
| color: theme.colorTextLabel, | ||
| fontFamily: theme.fontFamily, | ||
| fontSize: 20, | ||
| fontWeight: 'bold', | ||
| text: '', | ||
| }), | ||
| [theme.colorTextLabel, theme.fontFamily], | ||
| ); | ||
|
|
||
| const setWordsIfMounted = useCallback((newWords: Word[]) => { | ||
| if (isMountedRef.current) { | ||
| setWords(newWords); | ||
| } | ||
| } | ||
|
|
||
| update() { | ||
| const { data, encoding } = this.props; | ||
| }, []); | ||
|
|
||
| const generateCloud = useCallback( | ||
| ( | ||
| encoder: SimpleEncoder, | ||
| currentScaleFactor: number, | ||
| isValid: (word: Word[]) => boolean, | ||
| ) => { | ||
| cloudLayout() | ||
| .size([width * currentScaleFactor, height * currentScaleFactor]) | ||
| .words(data.map((d: Word) => ({ ...d }))) | ||
| .padding(5) | ||
| .rotate(ROTATION[rotation] || ROTATION.flat) | ||
| .text((d: PlainObject) => encoder.getText(d)) | ||
| .font((d: PlainObject) => encoder.getFontFamily(d)) | ||
| .fontWeight((d: PlainObject) => encoder.getFontWeight(d)) | ||
| .fontSize((d: PlainObject) => encoder.getFontSize(d)) | ||
| .on('end', (cloudWords: Word[]) => { | ||
| if (isValid(cloudWords) || currentScaleFactor > MAX_SCALE_FACTOR) { | ||
| setWordsIfMounted(cloudWords); |
Contributor
There was a problem hiding this comment.
Suggestion: The layout retries with larger currentScaleFactor, but the component never stores that factor in state, so the SVG viewBox stays at scale 1 and words laid out on larger canvases can be clipped. Persist the final scale factor together with the generated words. [logic error]
Severity Level: Major ⚠️
- ❌ Word Cloud charts can clip words at container edges.
- ⚠️ Storybook WordCloud demo misrepresents high-frequency words placement.
Suggested change
| const [scaleFactor] = useState(1); | |
| const isMountedRef = useRef(true); | |
| // Store previous props for comparison | |
| const prevPropsRef = useRef<{ | |
| data: PlainObject[]; | |
| encoding: Partial<WordCloudEncoding>; | |
| width: number; | |
| height: number; | |
| rotation: RotationType; | |
| } | null>(null); | |
| const createEncoder = useCallback( | |
| (enc?: Partial<WordCloudEncoding>): SimpleEncoder => | |
| new SimpleEncoder(enc ?? {}, { | |
| color: theme.colorTextLabel, | |
| fontFamily: theme.fontFamily, | |
| fontSize: 20, | |
| fontWeight: 'bold', | |
| text: '', | |
| }), | |
| [theme.colorTextLabel, theme.fontFamily], | |
| ); | |
| const setWordsIfMounted = useCallback((newWords: Word[]) => { | |
| if (isMountedRef.current) { | |
| setWords(newWords); | |
| } | |
| } | |
| update() { | |
| const { data, encoding } = this.props; | |
| }, []); | |
| const generateCloud = useCallback( | |
| ( | |
| encoder: SimpleEncoder, | |
| currentScaleFactor: number, | |
| isValid: (word: Word[]) => boolean, | |
| ) => { | |
| cloudLayout() | |
| .size([width * currentScaleFactor, height * currentScaleFactor]) | |
| .words(data.map((d: Word) => ({ ...d }))) | |
| .padding(5) | |
| .rotate(ROTATION[rotation] || ROTATION.flat) | |
| .text((d: PlainObject) => encoder.getText(d)) | |
| .font((d: PlainObject) => encoder.getFontFamily(d)) | |
| .fontWeight((d: PlainObject) => encoder.getFontWeight(d)) | |
| .fontSize((d: PlainObject) => encoder.getFontSize(d)) | |
| .on('end', (cloudWords: Word[]) => { | |
| if (isValid(cloudWords) || currentScaleFactor > MAX_SCALE_FACTOR) { | |
| setWordsIfMounted(cloudWords); | |
| const [scaleFactor, setScaleFactor] = useState(1); | |
| const isMountedRef = useRef(true); | |
| // Store previous props for comparison | |
| const prevPropsRef = useRef<{ | |
| data: PlainObject[]; | |
| encoding: Partial<WordCloudEncoding>; | |
| width: number; | |
| height: number; | |
| rotation: RotationType; | |
| } | null>(null); | |
| const createEncoder = useCallback( | |
| (enc?: Partial<WordCloudEncoding>): SimpleEncoder => | |
| new SimpleEncoder(enc ?? {}, { | |
| color: theme.colorTextLabel, | |
| fontFamily: theme.fontFamily, | |
| fontSize: 20, | |
| fontWeight: 'bold', | |
| text: '', | |
| }), | |
| [theme.colorTextLabel, theme.fontFamily], | |
| ); | |
| const setWordsIfMounted = useCallback( | |
| (newWords: Word[], nextScaleFactor: number) => { | |
| if (isMountedRef.current) { | |
| setWords(newWords); | |
| setScaleFactor(nextScaleFactor); | |
| } | |
| }, | |
| [], | |
| ); | |
| const generateCloud = useCallback( | |
| ( | |
| encoder: SimpleEncoder, | |
| currentScaleFactor: number, | |
| isValid: (word: Word[]) => boolean, | |
| ) => { | |
| cloudLayout() | |
| .size([width * currentScaleFactor, height * currentScaleFactor]) | |
| .words(data.map((d: Word) => ({ ...d }))) | |
| .padding(5) | |
| .rotate(ROTATION[rotation] || ROTATION.flat) | |
| .text((d: PlainObject) => encoder.getText(d)) | |
| .font((d: PlainObject) => encoder.getFontFamily(d)) | |
| .fontWeight((d: PlainObject) => encoder.getFontWeight(d)) | |
| .fontSize((d: PlainObject) => encoder.getFontSize(d)) | |
| .on('end', (cloudWords: Word[]) => { | |
| if (isValid(cloudWords) || currentScaleFactor > MAX_SCALE_FACTOR) { | |
| setWordsIfMounted(cloudWords, currentScaleFactor); |
Steps of Reproduction ✅
1. Open Superset with the frontend bundle that includes `MainPreset` and create or open a
chart with visualization type `Word Cloud` (this viz type is wired to the plugin via
`src/visualizations/presets/MainPreset.ts:73-127`, where `new
WordCloudChartPlugin().configure({ key: VizType.WordCloud })` registers the plugin
exported from `plugins/plugin-chart-word-cloud/src/plugin/index.ts:49-57`).
2. Use a dataset with many distinct category values and a wide font-size range, for
example by running the Storybook story `Basic` in
`plugins/plugin-chart-word-cloud/src/stories/WordCloud.stories.tsx:61-90`, which renders
`<SuperChart chartType="word-cloud2" ...>` backed by `WordCloudChartPlugin` and passes
`queriesData={[{ data }]}` and `size_to` up to 150 via `formData`.
3. When the chart renders, `WordCloud` in
`plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx:188-258` is mounted: `update()`
(lines reported as ~260-278) calls `generateCloud(encoder, 1, ...)`, and `generateCloud`
(lines 229-257) may recurse with larger `currentScaleFactor` while calling
`cloudLayout().size([width * currentScaleFactor, height * currentScaleFactor])` and
`.on('end', ...)` until all `topResults` fit; however, `scaleFactor` is defined as `const
[scaleFactor] = useState(1);` at line 199 and never updated, and `setWordsIfMounted` at
lines 223-227 only calls `setWords(newWords)` without recording the final
`currentScaleFactor`.
4. Because `viewBoxWidth` and `viewBoxHeight` are computed at `WordCloud.tsx:52-55` as
`width * scaleFactor` and `height * scaleFactor` using the stale `scaleFactor === 1`,
while the layout may have used `currentScaleFactor > 1`, some words are positioned by
d3-cloud outside the `[-width/2, width/2] x [-height/2, height/2]` viewBox and are
therefore clipped in the rendered `<svg>`, causing edge words to disappear or be cut off
in both the Storybook demo and real Word Cloud charts.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-word-cloud/src/chart/WordCloud.tsx
**Line:** 199:246
**Comment:**
*Logic Error: The layout retries with larger `currentScaleFactor`, but the component never stores that factor in state, so the SVG viewBox stays at scale `1` and words laid out on larger canvases can be clipped. Persist the final scale factor together with the generated words.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Member
Author
There was a problem hiding this comment.
Pre-existing behavior — the map-box plugin was deleted on master (c496415) and this PR restores it to its prior class-component state. This behavior is not introduced by this PR; it matches the pre-deletion master version. Leaving as-is.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The legacy-plugin-chart-map-box has been removed on master. Keeping the original class component form for these files avoids test failures that would be rebased away. Other files in this PR still convert as intended.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## chore/lint-cleanup-function-components #39452 +/- ##
==========================================================================
+ Coverage 64.45% 64.51% +0.06%
==========================================================================
Files 2541 2541
Lines 131662 131681 +19
Branches 30517 30545 +28
==========================================================================
+ Hits 84863 84957 +94
+ Misses 45333 45258 -75
Partials 1466 1466
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Part of the broader class→function component lint cleanup tracked in #37902.
Test plan
Additional information
This is sub-PR 2 of ~10, each targeting
chore/lint-cleanup-function-components.🤖 Generated with Claude Code