You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ga...@apache.org on 2026/08/10 19:52:51 UTC
(superset) branch master updated: fix(dashboard): changing a theme no longer discards unsaved edits or reloads charts
This is an automated email from the ASF dual-hosted git repository.
gabotorresruiz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 0dedc55f380 fix(dashboard): changing a theme no longer discards unsaved edits or reloads charts
0dedc55f380 is described below
commit 0dedc55f380996a1e9d6feb0253466e6397bf3bb
Author: Gabriel Torres Ruiz <ga...@gmail.com>
AuthorDate: Mon Aug 10 16:52:43 2026 -0300
fix(dashboard): changing a theme no longer discards unsaved edits or reloads charts
---
.../superset-core/src/theme/Theme.test.tsx | 37 +++++++++
.../packages/superset-core/src/theme/Theme.tsx | 41 ++++++----
.../src/components/CrudThemeProvider.test.tsx | 63 +++++++++++++++
.../src/components/CrudThemeProvider.tsx | 92 ++++++++++++++--------
.../src/dashboard/reducers/dashboardState.test.ts | 49 ++++++++++++
.../src/dashboard/reducers/dashboardState.ts | 2 +-
6 files changed, 236 insertions(+), 48 deletions(-)
diff --git a/superset-frontend/packages/superset-core/src/theme/Theme.test.tsx b/superset-frontend/packages/superset-core/src/theme/Theme.test.tsx
index b5ee3c86dc8..89a74d4a2b6 100644
--- a/superset-frontend/packages/superset-core/src/theme/Theme.test.tsx
+++ b/superset-frontend/packages/superset-core/src/theme/Theme.test.tsx
@@ -160,6 +160,43 @@ test('Theme.setConfig correctly applies algorithm changes', () => {
expect(serialized.algorithm).toBe(ThemeAlgorithm.DARK);
});
+test('Theme.setConfig with baseTheme merges the config over the base theme tokens', () => {
+ const baseTheme: AnyThemeConfig = {
+ token: { colorPrimary: '#111111', colorError: '#ff0000' },
+ };
+ const theme = Theme.fromConfig();
+ theme.setConfig({ token: { colorPrimary: '#0000ff' } }, baseTheme);
+
+ // Config wins for colorPrimary; the base theme fills the untouched colorError.
+ expect(theme.theme.colorPrimary).toBe('#0000ff');
+ expect(theme.theme.colorError).toBe('#ff0000');
+});
+
+test('Theme.setConfig with baseTheme keeps the base theme ECharts overrides', () => {
+ const baseTheme = {
+ token: { colorPrimary: '#111111' },
+ echartsOptionsOverrides: { backgroundColor: '#123456' },
+ echartsOptionsOverridesByChartType: {
+ pie: { itemStyle: { borderWidth: 2 } },
+ },
+ } as AnyThemeConfig & {
+ echartsOptionsOverrides: Record<string, unknown>;
+ echartsOptionsOverridesByChartType: Record<string, unknown>;
+ };
+ const theme = Theme.fromConfig();
+
+ // In-place update whose config sets no ECharts overrides: the base theme's
+ // overrides must survive, the same way its tokens do.
+ theme.setConfig({ token: { colorPrimary: '#0000ff' } }, baseTheme);
+
+ expect(theme.theme.echartsOptionsOverrides).toEqual({
+ backgroundColor: '#123456',
+ });
+ expect(theme.theme.echartsOptionsOverridesByChartType).toEqual({
+ pie: { itemStyle: { borderWidth: 2 } },
+ });
+});
+
test('Theme.toggleDarkMode switches to dark algorithm when toggling dark mode on', () => {
const theme = Theme.fromConfig();
diff --git a/superset-frontend/packages/superset-core/src/theme/Theme.tsx b/superset-frontend/packages/superset-core/src/theme/Theme.tsx
index 83e60c35c5c..b5b69ae7b3e 100644
--- a/superset-frontend/packages/superset-core/src/theme/Theme.tsx
+++ b/superset-frontend/packages/superset-core/src/theme/Theme.tsx
@@ -64,10 +64,12 @@ export class Theme {
* @param config - The theme configuration
* @param baseTheme - Optional base theme to apply under the config
*/
- static fromConfig(
+ // Merge a config over an optional base theme (arrays replace rather than
+ // deep-merge; a colorPrimary override without colorLink aligns colorLink).
+ private static mergeConfig(
config?: AnyThemeConfig,
baseTheme?: AnyThemeConfig,
- ): Theme {
+ ): AnyThemeConfig | undefined {
let mergedConfig: AnyThemeConfig | undefined = config;
if (baseTheme && config) {
@@ -76,9 +78,9 @@ export class Theme {
);
// In Ant Design v5, colorLink derives from colorInfo, not colorPrimary.
- // Currently we expectlinks to follow the brand/primary color. When the user
- // overrides colorPrimary without explicitly setting colorLink, update the
- // merged colorLink so links match the new primary palette.
+ // We expect links to follow the brand/primary color, so when a config
+ // overrides colorPrimary without setting colorLink, align the merged
+ // colorLink with the new primary palette.
if (config.token?.colorPrimary && !config.token?.colorLink) {
const mToken = mergedConfig?.token;
if (mToken) {
@@ -89,7 +91,14 @@ export class Theme {
mergedConfig = baseTheme;
}
- return new Theme({ config: mergedConfig });
+ return mergedConfig;
+ }
+
+ static fromConfig(
+ config?: AnyThemeConfig,
+ baseTheme?: AnyThemeConfig,
+ ): Theme {
+ return new Theme({ config: Theme.mergeConfig(config, baseTheme) });
}
private static getFilteredAntdTheme(
@@ -110,12 +119,14 @@ export class Theme {
}
/**
- * Update the theme using any theme configuration
- * Automatically handles both AntdThemeConfig and SerializableThemeConfig
- * Dark mode should be specified via the algorithm property in the config
+ * Update the theme using any theme configuration, optionally merged over a
+ * base theme. Automatically handles both AntdThemeConfig and
+ * SerializableThemeConfig. Dark mode should be specified via the algorithm
+ * property in the config.
*/
- setConfig(config: AnyThemeConfig): void {
- const antdConfig = normalizeThemeConfig(config);
+ setConfig(config: AnyThemeConfig, baseTheme?: AnyThemeConfig): void {
+ const mergedConfig = Theme.mergeConfig(config, baseTheme) ?? config;
+ const antdConfig = normalizeThemeConfig(mergedConfig);
if (antdConfig.token?.colorPrimary && !antdConfig.token?.colorLink) {
antdConfig.token.colorLink = antdConfig.token.colorPrimary;
@@ -124,11 +135,11 @@ export class Theme {
// First phase: Let Ant Design compute the tokens
const tokens = Theme.getFilteredAntdTheme(antdConfig);
- // Extract Superset-specific properties from top-level config.
- // These are custom properties that aren't part of Ant Design's token system
- // but need to be passed through to the SupersetTheme for ECharts customization.
+ // Extract Superset-specific properties from the merged config (not the raw
+ // config) so a base theme's ECharts overrides survive in-place updates, the
+ // same way the Ant Design tokens above are taken from the merged config.
const { echartsOptionsOverrides, echartsOptionsOverridesByChartType } =
- config as AnyThemeConfig & {
+ mergedConfig as AnyThemeConfig & {
echartsOptionsOverrides?: any;
echartsOptionsOverridesByChartType?: Record<string, any>;
};
diff --git a/superset-frontend/src/components/CrudThemeProvider.test.tsx b/superset-frontend/src/components/CrudThemeProvider.test.tsx
index 816ded2b1a6..f29046f0ec7 100644
--- a/superset-frontend/src/components/CrudThemeProvider.test.tsx
+++ b/superset-frontend/src/components/CrudThemeProvider.test.tsx
@@ -71,6 +71,7 @@ beforeEach(() => {
jest.restoreAllMocks();
jest.spyOn(Theme, 'fromConfig').mockReturnValue({
SupersetThemeProvider: MockSupersetThemeProvider,
+ setConfig: jest.fn(),
} as unknown as Theme);
mockNormalizeThemeConfig.mockImplementation(
config => config as ReturnType<typeof normalizeThemeConfig>,
@@ -334,6 +335,68 @@ test('skips the dashboard theme when an SDK theme config override is active', ()
).not.toBeInTheDocument();
});
+test('updates the existing Theme in place when the theme changes instead of recreating it', () => {
+ const setConfig = jest.fn();
+ (Theme.fromConfig as jest.Mock).mockReturnValue({
+ SupersetThemeProvider: MockSupersetThemeProvider,
+ setConfig,
+ });
+
+ const themeA = { token: { colorPrimary: '#aaaaaa' } };
+ const { rerender } = render(
+ <CrudThemeProvider
+ theme={{ id: 1, theme_name: 'A', json_data: JSON.stringify(themeA) }}
+ >
+ <div>Dashboard Content</div>
+ </CrudThemeProvider>,
+ );
+ expect(Theme.fromConfig).toHaveBeenCalledTimes(1);
+
+ const themeB = { token: { colorPrimary: '#bbbbbb' } };
+ rerender(
+ <CrudThemeProvider
+ theme={{ id: 2, theme_name: 'B', json_data: JSON.stringify(themeB) }}
+ >
+ <div>Dashboard Content</div>
+ </CrudThemeProvider>,
+ );
+
+ // The Theme instance is reused (not recreated) and updated in place, so the
+ // SupersetThemeProvider identity stays stable and the dashboard subtree is
+ // not remounted when the applied theme changes.
+ expect(Theme.fromConfig).toHaveBeenCalledTimes(1);
+ expect(setConfig).toHaveBeenCalledWith(themeB, expect.anything());
+});
+
+test('warns and keeps rendering when the in-place theme update throws', () => {
+ const setConfig = jest.fn(() => {
+ throw new Error('setConfig failed');
+ });
+ (Theme.fromConfig as jest.Mock).mockReturnValue({
+ SupersetThemeProvider: MockSupersetThemeProvider,
+ setConfig,
+ });
+ jest.spyOn(logging, 'warn').mockImplementation();
+
+ render(
+ <CrudThemeProvider
+ theme={{
+ id: 1,
+ theme_name: 'Custom Theme',
+ json_data: JSON.stringify({ token: { colorPrimary: '#ff0000' } }),
+ }}
+ >
+ <div>Dashboard Content</div>
+ </CrudThemeProvider>,
+ );
+
+ // The stable instance is created, then the in-place update via setConfig
+ // throws in the layout effect; the error is caught and children still render.
+ expect(setConfig).toHaveBeenCalled();
+ expect(logging.warn).toHaveBeenCalled();
+ expect(screen.getByText('Dashboard Content')).toBeInTheDocument();
+});
+
test('applies the dashboard theme when no SDK theme config override is active', () => {
const themeConfig = { token: { colorPrimary: '#ff0000' } };
render(
diff --git a/superset-frontend/src/components/CrudThemeProvider.tsx b/superset-frontend/src/components/CrudThemeProvider.tsx
index c44e72d7fee..52852d39ea8 100644
--- a/superset-frontend/src/components/CrudThemeProvider.tsx
+++ b/superset-frontend/src/components/CrudThemeProvider.tsx
@@ -16,7 +16,14 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ReactNode, useContext, useEffect, useMemo } from 'react';
+import {
+ ReactNode,
+ useContext,
+ useEffect,
+ useLayoutEffect,
+ useMemo,
+ useRef,
+} from 'react';
import { logging } from '@apache-superset/core/utils';
import {
Theme,
@@ -33,28 +40,24 @@ interface CrudThemeProviderProps {
}
/**
- * CrudThemeProvider applies a dashboard-specific theme using theme data
- * from the dashboard API response. Merges with the system's base theme
- * (light or dark) and loads custom fonts. Falls back to the global theme
- * if the theme data is missing or invalid.
+ * Applies a dashboard-specific theme from the dashboard API response, merged
+ * over the system's base theme (light or dark), and loads custom fonts. Falls
+ * back to the global theme when the theme data is missing or invalid.
+ *
+ * A single, stable Theme instance is updated in place instead of recreated, so
+ * the SupersetThemeProvider identity stays constant and the dashboard subtree
+ * is not remounted when the applied theme changes.
*/
export default function CrudThemeProvider({
children,
theme,
}: CrudThemeProviderProps) {
- // An explicit theme config override (e.g. supplied via the Embedded SDK)
- // applies on the global theme controller and must win over the
- // dashboard-level theme. When such an override is active, skip the
- // dashboard theme so the override is not shadowed by this nested provider.
const themeContext = useContext(ThemeContext);
const hasThemeConfigOverride = themeContext?.hasThemeConfigOverride ?? false;
- const { dashboardTheme, fontUrls } = useMemo(() => {
- // When an SDK override is active it fully owns theming, so skip parsing the
- // dashboard theme entirely. This also prevents the font-injection effect
- // below from loading dashboard fonts the override does not use.
+ const parsedTheme = useMemo(() => {
if (hasThemeConfigOverride || !theme?.json_data) {
- return { dashboardTheme: null, fontUrls: undefined };
+ return null;
}
try {
const themeConfig = JSON.parse(theme.json_data);
@@ -64,26 +67,53 @@ export default function CrudThemeProvider({
common: { theme: bootstrapTheme },
} = getBootstrapData();
const baseTheme = isDark ? bootstrapTheme.dark : bootstrapTheme.default;
- const createdTheme = Theme.fromConfig(
- normalizedConfig,
- baseTheme || undefined,
- );
const rawUrls = themeConfig?.token?.fontUrls;
- const urls = Array.isArray(rawUrls) ? (rawUrls as string[]) : undefined;
- return { dashboardTheme: createdTheme, fontUrls: urls };
+ const fontUrls = Array.isArray(rawUrls)
+ ? (rawUrls as string[])
+ : undefined;
+ return { normalizedConfig, baseTheme: baseTheme || undefined, fontUrls };
} catch (error) {
logging.warn('Failed to load dashboard theme:', error);
- return { dashboardTheme: null, fontUrls: undefined };
+ return null;
}
}, [theme?.json_data, hasThemeConfigOverride]);
+ // Create the stable instance once; update it in place on later changes.
+ const dashboardThemeRef = useRef<Theme | null>(null);
+ if (parsedTheme && !dashboardThemeRef.current) {
+ try {
+ dashboardThemeRef.current = Theme.fromConfig(
+ parsedTheme.normalizedConfig,
+ parsedTheme.baseTheme,
+ );
+ } catch (error) {
+ logging.warn('Failed to load dashboard theme:', error);
+ }
+ }
+
+ useLayoutEffect(() => {
+ if (parsedTheme && dashboardThemeRef.current) {
+ try {
+ dashboardThemeRef.current.setConfig(
+ parsedTheme.normalizedConfig,
+ parsedTheme.baseTheme,
+ );
+ } catch (error) {
+ logging.warn('Failed to load dashboard theme:', error);
+ }
+ }
+ }, [parsedTheme]);
+
useEffect(() => {
- if (hasThemeConfigOverride || !dashboardTheme || !fontUrls?.length) {
+ if (
+ !parsedTheme ||
+ !dashboardThemeRef.current ||
+ !parsedTheme.fontUrls?.length
+ ) {
return undefined;
}
-
- // JSON.stringify provides safe escaping to prevent CSS injection
- const css = fontUrls
+ // JSON.stringify escapes the URL to prevent CSS injection.
+ const css = parsedTheme.fontUrls
.map((url: string) => `@import url(${JSON.stringify(url)});`)
.join('\n');
const style = document.createElement('style');
@@ -94,15 +124,13 @@ export default function CrudThemeProvider({
return () => {
style.remove();
};
- }, [dashboardTheme, fontUrls, hasThemeConfigOverride]);
+ }, [parsedTheme]);
- if (!dashboardTheme || hasThemeConfigOverride) {
+ if (!parsedTheme || !dashboardThemeRef.current) {
return <>{children}</>;
}
- return (
- <dashboardTheme.SupersetThemeProvider>
- {children}
- </dashboardTheme.SupersetThemeProvider>
- );
+ const DashboardThemeProvider =
+ dashboardThemeRef.current.SupersetThemeProvider;
+ return <DashboardThemeProvider>{children}</DashboardThemeProvider>;
}
diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.test.ts b/superset-frontend/src/dashboard/reducers/dashboardState.test.ts
index dc57c4a7d15..7ceb1d66824 100644
--- a/superset-frontend/src/dashboard/reducers/dashboardState.test.ts
+++ b/superset-frontend/src/dashboard/reducers/dashboardState.test.ts
@@ -28,6 +28,7 @@ import {
SET_EDIT_MODE,
SET_FOCUSED_FILTER_FIELD,
SET_MAX_UNDO_HISTORY_EXCEEDED,
+ SET_REFRESH_FREQUENCY,
SET_UNSAVED_CHANGES,
TOGGLE_EXPAND_SLICE,
TOGGLE_FAVE_STAR,
@@ -391,6 +392,54 @@ describe('DashboardState reducer', () => {
});
});
+ test('SET_REFRESH_FREQUENCY never clears existing unsaved changes', () => {
+ // A persistent update marks the dashboard dirty.
+ expect(
+ typedDashboardStateReducer({} as Partial<DashboardState>, {
+ type: SET_REFRESH_FREQUENCY,
+ refreshFrequency: 30,
+ isPersistent: true,
+ }),
+ ).toEqual(
+ expect.objectContaining({
+ refreshFrequency: 30,
+ shouldPersistRefreshFrequency: true,
+ hasUnsavedChanges: true,
+ }),
+ );
+
+ // A non-persistent update must preserve, not clear, pending unsaved changes
+ // (e.g. the dashboard header's unmount cleanup during a remount).
+ expect(
+ typedDashboardStateReducer(
+ { hasUnsavedChanges: true } as Partial<DashboardState>,
+ {
+ type: SET_REFRESH_FREQUENCY,
+ refreshFrequency: 0,
+ isPersistent: false,
+ },
+ ),
+ ).toEqual(
+ expect.objectContaining({
+ refreshFrequency: 0,
+ shouldPersistRefreshFrequency: false,
+ hasUnsavedChanges: true,
+ }),
+ );
+
+ // With no pending changes, a non-persistent update leaves the flag false.
+ expect(
+ typedDashboardStateReducer(
+ { hasUnsavedChanges: false } as Partial<DashboardState>,
+ {
+ type: SET_REFRESH_FREQUENCY,
+ refreshFrequency: 0,
+ isPersistent: false,
+ },
+ ).hasUnsavedChanges,
+ ).toBe(false);
+ });
+
test('should set maxUndoHistoryExceeded', () => {
expect(
typedDashboardStateReducer({} as Partial<DashboardState>, {
diff --git a/superset-frontend/src/dashboard/reducers/dashboardState.ts b/superset-frontend/src/dashboard/reducers/dashboardState.ts
index 5a25aac94d9..4324f96a389 100644
--- a/superset-frontend/src/dashboard/reducers/dashboardState.ts
+++ b/superset-frontend/src/dashboard/reducers/dashboardState.ts
@@ -305,7 +305,7 @@ export default function dashboardStateReducer(
...state,
refreshFrequency: action.refreshFrequency,
shouldPersistRefreshFrequency: action.isPersistent,
- hasUnsavedChanges: action.isPersistent,
+ hasUnsavedChanges: action.isPersistent || state.hasUnsavedChanges,
};
},
[ON_REFRESH](): DashboardStateShape {