Repository navigation
fix(echarts): prevent x-axis time labels from overlapping - #43669
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43669 +/- ##
=======================================
Coverage 79.29% 79.30%
=======================================
Files 2889 2889
Lines 166762 166788 +26
Branches 38604 38620 +16
=======================================
+ Hits 132241 132266 +25
- Misses 32027 32028 +1
Partials 2494 2494
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:
|
dosubot
Bot
added
viz:charts:echarts
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const deduplicatedFormatter = showMaxLabel | ||
| ? (() => { | ||
| let lastLabel: string | undefined; | ||
| let lastValue: number | undefined; | ||
| const wrapper = (value: number | string) => { | ||
| // ECharts formats the labels in repeated ascending passes. Reset the | ||
| // dedup state when the sequence restarts so a forced boundary label | ||
| // (e.g. the min date) isn't blanked by the previous pass's last label | ||
| // when both format identically (e.g. a May-to-May range). | ||
| if ( | ||
| typeof value === 'number' && | ||
| lastValue !== undefined && | ||
| value <= lastValue | ||
| ) { | ||
| lastLabel = undefined; | ||
| } | ||
| if (typeof value === 'number') { | ||
| lastValue = value; | ||
| } | ||
| const label = | ||
| typeof xAxisFormatter === 'function' | ||
| ? (xAxisFormatter as Function)(value) | ||
| : String(value); | ||
| if (label === lastLabel) { | ||
| return ''; | ||
| } | ||
| lastLabel = label; | ||
| return label; | ||
| }; | ||
| if (typeof xAxisFormatter === 'function' && 'id' in xAxisFormatter) { | ||
| (wrapper as any).id = (xAxisFormatter as any).id; | ||
| } | ||
| return wrapper; | ||
| })() | ||
| ? createSpacedXAxisFormatter( | ||
| xAxisFormatter, | ||
| ...getXAxisDomain( | ||
| [rebasedData as Record<string, unknown>[]], | ||
| xAxisLabel, | ||
| ), | ||
| Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0), | ||
| ) |
There was a problem hiding this comment.
Suggestion: The spacing calculation assumes the plot width is the full chart width minus only the fixed 20px left and right offsets. The actual ECharts plot is further reduced by y-axis title margins, legend placement, and other padding, so this overestimates available label space and can leave labels visually overlapping when hideOverlap is disabled. Base the calculation on the effective grid width. [logic error]
Severity Level: Major ⚠️
- ⚠️ Dense labels can overlap with left-side axis titles.
- ⚠️ Timeseries charts use incorrect available x-axis width.
- ⚠️ MixedTimeseries has the same approximation at lines 672-682.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 1219:1227
**Comment:**
*Logic Error: The spacing calculation assumes the plot width is the full chart width minus only the fixed 20px left and right offsets. The actual ECharts plot is further reduced by y-axis title margins, legend placement, and other padding, so this overestimates available label space and can leave labels visually overlapping when `hideOverlap` is disabled. Base the calculation on the effective grid width.
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.
Acknowledged — per rebenitez1802's review, this is a real but acceptable heuristic. Cheap improvement noted: swap the fixed width - 2 * gridOffsetLeft for the already-computed padding.left/padding.right insets. Leaving as a follow-up rather than blocking this fix.
|
The flagged issue is correct. The current spacing calculation uses a simplified width ( To resolve this, you should calculate the effective grid width by accounting for these additional margins. 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/MixedTimeseries/transformProps.ts |
| xAxisLabelCharWidthPx: 7, | ||
| // Minimum gap (px) to keep between adjacent x-axis time labels. | ||
| xAxisLabelMinGapPx: 8, |
There was a problem hiding this comment.
Suggestion: The collision threshold assumes every rendered character is exactly seven pixels wide, but the formatter is also used for localized and custom time formats. Wide glyphs or long localized month names can exceed this estimate, causing the wrapper to retain labels whose actual rendered footprints overlap; narrow glyphs can also cause valid labels to be unnecessarily removed. Use measured text width or a conservative width calculation based on the actual formatter output and font. [possible bug]
Severity Level: Major ⚠️
- ⚠️ Custom time formats can remain visually overlapped.
- ⚠️ Localized month labels may be thinned inaccurately.
- ⚠️ Non-overlapping labels may be unnecessarily hidden.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
**Line:** 58:60
**Comment:**
*Possible Bug: The collision threshold assumes every rendered character is exactly seven pixels wide, but the formatter is also used for localized and custom time formats. Wide glyphs or long localized month names can exceed this estimate, causing the wrapper to retain labels whose actual rendered footprints overlap; narrow glyphs can also cause valid labels to be unnecessarily removed. Use measured text width or a conservative width calculation based on the actual formatter output and font.
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.
Acknowledged — per rebenitez1802's review, the fixed 7px/char estimate is a real but acceptable heuristic (worst case still beats no suppression). A measureText-based estimate would be a nice follow-up, not blocking this fix.
| const value = record[xAxisCol]; | ||
| if (typeof value === 'number') { | ||
| if (domainMin === undefined || value < domainMin) domainMin = value; | ||
| if (domainMax === undefined || value > domainMax) domainMax = value; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Suggestion: getXAxisDomain only recognizes numeric values, but temporal query rows can contain timestamp strings or Date objects. For those charts both bounds remain undefined, so pixelsPerMs is disabled and, because hideOverlap is simultaneously set to false, no overlap suppression is applied at all. Normalize temporal values to epoch milliseconds before calculating the domain. [logic error]
Severity Level: Major ⚠️
- ❌ Dense string-timestamp charts retain overlapping labels.
- ❌ Date-valued query records bypass spacing suppression.
- ⚠️ Timeseries and MixedTimeseries are both affected.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
**Line:** 311:316
**Comment:**
*Logic Error: `getXAxisDomain` only recognizes numeric values, but temporal query rows can contain timestamp strings or `Date` objects. For those charts both bounds remain undefined, so `pixelsPerMs` is disabled and, because `hideOverlap` is simultaneously set to false, no overlap suppression is applied at all. Normalize temporal values to epoch milliseconds before calculating the domain.
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.
Acknowledged — per rebenitez1802's review, this looks like a false positive on the standard pipeline: the chart-data API serializes temporal columns to epoch-ms numbers, so the numeric-only filter in getXAxisDomain is correct there. Not blocking this fix.
Code Review Agent Run #28dbffActionable Suggestions - 0Additional Suggestions - 2
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 |
rusackas
left a comment
rusackas
left a comment
There was a problem hiding this comment.
@sadpandajoe LGTM, thanks for the fix (and for consolidating the duplicated formatter logic while you were in there). The bot threads all seem to be poking at the same thing: the width heuristics. I think those are fine as-is, since the worst case still beats no suppression at all... but a measureText-based estimate could be a nice follow-up. Approving!
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Request changes: the core approach (label-level thinning while keeping hideOverlap: false) is sound and consolidating the duplicated inline formatters is a genuine improvement — but this introduces a default-settings regression for horizontal time charts, and a private tracker ID leaked into the new test names. The bot threads above flag real-but-acceptable heuristics; the horizontal regression is the one thing not yet covered.
🔴 High — Horizontal charts: the new spacing formatter over-thins the (vertical) time axis
createSpacedXAxisFormatter is applied whenever showMaxLabel is true (Timeseries/transformProps.ts:1214), gated only on xAxisType === AxisType.Time && rotation === 0 && resolvedTimeGrain — with no !isHorizontal guard. getAxisType returns AxisType.Time for any temporal column regardless of orientation, so this fires on horizontal bar/line charts at the default 0° rotation. But on horizontal orientation the time axis is swapped to run vertically ([xAxis, yAxis] = [yAxis, xAxis] at :1373), while the spacing model uses horizontal geometry (plotWidthPx = width − 40 and label.length * 7px).
Concretely: 24 monthly points at width=800 → computed per-tick spacing ≈ 33px vs a ≈ 64px threshold → all ~22 interior labels are blanked, only the two boundaries survive. On master the formatter was dedup-only, so every distinct label rendered. This is pure loss (vertically-stacked labels weren't overlapping horizontally to begin with), not a "worst case beats no suppression" trade-off.
Fix: gate the spacing formatter on showMaxLabel && !isHorizontal and fall back to the plain consecutive-dedup wrapper when horizontal.
🟡 Medium — Horizontal charts: the !isHorizontal guard on alignMaxLabel/alignMinLabel was dropped
On master, alignMaxLabel: 'right' / alignMinLabel: 'left' sit in a separate showMaxLabel && !isHorizontal spread, with a comment noting the alignments assume the axis runs along the bottom and misplace labels when a horizontal chart puts the axis on the side. This PR collapses everything into one unconditional block (Timeseries/transformProps.ts:1257-1262), so those alignment hints now apply to the side-mounted vertical axis — a direct revert of that previously-fixed behavior.
Fix: restore the separate ...(showMaxLabel && !isHorizontal && { alignMaxLabel: 'right', alignMinLabel: 'left' }) spread, keeping showMaxLabel/showMinLabel unconditional. (MixedTimeseries has no horizontal mode, so it needs no change.)
🟡 Medium — Forced boundary label can regress #39899 when the axis extent ≠ the data extent
isBoundary keys on value === domainMin || value === domainMax, where the domain comes from getXAxisDomain(rebasedData) — the query rows only. The time x-axis gets no explicit min/max (getMinAndMaxFromBounds returns {} for non-Value axes), so ECharts derives the extent from the union of all series on axis 0, including timeseries/overlay annotation lines (transformTimeseriesAnnotation pushes a type:'line' series) and time_shift overrides. When an overlay extends past the last data point, ECharts calls the formatter with a max boundary tick > domainMax, isBoundary is false, and the spacing check can blank the forced last label — the exact #39899 symptom.
It's self-mitigating (the pixelsPerMs computed from the narrower data span biases toward showing the label), so it's narrow — but the mechanism is real and the current tests can't catch it (see below). Worth protecting the first/last formatter invocation of each ascending pass positionally rather than matching against the data-derived domain.
🟢 Low — plotWidth = width − 40 ignores the real grid inset (and degenerates at tiny widths)
Math.max(width - 2 * TIMESERIES_CONSTANTS.gridOffsetLeft, 0) (Timeseries/transformProps.ts:1226) overestimates the plot area — with grid.containLabel the y-axis label + title width are reserved inside that box — so pixelsPerMs runs high and dense labels under-thin (residual overlap persists on charts with wide y-labels). This is the width thread above; it's an acceptable heuristic, but padding is already computed a few lines up, so improving it is nearly free: Math.max(width - (padding.left ?? 0) - (padding.right ?? 0), 0). Note the opposite degenerate too: when width ≤ 40, plotWidthPx clamps to 0 → pixelsPerMs = 0 → every non-boundary label blanked (thumbnails / tiny dashboard tiles).
🟢 Low — New tests: a private internal tracker ID is embedded in the test names
The two new tests are named after an internal Preset/Shortcut story ID (the SC-… prefix) — test/Timeseries/transformers.test.ts:355 and test/MixedTimeseries/transformProps.test.ts:1327. That ID has no meaning to OSS contributors and shouldn't land in the public history. Fix: rename to a behavioral title referencing the public issue, e.g. '#39899 - closely spaced x-axis time labels do not visually overlap'.
🟢 Low — Tests assert "thinning happened," not "thinning is sufficient"
Both tests only check hideOverlap === false, labels.filter(l => l === '').length > 0, and that the first/last labels are non-empty. A formatter that blanks a single interior label while leaving 19 dense ones overlapping would still pass. And because they build labels via data.map(ts => formatter(ts)), the last value always equals domainMax, so isBoundary short-circuits and the boundary-protection path is never exercised. Fix: assert adjacent surviving labels are at least the collision threshold apart, and add a case that calls the formatter with a trailing value > max(data) to exercise a boundary tick that differs from domainMax.
On the existing bot threads: the "string/Date timestamps" thread looks like a false positive on the standard pipeline — the chart-data API serializes temporal columns to epoch-ms numbers (json_int_dttm_ser), so getXAxisDomain's numeric filter is correct there; a defensive coercion is optional, not required. The grid-width and fixed-7px/char threads are real but acceptable heuristics (the width one has the cheap fix noted above).
Net: a solid fix that clearly improves the common vertical case — please add the !isHorizontal guard (High + Medium) and rename the two tests before merge; the rest are non-blocking.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #0e7118Actionable 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 |
rebenitez1802
left a comment
rebenitez1802
left a comment
There was a problem hiding this comment.
Approve — thanks for the quick turnaround, @sadpandajoe. a516f57 resolves everything I flagged in round 1, with good regression coverage. Verified against the delta:
- Horizontal over-thinning (was High) — fixed:
deduplicatedFormatternow falls back to the newcreateDedupXAxisFormatterwhenisHorizontal, so the horizontal-plot spacing geometry is no longer applied to the side-mounted time axis. The dedup helper is a faithful extraction of the previous inline logic (same pass-reset, dedup, and.idpropagation), so horizontal charts behave exactly as before. The#39899 - horizontal orientation does not over-thin the time axis labelstest locks it in. - Boundary-label alignment guard (was Medium) — fixed:
alignMaxLabel/alignMinLabelare back behind theshowMaxLabel && !isHorizontalspread (with the explanatory comment), whileshowMaxLabel/showMinLabelstay unconditional. The new alignment test codifies both orientations. - Test names (was Low) — fixed: renamed to reference the public issue.
Scope looks right — MixedTimeseries has no horizontal orientation mode, so it correctly needs no guard.
The remaining bot-thread items (grid-width estimate, fixed px/char) are acceptable heuristics and good follow-up candidates as discussed; the annotation/overlay boundary-extent edge case is narrow and self-mitigating. None block. Nice work consolidating the formatters.
When showMaxLabel is active (the default for most timeseries charts), axisLabel.hideOverlap must stay off to keep the forced boundary label visible (#39899), which also disabled ECharts' own overlap suppression for every other tick. Dense/closely-spaced time data would then render with all labels overlapping. Extend the x-axis label formatter to estimate each label's pixel footprint against the available per-tick spacing and blank labels that would visually collide, while always keeping the forced axis boundary labels visible. Applies to both Timeseries and MixedTimeseries charts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Timeseries/transformProps.ts assigns xAxisFormatter as TimeFormatter | StringConstructor | NumberFormatter | undefined, since it can be a NumberFormatter when xAxisDataType is Numeric. The showMaxLabel branch passes it to createSpacedXAxisFormatter, whose XAxisFormatterFn type omitted NumberFormatter, causing a tsc build failure even though that branch only runs for AxisType.Time (which never pairs with Numeric data). NumberFormatter is callable like TimeFormatter, so widening the union is safe.
…l orientation The x-axis label spacing formatter estimated collisions using horizontal-plot geometry (chart width, char width in px), but a horizontal-orientation chart swaps the time axis onto the side of the chart where that geometry no longer applies, causing most interior labels to be blanked. Fall back to the dedup-only formatter there, and restore the horizontal-orientation guard on the boundary label alignment hints that was dropped when the formatters were consolidated. Also renames two regression tests that had leaked an internal tracker ID into their titles.
a516f57 to
66ae59d
Compare
|
Bito Automatic Review Skipped – PR Already Merged |
SUMMARY
On a time-series chart with the default configuration (0° x-axis label rotation, a resolved time
grain), ECharts' native label-collision suppression (
axisLabel.hideOverlap) is disabled. This is adeliberate trade-off from a prior fix that guarantees the forced boundary/max date label is never
hidden by ECharts' own overlap detection, but it has the side effect of disabling overlap suppression
for every other tick on the axis too — so dense/closely-spaced time labels can render on top of each
other instead of being thinned or spaced out.
This adds label-level overlap suppression instead of touching the axis-level toggle: a shared
createSpacedXAxisFormatter+getXAxisDomainpair (utils/formatters.ts) estimates each label'srendered pixel footprint against the available per-tick spacing and blanks a label that would
visually collide with the previously-rendered one, while always keeping the forced boundary labels
visible.
axisLabel.hideOverlapstaysfalseand the existing boundary-label regression tests areuntouched.
Applied identically to
TimeseriesandMixedTimeseries, replacing the previously duplicated inlineformatter logic in each (which only deduplicated exact string-duplicate labels, not visually-colliding
ones).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Reproduced with a dense, minute-level time series (X-Axis Label Interval = All, Time Grain = Minute,
X-Axis Time Format =
%Y-%m-%d %H:%M:%S, 0° rotation) — the density needed to trigger the collision.Before (labels stacked on top of each other, unreadable):
After (labels cleanly spaced, no overlap):
TESTING INSTRUCTIONS
New regression tests reproduce dense/closely-spaced time labels for both
TimeseriesandMixedTimeseries(confirmed RED before the fix, GREEN after). Fullplugin-chart-echartssuite:994/994 passing.
Manually: create a time-series chart (Line/Area/Bar) with a temporal x-axis and a time grain fine
enough relative to the selected date range that many points/ticks land close together (e.g. Day grain
over several months), leave X-Axis Label Rotation at its default (0°) — x-axis labels no longer
overlap, and the first/last date labels remain visible.
ADDITIONAL INFORMATION