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 2 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}
| const timestamp = | ||
| value instanceof Date ? value.getTime() : Number(value ?? NaN); |
There was a problem hiding this comment.
Suggestion: Temporal query results commonly contain ISO date strings, but Number(value) returns NaN for values such as 2026-04-06T00:00:00Z. As a result, temporalTickValues becomes undefined for those weekly charts and ECharts keeps generating drifting calendar ticks. Normalize string timestamps with the existing normalizeTimestamp utility before converting them to milliseconds. [type error]
Severity Level: Major ⚠️
- ❌ Weekly ISO-string charts lose bucket-aligned axis ticks.
- ⚠️ ECharts renders calendar ticks instead of query buckets.
- ⚠️ Weekly labels and gridlines can drift from plotted points.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts
**Line:** 1015:1016
**Comment:**
*Type Error: Temporal query results commonly contain ISO date strings, but `Number(value)` returns `NaN` for values such as `2026-04-06T00:00:00Z`. As a result, `temporalTickValues` becomes `undefined` for those weekly charts and ECharts keeps generating drifting calendar ticks. Normalize string timestamps with the existing `normalizeTimestamp` utility before converting them to milliseconds.
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 fix| const WEEK_MS = 7 * 24 * 3600 * 1000; | ||
| const MONDAYS = Array.from( | ||
| { length: 13 }, | ||
| (_, i) => Date.UTC(2026, 3, 6) + i * WEEK_MS, | ||
| ); |
There was a problem hiding this comment.
Suggestion: The regression fixtures use numeric millisecond timestamps exclusively, so they do not exercise the ISO date-string values commonly returned for temporal query columns. If temporal strings are converted with Number(value), they become NaN and no custom ticks are produced, while all these tests still pass. Use representative ISO timestamp strings in at least one fixture and assert the normalized numeric tick values. [possible bug]
Severity Level: Major ⚠️
- ❌ Weekly charts with ISO temporal values lose bucket-aligned ticks.
- ⚠️ Labels revert to ECharts' drifting calendar tick generation.
- ⚠️ Weekly gridlines no longer align with plotted buckets.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts
**Line:** 2536:2540
**Comment:**
*Possible Bug: The regression fixtures use numeric millisecond timestamps exclusively, so they do not exercise the ISO date-string values commonly returned for temporal query columns. If temporal strings are converted with `Number(value)`, they become `NaN` and no custom ticks are produced, while all these tests still pass. Use representative ISO timestamp strings in at least one fixture and assert the normalized numeric tick 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 fix![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
| export function getTemporalTickValues( | ||
| data: DataRecord[], | ||
| xAxisLabel: string, | ||
| xAxisType: AxisType, | ||
| timeGrain?: string, | ||
| ): number[] | undefined { | ||
| if ( | ||
| xAxisType !== AxisType.Time || | ||
| !timeGrain || | ||
| !WEEKLY_TIME_GRAINS.has(timeGrain) | ||
| ) { | ||
| return undefined; | ||
| } | ||
| const values = new Set<number>(); | ||
| data.forEach(row => { | ||
| const value = row[xAxisLabel]; | ||
| const timestamp = | ||
| value instanceof Date ? value.getTime() : Number(value ?? NaN); | ||
| if (Number.isFinite(timestamp)) { | ||
| values.add(timestamp); | ||
| } | ||
| }); | ||
| return values.size ? [...values].sort((a, b) => a - b) : undefined; | ||
| } |
There was a problem hiding this comment.
The new getTemporalTickValues function (lines 999-1022) handles business-critical weekly time grain tick generation for ECharts, but has no unit test coverage. Adding tests per [11730] would catch edge cases (empty arrays, mixed Date/numeric inputs, non-weekly grains) before they surface in integration tests.
Code Review Run #0a3425
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| const temporalTickValues = getTemporalTickValues( | ||
| [...rebasedDataA, ...rebasedDataB], | ||
| xAxisLabel, | ||
| xAxisType, | ||
| resolvedTimeGrain, | ||
| ); |
There was a problem hiding this comment.
Suggestion: The tick helper is invoked with raw rebased query values, but temporal query results can contain ISO date strings, which ECharts accepts as valid time-axis values. getTemporalTickValues converts non-Date values with Number(...), so values such as 2026-04-06T00:00:00Z become NaN and are discarded; weekly MixedTimeseries charts using string timestamps therefore fall back to ECharts' drifting calendar ticks instead of being pinned. Parse temporal strings with the same date conversion used by the axis/data pipeline before generating custom values. [type error]
Severity Level: Major ⚠️
- ⚠️ Labels and gridlines revert to drifting calendar positions.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts
**Line:** 762:767
**Comment:**
*Type Error: The tick helper is invoked with raw rebased query values, but temporal query results can contain ISO date strings, which ECharts accepts as valid time-axis values. `getTemporalTickValues` converts non-`Date` values with `Number(...)`, so values such as `2026-04-06T00:00:00Z` become `NaN` and are discarded; weekly MixedTimeseries charts using string timestamps therefore fall back to ECharts' drifting calendar ticks instead of being pinned. Parse temporal strings with the same date conversion used by the axis/data pipeline before generating custom 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 fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43339 +/- ##
=======================================
Coverage 66.67% 66.67%
=======================================
Files 2876 2876
Lines 164007 164037 +30
Branches 37834 37853 +19
=======================================
+ Hits 109347 109372 +25
- Misses 52514 52519 +5
Partials 2146 2146
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 |
1 participant
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