fix(map-box): make opacity, lon, lat, and zoom controls functional (#38374)

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>

superset-frontend/plugins/legacy-plugin-chart-map-box/test/MapBox.test.tsx

@@ -0,0 +1,381 @@
+/*
+ * 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 { type ReactNode } from 'react';
+import { render } from '@testing-library/react';
+import MapBox from '../src/MapBox';
+
+// Capture the most recent viewport props passed to MapGL
+let lastMapGLProps: Record<string, unknown> = {};
+const mockFitBounds = jest.fn();
+
+jest.mock('react-map-gl', () => {
+  const MockMapGL = (props: Record<string, unknown>) => {
+    lastMapGLProps = props;
+    return <div data-test="map-gl">{props.children as ReactNode}</div>;
+  };
+  return { __esModule: true, default: MockMapGL };
+});
+
+jest.mock('@math.gl/web-mercator', () => ({
+  WebMercatorViewport: jest
+    .fn()
+    .mockImplementation(
+      ({ width, height }: { width: number; height: number }) => ({
+        fitBounds: (bounds: [[number, number], [number, number]]) =>
+          mockFitBounds(bounds, width, height),
+      }),
+    ),
+}));
+
+jest.mock('../src/ScatterPlotGlowOverlay', () => {
+  const MockOverlay = (props: Record<string, unknown>) => (
+    <div data-test="scatter-overlay" data-opacity={props.globalOpacity} />
+  );
+  return { __esModule: true, default: MockOverlay };
+});
+
+const defaultProps = {
+  width: 800,
+  height: 600,
+  clusterer: {
+    getClusters: jest.fn().mockReturnValue([]),
+  },
+  globalOpacity: 1,
+  mapboxApiKey: 'test-key',
+  mapStyle: 'mapbox://styles/mapbox/light-v9',
+  pointRadius: 60,
+  pointRadiusUnit: 'Pixels',
+  renderWhileDragging: true,
+  rgb: ['', 255, 0, 0] as (string | number)[],
+  hasCustomMetric: false,
+  bounds: [
+    [-74.0, 40.7],
+    [-73.9, 40.8],
+  ] as [[number, number], [number, number]],
+  onViewportChange: jest.fn(),
+};
+
+beforeEach(() => {
+  lastMapGLProps = {};
+  jest.clearAllMocks();
+  mockFitBounds.mockImplementation(
+    (
+      bounds: [[number, number], [number, number]],
+      width: number,
+      height: number,
+    ) => ({
+      latitude: Number(((bounds[0][1] + bounds[1][1]) / 2).toFixed(2)),
+      longitude: Number(((bounds[0][0] + bounds[1][0]) / 2).toFixed(2)),
+      zoom: Number((10 + width / 1000 + height / 10000).toFixed(2)),
+    }),
+  );
+});
+
+test('initializes viewport from bounds', () => {
+  render(<MapBox {...defaultProps} />);
+  expect(lastMapGLProps.latitude).toBe(40.75);
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.zoom).toBe(10.86);
+});
+
+test('updates viewport when viewport props change', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-73.95}
+      viewportLatitude={40.75}
+      viewportZoom={10}
+    />,
+  );
+
+  rerender(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+
+  expect(lastMapGLProps.longitude).toBe(-122.4);
+  expect(lastMapGLProps.latitude).toBe(37.8);
+  expect(lastMapGLProps.zoom).toBe(5);
+});
+
+test('does not loop when viewport state matches new props', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-73.95}
+      viewportLatitude={40.75}
+      viewportZoom={10}
+    />,
+  );
+
+  // Re-render with same props that match the initial viewport state
+  rerender(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-73.95}
+      viewportLatitude={40.75}
+      viewportZoom={10}
+    />,
+  );
+
+  // Viewport should still be the fitBounds-computed values since props didn't change
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.latitude).toBe(40.75);
+  expect(lastMapGLProps.zoom).toBe(10);
+});
+
+test('passes globalOpacity to ScatterPlotGlowOverlay', () => {
+  const { getByTestId } = render(
+    <MapBox {...defaultProps} globalOpacity={0.5} />,
+  );
+  const overlay = getByTestId('scatter-overlay');
+  expect(overlay.dataset.opacity).toBe('0.5');
+});
+
+test('initializes viewport from props when provided', () => {
+  render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+  expect(lastMapGLProps.longitude).toBe(-122.4);
+  expect(lastMapGLProps.latitude).toBe(37.8);
+  expect(lastMapGLProps.zoom).toBe(5);
+});
+
+test('handles undefined bounds gracefully', () => {
+  render(<MapBox {...defaultProps} bounds={undefined} />);
+  expect(lastMapGLProps.longitude).toBe(0);
+  expect(lastMapGLProps.latitude).toBe(0);
+  expect(lastMapGLProps.zoom).toBe(1);
+});
+
+test('applies partial viewport props on update', () => {
+  const { rerender } = render(<MapBox {...defaultProps} />);
+
+  rerender(<MapBox {...defaultProps} viewportLongitude={-122.4} />);
+
+  expect(lastMapGLProps.longitude).toBe(-122.4);
+  expect(lastMapGLProps.latitude).toBe(40.75);
+  expect(lastMapGLProps.zoom).toBe(10.86);
+});
+
+test('restores fitBounds when viewport props are cleared', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+
+  // Clear all viewport props (simulates user clearing the controls)
+  rerender(<MapBox {...defaultProps} />);
+
+  // Should revert to fitBounds values
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.latitude).toBe(40.75);
+  expect(lastMapGLProps.zoom).toBe(10.86);
+});
+
+test('restores only cleared viewport props, keeps the rest', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+
+  // Clear only longitude, keep lat/zoom
+  rerender(
+    <MapBox {...defaultProps} viewportLatitude={37.8} viewportZoom={5} />,
+  );
+
+  // Longitude reverts to fitBounds, lat/zoom stay
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.latitude).toBe(37.8);
+  expect(lastMapGLProps.zoom).toBe(5);
+});
+
+test('applies changed viewport props even when another is cleared simultaneously', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+
+  // Clear longitude, change latitude simultaneously
+  rerender(
+    <MapBox {...defaultProps} viewportLatitude={40.0} viewportZoom={5} />,
+  );
+
+  // Longitude reverts to fitBounds, latitude should be the NEW value
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.latitude).toBe(40.0);
+  expect(lastMapGLProps.zoom).toBe(5);
+});
+
+test('falls back to default viewport when cleared with undefined bounds', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      bounds={undefined}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+
+  // Clear viewport props — no bounds to fitBounds to
+  rerender(<MapBox {...defaultProps} bounds={undefined} />);
+
+  // Should fall back to {0, 0, 1}
+  expect(lastMapGLProps.longitude).toBe(0);
+  expect(lastMapGLProps.latitude).toBe(0);
+  expect(lastMapGLProps.zoom).toBe(1);
+});
+
+test('recomputes fitBounds when bounds change and no explicit viewport is set', () => {
+  const { rerender } = render(<MapBox {...defaultProps} />);
+
+  rerender(
+    <MapBox
+      {...defaultProps}
+      bounds={[
+        [-123.2, 36.5],
+        [-121.8, 38.1],
+      ]}
+    />,
+  );
+
+  expect(lastMapGLProps.longitude).toBe(-122.5);
+  expect(lastMapGLProps.latitude).toBe(37.3);
+  expect(lastMapGLProps.zoom).toBe(10.86);
+});
+
+test('recomputes fitBounds when chart size changes and no explicit viewport is set', () => {
+  const { rerender } = render(<MapBox {...defaultProps} />);
+
+  rerender(<MapBox {...defaultProps} width={1200} height={900} />);
+
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.latitude).toBe(40.75);
+  expect(lastMapGLProps.zoom).toBe(11.29);
+});
+
+test('recomputes only implicit viewport fields when bounds change', () => {
+  const { rerender } = render(
+    <MapBox {...defaultProps} viewportLongitude={-122.4} />,
+  );
+
+  rerender(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      bounds={[
+        [-123.2, 36.5],
+        [-121.8, 38.1],
+      ]}
+    />,
+  );
+
+  expect(lastMapGLProps.longitude).toBe(-122.4);
+  expect(lastMapGLProps.latitude).toBe(37.3);
+  expect(lastMapGLProps.zoom).toBe(10.86);
+});
+
+test('recomputes only implicit viewport fields when chart size changes', () => {
+  const { rerender } = render(
+    <MapBox {...defaultProps} viewportLatitude={37.8} />,
+  );
+
+  rerender(
+    <MapBox
+      {...defaultProps}
+      viewportLatitude={37.8}
+      width={1200}
+      height={900}
+    />,
+  );
+
+  expect(lastMapGLProps.longitude).toBe(-73.95);
+  expect(lastMapGLProps.latitude).toBe(37.8);
+  expect(lastMapGLProps.zoom).toBe(11.29);
+});
+
+test('recomputes implicit position when zoom stays explicit across bounds changes', () => {
+  const { rerender } = render(<MapBox {...defaultProps} viewportZoom={5} />);
+
+  rerender(
+    <MapBox
+      {...defaultProps}
+      viewportZoom={5}
+      bounds={[
+        [-123.2, 36.5],
+        [-121.8, 38.1],
+      ]}
+    />,
+  );
+
+  expect(lastMapGLProps.longitude).toBe(-122.5);
+  expect(lastMapGLProps.latitude).toBe(37.3);
+  expect(lastMapGLProps.zoom).toBe(5);
+});
+
+test('does not recompute fitBounds on bounds change when an explicit viewport is set', () => {
+  const { rerender } = render(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+    />,
+  );
+
+  rerender(
+    <MapBox
+      {...defaultProps}
+      viewportLongitude={-122.4}
+      viewportLatitude={37.8}
+      viewportZoom={5}
+      bounds={[
+        [-123.2, 36.5],
+        [-121.8, 38.1],
+      ]}
+    />,
+  );
+
+  expect(lastMapGLProps.longitude).toBe(-122.4);
+  expect(lastMapGLProps.latitude).toBe(37.8);
+  expect(lastMapGLProps.zoom).toBe(5);
+});

superset-frontend/plugins/legacy-plugin-chart-map-box/test/controlPanel.test.ts

@@ -0,0 +1,81 @@
+/**
+ * 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 type {
+  ControlPanelConfig,
+  CustomControlItem,
+} from '@superset-ui/chart-controls';
+import controlPanel from '../src/controlPanel';
+
+type ControlConfig = Required<CustomControlItem['config']>;
+
+function isCustomControlItem(
+  controlItem: unknown,
+): controlItem is CustomControlItem & { config: ControlConfig } {
+  return (
+    typeof controlItem === 'object' &&
+    controlItem !== null &&
+    'name' in controlItem &&
+    'config' in controlItem
+  );
+}
+
+function getControl(
+  panel: ControlPanelConfig,
+  controlName: string,
+): CustomControlItem & { config: ControlConfig } {
+  const item = (panel.controlPanelSections || [])
+    .flatMap(section => section?.controlSetRows || [])
+    .flat()
+    .find(
+      controlItem =>
+        isCustomControlItem(controlItem) && controlItem.name === controlName,
+    );
+
+  if (!isCustomControlItem(item)) {
+    throw new Error(`Control "${controlName}" not found`);
+  }
+
+  return item;
+}
+
+test('viewport controls default to empty values and rerender without query refresh', () => {
+  const longitudeControl = getControl(controlPanel, 'viewport_longitude');
+  const latitudeControl = getControl(controlPanel, 'viewport_latitude');
+  const zoomControl = getControl(controlPanel, 'viewport_zoom');
+
+  expect(longitudeControl.config.default).toBe('');
+  expect(latitudeControl.config.default).toBe('');
+  expect(zoomControl.config.default).toBe('');
+
+  expect(longitudeControl.config.renderTrigger).toBe(true);
+  expect(latitudeControl.config.renderTrigger).toBe(true);
+  expect(zoomControl.config.renderTrigger).toBe(true);
+
+  expect(longitudeControl.config.dontRefreshOnChange).toBe(true);
+  expect(latitudeControl.config.dontRefreshOnChange).toBe(true);
+  expect(zoomControl.config.dontRefreshOnChange).toBe(true);
+});
+
+test('opacity control rerenders immediately when changed', () => {
+  const opacityControl = getControl(controlPanel, 'global_opacity');
+
+  expect(opacityControl.config.default).toBe(1);
+  expect(opacityControl.config.renderTrigger).toBe(true);
+  expect(opacityControl.config.isFloat).toBe(true);
+});

superset-frontend/plugins/legacy-plugin-chart-map-box/test/transformProps.test.ts

@@ -0,0 +1,230 @@
+/*
+ * 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 { supersetTheme } from '@apache-superset/core/theme';
+
+jest.mock('supercluster', () => {
+  const MockSupercluster = jest.fn().mockImplementation(() => ({
+    load: jest.fn(),
+    getClusters: jest.fn().mockReturnValue([]),
+  }));
+  return { __esModule: true, default: MockSupercluster };
+});
+
+// Import after mocking supercluster to avoid ESM parse error
+// eslint-disable-next-line import/first
+import transformProps from '../src/transformProps';
+
+type TransformPropsResult = {
+  globalOpacity?: number;
+  onViewportChange?: (viewport: {
+    latitude: number;
+    longitude: number;
+    zoom: number;
+  }) => void;
+  viewportLongitude?: number;
+  viewportLatitude?: number;
+  viewportZoom?: number;
+};
+
+const baseFormData = {
+  clusteringRadius: 60,
+  globalOpacity: 0.8,
+  mapboxColor: 'rgb(0, 139, 139)',
+  mapboxStyle: 'mapbox://styles/mapbox/light-v9',
+  pandasAggfunc: 'sum',
+  pointRadiusUnit: 'Pixels',
+  renderWhileDragging: true,
+  viewportLongitude: -73.935242,
+  viewportLatitude: 40.73061,
+  viewportZoom: 9,
+};
+
+const baseQueriesData = [
+  {
+    data: {
+      bounds: [
+        [-74.0, 40.7],
+        [-73.9, 40.8],
+      ] as [[number, number], [number, number]],
+      geoJSON: { features: [] },
+      hasCustomMetric: false,
+      mapboxApiKey: 'test-api-key',
+    },
+  },
+];
+
+function createChartProps(overrides: Record<string, unknown> = {}) {
+  return new ChartProps({
+    formData: { ...baseFormData, ...overrides },
+    width: 800,
+    height: 600,
+    queriesData: baseQueriesData,
+    theme: supersetTheme,
+  });
+}
+
+function getTransformPropsResult(
+  overrides: Record<string, unknown> = {},
+): TransformPropsResult {
+  return transformProps(createChartProps(overrides)) as TransformPropsResult;
+}
+
+test('extracts globalOpacity from formData', () => {
+  const result = getTransformPropsResult({ globalOpacity: 0.5 });
+  expect(result.globalOpacity).toBe(0.5);
+});
+
+test('extracts viewport values from formData', () => {
+  const result = getTransformPropsResult({
+    viewportLongitude: -122.4,
+    viewportLatitude: 37.8,
+    viewportZoom: 12,
+  });
+  expect(result).toEqual(
+    expect.objectContaining({
+      viewportLongitude: -122.4,
+      viewportLatitude: 37.8,
+      viewportZoom: 12,
+    }),
+  );
+});
+
+test('clamps viewport values to safe map ranges', () => {
+  const result = getTransformPropsResult({
+    viewportLongitude: 190,
+    viewportLatitude: -100,
+    viewportZoom: 99,
+  });
+  expect(result).toEqual(
+    expect.objectContaining({
+      viewportLongitude: 180,
+      viewportLatitude: -90,
+      viewportZoom: 16,
+    }),
+  );
+});
+
+test('provides onViewportChange callback that updates control values', () => {
+  const setControlValue = jest.fn();
+  const chartProps = new ChartProps({
+    formData: baseFormData,
+    width: 800,
+    height: 600,
+    queriesData: baseQueriesData,
+    hooks: { setControlValue },
+    theme: supersetTheme,
+  });
+  const result = transformProps(chartProps) as TransformPropsResult;
+  expect(result.onViewportChange).toBeDefined();
+
+  result.onViewportChange!({
+    latitude: 51.5,
+    longitude: -0.12,
+    zoom: 10,
+  });
+
+  expect(setControlValue).toHaveBeenCalledWith('viewport_longitude', -0.12);
+  expect(setControlValue).toHaveBeenCalledWith('viewport_latitude', 51.5);
+  expect(setControlValue).toHaveBeenCalledWith('viewport_zoom', 10);
+});
+
+test('normalizes string viewport values to numbers', () => {
+  const result = getTransformPropsResult({
+    viewportLongitude: '-122.4',
+    viewportLatitude: '37.8',
+    viewportZoom: '12',
+  });
+  expect(result.viewportLongitude).toBe(-122.4);
+  expect(result.viewportLatitude).toBe(37.8);
+  expect(result.viewportZoom).toBe(12);
+});
+
+test('normalizes empty viewport values to undefined', () => {
+  const result = getTransformPropsResult({
+    viewportLongitude: '',
+    viewportLatitude: '',
+    viewportZoom: '',
+  });
+  expect(result.viewportLongitude).toBeUndefined();
+  expect(result.viewportLatitude).toBeUndefined();
+  expect(result.viewportZoom).toBeUndefined();
+});
+
+test('normalizes whitespace-only viewport values to undefined', () => {
+  const result = getTransformPropsResult({
+    viewportLongitude: '   ',
+    viewportLatitude: '\t',
+    viewportZoom: ' \n ',
+  });
+  expect(result.viewportLongitude).toBeUndefined();
+  expect(result.viewportLatitude).toBeUndefined();
+  expect(result.viewportZoom).toBeUndefined();
+});
+
+test('normalizes string opacity to number', () => {
+  const result = getTransformPropsResult({ globalOpacity: '0.5' });
+  expect(result.globalOpacity).toBe(0.5);
+});
+
+test('defaults empty opacity to 1', () => {
+  const result = getTransformPropsResult({ globalOpacity: '' });
+  expect(result.globalOpacity).toBe(1);
+});
+
+test('defaults whitespace-only opacity to 1', () => {
+  const result = getTransformPropsResult({ globalOpacity: '   ' });
+  expect(result.globalOpacity).toBe(1);
+});
+
+test('clamps opacity to [0, 1] range', () => {
+  expect(getTransformPropsResult({ globalOpacity: 5 }).globalOpacity).toBe(1);
+  expect(getTransformPropsResult({ globalOpacity: -1 }).globalOpacity).toBe(0);
+});
+
+test('passes through numeric values unchanged', () => {
+  const result = getTransformPropsResult({
+    viewportLongitude: -122.4,
+    viewportLatitude: 37.8,
+    viewportZoom: 12,
+    globalOpacity: 0.8,
+  });
+  expect(result.viewportLongitude).toBe(-122.4);
+  expect(result.viewportLatitude).toBe(37.8);
+  expect(result.viewportZoom).toBe(12);
+  expect(result.globalOpacity).toBe(0.8);
+});
+
+test('calls onError and returns empty object for invalid color', () => {
+  const onError = jest.fn();
+  const chartProps = new ChartProps({
+    formData: { ...baseFormData, mapboxColor: 'invalid-color' },
+    width: 800,
+    height: 600,
+    queriesData: baseQueriesData,
+    hooks: { onError },
+    theme: supersetTheme,
+  });
+  const result = transformProps(chartProps);
+  expect(onError).toHaveBeenCalledWith(
+    "Color field must be of form 'rgb(%d, %d, %d)'",
+  );
+  expect(result).toEqual({});
+});