You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "MelikHajlawi (via GitHub)" <gi...@apache.org> on 2026/03/05 19:56:11 UTC
[PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
MelikHajlawi opened a new pull request, #38451:
URL: https://github.com/apache/superset/pull/38451
### SUMMARY
Fixes #38190 – "Truncate Metric" had no effect in Mixed Chart.
**Problem**: The Mixed Chart ignored the `truncateMetric` and `truncateMetricB` form values, always displaying the full series name (metric + group‑by) in the legend and tooltip, regardless of the checkbox state.
**Solution**:
- Added `truncateMetric` and `truncateMetricB` to the destructured form data in `transformProps.ts`.
- Modified series name generation for both queries:
- When `truncateMetric` (or `truncateMetricB`) is `true`, the name is built using only the group‑by values (from `labelMap`/`labelMapB`).
- When `false`, the original logic (metric + group‑by, with optional query identifier) is used.
- Added the missing properties to the `EchartsMixedTimeseriesFormData` type and set defaults to `false` in `DEFAULT_FORM_DATA` (in `types.ts`).
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!-- Add screenshots here to show the visual change.
**Before**: Truncate Metric enabled – legend still shows "COUNT(name): boy".
**After**: Truncate Metric enabled – legend shows only "boy".
-->
_Before_ – Truncate Metric ON, but metric still visible in legend:
![before]<img width="1908" height="961" alt="Capture d'écran 2026-03-05 204450" src="https://github.com/user-attachments/assets/428d2d2e-ab12-4279-aceb-bf8654f50158" />
_After_ – Truncate Metric ON, legend shows only group‑by values:
![after]
<img width="1910" height="963" alt="Capture d'écran 2026-03-05 203856" src="https://github.com/user-attachments/assets/fa2bbf24-ab2e-4a58-8d02-4995a56c9dbb" />
(Replace the placeholder URLs with your actual screenshot links after uploading them.)
### TESTING INSTRUCTIONS
1. **Create a Mixed Chart** with two queries:
- Use a dataset with a time column, a metric, and a group‑by column (e.g., `birth_names` with `SUM(num)` and `gender` for Query A; `COUNT(*)` and `state` for Query B).
2. **Enable "Truncate Metric"** for both queries (under the Data tab in Query A and Query B sections).
3. **Click Update Chart**.
- **Expected**: Legend entries show only the group‑by values (e.g., `male`, `female` for Query A; state names for Query B). Tooltips also omit the metric.
4. **Disable the checkboxes** and update again.
- **Expected**: Full names (metric + group‑by) reappear.
5. Test edge cases:
- No group‑by – with truncation enabled, series should disappear from legend/tooltip (empty name).
- Multiple metrics per query – each series truncates independently.
- `showQueryIdentifiers` enabled – the identifier should not appear when truncation is on.
### ADDITIONAL INFORMATION
- [x] Has associated issue: #38190
- [ ] Required feature flags: none
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "MelikHajlawi (via GitHub)" <gi...@apache.org>.
MelikHajlawi commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4047363875
@sadpandajoe Thank you for the feedback. I've now added comprehensive unit tests in transformProps.test.ts that cover the truncation behavior for both queries, including cases with/without group‑by, and when truncation is enabled/disabled. All tests are passing locally. The PR is ready for another review. Thanks!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r3831177557
##########
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] || [];
Review Comment:
A verbose column label that equals a group value can replace the value here: `invert(verboseMap)` maps the series name back to the column key, so a `gender` column labeled `boy` renders `gender` rather than `boy` when truncation is enabled. Could this lookup prefer `entryName` before the inverted key?
##########
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:
Agreed—when query identifiers are enabled, truncation drops the query suffix, so identical values from A and B have indistinguishable legend and tooltip labels. Could the truncated names retain the identifier?
##########
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:
Agreed—Query B truncation reads unsuffixed keys while its formatter uses the suffixed secondary-series key, so a response keyed by that series falls back to the full name. Could this lookup include `seriesName`?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "MelikHajlawi (via GitHub)" <gi...@apache.org>.
MelikHajlawi commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2897472607
##########
project_structure.txt:
##########
Review Comment:
@sadpandajoe Thanks for pointing this out. The file was accidentally generated by a local tool and included in the commit. I've removed it from the branch.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4047345415
## **Sequence Diagram**
This PR updates Mixed Timeseries label generation so each query now respects its own truncate metric setting. When truncation is enabled and group by columns exist, legend and tooltip labels use only group by values; otherwise the original full naming is kept.
```mermaid
sequenceDiagram
participant User
participant ChartConfig
participant TransformProps
participant ECharts
User->>ChartConfig: Set truncate metric options for Query A and Query B
ChartConfig->>TransformProps: Send form data with truncate flags and groupby fields
TransformProps->>TransformProps: Build Query A series display names
alt Truncate enabled and groupby exists
TransformProps->>TransformProps: Use groupby values as series name
else Otherwise
TransformProps->>TransformProps: Use metric plus groupby naming
end
TransformProps->>TransformProps: Repeat naming logic for Query B with its own flag
TransformProps-->>ECharts: Return transformed series with final names
ECharts-->>User: Render legend and tooltip with updated labels
```
---
*Generated by [CodeAnt AI](https://codeant.ai)*
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4461414537
I opened a PR with a failing test for this... it validates the issue, and if you want to cherry pick it in here after rebasing (or copy the file... whatever works) we can use it to validate that this PR solves the problem.
https://github.com/apache/superset/pull/40146
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r4128541539
##########
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:
Yes. `DEFAULT_FORM_DATA` should match the shared control default (`true`) for consistency, particularly for legacy saved charts that lack these newly introduced fields. Set both values to `true`:
```typescript
showQueryIdentifiers: false,
truncateMetric: true,
truncateMetricB: true,
```
This ensures the transform layer and Explore render the same series names when the fields are omitted, avoiding a silent behavior change on re-save. The explicit `false` case remains unaffected because supplied form data overrides `DEFAULT_FORM_DATA`. Also verify/update the related tests and any documentation that currently assumes truncation defaults to `false`.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4047898807
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r3831182326
##########
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:
Yes. The truncated branch should append the query identifier when `showQueryIdentifiers` is enabled, while still omitting the metric:
```typescript
if (truncateMetric && groupby.length > 0) {
const groupbyValues = labelMap?.[seriesName] || [];
const truncatedName =
groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
displayName = showQueryIdentifiers
? `${truncatedName} (Query A)`
: truncatedName;
}
```
Apply the equivalent logic for Query B:
```typescript
if (truncateMetricB && groupbyB.length > 0) {
const groupbyValues =
labelMapB?.[seriesEntry] || labelMapB?.[entryName] || [];
const truncatedName =
groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
displayName = showQueryIdentifiers
? `${truncatedName} (Query B)`
: truncatedName;
}
```
Add coverage for `truncateMetric` and `truncateMetricB` with `showQueryIdentifiers: true`, expecting names such as `boy (Query A)` and `boy (Query B)`. This preserves the truncation behavior while ensuring legends and tooltips remain distinguishable between queries.
##########
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:
Yes, this is a valid issue. Query B already computes the suffixed key as `seriesName`, and the truncation lookup should use the same key as the formatter lookup.
Update it to prefer `seriesName`, while retaining the existing fallbacks for compatibility:
```typescript
const groupbyValues =
labelMapB?.[seriesName] ||
labelMapB?.[seriesEntry] ||
labelMapB?.[entryName] ||
[];
```
This ensures `truncateMetricB` works when `labelMapB` is keyed by the secondary series name (`${seriesEntry} (1)`), while preserving support for unsuffixed response keys.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2897232880
##########
project_structure.txt:
##########
Review Comment:
what is this file used for?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r3525964566
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -419,6 +457,7 @@ test('should add a formula annotation when X-axis column has dataset-level label
datasource: {
verboseMap: {
[timeColumnName]: timeColumnLabel,
+ sum__num: 'sum__num',
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicate transformProps test assertions</b></div>
<div id="fix">
Consider extracting duplicated test setup and assertion logic into shared utilities to reduce code duplication across test files. Similar patterns found in MixedTimeseries/transformProps.test.ts (lines 157-171, 204-218, 460-473) and Timeseries/transformProps.test.ts (lines 296-309, 330-341). A helper function for formula series validation would eliminate this duplication.
</div>
</div>
<small><i>Code Review Run #a8dcd4</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "netlify[bot] (via GitHub)" <gi...@apache.org>.
netlify[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4007456218
### <span aria-hidden="true">✅</span> Deploy Preview for *superset-docs-preview* ready!
| Name | Link |
|:-:|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | 0c42fd838ef4002f5d3a960bb58ce03ec8b8cfc2 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/projects/superset-docs-preview/deploys/69a9dfdc647c07000826a97b |
|<span aria-hidden="true">😎</span> Deploy Preview | [https://deploy-preview-38451--superset-docs-preview.netlify.app](https://deploy-preview-38451--superset-docs-preview.netlify.app) |
|<span aria-hidden="true">📱</span> Preview on mobile | <details><summary> Toggle QR Code... </summary><br /><br /><br /><br />_Use your smartphone camera to open QR code link._</details> |
---
<!-- [superset-docs-preview Preview](https://deploy-preview-38451--superset-docs-preview.netlify.app) -->
_To edit notification comments on pull requests, go to your [Netlify project configuration](https://app.netlify.com/projects/superset-docs-preview/configuration/notifications#deploy-notifications)._
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-5434953307
Heya @MelikHajlawi, checking back in. Still conflicting against `master`, can you rebase? One thing from before still stands: `master` picked up `displayLabelMap`/`displayLabelMapB` since this branched, so the truncate logic needs to populate those too, not just resolve the merge conflict. Holler if you want me to jump in with some commits.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2897239640
##########
superset/static/service-worker.js:
##########
Review Comment:
why are we commenting this whole file out? If this file isn't needed anymore we should just delete it.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4013335909
@MelikHajlawi see some new code and no tests, do the existing tests already cover the updates? do we need new tests?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2925775867
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -415,151 +420,162 @@ 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);
+
- let displayName: string;
+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) {
+ const groupbyValues = labelMap?.[seriesName] || [];
+ displayName = groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
+ } else {
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}`;
} 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);
- let displayName: string;
+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) {
+ const groupbyValues = labelMapB?.[seriesEntry] || labelMapB?.[entryName] || [];
+ displayName = groupbyValues.length > 0 ? groupbyValues.join(', ') : entryName;
+ } else {
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 seriesFormatter = getFormatter(
+ customFormattersSecondary,
+ formatterSecondary,
+ metricsB,
+ labelMapB?.[seriesName]?.[0],
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Inconsistent labelMap key usage</b></div>
<div id="fix">
The labelMapB lookup key for the formatter in Query B series uses seriesName (which includes '(1)'), but truncateMetric uses seriesEntry (without '(1)'). Since tests show label_map keys like 'boy' without '(1)', and seriesEntry matches this, using seriesName here risks undefined lookups and incorrect formatting. It looks like seriesEntry should be used consistently for labelMapB access.
</div>
</div>
<small><i>Code Review Run #f88980</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-5024919372
@MelikHajlawi thanks for cleaning up the console.logs and indentation — the fix logic and tests look right to me now. This is still showing as conflicting, and heads up: master's `transformProps.ts` grew `displayLabelMap`/`displayLabelMapB` (around line 477) that your truncate branches will need to populate with the truncated name, so the rebase isn't purely mechanical. Once it's green against master I think this is mergeable.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-6308540022
@MelikHajlawi this branch had drifted far enough behind `master` that a rebase wasn't practical (and I don't have the OAuth scope to push a rebase that touches `.github/workflows/*` files along the way), so I rebuilt your fix fresh against current `master` in #45555, keeping you as a co-author. It also folds in the fixes for everything raised in this thread, the query-identifier/truncation interaction sadpandajoe and codeant flagged, the displayLabelMap gap I flagged back in July (already covered by how `master` evolved since), and explains why I left the `DEFAULT_FORM_DATA` default as `false` rather than `true`.
Thanks for the original work here, sorry it took this long to land. Closing this one in favor of #45555.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4007392630
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4008055928
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4753107157
@MelikHajlawi this needs some touch-ups before it's mergeable.
There's a `console.log('truncateMetric'...)` (and one for full `formData`) left in `transformProps.ts` around line 228 that has to come out.
The two `rawSeriesA/B.forEach` blocks also got de-indented to column 0 in this diff. That won't survive prettier/eslint.
Once that's cleaned up and rebased, can you confirm it passes against the failing test from #40146 so we know it actually closes #38190?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4888231547
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r3525680797
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -433,3 +472,98 @@ test('should add a formula annotation when X-axis column has dataset-level label
expect(Array.isArray(formulaSeries?.data)).toBe(true);
expect((formulaSeries!.data as unknown[]).length).toBeGreaterThan(0);
});
+
+describe('truncateMetric behavior', () => {
+ const getChartProps = (overrides = {}) =>
+ createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queriesData,
+ formData: {
+ ...formData,
+ ...overrides,
+ },
+ queriesData,
+ datasource: {
+ verboseMap: { sum__num: 'sum__num' },
+ columnFormats: {},
+ currencyFormats: {},
+ },
+ });
+
+ test('should show only group‑by values when truncateMetric=true and groupby exists (both queries truncated)', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ truncateMetricB: true, // also truncate Query B to avoid full names
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
Review Comment:
**Suggestion:** Replace the `any[]` assertion with a specific series type (for example `SeriesOption[]`) so the test does not introduce `any`. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The new test code introduces `any[]`, which directly violates the no-`any` TypeScript rule. A specific series type should be used instead.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e0c9971dcb24e43bf9966124dfd779f&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=0e0c9971dcb24e43bf9966124dfd779f&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/test/MixedTimeseries/transformProps.test.ts
**Line:** 505:505
**Comment:**
*Custom Rule: Replace the `any[]` assertion with a specific series type (for example `SeriesOption[]`) so the test does not introduce `any`.
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=71a0125c0c2097c3da1b995d101db80e64da493b242385b74477f1cb3287981e&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=71a0125c0c2097c3da1b995d101db80e64da493b242385b74477f1cb3287981e&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -433,3 +472,98 @@ test('should add a formula annotation when X-axis column has dataset-level label
expect(Array.isArray(formulaSeries?.data)).toBe(true);
expect((formulaSeries!.data as unknown[]).length).toBeGreaterThan(0);
});
+
+describe('truncateMetric behavior', () => {
+ const getChartProps = (overrides = {}) =>
+ createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queriesData,
+ formData: {
+ ...formData,
+ ...overrides,
+ },
+ queriesData,
+ datasource: {
+ verboseMap: { sum__num: 'sum__num' },
+ columnFormats: {},
+ currencyFormats: {},
+ },
+ });
+
+ test('should show only group‑by values when truncateMetric=true and groupby exists (both queries truncated)', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ truncateMetricB: true, // also truncate Query B to avoid full names
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
+
+ // No series should contain the metric name
+ expect(series.map(s => s.name)).not.toContain('sum__num, boy');
+ expect(series.map(s => s.name)).not.toContain('sum__num, girl');
+ // All series should be truncated to group‑by values
+ expect(series.map(s => s.name)).toContain('boy');
+ expect(series.map(s => s.name)).toContain('girl');
+ });
+
+ test('should keep full name when truncateMetric=true but no groupby', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ groupby: [], // no groupby for Query A
+ groupbyB: ['gender'], // Query B still has groupby
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
+ const seriesNames = series.map(s => s.name);
+
+ // Query A (no groupby) should show metric name only
+ expect(seriesNames).toContain('sum__num');
+ // Query B (with groupby) should show full names
+ expect(seriesNames).toContain('sum__num, boy');
+ expect(seriesNames).toContain('sum__num, girl');
+ // No truncated names from Query A because no groupby
+ expect(seriesNames).not.toContain('boy');
+ expect(seriesNames).not.toContain('girl');
+ });
+
+ test('should show full name when truncateMetric=false', () => {
+ const chartProps = getChartProps({
+ truncateMetric: false,
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
+
+ expect(series.map(s => s.name)).toContain('sum__num, boy');
+ expect(series.map(s => s.name)).toContain('sum__num, girl');
+ });
+
+ test('truncateMetricB should work independently for Query B', () => {
+ const chartProps = getChartProps({
+ truncateMetricB: true, // only Query B truncated
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
Review Comment:
**Suggestion:** Change this `any[]` cast to a concrete series type so the query-B truncation test remains type-safe. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This newly added line uses `any[]`, which is disallowed by the no-`any` TypeScript rule and should be replaced with a concrete series type.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6d68c7b35a2047a7834cb77c11c13844&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=6d68c7b35a2047a7834cb77c11c13844&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/test/MixedTimeseries/transformProps.test.ts
**Line:** 558:558
**Comment:**
*Custom Rule: Change this `any[]` cast to a concrete series type so the query-B truncation test remains type-safe.
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=c5a639ac519b6b3c2cdb3f173fad64b17dde591d3bd19a8264b3142210885fd6&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=c5a639ac519b6b3c2cdb3f173fad64b17dde591d3bd19a8264b3142210885fd6&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -433,3 +472,98 @@ test('should add a formula annotation when X-axis column has dataset-level label
expect(Array.isArray(formulaSeries?.data)).toBe(true);
expect((formulaSeries!.data as unknown[]).length).toBeGreaterThan(0);
});
+
+describe('truncateMetric behavior', () => {
+ const getChartProps = (overrides = {}) =>
+ createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queriesData,
+ formData: {
+ ...formData,
+ ...overrides,
+ },
+ queriesData,
+ datasource: {
+ verboseMap: { sum__num: 'sum__num' },
+ columnFormats: {},
+ currencyFormats: {},
+ },
+ });
+
+ test('should show only group‑by values when truncateMetric=true and groupby exists (both queries truncated)', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ truncateMetricB: true, // also truncate Query B to avoid full names
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
+
+ // No series should contain the metric name
+ expect(series.map(s => s.name)).not.toContain('sum__num, boy');
+ expect(series.map(s => s.name)).not.toContain('sum__num, girl');
+ // All series should be truncated to group‑by values
+ expect(series.map(s => s.name)).toContain('boy');
+ expect(series.map(s => s.name)).toContain('girl');
+ });
+
+ test('should keep full name when truncateMetric=true but no groupby', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ groupby: [], // no groupby for Query A
+ groupbyB: ['gender'], // Query B still has groupby
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
Review Comment:
**Suggestion:** Replace the `any[]` assertion with a strongly typed series array to comply with the no-`any` rule. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
This added test casts the series array to `any[]`, which is exactly the prohibited pattern under the no-`any` rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c56520d65c70470fa56c4ecdb8752f63&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=c56520d65c70470fa56c4ecdb8752f63&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/test/MixedTimeseries/transformProps.test.ts
**Line:** 523:523
**Comment:**
*Custom Rule: Replace the `any[]` assertion with a strongly typed series array to comply with the no-`any` rule.
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=b9fbe8d3f55cc8e411e4c8c35e4d86aa0fc6b0ed54c9196e106dc5883731e108&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=b9fbe8d3f55cc8e411e4c8c35e4d86aa0fc6b0ed54c9196e106dc5883731e108&reaction=dislike'>👎</a>
##########
superset-frontend/plugins/plugin-chart-echarts/test/MixedTimeseries/transformProps.test.ts:
##########
@@ -433,3 +472,98 @@ test('should add a formula annotation when X-axis column has dataset-level label
expect(Array.isArray(formulaSeries?.data)).toBe(true);
expect((formulaSeries!.data as unknown[]).length).toBeGreaterThan(0);
});
+
+describe('truncateMetric behavior', () => {
+ const getChartProps = (overrides = {}) =>
+ createEchartsTimeseriesTestChartProps<
+ EchartsMixedTimeseriesFormData,
+ EchartsMixedTimeseriesProps
+ >({
+ ...MIXED_TIMESERIES_CHART_PROPS_DEFAULTS,
+ defaultQueriesData: queriesData,
+ formData: {
+ ...formData,
+ ...overrides,
+ },
+ queriesData,
+ datasource: {
+ verboseMap: { sum__num: 'sum__num' },
+ columnFormats: {},
+ currencyFormats: {},
+ },
+ });
+
+ test('should show only group‑by values when truncateMetric=true and groupby exists (both queries truncated)', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ truncateMetricB: true, // also truncate Query B to avoid full names
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
+
+ // No series should contain the metric name
+ expect(series.map(s => s.name)).not.toContain('sum__num, boy');
+ expect(series.map(s => s.name)).not.toContain('sum__num, girl');
+ // All series should be truncated to group‑by values
+ expect(series.map(s => s.name)).toContain('boy');
+ expect(series.map(s => s.name)).toContain('girl');
+ });
+
+ test('should keep full name when truncateMetric=true but no groupby', () => {
+ const chartProps = getChartProps({
+ truncateMetric: true,
+ groupby: [], // no groupby for Query A
+ groupbyB: ['gender'], // Query B still has groupby
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
+ const seriesNames = series.map(s => s.name);
+
+ // Query A (no groupby) should show metric name only
+ expect(seriesNames).toContain('sum__num');
+ // Query B (with groupby) should show full names
+ expect(seriesNames).toContain('sum__num, boy');
+ expect(seriesNames).toContain('sum__num, girl');
+ // No truncated names from Query A because no groupby
+ expect(seriesNames).not.toContain('boy');
+ expect(seriesNames).not.toContain('girl');
+ });
+
+ test('should show full name when truncateMetric=false', () => {
+ const chartProps = getChartProps({
+ truncateMetric: false,
+ groupby: ['gender'],
+ groupbyB: ['gender'],
+ showQueryIdentifiers: false,
+ });
+ const transformed = transformProps(chartProps);
+ const series = transformed.echartOptions.series as any[];
Review Comment:
**Suggestion:** Use a specific series type for this cast rather than `any[]` to keep new TypeScript code strictly typed. [custom_rule]
**Severity Level:** Minor ⚠️
<details>
<summary><b>Why it matters? 🤔 </b></summary>
The test introduces a new `any[]` assertion, so the suggestion correctly identifies a real violation of the no-`any` rule.
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=47575b598ff74e6b9f42f6784a76b5ac&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=47575b598ff74e6b9f42f6784a76b5ac&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/test/MixedTimeseries/transformProps.test.ts
**Line:** 544:544
**Comment:**
*Custom Rule: Use a specific series type for this cast rather than `any[]` to keep new TypeScript code strictly typed.
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=878bb5f1bca0b4afdbf9aa8e220464eb9a81c6993a290064a725135345df5e1a&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=878bb5f1bca0b4afdbf9aa8e220464eb9a81c6993a290064a725135345df5e1a&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "MelikHajlawi (via GitHub)" <gi...@apache.org>.
MelikHajlawi commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2897476941
##########
superset/static/service-worker.js:
##########
Review Comment:
@sadpandajoe I investigated and found that this file was originally added by an unmerged PR (#36191). It existed in my local branch, and when I built the frontend, it regenerated a larger webpack version, which I unintentionally staged. Since this file does not exist in upstream/master and is unrelated to my fix, I've removed it entirely from the PR.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-4013943124
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</details>
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
codeant-ai-for-open-source[bot] commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r2925216653
##########
superset-frontend/plugins/plugin-chart-echarts/src/MixedTimeseries/transformProps.ts:
##########
@@ -223,8 +223,13 @@ export default function transformProps(
showQueryIdentifiers = false,
metrics = [],
metricsB = [],
+ truncateMetric,
+ truncateMetricB,
}: EchartsMixedTimeseriesFormData = { ...DEFAULT_FORM_DATA, ...formData };
+ //debug logs for truncate_metric and truncate_metric_b values
+ console.log('truncateMetric:', truncateMetric, 'truncateMetricB:', truncateMetricB);
+ console.log('Full formData:', formData);
Review Comment:
**Suggestion:** Debug logging of full form data in chart transform code runs on every render and can expose potentially sensitive dashboard/filter payloads in browser logs while also adding unnecessary runtime overhead. Remove these logs from production path. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Full chart form payload exposed in browser console.
- ⚠️ Repeated logging adds render-path client overhead.
- ⚠️ Debug noise hinders troubleshooting real frontend issues.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open any Mixed Chart; chart type is enabled in `MainPreset.ts:130-132` and uses Mixed
transform path.
2. Each render calls `transformProps` via `SuperChartCore` processing
(`SuperChartCore.tsx:47-58`).
3. During every call, `transformProps.ts:231-232` writes both truncate flags and full
`formData` to browser console.
4. `formData` includes runtime chart config payload from Explore/Dashboard context
(`transformProps` destructures `...formData` at `transformProps.ts:124-228`), so those
values are continuously exposed in client logs.
5. This is production-path logging (not test-only), unlike most plugin source files where
`console.log` does not appear (`plugins/plugin-chart-echarts/src` grep shows this file as
the runtime source instance).
```
</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:** 231:232
**Comment:**
*Security: Debug logging of full form data in chart transform code runs on every render and can expose potentially sensitive dashboard/filter payloads in browser logs while also adding unnecessary runtime overhead. Remove these logs from production path.
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=0202233e025ba1f693deb6567aa18e7c0e22164792b65d15ad59604c14035cfc&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38451&comment_hash=0202233e025ba1f693deb6567aa18e7c0e22164792b65d15ad59604c14035cfc&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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on code in PR #38451:
URL: https://github.com/apache/superset/pull/38451#discussion_r4128539545
##########
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] || [];
Review Comment:
When both `truncateMetric` and `truncateMetricB` are enabled and Query A and Query B share a groupby value, both series get the exact same truncated name (e.g. both render as `boy`), because this branch never applies the Query A/Query B distinction that the untruncated path just below still uses — so the two series become indistinguishable in the legend and can't be toggled independently. This happens even with `showQueryIdentifiers` off, so it isn't limited to the identifier case already discussed above. None of the four new tests cover Query A and Query B sharing a groupby value. Could the truncated name retain some query marker when both queries collide on the same groupby value?
##########
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:
Agreed—for a chart whose saved form data omits the new `truncateMetric`/`truncateMetricB` keys (e.g. a legacy chart created before this field existed), the runtime default here is `false`, but the control checkbox itself defaults to checked. Re-saving that chart from Explore would silently flip its rendered series names by picking up the control's `true` default that the transform layer never assumed. Should `DEFAULT_FORM_DATA` match the control's default instead?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #38451: fix(mixed chart): respect truncate_metric setting for series names (#38190)
URL: https://github.com/apache/superset/pull/38451
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #38451:
URL: https://github.com/apache/superset/pull/38451#issuecomment-5902576501
@MelikHajlawi still conflicting against master, so this can't land as-is. Also worth fixing before it does: @sadpandajoe caught a real one in review, when `truncateMetric` and `truncateMetricB` are both on and Query A/B share a groupby value, both series end up rendering the exact same name (e.g. both just `boy`), so they're indistinguishable in the legend.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org