feat(theming): add base theme config #35220

gabotorresruiz · 2025-09-20T04:13:29Z

SUMMARY

This PR implements a base theme configuration system that provides consistent foundation themes across the application. This change introduces THEME_DEFAULT and THEME_DARK as complete base themes that serve as the foundation for all theme customization, ensuring consistent theming while maintaining flexibility.

TESTING INSTRUCTIONS

Test base theme configuration:
- Start Superset with the default configuration
- Verify the base themes are applied correctly (check colorPrimary, borders, etc.)
- Toggle between light/dark/system modes
- Confirm mode persistence after page reload

Test with custom theme tokens:

Modify THEME_DEFAULT in config.py:

THEME_DEFAULT = {
    "token": {
        "colorPrimary": "#ff0000",
        "borderRadius": 8,
        # ... other custom tokens
    }
}
THEME_DARK = {
    **THEME_DEFAULT,
    "algorithm": "dark"
}

Restart and verify custom tokens are applied

Test UI theme administration:
- Navigate to Settings > Themes
- Create custom themes and set as system default/dark
- Verify database themes properly overlay on base configuration
- Test "Remove as system theme"
- Restart and verify, should fallback to config themes
Test confirmation modals theming:
- In the Themes list, click "Set as system default theme" on any theme
- Verify the confirmation modal follows current theme colors
- Test in both light and dark modes
- Confirm modal buttons and styling inherit properly
Test dashboard-specific themes:
- Apply a theme to a dashboard using the bolt button
- Verify dashboard theme properly merges with base theme
- Navigate between dashboards to confirm proper theme switching

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- 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

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Blocking font CSS imports delay rendering ▹ view	🧠 Not in standard
	Asymmetric base theme fallback logic ▹ view	🧠 Not in scope
	Duplicated validation logic ▹ view	🧠 Not in scope
	Complex Theme Merging Logic ▹ view	🧠 Incorrect
	Shared mutable token dictionary between base themes ▹ view	🧠 Not in scope
	Complex theme resolution logic needs strategy pattern ▹ view	🧠 Not in scope

Files scanned

File Path	Reviewed
superset-frontend/packages/superset-ui-core/src/theme/GlobalStyles.tsx	✅
superset/daos/theme.py	✅
superset-frontend/src/types/bootstrapTypes.ts	✅
superset-frontend/packages/superset-ui-core/src/theme/types.ts	✅
superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx	✅
superset-frontend/src/pages/ThemeList/index.tsx	✅
superset/views/base.py	✅
superset-frontend/src/theme/ThemeController.ts	✅
superset/config.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

superset-frontend/packages/superset-ui-core/src/theme/GlobalStyles.tsx

superset/config.py

superset-frontend/src/theme/ThemeController.ts

superset/views/base.py

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

codecov · 2025-09-20T04:27:20Z

Codecov Report

❌ Patch coverage is 45.00000% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.84%. Comparing base (ecb3ac6) to head (d4eb799).
⚠️ Report is 15 commits behind head on master.

Files with missing lines	Patch %	Lines
superset/views/base.py	37.14%	19 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           master   #35220       +/-   ##
===========================================
+ Coverage        0   71.84%   +71.84%     
===========================================
  Files           0      587      +587     
  Lines           0    43520    +43520     
  Branches        0     4704     +4704     
===========================================
+ Hits            0    31267    +31267     
- Misses          0    11025    +11025     
- Partials        0     1228     +1228

