codeant-ai-for-open-source[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r3525687742
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -422,17 +424,23 @@ export default function transformProps(
let displayName: string;
- 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}`;
+ if (truncateMetric && groupby.length > 0) {
+ const groupbyValues = labelMap?.[seriesName] || [];
+ displayName =
+ groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
Review Comment:
**Suggestion:** When truncation is enabled, the series name is reduced to
group-by values without preserving query identifiers, so enabling “Show query
identifiers” no longer distinguishes Query A vs Query B. This creates ambiguous
legend/tooltip labels for mixed charts and breaks the expected contract of the
identifier toggle. Keep the query suffix in the truncated branch when
identifiers are enabled. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Legends can't distinguish Query A vs Query B.
⚠️ Tooltip labels ambiguous for truncated mixed series.
⚠️ ShowQueryIdentifiers toggle ignored when truncation enabled.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In a Mixed Timeseries chart, configure form data so that Query A and
Query B both have
a non-empty groupby (e.g., ['gender']) and set `truncateMetric: true`,
`truncateMetricB:
true`, and `showQueryIdentifiers: true` in `EchartsMixedTimeseriesFormData`
(see test
setup pattern at
`superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:80-137`).
2. Render the chart so that the plugin calls `transformProps(chartProps)`
defined at
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:121`,
which destructures `truncateMetric`, `truncateMetricB`, and
`showQueryIdentifiers` from
the merged form data at lines `213-228`.
3. For Query A, execution enters the `rawSeriesA.forEach` loop at
`transformProps.ts:161-176`. Because `truncateMetric` is true and
`groupby.length > 0`,
the branch at `transformProps.ts:168-171` runs, setting `displayName` based
solely on
`labelMap?.[seriesName]` (group-by values) and ignoring
`showQueryIdentifiers`. As a
result, series names become just `'boy'`, `'girl'`, etc., with no `(Query
A)` suffix.
4. For Query B, a symmetric path executes in `rawSeriesB.forEach` at
`transformProps.ts:498-525`: when `truncateMetricB` is true and
`groupbyB.length > 0`, the
code at `transformProps.ts:9-13` (actual lines `506-510`) sets `displayName`
to group-by
values only. Even though `showQueryIdentifiers` is true, both Query A and
Query B series
for the same group-by (e.g., `gender = 'boy'`) share identical
legend/tooltip names like
`'boy'`, making the “Show query identifiers” toggle ineffective for
truncated series.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4a69884bce334e4d8a93c08d1787384c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=4a69884bce334e4d8a93c08d1787384c&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 427:430
**Comment:**
*Incomplete Implementation: When truncation is enabled, the series name
is reduced to group-by values without preserving query identifiers, so enabling
“Show query identifiers” no longer distinguishes Query A vs Query B. This
creates ambiguous legend/tooltip labels for mixed charts and breaks the
expected contract of the identifier toggle. Keep the query suffix in the
truncated branch when identifiers are enabled.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=872d705976f24c9007a7bc70a0b9f5932ddd7c574858289bee08aefb0aa27507&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=872d705976f24c9007a7bc70a0b9f5932ddd7c574858289bee08aefb0aa27507&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts:
##########
@@ -136,6 +138,8 @@ export const DEFAULT_FORM_DATA:
EchartsMixedTimeseriesFormData = {
zoomable: TIMESERIES_DEFAULTS.zoomable,
richTooltip: TIMESERIES_DEFAULTS.richTooltip,
showQueryIdentifiers: false,
+ truncateMetric: false,
+ truncateMetricB: false,
Review Comment:
**Suggestion:** Setting truncation defaults to `false` here conflicts with
the chart-control default for “Truncate Metric” and changes behavior for
payloads that omit these fields (for example, legacy saved charts), causing
transform-time defaults to disagree with control defaults. Align these defaults
with the control-layer default to avoid inconsistent behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ Legacy mixed charts misaligned with truncation checkbox default.
⚠️ Users see full metric names despite default truncation.
⚠️ Behavior diverges between controls and transform-layer defaults.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Inspect the Mixed Timeseries control panel configuration in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/controlPanel.tsx:126-129`,
where the truncate controls are defined as `name:
\`truncate_metric${controlSuffix}\`` and
configured with `...sharedControls.truncate_metric` and `default:
sharedControls.truncate_metric.default`. The shared control itself is
defined at
`superset-frontend/packages/superset-ui-chart-controls/src/shared-controls/sharedControls.tsx:398-413`
with `default: true`, meaning the UI checkbox “Truncate Metric” defaults to
enabled.
2. Examine the Mixed Timeseries transform defaults in
`superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/types.ts:80-87`,
where
`DEFAULT_FORM_DATA` sets `truncateMetric: false` and `truncateMetricB:
false` at lines
`82-83`. In `transformProps` (`transformProps.ts:213-228`), form data is
merged via `{
...DEFAULT_FORM_DATA, ...formData }`, and `truncateMetric`/`truncateMetricB`
are
destructured from that merged object.
3. For a legacy or existing chart payload that contains only the snake-case
fields
(`truncate_metric`, `truncate_metric_b`) from the control layer but omits
the new
camel-case `truncateMetric` and `truncateMetricB` fields, the merged form
data seen by
`transformProps` will keep `truncateMetric` and `truncateMetricB` at their
DEFAULT_FORM_DATA values (`false`). As a result, when `rawSeriesA.forEach`
and
`rawSeriesB.forEach` execute (see truncation branches at
`transformProps.ts:168-171` and
`transformProps.ts:506-510`), the code behaves as if truncation is disabled,
even though
the front-end controls default to truncation enabled, leading to
inconsistent series
naming relative to the control-layer default.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fa73be693d494c2a9b34a77bdb2448dc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fa73be693d494c2a9b34a77bdb2448dc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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/types.ts
**Line:** 141:142
**Comment:**
*Logic Error: Setting truncation defaults to `false` here conflicts
with the chart-control default for “Truncate Metric” and changes behavior for
payloads that omit these fields (for example, legacy saved charts), causing
transform-time defaults to disagree with control defaults. Align these defaults
with the control-layer default to avoid inconsistent behavior.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=370743fbaccbd2b5074a20baebd757cfd3285e5ab9541666bba9f86517c71cd5&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=370743fbaccbd2b5074a20baebd757cfd3285e5ab9541666bba9f86517c71cd5&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -495,17 +503,24 @@ export default function transformProps(
let displayName: string;
- 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}`;
+ if (truncateMetricB && groupbyB.length > 0) {
+ const groupbyValues =
+ labelMapB?.[seriesEntry] || labelMapB?.[entryName] || [];
+ displayName =
+ groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
Review Comment:
**Suggestion:** The Query B truncation lookup does not use the suffixed key
variant already computed for secondary-series label map access, so
`truncateMetricB` can silently fail and fall back to full names when label-map
entries are keyed with the secondary suffix. Include the same key used by
secondary formatter lookup when resolving group-by labels. [incorrect variable
usage]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
❌ Query B truncation may silently fail with suffixed keys.
⚠️ Mixed charts show full metric names despite truncation.
⚠️ Users experience inconsistent truncation across secondary query.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a Mixed Timeseries chart where Query B has a non-empty groupby
(e.g., `groupbyB:
['gender']`) and truncation enabled via `truncateMetricB: true` in
`EchartsMixedTimeseriesFormData` (pattern as in tests at
`superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:81-97`).
2. Ensure the second query's `label_map` (provided through `queriesData[1]`)
is keyed by
the suffixed series name used elsewhere in the transform, e.g., entries
under keys like
`${seriesEntry} (1)`; this is the same key (`seriesName`) used when fetching
the formatter
at `transformProps.ts:29-35` (actual lines `527-533`), confirming the code
expects
suffixed keys for Query B.
3. When the chart renders, `transformProps` runs (entry at
`transformProps.ts:121`). In
`rawSeriesB.forEach` at `transformProps.ts:498-525`, the truncation branch at
`transformProps.ts:9-13` (`506-510`) attempts to resolve group-by labels from
`labelMapB?.[seriesEntry] || labelMapB?.[entryName]`. Because these keys
omit the `(1)`
suffix, `groupbyValues` is empty when `labelMapB` is keyed by `seriesName`,
causing the
fallback `displayName = entryName` to execute and leaving the full
metric-containing name
in place despite `truncateMetricB` being true.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ceccab4ce5c74c23ac1c1b103dea9832&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ceccab4ce5c74c23ac1c1b103dea9832&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<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:** 506:510
**Comment:**
*Incorrect Variable Usage: The Query B truncation lookup does not use
the suffixed key variant already computed for secondary-series label map
access, so `truncateMetricB` can silently fail and fall back to full names when
label-map entries are keyed with the secondary suffix. Include the same key
used by secondary formatter lookup when resolving group-by labels.
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.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=d3c497887d841a8e403d78db34e068b88119a5b185a2de43388e637362384a2d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=d3c497887d841a8e403d78db34e068b88119a5b185a2de43388e637362384a2d&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]
