Copilot commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2723771149
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -896,25 +913,77 @@ export class ThemeController {
/**
* Fetches a theme configuration from the CRUD API.
* @param themeId - The ID of the theme to fetch
- * @returns The theme configuration or null if not found
+ * @returns The theme configuration or null if fetch fails
*/
private async fetchCrudTheme(
themeId: string,
): Promise<AnyThemeConfig | null> {
try {
// Use SupersetClient for proper authentication handling
- const getTheme = makeApi<void, { result: { json_data: string } }>({
+ const getTheme = makeApi<
+ void,
+ { result: { json_data: string; theme_name?: string } }
+ >({
method: 'GET',
endpoint: `/api/v1/theme/${themeId}`,
});
const { result } = await getTheme();
const themeConfig = JSON.parse(result.json_data);
+ if (!themeConfig || typeof themeConfig !== 'object') {
+ console.error(`Invalid theme configuration for theme ${themeId}`);
+ return null;
+ }
+
+ // Return theme as-is
+ // Invalid tokens will be handled by Ant Design at runtime
+ // Runtime errors will be caught by applyThemeWithRecovery()
return themeConfig;
} catch (error) {
console.error('Failed to fetch CRUD theme:', error);
return null;
}
}
+
+ /**
+ * Fetches a fresh system default theme from the API for runtime recovery.
+ * Tries multiple fallback strategies to find a valid theme.
+ * @returns The system default theme configuration or null if not found
+ */
+ private async fetchSystemDefaultTheme(): Promise<AnyThemeConfig | null> {
+ try {
+ // Try to fetch theme marked as system default (is_system_default=true)
+ const defaultResponse = await fetch(
+
'/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))',
+ );
+ if (defaultResponse.ok) {
+ const data = await defaultResponse.json();
+ if (data.result?.length > 0) {
+ const themeConfig = JSON.parse(data.result[0].json_data);
+ if (themeConfig && typeof themeConfig === 'object') {
+ return themeConfig;
+ }
+ }
+ }
+
+ // Fallback: Try to fetch system theme named 'THEME_DEFAULT'
+ const fallbackResponse = await fetch(
+
'/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))',
+ );
+ if (fallbackResponse.ok) {
+ const fallbackData = await fallbackResponse.json();
Review Comment:
`fetchSystemDefaultTheme` uses the global `fetch` API directly against
`/api/v1/theme/` while other theme-related frontend calls (for example
`superset-frontend/src/pages/ThemeList/index.tsx:152-195` and `fetchCrudTheme`
in this file) consistently use `SupersetClient`/`makeApi`. For consistency with
the rest of the codebase and to centralize authentication, error handling, and
base URL concerns, consider refactoring this method to use
`SupersetClient`/`makeApi` as well.
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -796,7 +803,17 @@ export class ThemeController {
this.loadFonts(fontUrls);
} catch (error) {
console.error('Failed to apply theme:', error);
- this.fallbackToDefaultMode();
+ // Re-throw the error so updateTheme can handle fallback logic
+ throw error;
+ }
+ }
+
+ private async applyThemeWithRecovery(theme: AnyThemeConfig): Promise<void> {
+ try {
+ const normalizedConfig = normalizeThemeConfig(theme);
+ this.globalTheme.setConfig(normalizedConfig);
Review Comment:
`applyTheme` loads custom fonts via `loadFonts` but `applyThemeWithRecovery`
does not, so when `fallbackToDefaultMode` applies a freshly fetched system
default theme through `applyThemeWithRecovery`, any `fontUrls` in that theme
will be ignored and the fonts will not be loaded. This makes the recovery path
behave differently from normal theme application and may result in inconsistent
typography after a fallback. To keep behavior consistent, consider either
reusing `applyTheme` in the recovery path or adding the same font-loading logic
to `applyThemeWithRecovery`.
```suggestion
this.globalTheme.setConfig(normalizedConfig);
// Load custom fonts if specified in theme config, mirroring
applyTheme()
const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
?.fontUrls as string[] | undefined;
this.loadFonts(fontUrls);
```
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -796,7 +803,17 @@ export class ThemeController {
this.loadFonts(fontUrls);
} catch (error) {
console.error('Failed to apply theme:', error);
- this.fallbackToDefaultMode();
+ // Re-throw the error so updateTheme can handle fallback logic
+ throw error;
+ }
+ }
+
+ private async applyThemeWithRecovery(theme: AnyThemeConfig): Promise<void> {
+ try {
+ const normalizedConfig = normalizeThemeConfig(theme);
+ this.globalTheme.setConfig(normalizedConfig);
+ } catch (error) {
+ await this.fallbackToDefaultMode();
}
Review Comment:
`applyThemeWithRecovery` calls `fallbackToDefaultMode` in its catch block,
while `fallbackToDefaultMode` itself calls `applyThemeWithRecovery` when a
fresh system default theme is found. This mutual dependency can lead to
unbounded recursion if the fetched system theme consistently throws during
normalization or `setConfig`, causing repeated network calls and potential
stack overflows instead of falling back to the built-in default theme. Consider
restructuring the error paths so that the recovery logic cannot re-enter
`fallbackToDefaultMode` recursively (for example by letting
`applyThemeWithRecovery` rethrow here, or by having the fallback path use
`applyTheme` and the cached/built-in default theme instead of calling
`fallbackToDefaultMode` again).
##########
superset-frontend/src/theme/utils/themeStructureValidation.ts:
##########
@@ -0,0 +1,100 @@
+/**
+ * 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 { AnyThemeConfig } from '@apache-superset/core/ui';
+import { isValidTokenName } from './antdTokenNames';
+
+export interface ValidationIssue {
+ tokenName: string;
+ severity: 'error' | 'warning';
+ message: string;
+}
+
+export interface ValidationResult {
+ valid: boolean; // false if ANY errors exist (warnings don't affect this)
+ errors: ValidationIssue[];
+ warnings: ValidationIssue[];
+}
+
+/**
+ * Validates theme structure and token names.
+ * - ERRORS block save/apply (invalid structure, empty themes)
+ * - WARNINGS allow save/apply but show in editor (unknown tokens, null values)
+ *
+ * This validation does NOT check token values - Ant Design handles that at
runtime.
+ */
+export function validateTheme(themeConfig: AnyThemeConfig): ValidationResult {
+ const errors: ValidationIssue[] = [];
+ const warnings: ValidationIssue[] = [];
+
+ // ERROR: Null/invalid config
+ if (!themeConfig || typeof themeConfig !== 'object') {
+ errors.push({
+ tokenName: '_root',
+ severity: 'error',
+ message: 'Theme configuration must be a valid object',
+ });
+ return { valid: false, errors, warnings };
+ }
+
+ // ERROR: Empty theme (no tokens, no algorithm, no components)
+ const hasTokens =
+ themeConfig.token && Object.keys(themeConfig.token).length > 0;
+ const hasAlgorithm = Boolean(themeConfig.algorithm);
+ const hasComponents =
+ themeConfig.components && Object.keys(themeConfig.components).length > 0;
+
+ if (!hasTokens && !hasAlgorithm && !hasComponents) {
+ errors.push({
+ tokenName: '_root',
+ severity: 'error',
+ message:
+ 'Theme cannot be empty. Add at least one token, algorithm, or
component override.',
+ });
+ return { valid: false, errors, warnings };
+ }
+
+ // WARNING: Unknown token names (likely typos)
+ const tokens = themeConfig.token || {};
+ Object.entries(tokens).forEach(([name, value]) => {
+ // Null/undefined check
Review Comment:
`validateTheme` assumes `themeConfig.token` is an object and passes it
directly to `Object.entries`; if a user-supplied theme JSON contains a
non-object `token` value (e.g., string or array), this will throw at runtime.
Because `useThemeValidation` wraps `validateTheme` in a generic try/catch and
returns no annotations on any exception, such invalid themes would silently
bypass structural validation instead of surfacing a clear error. It would be
safer to guard the type of `themeConfig.token` and treat non-object values as a
structural error on `_root` before iterating.
--
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]
