Copilot commented on code in PR #38374:
URL: https://github.com/apache/superset/pull/38374#discussion_r2885614945
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -116,6 +119,34 @@ class MapBox extends Component<MapBoxProps, MapBoxState> {
onViewportChange!(viewport);
}
+ componentDidUpdate(prevProps: MapBoxProps) {
+ const { viewportLongitude, viewportLatitude, viewportZoom } = this.props;
+ const { viewport } = this.state;
+
+ const longitudeChanged =
+ prevProps.viewportLongitude !== viewportLongitude &&
+ viewportLongitude !== undefined &&
+ viewportLongitude !== viewport.longitude;
+ const latitudeChanged =
+ prevProps.viewportLatitude !== viewportLatitude &&
+ viewportLatitude !== undefined &&
+ viewportLatitude !== viewport.latitude;
+ const zoomChanged =
+ prevProps.viewportZoom !== viewportZoom &&
+ viewportZoom !== undefined &&
+ viewportZoom !== viewport.zoom;
+
+ if (longitudeChanged || latitudeChanged || zoomChanged) {
+ this.setState({
+ viewport: {
+ longitude: longitudeChanged ? viewportLongitude : viewport.longitude,
+ latitude: latitudeChanged ? viewportLatitude : viewport.latitude,
+ zoom: zoomChanged ? viewportZoom : viewport.zoom,
Review Comment:
The new viewport state update builds a fresh `{ longitude, latitude, zoom }`
object from optional props. Even with the guards, the ternaries still reference
`viewportLongitude`/`viewportLatitude`/`viewportZoom` which are typed as
`number | undefined`, and this also drops any existing viewport fields (e.g.
`isDragging`) from state. To keep this type-safe and preserve other viewport
state, update `viewport` by starting from the existing state (`...viewport`)
and overwrite fields using a value that is always a `number` (e.g. via nullish
coalescing or non-null assertions in the changed branches).
```suggestion
...viewport,
longitude: longitudeChanged ? viewportLongitude! :
viewport.longitude,
latitude: latitudeChanged ? viewportLatitude! : viewport.latitude,
zoom: zoomChanged ? viewportZoom! : viewport.zoom,
```
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/test/MapBox.test.tsx:
##########
@@ -0,0 +1,140 @@
+/*
+ * 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 { render } from '@testing-library/react';
+import MapBox from '../src/MapBox';
+
+// Capture the most recent viewport props passed to MapGL
+let lastMapGLProps: Record<string, unknown> = {};
+
+jest.mock('react-map-gl', () => {
+ const MockMapGL = (props: Record<string, unknown>) => {
+ lastMapGLProps = props;
+ return <div data-testid="map-gl">{props.children as React.ReactNode}</div>;
Review Comment:
The test mock uses `props.children as React.ReactNode`, but `React` isn’t
imported in this file. Use a direct `children` render without the cast, or
import the type (e.g. `import type { ReactNode } from 'react'`) and cast to
that, to avoid relying on an undeclared `React` namespace.
```suggestion
return <div data-testid="map-gl">{props.children}</div>;
```
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -116,6 +119,34 @@ class MapBox extends Component<MapBoxProps, MapBoxState> {
onViewportChange!(viewport);
}
+ componentDidUpdate(prevProps: MapBoxProps) {
+ const { viewportLongitude, viewportLatitude, viewportZoom } = this.props;
+ const { viewport } = this.state;
+
+ const longitudeChanged =
+ prevProps.viewportLongitude !== viewportLongitude &&
+ viewportLongitude !== undefined &&
+ viewportLongitude !== viewport.longitude;
+ const latitudeChanged =
+ prevProps.viewportLatitude !== viewportLatitude &&
+ viewportLatitude !== undefined &&
+ viewportLatitude !== viewport.latitude;
+ const zoomChanged =
+ prevProps.viewportZoom !== viewportZoom &&
+ viewportZoom !== undefined &&
+ viewportZoom !== viewport.zoom;
+
+ if (longitudeChanged || latitudeChanged || zoomChanged) {
+ this.setState({
+ viewport: {
+ longitude: longitudeChanged ? viewportLongitude : viewport.longitude,
+ latitude: latitudeChanged ? viewportLatitude : viewport.latitude,
+ zoom: zoomChanged ? viewportZoom : viewport.zoom,
+ },
+ });
Review Comment:
`componentDidUpdate` updates the internal viewport when the viewport props
change, but the component still ignores
`viewportLongitude`/`viewportLatitude`/`viewportZoom` on the initial mount
(constructor always initializes from `fitBounds`/defaults). This means saved
charts with default viewport controls set won’t start at the requested viewport
until the user changes a control. Consider initializing `this.state.viewport`
from these props when they are provided (or deriving state from props) so the
defaults take effect on first render as well.
--
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]
