codeant-ai-for-open-source[bot] commented on code in PR #38374:
URL: https://github.com/apache/superset/pull/38374#discussion_r2931065710
##########
superset-frontend/plugins/legacy-plugin-chart-map-box/src/MapBox.tsx:
##########
@@ -116,6 +105,104 @@ class MapBox extends Component<MapBoxProps, MapBoxState> {
onViewportChange!(viewport);
}
+ hasExplicitViewportProps(props: MapBoxProps = this.props) {
+ return (
+ props.viewportLongitude !== undefined ||
+ props.viewportLatitude !== undefined ||
+ props.viewportZoom !== undefined
+ );
+ }
+
+ 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;
+ const hasExplicitViewportProps = this.hasExplicitViewportProps();
+ const fitBoundsInputsChanged =
+ prevProps.width !== this.props.width ||
+ prevProps.height !== this.props.height ||
+ prevProps.bounds !== this.props.bounds;
+
+ // 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
+ : (viewportLongitude ?? viewport.longitude),
+ latitude: latitudeCleared
+ ? fitBounds.latitude
+ : (viewportLatitude ?? viewport.latitude),
+ zoom: zoomCleared ? fitBounds.zoom : (viewportZoom ?? viewport.zoom),
+ },
+ });
+ return;
+ }
+
+ if (fitBoundsInputsChanged && !hasExplicitViewportProps) {
+ const fitBounds = this.computeFitBoundsViewport();
+ const fitBoundsChanged =
+ fitBounds.longitude !== viewport.longitude ||
+ fitBounds.latitude !== viewport.latitude ||
+ fitBounds.zoom !== viewport.zoom;
+
+ if (fitBoundsChanged) {
+ this.setState({ viewport: fitBounds });
+ }
+ return;
+ }
Review Comment:
**Suggestion:** The fit-bounds refresh is currently skipped whenever any one
viewport prop is set, which causes stale center/zoom values for the
non-explicit fields when `bounds` or chart size changes. This breaks the
per-field override behavior used in initialization (`?? fitBounds`) and leads
to incorrect map positioning after data/size updates. Recompute fit-bounds on
input changes and then override only the explicitly provided viewport fields.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Filtered data updates keep stale latitude and zoom.
- ⚠️ Chart resize can preserve outdated fitted viewport values.
- ❌ Partial viewport defaults behave inconsistently across updates.
```
</details>
```suggestion
const fitBoundsInputsChanged =
prevProps.width !== this.props.width ||
prevProps.height !== this.props.height ||
prevProps.bounds !== this.props.bounds;
if (fitBoundsInputsChanged) {
const fitBounds = this.computeFitBoundsViewport();
const nextViewport = {
longitude: viewportLongitude ?? fitBounds.longitude,
latitude: viewportLatitude ?? fitBounds.latitude,
zoom: viewportZoom ?? fitBounds.zoom,
};
const fitBoundsChanged =
nextViewport.longitude !== viewport.longitude ||
nextViewport.latitude !== viewport.latitude ||
nextViewport.zoom !== viewport.zoom;
if (fitBoundsChanged) {
this.setState({ viewport: nextViewport });
}
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the MapBox visualization registered in
`superset-frontend/src/visualizations/presets/MainPreset.ts:134`
(`MapBoxChartPlugin`),
which loads `MapBox.tsx` and `transformProps.ts` via
`superset-frontend/plugins/legacy-plugin-chart-map-box/src/index.ts:54-55`.
2. Configure only one viewport control (for example longitude) while leaving
latitude/zoom
blank; `transformProps` passes per-field values (`viewportLongitude` set,
`viewportLatitude`/`viewportZoom` undefined) at
`.../transformProps.ts:56-58` and
`129-131`, and controls are independent fields in
`.../controlPanel.ts:272-313`.
3. Trigger new `bounds` or size inputs (e.g., filter/query update changes
`queriesData[0].data.bounds` read at `.../transformProps.ts:46-47`, or chart
resize
changes `width/height` passed at `102-103`).
4. In `MapBox.componentDidUpdate` (`.../MapBox.tsx:131-179`),
`fitBoundsInputsChanged`
becomes true, but because one explicit viewport prop exists
(`hasExplicitViewportProps`
true at `134`), the fit-bounds refresh block is skipped (`168` condition
fails).
Non-explicit fields (latitude/zoom) are not recomputed, leaving stale state
values
inconsistent with new bounds/size.
```
</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:** 134:179
**Comment:**
*Logic Error: The fit-bounds refresh is currently skipped whenever any
one viewport prop is set, which causes stale center/zoom values for the
non-explicit fields when `bounds` or chart size changes. This breaks the
per-field override behavior used in initialization (`?? fitBounds`) and leads
to incorrect map positioning after data/size updates. Recompute fit-bounds on
input changes and then override only the explicitly provided viewport fields.
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=654a2456f827d842b494e63a49580d1d8ae2ab53a1651ea7828ce0a9933e3440&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38374&comment_hash=654a2456f827d842b494e63a49580d1d8ae2ab53a1651ea7828ce0a9933e3440&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]
