fix(Pie): prevent cross-highlighting in multi-Pie charts with shared dataKey#6678
Conversation
WalkthroughThreads a graphical item id ( Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PieOuter as Pie (dataKey:'uv', id:outer)
participant PieInner as Pie (dataKey:'pv', id:inner)
participant TooltipCtx as TooltipDispatcher
participant TooltipState as TooltipInteractionState
User->>PieOuter: hover sector (coords)
PieOuter->>TooltipCtx: emit hover { coords, dataKey:'uv', graphicalItemId:outer }
TooltipCtx->>TooltipState: set active { coords, dataKey:'uv', index, graphicalItemId:outer }
rect rgb(220,245,220)
Note over TooltipState: match active sector using<br/>coords + dataKey + graphicalItemId
end
TooltipState->>PieOuter: mark sector active (renderActiveShape)
TooltipState->>PieInner: no activation (different dataKey or id)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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: 1
🧹 Nitpick comments (1)
src/polar/Pie.tsx (1)
38-42: Active-state gating by index, dataKey, and coordinate is sound; consider tighteningcoordinateMatchestypingThe new active-sector logic in
PieSectors:
- Reads
activeIndex,activeDataKey, andactiveCoordinatefrom tooltip selectors.- Defines a sector as active only when:
- its local index string matches
activeIndex,- either no
activeDataKeyis set or it equals this pie’sdataKey, and- its
tooltipPositionis within 0.01px ofactiveCoordinate.This neatly fixes the multi-Pie cross-highlighting case when pies share a dataKey but differ geometrically: only the sector whose tooltip position matches the active tooltip coordinate becomes active.
One small refinement:
coordinateMatchesis currently:const coordinateMatches = activeCoordinate && entry.tooltipPosition && Math.abs(activeCoordinate.x - entry.tooltipPosition.x) < 0.01 && Math.abs(activeCoordinate.y - entry.tooltipPosition.y) < 0.01;Since
entry.tooltipPositionis always defined andactiveCoordinatemay be undefined, this expression can beundefinedrather than a strict boolean. It still behaves as falsy in the&&chain, but normalizing it to a boolean would be clearer:- const coordinateMatches = - activeCoordinate && - entry.tooltipPosition && - Math.abs(activeCoordinate.x - entry.tooltipPosition.x) < 0.01 && - Math.abs(activeCoordinate.y - entry.tooltipPosition.y) < 0.01; + const coordinateMatches = + activeCoordinate != null && + Math.abs(activeCoordinate.x - entry.tooltipPosition.x) < 0.01 && + Math.abs(activeCoordinate.y - entry.tooltipPosition.y) < 0.01;This keeps behavior the same while making
coordinateMatchesexplicitlyboolean.Also applies to: 490-555
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/polar/Pie.tsx(9 hunks)test/polar/Pie-TwoLevelPieChart.spec.tsx(1 hunks)test/polar/Pie.spec.tsx(6 hunks)test/state/selectors/pieSelectors.spec.tsx(16 hunks)www/src/docs/exampleComponents/PieChart/CustomActiveShapePieChart.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-23T13:30:10.388Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.388Z
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:
test/polar/Pie-TwoLevelPieChart.spec.tsxwww/src/docs/exampleComponents/PieChart/CustomActiveShapePieChart.tsxsrc/polar/Pie.tsx
🧬 Code graph analysis (2)
test/polar/Pie-TwoLevelPieChart.spec.tsx (1)
src/polar/Pie.tsx (2)
PieSectorDataItem(158-162)Pie(856-896)
src/polar/Pie.tsx (4)
src/util/types.ts (1)
DataKey(59-59)src/state/graphicalItemsSlice.ts (1)
GraphicalItemId(19-19)src/state/selectors/tooltipSelectors.ts (2)
selectActiveTooltipDataKey(405-414)selectActiveTooltipCoordinate(435-444)src/util/useUniqueId.ts (1)
WithoutId(37-37)
⏰ 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 (4)
test/polar/Pie.spec.tsx (1)
360-406:dataKeyassertions in Pie tests correctly reflect the updated sector shapeThe added
dataKeyexpectations for:
activeShapepayload ('y'),PieLabelRenderProps('value'), and- event handler payloads (
'uv')are consistent with
PieSectorDataItemnow carrying adataKeyand withcomputePieSectorsassigning that field. This keeps the tests aligned with the new public shape of sector data and validates important integration points (labels, tooltips, and events).Also applies to: 500-555, 1862-1901, 1939-2035, 2069-2111
test/state/selectors/pieSelectors.spec.tsx (1)
47-377: Selector expectations fordataKeyare consistent with updatedPieSectorDataItemThe updated expected results now asserting
dataKey:
'uv'before the dataKey toggle,'pv'after toggling, and'value'for the custom-data test,match the
<Pie>configuration and the newPieSectorDataItemshape. This gives good coverage thatselectPieSectorsreflects the current dataKey and that the additional field doesn’t interfere with the spread of arbitrary payload properties.Also applies to: 389-719, 752-848
test/polar/Pie-TwoLevelPieChart.spec.tsx (1)
1-126: Two-level Pie tests accurately capture the active-shape regression and its fixBoth tests exercise the core scenarios:
- Two pies sharing
dataKey="value"now yield exactly one active shape for whichever sector is hovered, and clear correctly on mouse out.- Pies with different dataKeys (
uvvspv) still activate the right sector independently.This gives solid regression coverage for the cross-highlighting issue in multi-Pie charts and validates the new coordinate+dataKey-based activation behavior.
src/polar/Pie.tsx (1)
135-146: PropagatingdataKeyintoPieSectorDataItemand computePieSectors looks correctAdding
dataKey?: DataKey<any>toPieSectorDataand then assigningdataKeyinsidecomputePieSectorsensures eachPieSectorDataItemcarries the source dataKey:
- Tooltip payloads and selector outputs now have a stable
dataKeyto assert on.- Event handlers and labels receive sector props that include the same
dataKeythe<Pie>was configured with (string or function), matching the updated tests.The wiring from
PieSettings.dataKeythroughcomputePieSectorsto sector objects is consistent and preserves behavior for the default'value'dataKey.Also applies to: 560-635
| import { Pie, PieChart, Sector, Tooltip } from 'recharts'; | ||
| import { PieSectorDataItem } from 'recharts/types/polar/Pie'; | ||
| import { TooltipIndex } from 'recharts/types/state/tooltipSlice'; |
There was a problem hiding this comment.
Use shared PieSectorDataItem type, but avoid importing TooltipIndex from an internal path
Typing renderActiveShape with the exported PieSectorDataItem keeps this example aligned with the actual sector props and is a solid improvement.
However, TooltipIndex is imported from 'recharts/types/state/tooltipSlice', which is an internal implementation path and not part of the public API surface. Using it in a public-facing docs example encourages consumers to rely on unsupported deep imports.
For this example component, consider either:
- Typing
defaultIndexmore generically (e.g.number | undefined) or with a small local union that reflects the supported values, or - Re-exporting
TooltipIndexfrom the mainrechartsentry point and importing it from there before using it in docs.
Based on learnings, this avoids leaking internal state types into public examples.
Also applies to: 71-77
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6678 +/- ##
==========================================
- Coverage 94.02% 94.02% -0.01%
==========================================
Files 499 499
Lines 42547 42577 +30
Branches 4873 4878 +5
==========================================
+ Hits 40004 40032 +28
- Misses 2538 2540 +2
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ckifer
left a comment
There was a problem hiding this comment.
This might be a good interim but I don't really think this works long term - can you give a different approach a try and if it doesn't work we can merge this for now?
src/polar/Pie.tsx
Outdated
| // For Pie charts, when multiple Pies share the same dataKey, we need to ensure only the hovered Pie's sector is active. | ||
| // We do this by checking if the active tooltip coordinate matches this sector's tooltip position. | ||
| // This works because each sector has a unique tooltip position based on its midAngle and middleRadius. | ||
| const coordinateMatches = |
There was a problem hiding this comment.
This will lead to issues down the road if we ever want to support Pie tooltips that are not a fixed position. I wonder if there is a way to use graphicalItemId here to identify what pie is being hovered. Can we add graphicalItemId to the active item state?
There was a problem hiding this comment.
+1 let's have this based on the graphical item ID.
There was a problem hiding this comment.
@ckifer I have made the necessary changes and upadted stackblitz link as well. Please do take a look
Thanks!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/component/Tooltip/Tooltip.sync.spec.tsx (1)
446-461: Consider usingexpectLastCalledWithconsistently for spy assertionsThis spec already imports
expectLastCalledWith, but many tests still useexpect(spy).toHaveBeenLastCalledWith(...). Converting those toexpectLastCalledWith(spy, ...)would better follow the local testing conventions and keep typings/autocomplete uniform across the suite.Also applies to: 570-583, 795-901
src/state/tooltipSlice.ts (1)
358-363: Consider propagatingactiveGraphicalItemIdintokeyboardInteractionas well
TooltipInteractionStatenow carriesgraphicalItemId, and mouse interactions/sync state set it, butsetKeyboardInteractionignoresactiveGraphicalItemId. If you later support keyboard navigation between multiple graphical items that share the sameindex/dataKey(e.g. multiple Pies), you may want to storegraphicalItemIdhere too for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/context/tooltipContext.tsx(4 hunks)src/state/selectors/combiners/combineTooltipInteractionState.ts(1 hunks)src/state/selectors/tooltipSelectors.ts(1 hunks)src/state/tooltipSlice.ts(6 hunks)src/synchronisation/useChartSynchronisation.tsx(3 hunks)test/cartesian/Bar.spec.tsx(1 hunks)test/chart/ScatterChart.spec.tsx(3 hunks)test/component/Tooltip/Tooltip.sync.spec.tsx(11 hunks)test/component/Tooltip/Tooltip.visibility.spec.tsx(7 hunks)test/polar/Pie.spec.tsx(14 hunks)test/state/selectors/combiners/combineActiveTooltipIndex.spec.ts(1 hunks)test/synchronisation/eventCenter.spec.ts(2 hunks)test/synchronisation/useChartSynchronisation.spec.tsx(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/polar/Pie.spec.tsx
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run type checking on the codebase using
npm run check-types
**/*.{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/component/Tooltip/Tooltip.visibility.spec.tsxsrc/state/selectors/tooltipSelectors.tssrc/state/selectors/combiners/combineTooltipInteractionState.tstest/synchronisation/eventCenter.spec.tstest/chart/ScatterChart.spec.tsxsrc/state/tooltipSlice.tssrc/synchronisation/useChartSynchronisation.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxsrc/context/tooltipContext.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.spec.tsx
{test,www/test}/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Write unit tests in the
testorwww/testdirectories with.spec.tsxfile extension
Files:
test/component/Tooltip/Tooltip.visibility.spec.tsxtest/synchronisation/eventCenter.spec.tstest/chart/ScatterChart.spec.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.spec.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/component/Tooltip/Tooltip.visibility.spec.tsxtest/synchronisation/eventCenter.spec.tstest/chart/ScatterChart.spec.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.spec.tsx
test/component/**/*.spec.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use React Testing Library for testing component interactions and behavior upon rendering
Files:
test/component/Tooltip/Tooltip.visibility.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
test/component/Tooltip/Tooltip.visibility.spec.tsxsrc/state/selectors/tooltipSelectors.tssrc/state/selectors/combiners/combineTooltipInteractionState.tstest/synchronisation/eventCenter.spec.tstest/chart/ScatterChart.spec.tsxsrc/state/tooltipSlice.tssrc/synchronisation/useChartSynchronisation.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxsrc/context/tooltipContext.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.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/component/Tooltip/Tooltip.visibility.spec.tsxtest/synchronisation/eventCenter.spec.tstest/chart/ScatterChart.spec.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.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/component/Tooltip/Tooltip.visibility.spec.tsxtest/synchronisation/eventCenter.spec.tstest/chart/ScatterChart.spec.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.spec.tsx
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run ESLint and Prettier on the codebase using
npm run lint
Files:
src/state/selectors/tooltipSelectors.tssrc/state/selectors/combiners/combineTooltipInteractionState.tssrc/state/tooltipSlice.tssrc/synchronisation/useChartSynchronisation.tsxsrc/context/tooltipContext.tsx
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/state/selectors/tooltipSelectors.tssrc/state/selectors/combiners/combineTooltipInteractionState.tssrc/state/tooltipSlice.tssrc/synchronisation/useChartSynchronisation.tsxsrc/context/tooltipContext.tsx
🧠 Learnings (7)
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
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/component/Tooltip/Tooltip.visibility.spec.tsxsrc/state/selectors/tooltipSelectors.tstest/chart/ScatterChart.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.spec.tsx
📚 Learning: 2025-11-23T13:30:10.388Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.388Z
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:
test/component/Tooltip/Tooltip.visibility.spec.tsxsrc/state/selectors/tooltipSelectors.tssrc/state/selectors/combiners/combineTooltipInteractionState.tstest/chart/ScatterChart.spec.tsxsrc/state/tooltipSlice.tssrc/synchronisation/useChartSynchronisation.tsxtest/cartesian/Bar.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsxsrc/context/tooltipContext.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.tstest/synchronisation/useChartSynchronisation.spec.tsx
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use `userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers })` or the `userEventSetup` helper function from `test/helper/userEventSetup.ts` when creating userEvent instances
Applied to files:
test/synchronisation/eventCenter.spec.ts
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
Applied to files:
test/component/Tooltip/Tooltip.sync.spec.tsx
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
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/component/Tooltip/Tooltip.sync.spec.tsxtest/state/selectors/combiners/combineActiveTooltipIndex.spec.ts
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests
Applied to files:
test/state/selectors/combiners/combineActiveTooltipIndex.spec.ts
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
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/state/selectors/combiners/combineActiveTooltipIndex.spec.ts
🧬 Code graph analysis (1)
src/state/selectors/tooltipSelectors.ts (3)
src/state/store.ts (1)
RechartsRootState(95-95)src/state/selectors/selectors.ts (1)
selectTooltipInteractionState(76-84)src/state/tooltipSlice.ts (1)
TooltipInteractionState(118-162)
🔇 Additional comments (12)
test/synchronisation/useChartSynchronisation.spec.tsx (1)
44-60: Tooltip sync tests correctly assert newgraphicalItemIdfieldAdding
graphicalItemId: undefinedto the expectedsetSyncInteractionpayloads matches the updated sync state shape and the hook’s behavior (initial emit, hover, and leave). No issues here.Also applies to: 62-101, 115-141
src/synchronisation/useChartSynchronisation.tsx (1)
95-110:graphicalItemIdpropagation and reset look consistent in sync listener and emitter
- Reset path now clears
graphicalItemIdalongside other fields when sync cannot apply, which keeps state coherent.- Derived sync actions forward
action.payload.graphicalItemId, and outgoing sync from this hook explicitly sendsgraphicalItemId: undefined, which matches its axis‑driven role.
Implementation looks sound.Also applies to: 120-129, 236-245
test/component/Tooltip/Tooltip.sync.spec.tsx (1)
484-499: Tooltip sync tests correctly includegraphicalItemIdin interaction stateThe added
graphicalItemId: undefinedfields insetSyncInteractiondispatches andselectSynchronisedTooltipStateexpectations are consistent with the extended tooltip state and reducer behavior. Shapes and values all line up.Also applies to: 528-581, 795-901
src/context/tooltipContext.tsx (1)
21-38: Hook changes cleanly threadgraphicalItemIdinto tooltip interaction stateAdding the optional
graphicalItemIdparameter and passing it asactiveGraphicalItemIdin both mouse‑enter and click dispatchers is type‑safe, backward‑compatible, and aligns with the extended tooltip slice. Looks good.Also applies to: 50-67
src/state/tooltipSlice.ts (1)
145-162: Graphical item identification is threaded cleanly through tooltip state and mouse interactionsThe new
graphicalItemIdfield is added consistently toTooltipInteractionState,noInteraction,syncInteraction, and the mouse over/click reducers, and the payload type is extended appropriately. This gives you the extra discriminant you need without changing existing semantics.Also applies to: 180-186, 239-257, 268-273, 307-338
test/cartesian/Bar.spec.tsx (1)
1652-1719: Tooltip state expectation correctly updated forgraphicalItemIdIncluding
graphicalItemId: undefinedinitemInteraction.hoverkeeps the test aligned with the newTooltipInteractionStateshape while preserving existing behavior for Bar hover.test/synchronisation/eventCenter.spec.ts (1)
15-48: Sync interaction payload now matches extended tooltip stateAdding
graphicalItemId: undefinedto thesetSyncInteractionpayload and expectation correctly reflects the newTooltipSyncStateshape without changing test intent.test/chart/ScatterChart.spec.tsx (1)
1537-1591: Scatter tooltip state tests correctly account forgraphicalItemIdThe added
graphicalItemId: undefinedfields in theTooltipStateexpectations match the expanded interaction state shape and do not alter the behavioral assertions of these tests.test/state/selectors/combiners/combineActiveTooltipIndex.spec.ts (1)
7-13: Test helper now matchesTooltipInteractionStateshapeExtending
createInteractionwithgraphicalItemId: undefinedis the right way to keep these selector tests aligned with the updated tooltip interaction state type.test/component/Tooltip/Tooltip.visibility.spec.tsx (1)
1063-1227: Radar tooltip visibility state tests updated appropriately forgraphicalItemIdAdding
graphicalItemId: undefinedacross axis, item, keyboard, and sync interaction expectations keeps these visibility tests consistent with the extended tooltip state while preserving their behavioral focus.src/state/selectors/tooltipSelectors.ts (1)
416-425: New selectorselectActiveTooltipGraphicalItemIdaligns with existing tooltip selector patternsThis selector cleanly exposes
tooltipInteraction.graphicalItemIdviacreateSelector, matching the shape and style ofselectActiveTooltipDataKeyand fitting well into the existing selector API.src/state/selectors/combiners/combineTooltipInteractionState.ts (1)
62-69: Default-index interaction state now includesgraphicalItemIdExtending the defaultIndex-derived
TooltipInteractionStatewithgraphicalItemId: undefinedkeeps this path consistent with the updated type and other interaction sources.
…dataKey Fixes recharts#6259 Add coordinate-based matching to ensure only the hovered Pie shows active sectors when multiple Pies share the same dataKey. Compare tooltip coordinates with sector positions using 0.01 tolerance for precise Pie identification. - Add coordinate matching logic in PieSectors component - Include dataKey in sector data for tracking - Add tests for TwoLevelPieChart behavior
814d45f to
2992877
Compare
Bundle ReportChanges will increase total bundle size by 1.73kB (0.07%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/polar/Pie.tsx (2)
323-323:GraphicalItemIdplumbed through but unused – either hook into active matching or drop for now
id: GraphicalItemIdis added toPieSectorsProps, threaded throughSectorsWithAnimation, and passed down fromPieImpl, but it is never actually read inPieSectors. As it stands, this extra prop doesn’t change behavior and adds a bit of noise.Given earlier discussion about basing active state on graphical item identity, this
idlooks like the right hook to gateisActiveon the current Pie instance instead of relying solely on coordinates.Either (a) complete the wiring by reading
idinsidePieSectorsand combining it with anactiveGraphicalItemIdfrom tooltip state when computingisActive, or (b) remove theidparameter fromPieSectorsProps/SectorsWithAnimationand the<SectorsWithAnimation ... id={id} />call until it’s actually used. If you go with (a), adding a focused unit test that verifies only the Pie whoseGraphicalItemIdmatches the active tooltip item becomes active will help guard against regressions.Also applies to: 728-729, 732-733, 819-820, 887-887
554-566: Active-sector detection tied to tooltip coordinate may conflict with future/non-fixed tooltip positioningThe new
coordinateMatchescheck (with a very tight< 0.01epsilon) fixes the immediate “multi-Pie with shareddataKeyactivates all slices” bug by ensuring only the Pie whosetooltipPositionmatches the active tooltip coordinate becomes active. However, this also couplesisActiveto the tooltip’s coordinate:
- If
TooltipInteractionState.coordinateis ever decoupled from the sector center (e.g., custom or movable tooltip positions, or different coordinate strategies across synced charts),coordinateMatchesmay always be false and no sector will be active even thoughactiveIndexandactiveDataKeyare set.- Prior reviewer feedback already called out preferring a
GraphicalItemId-based approach to avoid exactly this coupling; this change doesn’t yet make use ofGraphicalItemIdeven thoughidis now threaded intoPieSectors.Given that
GraphicalItemIdis already available, I’d strongly recommend incorporating an “active graphical item” check intoisActive(e.g., via anactiveGraphicalItemIdselector) and relying primarily onid + activeIndex + activeDataKeyfor matching, using coordinates only as a fallback if needed. That would both solve the multi-Pie bug and avoid blocking future work on non-fixed Pie tooltips. Please confirm whether tooltip state already exposesgraphicalItemId; if so, we can wire it intoPieSectorsnow, otherwise this PR might be a good place to add that selector.
🧹 Nitpick comments (1)
src/polar/Pie.tsx (1)
135-145: PropagatingdataKeyintoPieSectorDataItemis reasonable but overrides userdataKeyfieldsAdding
dataKey?: DataKey<any>toPieSectorDataand then assigningdataKeyinsidecomputePieSectorsensures each sector carries the Pie’sdataKey, which is helpful for selectors and debugging.Because
entryWithCellInfois spread beforedataKey, anydataKeyfield present on the original data objects will now be overwritten by the chart’sdataKey. That’s probably what we want (sectordataKeymatches the accessor used), but it is a subtle behavioral change for users who previously relied on adataKeyproperty in their data payloads.Consider either documenting this override behavior for
PieSectorDataItem.dataKey, or, if preserving payloaddataKeyis important, renaming this field to something less likely to collide (e.g.,sectorDataKey) and updating the selectors accordingly. A quick repo search for consumers ofPieSectorDataItem['dataKey']can confirm current expectations.Also applies to: 660-677
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/context/tooltipContext.tsx(4 hunks)src/polar/Pie.tsx(9 hunks)src/state/selectors/combiners/combineTooltipInteractionState.ts(1 hunks)src/state/selectors/tooltipSelectors.ts(1 hunks)src/state/tooltipSlice.ts(6 hunks)src/synchronisation/useChartSynchronisation.tsx(3 hunks)test/cartesian/Bar.spec.tsx(1 hunks)test/chart/ScatterChart.spec.tsx(3 hunks)test/component/Tooltip/Tooltip.sync.spec.tsx(11 hunks)test/component/Tooltip/Tooltip.visibility.spec.tsx(7 hunks)test/polar/Pie-TwoLevelPieChart.spec.tsx(1 hunks)test/polar/Pie.spec.tsx(14 hunks)test/state/selectors/combiners/combineActiveTooltipIndex.spec.ts(1 hunks)test/state/selectors/pieSelectors.spec.tsx(16 hunks)test/synchronisation/eventCenter.spec.ts(2 hunks)test/synchronisation/useChartSynchronisation.spec.tsx(4 hunks)www/src/docs/exampleComponents/PieChart/CustomActiveShapePieChart.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- test/state/selectors/combiners/combineActiveTooltipIndex.spec.ts
- www/src/docs/exampleComponents/PieChart/CustomActiveShapePieChart.tsx
- test/component/Tooltip/Tooltip.visibility.spec.tsx
- test/synchronisation/useChartSynchronisation.spec.tsx
- src/context/tooltipContext.tsx
- test/synchronisation/eventCenter.spec.ts
- test/cartesian/Bar.spec.tsx
- test/polar/Pie-TwoLevelPieChart.spec.tsx
🧰 Additional context used
📓 Path-based instructions (9)
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run ESLint and Prettier on the codebase using
npm run lint
Files:
src/state/tooltipSlice.tssrc/state/selectors/tooltipSelectors.tssrc/polar/Pie.tsxsrc/synchronisation/useChartSynchronisation.tsxsrc/state/selectors/combiners/combineTooltipInteractionState.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run type checking on the codebase using
npm run check-types
**/*.{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:
src/state/tooltipSlice.tssrc/state/selectors/tooltipSelectors.tstest/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxsrc/polar/Pie.tsxsrc/synchronisation/useChartSynchronisation.tsxsrc/state/selectors/combiners/combineTooltipInteractionState.tstest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
src/state/tooltipSlice.tssrc/state/selectors/tooltipSelectors.tstest/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxsrc/polar/Pie.tsxsrc/synchronisation/useChartSynchronisation.tsxsrc/state/selectors/combiners/combineTooltipInteractionState.tstest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
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/state/tooltipSlice.tssrc/state/selectors/tooltipSelectors.tssrc/polar/Pie.tsxsrc/synchronisation/useChartSynchronisation.tsxsrc/state/selectors/combiners/combineTooltipInteractionState.ts
{test,www/test}/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Write unit tests in the
testorwww/testdirectories with.spec.tsxfile extension
Files:
test/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxtest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
test/**/*.spec.{ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Aim for 100% unit test code coverage when writing new code
Files:
test/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxtest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.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/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxtest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.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/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxtest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
test/component/**/*.spec.tsx
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Use React Testing Library for testing component interactions and behavior upon rendering
Files:
test/component/Tooltip/Tooltip.sync.spec.tsx
🧠 Learnings (5)
📚 Learning: 2025-11-23T13:30:10.388Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.388Z
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:
src/state/tooltipSlice.tssrc/state/selectors/tooltipSelectors.tstest/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxsrc/polar/Pie.tsxsrc/synchronisation/useChartSynchronisation.tsxsrc/state/selectors/combiners/combineTooltipInteractionState.tstest/component/Tooltip/Tooltip.sync.spec.tsx
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
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:
src/state/selectors/tooltipSelectors.tstest/polar/Pie.spec.tsxtest/chart/ScatterChart.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
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/polar/Pie.spec.tsxtest/state/selectors/pieSelectors.spec.tsxtest/component/Tooltip/Tooltip.sync.spec.tsx
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests
Applied to files:
test/state/selectors/pieSelectors.spec.tsx
📚 Learning: 2025-11-25T01:23:08.237Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-11-25T01:23:08.237Z
Learning: Applies to test/**/*.{test,spec}.{ts,tsx} : Use the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion
Applied to files:
test/component/Tooltip/Tooltip.sync.spec.tsx
🧬 Code graph analysis (2)
src/state/selectors/tooltipSelectors.ts (3)
src/state/store.ts (1)
RechartsRootState(95-95)src/state/selectors/selectors.ts (1)
selectTooltipInteractionState(76-84)src/state/tooltipSlice.ts (1)
TooltipInteractionState(118-162)
src/polar/Pie.tsx (4)
src/util/types.ts (1)
DataKey(60-60)src/state/graphicalItemsSlice.ts (1)
GraphicalItemId(19-19)src/state/selectors/tooltipSelectors.ts (2)
selectActiveTooltipDataKey(405-414)selectActiveTooltipCoordinate(446-455)src/util/useUniqueId.ts (1)
WithoutId(37-37)
⏰ 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 (10)
src/synchronisation/useChartSynchronisation.tsx (1)
95-107: Graphical item id propagation in tooltip sync is consistent
graphicalItemIdis correctly reset when clearing sync state, preserved from incoming sync payloads, and intentionally omitted (set toundefined) from locally-originated sync actions. This keeps the sync protocol stable while allowing the richer local state.Also applies to: 120-128, 236-244
src/state/tooltipSlice.ts (1)
118-162: Tooltip interaction now cleanly supports filtering by graphical item idThe new
graphicalItemIdfield is threaded through the type, initial state, and the hover/click item reducers, and is explicitly set in bothnoInteractionandsyncInteraction. All object literals that construct aTooltipInteractionStateinclude this field, so the shape stays consistent and there are no dangling undefined properties beyond what’s intended.Also applies to: 180-186, 239-257, 268-273, 307-315, 330-337
test/chart/ScatterChart.spec.tsx (1)
1535-1591: Scatter tooltip state expectations correctly updated for graphicalItemIdThe tooltip state snapshots now include
graphicalItemId: undefinedfor all interaction buckets, matching the extendedTooltipInteractionState/TooltipStateshape. This keeps the tests aligned with the store without altering semantics.src/state/selectors/tooltipSelectors.ts (1)
416-425: New selector for active graphical item id mirrors existing dataKey selector
selectActiveTooltipGraphicalItemIdfollows the same pattern asselectActiveTooltipDataKey, guarding against an undefined interaction state and simply exposingtooltipInteraction.graphicalItemId. This is a minimal, correct extension of the selector surface.test/polar/Pie.spec.tsx (2)
334-412: Pie sector tests now validate dataKey propagation across shapes and eventsThe expectations for
activeShapeprops, label render props, and click/hover/touch event payloads now include the sectordataKey('y','value','uv'as appropriate). This gives good coverage that the new field is consistently present in all Pie-sector-facing APIs.Also applies to: 505-552, 1872-1918, 1949-1995, 2003-2052, 2079-2128
789-807: Pie tooltip state snapshots updated for graphicalItemIdThe tooltip
itemInteractionexpectations now includegraphicalItemId: undefinedfor both click and hover, including defaultIndex and touch-driven cases. This keeps Pie’s tooltip tests in sync with the widened tooltip state.Also applies to: 819-836, 1042-1078, 1438-1451
test/component/Tooltip/Tooltip.sync.spec.tsx (1)
487-495: Tooltip synchronization tests correctly account for graphicalItemId in syncInteractionEvery
setSyncInteractionpayload and synchronised tooltip state expectation now includesgraphicalItemId: undefined, aligning with the extendedTooltipSyncState. The synchronization semantics (active flags, coordinates, index, label, sourceViewBox) remain unchanged and are still thoroughly exercised.Also applies to: 531-539, 572-580, 799-899
src/state/selectors/combiners/combineTooltipInteractionState.ts (1)
62-69: Default-index tooltip interaction initializes graphicalItemId explicitlyIn the
defaultIndex != nullpath, the constructedTooltipInteractionStatenow includesgraphicalItemId: undefined, which keeps it structurally consistent with the updated type and other initial-state paths.test/state/selectors/pieSelectors.spec.tsx (1)
47-377: Pie selector tests now assert per-sector dataKey in PieSectorDataItem
expectedResultBefore,expectedResultAfter, and the custom-data test all include adataKeyon eachPieSectorDataItem, matching the Pie’sdataKey('uv','pv','value'). This confirms the selector exposes the new field correctly without changing any other expectations.Also applies to: 389-719, 751-848
src/polar/Pie.tsx (1)
37-41: Tooltip selectors wiring looks correct and consistentImporting and using
selectActiveTooltipIndex,selectActiveTooltipDataKey, andselectActiveTooltipCoordinatekeepsPieSectorsaligned with the canonical tooltip state instead of re-deriving it locally. All three selectors are exercised and participate inisActive, with no obvious typing or lifecycle issues in this file.Please double‑check that the return types and semantics of these selectors (especially
selectActiveTooltipIndexbeing string vs number, andselectActiveTooltipCoordinatenever beingnullin normal Pie interactions) are covered by existing tests around pie tooltips and active shapes.Also applies to: 531-534
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/polar/Pie.tsx(9 hunks)test/polar/Pie.spec.tsx(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/polar/Pie.spec.tsx
🧰 Additional context used
📓 Path-based instructions (4)
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run ESLint and Prettier on the codebase using
npm run lint
Files:
src/polar/Pie.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (DEVELOPING.md)
Run type checking on the codebase using
npm run check-types
**/*.{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:
src/polar/Pie.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Ensure code lints by running
npm run lintand follows Airbnb's Style Guide
Files:
src/polar/Pie.tsx
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/polar/Pie.tsx
🧠 Learnings (1)
📚 Learning: 2025-11-23T13:30:10.388Z
Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.388Z
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:
src/polar/Pie.tsx
⏰ 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 (5)
src/polar/Pie.tsx (5)
37-41: LGTM: Necessary imports for graphical item tracking.The new selectors enable proper multi-pie coordination by accessing activeDataKey, activeGraphicalItemId, and activeIndex separately from the tooltip state.
322-322: LGTM: Essential addition for graphical item tracking.Adding
idto PieSectorsProps enables proper identification of which Pie component is being interacted with, addressing the cross-highlighting issue.
667-667: LGTM: Necessary data propagation for multi-pie coordination.Adding
dataKeyto the sector data enables the matching logic in PieSectors to correctly identify which sectors belong to which Pie when multiple Pies share the same dataKey.
723-727: LGTM: Proper ID propagation through component hierarchy.The
idparameter is correctly threaded throughSectorsWithAnimation→PieSectors→ event handlers, enabling each Pie component to maintain its own active state independently.Also applies to: 814-814, 882-882
529-561: Based on the web search results, I now have enough context to rewrite the review comment. The search results clarify:
- activeIndex type: It can be
number | string(TooltipIndex union) depending on version, so theString(i)conversion is justified- Coordinate-based matching: The 0.01-pixel tolerance is a known pattern for Recharts tooltips, but may not be necessary when using graphicalItemId matching
- Implementation approach: The graphicalItemId-based matching is a valid approach for multi-pie scenarios
Graphical item matching logic correctly implements the required fix for multi-pie cross-highlighting.
The implementation properly addresses the cross-highlighting issue by checking if the active graphical item ID matches this Pie's ID. The null checks provide backward compatibility when
graphicalItemIdis undefined.The
String(i) === activeIndexcomparison is appropriate becauseactiveIndexcan be a string or number (TooltipIndex union type in Recharts) depending on the redux state, making the string conversion necessary and intentional.However, verify whether coordinate-based matching (0.01-pixel tolerance mentioned in the PR description) should also be implemented. The current implementation uses graphicalItemId matching alone, which is a simpler and sufficient approach for multi-pie scenarios. Coordinate-based matching would only be needed if you want fallback matching when graphicalItemIds are unavailable.
No changes required if graphicalItemId matching meets the requirements.
|
|
||
| export type PieSectorData = GeometrySector & { | ||
| dataKey?: string; | ||
| dataKey?: DataKey<any>; |
There was a problem hiding this comment.
Type uses any in violation of coding guidelines.
The coding guidelines state: "Never use any type (implicit or explicit) in TypeScript code. Prefer unknown over any."
Consider whether DataKey<unknown> would be more appropriate here:
- dataKey?: DataKey<any>;
+ dataKey?: DataKey<unknown>;This would need verification across the codebase to ensure type compatibility.
As per coding guidelines, any types should be avoided.
📝 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.
| dataKey?: DataKey<any>; | |
| dataKey?: DataKey<unknown>; |
🤖 Prompt for AI Agents
In src/polar/Pie.tsx around line 135, the prop type declared as DataKey<any>
violates the no-any rule; change it to DataKey<unknown> and update any affected
usages: adjust function signatures and callers to accept DataKey<unknown>, add
explicit type guards or narrow/cast values where the concrete type is required,
and run TypeScript compilation and tests to find and fix remaining
incompatibilities across the codebase.
Description
Fixed issue where multiple Pie components with the same
dataKeywould incorrectly highlight together when hovering over one of them. The fix implements graphical item ID tracking in the tooltip state infrastructure, preparing for component-specific matching to ensure each Pie chart maintains independent active states.Related Issue
Fixes #6259
Motivation and Context
When rendering multiple Pie charts (e.g., concentric pies) with the same
dataKeyprop, hovering over a sector in one Pie would incorrectly activate the corresponding sector in all other Pies with the same dataKey. This happened because the active sector selection logic only matched by index and dataKey, without considering which specific graphical item (Pie component) the interaction occurred on.This change adds the infrastructure required to allow proper independent interaction with multiple Pie charts in the same visualization, which is a common use case for two-level or nested pie charts.
How Has This Been Tested?
graphicalItemIdfield in tooltip state expectationsPie.spec.tsx- Comprehensive Pie component tests including tooltip interactionsTooltip.sync.spec.tsx- Tooltip synchronization testsTooltip.visibility.spec.tsx- Tooltip visibility state testsuseChartSynchronisation.spec.tsx- Chart synchronization hook testsTesting approach:
graphicalItemIdfieldgraphicalItemId: undefinedScreenshots (if appropriate):
Live demo available at: https://stackblitz.com/edit/react-lnlglkfj-29bjqzhg?file=src%2Findex.js
Types of changes
Checklist:
Technical Implementation Details:
dataKeyfield toPieSectorDataItemtype (non-breaking - defaults to'value')PieSectors.tsxcomparingactiveCoordinatewithsector.tooltipPositiondataKeyin sector expectationsSummary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.