sadpandajoe commented on code in PR #37378: URL: https://github.com/apache/superset/pull/37378#discussion_r2775439605
########## superset-frontend/src/theme/utils/themeStructureValidation.ts: ########## @@ -0,0 +1,116 @@ +/** + * 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) + // Guard against non-object token values (e.g., string, array, number) + const rawToken = themeConfig.token; + const tokens = + rawToken && typeof rawToken === 'object' && !Array.isArray(rawToken) + ? rawToken + : {}; + + if (rawToken && tokens !== rawToken) { + errors.push({ + tokenName: '_root', + severity: 'error', + message: + 'Token configuration must be an object, not an array or primitive', + }); + return { valid: false, errors, warnings }; + } + + Object.entries(tokens).forEach(([name, value]) => { Review Comment: This guard **already exists**. See lines 102-118: ```typescript const rawComponents = themeConfig.components; if (rawComponents \!== undefined) { if (rawComponents === null || typeof rawComponents \!== 'object' || Array.isArray(rawComponents)) { errors.push({ tokenName: '_root', severity: 'error', message: 'Components configuration must be an object, not null, array, or primitive', }); return { valid: false, errors, warnings }; } } ``` ########## superset-frontend/src/theme/ThemeController.ts: ########## @@ -535,7 +546,7 @@ export class ThemeController { * Updates the theme. * @param theme - The new theme to apply */ - private updateTheme(theme?: AnyThemeConfig): void { + private async updateTheme(theme?: AnyThemeConfig): Promise<void> { Review Comment: This is **intentional fire-and-forget** pattern. The public API methods (`setTheme`, `setThemeMode`, etc.) are synchronous for simpler consumer usage. Theme application happens asynchronously in the background. Making callers await would require changing the entire public API surface to be async, which would be a breaking change. The current pattern: 1. Consumer calls `controller.setTheme(theme)` (sync) 2. Theme update happens in background (async) 3. Consumers get notified via `onChange` callback when complete Errors are handled internally with fallback logic. ########## superset-frontend/src/theme/ThemeController.ts: ########## @@ -551,18 +562,32 @@ export class ThemeController { this.persistMode(); this.notifyListeners(); } catch (error) { - console.error('Failed to update theme:', error); - this.fallbackToDefaultMode(); + await this.fallbackToDefaultMode(); } } /** - * Fallback to default mode with error recovery. + * Fallback to default mode with runtime error recovery. + * Tries to fetch a fresh system default theme from the API. */ - private fallbackToDefaultMode(): void { + private async fallbackToDefaultMode(): Promise<void> { Review Comment: Same as above - this is intentional fire-and-forget. The `setThemeMode` method is synchronous for ergonomic API design. The fallback happens asynchronously and errors are handled internally. ########## superset-frontend/src/theme/utils/themeStructureValidation.ts: ########## @@ -0,0 +1,150 @@ +/** + * 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) + // Guard against non-object token values (e.g., string, array, number) + const rawToken = themeConfig.token; + const tokens = + rawToken && typeof rawToken === 'object' && !Array.isArray(rawToken) + ? rawToken + : {}; + + if (rawToken && tokens !== rawToken) { Review Comment: The check properly handles falsy non-object values. 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 }; } } ``` If `rawToken` is `0`, `''`, or `false`: - It's not `undefined`, so we enter the `if` block - `typeof rawToken \!== 'object'` is `true` (since `typeof 0 === 'number'`, etc.) - So we push an error and return early The truthy check at line 100 (`rawToken ?? {}`) only runs AFTER the type guard has validated that `rawToken` is either `undefined` or a valid object. -- 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]
