fix(heatmap): correct tooltip axis value lookup and percentage calculations and add tests - #38522
fix(heatmap): correct tooltip axis value lookup and percentage calculations and add tests#38522yousoph wants to merge 1 commit into
Conversation
codeant-ai-for-open-source
Bot
added
the
size:L
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| test('tooltip formatter should handle numeric axes correctly', () => { | ||
| const numericData = [ | ||
| { year: 2020, quarter: 1, revenue: 100 }, | ||
| { year: 2021, quarter: 2, revenue: 150 }, | ||
| { year: 2022, quarter: 3, revenue: 200 }, | ||
| ]; | ||
|
|
||
| const chartProps = createChartProps( | ||
| { | ||
| sortXAxis: 'alpha_asc', | ||
| sortYAxis: 'alpha_asc', | ||
| xAxis: 'year', | ||
| groupby: ['quarter'], | ||
| }, | ||
| numericData, | ||
| ); | ||
|
|
||
| (chartProps as any).queriesData[0].colnames = [ | ||
| 'year', | ||
| 'quarter', | ||
| 'revenue', | ||
| ]; | ||
|
|
||
| const result = transformProps(chartProps as HeatmapChartProps); | ||
| const tooltipFormatter = (result.echartOptions.tooltip as any).formatter; | ||
|
|
||
| // With alpha_asc: xAxis = [2020, 2021, 2022], yAxis = [1, 2, 3] | ||
| // Index [1, 1, 150] should map to year 2021 and quarter 2 | ||
| const mockParams = { | ||
| value: [1, 1, 150], | ||
| }; | ||
|
|
||
| const tooltipHtml = tooltipFormatter(mockParams); | ||
|
|
||
| expect(tooltipHtml).toContain('2021'); | ||
| expect(tooltipHtml).toContain('2'); | ||
| }); |
There was a problem hiding this comment.
Suggestion: In the numeric-axes tooltip test, the formData metric is left as 'count' while the data and colnames use 'revenue', so transformProps reads undefined metric values and computes incorrect totals, making the test setup inconsistent with real usage and potentially hiding issues in percentage logic. [logic error]
Severity Level: Major ⚠️
- ⚠️ Heatmap numeric-axis tooltip test uses inconsistent metric configuration.
- ⚠️ Misconfigured test may miss regressions in metric-based logic.| test('tooltip formatter should handle numeric axes correctly', () => { | |
| const numericData = [ | |
| { year: 2020, quarter: 1, revenue: 100 }, | |
| { year: 2021, quarter: 2, revenue: 150 }, | |
| { year: 2022, quarter: 3, revenue: 200 }, | |
| ]; | |
| const chartProps = createChartProps( | |
| { | |
| sortXAxis: 'alpha_asc', | |
| sortYAxis: 'alpha_asc', | |
| xAxis: 'year', | |
| groupby: ['quarter'], | |
| }, | |
| numericData, | |
| ); | |
| (chartProps as any).queriesData[0].colnames = [ | |
| 'year', | |
| 'quarter', | |
| 'revenue', | |
| ]; | |
| const result = transformProps(chartProps as HeatmapChartProps); | |
| const tooltipFormatter = (result.echartOptions.tooltip as any).formatter; | |
| // With alpha_asc: xAxis = [2020, 2021, 2022], yAxis = [1, 2, 3] | |
| // Index [1, 1, 150] should map to year 2021 and quarter 2 | |
| const mockParams = { | |
| value: [1, 1, 150], | |
| }; | |
| const tooltipHtml = tooltipFormatter(mockParams); | |
| expect(tooltipHtml).toContain('2021'); | |
| expect(tooltipHtml).toContain('2'); | |
| }); | |
| test('tooltip formatter should handle numeric axes correctly', () => { | |
| const numericData = [ | |
| { year: 2020, quarter: 1, revenue: 100 }, | |
| { year: 2021, quarter: 2, revenue: 150 }, | |
| { year: 2022, quarter: 3, revenue: 200 }, | |
| ]; | |
| const chartProps = createChartProps( | |
| { | |
| sortXAxis: 'alpha_asc', | |
| sortYAxis: 'alpha_asc', | |
| xAxis: 'year', | |
| groupby: ['quarter'], | |
| metric: 'revenue', | |
| }, | |
| numericData, | |
| ); | |
| (chartProps as any).queriesData[0].colnames = [ | |
| 'year', | |
| 'quarter', | |
| 'revenue', | |
| ]; | |
| const result = transformProps(chartProps as HeatmapChartProps); | |
| const tooltipFormatter = (result.echartOptions.tooltip as any).formatter; | |
| // With alpha_asc: xAxis = [2020, 2021, 2022], yAxis = [1, 2, 3] | |
| // Index [1, 1, 150] should map to year 2021 and quarter 2 | |
| const mockParams = { | |
| value: [1, 1, 150], | |
| }; | |
| const tooltipHtml = tooltipFormatter(mockParams); | |
| expect(tooltipHtml).toContain('2021'); | |
| expect(tooltipHtml).toContain('2'); | |
| }); |
Steps of Reproduction ✅
1. Run the unit tests for the heatmap transform via `npm test -- transformProps.test.ts`
in `superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts`.
2. Observe the test `tooltip formatter should handle numeric axes correctly` starting at
line 492, which calls `createChartProps` without overriding `metric`, so it uses
`baseFormData.metric = 'count'` from the top of the file.
3. In the same test, `numericData` only contains a `revenue` field and
`queriesData[0].colnames` is overridden to `['year', 'quarter', 'revenue']`, so there is
no `count` column for the metric referenced by formData.
4. When `transformProps` is invoked with these `chartProps`, it computes heatmap series
values and any metric-based aggregations using the nonexistent `'count'` metric (reading
undefined/incorrect values), while the test only asserts on axis labels in the tooltip
output, so this inconsistent setup is not detected by the assertions.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts
**Line:** 492:528
**Comment:**
*Logic Error: In the numeric-axes tooltip test, the formData metric is left as `'count'` while the data and `colnames` use `'revenue'`, so `transformProps` reads undefined metric values and computes incorrect totals, making the test setup inconsistent with real usage and potentially hiding issues in percentage logic.
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.
There was a problem hiding this comment.
This one's real... the numeric-axes test should set metric: 'revenue' so totals aren't computed off a missing column. Same fix Joe's assertion note points at.
![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 #746e1e
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts - 1
- Fix index type conversion · Line 375-376
Review Details
-
Files reviewed - 2 · Commit Range:
e508645..4d5d74f- superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts
-
Files skipped - 0
-
Tools
- Eslint (Linter) - ✔︎ Successful
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ 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
4d5d74f to
84c2e5d
Compare
Code Review Agent Run #80dbcdActionable 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 |
Add comprehensive tests to ensure the tooltip formatter correctly displays actual axis values instead of numeric indices. This prevents regression of the bug fixed in the previous commit where tooltips showed "0 (1)" instead of actual labels like "Monday (Morning)". Tests cover: - Tooltip displays actual axis values with alphabetical sorting - Tooltip works correctly with different sort orders (asc/desc) - Tooltip works correctly with value-based sorting - Percentage calculations use actual values when normalizeAcross is enabled - Tooltip handles numeric axes correctly Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
84c2e5d to
076f85d
Compare
There was a problem hiding this comment.
Code Review Agent Run #e016ae
Actionable Suggestions - 1
-
superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts - 1
- Test data mismatch · Line 499-507
Review Details
-
Files reviewed - 2 · Commit Range:
076f85d..076f85d- superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts
- superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/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
There was a problem hiding this comment.
Pull request overview
Adds regression coverage (and adjusts implementation) to ensure ECharts heatmap tooltips display real x/y axis labels rather than axis indices, including correct percentage calculations under normalization.
Changes:
- Add multiple tooltip-focused tests covering axis index → label lookup, sorting modes, normalization percentages, and numeric axes.
- Update heatmap tooltip percentage calculation to key totals by actual axis values (from query
colnames) rather than derived labels/casted strings. - Remove unused label-derivation logic (
getColumnLabel) in favor of query-provided column names.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts | Adds regression tests for tooltip label lookup, sorting, normalization percentages, and numeric axis handling. |
| superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts | Fixes tooltip totals lookup to use real axis column names/values (not indices/labels) for percentage calculations. |
You can also share your feedback on Copilot code review. Take the survey.
| xAxisColumnName, | ||
| yAxisColumnName, |
There was a problem hiding this comment.
Good catch... the SUMMARY still says test-only from before the calculateTotals change landed on this branch. Description should be updated to match the title.
| const tooltipHtml = tooltipFormatter(mockParams); | ||
|
|
||
| expect(tooltipHtml).toContain('2021'); | ||
| expect(tooltipHtml).toContain('2'); |
There was a problem hiding this comment.
probably can get rid of this or need to update to a better expect. If you take a look at L526, it will pass if it contains '2021', but '2' is part of '2021' so thi sline will always pass as long as the above line passes.
yousoph
changed the title
yousoph
added
the
🎪 ⚡ showtime-trigger-start
github-actions
Bot
added
🎪 076f85d 🚦 building
🎪 ⌛ 48h
|
🎪 Showtime deployed environment on GHA for 076f85d • Environment: http://44.250.239.7:8080 (admin/admin) |
msyavuz
left a comment
msyavuz
left a comment
There was a problem hiding this comment.
I am on the fence with this one. Test coverage looks good but it looks like these changes should be failing some regression tests
| // Convert yValue to a key type (string or number) for totals lookup | ||
| const yKey = yValue as string | number; | ||
| percentage = value / totals.y[yKey]; |
There was a problem hiding this comment.
This doesn't actually do any conversions. It is just type assertion for yKey and xKey. Previous behaviour was to cast it as String.
| xAxisColumnName, | ||
| yAxisColumnName, |
There was a problem hiding this comment.
This change might be a regression for this chart when the columns are not physical and have labels
|
@yousoph the core fix here is right... I traced it on master: A few things before we can merge though. Joe's I've resolved the noise/duplicate bot threads above and left the two real ones open. Happy to re-review after those... holler if you want a hand. |
|
Re-pointed this onto my own fork so the review fixes could be pushed directly — continued in #41864, which carries all four requested fixes from @rusackas's list (dropped the no-op |
github-actions
Bot
removed
the
🎪 ⌛ 48h
User description
SUMMARY
Fixes the heatmap tooltip so it displays actual axis values (e.g., "Monday", "Morning") instead of numeric indices (0, 1, 2...), corrects the tooltip percentage calculation, and adds comprehensive regression tests.
Context: After PR #36302 changed the heatmap data structure to use axis indices for proper sorting, the tooltip formatter was not updated to look up actual values from the sorted arrays. This caused tooltips to display raw indices instead of formatted axis labels.
What this PR does:
calculateTotalsoff the querycolnames(the real column names present in the data) instead ofgetColumnLabel(...)— the old keying producedNaNpercentages whenever the derived label diverged from the data columns (notably arraygroupby)normalizeAcrossis enabledWhy these tests matter:
These tests prevent future regressions by verifying the tooltip formatter correctly:
sortedXAxisValuesandsortedYAxisValuesarraystotals.x[xValue]andtotals.y[yValue]BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — the change affects tooltip text/percentage values (indices → axis labels,
NaN%→ correct %), which are covered by the added regression tests.TESTING INSTRUCTIONS
CodeAnt-AI Description
Fix heatmap tooltip to show actual axis labels and correct percentage calculations
What Changed
Impact
✅ Clearer heatmap tooltips✅ Correct heatmap percentage values✅ Fewer tooltip regressions💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.