fix(echarts): place weekly time-axis ticks on the data buckets - #43339
fix(echarts): place weekly time-axis ticks on the data buckets#43339EnxDev wants to merge 6 commits into
Conversation
dosubot
Bot
added
explore:time
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #0a3425
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts - 1
- Missing unit tests for new function · Line 999-1022
Additional Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/constants.ts - 1
-
Missing test coverage for new constant · Line 94-100The new constant has no test coverage. Other code paths that reference the same five weekly granularities (e.g., `formatters.ts:80-84`) could drift from this definition over time. Add a unit test for `WEEKLY_TIME_GRAINS` to lock in the expected values and catch future divergences.
-
Review Details
-
Files reviewed - 6 · Commit Range:
296ab06..296ab06- superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/constants.ts
- superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
- superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.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
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43339 +/- ##
==========================================
+ Coverage 57.39% 57.40% +0.01%
==========================================
Files 2877 2877
Lines 165232 165279 +47
Branches 38184 38213 +29
==========================================
+ Hits 94836 94884 +48
+ Misses 69501 69500 -1
Partials 895 895
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 #9d5b33Actionable Suggestions - 0Additional Suggestions - 1
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 |
# Conflicts: # superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts # superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts # superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
Checked DST, the non-weekly grains, forceMaxInterval, time-shift and the bar variants — those all hold up. Three questions on the edges.
| // A timeseries annotation contributes its own timestamps and widens the axis | ||
| // past the buckets, and ECharts clips pinned ticks to the extent, so that | ||
| // span would render bare — leave those charts on ECharts' own ticks. | ||
| const hasTimeseriesAnnotation = annotationLayers.some( |
There was a problem hiding this comment.
Any shown timeseries annotation disables pinning, even one whose records sit entirely inside the bucket range and widen nothing — worth comparing its extent to the buckets instead of gating on layer type?
There was a problem hiding this comment.
reasonable idea but nontrivial scope; current behavior is safely conservative
| // by ECharts' overlap detection (#39899). Pinned ticks label | ||
| // every bucket, which does crowd, so thinning always wins there. | ||
| hideOverlap: | ||
| !!temporalTickValues || |
There was a problem hiding this comment.
hideOverlap is forced on for weekly here but showMaxLabel: true is still emitted below — does that reopen #39899 for weekly grains, given hideOverlap only drops the max label's immediate neighbour?
There was a problem hiding this comment.
Real risk, confirmed via ECharts' AxisBuilder internals (only the immediate neighbor is shielded). Fixed by skipping the showMaxLabel override when ticks are pinned
| value instanceof Date | ||
| ? value.getTime() | ||
| : typeof value === 'string' | ||
| ? new Date(value).getTime() |
There was a problem hiding this comment.
new Date(value) parses a zone-less bucket string as UTC while echarts' parseDate treats it as local, so at UTC+2 the point sits at 04-05T22:00Z and the pinned tick at 04-06T00:00Z — does that offset the tick and clip the boundary one? The new test uses .toISOString(), so it wouldn't catch this.
There was a problem hiding this comment.
Real bug, confirmed against ECharts source. Fixed with parseTemporalString()
Code Review Agent Run #5ce1a8Actionable 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 |
2 participants
SUMMARY
ECharts'
timeaxis builds its ticks from a built-in calendar ladder (year → month → day → hour …). That ladder has no week unit, so for weekly data it falls back to stepping N days from the 1st of each month and re-anchoring at every month boundary.The plotted points stay on the real week starts while the labels drift across weekdays and snap to month starts — with Monday buckets at
04-06 04-13 04-20 …the axis renders04-08 04-15 04-22 … 05-01 06-01. The query result and the Results pane are correct; only the axis is wrong.Two things worth calling out, both measured rather than assumed:
minInterval/maxIntervalcannot fix this.TIMEGRAIN_TO_TIMESTAMPhas no weekly entry, and adding one changes nothing — those options bound how far apart ticks sit, not which instants they land on. Tick positions come out byte-identical with and without a one-weekminInterval.The fix is therefore scoped to the five weekly grains.
getTemporalTickValues()returns the sorted, de-duplicated bucket timestamps, and the axis pinsaxisLabel,axisTickandsplitLineto them via ECharts'customValues(available since 5.5; we are on 6.1). Grains ECharts already places correctly keep their calendar-nice labels, so this is not a blanket change to every temporal axis.hideOverlapstill thins labels on wide ranges — 2000 candidate ticks render as 19 labels — so dense weekly charts stay readable and every label that survives is a real bucket.This also removes the reason to reach for the string/categorical-axis workaround, which silently disables Time Comparison: the axis stays a time axis, so time-shift keeps working.
Applies to Line, Bar, Area, Step, Smooth Line and Scatter (shared
Timeseries/transformProps) and to Mixed Timeseries, where both queries contribute buckets. Gantt's time axis is deliberately left alone — it plots durations, not buckets.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
comparison.mp4
TESTING INSTRUCTIONS
cleaned_sales_data).%m-%d.be one of them. On master, several are not, and some snap to month starts.
master.
1 year ago) and confirm it still renders — previouslythe only way to get correct weekly labels was a categorical axis, which disables it.
Unit tests:
npm run test -- plugins/plugin-chart-echarts(923 tests, 13 new).ADDITIONAL INFORMATION