fix(world-map): reset hover highlight on mouse out #37580
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Code Review Agent Run #e879ecActionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+253
to
+254
| // Store original fill color for restoration | ||
| d3.select(this).attr('data-original-fill', d3.select(this).style('fill')); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ World Map hover state may not reset visually.
- ⚠️ Affects Explore and Dashboard world-map charts.
- ⚠️ Visual inconsistency in no-data country highlighting.
Suggested change
| // Store original fill color for restoration | |
| d3.select(this).attr('data-original-fill', d3.select(this).style('fill')); | |
| // 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')); | |
| } |
Steps of Reproduction ✅
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).Prompt for AI Agent 🤖
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.| * under the License. | ||
| */ | ||
|
|
||
| import d3 from 'd3'; |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ WorldMap unit tests can fail with TypeError.
- ⚠️ CI test runs may be flaky across environments.
Suggested change
| import d3 from 'd3'; | |
| import * as d3 from 'd3'; |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.| }; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ WorldMap tests may intermittently assert incorrect handler behavior.
- ⚠️ CI builds can show flaky failures for this test file.
Suggested change
| jest.clearAllMocks(); | |
| // Reset mock implementations and clear call history to avoid leakage between tests | |
| jest.resetAllMocks(); |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.|
|
||
| afterEach(() => { | ||
| document.body.removeChild(container); | ||
| }); |
Contributor
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ WorldMap tests leak spies causing cross-test interference.
- ⚠️ Other tests may be affected by lingering spies.
Suggested change
| }); | |
| // Restore any spies (e.g. jest.spyOn) to avoid leaking altered implementations across tests | |
| jest.restoreAllMocks(); |
Steps of Reproduction ✅
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.Prompt for AI Agent 🤖
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Not applicable.
TESTING INSTRUCTIONS
Manual testing:
original data-driven color.
Automated testing:
on mouse-out for both data and no-data regions.
ADDITIONAL INFORMATION