codeant-ai-for-open-source[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2891981500
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -413,151 +418,172 @@ export default function transformProps(
const array = ensureIsArray(chartProps.rawFormData?.time_compare);
const inverted = invert(verboseMap);
- rawSeriesA.forEach(entry => {
- const entryName = String(entry.name || '');
- const seriesName = inverted[entryName] || entryName;
- const colorScaleKey = getOriginalSeries(seriesName, array);
+ rawSeriesA.forEach(entry => {
+ const entryName = String(entry.name || '');
+ const seriesName = inverted[entryName] || entryName;
+ const colorScaleKey = getOriginalSeries(seriesName, array);
- let displayName: string;
+ let displayName: string;
+ if (truncateMetric) {
+ // Truncate metric: show only the group-by values (or nothing if no
group-by)
+ if (groupby.length > 0) {
+ const groupbyValues = labelMap?.[seriesName] || [];
+ displayName = groupbyValues.join(', ');
+ console.log('Query A - entryName:', entryName, 'seriesName:',
seriesName, 'groupbyValues:', groupbyValues, 'displayName:', displayName);
+ } else {
+ displayName = '';
+ }
+ } else {
+ // Original logic (metric included, optional query identifier)
if (groupby.length > 0) {
- // When we have groupby, format as "metric, dimension"
const metricPart: string = showQueryIdentifiers
? `${MetricDisplayNameA} (Query A)`
: MetricDisplayNameA;
displayName = entryName.includes(metricPart)
? entryName
: `${metricPart}, ${entryName}`;
+
+ console.log('Query A (no trunc) - entryName:', entryName,
'displayName:', displayName);
} else {
- // When no groupby, format as just the entry name with optional query
identifier
displayName = showQueryIdentifiers ? `${entryName} (Query A)` :
entryName;
}
+ }
- const seriesFormatter = getFormatter(
- customFormatters,
- formatter,
- metrics,
- labelMap?.[seriesName]?.[0],
- !!contributionMode,
- );
+ const seriesFormatter = getFormatter(
+ customFormatters,
+ formatter,
+ metrics,
+ labelMap?.[seriesName]?.[0],
+ !!contributionMode,
+ );
- const transformedSeries = transformSeries(
- {
- ...entry,
- id: `${displayName || ''}`,
- name: `${displayName || ''}`,
- },
- colorScale,
- colorScaleKey,
- {
- area,
- markerEnabled,
- markerSize,
- areaOpacity: opacity,
- seriesType,
- showValue,
- onlyTotal,
- stack: Boolean(stack),
- stackIdSuffix: '\na',
- yAxisIndex,
- filterState,
- seriesKey: entry.name,
- sliceId,
- queryIndex: 0,
- formatter:
- seriesType === EchartsTimeseriesSeriesType.Bar
- ? getOverMaxHiddenFormatter({
- max: yAxisMax,
- formatter: seriesFormatter,
- })
- : seriesFormatter,
- totalStackedValues: sortedTotalValuesA,
- showValueIndexes: showValueIndexesA,
- thresholdValues,
- timeShiftColor,
- theme,
- },
- );
+ const transformedSeries = transformSeries(
+ {
+ ...entry,
+ id: `${displayName || ''}`,
+ name: `${displayName || ''}`,
+ },
+ colorScale,
+ colorScaleKey,
+ {
+ area,
+ markerEnabled,
+ markerSize,
+ areaOpacity: opacity,
+ seriesType,
+ showValue,
+ onlyTotal,
+ stack: Boolean(stack),
+ stackIdSuffix: '\na',
+ yAxisIndex,
+ filterState,
+ seriesKey: entry.name,
+ sliceId,
+ queryIndex: 0,
+ formatter:
+ seriesType === EchartsTimeseriesSeriesType.Bar
+ ? getOverMaxHiddenFormatter({
+ max: yAxisMax,
+ formatter: seriesFormatter,
+ })
+ : seriesFormatter,
+ totalStackedValues: sortedTotalValuesA,
+ showValueIndexes: showValueIndexesA,
+ thresholdValues,
+ timeShiftColor,
+ theme,
+ },
+ );
- if (transformedSeries) {
- series.push(transformedSeries);
- mapSeriesIdToAxis(transformedSeries, yAxisIndex);
- }
- });
+ if (transformedSeries) {
+ series.push(transformedSeries);
+ mapSeriesIdToAxis(transformedSeries, yAxisIndex);
+ }
+});
- rawSeriesB.forEach(entry => {
- const entryName = String(entry.name || '');
- const seriesEntry = inverted[entryName] || entryName;
- const seriesName = `${seriesEntry} (1)`;
- const colorScaleKey = getOriginalSeries(seriesEntry, array);
+rawSeriesB.forEach(entry => {
+ const entryName = String(entry.name || '');
+ const seriesEntry = inverted[entryName] || entryName;
+ const seriesName = `${seriesEntry} (1)`;
+ const colorScaleKey = getOriginalSeries(seriesEntry, array);
- let displayName: string;
+ let displayName: string;
+ if (truncateMetricB) {
+ // Truncate metric: show only the group-by values (or nothing if no
group-by)
+ if (groupbyB.length > 0) {
+ const groupbyValues = labelMapB?.[seriesName] || [];
+ displayName = groupbyValues.join(', ');
+ console.log('Query B - entryName:', entryName, 'seriesName:',
seriesName, 'groupbyValues:', groupbyValues, 'displayName:', displayName);
+ } else {
+ displayName = '';
+ }
+ } else {
+ // Original logic (metric included, optional query identifier)
if (groupbyB.length > 0) {
- // When we have groupby, format as "metric, dimension"
const metricPart: string = showQueryIdentifiers
? `${MetricDisplayNameB} (Query B)`
: MetricDisplayNameB;
displayName = entryName.includes(metricPart)
? entryName
: `${metricPart}, ${entryName}`;
} else {
- // When no groupby, format as just the entry name with optional query
identifier
displayName = showQueryIdentifiers ? `${entryName} (Query B)` :
entryName;
}
+ }
- const seriesFormatter = getFormatter(
- customFormattersSecondary,
- formatterSecondary,
- metricsB,
- labelMapB?.[seriesName]?.[0],
- !!contributionMode,
- );
-
- const transformedSeries = transformSeries(
- {
- ...entry,
- id: `${displayName || ''}`,
- name: `${displayName || ''}`,
- },
+ const seriesFormatter = getFormatter(
+ customFormattersSecondary,
+ formatterSecondary,
+ metricsB,
+ labelMapB?.[seriesName]?.[0],
+ !!contributionMode,
+ );
- colorScale,
- colorScaleKey,
- {
- area: areaB,
- markerEnabled: markerEnabledB,
- markerSize: markerSizeB,
- areaOpacity: opacityB,
- seriesType: seriesTypeB,
- showValue: showValueB,
- onlyTotal: onlyTotalB,
- stack: Boolean(stackB),
- stackIdSuffix: '\nb',
- yAxisIndex: yAxisIndexB,
- filterState,
- seriesKey: entry.name,
- sliceId,
- queryIndex: 1,
- formatter:
- seriesTypeB === EchartsTimeseriesSeriesType.Bar
- ? getOverMaxHiddenFormatter({
- max: maxSecondary,
- formatter: seriesFormatter,
- })
- : seriesFormatter,
- totalStackedValues: sortedTotalValuesB,
- showValueIndexes: showValueIndexesB,
- thresholdValues: thresholdValuesB,
- timeShiftColor,
- theme,
- },
- );
+ const transformedSeries = transformSeries(
+ {
+ ...entry,
+ id: `${displayName || ''}`,
+ name: `${displayName || ''}`,
+ },
+ colorScale,
+ colorScaleKey,
+ {
+ area: areaB,
+ markerEnabled: markerEnabledB,
+ markerSize: markerSizeB,
+ areaOpacity: opacityB,
+ seriesType: seriesTypeB,
+ showValue: showValueB,
+ onlyTotal: onlyTotalB,
+ stack: Boolean(stackB),
+ stackIdSuffix: '\nb',
+ yAxisIndex: yAxisIndexB,
+ filterState,
+ seriesKey: entry.name,
+ sliceId,
+ queryIndex: 1,
+ formatter:
+ seriesTypeB === EchartsTimeseriesSeriesType.Bar
+ ? getOverMaxHiddenFormatter({
+ max: maxSecondary,
+ formatter: seriesFormatter,
+ })
+ : seriesFormatter,
+ totalStackedValues: sortedTotalValuesB,
+ showValueIndexes: showValueIndexesB,
+ thresholdValues: thresholdValuesB,
+ timeShiftColor,
+ theme,
+ },
+ );
- if (transformedSeries) {
- series.push(transformedSeries);
- mapSeriesIdToAxis(transformedSeries, yAxisIndexB);
- }
- });
+ if (transformedSeries) {
+ series.push(transformedSeries);
+ mapSeriesIdToAxis(transformedSeries, yAxisIndexB);
+ }
+});
Review Comment:
**Suggestion:** When the truncate-metric flags are enabled but there are no
group-by columns, the current logic sets the series display name to an empty
string for both queries, which produces blank legend/tooltip labels and
non-unique (empty) series ids; the condition should only apply truncation when
there is at least one group-by. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Mixed timeseries legend shows blank labels when truncation enabled.
- ⚠️ Tooltip rows have empty series names, confusing comparisons.
- ⚠️ Multiple series share empty id, weakening legend interactions.
```
</details>
```suggestion
rawSeriesA.forEach(entry => {
const entryName = String(entry.name || '');
const seriesName = inverted[entryName] || entryName;
const colorScaleKey = getOriginalSeries(seriesName, array);
let displayName: string;
if (truncateMetric && groupby.length > 0) {
// Truncate metric: show only the group-by values when a group-by is
present
const groupbyValues = labelMap?.[seriesName] || [];
displayName = groupbyValues.join(', ');
console.log(
'Query A - entryName:',
entryName,
'seriesName:',
seriesName,
'groupbyValues:',
groupbyValues,
'displayName:',
displayName,
);
} else {
// Original logic (metric included, optional query identifier)
if (groupby.length > 0) {
const metricPart: string = showQueryIdentifiers
? `${MetricDisplayNameA} (Query A)`
: MetricDisplayNameA;
displayName = entryName.includes(metricPart)
? entryName
: `${metricPart}, ${entryName}`;
console.log(
'Query A (no trunc) - entryName:',
entryName,
'displayName:',
displayName,
);
} else {
displayName = showQueryIdentifiers ? `${entryName} (Query A)` :
entryName;
}
}
const seriesFormatter = getFormatter(
customFormatters,
formatter,
metrics,
labelMap?.[seriesName]?.[0],
!!contributionMode,
);
const transformedSeries = transformSeries(
{
...entry,
id: `${displayName || ''}`,
name: `${displayName || ''}`,
},
colorScale,
colorScaleKey,
{
area,
markerEnabled,
markerSize,
areaOpacity: opacity,
seriesType,
showValue,
onlyTotal,
stack: Boolean(stack),
stackIdSuffix: '\na',
yAxisIndex,
filterState,
seriesKey: entry.name,
sliceId,
queryIndex: 0,
formatter:
seriesType === EchartsTimeseriesSeriesType.Bar
? getOverMaxHiddenFormatter({
max: yAxisMax,
formatter: seriesFormatter,
})
: seriesFormatter,
totalStackedValues: sortedTotalValuesA,
showValueIndexes: showValueIndexesA,
thresholdValues,
timeShiftColor,
theme,
},
);
if (transformedSeries) {
series.push(transformedSeries);
mapSeriesIdToAxis(transformedSeries, yAxisIndex);
}
});
rawSeriesB.forEach(entry => {
const entryName = String(entry.name || '');
const seriesEntry = inverted[entryName] || entryName;
const seriesName = `${seriesEntry} (1)`;
const colorScaleKey = getOriginalSeries(seriesEntry, array);
let displayName: string;
if (truncateMetricB && groupbyB.length > 0) {
// Truncate metric: show only the group-by values when a group-by is
present
const groupbyValues = labelMapB?.[seriesName] || [];
displayName = groupbyValues.join(', ');
console.log(
'Query B - entryName:',
entryName,
'seriesName:',
seriesName,
'groupbyValues:',
groupbyValues,
'displayName:',
displayName,
);
} else {
// Original logic (metric included, optional query identifier)
if (groupbyB.length > 0) {
const metricPart: string = showQueryIdentifiers
? `${MetricDisplayNameB} (Query B)`
: MetricDisplayNameB;
displayName = entryName.includes(metricPart)
? entryName
: `${metricPart}, ${entryName}`;
} else {
displayName = showQueryIdentifiers ? `${entryName} (Query B)` :
entryName;
}
}
const seriesFormatter = getFormatter(
customFormattersSecondary,
formatterSecondary,
metricsB,
labelMapB?.[seriesName]?.[0],
!!contributionMode,
);
const transformedSeries = transformSeries(
{
...entry,
id: `${displayName || ''}`,
name: `${displayName || ''}`,
},
colorScale,
colorScaleKey,
{
area: areaB,
markerEnabled: markerEnabledB,
markerSize: markerSizeB,
areaOpacity: opacityB,
seriesType: seriesTypeB,
showValue: showValueB,
onlyTotal: onlyTotalB,
stack: Boolean(stackB),
stackIdSuffix: '\nb',
yAxisIndex: yAxisIndexB,
filterState,
seriesKey: entry.name,
sliceId,
queryIndex: 1,
formatter:
seriesTypeB === EchartsTimeseriesSeriesType.Bar
? getOverMaxHiddenFormatter({
max: maxSecondary,
formatter: seriesFormatter,
})
: seriesFormatter,
totalStackedValues: sortedTotalValuesB,
showValueIndexes: showValueIndexesB,
thresholdValues: thresholdValuesB,
timeShiftColor,
theme,
},
);
if (transformedSeries) {
series.push(transformedSeries);
mapSeriesIdToAxis(transformedSeries, yAxisIndexB);
}
});
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In Superset, create or edit a chart of type "Mixed Time Series" (which
uses
`transformProps` in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts`).
2. In the chart control panel, configure at least one primary metric
(`metrics`) and leave
both `groupby` and `groupbyB` empty, then enable the "Truncate Metric"
option for the
primary (and/or secondary) query so that `truncateMetric` /
`truncateMetricB` are `true`
in the form data (see destructuring at lines 221‑226).
3. Run or refresh the chart so the frontend calls `transformProps`, which
builds
`rawSeriesA` / `rawSeriesB` and then executes the `rawSeriesA.forEach` loop
(lines
421‑503) and `rawSeriesB.forEach` loop (lines 505‑586) with `truncateMetric
=== true` and
`groupby.length === 0` / `groupbyB.length === 0`.
4. In those loops, the `if (truncateMetric)` / `if (truncateMetricB)`
branches take the
`else` path for the inner `if (groupby.length > 0)` / `if (groupbyB.length >
0)` checks
and set `displayName = ''`; `transformSeries` is then called with `id: ''`
and `name: ''`,
so later when legend data is built from `entry.id || entry.name || ''` in
the legend
configuration near the bottom of `transformProps.ts`, all affected series
appear with
blank legend labels and share the same empty identifier.
```
</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/MixedTimeseries/transformProps.ts
**Line:** 421:586
**Comment:**
*Logic Error: When the truncate-metric flags are enabled but there are
no group-by columns, the current logic sets the series display name to an empty
string for both queries, which produces blank legend/tooltip labels and
non-unique (empty) series ids; the condition should only apply truncation when
there is at least one group-by.
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%2F38451&comment_hash=8e304a3bdcfe03e9a9e63c00bab607f31caa4c07bf31450067367b82d702d7c9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=8e304a3bdcfe03e9a9e63c00bab607f31caa4c07bf31450067367b82d702d7c9&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]
