Repository navigation
Conversation
dosubot
Bot
added
explore:control
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #39760 +/- ##
==========================================
- Coverage 65.27% 65.19% -0.09%
==========================================
Files 2795 2800 +5
Lines 157835 158350 +515
Branches 36080 36224 +144
==========================================
+ Hits 103027 103235 +208
- Misses 52816 53122 +306
- Partials 1992 1993 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #6db292
Actionable Suggestions - 8
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
- Translation Inconsistency · Line 528-528
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts - 1
- Unhandled JSON Parse Error · Line 32-42
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 1
- Missing useEffect Dependency · Line 447-449
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts - 1
- Test naming typo · Line 216-216
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts - 1
- Type Safety Violation · Line 98-101
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts - 2
- Dead code introduced · Line 25-25
- Misleading test name · Line 113-113
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts - 1
- Logic Error and Logging Inconsistency · Line 57-70
Additional Suggestions - 8
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 2
-
Inconsistent state usage in useEffect · Line 173-174The useEffect destructures `extentMode` and fixed extent values from `mapMaxExtent`, but to align with the state management introduced in the diff (which uses `currentMapMaxExtent` to avoid unnecessary re-renders), it should use `currentMapMaxExtent`. Additionally, the dependency array should include `currentMapMaxExtent` to ensure the effect re-runs when the stable extent state changes, preventing stale extent application if `mapMaxExtent` updates without `mapView` changing.
Code suggestion
@@ -170,7 +170,7 @@ useEffect(() => { const view = olMap.getView(); const { mode, fixedLatitude, fixedLongitude, fixedZoom } = mapView; - const { extentMode, fixedMaxX, fixedMaxY, fixedMinX, fixedMinY } = - mapMaxExtent; + const { extentMode, fixedMaxX, fixedMaxY, fixedMinX, fixedMinY } = + currentMapMaxExtent; switch (mode) { case 'CUSTOM': { @@ -254,4 +254,4 @@ } }, [mapView]); + }, [mapView, currentMapMaxExtent]); -
Initial Control Value Missing · Line 215-271The initial useEffect sets the view extent for CUSTOM mode but does not update the map_max_extent control value, unlike the default case. This inconsistency means the control starts with incorrect values for CUSTOM, affecting `setControlValue` reliability.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx - 1
-
Incorrect Zero Handling · Line 36-37Use explicit undefined checks for value.fixedMaxX and value.fixedMaxY (and similarly for fixedMinX and fixedMinY) to avoid treating 0 as falsy, preserving the existing conditional rendering logic.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 3
-
Duplicated style parsing logic · Line 169-177The style parsing code in createDataLayer duplicates the logic in createWfsLayer, increasing maintenance burden if the parsing needs updates.
-
Inaccurate JSDoc parameter · Line 158-159The JSDoc for createDataLayer references a 'featureCollection' parameter that isn't in the function signature, which could confuse developers.
Code suggestion
@@ -158,3 +158,2 @@ - * @param dataLayerConf The layer configuration - * @param featureCollection The featureCollection for the layer source - * + * @param dataLayerConf The layer configuration + *
-
Inaccurate JSDoc parameter · Line 189-189The JSDoc for createLayer references a 'featureCollection' parameter that isn't in the function signature, which could confuse developers.
Code suggestion
@@ -188,2 +188,1 @@ - * @param layerConf The layer configuration - * @param featureCollection The featureCollection for the dataLayer + * @param layerConf The layer configuration
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts - 2
-
Incomplete test assertions · Line 63-70The test for multiple WKB columns checks the array length and first index but misses verifying the second index (expected 3). This reduces test robustness, as it could mask bugs in index calculation. Add expect(result[1]).toEqual(3); to match the thoroughness of the getGeojsonColumns test.
Code suggestion
--- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts +++ superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts @@ -63,6 +63,7 @@ test('gets multiple WKB columns', () => { const wkbCol = '0101000020E610000000000000000020400000000000804A40'; const columns = ['foo', 'bar', wkbCol, wkbCol]; + const result = getWkbColumns(columns); expect(result).toHaveLength(2); expect(result[0]).toEqual(2); + expect(result[1]).toEqual(3); });
-
Incomplete test assertions · Line 90-97Similar to the WKB test, the multiple WKT columns test verifies length and first index but omits the second index (expected 3). This inconsistency with the getGeojsonColumns test could allow undetected issues. Add expect(result[1]).toEqual(3); for complete validation.
Code suggestion
--- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts +++ superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts @@ -90,6 +90,7 @@ test('gets multiple WKT columns', () => { const wktCol = 'POINT(8 53)'; const columns = ['foo', 'bar', wktCol, wktCol]; + const result = getWktColumns(columns); expect(result).toHaveLength(2); expect(result[0]).toEqual(2); + expect(result[1]).toEqual(3); });
-
Review Details
-
Files reviewed - 32 · Commit Range:
7e6ab17..7e6ab17- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
jansule
force-pushed
the
2026-04-28-only-carto
branch
5 times, most recently
from
April 29, 2026 14:28
32a64b3 to
85b4aa4
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #aa3025
Actionable Suggestions - 7
-
superset/config.py - 1
- Broken Attribution Link · Line 2474-2474
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts - 1
- Semantic Duplication · Line 25-29
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 1
- Documentation Mismatch · Line 189-189
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
- Broad exception catch · Line 479-479
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
- Code Duplication · Line 106-106
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts - 2
- Incomplete test assertion · Line 63-70
- Incomplete test assertion · Line 90-97
Additional Suggestions - 3
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts - 1
-
Performance Regression · Line 25-29The new getBootstrapData always parses the bootstrap JSON on each invocation, which can hurt performance if called frequently. Consider introducing a module-level cache (e.g., a `cachedBootstrapData` variable) to parse the data only once, similar to the original implementation.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 1
-
Semantic Duplication · Line 251-267The extent calculation logic (calculateExtent, transformExtent, and setControlValue for 'map_max_extent') is duplicated between the initial useEffect (lines 251-267) and the view change listener (lines 356-385). This creates a maintenance risk if the logic needs updates. Consider extracting it into a helper function like `updateMapMaxExtent` to ensure consistency.
-
-
superset-frontend/webpack.config.js - 1
-
Redundant webpack rule · Line 513-521The new webpack rule duplicates the existing rule at lines 500-504, which already handles fullySpecified resolution for geostyler .js files. This creates unnecessary duplication and potential maintenance confusion. Remove the new rule to keep the config clean.
-
Review Details
-
Files reviewed - 32 · Commit Range:
85b4aa4..85b4aa4- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
jansule
force-pushed
the
2026-04-28-only-carto
branch
2 times, most recently
from
April 30, 2026 11:25
2b7634f to
d42dce1
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #96ad42
Actionable Suggestions - 10
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts - 1
- Broad exception catch · Line 44-47
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts - 1
- Code Quality & Performance Issues · Line 25-41
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx - 1
- Type Safety Violation · Line 103-103
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 5
- TypeScript: Missing return type · Line 327-398
- Code: Semantic duplication · Line 327-398
- TypeScript: Missing return type · Line 327-398
- TypeScript: Missing return type · Line 418-445
- TypeScript: Avoid `any` types · Line 418-445
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts - 1
- TypeScript error suppression inconsistency · Line 165-171
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx - 1
- Error Handling Missing · Line 155-175
Additional Suggestions - 4
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 1
-
Inconsistent extent control update · Line 379-385Wrap the `setControlValue('map_max_extent', …)` call in a condition checking `currentMapMaxExtent.extentMode !== 'CUSTOM'`, and ensure you destructure `extentMode` from `currentMapMaxExtent` at the start of the listener effect so custom extents aren’t overridden.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts - 1
-
Missing return types · Line 32-183All exported functions should include explicit return type hints for better type safety and consistency, as per repository standards.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx - 1
-
Drag and Drop Null Safety · Line 150-153In the drag-and-drop hover logic, if clientOffset is null, hoverClientY defaults to 0, which may cause incorrect layer reordering. Add `if (!clientOffset) return;` after getting clientOffset to skip the move logic safely.
Code suggestion
@@ -150,4 +150,5 @@ - const clientOffset = monitor.getClientOffset(); - const hoverClientY = clientOffset?.y - ? clientOffset?.y - hoverBoundingRect.top - : 0; + const clientOffset = monitor.getClientOffset(); + if (!clientOffset) return; + const hoverClientY = clientOffset?.y + ? clientOffset?.y - hoverBoundingRect.top + : 0;
-
-
superset-frontend/webpack.config.js - 1
-
Redundant Webpack Rule · Line 513-521The new webpack rule for .js files in node_modules/geostyler with fullySpecified: false is redundant because the existing rule at lines 500-504 already applies the same resolve option to geostyler and its parser packages. This duplication increases maintenance burden without providing additional functionality.
-
Review Details
-
Files reviewed - 32 · Commit Range:
d42dce1..d42dce1- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 3
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/package.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
Member
|
Thanks for chipping away at the bot comments. Once those are addressed or refuted and marked resolved, and the conflicts are ironed out, @villebro and I will take a test spin and review this. Appreciate the maintenance work :D |
jansule
force-pushed
the
2026-04-28-only-carto
branch
from
May 18, 2026 16:17
d42dce1 to
4f4f125
Compare
Contributor
Author
Contributor
There was a problem hiding this comment.
Code Review Agent Run #2bc862
Actionable Suggestions - 5
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
- CWE-1041: Semantic duplication with getGeomColumns · Line 284-291
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx - 1
- Incorrect FC with forwardRef typing · Line 186-189
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 1
- Unsafe array access · Line 262-262
-
superset-frontend/webpack.config.js - 1
- SEMANTIC_DUPLICATION in webpack rules · Line 516-524
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts - 1
- Semantic duplication of NULL_STRING · Line 35-35
Additional Suggestions - 10
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
-
Missing data prop for GeoStyler · Line 688-691The `StyledGeoStyler` component at line 688 is missing the `data` prop. The `geostylerData` state is populated for WFS layers via `readWfsData` (lines 390-427) and for DATA layers (lines 381-387), but is not passed to the styled component. This prevents the symbolizer editor from accessing the data schema, limiting functionality for WFS layers. The `data` prop is defined in `GeoStylerWrapperProps` (types.ts:118) and passed through to `CardStyle`.
Code suggestion
--- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx +++ superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx @@ -686,6 +686,7 @@ export const LayerConfigsPopoverContent: FC< <GeoStylerContext.Provider value={geoStylerContext}> <StyledGeoStyler style={currentLayerConf.style} + data={geostylerData} onStyleChange={onStyleChange} /> </GeoStylerContext.Provider>
-
-
superset/config.py - 1
-
Hardcoded third-party service URL · Line 2467-2484The `MAP_DEFAULT_LAYERS` configuration hardcodes a URL to `https://ows.terrestris.de/osm-gray/service` (line 2474), a third-party service that could become unavailable. Users relying on this default without overrides may experience service disruptions.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts - 1
-
Missing unit tests for new function · Line 68-71New function `getMapExtentPadding` lacks unit tests. Per repo standards, add tests verifying both branches: padding value provided returns filled array, undefined returns undefined.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 1
-
JSDoc parameter mismatch · Line 159-159JSDoc documents `@param featureCollection` but function signature doesn't include this parameter. Remove or update documentation to match actual parameters.
Code suggestion
--- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx +++ superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx @@ -155,7 +155,6 @@ /** * Create a DATA layer. * * @param dataLayerConf The layer configuration - * @param featureCollection The featureCollection for the layer source * * @returns The created DATA layer */
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx - 1
-
Redundant id assignment logic · Line 88-91Redundant condition in `ensureLayerId`: when `layerConf.id` exists, spreading and re-assigning the same id is unnecessary. Use nullish coalescing instead.
Code suggestion
--- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx (lines 88-91) --- 88: const ensureLayerId = (layerConf: LayerConf): LayerConfWithId => 89: - layerConf.id 90: - ? { ...layerConf, id: layerConf.id } 91: - : { ...layerConf, id: nanoid() }; 89: + ({ ...layerConf, id: layerConf.id ?? nanoid() });
-
-
superset/views/base.py - 1
-
Undocumented configuration keys · Line 126-128The new map configuration keys are not documented. While `MAPBOX_API_KEY` is documented in `docs/admin_docs/configuration/map-tiles.mdx`, the new `MAP_PROJECTIONS`, `MAP_DEFAULT_LAYERS`, and `MAP_GEOSTYLER_COMPOSITION` keys have no user-facing documentation. Users configuring map features will not know these options exist.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts - 1
-
any type usage in new exports · Line 84-84Uses `any` type which violates project standards. While existing code has same issue, new code should use proper types per `.cursor/rules/dev-standard.mdc`.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts - 2
-
Missing return type annotation · Line 26-57Function `geojsonDataToFeatureCollection` lacks return type annotation. Per BITO.md rule [7819], all functions should have explicit return types for better type safety and documentation.
Code suggestion
--- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts +++ superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts @@ -26,7 +26,7 @@ export const geojsonDataToFeatureCollection = ( data: QueryData['data'], geomColumn: string, -) => { +): FeatureCollection => { const features = data .map((d: DataRecord) => {
-
Missing return type annotation · Line 59-75Function `dataToFeatureCollection` lacks return type annotation. Per BITO.md rule [7819], all functions should have explicit return types for better type safety and documentation.
Code suggestion
--- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts +++ superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts @@ -59,7 +59,7 @@ export const dataToFeatureCollection = ( data: QueryData['data'], geomColumn: string, geomFormat: GeometryFormat.WKB | GeometryFormat.WKT, -) => { +): FeatureCollection => { const geojsonFormat = new GeoJSON({
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts - 1
-
Naming inconsistency in control types · Line 199-199Control type uses direct component reference `MapMaxExtentViewControl` while all other controls in file use string literals (e.g., 'SliderControl'). This naming inconsistency reduces maintainability. Consider registering the component as a named control type for consistency.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts - 1
- Avoid `any` type in type definition · Line 99-99
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 1
- FilterState type mismatch · Line 221-221
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx - 1
- Direct antd import violates standards · Line 22-22
Review Details
-
Files reviewed - 32 · Commit Range:
4f4f125..4f4f125- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
jansule
force-pushed
the
2026-04-28-only-carto
branch
from
May 21, 2026 08:18
4f4f125 to
caf869b
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #c40662
Actionable Suggestions - 4
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx - 1
- CWE-N/A: Missing Unit Test Coverage · Line 21-21
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
- Use @superset-ui/core for Upload · Line 31-31
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts - 1
- Missing test coverage for new formats · Line 97-117
-
superset-frontend/webpack.config.js - 1
- Dead code: non-existent path exclusions · Line 522-530
Additional Suggestions - 14
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
-
Missing WKT Geometry Type · Line 89-97The `WktFormatIdentifiers` array at line 89-97 is missing 'GEOMETRYCOLLECTION', a valid WKT geometry type. The `wktToGeoJSON` function in mapUtil.tsx can handle this type, but the identifier detection will fail, causing geometry columns to be incorrectly classified as non-geometry and leading to incorrect column filtering in `groupByLocationGenericX` and `stripGeomFromColnamesAndTypes`.
Code suggestion
--- a/superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts +++ b/superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts @@ -87,6 +87,7 @@ export const getWktColumns = (columns: string[]) => [ 'MULTIPOINT', 'MULTILINESTRING', 'MULTIPOLYGON', + 'GEOMETRYCOLLECTION', ];
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts - 3
-
url moved from BaseLayerConf · Line 100-100The `url` property was moved from `BaseLayerConf` to specific layer types (WfsLayerConf, XyzLayerConf, WmsLayerConf). Code accessing `url` via `BaseLayerConf` references will break. Ensure all consumers use discriminated union access or type guards.
-
DataLayerConf url optional inconsistency · Line 127-130`DataLayerConf` has optional `url` while other layer types (WFS, XYZ, WMS) have required `url`. This is intentional for data layers that load from local sources, but verify this matches the intended design.
-
Redundant undefined in optional type · Line 86-86The type `mapExtentPadding?: number | undefined` is redundant. The optional modifier `?` already implies the property can be `undefined`. Use `mapExtentPadding?: number` for cleaner TypeScript syntax.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
-
Missing data prop for GeoStyler · Line 688-691The `data` prop was removed from StyledGeoStyler but is required to pass WFS fetched data to GeoStyler's CardStyle. The `createGeoStylerContext` function uses `data?.exampleFeatures.features.length` (geoStylerUtil.ts:109) to enable/disable classification features. Without this prop, WFS data preview and classification will not function.
Code suggestion
@@ -688,3 +688,4 @@ <StyledGeoStyler style={currentLayerConf.style} onStyleChange={onStyleChange} + data={geostylerData} />
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts - 1
-
CWE-483: Missing Test Cases · Line 202-214Test only validates the positive path (non-empty selectedValues). The `getSelectedFeatures` function at line 225-226 in layerUtil.tsx explicitly checks for null/undefined, yet these edge cases are not covered. Consider adding assertions for null, undefined, and empty array to ensure behavior is intentional and regressions are caught.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts - 1
-
Incomplete test assertions · Line 76-94The test adds new fields to `formData` (geomFormat, mapExtentPadding, mapMaxExtent, maxZoom, minZoom) but does not assert they are correctly passed through to the output. Per testing best practices, tests should verify the behavior they claim to test.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts - 1
-
Inconsistent caching pattern · Line 41-43Caching pattern diverges from original `getBootstrapData.ts` (line 29). Original caches `DEFAULT_BOOTSTRAP_DATA` directly; new code caches `null` and applies fallback at return. This creates divergent state after first call with missing data.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 1
-
Implicit default for 'NONE' · Line 242-242The switch statement uses `default` to handle the 'NONE' mode, but the type definition uses explicit `extentMode: 'NONE' | 'CUSTOM'`. Using `case 'NONE':` would improve consistency and type safety.
Code suggestion
--- a/superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx +++ b/superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx @@ -239,7 +239,7 @@ export const OlChartMap = (props: OlChartMapProps) => { ); break; } - default: { + case 'NONE': { const newView = new View({ center: view.getCenter(), zoom: view.getZoom(),
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx - 2
-
Missing JSDoc on helper function · Line 61-74The private helper function `fitToData` lacks JSDoc documentation. Add a JSDoc block describing its purpose and parameters, following the pattern established by `fitMapToCharts`.
-
Missing JSDoc on SRID extractor · Line 76-90The private function `extractSridFromWkt` lacks JSDoc documentation. Add a JSDoc block describing its purpose, parameters, and return type.
-
-
superset-frontend/webpack.config.js - 1
-
Maintainability: multiple d3 exclusions · Line 532-569Multiple d3-* modules are individually excluded via separate path.resolve calls. This creates maintenance burden and potential for divergence if new d3 subpackages are added. Consider a single pattern matching all d3 modules.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx - 2
-
Avoid polished mix function · Line 24-24The 'mix' function from 'polished' is used to compute a hover color. The existing MapViewControl.tsx in the same codebase uses `theme.colorPrimaryBgHover` directly, which is more idiomatic and avoids unnecessary CSS computation.
Code suggestion
--- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx +++ superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx @@ -21,7 +21,6 @@ import { css, styled } from '@apache-superset/core/theme'; import { Button } from 'antd'; import { Radio } from '@superset-ui/core/components'; -import { mix } from 'polished'; import { FC } from 'react'; import { RadioChangeEvent } from 'antd/lib/radio'; import { @@ -48,7 +47,7 @@ margin-left: 9px; margin-top: 10px; &:hover { - background-color: ${mix(0.1, theme.colorPrimary, theme.colorPrimaryBg)}; + background-color: ${theme.colorPrimaryBgHover}; color: ${theme.colorPrimaryText}; } `}
-
Redundant optional chaining · Line 103-106The optional chaining operator `?.` is used on `value` in lines 103-106, but the code already returns early if `!value` on line 99. This is redundant and could mislead readers about the null-safety state.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx - 3
- CWE-352: Missing CSRF Token in Fetch · Line 158-164
- Type Safety: `as any` Cast Violation · Line 151-153
- Popover uses wrong prop for styling · Line 285-290
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts - 1
- Type mismatch in return value · Line 72-72
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 3
- Missing useEffect dependency · Line 409-415
- Wrong coordinate system · Line 428-429
- Unnecessary object allocation · Line 243-249
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx - 1
- CWE-1096: antd Direct Import · Line 22-22
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts - 3
- Missing return type hint · Line 37-37
- Missing return type hint · Line 49-49
- Missing return type hint · Line 55-55
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 1
- FilterState property mismatch · Line 225-235
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx - 1
- Use @superset-ui/core/components for Tag · Line 21-21
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts - 1
- SEMANTIC_DEAD_CODE: Unused imports from removed Tree types · Line 19-19
Review Details
-
Files reviewed - 32 · Commit Range:
caf869b..caf869b- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
jansule
force-pushed
the
2026-04-28-only-carto
branch
from
July 20, 2026 09:12
caf869b to
8cb1a3a
Compare
jansule
force-pushed
the
2026-04-28-only-carto
branch
3 times, most recently
from
July 20, 2026 12:02
ee8d14d to
a1a632d
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #80d42d
Actionable Suggestions - 5
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
- Missing test coverage for reserveLabelColors · Line 412-439
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts - 1
- Incomplete default value · Line 206-208
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx - 1
- Missing fetch abort cleanup · Line 146-182
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 1
- Semantic duplication of extent logic · Line 310-341
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx - 1
- Duplicated geometry parsing logic · Line 92-117
Additional Suggestions - 12
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx - 1
-
Inconsistent SRID handling WKB vs WKT · Line 92-100The `wkbToGeoJSON` function lacks `dataProjection` configuration while `wktToGeoJSON` sets `dataProjection: 'EPSG:4326'` by default. WKB from PostGIS (EWKB) commonly includes embedded SRID that would be ignored. This creates inconsistent behavior between WKB and WKT parsing paths. Consider either adding dataProjection to wkbToGeoJSON or adding explicit SRID extraction for EWKB support.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts - 1
-
Silent error swallowing · Line 58-63The bare `catch` block silently returns an empty `FeatureCollection` on any error, including when `sliceId` is undefined (since it's destructured from `formData` without a default). Users will see a blank map with no indication of the failure. The underlying `reserveLabelColors` function (called within `getChartConfigs` at `transformPropsUtil.ts:507`) passes `sliceId` to `colorFn()` at line 436 — if `sliceId` is undefined, this could cause silent failures.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts - 1
-
Variable shadowing in test · Line 235-235Variable `selectionLayer` at line 235 shadows the module-level variable declared at line 94. While this works due to local scope, it creates confusion about which `selectionLayer` is being tested. Consider renaming to `resultLayer` or `testSelectionLayer`.
-
-
superset/config.py - 1
-
CWE-310: Third-Party URL Privacy Risk · Line 2876-2876The hardcoded WMS URL `https://ows.terrestris.de/osm-gray/service` causes client browsers to directly request a third-party service for all map visualizations. This may raise privacy concerns in regulated environments (GDPR, data residency) where users may not consent to their geographic data being transmitted to external parties. Consider making this configurable or defaulting to empty.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts - 1
-
Performance: repeated projection registration · Line 78-79Lines 78-79: `registerMapProjections` is called on every `CartodiagramPlugin` instantiation (e.g., called twice in MainPreset.ts line 199). While `proj4.defs()` may not throw on duplicate definitions, repeated calls create unnecessary overhead and potential side effects if projection definitions change between instantiations.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts - 2
-
Missing unit test coverage · Line 26-57Both functions lack test coverage. `geojsonDataToFeatureCollection` handles JSON.parse edge cases (lines 36-47) and `dataToFeatureCollection` delegates to OpenLayers parsers. Comprehensive tests should cover: null/undefined geometries, malformed JSON, empty data arrays, and WKB/WKT parsing failures.
-
Repeated GeoJSON format instantiation · Line 64-66In `dataToFeatureCollection`, a new `GeoJSON` format instance is created on every function call (line 64-66). If this function is called repeatedly in a data pipeline, caching the instance would improve performance.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
-
Styling consolidation may affect button visuals · Line 76-76The refactor removes three specialized styled button components (StyledButtonContainer, StyledCloseButton, StyledSaveButton) and consolidates them into a single `StyledButton` with `buttonStyle` prop. The original StyledCloseButton had explicit `box-shadow: none; border-width: 0px; border-style: none; border-color: transparent;` which the new implementation inherits from Button component defaults. Verify visual appearance matches expectations.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx - 1
-
Misleading code comment · Line 165-167The comment at lines 165-167 claims the mutation avoids 'expensive index searches', but no index searches occur in the code. The actual benefit is avoiding DOM queries to find item position. Consider clarifying or removing this misleading comment.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts - 3
-
Object injection sink vulnerability · Line 69-69Dynamic property access `colTypes[cur]` detected as potential object injection sink. Consider validating `cur` before use or using a safer alternative like `Object.hasOwn(colTypes, cur)` check.
-
Object injection sink in locale lookup · Line 120-120Dynamic property access `locale[appLocale]` detected as potential object injection sink. Similar pattern exists at line 127. Validate input before use.
-
Object injection sink in locale key access · Line 127-127Dynamic property access `locale[localeKey]` detected as potential object injection sink (see line 120 for similar issue). Validate `localeKey` before access.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 2
- Wrong extent source for FIT_DATA · Line 261-261
- Dead code / contradictory logic · Line 212-213
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx - 1
- Duplicated SRID extraction between functions · Line 126-161
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts - 1
- Incorrect WKB test expectation · Line 119-130
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
- Code duplication with echarts series utilities · Line 357-378
Review Details
-
Files reviewed - 32 · Commit Range:
a1a632d..a1a632d- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
jansule
force-pushed
the
2026-04-28-only-carto
branch
from
July 20, 2026 12:51
a1a632d to
e49a07b
Compare
Contributor
Author
|
@villebro I know it's been a while, but could you take another look? The failing checks are not related to my changes. |
Contributor
There was a problem hiding this comment.
Code Review Agent Run #5c3657
Actionable Suggestions - 7
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx - 1
- Tag component functionality removed · Line 22-22
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx - 1
- TODO acknowledges incomplete logic · Line 260-260
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts - 1
- Missing unit tests for new function · Line 68-71
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
- WKB parser reuse · Line 78-78
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 2
- Redundant type assignment · Line 331-363
- Missing `data` prop in GeoStyler · Line 690-690
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts - 1
- Missing unit tests for new utilities · Line 1-183
Additional Suggestions - 11
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 2
-
Duplicated extent transform logic · Line 345-352The extent transformation logic is duplicated here and in lines 271-277. Extract to a shared function like `getCurrentExtentIn4326(olMap, view)` to avoid divergence risk when future changes are needed.
-
Duplicate zoom setter effects · Line 440-448Two nearly identical useEffects (440-443 and 445-448) both get the view and set zoom values. Consolidating reduces duplication and improves maintainability.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts - 1
-
WKB SRID extraction missing · Line 103-104The `wkbToGeoJSON` function does not extract SRID from WKB hex input. The WKB string contains embedded SRID data (e.g., `0020E610` marks EPSG:4326), but `featureProjection` alone is specified without `dataProjection`. This causes incorrect reprojection when WKB coordinates use non-EPSG:4326 projections. Compare to `wktToGeoJSON` at line 106-113 which properly extracts and uses SRID.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts - 1
-
Incomplete test assertions for WKB/WKT · Line 322-350Tests for WKB/WKT geometry handling (lines 322-350) only verify `features.length` and `coordinates.length`, but do not confirm that the actual geometry transformation functions (`wkbToGeoJSON`, `wktToGeoJSON`) were invoked or that the resulting geometry objects are structurally correct. Per BITO.md rule [6262], tests should verify actual business logic, not just surface-level property checks. Consider adding: (1) `expect(chartTransformer).toHaveBeenCalledTimes(2)` for each format, or (2) assertions validating the geometry object structure (type, coordinates array) matches expected GeoJSON output from those transformation functions.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx - 2
-
WKB dataProjection missing · Line 122-156`dataRecordsToOlFeatures` sets `dataProjection` only for WKT format (line 143-148) when SRID is extracted. For WKB, `dataProjection` remains unset, defaulting to OpenLayers' implicit default. This differs from `wktToGeoJSON` which explicitly sets `dataProjection: extract.srid ?? 'EPSG:4326'`. Inconsistent projection handling can cause incorrect coordinate transformations.
-
Dead exported function · Line 47-47The function `fitMapToCharts` is defined at line 47 but is no longer imported anywhere after this PR swapped the import to `fitMapToData`. This leaves dead exported code in the module. Either remove it or document why it must remain.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts - 1
-
Semantic duplication in test data · Line 84-188There’s a lot of repetition in the non-time-series test arrays. Consider defining a shared `nonTimeSeriesDataValues` constant for the repeated {my_value, my_count} entries, then building each geometry-specific array by mapping over that constant.
-
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts - 1
-
Case-sensitive WKT detection · Line 101-101The WKT format detection uses case-sensitive `startsWith` on line 101. Standard WKT can be uppercase or lowercase depending on the source system. This could cause false negatives for lowercase WKT values (e.g., 'point(0 0)').
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 1
-
Function defined inside component · Line 180-188`getAppLocale` is defined inside the component function body (lines 180-188). Per BITO.md adaptive rule 12745, imports and helper functions should be at module level unless there's a documented circular dependency. While a language change triggers a page reload (making this functionally safe), moving it outside improves maintainability.
-
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts - 1
-
Missing documentation for type design · Line 25-30The removal of `url` from `BaseLayerConf` (line 25-30) is correct since only WFS/XYZ/WMS layers have URLs, not DATA layers. However, to prevent confusion for future maintainers who might expect `url` to be inherited, a clarifying JSDoc comment on `BaseLayerConf` would be beneficial.
-
-
superset/config.py - 1
-
Stale plugin name in docstring · Line 2869-2871The comment references 'plugin-chart-cartodiagram' as a package name; if the package is renamed this comment becomes stale. Use a version-stable description like 'of the cartodiagram map visualization plugin' instead.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts - 3
- Missing unit test coverage · Line 1-75
- Missing return type annotation · Line 26-26
- Missing return type annotation · Line 59-59
-
superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx - 1
- Inconsistent map projection · Line 417-418
-
superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx - 2
- Type mismatch in styled component · Line 139-146
- Dead code removal verified · Line 105-128
-
superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts - 1
- WKB SRID test coverage gap · Line 119-130
Review Details
-
Files reviewed - 32 · Commit Range:
e49a07b..e49a07b- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentTag.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/MapMaxExtentControl/MapMaxExtentViewControl.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/components/OlChartMap.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/constants.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/controlPanel.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/index.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/plugin/transformProps.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/typeguards.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/types.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/bootstrapDataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/dataUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/geometryUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/layerUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/mapUtil.tsx
- superset-frontend/plugins/plugin-chart-cartodiagram/src/util/transformPropsUtil.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/plugin/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/testData.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/layerUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/mapUtil.test.ts
- superset-frontend/plugins/plugin-chart-cartodiagram/test/util/transformPropsUtil.test.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/FlatLayerTree.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsControl.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerConfigsPopoverContent.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/LayerTreeItem.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/dragDropUtil.tsx
- superset-frontend/src/explore/components/controls/LayerConfigsControl/geoStylerUtil.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/typeguards.ts
- superset-frontend/src/explore/components/controls/LayerConfigsControl/types.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
- superset-frontend/webpack.config.js
- superset/config.py
- superset/views/base.py
-
Files skipped - 2
- superset-frontend/package-lock.json - Reason: Filter setting
- superset-frontend/plugins/plugin-chart-cartodiagram/package.json - Reason: Filter setting
-
Tools
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
jansule
force-pushed
the
2026-04-28-only-carto
branch
2 times, most recently
from
July 28, 2026 13:41
f8e231b to
69ae0ce
Compare
jansule
force-pushed
the
2026-04-28-only-carto
branch
from
July 28, 2026 13:53
69ae0ce to
c114340
Compare
github-actions
Bot
added
the
requires:rebase
3 of 6 tasks
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
This updates the cartodiagram viz type with following additional features:
This reflects the changes that were done in the last year in our superset fork, which we now want to stream back to upstream. This also works as a preparational step for the introduction of a new geo-related visualization type, that will be introduced in a separate PR.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Support for additional geo-data formats WellKnownText (EWKT), WellKnownBinary (EWKB)
Additional controls Map Padding, Min Zoom, Max Zoom
TESTING INSTRUCTIONS
Example Datasets:
GeoJson
EWKT
EWKB
Simply paste the above statements into the SQL-Lab and create three datasets. You can then use these three datasets respectively to first create a pie chart and afterwards the cartodiagram. Make sure to select the right geometry column (
geom) and the right geometry format. If you see the charts on the map, everything works as expected.ADDITIONAL INFORMATION