Copilot commented on code in PR #37407:
URL: https://github.com/apache/superset/pull/37407#discussion_r2728777259
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -252,12 +312,18 @@ export default function EchartsTimeseries({
});
});
+ // Provide cross-filter for dimensions OR categorical X-axis (issue
#25334)
+ let crossFilter;
+ if (hasDimensions) {
+ crossFilter = getCrossFilterDataMask(seriesName);
+ } else if (canCrossFilterByXAxis && data) {
+ crossFilter = getXAxisCrossFilterDataMask(data[0]);
Review Comment:
There's no validation that the X-axis value exists before using it. If data
is an empty array or data[0] is undefined, the code will convert undefined to
the string "undefined" and attempt to filter on it, which will likely cause
incorrect behavior.
Consider adding a check to ensure the X-axis value is not undefined before
proceeding with the cross-filter operation.
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -154,6 +154,43 @@ export default function EchartsTimeseries({
[groupby, labelMap, selectedValues],
);
+ // Cross-filter using X-axis value when no dimensions are set (issue #25334)
+ const getXAxisCrossFilterDataMask = useCallback(
+ (xAxisValue: string | number) => {
+ const stringValue = String(xAxisValue);
+ const selected: string[] = Object.values(selectedValues);
+ let values: string[];
+ if (selected.includes(stringValue)) {
+ values = selected.filter(v => v !== stringValue);
+ } else {
+ values = [stringValue];
+ }
+ return {
+ dataMask: {
+ extraFormData: {
+ filters:
+ values.length === 0
+ ? []
+ : [
+ {
+ col: xAxis.label,
+ op: 'IN' as const,
+ val: values,
+ },
+ ],
+ },
+ filterState: {
+ label: values.length ? values : undefined,
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: selected.includes(stringValue),
+ };
+ },
+ [selectedValues, xAxis.label],
+ );
Review Comment:
When X-axis cross-filtering is active, the selectedValues will contain
X-axis values (e.g., "Product A") rather than series names (e.g., "Sales").
This causes a visual feedback issue in transformers.ts (around line 241) where
the code checks `!filterState?.selectedValues.includes(name)` to determine if a
series should be dimmed.
Since series names (metric names like "Sales") won't match X-axis values
(like "Product A"), all series will be incorrectly marked as filtered and
rendered with semi-transparent opacity. This doesn't break the actual filtering
functionality (which happens via extraFormData), but provides incorrect visual
feedback to users - all bars/lines will appear dimmed when X-axis filtering is
active.
Consider either:
1. Disabling the opacity-based visual feedback when using X-axis filtering
(by not setting selectedValues in filterState for X-axis filters)
2. Implementing a different visual feedback mechanism for X-axis filtering
3. Adding a flag to filterState to distinguish between dimension-based and
X-axis-based filtering
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -154,6 +154,43 @@ export default function EchartsTimeseries({
[groupby, labelMap, selectedValues],
);
+ // Cross-filter using X-axis value when no dimensions are set (issue #25334)
+ const getXAxisCrossFilterDataMask = useCallback(
+ (xAxisValue: string | number) => {
+ const stringValue = String(xAxisValue);
+ const selected: string[] = Object.values(selectedValues);
+ let values: string[];
+ if (selected.includes(stringValue)) {
+ values = selected.filter(v => v !== stringValue);
+ } else {
+ values = [stringValue];
+ }
+ return {
+ dataMask: {
+ extraFormData: {
+ filters:
+ values.length === 0
+ ? []
+ : [
+ {
+ col: xAxis.label,
+ op: 'IN' as const,
+ val: values,
+ },
+ ],
+ },
+ filterState: {
+ label: values.length ? values : undefined,
+ value: values.length ? values : null,
+ selectedValues: values.length ? values : null,
+ },
+ },
+ isCurrentValueSelected: selected.includes(stringValue),
Review Comment:
Converting the X-axis value to a string may cause filtering issues when the
X-axis column contains numeric values. The filter value type should match the
column's data type. For example, if the category column contains years as
numbers (2020, 2021, etc.), converting them to strings ('2020', '2021') may not
match the column type in the database, causing the filter to fail.
Consider preserving the original type of xAxisValue instead of converting it
to a string. The selectedValues comparison on line 163 could use String() for
comparison purposes, but the actual filter value should retain its original
type.
```suggestion
const selected = Object.values(
selectedValues,
) as (string | number)[];
const selectedStrings = selected.map(v => String(v));
let filterValues: (string | number)[];
let stateValues: string[];
if (selectedStrings.includes(stringValue)) {
// Deselect current value
filterValues = selected.filter(v => String(v) !== stringValue);
stateValues = selectedStrings.filter(v => v !== stringValue);
} else {
// Select current value
filterValues = [...selected, xAxisValue];
stateValues = [...selectedStrings, stringValue];
}
return {
dataMask: {
extraFormData: {
filters:
filterValues.length === 0
? []
: [
{
col: xAxis.label,
op: 'IN' as const,
val: filterValues,
},
],
},
filterState: {
label: stateValues.length ? stateValues : undefined,
value: stateValues.length ? stateValues : null,
selectedValues: stateValues.length ? stateValues : null,
},
},
isCurrentValueSelected: selectedStrings.includes(stringValue),
```
##########
superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/EchartsTimeseries.tsx:
##########
@@ -164,18 +201,41 @@ export default function EchartsTimeseries({
[emitCrossFilters, setDataMask, getCrossFilterDataMask],
);
+ // Handle cross-filter using X-axis value when no dimensions (issue #25334)
+ const handleXAxisChange = useCallback(
+ (xAxisValue: string | number) => {
+ if (!emitCrossFilters) {
+ return;
+ }
+ setDataMask(getXAxisCrossFilterDataMask(xAxisValue).dataMask);
+ },
+ [emitCrossFilters, setDataMask, getXAxisCrossFilterDataMask],
+ );
+
+ // Determine if X-axis can be used for cross-filtering (categorical axis
without dimensions)
+ const canCrossFilterByXAxis =
+ !hasDimensions && xAxis.type === AxisType.Category;
+
const eventHandlers: EventHandlers = {
click: props => {
- if (!hasDimensions) {
+ // Allow cross-filter by dimensions OR by categorical X-axis (issue
#25334)
+ if (!hasDimensions && !canCrossFilterByXAxis) {
return;
}
if (clickTimer.current) {
clearTimeout(clickTimer.current);
}
// Ensure that double-click events do not trigger single click event. So
we put it in the timer.
clickTimer.current = setTimeout(() => {
- const { seriesName: name } = props;
- handleChange(name);
+ if (hasDimensions) {
+ // Cross-filter by dimension (original behavior)
+ const { seriesName: name } = props;
+ handleChange(name);
+ } else if (canCrossFilterByXAxis && props.data) {
+ // Cross-filter by X-axis value when no dimensions (issue #25334)
+ const xAxisValue = props.data[0];
+ handleXAxisChange(xAxisValue);
Review Comment:
There's no validation that the X-axis value exists before using it. If
props.data is an empty array or data[0] is undefined, the code will convert
undefined to the string "undefined" and attempt to filter on it, which will
likely cause incorrect behavior.
Consider adding a check to ensure the X-axis value is not undefined before
proceeding with the cross-filter operation.
```suggestion
} else if (
canCrossFilterByXAxis &&
Array.isArray(props.data) &&
props.data.length > 0
) {
// Cross-filter by X-axis value when no dimensions (issue #25334)
const xAxisValue = props.data[0];
if (xAxisValue !== undefined && xAxisValue !== null) {
handleXAxisChange(xAxisValue);
}
```
--
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]
