codeant-ai-for-open-source[bot] commented on code in PR #37580:
URL: https://github.com/apache/superset/pull/37580#discussion_r2747127802
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js:
##########
@@ -244,7 +244,27 @@ function WorldMap(element, props) {
datamap.svg
.selectAll('.datamaps-subunit')
.on('contextmenu', handleContextMenu)
- .on('click', handleClick);
+ .on('click', handleClick)
+ .on('mouseover', function onMouseOver() {
+ if (inContextMenu) {
+ return;
+ }
+ const countryId = d3.select(this).attr('class').split(' ')[1];
+ // Store original fill color for restoration
+ d3.select(this).attr('data-original-fill',
d3.select(this).style('fill'));
Review Comment:
**Suggestion:** Overwrites the stored original fill with the highlighted
color: the handler always sets `data-original-fill` on every mouseover, which
can capture the highlight color (if Datamaps already applied it) and then
restore to that wrong color on mouseout. Only store the original fill if it
hasn't been stored yet. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ World Map hover state may not reset visually.
- ⚠️ Affects Explore and Dashboard world-map charts.
- ⚠️ Visual inconsistency in no-data country highlighting.
```
</details>
```suggestion
// Store original fill color for restoration only if it's not
already stored
if (!d3.select(this).attr('data-original-fill')) {
d3.select(this).attr('data-original-fill',
d3.select(this).style('fill'));
}
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Render a World Map (WorldMap() in
superset-frontend/plugins/legacy-plugin-chart-world-map/src/WorldMap.js).
The datamap
instance is created and the done callback attaches mouse handlers at lines
245-267.
2. The mouseover handler (lines 248-255) unconditionally sets
data-original-fill to the
element's current fill: d3.select(this).attr('data-original-fill',
d3.select(this).style('fill')).
3. Datamaps itself applies a highlight fill on hover (configured via
geographyConfig.highlightFillColor earlier in the file). If Datamaps updates
the element's
style before this handler runs, the value written into data-original-fill
will be the
highlighted color rather than the pre-hover color.
4. On mouseout (lines 256-267) originalFill is read and restored. If
originalFill contains
the highlight color (from step 3), the element is set back to the highlight
color and the
visible state will not revert to the true original. Reproduce by hovering a
no-data
country and observing it remain visually highlighted after mouseout
(incorrect reset).
```
</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-world-map/src/WorldMap.js
**Line:** 253:254
**Comment:**
*Logic Error: Overwrites the stored original fill with the highlighted
color: the handler always sets `data-original-fill` on every mouseover, which
can capture the highlight color (if Datamaps already applied it) and then
restore to that wrong color on mouseout. Only store the original fill if it
hasn't been stored yet.
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-world-map/test/WorldMap.test.ts:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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 d3 from 'd3';
Review Comment:
**Suggestion:** Importing d3 as a default export may fail at runtime if the
d3 package doesn't provide a default export in the test environment; this will
cause `jest.spyOn(d3, 'select')` to throw because `d3.select` is undefined. Use
a namespace import so `d3.select` is reliably available. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ WorldMap unit tests can fail with TypeError.
- ⚠️ CI test runs may be flaky across environments.
```
</details>
```suggestion
import * as d3 from 'd3';
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Run the WorldMap test file:
superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts
(for example
via `yarn test WorldMap.test.ts`).
2. The test "stores original fill color on mouseover" (defined starting at
line 107 in
that file) reaches the code that calls jest.spyOn(d3, 'select') at line 136:
`jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);`.
3. If the environment's module resolution does not provide a default export
for d3, the
default import `import d3 from 'd3';` at line 20 results in `d3` being
undefined or
lacking `select`. When the test executes `jest.spyOn(d3, 'select')` at line
136, Jest will
throw because `d3.select` is undefined (TypeError from jest.spyOn).
4. The failure is concrete and reproducible: running that test file in an
environment
without a default d3 export will produce an immediate test error at the spy
site. Using
`import * as d3 from 'd3';` guarantees `d3.select` is present in
CommonJS/ESM interop
environments, avoiding the TypeError.
```
</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-world-map/test/WorldMap.test.ts
**Line:** 20:20
**Comment:**
*Possible Bug: Importing d3 as a default export may fail at runtime if
the d3 package doesn't provide a default export in the test environment; this
will cause `jest.spyOn(d3, 'select')` to throw because `d3.select` is
undefined. Use a namespace import so `d3.select` is reliably available.
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-world-map/test/WorldMap.test.ts:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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 d3 from 'd3';
+import WorldMap from '../src/WorldMap';
+
+// Mock Datamap
+const mockBubbles = jest.fn();
+const mockUpdateChoropleth = jest.fn();
+const mockSvg = {
+ selectAll: jest.fn().mockReturnThis(),
+ on: jest.fn().mockReturnThis(),
+ attr: jest.fn().mockReturnThis(),
+ style: jest.fn().mockReturnThis(),
+};
+
+jest.mock('datamaps/dist/datamaps.all.min', () => {
+ return jest.fn().mockImplementation((config) => {
+ // Call the done callback immediately to simulate Datamap initialization
+ if (config.done) {
+ config.done({
+ svg: mockSvg,
+ });
+ }
+ return {
+ bubbles: mockBubbles,
+ updateChoropleth: mockUpdateChoropleth,
+ svg: mockSvg,
+ };
+ });
+});
+
+describe('WorldMap hover behavior', () => {
+ let container: HTMLElement;
+ const mockFormatter = jest.fn((val) => String(val));
+
+ const baseProps = {
+ data: [
+ { country: 'USA', name: 'United States', m1: 100, m2: 200, code: 'US' },
+ { country: 'CAN', name: 'Canada', m1: 50, m2: 100, code: 'CA' },
+ ],
+ width: 600,
+ height: 400,
+ maxBubbleSize: 25,
+ showBubbles: false,
+ linearColorScheme: 'schemeRdYlBu',
+ color: '#61B0B7',
+ colorBy: 'country',
+ colorScheme: 'supersetColors',
+ sliceId: 123,
+ theme: {
+ colorBorder: '#e0e0e0',
+ colorSplit: '#333',
+ colorIcon: '#000',
+ colorTextSecondary: '#666',
+ },
+ countryFieldtype: 'code',
+ entity: 'country',
+ onContextMenu: jest.fn(),
+ setDataMask: jest.fn(),
+ inContextMenu: false,
+ filterState: { selectedValues: [] },
+ emitCrossFilters: false,
+ formatter: mockFormatter,
+ };
+
+ beforeEach(() => {
+ jest.clearAllMocks();
+ container = document.createElement('div');
+ document.body.appendChild(container);
+ });
+
+ afterEach(() => {
+ document.body.removeChild(container);
+ });
Review Comment:
**Suggestion:** Spies created with `jest.spyOn` are not restored anywhere in
the test file; leaving spies in place can interfere with other tests. Restore
spies after each test by calling `jest.restoreAllMocks()` in `afterEach`.
[possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ WorldMap tests leak spies causing cross-test interference.
- ⚠️ Other tests may be affected by lingering spies.
```
</details>
```suggestion
// Restore any spies (e.g. jest.spyOn) to avoid leaking altered
implementations across tests
jest.restoreAllMocks();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Multiple tests in this file call `jest.spyOn(d3, 'select')` to replace
`d3.select` with
controlled return values (see calls at lines 136, 185, 235, 275 where
`jest.spyOn(d3,
'select')` is invoked).
2. The current afterEach (lines 89-91) only removes the test DOM container
and does not
call `jest.restoreAllMocks()`, so spies applied by `jest.spyOn(...)` remain
in effect for
subsequent tests.
3. Run the test suite: a test that expects the real behavior of `d3.select`
(or a
different spy) can be executed after another test that established a spy,
leading to tests
observing the wrong implementation or unexpected behavior (for example, a
stale spy
returning a mocked selection).
4. Add `jest.restoreAllMocks()` in afterEach to restore original
implementations and avoid
cross-test interference; this is a concrete fix observed to prevent spy
leakage in
Jest-based tests.
```
</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-world-map/test/WorldMap.test.ts
**Line:** 91:91
**Comment:**
*Possible Bug: Spies created with `jest.spyOn` are not restored
anywhere in the test file; leaving spies in place can interfere with other
tests. Restore spies after each test by calling `jest.restoreAllMocks()` in
`afterEach`.
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-world-map/test/WorldMap.test.ts:
##########
@@ -0,0 +1,307 @@
+/**
+ * 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 d3 from 'd3';
+import WorldMap from '../src/WorldMap';
+
+// Mock Datamap
+const mockBubbles = jest.fn();
+const mockUpdateChoropleth = jest.fn();
+const mockSvg = {
+ selectAll: jest.fn().mockReturnThis(),
+ on: jest.fn().mockReturnThis(),
+ attr: jest.fn().mockReturnThis(),
+ style: jest.fn().mockReturnThis(),
+};
+
+jest.mock('datamaps/dist/datamaps.all.min', () => {
+ return jest.fn().mockImplementation((config) => {
+ // Call the done callback immediately to simulate Datamap initialization
+ if (config.done) {
+ config.done({
+ svg: mockSvg,
+ });
+ }
+ return {
+ bubbles: mockBubbles,
+ updateChoropleth: mockUpdateChoropleth,
+ svg: mockSvg,
+ };
+ });
+});
+
+describe('WorldMap hover behavior', () => {
+ let container: HTMLElement;
+ const mockFormatter = jest.fn((val) => String(val));
+
+ const baseProps = {
+ data: [
+ { country: 'USA', name: 'United States', m1: 100, m2: 200, code: 'US' },
+ { country: 'CAN', name: 'Canada', m1: 50, m2: 100, code: 'CA' },
+ ],
+ width: 600,
+ height: 400,
+ maxBubbleSize: 25,
+ showBubbles: false,
+ linearColorScheme: 'schemeRdYlBu',
+ color: '#61B0B7',
+ colorBy: 'country',
+ colorScheme: 'supersetColors',
+ sliceId: 123,
+ theme: {
+ colorBorder: '#e0e0e0',
+ colorSplit: '#333',
+ colorIcon: '#000',
+ colorTextSecondary: '#666',
+ },
+ countryFieldtype: 'code',
+ entity: 'country',
+ onContextMenu: jest.fn(),
+ setDataMask: jest.fn(),
+ inContextMenu: false,
+ filterState: { selectedValues: [] },
+ emitCrossFilters: false,
+ formatter: mockFormatter,
+ };
+
+ beforeEach(() => {
+ jest.clearAllMocks();
Review Comment:
**Suggestion:** The test suite calls `jest.clearAllMocks()` in `beforeEach`,
which clears mock call history but does not reset mock implementations created
in earlier tests (for example, `mockImplementation` on `mockSvg.on`). This can
cause test cross-contamination and flakiness; replace with
`jest.resetAllMocks()` (and also clear calls) to restore mocks to their
original implementations before each test. [possible bug]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ WorldMap tests may intermittently assert incorrect handler behavior.
- ⚠️ CI builds can show flaky failures for this test file.
```
</details>
```suggestion
// Reset mock implementations and clear call history to avoid leakage
between tests
jest.resetAllMocks();
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe tests that set mock implementations on the shared mockSvg object,
e.g. the
"stores original fill color on mouseover" test sets
`mockSvg.on.mockImplementation(...)`
when capturing the mouseover handler at lines ~139-144 in the test file.
2. The next test in the file runs its setup (the beforeEach starting at line
83), which
currently calls only `jest.clearAllMocks()` at line 84. `clearAllMocks()`
clears call
history but does NOT revert mock implementations created by
`mockImplementation`.
3. Because the previous test's `mockImplementation` persists, the subsequent
test can see
the prior custom behavior of `mockSvg.on`, causing incorrect handler capture
or unexpected
calls (for example, mouseover/mouseout handlers bound in one test being
active in
another).
4. Reproduce by running the full WorldMap.test.ts file multiple times or
changing test
order: flakiness appears as tests intermittently fail expectations about
handler
registration or attr/style calls. Replacing `jest.clearAllMocks()` with
`jest.resetAllMocks()` (or calling both reset then clear) will restore mocks
to their
original implementations and remove the cross-test leakage.
```
</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-world-map/test/WorldMap.test.ts
**Line:** 84:84
**Comment:**
*Possible Bug: The test suite calls `jest.clearAllMocks()` in
`beforeEach`, which clears mock call history but does not reset mock
implementations created in earlier tests (for example, `mockImplementation` on
`mockSvg.on`). This can cause test cross-contamination and flakiness; replace
with `jest.resetAllMocks()` (and also clear calls) to restore mocks to their
original implementations before each test.
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]
