Re: [PR] fix(mixed chart): respect truncate_metric setting for series names (#38190) [superset]

["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