fix(world-map): preserve bubbles and exclude only null metrics from color scale#39926
fix(world-map): preserve bubbles and exclude only null metrics from color scale#39926massucattoj wants to merge 4 commits into
Conversation
![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 #74c8a4
Actionable Suggestions - 1
-
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.ts - 1
- Bubble rendering inconsistency · Line 167-167
Review Details
-
Files reviewed - 2 · Commit Range:
22d80f1..22d80f1- superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.ts
- superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts
-
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 Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
sadpandajoe
force-pushed
the
fix/world-map-exclude-null-zero-metrics
branch
from
22d80f1 to
9b623fa
Compare
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes World Map “Color by: Metric” behavior so countries with NULL or 0 metric values are treated as “no data” (default grey) rather than being assigned a color from the linear scale.
Changes:
- Filter out rows with
m1 === nullorm1 === 0before computing the color scale and building choropleth data. - Update unit tests to assert those countries are excluded from the datamap
datapayload when coloring by metric.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.ts | Filters out null/0 metric rows before extent calculation and choropleth data construction. |
| superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts | Adjusts tests to validate excluded countries are omitted from datamap data when coloring by metric. |
Comments suppressed due to low confidence (1)
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.ts:171
- After this change,
processedDatais built fromcolorableData, butradiusScaleis still derived earlier fromfilteredData. This can skew bubble sizes for the remaining displayed countries if the excluded rows had large/smallm2values, because the scale’s domain will include data that is no longer rendered. Recommend computing the radius extent/scale from the same dataset that is actually rendered in this branch (e.g.,colorableDatawhencolorBy === Metric), or otherwise ensuring the radius scale domain matchesprocessedData.
processedData = colorableData.map(d => ({
...d,
radius: radiusScale(Math.sqrt(d.m2)),
fillColor: colorFn(d.m1) ?? theme.colorBorder,
}));
| // Exclude null/zero so they render as "no data" instead of getting a scale color. | ||
| const colorableData = filteredData.filter( | ||
| d => d.m1 != null && d.m1 !== 0, | ||
| ); | ||
| const rawExtents = d3Extent(colorableData, d => d.m1); |
| { | ||
| country: 'CAN', | ||
| name: 'Canada', | ||
| m1: null as unknown as number, |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #39926 +/- ##
=======================================
Coverage 64.20% 64.20%
=======================================
Files 2592 2592
Lines 139232 139233 +1
Branches 32327 32327
=======================================
+ Hits 89389 89390 +1
Misses 48308 48308
Partials 1535 1535
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Code Review Agent Run #559e2cActionable 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 |
kgabryje
left a comment
kgabryje
left a comment
There was a problem hiding this comment.
Thanks for the fix — the bug is real and the screenshots show the improvement clearly. I'd like to flag a few concerns before this lands.
1. Bubbles will silently disappear for excluded countries
In WorldMap.ts, processedData is the same array later passed to map.bubbles(processedData). Bubble size is driven by m2, while color is driven by m1. By filtering rows out of processedData based on m1, the fix also removes their bubbles — even when m2 is a perfectly valid number worth visualizing.
(Copilot's reviewer flagged a related symptom: the radius scale domain is computed on filteredData but bubbles are now drawn from colorableData. Same root cause — the fix conflated "exclude from color scale" with "exclude from rendering".)
Suggested shape:
const colorableData = filteredData.filter(d => d.m1 != null && d.m1 !== 0);
const rawExtents = d3Extent(colorableData, d => d.m1);
// ... extents / colorFn unchanged ...
processedData = filteredData.map(d => ({
...d,
radius: radiusScale(Math.sqrt(d.m2)),
fillColor:
d.m1 != null && d.m1 !== 0
? colorFn(d.m1) ?? theme.colorBorder
: theme.colorBorder,
}));That way the color scale domain ignores null/zero, the choropleth fill goes grey for those countries, but bubbles and click/cross-filter interactivity are preserved.
2. Treating 0 as "no data" is a behavior change, not a bug fix
NULL and 0 aren't equivalent:
NULL→ genuinely missing → grey is correct.0→ a real measurement (e.g. "0 sales", "0 incidents", "0 cases"). Rendering it as the scale minimum is the correct choropleth behavior.
This PR will silently hide legitimate zero-value countries for every existing user of the chart, with no feature flag and no linked issue/discussion. It could surprise people whose datasets contain real zeros (which is common — count metrics, KPIs, etc.).
Two options that feel safer:
- Conservative: only exclude
NULL/undefinedin this PR; leave zeros alone. - Configurable: expose a chart control like "Treat zero as no data" and default it off, so users can opt in.
3. Side effect: lost cross-filter / drill-down on excluded countries
Because excluded countries are absent from mapData, handleClick, handleContextMenu, and getCrossFilterDataMask will all silently no-op for them. Defensible if you truly consider those rows missing, but worth calling out — users could previously click an m1=0 country to filter by it.
4. Test assertions weakened
The original test asserted data.USA.fillColor === '#e0e0e0' — the user-visible outcome. The new tests assert expect(data).not.toHaveProperty('CAN') — an implementation detail. The new assertion would still pass if a future refactor dropped the country for some unrelated reason.
Stronger tests would verify both:
- the country still appears in
mapDatawithfillColor === theme.colorBorder(i.e. grey), and - a bubble is still rendered for it when
m2is present (which currently it isn't — see #1).
Things that look good
m1 != null(loose) correctly catches bothnullandundefined.- The empty-
colorableDatacase is handled by the existing[0, 1]extents fallback. - The hover/popup grey fallback via
theme.colorBorderdefaultFill is preserved.
massucattoj
changed the title
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #e64995Actionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
SUMMARY
When using the World Map chart with
Color by: Metric, countries with ametric value of
NULLor0were assigned a color from the linear scaleinstead of being treated as "no data".
NULLvalues often ended up renderedin the darkest shade of the palette, misleadingly suggesting a maximum
value, while
0values were colored as the scale minimum. Both should beindistinguishable from countries that don't appear in the dataset (rendered
in the default grey).
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Before:
After:
TESTING INSTRUCTIONS
"no data" grey, identical to other countries that do not appear in the
dataset.
ADDITIONAL INFORMATION