Copilot commented on code in PR #37378:
URL: https://github.com/apache/superset/pull/37378#discussion_r2756314022
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -896,25 +936,82 @@ export class ThemeController {
/**
* Fetches a theme configuration from the CRUD API.
* @param themeId - The ID of the theme to fetch
- * @returns The theme configuration or null if not found
+ * @returns The theme configuration or null if fetch fails
*/
private async fetchCrudTheme(
themeId: string,
): Promise<AnyThemeConfig | null> {
try {
// Use SupersetClient for proper authentication handling
- const getTheme = makeApi<void, { result: { json_data: string } }>({
+ const getTheme = makeApi<
+ void,
+ { result: { json_data: string; theme_name?: string } }
+ >({
method: 'GET',
endpoint: `/api/v1/theme/${themeId}`,
});
const { result } = await getTheme();
const themeConfig = JSON.parse(result.json_data);
+ if (!themeConfig || typeof themeConfig !== 'object') {
+ console.error(`Invalid theme configuration for theme ${themeId}`);
+ return null;
+ }
+
+ // Return theme as-is
+ // Invalid tokens will be handled by Ant Design at runtime
+ // Runtime errors will be caught by applyThemeWithRecovery()
return themeConfig;
} catch (error) {
console.error('Failed to fetch CRUD theme:', error);
return null;
}
}
+
+ /**
+ * Fetches a fresh system default theme from the API for runtime recovery.
+ * Tries multiple fallback strategies to find a valid theme.
+ *
+ * Note: Uses raw fetch() instead of SupersetClient because ThemeController
+ * initializes early in the app lifecycle, before SupersetClient is fully
+ * configured. This avoids boot-time circular dependencies.
+ *
+ * @returns The system default theme configuration or null if not found
+ */
+ 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;
Review 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.
```suggestion
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;
```
##########
superset-frontend/src/features/themes/ThemeModal.test.tsx:
##########
@@ -426,13 +524,26 @@ test('saves changes when clicking Save button in unsaved
changes alert', async (
await screen.findByText('You have unsaved changes'),
).toBeInTheDocument();
- const saveButton = screen.getByRole('button', { name: 'Save' });
+ // Wait for the Save button in the alert to be enabled
+ const saveButton = await waitFor(
+ () => {
+ const button = screen.getByRole('button', { name: 'Save' });
+ expect(button).toBeEnabled();
+ return button;
+ },
+ { timeout: 10000 },
+ );
await userEvent.click(saveButton);
// Wait for API call to complete
await screen.findByRole('dialog');
- expect(fetchMock.callHistory.called()).toBe(true);
-});
+ await waitFor(
+ () => {
+ expect(fetchMock.callHistory.called()).toBe(true);
+ },
+ { timeout: 15000 },
+ );
+}, 30000);
Review 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.
--
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]
