fix(plugin-chart-echarts): fallback temporal formatters when labels would show NaN#39208
fix(plugin-chart-echarts): fallback temporal formatters when labels would show NaN#39208JCelento wants to merge 5 commits intoapache:masterfrom
Conversation
Code Review Agent Run #688c4bActionable 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 |
dosubot
bot
added
plugins
viz:charts:echarts
JCelento
force-pushed
the
fix/echarts-timeseries-epoch-x-axis-labels
branch
from
5db7b68 to
40365c9
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
JCelento
force-pushed
the
fix/echarts-timeseries-epoch-x-axis-labels
branch
from
1b6cff6 to
8c645e4
Compare
Code Review Agent Run #06cc12Actionable 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39208 +/- ##
==========================================
+ Coverage 64.40% 64.43% +0.02%
==========================================
Files 2553 2553
Lines 132558 132616 +58
Branches 30746 30765 +19
==========================================
+ Hits 85377 85447 +70
+ Misses 45695 45683 -12
Partials 1486 1486
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JCelento
force-pushed
the
fix/echarts-timeseries-epoch-x-axis-labels
branch
2 times, most recently
from
dea3109 to
e677a20
Compare
Code Review Agent Run #ab9d86Actionable 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 |
JCelento
force-pushed
the
fix/echarts-timeseries-epoch-x-axis-labels
branch
2 times, most recently
from
ff8e2ae to
06269c4
Compare
![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 #cf00b3
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts - 1
- Type Mismatch in Return Type · Line 173-176
Review Details
-
Files reviewed - 8 · Commit Range:
3c97222..06269c4- 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/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
- superset-frontend/plugins/plugin-chart-echarts/test/utils/formatters.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/utils/series.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 getXAxisFormatter( | ||
| format?: string, | ||
| timeGrain?: string, | ||
| ): TimeFormatter | StringConstructor | undefined { | ||
| ): TimeFormatter | undefined { |
There was a problem hiding this comment.
The getXAxisFormatter function's return type is declared as TimeFormatter | undefined, but the implementation always returns a TimeFormatter. This type mismatch could confuse callers and lead to unnecessary null checks, especially since existing usage asserts non-null with '!'. Update the type to match the actual behavior.
Code suggestion
Check the AI-generated fix before applying
| export function getXAxisFormatter( | |
| format?: string, | |
| timeGrain?: string, | |
| ): TimeFormatter | StringConstructor | undefined { | |
| ): TimeFormatter | undefined { | |
| export function getXAxisFormatter( | |
| format?: string, | |
| timeGrain?: string, | |
| ): TimeFormatter { |
Code Review Run #cf00b3
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
There was a problem hiding this comment.
getInstance().get(formatId) at line 73 of TimeFormatterRegistrySingleton.ts can return undefined
There was a problem hiding this comment.
No, the suggestion isn’t fully correct — getTimeFormatter can return undefined when an unrecognized format is passed, so getXAxisFormatter may also return undefined in that case.
superset-frontend/plugins/plugin-chart-echarts/src/utils/formatters.ts
export function getXAxisFormatter(
format?: string,
timeGrain?: string,
): TimeFormatter | undefined {
if (format === SMART_DATE_ID || !format) {
return getSmartDateFormatter(timeGrain);
}
return getTimeFormatter(format);
}
2b8d12c to
700eb81
Compare
Code Review Agent Run #a13ce3Actionable 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 |
JCelento
force-pushed
the
fix/echarts-timeseries-epoch-x-axis-labels
branch
from
b35850b to
8a94469
Compare
Code Review Agent Run #1b4967Actionable 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 |
…r and getXAxisFormatter getTooltipTimeFormatter previously returned StringConstructor when no format was provided, and getXAxisFormatter had an unreachable return String branch. Both are passed to withNaNFallback which requires TimeFormatter, causing TS2345 type errors. Changed both to always return TimeFormatter by defaulting to smart date formatters instead of String.
JCelento
force-pushed
the
fix/echarts-timeseries-epoch-x-axis-labels
branch
from
8a94469 to
70cca18
Compare
Code Review Agent Run #2daf30Actionable 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 |
1 participant
SUMMARY
Temporal tooltips and x-axis labels on ECharts Timeseries and Mixed timeseries charts could render strings
containing
NaNwhen the primary d3-based formatter failed on invalid or edge-case timestamp values.This change introduces
withNaNFallbackinplugin-chart-echartsto wrap the existing tooltip and x-axistemporal formatters: if the primary result is not a safe string (contains
NaN, or throws), we fall back to thesmart date / verbose smart date formatters, and finally
String(value)so labels never show as brokenNaNtext.TESTING INSTRUCTIONS
time where formatting could fail).
NaN/NaN/NaN/NaNstyle output.cd superset-frontend && npm test -- plugins/plugin-chart-echarts/test/(or your usual test command for thatpackage).
ADDITIONAL INFORMATION