Store ZIndexPortal refs instead of IDs to unblock shadow DOM#6753
Store ZIndexPortal refs instead of IDs to unblock shadow DOM#6753PavelVanecek merged 2 commits intomainfrom
Conversation
WalkthroughThe pull request refactors the z-index registry to track DOM Element references instead of string IDs. Changes span the state slice, selectors, portal components, and tests, updating registration/unregistration actions to carry actual element objects and adjusting all consumer code accordingly. A new shadow DOM example is added. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx (1)
21-40: Consider memoizing or stabilizingchildrento avoid unnecessary re-renders.Including
childrenin the dependency array will cause the shadow DOM content to unmount and remount on every parent render, sincechildrenis typically a new object reference each time. For a documentation example this may be acceptable, but in production usage this pattern could cause performance issues and state loss.If this is intentional for demonstration purposes, a brief comment explaining the trade-off would help future readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
test-vr/__snapshots__/tests/www/ZIndex.spec-vr.tsx-snapshots/ShadowDomExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ZIndex.spec-vr.tsx-snapshots/ShadowDomExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/ZIndex.spec-vr.tsx-snapshots/ShadowDomExample-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (9)
src/state/zIndexSlice.ts(5 hunks)src/zIndex/ZIndexLayer.tsx(2 hunks)src/zIndex/ZIndexPortal.tsx(1 hunks)src/zIndex/zIndexSelectors.ts(2 hunks)test-vr/tests/www/ZIndex.spec-vr.tsx(1 hunks)test/zIndex/AllZIndexPortals.spec.tsx(3 hunks)test/zIndex/zIndexSelectors.spec.ts(4 hunks)www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx(1 hunks)www/src/docs/apiExamples/ZIndexLayer/index.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.{ts,tsx}: Never useanytype (implicit or explicit) in TypeScript code
Preferunknownoveranyand refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not useastype assertions in TypeScript; the only exception isas const
Files:
test-vr/tests/www/ZIndex.spec-vr.tsxwww/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsxsrc/zIndex/ZIndexPortal.tsxsrc/zIndex/zIndexSelectors.tssrc/zIndex/ZIndexLayer.tsxwww/src/docs/apiExamples/ZIndexLayer/index.tsxtest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsxsrc/state/zIndexSlice.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
test-vr/tests/www/ZIndex.spec-vr.tsxwww/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsxsrc/zIndex/ZIndexPortal.tsxsrc/zIndex/zIndexSelectors.tssrc/zIndex/ZIndexLayer.tsxwww/src/docs/apiExamples/ZIndexLayer/index.tsxtest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsxsrc/state/zIndexSlice.ts
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
All imports from
rechartsmust use the public API entry point; imports from internal paths likerecharts/types/*orrecharts/src/*are not allowed
Files:
test-vr/tests/www/ZIndex.spec-vr.tsxwww/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsxsrc/zIndex/ZIndexPortal.tsxsrc/zIndex/zIndexSelectors.tssrc/zIndex/ZIndexLayer.tsxwww/src/docs/apiExamples/ZIndexLayer/index.tsxtest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsxsrc/state/zIndexSlice.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any strings or formatting choices in library code; users should provide localized strings as desired
Files:
src/zIndex/ZIndexPortal.tsxsrc/zIndex/zIndexSelectors.tssrc/zIndex/ZIndexLayer.tsxsrc/state/zIndexSlice.ts
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
test/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (test/README.md)
test/**/*.{test,spec}.{ts,tsx}: Aim for 100% unit test code coverage when writing new code
Prefer to use thecreateSelectorTestCasehelper function when writing or modifying tests
Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion
Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance
MockgetBoundingClientRectin tests using the helper function provided intest/helper/MockGetBoundingClientRect.ts
Usevi.useFakeTimers()in all tests due to Redux autoBatchEnhancer dependency on timers andrequestAnimationFrame
Callvi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper, and avoidvi.runAllTimers()to prevent infinite loops
UseuserEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })or theuserEventSetuphelper function fromtest/helper/userEventSetup.tswhen creating userEvent instances
When testing tooltips on hover, usevi.runOnlyPendingTimers()after eachuserEvent.hover()call or use theshowTooltiphelper function fromtooltipTestHelpers.tsto account for requestAnimationFrame delays
Files:
test/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
When running unit tests, prefer to run a single test file using
npm run test -- path/to/TestFile.spec.tsxrather than running all tests withnpm test
Files:
test/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
🧠 Learnings (11)
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Verify the number of selector calls using the spy object from `createSelectorTestCase` to spot unnecessary re-renders and improve performance
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxsrc/zIndex/zIndexSelectors.tstest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to test/component/**/*.spec.tsx : Use React Testing Library for testing component interactions and behavior upon rendering
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxwww/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsxtest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use `vi.useFakeTimers()` in all tests due to Redux autoBatchEnhancer dependency on timers and `requestAnimationFrame`
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxtest/zIndex/zIndexSelectors.spec.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Mock `getBoundingClientRect` in tests using the helper function provided in `test/helper/MockGetBoundingClientRect.ts`
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Call `vi.runOnlyPendingTimers()` to advance timers after renders when not using `createSelectorTestCase` helper, and avoid `vi.runAllTimers()` to prevent infinite loops
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxtest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
📚 Learning: 2025-11-25T01:22:59.729Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to storybook/stories/**/*.stories.tsx : Use Storybook for smoke tests and add play functions with assertions for actual tests
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxtest/zIndex/zIndexSelectors.spec.ts
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxtest/zIndex/zIndexSelectors.spec.tstest/zIndex/AllZIndexPortals.spec.tsx
📚 Learning: 2025-11-25T01:23:08.250Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.250Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : When testing tooltips on hover, use `vi.runOnlyPendingTimers()` after each `userEvent.hover()` call or use the `showTooltip` helper function from `tooltipTestHelpers.ts` to account for requestAnimationFrame delays
Applied to files:
test-vr/tests/www/ZIndex.spec-vr.tsxtest/zIndex/zIndexSelectors.spec.ts
📚 Learning: 2025-11-23T13:30:10.395Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.395Z
Learning: The `TooltipIndex` type from recharts is defined in `src/state/tooltipSlice.ts` but is not currently exported from the public API entry points. It should not be imported from `recharts/types/state/tooltipSlice` as this is an internal implementation path. An ESLint rule is needed to prevent regressions.
Applied to files:
www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsxsrc/zIndex/zIndexSelectors.tstest/zIndex/AllZIndexPortals.spec.tsxsrc/state/zIndexSlice.ts
📚 Learning: 2025-12-06T03:36:59.377Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: DEVELOPING.md:0-0
Timestamp: 2025-12-06T03:36:59.377Z
Learning: Applies to **/*.{ts,tsx,js,jsx} : All imports from `recharts` must use the public API entry point; imports from internal paths like `recharts/types/*` or `recharts/src/*` are not allowed
Applied to files:
www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx
📚 Learning: 2025-11-16T09:14:24.918Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6640
File: src/cartesian/Bar.tsx:156-159
Timestamp: 2025-11-16T09:14:24.918Z
Learning: In recharts, SSR (Server-Side Rendering) is not yet supported—charts don't render at all in SSR environments. The `isAnimationActive: 'auto'` mode is preparatory work for future SSR support, so testing of this mode should be deferred until SSR support is actually implemented.
Applied to files:
www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx
🧬 Code graph analysis (8)
test-vr/tests/www/ZIndex.spec-vr.tsx (1)
www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx (1)
ShadowDomExample(53-73)
www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx (1)
storybook/stories/API/props/AnimationProps.ts (1)
isAnimationActive(29-36)
src/zIndex/ZIndexPortal.tsx (1)
src/state/hooks.ts (1)
useAppDispatch(9-15)
src/zIndex/zIndexSelectors.ts (1)
src/state/store.ts (1)
RechartsRootState(23-38)
src/zIndex/ZIndexLayer.tsx (2)
src/state/hooks.ts (1)
useAppSelector(40-50)src/zIndex/zIndexSelectors.ts (1)
selectZIndexPortalElement(13-34)
www/src/docs/apiExamples/ZIndexLayer/index.tsx (2)
www/src/docs/exampleComponents/types.ts (1)
ChartExample(3-23)www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx (1)
ShadowDomExample(53-73)
test/zIndex/zIndexSelectors.spec.ts (2)
src/state/store.ts (1)
createRechartsStore(57-110)src/zIndex/zIndexSelectors.ts (2)
selectZIndexPortalElement(13-34)selectAllRegisteredZIndexes(36-50)
test/zIndex/AllZIndexPortals.spec.tsx (1)
src/util/DataUtils.ts (1)
isNotNil(202-204)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build, Test, Pack
🔇 Additional comments (15)
www/src/docs/apiExamples/ZIndexLayer/index.tsx (1)
4-5: LGTM!The new ShadowDomExample is correctly imported and added to the examples array following the established pattern. The structure matches the
ChartExampletype requirements.Also applies to: 13-17
test-vr/tests/www/ZIndex.spec-vr.tsx (1)
11-14: LGTM!The VR test correctly disables animations for deterministic screenshot comparisons, following the same pattern as the existing
PrahaMetrotest.src/zIndex/ZIndexLayer.tsx (1)
90-92: Clean refactor that correctly addresses the shadow DOM limitation.The change from
document.getElementByIdto direct element references is the right approach. React'screatePortalacceptsElement | DocumentFragmentas the container, so passing the element reference directly is valid and avoids the shadow DOM boundary issue.Also applies to: 99-108
src/zIndex/zIndexSelectors.ts (1)
13-34: LGTM!The selector is correctly updated with explicit return type
Element | undefined. The logic properly accesses the new element-based state structure while maintaining the same null-checking pattern.test/zIndex/AllZIndexPortals.spec.tsx (2)
39-40: Good adaptation to element-based portal handling.Using
g[tabindex="-1"]as the selector is a reasonable approach sinceZIndexPortalcomponents render<g>elements with this attribute. This decouples the test from the previous ID-based implementation.Also applies to: 48-49
65-82: Thoughtful handling of element comparison edge case.The comment explaining why IDs are compared instead of element references directly (to avoid React warnings about accessing internal properties) is helpful for future maintainers. The
isNotNilfilter correctly handles cases whereentry.elementmight be undefined.www/src/docs/apiExamples/ZIndexLayer/ShadowDomExample.tsx (1)
53-73: LGTM!The example component properly demonstrates shadow DOM usage with Recharts, using correct public API imports and appropriate prop types.
test/zIndex/zIndexSelectors.spec.ts (4)
18-36: LGTM!The test correctly verifies element-based portal registration and selection. It properly creates distinct DOM elements for each test case and validates that the selector returns the correct element references after registration and undefined after unregistration.
38-55: LGTM!Good coverage of edge cases including undefined zIndex handling and proper differentiation between panorama and main chart element registrations at the same z-index.
104-111: LGTM!Good addition to verify that element registration doesn't affect the zIndex list memoization, ensuring the selector properly separates concerns between portal existence and element references.
127-149: LGTM!Thorough test coverage for element registration/unregistration behavior, correctly verifying that the zIndex list remains stable and memoized when only element references change.
src/zIndex/ZIndexPortal.tsx (1)
7-20: LGTM!The migration from ID-based to ref-based portal registration is well implemented. The conditional registration check (
if (ref.current)) correctly guards against null refs, and the cleanup properly unregisters using onlyzIndexandisPanorama. This approach will work correctly in shadow DOM contexts wheredocument.getElementByIdis unavailable.src/state/zIndexSlice.ts (3)
91-109: LGTM!The use of
castDraftfrom Immer is the correct approach for storing DOM Element references in Redux state. This works well becauseserializableCheckis already disabled in the store configuration (src/state/store.ts).The implementation correctly handles both registration into existing entries and creation of new entries with proper conditional assignment based on
isPanorama.
110-122: LGTM!The unregistration correctly only clears the element reference without affecting the consumer count or entry existence. This maintains proper separation between portal lifecycle (
registerZIndexPortal/unregisterZIndexPortal) and element reference management.
10-26: LGTM!The type documentation is well updated to reflect the shift from element IDs to Element references. The explanation about the "registration in progress" state (undefined element) is helpful for understanding the two-phase registration pattern.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6753 +/- ##
==========================================
- Coverage 93.91% 93.82% -0.09%
==========================================
Files 514 515 +1
Lines 42833 42897 +64
Branches 4991 4989 -2
==========================================
+ Hits 40227 40249 +22
- Misses 2600 2642 +42
Partials 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Bundle ReportChanges will decrease total bundle size by 552 bytes (-0.02%) ⬇️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
ckifer
left a comment
There was a problem hiding this comment.
Any downsides to doing this at all besides storing more unserializable state?
|
Jest/vitest don't like tests with elements in them, the error message was quite cryptic. Everything else seems to work fine. |
Description
document.getElementById is not available inside a shadow DOM so we can't use that.
Related Issue
Fixes #6752
Types of changes
Checklist:
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.