bito-code-review[bot] commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r3614248511
##########
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx:
##########
@@ -86,12 +108,83 @@ export const LayerConfigsControl:
FC<LayerConfigsControlProps> = ({
name,
label,
description,
+ formData,
+ formWatchers,
+ featureCollectionTransformer,
renderTrigger,
hovered,
+ enableDataLayer = false,
+ colTypeMapping,
validationErrors,
}) => {
const [popoverVisible, setPopoverVisible] = useState<boolean>(false);
const [editItem, setEditItem] = useState<EditItem>(getEmptyEditItem());
+ const [currentFormData, setCurrentFormData] = useState(formData);
+ const [chartData, setChartData] = useState<FeatureCollection | undefined>();
+
+ /**
+ * We only want to watch for changes for a dynamic set of properties
+ * of the formData. To prevent unwanted http requests in the render cycles,
+ * we use the proxy currentFormData instead.
+ */
+ useEffect(() => {
+ setCurrentFormData(oldCurrentFormData => {
+ if (!formWatchers) {
+ return oldCurrentFormData;
+ }
+
+ const hasChanges = formWatchers.some(
+ watcher => !isEqual(formData?.[watcher],
oldCurrentFormData?.[watcher]),
+ );
+ if (hasChanges) {
+ return formData;
+ }
+ return oldCurrentFormData;
+ });
+ }, [formData, formWatchers]);
+
+ useEffect(() => {
+ if (!currentFormData || !enableDataLayer) {
+ return;
+ }
+ const buildQueryRegistry = getChartBuildQueryRegistry();
+ const chartQueryBuilder = buildQueryRegistry.get(
+ currentFormData.viz_type,
+ ) as any;
+ const chartQuery = chartQueryBuilder(currentFormData);
+ const fetchChartData = async () => {
+ try {
+ const body = JSON.stringify(chartQuery);
+ const response = await fetch('/api/v1/chart/data', {
+ method: 'POST',
+ headers: {
+ 'Content-Type': 'application/json',
+ },
+ body,
+ });
+ if (!response.ok) {
+ return;
+ }
+ const responseJson = await response.json();
+ let { data } = responseJson.result[0];
+
+ if (featureCollectionTransformer) {
+ data = featureCollectionTransformer(data, currentFormData);
+ }
+
+ setChartData(data);
+ } catch (err) {
+ console.error('Error fetching chart data for LayerConfigsControl',
err);
+ }
+ };
+
+ fetchChartData();
+ }, [currentFormData, enableDataLayer, featureCollectionTransformer]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing fetch abort cleanup</b></div>
<div id="fix">
The async `fetchChartData` has no cleanup mechanism. When the component
unmounts or `formData` changes rapidly, pending requests may complete after
unmount, causing React state updates on unmounted components and memory leaks.
Add `AbortController` with cleanup.
</div>
</div>
<small><i>Code Review Run #80d42d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx:
##########
@@ -50,3 +57,157 @@ export const fitMapToCharts = (olMap: Map, chartConfigs:
ChartConfig) => {
size: [250, 250],
});
};
+
+const fitToData = (
+ olMap: Map,
+ features: Feature[],
+ padding?: FitOptions['padding'] | undefined,
+) => {
+ const view = olMap.getView();
+ const extent = getExtentFromFeatures(features) || defaultExtent;
+
+ if (padding) {
+ view.fit(extent, { padding });
+ } else {
+ view.fit(extent);
+ }
+};
+
+const extractSridFromWkt = (wkt: string) => {
+ const extract: { geom: string; srid: string | null } = {
+ geom: wkt,
+ srid: null,
+ };
+ if (wkt.startsWith('SRID=')) {
+ // WKT with SRID, strip it
+ const srid = wkt.match(/SRID=(\d+);/);
+ if (srid) {
+ extract.srid = `EPSG:${srid[1]}`;
+ extract.geom = wkt.replace(/SRID=\d+;/, '');
+ }
+ }
+ return extract;
+};
+
+export const wkbToGeoJSON = (wkb: string) => {
+ const format = new WKB();
+ const feature = format.readFeature(wkb, {
+ featureProjection: 'EPSG:3857',
+ });
+ return new GeoJSON().writeFeatureObject(feature, {
+ featureProjection: 'EPSG:3857',
+ });
+};
+
+export const wktToGeoJSON = (wkt: string) => {
+ const format = new WKT();
+ const wktOpts: ReadOptions = {
+ featureProjection: 'EPSG:3857',
+ dataProjection: 'EPSG:4326', // default to WGS84
+ };
+ const extract = extractSridFromWkt(wkt);
+ const cleanedWkt = extract.geom;
+ if (extract.srid) {
+ wktOpts.dataProjection = extract.srid;
+ }
+ const feature = format.readFeature(cleanedWkt, wktOpts);
+ return new GeoJSON().writeFeatureObject(feature, {
+ featureProjection: 'EPSG:3857',
+ });
+};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated geometry parsing logic</b></div>
<div id="fix">
The `wkbToGeoJSON` (lines 92-100) and `wktToGeoJSON` (lines 102-117)
functions contain nearly identical structure: create format instance, read
feature, write GeoJSON. This duplication (8 vs 16 lines) creates maintenance
risk where changes to one may need mirroring in the other. Extract shared logic
into a private helper.
</div>
</div>
<small><i>Code Review Run #80d42d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:
##########
@@ -232,37 +306,165 @@ export const stripGeomColumnFromLabelMap = (
export const stripGeomColumnFromQueryData = (
queryData: any,
geomColumn: string,
+ geomFormat: GeometryFormat,
) => {
const queryDataClone = {
...structuredClone(queryData),
- ...stripGeomFromColnamesAndTypes(queryData, geomColumn),
+ ...stripGeomFromColnamesAndTypes(queryData, geomColumn, geomFormat),
};
if (queryDataClone.label_map) {
queryDataClone.label_map = stripGeomColumnFromLabelMap(
queryData.label_map,
geomColumn,
+ geomFormat,
);
}
return queryDataClone;
};
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const formatSeriesName = (
+ name: DataRecordValue | undefined,
+ {
+ numberFormatter,
+ timeFormatter,
+ coltype,
+ }: {
+ numberFormatter?: ValueFormatter;
+ timeFormatter?: TimeFormatter;
+ coltype?: GenericDataType;
+ } = {},
+) => {
+ if (name === undefined || name === null) {
+ return NULL_STRING;
+ }
+ if (typeof name === 'boolean' || typeof name === 'bigint') {
+ return name.toString();
+ }
+ if (name instanceof Date || coltype === GenericDataType.Temporal) {
+ const normalizedName =
+ typeof name === 'string' ? normalizeTimestamp(name) : name;
+ const d =
+ normalizedName instanceof Date
+ ? normalizedName
+ : new Date(normalizedName);
+
+ return timeFormatter ? timeFormatter(d) : d.toISOString();
+ }
+ if (typeof name === 'number') {
+ return numberFormatter ? numberFormatter(name) : name.toString();
+ }
+ return name;
+};
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const extractGroupbyLabel = ({
+ datum = {},
+ groupby,
+ numberFormatter,
+ timeFormatter,
+ coltypeMapping = {},
+}: {
+ datum?: DataRecord;
+ groupby?: string[] | null;
+ numberFormatter?: NumberFormatter;
+ timeFormatter?: TimeFormatter;
+ coltypeMapping?: Record<string, GenericDataType>;
+}) =>
+ ensureIsArray(groupby)
+ .map(val =>
+ formatSeriesName(datum[val], {
+ numberFormatter,
+ timeFormatter,
+ ...(coltypeMapping[val] && { coltype: coltypeMapping[val] }),
+ }),
+ )
+ .join(', ');
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
+export const getColtypesMapping = ({
+ coltypes = [],
+ colnames = [],
+}: Pick<ChartDataResponseResult, 'coltypes' | 'colnames'>): Record<
+ string,
+ GenericDataType
+> =>
+ colnames.reduce(
+ (accumulator, item, index) => ({ ...accumulator, [item]: coltypes[index]
}),
+ {},
+ );
+
+/**
+ * Reserve label colors for the chart.
+ *
+ * We call the CategoricalColorNamespace singleton to reserve
+ * label colors for the chart. This is necessary to ensure that
+ * we do not run into color collisions when rendering multiple
+ * charts in the same cartodiagram.
+ *
+ * TODO This only works in the context of a dashboard,
+ * since only there, the CategoricalColorNamespace singleton is being
used.
+ * In the explore view, label colors cannot be reserved without changing
+ * the overall color handling in superset.
+ *
+ * @param formData The formdata of the underlying chart
+ * @param dataByLocation The data grouped by location
+ * @param strippedQueryData The query data without the geom column
+ * @param sliceId The id of the chart slice
+ */
+export const reserveLabelColors = (
+ formData: Record<string, any>,
+ dataByLocation: LocationConfigMapping,
+ strippedQueryData: any,
+ sliceId: number,
+) => {
+ // Call color singleton to reserve label colors
+ // get needed control values from underlying chart config
+ const { colorScheme = '', groupby = [], dateFormat } = formData;
+ const colorFn = CategoricalColorNamespace.getScale(colorScheme as string);
+ const groupbyLabels = groupby.map(getColumnLabel);
+ Object.keys(dataByLocation).forEach(location => {
+ const coltypeMapping = getColtypesMapping({
+ ...strippedQueryData,
+ data: dataByLocation[location],
+ });
+ dataByLocation[location].forEach(datum => {
+ const name = extractGroupbyLabel({
+ datum,
+ groupby: groupbyLabels,
+ coltypeMapping,
+ timeFormatter: getTimeFormatter(dateFormat),
+ });
+
+ colorFn(name, sliceId);
+ });
+ });
+};
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>TypeScript: Avoid `any` types</b></div>
<div id="fix">
Parameter `strippedQueryData` is typed as `any`, violating TypeScript best
practices. Use `ChartDataResponseResult` for consistency and safety.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
+++
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
@@ -418,7 +418,7 @@
export const reserveLabelColors = (
formData: Record<string, any>,
dataByLocation: LocationConfigMapping,
strippedQueryData: any,
sliceId: number,
) => {
```
</div>
</details>
</div>
<details>
<summary><b>Citations</b></summary>
<ul>
<li>
Rule Violated: <a
href="https://github.com/apache/superset/blob/d42dce1/.cursor/rules/dev-standard.mdc#L16";>dev-standard.mdc:16</a>
</li>
</ul>
</details>
<div id="suggestion">
<div id="issue"><b>TypeScript: Missing return type</b></div>
<div id="fix">
Function `reserveLabelColors` lacks an explicit return type annotation. Add
`: void` since it doesn't return a value.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
+++
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
@@ -418,7 +418,7 @@
export const reserveLabelColors = (
formData: Record<string, any>,
dataByLocation: LocationConfigMapping,
strippedQueryData: any,
sliceId: number,
- ) => {
+ ): void => {
// Call color singleton to reserve label colors
// get needed control values from underlying chart config
const { colorScheme = '', groupby = [], dateFormat } = formData;
```
</div>
</details>
</div>
<small><i>Code Review Run #96ad42</i></small>
</div><div>
<div id="suggestion">
<div id="issue"><b>Missing test coverage for reserveLabelColors</b></div>
<div id="fix">
The new `reserveLabelColors` function (lines 412-439) has no corresponding
tests in transformPropsUtil.test.ts. Without tests, regression risk increases
if the color reservation logic changes.
</div>
</div>
<small><i>Code Review Run #80d42d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx:
##########
@@ -196,6 +307,39 @@ export const OlChartMap = (props: OlChartMapProps) => {
addLayers(layerConfigs);
}, [olMap, layerConfigs]);
+ useEffect(() => {
+ const { extentMode, fixedMaxX, fixedMaxY, fixedMinX, fixedMinY } =
+ currentMapMaxExtent;
+ const view = olMap.getView();
+ const center = view.getCenter();
+ const zoom = view.getZoom();
+ let extent;
+
+ if (
+ extentMode === 'CUSTOM' &&
+ fixedMaxX !== undefined &&
+ fixedMaxY !== undefined &&
+ fixedMinX !== undefined &&
+ fixedMinY !== undefined
+ ) {
+ extent = transformExtent(
+ [fixedMinX, fixedMinY, fixedMaxX, fixedMaxY],
+ 'EPSG:4326',
+ 'EPSG:3857',
+ );
+ }
+
+ olMap.setView(
+ new View({
+ center,
+ zoom,
+ extent,
+ maxZoom: view.getMaxZoom(),
+ minZoom: view.getMinZoom(),
+ }),
+ );
+ }, [olMap, currentMapMaxExtent]);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication of extent logic</b></div>
<div id="fix">
This new useEffect duplicates the extent transformation and view update
logic from the initial effect (lines 215-272). The initial effect runs once
(empty deps), the new effect runs on `currentMapMaxExtent` changes. Both do:
get extent from config, transform from EPSG:4326 to EPSG:3857, call
`olMap.setView()`.
</div>
</div>
<small><i>Code Review Run #80d42d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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,
+ },
+ },
+ ],
+ [
+ {
+ name: 'map_max_extent',
+ config: {
+ type: MapMaxExtentViewControl,
+ renderTrigger: true,
+ description: t(
+ "The constrained extent of the map on application start. NONE
won't set any constrained extent. CUSTOM allows users to define the extent
manually based on the current shown map extent.",
+ ),
+ label: t('Max. Extent'),
+ dontRefreshOnChange: true,
+ default: {
+ extentMode: 'NONE',
+ },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incomplete default value</b></div>
<div id="fix">
The `map_max_extent` default only sets `extentMode: 'NONE'` but omits
required fields `maxX`, `maxY`, `minX`, `minY` defined in `MapMaxExtentConfigs`
type. When the `MapMaxExtentViewControl` component calls `onButtonClick`, it
copies these undefined values to `fixedMaxX`, `fixedMaxY`, etc., causing
incorrect behavior.
</div>
</div>
<small><i>Code Review Run #80d42d</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
