Fix PolarAngleAxis event types, add tests for remaining components#7000
Fix PolarAngleAxis event types, add tests for remaining components#7000
Conversation
WalkthroughThe PR exports TickItem as a public type and refactors PolarAngleAxis event handlers to use TickItem and SVGTextElement for improved type safety. Test utilities (Customized wrapper) are removed from cartesian tests, and comprehensive type-checking tests are added across PolarAngleAxis, PolarRadiusAxis, and shape components to verify event handler compatibility. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Fix all issues with AI agents
In `@test/polar/PolarAngleAxis/PolarAngleAxis.spec.tsx`:
- Line 2956: Leftover debug() call in PolarAngleAxis.spec.tsx prints the DOM on
every test run; remove the debug() invocation (the standalone debug() statement)
from the test file (where PolarAngleAxis tests are defined) so tests no longer
log the DOM—simply delete the call and run the test suite to confirm no other
debug artifacts remain.
- Around line 2926-2999: The test "should fire event handlers when provided" is
missing vi.useFakeTimers(), so add vi.useFakeTimers() at the start of that it
block (before render) to ensure timers/raf are faked for the rendered
RadarChart/PolarAngleAxis interactions, and then restore timers with
vi.useRealTimers() at the end of the test (or in an afterEach) to avoid leaking
fake timers; locate the block by the it title "should fire event handlers when
provided" (and related symbols like userEventSetup, render, onClick,
PolarAngleAxis) and insert the calls accordingly.
🧹 Nitpick comments (2)
src/polar/PolarAngleAxis.tsx (1)
240-242: Add JSDoc comments for touch event handlers.The mouse event handlers (lines 208–239) all have JSDoc descriptions, but the three touch event handlers lack them. Since JSDoc in source files feeds auto-generated API documentation, these should be documented consistently.
📝 Proposed fix
+ /** + * The customized event handler of touchstart on the ticks of this axis + */ onTouchStart?: (data: TickItem, index: number, e: React.TouchEvent<SVGTextElement>) => void; + /** + * The customized event handler of touchmove on the ticks of this axis + */ onTouchMove?: (data: TickItem, index: number, e: React.TouchEvent<SVGTextElement>) => void; + /** + * The customized event handler of touchend on the ticks of this axis + */ onTouchEnd?: (data: TickItem, index: number, e: React.TouchEvent<SVGTextElement>) => void;As per coding guidelines, "JSDoc comments and TypeScript definitions in source files must be kept up-to-date as they are used to autogenerate API documentation via 'npm run omnidoc'".
test/polar/PolarAngleAxis/PolarAngleAxis.spec.tsx (1)
2973-2997: PreferexpectLastCalledWithhelper overexpect(spy).toHaveBeenLastCalledWith(...).Line 2969 correctly uses
expectLastCalledWith, but lines 2973, 2975, 2979, 2981, 2985, 2989, 2993, 2997 fall back toexpect(spy).toHaveBeenLastCalledWith(...). For consistency and better typing, use the helper throughout. As per coding guidelines: "Use theexpectLastCalledWithhelper function instead ofexpect(spy).toHaveBeenLastCalledWith(...)for better typing and autocompletion".♻️ Example fix for a few lines (apply same pattern to all)
- expect(onMouseEnter).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onMouseEnter, tickItem, 0, expect.any(Object)); - expect(onMouseOver).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onMouseOver, tickItem, 0, expect.any(Object)); - expect(onMouseLeave).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onMouseLeave, tickItem, 0, expect.any(Object)); - expect(onMouseOut).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onMouseOut, tickItem, 0, expect.any(Object)); - expect(onMouseMove).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onMouseMove, tickItem, 0, expect.any(Object)); - expect(onTouchStart).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onTouchStart, tickItem, 0, expect.any(Object)); - expect(onTouchMove).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onTouchMove, tickItem, 0, expect.any(Object)); - expect(onTouchEnd).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); + expectLastCalledWith(onTouchEnd, tickItem, 0, expect.any(Object));
| describe('events', () => { | ||
| it('should fire event handlers when provided', async () => { | ||
| const userEvent = userEventSetup(); | ||
| const onClick: Mock<(tickItem: TickItem, index: number, e: React.MouseEvent) => void> = vi.fn(); | ||
| const onMouseEnter: Mock<(tickItem: TickItem, index: number, e: React.MouseEvent) => void> = vi.fn(); | ||
| const onMouseLeave: Mock<(tickItem: TickItem, index: number, e: React.MouseEvent) => void> = vi.fn(); | ||
| const onMouseOver: Mock<(tickItem: TickItem, index: number, e: React.MouseEvent) => void> = vi.fn(); | ||
| const onMouseOut: Mock<(tickItem: TickItem, index: number, e: React.MouseEvent) => void> = vi.fn(); | ||
| const onMouseMove: Mock<(tickItem: TickItem, index: number, e: React.MouseEvent) => void> = vi.fn(); | ||
| const onTouchStart: Mock<(tickItem: TickItem, index: number, e: React.TouchEvent) => void> = vi.fn(); | ||
| const onTouchMove: Mock<(tickItem: TickItem, index: number, e: React.TouchEvent) => void> = vi.fn(); | ||
| const onTouchEnd: Mock<(tickItem: TickItem, index: number, e: React.TouchEvent) => void> = vi.fn(); | ||
|
|
||
| const { container, debug } = render( | ||
| <RadarChart width={100} height={100} data={[{ x: 1, y: 1 }]}> | ||
| <PolarRadiusAxis dataKey="x" /> | ||
| <PolarAngleAxis | ||
| dataKey="y" | ||
| onClick={onClick} | ||
| onMouseEnter={onMouseEnter} | ||
| onMouseLeave={onMouseLeave} | ||
| onMouseOver={onMouseOver} | ||
| onMouseOut={onMouseOut} | ||
| onMouseMove={onMouseMove} | ||
| onTouchStart={onTouchStart} | ||
| onTouchMove={onTouchMove} | ||
| onTouchEnd={onTouchEnd} | ||
| /> | ||
| </RadarChart>, | ||
| ); | ||
| debug(); | ||
| const axisLabel = container.querySelector('.recharts-polar-angle-axis-tick'); | ||
| assertNotNull(axisLabel); | ||
|
|
||
| await userEvent.click(axisLabel); | ||
| expect(onClick).toHaveBeenCalledTimes(1); | ||
|
|
||
| const tickItem: TickItem = { | ||
| coordinate: 90, | ||
| index: 0, | ||
| offset: 360, | ||
| value: 1, | ||
| }; | ||
| expectLastCalledWith(onClick, tickItem, 0, expect.any(Object)); | ||
|
|
||
| await userEvent.hover(axisLabel); | ||
| expect(onMouseEnter).toHaveBeenCalledTimes(1); | ||
| expect(onMouseEnter).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
| expect(onMouseOver).toHaveBeenCalledTimes(1); | ||
| expect(onMouseOver).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
|
|
||
| await userEvent.unhover(axisLabel); | ||
| expect(onMouseLeave).toHaveBeenCalledTimes(1); | ||
| expect(onMouseLeave).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
| expect(onMouseOut).toHaveBeenCalledTimes(1); | ||
| expect(onMouseOut).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
|
|
||
| await userEvent.pointer({ target: axisLabel, keys: '[MouseMove]' }); | ||
| expect(onMouseMove).toHaveBeenCalledTimes(1); | ||
| expect(onMouseMove).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
|
|
||
| fireEvent.touchStart(axisLabel); | ||
| expect(onTouchStart).toHaveBeenCalledTimes(1); | ||
| expect(onTouchStart).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
|
|
||
| fireEvent.touchMove(axisLabel); | ||
| expect(onTouchMove).toHaveBeenCalledTimes(1); | ||
| expect(onTouchMove).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
|
|
||
| fireEvent.touchEnd(axisLabel); | ||
| expect(onTouchEnd).toHaveBeenCalledTimes(1); | ||
| expect(onTouchEnd).toHaveBeenLastCalledWith(tickItem, 0, expect.any(Object)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Missing vi.useFakeTimers() in this test.
Per coding guidelines, all tests should use vi.useFakeTimers() due to Redux autoBatchEnhancer dependency on timers and requestAnimationFrame. This test uses direct render (not createSelectorTestCase), so timers aren't automatically managed. As per coding guidelines: "Use vi.useFakeTimers() in all tests due to Redux autoBatchEnhancer dependency on timers and requestAnimationFrame".
🤖 Prompt for AI Agents
In `@test/polar/PolarAngleAxis/PolarAngleAxis.spec.tsx` around lines 2926 - 2999,
The test "should fire event handlers when provided" is missing
vi.useFakeTimers(), so add vi.useFakeTimers() at the start of that it block
(before render) to ensure timers/raf are faked for the rendered
RadarChart/PolarAngleAxis interactions, and then restore timers with
vi.useRealTimers() at the end of the test (or in an afterEach) to avoid leaking
fake timers; locate the block by the it title "should fire event handlers when
provided" (and related symbols like userEventSetup, render, onClick,
PolarAngleAxis) and insert the calls accordingly.
| /> | ||
| </RadarChart>, | ||
| ); | ||
| debug(); |
There was a problem hiding this comment.
Remove debug() call — leftover debug artifact.
This will print the DOM tree to console on every test run.
🧹 Proposed fix
- debug();📝 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.
| debug(); |
🤖 Prompt for AI Agents
In `@test/polar/PolarAngleAxis/PolarAngleAxis.spec.tsx` at line 2956, Leftover
debug() call in PolarAngleAxis.spec.tsx prints the DOM on every test run; remove
the debug() invocation (the standalone debug() statement) from the test file
(where PolarAngleAxis tests are defined) so tests no longer log the DOM—simply
delete the call and run the test suite to confirm no other debug artifacts
remain.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7000 +/- ##
=======================================
Coverage 90.10% 90.10%
=======================================
Files 522 522
Lines 38848 38848
Branches 5347 5347
=======================================
Hits 35004 35004
Misses 3835 3835
Partials 9 9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 357 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
|
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Description
Followup to make sure all our component types are compatible with the new function we're adding in 3.8
Related Issue
#6645
Summary by CodeRabbit
New Features
TickItemas part of the public APIPolarAngleAxisEventstype with comprehensive event handlers for axis tick interactionsonTouchStart,onTouchMove,onTouchEndfor PolarAngleAxisTests