codeant-ai-for-open-source[bot] commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2755623008
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -796,10 +821,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
+ const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
+ ?.fontUrls as string[] | undefined;
+ this.loadFonts(fontUrls);
Review Comment:
**Suggestion:** When applying the fetched system default theme in
applyThemeWithRecovery, any error thrown by normalizeThemeConfig or setConfig
is caught by fallbackToDefaultMode and silently swallowed, so the failure is
completely invisible in logs and very hard to diagnose; wrapping this method in
a try/catch that logs before rethrowing preserves the error information while
keeping the existing fallback behavior. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Recovery failures produce limited diagnostic logs.
- ⚠️ Theme recovery debugging is harder for engineers.
- ⚠️ Runtime theme fallback may proceed silently.
```
</details>
```suggestion
try {
const normalizedConfig = normalizeThemeConfig(theme);
this.globalTheme.setConfig(normalizedConfig);
// Load custom fonts if specified, mirroring applyTheme() behavior
const fontUrls = (normalizedConfig?.token as Record<string, unknown>)
?.fontUrls as string[] | undefined;
this.loadFonts(fontUrls);
} catch (error) {
console.error(
'Failed to apply system default theme during recovery:',
error,
);
throw error;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open superset-frontend/src/theme/ThemeController.ts and locate
applyThemeWithRecovery
at lines 829-841. Note this method is invoked by fallbackToDefaultMode (see
fallbackToDefaultMode where it calls await
this.applyThemeWithRecovery(freshSystemTheme)).
2. Arrange for fallbackToDefaultMode to run in the code path: for example,
trigger
updateTheme to throw by having an invalid theme applied (updateTheme's
try/catch will call
fallbackToDefaultMode). The call chain is updateTheme ->
fallbackToDefaultMode ->
fetchSystemDefaultTheme -> applyThemeWithRecovery (applyThemeWithRecovery is
defined at
superset-frontend/src/theme/ThemeController.ts:829-841).
3. Cause normalizeThemeConfig or this.globalTheme.setConfig to throw (e.g.,
by returning a
fetched system theme JSON with malformed structure from
fetchSystemDefaultTheme). When
applyThemeWithRecovery throws, current implementation re-throws to the
caller without
logging inside this method.
4. Observe behavior: fallbackToDefaultMode calls applyThemeWithRecovery and
catches errors
from the attempt (fallbackToDefaultMode silently proceeds to other
fallbacks). Because
applyThemeWithRecovery contains no internal logging, the original error
context is not
logged at the point of failure (only higher-level code paths log generic
messages). Adding
a try/catch with console.error in applyThemeWithRecovery would surface the
failure with
full stack and message before rethrowing.
```
</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:** 834:840
**Comment:**
*Possible Bug: When applying the fetched system default theme in
applyThemeWithRecovery, any error thrown by normalizeThemeConfig or setConfig
is caught by fallbackToDefaultMode and silently swallowed, so the failure is
completely invisible in logs and very hard to diagnose; wrapping this method in
a try/catch that logs before rethrowing preserves the error information while
keeping the existing fallback behavior.
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/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,191 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
+});
+
+test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched
theme', async () => {
+ // Test: fallbackToDefaultMode fetches theme from API and applies it
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery
+
+ const controller = createController();
+
+ // Mock fetch to return a system default theme from API
+ const systemTheme = { token: { colorPrimary: '#recovery-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // Track setConfig calls to verify the fetched theme is applied
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode (simulates what happens after applyTheme
fails)
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify API was called to fetch system default theme
+ expect(mockFetch).toHaveBeenCalledWith(
+ expect.stringContaining('/api/v1/theme/'),
+ );
+
+ // Verify the fetched theme was applied via applyThemeWithRecovery
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
Review Comment:
**Suggestion:** This test overrides the global `fetch` function and never
restores it, so later tests in the same Jest environment will keep using this
mock instead of their own, leading to hidden coupling and potentially incorrect
behavior in unrelated tests. [resource leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Jest tests using real fetch may fail unpredictably.
- ⚠️ Theme recovery tests can become flaky across runs.
- ⚠️ Other test files in same worker may see mocked fetch.
```
</details>
```suggestion
const originalFetch = global.fetch;
global.fetch = mockFetch;
try {
// Track setConfig calls to verify the fetched theme is applied
const setConfigCalls: unknown[] = [];
mockSetConfig.mockImplementation((config: unknown) => {
setConfigCalls.push(config);
});
// Trigger fallbackToDefaultMode (simulates what happens after
applyTheme fails)
await (controller as any).fallbackToDefaultMode();
// Verify API was called to fetch system default theme
expect(mockFetch).toHaveBeenCalledWith(
expect.stringContaining('/api/v1/theme/'),
);
// Verify the fetched theme was applied via applyThemeWithRecovery
expect(setConfigCalls.length).toBe(1);
expect(setConfigCalls[0]).toEqual(
expect.objectContaining({
token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
}),
);
// Verify controller is in default mode
expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
} finally {
global.fetch = originalFetch;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run Jest for this test file
(superset-frontend/src/theme/tests/ThemeController.test.ts).
2. Test execution reaches the assignment at
src/theme/tests/ThemeController.test.ts:903
where the code sets global.fetch = mockFetch (the mock is created at lines
~895-901 and
assigned at ~903 in the same test).
3. The test does not restore global.fetch after completion (no
cleanup/restore present in
this test), so the mocked fetch remains on the Node global object for the
remainder of the
Jest process.
4. Subsequent tests running in the same Jest worker (either later tests in
this file or
other test files executed in the same process) will observe the mocked
global.fetch
instead of the original implementation, producing unexpected behavior or
false
positives/failures. The lack of restoration is verifiable by running the
file with
--runInBand and observing global.fetch remains the jest.fn after this test
finishes.
```
</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/tests/ThemeController.test.ts
**Line:** 903:928
**Comment:**
*Resource Leak: This test overrides the global `fetch` function and
never restores it, so later tests in the same Jest environment will keep using
this mock instead of their own, leading to hidden coupling and potentially
incorrect behavior in unrelated tests.
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/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,191 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
+});
+
+test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched
theme', async () => {
+ // Test: fallbackToDefaultMode fetches theme from API and applies it
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery
+
+ const controller = createController();
+
+ // Mock fetch to return a system default theme from API
+ const systemTheme = { token: { colorPrimary: '#recovery-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // Track setConfig calls to verify the fetched theme is applied
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode (simulates what happens after applyTheme
fails)
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify API was called to fetch system default theme
+ expect(mockFetch).toHaveBeenCalledWith(
+ expect.stringContaining('/api/v1/theme/'),
+ );
+
+ // Verify the fetched theme was applied via applyThemeWithRecovery
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+test('recovery flow: both API fetches fail → falls back to cached default
theme', async () => {
+ // Test: When fetchSystemDefaultTheme fails, fallbackToDefaultMode uses
cached theme
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme (fails) →
applyTheme(cached)
+
+ const controller = createController();
+
+ // Mock fetch to fail for both API endpoints
+ const mockFetch = jest.fn().mockRejectedValue(new Error('Network error'));
+ global.fetch = mockFetch;
+
+ // Track setConfig calls
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify fetch was attempted
+ expect(mockFetch).toHaveBeenCalled();
+
+ // Verify fallback to cached default theme was applied via applyTheme
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed', // From DEFAULT_THEME in test setup
+ }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+test('recovery flow: fetched theme fails to apply → falls back to cached
default', async () => {
+ // Test: When applyThemeWithRecovery fails, fallbackToDefaultMode uses
cached theme
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery (fails) → applyTheme(cached)
+
+ const controller = createController();
+
+ // Mock fetch to return a theme
+ const systemTheme = { token: { colorPrimary: '#bad-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // First setConfig call (applyThemeWithRecovery) fails, second (applyTheme)
succeeds
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ if (setConfigCalls.length === 1) {
+ throw new Error('Fetched theme failed to apply');
+ }
+ });
+
+ // Trigger fallbackToDefaultMode
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify fetch was called
+ expect(mockFetch).toHaveBeenCalled();
+
+ // Verify both attempts were made: fetched theme (failed) then cached default
+ expect(setConfigCalls.length).toBe(2);
+
+ // First call was the fetched theme (which failed)
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#bad-theme' }),
+ }),
+ );
+
+ // Second call was the cached default theme
+ expect(setConfigCalls[1]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed', // From DEFAULT_THEME
+ }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
Review Comment:
**Suggestion:** Similar to the other recovery tests, this one overwrites
`global.fetch` with a mock and never restores it, so the mock can leak into
other tests and cause them to see the wrong fetch behavior. [resource leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unrelated tests that call fetch fail intermittently.
- ⚠️ CI test runs may become flaky and unreliable.
- ⚠️ Debugging intermittent test failures increases.
```
</details>
```suggestion
const originalFetch = global.fetch;
global.fetch = mockFetch;
try {
// First setConfig call (applyThemeWithRecovery) fails, second
(applyTheme) succeeds
const setConfigCalls: unknown[] = [];
mockSetConfig.mockImplementation((config: unknown) => {
setConfigCalls.push(config);
if (setConfigCalls.length === 1) {
throw new Error('Fetched theme failed to apply');
}
});
// Trigger fallbackToDefaultMode
await (controller as any).fallbackToDefaultMode();
// Verify fetch was called
expect(mockFetch).toHaveBeenCalled();
// Verify both attempts were made: fetched theme (failed) then cached
default
expect(setConfigCalls.length).toBe(2);
// First call was the fetched theme (which failed)
expect(setConfigCalls[0]).toEqual(
expect.objectContaining({
token: expect.objectContaining({ colorPrimary: '#bad-theme' }),
}),
);
// Second call was the cached default theme
expect(setConfigCalls[1]).toEqual(
expect.objectContaining({
token: expect.objectContaining({
colorBgBase: '#ededed', // From DEFAULT_THEME
}),
}),
);
// Verify controller is in default mode
expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
} finally {
global.fetch = originalFetch;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute the test file
superset-frontend/src/theme/tests/ThemeController.test.ts under
Jest.
2. The test constructs a mock fetch and assigns it to the global at
src/theme/tests/ThemeController.test.ts:981 (mock defined ~977, assigned at
~981).
3. This test does not restore the original global.fetch on completion, so
the jest.fn
remains present on the global object.
4. Any subsequent test in the same Jest worker that expects either a
different fetch mock
or the original fetch implementation will run against this leftover mock,
causing
unexpected invocations or assertion mismatches; running the file with
--runInBand will
make this leak observable.
```
</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/tests/ThemeController.test.ts
**Line:** 981:1018
**Comment:**
*Resource Leak: Similar to the other recovery tests, this one
overwrites `global.fetch` with a mock and never restores it, so the mock can
leak into other tests and cause them to see the wrong fetch behavior.
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/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:
**Suggestion:** The test asserts that a Superset-specific token named
"fontUrls" is recognized by the custom token detection, but the current
implementation of the token registry does not define "fontUrls" as a Superset
custom token, so this expectation will fail and incorrectly signal a regression
in the implementation. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test failure blocks CI checks.
- ⚠️ Theme token utilities tests unreliable.
- ⚠️ Developers hit failing tests locally.
```
</details>
```suggestion
expect(isSupersetCustomToken('echartsOptionsOverrides')).toBe(true);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the unit tests for the theme utilities: from repository root run
`yarn test
src/theme/utils/antdTokenNames.test.ts`. This executes the test file
`superset-frontend/src/theme/utils/antdTokenNames.test.ts` which contains
the failing
case.
2. The test named "isSupersetCustomToken identifies Superset-specific
tokens" (located at
superset-frontend/src/theme/utils/antdTokenNames.test.ts:50-55) performs the
assertion at
line 54: `expect(isSupersetCustomToken('fontUrls')).toBe(true);`.
3. The test calls isSupersetCustomToken implemented at
superset-frontend/src/theme/utils/antdTokenNames.ts:92-94 which returns
`SUPERSET_CUSTOM_TOKENS.has(tokenName)`. If `SUPERSET_CUSTOM_TOKENS` does
not include
'fontUrls', the call returns false and the assertion at line 54 fails with
Jest output
like "Expected false to be true".
4. To confirm, open superset-frontend/src/theme/utils/antdTokenNames.ts and
inspect the
`SUPERSET_CUSTOM_TOKENS` definition (search for the constant in that file).
If 'fontUrls'
is absent, the test expectation is incorrect and should be replaced with a
known token
(e.g., 'echartsOptionsOverrides') or the implementation updated to include
'fontUrls'.
```
</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/antdTokenNames.test.ts
**Line:** 54:54
**Comment:**
*Logic Error: The test asserts that a Superset-specific token named
"fontUrls" is recognized by the custom token detection, but the current
implementation of the token registry does not define "fontUrls" as a Superset
custom token, so this expectation will fail and incorrectly signal a regression
in the implementation.
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/tests/ThemeController.test.ts:
##########
@@ -833,6 +833,191 @@ test('ThemeController handles theme application errors',
() => {
fallbackSpy.mockRestore();
});
+test('ThemeController constructor recovers from corrupted stored theme', () =>
{
+ // Simulate corrupted dev theme override in storage
+ const corruptedTheme = { token: { colorPrimary: '#ff0000' } };
+ mockLocalStorage.getItem.mockImplementation((key: string) => {
+ if (key === 'superset-dev-theme-override') {
+ return JSON.stringify(corruptedTheme);
+ }
+ return null;
+ });
+
+ // Mock Theme.fromConfig to return object with toSerializedConfig
+ mockThemeFromConfig.mockReturnValue({
+ ...mockThemeObject,
+ toSerializedConfig: () => corruptedTheme,
+ });
+
+ // First call throws (corrupted theme), second call succeeds (fallback)
+ let callCount = 0;
+ mockSetConfig.mockImplementation(() => {
+ callCount += 1;
+ if (callCount === 1) {
+ throw new Error('Invalid theme configuration');
+ }
+ });
+
+ const consoleWarnSpy = jest.spyOn(console, 'warn').mockImplementation();
+
+ // Should not throw - constructor should recover
+ const controller = createController();
+
+ // Verify recovery happened
+ expect(consoleWarnSpy).toHaveBeenCalledWith(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ expect.any(Error),
+ );
+
+ // Verify invalid overrides were cleared from storage
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-dev-theme-override',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-crud-theme-id',
+ );
+ expect(mockLocalStorage.removeItem).toHaveBeenCalledWith(
+ 'superset-applied-theme-id',
+ );
+
+ // Verify controller is in a valid state
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+
+ consoleWarnSpy.mockRestore();
+});
+
+test('recovery flow: fetchSystemDefaultTheme returns theme → applies fetched
theme', async () => {
+ // Test: fallbackToDefaultMode fetches theme from API and applies it
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme →
applyThemeWithRecovery
+
+ const controller = createController();
+
+ // Mock fetch to return a system default theme from API
+ const systemTheme = { token: { colorPrimary: '#recovery-theme' } };
+ const mockFetch = jest.fn().mockResolvedValueOnce({
+ ok: true,
+ json: async () => ({
+ result: [{ json_data: JSON.stringify(systemTheme) }],
+ }),
+ });
+ global.fetch = mockFetch;
+
+ // Track setConfig calls to verify the fetched theme is applied
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode (simulates what happens after applyTheme
fails)
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify API was called to fetch system default theme
+ expect(mockFetch).toHaveBeenCalledWith(
+ expect.stringContaining('/api/v1/theme/'),
+ );
+
+ // Verify the fetched theme was applied via applyThemeWithRecovery
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({ colorPrimary: '#recovery-theme' }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
+});
+
+test('recovery flow: both API fetches fail → falls back to cached default
theme', async () => {
+ // Test: When fetchSystemDefaultTheme fails, fallbackToDefaultMode uses
cached theme
+ // Flow: fallbackToDefaultMode → fetchSystemDefaultTheme (fails) →
applyTheme(cached)
+
+ const controller = createController();
+
+ // Mock fetch to fail for both API endpoints
+ const mockFetch = jest.fn().mockRejectedValue(new Error('Network error'));
+ global.fetch = mockFetch;
+
+ // Track setConfig calls
+ const setConfigCalls: unknown[] = [];
+ mockSetConfig.mockImplementation((config: unknown) => {
+ setConfigCalls.push(config);
+ });
+
+ // Trigger fallbackToDefaultMode
+ await (controller as any).fallbackToDefaultMode();
+
+ // Verify fetch was attempted
+ expect(mockFetch).toHaveBeenCalled();
+
+ // Verify fallback to cached default theme was applied via applyTheme
+ expect(setConfigCalls.length).toBe(1);
+ expect(setConfigCalls[0]).toEqual(
+ expect.objectContaining({
+ token: expect.objectContaining({
+ colorBgBase: '#ededed', // From DEFAULT_THEME in test setup
+ }),
+ }),
+ );
+
+ // Verify controller is in default mode
+ expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
Review Comment:
**Suggestion:** This test also assigns a mock to the global `fetch` function
without restoring the original, so the mocked implementation can persist past
the test boundary and interfere with other tests' use of `fetch`. [resource
leak]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Downstream tests using network fetch fail unpredictably.
- ⚠️ Test suite runs become flaky locally and CI.
- ⚠️ Debug time increases for intermittent failures.
```
</details>
```suggestion
const originalFetch = global.fetch;
global.fetch = mockFetch;
try {
// Track setConfig calls
const setConfigCalls: unknown[] = [];
mockSetConfig.mockImplementation((config: unknown) => {
setConfigCalls.push(config);
});
// Trigger fallbackToDefaultMode
await (controller as any).fallbackToDefaultMode();
// Verify fetch was attempted
expect(mockFetch).toHaveBeenCalled();
// Verify fallback to cached default theme was applied via applyTheme
expect(setConfigCalls.length).toBe(1);
expect(setConfigCalls[0]).toEqual(
expect.objectContaining({
token: expect.objectContaining({
colorBgBase: '#ededed', // From DEFAULT_THEME in test setup
}),
}),
);
// Verify controller is in default mode
expect(controller.getCurrentMode()).toBe(ThemeMode.DEFAULT);
} finally {
global.fetch = originalFetch;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the test file
superset-frontend/src/theme/tests/ThemeController.test.ts with Jest.
2. Execution reaches the failing-fetch test where at
src/theme/tests/ThemeController.test.ts:939 the code assigns global.fetch =
mockFetch
(mock defined at ~937).
3. The test finishes without restoring global.fetch (no teardown in this
test), leaving
the rejected mock on the global object.
4. Other tests executed later in the same Jest worker will encounter the
rejected mock
fetch (observable by inspecting global.fetch after test completion), causing
unrelated
tests that expect network calls or different fetch behavior to fail or
behave incorrectly.
```
</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/tests/ThemeController.test.ts
**Line:** 939:964
**Comment:**
*Resource Leak: This test also assigns a mock to the global `fetch`
function without restoring the original, so the mocked implementation can
persist past the test boundary and interfere with other tests' use of `fetch`.
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:
##########
@@ -143,8 +143,26 @@ export class ThemeController {
// Setup change callback
if (onChange) this.onChangeCallbacks.add(onChange);
- // Apply initial theme and persist mode
- this.applyTheme(initialTheme);
+ // Apply initial theme with recovery for corrupted stored themes
+ try {
+ this.applyTheme(initialTheme);
+ } catch (error) {
+ // Corrupted dev override or CRUD theme in storage - clear and retry
with defaults
+ console.warn(
+ 'Failed to apply stored theme, clearing invalid overrides:',
+ error,
+ );
+ this.devThemeOverride = null;
+ this.crudThemeId = null;
+ this.storage.removeItem(STORAGE_KEYS.DEV_THEME_OVERRIDE);
+ this.storage.removeItem(STORAGE_KEYS.CRUD_THEME_ID);
+ this.storage.removeItem(STORAGE_KEYS.APPLIED_THEME_ID);
+
+ // Retry with clean default theme
+ this.currentMode = ThemeMode.DEFAULT;
+ const safeTheme = this.defaultTheme || {};
+ this.applyTheme(safeTheme);
+ }
this.persistMode();
}
Review Comment:
**Suggestion:** The initial theme application in the constructor directly
calls the low-level applyTheme method and never notifies registered listeners,
so any consumer relying on the onChange callback will not be informed of the
initial theme and can render using stale theme state until a later explicit
update happens. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Components subscribing via onChange won't receive initial theme.
- ⚠️ UI consumers may render with stale theme on first paint.
- ⚠️ Theme-based initialization code may run with wrong tokens.
```
</details>
```suggestion
this.notifyListeners();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the file superset-frontend/src/theme/ThemeController.ts and locate
the constructor
block at lines 143-166 (constructor initialization and initial theme
application). Observe
these lines add an onChange callback and call this.applyTheme(initialTheme)
without
calling notifyListeners().
2. In a running application (or a unit test), construct a ThemeController
instance with an
onChange callback: new ThemeController({ onChange: myCallback }) from
anywhere that
instantiates ThemeController (constructor code is defined at
superset-frontend/src/theme/ThemeController.ts:143-166). The callback is
added at the code
shown in those lines.
3. After the constructor finishes, verify myCallback was not invoked. The
code path in the
constructor (lines 143-166) does not call this.notifyListeners(), so
listeners registered
via the constructor option receive no initial notification.
4. Confirm that later theme changes do call listeners by triggering an
explicit theme
update (for example, call setTemporaryTheme or setTheme which eventually call
updateTheme). You will observe notifyListeners is called from updateTheme,
but the initial
registration in the constructor never produced an initial notification
(constructor block
at superset-frontend/src/theme/ThemeController.ts:143-166). This proves the
missing
notifyListeners() in the constructor prevents onChange consumers from
receiving the
initial theme.
```
</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:** 167:167
**Comment:**
*Logic Error: The initial theme application in the constructor directly
calls the low-level applyTheme method and never notifies registered listeners,
so any consumer relying on the onChange callback will not be informed of the
initial theme and can render using stale theme state until a later explicit
update happens.
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/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:
**Suggestion:** The test for `getAllValidTokenNames` assumes that the
Superset token list contains "fontUrls", but since this token is not actually
part of the `SUPERSET_CUSTOM_TOKENS` set in the implementation, the assertion
will fail, making the test suite inconsistent with real behavior. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Unit test failure blocks CI checks.
- ⚠️ Theme token list validation tests unreliable.
- ⚠️ PRs modifying tokens risk false negatives.
```
</details>
```suggestion
expect(result.supersetTokens).toContain('echartsOptionsOverrides');
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the specific test block: `yarn test
src/theme/utils/antdTokenNames.test.ts` which
runs the "getAllValidTokenNames has reasonable token counts" test (file
lines ~74-92).
2. The test creates `result = getAllValidTokenNames()` and asserts at line
86:
`expect(result.supersetTokens).toContain('fontUrls');`.
3. `getAllValidTokenNames()` is implemented at
superset-frontend/src/theme/utils/antdTokenNames.ts:100-116; it sets
`supersetTokens =
Array.from(SUPERSET_CUSTOM_TOKENS)`. If `SUPERSET_CUSTOM_TOKENS` does not
include
'fontUrls', `result.supersetTokens` will not contain it and the assertion at
line 86 fails
with Jest output like "Expected array to contain 'fontUrls'".
4. Verify the token list by inspecting the `SUPERSET_CUSTOM_TOKENS`
definition in
superset-frontend/src/theme/utils/antdTokenNames.ts. If 'fontUrls' is not
present the test
should be updated to assert an actual token (for example
'echartsOptionsOverrides') or the
token set should be intentionally extended.
```
</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/antdTokenNames.test.ts
**Line:** 86:86
**Comment:**
*Logic Error: The test for `getAllValidTokenNames` assumes that the
Superset token list contains "fontUrls", but since this token is not actually
part of the `SUPERSET_CUSTOM_TOKENS` set in the implementation, the assertion
will fail, making the test suite inconsistent with real behavior.
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]
