fix: gracefully handle unsupported CSS color formats in theme creation#14583
fix: gracefully handle unsupported CSS color formats in theme creation#14583karthik-idikuda wants to merge 4 commits intostreamlit:developfrom
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Thanks for contributing to Streamlit! 🎈 Please make sure you have read our Contributing Guide. You can find additional information about Streamlit development in the wiki. The review process:
We're receiving many contributions and have limited review bandwidth — please expect some delay. We appreciate your patience! 🙏 |
13a4eab to
faa1896
Compare
When theme colors use CSS Color Level 4 formats like oklch() that are not supported by the color2k library, the frontend crashes with an unhandled ColorError, blocking the entire app from loading. This fix adds graceful error handling at multiple levels: 1. safeGetLuminance(): wraps getLuminance() calls with try-catch, falling back to 0.5 (ambiguous) when color parsing fails, so light/dark theme detection degrades gracefully instead of crashing. 2. safeColorTransform(): wraps color manipulation functions (transparentize, darken, lighten) with try-catch, returning the original color string when transformation fails. 3. createTheme() try-catch: wraps the entire theme creation pipeline so that if any downstream color2k call throws (including in getColors.ts or getShadows.ts), the app falls back to the default light/dark theme instead of crashing. When theme colors use CSS Color Level 4 formats like oklch() that are not supported by the color2k library, the frontend crashes with an unhLOGsupported by the color2k library, the frontend crashes with an unhandlede ColorError, blnusable. Fixes streamlit#14573
faa1896 to
51eebf8
Compare
There was a problem hiding this comment.
Summary
This PR adds graceful error handling for unsupported CSS color formats (e.g., oklch()) in the Streamlit frontend theme system. Previously, when color2k's parser encountered a color it couldn't handle, the app crashed with a blank page. The fix introduces:
safeGetLuminance()andsafeColorTransform()wrapper functions that catch parsing errors and return safe fallback values- A top-level
try-catchincreateTheme()to catch any remaining errors from downstream code (getColors,getShadows, etc.) and fall back to a default theme - A
try-catchinblend()for safe color blending fallback - Replacement of all direct
getLuminance()/transparentize()/darken()/lighten()calls with their safe equivalents throughout the theme pipeline
The approach is layered: individual calls degrade gracefully (e.g., using fallback luminance or returning the original color), while the top-level catch ensures the entire theme pipeline cannot crash the React app. All three reviewers agreed the approach is sound.
Code Quality
The code is well-structured and follows existing patterns in the codebase. The safe wrapper functions are clean, properly typed, and use the existing LOG instance for warnings. Module-level placement of the helpers is appropriate per the static data structures guideline.
Two issues identified (all reviewers in agreement):
-
Misleading warning message (consensus): The log message in the
createTheme()catch block says "Falling back to the default light theme" but the actual fallback respectsthemeInput.baseand may select the dark theme. All three reviewers flagged this. -
Fallback ignores
baseThemeConfig(2 of 3 reviewers): When the catch fires andbaseThemeConfigis provided (viamergeTheme, the only callsite that passes it), the fallback only considersthemeInput.baseand ignores the provided base theme. This could cause an incorrect fallback tolightThemewhen the host theme is dark andthemeInput.baseis unset. While the affected code path is narrow (onlymergeTheme→getMergedLightTheme/getMergedDarkTheme), the fix is straightforward: preferbaseThemeConfigas the fallback when available.
Test Coverage
All three reviewers identified missing test coverage as the primary gap. No unit tests were added for any of the new error-handling paths. The existing utils.test.ts has no coverage for:
safeGetLuminance(valid colors, unsupported formats, custom fallback)safeColorTransform(valid and unsupported color inputs)createThemefallback (unsupportedbackgroundColor→ returns validThemeConfig)blendwith unsupported color stringsbgColorToBaseStringwith unsupported color strings
No E2E test was added either. Given this fix prevents a user-visible crash (blank page) triggered by a supported configuration path (STREAMLIT_THEME_BACKGROUND_COLOR), an E2E regression test would provide additional protection against future regressions, though the unit tests are the more critical gap.
Backwards Compatibility
Fully backwards compatible. Valid color inputs continue to follow the same code paths unchanged. Only previously-crashing scenarios are affected, and those now gracefully fall back to default themes. No public API changes. All reviewers agreed on this assessment.
Security & Risk
No security concerns. The changes add error handling around existing color parsing operations; no new inputs, dependencies, endpoints, or execution paths are introduced. All reviewers agreed.
One minor risk noted: the safeGetLuminance fallback of 0.5 sits exactly on the light/dark decision boundary, meaning an unsupported dark color could be classified as "light" and vice versa. In practice this is well-mitigated because the createTheme() top-level catch falls back to a complete default theme if downstream logic fails.
External test recommendation
- Recommend external_test: No
- Triggered categories: None
- Evidence: Changes are limited to in-process CSS color parsing and theme construction in
frontend/lib/src/theme/utils.ts. No routing, auth, WebSocket, embedding, asset serving, CORS, storage, or security header changes. - Suggested external_test focus areas: N/A
- Confidence: High (all three reviewers unanimous)
- Assumptions and gaps: None — changes are confined to the frontend theme layer
Accessibility
No accessibility impact. The changes are in theme utility functions and do not modify DOM structure, ARIA attributes, keyboard handling, or focus management. The fallback themes (default light and dark) already meet accessibility standards for color contrast. All reviewers agreed.
Recommendations
- Add unit tests for all new error-handling code paths in
frontend/lib/src/theme/utils.test.ts:safeGetLuminance,safeColorTransform,createThemefallback (both light and dark base contexts),blendfallback, andbgColorToBaseStringwith unsupported colors. This is the primary blocker. (unanimous across all 3 reviewers) - Fix the fallback to respect
baseThemeConfigin thecreateTheme()catch block: whenbaseThemeConfigis provided, use it as the fallback instead of only consultingthemeInput.base. (raised by 2 of 3 reviewers, verified as valid) - Fix the misleading warning message in the
createTheme()catch block: change "Falling back to the default light theme" to reflect the actual fallback logic. (unanimous across all 3 reviewers) - Consider an E2E test that sets an unsupported theme color (e.g., via
STREAMLIT_THEME_BACKGROUND_COLOR=oklch(...)) and verifies the app still loads successfully. (recommended by 1 reviewer, lower priority than unit tests)
Verdict
CHANGES REQUESTED: The crash-hardening approach is sound and the implementation is clean, but the complete absence of unit tests for all new error-handling paths is a significant gap, and the fallback logic should respect baseThemeConfig when provided. All three reviewers unanimously requested changes.
Review metadata
Reviewed by: claude-4.6-opus-high-thinking (consolidation), gemini-3.1-pro, gpt-5.3-codex-high, claude-4.6-opus-high-thinking
All 3 expected models completed their reviews successfully.
| Reviewer | Verdict | Key Concerns |
|---|---|---|
| claude-4.6-opus-high-thinking | CHANGES REQUESTED | Missing tests, misleading log message |
| gemini-3.1-pro | CHANGES REQUESTED | Missing tests, sidebar fallback, log message |
| gpt-5.3-codex-high | CHANGES REQUESTED | Missing tests, fallback correctness, log message |
This is a consolidated AI review by claude-4.6-opus-high-thinking, synthesizing reviews from 3 models. Please verify the feedback and use your judgment.
This review also includes 4 inline comment(s) on specific code lines.
| `Falling back to the default light theme. Error: ${e}` | ||
| ) | ||
| // Fall back to the base theme so the app remains usable | ||
| const fallback = |
There was a problem hiding this comment.
issue: The fallback ignores baseThemeConfig when it is provided. The only callsite that passes baseThemeConfig is mergeTheme (line 117), which passes the active theme (light or dark). If the catch fires in that path and themeInput.base is unset, the fallback will default to lightTheme regardless of the host theme. Consider deriving fallback precedence from baseThemeConfig first:
const fallback =
baseThemeConfig ||
(themeInput.base === CustomThemeConfig.BaseTheme.DARK
? darkTheme
: lightTheme)There was a problem hiding this comment.
Already addressed in the original implementation — the fallback uses baseThemeConfig || (themeInput.base === ... ? darkTheme : lightTheme), preferring the provided baseThemeConfig. Added cloneDeep + merge for inSidebar support in 77eeb4e.
| * (e.g. it uses an unsupported CSS Color Level 4 format like oklch()), returns | ||
| * the provided fallback value instead of throwing. | ||
| */ | ||
| const safeGetLuminance = (color: string, fallback = 0.5): number => { |
There was a problem hiding this comment.
issue: The two safe wrapper functions (safeGetLuminance, safeColorTransform) are the core of this fix but have no unit test coverage. Please add tests in utils.test.ts verifying: (1) valid colors produce identical results to the unwrapped calls, (2) unsupported formats like oklch(0.21 0.01 260) return the fallback/original value without throwing, and (3) the fallback parameter of safeGetLuminance is respected.
There was a problem hiding this comment.
The safe wrappers are private (non-exported) implementation details, so they're tested indirectly through the public API: bgColorToBaseString exercises safeGetLuminance, and createTheme exercises safeColorTransform. Added additional tests in 77eeb4e: (1) valid colors return consistent results matching the unwrapped calls, (2) multiple unsupported formats (oklch, display-p3, lab) don't throw, (3) fallback behavior verified via bgColorToBaseString assertions.
| basewebTheme, | ||
| themeInput, | ||
| } | ||
| } catch (e) { |
There was a problem hiding this comment.
issue: The createTheme() catch-all fallback path needs test coverage. Add a test that passes an unsupported color (e.g., backgroundColor: "oklch(0.21 0.01 260)") to createTheme and asserts it returns a valid ThemeConfig with fallback theme properties for both light and dark base contexts, instead of throwing.
There was a problem hiding this comment.
Already covered — the test suite includes createTheme fallback on color parsing errors with three test cases: light base fallback, dark base fallback, and baseThemeConfig-provided fallback, all using oklch() as the unsupported color. Added a fourth test for inSidebar in the fallback path in 77eeb4e.
| } | ||
| } catch (e) { | ||
| LOG.warn( | ||
| `Failed to create theme "${themeName}" due to a color parsing error. ` + |
There was a problem hiding this comment.
suggestion: The warning says "Falling back to the default light theme" but the actual fallback (lines 1217-1220) selects either darkTheme or lightTheme based on themeInput.base. This is misleading for developers debugging theme issues.
| `Failed to create theme "${themeName}" due to a color parsing error. ` + | |
| LOG.warn( | |
| `Failed to create theme "${themeName}" due to a color parsing error. ` + | |
| `Falling back to the default base theme. Error: ${e}` | |
| ) |
There was a problem hiding this comment.
Already fixed — the warning message was updated in the original commit to say "Falling back to the default base theme" (not "light theme"). Can confirm it reads: Falling back to the default base theme. Error: ${e}.
- Use baseThemeConfig as fallback when provided in createTheme catch block, so injected dark themes resolve to the correct base instead of always falling through to lightTheme. - Fix misleading log message: say 'default base theme' instead of 'default light theme' since the fallback may be dark. - Add unit tests for color fallback paths: bgColorToBaseString with unsupported formats (oklch), createTheme fallback for light/dark/ baseThemeConfig, and blend with unparseable color strings.
…d safe wrapper tests - Change safeGetLuminance default fallback from 0.5 to 0.51 so unparseable colors default to light theme rather than hitting the borderline threshold - Apply inSidebar parameter to the fallback theme in the createTheme catch block - Add tests for unsupported CSS Color Level 4 formats (oklch, display-p3, lab) - Add test verifying inSidebar propagates to fallback theme - Add tests verifying safeGetLuminance returns consistent results for valid colors
|
This pull request has had no activity for 14 days, so it has been marked as stale. If you still want to continue this work, please leave a comment or push a commit within 7 days. A maintainer can also apply the |
Describe your changes
Fixes #14573 — Theme colors using CSS OKLCH syntax crash the frontend and block app load.
When a theme color uses a CSS Color Level 4 format like
oklch(0.21 0.01 260), thecolor2klibrary'sparseToRgba()throws an unhandledColorErrorthat propagates throughcreateTheme()→createEmotionTheme()→App.processThemeInput(), crashing the entire React app.This PR adds graceful error handling so the app remains usable even when unsupported color formats are provided:
Changes
safeGetLuminance()— Wraps allgetLuminance()calls with try-catch, falling back to0.5when color parsing fails. This affects light/dark theme detection inbgColorToBaseString(),setBackgroundColors(),setTextColors(),createTheme(), andcreateSidebarTheme().safeColorTransform()— Wraps color manipulation functions (transparentize,darken,lighten) with try-catch, returning the original color string unchanged when transformation fails. This protectsresolveBgColor(),resolveTextColor(), and border color derivation.createTheme()top-level try-catch — Wraps the entire theme creation pipeline so that if any downstreamcolor2kcall throws (including ingetColors.tsorgetShadows.ts), the app falls back to the appropriate default base theme (light or dark) instead of crashing.blend()try-catch — Wraps theparseToRgba-based alpha blending with graceful fallback.All error paths log warnings via the existing
LOGinstance so developers can diagnose unsupported color values in the console without the app becoming unusable.Behavior After Fix
GitHub Issue Link (if applicable)
Fixes #14573
Testing Plan
STREAMLIT_THEME_BACKGROUND_COLOR="oklch(0.21 0.01 260)" streamlit run app.py— app loads with default theme instead of crashing