Copilot commented on code in PR #38384:
URL: https://github.com/apache/superset/pull/38384#discussion_r2882201774
##########
superset-frontend/src/components/CrudThemeProvider.tsx:
##########
@@ -16,68 +16,45 @@
* specific language governing permissions and limitations
* under the License.
*/
-import { ReactNode, useEffect, useState } from 'react';
-import { useThemeContext } from 'src/theme/ThemeProvider';
+import { ReactNode, useMemo } from 'react';
+import { logging } from '@apache-superset/core';
import { Theme } from '@apache-superset/core/ui';
-import { Loading } from '@superset-ui/core/components';
+import type { Dashboard } from 'src/types/Dashboard';
interface CrudThemeProviderProps {
children: ReactNode;
- themeId?: number | null;
+ theme?: Dashboard['theme'];
}
/**
- * CrudThemeProvider asks the ThemeController for a dashboard theme provider.
- * Flow: Dashboard loads → asks controller → controller fetches theme →
- * returns provider → dashboard uses it.
- *
- * CRITICAL: This does NOT modify the global controller - it creates an
isolated dashboard theme.
+ * CrudThemeProvider applies a dashboard-specific theme using theme data
+ * from the dashboard API response. Falls back to the global theme if
+ * the theme data is missing or invalid.
*/
export default function CrudThemeProvider({
children,
- themeId,
+ theme,
}: CrudThemeProviderProps) {
- const globalThemeContext = useThemeContext();
- const [dashboardTheme, setDashboardTheme] = useState<Theme | null>(null);
-
- useEffect(() => {
- if (themeId) {
- // Ask the controller to create a SEPARATE dashboard theme provider
- // This should NOT affect the global controller or navbar
- const loadDashboardTheme = async () => {
- try {
- const dashboardThemeProvider =
- await globalThemeContext.createDashboardThemeProvider(
- String(themeId),
- );
- setDashboardTheme(dashboardThemeProvider);
- } catch (error) {
- console.error('Failed to load dashboard theme:', error);
- setDashboardTheme(null);
- }
- };
-
- loadDashboardTheme();
- } else {
- setDashboardTheme(null);
+ const dashboardTheme = useMemo(() => {
+ if (!theme?.json_data) {
+ return null;
}
- }, [themeId, globalThemeContext]);
-
- // If no themeId, just render children (they use global theme)
- if (!themeId) {
- return <>{children}</>;
- }
+ try {
+ const themeConfig = JSON.parse(theme.json_data);
+ return Theme.fromConfig(themeConfig);
+ } catch (error) {
+ logging.warn('Failed to load dashboard theme:', error);
+ return null;
+ }
Review Comment:
By bypassing `ThemeController.createDashboardThemeProvider`, dashboard
themes no longer (1) merge with the configured base theme (THEME_DEFAULT /
THEME_DARK) and (2) load `token.fontUrls` via `ThemeController.loadFonts()`. As
a result, dashboard themes that rely on instance defaults or custom fonts may
render incorrectly. Consider reusing ThemeController logic via a new API like
`createDashboardThemeProviderFromConfig(themeConfig)` (no fetch) or otherwise
replicating the base-theme merge + font loading for inline `json_data`.
##########
superset-frontend/src/dashboard/containers/DashboardPage.tsx:
##########
@@ -294,13 +291,7 @@ export const DashboardPage: FC<PageProps> = ({ idOrSlug }:
PageProps) => {
<>
<SyncDashboardState dashboardPageId={dashboardPageId} />
<DashboardPageIdContext.Provider value={dashboardPageId}>
- <CrudThemeProvider
- themeId={
- dashboardTheme !== undefined
- ? dashboardTheme?.id
- : dashboard?.theme?.id
- }
- >
+ <CrudThemeProvider theme={dashboard?.theme}>
Review Comment:
This change removes `dashboardInfo.theme` as an input to
`CrudThemeProvider`, so theme changes made in the Dashboard Properties modal
during the same session will no longer be reflected (this previously worked by
driving `themeId` from Redux state for users who could read Theme). Consider
keeping `dashboardInfo.theme` as a higher-priority source, or supporting an
admin-only fallback path (e.g., fetch by id when inline `json_data` is missing)
so live theme switching doesn’t regress for authorized users.
##########
superset-frontend/src/dashboard/containers/DashboardPage.test.tsx:
##########
@@ -0,0 +1,179 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import type { ReactNode } from 'react';
+import { Suspense } from 'react';
+import { render, screen, waitFor } from 'spec/helpers/testing-library';
+import {
+ useDashboard,
+ useDashboardCharts,
+ useDashboardDatasets,
+} from 'src/hooks/apiResources';
+import CrudThemeProvider from 'src/components/CrudThemeProvider';
+import DashboardPage from './DashboardPage';
+
+const mockTheme = {
+ id: 42,
+ theme_name: 'Branded',
+ json_data: '{"token":{"colorPrimary":"#1677ff"}}',
+};
+
+const mockDashboard = {
+ id: 1,
+ slug: null,
+ url: '/superset/dashboard/1/',
+ dashboard_title: 'Test Dashboard',
+ thumbnail_url: null,
+ published: true,
+ css: null,
+ json_metadata: '{}',
+ position_json: '{}',
+ changed_by_name: 'admin',
+ changed_by: { id: 1, first_name: 'Admin', last_name: 'User' },
+ changed_on: '2024-01-01T00:00:00',
+ charts: [],
+ owners: [],
+ roles: [],
+ theme: mockTheme,
+};
+
+jest.mock('src/hooks/apiResources', () => ({
+ useDashboard: jest.fn(),
+ useDashboardCharts: jest.fn(),
+ useDashboardDatasets: jest.fn(),
+}));
+
+jest.mock('src/dashboard/actions/hydrate', () => ({
+ ...jest.requireActual('src/dashboard/actions/hydrate'),
+ hydrateDashboard: jest.fn(() => ({ type: 'MOCK_HYDRATE' })),
+}));
+
+jest.mock('src/dashboard/actions/datasources', () => ({
+ ...jest.requireActual('src/dashboard/actions/datasources'),
+ setDatasources: jest.fn(() => ({ type: 'MOCK_SET_DATASOURCES' })),
+}));
+
+jest.mock('src/dashboard/actions/dashboardState', () => ({
+ ...jest.requireActual('src/dashboard/actions/dashboardState'),
+ setDatasetsStatus: jest.fn(() => ({ type: 'MOCK_SET_DATASETS_STATUS' })),
+}));
+
+jest.mock('src/components/CrudThemeProvider', () => ({
+ __esModule: true,
+ default: jest.fn(({ children }: { children: ReactNode }) => (
+ <div>{children}</div>
+ )),
+}));
+
+jest.mock('src/dashboard/components/DashboardBuilder/DashboardBuilder', () =>
({
+ __esModule: true,
+ default: () => <div data-testid="dashboard-builder">DashboardBuilder</div>,
+}));
+
+jest.mock('src/dashboard/components/SyncDashboardState', () => ({
+ __esModule: true,
+ default: () => null,
+ getDashboardContextLocalStorage: () => ({}),
+}));
+
+jest.mock('src/dashboard/containers/Dashboard', () => ({
+ __esModule: true,
+ default: ({ children }: { children: ReactNode }) => <div>{children}</div>,
+}));
+
+jest.mock('src/components/MessageToasts/withToasts', () => ({
+ useToasts: () => ({ addDangerToast: jest.fn(), addSuccessToast: jest.fn() }),
+}));
+
+jest.mock('src/dashboard/util/injectCustomCss', () => ({
+ __esModule: true,
+ default: () => () => {},
+}));
+
+jest.mock('src/dashboard/util/activeAllDashboardFilters', () => ({
+ getAllActiveFilters: () => ({}),
+ getRelevantDataMask: () => ({}),
+}));
+
+jest.mock('src/dashboard/util/activeDashboardFilters', () => ({
+ getActiveFilters: () => ({}),
+}));
+
+jest.mock('src/utils/urlUtils', () => ({
+ getUrlParam: () => null,
+}));
+
+jest.mock('src/dashboard/components/nativeFilters/FilterBar/keyValue', () => ({
+ getFilterValue: jest.fn(),
+ getPermalinkValue: jest.fn(),
+}));
+
+jest.mock('src/dashboard/contexts/AutoRefreshContext', () => ({
+ AutoRefreshProvider: ({ children }: { children: ReactNode }) => (
+ <>{children}</>
+ ),
+}));
+
+const mockUseDashboard = useDashboard as jest.Mock;
+const mockUseDashboardCharts = useDashboardCharts as jest.Mock;
+const mockUseDashboardDatasets = useDashboardDatasets as jest.Mock;
+const MockCrudThemeProvider = CrudThemeProvider as unknown as jest.Mock;
+
+beforeEach(() => {
+ jest.clearAllMocks();
+ mockUseDashboard.mockReturnValue({
+ result: mockDashboard,
+ error: null,
+ });
+ mockUseDashboardCharts.mockReturnValue({
+ result: [],
+ error: null,
+ });
+ mockUseDashboardDatasets.mockReturnValue({
+ result: [],
+ error: null,
+ status: 'complete',
+ });
+});
+
+test('passes full theme object from dashboard API response to
CrudThemeProvider', async () => {
+ render(
+ <Suspense fallback="loading">
+ <DashboardPage idOrSlug="1" />
+ </Suspense>,
+ {
+ useRedux: true,
+ useRouter: true,
+ initialState: {
+ dashboardInfo: { id: 1, metadata: {} },
+ dashboardState: { sliceIds: [] },
+ nativeFilters: { filters: {} },
+ dataMask: {},
+ },
+ },
+ );
+
+ await waitFor(() => {
+ expect(screen.queryByText('loading')).not.toBeInTheDocument();
+ });
+
+ expect(MockCrudThemeProvider).toHaveBeenCalledWith(
+ expect.objectContaining({ theme: mockTheme }),
+ expect.anything(),
Review Comment:
`CrudThemeProvider` is a mocked function component, so React will call it
with a single `props` argument. The `toHaveBeenCalledWith(...,
expect.anything())` assertion will fail because there is no second argument.
Update the expectation to only assert on the props object (or use
`toHaveBeenCalled()` + inspect `mock.calls[0][0]`).
```suggestion
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
