fix(themes): serve system themes with the algorithm of the slot they fill - #42700
Conversation
…fill A theme picked as the system dark theme keeps its own algorithm when merged onto the config dark base, so a light theme in the dark slot was served with algorithm "default". Ant Design then derived a mix of light and dark tokens and dark mode rendered as a half-themed UI. Force each theme to carry the algorithm of the slot it fills, and warn admins in the confirmation dialog when the theme they pick contradicts the slot.
Code Review Agent Run #c6de6fActionable Suggestions - 0Additional Suggestions - 1
Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
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 |
| const algorithms = getThemeAlgorithms(jsonData); | ||
| if (!algorithms.length) return false; | ||
|
|
||
| return algorithms.includes(ThemeAlgorithm.DARK) !== isDarkSlot; |
There was a problem hiding this comment.
Suggestion: The conflict check treats every algorithm list that does not contain ThemeAlgorithm.DARK as a light algorithm. A theme declaring only a modifier such as compact is compatible with either slot, and the backend preserves that modifier while adding the slot algorithm. This condition therefore displays a misleading warning for valid modifier-only themes; check for an explicit opposing algorithm instead of using absence of dark as evidence of default. [incorrect condition logic]
Severity Level: Minor 🧹
- ⚠️ System-dark confirmation warns for compact-only themes.
- ⚠️ Admins may incorrectly believe compact themes are incompatible.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/features/themes/utils.ts
**Line:** 47:47
**Comment:**
*Incorrect Condition Logic: The conflict check treats every algorithm list that does not contain `ThemeAlgorithm.DARK` as a light algorithm. A theme declaring only a modifier such as `compact` is compatible with either slot, and the backend preserves that modifier while adding the slot algorithm. This condition therefore displays a misleading warning for valid modifier-only themes; check for an explicit opposing algorithm instead of using absence of `dark` as evidence of `default`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| 'Are you sure you want to set "%s" as the system default theme? This will apply to all users who haven\'t set a personal preference.', | ||
| theme.theme_name, | ||
| )} | ||
| {hasConflictingAlgorithm(theme.json_data, false) && ( |
There was a problem hiding this comment.
Suggestion: The warning is calculated from the raw theme.json_data, but the backend applies the configured base theme before enforcing the slot algorithm. Consequently, a partial theme with no algorithm field can inherit an explicit opposing algorithm from the configured theme and still be assigned without any warning. Resolve the effective merged algorithm, or use the same fallback configuration as the backend, before calling hasConflictingAlgorithm. [api mismatch]
Severity Level: Minor 🧹
- ⚠️ Theme assignment warning omits inherited algorithm conflicts.
- ⚠️ Admins may miss that configured colors are remapped.(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/pages/ThemeList/index.tsx
**Line:** 283:283
**Comment:**
*Api Mismatch: The warning is calculated from the raw `theme.json_data`, but the backend applies the configured base theme before enforcing the slot algorithm. Consequently, a partial theme with no `algorithm` field can inherit an explicit opposing algorithm from the configured theme and still be assigned without any warning. Resolve the effective merged algorithm, or use the same fallback configuration as the backend, before calling `hasConflictingAlgorithm`.
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.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #42700 +/- ##
==========================================
- Coverage 65.43% 65.42% -0.02%
==========================================
Files 2810 2811 +1
Lines 159460 159554 +94
Branches 36396 36418 +22
==========================================
+ Hits 104350 104389 +39
- Misses 53068 53116 +48
- Partials 2042 2049 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
alexandrusoare
left a comment
There was a problem hiding this comment.
Thank you for your work @msyavuz, a clean fix and solid test coverage!
I have only one question, have you considered just hiding the "Set as dark theme" action for light-algorithm themes (and vice versa) to prevent the mismatch entirely, rather than warning and correcting it downstream?
Yes, i wanted to cover non-UI paths as well. Headles, embedded etc. |
SUMMARY
Picking a light theme as the system dark theme breaks dark mode: the UI renders a mix of light and dark tokens (black borders over otherwise unchanged styling) instead of a dark UI.
get_theme_bootstrap_data()merges the DB-selected theme over the config base (_THEME_DARK_BASE,algorithm: "dark"), and the DB theme'salgorithmkey wins the merge. So a theme authored withalgorithm: "default"is served in the dark slot asdefault, and Ant Design derives light-mapped tokens from dark-leaning seed values.Each system theme is now served with the algorithm of the slot it fills (modifiers like
compactpreserved), which also repairs deployments already in this state. The Themes list additionally warns admins when the theme they are assigning declares the opposite algorithm.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — no visual change on correctly configured deployments; the fix restores the theme's normal dark rendering.
TESTING INSTRUCTIONS
THEME_DEFAULT) as the system dark theme. A warning now appears in the confirmation dialog.pytest tests/unit_tests/themes/test_utils.py tests/unit_tests/views/test_base_theme_helpers.pynpm run test -- src/features/themes/utils.test.ts src/pages/ThemeListADDITIONAL INFORMATION
ENABLE_UI_THEME_ADMINISTRATIONis on by default and is what exposes the broken path