Flag	Coverage Δ
hive	`46.27% <45.00%> (?)`
mysql	`70.87% <45.00%> (?)`
postgres	`70.92% <45.00%> (?)`
presto	`49.95% <45.00%> (?)`
python	`71.81% <45.00%> (?)`
sqlite	`70.51% <45.00%> (?)`
unit	`100.00% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

mistercrunch · 2025-09-22T00:50:48Z

superset-frontend/src/types/bootstrapTypes.ts

 export interface BootstrapThemeDataConfig {
  default: SerializableThemeConfig | {};
  dark: SerializableThemeConfig | {};
+  baseThemeDefault?: SerializableThemeConfig | null;


I have to admit I didn't think about this when ideating. It's kind of getting out-of-control that we're sending 4 theme configs over from the backend on every page load. Wondering if there's a way that the THEME_DEFAULT/THEME_DARK could be treated as BASE?

What if we just treated themes in CRUD as overrides on top of THEME_DEFAULT/THEME_DARK?

That's a great point! You're absolutely right that sending 4 theme configs on every page load is getting excessive. I think your suggestion to treat THEME_DEFAULT/THEME_DARK as the base themes makes a lot of sense.

Here's what I'm thinking:
Current architecture:

BASE_THEME_DEFAULT/BASE_THEME_DARK: organization-wide base tokens

THEME_DEFAULT/THEME_DARK: system themes (merged with base)

User CRUD themes: also merged with base

Simplified architecture:

THEME_DEFAULT/THEME_DARK: act as both system defaults and base themes

User CRUD themes: simple overrides on top of the system themes

Let me know your thoughts on this @mistercrunch

superset-frontend/packages/superset-ui-core/src/theme/GlobalStyles.tsx

rusackas · 2025-09-22T17:42:56Z

Does the PR description need an update/reboot after the latest changes?

mistercrunch · 2025-09-23T06:37:22Z

superset-frontend/packages/superset-ui-core/src/theme/Theme.tsx

+        };
+      }
+
+      if (baseTheme.components || config.components) {


didn't look deep into the logic, but is this whole section a use case for lodash's merge function? Maybe can save ~30 LoC

Yep, mergeWith will do the trick. Nice catch!

mistercrunch · 2025-09-23T06:48:22Z

superset-frontend/src/pages/ThemeList/index.tsx

                <Tooltip title={t('This is the system dark theme')}>
-                  <Tag color="default">
+                  <Tag
+                    color="default"


NIT: repeating the lines above, wondering the elegant way to do this. Maybe either marginRight on that first one or [maybe better] wrapping both into <Space size="small" style={{ display: 'inline-flex' }}> might be most antd-aligned, style-free. I think it'll generate and empty div but that's probably ok.

Somehow came to hate most css and even style (to a slightly lesser extend) while working on theming for months. I think I'm getting a little OCD about it...

You're right! I think I found a "better/elegant" way of doing this

mistercrunch · 2025-09-23T06:53:14Z

superset-frontend/src/theme/ThemeController.ts

+   * @param theme - The theme configuration to check
+   * @returns True if the theme is dark mode, false otherwise
+   */
+  private isThemeDark(theme: AnyThemeConfig): boolean {


already exists in packages/superset-ui-core/src/theme/utils/themeUtils.ts

Yes but that function serves a different purpose, it checks if a computed theme is dark by examining the background color.

The ThemeController needed something different, it needs to check if a configuration uses dark algorithm before the theme is computed. So I added a new function isThemeConfigDark alongside the isThemeDark in packages/superset-ui-core/src/theme/utils/themeUtils.ts

oh right. Though in theory could potentially have a dark theme without the dark algorithm, though highly unlikely/impractical. I think the main Theme class has or used to have an isDark method too. theme.fromConfig(themeConfig).isDark or similar might work too but require a bit more computation.

there's a subtle difference between a theme config and a computed theme, probably the typing system should be aware of the difference (maybe it is already, I'd have to check) ... The Theme class knows how to generate one from the other. There might be two isThemeConfigDark and isThemeDark .... If we need both maybe they both live in utils.

I'm not sure what makes sense to do in the context of this PR, but might be good to sort this out.

@mistercrunch please take a look into this --> d4eb799#diff-b48e95953d3af3d6d542dd931e3269b3f74031f32a292dd4b77fb11ddba31097R128

Let me know your thoughts

mistercrunch · 2025-09-23T06:56:20Z

superset/daos/theme.py

+                "Multiple system default themes found (%s), none will be used",
                len(system_defaults),
            )
+            return None


this line probably not needed

mistercrunch · 2025-09-23T06:56:55Z

superset/daos/theme.py

+                "Multiple system dark themes found (%s), none will be used",
                len(system_darks),
            )
+            return None


line not needed as there's a catchall bellow

Agree as well!

mistercrunch · 2025-09-23T07:01:12Z

superset/views/base.py

+
+def _load_theme_from_model(
+    theme_model: Any | None, fallback_theme: dict[str, Any] | None, theme_type: str
+) -> dict[str, Any] | None:


there's a type for it here superset/themes/types.py

Sure! I'll change this

mistercrunch · 2025-09-23T07:01:55Z

superset/views/base.py

+    return fallback_theme
+
+
+def _process_theme(theme: dict[str, Any] | None, theme_type: str) -> dict[str, Any]:


superset/themes/types.py

msyavuz · 2025-09-23T11:14:37Z

superset-frontend/src/theme/ThemeController.ts

+  private isEmptyTheme(theme: any): boolean {
+    if (!theme) return true;
+
+    // If it's an empty object {}, it's empty
+    if (typeof theme === 'object' && Object.keys(theme).length === 0)
+      return true;
+
+    // If theme has an algorithm, it's not empty (even without tokens)
+    if (theme.algorithm) return false;
+
+    // Check if theme has any tokens defined
+    if (theme.token && Object.keys(theme.token).length > 0) return false;
+
+    // Check if theme has components defined
+    if (theme.components && Object.keys(theme.components).length > 0)
+      return false;
+
+    return true;
+  }


This doesn't look right to me, can we add proper types to the argument? First two checks might not be needed

You're right, I was overcomplicating things here. Thanks, nice catch!

superset-frontend/packages/superset-ui-core/src/theme/types.ts

mistercrunch · 2025-09-24T00:52:13Z

superset-frontend/packages/superset-ui-core/src/components/Typography/Typography.test.tsx

  it('renders strong text', () => {
    render(<Typography.Text strong>Strong Text</Typography.Text>);
-    expect(screen.getByText('Strong Text')).toHaveStyle('font-weight: 500');
+    expect(screen.getByText('Strong Text')).toHaveStyle('font-weight: 600');


I don't understand why this test needed to change, from my understanding we just moved the default base theme from the frontend to the backend.

The test changed because fontWeightStrong: 500 theme token was moved from the frontend (Theme.tsx) to the backend (config.py). During unit tests, the backend config isn't available, so Ant Design's default (600) is used instead. The test now reflects this fallback behavior.

Oh, then I'm surprised we don't have more unit tests failing. Noting this test doesn't seem to use the spec/helpers/ render (which renders with a theme provider), it does import '@testing-library/jest-dom'; instead of @superset-ui/core/spec, wondering if it has something to do with it.

Sidetrack (outside the scope of this PR): It's a bit of a mess around this, might be good to not allow importing @testing-library directly so we're always providing a theme context. It's a bit confusing since each library might have slightly different helpers (useRedux:true in main app for instance).

Not sure what to do with all this, maybe it's just out-of-scope of this PR and this change is ok.

pull-request-size bot added the size/XXL label Sep 20, 2025

github-actions bot added the packages label Sep 20, 2025

dosubot bot added change:frontend Requires changing the frontend global:theming Related to theming Superset labels Sep 20, 2025

korbit-ai bot reviewed Sep 20, 2025

View reviewed changes

gabotorresruiz force-pushed the feat/create-base-theme-config branch 2 times, most recently from c97b708 to eede35d Compare September 21, 2025 22:03

mistercrunch reviewed Sep 22, 2025

View reviewed changes

superset-frontend/packages/superset-ui-core/src/theme/GlobalStyles.tsx Show resolved Hide resolved

gabotorresruiz added 2 commits September 21, 2025 22:54

feat(theming): add base theme config

01678b3

feat(theming): simplify and unify base/system theme logic

1559605

gabotorresruiz force-pushed the feat/create-base-theme-config branch from eede35d to 1559605 Compare September 22, 2025 13:42

gabotorresruiz requested a review from mistercrunch September 22, 2025 16:43

rusackas requested a review from msyavuz September 22, 2025 17:40

gabotorresruiz requested a review from rusackas September 22, 2025 21:19

mistercrunch reviewed Sep 23, 2025

View reviewed changes

msyavuz reviewed Sep 23, 2025

View reviewed changes

gabotorresruiz force-pushed the feat/create-base-theme-config branch 3 times, most recently from 67899ab to aac1df4 Compare September 23, 2025 20:20

fix(theming): address PR comments + some fixes and improvements

d4eb799

gabotorresruiz force-pushed the feat/create-base-theme-config branch from aac1df4 to d4eb799 Compare September 23, 2025 21:02

mistercrunch reviewed Sep 24, 2025

View reviewed changes

gabotorresruiz requested review from msyavuz and mistercrunch September 25, 2025 00:20

		return fallback_theme


		def _process_theme(theme: dict[str, Any] \| None, theme_type: str) -> dict[str, Any]:

feat(theming): add base theme config #35220

Are you sure you want to change the base?

feat(theming): add base theme config #35220

Conversation

gabotorresruiz commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Sep 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rusackas commented Sep 22, 2025

Uh oh!

mistercrunch Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gabotorresruiz commented Sep 20, 2025 •

edited

Loading

korbit-ai bot left a comment •

edited

Loading

codecov bot commented Sep 20, 2025 •

edited

Loading

mistercrunch Sep 23, 2025 •

edited

Loading