Moved CSS named-color table to gosub_shared so inline styles can reso…#1137
Conversation
📝 WalkthroughWalkthroughThe change centralizes CSS named-color data and lookup helpers in ChangesShared CSS color lookup
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gosub_shared/src/css_colors.rs (1)
13-30: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueConsider optimizing lookups for high-throughput parsing.
The linear searches over the static arrays using
eq_ignore_ascii_caseare simple and correct. However, if color resolution becomes a hot path during CSS parsing, consider using perfect hash maps (e.g., via thephfcrate) or sorting the arrays alphabetically to enable binary searching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gosub_shared/src/css_colors.rs` around lines 13 - 30, The color lookup functions named_color_hex, is_system_color, and is_named_color currently scan static arrays linearly. Optimize these lookups for high-throughput parsing by replacing the linear searches with a suitable static lookup structure, such as a perfect hash map or alphabetically sorted arrays with binary search, while preserving ASCII case-insensitive matching and existing return behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/gosub_shared/src/css_colors.rs`:
- Around line 13-30: The color lookup functions named_color_hex,
is_system_color, and is_named_color currently scan static arrays linearly.
Optimize these lookups for high-throughput parsing by replacing the linear
searches with a suitable static lookup structure, such as a perfect hash map or
alphabetically sorted arrays with binary search, while preserving ASCII
case-insensitive matching and existing return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1a7c6b99-ab51-4b03-8487-335ec68b496e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/gosub_css3/Cargo.tomlcrates/gosub_css3/src/colors.rscrates/gosub_render_pipeline/src/common/document/inline_style.rscrates/gosub_shared/src/css_colors.rscrates/gosub_shared/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/gosub_css3/Cargo.toml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gosub_render_pipeline/src/common/document/inline_style.rs`:
- Around line 46-47: Update the inline-style function matching around the
rgb/rgba match arms to use ASCII case-insensitive prefix checks, consistent with
the existing transparent handling. Preserve dispatch to parse_rgb and parse_rgba
so uppercase and mixed-case CSS function names are parsed correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ba0147ba-0bb0-4840-a861-0413418b7c41
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/gosub_css3/Cargo.tomlcrates/gosub_css3/src/colors.rscrates/gosub_render_pipeline/src/common/document/inline_style.rscrates/gosub_shared/src/css_colors.rscrates/gosub_shared/src/lib.rs
💤 Files with no reviewable changes (1)
- crates/gosub_css3/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/gosub_shared/src/lib.rs
- crates/gosub_shared/src/css_colors.rs
- crates/gosub_css3/src/colors.rs
| s if s.starts_with("rgb(") => parse_rgb(s), | ||
| s if s.starts_with("rgba(") => parse_rgba(s), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make rgb( and rgba( checks case-insensitive.
CSS function names are ASCII case-insensitive. Because starts_with is case-sensitive, valid inline styles like color: RGB(0, 0, 0) will bypass these match arms, fail the named_color_hex lookup, and incorrectly fall back to a keyword.
Consider using a case-insensitive prefix check to match the behavior implemented for transparent.
💡 Proposed fix
- s if s.starts_with("rgb(") => parse_rgb(s),
- s if s.starts_with("rgba(") => parse_rgba(s),
+ s if s.get(..4).is_some_and(|p| p.eq_ignore_ascii_case("rgb(")) => parse_rgb(s),
+ s if s.get(..5).is_some_and(|p| p.eq_ignore_ascii_case("rgba(")) => parse_rgba(s),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| s if s.starts_with("rgb(") => parse_rgb(s), | |
| s if s.starts_with("rgba(") => parse_rgba(s), | |
| s if s.get(..4).is_some_and(|p| p.eq_ignore_ascii_case("rgb(")) => parse_rgb(s), | |
| s if s.get(..5).is_some_and(|p| p.eq_ignore_ascii_case("rgba(")) => parse_rgba(s), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/gosub_render_pipeline/src/common/document/inline_style.rs` around
lines 46 - 47, Update the inline-style function matching around the rgb/rgba
match arms to use ASCII case-insensitive prefix checks, consistent with the
existing transparent handling. Preserve dispatch to parse_rgb and parse_rgba so
uppercase and mixed-case CSS function names are parsed correctly.
1 participant
…lve named colors
Since we don't want to have dependencies between the render pipeline and css3 crates, we moved the named colors to gosub_shared (better solution available once we came up with one)
Summary by CodeRabbit
transparentfully transparent.transparenthandling.