codeant-ai-for-open-source[bot] commented on code in PR #37716:
URL: https://github.com/apache/superset/pull/37716#discussion_r2771779095
##########
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'));
+ })
+ .on('mouseout', function onMouseOut() {
+ if (inContextMenu) {
+ return;
+ }
+ const countryId = d3.select(this).attr('class').split(' ')[1];
+ const originalFill = d3.select(this).attr('data-original-fill');
+ // Restore the original fill color (data-based or default no-data
color)
+ if (originalFill) {
+ d3.select(this).style('fill', originalFill);
+ d3.select(this).attr('data-original-fill', null);
Review Comment:
**Suggestion:** The hover handlers store the element's current `fill` style
on mouseover and restore it on mouseout, but by the time the mouseover handler
runs, Datamaps has already applied the highlight color, so you end up saving
the highlight color as the "original" and then restoring the same highlight
color on mouseout, causing the country to remain highlighted instead of
reverting to its data-based or default color. To fix this, derive the original
fill color from the underlying `mapData` (falling back to the default fill)
rather than from the current DOM style, and use that value when restoring the
fill. [logic error]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ World Map chart: hover highlight persists after mouseout.
- ⚠️ Countries with data display incorrect color after hover.
- ⚠️ Visual interaction on dashboards with World Map affected.
```
</details>
```suggestion
const element = d3.select(this);
const classes = element.attr('class') || '';
const countryId = classes.split(' ')[1];
const countryData = mapData[countryId];
const originalFill =
(countryData && countryData.fillColor) || theme.colorBorder;
// Store original fill color for restoration
element.attr('data-original-fill', originalFill);
})
.on('mouseout', function onMouseOut() {
if (inContextMenu) {
return;
}
const element = d3.select(this);
const originalFill = element.attr('data-original-fill');
// Restore the original fill color (data-based or default
no-data color)
if (originalFill) {
element.style('fill', originalFill);
element.attr('data-original-fill', null);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Load a World Map chart so WorldMap() runs and Datamap is created (see
WorldMap.js:243-267, the `done: datamap => { ... }` callback). The Datamap
configuration
enabling highlight is set earlier at WorldMap.js:234 (`highlightOnHover:
!inContextMenu`).
2. Move the mouse over a country on the rendered map. Datamaps applies its
built-in
highlight because `highlightOnHover` is enabled (WorldMap.js:234). The
library's highlight
styling is applied before the user-defined handlers in the `done` callback
execute.
3. The PR's mouseover handler at WorldMap.js:248-255 executes and runs the
store line at
WorldMap.js:254: `d3.select(this).attr('data-original-fill',
d3.select(this).style('fill'));`. Because Datamaps already set the highlight,
`style('fill')` now returns the highlight color, which is saved as
"original".
4. When the pointer leaves the country, the mouseout handler at
WorldMap.js:256-266 reads
that `data-original-fill` (WorldMap.js:261) and applies it at WorldMap.js:264
(`d3.select(this).style('fill', originalFill);`). The applied value is the
highlight color
saved earlier, so the country remains highlighted after mouseout instead of
reverting to
its data-based or default color.
```
</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:** 252:265
**Comment:**
*Logic Error: The hover handlers store the element's current `fill`
style on mouseover and restore it on mouseout, but by the time the mouseover
handler runs, Datamaps has already applied the highlight color, so you end up
saving the highlight color as the "original" and then restoring the same
highlight color on mouseout, causing the country to remain highlighted instead
of reverting to its data-based or default color. To fix this, derive the
original fill color from the underlying `mapData` (falling back to the default
fill) rather than from the current DOM style, and use that value when restoring
the fill.
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,319 @@
+/**
+ * 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,
+ };
+ });
+});
+
+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);
+});
+
+test('sets up mouseover and mouseout handlers on countries', () => {
+ WorldMap(container, baseProps);
+
+ expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit');
+ const onCalls = mockSvg.on.mock.calls;
+
+ // Find mouseover and mouseout handler registrations
+ const hasMouseover = onCalls.some(call => call[0] === 'mouseover');
+ const hasMouseout = onCalls.some(call => call[0] === 'mouseout');
+
+ expect(hasMouseover).toBe(true);
+ expect(hasMouseout).toBe(true);
+});
+
+test('stores original fill color on mouseover', () => {
+ // Create a mock DOM element with d3 selection capabilities
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ container.appendChild(mockElement);
+
+ let mouseoverHandler: ((this: HTMLElement) => void) | null = null;
+
+ // Mock d3.select to return the mock element
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.setAttribute(attrName, value);
+ } else {
+ return mockElement.getAttribute(attrName);
+ }
+ return mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ } else {
+ return mockElement.style[styleName as any];
+ }
+ return mockD3Selection;
+ }),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ // Capture the mouseover handler
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseover') {
+ mouseoverHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, baseProps);
+
+ // Simulate mouseover
+ if (mouseoverHandler) {
+ mouseoverHandler.call(mockElement);
+ }
+
+ // Verify that data-original-fill attribute was set
+ expect(mockD3Selection.attr).toHaveBeenCalledWith(
+ 'data-original-fill',
+ expect.any(String),
+ );
+});
+
+test('restores original fill color on mouseout for country with data', () => {
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ mockElement.setAttribute('data-original-fill', 'rgb(100, 150, 200)');
+ container.appendChild(mockElement);
+
+ let mouseoutHandler: ((this: HTMLElement) => void) | null = null;
+
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string | null) => {
+ if (value !== undefined) {
+ if (value === null) {
+ mockElement.removeAttribute(attrName);
+ } else {
+ mockElement.setAttribute(attrName, value);
+ }
+ }
+ return mockElement.getAttribute(attrName) || mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ }
+ return mockElement.style[styleName as any] || mockD3Selection;
Review Comment:
**Suggestion:** In the "restores original fill color on mouseout for country
with data" test, the d3 `attr` and `style` mocks return the underlying string
value even when used as setters, which breaks method chaining and can cause the
hover handler under test to throw a TypeError when it tries to chain further d3
calls. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit tests fail with TypeError in WorldMap mouseout test.
- ⚠️ CI job running test suite will be blocked by failure.
- ⚠️ WorldMap hover behavior tests unreliable.
```
</details>
```suggestion
// When used as a setter, mimic D3's chainable API by returning the
selection
return mockD3Selection;
}
// Getter: return the current attribute value
return mockElement.getAttribute(attrName);
}),
style: jest.fn((styleName: string, value?: string) => {
if (value !== undefined) {
mockElement.style[styleName as any] = value;
// Setter should return the selection for chaining
return mockD3Selection;
}
// Getter: return the current style value
return mockElement.style[styleName as any];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open the test file
`superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts`.
2. Locate the "restores original fill color on mouseout for country with
data" test and
the mock selection at lines 170-189 where `mockD3Selection` is defined.
3. Note `mockSvg.on.mockImplementation` captures the mouseout handler at
lines 194-199,
and `WorldMap(container, baseProps)` is invoked at line 201 to register
handlers.
4. When the test simulates the mouseout (mouseoutHandler.call(mockElement)
at lines
204-206), the world-map mouseout handler will call d3 selection setters in a
chain (e.g.,
d3.select(this).style('fill', original).attr('data-original-fill', null)).
5. Because the current `attr` / `style` setters return the raw string (via
`mockElement.getAttribute(...) || mockD3Selection`) instead of the selection
object, the
subsequent chained call will receive a string, causing a TypeError when the
code attempts
to call `.attr()` or `.style()` on that return value. This reproduces as a
failing test
run (TypeError thrown during handler execution).
6. Expected correct behavior: setter calls should return the mock selection
(chainable) so
the handler completes without throwing. The proposed change returns
`mockD3Selection` for
setters, 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:** 178:185
**Comment:**
*Logic Error: In the "restores original fill color on mouseout for
country with data" test, the d3 `attr` and `style` mocks return the underlying
string value even when used as setters, which breaks method chaining and can
cause the hover handler under test to throw a TypeError when it tries to chain
further d3 calls.
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,319 @@
+/**
+ * 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,
+ };
+ });
+});
+
+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);
+});
+
+test('sets up mouseover and mouseout handlers on countries', () => {
+ WorldMap(container, baseProps);
+
+ expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit');
+ const onCalls = mockSvg.on.mock.calls;
+
+ // Find mouseover and mouseout handler registrations
+ const hasMouseover = onCalls.some(call => call[0] === 'mouseover');
+ const hasMouseout = onCalls.some(call => call[0] === 'mouseout');
+
+ expect(hasMouseover).toBe(true);
+ expect(hasMouseout).toBe(true);
+});
+
+test('stores original fill color on mouseover', () => {
+ // Create a mock DOM element with d3 selection capabilities
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ container.appendChild(mockElement);
+
+ let mouseoverHandler: ((this: HTMLElement) => void) | null = null;
+
+ // Mock d3.select to return the mock element
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.setAttribute(attrName, value);
+ } else {
+ return mockElement.getAttribute(attrName);
+ }
+ return mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ } else {
+ return mockElement.style[styleName as any];
+ }
+ return mockD3Selection;
+ }),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ // Capture the mouseover handler
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseover') {
+ mouseoverHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, baseProps);
+
+ // Simulate mouseover
+ if (mouseoverHandler) {
+ mouseoverHandler.call(mockElement);
+ }
+
+ // Verify that data-original-fill attribute was set
+ expect(mockD3Selection.attr).toHaveBeenCalledWith(
+ 'data-original-fill',
+ expect.any(String),
+ );
+});
+
+test('restores original fill color on mouseout for country with data', () => {
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ mockElement.setAttribute('data-original-fill', 'rgb(100, 150, 200)');
+ container.appendChild(mockElement);
+
+ let mouseoutHandler: ((this: HTMLElement) => void) | null = null;
+
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string | null) => {
+ if (value !== undefined) {
+ if (value === null) {
+ mockElement.removeAttribute(attrName);
+ } else {
+ mockElement.setAttribute(attrName, value);
+ }
+ }
+ return mockElement.getAttribute(attrName) || mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ }
+ return mockElement.style[styleName as any] || mockD3Selection;
+ }),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ // Capture the mouseout handler
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseout') {
+ mouseoutHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, baseProps);
+
+ // Simulate mouseout
+ if (mouseoutHandler) {
+ mouseoutHandler.call(mockElement);
+ }
+
+ // Verify that original fill was restored
+ expect(mockD3Selection.style).toHaveBeenCalledWith(
+ 'fill',
+ 'rgb(100, 150, 200)',
+ );
+ expect(mockD3Selection.attr).toHaveBeenCalledWith('data-original-fill',
null);
+});
+
+test('restores default fill color on mouseout for country with no data', () =>
{
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit XXX');
+ mockElement.style.fill = '#e0e0e0'; // Default border color
+ mockElement.setAttribute('data-original-fill', '#e0e0e0');
+ container.appendChild(mockElement);
+
+ let mouseoutHandler: ((this: HTMLElement) => void) | null = null;
+
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string | null) => {
+ if (value !== undefined) {
+ if (value === null) {
+ mockElement.removeAttribute(attrName);
+ } else {
+ mockElement.setAttribute(attrName, value);
+ }
+ }
+ return mockElement.getAttribute(attrName) || mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ }
+ return mockElement.style[styleName as any] || mockD3Selection;
+ }),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseout') {
+ mouseoutHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, baseProps);
+
+ // Simulate mouseout
+ if (mouseoutHandler) {
+ mouseoutHandler.call(mockElement);
+ }
+
+ // Verify that default fill was restored (no-data color)
+ expect(mockD3Selection.style).toHaveBeenCalledWith('fill', '#e0e0e0');
+ expect(mockD3Selection.attr).toHaveBeenCalledWith('data-original-fill',
null);
+});
+
+test('does not handle mouse events when inContextMenu is true', () => {
+ const propsWithContextMenu = {
+ ...baseProps,
+ inContextMenu: true,
+ };
+
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ container.appendChild(mockElement);
+
+ let mouseoverHandler: ((this: HTMLElement) => void) | null = null;
+ let mouseoutHandler: ((this: HTMLElement) => void) | null = null;
+
+ const mockD3Selection = {
+ attr: jest.fn(() => mockD3Selection),
+ style: jest.fn(() => mockD3Selection),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseover') {
+ mouseoverHandler = handler;
+ }
+ if (event === 'mouseout') {
+ mouseoutHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, propsWithContextMenu);
+
+ // Simulate mouseover and mouseout
+ if (mouseoverHandler) {
+ mouseoverHandler.call(mockElement);
+ }
+ if (mouseoutHandler) {
+ mouseoutHandler.call(mockElement);
+ }
+
+ // When inContextMenu is true, handlers should exit early without modifying
anything
+ // We verify this by checking that attr and style weren't called to change
fill
+ const attrCalls = mockD3Selection.attr.mock.calls;
+ const fillChangeCalls = attrCalls.filter(
+ call => call[0] === 'data-original-fill' && call[1] !== undefined,
+ );
+
+ // The handlers should return early, so no state changes
+ expect(fillChangeCalls.length).toBe(0);
Review Comment:
**Suggestion:** The "does not handle mouse events when inContextMenu is
true" test only checks that the `data-original-fill` attribute is not set, but
it never asserts that the fill style itself is untouched, so the test can pass
even if the hover logic still changes the country color while in context-menu
mode. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Test may miss color-change regressions.
- ❌ UI hover behavior could regress unnoticed.
- ⚠️ WorldMap context-menu behavior assertions incomplete.
```
</details>
```suggestion
const styleCalls = mockD3Selection.style.mock.calls;
const styleFillChangeCalls = styleCalls.filter(
call => call[0] === 'fill' && call[1] !== undefined,
);
// The handlers should return early, so no attribute or fill style changes
expect(fillChangeCalls.length).toBe(0);
expect(styleFillChangeCalls.length).toBe(0);
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts`
and go to the "does not handle mouse events when inContextMenu is true" test.
2. The test sets up `mockD3Selection` at lines ~281-286, spies on
`d3.select` at line 288,
registers handlers via `mockSvg.on.mockImplementation` at lines 290-298, and
calls
`WorldMap(container, propsWithContextMenu)` at line 300.
3. The test simulates mouseover/mouseout at lines 303-308 by calling the
captured
handlers.
4. The current assertions at lines 312-318 only inspect `attr` calls for
`data-original-fill` and assert no such set-call occurred. They do not
inspect `style`
calls, so a handler that skips setting `data-original-fill` but still calls
`.style('fill', ...)` would not be detected by this assertion.
5. To reproduce the false-positive: modify (locally) the mouse handlers to
early-return
before attr but still set `.style('fill', someColor)`. Running the test
as-is (lines
303-318) will still pass because `fillChangeCalls.length` remains 0, masking
the unwanted
style change.
6. The improved code also inspects `mockD3Selection.style.mock.calls` for
`'fill'` changes
and asserts none occurred, preventing this blind spot.
```
</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:** 317:318
**Comment:**
*Logic Error: The "does not handle mouse events when inContextMenu is
true" test only checks that the `data-original-fill` attribute is not set, but
it never asserts that the fill style itself is untouched, so the test can pass
even if the hover logic still changes the country color while in context-menu
mode.
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,319 @@
+/**
+ * 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,
+ };
+ });
+});
+
+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);
+});
+
+test('sets up mouseover and mouseout handlers on countries', () => {
+ WorldMap(container, baseProps);
+
+ expect(mockSvg.selectAll).toHaveBeenCalledWith('.datamaps-subunit');
+ const onCalls = mockSvg.on.mock.calls;
+
+ // Find mouseover and mouseout handler registrations
+ const hasMouseover = onCalls.some(call => call[0] === 'mouseover');
+ const hasMouseout = onCalls.some(call => call[0] === 'mouseout');
+
+ expect(hasMouseover).toBe(true);
+ expect(hasMouseout).toBe(true);
+});
+
+test('stores original fill color on mouseover', () => {
+ // Create a mock DOM element with d3 selection capabilities
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ container.appendChild(mockElement);
+
+ let mouseoverHandler: ((this: HTMLElement) => void) | null = null;
+
+ // Mock d3.select to return the mock element
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.setAttribute(attrName, value);
+ } else {
+ return mockElement.getAttribute(attrName);
+ }
+ return mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ } else {
+ return mockElement.style[styleName as any];
+ }
+ return mockD3Selection;
+ }),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ // Capture the mouseover handler
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseover') {
+ mouseoverHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, baseProps);
+
+ // Simulate mouseover
+ if (mouseoverHandler) {
+ mouseoverHandler.call(mockElement);
+ }
+
+ // Verify that data-original-fill attribute was set
+ expect(mockD3Selection.attr).toHaveBeenCalledWith(
+ 'data-original-fill',
+ expect.any(String),
+ );
+});
+
+test('restores original fill color on mouseout for country with data', () => {
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit USA');
+ mockElement.style.fill = 'rgb(100, 150, 200)';
+ mockElement.setAttribute('data-original-fill', 'rgb(100, 150, 200)');
+ container.appendChild(mockElement);
+
+ let mouseoutHandler: ((this: HTMLElement) => void) | null = null;
+
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string | null) => {
+ if (value !== undefined) {
+ if (value === null) {
+ mockElement.removeAttribute(attrName);
+ } else {
+ mockElement.setAttribute(attrName, value);
+ }
+ }
+ return mockElement.getAttribute(attrName) || mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ }
+ return mockElement.style[styleName as any] || mockD3Selection;
+ }),
+ classed: jest.fn().mockReturnThis(),
+ selectAll: jest.fn().mockReturnValue({ remove: jest.fn() }),
+ };
+
+ jest.spyOn(d3, 'select').mockReturnValue(mockD3Selection as any);
+
+ // Capture the mouseout handler
+ mockSvg.on.mockImplementation((event: string, handler: any) => {
+ if (event === 'mouseout') {
+ mouseoutHandler = handler;
+ }
+ return mockSvg;
+ });
+
+ WorldMap(container, baseProps);
+
+ // Simulate mouseout
+ if (mouseoutHandler) {
+ mouseoutHandler.call(mockElement);
+ }
+
+ // Verify that original fill was restored
+ expect(mockD3Selection.style).toHaveBeenCalledWith(
+ 'fill',
+ 'rgb(100, 150, 200)',
+ );
+ expect(mockD3Selection.attr).toHaveBeenCalledWith('data-original-fill',
null);
+});
+
+test('restores default fill color on mouseout for country with no data', () =>
{
+ const mockElement = document.createElement('path');
+ mockElement.setAttribute('class', 'datamaps-subunit XXX');
+ mockElement.style.fill = '#e0e0e0'; // Default border color
+ mockElement.setAttribute('data-original-fill', '#e0e0e0');
+ container.appendChild(mockElement);
+
+ let mouseoutHandler: ((this: HTMLElement) => void) | null = null;
+
+ const mockD3Selection = {
+ attr: jest.fn((attrName: string, value?: string | null) => {
+ if (value !== undefined) {
+ if (value === null) {
+ mockElement.removeAttribute(attrName);
+ } else {
+ mockElement.setAttribute(attrName, value);
+ }
+ }
+ return mockElement.getAttribute(attrName) || mockD3Selection;
+ }),
+ style: jest.fn((styleName: string, value?: string) => {
+ if (value !== undefined) {
+ mockElement.style[styleName as any] = value;
+ }
+ return mockElement.style[styleName as any] || mockD3Selection;
Review Comment:
**Suggestion:** In the "restores default fill color on mouseout for country
with no data" test, the mocked d3 `attr` and `style` functions also return the
element's values on setter calls, which breaks D3-style chaining and can cause
the tested mouseout handler to fail when chaining further operations. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unit tests fail in default-color mouseout scenario.
- ⚠️ CI tests for WorldMap become flaky.
- ⚠️ Mouseout logic assertions unreliable.
```
</details>
```suggestion
// Setter should return the selection to allow chaining
return mockD3Selection;
}
// Getter returns the attribute value
return mockElement.getAttribute(attrName);
}),
style: jest.fn((styleName: string, value?: string) => {
if (value !== undefined) {
mockElement.style[styleName as any] = value;
// Setter should return the selection to allow chaining
return mockD3Selection;
}
// Getter returns the style value
return mockElement.style[styleName as any];
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Open
`superset-frontend/plugins/legacy-plugin-chart-world-map/test/WorldMap.test.ts`.
2. Find the "restores default fill color on mouseout for country with no
data" test and
its `mockD3Selection` at lines 225-244 where `attr` and `style` are mocked.
3. Observe `mockSvg.on.mockImplementation` capturing the `mouseout` handler
at lines
248-253 and `WorldMap(container, baseProps)` invoked at line 255 to attach
handlers.
4. Test simulates the mouseout with `mouseoutHandler.call(mockElement)` at
lines 258-260.
The chart mouseout handler executes chained d3 calls like `.style('fill',
value).attr('data-original-fill', null)`.
5. With the current mocks, setter calls return the element's string value
(truthy) instead
of `mockD3Selection`, breaking chaining and causing subsequent calls to fail
or throw in
the handler during the test run — reproducing as a failed test or TypeError
in CI.
6. The improved mocks return `mockD3Selection` when used as setters,
preserving D3
chaining semantics and allowing the handler to run and assertions to
validate correctly.
```
</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:** 233:240
**Comment:**
*Logic Error: In the "restores default fill color on mouseout for
country with no data" test, the mocked d3 `attr` and `style` functions also
return the element's values on setter calls, which breaks D3-style chaining and
can cause the tested mouseout handler to fail when chaining further operations.
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]
