Fix(z-index): prevent elements from disappearing on rapid zIndex changes #7002
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new interactive React example component demonstrating Recharts z-index behavior with legend-driven line opacity and stacking control, and registers it in the LineChart documentation examples gallery. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@test/zIndex/DynamicZIndex.spec.tsx`:
- Around line 47-49: The comment in the DynamicZIndex.spec.tsx test is stale and
contradicts the assertion in the test that expects two lines (no flicker);
update or remove the comment so it matches the test intent ("should not remove
data curves") and the PR fix: delete the phrase about expecting a flicker/return
null and replace it with a brief note that the immediate expectation is 2 lines
(no flicker) to reflect the assertion and goal of the change in the test case
(refer to the test named DynamicZIndex and the assertion checking for two
lines).
- Line 1: Add vi fake timers to this test file: import vi from 'vitest' (or
include vi in the existing import), call vi.useFakeTimers() at test setup (e.g.,
top-level beforeAll or immediately inside the describe for DynamicZIndex tests)
and ensure you call vi.runOnlyPendingTimers() after any render/interaction steps
(or in afterEach) to flush timers; also restore timers with vi.useRealTimers()
in teardown if needed.
| @@ -0,0 +1,60 @@ | |||
| import { describe, it, expect } from 'vitest'; | |||
There was a problem hiding this comment.
Missing vi.useFakeTimers() — required per project guidelines.
All tests in this repo must use vi.useFakeTimers() due to Redux autoBatchEnhancer dependency on timers and requestAnimationFrame. Without fake timers, this test may behave non-deterministically. You'll also need to import vi from vitest and call vi.runOnlyPendingTimers() after renders/interactions.
Proposed fix
-import { describe, it, expect } from 'vitest';
+import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
import React, { useState } from 'react';
import { render, act, waitFor } from '@testing-library/react';
import { LineChart, Line, XAxis, YAxis } from '../../src';
describe('Dynamic zIndex updates', () => {
+ beforeEach(() => {
+ vi.useFakeTimers();
+ });
+
+ afterEach(() => {
+ vi.useRealTimers();
+ });
+
it('should keep the line in the DOM when zIndex changes dynamically', async () => {As per coding guidelines: "Use vi.useFakeTimers() in all tests due to Redux autoBatchEnhancer dependency on timers and requestAnimationFrame".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { describe, it, expect } from 'vitest'; | |
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | |
| import React, { useState } from 'react'; | |
| import { render, act, waitFor } from '@testing-library/react'; | |
| import { LineChart, Line, XAxis, YAxis } from '../../src'; | |
| describe('Dynamic zIndex updates', () => { | |
| beforeEach(() => { | |
| vi.useFakeTimers(); | |
| }); | |
| afterEach(() => { | |
| vi.useRealTimers(); | |
| }); | |
| it('should keep the line in the DOM when zIndex changes dynamically', async () => { |
🤖 Prompt for AI Agents
In `@test/zIndex/DynamicZIndex.spec.tsx` at line 1, Add vi fake timers to this
test file: import vi from 'vitest' (or include vi in the existing import), call
vi.useFakeTimers() at test setup (e.g., top-level beforeAll or immediately
inside the describe for DynamicZIndex tests) and ensure you call
vi.runOnlyPendingTimers() after any render/interaction steps (or in afterEach)
to flush timers; also restore timers with vi.useRealTimers() in teardown if
needed.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7002 +/- ##
==========================================
- Coverage 90.10% 90.09% -0.01%
==========================================
Files 522 523 +1
Lines 38848 38921 +73
Branches 5347 5353 +6
==========================================
+ Hits 35004 35067 +63
- Misses 3835 3845 +10
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Are you sure this is the fix? Last time I checked, moving from DOM to Portal triggers unmount anyway. So we would end up with the exact same problem, but this time with a flash of content in the wrong z-index position. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/zIndex/ZIndexLayer.tsx`:
- Around line 93-106: When shouldRenderInPortal becomes false we must unregister
any previously registered zIndexes to avoid leaks: in the useLayoutEffect that
currently returns noop, add a branch for !shouldRenderInPortal that iterates
registeredZIndexesRef.current, dispatches unregisterZIndexPortal({ zIndex:
<each> }) for each registered zIndex, and clears the set
(registeredZIndexesRef.current.clear()); keep the existing registration logic
when shouldRenderInPortal is true and still guard with
registeredZIndexesRef.current.has(zIndex) before dispatching
registerZIndexPortal.
🧹 Nitpick comments (1)
src/zIndex/ZIndexLayer.tsx (1)
124-137: Mutating Set duringforEach— safe but worth a brief note.Deleting entries from a
SetduringforEachis well-defined per the ES spec, so this is correct. A small inline comment could prevent future maintainers from second-guessing it.
bfb2d0e to
27d635d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/zIndex/ZIndexLayer.tsx`:
- Around line 103-108: The forEach callback is implicitly returning
dispatch(...) which triggers a static-analysis warning; change the arrow
callbacks to use a block body that calls dispatch without returning (e.g.,
replace z => dispatch(unregisterZIndexPortal({ zIndex: z })) with z => {
dispatch(unregisterZIndexPortal({ zIndex: z })); } ) for the cleanup using
registeredZIndexesRef.current and do the same for the similar usage in the
unmount cleanup (the place that calls dispatch(register/unregisterZIndexPortal)
and touches lastPortalElementRef.current). Ensure no value is returned from the
forEach callbacks.
🧹 Nitpick comments (1)
src/zIndex/ZIndexLayer.tsx (1)
143-150: Rendering into a stale portal during transitions — intentional trade-off worth documenting.When
zIndexchanges rapidly, children render into the old portal<g>(at the previous z-order) until the new one is ready. This avoids the disappearing-content bug but produces a brief frame where content sits at the wrong layer. The reviewer PavelVanecek flagged a similar concern.This is a reasonable trade-off for the fix, but consider adding a brief inline comment noting that the stale-portal render is at the wrong z-order intentionally, so future maintainers don't treat it as a bug.
| // Portal rendering was disabled — clean up any stale registrations | ||
| const registered = registeredZIndexesRef.current; | ||
| registered.forEach(z => dispatch(unregisterZIndexPortal({ zIndex: z }))); | ||
| registered.clear(); | ||
| lastPortalElementRef.current = undefined; | ||
| return; |
There was a problem hiding this comment.
Static analysis: forEach callback should not return a value.
dispatch() returns a value, and the arrow shorthand z => dispatch(...) implicitly returns it. Biome flags this as useIterableCallbackReturn. Same issue on line 134.
Proposed fix
- registered.forEach(z => dispatch(unregisterZIndexPortal({ zIndex: z })));
+ registered.forEach(z => {
+ dispatch(unregisterZIndexPortal({ zIndex: z }));
+ });And similarly for the unmount cleanup:
- registered.forEach(z => dispatch(unregisterZIndexPortal({ zIndex: z })));
+ registered.forEach(z => {
+ dispatch(unregisterZIndexPortal({ zIndex: z }));
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Portal rendering was disabled — clean up any stale registrations | |
| const registered = registeredZIndexesRef.current; | |
| registered.forEach(z => dispatch(unregisterZIndexPortal({ zIndex: z }))); | |
| registered.clear(); | |
| lastPortalElementRef.current = undefined; | |
| return; | |
| // Portal rendering was disabled — clean up any stale registrations | |
| const registered = registeredZIndexesRef.current; | |
| registered.forEach(z => { | |
| dispatch(unregisterZIndexPortal({ zIndex: z })); | |
| }); | |
| registered.clear(); | |
| lastPortalElementRef.current = undefined; | |
| return; |
🧰 Tools
🪛 Biome (2.3.14)
[error] 105-105: This callback passed to forEach() iterable method should not return a value.
Either remove this return or remove the returned value.
(lint/suspicious/useIterableCallbackReturn)
🤖 Prompt for AI Agents
In `@src/zIndex/ZIndexLayer.tsx` around lines 103 - 108, The forEach callback is
implicitly returning dispatch(...) which triggers a static-analysis warning;
change the arrow callbacks to use a block body that calls dispatch without
returning (e.g., replace z => dispatch(unregisterZIndexPortal({ zIndex: z }))
with z => { dispatch(unregisterZIndexPortal({ zIndex: z })); } ) for the cleanup
using registeredZIndexesRef.current and do the same for the similar usage in the
unmount cleanup (the place that calls dispatch(register/unregisterZIndexPortal)
and touches lastPortalElementRef.current). Ensure no value is returned from the
forEach callbacks.
|
Hi @PavelVanecek , thanks for pointing that out and you're absolutely right about the version of this fix. Rendering inline as a fallback would still trigger an unmount when transitioning from inline to portal, just trading "flash of nothing" for "flash at wrong position." I've since reworked the approach. Instead of falling back to inline rendering, the fix now keeps the old portal alive during the transition: When zIndex changes (e.g., 10 to 11), we register the new zIndex without immediately unregistering the old one Happy to discuss if you see any issues with this approach or if you'd prefer a different direction entirely! |
|
That might work. Let's also add a new website example so that we can test this in browser. Undo the current changes and push the example chart only so that we can compare before & after. |
Adds a website example where hovering over legend items dynamically changes the zIndex of lines. This reproduces the issue where chart elements disappear during rapid zIndex transitions. The example uses three Line components with legend-driven opacity and zIndex changes. Hover over legend items in quick succession to observe the bug — lines vanish from the chart.
27d635d to
5764b4a
Compare
|
I've pushed just the website example for now — it's a DynamicZIndexLineChart under the LineChart examples. You can hover over the legend items rapidly to see the lines disappear. All fix-related changes have been reverted so this commit shows the "before" state. Once you've confirmed the example reproduces the issue well, I'll add the fix as a separate commit so we can compare before & after. Let me know if the example needs any changes! |
ckifer
left a comment
ckifer
left a comment
There was a problem hiding this comment.
Example LGTM. I'll merge this and you can make a new pr with the changes
Bundle ReportBundle size has no change ✅ |
3 participants
Description
When
zIndexis updated dynamically (e.g., hovering legend items in quick succession), chart elements (curves, bars, etc.) would disappear from the DOM entirely. This was caused by ZIndexLayer returningnullwhile waiting for a new portal element to register after azIndexchange.This fix updates ZIndexLayer to render children inline (without a portal) as a fallback when the portal element is not yet available, ensuring content remains visible during the async portal registration cycle.
Related Issue
Fixes #6789
Motivation and Context
The portal-based
zIndexsystem in Recharts v3 is inherently asynchronous:zIndexis registered viauseLayoutEffect→ Redux dispatch.ZIndexPortalsre-renders to create the new portal DOM element.createPortal.Between steps 1–3, ZIndexLayer was returning
null, unmounting the content. With rapidzIndexchanges, this caused persistent disappearance of chart elements.The fix renders children inline during this gap, so the content is always visible — it just temporarily renders outside the portal layer until the portal is ready.
How Has This Been Tested?
zIndexupdates on Line components and asserts that both lines remain in the DOM after each update.Types of changes
Checklist:
Summary by CodeRabbit