feat(themes): add enhanced validation and error handling with fallback mechanisms #37378
+1,317
−62
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Code Review Agent Run #5f8c7cActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
dosubot
bot
added
change:frontend
Member
Author
|
Opening this PR to fix merge conflicts within #35349 |
Member
Author
|
This PR has already been tested and has passed qa |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const normalizedConfig = normalizeThemeConfig(theme); | ||
| this.globalTheme.setConfig(normalizedConfig); | ||
| } catch (error) { | ||
| await this.fallbackToDefaultMode(); |
Contributor
There was a problem hiding this comment.
Suggestion: applyThemeWithRecovery catches errors and calls fallbackToDefaultMode, but fallbackToDefaultMode itself calls applyThemeWithRecovery for a fetched fresh theme — this creates possible infinite recursion (fallback -> applyThemeWithRecovery -> fallback -> ...). Instead propagate the error to the caller so the caller (which already has a fallback try/catch) can handle it without recursive re-entry. [logic error]
Severity Level: Critical 🚨
- ❌ Theme recovery can recurse causing stack overflow.
- ❌ UI may become unresponsive during recovery attempts.
- ⚠️ Initial theme application and fallback paths affected.
- ⚠️ Error handling logic loops on invalid server themes.
Suggested change
| await this.fallbackToDefaultMode(); | |
| // Propagate error to caller so caller's fallback logic can decide next steps | |
| throw error; |
Steps of Reproduction ✅
1. Open superset-frontend/src/theme/ThemeController.ts and locate:
- fallbackToDefaultMode (starts around line 555) which, when it finds a fresh system
theme, calls `await this.applyThemeWithRecovery(freshSystemTheme);` (inside a
try/catch).
- applyThemeWithRecovery defined at lines 811-817.
2. Trigger a theme application failure so updateTheme's catch runs and calls
fallbackToDefaultMode (updateTheme catch at line ~546 -> calls fallbackToDefaultMode).
3. fallbackToDefaultMode fetches a fresh system theme (fetchSystemDefaultTheme at line
954). If the fetched theme is invalid in the same way (e.g., malformed tokens),
applyThemeWithRecovery will catch and call fallbackToDefaultMode again.
4. This produces a recursive cycle: fallbackToDefaultMode -> applyThemeWithRecovery ->
fallbackToDefaultMode -> ... until call stack exhaustion or synchronous crash. The
recursion is concrete because fallbackToDefaultMode explicitly calls
applyThemeWithRecovery (lines ~561-566) and applyThemeWithRecovery currently calls
fallbackToDefaultMode on error (lines 814-816).
5. Result: reproduceable stack overflow / unresponsive UI when a server-provided system
theme is invalid or causes the same error as the active theme.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/theme/ThemeController.ts
**Line:** 816:816
**Comment:**
*Logic Error: `applyThemeWithRecovery` catches errors and calls `fallbackToDefaultMode`, but `fallbackToDefaultMode` itself calls `applyThemeWithRecovery` for a fetched fresh theme — this creates possible infinite recursion (fallback -> applyThemeWithRecovery -> fallback -> ...). Instead propagate the error to the caller so the caller (which already has a fallback try/catch) can handle it without recursive re-entry.
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.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enhances the theming system by adding structured theme validation (including token name checks), integrating that validation into the ThemeModal UX, and introducing more robust runtime fallback behavior in the ThemeController when theme application fails.
Changes:
- Introduces
validateThemeand Ant Design token introspection utilities to validate theme structure and token names, with clear separation between blocking errors and non‑blocking warnings. - Adds a reusable
useThemeValidationhook and wires it intoThemeModalto surface live JSON/token validation and to gate save/apply actions on structural correctness. - Extends
ThemeControllerwith more robust error-handling and fallback behavior, including fetching a system default theme from the API when theme application fails, and updates tests and modal behavior accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
superset-frontend/src/theme/utils/themeStructureValidation.ts |
Adds validateTheme to enforce basic theme structure (non-empty, object config) and flag unknown/null tokens as warnings. |
superset-frontend/src/theme/utils/themeStructureValidation.test.ts |
Provides unit tests for validateTheme, covering valid themes, empty/invalid configs, null tokens, and warning aggregation. |
superset-frontend/src/theme/utils/antdTokenNames.ts |
Introduces utilities to derive the set of valid Ant Design design tokens at runtime, extend them with Superset-specific tokens, and expose validation and introspection helpers. |
superset-frontend/src/theme/utils/antdTokenNames.test.ts |
Tests token-name utilities, ensuring known AntD and Superset tokens are recognized, unknown names are rejected, and counts/categories look reasonable. |
superset-frontend/src/theme/hooks/useThemeValidation.ts |
Implements a hook that combines JSON syntax validation with validateTheme structure/token checks, producing Ace-style error/warning annotations and error/warning flags. |
superset-frontend/src/theme/hooks/useThemeValidation.test.ts |
Adds tests verifying that useThemeValidation correctly flags valid themes, unknown tokens, empty themes, invalid JSON, custom tokens, and respects the enabled option. |
superset-frontend/src/theme/ThemeController.ts |
Refactors theme update/fallback logic to call the CRUD fetch helper when building dashboard themes, makes updateTheme/fallbackToDefaultMode async, adds applyThemeWithRecovery, and introduces fetchSystemDefaultTheme to pull a system default theme from /api/v1/theme on failure. |
superset-frontend/src/features/themes/ThemeModal.tsx |
Replaces ad-hoc JSON validation with useThemeValidation, updates save/apply enablement to depend on structural errors (not warnings), and enhances the JSON configuration helper text to mention warning highlighting. |
superset-frontend/src/features/themes/ThemeModal.test.tsx |
Updates ThemeModal tests to work with the new validation behavior (including debouncing), mocks the JSON editor for easier interaction, and adds waits/assertions that buttons enable only once validation passes. |
Comments suppressed due to low confidence (1)
superset-frontend/src/theme/ThemeController.ts:579
- The new recovery path in
fallbackToDefaultMode/fetchSystemDefaultThemethat fetches a system default theme from/api/v1/themeand applies it viaapplyThemeWithRecoveryis not covered by the existingThemeControllertests, which currently focus on bootstrap data and direct theme application/error handling. Given the additional network fetch, multi-step fallbacks, and new recovery behavior, adding unit tests that exercise success, "no system default" and error scenarios would help prevent regressions in this critical path.
private async fallbackToDefaultMode(): Promise<void> {
this.currentMode = ThemeMode.DEFAULT;
// 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 || {};
this.applyTheme(defaultTheme);
this.persistMode();
this.notifyListeners();
}
Comment on lines
73
to
75
| const tokens = themeConfig.token || {}; | ||
| Object.entries(tokens).forEach(([name, value]) => { | ||
| // Null/undefined check |
There was a problem hiding this comment.
validateTheme assumes themeConfig.token is an object and passes it directly to Object.entries; if a user-supplied theme JSON contains a non-object token value (e.g., string or array), this will throw at runtime. Because useThemeValidation wraps validateTheme in a generic try/catch and returns no annotations on any exception, such invalid themes would silently bypass structural validation instead of surfacing a clear error. It would be safer to guard the type of themeConfig.token and treat non-object values as a structural error on _root before iterating.
Comment on lines
+955
to
+1003
| try { | ||
| // Try to fetch theme marked as system default (is_system_default=true) | ||
| const defaultResponse = await fetch( | ||
| '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', | ||
| ); | ||
| if (defaultResponse.ok) { | ||
| const data = await defaultResponse.json(); | ||
| if (data.result?.length > 0) { | ||
| const themeConfig = JSON.parse(data.result[0].json_data); | ||
| if (themeConfig && typeof themeConfig === 'object') { | ||
| return themeConfig; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: Try to fetch system theme named 'THEME_DEFAULT' | ||
| const fallbackResponse = await fetch( | ||
| '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', | ||
| ); | ||
| if (fallbackResponse.ok) { | ||
| const fallbackData = await fallbackResponse.json(); |
There was a problem hiding this comment.
fetchSystemDefaultTheme uses the global fetch API directly against /api/v1/theme/ while other theme-related frontend calls (for example superset-frontend/src/pages/ThemeList/index.tsx:152-195 and fetchCrudTheme in this file) consistently use SupersetClient/makeApi. For consistency with the rest of the codebase and to centralize authentication, error handling, and base URL concerns, consider refactoring this method to use SupersetClient/makeApi as well.
Comment on lines
811
to
817
| private async applyThemeWithRecovery(theme: AnyThemeConfig): Promise<void> { | ||
| try { | ||
| const normalizedConfig = normalizeThemeConfig(theme); | ||
| this.globalTheme.setConfig(normalizedConfig); | ||
| } catch (error) { | ||
| await this.fallbackToDefaultMode(); | ||
| } |
There was a problem hiding this comment.
applyThemeWithRecovery calls fallbackToDefaultMode in its catch block, while fallbackToDefaultMode itself calls applyThemeWithRecovery when a fresh system default theme is found. This mutual dependency can lead to unbounded recursion if the fetched system theme consistently throws during normalization or setConfig, causing repeated network calls and potential stack overflows instead of falling back to the built-in default theme. Consider restructuring the error paths so that the recovery logic cannot re-enter fallbackToDefaultMode recursively (for example by letting applyThemeWithRecovery rethrow here, or by having the fallback path use applyTheme and the cached/built-in default theme instead of calling fallbackToDefaultMode again).
| private async applyThemeWithRecovery(theme: AnyThemeConfig): Promise<void> { | ||
| try { | ||
| const normalizedConfig = normalizeThemeConfig(theme); | ||
| this.globalTheme.setConfig(normalizedConfig); |
There was a problem hiding this comment.
applyTheme loads custom fonts via loadFonts but applyThemeWithRecovery does not, so when fallbackToDefaultMode applies a freshly fetched system default theme through applyThemeWithRecovery, any fontUrls in that theme will be ignored and the fonts will not be loaded. This makes the recovery path behave differently from normal theme application and may result in inconsistent typography after a fallback. To keep behavior consistent, consider either reusing applyTheme in the recovery path or adding the same font-loading logic to applyThemeWithRecovery.
Suggested change
| this.globalTheme.setConfig(normalizedConfig); | |
| this.globalTheme.setConfig(normalizedConfig); | |
| // Load custom fonts if specified in theme config, mirroring applyTheme() | |
| const fontUrls = (normalizedConfig?.token as Record<string, unknown>) | |
| ?.fontUrls as string[] | undefined; | |
| this.loadFonts(fontUrls); |
Contributor
Code Review Agent Run #b29282Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| this.applyTheme(safeTheme); | ||
| } | ||
| this.persistMode(); | ||
| } |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ 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.
Suggested change
| } | |
| this.notifyListeners(); |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
+834
to
+840
| 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); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Recovery failures produce limited diagnostic logs.
- ⚠️ Theme recovery debugging is harder for engineers.
- ⚠️ Runtime theme fallback may proceed silently.
Suggested change
| 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); | |
| 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; | |
| } |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
903
to
928
| 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); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ 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.
Suggested change
| 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); | |
| 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; | |
| } |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
939
to
964
| 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); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ Downstream tests using network fetch fail unpredictably.
- ⚠️ Test suite runs become flaky locally and CI.
- ⚠️ Debug time increases for intermittent failures.
Suggested change
| 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); | |
| 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; | |
| } |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
981
to
1018
| 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); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ Unrelated tests that call fetch fail intermittently.
- ⚠️ CI test runs may become flaky and unreliable.
- ⚠️ Debugging intermittent test failures increases.
Suggested change
| 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); | |
| 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; | |
| } |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.| expect(isSupersetCustomToken('brandLogoUrl')).toBe(true); | ||
| expect(isSupersetCustomToken('brandSpinnerSvg')).toBe(true); | ||
| expect(isSupersetCustomToken('fontSizeXS')).toBe(true); | ||
| expect(isSupersetCustomToken('fontUrls')).toBe(true); |
Contributor
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ Unit test failure blocks CI checks.
- ⚠️ Theme token utilities tests unreliable.
- ⚠️ Developers hit failing tests locally.
Suggested change
| expect(isSupersetCustomToken('fontUrls')).toBe(true); | |
| expect(isSupersetCustomToken('echartsOptionsOverrides')).toBe(true); |
Steps of Reproduction ✅
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'.Prompt for AI Agent 🤖
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.| // Superset custom tokens should exist | ||
| expect(result.supersetTokens.length).toBeGreaterThan(0); | ||
| expect(result.supersetTokens).toContain('brandLogoUrl'); | ||
| expect(result.supersetTokens).toContain('fontUrls'); |
Contributor
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ Unit test failure blocks CI checks.
- ⚠️ Theme token list validation tests unreliable.
- ⚠️ PRs modifying tokens risk false negatives.
Suggested change
| expect(result.supersetTokens).toContain('fontUrls'); | |
| expect(result.supersetTokens).toContain('echartsOptionsOverrides'); |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Contributor
Code Review Agent Run #12dfcfActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
sadpandajoe
force-pushed
the
joe/feat-theme-error-handling
branch
from
c415df8 to
eca140a
Compare
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review Agent Run #ad4e65
Actionable Suggestions - 2
-
superset-frontend/src/theme/ThemeController.ts - 2
- Unhandled error in constructor · Line 164-164
- Inconsistent auth in API calls · Line 982-1016
Additional Suggestions - 2
-
superset-frontend/src/theme/utils/antdTokenNames.test.ts - 1
-
Incorrect Test Expectation · Line 28-28The test expects 'padding' to be a valid Ant Design token, but official antd v5.26.0 documentation confirms 'padding' is not a global design token in the token object returned by theme.getDesignToken(). This will cause test failures. Remove the assertions for 'padding' in both test cases (lines 28 and 107).
-
-
superset-frontend/src/theme/ThemeController.ts - 1
-
Unnecessary async · Line 829-841applyThemeWithRecovery is marked async but contains no await calls, unnecessarily complicating the interface.
-
Review Details
-
Files reviewed - 10 · Commit Range:
675d87b..eca140a- superset-frontend/src/features/themes/ThemeModal.test.tsx
- superset-frontend/src/features/themes/ThemeModal.tsx
- superset-frontend/src/theme/ThemeController.ts
- superset-frontend/src/theme/hooks/useThemeValidation.test.ts
- superset-frontend/src/theme/hooks/useThemeValidation.ts
- superset-frontend/src/theme/tests/ThemeController.test.ts
- superset-frontend/src/theme/utils/antdTokenNames.test.ts
- superset-frontend/src/theme/utils/antdTokenNames.ts
- superset-frontend/src/theme/utils/themeStructureValidation.test.ts
- superset-frontend/src/theme/utils/themeStructureValidation.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
| // Retry with clean default theme | ||
| this.currentMode = ThemeMode.DEFAULT; | ||
| const safeTheme = this.defaultTheme || {}; | ||
| this.applyTheme(safeTheme); |
Contributor
There was a problem hiding this comment.
Unhandled error in constructor
If the retry theme application fails in the constructor, an unhandled error is thrown, potentially breaking initialization.
Code Review Run #ad4e65
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Comment on lines
982
to
1017
| private async fetchSystemDefaultTheme(): Promise<AnyThemeConfig | null> { | ||
| try { | ||
| // Try to fetch theme marked as system default (is_system_default=true) | ||
| const defaultResponse = await fetch( | ||
| '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', | ||
| ); | ||
| if (defaultResponse.ok) { | ||
| const data = await defaultResponse.json(); | ||
| if (data.result?.length > 0) { | ||
| const themeConfig = JSON.parse(data.result[0].json_data); | ||
| if (themeConfig && typeof themeConfig === 'object') { | ||
| return themeConfig; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: Try to fetch system theme named 'THEME_DEFAULT' | ||
| const fallbackResponse = await fetch( | ||
| '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', | ||
| ); | ||
| if (fallbackResponse.ok) { | ||
| const fallbackData = await fallbackResponse.json(); | ||
| if (fallbackData.result?.length > 0) { | ||
| const themeConfig = JSON.parse(fallbackData.result[0].json_data); | ||
| if (themeConfig && typeof themeConfig === 'object') { | ||
| return themeConfig; | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Silently handle fetch errors | ||
| } | ||
|
|
||
| return null; | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent auth in API calls
fetchSystemDefaultTheme uses unauthenticated fetch while other theme API calls use makeApi, potentially causing auth failures for protected endpoints.
Code Review Run #ad4e65
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| }, | ||
| { timeout: 15000 }, | ||
| ); | ||
| }, 30000); |
There was a problem hiding this comment.
This test has been given an extended timeout of 30 seconds, which is unusually long for a unit test. This suggests the test may be testing too many asynchronous operations or waiting unnecessarily. Consider breaking this into smaller, more focused tests or reducing the timeout if the extended duration is not actually needed.
Comment on lines
983
to
1016
| try { | ||
| // Try to fetch theme marked as system default (is_system_default=true) | ||
| const defaultResponse = await fetch( | ||
| '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', | ||
| ); | ||
| if (defaultResponse.ok) { | ||
| const data = await defaultResponse.json(); | ||
| if (data.result?.length > 0) { | ||
| const themeConfig = JSON.parse(data.result[0].json_data); | ||
| if (themeConfig && typeof themeConfig === 'object') { | ||
| return themeConfig; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Fallback: Try to fetch system theme named 'THEME_DEFAULT' | ||
| const fallbackResponse = await fetch( | ||
| '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', | ||
| ); | ||
| if (fallbackResponse.ok) { | ||
| const fallbackData = await fallbackResponse.json(); | ||
| if (fallbackData.result?.length > 0) { | ||
| const themeConfig = JSON.parse(fallbackData.result[0].json_data); | ||
| if (themeConfig && typeof themeConfig === 'object') { | ||
| return themeConfig; | ||
| } | ||
| } | ||
| } | ||
| } catch (error) { | ||
| // Silently handle fetch errors | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
The comment says 'Silently handle fetch errors' but there's no actual error handling or logging in the catch block. Consider adding console.error() or another logging mechanism to help with debugging when theme fetching fails, or update the comment to reflect that errors are truly being suppressed without any tracking.
Suggested change
| try { | |
| // Try to fetch theme marked as system default (is_system_default=true) | |
| const defaultResponse = await fetch( | |
| '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', | |
| ); | |
| if (defaultResponse.ok) { | |
| const data = await defaultResponse.json(); | |
| if (data.result?.length > 0) { | |
| const themeConfig = JSON.parse(data.result[0].json_data); | |
| if (themeConfig && typeof themeConfig === 'object') { | |
| return themeConfig; | |
| } | |
| } | |
| } | |
| // Fallback: Try to fetch system theme named 'THEME_DEFAULT' | |
| const fallbackResponse = await fetch( | |
| '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', | |
| ); | |
| if (fallbackResponse.ok) { | |
| const fallbackData = await fallbackResponse.json(); | |
| if (fallbackData.result?.length > 0) { | |
| const themeConfig = JSON.parse(fallbackData.result[0].json_data); | |
| if (themeConfig && typeof themeConfig === 'object') { | |
| return themeConfig; | |
| } | |
| } | |
| } | |
| } catch (error) { | |
| // Silently handle fetch errors | |
| } | |
| return null; | |
| try { | |
| // Try to fetch theme marked as system default (is_system_default=true) | |
| const defaultResponse = await fetch( | |
| '/api/v1/theme/?q=(filters:!((col:is_system_default,opr:eq,value:!t)))', | |
| ); | |
| if (defaultResponse.ok) { | |
| const data = await defaultResponse.json(); | |
| if (data.result?.length > 0) { | |
| const themeConfig = JSON.parse(data.result[0].json_data); | |
| if (themeConfig && typeof themeConfig === 'object') { | |
| return themeConfig; | |
| } | |
| } | |
| } | |
| // Fallback: Try to fetch system theme named 'THEME_DEFAULT' | |
| const fallbackResponse = await fetch( | |
| '/api/v1/theme/?q=(filters:!((col:theme_name,opr:eq,value:THEME_DEFAULT),(col:is_system,opr:eq,value:!t)))', | |
| ); | |
| if (fallbackResponse.ok) { | |
| const fallbackData = await fallbackResponse.json(); | |
| if (fallbackData.result?.length > 0) { | |
| const themeConfig = JSON.parse(fallbackData.result[0].json_data); | |
| if (themeConfig && typeof themeConfig === 'object') { | |
| return themeConfig; | |
| } | |
| } | |
| } | |
| } catch (error) { | |
| // Handle fetch errors without throwing, but log for debugging purposes | |
| console.error('Failed to fetch system default theme:', error); | |
| } | |
| return null; |
| readOnly?: boolean; | ||
| }) => ( | ||
| <textarea | ||
| data-test="json-editor" |
Contributor
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ ThemeModal unit tests fail in CI.
- ❌ Local test runs error immediately locating editor.
- ⚠️ Blocks related theme modal test coverage verification.
Suggested change
| data-test="json-editor" | |
| data-testid="json-editor" |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.| 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'); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ This unit test may become flaky intermittently.
- ⚠️ CI pipelines show non-deterministic failures.
- ⚠️ Slows developer debugging of validator changes.
Suggested change
| expect(validationResult.warnings[0].tokenName).toBe('unknownTokenName'); | |
| expect( | |
| validationResult.warnings.some( | |
| warning => warning.tokenName === 'unknownTokenName', | |
| ), | |
| ).toBe(true); |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
+527
to
+528
| // Give extra time for all state updates to complete | ||
| await new Promise(resolve => setTimeout(resolve, 500)); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ⚠️ Slows ThemeModal test by 500ms each run.
- ⚠️ Increases CI time across many test runs.
- ⚠️ Can still cause intermittent flakiness on slow runners.
Suggested change
| // Give extra time for all state updates to complete | |
| await new Promise(resolve => setTimeout(resolve, 500)); | |
| // All necessary state updates are awaited via the previous waitFor, no extra timeout needed. |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
+861
to
+886
| 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(); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ 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.
Suggested change
| 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(); | |
| // 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); |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.| 'fontSizeXXL', | ||
| 'fontWeightNormal', | ||
| 'fontWeightLight', | ||
| 'fontWeightStrong', |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ 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.
Suggested change
| 'fontWeightStrong', |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Comment on lines
591
to
592
| const defaultTheme: AnyThemeConfig = | ||
| this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme || {}; |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ Theme recovery may reapply corrupted dev overrides.
- ⚠️ ThemeModal preview shows broken themes after recovery.
- ⚠️ Admins cannot reliably reset to safe default themes.
Suggested change
| const defaultTheme: AnyThemeConfig = | |
| this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme || {}; | |
| const defaultTheme: AnyThemeConfig = this.defaultTheme || {}; |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.| // Final fallback: use cached default theme or built-in theme | ||
| const defaultTheme: AnyThemeConfig = | ||
| this.getThemeForMode(ThemeMode.DEFAULT) || this.defaultTheme || {}; | ||
|
|
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ Unhandled promise rejections in browser console.
- ⚠️ Theme updates produce noisy client errors.
- ⚠️ UI flows (setTheme, setThemeMode) may appear flaky.
Suggested change
| try { | |
| this.applyTheme(defaultTheme); | |
| } catch (error) { | |
| console.error('Failed to apply fallback default theme:', error); | |
| return; | |
| } |
Steps of Reproduction ✅
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).Prompt for AI Agent 🤖
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.
Contributor
Code Review Agent Run #868be5Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
…dates Fixed 5 CI test failures caused by race conditions when checking button state and API call status: 1. "enables save button when theme name is entered" - Wait for button to enable after JSON validation completes 2. "validates JSON format and enables save button" - Same async validation wait 3. "saves changes when clicking Save button in unsaved changes alert" - Wait for fetch mock to register the API call 4. "creates new theme when saving" - Wait for POST request to be tracked 5. "handles API errors gracefully" - Wait for button state after validation All tests now use waitFor() to handle async state updates that may complete slower in CI environments than locally. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Increased timeout from default 1s to 10s for tests that check if the save button becomes enabled after form validation. In slower CI environments, the validation useEffect may take longer to run after userEvent.type() completes. Also refactored to query the button inside waitFor callback rather than before, ensuring we're checking the latest DOM state. Fixes 5 flaky test failures in CI: - enables save button when theme name is entered - validates JSON format and enables save button - handles API errors gracefully 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
…ests Fixed remaining 3 CI test failures by ensuring form validation completes before attempting save actions: 1. "saves changes when clicking Save button in unsaved changes alert" - Added waitFor to check button is enabled before clicking Cancel - Ensures validation is complete before triggering unsaved changes alert 2. "creates new theme when saving" - Wait for button to be enabled before clicking save - Previously was clicking immediately after finding the button 3. "handles API errors gracefully" - Added waitFor around fetchMock.called() check - API call may happen slightly after dialog appears All tests now consistently wait for async validation to complete before proceeding with actions, preventing race conditions in CI. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
ROOT CAUSE: userEvent.type() appends text instead of replacing it. The addValidJsonData() helper was appending JSON to existing textarea content, creating invalid JSON that failed validation, causing save buttons to remain disabled indefinitely. FIX: Added userEvent.clear() before userEvent.type() in addValidJsonData() helper to ensure textarea is empty before adding valid JSON. ADDITIONAL FIXES: - Added waitFor() for Save button in unsaved changes alert to be enabled - Added 500ms delay after validation to ensure state updates complete - Increased timeouts for fetchMock.called() checks to handle CI timing - Increased test timeout to 30s for complex async flows All 22 tests now pass consistently. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Fix import path from non-existent '@superset-ui/core/theme/types' to '@apache-superset/core/ui' where types are actually exported - Fix type errors in antdTokenNames.ts: - Change SUPERSET_CUSTOM_TOKENS to Set<string> to avoid keyof union type issues - Remove unused SupersetSpecificTokens import - Add type assertion for return value to satisfy strict type checking Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Fix infinite recursion between applyThemeWithRecovery and fallbackToDefaultMode by removing try/catch and letting errors propagate to caller (FB-1/FB-4) - Add font loading to applyThemeWithRecovery to match applyTheme behavior for consistent typography after fallback (FB-5) - Add type guard for themeConfig.token to handle non-object values like arrays or strings gracefully (FB-2) - Add comment explaining why fetchSystemDefaultTheme uses raw fetch instead of SupersetClient (FB-3 - architectural constraint) - Add tests for new token type validation Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add try/catch in ThemeController constructor to recover from corrupted localStorage themes (dev override, CRUD theme) - On startup failure: clear invalid overrides and fall back to default theme instead of crashing the app - Add fontUrls to SUPERSET_CUSTOM_TOKENS to prevent false warnings for valid font loading configurations - Add test for constructor recovery behavior Co-Authored-By: Claude Opus 4.5 <[email protected]>
…g test - Replace brittle token count assertions (> 400) with presence checks for known tokens to avoid failures on Ant Design upgrades - Add test verifying warnings (unknown tokens) do not block save in ThemeModal - ensures error/warning distinction works correctly Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Add test verifying fallbackToDefaultMode fetches system default theme from API when theme application fails - Add test verifying fallback to cached default when API fetch fails - Covers the full recovery flow: applyTheme failure → API fetch → cached/built-in default Co-Authored-By: Claude Opus 4.5 <[email protected]>
Tests for the full recovery flow in fallbackToDefaultMode: 1. API returns theme → applies fetched theme via applyThemeWithRecovery 2. Both API fetches fail → falls back to cached default theme 3. Fetched theme fails to apply → falls back to cached default theme Each test verifies: - API fetch is called with correct endpoint - The correct theme config is passed to setConfig - Controller ends up in DEFAULT mode Co-Authored-By: Claude Opus 4.5 <[email protected]>
- FB-12: Restore global.fetch after tests to prevent test pollution - FB-15: Add console.warn for fetchSystemDefaultTheme errors - FB-8: Remove redundant type assertion in antdTokenNames.ts - FB-9: Add warning verification to ThemeModal test Co-Authored-By: Claude Opus 4.5 <[email protected]>
sadpandajoe
force-pushed
the
joe/feat-theme-error-handling
branch
from
7463b94 to
e54eb29
Compare
Contributor
There was a problem hiding this comment.
Code Review Agent Run #484b31
Actionable Suggestions - 3
-
superset-frontend/src/theme/utils/themeStructureValidation.ts - 1
- Missing Components Structure Validation · Line 88-90
-
superset-frontend/src/theme/ThemeController.ts - 2
- Async method called without await · Line 549-549
- Async fallback not awaited · Line 573-573
Review Details
-
Files reviewed - 10 · Commit Range:
021cd55..e54eb29- superset-frontend/src/features/themes/ThemeModal.test.tsx
- superset-frontend/src/features/themes/ThemeModal.tsx
- superset-frontend/src/theme/ThemeController.ts
- superset-frontend/src/theme/hooks/useThemeValidation.test.ts
- superset-frontend/src/theme/hooks/useThemeValidation.ts
- superset-frontend/src/theme/tests/ThemeController.test.ts
- superset-frontend/src/theme/utils/antdTokenNames.test.ts
- superset-frontend/src/theme/utils/antdTokenNames.ts
- superset-frontend/src/theme/utils/themeStructureValidation.test.ts
- superset-frontend/src/theme/utils/themeStructureValidation.ts
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at [email protected].
Documentation & Help
AI Code Review powered by
Comment on lines
+88
to
+90
| } | ||
|
|
||
| Object.entries(tokens).forEach(([name, value]) => { |
Contributor
There was a problem hiding this comment.
Missing Components Structure Validation
The validation checks token configuration structure but lacks the same guard for components, which could lead to inconsistent error handling and potential runtime issues if components is not an object. Add a guard after the token validation to ensure components is a valid object.
Code suggestion
Check the AI-generated fix before applying
Suggested change
| } | |
| Object.entries(tokens).forEach(([name, value]) => { | |
| } | |
| // Guard against non-object components values | |
| const rawComponents = themeConfig.components; | |
| if (rawComponents && (typeof rawComponents !== 'object' || Array.isArray(rawComponents))) { | |
| errors.push({ | |
| tokenName: '_root', | |
| severity: 'error', | |
| message: 'Components configuration must be an object, not an array or primitive', | |
| }); | |
| return { valid: false, errors, warnings }; | |
| } | |
| Object.entries(tokens).forEach(([name, value]) => { |
Code Review Run #484b31
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| * @param theme - The new theme to apply | ||
| */ | ||
| private updateTheme(theme?: AnyThemeConfig): void { | ||
| private async updateTheme(theme?: AnyThemeConfig): Promise<void> { |
Contributor
There was a problem hiding this comment.
Async method called without await
updateTheme is now async, but callers like setTheme, setThemeMode, resetTheme, setTemporaryTheme, and handleSystemThemeChange do not await it, potentially causing theme updates to race with subsequent operations.
Code Review Run #484b31
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| * Tries to fetch a fresh system default theme from the API. | ||
| */ | ||
| private fallbackToDefaultMode(): void { | ||
| private async fallbackToDefaultMode(): Promise<void> { |
Contributor
There was a problem hiding this comment.
Async fallback not awaited
fallbackToDefaultMode is now async, but setThemeMode calls it without await, which may cause incomplete fallback operations.
Code Review Run #484b31
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
SUMMARY
Implements enhanced theme validation and error handling for Superset's theme system with comprehensive fallback mechanisms. This PR introduces:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
To test live token validation:
- Set
ENHANCED_THEME_VALIDATION: Truein feature flagsADDITIONAL INFORMATION