Skip to content

fix(plugin-chart-echarts): omit stacked value labels on zero-height segments - #42756

Open
MannXo wants to merge 1 commit into
apache:masterfrom
MannXo:fix/42702-stacked-zero-value-label-overlap
Open

fix(plugin-chart-echarts): omit stacked value labels on zero-height segments#42756
MannXo wants to merge 1 commit into
apache:masterfrom
MannXo:fix/42702-stacked-zero-value-label-overlap

Conversation

@MannXo

@MannXo MannXo commented Aug 4, 2026

Copy link
Copy Markdown

SUMMARY

Fixes #42702.

On a stacked echarts_timeseries_bar with "Show Value" on and "Only Total" off, a series whose value is 0 for a category still gets a value label. A zero-height stacked segment begins and ends at the same coordinate as the top of the segment beneath it, so echarts draws that label on top of the label belonging to the segment below, and the two numbers render over each other as unreadable text.

This adds one guard: a stacked segment with no height carries no label.

One correction to the root cause in the issue. The issue reports that with the default percentage_threshold: 0 the condition becomes numericValue >= 0. It does not. 0 is falsy, so thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER evaluates to Number.MIN_SAFE_INTEGER and the condition is numericValue >= Number.MIN_SAFE_INTEGER, which is true for everything. Observed against the label formatter:

thresholdValues=[0]     0 -> "0"     -5 -> "-5"     32 -> "32"
thresholdValues=[10]    0 -> ""       5 -> ""       50 -> "50"

If the mechanism were >= 0, that -5 would have been dropped. So the || is doing its job: a 0 threshold means "no threshold filtering", and a real threshold already suppresses zeros. That makes this a label-collision bug rather than a threshold bug, and it is why the fix does not touch the threshold expression. Rewriting that comparison would change threshold semantics for every user and would suppress legitimate negative labels.

A second defect on the same code path. A null value renders the literal string "null" as a chart label, with the production number formatter:

0 -> "0"     null -> "null"     32 -> "32"     -5 -> "-5"

A null segment has no height either, so it collides identically. The guard covers both under one rule rather than special-casing zero.

Prior art in this plugin. Timeseries/transformProps.ts already omits zero observations from the rich tooltip of a stacked series (if (value.observation === 0 && stack) return;). This applies the same judgement to the per-series value label.

Scope: stacked series only. Unstacked labels sit on the bar itself with nothing to collide with, and they return earlier in the formatter. Only-total labels render the stack total and are unaffected. The stack && clause is deliberate for legibility even though the earlier !stack return makes it defensive rather than load-bearing, matching how the tooltip guard above spells out the same condition.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Stacked bar, two metrics, "Show Value" on, "Only Total" off, default percentage threshold, with the upper series at 0 for two categories.

Before After
image image

The label coordinates from the rendered SVG, so the images and the numbers agree:

before                              after
x=146.3 y=143.0 '0'                 x=146.3 y=143.0 '32'
x=146.3 y=143.0 '32'                x=775.2 y=179.2 '27'
x=775.2 y=179.2 '0'
x=775.2 y=179.2 '27'

overlapping pairs: ('0','32'), ('0','27')       overlapping pairs: none

TESTING INSTRUCTIONS

Manual:

  1. Create an echarts_timeseries_bar chart with two metrics, for example A and B.
  2. Set Stacked, turn on "Show Value", leave "Only Total" off and "Percentage threshold" at its default.
  3. Include a category where the metric stacked on top is 0 and the one below it is not, for example {"category": "1-3g", "B": 32, "A": 0}.
  4. Run the chart. Before this change the 0 label is drawn over the 32 label as doubled text. After it, only 32 is labelled.
  5. Confirm the unchanged cases: a category where both metrics are non-zero still labels both segments; switching off Stacked still labels a 0; turning on "Only Total" still shows the stack total.

Automated, in plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts:

npx jest plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts

Five cases cover zero and null suppressed when stacked, non-zero and negative kept, zero kept when not stacked, percentage_threshold still filtering below-threshold values, and only-total labels unchanged.

Gates run locally on Node 24.16.0, using the commands from .github/workflows/superset-frontend.yml:

npx jest plugins/plugin-chart-echarts     69 suites, 772 tests, all passing
npm run lint                              clean
npm run plugins:build && npm run type     clean
npm ls --all --package-lock-only --depth=0    clean
pre-commit run --files <the two changed files>   all hooks passing

Each new assertion was checked by reverting the behaviour it covers and confirming the intended test fails: removing the guard fails the zero-height case, and guarding zero without null fails it too. One further sabotage, removing the stack && clause, is not caught, because the !stack branch returns before the guard is reached, so no test can distinguish it. Noting that rather than presenting the set as complete.

ADDITIONAL INFORMATION

…egments

A stacked segment whose value is 0 or null has no height, so it begins and ends
at the same coordinate as the top of the segment beneath it. With "Show Value"
on and "Only Total" off, echarts draws its label at that shared coordinate, on
top of the label belonging to the segment below. The result is two numbers
rendered over each other as unreadable text.

`percentage_threshold` does not filter these out, and it is not meant to. It
defaults to 0, and `thresholdValues[dataIndex] || Number.MIN_SAFE_INTEGER` turns
a 0 threshold into "no filtering" rather than "filter at zero" -- which is why
negative values also keep their labels at the default. Changing that expression
would alter threshold semantics and suppress legitimate negative labels, so the
guard is separate: a segment with no height carries no label.

Scoped to stacked series. Unstacked labels sit on the bar itself with nothing to
collide with, and they return earlier in the formatter. Only-total labels render
the stack total and are untouched.

This mirrors the rich tooltip, which already omits zero observations from a
stacked series in Timeseries/transformProps.ts.

Fixes apache#42702
@dosubot dosubot Bot added change:frontend Requires changing the frontend viz:charts:echarts Related to Echarts viz:charts:timeseries Related to Timeseries labels Aug 4, 2026
@bito-code-review

bito-code-review Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Code Review Agent Run #5e26f6

Actionable Suggestions - 0
Review Details
  • Files reviewed - 2 · Commit Range: 20b8ee2..20b8ee2
    • superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts
    • superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformers.test.ts
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful
    • Eslint (Linter) - ✔︎ Successful

Bito Usage Guide

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 documentation for additional commands.

Configuration

This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.

Documentation & Help

AI Code Review powered by Bito Logo

@codecov

codecov Bot commented Aug 4, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 65.59%. Comparing base (e66c7fc) to head (20b8ee2).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #42756      +/-   ##
==========================================
- Coverage   65.59%   65.59%   -0.01%     
==========================================
  Files        2819     2819              
  Lines      160166   160168       +2     
  Branches    36569    36570       +1     
==========================================
- Hits       105059   105055       -4     
- Misses      53059    53065       +6     
  Partials     2048     2048              
Flag Coverage Δ
javascript 71.74% <100.00%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

change:frontend Requires changing the frontend plugins size/M viz:charts:echarts Related to Echarts viz:charts:timeseries Related to Timeseries

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Timeseries Bar (stacked): zero-value series label overlaps the adjacent segment's label when percentage_threshold is 0

1 participant