Skip to content

fix: improve invalid color error messages for literal color props#6433

Open
BABTUNA wants to merge 5 commits intoreflex-dev:mainfrom
BABTUNA:fix-4955-invalid-color-error-message
Open

fix: improve invalid color error messages for literal color props#6433
BABTUNA wants to merge 5 commits intoreflex-dev:mainfrom
BABTUNA:fix-4955-invalid-color-error-message

Conversation

@BABTUNA
Copy link
Copy Markdown
Contributor

@BABTUNA BABTUNA commented Apr 30, 2026

Summary

  • validate literal CSS color values with clearer errors for color-like style keys
  • apply the same literal color validation to typed color props whose type includes Color (for example Var[str | Color])
  • add regressions for invalid style color values, valid color formats, and typed color prop rejection

Validation

  • uv run ruff check packages/reflex-base/src/reflex_base/style.py packages/reflex-base/src/reflex_base/components/component.py tests/units/test_style.py tests/units/components/test_component.py
  • uv run pytest tests/units/test_style.py tests/units/components/test_component.py -k "invalid_typed_color_prop_raises_error or invalid_literal_color_style_value_raises_error or valid_literal_color_style_values_are_accepted" -q

Closes #4955

Validate literal CSS color strings with clearer errors for color-like style keys and typed component color props (e.g. Var[str | Color]).

Adds regressions for invalid style color values, valid color formats, and typed color prop rejection.
@BABTUNA BABTUNA requested a review from a team as a code owner April 30, 2026 22:55
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Apr 30, 2026

Greptile Summary

This PR adds early-stage validation for literal CSS color string values, both in the Style dictionary (convert_item) and on typed component props whose type includes Color (e.g., Var[str | Color]). It also refactors the expected_type derivation in component.py to safely handle edge cases where get_args returns an empty tuple, which was a latent IndexError risk.

Confidence Score: 4/5

Safe to merge; all findings are style suggestions only, no logic bugs identified.

All review findings are P2 (style/maintainability). The core validation logic is correct, test coverage is solid, and the component.py refactor is a net improvement over the original code.

packages/reflex-base/src/reflex_base/style.py — three duplicate frozensets and the permissive CSS-function suffix check are worth a second look before this pattern is extended.

Important Files Changed

Filename Overview
packages/reflex-base/src/reflex_base/style.py Adds CSS color validation logic (~330 new lines): color style key set, keyword list, regex, and validate_literal_css_color_value. Three identical frozensets for SVG paint keys are a minor maintainability concern; the function-prefix heuristic is intentionally permissive rather than a full CSS parser.
packages/reflex-base/src/reflex_base/components/component.py Refactors expected_type computation to avoid a potential IndexError when get_args returns an empty tuple, and adds color validation for typed `Var[str
tests/units/test_style.py Adds regression tests for invalid and valid literal color style values; coverage is good across named colors, hex, CSS functions, var(), url(), and context-stroke.
tests/units/components/test_component.py Adds a test verifying that a typed `Var[str

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Style dict or Component prop assignment] --> B{Is value a plain str?}
    B -- No --> Z[Pass through unchanged]
    B -- Yes --> C{Is key a color style key?}
    C -- No --> Z
    C -- Yes --> D[_is_valid_css_color_value]
    D --> E{Contains REFLEX_VAR tag?}
    E -- Yes --> OK[Valid]
    E -- No --> F{Ends with important?}
    F -- Yes --> G[Strip suffix, re-check]
    F -- No --> H{Is none?}
    G --> H
    H -- Yes --> I{fill or stroke key?}
    I -- Yes --> OK
    I -- No --> ERR[ValueError raised]
    H -- No --> J{context-fill or context-stroke?}
    J -- Yes --> K{fill or stroke key?}
    K -- Yes --> OK
    K -- No --> ERR
    J -- No --> L{Named CSS color keyword?}
    L -- Yes --> OK
    L -- No --> M{Hex color format?}
    M -- Yes --> OK
    M -- No --> N{url prefix and fill or stroke key?}
    N -- Yes --> OK
    N -- No --> O{CSS color function prefix and ends with paren?}
    O -- Yes --> OK
    O -- No --> ERR
Loading

Reviews (1): Last reviewed commit: "fix: improve invalid color errors for li..." | Re-trigger Greptile

Comment on lines +322 to +335
_CSS_URL_COLOR_KEYS = frozenset({
"fill",
"stroke",
})

_CSS_NONE_COLOR_KEYS = frozenset({
"fill",
"stroke",
})

_CSS_CONTEXT_PAINT_KEYS = frozenset({
"fill",
"stroke",
})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Three identical frozensets for SVG paint keys

_CSS_URL_COLOR_KEYS, _CSS_NONE_COLOR_KEYS, and _CSS_CONTEXT_PAINT_KEYS are all frozenset({"fill", "stroke"}). If a new SVG color property is added later (e.g., marker-start), all three must be updated in sync — a future maintainer may update only one and silently break the other two checks. A single shared constant would make the relationship explicit.

# All three SVG paint-specific key sets currently share the same members.
_SVG_PAINT_KEYS = frozenset({"fill", "stroke"})

_CSS_URL_COLOR_KEYS = _SVG_PAINT_KEYS
_CSS_NONE_COLOR_KEYS = _SVG_PAINT_KEYS
_CSS_CONTEXT_PAINT_KEYS = _SVG_PAINT_KEYS

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok I'll clean it up a bit

Comment on lines +389 to +392
return lower.endswith(")") and any(
lower.startswith(prefix)
for prefix in _CSS_COLOR_FUNCTION_PREFIXES + _CSS_COLOR_VALUE_PREFIXES
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 CSS function prefix check doesn't guard the closing parenthesis position

The check lower.endswith(")") and any(lower.startswith(prefix) ...) accepts structurally odd strings like "rgb(foo) bar()" where the closing paren is not the direct closing paren of the function. This is a known limitation of not doing full CSS parsing. If future stricter validation is added, this is the line to revisit. At minimum, a brief comment documenting this intentional trade-off would help future maintainers.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is intentionally permissive. Will add comment

BABTUNA added 2 commits April 30, 2026 19:10
Resolve component.py conflict and keep literal color validation logic.

Also apply review cleanup: share SVG paint key constants and document permissive function-prefix parsing check in style validation.
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Apr 30, 2026

Merging this PR will not alter performance

✅ 17 untouched benchmarks
⏩ 2 skipped benchmarks1


Comparing BABTUNA:fix-4955-invalid-color-error-message (4a9b718) with main (ada8c53)2

Open in CodSpeed

Footnotes

  1. 2 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (c593a7d) during the generation of this report, so ada8c53 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Better Error Message for Invalid Colors

1 participant