fix(colors): reassign colliding series when dashboard locks shared dimension color (#39297)

Co-authored-by: codeant-ai-for-open-source[bot] <244253245+codeant-ai-for-open-source[bot]@users.noreply.github.com>

superset-frontend/packages/superset-ui-core/src/color/CategoricalColorScale.ts

@@ -94,11 +94,20 @@ class CategoricalColorScale extends ExtensibleFunction {
 
   /**
    * Increment the color range with analogous colors
+   *
+   * @param forceMinimumExpansion When true, expand at least once even if the
+   * ordinal domain is still shorter than the palette. Shared dashboard labels
+   * can resolve from the global map without entering the scale domain, so
+   * domain-based sizing alone would skip expansion while collision resolution
+   * still needs analogous colors.
    */
-  incrementColorRange() {
+  incrementColorRange(forceMinimumExpansion = false) {
-    const multiple = Math.floor(
+    const domainBasedMultiple = Math.floor(
       this.domain().length / this.originColors.length,
     );
+    const multiple = forceMinimumExpansion
+      ? Math.max(domainBasedMultiple, 1)
+      : domainBasedMultiple;
     // the domain has grown larger than the original range
     // increments the range with analogous colors
     if (multiple > this.multiple) {
@@ -144,6 +153,7 @@ class CategoricalColorScale extends ExtensibleFunction {
       if (isFeatureEnabled(FeatureFlag.UseAnalogousColors)) {
         this.incrementColorRange();
       }
+
       if (
         // feature flag to be deprecated (will become standard behaviour)
         isFeatureEnabled(FeatureFlag.AvoidColorsCollision) &&
@@ -154,6 +164,39 @@ class CategoricalColorScale extends ExtensibleFunction {
       }
     }
 
+    if (
+      isFeatureEnabled(FeatureFlag.AvoidColorsCollision) &&
+      source === LabelsColorMapSource.Dashboard &&
+      (forcedColor || isExistingLabel)
+    ) {
+      const colliding = [...this.chartLabelsColorMap.entries()].filter(
+        ([labelKey, c]) => c === color && labelKey !== cleanedValue,
+      );
+      if (
+        colliding.length > 0 &&
+        isFeatureEnabled(FeatureFlag.UseAnalogousColors)
+      ) {
+        this.incrementColorRange(true);
+      }
+      for (const [otherLabel] of colliding) {
+        if (
+          Object.prototype.hasOwnProperty.call(this.forcedColors, otherLabel)
+        ) {
+          continue;
+        }
+        const newColor = this.getNextAvailableColor(otherLabel, color);
+        this.chartLabelsColorMap.set(otherLabel, newColor);
+        if (sliceId) {
+          this.labelsColorMapInstance.addSlice(
+            otherLabel,
+            newColor,
+            sliceId,
+            appliedColorScheme,
+          );
+        }
+      }
+    }
+
     // keep track of values in this slice
     this.chartLabelsColorMap.set(cleanedValue, color);
 

superset-frontend/packages/superset-ui-core/test/color/CategoricalColorScale.test.ts

@@ -21,6 +21,7 @@ import { ScaleOrdinal } from 'd3-scale';
 import {
   CategoricalColorScale,
   FeatureFlag,
+  getLabelsColorMap,
   LabelsColorMapSource,
 } from '@superset-ui/core';
 
@@ -199,10 +200,42 @@ describe('CategoricalColorScale', () => {
       const returnedColor = scale.getColor(value, sliceId);
       expect(returnedColor).toBe(expectedColor);
     });
+    test('reassigns colliding colors when no sliceId is provided', () => {
+      window.featureFlags = {
+        [FeatureFlag.AvoidColorsCollision]: true,
+      };
+      const PALETTE = ['red', 'blue', 'green'];
+
+      const chartAScale = new CategoricalColorScale(PALETTE);
+      const labelsColorMap = chartAScale.labelsColorMapInstance;
+      labelsColorMap.reset();
+      labelsColorMap.source = LabelsColorMapSource.Dashboard;
+
+      try {
+        chartAScale.getColor('Trains', 101, 'testScheme');
+
+        const chartBScale = new CategoricalColorScale(PALETTE);
+        // Call getColor without sliceId (or with undefined)
+        chartBScale.getColor('Classic Cars', undefined, 'testScheme');
+        chartBScale.getColor('Trains', undefined, 'testScheme');
+
+        const classicCarsColor =
+          chartBScale.chartLabelsColorMap.get('Classic Cars');
+        const trainsColor = chartBScale.chartLabelsColorMap.get('Trains');
+
+        expect(trainsColor).toBe('red');
+        expect(classicCarsColor).toBeDefined();
+        expect(classicCarsColor).not.toBe('red');
+      } finally {
+        labelsColorMap.reset();
+        labelsColorMap.source = LabelsColorMapSource.Dashboard;
+      }
+    });
     test('conditionally calls getNextAvailableColor', () => {
       window.featureFlags = {
         [FeatureFlag.AvoidColorsCollision]: true,
       };
+      scale.labelsColorMapInstance.source = LabelsColorMapSource.Explore;
 
       scale.getColor('testValue1');
       scale.getColor('testValue2');
@@ -225,6 +258,27 @@ describe('CategoricalColorScale', () => {
 
       expect(getNextAvailableColorSpy).not.toHaveBeenCalled();
     });
+    test('reassigns non-forced labels when a dashboard-synced label would duplicate their color', () => {
+      window.featureFlags = {
+        [FeatureFlag.AvoidColorsCollision]: true,
+      };
+
+      const dashScale = new CategoricalColorScale(['red', 'blue', 'green']);
+      const sliceId = 501;
+      const colorScheme = 'preset';
+
+      dashScale.labelsColorMapInstance.source = LabelsColorMapSource.Dashboard;
+      jest
+        .spyOn(dashScale.labelsColorMapInstance, 'getColorMap')
+        .mockReturnValue(new Map([['Trains', 'red']]));
+
+      dashScale.getColor('Classic Cars', sliceId, colorScheme);
+      dashScale.getColor('Trains', sliceId, colorScheme);
+
+      expect(dashScale.chartLabelsColorMap.get('Trains')).toBe('red');
+      expect(dashScale.chartLabelsColorMap.get('Classic Cars')).not.toBe('red');
+      expect(dashScale.chartLabelsColorMap.get('Classic Cars')).toBeDefined();
+    });
   });
 
   describe('.setColor(value, forcedColor)', () => {
@@ -479,6 +533,131 @@ describe('CategoricalColorScale', () => {
     });
   });
 
+  describe('dashboard shared-dimension color collision', () => {
+    let labelsColorMap: ReturnType<typeof getLabelsColorMap>;
+
+    beforeEach(() => {
+      window.featureFlags = {
+        [FeatureFlag.AvoidColorsCollision]: true,
+      };
+      const sentinel = new CategoricalColorScale(['red', 'blue', 'green']);
+      labelsColorMap = sentinel.labelsColorMapInstance;
+      labelsColorMap.reset();
+      labelsColorMap.source = LabelsColorMapSource.Dashboard;
+    });
+
+    afterEach(() => {
+      jest.restoreAllMocks();
+      labelsColorMap.reset();
+    });
+
+    test('reproduces the bug without the fix: Classic Cars and Trains would both be red', () => {
+      window.featureFlags = {
+        [FeatureFlag.AvoidColorsCollision]: false,
+      };
+
+      const PALETTE = ['red', 'blue', 'green'];
+
+      const chartAScale = new CategoricalColorScale(PALETTE);
+      chartAScale.getColor('Trains', 101, 'testScheme');
+      expect(labelsColorMap.getColorMap().get('Trains')).toBe('red');
+
+      const chartBScale = new CategoricalColorScale(PALETTE);
+      chartBScale.getColor('Classic Cars', 102, 'testScheme');
+      chartBScale.getColor('Trains', 102, 'testScheme');
+
+      const classicCarsColor =
+        chartBScale.chartLabelsColorMap.get('Classic Cars');
+      const trainsColor = chartBScale.chartLabelsColorMap.get('Trains');
+
+      expect(trainsColor).toBe('red');
+      expect(classicCarsColor).toBe('red');
+    });
+
+    test('fix: Classic Cars is reassigned when Trains locks red from the dashboard', () => {
+      const PALETTE = ['red', 'blue', 'green'];
+
+      const chartAScale = new CategoricalColorScale(PALETTE);
+      chartAScale.getColor('Trains', 101, 'testScheme');
+      expect(labelsColorMap.getColorMap().get('Trains')).toBe('red');
+
+      const chartBScale = new CategoricalColorScale(PALETTE);
+      chartBScale.getColor('Classic Cars', 102, 'testScheme');
+      chartBScale.getColor('Trains', 102, 'testScheme');
+
+      const classicCarsColor =
+        chartBScale.chartLabelsColorMap.get('Classic Cars');
+      const trainsColor = chartBScale.chartLabelsColorMap.get('Trains');
+
+      expect(trainsColor).toBe('red');
+      expect(classicCarsColor).toBeDefined();
+      expect(classicCarsColor).not.toBe('red');
+    });
+
+    test('fix: no series in Chart B share a color when palette has enough colors', () => {
+      const PALETTE = ['red', 'blue', 'green'];
+
+      const chartAScale = new CategoricalColorScale(PALETTE);
+      chartAScale.getColor('Trains', 101, 'testScheme');
+
+      const chartBScale = new CategoricalColorScale(PALETTE);
+      chartBScale.getColor('Classic Cars', 102, 'testScheme');
+      chartBScale.getColor('Trains', 102, 'testScheme');
+
+      const colors = Array.from(chartBScale.chartLabelsColorMap.values());
+      const uniqueColors = new Set(colors);
+
+      expect(uniqueColors.size).toBe(colors.length);
+    });
+
+    test('fix: increments analogous color range for dashboard collisions when UseAnalogousColors is enabled', () => {
+      window.featureFlags = {
+        [FeatureFlag.AvoidColorsCollision]: true,
+        [FeatureFlag.UseAnalogousColors]: true,
+      };
+
+      const PALETTE = ['red', 'blue', 'green'];
+
+      const chartAScale = new CategoricalColorScale(PALETTE);
+      chartAScale.getColor('Trains', 101, 'testScheme');
+
+      const chartBScale = new CategoricalColorScale(PALETTE);
+      const addSliceSpy = jest.spyOn(
+        chartBScale.labelsColorMapInstance,
+        'addSlice',
+      );
+      chartBScale.getColor('Classic Cars', 102, 'testScheme');
+      chartBScale.getColor('Model T', 102, 'testScheme');
+      chartBScale.getColor('Trains', 102, 'testScheme');
+
+      expect(chartBScale.chartLabelsColorMap.get('Trains')).toBe('red');
+      expect(chartBScale.chartLabelsColorMap.get('Classic Cars')).toBeDefined();
+      expect(chartBScale.chartLabelsColorMap.get('Classic Cars')).not.toBe(
+        'red',
+      );
+      expect(chartBScale.range()).toHaveLength(6);
+      expect(
+        addSliceSpy.mock.calls.some(
+          ([label, color]) => label === 'Classic Cars' && color !== 'red',
+        ),
+      ).toBe(true);
+    });
+
+    test('fix: forced colors (user-set in dashboard JSON) are never reassigned', () => {
+      const PALETTE = ['red', 'blue', 'green'];
+      const forcedColors = { 'Classic Cars': 'red' };
+
+      const chartAScale = new CategoricalColorScale(PALETTE);
+      chartAScale.getColor('Trains', 101, 'testScheme');
+
+      const chartBScale = new CategoricalColorScale(PALETTE, forcedColors);
+      chartBScale.getColor('Classic Cars', 102, 'testScheme');
+      chartBScale.getColor('Trains', 102, 'testScheme');
+
+      expect(chartBScale.chartLabelsColorMap.get('Classic Cars')).toBe('red');
+    });
+  });
+
   describe("is compatible with D3's ScaleOrdinal", () => {
     test('passes type check', () => {
       const scale: ScaleOrdinal<{ toString(): string }, string> =