Repository navigation
Conversation
Code Review Agent Run #d8d8e3Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| if (!timeGrain) { | ||
| return baseFormatter; | ||
| return new TimeFormatter({ | ||
| id: SMART_DATE_ID, | ||
| label: baseFormatter.label, | ||
| formatFunc: (date: Date) => { | ||
| const floored = new Date(date); | ||
| floored.setMilliseconds(0); | ||
| return baseFormatter(floored); | ||
| }, | ||
| }); |
There was a problem hiding this comment.
Suggestion: With no grain, legitimate sub-second timestamps are floored before formatting, so distinct ticks within one second receive identical labels and lose their only displayed precision.
Assessment: 🟠 Major · 🔁 Occurrence: Sometimes · 🏷️ Logic error
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:** 49:58
**Comment:**
*Logic Error: With no grain, legitimate sub-second timestamps are floored before formatting, so distinct ticks within one second receive identical labels and lose their only displayed precision.
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.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of 784b0fb.
When no time grain is provided, the function returns the base formatter before creating or normalizing a date, preserving sub-second timestamps.
If that's not right, unresolve this thread and CodeAnt will leave it open.
There was a problem hiding this comment.
✅ CodeAnt verified this suggestion was addressed in subsequent commits and marked this thread resolved as of d1f8501.
When no time grain is provided, the function returns the base formatter before creating or normalizing a date, preserving sub-second timestamps.
If that's not right, unresolve this thread and CodeAnt will leave it open.
|
The flagged issue is correct. The current implementation in the PR explicitly floors the date to the second by setting milliseconds to zero, which causes distinct timestamps within the same second to be formatted identically, losing sub-second precision. To resolve this while still avoiding the millisecond tier for axis-boundary noise, you should remove the forced flooring and instead configure the formatter to ignore sub-second precision only when it is not explicitly required. A better approach is to ensure the formatter's adaptive logic does not trigger the millisecond tier for these specific boundary labels, or to use a format that does not include milliseconds by default. Since there are no other comments in this PR, no further actions are required. superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #44701 +/- ##
===========================================
+ Coverage 57.31% 82.32% +25.00%
===========================================
Files 2997 2997
Lines 185294 185302 +8
Branches 42888 42896 +8
===========================================
+ Hits 106198 152544 +46346
+ Misses 78056 29996 -48060
- Partials 1040 2762 +1722
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:
|
|
Valid point — verified the duplication is exact: with |
Code Review Agent Run #48ecc4Actionable 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 |
|
Thanks for the writeup on the root cause here, @kokhlo, that's a solid RCA. One thing before this merges though: codeant's still-open thread on |
github-actions
Bot
added
the
requires:rebase
9023d01 to
9cf2b29
Compare
|
Rebased onto current The one hunk that needed a decision was in the test file: Diff vs |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
github-actions
Bot
removed
the
requires:rebase
Code Review Agent Run #8be9dbActionable 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 |
github-actions
Bot
added
the
requires:rebase
# Conflicts: # superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts
… precision Target the boundary labels specifically instead of flooring every no-grain tick: createPaddedExtentFloorFormatter wraps the axis formatter and floors to the second only ticks that fall outside the data domain (ECharts axis-extent padding), while ticks inside the domain keep their sub-second precision. Wired in Timeseries and MixedTimeseries via getXAxisDomain.
|
You're right — flooring every no-grain tick throws away precision the series may actually carry. Reworked in 784b0fb to target the boundary label specifically, and dropped the shared-wrapper change in `getSmartDateFormatter` entirely (the no-grain path returns the raw base formatter again, as on master). The flooring now lives in a new `createPaddedExtentFloorFormatter` wired into the Timeseries and MixedTimeseries axes: it receives the data domain from `getXAxisDomain` and floors to the second only ticks that fall outside that domain — ECharts axis-extent padding. Ticks inside the domain pass through untouched. Verified through the real `transformProps` (Top-10-Games-shaped chart, temporal `year` column, no grain), same three ticks on each head:
Also rebased onto current master — the conflict with the new `coerceTemporalMs`/`getXAxisDomain` tests is resolved by keeping both sides (`mergeable: true`). Unit tests: formatters + Timeseries + MixedTimeseries suites, 518 passed. |
github-actions
Bot
added
risk:chart-metadata
and removed
requires:rebase
![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 #0fd67a
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts - 1
- duplicated formatter-id propagation · Line 435-437
Additional Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts - 1
-
Redundant domain scan · Line 765-771`getXAxisDomain` now runs unconditionally on every render, but its result is only consumed when `xAxisType === AxisType.Time` (via `createPaddedExtentFloorFormatter` and `showMaxLabel`). For non-Time (e.g. category) axes this adds a full O(n) scan over all records — `coerceTemporalMs` constructs a `new Date` per string value — that was previously skipped. Guard it behind the Time check.
-
Review Details
-
Files reviewed - 4 · Commit Range:
9cf2b29..784b0fb- 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/utils/formatters.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.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 an incremental AI Review. -
/review full- 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
…le domain scan Two review findings on the padded-extent formatter. Every x-axis formatter factory had to copy the wrapped formatter's id onto its wrapper, or ECharts re-renders each tick because its label cache misses. The third copy made the contract three times over, so it now lives in one helper. The data-extent scan coerces every record's x value, and only the two Time-axis formatters read its result, so it now runs behind that check instead of on every render of a category axis.
Code Review Agent Run #db0b69Actionable 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 |
This branch has not been deployed
2 participants
SUMMARY
Fixes #44698 (claim comment with the full RCA: #44698 (comment))
On a temporal x-axis with no time grain (e.g. the bundled Top 10 Games example, whose temporal
yearBIGINT column is used directly with notime_grain_sqla), the last axis label renders as.943msinstead of a year.Root cause chain, verified on main:
getXAxisFormatter(xAxisTimeFormat, resolvedTimeGrain)→ with no grain,getSmartDateFormatter(undefined)returns the rawsmart_dateformatter — thesetMilliseconds(0)normalization exists only inside the grain-aware wrapper (plugins/plugin-chart-echarts/src/utils/formatters.ts).smart_date's finest tier ismillisecond: '.%Lms'(packages/superset-ui-core/src/time-format/formatters/smartDate.ts).showMaxLabelboundary label formats the padded value — which carries sub-second noise. Confirmed against the repo's own echarts: the forced max label receives values like2009-01-01T00:00:00.943Z, rendering.943mswhile every real tick is a clean year.This regressed when #38017/#41350 replaced #38083's grain-agnostic floor-to-second axis wrapper with the grain-aware normalizer — the no-grain path lost the protection.
BEFORE/AFTER
formatter(new Date('2009-01-01T00:00:00.943Z'))→".943ms""2009"(identical to the clean2009-01-01T00:00:00Ztick)TESTING
test/utils/formatters.test.ts: RED on main (.943msvs2009), GREEN with the fixformatters.test.ts24 passed,test/Timeseries14 suites / 384 passed,test/MixedTimeseries4 suites / 65 passed, oxfmt + oxlint cleanADDITIONAL INFORMATION