fix(explore): zero-value stacked bar segment no longer overlaps its neighbor's label - #42882
fix(explore): zero-value stacked bar segment no longer overlaps its neighbor's label#42882vjymisal0 wants to merge 4 commits into
Conversation
…eighbor's label On a stacked Timeseries Bar chart with "Show Value" enabled (not "Only Total"), a series whose value was exactly 0 for a category still got a visible label, rendered at the same position as the label of the segment directly below it, since a zero-height stacked segment starts and ends where the previous segment's top is. The per-series label formatter only compared the value against thresholdValues[dataIndex], which is 0 for every row under the default percentage_threshold of 0 — so `numericValue >= 0` always passed for a literal 0. Now requires the value to be strictly positive first, independent of the configured threshold. Closes apache#42702
dosubot
Bot
added
viz:charts:echarts
Code Review Agent Run #ff2283Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if ( | ||
| numericValue > 0 && | ||
| numericValue >= | ||
| (thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER) | ||
| (thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER) |
There was a problem hiding this comment.
Suggestion: The strictly-positive guard suppresses every negative stacked-segment label, even when the configured percentage threshold allows it. Negative values have a nonzero segment position and the previous comparison could legitimately render them, particularly when the stacked total and threshold are negative. Restrict the new guard to excluding exactly zero while preserving the existing threshold behavior for negative values. [logic error]
Severity Level: Major ⚠️
- ⚠️ Negative stacked segment labels disappear under negative totals.
- ⚠️ Configured percentage thresholds no longer preserve prior behavior.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
**Line:** 477:480
**Comment:**
*Logic Error: The strictly-positive guard suppresses every negative stacked-segment label, even when the configured percentage threshold allows it. Negative values have a nonzero segment position and the previous comparison could legitimately render them, particularly when the stacked total and threshold are negative. Restrict the new guard to excluding exactly zero while preserving the existing threshold behavior for negative values.
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.
Fixed in 26a1ab9 — changed the guard from numericValue > 0 to numericValue !== 0 so negative values are no longer suppressed, and added a regression test.
|
The flagged issue is correct. The current implementation uses Here is the corrected implementation for the logic in if (
numericValue !== 0 &&
numericValue >=
(thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER)
) {I have validated this against the provided diff. Would you like me to check the other comments on this PR and implement fixes for them as well? superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts |
…ones numericValue > 0 also suppressed a genuine negative value whenever it cleared the (possibly also negative) threshold for a category whose stacked total is itself negative — the zero-height overlap bug this guard fixes has nothing to do with sign, only with a value of exactly 0. Changed to numericValue !== 0 and added a regression test for the negative case. Thanks to the Bito AI review on this PR for catching this.
|
Good catch — pushed a fix. Changed |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42882 +/- ##
==========================================
+ Coverage 57.10% 66.37% +9.26%
==========================================
Files 2857 2857
Lines 161125 161047 -78
Branches 37060 37048 -12
==========================================
+ Hits 92014 106888 +14874
+ Misses 68259 52143 -16116
- Partials 852 2016 +1164
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:
|
Code Review Agent Run #b3a042Actionable 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 |
|
@vjymisal0 can you add before and after screenshots of your fix into the PR description? |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:171
- The test comment explains the pre-fix behavior as
numericValue >= 0, but the implementation uses(thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER), so with a default threshold of 0 the check effectively doesn’t filter values at all. Updating the comment to match the actual logic will avoid confusion for future readers.
// percentage_threshold defaults to 0, so thresholdValues[dataIndex] is
// 0 too — a value of exactly 0 would satisfy `numericValue >= 0`
// without the explicit `numericValue !== 0` guard.
transformSeries expects opts.formatter: ValueFormatter (NumberFormatter | CurrencyFormatter), but the two zero/negative-value regression tests passed a plain arrow function, which fails tsc under lint-frontend CI. Wrap the same format logic in a NumberFormatter instance instead.
|
Pushed 5ffaf3a — the two regression tests were passing a plain arrow function for |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #2538c4Actionable 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
On a stacked Timeseries Bar chart (
echarts_timeseries_bar) with multiple metrics/series and "Show Value" enabled (not "Only Total"), a series whose value is exactly0for a given x-axis category still gets a visible value label, rendered at the same pixel position as the label of the segment directly below it — a zero-height stacked segment starts and ends at the same y-coordinate as the top of the previous segment, so the two labels overlap into unreadable, doubled text.Root cause, in the per-series
label.formatterinTimeseries/transformers.ts: the label is shown whennumericValue >= thresholdValues[dataIndex].thresholdValues[dataIndex]comes from((percentageThreshold || 0) / 100) * valuesinextractDataTotalValues, so under the defaultpercentage_threshold: 0it's0for every row — and0 >= 0is true, so a segment with literally no height still gets a label.Fix: require the value to be strictly positive before showing a per-series stacked label, independent of the configured threshold. A zero-height segment has no meaningful position to attach a label to regardless of threshold settings.
Related to, but distinct from, #42701 (which is about the "Only Total" sum being wrong rather than per-series label overlap) — I opened a separate PR (#42881) for that one since the two bugs are in different functions with independent fixes.
Closes #42702
TESTING INSTRUCTIONS
Added
does not render a per-series stacked label for a zero-value segment (#42702)totest/Timeseries/transformers.test.ts'stransformSeriessuite, exercising thelabel.formatterdirectly: asserts an empty string for a0value and the formatted string for a non-zero one, withthresholdValues: [0]to match the defaultpercentage_threshold.Ran locally:
240 tests pass, oxlint clean.
Manual repro steps are in the linked issue (stacked bar chart, 2 metrics where one is 0 for a category, Show Value on, Only Total off).
ADDITIONAL INFORMATION