codeant-ai-for-open-source[bot] commented on code in PR #38374:
URL: https://github.com/apache/superset/pull/38374#discussion_r2921601113
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -116,6 +105,76 @@ class MapBox extends Component<MapBoxProps, MapBoxState> {
onViewportChange!(viewport);
}
+ computeFitBoundsViewport(): Viewport {
+ const { width = 400, height = 400, bounds } = this.props;
+ 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 { viewportLongitude, viewportLatitude, viewportZoom } = this.props;
+ const { viewport } = this.state;
+
+ // Detect when viewport props are cleared (changed from defined to
undefined)
+ // to restore fitBounds behavior
+ const longitudeCleared =
+ prevProps.viewportLongitude !== undefined &&
+ viewportLongitude === undefined;
+ const latitudeCleared =
+ prevProps.viewportLatitude !== undefined &&
+ viewportLatitude === undefined;
+ const zoomCleared =
+ prevProps.viewportZoom !== undefined && viewportZoom === undefined;
+
+ if (longitudeCleared || latitudeCleared || zoomCleared) {
+ const fitBounds = this.computeFitBoundsViewport();
+ this.setState({
+ viewport: {
+ ...viewport,
+ longitude: longitudeCleared
+ ? fitBounds.longitude
+ : viewport.longitude,
+ latitude: latitudeCleared ? fitBounds.latitude : viewport.latitude,
+ zoom: zoomCleared ? fitBounds.zoom : viewport.zoom,
+ },
+ });
+ return;
+ }
Review Comment:
**Suggestion:** When one viewport prop is cleared and another is changed in
the same render, the clear branch returns early and keeps the old state values
for the changed props. Because the next update sees unchanged props, the new
values are never applied. Update the clear branch to also apply any defined
incoming viewport props. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MapBox viewport controls can apply stale latitude/zoom.
- ⚠️ Clearing one field can suppress simultaneous updates.
- ⚠️ Users need extra edits to reach desired viewport.
```
</details>
```suggestion
if (longitudeCleared || latitudeCleared || zoomCleared) {
const fitBounds = this.computeFitBoundsViewport();
this.setState({
viewport: {
...viewport,
longitude: longitudeCleared
? fitBounds.longitude
: viewportLongitude ?? viewport.longitude,
latitude: latitudeCleared
? fitBounds.latitude
: viewportLatitude ?? viewport.latitude,
zoom: zoomCleared ? fitBounds.zoom : viewportZoom ??
viewport.zoom,
},
});
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use MapBox chart controls that are render-triggered (`viewport_longitude`,
`viewport_latitude`, `viewport_zoom` in
`superset-frontend/plugins/legacy-plugin-chart-map-box/src/controlPanel.ts:272-304`),
so
control changes re-render chart without query.
2. Control input updates flow through `TextControl` debounce
(`src/explore/components/controls/TextControl/index.tsx:90-98`) to `Control`
dispatcher
(`src/explore/components/Control.tsx:74-76`), then reducer updates
`form_data` and sets
`triggerRender` for render-trigger controls
(`src/explore/reducers/exploreReducer.ts:333-205`).
3. Explore chart passes updated `formData` into chart render path
(`src/explore/components/ExploreChartPanel/index.tsx:65-82`),
`transformProps` maps
`viewportLongitude/Latitude/Zoom` props
(`plugins/legacy-plugin-chart-map-box/src/transformProps.ts:56-59,129-131`),
and
`MapBox.componentDidUpdate` runs (`.../MapBox.tsx:123+`).
4. Reproduce with one prop update payload where one field is cleared and
another changes
together (same rerender shape as `MapBox` rerenders in
`plugins/legacy-plugin-chart-map-box/test/MapBox.test.tsx`): old props
`{lon:-122.4,
lat:37.8, zoom:5}` -> new props `{lon:undefined, lat:40.0, zoom:5}`. In
current code
branch (`MapBox.tsx:138-151`), latitude is taken from old state
(`viewport.latitude`),
then function returns early; next update compares equal props and never
applies new
latitude (`MapBox.tsx:153-164`).
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx
**Line:** 138:151
**Comment:**
*Logic Error: When one viewport prop is cleared and another is changed
in the same render, the clear branch returns early and keeps the old state
values for the changed props. Because the next update sees unchanged props, the
new values are never applied. Update the clear branch to also apply any defined
incoming viewport props.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38374&comment_hash=14d541f369a770fda340f26b71094f97bdf57e1b4a26e518b62dc40547e0046d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38374&comment_hash=14d541f369a770fda340f26b71094f97bdf57e1b4a26e518b62dc40547e0046d&reaction=dislike'>👎</a>
--
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]
