feat: add custom colors and thresholds for CountryMap plugin #36103
Conversation
Code Review Agent Run #3fbf4fActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![korbit-ai[bot]](https://avatars.githubusercontent.com/in/322216?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/ReactCountryMap.jsx | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts | ✅ |
| superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
There was a problem hiding this comment.
Pull Request Overview
This PR adds JSON-based customization capabilities to the CountryMap plugin, allowing users to override the default linear color scheme with custom colors and percentage-based thresholds. The feature is configurable through a new "Custom Color Scale" setting in the Chart Options panel.
Key Changes
- Added a new
customColorScaleTextAreaControl that accepts JSON configuration for color thresholds defined by percentages - Implemented color priority logic: conditional rules > custom percentage scale > linear palette > gradient fallback
- Added helper functions for color normalization, safe number parsing, and RGBA to hex conversion
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 17 comments.
| File | Description |
|---|---|
| transformProps.js | Parses JSON configuration from form data for custom color rules and scales |
| controlPanel.ts | Adds new TextAreaControl for custom color scale configuration with JSON syntax |
| ReactCountryMap.jsx | Minor formatting adjustments (whitespace) |
| CountryMap.js | Core implementation of custom color scaling logic with multiple fallback strategies |
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/controlPanel.ts
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/transformProps.js
Outdated
Show resolved
Hide resolved
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js
Show resolved
Hide resolved
|
Latest commit fixes an unwanted interpolation issue that would create scales between custom-defined steps (while we want ranges with the same color instead). |
|
Hi, |
|
Looks like pre-commit might clear up some of the CI issues. |
|
Thank you @rusackas i installed and setup pre-commit for the project, and should update the PR in the beginning of the week |
|
Hello @rusackas, |
|
CodeAnt AI is reviewing your PR. |
codeant-ai-for-open-source
bot
added
the
size:L
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| export interface VirtualTableProps<RecordType> | ||
| extends AntTableProps<RecordType> { | ||
| export interface VirtualTableProps< | ||
| RecordType, |
There was a problem hiding this comment.
Suggestion: Inconsistent generic constraint: the VirtualTable component uses RecordType extends object in its generic, but the new VirtualTableProps declaration does not constrain RecordType; make the interface generic consistent by adding extends object to avoid allowing incompatible types and to keep type constraints aligned. [type error]
Severity Level: Minor
| RecordType, | |
| RecordType extends object, |
Why it matters? ⭐
Good, actionable suggestion. The component signature uses RecordType extends object
while the props interface currently allows any type. Adding extends object keeps
the API consistent and prevents accidental use with primitive record types that
would be incompatible with how the component expects row objects.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/packages/superset-ui-core/src/components/Table/VirtualTable.tsx
**Line:** 35:35
**Comment:**
*Type Error: Inconsistent generic constraint: the `VirtualTable` component uses `RecordType extends object` in its generic, but the new `VirtualTableProps` declaration does not constrain `RecordType`; make the interface generic consistent by adding `extends object` to avoid allowing incompatible types and to keep type constraints aligned.
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.| export interface ColumnInterface<D extends object> | ||
| extends UseGlobalFiltersColumnOptions<D>, | ||
| UseSortByColumnOptions<D> { | ||
| extends UseGlobalFiltersColumnOptions<D>, UseSortByColumnOptions<D> { |
There was a problem hiding this comment.
Suggestion: The ColumnInterface was extended with UseGlobalFiltersColumnOptions<D>, which is not imported in this file and will cause a TypeScript unresolved type error; remove the unreferenced type (or import the correct column-level global filter type) — minimal fix shown removes the unimported symbol and keeps the already-imported UseSortByColumnOptions. [type error]
Severity Level: Minor
| extends UseGlobalFiltersColumnOptions<D>, UseSortByColumnOptions<D> { | |
| extends UseSortByColumnOptions<D> { |
Why it matters? ⭐
This is a valid, actionable fix: UseGlobalFiltersColumnOptions isn't imported and likely wasn't intended here. Dropping it and keeping UseSortByColumnOptions fixes the unresolved type while preserving sort-related typings.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/plugin-chart-table/src/DataTable/types/react-table.d.ts
**Line:** 99:99
**Comment:**
*Type Error: The `ColumnInterface` was extended with `UseGlobalFiltersColumnOptions<D>`, which is not imported in this file and will cause a TypeScript unresolved type error; remove the unreferenced type (or import the correct column-level global filter type) — minimal fix shown removes the unimported symbol and keeps the already-imported `UseSortByColumnOptions`.
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.| if (Array.isArray(scale)) return scale; | ||
| if (typeof scale === 'string') { | ||
| try { | ||
| return JSON.parse(scale); |
There was a problem hiding this comment.
Suggestion: normalizeScale currently returns an empty array if the JSON-parsed value is an object (not an array). If the caller passes a single scale object serialized as JSON, it will be ignored. Update normalizeScale to accept a JSON string that parses to either an array or a single object by wrapping a parsed object into an array. [logic error]
Severity Level: Minor
| return JSON.parse(scale); | |
| const parsed = JSON.parse(scale); | |
| // If a single object was provided, wrap it into an array for consistent downstream handling | |
| if (Array.isArray(parsed)) return parsed; | |
| if (parsed && typeof parsed === 'object') return [parsed]; | |
| return []; |
Why it matters? ⭐
Correct. Currently a JSON string that parses to a single object will be returned as an object and later discarded because callers expect an array (normalizedScaleWithColors uses Array.isArray). Wrapping a parsed object into an array makes downstream code behave as intended and fixes a real 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:** 99:99
**Comment:**
*Logic Error: `normalizeScale` currently returns an empty array if the JSON-parsed value is an object (not an array). If the caller passes a single scale object serialized as JSON, it will be ignored. Update `normalizeScale` to accept a JSON string that parses to either an array or a single object by wrapping a parsed object into an array.
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.| stroke: ${theme.colorSplit}; | ||
| } | ||
| g.text-layer text, g.text-layer text.big-text, g.text-layer text.result-text { |
There was a problem hiding this comment.
Suggestion: The selector list g.text-layer text, g.text-layer text.big-text, g.text-layer text.result-text is redundant because g.text-layer text already matches text elements with classes; keep a single selector to avoid duplication and reduce maintenance risk. [logic error]
Severity Level: Minor
| g.text-layer text, g.text-layer text.big-text, g.text-layer text.result-text { | |
| g.text-layer text { |
Why it matters? ⭐
The selector g.text-layer text already matches all descendant elements, including
those with big-text and result-text classes. Consolidating to a single selector reduces
duplication and maintenance burden without changing behavior.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/plugins/legacy-plugin-chart-country-map/src/ReactCountryMap.jsx
**Line:** 78:78
**Comment:**
*Logic Error: The selector list `g.text-layer text, g.text-layer text.big-text, g.text-layer text.result-text` is redundant because `g.text-layer text` already matches text elements with classes; keep a single selector to avoid duplication and reduce maintenance risk.
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.| parsedColorRules = customColorRules ? JSON.parse(customColorRules) : []; | ||
| } catch (error) { | ||
| console.warn( | ||
| 'Invalid JSON in customColorRules. Please check your configuration syntax:', | ||
| error && error.message ? error.message : error, | ||
| ); |
There was a problem hiding this comment.
Suggestion: Passing non-string values (objects/arrays) to JSON.parse will throw a runtime error or a SyntaxError; ensure you only call JSON.parse on strings and otherwise accept already-parsed objects/arrays or fallback to an empty array. Also explicitly reset parsedColorRules in the catch so its value is predictable after a failed parse. [type error]
Severity Level: Minor
| parsedColorRules = customColorRules ? JSON.parse(customColorRules) : []; | |
| } catch (error) { | |
| console.warn( | |
| 'Invalid JSON in customColorRules. Please check your configuration syntax:', | |
| error && error.message ? error.message : error, | |
| ); | |
| if (!customColorRules) { | |
| parsedColorRules = []; | |
| } else if (typeof customColorRules === 'string') { | |
| parsedColorRules = JSON.parse(customColorRules); | |
| } else { | |
| // already an object/array — accept as-is | |
| parsedColorRules = customColorRules; | |
| } | |
| } catch (error) { | |
| console.warn( | |
| 'Invalid JSON in customColorRules. Please check your configuration syntax:', | |
| error && error.message ? error.message : error, | |
| ); | |
| parsedColorRules = []; |
Why it matters? ⭐
The current code unconditionally calls JSON.parse when a value exists — if the caller already passes an object/array, JSON.parse will throw. Guarding on typeof and accepting already-parsed objects avoids unnecessary runtime errors and makes the behavior predictable. Resetting to [] in the catch also makes the post-error state explicit (though parsedColorRules is initialized to [] already).
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:** 35:40
**Comment:**
*Type Error: Passing non-string values (objects/arrays) to JSON.parse will throw a runtime error or a SyntaxError; ensure you only call JSON.parse on strings and otherwise accept already-parsed objects/arrays or fallback to an empty array. Also explicitly reset `parsedColorRules` in the catch so its value is predictable after a failed parse.
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.| setShowTooltip( | ||
| Boolean( | ||
| !isTransitioning && | ||
| (!isActive || | ||
| (chartNameRef.current && | ||
| chartNameRef.current.scrollWidth > | ||
| chartNameRef.current.clientWidth)), | ||
| (!isActive || | ||
| (chartNameRef.current && | ||
| chartNameRef.current.scrollWidth > | ||
| chartNameRef.current.clientWidth)), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
Suggestion: The boolean calculation is repeated and uses Boolean(...) with a complex inline expression; factor out the measured element once, use optional chaining/explicit check, and set a clear boolean to avoid subtle coercion and repeated property access. [logic error]
Severity Level: Minor
| setShowTooltip( | |
| Boolean( | |
| !isTransitioning && | |
| (!isActive || | |
| (chartNameRef.current && | |
| chartNameRef.current.scrollWidth > | |
| chartNameRef.current.clientWidth)), | |
| (!isActive || | |
| (chartNameRef.current && | |
| chartNameRef.current.scrollWidth > | |
| chartNameRef.current.clientWidth)), | |
| ), | |
| ); | |
| { | |
| const el = chartNameRef.current; | |
| const isOverflowing = el ? el.scrollWidth > el.clientWidth : false; | |
| setShowTooltip(!isTransitioning && (!isActive || isOverflowing)); | |
| } |
Why it matters? ⭐
Factoring out chartNameRef.current into a local variable and using an explicit boolean improves readability and avoids repeated property access; it doesn't change semantics and is a safe, beneficial refactor.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset-frontend/src/explore/components/controls/VizTypeControl/VizTile.tsx
**Line:** 56:64
**Comment:**
*Logic Error: The boolean calculation is repeated and uses `Boolean(...)` with a complex inline expression; factor out the measured element once, use optional chaining/explicit check, and set a clear boolean to avoid subtle coercion and repeated property access.
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. |
💡 Enhance Your PR ReviewsWe noticed that 3 feature(s) are not configured for this repository. Enabling these features can help improve your code quality and workflow: 🚦 Quality GatesStatus: Quality Gates are not enabled at the organization level 🎫 Jira Ticket ComplianceStatus: Jira credentials file not found. Please configure Jira integration in your settings ⚙️ Custom RulesStatus: No custom rules configured. Add rules via organization settings or .codeant/review.json in your repository Want to enable these features? Contact your organization admin or check our documentation for setup instructions. |
|
Hi, |
Facyla
force-pushed
the
feat/custom-color-scales-for-countrymap-plugin
branch
from
28a6282 to
73fb398
Compare
github-actions
bot
added
i18n
github-actions
bot
added
api
Facyla
force-pushed
the
feat/custom-color-scales-for-countrymap-plugin
branch
from
73fb398 to
6f8052b
Compare
|
Hi, |
2 participants
User description
SUMMARY
Add a JSON-based customisation block that allows to override linearColorScheme colors and thresholds.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
A new setting is available is "Chart Options" panel
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION
CodeAnt-AI Description
Add custom color rules and percent-based color scales to the Country Map chart
What Changed
Impact
✅ Custom color steps for Country Map✅ Easier color selection for custom scales✅ Clearer handling of invalid JSON and non-numeric metrics💡 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.