superset icon indicating copy to clipboard operation
superset copied to clipboard

feat(themes): add enhanced validation and error handling with fallback mechanisms

Open rebenitez1802 opened this issue 2 months ago • 14 comments

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

Theme_Token_validation

TESTING INSTRUCTIONS

To test live token validation:

  1. Enable enhanced validation: - Set ENHANCED_THEME_VALIDATION: True in 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

rebenitez1802 avatar Sep 30 '25 23:09 rebenitez1802

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

korbit-ai[bot] avatar Sep 30 '25 23:09 korbit-ai[bot]

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.

codecov[bot] avatar Sep 30 '25 23:09 codecov[bot]

Do we really need the feature flag? I think everyone is in favor of more validation :) Seems like it should be default behavior.

rusackas avatar Oct 01 '25 17:10 rusackas

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.

rebenitez1802 avatar Oct 02 '25 14:10 rebenitez1802

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:

  1. Reimplementing Ant Design's token system within our own validation layer
  2. Adding maintenance overhead. Every Ant Design update would require updating our validators
  3. 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 avatar Oct 13 '25 22:10 gabotorresruiz

@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?

rebenitez1802 avatar Oct 14 '25 01:10 rebenitez1802

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.

betodealmeida avatar Nov 03 '25 17:11 betodealmeida

🎪 Showtime is building environment on GHA for b75309c

github-actions[bot] avatar Nov 14 '25 18:11 github-actions[bot]

🎪 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

github-actions[bot] avatar Nov 14 '25 18:11 github-actions[bot]

🎪 Showtime is building environment on GHA for b75309c

github-actions[bot] avatar Nov 19 '25 18:11 github-actions[bot]

🎪 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

github-actions[bot] avatar Nov 19 '25 18:11 github-actions[bot]

🎪 Showtime is building environment on GHA for b75309c

github-actions[bot] avatar Dec 01 '25 17:12 github-actions[bot]

🎪 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

github-actions[bot] avatar Dec 01 '25 17:12 github-actions[bot]

Hi @rebenitez1802, The Environment --> Environment: http://35.163.123.129:8080/ is not coming up, Can you spin up it again..?

Naveensirigiri avatar Dec 04 '25 16:12 Naveensirigiri