korbit-ai[bot] commented on code in PR #34321:
URL: https://github.com/apache/superset/pull/34321#discussion_r2231744584
##########
superset-frontend/src/theme/ThemeProvider.tsx:
##########
@@ -71,15 +71,28 @@ export function SupersetThemeProvider({
[themeController],
);
+ const canSetThemeMode = useCallback(
+ () => themeController.canSetThemeMode(),
+ [themeController],
+ );
Review Comment:
### Unnecessary Callback Wrapper <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The canSetThemeMode function is a wrapper that adds unnecessary complexity
by using useCallback for a simple delegation to
themeController.canSetThemeMode().
###### Why this matters
Adding a callback wrapper for a simple delegation method increases
complexity without providing significant performance benefits. The function
will still be recreated if themeController changes.
###### Suggested change ∙ *Feature Preview*
Remove the useCallback wrapper and directly expose the themeController
method in the context value:
```typescript
const contextValue = useMemo(
() => ({
...,
canSetThemeMode: themeController.canSetThemeMode.bind(themeController),
}),
[themeController]
);
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18e35d8f-cd99-47d3-9e05-a37a1f5769ad/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18e35d8f-cd99-47d3-9e05-a37a1f5769ad?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18e35d8f-cd99-47d3-9e05-a37a1f5769ad?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18e35d8f-cd99-47d3-9e05-a37a1f5769ad?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/18e35d8f-cd99-47d3-9e05-a37a1f5769ad)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:740b2b67-d081-4cb5-a880-78383d377e86 -->
[](740b2b67-d081-4cb5-a880-78383d377e86)
##########
superset-frontend/src/theme/ThemeController.ts:
##########
@@ -406,26 +508,24 @@ export class ThemeController {
* @returns The theme configuration for the specified mode or null if not
available
*/
private getThemeForMode(mode: ThemeMode): AnyThemeConfig | null {
- const { allowOSPreference = DEFAULT_THEME_SETTINGS.allowOSPreference } =
- this.themeSettings;
-
let resolvedMode: ThemeMode = mode;
if (mode === ThemeMode.SYSTEM) {
- if (!allowOSPreference) return null;
+ if (!this.getThemeSettings('allowOSPreference')) return null;
resolvedMode = ThemeController.getSystemPreferredMode();
}
- if (!this.hasBootstrapThemes) {
- const baseTheme = this.defaultTheme.token as Partial<SupersetTheme>;
- return getAntdConfig(baseTheme, resolvedMode === ThemeMode.DARK);
- }
+ // Handle case where no bootstrap themes are available
+ if (!this.hasBootstrapThemes)
+ return this.createFallbackTheme(resolvedMode === ThemeMode.DARK);
+
+ // Handle bootstrap themes with per-mode fallback to superset defaults
+ const selectedTheme =
+ resolvedMode === ThemeMode.DARK ? this.darkTheme : this.defaultTheme;
- // Handle bootstrap themes using existing normalization
- const selectedTheme: AnyThemeConfig =
- resolvedMode === ThemeMode.DARK
- ? this.darkTheme || this.defaultTheme
- : this.defaultTheme;
+ // If the selected theme is missing or empty, fallback to superset
built-in theme
+ if (!selectedTheme || this.isEmptyTheme(selectedTheme))
+ return this.createFallbackTheme(resolvedMode === ThemeMode.DARK);
return selectedTheme;
}
Review Comment:
### Complex theme resolution flow <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The theme resolution logic contains multiple nested conditions and fallback
paths that make the flow difficult to follow.
###### Why this matters
Complex theme resolution logic with multiple exit points makes it harder to
understand and maintain the theme selection process.
###### Suggested change ∙ *Feature Preview*
```typescript
private getThemeForMode(mode: ThemeMode): AnyThemeConfig | null {
// Early exit for system mode
if (mode === ThemeMode.SYSTEM) {
if (!this.getThemeSettings('allowOSPreference')) {
return null;
}
mode = ThemeController.getSystemPreferredMode();
}
const isDarkMode = mode === ThemeMode.DARK;
// No bootstrap themes case
if (!this.hasBootstrapThemes) {
return this.createFallbackTheme(isDarkMode);
}
// Select appropriate theme
const selectedTheme = isDarkMode ? this.darkTheme : this.defaultTheme;
// Return fallback if selected theme is invalid
if (!selectedTheme || this.isEmptyTheme(selectedTheme)) {
return this.createFallbackTheme(isDarkMode);
}
return selectedTheme;
}
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5df071e5-1d08-4b88-9a89-7c9e8360d5c7/upvote)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5df071e5-1d08-4b88-9a89-7c9e8360d5c7?what_not_true=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5df071e5-1d08-4b88-9a89-7c9e8360d5c7?what_out_of_scope=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5df071e5-1d08-4b88-9a89-7c9e8360d5c7?what_not_in_standard=true)
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5df071e5-1d08-4b88-9a89-7c9e8360d5c7)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:3f9f70c0-278d-40fc-ade9-b0c53c735310 -->
[](3f9f70c0-278d-40fc-ade9-b0c53c735310)
--
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]
