New Feature : Add an option - Color by X axis to Bar Charts , when no… #37531
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… dimensions are set
Contributor
Code Review Agent Run #1a30b3Actionable 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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+405
to
+408
| xAxisValues.forEach(xValue => { | ||
| const colorKey = String(xValue); | ||
| series.push({ | ||
| name: String(xValue), |
Contributor
There was a problem hiding this comment.
Suggestion: Performance & correctness: when creating hidden legend series you append one series per x-axis data point without deduplicating, which can create duplicate legend entries and a large number of unnecessary series; deduplicate x-axis values first before pushing hidden series. [performance]
Severity Level: Major ⚠️
- ❌ Timeseries chart legend shows duplicate entries.
- ⚠️ Increased series[] size slows ECharts rendering.
- ⚠️ Legend clutter harms user color interpretation.
Suggested change
| xAxisValues.forEach(xValue => { | |
| const colorKey = String(xValue); | |
| series.push({ | |
| name: String(xValue), | |
| // Deduplicate x-axis values to avoid duplicate legend entries and excessive series | |
| const uniqueXValues = Array.from( | |
| new Set( | |
| xAxisValues | |
| .map(v => (v === null || v === undefined ? String(v) : String(v))), | |
| ), | |
| ); | |
| uniqueXValues.forEach(xValue => { | |
| const colorKey = String(xValue); | |
| series.push({ | |
| name: xValue, |
Steps of Reproduction ✅
1. Open the Timeseries chart code path: transformProps in
plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts (function transformProps).
The x-axis legend augmentation code lives at lines ~378-423 and the hidden-series creation
is at lines 404-421.
2. Enable the new option in formData: showColorByXAxis = true and ensure groupBy is empty
(groupBy.length === 0). These formData values are read at the top of transformProps
(extracted into showColorByXAxis and groupBy).
3. Provide a dataset such that the first rendered series (series[0]) contains repeated x
values (e.g., firstSeries.data contains multiple points with the same x value).
transformProps extracts xAxisValues by iterating firstSeries.data and pushing every x it
finds (lines 390-402).
4. transformProps then executes the hidden-series creation loop at lines 404-421, pushing
one hidden series per xAxisValues entry (no deduplication). Observe duplicate legend items
in the rendered chart legend (legend.data is later driven by legendData built from the
same firstSeries.data at lines ~565-582), and a larger-than-necessary series[] length
causing extra work when ECharts processes series.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 405:408
**Comment:**
*Performance: Performance & correctness: when creating hidden legend series you append one series per x-axis data point without deduplicating, which can create duplicate legend entries and a large number of unnecessary series; deduplicate x-axis values first before pushing hidden series.
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.
Comment on lines
+566
to
+580
| ? // When showColorByXAxis is enabled, show only x-axis values | ||
| (() => { | ||
| const firstSeries = series[0]; | ||
| if (firstSeries && Array.isArray(firstSeries.data)) { | ||
| return (firstSeries.data as any[]).map(point => { | ||
| if (point && typeof point === 'object' && 'value' in point) { | ||
| const val = point.value; | ||
| return String(Array.isArray(val) ? val[0] : val); | ||
| } else if (Array.isArray(point)) { | ||
| return String(point[0]); | ||
| } else { | ||
| return String(point); | ||
| } | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
Suggestion: Logic/robustness: legendData is built from firstSeries.data and can include duplicate or empty/undefined entries; filter out empty/undefined names and deduplicate them so legend items are stable and meaningful. [logic error]
Severity Level: Major ⚠️
- ❌ Timeseries legend shows empty or duplicated labels.
- ⚠️ Legend sorting and selection behave inconsistently.
- ⚠️ User confusion interpreting color-by-x legend.
Suggested change
| ? // When showColorByXAxis is enabled, show only x-axis values | |
| (() => { | |
| const firstSeries = series[0]; | |
| if (firstSeries && Array.isArray(firstSeries.data)) { | |
| return (firstSeries.data as any[]).map(point => { | |
| if (point && typeof point === 'object' && 'value' in point) { | |
| const val = point.value; | |
| return String(Array.isArray(val) ? val[0] : val); | |
| } else if (Array.isArray(point)) { | |
| return String(point[0]); | |
| } else { | |
| return String(point); | |
| } | |
| }); | |
| } | |
| ? // When showColorByXAxis is enabled, show only x-axis values (filtered + deduped) | |
| (() => { | |
| const firstSeries = series[0]; | |
| if (firstSeries && Array.isArray(firstSeries.data)) { | |
| const names = (firstSeries.data as any[]).map(point => { | |
| if (point && typeof point === 'object' && 'value' in point) { | |
| const val = point.value; | |
| return String(Array.isArray(val) ? val[0] : val); | |
| } else if (Array.isArray(point)) { | |
| return String(point[0]); | |
| } else { | |
| return String(point); | |
| } | |
| }) | |
| // filter out empty/invalid entries and deduplicate | |
| .filter(name => name !== '' && name !== 'undefined' && name !== 'null'); | |
| return Array.from(new Set(names)); |
Steps of Reproduction ✅
1. Call transformProps in plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
with formData.showColorByXAxis = true and no groupBy (groupBy.length === 0). The
legendData computation is at lines ~565-592.
2. If the first rendered series (series[0]) contains points where the x value is
null/undefined/empty, the mapping at lines 569-578 will convert those to the strings
"null" or "undefined" or empty strings, and include them in the returned array.
3. If the firstSeries.data contains duplicate x values, the current map returns duplicates
(no deduplication). These values are later fed to echartOptions.legend.data (see legend
configuration at lines ~771-792), producing duplicate or meaningless legend entries in the
chart UI.
4. Reproduce in UI: render a Timeseries chart with showColorByXAxis enabled and a dataset
where firstSeries.data includes duplicate or empty x values; verify duplicate/empty legend
items appear. Fixing by filtering out empty values and deduplicating (as suggested) yields
stable, meaningful legend items.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts
**Line:** 566:580
**Comment:**
*Logic Error: Logic/robustness: `legendData` is built from `firstSeries.data` and can include duplicate or empty/undefined entries; filter out empty/undefined names and deduplicate them so legend items are stable and meaningful.
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.
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Unchecked color by x axis (Identical to original behavior)-
Checked color by x axis-
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION