feat(plugin-chart-echarts): add Butterfly Chart - #43346
Conversation
This reverts commit 35aaeb6.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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 #ce7ea6
Actionable Suggestions - 2
-
superset-frontend/packages/superset-ui-core/src/chart/types/VizType.ts - 1
- CWE-11234: Missing Chart Plugin Implementation · Line 64-64
-
superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/constants.ts - 1
- Dead default without control · Line 29-29
Additional Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/index.ts - 1
-
Missing thumbnail image · Line 49-49The `thumbnail` field uses an empty string `''`, but other comparable chart plugins in this package (e.g., Bullet, Histogram) import and use actual thumbnail images from their `./images/` directories. Adding a proper thumbnail will improve chart identification in the UI gallery.
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/transformProps.ts - 1
- Inconsistent currency formatting · Line 127-129
Review Details
-
Files reviewed - 13 · Commit Range:
19151d1..ee7845d- superset-frontend/packages/superset-ui-core/src/chart/types/VizType.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/Butterfly.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/buildQuery.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/constants.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/controlPanel.tsx
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/index.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/types.ts
- superset-frontend/plugins/plugin-chart-echarts/src/index.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Butterfly/buildQuery.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Butterfly/controlPanel.test.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Butterfly/transformProps.test.ts
- superset-frontend/src/visualizations/presets/MainPreset.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✖︎ Failed
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
| TimePivot = 'time_pivot', | ||
| TimeTable = 'time_table', | ||
| Timeseries = 'echarts_timeseries', | ||
| Tornado = 'tornado', |
There was a problem hiding this comment.
The Tornado enum entry references 'tornado' but no Tornado chart plugin implementation exists anywhere in the codebase (no plugin in plugin-chart-echarts, no import in MainPreset.ts, and no backend implementation). This creates an unusable enum value — if a user selects 'Tornado' as a visualization, the system has no plugin to handle it. Compare with Butterfly which is fully implemented and registered. Either remove this enum entry or implement the complete Tornado chart plugin first. (See also: CWE-11234)
Code Review Run #ce7ea6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| ...DEFAULT_LEGEND_FORM_DATA, | ||
| ...DEFAULT_TITLE_FORM_DATA, | ||
| showValue: true, | ||
| truncateXAxis: true, |
There was a problem hiding this comment.
The truncateXAxis: true property at line 29 is dead code. This property is not exposed as a configurable control in controlPanel.tsx and is not consumed in transformProps.ts — only showValue and xAxisLabelRotation are extracted from formData. Comparing against the similar Bubble chart (which uses truncateXAxis in both controlPanel and transformProps), this Butterfly implementation is incomplete and the default cannot be changed by users.
Code Review Run #ce7ea6
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| export const DEFAULT_FORM_DATA = { | ||
| ...DEFAULT_LEGEND_FORM_DATA, | ||
| ...DEFAULT_TITLE_FORM_DATA, | ||
| showValue: true, |
There was a problem hiding this comment.
Suggestion: The chart default enables showValue, while the corresponding showValueControl defaults to false. When show_value is absent from new or legacy form data, the transform fallback displays labels even though the control panel presents the option as unchecked, causing the chart and saved configuration defaults to disagree. Use the same default in both places. [logic error]
Severity Level: Major ⚠️
- ⚠️ Butterfly charts with absent `show_value` display unexpected bar labels.
- ⚠️ The Chart Options checkbox reports the opposite default state.
- ⚠️ Saved or partially populated form data can preserve this mismatch.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Butterfly/constants.ts
**Line:** 28:28
**Comment:**
*Logic Error: The chart default enables `showValue`, while the corresponding `showValueControl` defaults to false. When `show_value` is absent from new or legacy form data, the transform fallback displays labels even though the control panel presents the option as unchecked, causing the chart and saved configuration defaults to disagree. Use the same default in both places.
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 #43346 +/- ##
==========================================
- Coverage 66.66% 66.65% -0.01%
==========================================
Files 2873 2879 +6
Lines 163654 163735 +81
Branches 37771 37791 +20
==========================================
+ Hits 109093 109141 +48
- Misses 52432 52465 +33
Partials 2129 2129
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:
|
2 participants
SUMMARY
A new visualization type, the Butterfly Chart (based on ECharts), has been added.
The butterfly chart compares two metrics across categories: horizontal bars extend to the left and right from a central axis. It is useful for comparing two indicators across the same categories (e.g., men vs. women, planned vs. actual, or two time periods).
What has been added:
EchartsButterflyChartPlugin(viz_type:butterfly) inplugin-chart-echartsRegistration in
MainPresetandVizTypeQuery: one dimension (groupby), two metrics (
left_metric,right_metric), sorting viaorderby(defaulting to sorting by category)Settings: colors and labels for left/right series, legend, axis formatting, value display on bars, category label rotation
Unit tests for
buildQuery,transformProps, andcontrolPanelTechnical details:
The left metric is displayed with negative values, and the right with positive values (both using
Math.abs)BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
run test
npm run test superset-frontend/plugins/plugin-chart-echarts/test/Butterfly
ADDITIONAL INFORMATION