fix(Dashboard): Keep track of charts origin color schemes

superset-frontend/cypress-base/cypress/e2e/dashboard/editmode.test.ts

@@ -507,7 +507,7 @@ describe('Dashboard edit', () => {
       // label Anthony
       cy.get('[data-test-chart-name="Trends"] .line .nv-legend-symbol')
         .eq(2)
-        .should('have.css', 'fill', 'rgb(108, 69, 146)');
+        .should('have.css', 'fill', 'rgb(0, 128, 246)');
 
       // open main tab and nested tab
       openTab(0, 0);
@@ -518,7 +518,7 @@ describe('Dashboard edit', () => {
         '[data-test-chart-name="Top 10 California Names Timeseries"] .line .nv-legend-symbol',
       )
         .first()
-        .should('have.css', 'fill', 'rgb(108, 69, 146)');
+        .should('have.css', 'fill', 'rgb(0, 128, 246)');
     });
 
     it('should apply the color scheme across main tabs', () => {

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

@@ -17,7 +17,9 @@
  * under the License.
  */
 
+import { getCategoricalSchemeRegistry } from '.';
 import { makeSingleton } from '../utils';
+import CategoricalColorNamespace from './CategoricalColorNamespace';
 
 export enum LabelsColorMapSource {
   Dashboard,
@@ -38,7 +40,16 @@ export class LabelsColorMap {
     this.source = LabelsColorMapSource.Dashboard;
   }
 
-  updateColorMap(categoricalNamespace: any, colorScheme?: string) {
+  /**
+   * Wipes out the color map and updates it with the new color scheme.
+   *
+   * @param categoricalNamespace - the namespace to use for color mapping
+   * @param colorScheme - color scheme
+   */
+  updateColorMap(
+    categoricalNamespace: CategoricalColorNamespace,
+    colorScheme?: string,
+  ) {
     const newColorMap = new Map();
     this.colorMap.clear();
     this.chartsLabelsMap.forEach((chartConfig, sliceId) => {
@@ -58,6 +69,16 @@ export class LabelsColorMap {
     return this.colorMap;
   }
 
+  /**
+   *
+   * Called individually by each plugin via getColor fn.
+   *
+   * @param label - the label name
+   * @param color - the color
+   * @param sliceId - the chart id
+   * @param colorScheme - the color scheme
+   *
+   */
   addSlice(
     label: string,
     color: string,
@@ -75,12 +96,34 @@ export class LabelsColorMap {
       labels.push(label);
       this.chartsLabelsMap.set(sliceId, {
         labels,
-        scheme: colorScheme,
+        scheme: this.chartsLabelsMap.get(sliceId)?.scheme || colorScheme,
       });
     }
     this.colorMap.set(label, color);
   }
 
+  /**
+   * Keeps track of original colors of charts in their formData.
+   *
+   * @param sliceId - chart id
+   * @param colorScheme - color scheme
+   */
+  setSliceOriginColorScheme(sliceId: number, colorScheme?: string) {
+    const categoricalSchemes = getCategoricalSchemeRegistry();
+    const fallbackColorScheme =
+      categoricalSchemes.getDefaultKey()?.toString() ?? 'supersetColors';
+
+    this.chartsLabelsMap.set(sliceId, {
+      labels: this.chartsLabelsMap.get(sliceId)?.labels || [],
+      scheme: colorScheme || fallbackColorScheme,
+    });
+  }
+
+  /**
+   * Remove a slice from the color map.
+   *
+   * @param sliceId - the chart
+   */
   removeSlice(sliceId: number) {
     if (this.source !== LabelsColorMapSource.Dashboard) return;
 
@@ -96,6 +139,9 @@ export class LabelsColorMap {
     this.colorMap = newColorMap;
   }
 
+  /**
+   * Clear the shared labels color map.
+   */
   clear() {
     this.chartsLabelsMap.clear();
     this.colorMap.clear();

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

@@ -60,6 +60,29 @@ describe('LabelsColorMap', () => {
     expect(getLabelsColorMap()).toBeInstanceOf(LabelsColorMap);
   });
 
+  describe('.setSliceOriginColorScheme(sliceId, scheme)', () => {
+    it('should set slice color scheme', () => {
+      const labelsColorMap = getLabelsColorMap();
+      labelsColorMap.setSliceOriginColorScheme(1, 'testColors');
+      expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+        'testColors',
+      );
+    });
+    it('should fallback to default color scheme if not provided', () => {
+      const labelsColorMap = getLabelsColorMap();
+      labelsColorMap.setSliceOriginColorScheme(1);
+      expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+        'SUPERSET_DEFAULT',
+      );
+    });
+    it('should re-use existing labels when setting color scheme', () => {
+      const labelsColorMap = getLabelsColorMap();
+      labelsColorMap.addSlice('a', 'red', 1);
+      labelsColorMap.setSliceOriginColorScheme(1, 'testColors');
+      expect(labelsColorMap.chartsLabelsMap.get(1)?.labels).toEqual(['a']);
+    });
+  });
+
   describe('.addSlice(value, color, sliceId)', () => {
     it('should add to sliceLabelColorMap when first adding label', () => {
       const labelsColorMap = getLabelsColorMap();
@@ -98,6 +121,24 @@ describe('LabelsColorMap', () => {
       labelsColorMap.addSlice('a', 'red', 1);
       expect(Object.fromEntries(labelsColorMap.chartsLabelsMap)).toEqual({});
     });
+
+    it('should use set slice color scheme via setSliceOriginColorScheme', () => {
+      const labelsColorMap = getLabelsColorMap();
+      labelsColorMap.setSliceOriginColorScheme(1, 'testColors2');
+      labelsColorMap.addSlice('a', 'red', 1);
+      expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+        'testColors2',
+      );
+    });
+
+    it('should use given color scheme when not set via setSliceOriginColorScheme', () => {
+      const labelsColorMap = getLabelsColorMap();
+      labelsColorMap.setSliceOriginColorScheme(2, 'testColors2');
+      labelsColorMap.addSlice('a', 'red', 1, 'testColors');
+      expect(labelsColorMap.chartsLabelsMap.get(1)?.scheme).toEqual(
+        'testColors',
+      );
+    });
   });
 
   describe('.remove(sliceId)', () => {

superset-frontend/src/dashboard/actions/dashboardState.js

@@ -595,6 +595,7 @@ export function addSliceToDashboard(id) {
       dispatch(fetchDatasourceMetadata(form_data.datasource)),
     ]).then(() => {
       dispatch(addSlice(selectedSlice));
+      getLabelsColorMap().setSliceOriginColorScheme(id, form_data.color_scheme);
     });
   };
 }
@@ -686,62 +687,73 @@ const updateDashboardMetadata = async (id, metadata, dispatch) => {
   dispatch(dashboardInfoChanged({ metadata }));
 };
 
-export const updateDashboardLabelsColor = () => async (dispatch, getState) => {
-  const {
-    dashboardInfo: { id, metadata },
-  } = getState();
-  const categoricalSchemes = getCategoricalSchemeRegistry();
-  const colorScheme = metadata?.color_scheme;
-  const colorSchemeRegistry = categoricalSchemes.get(
-    metadata?.color_scheme,
-    true,
-  );
-  const defaultScheme = categoricalSchemes.defaultKey;
-  const fallbackScheme = defaultScheme?.toString() || 'supersetColors';
-  const colorSchemeDomain = metadata?.color_scheme_domain || [];
+export const updateDashboardLabelsColor =
+  charts => async (dispatch, getState) => {
+    const {
+      dashboardInfo: { id, metadata },
+    } = getState();
+    const categoricalSchemes = getCategoricalSchemeRegistry();
+    const sharedColorMapInstance = getLabelsColorMap();
+    const colorScheme = metadata?.color_scheme;
+    const colorSchemeRegistry = categoricalSchemes.get(
+      metadata?.color_scheme,
+      true,
+    );
+    const defaultScheme = categoricalSchemes.defaultKey;
+    const fallbackScheme = defaultScheme?.toString() || 'supersetColors';
+    const colorSchemeDomain = metadata?.color_scheme_domain || [];
 
-  try {
-    const updatedMetadata = { ...metadata };
-    let updatedScheme = metadata?.color_scheme;
+    try {
+      const updatedMetadata = { ...metadata };
+      let updatedScheme = metadata?.color_scheme;
 
-    // Color scheme does not exist anymore, fallback to default
-    if (colorScheme && !colorSchemeRegistry) {
-      updatedScheme = fallbackScheme;
-      updatedMetadata.color_scheme = updatedScheme;
-      updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme);
+      // track original color scheme for each chart
+      charts.forEach(chart =>
+        sharedColorMapInstance.setSliceOriginColorScheme(
+          chart.id,
+          chart.form_data?.color_scheme,
+        ),
+      );
 
-      dispatch(setColorScheme(updatedScheme));
-      // must re-apply colors from fresh labels color map
-      applyColors(updatedMetadata, true);
+      // Color scheme does not exist anymore, fallback to default
+      if (colorScheme && !colorSchemeRegistry) {
+        updatedScheme = fallbackScheme;
+        updatedMetadata.color_scheme = updatedScheme;
+        updatedMetadata.color_scheme_domain = getColorSchemeDomain(colorScheme);
+
+        dispatch(setColorScheme(updatedScheme));
+        // must re-apply colors from fresh labels color map
+        applyColors(updatedMetadata, true);
+      }
+
+      // stored labels color map and applied might differ
+      // new data may appear in the map (data changes)
+      // or new slices may appear while changing tabs
+      // requires merging the stored and applied maps
+      const isMapSynced = isLabelsColorMapSynced(metadata);
+      if (!isMapSynced) {
+        // re-apply a fresh labels color map
+        applyColors(updatedMetadata, false, true);
+        // pull and store the just applied labels color map
+        updatedMetadata.shared_label_colors = getLabelsColorMapEntries();
+      }
+
+      // the stored color domain registry and fresh might differ at this point
+      const freshColorSchemeDomain = getColorSchemeDomain(colorScheme);
+      const isRegistrySynced =
+        colorSchemeDomain.toString() === freshColorSchemeDomain.toString();
+
+      if (colorScheme && !isRegistrySynced) {
+        updatedMetadata.color_scheme_domain = freshColorSchemeDomain;
+      }
+
+      if (
+        (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) ||
+        !isMapSynced
+      ) {
+        await updateDashboardMetadata(id, updatedMetadata, dispatch);
+      }
+    } catch (error) {
+      console.error('Failed to update dashboard color settings:', error);
     }
-
-    // stored labels color map and applied might differ
-    // new data may appear in the map (data changes)
-    // or new slices may appear while changing tabs
-    const isMapSynced = isLabelsColorMapSynced(metadata);
-    if (!isMapSynced) {
-      // re-apply a fresh labels color map
-      applyColors(updatedMetadata, false, true);
-      // pull and store the just applied labels color map
-      updatedMetadata.shared_label_colors = getLabelsColorMapEntries();
-    }
-
-    // the stored color domain registry and fresh might differ at this point
-    const freshColorSchemeDomain = getColorSchemeDomain(colorScheme);
-    const isRegistrySynced =
-      colorSchemeDomain.toString() === freshColorSchemeDomain.toString();
-
-    if (colorScheme && !isRegistrySynced) {
-      updatedMetadata.color_scheme_domain = freshColorSchemeDomain;
-    }
-
-    if (
-      (colorScheme && (!colorSchemeRegistry || !isRegistrySynced)) ||
-      !isMapSynced
-    ) {
-      await updateDashboardMetadata(id, updatedMetadata, dispatch);
-    }
-  } catch (error) {
-    console.error('Failed to update dashboard color settings:', error);
-  }
-};
+  };

superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx

@@ -31,6 +31,7 @@ import { pick } from 'lodash';
 import Tabs from 'src/components/Tabs';
 import DashboardGrid from 'src/dashboard/containers/DashboardGrid';
 import {
+  Chart,
   DashboardInfo,
   DashboardLayout,
   LayoutItem,
@@ -85,8 +86,13 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
   const directPathToChild = useSelector<RootState, string[]>(
     state => state.dashboardState.directPathToChild,
   );
-  const chartIds = useSelector<RootState, number[]>(state =>
-    Object.values(state.charts).map(chart => chart.id),
+  const charts = useSelector<RootState, Chart[]>(state =>
+    Object.values(state.charts),
+  );
+
+  const chartIds: number[] = useMemo(
+    () => charts.map(chart => chart.id),
+    [charts],
   );
 
   const prevTabIndexRef = useRef();
@@ -144,9 +150,11 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
     // verify freshness of color map on tab change
     // and when loading for first time
     setTimeout(() => {
-      dispatch(updateDashboardLabelsColor());
+      if (charts.length) {
+        dispatch(updateDashboardLabelsColor(charts));
+      }
     }, 500);
-  }, [directPathToChild, dispatch]);
+  }, [directPathToChild, dispatch, charts]);
 
   useEffect(() => {
     const labelsColorMap = getLabelsColorMap();

superset-frontend/src/utils/colorScheme.ts

@@ -115,18 +115,10 @@ export const applyColors = (
   const colorNameSpace = getColorNamespace(metadata?.color_namespace);
   const categoricalNamespace =
     CategoricalColorNamespace.getNamespace(colorNameSpace);
-  let colorScheme = metadata?.color_scheme;
+  const colorScheme = metadata?.color_scheme;
   const customLabelColors = metadata?.label_colors || {};
-  // when scheme unset, update only custom label colors
   const labelsColorMap = metadata?.shared_label_colors || {};
 
-  if (!colorScheme) {
-    // a fallback color scheme must be set to generate shared label colors
-    const categoricalSchemes = getCategoricalSchemeRegistry();
-    colorScheme =
-      categoricalSchemes.getDefaultKey()?.toString() ?? 'supersetColors';
-  }
-
   if (fresh) {
     // reset forced colors (custom labels + labels color map)
     categoricalNamespace.resetColors();