sadpandajoe commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2775435836
##########
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:
Thanks for the review! After investigation, this is **not a recursion risk**:
- `applyThemeWithRecovery` does NOT call `fallbackToDefaultMode` - it
re-throws errors to the caller
- Lines 839-842 explicitly document this: *"This method re-throws errors to
the caller instead of calling fallbackToDefaultMode directly, to avoid infinite
recursion"*
The call flow is: `fallbackToDefaultMode` → `applyThemeWithRecovery` →
(error) → re-throw → back to `fallbackToDefaultMode`'s catch block → falls
through to final cached default theme.
##########
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:
This guard already exists. See lines 84-99:
```typescript
const rawToken = themeConfig.token;
if (rawToken \!== undefined) {
if (rawToken === null || typeof rawToken \!== 'object' ||
Array.isArray(rawToken)) {
errors.push({...});
return { valid: false, errors, warnings };
}
}
```
Non-object token values (including arrays) are properly rejected before
`Object.entries` is called.
##########
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:
This is **intentional design**. See the comment at lines 985-988:
> *"Uses raw fetch() instead of SupersetClient because ThemeController
initializes early in the app lifecycle, before SupersetClient is fully
configured. This avoids boot-time circular dependencies."*
Using SupersetClient here would create initialization order issues.
##########
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:
Same as the other recursion comment - `applyThemeWithRecovery` re-throws
errors instead of calling `fallbackToDefaultMode`, which is explicitly
documented in the code comments (lines 839-842). No recursion risk.
##########
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:
Font loading **is already present** in `applyThemeWithRecovery`. See lines
846-849:
```typescript
// Load custom fonts if specified, mirroring applyTheme() behavior
const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
?.fontUrls as string[] | undefined;
this.loadFonts(fontUrls);
```
--
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]
