codeant-ai-for-open-source[bot] commented on code in PR #36732:
URL: https://github.com/apache/superset/pull/36732#discussion_r2769933551
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,10 +41,58 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ customColorScale: PropTypes.array,
};
const maps = {};
Review Comment:
**Suggestion:** The `customColorScale` prop is typed as an array in
`propTypes`, but the implementation explicitly supports both arrays and JSON
strings (parsed by `normalizeScale`), so passing a string (as the UI text area
does) will constantly produce React prop-type warnings and can mislead future
maintainers about the expected type. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Dev console displays prop-type warnings for Custom Color Scale.
- ⚠️ Plugin maintainers misled about expected prop type.
```
</details>
```suggestion
customColorScale: PropTypes.oneOfType([PropTypes.array, PropTypes.string]),
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the CountryMap visualization and paste a JSON string into the
"Custom Color Scale"
textarea (the UI sends a string). The CountryMap component receives props
and destructures
customColorScale at CountryMap.js:96-107 (the defaulting to
props.customColorScale is at
line ~104).
2. CountryMap calls normalizeScale(customColorScale) at CountryMap.js:111
(normalizeScale
defined at CountryMap.js:84-94), which accepts both arrays and JSON strings,
so a string
input is valid and parsed.
3. However prop-types for the component are defined at CountryMap.js:34-49
and line 48
currently reads "customColorScale: PropTypes.array," — React prop-types
runtime validation
(in development) will log a warning when a string is passed.
4. Reproduce by running the dev server, inputting a JSON string as
described, and
observing a console warning: "Invalid prop `customColorScale` of type
`string` supplied to
`CountryMap`, expected `array`."
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
**Line:** 48:48
**Comment:**
*Type Error: The `customColorScale` prop is typed as an array in
`propTypes`, but the implementation explicitly supports both arrays and JSON
strings (parsed by `normalizeScale`), so passing a string (as the UI text area
does) will constantly produce React prop-type warnings and can mislead future
maintainers about the expected type.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -53,22 +101,129 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorScale = props.customColorScale || [],
colorScheme,
sliceId,
} = props;
const container = element;
const format = getNumberFormatter(numberFormat);
- const linearColorScale = getSequentialSchemeRegistry()
- .get(linearColorScheme)
- .createLinearScale(d3Extent(data, v => v.metric));
- const colorScale = CategoricalColorNamespace.getScale(colorScheme);
+ const normalizedScale = normalizeScale(customColorScale);
+ const normalizedScaleWithColors = Array.isArray(normalizedScale)
+ ? normalizedScale.map(e => {
+ if (!e || typeof e !== 'object') return e;
+ return { ...e, color: normalizeColorKeyword(e.color) };
+ })
+ : [];
+
+ const parsedData = Array.isArray(data)
+ ? data.map(r => ({ ...r, metric: safeNumber(r.metric) }))
+ : [];
+
+ const numericValues = parsedData
+ .map(r => r.metric)
+ .filter(v => Number.isFinite(v));
+
+ let minValue = 0;
+ let maxValue = 1;
+ if (numericValues.length > 0) {
+ const extent = d3Extent(numericValues);
+ minValue = extent[0];
+ maxValue = extent[1];
+ }
+ const valueRange = maxValue - minValue;
+ const valueRangeNonZero = valueRange === 0 ? 1 : valueRange;
+
+ let percentColorScale = null;
+ if (
+ Array.isArray(normalizedScaleWithColors) &&
+ normalizedScaleWithColors.length >= 2
+ ) {
+ const sorted = normalizedScaleWithColors
+ .filter(
+ e =>
+ e &&
+ typeof e.percent === 'number' &&
+ e.percent >= 0 &&
+ e.percent <= 100 &&
+ typeof e.color === 'string',
+ )
+ .slice()
+ .sort((a, b) => a.percent - b.percent);
+
+ if (sorted.length >= 2) {
+ const domainPerc = sorted.map(e => e.percent);
+ const rangeColors = sorted.map(e => e.color);
+ percentColorScale = d3.scale
+ .linear()
+ .domain(domainPerc)
+ .range(rangeColors)
+ .clamp(true)
+ // Remove interpolation to avoid blending between steps - always
return lower boundary
+ .interpolate(function (a, b) {
+ return function(t) { return a; };
+ });
+ }
+ }
+
+ let linearPaletteScale = null;
+ if (linearColorScheme) {
+ try {
+ const seq = getSequentialSchemeRegistry().get(linearColorScheme);
+ if (seq && typeof seq.createLinearScale === 'function') {
+ linearPaletteScale = seq.createLinearScale([minValue, maxValue]);
+ } else if (seq && Array.isArray(seq.colors) && seq.colors.length >= 2) {
+ linearPaletteScale = d3.scale
+ .linear()
+ .domain([minValue, maxValue])
+ .range([seq.colors[0], seq.colors[seq.colors.length - 1]])
+ .interpolate(d3.interpolateRgb);
+ }
+ } catch {
+ linearPaletteScale = null;
+ }
+ }
const colorMap = {};
- data.forEach(d => {
- colorMap[d.country_id] = colorScheme
- ? colorScale(d.country_id, sliceId)
- : linearColorScale(d.metric);
+ parsedData.forEach(r => {
+ const iso = r.country_id;
+ const value = r.metric;
+ if (!iso) return;
+ if (!Number.isFinite(value)) {
+ colorMap[iso] = 'none';
+ return;
+ }
+
+ if (percentColorScale) {
+ if (minValue === maxValue) {
+ // All values are the same; map to central color (e.g., 50%)
+ try {
Review Comment:
**Suggestion:** The hover handler darkens the region fill by calling
`d3.rgb(c).darker()` on whatever CSS color string is in use, but D3 v3 cannot
parse some valid CSS colors you now explicitly support (like `#RRGGBBAA`),
producing `rgb(NaN,NaN,NaN)` and breaking the hover darkening effect for those
colors. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ CountryMap hover darkening visual effect broken.
- ⚠️ Regions render invalid fill values on hover.
```
</details>
```suggestion
try {
const rgbColor = d3.rgb(c);
if (
rgbColor &&
Number.isFinite(rgbColor.r) &&
Number.isFinite(rgbColor.g) &&
Number.isFinite(rgbColor.b)
) {
c = rgbColor.darker().toString();
}
} catch {
// If d3.rgb cannot parse the color, leave it unchanged
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a custom color scale including an 8-digit hex color with alpha,
e.g.
"#FF0000AA". The normalization happens at CountryMap.js:111 where
normalizeColorKeyword is
applied to incoming scale entries.
2. normalizeColorKeyword (CountryMap.js:53-73) allows 8-digit hex
(#RRGGBBAA) and returns
that string unchanged, so the map renders regions with that color.
3. Move the mouse over a region to trigger the hover handler at
CountryMap.js:196-205
(mouseenter). The code calls d3.rgb(c).darker() on the 8-digit-hex string.
4. In D3 v3 (used here), d3.rgb cannot parse some formats like #RRGGBBAA,
producing
rgb(NaN,NaN,NaN) and causing the darker()/toString() result to be invalid —
hover fill
becomes incorrect or visual effect is broken.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:**
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
**Line:** 200:200
**Comment:**
*Logic Error: The hover handler darkens the region fill by calling
`d3.rgb(c).darker()` on whatever CSS color string is in use, but D3 v3 cannot
parse some valid CSS colors you now explicitly support (like `#RRGGBBAA`),
producing `rgb(NaN,NaN,NaN)` and breaking the hover darkening effect for those
colors.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
```
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
