codeant-ai-for-open-source[bot] commented on code in PR #36323:
URL: https://github.com/apache/superset/pull/36323#discussion_r2585818139
##########
superset-frontend/plugins/legacy-plugin-chart-country-map/src/CountryMap.js:
##########
@@ -128,45 +120,18 @@ function CountryMap(element, props) {
'transform',
`translate(${halfWidth},${halfHeight})scale(${k})translate(${-x},${-y})`,
);
- textLayer
- .style('opacity', 0)
- .attr(
- 'transform',
- `translate(0,0)translate(${x},${hasCenter ? y - 5 : 45})`,
- )
- .transition()
- .duration(750)
- .style('opacity', 1);
- bigText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 6 : 16);
- resultText
- .transition()
- .duration(750)
- .style('font-size', hasCenter ? 16 : 24);
};
backgroundRect.on('click', clicked);
- const selectAndDisplayNameOfRegion = function selectAndDisplayNameOfRegion(
- feature,
- ) {
- let name = '';
+ const getNameOfRegion = function getNameOfRegion(feature) {
if (feature && feature.properties) {
if (feature.properties.ID_2) {
Review Comment:
**Suggestion:** Logic bug: `getNameOfRegion` checks
`feature.properties.ID_2` with a truthy test, which treats zero or other falsy
but valid IDs as "missing" and incorrectly falls back to NAME_1; change the
check to test for presence (null/undefined) instead of truthiness. [logic error]
**Severity Level:** Minor ⚠️
```suggestion
// Use a null/undefined check so valid falsy IDs (e.g. 0) are handled
correctly
if (feature.properties.ID_2 != null) {
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The current truthy test incorrectly treats valid falsy values (e.g. 0) as
absent. Changing to a null/undefined check (!= null) is correct and fixes a
logic bug visible in this hunk.
</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:** 129:129
**Comment:**
*Logic Error: Logic bug: `getNameOfRegion` checks
`feature.properties.ID_2` with a truthy test, which treats zero or other falsy
but valid IDs as "missing" and incorrectly falls back to NAME_1; change the
check to test for presence (null/undefined) instead of truthiness.
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/test/CountryMap.test.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import '@testing-library/jest-dom';
+import { render, fireEvent } from '@testing-library/react';
Review Comment:
**Suggestion:** The test uses `waitFor` but it is not imported; add
`waitFor` to the testing-library import so the fix that waits for async DOM
updates can be used safely. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
import { render, fireEvent, waitFor } from '@testing-library/react';
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Adding `waitFor` is reasonable and necessary if you apply the other
suggestions that replace synchronous DOM queries with `waitFor`. On its own
it'll introduce an unused import until the tests are updated, but in the
context of this PR it's a required, harmless change to enable reliable async
assertions.
</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/test/CountryMap.test.tsx
**Line:** 21:21
**Comment:**
*Race Condition: The test uses `waitFor` but it is not imported; add
`waitFor` to the testing-library import so the fix that waits for async DOM
updates can be used safely.
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/test/CountryMap.test.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import '@testing-library/jest-dom';
+import { render, fireEvent } from '@testing-library/react';
+import d3 from 'd3';
+import ReactCountryMap from '../src/ReactCountryMap';
+
+jest.spyOn(d3, 'json');
+
+type Projection = ((...args: unknown[]) => void) & {
+ scale: () => Projection;
+ center: () => Projection;
+ translate: () => Projection;
+};
+
+type PathFn = (() => string) & {
+ projection: jest.Mock;
+ bounds: jest.Mock<[[number, number], [number, number]]>;
+ centroid: jest.Mock<[number, number]>;
+};
+
+const mockPath: PathFn = jest.fn(() => 'M10 10 L20 20') as unknown as PathFn;
+mockPath.projection = jest.fn();
+mockPath.bounds = jest.fn(() => [
+ [0, 0],
+ [100, 100],
+]);
+mockPath.centroid = jest.fn(() => [50, 50]);
+
+jest.spyOn(d3.geo, 'path').mockImplementation(() => mockPath);
+
+// Mock d3.geo.mercator
+jest.spyOn(d3.geo, 'mercator').mockImplementation(() => {
+ const proj = (() => {}) as Projection;
+ proj.scale = () => proj;
+ proj.center = () => proj;
+ proj.translate = () => proj;
+ return proj;
+});
+
+// Mock d3.mouse
+jest.spyOn(d3, 'mouse').mockReturnValue([100, 50]);
+
+const mockMapData = {
+ type: 'FeatureCollection',
+ features: [
+ {
+ type: 'Feature',
+ properties: { ISO: 'CAN', NAME_1: 'Canada' },
+ geometry: {},
+ },
+ ],
+};
+
+type D3JsonCallback = (error: Error | null, data: unknown) => void;
+
+describe('CountryMap (legacy d3)', () => {
+ beforeEach(() => {
+ jest.clearAllMocks();
+ });
+
+ it('renders a map after d3.json loads data', async () => {
+ d3.json.mockImplementation((_url: string, cb: D3JsonCallback) =>
+ cb(null, mockMapData),
+ );
+
+ render(
+ <ReactCountryMap
+ width={500}
+ height={300}
+ data={[{ country_id: 'CAN', metric: 100 }]}
+ country="canada"
+ linearColorScheme="bnbColors"
+ colorScheme=""
+ numberFormat=".2f"
+ />,
+ );
+
+ expect(d3.json).toHaveBeenCalledTimes(1);
+
+ const region = await document.querySelector('path.region');
+ expect(region).not.toBeNull();
+ });
+
+ it('shows tooltip on mouseenter/mousemove/mouseout', async () => {
+ d3.json.mockImplementation((_url: string, cb: D3JsonCallback) =>
+ cb(null, mockMapData),
+ );
+
+ render(
+ <ReactCountryMap
+ width={500}
+ height={300}
+ data={[{ country_id: 'CAN', metric: 100 }]}
+ country="canada"
+ linearColorScheme="bnbColors"
+ colorScheme=""
+ />,
+ );
+
+ const region = await document.querySelector('path.region');
+ expect(region).not.toBeNull();
+
+ const popup = document.querySelector('.hover-popup');
Review Comment:
**Suggestion:** The second test also queries the DOM immediately after
render for both the region and the tooltip; `await document.querySelector` does
not wait for async updates and can make the test flaky—use `waitFor` for both
queries to reliably wait for the elements to be mounted. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
const region = await waitFor(() =>
document.querySelector('path.region'));
expect(region).not.toBeNull();
const popup = await waitFor(() =>
document.querySelector('.hover-popup'));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Both queries are subject to the same async rendering timing and the file
currently misuses `await` on a sync query. Converting both to `waitFor` will
reduce flakiness and correctly wait for the elements to be mounted.
</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/test/CountryMap.test.tsx
**Line:** 118:121
**Comment:**
*Race Condition: The second test also queries the DOM immediately after
render for both the region and the tooltip; `await document.querySelector` does
not wait for async updates and can make the test flaky—use `waitFor` for both
queries to reliably wait for the elements to be mounted.
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/test/CountryMap.test.tsx:
##########
@@ -0,0 +1,130 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import '@testing-library/jest-dom';
+import { render, fireEvent } from '@testing-library/react';
+import d3 from 'd3';
+import ReactCountryMap from '../src/ReactCountryMap';
+
+jest.spyOn(d3, 'json');
+
+type Projection = ((...args: unknown[]) => void) & {
+ scale: () => Projection;
+ center: () => Projection;
+ translate: () => Projection;
+};
+
+type PathFn = (() => string) & {
+ projection: jest.Mock;
+ bounds: jest.Mock<[[number, number], [number, number]]>;
+ centroid: jest.Mock<[number, number]>;
+};
+
+const mockPath: PathFn = jest.fn(() => 'M10 10 L20 20') as unknown as PathFn;
+mockPath.projection = jest.fn();
+mockPath.bounds = jest.fn(() => [
+ [0, 0],
+ [100, 100],
+]);
+mockPath.centroid = jest.fn(() => [50, 50]);
+
+jest.spyOn(d3.geo, 'path').mockImplementation(() => mockPath);
+
+// Mock d3.geo.mercator
+jest.spyOn(d3.geo, 'mercator').mockImplementation(() => {
+ const proj = (() => {}) as Projection;
+ proj.scale = () => proj;
+ proj.center = () => proj;
+ proj.translate = () => proj;
+ return proj;
+});
+
+// Mock d3.mouse
+jest.spyOn(d3, 'mouse').mockReturnValue([100, 50]);
+
+const mockMapData = {
+ type: 'FeatureCollection',
+ features: [
+ {
+ type: 'Feature',
+ properties: { ISO: 'CAN', NAME_1: 'Canada' },
+ geometry: {},
+ },
+ ],
+};
+
+type D3JsonCallback = (error: Error | null, data: unknown) => void;
+
+describe('CountryMap (legacy d3)', () => {
+ beforeEach(() => {
+ jest.clearAllMocks();
+ });
+
+ it('renders a map after d3.json loads data', async () => {
+ d3.json.mockImplementation((_url: string, cb: D3JsonCallback) =>
+ cb(null, mockMapData),
+ );
+
+ render(
+ <ReactCountryMap
+ width={500}
+ height={300}
+ data={[{ country_id: 'CAN', metric: 100 }]}
+ country="canada"
+ linearColorScheme="bnbColors"
+ colorScheme=""
+ numberFormat=".2f"
+ />,
+ );
+
+ expect(d3.json).toHaveBeenCalledTimes(1);
+
+ const region = await document.querySelector('path.region');
Review Comment:
**Suggestion:** The test queries the DOM immediately after rendering using
`document.querySelector` with `await` (which does not wait). This creates a
race condition where the component's async state updates (triggered by the
mocked d3.json callback) may not have applied yet; replace the direct query
with `await waitFor(() => document.querySelector(...))` to wait for the element
to appear. [race condition]
**Severity Level:** Minor ⚠️
```suggestion
const region = await waitFor(() =>
document.querySelector('path.region'));
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The existing code incorrectly uses `await` on a synchronous DOM query, which
doesn't wait for async rendering triggered by the mocked d3.json. Replacing it
with `await waitFor(...)` fixes a real race condition and makes the test
deterministic.
</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/test/CountryMap.test.tsx
**Line:** 98:98
**Comment:**
*Race Condition: The test queries the DOM immediately after rendering
using `document.querySelector` with `await` (which does not wait). This creates
a race condition where the component's async state updates (triggered by the
mocked d3.json callback) may not have applied yet; replace the direct query
with `await waitFor(() => document.querySelector(...))` to wait for the element
to appear.
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:
##########
@@ -176,17 +141,34 @@ function CountryMap(element, props) {
c = d3.rgb(c).darker().toString();
}
d3.select(this).style('fill', c);
- selectAndDisplayNameOfRegion(d);
+ // Display information popup
const result = data.filter(
region => region.country_id === d.properties.ISO,
);
- updateMetrics(result);
+
+ const position = d3.mouse(svg.node());
Review Comment:
**Suggestion:** Positioning bug: the hover popup position is computed using
`d3.mouse(svg.node())`, which gives coordinates relative to the SVG; the popup
is appended inside the container div, so using SVG coordinates can mis-position
the HTML popup if the SVG is offset inside the container — use the container's
DOM node (div.node()) to compute coordinates relative to the container instead.
[possible bug]
**Severity Level:** Critical 🚨
```suggestion
const position = d3.mouse(div.node());
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
The popup is appended to the container div; computing mouse coordinates
relative to the SVG can misalign the HTML popup if the SVG isn't at container
origin. Using div.node() is more robust for positioning the appended DOM node.
</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:** 149:149
**Comment:**
*Possible Bug: Positioning bug: the hover popup position is computed
using `d3.mouse(svg.node())`, which gives coordinates relative to the SVG; the
popup is appended inside the container div, so using SVG coordinates can
mis-position the HTML popup if the SVG is offset inside the container — use the
container's DOM node (div.node()) to compute coordinates relative to the
container instead.
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/ReactCountryMap.jsx:
##########
@@ -43,33 +43,29 @@ export default styled(CountryMap)`
pointer-events: all;
}
+ .superset-legacy-chart-country-map .hover-popup {
+ position: absolute;
+ color: ${theme.colorTextSecondary};
+ display: none;
+ padding: 4px;
+ border-radius: 1px;
+ background-color: ${theme.colorBgElevated};
+ box-shadow: ${theme.boxShadow};
Review Comment:
**Suggestion:** `theme.boxShadow` may be undefined for some theme configs
and would render the literal `undefined` in CSS; provide a safe fallback (e.g.,
`none`) to avoid invalid CSS being emitted. [possible bug]
**Severity Level:** Critical 🚨
```suggestion
box-shadow: ${theme.boxShadow || 'none'};
```
<details>
<summary><b>Why it matters? ⭐ </b></summary>
Providing a fallback for `theme.boxShadow` avoids emitting `box-shadow:
undefined;` if a theme lacks that property.
It's a small, safe hardening that prevents invalid CSS and is easy to apply.
The change is not strictly required if the theme guarantees the value, but it's
a sensible defensive fix.
</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/ReactCountryMap.jsx
**Line:** 53:53
**Comment:**
*Possible Bug: `theme.boxShadow` may be undefined for some theme
configs and would render the literal `undefined` in CSS; provide a safe
fallback (e.g., `none`) to avoid invalid CSS being emitted.
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]
