codeant-ai-for-open-source[bot] commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r3613201734
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx:
##########
@@ -258,16 +424,39 @@ export const OlChartMap = (props: OlChartMapProps) => {
if (!chartLayer) {
return;
}
- const extent = chartLayer.getExtent();
+
+ const features = new GeoJSON().readFeatures(chartLayer.chartConfigs, {
+ featureProjection: 'EPSG:4326',
+ });
+
+ const extent = getExtentFromFeatures(features);
+
if (!extent) {
return;
}
+
+ const transformedExtent = transformExtent(
+ extent,
+ 'EPSG:4326',
+ olMap.getView().getProjection(),
+ );
+
const view = olMap.getView();
- view.fit(extent, {
- size: [250, 250],
- });
+ const padding = getMapExtentPadding(mapExtentPadding);
+
+ view.fit(transformedExtent, { padding });
}
- }, [olMap, currentMapView.mode]);
+ }, [olMap, currentMapView.mode, mapExtentPadding]);
Review Comment:
**Suggestion:** The FIT_DATA extent effect does not depend on chart data, so
when features change but mode stays FIT_DATA, the map will not refit and will
show an outdated extent. Include the chart data dependency (or a stable
version/hash of it) so the fit runs on data updates. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ⚠️ New data may render outside current viewport.
- ⚠️ Users must manually adjust map after data refresh.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create or open a cartodiagram chart where `map_view` defaults to FIT_DATA
in the
control panel (`controlPanel.ts:126-138`), so `currentMapView.mode` is
initially
`'FIT_DATA'`.
2. The FIT_DATA effect in `OlChartMap.tsx:416-449` runs once after mount; it
locates the
`ChartLayer` on the map, parses its `chartConfigs` into features, computes
an extent, and
calls `view.fit(transformedExtent, { padding })`.
3. Change filters or time range in Explore so that a new query is issued and
new
`chartConfigs` are passed via `transformProps` (in
`superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts`)
to
`OlChartMap`, while `map_view.mode` remains `'FIT_DATA'`.
4. Because the FIT_DATA effect depends only on `[olMap, currentMapView.mode,
mapExtentPadding]` (line 449) and not on any chart data or `chartConfigs`,
it does not
re-run on data changes; the map remains fitted to the old extent, and new
data points may
appear off-screen until the user manually pans or zooms.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=af75daa444ec4805b8fe57c5d039a2d7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=af75daa444ec4805b8fe57c5d039a2d7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
**Line:** 449:449
**Comment:**
*Logic Error: The FIT_DATA extent effect does not depend on chart data,
so when features change but mode stays FIT_DATA, the map will not refit and
will show an outdated extent. Include the chart data dependency (or a stable
version/hash of it) so the fit runs on data updates.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=1499bf15d5bd4f94e7f8bdfc90dcfb55abe39919bebba935445e380e3bea33a4&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=1499bf15d5bd4f94e7f8bdfc90dcfb55abe39919bebba935445e380e3bea33a4&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx:
##########
@@ -159,6 +211,64 @@ export const OlChartMap = (props: OlChartMapProps) => {
break;
}
}
+
+ switch (extentMode) {
+ case 'CUSTOM': {
+ if (
+ fixedMaxX === undefined ||
+ fixedMaxY === undefined ||
+ fixedMinX === undefined ||
+ fixedMinY === undefined
+ ) {
+ break;
+ }
+ const [minx, miny, maxx, maxy] = transformExtent(
+ [fixedMinX, fixedMinY, fixedMaxX, fixedMaxY],
+ 'EPSG:4326',
+ 'EPSG:3857',
+ );
+
+ olMap.setView(
+ new View({
+ center: view.getCenter(),
+ zoom: view.getZoom(),
+ extent: [minx, miny, maxx, maxy],
+ maxZoom: view.getMaxZoom(),
+ minZoom: view.getMinZoom(),
+ }),
+ );
+ break;
+ }
+ default: {
+ const newView = new View({
+ center: view.getCenter(),
+ zoom: view.getZoom(),
+ maxZoom: view.getMaxZoom(),
+ minZoom: view.getMinZoom(),
+ });
+ olMap.setView(newView);
+
+ const [minx, miny, maxx, maxy] = newView.calculateExtent(
+ olMap.getSize(),
Review Comment:
**Suggestion:** `calculateExtent` is called with `olMap.getSize()` without
checking for `undefined`; OpenLayers can return undefined before layout/target
initialization, which can throw at runtime. Guard size availability before
calling `calculateExtent`. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Cartodiagram visualization can crash on initial render.
- ⚠️ Charts in hidden tabs risk initialization failure.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a cartodiagram chart with the default `map_max_extent` control
value
(`extentMode: 'NONE'`) from `controlPanel.ts:199-208`, then open the chart
in Explore.
2. On mount, `OlChartMap` first attaches the map to its DOM target via
`olMap.setTarget(mapId)` in the effect at `OlChartMap.tsx:80-82`.
3. The "Set initial map extent" effect at `OlChartMap.tsx:171-272` runs with
an empty
dependency array; for `extentMode === 'NONE'` it creates `newView` and then
calls
`newView.calculateExtent(olMap.getSize())` at lines 251-252.
4. If `olMap.getSize()` is still `undefined` at that moment (OpenLayers
returns undefined
before the container has a measured size, e.g., when the chart is rendered
in a hidden tab
or during early layout), `calculateExtent(undefined)` will dereference the
size array
internally and throw a TypeError, crashing the cartodiagram visualization
during
initialization.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=316cdf79eed94475bc0f291f056ee0c6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=316cdf79eed94475bc0f291f056ee0c6&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
**Line:** 251:252
**Comment:**
*Possible Bug: `calculateExtent` is called with `olMap.getSize()`
without checking for `undefined`; OpenLayers can return undefined before
layout/target initialization, which can throw at runtime. Guard size
availability before calling `calculateExtent`.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=554eb318eb74d85b0d45590edd025879c8482fd4f0b7211913dc56645f47b6da&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=554eb318eb74d85b0d45590edd025879c8482fd4f0b7211913dc56645f47b6da&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts:
##########
@@ -101,16 +128,87 @@ const config: ControlPanelConfig = {
type: 'MapViewControl',
renderTrigger: true,
description: t(
- 'The extent of the map on application start. FIT DATA
automatically sets the extent so that all data points are included in the
viewport. CUSTOM allows users to define the extent manually.',
+ 'The map center on application start. FIT DATA automatically
sets the center so that all data points are included in the viewport. CUSTOM
allows users to define the center manually.',
),
- label: t('Extent'),
+ label: t('Initial Map Center'),
dontRefreshOnChange: true,
default: {
mode: 'FIT_DATA',
},
},
},
],
+ [
+ {
+ name: 'map_extent_padding',
+ config: {
+ type: 'SliderControl',
+ renderTrigger: true,
+ label: t('Map Padding'),
+ description: t(
+ 'Set the map extent padding. The selected value is applied to
all edges of the map.',
+ ),
+ default: 30,
+ min: 0,
+ max: 100,
+ step: 10,
+ visibility: ({
+ controls,
+ }: {
+ controls: ControlPanelsContainerProps['controls'] & {
+ map_view?: { value?: Partial<MapViewConfigs> };
+ };
+ }) => Boolean(controls?.map_view?.value?.mode === 'FIT_DATA'),
+ },
+ },
+ ],
+ [
+ {
+ name: 'min_zoom',
+ config: {
+ type: 'SliderControl',
+ renderTrigger: true,
+ label: t('Min Zoom'),
+ description: t('The minimal zoom of the map'),
+ default: MIN_ZOOM_LEVEL,
+ min: MIN_ZOOM_LEVEL,
+ max: MAX_ZOOM_LEVEL,
+ step: 1,
+ },
+ },
+ ],
+ [
+ {
+ name: 'max_zoom',
+ config: {
+ type: 'SliderControl',
+ renderTrigger: true,
+ label: t('Max Zoom'),
+ description: t('The maximal zoom of the map'),
+ default: MAX_ZOOM_LEVEL,
+ min: MIN_ZOOM_LEVEL,
+ max: MAX_ZOOM_LEVEL,
+ step: 1,
+ },
+ },
Review Comment:
**Suggestion:** `min_zoom` and `max_zoom` are independently editable without
cross-field validation, allowing `min_zoom > max_zoom` and creating
inconsistent zoom constraints at runtime. Add validation to enforce `min_zoom
<= max_zoom`. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Users can configure nonsensical zoom constraints.
- ⚠️ Map zooming may behave unpredictably or feel broken.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the cartodiagram chart configuration UI, use the "Map Options" section
defined in
`controlPanel.ts:121-212` to edit the `min_zoom` slider
(`controlPanel.ts:167-178`) and
`max_zoom` slider (`controlPanel.ts:181-192`).
2. Set `min_zoom` to a value higher than `max_zoom` (for example, `min_zoom
= 12`,
`max_zoom = 5`), which the current control configuration allows because
there is no
cross-field validation.
3. These values are passed through `transformProps`
(`plugin/transformProps.ts`) to
`OlChartMap` as `minZoom` and `maxZoom` props (`OlChartMap.tsx:59-62`).
4. The dedicated effects at `OlChartMap.tsx:452-459` call
`view.setMinZoom(minZoom)` and
`view.setMaxZoom(maxZoom)` respectively; with `minZoom > maxZoom`, the zoom
constraints
are inconsistent and can cause unexpected clamping or inability to zoom,
degrading map
interaction.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=839f4c9262b54cb394e20527d718dca1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=839f4c9262b54cb394e20527d718dca1&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
**Line:** 167:193
**Comment:**
*Logic Error: `min_zoom` and `max_zoom` are independently editable
without cross-field validation, allowing `min_zoom > max_zoom` and creating
inconsistent zoom constraints at runtime. Add validation to enforce `min_zoom
<= max_zoom`.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=7be1b549ecfde6c14614874fceb98dbf6371a473741f4e212feca60fa569e34b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=7be1b549ecfde6c14614874fceb98dbf6371a473741f4e212feca60fa569e34b&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts:
##########
@@ -0,0 +1,75 @@
+/**
+ * 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 { DataRecord, QueryData } from '@superset-ui/core';
+import { Feature, FeatureCollection } from 'geojson';
+import { Feature as OlFeature } from 'ol';
+import GeoJSON from 'ol/format/GeoJSON';
+import { dataRecordsToOlFeatures } from './mapUtil';
+import { GeometryFormat } from '../constants';
+
+export const geojsonDataToFeatureCollection = (
+ data: QueryData['data'],
+ geomColumn: string,
+) => {
+ const features = data
+ .map((d: DataRecord) => {
+ const { [geomColumn]: geometry, ...restProps } = d;
+ if (!geometry) {
+ return undefined;
+ }
+ try {
+ const parsedGeom = JSON.parse(geometry as string);
Review Comment:
**Suggestion:** This parser unconditionally treats geometry as a JSON
string; if the datasource already returns a GeoJSON object, `JSON.parse` throws
and the feature is silently dropped, causing missing map data. Handle both
string and object geometry types before parsing. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Valid geospatial rows can be silently omitted from map.
- ⚠️ Charts against JSON geometry columns show incomplete data.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use a datasource where the geometry column is stored as JSON and Superset
returns it to
the frontend as a GeoJSON object in `QueryData['data']` (rather than a
string), for
example via a JSON-type column.
2. Call `geojsonDataToFeatureCollection` from
`superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts:26-57`
with this
query data and the geometry column name, as done from cartodiagram transform
logic.
3. For each record, the destructuring at `dataUtil.ts:32` assigns the
GeoJSON object to
`geometry`; the code then executes `JSON.parse(geometry as string)` at line
37.
4. Because `geometry` is already an object, `JSON.parse` throws, the error
is caught at
line 45, and `undefined` is returned; the subsequent `.filter` drops these
features,
producing a `FeatureCollection` at lines 51-56 that is missing valid
geometries, so parts
or all of the dataset fail to render on the map.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=05699a2634384a49a8dc104e690c421c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=05699a2634384a49a8dc104e690c421c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
**Line:** 37:37
**Comment:**
*Type Error: This parser unconditionally treats geometry as a JSON
string; if the datasource already returns a GeoJSON object, `JSON.parse` throws
and the feature is silently dropped, causing missing map data. Handle both
string and object geometry types before parsing.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=e3925eb341abe590c73cf6513344a4328917d22053ccf22f49a95413545c555e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39760&comment_hash=e3925eb341abe590c73cf6513344a4328917d22053ccf22f49a95413545c555e&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]
