codeant-ai-for-open-source[bot] commented on code in PR #38522:
URL: https://github.com/apache/superset/pull/38522#discussion_r2906629488
##########
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts:
##########
@@ -370,20 +370,32 @@ export default function transformProps(
metricLabel,
);
const paramsValue = params.value as (string | number)[];
- const x = paramsValue?.[0];
- const y = paramsValue?.[1];
+ // paramsValue contains [xIndex, yIndex, metricValue, rankValue?]
+ // We need to look up the actual axis values from the sorted arrays
+ const xIndex = paramsValue?.[0] as number;
+ const yIndex = paramsValue?.[1] as number;
const value = paramsValue?.[2] as number | null | undefined;
- const formattedX = xAxisFormatter(x);
- const formattedY = yAxisFormatter(y);
+ const xValue = sortedXAxisValues[xIndex];
+ const yValue = sortedYAxisValues[yIndex];
+ // Format the axis values for display (handle null/undefined with
empty string fallback)
+ // Convert to string/number for formatter compatibility
+ const formattedX =
+ xValue !== null && xValue !== undefined
+ ? xAxisFormatter(xValue as string | number)
+ : '';
+ const formattedY =
+ yValue !== null && yValue !== undefined
+ ? yAxisFormatter(yValue as string | number)
+ : '';
const formattedValue = valueFormatter(value);
let percentage = 0;
let suffix = 'heatmap';
if (typeof value === 'number') {
if (normalizeAcross === 'x') {
- percentage = value / totals.x[x];
+ percentage = value / totals.x[String(xValue)];
suffix = formattedX;
} else if (normalizeAcross === 'y') {
- percentage = value / totals.y[y];
+ percentage = value / totals.y[String(yValue)];
suffix = formattedY;
} else {
percentage = value / totals.total;
Review Comment:
**Suggestion:** When `normalizeAcross` is `'x'` or `'y'` (or the whole
heatmap), the code divides by the corresponding total without checking if that
total is zero or undefined, so cases where all values for an axis (or the whole
heatmap) are 0 will produce NaN percentages in the tooltip. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Heatmap tooltips show NaN percentage for zero totals.
- ⚠️ Users misinterpret normalized heatmaps with all-zero categories.
```
</details>
```suggestion
const formattedValue = valueFormatter(value);
let percentage = 0;
let suffix = 'heatmap';
if (typeof value === 'number') {
if (normalizeAcross === 'x') {
const xTotal = totals.x[String(xValue)] || 0;
percentage = xTotal ? value / xTotal : 0;
suffix = formattedX;
} else if (normalizeAcross === 'y') {
const yTotal = totals.y[String(yValue)] || 0;
percentage = yTotal ? value / yTotal : 0;
suffix = formattedY;
} else {
const total = totals.total || 0;
percentage = total ? value / total : 0;
suffix = 'heatmap';
}
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In
`superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts`,
call
the default-exported `transformProps` with `formData.normalizeAcross` set to
`'x'` and a
metric configured in `formData.metric` (function definition and
`echartOptions`
construction are in this file; tooltip is configured starting at lines
360–405 in the
diff).
2. Provide `queriesData[0].data` such that for at least one x-axis category
all metric
values are `0` (e.g., two rows with the same x value, different y values,
and metric
column `metricLabel` equal to `0`), so that in `calculateTotals` (lines
~310–336 in the
same file) the corresponding `totals.x[thatXValue]` and the overall
`totals.total` remain
`0`.
3. When `transformProps` builds the series data (lines ~330–359), it encodes
each data
point as `[xIndex, yIndex, metricValue]`; then ECharts later calls the
tooltip formatter
configured at lines 365–405 with a `params` whose `value` is, for the
all-zero cell,
`[xIndex, yIndex, 0]`.
4. Inside the tooltip formatter at lines 390–401, the code executes
`percentage = value /
totals.x[String(xValue)]` (or `/ totals.y[...]` or `/ totals.total`
depending on
`normalizeAcross`); with `value === 0` and the corresponding total being
`0`, this
computes `0 / 0`, producing `NaN`, which is then passed to
`percentFormatter` and rendered
as a `NaN%` percentage in the tooltip HTML returned by `tooltipHtml`.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/plugin-chart-echarts/src/Heatmap/transformProps.ts
**Line:** 390:401
**Comment:**
*Logic Error: When `normalizeAcross` is `'x'` or `'y'` (or the whole
heatmap), the code divides by the corresponding total without checking if that
total is zero or undefined, so cases where all values for an axis (or the whole
heatmap) are 0 will produce NaN percentages in the tooltip.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38522&comment_hash=4f9f013e89a5a30cf624aa4902b4c7d85ea240b77559839bb5092b5e2688656e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38522&comment_hash=4f9f013e89a5a30cf624aa4902b4c7d85ea240b77559839bb5092b5e2688656e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/Heatmap/transformProps.test.ts:
##########
@@ -359,4 +359,171 @@ describe('Heatmap transformProps', () => {
);
expect((resultWithoutLegend.echartOptions.legend as any).show).toBe(false);
});
+
+ test('tooltip formatter should display actual axis values, not indices', ()
=> {
+ const chartProps = createChartProps({
+ sortXAxis: 'alpha_asc',
+ sortYAxis: 'alpha_asc',
+ });
+ const result = transformProps(chartProps as HeatmapChartProps);
+
+ const tooltipFormatter = (result.echartOptions.tooltip as any).formatter;
+ expect(typeof tooltipFormatter).toBe('function');
+
+ // Simulate tooltip data: [xIndex, yIndex, value]
+ // With alpha_asc sorting: xAxis = ['Friday', 'Monday', 'Thursday',
'Tuesday', 'Wednesday']
+ // yAxis = [9, 10, 11, 14, 15, 16]
+ // So index [1, 2, 15] should map to 'Monday' and hour 11
+ const mockParams = {
+ value: [1, 2, 15],
+ };
+
+ const tooltipHtml = tooltipFormatter(mockParams);
+
+ // Tooltip should contain the actual day name 'Monday', not the index '1'
+ expect(tooltipHtml).toContain('Monday');
+ // Tooltip should contain the actual hour '11', not the index '2'
+ expect(tooltipHtml).toContain('11');
+ // Should not contain raw indices
+ expect(tooltipHtml).not.toMatch(/\b1\s*\(/);
+ expect(tooltipHtml).not.toMatch(/\(\s*2\b/);
+ });
+
+ test('tooltip formatter should work with different sort orders', () => {
+ // Test with descending sort
+ const chartProps = createChartProps({
+ sortXAxis: 'alpha_desc',
+ sortYAxis: 'alpha_desc',
+ });
+ const result = transformProps(chartProps as HeatmapChartProps);
+
+ const tooltipFormatter = (result.echartOptions.tooltip as any).formatter;
+
+ // With alpha_desc sorting: xAxis = ['Wednesday', 'Tuesday', 'Thursday',
'Monday', 'Friday']
+ // yAxis = [16, 15, 14, 11, 10, 9]
+ // So index [4, 5, 20] should map to 'Friday' and hour 9
+ const mockParams = {
+ value: [4, 5, 20],
+ };
+
+ const tooltipHtml = tooltipFormatter(mockParams);
+
+ expect(tooltipHtml).toContain('Friday');
+ expect(tooltipHtml).toContain('9');
+ });
+
+ test('tooltip formatter should work with value-based sorting', () => {
+ const chartProps = createChartProps({
+ sortXAxis: 'value_asc',
+ sortYAxis: 'value_asc',
+ });
+ const result = transformProps(chartProps as HeatmapChartProps);
+
+ const tooltipFormatter = (result.echartOptions.tooltip as any).formatter;
+
+ // With value_asc sorting on X: ['Wednesday', 'Tuesday', 'Thursday',
'Friday', 'Monday']
+ // With value_asc sorting on Y: [11, 9, 10, 14, 15, 16]
+ // So index [0, 0, 8] should map to 'Wednesday' and hour 11
+ const mockParams = {
+ value: [0, 0, 8],
+ };
+
+ const tooltipHtml = tooltipFormatter(mockParams);
+
+ expect(tooltipHtml).toContain('Wednesday');
+ expect(tooltipHtml).toContain('11');
+ });
+
+ test('tooltip percentage calculation should use actual values, not indices',
() => {
+ const testData = [
+ { day_of_week: 'Monday', hour: 9, count: 10 },
+ { day_of_week: 'Monday', hour: 10, count: 20 },
+ { day_of_week: 'Tuesday', hour: 9, count: 30 },
+ ];
+
+ // Test normalizeAcross: 'x'
+ const chartPropsX = createChartProps(
+ {
+ sortXAxis: 'alpha_asc',
+ showPercentage: true,
+ normalizeAcross: 'x',
+ },
+ testData,
+ );
+ const resultX = transformProps(chartPropsX as HeatmapChartProps);
+ const tooltipFormatterX = (resultX.echartOptions.tooltip as any).formatter;
+
+ // With alpha_asc: xAxis = ['Monday', 'Tuesday'], yAxis = [9, 10]
+ // Monday total = 30, Tuesday total = 30
+ // Point [0, 0, 10] = Monday/9 with value 10
+ // Percentage should be 10/30 = 33.33%, not based on index
+ const mockParamsX = {
+ value: [0, 0, 10],
+ };
+
+ const tooltipHtmlX = tooltipFormatterX(mockParamsX);
+ expect(tooltipHtmlX).toContain('33');
+ expect(tooltipHtmlX).toContain('%');
+
+ // Test normalizeAcross: 'y'
+ const chartPropsY = createChartProps(
+ {
+ sortXAxis: 'alpha_asc',
+ showPercentage: true,
+ normalizeAcross: 'y',
+ },
+ testData,
+ );
+ const resultY = transformProps(chartPropsY as HeatmapChartProps);
+ const tooltipFormatterY = (resultY.echartOptions.tooltip as any).formatter;
+
+ // Hour 9 total = 40 (10 + 30)
+ // Point [0, 0, 10] = Monday/9 with value 10
+ // Percentage should be 10/40 = 25%
+ const mockParamsY = {
+ value: [0, 0, 10],
+ };
+
+ const tooltipHtmlY = tooltipFormatterY(mockParamsY);
+ expect(tooltipHtmlY).toContain('25');
+ expect(tooltipHtmlY).toContain('%');
+ });
+
+ 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');
+ });
Review 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]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Heatmap numeric-axis tooltip test uses inconsistent metric
configuration.
- ⚠️ Misconfigured test may miss regressions in metric-based logic.
```
</details>
```suggestion
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');
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
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.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38522&comment_hash=5a4e653affdcea93e56edafcc9f610d420abde377c799056c3513d81d972a4f5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38522&comment_hash=5a4e653affdcea93e56edafcc9f610d420abde377c799056c3513d81d972a4f5&reaction=dislike'>👎</a>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
