fix: Last Month Label Missing on X-Axis in Time Series Line Charts #36085
Conversation
Code Review Agent Run #3e36a9Actionable 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 |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review by Korbit AI
Korbit automatically attempts to detect when you fix issues in new commits.
| Category | Issue | Status |
|---|---|---|
 |
Time axis label overlap not prevented ▹ view | |
 |
Unnecessary computation in hideOverlap property ▹ view |
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
| nameLocation: 'middle', | ||
| axisLabel: { | ||
| hideOverlap: true, | ||
| hideOverlap: xAxisType !== AxisType.Time, |
There was a problem hiding this comment.
Time axis label overlap not prevented 
Tell me more
What is the issue?
The conditional logic for hideOverlap may cause label overlap issues for time-based charts where labels could still overlap despite being disabled.
Why this matters
When xAxisType is AxisType.Time, hideOverlap will be set to false, which means overlapping labels will be displayed. This could result in unreadable or cluttered time axis labels, especially when there are many data points or long time labels that naturally overlap.
Suggested change ∙ Feature Preview
Consider the specific use case requirements. If time labels should never overlap, keep hideOverlap: true for all axis types. If time labels need to show all values even when overlapping, consider adding additional logic to handle label formatting or rotation:
hideOverlap: xAxisType !== AxisType.Time ? true : shouldShowAllTimeLabels,Or implement proper label spacing/rotation for time axes to prevent overlap while showing all labels.
Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
| nameLocation: 'middle', | ||
| axisLabel: { | ||
| hideOverlap: true, | ||
| hideOverlap: xAxisType !== AxisType.Time, |
There was a problem hiding this comment.
Unnecessary computation in hideOverlap property 
Tell me more
What is the issue?
The hideOverlap property is being computed on every render by comparing xAxisType to AxisType.Time, which is an unnecessary computation since these values don't change during the component lifecycle.
Why this matters
This creates a minor performance overhead as the comparison is executed on every function call, even though the result will be the same for a given xAxisType value throughout the component's lifecycle.
Suggested change ∙ Feature Preview
Pre-compute the boolean value and store it in a variable before the xAxis object definition:
const shouldHideOverlap = xAxisType !== AxisType.Time;
let xAxis: any = {
type: xAxisType,
name: xAxisTitle,
nameGap: convertInteger(xAxisTitleMargin),
nameLocation: 'middle',
axisLabel: {
hideOverlap: shouldHideOverlap,
formatter: xAxisFormatter,
rotate: xAxisLabelRotation,
interval: xAxisLabelInterval,
},
// ... rest of config
};Provide feedback to improve future suggestions
💬 Looking for more details? Reply to this comment to chat with Korbit.
sadpandajoe
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue where the last month label was missing on the X-axis in time series line charts. The fix conditionally sets the hideOverlap property based on axis type: false for time-based axes (to ensure all labels including the last one are displayed) and true for categorical/value axes (to prevent overlapping labels).
Key Changes:
- Modified
hideOverlaplogic intransformProps.tsto be conditional based on axis type - Added test coverage for both time-based and categorical x-axis scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts | Updated hideOverlap property to be false for time axes and true for other axis types |
| superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts | Added two test cases to verify hideOverlap behavior for time-based and categorical axes |
superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
Outdated
Show resolved
Hide resolved
1 participant
Summary
Fixes #33905
plugins/plugin-chart-echarts/src/Timeseries/transformProps.tsUpdated logic to set
axisLabel.hideOverlapbased on the axis type:hideOverlap = false(always show final labels)hideOverlap = true(suppress overlapping labels)Regression Coverage
plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.tshideOverlapis false for time-based axes.hideOverlapis true when the x-axis is forced to be categorical.