alex-poor commented on code in PR #39906:
URL: https://github.com/apache/superset/pull/39906#discussion_r3255796796
##########
superset-frontend/plugins/preset-chart-deckgl/src/utils/crossFiltersDataMask.ts:
##########
@@ -408,11 +409,50 @@ const getLineColumnFilters = ({
data: PickingInfo;
}): FilterResult => {
const path = (data?.object?.path || data.object?.polygon) as string;
- const val = JSON.stringify(path);
if (!formData.line_column) throw new Error('Line column is required');
if (!path) throw new Error('Position of picked data is required');
+ // Preferred path: emit on a dimension column the user selected. The value
+ // can land either directly on the picked feature (groupby/excluded keys are
+ // spread by addPropertiesToFeature) or under extraProps when it overlaps
+ // with js_columns (addJsColumnsToExtraProps).
+ if (formData.cross_filter_column) {
+ const col = formData.cross_filter_column;
+ const obj = data.object ?? {};
+ const extraProps = (obj.extraProps ?? {}) as Record<string, unknown>;
+ const dimensionVal = (obj[col] ?? extraProps[col]) as
+ | string
+ | number
+ | boolean
+ | null
+ | undefined;
+
+ if (dimensionVal == null) {
Review Comment:
Not actionable as written. Two reasons:
1. The suggested remediation "remove and re-add the chart" is incorrect.
Superset's actual workflow is: open the chart in Explore → click Run to
re-execute the query → Save. The bot's instruction would have users delete and
re-create their work.
2. This error is thrown inside deck.gl's `onClick` handler, which swallows
exceptions silently (same point that prompted the previous review round — see
Damian's comments). End users never see the message; only developers debugging
in browser DevTools console see it. Padding it with verbose user-facing
remediation steps gives no benefit. The current terse message matches the
file's existing style (`'Line column is required'`, `'Position of picked data
is required'`, etc).
##########
superset-frontend/plugins/preset-chart-deckgl/src/utils/crossFiltersDataMask.ts:
##########
@@ -436,6 +476,40 @@ const getGeojsonFilters = ({
formData: LayerFormData;
data: PickingInfo;
}): FilterResult => {
+ // Preferred path: emit on a property of the picked GeoJSON Feature.
+ if (formData.cross_filter_column) {
+ const col = formData.cross_filter_column;
+ const properties = (data.object?.properties ?? {}) as Record<
+ string,
+ unknown
+ >;
+ const dimensionVal = properties[col] as
+ | string
+ | number
+ | boolean
+ | null
+ | undefined;
+
+ if (dimensionVal == null) {
+ throw new Error(
+ `Cross-filter column "${col}" not present on picked feature
properties`,
Review Comment:
Same as the polygon comment above — error is dev-console-only (swallowed by
deck.gl), and the bot's suggested user-facing remediation is wrong ("remove and
re-add" isn't how you refresh a chart's query). Not changing.
--
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]
