You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "Arunodoy18 (via GitHub)" <gi...@apache.org> on 2026/01/30 16:44:48 UTC
[PR] fix(world-map): reset hover highlight on mouse out [superset]
Arunodoy18 opened a new pull request, #37580:
URL: https://github.com/apache/superset/pull/37580
### SUMMARY
Fixes an issue in the World Map chart where hover highlighting did not reset
correctly after mouse-out for countries with no data. This caused countries
without data to remain highlighted even after the cursor moved away, leading
to an incorrect visual state in both Explore and Dashboards.
This change ensures that on mouse-out, each country correctly returns to its
original fill color:
- data-driven color for countries with data
- neutral “no data” color for countries without data
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable.
### TESTING INSTRUCTIONS
**Manual testing:**
1. Create or open a World Map chart with some countries that have no data.
2. Hover over a country with no data and observe the hover highlight.
3. Move the mouse away from the country.
4. Verify that the country returns to the neutral “no data” color.
5. Repeat the same steps for a country with data and confirm it resets to the
original data-driven color.
**Automated testing:**
- Updated/added frontend tests to verify that hover state is correctly cleared
on mouse-out for both data and no-data regions.
### ADDITIONAL INFORMATION
- [x] Has associated issue: Fixes #37569
- [ ] Required feature flags:
- [x] Changes UI
- [ ] Includes DB Migration (follow approval process in SIP-59)
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #37580:
URL: https://github.com/apache/superset/pull/37580#issuecomment-3874624901
Closing this one since the other PR merged, but if I'm doing the wrong thing here, holler and we can reopen/revisit. Thanks for the contribution!!
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "Arunodoy18 (via GitHub)" <gi...@apache.org>.
Arunodoy18 commented on code in PR #37580:
URL: https://github.com/apache/superset/pull/37580#discussion_r2769572574
##########
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:
Done
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37580:
URL: https://github.com/apache/superset/pull/37580#issuecomment-3857488768
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
<details>
<summary>
Bito Usage Guide
</summary>
**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 <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/available-commands" target="_blank" rel="noopener noreferrer">documentation</a> for additional commands.
**Configuration**
This repository uses `Superset` You can customize the agent settings <a href="https://alpha.bito.ai/home/ai-agents/code-review-agent" target="_blank" rel="noopener noreferrer">here</a> or contact your Bito workspace admin at evan@preset.io.
**Documentation & Help**
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/getting-started/install-run-using-bito-cloud/create-or-customize-an-agent-instance" target="_blank" rel="noopener noreferrer">Customize agent settings</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/implementing-custom-code-review-rules" target="_blank" rel="noopener noreferrer">Review rules</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent" target="_blank" rel="noopener noreferrer">General documentation</a>
- <a href="https://docs.bito.ai/bito-dev-agents/ai-code-review-agent/faqs" target="_blank" rel="noopener noreferrer">FAQ</a>
</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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "codeant-ai-for-open-source[bot] (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "alexandrusoare (via GitHub)" <gi...@apache.org>.
alexandrusoare commented on PR #37580:
URL: https://github.com/apache/superset/pull/37580#issuecomment-3840193999
Thank you for the contribution, great work! Can we tend to the CI issues?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "geido (via GitHub)" <gi...@apache.org>.
geido commented on code in PR #37580:
URL: https://github.com/apache/superset/pull/37580#discussion_r2758192541
##########
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:
Agreed
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #37580:
URL: https://github.com/apache/superset/pull/37580#issuecomment-3824670105
AI Code Review is in progress (usually takes 3 to 15 minutes unless it's a very large PR).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "Arunodoy18 (via GitHub)" <gi...@apache.org>.
Arunodoy18 commented on code in PR #37580:
URL: https://github.com/apache/superset/pull/37580#discussion_r2769567150
##########
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:
Done
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "msyavuz (via GitHub)" <gi...@apache.org>.
msyavuz commented on code in PR #37580:
URL: https://github.com/apache/superset/pull/37580#discussion_r2758115277
##########
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', () => {
Review Comment:
Can we flatten this and get rid of the describe block?
##########
superset-frontend/plugins/legacy-plugin-chart-world-map/test/tsconfig.json:
##########
@@ -0,0 +1,8 @@
+{
+ "extends": "../../../tsconfig.options.json",
+ "include": ["**/*"],
+ "compilerOptions": {
+ "esModuleInterop": true,
+ "types": ["jest", "node"]
+ }
+}
Review Comment:
This shouldn't be necessary, we already have a tsconfig in the plugin's directory
##########
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:
This seems relevant
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #37580:
URL: https://github.com/apache/superset/pull/37580#issuecomment-3862782681
I think we can close this in favor of the other PR linked by @justinpark
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix(world-map): reset hover highlight on mouse out [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas closed pull request #37580: fix(world-map): reset hover highlight on mouse out
URL: https://github.com/apache/superset/pull/37580
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org