codeant-ai-for-open-source[bot] commented on code in PR #40100:
URL: https://github.com/apache/superset/pull/40100#discussion_r3258842667
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts:
##########
@@ -41,6 +41,60 @@ interface PolygonFeature {
metrics?: Record<string, number | string>;
}
+type PolygonCoordinates = number[][];
+
+function isPlainRecord(value: unknown): value is Record<string, unknown> {
+ return typeof value === 'object' && value !== null && !Array.isArray(value);
+}
+
+function getOwnRecordValue(
+ record: DataRecord,
+ key: string | undefined,
+): string | number | null | undefined {
+ if (!key || !Object.prototype.hasOwnProperty.call(record, key)) {
+ return undefined;
+ }
+
+ return record[key];
+}
+
+function getGeoJsonGeometry(value: unknown): Record<string, unknown> | null {
+ if (!isPlainRecord(value)) {
+ return null;
+ }
+
+ if (value.coordinates) {
+ return value;
+ }
+
+ return isPlainRecord(value.geometry) ? value.geometry : null;
+}
+
+function getPolygonCoordinateParts(
+ value: unknown,
+): PolygonCoordinates[] | null {
+ if (Array.isArray(value)) {
+ return [value as PolygonCoordinates];
+ }
+
+ const geometry = getGeoJsonGeometry(value);
+ if (!geometry?.coordinates || !Array.isArray(geometry.coordinates)) {
+ return null;
+ }
+
+ if (geometry.type === 'MultiPolygon') {
+ return geometry.coordinates.flatMap(polygon =>
+ Array.isArray(polygon)
+ ? [(polygon[0] || polygon) as PolygonCoordinates]
+ : [],
+ );
Review Comment:
**Suggestion:** The MultiPolygon conversion drops interior rings by
selecting only `polygon[0]`, so polygons with holes are rendered as fully
filled shapes. Keep the full ring structure for each polygon part instead of
truncating to the outer ring, otherwise valid GeoJSON topology is lost during
transformation. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Polygon chart misrenders GeoJSON polygons containing interior holes.
- ⚠️ Spatial analyses may ignore excluded internal regions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a deck.gl Polygon chart using `PolygonChartPlugin` defined in
`superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/index.ts:43-51`,
with
`rawFormData.line_type = 'json'` and `rawFormData.line_column = 'geom'` as
in the test
fixture at `transformProps.test.ts:37-41`.
2. Ensure the query returns a row where the `geom` column is a JSON string
representing a
GeoJSON `MultiPolygon` whose first polygon contains an outer ring and at
least one inner
ring (hole), e.g.
`{"type":"MultiPolygon","coordinates":[[[[x1,y1],...outer...],[...[hole]...]]]}`.
3. When the chart runs, `PolygonChartPlugin` calls `transformProps` from
`./transformProps` (import at `index.ts:25`), which invokes
`processPolygonData` in
`transformProps.ts:98-101`; for `line_type: 'json'` this calls
`getPolygonCoordinateParts(parsed)` at `transformProps.ts:162-168`.
4. Inside `getPolygonCoordinateParts` (`transformProps.ts:73-95`), the
`MultiPolygon`
branch at lines `85-90` executes `geometry.coordinates.flatMap(polygon =>
Array.isArray(polygon) ? [(polygon[0] || polygon) as PolygonCoordinates] :
[])`, so for
each polygon only `polygon[0]` (the outer ring) is returned and all
subsequent rings
(holes) are dropped, resulting in `polygonCoordParts` that no longer contain
inner rings,
and the resulting features rendered by `Polygon.tsx` (PolygonLayer
`getPolygon` accessor
at `Polygon.tsx:8-19` display filled polygons instead of polygons with holes.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fpreset-chart-deckgl%2Fsrc%2Flayers%2FPolygon%2FtransformProps.ts%0A%2A%2ALine%3A%2A%2A%2086%3A90%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20MultiPolygon%20conversion%20drops%20interior%20rings%20by%20selecting%20only%20%60polygon%5B0%5D%60%2C%20so%20polygons%20with%20holes%20are%20rendered%20as%20fully%20filled%20shapes.%20Keep%20the%20full%20ring%20structure%20for%20each%20polygon%20part%20instead%20of%20truncating%20to%20the%20outer%20ring%2C%20otherwise%20valid%20GeoJSON%20topology%20is%20lost%20during%20transformation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%2
0check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt=This%20is%20a%20comment%20left%20during%20a%20code%20review.%0A%0A%2A%2APath%3A%2A%2A%20superset-frontend%2Fplugins%2Fpreset-chart-deckgl%2Fsrc%2Flayers%2FPolygon%2FtransformProps.ts%0A%2A%2ALine%3A%2A%2A%2086%3A90%0A%2A%2AComment%3A%2A%2A%0A%09%2AIncomplete%20Implementation%3A%20The%20MultiPolygon%20conversion%20drops%20interior%20rings%20by%20selecting%20only%20%60polygon%5B0%5D%60%2C%20so%20polygons%20with%20holes%20are%20rendered%20as%20fully%20filled%20shapes.%20Keep%20the%20full%20ring%20structure%20for%20each%20polygon%20part%20instead%20of%20truncating%20to%20the%20outer%20ring%2C%20otherwise%20valid%20GeoJSO
N%20topology%20is%20lost%20during%20transformation.%0A%0AValidate%20the%20correctness%20of%20the%20flagged%20issue.%20If%20correct%2C%20How%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20implement%20it%20and%20please%20make%20it%20concise.%0AOnce%20fix%20is%20implemented%2C%20also%20check%20other%20comments%20on%20the%20same%20PR%2C%20and%20ask%20user%20if%20the%20user%20wants%20to%20fix%20the%20rest%20of%20the%20comments%20as%20well.%20if%20said%20yes%2C%20then%20fetch%20all%20the%20comments%20validate%20the%20correctness%20and%20implement%20a%20minimal%20fix%0A)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/preset-chart-deckgl/src/layers/Polygon/transformProps.ts
**Line:** 86:90
**Comment:**
*Incomplete Implementation: The MultiPolygon conversion drops interior
rings by selecting only `polygon[0]`, so polygons with holes are rendered as
fully filled shapes. Keep the full ring structure for each polygon part instead
of truncating to the outer ring, otherwise valid GeoJSON topology is lost
during transformation.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40100&comment_hash=8c958ab776a7f4e84b5370b7be367fa86675b76895f09da93f1cb7c81d6ad064&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40100&comment_hash=8c958ab776a7f4e84b5370b7be367fa86675b76895f09da93f1cb7c81d6ad064&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
