Charts: Fix conversion funnel chart crash on non-hex colors#47786
Charts: Fix conversion funnel chart crash on non-hex colors#47786
Conversation
Add colorToRgba() utility that gracefully handles any valid CSS color format (hex, rgb, hsl, named colors) and returns null for unparseable values instead of throwing. Replace hexToRgba() with colorToRgba() in ConversionFunnelChart to prevent crashes when themes provide non-hex color values. Fixes CHARTS-196
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
github-actions
bot
added
the
[Status] Needs Author Reply
Code Coverage SummaryCoverage changed in 1 file.
|
kangzj
added
[Status] Needs Team Review
hexToRgba was only used in the conversion funnel chart (now migrated) and a leaderboard storybook story. colorToRgba is a strict superset that handles any CSS color format, making the hex-only variant dead code.
| * @param alpha - The opacity value. Values outside the [0, 1] range will be clamped by d3. | ||
| * @return The rgba color string (e.g., 'rgba(255, 0, 0, 0.5)'), or null if the color is invalid | ||
| */ | ||
| export const hexToRgba = ( hex: string, alpha: number ): string => { |
There was a problem hiding this comment.
Since we have exported hexToRgba as part of the public API, should we keep it and mark it as deprecated, or treat this as a breaking change in the changelog?
There was a problem hiding this comment.
Kept hexToRgba as a deprecated wrapper that delegates to colorToRgba, preserving the original throwing behavior for backward compatibility. Added tests for the deprecated function. Resolved in bc3ac3c.
There was a problem hiding this comment.
We are past 1.0 now so I think we need to deprecate, and remove in 2.0
There was a problem hiding this comment.
Agreed and addressed
|
@claude review please |
|
Claude finished @adamwoodnz's task in 1m 29s —— View job Code Review
Overall this is a clean, well-targeted fix. The approach of using d3-color as the parsing backend is solid — it handles the full CSS color gamut correctly. A few items worth discussing:
|
|
@claude I'm curious - this seems like a valid defensive improvement, but is it actually possible for this bug to occur? It appears the provider pipeline prevents non hex colors reaching |
|
Claude finished @adamwoodnz's task in 2m 39s —— View job Analysis: Can non-hex colors reach
|
| Input | Handled? | Output |
|---|---|---|
#ff0000 (hex) |
✅ | #ff0000 |
#f00 (3-digit hex) |
✅ | #ff0000 |
rgb(255, 0, 0) |
✅ → d3-color → formatHex | #ff0000 |
hsl(0, 100%, 50%) |
✅ → d3-color → formatHex | #ff0000 |
var(--my-color) |
✅ → resolved → recurse | hex |
red (named color) |
❌ falls through to return color |
red |
So named CSS colors are the gap — normalizeColorToHex has no branch for them and returns them as-is. The examples in the PR description (rgb(...), hsl(...)) are actually already normalized to hex by the pipeline and wouldn't have crashed with the old code.
A second protection layer
Even with a named primaryColor, the old code only crashed in hexToRgba when computing barBackgroundColor:
// global-charts-provider.tsx:249
const barBackgroundColor =
backgroundColor || colorToRgba( barColor, 0.08 ) || 'rgba(0, 0, 0, 0.08)';The default theme provides backgroundColor: '#F3F4F6', which is always merged in by mergeThemes/deepmerge. So the colorToRgba(barColor, 0.08) branch is only reached if a consumer explicitly overrides backgroundColor to a falsy value ('', or similar).
The triggerable scenario
All three of these must be true simultaneously:
- Consumer provides
primaryColoras a named CSS color (e.g.'red') in the theme'sconversionFunnelChartconfig - Consumer explicitly sets
backgroundColor: ''(or otherwise falsy) in the same config - No
GlobalChartsProviderexists, or the existing provider doesn't normalize named colors
This is a fairly specific combination. It's unlikely to occur in practice today, but it's a legitimate edge case that's worth guarding against defensively — particularly given normalizeColorToHex has a known blind spot for named colors.
The fix is correct. colorToRgba handles named colors via d3-color and returns null on failure, which the fallback || 'rgba(0, 0, 0, 0.08)' covers. Whether it's worth also fixing normalizeColorToHex to handle named colors (closing the gap at source) is a separate conversation.
FYI I've refreshed #46349 which adds support for various color format including named colors, so that would cover the edge case identified. |
github-actions
bot
removed
[Status] In Progress
[Status] Needs Team Review
3 participants
Fixes https://linear.app/a8c/issue/CHARTS-196/find-32-conversion-funnel-chart-crashes-on-non-hex-colors
Proposed changes
colorToRgba()utility that accepts any valid CSS color format (hex, rgb, hsl, named colors) and returnsnullfor unparseable values instead of throwinghexToRgba()withcolorToRgba()inConversionFunnelChartto prevent crashes when themes provide non-hex color values (e.g.,rgb(...),hsl(...), named colors)Other information
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
ConversionFunnelChartinside aGlobalChartsProviderwith a non-hex primary color (e.g.,rgb(255, 0, 0),hsl(0, 100%, 50%), orred)Error: Hex color must start with #