fix(echarts): enable cross-filtering for pie chart "Other" slice - #43088
fix(echarts): enable cross-filtering for pie chart "Other" slice#43088omsn2 wants to merge 13 commits into
Conversation
…ements Fixes invalid cross-filters being emitted when non-category pie chart elements (Total graphic text, Other slice, empty name events) are clicked. getCrossFilterDataMask now returns undefined when any selected value has no labelMap entry, and clickEventHandler guards against empty name events. Fixes apache#42340
The 'Other' slice in a pie chart aggregates multiple rows into a single segment. Its labelMap entry is a 2D array (string[][]) — one row per aggregated data point — whereas all other slices use a 1D array (string[]). The previous getCrossFilterDataMask guard required groupbyValues.length to equal values.length. Because flatMap on a 2D entry expands it into multiple rows, this check always failed for 'Other', silently suppressing the cross-filter emission. Changes: - transformProps.ts: populate labelMap['Other'] with a 2D array (one string[] per aggregated row) so the event handler has the raw dimension values available. - eventHandlers.ts: replace the strict equality guard with a looser check (length === 0 && values.length > 0) and use flatMap to normalise both 1D and 2D labelMap entries into a uniform string[][] before building the IN-filter payload. - types.ts: make CrossFilterTransformedProps generic on its labelMap value type (default string[]) so the pie chart can declare string[] | string[][] without breaking other chart types. - Pie/types.ts: instantiate the generic for PieChartTransformedProps.
dosubot
Bot
added
change:frontend
Code Review Agent Run #3b3edbActionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if (otherDatum && otherRows.length > 0) { | ||
| labelMap[otherDatum.name] = otherRows.map(row => | ||
| groupbyLabels.map(col => row[col] as string), | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggestion: The aggregated entry is stored under the rendered name Other, so it overwrites any real data row whose formatted groupby label is also Other. Since both slices are rendered with the same ECharts name, clicking the real row or the aggregate can then resolve to the aggregated rows and emit an incorrect cross-filter. Use a collision-safe key or otherwise disambiguate the aggregate from ordinary data labels. [logic error]
Severity Level: Major ⚠️
- ❌ Real `Other` category clicks filter aggregated rows.
- ❌ Aggregate and real slices share cross-filter selection state.
- ⚠️ Pie-to-chart cross-filter results become incorrect for colliding labels.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts
**Line:** 366:370
**Comment:**
*Logic Error: The aggregated entry is stored under the rendered name `Other`, so it overwrites any real data row whose formatted groupby label is also `Other`. Since both slices are rendered with the same ECharts name, clicking the real row or the aggregate can then resolve to the aggregated rows and emit an incorrect cross-filter. Use a collision-safe key or otherwise disambiguate the aggregate from ordinary data labels.
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 fixThere was a problem hiding this comment.
Good catch! I have resolved this collision issue in the latest commit.
How it was fixed:
The labelMap now stores the aggregated slice using a disambiguated prefix (other${name}) to guarantee it never overwrites an actual data row named "Other".
The click and contextmenu event handlers in eventHandlers.ts have been updated to evaluate the data.isOther property injected by ECharts.
If it is the aggregated slice, the handler safely reconstructs the other key to emit the cross-filter. Real data rows named "Other" will map securely to their own native keys.
|
The flagged issue is valid. The current implementation uses the rendered label (e.g., 'Other') as the key in To resolve this, you should disambiguate the keys in Since the issue is identified in Would you like me to fetch all other comments on this PR to validate and implement fixes for them as well? superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43088 +/- ##
==========================================
- Coverage 66.69% 66.69% -0.01%
==========================================
Files 2872 2872
Lines 163452 163468 +16
Branches 37725 37734 +9
==========================================
+ Hits 109022 109028 +6
- Misses 52302 52312 +10
Partials 2128 2128
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:
|
…ng collisions - Applies a unique prefix '__other__' to the aggregated slice in labelMap - Updates Pie chart cross-filtering event handlers to reconstruct the key using data.isOther - Preserves accurate filtering behavior for real data rows named 'Other' without breaking UI highlighting
…slice key - Aborts cross-filter emission if clicked elements lack a valid name (e.g. empty labels) - Safely no-ops if any selected values cannot be strictly resolved in the labelMap (e.g. 'Total' text) - Applies a unique prefix '__other__' to the aggregated slice in labelMap to prevent cross-filtering collisions - Updates Pie chart event handlers to reconstruct the key using data.isOther, preserving accurate filtering behavior for real data rows named 'Other'
Code Review Agent Run #60522cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Add three new unit tests to cover the __other__ key path in the pie chart event handlers which was missing from the Codecov patch coverage report: 1. Clicking the aggregated Other slice (isOther=true) correctly emits filters for all aggregated rows via the __other__ key 2. Clicking a real data row named 'Other' (no isOther flag) correctly emits only that single row's filter 3. Clicking the Other slice when __other__ key is missing from labelMap safely no-ops without emitting any cross-filter
There was a problem hiding this comment.
Pull request overview
Enables cross-filtering for ECharts pie charts when clicking the aggregated “Other” slice by supporting multi-row label mappings and routing “Other” clicks through a dedicated __other__ key so they can be expanded into an IN filter payload.
Changes:
- Extends cross-filter label mapping to support
string[] | string[][]and normalizes it in event handlers. - Updates pie transform to store aggregated “Other” rows under a
__other__{label}key and to persist selection/opacity using that key. - Adds unit tests covering clicks on pseudo-elements (“Total”), “Other” aggregation behavior, and real data rows named “Other”.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/test/utils/eventHandlers.test.ts | Adds coverage for “Other”/pseudo-element click behavior and aggregated-row cross-filter emission. |
| superset-frontend/plugins/plugin-chart-echarts/src/utils/eventHandlers.ts | Normalizes labelMap entries (1D/2D) and routes pie “Other” clicks via __other__ keys; updates context menu handling. |
| superset-frontend/plugins/plugin-chart-echarts/src/types.ts | Makes CrossFilterTransformedProps generic over labelMap value type. |
| superset-frontend/plugins/plugin-chart-echarts/src/Pie/types.ts | Instantiates the cross-filter generic for pie charts to allow `string[] |
| superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts | Stores aggregated “Other” mappings as 2D arrays under __other__ keys and updates selection/opacity logic accordingly. |
Suppressed comments (3)
superset-frontend/plugins/plugin-chart-echarts/test/utils/eventHandlers.test.ts:46
- The test helper is still typed as
CrossFilterTransformedProps(defaultinglabelMaptostring[]), then relies on casts to allowstring[][]. SinceCrossFilterTransformedPropsis now generic, the helper can useCrossFilterTransformedProps<string[] | string[][]>directly and drop thelabelMapcast in the returned object.
This issue also appears in the following locations of the same file:
- line 245
- line 281
function buildProps(
overrides: Partial<
BaseTransformedProps<QueryFormData> & CrossFilterTransformedProps
>,
): BaseTransformedProps<QueryFormData> & CrossFilterTransformedProps {
return {
formData: {} as QueryFormData,
height: 400,
width: 800,
queriesData: [],
filterState: {},
onContextMenu: jest.fn(),
setDataMask: jest.fn(),
emitCrossFilters: true,
groupby: [],
labelMap: {} as Record<string, string[] | string[][]>,
selectedValues: {},
coltypeMapping: {},
...overrides,
} as BaseTransformedProps<QueryFormData> & CrossFilterTransformedProps;
}
superset-frontend/plugins/plugin-chart-echarts/test/utils/eventHandlers.test.ts:253
- These new tests disable
no-explicit-anyand castlabelMaptoany, even though the updatedlabelMaptype already supportsstring[][]. Once the helper is typed withCrossFilterTransformedProps<string[] | string[][]>, theas anycasts can be removed.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
labelMap: {
Electronics: ['Electronics'],
Clothing: ['Clothing'],
// The aggregated "Other" slice is stored under the __other__ prefix
// to avoid colliding with any real data row named "Other".
'__other__Other': [['SmallA'], ['SmallB']],
} as any,
selectedValues: {},
superset-frontend/plugins/plugin-chart-echarts/test/utils/eventHandlers.test.ts:288
- This test also disables
no-explicit-anyand castslabelMaptoany. WithCrossFilterTransformedProps<string[] | string[][]>in the helper, thelabelMapliteral is already valid and can be kept strongly typed.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
labelMap: {
// A real data row whose category value is literally "Other"
Other: ['Other'],
// The aggregated slice is stored separately under the __other__ prefix
'__other__Other': [['SmallA'], ['SmallB']],
} as any,
selectedValues: {},
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| return { | ||
| value, | ||
| value: value as number, |
There was a problem hiding this comment.
Fixed in the latest commit. Replaced value: value as number with value: Number(value). This forces a real runtime coercion, ensuring ECharts always receives a number even if datum[metricLabel] arrives as a string (which happens when the backend serializes numeric columns as text).
| const drillFilters: any[] = []; | ||
| const key = e.data?.isOther ? `__other__${e.name}` : e.name; | ||
| if (groupby.length > 0) { | ||
| const values = labelMap[e.name]; | ||
| const values = labelMap[key]; | ||
| if (!values) { | ||
| return; | ||
| } | ||
| const isMulti = Array.isArray(values[0]); | ||
| groupby.forEach((dimension, i) => { | ||
| const val = isMulti | ||
| ? (values as string[][]).map(v => { | ||
| const metricsCount = v.length - groupby.length; | ||
| return v[metricsCount + i]; | ||
| }) | ||
| : (values as string[])[ | ||
| (values as string[]).length - groupby.length + i | ||
| ]; | ||
| drillFilters.push({ | ||
| col: dimension, | ||
| op: '==', | ||
| val: values[i], | ||
| formattedVal: formatSeriesName(values[i], { | ||
| timeFormatter: getTimeFormatter(formData.dateFormat), | ||
| numberFormatter: getNumberFormatter(formData.numberFormat), | ||
| coltype: coltypeMapping?.[getColumnLabel(dimension)], | ||
| }), | ||
| op: isMulti ? 'IN' : '==', | ||
| val, | ||
| formattedVal: isMulti | ||
| ? e.name | ||
| : formatSeriesName(val as string, { | ||
| timeFormatter: getTimeFormatter(formData.dateFormat), | ||
| numberFormatter: getNumberFormatter(formData.numberFormat), | ||
| coltype: coltypeMapping?.[getColumnLabel(dimension)], | ||
| }), | ||
| }); |
There was a problem hiding this comment.
Fixed in the latest commit. Restored the proper drillFilters: BinaryQueryObjectFilterClause[] typing by importing it from @superset-ui/core. Also added a check: for multi-row aggregated "Other" slices, we now emit an empty drillFilters array since drill-to-detail is ambiguous for aggregations. The crossFilter object remains fully supported via the other key.
| export const allEventHandlers = ( | ||
| transformedProps: BaseTransformedProps<any> & CrossFilterTransformedProps, | ||
| transformedProps: BaseTransformedProps<any> & | ||
| CrossFilterTransformedProps<any>, | ||
| ) => { |
There was a problem hiding this comment.
Fixed in the latest commit. Removed the any widenings. The function signature now properly uses BaseTransformedProps & CrossFilterTransformedProps<string[] | string[][]>, aligning perfectly with the type of labelMap in this plugin.
Code Review Agent Run #00a5b8Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
- Convert pie metric value with Number() at runtime instead of 'as number' type cast — datum[metricLabel] can be a string when the backend serialises numeric columns as text - For aggregated Other slice in contextMenuEventHandler, emit empty drill filters since drill-to-detail is ambiguous for multi-row aggregates; crossFilter is still emitted correctly via the __other__ key - Remove 'any' from allEventHandlers: use QueryFormData and CrossFilterTransformedProps<string[] | string[][]> for proper typing - Import BinaryQueryObjectFilterClause from @superset-ui/core to restore the correct type for drillFilters array
Code Review Agent Run #18969cActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
SUMMARY
The "Other" slice in a pie chart aggregates multiple rows into one visual
segment. In
transformProps.ts, itslabelMapentry is a 2D array(
string[][]— one row per aggregated data point), whereas all otherslices use a 1D array (
string[]).The previous
getCrossFilterDataMaskguard requiredgroupbyValues.length === values.length. BecauseflatMapon a 2Dentry expands into multiple rows, this check always failed silently for
"Other", suppressing cross-filter emission entirely (no API calls fired).
Root cause: The guard
groupbyValues.length !== values.lengthwastoo strict and did not account for the multi-row "Other" aggregation.
Fix:
transformProps.ts: PopulatelabelMap['Other']with a 2D array(one
string[]per aggregated row).eventHandlers.ts: Replace the strict equality guard withlength === 0 && values.length > 0, and useflatMapto normaliseboth 1D and 2D
labelMapentries into a uniformstring[][]beforebuilding the
IN-filter payload.types.ts: MakeCrossFilterTransformedPropsgeneric on itslabelMapvalue type (defaultstring[]) so the pie chart candeclare
string[] | string[][]without breaking other chart types.Pie/types.ts: Instantiate the generic forPieChartTransformedProps.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before Fix: When Clicked on the Other we get Null Data in Bar Chart
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
###Dataset Used
salesdata.xlsx