bito-code-review[bot] commented on code in PR #44148:
URL: https://github.com/apache/superset/pull/44148#discussion_r4154681439
##########
superset-frontend/plugins/plugin-chart-world-map/src/WorldMap.ts:
##########
@@ -191,7 +193,8 @@ function WorldMap(element: HTMLElement, props:
WorldMapProps): void {
const selected = Object.values(filterState.selectedValues || {});
const key = source.id || source.country;
const country =
- countryFieldtype === 'name' ? mapData[key]?.name : mapData[key]?.code;
+ mapData[key]?.sourceValue ??
+ (countryFieldtype === 'name' ? mapData[key]?.name : mapData[key]?.code);
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Duplicated value resolution</b></div>
<div id="fix">
The same two-line `mapData[key]?.sourceValue ?? (countryFieldtype === 'name'
? ...)` resolution now lives in both `getCrossFilterDataMask` (196-197) and
`handleContextMenu` (251-252). Since cross-filter and context-menu filters must
yield identical values, extract a small `getCountryValue(key)` helper and call
it from both handlers to prevent future divergence.
</div>
</div>
<small><i>Code Review Run #a4eabd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
superset-frontend/plugins/plugin-chart-country-map/src/CountryMap.ts:
##########
@@ -178,6 +182,13 @@ function CountryMap(element: HTMLElement, props:
CountryMapProps) {
// Track mouse position to distinguish clicks from drags
let mousedownPos: { x: number; y: number } | null = null;
+ const sourceValue = (code: string) => {
+ if (!sourceValues) return code;
+ return Object.prototype.hasOwnProperty.call(sourceValues, code)
+ ? sourceValues[code]
+ : undefined;
+ };
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Untested new guard behavior</b></div>
<div id="fix">
The rewritten `sourceValue` now returns `undefined` for codes missing from
`sourceValues`, and the new guards in `getCrossFilterDataMask` (line 200) and
`handleContextMenu` (line 239) silently suppress clicks/drills on those
regions. No test in `CountryMap.test.tsx` passes `sourceValues`, so this new
undefined-guard behavior is unexercised; a revert of line 189 to the old `:
code` fallback would pass the suite. Consider component tests covering both
branches.
</div>
</div>
<small><i>Code Review Run #a4eabd</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
