sadpandajoe commented on code in PR #37378: URL: https://github.com/apache/superset/pull/37378#discussion_r2775438482
########## superset-frontend/src/theme/utils/antdTokenNames.test.ts: ########## @@ -0,0 +1,108 @@ +/** + * 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 { + isValidTokenName, + isSupersetCustomToken, + getAllValidTokenNames, +} from './antdTokenNames'; + +test('isValidTokenName recognizes standard Ant Design tokens', () => { + expect(isValidTokenName('colorPrimary')).toBe(true); + expect(isValidTokenName('fontSize')).toBe(true); + expect(isValidTokenName('padding')).toBe(true); + expect(isValidTokenName('borderRadius')).toBe(true); +}); + +test('isValidTokenName recognizes Superset custom tokens', () => { + expect(isValidTokenName('brandLogoUrl')).toBe(true); + expect(isValidTokenName('brandSpinnerSvg')).toBe(true); + expect(isValidTokenName('fontSizeXS')).toBe(true); + expect(isValidTokenName('echartsOptionsOverrides')).toBe(true); +}); + +test('isValidTokenName rejects unknown tokens', () => { + expect(isValidTokenName('fooBarBaz')).toBe(false); + expect(isValidTokenName('colrPrimary')).toBe(false); + expect(isValidTokenName('invalidToken')).toBe(false); +}); + +test('isValidTokenName handles edge cases', () => { + expect(isValidTokenName('')).toBe(false); + expect(isValidTokenName(' ')).toBe(false); +}); + +test('isSupersetCustomToken identifies Superset-specific tokens', () => { + expect(isSupersetCustomToken('brandLogoUrl')).toBe(true); + expect(isSupersetCustomToken('brandSpinnerSvg')).toBe(true); + expect(isSupersetCustomToken('fontSizeXS')).toBe(true); + expect(isSupersetCustomToken('fontUrls')).toBe(true); Review Comment: `fontUrls` **is** in `SUPERSET_CUSTOM_TOKENS`. See line 50 of antdTokenNames.ts: ```typescript const SUPERSET_CUSTOM_TOKENS: Set<string> = new Set([ // ... other tokens ... 'fontUrls', // ← Line 50 ]); ``` The test correctly verifies this. ########## superset-frontend/src/theme/utils/antdTokenNames.test.ts: ########## @@ -0,0 +1,108 @@ +/** + * 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 { + isValidTokenName, + isSupersetCustomToken, + getAllValidTokenNames, +} from './antdTokenNames'; + +test('isValidTokenName recognizes standard Ant Design tokens', () => { + expect(isValidTokenName('colorPrimary')).toBe(true); + expect(isValidTokenName('fontSize')).toBe(true); + expect(isValidTokenName('padding')).toBe(true); + expect(isValidTokenName('borderRadius')).toBe(true); +}); + +test('isValidTokenName recognizes Superset custom tokens', () => { + expect(isValidTokenName('brandLogoUrl')).toBe(true); + expect(isValidTokenName('brandSpinnerSvg')).toBe(true); + expect(isValidTokenName('fontSizeXS')).toBe(true); + expect(isValidTokenName('echartsOptionsOverrides')).toBe(true); +}); + +test('isValidTokenName rejects unknown tokens', () => { + expect(isValidTokenName('fooBarBaz')).toBe(false); + expect(isValidTokenName('colrPrimary')).toBe(false); + expect(isValidTokenName('invalidToken')).toBe(false); +}); + +test('isValidTokenName handles edge cases', () => { + expect(isValidTokenName('')).toBe(false); + expect(isValidTokenName(' ')).toBe(false); +}); + +test('isSupersetCustomToken identifies Superset-specific tokens', () => { + expect(isSupersetCustomToken('brandLogoUrl')).toBe(true); + expect(isSupersetCustomToken('brandSpinnerSvg')).toBe(true); + expect(isSupersetCustomToken('fontSizeXS')).toBe(true); + expect(isSupersetCustomToken('fontUrls')).toBe(true); +}); + +test('isSupersetCustomToken returns false for Ant Design tokens', () => { + expect(isSupersetCustomToken('colorPrimary')).toBe(false); + expect(isSupersetCustomToken('fontSize')).toBe(false); +}); + +test('isSupersetCustomToken returns false for unknown tokens', () => { + expect(isSupersetCustomToken('fooBar')).toBe(false); +}); + +test('getAllValidTokenNames returns categorized token names', () => { + const result = getAllValidTokenNames(); + + expect(result).toHaveProperty('antdTokens'); + expect(result).toHaveProperty('supersetTokens'); + expect(result).toHaveProperty('total'); +}); + +test('getAllValidTokenNames has reasonable token counts', () => { + const result = getAllValidTokenNames(); + + // Ant Design tokens should exist (avoid brittle exact count that breaks on upgrades) + expect(result.antdTokens.length).toBeGreaterThan(0); + expect(result.antdTokens).toContain('colorPrimary'); + expect(result.antdTokens).toContain('fontSize'); + expect(result.antdTokens).toContain('borderRadius'); + + // Superset custom tokens should exist + expect(result.supersetTokens.length).toBeGreaterThan(0); + expect(result.supersetTokens).toContain('brandLogoUrl'); + expect(result.supersetTokens).toContain('fontUrls'); Review Comment: Same as above - `fontUrls` is present in `SUPERSET_CUSTOM_TOKENS` at line 50. ########## superset-frontend/src/theme/utils/antdTokenNames.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 { theme } from 'antd'; + +/** + * Superset-specific custom tokens that extend Ant Design's token system. + * These keys are derived from the SupersetSpecificTokens interface to ensure consistency. + */ +const SUPERSET_CUSTOM_TOKENS: Set<string> = new Set([ + // Font extensions + 'fontSizeXS', + 'fontSizeXXL', + 'fontWeightNormal', + 'fontWeightLight', + 'fontWeightStrong', Review Comment: **Valid feedback - fixed!** ✅ `fontWeightStrong` is indeed an Ant Design token (confirmed by checking `theme.getDesignToken()`). Removed it from `SUPERSET_CUSTOM_TOKENS` in commit coming up. ########## 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> { this.currentMode = ThemeMode.DEFAULT; - // Get the default theme which will have the correct algorithm + // Try to fetch fresh system default theme from server + const freshSystemTheme = await this.fetchSystemDefaultTheme(); + + if (freshSystemTheme) { + try { + await this.applyThemeWithRecovery(freshSystemTheme); + this.persistMode(); + this.notifyListeners(); + return; + } catch (error) { + // Fresh theme also failed, continue to final fallback + } + } + + // Final fallback: use cached default theme or built-in theme const defaultTheme: AnyThemeConfig = this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme || {}; Review Comment: `devThemeOverride` is **cleared before** `fallbackToDefaultMode` runs. See `updateTheme` lines 566-572: ```typescript catch (error) { // Clear potentially corrupted overrides before fallback this.devThemeOverride = null; this.storage.removeItem(STORAGE_KEYS.DEV_THEME_OVERRIDE); // ... await this.fallbackToDefaultMode(); } ``` So when `getThemeForMode(ThemeMode.DEFAULT)` is called in `fallbackToDefaultMode`, `devThemeOverride` is already `null` and won't be returned. ########## 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> { this.currentMode = ThemeMode.DEFAULT; - // Get the default theme which will have the correct algorithm + // Try to fetch fresh system default theme from server + const freshSystemTheme = await this.fetchSystemDefaultTheme(); + + if (freshSystemTheme) { + try { + await this.applyThemeWithRecovery(freshSystemTheme); + this.persistMode(); + this.notifyListeners(); + return; + } catch (error) { + // Fresh theme also failed, continue to final fallback + } + } + + // Final fallback: use cached default theme or built-in theme const defaultTheme: AnyThemeConfig = this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme || {}; Review Comment: The final `applyTheme` in `fallbackToDefaultMode` is the *last resort fallback*. At this point we're applying the cached default theme (or `{}` if nothing else). If this throws, there's nothing more we can do - the theme system is fundamentally broken. Adding a try/catch here would just suppress the error without any recovery option. It's better to let it propagate so developers can see what's wrong. -- 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]
