You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "rusackas (via GitHub)" <gi...@apache.org> on 2026/06/26 05:35:20 UTC
[PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
rusackas opened a new pull request, #41442:
URL: https://github.com/apache/superset/pull/41442
### SUMMARY
Follow-up to #31477 (cc @gerbermichi π), which added the **Show Null Values** control to the Sunburst chart. That PR shipped the feature, but a couple of touch-ups flagged in review never made it in. This finishes them off and, importantly, makes the change **non-breaking for charts that already exist**.
Three distinct issues:
**1. Legacy charts silently stopped showing nulls (the breaking part).**
A chart saved before this control existed has no `showNullValues` key in its form data, so it read as `undefined`. Since `transformProps` passes `!showNullValues` to `treeBuilder`, `!undefined === true` forced filtering **on** β hiding null slices that used to render, which is the *opposite* of the control's `default: true`. New chart created today β shows nulls; identical chart saved last week β silently hides them after upgrade. Fixed by defaulting `showNullValues = true` in the destructure so legacy form data preserves the old behavior.
**2. Single-level and root-level nulls slipped through.**
The filter only ran inside the multi-level (`else`) branch, applied to a node's `children`. So:
- a single-column sunburst never filtered at all (every node is built in the leaf branch), and
- a `null` in the **root** column was never removed (the top-level array has no parent to filter it).
Now the filter is applied once at the end of every recursion level, covering roots, leaves, and nested children uniformly.
**3. Hidden nulls left an empty gap in the ring.**
Children were filtered *after* the parent reduced over them, so the parent arc kept the hidden null's value while one outer slice vanished β an angular gap. Children are now filtered *before* the reduce, so a parent's `value`/`secondaryValue` always match its visible children.
### Scenarios β what is and isn't a behavior change
| Chart | Before this PR | After this PR |
|---|---|---|
| No null values | unchanged | unchanged |
| New chart (default "show") | shows nulls | shows nulls |
| **Legacy multi-level chart with inner-layer nulls** | #31477 silently hid them | **shows them again** (regression fixed) |
| Legacy single-level chart with nulls | toggle was inert | shows nulls by default; toggle now works |
| Legacy multi-level chart with root nulls | root null slipped through | shows by default; toggle now removes it |
| Any chart with "Show Null Values" unchecked | partially worked / left gaps | nulls removed cleanly, no gap |
Net effect: the only *existing* charts that rendered differently under #31477 were multi-level sunbursts with nulls in a non-root layer (they lost those slices). This PR restores them and makes the toggle behave consistently everywhere.
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A β logic/behavior fix. Covered by unit tests below.
### TESTING INSTRUCTIONS
- `cd superset-frontend && npx jest plugins/plugin-chart-echarts/test/utils/treeBuilder.test.ts plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts`
- New/updated tests cover: legacy `undefined` default keeps nulls; single-level filtering on/off; root-level filtering on/off; nested filtering excludes hidden nulls from parent totals (incl. secondary metric).
- Manual: create a sunburst with a single column containing nulls, and a multi-column one with nulls at the root and inner layers; toggle **Show Null Values** and confirm nulls appear/disappear at every level with no gaps.
### ADDITIONAL INFORMATION
- [x] Changes UI (behavior of an existing control)
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Includes DB Migration
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
π€ Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41442:
URL: https://github.com/apache/superset/pull/41442#issuecomment-4806675605
<!-- Bito Reply -->
The suggestion to replace the `any` cast is correct, as it improves type safety in the test suite. You can resolve this by defining a narrow interface that matches the structure of the ECharts options object used in the test, or by importing the existing `EChartsOption` type from the ECharts library and extending it if necessary.
To implement this, you can define an interface for the series data structure:
```typescript
interface SunburstSeriesData {
value: number;
children?: SunburstSeriesData[];
}
interface SunburstEchartsOption {
series: {
data: SunburstSeriesData[];
}[];
}
```
Then, cast the `echartOptions` to this interface instead of `any`:
```typescript
const seriesValues = (props: ChartProps) =>
(
(transformProps(props as EchartsSunburstChartProps).echartOptions as SunburstEchartsOption)
.series[0].data
)
.map(node => node.value)
.sort((a, b) => a - b);
```
There are no other review comments on this pull request. Would you like me to check for any other potential type safety improvements in this file?
**superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts**
```
interface SunburstSeriesData {
value: number;
children?: SunburstSeriesData[];
}
interface SunburstEchartsOption {
series: {
data: SunburstSeriesData[];
}[];
}
```
**superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts**
```
const seriesValues = (props: ChartProps) =>
(
(transformProps(props as EchartsSunburstChartProps).echartOptions as SunburstEchartsOption)
.series[0].data
)
.map(node => node.value)
.sort((a, b) => a - b);
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #41442:
URL: https://github.com/apache/superset/pull/41442#issuecomment-4806715250
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/41442?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 64.48%. Comparing base ([`7e17c70`](https://app.codecov.io/gh/apache/superset/commit/7e17c70cba265080f4d2f86e334064ee773e449c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`e89d2c7`](https://app.codecov.io/gh/apache/superset/commit/e89d2c7dcec91963dc3d3d6cf88c3d210073c380?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #41442 +/- ##
==========================================
- Coverage 64.49% 64.48% -0.01%
==========================================
Files 2664 2664
Lines 146080 146079 -1
Branches 33696 33696
==========================================
- Hits 94209 94203 -6
- Misses 50163 50168 +5
Partials 1708 1708
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/41442/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Ξ | |
|---|---|---|
| [javascript](https://app.codecov.io/gh/apache/superset/pull/41442/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `68.91% <100.00%> (-0.01%)` | :arrow_down: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/superset/pull/41442?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #41442:
URL: https://github.com/apache/superset/pull/41442
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3479372925
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts:
##########
@@ -90,4 +90,7 @@ export function treeBuilder(
},
[] as TreeNode[],
);
+ // Filter at every level so single-level charts and root nodes are covered,
+ // not just nested children.
+ return filterNullNames ? nodes.filter(node => node.name !== null) : nodes;
Review Comment:
**Suggestion:** The new null-filter only removes nodes whose own `name` is null, but it keeps non-null parent nodes even when all their children were filtered out. That creates zero-valued parent nodes (`value = 0`, `secondaryValue = 0`), and Sunburst later divides `secondaryValue / value` for coloring/tooltip percentages, producing `NaN` in secondary-metric mode. When `filterNullNames` is enabled, also drop parent nodes that end up with no children (or zero aggregate value) so hidden-null branches are removed entirely. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β Sunburst secondary-metric colors receive NaN ratio inputs.
- β Tooltips can display NaN% comparison values to users.
- β οΈ Hidden-null branches leave meaningless zero-value parent nodes.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure a Sunburst chart using at least two group-by columns (e.g. `columns = [colA,
colB]`) and both a primary and secondary metric, so that `secondaryMetric` and `metric`
differ; this drives `secondaryMetricLabel` and `metricLabel` in
`Sunburst/transformProps.ts` lines 129β132.
2. Provide data where, for at least one top-level group (e.g. `colA = 'foo'`), all rows
under that group have `colB = null`. In `treeBuilder.ts` lines 41β48, the recursive call
`treeBuilder(value, restGroupby, ...)` with `filterNullNames` true (see next step) builds
only leaf nodes whose `name` is `null`, and the final filter at lines 93β95 removes them,
returning an empty `children` array for that branch.
3. Turn off the "Show Null Values" control for this Sunburst. In
`Sunburst/transformProps.ts` lines 71β74, `showNullValues` is read from `formData`, and
the call to `treeBuilder` at lines 134β139 passes `!showNullValues` as the
`filterNullNames` flag. With the control off, `filterNullNames` is `true`, so the
recursive call returns no children and the parent node (e.g. `name = 'foo'`) is still
pushed in `treeBuilder.ts` lines 72β88 with `children = []`, `value = 0`, and
`secondaryValue = 0`.
4. Render the chart with a secondary metric color encoding. In
`Sunburst/transformProps.ts`, `getLinearDomain` is called at lines 155β159 with callback
`node => node.secondaryValue / node.value`, and later `linearColorScale(secondaryValue /
value)` is used for `itemStyle.color` at lines 191β200, while `formatTooltip` computes
`compareValuePercentage = node.secondaryValue / node.value` at lines 7β10. For the
zero-valued parent node created in step 3, `secondaryValue / value` evaluates to `0 / 0`,
yielding `NaN`, which propagates into the domain computation and tooltip percentage,
producing invalid ratios (and potentially NaN-based colors) even though the only effect
intended by `filterNullNames` was to hide null children.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4db29b1265864f4193bc1ccdb8033388&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4db29b1265864f4193bc1ccdb8033388&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/plugin-chart-echarts/src/utils/treeBuilder.ts
**Line:** 93:95
**Comment:**
*Incorrect Condition Logic: The new null-filter only removes nodes whose own `name` is null, but it keeps non-null parent nodes even when all their children were filtered out. That creates zero-valued parent nodes (`value = 0`, `secondaryValue = 0`), and Sunburst later divides `secondaryValue / value` for coloring/tooltip percentages, producing `NaN` in secondary-metric mode. When `filterNullNames` is enabled, also drop parent nodes that end up with no children (or zero aggregate value) so hidden-null branches are removed entirely.
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%2F41442&comment_hash=c4cbd549d35af0ace73bfd82d8b33856393aedf0991d663eb4935ee209a358e6&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41442&comment_hash=c4cbd549d35af0ace73bfd82d8b33856393aedf0991d663eb4935ee209a358e6&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "aminghadersohi (via GitHub)" <gi...@apache.org>.
aminghadersohi commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3481432577
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts:
##########
@@ -90,4 +90,13 @@ export function treeBuilder(
},
[] as TreeNode[],
);
+ // Filter at every level so single-level charts and root nodes are covered,
+ // not just nested children. A parent whose children were all null-filtered
+ // is dropped too: keeping it would leave a zero-value arc that yields a NaN
+ // secondaryValue/value ratio for coloring and tooltips.
+ return filterNullNames
+ ? nodes.filter(
+ node => node.name !== null && !(node.children && !node.children.length),
Review Comment:
NIT: `!(node.children && !node.children.length)` is equivalent to `node.children?.length !== 0` β the optional-chaining form is shorter and its intent (keep leaves, drop empty parents) is easier to read at a glance.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3479710787
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts:
##########
@@ -90,4 +90,7 @@ export function treeBuilder(
},
[] as TreeNode[],
);
+ // Filter at every level so single-level charts and root nodes are covered,
+ // not just nested children.
+ return filterNullNames ? nodes.filter(node => node.name !== null) : nodes;
Review Comment:
Good catch, this is real. A parent whose children all got null-filtered was kept as a zero-value arc, so `secondaryValue / value` came out NaN. Dropping those empty parents too now, with a regression test.
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts:
##########
@@ -51,3 +51,49 @@ test('series label has no textBorderColor or textBorderWidth', () => {
expect(series.label).not.toHaveProperty('textBorderColor');
expect(series.label).not.toHaveProperty('textBorderWidth');
});
+
+const nullValueProps = (showNullValues?: boolean) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ columns: ['category'],
+ metric: 'sum__value',
+ ...(showNullValues === undefined ? {} : { showNullValues }),
+ },
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__value: 10 },
+ { category: 'B', sum__value: 20 },
+ { category: null, sum__value: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+
+const seriesValues = (props: ChartProps) =>
+ (
+ (transformProps(props as EchartsSunburstChartProps).echartOptions as any)
Review Comment:
Fair, dropped the `any` casts in favor of a small typed `firstSeries` helper.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41442:
URL: https://github.com/apache/superset/pull/41442#issuecomment-4811624035
**Bito Automatic Review Skipped β PR Already Merged**<br/><br/>Bito scheduled an automatic review for this pull request, but the review was skipped because this PR was merged before the review could be run.<br/>No action is needed if you didn't intend to review it. To get a review, you can type `/review` in a comment and save it
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41442:
URL: https://github.com/apache/superset/pull/41442#issuecomment-4808541280
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**Commands**
Type the following command in the pull request comment and save the comment.
- `/review` - Manually triggers a full AI review.
- `/pause` - Pauses automatic reviews on this pull request.
- `/resume` - Resumes automatic reviews.
- `/resolve` - Marks all Bito-posted review comments as resolved.
- `/abort` - Cancels all in-progress reviews.
Refer to the <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3479713215
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts:
##########
@@ -51,3 +51,49 @@ test('series label has no textBorderColor or textBorderWidth', () => {
expect(series.label).not.toHaveProperty('textBorderColor');
expect(series.label).not.toHaveProperty('textBorderWidth');
});
+
+const nullValueProps = (showNullValues?: boolean) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ columns: ['category'],
+ metric: 'sum__value',
+ ...(showNullValues === undefined ? {} : { showNullValues }),
+ },
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__value: 10 },
+ { category: 'B', sum__value: 20 },
+ { category: null, sum__value: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+
+const seriesValues = (props: ChartProps) =>
+ (
+ (transformProps(props as EchartsSunburstChartProps).echartOptions as any)
+ .series[0].data as { value: number }[]
+ )
+ .map(node => node.value)
+ .sort((a, b) => a - b);
+
+// Charts saved before the "Show Null Values" control existed have no
+// `showNullValues` in form data; they must keep showing nulls (non-breaking).
+test('keeps null values when showNullValues is unset (legacy charts)', () => {
+ expect(seriesValues(nullValueProps(undefined))).toEqual([5, 10, 20]);
+});
+
+test('keeps null values when showNullValues is true', () => {
+ expect(seriesValues(nullValueProps(true))).toEqual([5, 10, 20]);
+});
Review Comment:
<!-- Bito Reply -->
The reviewer's suggestion to consolidate these tests is based on the principle of reducing redundancy when outputs are identical. However, your reasoning for keeping them separate is sound: they verify distinct business logic pathsβspecifically, how legacy charts (where the key is missing) versus modern charts (where the key is explicitly set to true) handle null values. Maintaining these as separate, explicit tests ensures that both conditions remain covered and prevents potential regressions in legacy behavior.
**superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts**
```
// Charts saved before the "Show Null Values" control existed have no
// `showNullValues` in form data; they must keep showing nulls (non-breaking).
test('keeps null values when showNullValues is unset (legacy charts)', () => {
expect(seriesValues(nullValueProps(undefined))).toEqual([5, 10, 20]);
});
test('keeps null values when showNullValues is true', () => {
expect(seriesValues(nullValueProps(true))).toEqual([5, 10, 20]);
});
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3479357954
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts:
##########
@@ -51,3 +51,49 @@ test('series label has no textBorderColor or textBorderWidth', () => {
expect(series.label).not.toHaveProperty('textBorderColor');
expect(series.label).not.toHaveProperty('textBorderWidth');
});
+
+const nullValueProps = (showNullValues?: boolean) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ columns: ['category'],
+ metric: 'sum__value',
+ ...(showNullValues === undefined ? {} : { showNullValues }),
+ },
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__value: 10 },
+ { category: 'B', sum__value: 20 },
+ { category: null, sum__value: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+
+const seriesValues = (props: ChartProps) =>
+ (
+ (transformProps(props as EchartsSunburstChartProps).echartOptions as any)
Review Comment:
**Suggestion:** Replace the `any` cast with a concrete chart options/series type (or a narrow reusable interface) so the series data path is type-checked instead of bypassing TypeScript safety. [custom_rule]
**Severity Level:** Minor β οΈ
<details>
<summary><b>Why it matters? π€ </b></summary>
The file is TypeScript test code, and the new helper casts `echartOptions` to `any` to access `.series[0].data`. This directly matches the rule against using `any` in new or modified TypeScript/TSX code. A concrete chart options type or narrower interface should be used instead.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=14376716fa0d4cad95df3aa4bc6c5b69&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=14376716fa0d4cad95df3aa4bc6c5b69&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(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/plugin-chart-echarts/test/Sunburst/transformProps.test.ts
**Line:** 80:80
**Comment:**
*Custom Rule: Replace the `any` cast with a concrete chart options/series type (or a narrow reusable interface) so the series data path is type-checked instead of bypassing TypeScript safety.
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%2F41442&comment_hash=9a3685f429f5d96812879e4f6268a832e6e36ac587d6e1a067464f61fd01d215&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41442&comment_hash=9a3685f429f5d96812879e4f6268a832e6e36ac587d6e1a067464f61fd01d215&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #41442:
URL: https://github.com/apache/superset/pull/41442#issuecomment-4806667150
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3481809330
##########
superset-frontend/plugins/plugin-chart-echarts/src/utils/treeBuilder.ts:
##########
@@ -90,4 +90,13 @@ export function treeBuilder(
},
[] as TreeNode[],
);
+ // Filter at every level so single-level charts and root nodes are covered,
+ // not just nested children. A parent whose children were all null-filtered
+ // is dropped too: keeping it would leave a zero-value arc that yields a NaN
+ // secondaryValue/value ratio for coloring and tooltips.
+ return filterNullNames
+ ? nodes.filter(
+ node => node.name !== null && !(node.children && !node.children.length),
Review Comment:
Yep, cleaner. Switched to `node.children?.length !== 0`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #41442:
URL: https://github.com/apache/superset/pull/41442#issuecomment-4806668836
@gerbermichi welp, I went and merged #31477 with my review comment still sitting in "pending" π€¦ ... it was asking about the `showNullValues` reading as undefined on legacy charts, and root/single-level nulls slipping through the filter. Rather than make you chase that down post-merge, I took a swing at it here (plus an empty-gap bug where the parent arc kept the hidden null's value), and defaulted things so existing charts don't change behavior. Since it's your feature, mind giving it a sanity-check when you get a sec? No pressure either way.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3479369982
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts:
##########
@@ -51,3 +51,49 @@ test('series label has no textBorderColor or textBorderWidth', () => {
expect(series.label).not.toHaveProperty('textBorderColor');
expect(series.label).not.toHaveProperty('textBorderWidth');
});
+
+const nullValueProps = (showNullValues?: boolean) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ columns: ['category'],
+ metric: 'sum__value',
+ ...(showNullValues === undefined ? {} : { showNullValues }),
+ },
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__value: 10 },
+ { category: 'B', sum__value: 20 },
+ { category: null, sum__value: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+
+const seriesValues = (props: ChartProps) =>
+ (
+ (transformProps(props as EchartsSunburstChartProps).echartOptions as any)
+ .series[0].data as { value: number }[]
+ )
+ .map(node => node.value)
+ .sort((a, b) => a - b);
+
+// Charts saved before the "Show Null Values" control existed have no
+// `showNullValues` in form data; they must keep showing nulls (non-breaking).
+test('keeps null values when showNullValues is unset (legacy charts)', () => {
+ expect(seriesValues(nullValueProps(undefined))).toEqual([5, 10, 20]);
+});
+
+test('keeps null values when showNullValues is true', () => {
+ expect(seriesValues(nullValueProps(true))).toEqual([5, 10, 20]);
+});
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Semantically duplicative null-filter tests</b></div>
<div id="fix">
Tests at lines 88-94 verify identical behavior with identical results. Per BITO.md rule 6262, tests should verify distinct business logic behaviors β when input conditions produce the same output, consolidate them to a parameterized test to prevent regressions from silently hiding in test duplication.
</div>
</div>
<small><i>Code Review Run #c3d980</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(sunburst): make "Show Null Values" non-breaking and cover all layers [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on code in PR #41442:
URL: https://github.com/apache/superset/pull/41442#discussion_r3479712383
##########
superset-frontend/plugins/plugin-chart-echarts/test/Sunburst/transformProps.test.ts:
##########
@@ -51,3 +51,49 @@ test('series label has no textBorderColor or textBorderWidth', () => {
expect(series.label).not.toHaveProperty('textBorderColor');
expect(series.label).not.toHaveProperty('textBorderWidth');
});
+
+const nullValueProps = (showNullValues?: boolean) =>
+ new ChartProps({
+ formData: {
+ colorScheme: 'bnbColors',
+ datasource: '3__table',
+ columns: ['category'],
+ metric: 'sum__value',
+ ...(showNullValues === undefined ? {} : { showNullValues }),
+ },
+ width: 800,
+ height: 600,
+ queriesData: [
+ {
+ data: [
+ { category: 'A', sum__value: 10 },
+ { category: 'B', sum__value: 20 },
+ { category: null, sum__value: 5 },
+ ],
+ },
+ ],
+ theme: supersetTheme,
+ });
+
+const seriesValues = (props: ChartProps) =>
+ (
+ (transformProps(props as EchartsSunburstChartProps).echartOptions as any)
+ .series[0].data as { value: number }[]
+ )
+ .map(node => node.value)
+ .sort((a, b) => a - b);
+
+// Charts saved before the "Show Null Values" control existed have no
+// `showNullValues` in form data; they must keep showing nulls (non-breaking).
+test('keeps null values when showNullValues is unset (legacy charts)', () => {
+ expect(seriesValues(nullValueProps(undefined))).toEqual([5, 10, 20]);
+});
+
+test('keeps null values when showNullValues is true', () => {
+ expect(seriesValues(nullValueProps(true))).toEqual([5, 10, 20]);
+});
Review Comment:
These two aren't actually duplicates, they pin different code paths: one with no showNullValues key at all (legacy charts, must keep nulls) and one with it set to true. Same output, different conditions, so I'd rather keep both explicit.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org