feat(themes): add enhanced validation and error handling with fallback mechanisms
SUMMARY
Implements enhanced theme validation and error handling for Superset's theme system with comprehensive fallback mechanisms. This PR introduces:
- Enhanced theme token validation with detailed error reporting for invalid theme tokens
- error recovery with fallback to system default themes when theme application fails
- Feature flag controlled validation (ENHANCED_THEME_VALIDATION) to enable/disable advanced validation
- Partial theme loading that filters out invalid tokens while preserving valid ones
- Improved ThemeModal with real-time validation feedback and user guidance
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
To test live token validation:
- Enable enhanced validation:
- Set
ENHANCED_THEME_VALIDATION: Truein feature flags
ADDITIONAL INFORMATION
- [ ] Has associated issue:
- [ ] Required feature flags:
- [x] 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
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment /korbit-review.
Your admin can change your review schedule in the Korbit Console
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 71.86%. Comparing base (412587a) to head (b75309c).
:warning: Report is 288 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #35349 +/- ##
===========================================
+ Coverage 0 71.86% +71.86%
===========================================
Files 0 589 +589
Lines 0 43607 +43607
Branches 0 4719 +4719
===========================================
+ Hits 0 31336 +31336
- Misses 0 11032 +11032
- Partials 0 1239 +1239
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
- :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
Do we really need the feature flag? I think everyone is in favor of more validation :) Seems like it should be default behavior.
Do we really need the feature flag? I think everyone is in favor of more validation :) Seems like it should be default behavior.
My thought process was because is not a small iteration and also the rules could require a little more refinement it was safer to put it behind a FF but if everyone agrees I can remove it.
I really appreciate the effort to improve theme validation, but I’m concerned about the long-term maintainability of this approach. Here are my thoughts:
Ant Design silently ignores invalid tokens by design. It's permissive and forward compatible. This means:
Token typos--> ignored by AntD, defaults are used"#invalid"values --> ignored by AntD, defaults are used- Unknown tokens from future versions --> continue to work without validation updates
By introducing custom validators (useThemeValidation.ts, themeTokenValidation.ts), we're essentially:
- Reimplementing
Ant Design'stoken system within our own validation layer - Adding maintenance overhead. Every
Ant Designupdate would require updating our validators - Risking incompatibility. Valid future tokens could be rejected by our outdated validation
So I think we should keep it simple and only have structural validations:
- JSON syntax validation
- Empty theme blocking
- Algorithm type validation ("dark" | "light" | "system")
- Object type validation (token, components)
So the ENHANCED_THEME_VALIDATION feature flag, even though it’s optional, I think that if we make an structural validation, we might not need it anymore.
To sum up, I think we should just simplify to:
- Basic structural validation (syntax, type, emptiness)
- Runtime fallback if theme loading fails
- User notifications for load failures
This gives users clear feedback while avoiding the complexity of duplicating Ant Design’s validation logic.
What do you think? Happy to discuss trade-offs!
@gabotorresruiz Thank you for the thoughtful feedback! You're absolutely right about the maintenance concerns and forward compatibility issues.
I'd like to refactor to a simpler approach that addresses your concerns while keeping the most valuable part: catching token name typos.
I would like to keep token validations as a soft validation, Instead of manually maintaining validators, we'll extract valid token names at runtime from theme.getDesignToken() and combine them with Superset's custom tokens. This gives us:
- ✅ Zero maintenance (auto-syncs with installed Ant Design version)
- ✅ Forward compatible (new tokens automatically work)
- ✅ Catches typos (e.g.,
colrPrimary→ warning) - ✅ Non-blocking (warnings don't prevent saves)
Your point about Ant Design being "permissive and forward compatible" is spot-on. Instead of reimplementing their validation, we embrace it. We only warn about likely typos, and let Ant Design handle value validation as designed.
User Experience
- Valid tokens → no annotation
- Unknown tokens → yellow warning (can still save)
- Empty theme → red error (blocks save)
- Invalid values → Ant Design handles at runtime
Does this simplified approach address your concerns?
Have we considered using <ThemeEditor /> from https://github.com/ant-design/antd-token-previewer? Then we wouldn't need validation, and users would be able to see how Superset looks like as they're editing the theme.
🎪 Showtime deployed environment on GHA for b75309c
• Environment: http://54.200.58.28:8080 (admin/admin) • Lifetime: 48h auto-cleanup • Updates: New commits create fresh environments automatically
🎪 Showtime deployed environment on GHA for b75309c
• Environment: http://35.95.113.125:8080 (admin/admin) • Lifetime: 48h auto-cleanup • Updates: New commits create fresh environments automatically
🎪 Showtime deployed environment on GHA for b75309c
• Environment: http://35.163.123.129:8080 (admin/admin) • Lifetime: 48h auto-cleanup • Updates: New commits create fresh environments automatically
Hi @rebenitez1802, The Environment --> Environment: http://35.163.123.129:8080/ is not coming up, Can you spin up it again..?