Copilot commented on code in PR #45555:
URL: https://github.com/apache/superset/pull/45555#discussion_r4340048666
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -651,7 +669,23 @@ export default function transformProps(
let displayName: string;
- if (groupbyB.length > 0) {
+ if (truncateMetricB && groupbyB.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValuesB && ensureIsArray(metricsB).length > 1
+ ? labelMapValuesB.slice(1)
+ : (labelMapValuesB ?? []);
+ const truncatedName =
+ groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
+ displayName = showQueryIdentifiers
+ ? `${truncatedName} (Query B)`
+ : truncatedName;
+ } else if (groupbyB.length > 0) {
Review Comment:
When `truncateMetric` and/or `truncateMetricB` are enabled while
`showQueryIdentifiers` is off, Query A and Query B can produce identical
`displayName`s for overlapping groupby values (e.g., both become `boy`). In
this transform, the rendered series `name` is used as the tooltip/legend key
and also drives `primarySeries` vs `secondarySeries` selection; duplicate names
can collapse tooltip rows, map the wrong formatter/label-map entry, and make
legends/cross-filter behavior ambiguous. Consider ensuring uniqueness whenever
both queries are present and groupby-based truncation is applied—e.g.,
auto-append `(Query A/B)` when a collision is detected, or otherwise
disambiguate the names even if the control is off.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -544,7 +546,23 @@ export default function transformProps(
let displayName: string;
- if (groupby.length > 0) {
+ if (truncateMetric && groupby.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValues && ensureIsArray(metrics).length > 1
+ ? labelMapValues.slice(1)
+ : (labelMapValues ?? []);
+ const truncatedName =
+ groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
+ displayName = showQueryIdentifiers
+ ? `${truncatedName} (Query A)`
+ : truncatedName;
+ } else if (groupby.length > 0) {
Review Comment:
The truncation logic for Query A and Query B is effectively duplicated
(including the multi-line comment). To reduce the chance of drift between A/B
behavior, consider extracting a small helper (e.g., `getTruncatedGroupbyName({
labelMapValues, metrics, entryName, queryLabel })`) that returns the computed
name, and reuse it in both branches.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -544,7 +546,23 @@ export default function transformProps(
let displayName: string;
- if (groupby.length > 0) {
+ if (truncateMetric && groupby.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValues && ensureIsArray(metrics).length > 1
+ ? labelMapValues.slice(1)
+ : (labelMapValues ?? []);
+ const truncatedName =
+ groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
+ displayName = showQueryIdentifiers
+ ? `${truncatedName} (Query A)`
+ : truncatedName;
+ } else if (groupby.length > 0) {
Review Comment:
When `truncateMetric` and/or `truncateMetricB` are enabled while
`showQueryIdentifiers` is off, Query A and Query B can produce identical
`displayName`s for overlapping groupby values (e.g., both become `boy`). In
this transform, the rendered series `name` is used as the tooltip/legend key
and also drives `primarySeries` vs `secondarySeries` selection; duplicate names
can collapse tooltip rows, map the wrong formatter/label-map entry, and make
legends/cross-filter behavior ambiguous. Consider ensuring uniqueness whenever
both queries are present and groupby-based truncation is applied—e.g.,
auto-append `(Query A/B)` when a collision is detected, or otherwise
disambiguate the names even if the control is off.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -651,7 +669,23 @@ export default function transformProps(
let displayName: string;
- if (groupbyB.length > 0) {
+ if (truncateMetricB && groupbyB.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValuesB && ensureIsArray(metricsB).length > 1
+ ? labelMapValuesB.slice(1)
+ : (labelMapValuesB ?? []);
+ const truncatedName =
+ groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
+ displayName = showQueryIdentifiers
+ ? `${truncatedName} (Query B)`
+ : truncatedName;
+ } else if (groupbyB.length > 0) {
Review Comment:
The truncation logic for Query A and Query B is effectively duplicated
(including the multi-line comment). To reduce the chance of drift between A/B
behavior, consider extracting a small helper (e.g., `getTruncatedGroupbyName({
labelMapValues, metrics, entryName, queryLabel })`) that returns the computed
name, and reuse it in both branches.
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -651,7 +669,23 @@ export default function transformProps(
let displayName: string;
- if (groupbyB.length > 0) {
+ if (truncateMetricB && groupbyB.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValuesB && ensureIsArray(metricsB).length > 1
+ ? labelMapValuesB.slice(1)
+ : (labelMapValuesB ?? []);
Review Comment:
`ensureIsArray(metrics).length > 1` (and the B equivalent) is recomputed for
every series entry during the `rawSeriesA`/`rawSeriesB` loops. Since
`metrics`/`metricsB` are invariant within the transform, consider hoisting
booleans like `const hasMultipleMetricsA = ensureIsArray(metrics).length > 1;`
and `hasMultipleMetricsB` outside the loops to avoid repeated work (minor, but
easy).
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -544,7 +546,23 @@ export default function transformProps(
let displayName: string;
- if (groupby.length > 0) {
+ if (truncateMetric && groupby.length > 0) {
+ // Drop the metric part entirely and show only the groupby/dimension
+ // values. The label-map tuple is [metric, ...dimensions] only when the
+ // query has more than one metric; with a single metric the whole
+ // tuple is dimensions. Still apply the query-identifier suffix when
asked
+ // for, or Query A and Query B become indistinguishable whenever they
+ // share a groupby value (#38451).
+ const groupbyValues =
+ labelMapValues && ensureIsArray(metrics).length > 1
+ ? labelMapValues.slice(1)
+ : (labelMapValues ?? []);
Review Comment:
`ensureIsArray(metrics).length > 1` (and the B equivalent) is recomputed for
every series entry during the `rawSeriesA`/`rawSeriesB` loops. Since
`metrics`/`metricsB` are invariant within the transform, consider hoisting
booleans like `const hasMultipleMetricsA = ensureIsArray(metrics).length > 1;`
and `hasMultipleMetricsB` outside the loops to avoid repeated work (minor, but
easy).
--
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]
