codeant-ai-for-open-source[bot] commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2756488037
##########
superset-frontend/src/features/themes/ThemeModal.test.tsx:
##########
@@ -304,10 +357,52 @@ test('validates JSON format and enables save button',
async () => {
const nameInput = screen.getByPlaceholderText('Enter theme name');
await userEvent.type(nameInput, 'Test Theme');
+ await addValidJsonData();
- const saveButton = await screen.findByRole('button', { name: 'Add' });
+ // Wait for validation to complete and button to become enabled
+ await waitFor(
+ () => {
+ const saveButton = screen.getByRole('button', { name: 'Add' });
+ expect(saveButton).toBeEnabled();
+ },
+ { timeout: 10000 },
+ );
+});
+
+test('warnings do not block save - unknown tokens allow save with warnings',
async () => {
+ // First verify the test data actually produces warnings (not errors)
+ const testTheme = {
+ token: { colorPrimary: '#1890ff', unknownTokenName: 'value' },
+ };
+ const validationResult = validateTheme(testTheme);
+ expect(validationResult.valid).toBe(true); // No errors
+ expect(validationResult.warnings.length).toBeGreaterThan(0); // Has warnings
+ expect(validationResult.warnings[0].tokenName).toBe('unknownTokenName');
Review Comment:
**Suggestion:** The test for warning behavior assumes that the first element
in the `warnings` array corresponds to a specific token, which makes the test
brittle if the validator ever changes the order of warnings; instead, it should
assert that *some* warning has the expected token name. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ This unit test may become flaky intermittently.
- ⚠️ CI pipelines show non-deterministic failures.
- ⚠️ Slows developer debugging of validator changes.
```
</details>
```suggestion
expect(
validationResult.warnings.some(
warning => warning.tokenName === 'unknownTokenName',
),
).toBe(true);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open superset-frontend/src/features/themes/ThemeModal.test.tsx and find
the validation
assertions located at ThemeModal.test.tsx:377-380. validateTheme is imported
at
ThemeModal.test.tsx:25.
2. Run only the test 'warnings do not block save - unknown tokens allow save
with
warnings' (the test added starting at ThemeModal.test.tsx:372) using the
test runner
(e.g., yarn test ThemeModal.test.tsx -t "warnings do not block save").
3. validateTheme(testTheme) (ThemeModal.test.tsx:377) returns an object with
warnings
array. If the validator implementation changes the order of warnings (or
accumulates
warnings from different code paths), the exact element at warnings[0] may
not be the
unknownTokenName and the expectation at ThemeModal.test.tsx:380 fails.
4. The failing assertion is a brittle check of a specific index rather than
membership;
replacing it with a .some(...) membership check (as proposed) prevents
order-dependent
flakes.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/themes/ThemeModal.test.tsx
**Line:** 380:380
**Comment:**
*Logic Error: The test for warning behavior assumes that the first
element in the `warnings` array corresponds to a specific token, which makes
the test brittle if the validator ever changes the order of warnings; instead,
it should assert that *some* warning has the expected token name.
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.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:
**Suggestion:** The token `fontWeightStrong` is an Ant Design core design
token but is incorrectly listed as a Superset-specific custom token, so
`isSupersetCustomToken` and `getAllValidTokenNames` will wrongly classify it as
Superset-only instead of antd, which can break any logic that treats custom
tokens differently from native antd tokens. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Theme token classification incorrect during validation.
- ⚠️ ThemeModal may label token as Superset-specific.
- ❌ Token lists returned by getAllValidTokenNames wrong.
- ⚠️ Validation/filtering may drop or mishandle styling token.
```
</details>
```suggestion
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Import and call getAllValidTokenNames() from
superset-frontend/src/theme/utils/antdTokenNames.ts (function defined at
lines 100-115) in
a unit test or small script. This invokes getValidTokenNames() at lines
63-76.
2. getValidTokenNames() (lines 63-76) calls theme.getDesignToken() and
builds a set
containing Ant Design keys plus SUPERSET_CUSTOM_TOKENS
(SUPERSET_CUSTOM_TOKENS declared at
lines 25-31).
3. getAllValidTokenNames() computes antdTokens via
Array.from(allTokens).filter(t =>
!isSupersetCustomToken(t)) (lines 106-108). isSupersetCustomToken() (lines
92-94) returns
true for 'fontWeightStrong' because it is present in SUPERSET_CUSTOM_TOKENS
(line 31).
4. Observe that 'fontWeightStrong' is absent from the returned antdTokens
array and
appears only in supersetTokens. Any consumer relying on antdTokens or
isSupersetCustomToken() (e.g., theme validation / token filtering code that
imports these
functions) will see the token misclassified. The improved change removes
'fontWeightStrong' from SUPERSET_CUSTOM_TOKENS (so isSupersetCustomToken()
will return
false), correcting classification.
```
</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.ts
**Line:** 31:31
**Comment:**
*Logic Error: The token `fontWeightStrong` is an Ant Design core design
token but is incorrectly listed as a Superset-specific custom token, so
`isSupersetCustomToken` and `getAllValidTokenNames` will wrongly classify it as
Superset-only instead of antd, which can break any logic that treats custom
tokens differently from native antd tokens.
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/features/themes/ThemeModal.test.tsx:
##########
@@ -418,6 +513,19 @@ test('saves changes when clicking Save button in unsaved
changes alert', async (
const nameInput = screen.getByPlaceholderText('Enter theme name');
await userEvent.type(nameInput, 'Modified Theme');
+ await addValidJsonData();
+
+ // Wait for validation to complete before canceling
+ await waitFor(
+ () => {
+ const addButton = screen.getByRole('button', { name: 'Add' });
+ expect(addButton).toBeEnabled();
+ },
+ { timeout: 10000 },
+ );
+
+ // Give extra time for all state updates to complete
+ await new Promise(resolve => setTimeout(resolve, 500));
Review Comment:
**Suggestion:** Using a hard-coded 500ms timeout to "give extra time"
introduces unnecessary test delay and potential flakiness, since the test is
not tied to any specific condition and may still race on slower environments;
it is safer to rely solely on explicit waits for the relevant DOM state.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Slows ThemeModal test by 500ms each run.
- ⚠️ Increases CI time across many test runs.
- ⚠️ Can still cause intermittent flakiness on slow runners.
```
</details>
```suggestion
// All necessary state updates are awaited via the previous waitFor, no
extra timeout needed.
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open superset-frontend/src/features/themes/ThemeModal.test.tsx and locate
the
artificial sleep at ThemeModal.test.tsx:527-528 inside the test 'saves
changes when
clicking Save button in unsaved changes alert' (test block beginning at
~ThemeModal.test.tsx:513).
2. Run the test file or full test suite (e.g., yarn test). The test will
always pause for
500ms at ThemeModal.test.tsx:528, adding latency to the run time.
3. On slower CI runners or under load, the arbitrary 500ms may still be
insufficient or
unnecessary; the test still depends on explicit waitFor(...) earlier
(ThemeModal.test.tsx:518-524) and later (ThemeModal.test.tsx:538-545). The
timeout is not
tied to a deterministic DOM condition and can either waste time or produce
flakiness.
4. Removing the hard sleep and relying on explicit waitFor checks (as
suggested) yields
deterministic waits and reduces test duration and flakiness.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/themes/ThemeModal.test.tsx
**Line:** 527:528
**Comment:**
*Possible Bug: Using a hard-coded 500ms timeout to "give extra time"
introduces unnecessary test delay and potential flakiness, since the test is
not tied to any specific condition and may still race on slower environments;
it is safer to rely solely on explicit waits for the relevant DOM state.
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:
##########
@@ -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:
**Suggestion:** The async `fallbackToDefaultMode` method is called without
`await`, and its final `applyTheme` call is not wrapped in a try/catch, so if
applying the fallback theme throws, the resulting rejected Promise will be
unhandled at the call sites, causing unhandled promise rejection errors in the
browser. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unhandled promise rejections in browser console.
- ⚠️ Theme updates produce noisy client errors.
- ⚠️ UI flows (setTheme, setThemeMode) may appear flaky.
```
</details>
```suggestion
try {
this.applyTheme(defaultTheme);
} catch (error) {
console.error('Failed to apply fallback default theme:', error);
return;
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call a public method that triggers theme application without awaiting it
— e.g.,
ThemeController.setTheme(...) which calls this.updateTheme(...) but does not
await the
Promise (setTheme is synchronous and invokes updateTheme directly).
2. Inside updateTheme, an error occurs during applyTheme(...) and the catch
handler runs
await this.fallbackToDefaultMode(); (see updateTheme catch invoking
fallbackToDefaultMode
at superset-frontend/src/theme/ThemeController.ts line ~565).
3. fallbackToDefaultMode is async; its final fallback calls
this.applyTheme(defaultTheme)
synchronously (lines 593-593). If applyTheme throws while
fallbackToDefaultMode is
executing, that exception rejects the fallbackToDefaultMode Promise.
4. Because callers like setTheme do not await updateTheme(), the rejected
Promise from
fallbackToDefaultMode / updateTheme becomes an unhandled promise rejection
in the browser
console. This manifests as uncaught errors and noisy logs during normal UI
flows (theme
set/reset operations).
```
</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:** 593:593
**Comment:**
*Possible Bug: The async `fallbackToDefaultMode` method is called
without `await`, and its final `applyTheme` call is not wrapped in a try/catch,
so if applying the fallback theme throws, the resulting rejected Promise will
be unhandled at the call sites, causing unhandled promise rejection errors in
the browser.
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/features/themes/ThemeModal.test.tsx:
##########
@@ -37,6 +38,27 @@ jest.mock('src/dashboard/util/permissionUtils', () => ({
isUserAdmin: jest.fn(() => true),
}));
+// Mock JsonEditor to avoid direct DOM manipulation in tests
+jest.mock('@superset-ui/core/components/AsyncAceEditor', () => ({
+ ...jest.requireActual('@superset-ui/core/components/AsyncAceEditor'),
+ JsonEditor: ({
+ onChange,
+ value,
+ readOnly,
+ }: {
+ onChange: (value: string) => void;
+ value: string;
+ readOnly?: boolean;
+ }) => (
+ <textarea
+ data-test="json-editor"
Review Comment:
**Suggestion:** The mocked JsonEditor component sets a `data-test`
attribute, but the helper functions use `screen.getByTestId('json-editor')`,
which queries `data-testid`; this mismatch will cause all helpers using
`getByTestId` to throw because the element cannot be found. [possible bug]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ ThemeModal unit tests fail in CI.
- ❌ Local test runs error immediately locating editor.
- ⚠️ Blocks related theme modal test coverage verification.
```
</details>
```suggestion
data-testid="json-editor"
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file
superset-frontend/src/features/themes/ThemeModal.test.tsx and locate
the mocked JsonEditor definition at ThemeModal.test.tsx:44-59. The mock
returns a
<textarea data-test="json-editor"> (lines 53-55 in the PR diff).
2. Run the test suite (e.g., yarn test ThemeModal.test.tsx). Multiple helper
functions in
the same file call screen.getByTestId('json-editor') — see addValidJsonData
at
ThemeModal.test.tsx:116-125 which executes screen.getByTestId('json-editor')
at
ThemeModal.test.tsx:122.
3. When addValidJsonData (ThemeModal.test.tsx:116) or addJsonWithUnknownToken
(ThemeModal.test.tsx:128) calls screen.getByTestId('json-editor')
(ThemeModal.test.tsx:122
/ 135), testing-library will throw because no element with
data-testid="json-editor"
exists (the mock set data-test instead).
4. The test run fails immediately with a getByTestId error (element not
found). The fix is
to change the mock to use data-testid (as proposed) so screen.getByTestId
resolves to the
mocked textarea.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset-frontend/src/features/themes/ThemeModal.test.tsx
**Line:** 54:54
**Comment:**
*Possible Bug: The mocked JsonEditor component sets a `data-test`
attribute, but the helper functions use `screen.getByTestId('json-editor')`,
which queries `data-testid`; this mismatch will cause all helpers using
`getByTestId` to throw because the element cannot be found.
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,206 @@ 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();
Review Comment:
**Suggestion:** This test creates a second spy on `console.warn` and then
restores it, which overrides and effectively disables the shared `consoleSpy`
spy defined at the top of the file for all subsequent tests; this can cause
later tests that rely on `consoleSpy` to silently stop capturing warnings or
assertions to fail unexpectedly. Use the existing `consoleSpy` instead of
creating and restoring a new spy within this test. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ ThemeController.test.ts assertions may silently stop capturing warnings.
- ⚠️ CI runs for theme tests may become flaky or fail.
- ⚠️ Local developers get confusing test failures.
```
</details>
```suggestion
// Should not throw - constructor should recover
const controller = createController();
// Verify recovery happened
expect(consoleSpy).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);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file at
superset-frontend/src/theme/tests/ThemeController.test.ts and
locate the shared spy definition created near the top of the file (the
shared spy is
declared as `const consoleSpy = jest.spyOn(console,
'warn').mockImplementation();` defined
immediately after the `createController` helper — see createController at
`.../ThemeController.test.ts:106-112` in the repository context). This
shared spy is
expected to capture all console.warn calls across tests.
2. Run the Jest test runner for this file (e.g., `yarn test
ThemeController.test.ts` or
`npm run test -- ThemeController.test.ts`). Jest will execute tests in file
order. When
execution reaches the test "ThemeController constructor recovers from
corrupted stored
theme" (the test block added between lines `836` and `887` in the PR), the
test executes
the code at `.../ThemeController.test.ts:861` which calls
`jest.spyOn(console, 'warn')`
again, creating a second spy (local variable `consoleWarnSpy`).
3. Later in that same test, the code calls `consoleWarnSpy.mockRestore()`
(line `886`).
mockRestore() restores console.warn to the original implementation, which
has the side
effect of removing the previously-installed shared spy (`consoleSpy`). This
happens while
other tests in the same file are still scheduled to run.
4. After this restoration, any subsequent tests that assert on or rely upon
the shared
`consoleSpy` (the spy created at file top) will not capture warnings. This
produces
missing spy calls and can cause downstream assertions to fail or tests to
appear flaky.
Replacing the local spy usage with assertions against the shared
`consoleSpy` (or avoiding
mockRestore here) prevents the shared spy from being disabled and reproduces
a passing
test run.
```
</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:** 861:886
**Comment:**
*Logic Error: This test creates a second spy on `console.warn` and then
restores it, which overrides and effectively disables the shared `consoleSpy`
spy defined at the top of the file for all subsequent tests; this can cause
later tests that rely on `consoleSpy` to silently stop capturing warnings or
assertions to fail unexpectedly. Use the existing `consoleSpy` instead of
creating and restoring a new spy within this test.
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:
##########
@@ -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:
**Suggestion:** In the final fallback path, using the mode-based resolver
can still return the development override instead of a clean default theme, so
if the dev override was the source of the error, the controller will reapply
the same broken theme instead of a safe default, defeating the recovery
mechanism. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Theme recovery may reapply corrupted dev overrides.
- ⚠️ ThemeModal preview shows broken themes after recovery.
- ⚠️ Admins cannot reliably reset to safe default themes.
```
</details>
```suggestion
const defaultTheme: AnyThemeConfig = this.defaultTheme || {};
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the running app, create a corrupted development override by calling
ThemeController.setTemporaryTheme(...) which sets this.devThemeOverride and
persists it
(method present in superset-frontend/src/theme/ThemeController.ts; the
implementation sets
this.devThemeOverride and then calls this.updateTheme(...)). Observe a
broken theme is
stored in memory and storage.
2. Trigger a runtime failure when the app later tries to apply themes (for
example, an
invalid token causes applyTheme to throw inside ThemeController.applyTheme
at the apply
step — see applyTheme re-throw behavior around the applyTheme method in this
file).
3. updateTheme catches the error and calls fallbackToDefaultMode via await
this.fallbackToDefaultMode(); (see updateTheme catch at
superset-frontend/src/theme/ThemeController.ts line ~565 where
fallbackToDefaultMode is
invoked).
4. Inside fallbackToDefaultMode the "final fallback" resolves defaultTheme
using
this.getThemeForMode(ThemeMode.DEFAULT) (lines 591-592). Because
getThemeForMode gives
priority to this.devThemeOverride (it returns the dev override whenever
present), the
controller will reapply the same corrupted devThemeOverride instead of a
clean built-in
default, defeating recovery.
Note: This reproduces with normal usage of the ThemeController APIs
(setTemporaryTheme ->
later theme application error -> fallback flow). The root cause is
getThemeForMode
precedence combined with not clearing devThemeOverride before the final
fallback
selection.
```
</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:** 591:592
**Comment:**
*Logic Error: In the final fallback path, using the mode-based resolver
can still return the development override instead of a clean default theme, so
if the dev override was the source of the error, the controller will reapply
the same broken theme instead of a safe default, defeating the recovery
mechanism.
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]
