codeant-ai-for-open-source[bot] commented on code in PR #39906:
URL: https://github.com/apache/superset/pull/39906#discussion_r3194592280
##########
superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/controlPanel.ts:
##########
@@ -367,6 +368,7 @@ const config: ControlPanelConfig = {
{
label: t('Advanced'),
controlSetRows: [
+ [crossFilterColumn],
Review Comment:
**Suggestion:** Adding the `cross_filter_column` control to GeoJSON without
wiring that field into the GeoJSON query/transform path creates a contract
mismatch: users can select a dataset column, but click handling reads only
`data.object.properties[col]` from the parsed GeoJSON blob. When that property
is not embedded in the GeoJSON payload, cross-filtering silently falls back to
the legacy geometry filter and still produces non-matching filters. Update the
GeoJSON query/transform flow to ensure the selected cross-filter dimension is
present on picked features before exposing this control. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Deck.gl GeoJSON cross-filters fail for some dataset schemas.
- ⚠️ Cross-filter column UI may not match GeoJSON properties.
- ⚠️ Users may see legacy geometry filters despite configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Deck.gl GeoJSON chart in the UI that uses a dataset whose
table has a
`geojson` column containing GeoJSON features (without a `sa3_name` property)
and a
separate SQL column `sa3_name`. In the frontend this chart is backed by
`transformProps`
at
`superset-frontend/plugins/preset-chart-deckgl/src/layers/Geojson/transformProps.ts:24-49`,
which parses only the `formData.geojson` column value into GeoJSON features
and does not
merge other SQL columns into `feature.properties`.
2. In the chart's Advanced section, use the newly added
`cross_filter_column` control
(`[crossFilterColumn]` wired in `controlPanel.ts:20-27`, corresponding to
diff line 371)
to select the dataset column `sa3_name`. This control is defined via
`crossFilterColumn`
in `Shared_DeckGL.tsx:185-27`, which reuses `sharedControls.groupby` so it
offers dataset
columns, but `DeckGeoJson`'s `buildQuery.ts:39-65` only ensures the
`geojson` column (and
js/tooltip columns) are queried and is unaware of `cross_filter_column`.
3. Add this chart to a dashboard alongside another chart filtered on the
same logical
dimension (e.g., a bar chart grouped by `sa3_name`). Enable cross-filtering
for the
GeoJSON chart so clicks go through `commonLayerProps` (`common.tsx:50-55`),
which on click
calls `getCrossFilterDataMask({ data, filterState, formData })` at
`common.tsx:118-122`.
4. Click a region on the GeoJSON chart. `getCrossFilterDataMask` dispatches
to
`getGeojsonFilters` in `crossFiltersDataMask.ts:15-76`, which looks up
`formData.cross_filter_column` in `data.object.properties[col]`. Because
`transformProps`
never attached the SQL `sa3_name` column and the GeoJSON payload lacks a
`sa3_name`
property, `dimensionVal` is `null`, triggering the console warning at
`crossFiltersDataMask.ts:48-52` and falling back to the legacy
geometry-based LIKE filter
(`REPLACE(formData.geojson, ' ', '') LIKE '%<coords>%'`). The resulting
filter cannot
match the other chart's `sa3_name` dimension, so the receiving chart shows
"No data"
despite the user having selected a `cross_filter_column`. Note that this
failure only
occurs for columns not present in `feature.properties`; when the GeoJSON
payload includes
the property (as in the test `deck_geojson with cross_filter_column emits
filter from
feature.properties` in `crossFiltersDataMask.test.ts:587-67`), the dimension
filter works
as intended, so the suggestion's claim that selected dimensions can "never"
be emitted is
overstated.
```
</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%2FGeojson%2FcontrolPanel.ts%0A%2A%2ALine%3A%2A%2A%20371%3A371%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Adding%20the%20%60cross_filter_column%60%20control%20to%20GeoJSON%20without%20wiring%20that%20field%20into%20the%20GeoJSON%20query%2Ftransform%20path%20creates%20a%20contract%20mismatch%3A%20users%20can%20select%20a%20dataset%20column%2C%20but%20click%20handling%20reads%20only%20%60data.object.properties%5Bcol%5D%60%20from%20the%20parsed%20GeoJSON%20blob.%20When%20that%20property%20is%20not%20embedded%20in%20the%20GeoJSON%20payload%2C%20cross-filtering%20silently%20falls%20back%20to%20the%20legacy%20geometry%20filter%20and%20still%20produces%20non-matching%20filters.%20Update%20the%20GeoJSON%20query%2Ftransform%20flow%20to%20ensure%20the%20selected%20
cross-filter%20dimension%20is%20present%20on%20picked%20features%20before%20exposing%20this%20control.%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)
| [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%2FGeojson%2FcontrolPanel.ts%0A%2A%2ALine%3A%2A%2A%20371%3A371%0A%2A%2AComment%3A%2A%2A%0A%09%2ALogic%20Error%3A%20Addi
ng%20the%20%60cross_filter_column%60%20control%20to%20GeoJSON%20without%20wiring%20that%20field%20into%20the%20GeoJSON%20query%2Ftransform%20path%20creates%20a%20contract%20mismatch%3A%20users%20can%20select%20a%20dataset%20column%2C%20but%20click%20handling%20reads%20only%20%60data.object.properties%5Bcol%5D%60%20from%20the%20parsed%20GeoJSON%20blob.%20When%20that%20property%20is%20not%20embedded%20in%20the%20GeoJSON%20payload%2C%20cross-filtering%20silently%20falls%20back%20to%20the%20legacy%20geometry%20filter%20and%20still%20produces%20non-matching%20filters.%20Update%20the%20GeoJSON%20query%2Ftransform%20flow%20to%20ensure%20the%20selected%20cross-filter%20dimension%20is%20present%20on%20picked%20features%20before%20exposing%20this%20control.%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/Geojson/controlPanel.ts
**Line:** 371:371
**Comment:**
*Logic Error: Adding the `cross_filter_column` control to GeoJSON
without wiring that field into the GeoJSON query/transform path creates a
contract mismatch: users can select a dataset column, but click handling reads
only `data.object.properties[col]` from the parsed GeoJSON blob. When that
property is not embedded in the GeoJSON payload, cross-filtering silently falls
back to the legacy geometry filter and still produces non-matching filters.
Update the GeoJSON query/transform flow to ensure the selected cross-filter
dimension is present on picked features before exposing this control.
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%2F39906&comment_hash=f3e5ea3d7b60be098bc156b30587390594952e19c275fb49a299c0b3d3d1390f&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F39906&comment_hash=f3e5ea3d7b60be098bc156b30587390594952e19c275fb49a299c0b3d3d1390f&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]
