fix(dashboard): changing a theme no longer discards unsaved edits or reloads charts - #42815
fix(dashboard): changing a theme no longer discards unsaved edits or reloads charts#42815gabotorresruiz wants to merge 1 commit into
Conversation
3354d82 to
b00fe08
Compare
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #42815 +/- ##
=======================================
Coverage 66.37% 66.37%
=======================================
Files 2857 2857
Lines 161048 161066 +18
Branches 37046 37054 +8
=======================================
+ Hits 106892 106915 +23
+ Misses 52141 52136 -5
Partials 2015 2015
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:
|
5b5ad42 to
4357db3
Compare
gabotorresruiz
changed the title
8ab88d5 to
9a5aba1
Compare
2532354 to
db91536
Compare
dosubot
Bot
added
change:frontend
Code Review Agent Run #b78dd2Actionable Suggestions - 0Additional Suggestions - 1
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 AI Code Review powered by |
…reloads charts Changing a dashboard's theme in edit mode had two problems, both rooted in a remount: 1. CrudThemeProvider created a new Theme instance on every theme change. Since each Theme binds its own SupersetThemeProvider, this swapped the provider identity and remounted the whole dashboard subtree, reloading every chart. It now keeps a single stable Theme instance updated in place via Theme.setConfig (which gains an optional baseTheme merged the same way fromConfig does), so switching themes no longer remounts. 2. That remount unmounted the header, whose auto-refresh cleanup dispatches setRefreshFrequency(0). The SET_REFRESH_FREQUENCY reducer wrote hasUnsavedChanges from action.isPersistent, clearing the flag and disabling Save right after a theme was applied. A refresh-frequency update now only adds unsaved changes, never clears them.
db91536 to
a56a8f2
Compare
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| const antdConfig = normalizeThemeConfig(config); | ||
| setConfig(config: AnyThemeConfig, baseTheme?: AnyThemeConfig): void { | ||
| const antdConfig = normalizeThemeConfig( | ||
| Theme.mergeConfig(config, baseTheme) ?? config, |
There was a problem hiding this comment.
Suggestion: The merged configuration is used only for Ant Design token generation, while the Superset-specific ECharts overrides are still extracted from the unmerged config. When baseTheme provides these overrides and the dashboard config does not, the initial fromConfig call includes them but every later in-place update removes them, causing ECharts charts to lose their global or chart-specific customizations. Extract the Superset-specific fields from the same merged configuration passed to normalization. [api mismatch]
Severity Level: Major ⚠️
- ❌ ECharts charts lose inherited global customizations after theme updates.
- ⚠️ Chart-specific ECharts styling disappears during dashboard theme changes.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-core/src/theme/Theme.tsx
**Line:** 129:129
**Comment:**
*Api Mismatch: The merged configuration is used only for Ant Design token generation, while the Superset-specific ECharts overrides are still extracted from the unmerged `config`. When `baseTheme` provides these overrides and the dashboard config does not, the initial `fromConfig` call includes them but every later in-place update removes them, causing ECharts charts to lose their global or chart-specific customizations. Extract the Superset-specific fields from the same merged configuration passed to normalization.
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
|
The flagged issue is valid. The current implementation of To resolve this, update private static mergeConfig(
config?: AnyThemeConfig,
baseTheme?: AnyThemeConfig,
): AnyThemeConfig | undefined {
if (baseTheme && config) {
// Merge the entire config object, not just the token property
const mergedConfig = { ...baseTheme, ...config, token: { ...baseTheme.token, ...config.token } };
// In Ant Design v5, colorLink derives from colorInfo, not colorPrimary.
if (config.token?.colorPrimary && !config.token?.colorLink) {
mergedConfig.token.colorLink = config.token.colorPrimary;
}
return mergedConfig;
}
return config;
}There are other comments on this PR. Would you like me to fetch and validate them as well? superset-frontend/packages/superset-core/src/theme/Theme.tsx |
Code Review Agent Run #ca9c3aActionable Suggestions - 0Review 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 AI Code Review powered by |
2 participants
SUMMARY
Changing a dashboard's theme in edit mode had two problems, both from a remount:
CrudThemeProviderrecreated theThemeon every theme change. Since eachThemebinds its ownSupersetThemeProvider, this swapped the provider identity and remounted the whole dashboard subtree, reloading every chart. It now keeps one stableThemeinstance updated in place viaTheme.setConfig(which gains an optionalbaseTheme, merged the same wayfromConfigdoes).That remount unmounted the dashboard header, whose auto-refresh cleanup dispatches
setRefreshFrequency(0). TheSET_REFRESH_FREQUENCYreducer wrotehasUnsavedChangesfromaction.isPersistent, clearing the flag so Save went disabled right after a theme was applied. A refresh-frequency update now only adds unsaved changes, never clears them.Together, a theme can be applied, changed, or removed and saved through the UI without reloading charts. Backward compatible: existing
setConfigandfromConfigcallers are unaffected.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION