You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "gabotorresruiz (via GitHub)" <gi...@apache.org> on 2025/07/25 18:06:51 UTC
[PR] fix(theming): Superset default theme Fallback [superset]
gabotorresruiz opened a new pull request, #34321:
URL: https://github.com/apache/superset/pull/34321
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
This PR fixes a critical issue where users providing partial Bootstrap theme configurations (e.g., only THEME_DEFAULT defined with empty THEME_DARK) couldn't switch to dark mode (nothing got changed since it wasn't a `THEME_DARK` been set. The solution was to implement a fallback logic that uses Superset's built-in themes with proper Ant Design algorithms when Bootstrap themes are incomplete or missing.
Also I've replaced the `THEME_ENABLE_DARK_THEME_SWITCH` feature flag to start using the `THEME_SETTINGS` approach.
Changes:
- Replaced `THEME_ENABLE_DARK_THEME_SWITCH` feature flag with `THEME_SETTINGS.allowSwitching`
- Updated `RightMenu.tsx` to use theme context's `canSetThemeMode()` instead of feature flag checks
- Added `canSetThemeMode()` method to theme context interface
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
1. Go to `superset/config.py` and only define a `THEME_DEFAULT`.
E.g.:
```
THEME_DEFAULT: Theme = {"token": {"brandLogoUrl": "https://picsum.photos/200"}}
```
2. Since the default for `THEME_SETTINGS.allowSwitching` is `true` if no theme settings are defined
3. Reload the UI and change the theme mode to dark mode
4. You should see how the UI defaults to the superset dark mode theme since we didn't pass a `THEME_DARK` from configs
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(theming): Superset default theme Fallback [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(theming): Superset default theme Fallback [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34321:
URL: https://github.com/apache/superset/pull/34321#issuecomment-3137887707
If you rebase is it fixed? I think I might have handled that in a previous PR (unsure)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(theming): Superset default theme Fallback [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34321:
URL: https://github.com/apache/superset/pull/34321#issuecomment-3140539902
Might be easier to start fresh than dealing with the merge conflicts here (?) Whatever works best for you.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(theming): Superset default theme Fallback [superset]
Posted by "gabotorresruiz (via GitHub)" <gi...@apache.org>.
gabotorresruiz commented on PR #34321:
URL: https://github.com/apache/superset/pull/34321#issuecomment-3140377579
Hey @mistercrunch I think this is 80% already fixed by you, but I would love to fix this conflicts and add the fallback logic that I did here just as a defensive code
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(theming): Superset default theme Fallback [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch closed pull request #34321: fix(theming): Superset default theme Fallback
URL: https://github.com/apache/superset/pull/34321
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(theming): Superset default theme Fallback [superset]
Posted by "mistercrunch (via GitHub)" <gi...@apache.org>.
mistercrunch commented on PR #34321:
URL: https://github.com/apache/superset/pull/34321#issuecomment-3180963512
Closing since logic changed quite a bit in this area. We should check whether there's still an issue, and cook a fix on top of `master` if that's the case.
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org