codeant-ai-for-open-source[bot] commented on code in PR #37378: URL: https://github.com/apache/superset/pull/37378#discussion_r2766283361
########## 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) { + errors.push({ + tokenName: '_root', + severity: 'error', + message: + 'Token configuration must be an object, not an array or primitive', + }); + return { valid: false, errors, warnings }; + } + + // ERROR: components must be an object if present + const rawComponents = themeConfig.components; + if ( + rawComponents !== undefined && + rawComponents !== null && + (typeof rawComponents !== 'object' || Array.isArray(rawComponents)) + ) { + errors.push({ + tokenName: '_root', + severity: 'error', + message: + 'Components configuration must be an object, not an array or primitive', + }); + return { valid: false, errors, warnings }; + } + + // ERROR: algorithm must be a string or array of strings if present + const rawAlgorithm = themeConfig.algorithm; + if (rawAlgorithm !== undefined && rawAlgorithm !== null) { + const isValidAlgorithm = + typeof rawAlgorithm === 'string' || + (Array.isArray(rawAlgorithm) && + rawAlgorithm.every(a => typeof a === 'string')); + if (!isValidAlgorithm) { + errors.push({ + tokenName: '_root', + severity: 'error', + message: + 'Algorithm must be a string or array of strings (e.g., "dark" or ["dark", "compact"])', Review Comment: **Suggestion:** The algorithm type check only accepts strings or arrays of strings, but Ant Design's theme config typically uses algorithm functions (or arrays of functions), so valid themes that supply standard Ant Design algorithms (e.g., `theme.darkAlgorithm`) will be incorrectly rejected as invalid. To avoid falsely failing these valid configs, broaden the validation to also accept functions and arrays of functions as valid algorithm types while keeping string support for name-based algorithms. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ Valid algorithm functions rejected by validateTheme. - ⚠️ Theme application UI may block valid themes. - ⚠️ Dark/light algorithm function usage prevented. ``` </details> ```suggestion typeof rawAlgorithm === 'function' || (Array.isArray(rawAlgorithm) && rawAlgorithm.every( a => typeof a === 'string' || typeof a === 'function', )); if (!isValidAlgorithm) { errors.push({ tokenName: '_root', severity: 'error', message: 'Algorithm must be a string, function, or an array of strings/functions (e.g., "dark" or ["dark", "compact"])', ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Import and call the validator: from a unit test or console import validateTheme from superset-frontend/src/theme/utils/themeStructureValidation.ts (function defined at line 41) and call it with a theme that sets algorithm to a function, for example validateTheme({ algorithm: () => {} }). 2. Execution reaches the algorithm validation block at lines 107-122, where isValidAlgorithm only accepts strings or arrays of strings (lines 109-112). 3. The provided function value does not match the allowed types, so the validator pushes an error at lines 113-121 and returns valid: false. 4. This reproduces an incorrect rejection of a valid Ant Design algorithm function, blocking theme acceptance. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/theme/utils/themeStructureValidation.ts **Line:** 111:118 **Comment:** *Possible Bug: The algorithm type check only accepts strings or arrays of strings, but Ant Design's theme config typically uses algorithm functions (or arrays of functions), so valid themes that supply standard Ant Design algorithms (e.g., `theme.darkAlgorithm`) will be incorrectly rejected as invalid. To avoid falsely failing these valid configs, broaden the validation to also accept functions and arrays of functions as valid algorithm types while keeping string support for name-based algorithms. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## 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: **Suggestion:** The `rawToken` validation relies on truthiness, so primitive falsy values like `0`, `''`, or `false` are treated as "no token config" and skip the structural error, meaning a theme with an invalid primitive `token` field can be considered valid as long as `algorithm` or `components` are present. This leads to structurally invalid themes passing validation and failing later when Ant Design expects `token` to be an object. To fix this, check explicitly for `undefined`/`null` instead of using a truthy check when deciding whether to flag a non-object `token` as an error. [logic error] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ validateTheme accepts primitive token values incorrectly. - ⚠️ Theme editor may save invalid token primitives. - ⚠️ Later Ant Design token usage may fail at runtime. ``` </details> ```suggestion if (rawToken !== undefined && rawToken !== null && tokens !== rawToken) { ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Import and call the validator: from a unit test or console import validateTheme from superset-frontend/src/theme/utils/themeStructureValidation.ts (function defined at line 41) and call it with a theme object that uses a primitive falsy token value, e.g. validateTheme({ token: 0, algorithm: 'dark' }). 2. Execution reaches the rawToken/tokens logic at lines 74-78 and assigns tokens = {} because rawToken (0) is falsy (see lines 74-78). 3. The conditional at lines 80-88 (if (rawToken && tokens !== rawToken)) is skipped because rawToken is falsy, so no structural error is pushed. 4. The function returns valid (final return at lines 145-149 computes valid from errors.length), so a structurally invalid theme (primitive token) is accepted; this reproduces the incorrect acceptance of a primitive falsy token. ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/theme/utils/themeStructureValidation.ts **Line:** 80:80 **Comment:** *Logic Error: The `rawToken` validation relies on truthiness, so primitive falsy values like `0`, `''`, or `false` are treated as "no token config" and skip the structural error, meaning a theme with an invalid primitive `token` field can be considered valid as long as `algorithm` or `components` are present. This leads to structurally invalid themes passing validation and failing later when Ant Design expects `token` to be an object. To fix this, check explicitly for `undefined`/`null` instead of using a truthy check when deciding whether to flag a non-object `token` as an error. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> ########## superset-frontend/src/theme/ThemeController.ts: ########## @@ -796,10 +830,25 @@ 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> { + // Note: This method re-throws errors to the caller instead of calling + // fallbackToDefaultMode directly, to avoid infinite recursion since + // fallbackToDefaultMode calls this method. The caller's try/catch + // handles the fallback flow. + const normalizedConfig = normalizeThemeConfig(theme); + this.globalTheme.setConfig(normalizedConfig); + + // Load custom fonts if specified, mirroring applyTheme() behavior Review Comment: **Suggestion:** The implementations of `applyTheme` and `applyThemeWithRecovery` duplicate the same normalization, config application, and font-loading logic, increasing the risk that future changes will update one path but not the other and cause inconsistent behavior between normal and recovery theme application. [possible bug] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Recovery theme path may behave differently than normal path. - ⚠️ Inconsistent font loading between apply paths. - ⚠️ Harder debugging due to duplicated logic. ``` </details> ```suggestion this.applyThemeInternal(theme); } catch (error) { console.error('Failed to apply theme:', error); // Re-throw the error so updateTheme can handle fallback logic throw error; } } private applyThemeWithRecovery(theme: AnyThemeConfig): void { // Note: This method re-throws errors to the caller instead of calling // fallbackToDefaultMode directly, to avoid infinite recursion since // fallbackToDefaultMode calls this method. The caller's try/catch // handles the fallback flow. this.applyThemeInternal(theme); } private applyThemeInternal(theme: AnyThemeConfig): void { const normalizedConfig = normalizeThemeConfig(theme); this.globalTheme.setConfig(normalizedConfig); // Load custom fonts if specified in theme config ``` <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Open ThemeController and inspect the two methods that apply themes: - applyTheme(...) (declared starting at line ~830 in the PR hunk). - applyThemeWithRecovery(...) (immediately following, ending at ~850). Both methods perform normalization, setConfig, and loadFonts with duplicated code. 2. Make a code change that updates applyTheme(...) only (for example, add extra validation or a try/catch branch) but forget to update applyThemeWithRecovery(...). - This is a realistic human-edit regression: both methods are adjacent in ThemeController.ts and a future maintainer is likely to modify only the more prominent applyTheme(...) path. 3. Trigger a recovery path that uses applyThemeWithRecovery(...) (e.g., fallbackToDefaultMode calls applyThemeWithRecovery with freshSystemTheme at lines 586-593). - The recovery path behavior will differ from the main applyTheme path because the duplicated logic was not kept in sync, leading to inconsistent normalization, font loading, or error handling. 4. Observe inconsistent behavior between normal theme application and recovery flows: fonts may not load, normalization might be different or errors handled differently. Explanation why this matters: - The duplication is concrete and lives in adjacent code paths that are both executed at runtime (normal apply and recovery apply). The reproduction requires a realistic future change mistake and exercising the recovery path (which the PR expanded). ``` </details> <details> <summary><b>Prompt for AI Agent 🤖 </b></summary> ```mdx This is a comment left during a code review. **Path:** superset-frontend/src/theme/ThemeController.ts **Line:** 832:846 **Comment:** *Possible Bug: The implementations of `applyTheme` and `applyThemeWithRecovery` duplicate the same normalization, config application, and font-loading logic, increasing the risk that future changes will update one path but not the other and cause inconsistent behavior between normal and recovery theme application. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. ``` </details> -- 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]
