You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gabotorresruiz (via GitHub)" <gi...@apache.org> on 2025/11/28 05:17:24 UTC
[PR] feat(theming): add per-theme custom font URL support [superset]
gabotorresruiz opened a new pull request, #36317:
URL: https://github.com/apache/superset/pull/36317
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
Adds support for custom font URLs in theme configurations and migrates default fonts from bundled packages to CDN loading. Now we can specify a `fontUrls` array in theme tokens to load external fonts (Google Fonts, Adobe Fonts) that are automatically injected when the theme is applied.
- Removes bundled @fontsource packages in favor of loading Inter and Fira Code via Google Fonts
- Removes `CUSTOM_FONT_URLS` config in favor of per-theme `fontUrls` in theme tokens
- Configures CSP to dynamically allow font domains from `THEME_FONT_URL_ALLOWED_DOMAINS`
### AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Create/update a theme with `fontFamily` and `fontUrls` in the token configuration:
```json
{
"token": {
"fontFamily": "Science Gothic, sans-serif",
"fontUrls": ["https://fonts.googleapis.com/css2?family=Science+Gothic:wght@100..900&display=swap"]
},
"algorithm": "dark"
}
```
2. Apply the theme and verify the font loads correctly
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #36317:
URL: https://github.com/apache/superset/pull/36317#issuecomment-3587953569
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #36317:
URL: https://github.com/apache/superset/pull/36317#discussion_r2578027023
##########
superset/themes/utils.py:
##########
@@ -120,3 +124,62 @@ def sanitize_theme_tokens(theme_config: Dict[str, Any]) -> Dict[str, Any]:
sanitized_config["token"] = tokens
return sanitized_config
+
+
+def _validate_single_font_url(index: int, url: Any, allowed_domains: list[str]) -> str:
+ """
+ Validate a single font URL
+ """
+ if not isinstance(url, str):
+ raise ValidationError(f"fontUrls[{index}] must be a string")
+
+ # Reuse existing sanitize_url (blocks javascript:, data:, etc.)
+ sanitized = sanitize_url(url.strip())
+ if not sanitized:
+ raise ValidationError(f"fontUrls[{index}] contains invalid URL")
+
+ # Enforce HTTPS and validate domain
+ try:
+ parsed = urlparse(sanitized)
+ if parsed.scheme != "https":
+ raise ValidationError(f"fontUrls[{index}] must use HTTPS")
+ if parsed.netloc.lower() not in [d.lower() for d in allowed_domains]:
+ raise ValidationError(
+ f"fontUrls[{index}] domain '{parsed.netloc}' not in allowed list"
+ )
+ except ValidationError:
+ raise
+ except Exception as e:
+ raise ValidationError(f"fontUrls[{index}] is malformed: {e}") from e
+
+ return sanitized
+
+
+def validate_font_urls(font_urls: Any) -> list[str]:
+ """
+ Validate fontUrls array in theme token configuration
Review Comment:
The docstring for `validate_font_urls` is minimal and doesn't describe the parameters, return value, or potential exceptions. Consider adding more complete documentation:
```python
def validate_font_urls(font_urls: Any) -> list[str]:
"""
Validate fontUrls array in theme token configuration.
Performs security validation including:
- Type checking (must be array of strings)
- URL sanitization (blocks javascript:, data: schemes)
- HTTPS enforcement
- Domain whitelisting
- Maximum URL count limit
Args:
font_urls: Font URLs to validate (None, list, or other type)
Returns:
list[str]: List of validated and sanitized font URLs (empty list if input is None)
Raises:
ValidationError: If validation fails for any reason
"""
```
```suggestion
Validate and sanitize the fontUrls array in theme token configuration.
Performs security validation including:
- Type checking (must be a list of strings)
- URL sanitization (blocks javascript:, data: schemes)
- HTTPS enforcement
- Domain whitelisting
- Maximum URL count limit
Args:
font_urls (Any): Font URLs to validate (None, list, or other type).
Returns:
list[str]: List of validated and sanitized font URLs (empty list if input is None).
Raises:
ValidationError: If validation fails for any reason, such as:
- font_urls is not a list
- Any URL is not a string
- Any URL is invalid, not HTTPS, or not in the allowed domains
- The number of URLs exceeds the configured maximum
```
##########
superset/themes/utils.py:
##########
@@ -120,3 +124,62 @@ def sanitize_theme_tokens(theme_config: Dict[str, Any]) -> Dict[str, Any]:
sanitized_config["token"] = tokens
return sanitized_config
+
+
+def _validate_single_font_url(index: int, url: Any, allowed_domains: list[str]) -> str:
+ """
+ Validate a single font URL
Review Comment:
The docstring for `_validate_single_font_url` is minimal and doesn't describe the parameters, return value, or potential exceptions. Consider adding more complete documentation:
```python
def _validate_single_font_url(index: int, url: Any, allowed_domains: list[str]) -> str:
"""
Validate a single font URL for security and domain whitelisting.
Args:
index: The position of the URL in the fontUrls array (for error messages)
url: The URL to validate (expected to be a string)
allowed_domains: List of allowed domain names for font URLs
Returns:
str: The sanitized URL
Raises:
ValidationError: If URL is invalid, uses non-HTTPS scheme, or domain not allowed
"""
```
```suggestion
Validate a single font URL for security and domain whitelisting.
Args:
index: The position of the URL in the fontUrls array (for error messages)
url: The URL to validate (expected to be a string)
allowed_domains: List of allowed domain names for font URLs
Returns:
str: The sanitized URL
Raises:
ValidationError: If URL is invalid, uses non-HTTPS scheme, or domain not allowed
```
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas merged PR #36317:
URL: https://github.com/apache/superset/pull/36317
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36317:
URL: https://github.com/apache/superset/pull/36317#issuecomment-3598004484
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) deployed environment on [GHA](https://github.com/apache/superset/actions/runs/19831781844) for [69db2f0](https://github.com/apache/superset/commit/69db2f0)
• **Environment:** http://34.220.94.119:8080 (admin/admin)
• **Lifetime:** 48h auto-cleanup
• **Updates:** New commits create fresh environments automatically
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on code in PR #36317:
URL: https://github.com/apache/superset/pull/36317#discussion_r2570513892
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -781,12 +789,41 @@ export class ThemeController {
// The merging with base theme happens in getThemeForMode() and other methods
// that prepare themes before passing them to applyTheme()
this.globalTheme.setConfig(normalizedConfig);
+
+ // Load custom fonts if specified in theme config
+ const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
+ ?.fontUrls as string[] | undefined;
+ this.loadFonts(fontUrls);
} catch (error) {
console.error('Failed to apply theme:', error);
this.fallbackToDefaultMode();
}
}
+ /**
+ * Loads custom fonts from theme configuration.
+ * Injects CSS @import statements for font URLs that haven't been loaded yet.
+ * @param fontUrls - Array of font URLs to load (e.g., Google Fonts, Adobe Fonts)
+ */
+ private loadFonts(fontUrls?: string[]): void {
+ if (!fontUrls?.length) return;
+
+ // Filter out already loaded fonts
+ const newUrls = fontUrls.filter(url => !this.loadedFontUrls.has(url));
+ if (newUrls.length === 0) return;
+
+ // Use CSS @import for font loading
+ const css = newUrls.map(url => `@import url("${url}");`).join('\n');
+
+ const style = document.createElement('style');
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CSS Injection Risk</b></div>
<div id="fix">
The template literal injection of font URLs into CSS @import statements could allow injection attacks if URLs contain quotes. It looks like JSON.stringify would provide safe escaping here.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
// Use CSS @import for font loading
const css = newUrls.map(url => `@import url(${JSON.stringify(url)});`).join('\n');
const style = document.createElement('style');
````
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36317#issuecomment-3587953569>#607d71</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/src/theme/tests/ThemeController.test.ts:
##########
@@ -102,1272 +102,1391 @@ const mockThemeObject = {
theme: DEFAULT_THEME,
} as unknown as Theme;
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('LocalStorageAdapter', () => {
- let adapter: LocalStorageAdapter;
- const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
-
- beforeEach(() => {
- adapter = new LocalStorageAdapter();
- jest.clearAllMocks();
- Object.defineProperty(window, 'localStorage', {
- value: mockLocalStorage,
- writable: true,
- });
+// Helper to create a fresh ThemeController with common setup
+const createController = (
+ options: Partial<ConstructorParameters<typeof ThemeController>[0]> = {},
+) =>
+ new ThemeController({
+ themeObject: mockThemeObject,
+ ...options,
});
- afterAll(() => {
- consoleSpy.mockRestore();
- });
-
- test('should return item from localStorage', () => {
- mockLocalStorage.getItem.mockReturnValue('test-value');
+// Shared console spies
+const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
+const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
- const result = adapter.getItem('test-key');
+beforeEach(() => {
+ jest.clearAllMocks();
- expect(mockLocalStorage.getItem).toHaveBeenCalledTimes(1);
- expect(mockLocalStorage.getItem).toHaveBeenCalledWith('test-key');
- expect(result).toBe('test-value');
+ // Setup DOM environment
+ Object.defineProperty(window, 'localStorage', {
+ value: mockLocalStorage,
+ writable: true,
});
- test('should set item in localStorage', () => {
- adapter.setItem('test-key', 'test-value');
+ Object.defineProperty(window, 'matchMedia', {
+ value: mockMatchMedia,
+ writable: true,
+ });
- expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
- expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
- 'test-key',
- 'test-value',
- );
+ mockMatchMedia.mockReturnValue({
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
});
- test('should handle localStorage errors while setting an item', () => {
- mockLocalStorage.setItem.mockImplementation(() => {
- throw new Error('Storage error');
- });
+ mockSetConfig.mockImplementation(() => {});
+ mockThemeFromConfig.mockReturnValue(mockThemeObject);
- adapter.setItem('test-key', 'test-value');
+ // Mock Theme constructor
+ (Theme as any).fromConfig = mockThemeFromConfig;
- expect(consoleSpy).toHaveBeenCalledTimes(1);
- expect(consoleSpy).toHaveBeenCalledWith(
- 'Failed to write to localStorage:',
- expect.any(Error),
- );
- });
+ // Reset localStorage mocks
+ mockLocalStorage.getItem.mockReturnValue(null);
+ mockLocalStorage.setItem.mockImplementation(() => {});
+ mockLocalStorage.removeItem.mockImplementation(() => {});
- test('should remove item from localStorage', () => {
- adapter.removeItem('test-key');
+ // Default BootstrapData
+ mockGetBootstrapData.mockReturnValue(createMockBootstrapData());
+});
- expect(mockLocalStorage.removeItem).toHaveBeenCalledTimes(1);
- expect(mockLocalStorage.removeItem).toHaveBeenCalledWith('test-key');
- });
+afterAll(() => {
+ consoleSpy.mockRestore();
+ consoleErrorSpy.mockRestore();
+});
- test('should handle localStorage errors while removing an item', () => {
- mockLocalStorage.removeItem.mockImplementation(() => {
- throw new Error('Storage error');
- });
+// LocalStorageAdapter tests
+test('LocalStorageAdapter returns item from localStorage', () => {
+ const adapter = new LocalStorageAdapter();
+ mockLocalStorage.getItem.mockReturnValue('test-value');
- adapter.removeItem('test-key');
+ const result = adapter.getItem('test-key');
- expect(consoleSpy).toHaveBeenCalledTimes(1);
- expect(consoleSpy).toHaveBeenCalledWith(
- 'Failed to remove from localStorage:',
- expect.any(Error),
- );
- });
+ expect(mockLocalStorage.getItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.getItem).toHaveBeenCalledWith('test-key');
+ expect(result).toBe('test-value');
});
-// eslint-disable-next-line no-restricted-globals -- TODO: Migrate from describe blocks
-describe('ThemeController', () => {
- let controller: ThemeController;
- const consoleSpy = jest.spyOn(console, 'warn').mockImplementation();
- const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation();
+test('LocalStorageAdapter sets item in localStorage', () => {
+ const adapter = new LocalStorageAdapter();
- beforeEach(() => {
- jest.clearAllMocks();
+ adapter.setItem('test-key', 'test-value');
- // Setup DOM environment
- Object.defineProperty(window, 'localStorage', {
- value: mockLocalStorage,
- writable: true,
- });
+ expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
+ 'test-key',
+ 'test-value',
+ );
+});
- Object.defineProperty(window, 'matchMedia', {
- value: mockMatchMedia,
- writable: true,
- });
+test('LocalStorageAdapter handles localStorage errors while setting an item', () => {
+ const adapter = new LocalStorageAdapter();
+ mockLocalStorage.setItem.mockImplementation(() => {
+ throw new Error('Storage error');
+ });
- mockMatchMedia.mockReturnValue({
- matches: false,
- addEventListener: jest.fn(),
- removeEventListener: jest.fn(),
- });
+ adapter.setItem('test-key', 'test-value');
- mockSetConfig.mockImplementation(() => {});
- mockThemeFromConfig.mockReturnValue(mockThemeObject);
+ expect(consoleSpy).toHaveBeenCalledTimes(1);
+ expect(consoleSpy).toHaveBeenCalledWith(
+ 'Failed to write to localStorage:',
+ expect.any(Error),
+ );
+});
- // Mock Theme constructor
- (Theme as any).fromConfig = mockThemeFromConfig;
+test('LocalStorageAdapter removes item from localStorage', () => {
+ const adapter = new LocalStorageAdapter();
- // Reset localStorage mocks
- mockLocalStorage.getItem.mockReturnValue(null);
- mockLocalStorage.setItem.mockImplementation(() => {});
- mockLocalStorage.removeItem.mockImplementation(() => {});
+ adapter.removeItem('test-key');
- // Default BootstrapData
- mockGetBootstrapData.mockReturnValue(createMockBootstrapData());
- });
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith('test-key');
+});
- afterAll(() => {
- consoleSpy.mockRestore();
- consoleErrorSpy.mockRestore();
+test('LocalStorageAdapter handles localStorage errors while removing an item', () => {
+ const adapter = new LocalStorageAdapter();
+ mockLocalStorage.removeItem.mockImplementation(() => {
+ throw new Error('Storage error');
});
- test('should initialize with default options', () => {
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
+ adapter.removeItem('test-key');
- expect(controller.getTheme()).toBe(mockThemeObject);
- });
+ expect(consoleSpy).toHaveBeenCalledTimes(1);
+ expect(consoleSpy).toHaveBeenCalledWith(
+ 'Failed to remove from localStorage:',
+ expect.any(Error),
+ );
+});
- test('should use BootstrapData themes when available', () => {
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
-
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- expect(mockSetConfig).toHaveBeenCalledWith(
- expect.objectContaining({
- token: expect.objectContaining({
- colorBgBase: '#ededed',
- colorPrimary: '#c96f0f',
- }),
- }),
- );
- });
+// ThemeController initialization tests
+test('ThemeController initializes with default options', () => {
+ const controller = createController();
+
+ expect(controller.getTheme()).toBe(mockThemeObject);
+});
+
+test('ThemeController uses BootstrapData themes when available', () => {
+ createController();
- test('should fallback to Superset default theme when BootstrapData themes are empty', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: {},
- dark: {},
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed',
+ colorPrimary: '#c96f0f',
}),
- );
+ }),
+ );
+});
- const fallbackTheme = {
- token: {
+test('ThemeController fallbacks to Superset default theme when BootstrapData themes are empty', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: {},
+ }),
+ );
+
+ const fallbackTheme = {
+ token: {
+ colorBgBase: '#ffffff',
+ colorPrimary: '#1890ff',
+ },
+ };
+
+ createController({ defaultTheme: fallbackTheme });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ ...fallbackTheme,
+ algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
+
+test('ThemeController handles system theme preference', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: DARK_THEME,
+ }),
+ );
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM);
+});
+
+test('ThemeController handles only default theme', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: {},
+ }),
+ );
+
+ const controller = createController();
+
+ jest.clearAllMocks();
+
+ expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow(
+ 'Theme mode changes are not allowed when only one theme is available',
+ );
+
+ expect(mockSetConfig).not.toHaveBeenCalled();
+});
+
+test('ThemeController handles only dark theme', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: DARK_THEME,
+ }),
+ );
+
+ const lightThemeFallback = {
+ token: {
+ colorBgBase: '#fff',
+ colorTextBase: '#000',
+ colorPrimary: '#1890ff',
+ },
+ };
+
+ const controller = createController({ defaultTheme: lightThemeFallback });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+
+ const calledWith = mockSetConfig.mock.calls[0][0];
+
+ expect(calledWith.token.colorBgBase).toBe('#fff');
+ expect(calledWith.token.colorTextBase).toBe('#000');
+
+ expect(controller.canSetMode()).toBe(true);
+
+ jest.clearAllMocks();
+ controller.setThemeMode(ThemeMode.DARK);
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+});
+
+test('ThemeController handles completely empty BootstrapData', () => {
+ const fallbackTheme = {
+ token: {
+ colorBgBase: '#ffffff',
+ colorPrimary: '#1890ff',
+ },
+ };
+
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: {},
+ }),
+ );
+
+ createController({ defaultTheme: fallbackTheme });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
colorBgBase: '#ffffff',
colorPrimary: '#1890ff',
- },
- };
-
- controller = new ThemeController({
- themeObject: mockThemeObject,
- defaultTheme: fallbackTheme,
- });
-
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- expect(mockSetConfig).toHaveBeenCalledWith(
- expect.objectContaining({
- ...fallbackTheme,
- algorithm: antdThemeImport.defaultAlgorithm,
}),
- );
+ algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
+
+test('ThemeController handles missing theme object', () => {
+ const fallbackTheme = { token: { colorPrimary: '#fallback' } };
+
+ mockGetBootstrapData.mockReturnValue({
+ common: {
+ application_root: '/',
+ static_assets_prefix: '/static/assets/',
+ conf: {},
+ locale: 'en',
+ feature_flags: {},
+ language_pack: {},
+ extra_categorical_color_schemes: [],
+ extra_sequential_color_schemes: [],
+ menu_data: {},
+ d3_format: {},
+ d3_time_format: {},
+ } as any,
});
- test('should handle system theme preference', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: DEFAULT_THEME,
- dark: DARK_THEME,
+ createController({ defaultTheme: fallbackTheme });
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorPrimary: '#fallback',
}),
- );
+ algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
+// Theme Management tests
- expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM);
- });
+test('ThemeController updates theme when allowed', () => {
+ const controller = createController();
- test('should handle only default theme', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: DEFAULT_THEME,
- dark: {},
- }),
- );
+ const newTheme = {
+ token: {
+ colorBgBase: '#000000',
+ colorPrimary: '#ff0000',
+ },
+ };
- controller = new ThemeController({
- themeObject: mockThemeObject,
- });
+ controller.setTheme(newTheme);
- // Clear the call from initialization
- jest.clearAllMocks();
+ expect(mockSetConfig).toHaveBeenCalledWith(expect.objectContaining(newTheme));
+});
- // Should throw when trying to change mode with only one theme
- expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow(
- 'Theme mode changes are not allowed when only one theme is available',
- );
+test('ThemeController changes theme mode when allowed', () => {
+ const controller = createController();
- // Config should not have been called since the error was thrown
- expect(mockSetConfig).not.toHaveBeenCalled();
- });
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.DARK);
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DARK);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
+ 'superset-theme-mode',
+ ThemeMode.DARK,
+ );
+});
+
+test('ThemeController handles missing theme gracefully', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: {},
+ }),
+ );
+
+ const controller = createController();
+
+ expect(() => controller.setThemeMode(ThemeMode.DARK)).toThrow(
+ 'Theme mode changes are not allowed when only one theme is available',
+ );
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+ expect(consoleSpy).not.toHaveBeenCalled();
+});
+
+test('ThemeController does not change mode if already set', () => {
+ const controller = createController();
- test('should handle only dark theme', () => {
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: {},
- dark: DARK_THEME,
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.SYSTEM);
+
+ expect(mockLocalStorage.setItem).not.toHaveBeenCalled();
+});
+
+test('ThemeController resets to default theme', () => {
+ const controller = createController();
+
+ controller.setThemeMode(ThemeMode.DARK);
+ controller.resetTheme();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#141118',
+ colorTextBase: '#fdc7c7',
}),
- );
+ }),
+ );
+});
- // Provide an explicit light theme fallback for this test
- const lightThemeFallback = {
- token: {
- colorBgBase: '#fff',
- colorTextBase: '#000',
- colorPrimary: '#1890ff',
- },
- };
+// System Theme Changes tests
+test('ThemeController listens to system theme changes', () => {
+ const mockMediaQuery = {
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQuery);
+
+ createController();
+
+ expect(mockMediaQuery.addEventListener).toHaveBeenCalledTimes(1);
+ expect(mockMediaQuery.addEventListener).toHaveBeenCalledWith(
+ 'change',
+ expect.any(Function),
+ );
+});
- controller = new ThemeController({
- themeObject: mockThemeObject,
- defaultTheme: lightThemeFallback,
- });
+test('ThemeController updates theme when system preference changes and mode is SYSTEM', () => {
+ const mockMediaQuery = {
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQuery);
- // When only dark theme is available, controller uses the default fallback theme initially
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ const controller = createController();
+ controller.setThemeMode(ThemeMode.SYSTEM);
- const calledWith = mockSetConfig.mock.calls[0][0];
+ mockMediaQuery.matches = true;
+ const changeHandler = mockMediaQuery.addEventListener.mock.calls[0][1];
- // Should use the default theme fallback (not dark) for initial load
- expect(calledWith.token.colorBgBase).toBe('#fff');
- expect(calledWith.token.colorTextBase).toBe('#000');
+ changeHandler();
- // Should allow mode changes since dark theme exists
- expect(controller.canSetMode()).toBe(true);
+ expect(mockSetConfig).toHaveBeenCalled();
+});
- // Should be able to switch to dark mode
- jest.clearAllMocks();
- controller.setThemeMode(ThemeMode.DARK);
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- });
+test('ThemeController does not update theme when mode is not SYSTEM', () => {
+ const mockMediaQuery = {
+ matches: false,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQuery);
- test('should handle completely empty BootstrapData', () => {
- const fallbackTheme = {
- token: {
- colorBgBase: '#ffffff',
- colorPrimary: '#1890ff',
- },
- };
+ const controller = createController();
+ controller.setThemeMode(ThemeMode.DEFAULT);
+
+ const initialCallCount = mockSetConfig.mock.calls.length;
+
+ mockMediaQuery.matches = true;
+ const changeHandler = mockMediaQuery.addEventListener.mock.calls[0][1];
+
+ changeHandler();
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(initialCallCount);
+});
+
+test('ThemeController switches to dark theme when system is dark and mode is SYSTEM', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: DEFAULT_THEME,
+ dark: DARK_THEME,
+ }),
+ );
+
+ const mockMediaQueryDark = {
+ matches: true,
+ addEventListener: jest.fn(),
+ removeEventListener: jest.fn(),
+ };
+ mockMatchMedia.mockReturnValue(mockMediaQueryDark);
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.SYSTEM);
+
+ expect(mockSetConfig).toHaveBeenCalled();
+ const lastCall =
+ mockSetConfig.mock.calls[mockSetConfig.mock.calls.length - 1][0];
+ expect(lastCall.token.colorBgBase).toBe(DARK_THEME.token!.colorBgBase);
+ expect(lastCall.token.colorTextBase).toBe(DARK_THEME.token!.colorTextBase);
+});
+
+// Persistence tests
+
+test('ThemeController saves theme mode to localStorage', () => {
+ const controller = createController();
+
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.DARK);
+
+ expect(mockLocalStorage.setItem).toHaveBeenCalledTimes(1);
+ expect(mockLocalStorage.setItem).toHaveBeenCalledWith(
+ 'superset-theme-mode',
+ ThemeMode.DARK,
+ );
+});
- mockGetBootstrapData.mockReturnValue(
- createMockBootstrapData({
- default: {},
- dark: {},
+test('ThemeController loads saved theme mode from localStorage', () => {
+ mockLocalStorage.getItem.mockReturnValue(ThemeMode.DARK);
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DARK);
+});
+
+test('ThemeController handles invalid saved theme mode', () => {
+ mockGetBootstrapData.mockReturnValue(
+ createMockBootstrapData({
+ default: {},
+ dark: {},
+ }),
+ );
+
+ mockLocalStorage.getItem.mockReturnValue('invalid-mode' as any);
+
+ const controller = createController();
+
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+// Theme Structure tests
+
+test('ThemeController handles theme with token structure', () => {
+ const controller = createController();
+
+ const customTheme = {
+ token: {
+ colorBgBase: '#ff0000',
+ colorTextBase: '#ffffff',
+ colorPrimary: '#00ff00',
+ },
+ };
+
+ jest.clearAllMocks();
+
+ controller.setTheme(customTheme);
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ff0000',
+ colorTextBase: '#ffffff',
+ colorPrimary: '#00ff00',
}),
- );
-
- controller = new ThemeController({
- themeObject: mockThemeObject,
- defaultTheme: fallbackTheme,
- });
-
- expect(mockSetConfig).toHaveBeenCalledTimes(1);
- expect(mockSetConfig).toHaveBeenCalledWith(
- expect.objectContaining({
- token: expect.objectContaining({
- colorBgBase: '#ffffff',
- colorPrimary: '#1890ff',
- }),
- algorithm: antdThemeImport.defaultAlgorithm,
+ }),
+ );
+});
+
+test('ThemeController preserves algorithm property from dark theme', () => {
+ const controller = createController();
+
+ jest.clearAllMocks();
+
+ controller.setThemeMode(ThemeMode.DARK);
+
+ expect(mockSetConfig).toHaveBeenCalledTimes(1);
+ expect(mockSetConfig).toHaveBeenCalledWith(
+ expect.objectContaining({
+ algorithm: antdThemeImport.darkAlgorithm,
+ token: expect.objectContaining({
+ colorBgBase: '#141118',
+ colorTextBase: '#fdc7c7',
}),
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Incorrect test assertion</b></div>
<div id="fix">
The test assertion for 'ThemeController resets to default theme' incorrectly expects dark theme colors when resetTheme() should apply default theme colors. This could cause the test to fail or pass incorrectly, masking bugs in the reset functionality.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
````suggestion
token: expect.objectContaining({
colorBgBase: '#ededed',
colorTextBase: '#120f0f',
}),
````
</div>
</details>
</div>
<small><i>Code Review Run <a href=https://github.com/apache/superset/pull/36317#issuecomment-3587953569>#607d71</a></i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #36317:
URL: https://github.com/apache/superset/pull/36317#issuecomment-3597931558
🎪 [Showtime](https://github.com/mistercrunch/superset-showtime) is building environment on [GHA](https://github.com/apache/superset/actions/runs/19831781844) for [69db2f0](https://github.com/apache/superset/commit/69db2f0)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #36317:
URL: https://github.com/apache/superset/pull/36317#issuecomment-3599125185
Hey @rusackas, awesome catch, thanks! I fixed that and cleaned up a couple of other things I spotted with @mistercrunch
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] feat(theming): add per-theme custom font URL support [superset]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #36317:
URL: https://github.com/apache/superset/pull/36317#issuecomment-3587965672
## [Codecov](https://app.codecov.io/gh/apache/superset/pull/36317?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `19.56522%` with `37 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 48.01%. Comparing base ([`76d897e`](https://app.codecov.io/gh/apache/superset/commit/76d897eaa2f9e137102bc194c2e3109c29d0348f?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`b729d17`](https://app.codecov.io/gh/apache/superset/commit/b729d17d944c60b3e829534392e9113ee343a4db?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2976 commits behind head on master.
| [Files with missing lines](https://app.codecov.io/gh/apache/superset/pull/36317?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [superset/themes/utils.py](https://app.codecov.io/gh/apache/superset/pull/36317?src=pr&el=tree&filepath=superset%2Fthemes%2Futils.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGhlbWVzL3V0aWxzLnB5) | 16.66% | [25 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36317?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [superset/themes/schemas.py](https://app.codecov.io/gh/apache/superset/pull/36317?src=pr&el=tree&filepath=superset%2Fthemes%2Fschemas.py&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3VwZXJzZXQvdGhlbWVzL3NjaGVtYXMucHk=) | 7.69% | [12 Missing :warning: ](https://app.codecov.io/gh/apache/superset/pull/36317?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #36317 +/- ##
===========================================
- Coverage 60.48% 48.01% -12.48%
===========================================
Files 1931 636 -1295
Lines 76236 46860 -29376
Branches 8568 5094 -3474
===========================================
- Hits 46114 22501 -23613
+ Misses 28017 23708 -4309
+ Partials 2105 651 -1454
```
| [Flag](https://app.codecov.io/gh/apache/superset/pull/36317/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [hive](https://app.codecov.io/gh/apache/superset/pull/36317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `43.72% <19.56%> (-5.44%)` | :arrow_down: |
| [javascript](https://app.codecov.io/gh/apache/superset/pull/36317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [presto](https://app.codecov.io/gh/apache/superset/pull/36317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `47.31% <19.56%> (-6.49%)` | :arrow_down: |
| [python](https://app.codecov.io/gh/apache/superset/pull/36317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `48.01% <19.56%> (-15.50%)` | :arrow_down: |
| [unit](https://app.codecov.io/gh/apache/superset/pull/36317/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `100.00% <ø> (+42.36%)` | :arrow_up: |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/superset/pull/36317?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org