codeant-ai-for-open-source[bot] commented on code in PR #36732:
URL: https://github.com/apache/superset/pull/36732#discussion_r2630494484
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -41,10 +41,65 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ customColorRules: PropTypes.array,
};
const maps = {};
+function normalizeColorKeyword(color) {
+ if (color == null) return '#000000';
+ const c = String(color).trim();
+
+ // Hex colors (#RGB, #RRGGBB, #RGBA, #RRGGBBAA)
+ if (/^#([0-9a-f]{3}|[0-9a-f]{4}|[0-9a-f]{6}|[0-9a-f]{8})$/i.test(c)) return
c;
+
+ // CSS color functions (rgb, rgba, hsl, hsla) with flexible spacing and alpha
+ const colorFuncRegex =
+
/^(rgb|rgba)\(\s*(\d{1,3}%?\s*,\s*){2}\d{1,3}%?(?:\s*,\s*(\d*\.?\d+))?\s*\)$/i;
+ const colorFuncHslRegex =
+ /^(hsl|hsla)\(\s*\d+\s*,\s*\d+%\s*,\s*\d+%(?:\s*,\s*(\d*\.?\d+))?\s*\)$/i;
+ if (colorFuncRegex.test(c) || colorFuncHslRegex.test(c)) return c;
+
+ // Named CSS colors and system colors
+ const s = new Option().style;
+ s.color = c.toLowerCase();
+ if (s.color) return c;
Review Comment:
**Suggestion:** Runtime error risk in non-browser environments: `new
Option()` is used to detect named CSS colors which can throw or be undefined
during server-side rendering or unit tests; guard usage with a typeof check or
try/catch so code doesn't crash outside a browser. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
// Named CSS colors and system colors - guard for non-browser environments
try {
if (typeof Option !== 'undefined') {
const s = new Option().style;
s.color = c.toLowerCase();
if (s.color) return c;
}
} catch {
// ignore environment where Option is not available
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
new Option() is a DOM API and will throw (Option is not defined) in
Node/SSR/test environments.
Wrapping the usage in a typeof check / try-catch as suggested prevents
runtime crashes outside browsers — a meaningful, low-risk hardening.
</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:** 63:66
**Comment:**
*Possible Bug: Runtime error risk in non-browser environments: `new
Option()` is used to detect named CSS colors which can throw or be undefined
during server-side rendering or unit tests; guard usage with a typeof check or
try/catch so code doesn't crash outside a browser.
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 +108,127 @@ function CountryMap(element, props) {
country,
linearColorScheme,
numberFormat,
+ customColorScale = [],
Review Comment:
**Suggestion:** Logic bug: the component reads `customColorScale` from props
but propTypes define `customColorRules`; if the parent passes
`customColorRules` the new code will ignore it and always use an empty array,
breaking custom color rules. Use the actual prop or fall back to
`customColorRules`. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
customColorScale = props.customColorRules || [],
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The PR added propTypes.customColorRules but the component reads
customColorScale from props — that's a real mismatch.
If callers pass customColorRules the current code will ignore them. The
suggested fallback to props.customColorRules is valid and fixes the logic bug.
</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:** 111:111
**Comment:**
*Logic Error: Logic bug: the component reads `customColorScale` from
props but propTypes define `customColorRules`; if the parent passes
`customColorRules` the new code will ignore it and always use an empty array,
breaking custom color rules. Use the actual prop or fall back to
`customColorRules`.
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:
##########
@@ -41,10 +41,65 @@ const propTypes = {
linearColorScheme: PropTypes.string,
mapBaseUrl: PropTypes.string,
numberFormat: PropTypes.string,
+ customColorRules: PropTypes.array,
};
const maps = {};
+function normalizeColorKeyword(color) {
+ if (color == null) return '#000000';
+ const c = String(color).trim();
+
+ // Hex colors (#RGB, #RRGGBB, #RGBA, #RRGGBBAA)
+ if (/^#([0-9a-f]{3}|[0-9a-f]{4}|[0-9a-f]{6}|[0-9a-f]{8})$/i.test(c)) return
c;
+
+ // CSS color functions (rgb, rgba, hsl, hsla) with flexible spacing and alpha
+ const colorFuncRegex =
+
/^(rgb|rgba)\(\s*(\d{1,3}%?\s*,\s*){2}\d{1,3}%?(?:\s*,\s*(\d*\.?\d+))?\s*\)$/i;
+ const colorFuncHslRegex =
+ /^(hsl|hsla)\(\s*\d+\s*,\s*\d+%\s*,\s*\d+%(?:\s*,\s*(\d*\.?\d+))?\s*\)$/i;
+ if (colorFuncRegex.test(c) || colorFuncHslRegex.test(c)) return c;
+
+ // Named CSS colors and system colors
+ const s = new Option().style;
+ s.color = c.toLowerCase();
+ if (s.color) return c;
+
+ // Fallback
+ return '#000000';
+}
+
+function safeNumber(v) {
+ if (v === null || v === undefined || v === '') return NaN;
+ const n = Number(v);
+ return Number.isFinite(n) ? n : NaN;
+}
+
+function rgbaToHex(rgba) {
+ if (typeof rgba === 'string') return rgba;
+ if (Array.isArray(rgba)) return rgbaToHex(rgba[0]);
Review Comment:
**Suggestion:** Incorrect handling of numeric color arrays in `rgbaToHex`:
if `rgba` is an array of numeric channels (e.g. [r,g,b]) the function recurses
into the first numeric element and returns null; explicitly handle numeric
arrays of channels to produce a hex string. [type error]
**Severity Level:** Minor ⚠️
```suggestion
if (Array.isArray(rgba)) {
// Handle numeric channel arrays like [r, g, b]
if (rgba.length >= 3 && typeof rgba[0] === 'number') {
const [r, g, b] = rgba;
if (r === undefined || g === undefined || b === undefined) return null;
const toHex = n => {
const hex = Math.round(n).toString(16);
return hex.length === 1 ? `0${hex}` : hex;
};
return `#${toHex(r)}${toHex(g)}${toHex(b)}`;
}
return rgbaToHex(rgba[0]);
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current Array handling recurses into the first element, so numeric
arrays like [r,g,b] become a number and return null. The proposed explicit
handling of numeric channel arrays fixes that bug and returns a proper hex
string.
</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:** 80:80
**Comment:**
*Type Error: Incorrect handling of numeric color arrays in `rgbaToHex`:
if `rgba` is an array of numeric channels (e.g. [r,g,b]) the function recurses
into the first numeric element and returns null; explicitly handle numeric
arrays of channels to produce a hex string.
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/transformProps.js:
##########
@@ -35,5 +47,6 @@ export default function transformProps(chartProps) {
numberFormat,
colorScheme,
sliceId,
+ customColorScale: parsedColorScale,
Review Comment:
**Suggestion:** The returned `customColorScale` may be null or a non-array
value if parsing returned such a value; ensure the value exported in props is
always an array so downstream consumers that expect an array won't crash.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
customColorScale: Array.isArray(parsedColorScale) ? parsedColorScale :
[],
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Ensuring the returned prop is always an array is sensible defensive
programming. If the parse step is missed or returns null/object, downstream
code expecting an array would break; wrapping with Array.isArray(...) is a
small, safe hardening.
</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/transformProps.js
**Line:** 50:50
**Comment:**
*Possible Bug: The returned `customColorScale` may be null or a
non-array value if parsing returned such a value; ensure the value exported in
props is always an array so downstream consumers that expect an array won't
crash.
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/transformProps.js:
##########
@@ -24,8 +24,20 @@ export default function transformProps(chartProps) {
selectCountry,
colorScheme,
sliceId,
+ customColorScale,
} = formData;
+ let parsedColorScale = [];
+
+ try {
+ parsedColorScale = customColorScale ? JSON.parse(customColorScale) : [];
Review Comment:
**Suggestion:** JSON.parse is being called unconditionally on
`customColorScale` which can be a non-string (already-parsed array/object) or a
string that parses to non-array (e.g., "null"); this will either throw (caught
and silently drop user's config) or produce a non-array value; only parse when
the input is a non-empty string and ensure the parsed result is an array,
otherwise accept an already-array value or fall back to []. [type error]
**Severity Level:** Minor ⚠️
```suggestion
if (typeof customColorScale === 'string') {
const trimmed = customColorScale.trim();
if (trimmed === '') {
parsedColorScale = [];
} else {
const parsed = JSON.parse(trimmed);
parsedColorScale = Array.isArray(parsed) ? parsed : [];
}
} else if (Array.isArray(customColorScale)) {
parsedColorScale = customColorScale;
} else {
parsedColorScale = [];
}
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Good catch — the current line blindly calls JSON.parse on anything truthy
which can throw or produce non-array results (or previously swallow arrays due
to the catch). The proposed guard (only parse strings, accept arrays, ensure
Array.isArray) fixes a real bug and makes the prop stable for consumers.
</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/transformProps.js
**Line:** 33:33
**Comment:**
*Type Error: JSON.parse is being called unconditionally on
`customColorScale` which can be a non-string (already-parsed array/object) or a
string that parses to non-array (e.g., "null"); this will either throw (caught
and silently drop user's config) or produce a non-array value; only parse when
the input is a non-empty string and ensure the parsed result is an array,
otherwise accept an already-array value or fall back to [].
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]
