bito-code-review[bot] commented on code in PR #39760:
URL: https://github.com/apache/superset/pull/39760#discussion_r3271365421
##########
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts:
##########
@@ -202,21 +268,30 @@ export const stripGeomFromColnamesAndTypes = (
*
* @param queryData The querydata.
* @param geomColumn Name of the geom column.
+ * @param geomFormat The format of the geometries.
* @returns labelMap without the geom column.
*/
export const stripGeomColumnFromLabelMap = (
labelMap: { [key: string]: string[] },
geomColumn: string,
+ geomFormat: GeometryFormat,
) => {
const newLabelMap: Record<string, string[]> = {};
Object.entries(labelMap).forEach(([key, value]) => {
if (key === geomColumn) {
return;
}
- const geojsonCols = getGeojsonColumns(value);
- const filter = geojsonCols.length ? [geojsonCols[0]] : [];
+ let geomColumns: number[];
+ if (geomFormat === GeometryFormat.GEOJSON) {
+ geomColumns = getGeojsonColumns(value);
+ } else if (geomFormat === GeometryFormat.WKB) {
+ geomColumns = getWkbColumns(value);
+ } else {
+ geomColumns = getWktColumns(value);
+ }
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-1041: Semantic duplication with
getGeomColumns</b></div>
<div id="fix">
Semantic duplication: Lines 284-291 replicate the logic in `getGeomColumns`
(lines 109-120). Use the existing helper to avoid divergence risk.
([CWE-1041](https://cwe.mitre.org/data/definitions/1041.html))
</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
@@ -281,15 +281,8 @@ export const stripGeomColumnFromLabelMap = (
if (key === geomColumn) {
return;
}
- let geomColumns: number[];
- if (geomFormat === GeometryFormat.GEOJSON) {
- geomColumns = getGeojsonColumns(value);
- } else if (geomFormat === GeometryFormat.WKB) {
- geomColumns = getWkbColumns(value);
- } else {
- geomColumns = getWktColumns(value);
- }
+ const geomColumns = getGeomColumns(geomFormat, value);
const filter = geomColumns.length ? [geomColumns[0]] : [];
const columnName = createColumnName(value, filter);
const restItems = value.filter((v, idx) => !geomColumns.includes(idx));
```
</div>
</details>
</div>
<small><i>Code Review Run #2bc862</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/webpack.config.js:
##########
@@ -513,10 +513,70 @@ const config = {
exclude: [/\.test.tsx?$/, /node_modules/],
use: [createSwcLoader('typescript', true)],
},
+ {
+ test: /\.js$/,
+ include: /node_modules\/geostyler/,
+ resolve: {
+ // Currently, GeoStyler uses imports without file extension
+ // on ESM modules, which is not supported by webpack.
+ fullySpecified: false,
+ },
+ },
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>SEMANTIC_DUPLICATION in webpack rules</b></div>
<div id="fix">
This new rule is semantically duplicating the existing rule at lines
505-510. Both target `.js` files in `node_modules/geostyler` with identical
`fullySpecified: false` resolve configuration. The pre-existing rule is more
comprehensive, covering `geostyler-openlayers-parser`,
`geostyler-mapbox-parser`, and `geostyler-sld-parser` in addition to
`geostyler` itself. Remove this redundant rule to avoid maintenance divergence.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset-frontend/webpack.config.js
+++ superset-frontend/webpack.config.js
@@ -513,14 +513,6 @@
use: [createSwcLoader('typescript', true)],
},
- {
- test: /\.js$/,
- include: /node_modules\/geostyler/,
- resolve: {
- // Currently, GeoStyler uses imports without file extension
- // on ESM modules, which is not supported by webpack.
- fullySpecified: false,
- },
- },
{
test: /\.jsx?$/,
```
</div>
</details>
</div>
<small><i>Code Review Run #2bc862</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/constants.ts:
##########
@@ -0,0 +1,40 @@
+/**
+ * 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.
+ */
+export const TIMESLIDER_HEIGHT = 28;
+
+export enum TimesliderTooltipFormat {
+ DATE,
+ TIME,
+ DATETIME,
+}
+
+export enum GeometryFormat {
+ GEOJSON = 'GEOJSON',
+ WKB = 'WKB',
+ WKT = 'WKT',
+}
+
+// copy of
+// superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
+export const NULL_STRING = '<NULL>';
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantic duplication of NULL_STRING</b></div>
<div id="fix">
`NULL_STRING` is already defined in `src/utils/common.ts` with the same
value. This creates semantic duplication with divergence risk. Consider
importing from the shared location instead of copying.
</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/constants.ts
+++ superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
@@ -30,9 +30,8 @@ export enum GeometryFormat {
WKT = 'WKT',
}
-// copy of
-// superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
-export const NULL_STRING = '<NULL>';
+// Import shared constant instead of duplicating
+export { NULL_STRING } from 'src/utils/common';
export const SELECTION_LAYER_NAME = 'thematic-selection-layer';
```
</div>
</details>
</div>
<small><i>Code Review Run #2bc862</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/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx:
##########
@@ -81,8 +104,89 @@ export const StyledLayerTreeItem = styled(LayerTreeItem)`
`}
`;
-// forwardRef is needed here in order for emotion and antd tree to work
properly
-export const FlatLayerTree = forwardRef<HTMLDivElement, FlatLayerTreeProps>(
+interface DraggableLayerTreeItemProps {
+ layerConf: LayerConfWithId;
+ index: number;
+ draggable?: boolean;
+ onEditLayer: (layerConf: LayerConfWithId, idx: number) => void;
+ onRemoveLayer: (idx: number) => void;
+ onMoveLayerByIndex: (dragIndex: number, hoverIndex: number) => void;
+}
+
+const DraggableLayerTreeItem: FC<DraggableLayerTreeItemProps> = ({
+ layerConf,
+ index,
+ draggable = false,
+ onEditLayer,
+ onRemoveLayer,
+ onMoveLayerByIndex,
+}) => {
+ const ref = useRef<HTMLDivElement>(null);
+
+ const [, drag] = useDrag<DragItem, void, unknown>({
+ item: {
+ type: LAYER_CONFIG_DRAG_TYPE,
+ dragIndex: index,
+ },
+ canDrag: draggable,
+ });
+
+ const [, drop] = useDrop({
+ accept: LAYER_CONFIG_DRAG_TYPE,
+ hover(item: DragItem, monitor: DropTargetMonitor) {
+ // Stop early when dragging is disabled or the item node is not ready yet
+ if (!draggable || !ref.current) {
+ return;
+ }
+ const { dragIndex } = item;
+ const hoverIndex = index;
+ if (dragIndex === hoverIndex) {
+ return;
+ }
+
+ const hoverBoundingRect = ref.current.getBoundingClientRect();
+ const hoverMiddleY =
+ (hoverBoundingRect.bottom - hoverBoundingRect.top) / 2;
+ const clientOffset = monitor.getClientOffset();
+ const hoverClientY = clientOffset?.y
+ ? clientOffset?.y - hoverBoundingRect.top
+ : 0;
+
+ // Move only after crossing the middle to avoid jumpy reordering
+ if (dragIndex < hoverIndex && hoverClientY < hoverMiddleY) {
+ return;
+ }
+ if (dragIndex > hoverIndex && hoverClientY > hoverMiddleY) {
+ return;
+ }
+
+ onMoveLayerByIndex(dragIndex, hoverIndex);
+ // Note: we're mutating the monitor item here!
+ // Generally it's better to avoid mutations,
+ // but it's good here for the sake of performance
+ // to avoid expensive index searches.
+ // eslint-disable-next-line no-param-reassign
+ item.dragIndex = hoverIndex;
+ },
+ });
+
+ drag(drop(ref));
+
+ return (
+ <DragContainer ref={ref}>
+ <StyledLayerTreeItem
+ layerConf={layerConf}
+ onEditClick={() => onEditLayer(layerConf, index)}
+ onRemoveClick={() => onRemoveLayer(index)}
+ />
+ </DragContainer>
+ );
+};
+
+export const FlatLayerTree: FC<FlatLayerTreeProps> = forwardRef<
+ HTMLDivElement,
+ FlatLayerTreeProps
+>(
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect FC with forwardRef typing</b></div>
<div id="fix">
Combining `FC<FlatLayerTreeProps>` with `forwardRef<HTMLDivElement,
FlatLayerTreeProps>` is redundant. `FC` adds implicit `children` and conflicts
with `forwardRef` typing. Use `forwardRef` directly without `FC` wrapper.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
---
superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
+++
superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
@@ -183,7 +183,7 @@ const DraggableLayerTreeItem:
FC<DraggableLayerTreeItemProps> = ({
);
};
-export const FlatLayerTree: FC<FlatLayerTreeProps> = forwardRef<
+export const FlatLayerTree = forwardRef<
HTMLDivElement,
FlatLayerTreeProps
>(
```
</div>
</details>
</div>
<small><i>Code Review Run #2bc862</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/layerUtil.tsx:
##########
@@ -152,8 +197,71 @@ export const createLayer = async (layerConf: LayerConf) =>
{
layer = await createWfsLayer(layerConf);
} else if (isXyzLayerConf(layerConf)) {
layer = createXyzLayer(layerConf);
+ } else if (isDataLayerConf(layerConf)) {
+ layer = await createDataLayer(layerConf);
} else {
console.warn('Provided layerconfig is not recognized');
}
return layer;
};
+
+export const removeSelectionLayer = (olMap: Map) => {
+ const selectionLayer = olMap
+ .getLayers()
+ .getArray()
+ .filter(l => l.get(LAYER_NAME_PROP) === SELECTION_LAYER_NAME)
+ .pop();
+ if (selectionLayer) {
+ olMap.removeLayer(selectionLayer);
+ }
+};
+
+export const getSelectedFeatures = (
+ dataLayers: VectorLayer<VectorSource>[],
+ filterState: FilterState,
+ crossFilterColumn: string,
+) => {
+ let selectedFeatures: Feature[] = [];
+ if (
+ filterState.selectedValues !== null &&
+ filterState.selectedValues !== undefined &&
+ dataLayers
+ ) {
+ selectedFeatures = dataLayers.flatMap(dataLayer =>
+ dataLayer
+ .getSource()!
+ .getFeatures()
+ .filter(f =>
+ filterState.selectedValues.includes(f.get(crossFilterColumn)),
+ ),
+ );
+ }
+ return selectedFeatures;
+};
+
+export const setSelectionBackgroundOpacity = (
+ dataLayers: VectorLayer<VectorSource>[],
+ opacity: number,
+) => {
+ dataLayers.forEach(dataLayer => {
+ dataLayer.setOpacity(opacity);
+ });
+};
+
+export const createSelectionLayer = (
+ dataLayers: VectorLayer<VectorSource>[],
+ features: Feature[],
+) => {
+ const selectionLayer = new VectorLayer({
+ source: new VectorSource({
+ features,
+ }),
+ });
+ selectionLayer.set(LAYER_NAME_PROP, SELECTION_LAYER_NAME);
+ // TODO how can we handle multiple data layers?
+ const layerStyle = dataLayers[0].getStyle();
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Unsafe array access</b></div>
<div id="fix">
Accessing `dataLayers[0]` without checking array length risks runtime error
if empty array passed. Add guard check.
</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/layerUtil.tsx
+++
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
@@ -258,7 +258,9 @@
});
selectionLayer.set(LAYER_NAME_PROP, SELECTION_LAYER_NAME);
// TODO how can we handle multiple data layers?
- const layerStyle = dataLayers[0].getStyle();
+ const layerStyle = dataLayers.length > 0 ? dataLayers[0].getStyle() :
undefined;
- if (layerStyle) {
+ if (layerStyle) {
selectionLayer.setStyle(layerStyle);
}
return selectionLayer;
```
</div>
</details>
</div>
<small><i>Code Review Run #2bc862</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]
