feat: add custom thresholds and colors to CountryMap plugin #36732
Conversation
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai-for-open-source
bot
added
the
size:L
Nitpicks 🔍
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| // Named CSS colors and system colors | ||
| const s = new Option().style; | ||
| s.color = c.toLowerCase(); | ||
| if (s.color) return c; |
There was a problem hiding this 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 🚨
| // Named CSS colors and system colors | |
| const s = new Option().style; | |
| s.color = c.toLowerCase(); | |
| if (s.color) return c; | |
| // 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 | |
| } |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.|
|
||
| function rgbaToHex(rgba) { | ||
| if (typeof rgba === 'string') return rgba; | ||
| if (Array.isArray(rgba)) return rgbaToHex(rgba[0]); |
There was a problem hiding this 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
| if (Array.isArray(rgba)) return rgbaToHex(rgba[0]); | |
| 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]); | |
| } |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.| @@ -53,22 +108,127 @@ function CountryMap(element, props) { | |||
| country, | |||
| linearColorScheme, | |||
| numberFormat, | |||
| customColorScale = [], | |||
There was a problem hiding this 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
| customColorScale = [], | |
| customColorScale = props.customColorRules || [], |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.| let parsedColorScale = []; | ||
|
|
||
| try { | ||
| parsedColorScale = customColorScale ? JSON.parse(customColorScale) : []; |
There was a problem hiding this 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
| parsedColorScale = customColorScale ? JSON.parse(customColorScale) : []; | |
| 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 = []; | |
| } |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.| @@ -35,5 +47,6 @@ export default function transformProps(chartProps) { | |||
| numberFormat, | |||
| colorScheme, | |||
| sliceId, | |||
| customColorScale: parsedColorScale, | |||
There was a problem hiding this 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 🚨
| customColorScale: parsedColorScale, | |
| customColorScale: Array.isArray(parsedColorScale) ? parsedColorScale : [], |
Why it matters? ⭐
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.
Prompt for AI Agent 🤖
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.
|
CodeAnt AI finished reviewing your PR. |
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #d66de9
Actionable Suggestions - 2
-
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js - 2
- Prop name mismatch in PropTypes · Line 44-44
- Incorrect Color Scale Implementation · Line 159-169
Additional Suggestions - 3
-
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js - 1
-
Convert JS to TS for type safety · Line 27-27This JavaScript file should be converted to TypeScript to align with codebase standards that prohibit new JavaScript files and require type safety. The added customColorScale parsing logic is correct but would benefit from TypeScript types for better maintainability.
-
-
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js - 2
-
Outdated CSS color regexes · Line 57-60The regexes for CSS color functions only match comma-separated syntax (e.g., rgb(255, 0, 0)), but modern CSS supports space-separated syntax (e.g., rgb(255 0 0)). This could cause valid modern color strings to be rejected as invalid, falling back to '#000000'.
-
Unused function (dead code) · Line 78-89The rgbaToHex function is defined but never called in the codebase. While it might be intended for future color handling, removing it now keeps the code clean and avoids maintenance overhead.
-
Review Details
-
Files reviewed - 3 · Commit Range:
ca2e1b8..30fcc46- superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
- superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts
- superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js
-
Files skipped - 0
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- Eslint (Linter) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
| @@ -41,10 +41,65 @@ const propTypes = { | |||
| linearColorScheme: PropTypes.string, | |||
| mapBaseUrl: PropTypes.string, | |||
| numberFormat: PropTypes.string, | |||
| customColorRules: PropTypes.array, | |||
There was a problem hiding this comment.
The PropTypes definition adds 'customColorRules' as an array prop, but the component destructures 'customColorScale' from props. This mismatch could prevent the prop from being properly validated or used, potentially leading to incorrect color scaling behavior if the prop is passed.
Code Review Run #d66de9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
| 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 () { | ||
| return a; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
The percentColorScale uses d3.scale.linear with a custom interpolate function that always returns the lower boundary color, which prevents the last color in the range from ever being used. For step-based color scales with explicit thresholds, d3.scale.threshold should be used instead, as it properly maps intervals to discrete colors without this limitation.
Code Review Run #d66de9
Should Bito avoid suggestions like this for future reviews? (Manage Rules)
- Yes, avoid them
1 participant
User description
SUMMARY
Add a JSON-based customization block that allows to override linearColorScheme colors and thresholds with custom thresholds and colors.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
Paste a sample customization in "Custom Color Scale": the linearScheme is overrided, resulting in the customized Color Scheme to apply.
[ { "percent": 0, "color": "white" }, { "percent": 0.01, "color": "#FF0000AA" }, { "percent": 20, "color": "#FA0" }, { "percent": 50, "color": "#FF0" }, { "percent": 65, "color": "#9F0" }, { "percent": 99.99, "color": "#000A" }, { "percent": 100, "color": "black" } ]The inline help provides a sample configuration that can also be copied/pasted (with some cleaning and lines returns).
Edit sample values to adjust to the map needs: the changes appear immediately on map
The provided configuration requires a full valid JSON, including brackets, commas except for the last line, quoted keys and quoted values for color codes.
ADDITIONAL INFORMATION
CodeAnt-AI Description
Allow custom percent-based color scales for CountryMap and add a color picker
What Changed
Impact
✅ Clearer thematic maps with user-defined thresholds✅ Immediate visual feedback when editing color scales✅ Fewer rendering errors from invalid custom color inputs💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.