fix(echart): multiple time shift line pattern (#38866)

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformProps.ts

@@ -124,6 +124,60 @@ import {
 import { safeParseEChartOptions } from '../utils/safeEChartOptionsParser';
 import { mergeCustomEChartOptions } from '../utils/mergeCustomEChartOptions';
 
+const visibleDashPatterns: ([number, number] | 'dashed' | 'dotted')[] = [
+  'dashed',
+  'dotted',
+  [6, 15], // narrow dashed
+  [2, 10], // wide dotted
+  [20, 3], // wide dashed
+];
+const visibleSymbols = [
+  'rect',
+  'triangle',
+  'diamond',
+  'roundRect',
+  'pin',
+] as const;
+
+function getSymbolMarker(symbol: string, color: string) {
+  const size = 10;
+  switch (symbol) {
+    case 'circle':
+      return `<span style="
+        display:inline-block;width:${size}px;height:${size}px;
+        border-radius:50%;background:${color};margin-right:5px"></span>`;
+    case 'rect':
+      return `<span style="
+        display:inline-block;width:${size}px;height:${size}px;
+        background:${color};margin-right:5px"></span>`;
+    case 'roundRect':
+      return `<span style="
+        display:inline-block;width:${size}px;height:${size}px;border-radius:2px;
+        background:${color};margin-right:5px"></span>`;
+    case 'triangle':
+      return `<span style="
+        display:inline-block;width:0;height:0;
+        border-left:${size / 2}px solid transparent;
+        border-right:${size / 2}px solid transparent;
+        border-bottom:${size}px solid ${color};
+        margin-right:5px"></span>`;
+    case 'diamond':
+      return `<span style="
+        display:inline-block;width:${size - 2}px;height:${size - 2}px;
+        background:${color};transform: rotate(45deg) translateX(1px) translateY(-1px);
+        margin-right:5px"></span>`;
+    case 'pin':
+      return `<span style="
+        display:inline-block;width:${size - 2}px;height:${size - 2}px;
+        background:${color};transform: rotate(45deg) translateX(1px) translateY(-1px);
+        border-radius:50%;border-bottom-right-radius:0;margin-right:5px"></span>`;
+    default:
+      return `<span style="
+        display:inline-block;width:${size}px;height:${size}px;
+        border-radius:50%;background:${color};margin-right:5px"></span>`;
+  }
+}
+
 export default function transformProps(
   chartProps: EchartsTimeseriesChartProps,
 ): TimeseriesChartTransformedProps {
@@ -346,7 +400,8 @@ export default function transformProps(
     );
 
     const lineStyle: LineStyleOption = {};
-    if (derivedSeries) {
+    let lineSymbol;
+    if (derivedSeries && timeShiftColor) {
       // Get the time offset for this series to assign different dash patterns
       const offset = getTimeOffset(entry, array) || seriesName;
       if (!offsetLineWidths[offset]) {
@@ -355,11 +410,11 @@ export default function transformProps(
       // Use visible dash patterns that vary by offset index
       // Pattern: [dash length, gap length] - scaled to be clearly visible
       const patternIndex = offsetLineWidths[offset];
-      lineStyle.type = [
-        (patternIndex % 5) + 4, // dash: 4-8px (visible)
-        (patternIndex % 3) + 3, // gap: 3-5px (visible)
-      ];
+      lineStyle.type =
+        visibleDashPatterns[patternIndex % visibleDashPatterns.length];
+
       lineStyle.opacity = OpacityEnum.DerivedSeries;
+      lineSymbol = visibleSymbols[patternIndex % visibleSymbols.length];
     }
 
     // Calculate min/max from data for horizontal bar charts
@@ -443,6 +498,7 @@ export default function transformProps(
         sliceId,
         isHorizontal,
         lineStyle,
+        lineSymbol,
         timeCompare: array,
         timeShiftColor,
         theme,
@@ -934,10 +990,16 @@ export default function transformProps(
             if (value.observation === 0 && stack) {
               return;
             }
+            const seriesForKey = series.find(s => s.name === key);
+            const symbolForSeries = (seriesForKey as any)?.symbol || 'circle';
+            const marker = value.color
+              ? getSymbolMarker(symbolForSeries, value.color)
+              : value.marker;
             const row = formatForecastTooltipSeries({
               ...value,
               seriesName: key,
               formatter,
+              marker,
             });
 
             const annotationRow = annotationLayers.some(

superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/transformers.ts

@@ -221,6 +221,7 @@ export function transformSeries(
     seriesKey?: OptionName;
     sliceId?: number;
     isHorizontal?: boolean;
+    lineSymbol?: string;
     lineStyle?: LineStyleOption;
     queryIndex?: number;
     timeCompare?: string[];
@@ -359,8 +360,10 @@ export function transformSeries(
 
   // Use filled circles in dark mode to avoid the white fill issue with hollow circles
   // Use emptyCircle explicitly in light mode
-  const symbol =
-    plotType === 'line' ? (isDarkMode ? 'circle' : 'emptyCircle') : undefined;
+  let symbol;
+  if (plotType === 'line') {
+    symbol = opts.lineSymbol || (isDarkMode ? 'circle' : 'emptyCircle');
+  }
 
   return {
     ...series,

superset-frontend/plugins/plugin-chart-echarts/src/types.ts

@@ -92,6 +92,7 @@ export type ForecastValue = {
   forecastTrend?: number;
   forecastLower?: number;
   forecastUpper?: number;
+  color?: string;
 };
 
 export type LegendFormData = {

superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts

@@ -60,7 +60,7 @@ export const extractForecastValuesFromTooltipParams = (
 ): Record<string, ForecastValue> => {
   const values: Record<string, ForecastValue> = {};
   params.forEach(param => {
-    const { marker, seriesId, value } = param;
+    const { marker, seriesId, value, color } = param;
     const context = extractForecastSeriesContext(seriesId);
     const numericValue = isHorizontal ? value[0] : value[1];
     if (typeof numericValue === 'number') {
@@ -69,6 +69,7 @@ export const extractForecastValuesFromTooltipParams = (
           marker: marker || '',
         };
       const forecastValues = values[context.name];
+      forecastValues.color = color;
       if (context.type === ForecastSeriesEnum.Observation)
         forecastValues.observation = numericValue;
       if (context.type === ForecastSeriesEnum.ForecastTrend)

superset-frontend/plugins/plugin-chart-echarts/test/Timeseries/transformProps.test.ts

@@ -955,6 +955,7 @@ test('should apply dashed line style to time comparison series with single metri
     formData: {
       ...timeCompareFormData,
       time_compare: ['1 week ago'],
+      timeShiftColor: true,
       comparison_type: ComparisonType.Values,
     },
     queriesData: queriesDataWithTimeCompare,
@@ -974,18 +975,9 @@ test('should apply dashed line style to time comparison series with single metri
   expect(comparisonSeries).toBeDefined();
   // Main series should not have a dash pattern array
   expect(Array.isArray(mainSeries?.lineStyle?.type)).toBe(false);
-  // Comparison series should have a visible dash pattern array [dash, gap]
-  expect(Array.isArray(comparisonSeries?.lineStyle?.type)).toBe(true);
-  expect(
-    Array.isArray(comparisonSeries?.lineStyle?.type)
-      ? comparisonSeries.lineStyle.type[0]
-      : undefined,
-  ).toBeGreaterThanOrEqual(4);
-  expect(
-    Array.isArray(comparisonSeries?.lineStyle?.type)
-      ? comparisonSeries.lineStyle.type[1]
-      : undefined,
-  ).toBeGreaterThanOrEqual(3);
+  expect(mainSeries?.lineStyle?.type).not.toBe('dotted');
+  // Comparison series should have a visible dash pattern
+  expect(comparisonSeries?.lineStyle?.type).toBe('dotted');
 });
 
 test('should apply dashed line style to time comparison series with metric__offset pattern', () => {
@@ -1008,6 +1000,7 @@ test('should apply dashed line style to time comparison series with metric__offs
     formData: {
       ...timeCompareFormData,
       time_compare: ['1 week ago'],
+      timeShiftColor: true,
       comparison_type: ComparisonType.Values,
     },
     queriesData: queriesDataWithTimeCompare,
@@ -1029,18 +1022,8 @@ test('should apply dashed line style to time comparison series with metric__offs
   expect(comparisonSeries).toBeDefined();
   // Main series should not have a dash pattern array
   expect(Array.isArray(mainSeries?.lineStyle?.type)).toBe(false);
-  // Comparison series should have a visible dash pattern array [dash, gap]
-  expect(Array.isArray(comparisonSeries?.lineStyle?.type)).toBe(true);
-  expect(
-    Array.isArray(comparisonSeries?.lineStyle?.type)
-      ? comparisonSeries.lineStyle.type[0]
-      : undefined,
-  ).toBeGreaterThanOrEqual(4);
-  expect(
-    Array.isArray(comparisonSeries?.lineStyle?.type)
-      ? comparisonSeries.lineStyle.type[1]
-      : undefined,
-  ).toBeGreaterThanOrEqual(3);
+  // Comparison series should have a visible dash pattern
+  expect(comparisonSeries?.lineStyle?.type).toBe('dotted');
 });
 
 test('should apply connectNulls to time comparison series', () => {
@@ -1352,3 +1335,52 @@ test('should not apply axis bounds calculation when seriesType is not Bar for ho
   // Should not have explicit max set when seriesType is not Bar
   expect(xAxisRaw.max).toBeUndefined();
 });
+
+test('should assign distinct dash patterns for multiple time offsets consistently', () => {
+  const queriesDataWithMultipleOffsets = [
+    createTestQueryData([
+      {
+        sum__num: 100,
+        '1 year ago': 80,
+        '2 years ago': 60,
+        __timestamp: 599616000000,
+      },
+      {
+        sum__num: 150,
+        '1 year ago': 120,
+        '2 years ago': 90,
+        __timestamp: 599916000000,
+      },
+    ]),
+  ];
+
+  const chartProps = createTestChartProps({
+    formData: {
+      ...timeCompareFormData,
+      time_compare: ['1 year ago', '2 years ago'],
+      comparison_type: ComparisonType.Values,
+      timeShiftColor: true,
+    },
+    queriesData: queriesDataWithMultipleOffsets,
+  });
+
+  const transformed = transformProps(chartProps);
+  const series = (transformed.echartOptions.series as SeriesOption[]) || [];
+
+  const series1 = series.find(s => s.name === '1 year ago') as any;
+  const series2 = series.find(s => s.name === '2 years ago') as any;
+
+  expect(series1).toBeDefined();
+  expect(series2).toBeDefined();
+
+  const pattern1 = series1.lineStyle?.type;
+  const symbol1 = series1.symbol;
+  const pattern2 = series2.lineStyle?.type;
+  const symbol2 = series2.symbol;
+
+  // must be different patterns
+  expect(pattern1).not.toEqual(pattern2);
+
+  // must be different patterns
+  expect(symbol1).not.toEqual(symbol2);
+});