Skip to content

Commit

Permalink
fix(Dashboard): Ensure shared label colors are updated (#31031)
Browse files Browse the repository at this point in the history
  • Loading branch information
@geido
geido authored Nov 23, 2024
1 parent 67ad7da commit 91301bc
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 49 deletions.
73 changes: 34 additions & 39 deletions superset-frontend/src/dashboard/actions/dashboardState.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
t,
getClientErrorObject,
getCategoricalSchemeRegistry,
promiseTimeout,
} from '@superset-ui/core';
import {
addChart,
Expand Down Expand Up @@ -737,9 +738,9 @@ export const persistDashboardLabelsColor = () => async (dispatch, getState) => {
} = getState();

if (labelsColorMapMustSync || sharedLabelsColorsMustSync) {
storeDashboardMetadata(id, metadata);
dispatch(setDashboardLabelsColorMapSynced());
dispatch(setDashboardSharedLabelsColorsSynced());
storeDashboardMetadata(id, metadata);
}
};

Expand All @@ -755,16 +756,13 @@ export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => {
try {
const updatedMetadata = { ...metadata };
const customLabelsColor = metadata.label_colors || {};
const sharedLabelsColor = metadata.shared_label_colors || [];
let hasChanged = false;

// backward compatibility of shared_label_colors
const sharedLabels = metadata.shared_label_colors || [];
if (!Array.isArray(sharedLabels) && Object.keys(sharedLabels).length > 0) {
hasChanged = true;
updatedMetadata.shared_label_colors = getFreshSharedLabels(
Object.keys(sharedLabelsColor),
);
updatedMetadata.shared_label_colors = [];
}
// backward compatibility of map_label_colors
const hasMapLabelColors =
Expand Down Expand Up @@ -828,46 +826,50 @@ export const applyDashboardLabelsColorOnLoad = metadata => async dispatch => {
* @returns void
*/
export const ensureSyncedLabelsColorMap = metadata => (dispatch, getState) => {
const {
dashboardState: { labelsColorMapMustSync },
} = getState();
const updatedMetadata = { ...metadata };
const customLabelsColor = metadata.label_colors || {};
const isMapSynced = isLabelsColorMapSynced(metadata);
const mustSync = !isMapSynced;

if (mustSync) {
const freshestColorMapEntries = getLabelsColorMapEntries(customLabelsColor);
updatedMetadata.map_label_colors = freshestColorMapEntries;
dispatch(setDashboardMetadata(updatedMetadata));
}
const syncLabelsColorMap = () => {
const {
dashboardState: { labelsColorMapMustSync },
} = getState();
const updatedMetadata = { ...metadata };
const customLabelsColor = metadata.label_colors || {};
const isMapSynced = isLabelsColorMapSynced(metadata);
const mustSync = !isMapSynced;

if (mustSync && !labelsColorMapMustSync) {
// prepare to persist the just applied labels color map
dispatch(setDashboardLabelsColorMapSync());
}
if (!mustSync && labelsColorMapMustSync) {
dispatch(setDashboardLabelsColorMapSynced());
}
if (mustSync) {
const freshestColorMapEntries =
getLabelsColorMapEntries(customLabelsColor);
updatedMetadata.map_label_colors = freshestColorMapEntries;
dispatch(setDashboardMetadata(updatedMetadata));
}

if (mustSync && !labelsColorMapMustSync) {
// prepare to persist the just applied labels color map
dispatch(setDashboardLabelsColorMapSync());
}
};
promiseTimeout(syncLabelsColorMap, 500);
};

/**
*
* Ensure that the stored shared labels colors match current.
*
* @param {*} metadata - the dashboard metadata
* @param {*} forceFresh - when true it will use the fresh shared labels ignoring stored ones
* @returns void
*/
export const ensureSyncedSharedLabelsColors =
metadata => (dispatch, getState) => {
// using a timeout to let the rendered charts finish processing labels
setTimeout(() => {
(metadata, forceFresh = false) =>
(dispatch, getState) => {
const syncSharedLabelsColors = () => {
const {
dashboardState: { sharedLabelsColorsMustSync },
} = getState();
const updatedMetadata = { ...metadata };
const sharedLabelsColors = metadata.shared_label_colors || [];
const freshLabelsColors = getFreshSharedLabels(sharedLabelsColors);
const freshLabelsColors = getFreshSharedLabels(
forceFresh ? [] : sharedLabelsColors,
);
const isSharedLabelsColorsSynced = isEqual(
sharedLabelsColors,
freshLabelsColors,
Expand All @@ -884,10 +886,8 @@ export const ensureSyncedSharedLabelsColors =
// prepare to persist the shared labels colors
dispatch(setDashboardSharedLabelsColorsSync());
}
if (!mustSync && sharedLabelsColorsMustSync) {
dispatch(setDashboardSharedLabelsColorsSynced());
}
}, 500);
};
promiseTimeout(syncSharedLabelsColors, 500);
};

/**
Expand All @@ -909,7 +909,6 @@ export const updateDashboardLabelsColor =
const fullLabelsColors = metadata.map_label_colors || {};
const sharedLabelsColors = metadata.shared_label_colors || [];
const customLabelsColors = metadata.label_colors || {};
const updatedMetadata = { ...metadata };

// for dashboards with no color scheme, the charts should always use their individual schemes
// this logic looks for unique labels (not shared across multiple charts) of each rendered chart
Expand Down Expand Up @@ -965,11 +964,7 @@ export const updateDashboardLabelsColor =
const shouldGoFresh = shouldReset.length > 0 ? shouldReset : false;
const shouldMerge = !shouldGoFresh;
// re-apply the color map first to get fresh maps accordingly
applyColors(updatedMetadata, shouldGoFresh, shouldMerge);
// new data may have appeared in the map (data changes)
// or new slices may have appeared while changing tabs
dispatch(ensureSyncedLabelsColorMap(updatedMetadata));
dispatch(ensureSyncedSharedLabelsColors(updatedMetadata));
applyColors(metadata, shouldGoFresh, shouldMerge);
} catch (e) {
console.error('Failed to update colors for new charts and labels:', e);
}
Expand Down
39 changes: 30 additions & 9 deletions superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,8 @@ import {
applyDashboardLabelsColorOnLoad,
updateDashboardLabelsColor,
persistDashboardLabelsColor,
ensureSyncedSharedLabelsColors,
ensureSyncedLabelsColorMap,
} from 'src/dashboard/actions/dashboardState';
import { getColorNamespace, resetColors } from 'src/utils/colorScheme';
import { NATIVE_FILTER_DIVIDER_PREFIX } from '../nativeFilters/FiltersConfigModal/utils';
Expand Down Expand Up @@ -96,7 +98,6 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const [dashboardLabelsColorInitiated, setDashboardLabelsColorInitiated] =
useState(false);
const prevRenderedChartIds = useRef<number[]>([]);

const prevTabIndexRef = useRef();
const tabIndex = useMemo(() => {
const nextTabIndex = findTabIndexByComponentId({
Expand All @@ -110,6 +111,18 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
prevTabIndexRef.current = nextTabIndex;
return nextTabIndex;
}, [dashboardLayout, directPathToChild]);
// when all charts have rendered, enforce fresh shared labels
const shouldForceFreshSharedLabelsColors =
dashboardLabelsColorInitiated &&
renderedChartIds.length > 0 &&
chartIds.length === renderedChartIds.length &&
prevRenderedChartIds.current.length < renderedChartIds.length;

const onBeforeUnload = useCallback(() => {
dispatch(persistDashboardLabelsColor());
resetColors(getColorNamespace(dashboardInfo?.metadata?.color_namespace));
prevRenderedChartIds.current = [];
}, [dashboardInfo?.metadata?.color_namespace, dispatch]);

useEffect(() => {
if (nativeFilterScopes.length === 0) {
Expand Down Expand Up @@ -148,11 +161,12 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
const activeKey = min === 0 ? DASHBOARD_GRID_ID : min.toString();
const TOP_OF_PAGE_RANGE = 220;

const onBeforeUnload = useCallback(() => {
dispatch(persistDashboardLabelsColor());
resetColors(getColorNamespace(dashboardInfo?.metadata?.color_namespace));
prevRenderedChartIds.current = [];
}, [dashboardInfo?.metadata?.color_namespace, dispatch]);
useEffect(() => {
if (shouldForceFreshSharedLabelsColors) {
// all available charts have rendered, enforce freshest shared label colors
dispatch(ensureSyncedSharedLabelsColors(dashboardInfo.metadata, true));
}
}, [dashboardInfo.metadata, dispatch, shouldForceFreshSharedLabelsColors]);

useEffect(() => {
// verify freshness of color map
Expand All @@ -161,7 +175,6 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {

if (
dashboardLabelsColorInitiated &&
dashboardInfo?.id &&
numRenderedCharts > 0 &&
prevRenderedChartIds.current.length < numRenderedCharts
) {
Expand All @@ -170,22 +183,30 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
);
prevRenderedChartIds.current = renderedChartIds;
dispatch(updateDashboardLabelsColor(newRenderedChartIds));
// new data may have appeared in the map (data changes)
// or new slices may have appeared while changing tabs
dispatch(ensureSyncedLabelsColorMap(dashboardInfo.metadata));

if (!shouldForceFreshSharedLabelsColors) {
dispatch(ensureSyncedSharedLabelsColors(dashboardInfo.metadata));
}
}
}, [
dashboardInfo?.id,
renderedChartIds,
dispatch,
dashboardLabelsColorInitiated,
dashboardInfo.metadata,
shouldForceFreshSharedLabelsColors,
]);

useEffect(() => {
const labelsColorMap = getLabelsColorMap();
labelsColorMap.source = LabelsColorMapSource.Dashboard;

if (dashboardInfo?.id && !dashboardLabelsColorInitiated) {
dispatch(applyDashboardLabelsColorOnLoad(dashboardInfo.metadata));
// apply labels color as dictated by stored metadata (if any)
setDashboardLabelsColorInitiated(true);
dispatch(applyDashboardLabelsColorOnLoad(dashboardInfo.metadata));
}

return () => {
Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/src/utils/colorScheme.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export const getColorNamespace = (namespace?: string) => namespace || undefined;
* Get labels shared across all charts in a dashboard.
* Merges a fresh instance of shared label colors with a stored one.
*
* @param currentSharedLabels - existing shared labels to merge with fresh
* @returns Record<string, string>
*/
export const getFreshSharedLabels = (
Expand Down Expand Up @@ -74,7 +75,7 @@ export const getSharedLabelsColorMapEntries = (
* @returns all color entries except custom label colors
*/
export const getLabelsColorMapEntries = (
customLabelsColor: Record<string, string>,
customLabelsColor: Record<string, string> = {},
): Record<string, string> => {
const labelsColorMapInstance = getLabelsColorMap();
const allEntries = Object.fromEntries(labelsColorMapInstance.getColorMap());
Expand Down

0 comments on commit 91301bc

Please sign in to comment.