Conversation
WalkthroughThe PR adds defensive null-check guards to touch event handling, introduces a new test utility to intercept console warnings and errors as test failures, integrates this utility into test setup, and updates multiple test files with async/await conversions, SVG wrapping, and formatting changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
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: 1
🧹 Nitpick comments (3)
test/polar/Pie.spec.tsx (1)
745-769: Use a stable sector selector instead of relying on layer indexQuerying container.querySelectorAll('.recharts-layer')[4] is brittle. Prefer targeting the sector element directly to reduce flakiness and future DOM reshuffle breakage. Also, the explicit 1000ms test timeout is likely unnecessary with fake timers and user-event.
Apply:
- const sector = container.querySelectorAll('.recharts-layer')[4]; + // Pick the first rendered sector path directly; less brittle than relying on layer index + const sector = container.querySelector('.recharts-pie-sector .recharts-sector')!;Optionally drop the custom timeout:
- }, 1000); + });src/state/touchEventsMiddleware.ts (1)
23-25: Good defensive guards against empty touches and missing APIsEarly returns avoid dereferencing undefined touches and calling a missing elementFromPoint in test/SSR environments. Consider a slightly stricter check:
- if (document.elementFromPoint == null) { + if (typeof document.elementFromPoint !== 'function') { return; }Otherwise, looks solid.
Also applies to: 48-50
test/helper/consoleWarningToError.spec.ts (1)
1-39: Solid coverage of ignore/throw pathsTests validate substring and RegExp ignores and throwing on warn/error. Consider adding a case for React-style formatted messages (e.g., 'Warning: %s', 'Foo') to exercise formatMessage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/state/touchEventsMiddleware.ts(2 hunks)test/cartesian/Bar.spec.tsx(1 hunks)test/cartesian/Brush.spec.tsx(3 hunks)test/cartesian/CartesianGrid.spec.tsx(10 hunks)test/helper/consoleWarningToError.spec.ts(1 hunks)test/helper/consoleWarningToError.ts(1 hunks)test/polar/Pie.spec.tsx(1 hunks)test/vitest.setup.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
test/**/vitest.setup.ts
📄 CodeRabbit inference engine (test/README.md)
Ensure vi.useFakeTimers() is configured in the Vitest setup file
Files:
test/vitest.setup.ts
src/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Do not hardcode any user-facing strings or formatting choices in the library code; leave internationalization to library users
Files:
src/state/touchEventsMiddleware.ts
🧠 Learnings (11)
📓 Common learnings
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Prefer using the createSelectorTestCase helper when writing or modifying tests
Applied to files:
test/helper/consoleWarningToError.tstest/vitest.setup.tstest/helper/consoleWarningToError.spec.tstest/cartesian/CartesianGrid.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Mock getBoundingClientRect using test/helper/MockGetBoundingClientRect.ts when rendering Recharts components (e.g., Tooltip, Legend, charts)
Applied to files:
test/cartesian/Bar.spec.tsxtest/vitest.setup.tstest/cartesian/Brush.spec.tsxtest/cartesian/CartesianGrid.spec.tsxtest/polar/Pie.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : When testing selectors, verify render counts using the spy and rerenderSameComponent from createSelectorTestCase
Applied to files:
test/cartesian/Bar.spec.tsxtest/vitest.setup.tstest/cartesian/Brush.spec.tsxtest/cartesian/CartesianGrid.spec.tsxtest/polar/Pie.spec.tsx
📚 Learning: 2025-10-25T07:36:02.229Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-10-25T07:36:02.229Z
Learning: Recharts aims for simple, declarative, and composable charts; prioritize consistency, usability, performance, and accessibility
Applied to files:
test/cartesian/Bar.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/vitest.setup.ts : Ensure vi.useFakeTimers() is configured in the Vitest setup file
Applied to files:
test/vitest.setup.tstest/helper/consoleWarningToError.spec.ts
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : When using user-event, initialize with userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers }) or use test/helper/userEventSetup.ts
Applied to files:
test/vitest.setup.tstest/polar/Pie.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Avoid vi.runAllTimers() in tests to prevent infinite loops caused by chained scheduled timers
Applied to files:
test/vitest.setup.tstest/helper/consoleWarningToError.spec.tstest/polar/Pie.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : In tests not using createSelectorTestCase, advance timers after renders with vi.runOnlyPendingTimers()
Applied to files:
test/vitest.setup.tstest/cartesian/Brush.spec.tsxtest/cartesian/CartesianGrid.spec.tsxtest/polar/Pie.spec.tsx
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Write unit tests using Vitest and React Testing Library
Applied to files:
test/vitest.setup.tstest/helper/consoleWarningToError.spec.ts
📚 Learning: 2025-10-25T07:35:46.188Z
Learnt from: CR
Repo: recharts/recharts PR: 0
File: test/README.md:0-0
Timestamp: 2025-10-25T07:35:46.188Z
Learning: Applies to test/**/*.test.{ts,tsx} : Use expectLastCalledWith instead of toHaveBeenLastCalledWith for typed last-call assertions
Applied to files:
test/cartesian/Brush.spec.tsxtest/polar/Pie.spec.tsx
🧬 Code graph analysis (5)
test/cartesian/Bar.spec.tsx (1)
test/helper/createSelectorTestCase.tsx (1)
rechartsTestRender(172-184)
test/vitest.setup.ts (1)
test/helper/consoleWarningToError.ts (1)
setupConsoleWarningToError(81-115)
test/helper/consoleWarningToError.spec.ts (1)
test/helper/consoleWarningToError.ts (3)
IGNORE_WARNINGS(21-31)restoreConsole(117-124)setupConsoleWarningToError(81-115)
test/cartesian/CartesianGrid.spec.tsx (1)
src/cartesian/CartesianGrid.tsx (1)
CartesianGrid(400-529)
test/polar/Pie.spec.tsx (1)
test/helper/userEventSetup.ts (1)
userEventSetup(13-17)
⏰ 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). (2)
- GitHub Check: Build, Test, Pack
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
test/cartesian/Bar.spec.tsx (1)
1971-1976: LGTM: render helper change is appropriateSwitching to rechartsTestRender is consistent with selector-based tests and keeps the z-index assertions intact. No issues.
test/cartesian/CartesianGrid.spec.tsx (1)
1682-1693: Wrapping grid-only renders in is the right moveThis makes the generated DOM consistent with SVG expectations and stabilizes selectors/attributes across cases. Looks good.
Also applies to: 1754-1764, 1772-1781, 1788-1798, 1805-1815, 1826-1835, 1861-1872, 1891-1899, 1919-1929, 1989-1999, 2006-2014, 2021-2030, 2037-2046, 2056-2063, 2071-2079, 2090-2099, 2122-2129
test/cartesian/Brush.spec.tsx (1)
96-102: Async test + awaited SVG snapshots are correctAwaiting toMatchFileSnapshot aligns with async matcher semantics and avoids race conditions. No further action.
Also applies to: 131-131, 150-150
test/vitest.setup.ts (1)
4-4: Remove vi.runAllTimers() calls; use vi.runOnlyPendingTimers() insteadThe verification sweep found 4 instances of
vi.runAllTimers()that violate coding guidelines and can cause infinite loops:
- test/animation/timeoutController.spec.ts: lines 23, 41, 57
- test/animation/configUpdate.spec.ts: line 369
Replace these with
vi.runOnlyPendingTimers()per test guidelines. Console interception setup in test/vitest.setup.ts is correctly configured.⛔ Skipped due to learnings
Learnt from: CR Repo: recharts/recharts PR: 0 File: test/README.md:0-0 Timestamp: 2025-10-25T07:35:46.188Z Learning: Applies to test/**/*.test.{ts,tsx} : Avoid vi.runAllTimers() in tests to prevent infinite loops caused by chained scheduled timersLearnt from: CR Repo: recharts/recharts PR: 0 File: test/README.md:0-0 Timestamp: 2025-10-25T07:35:46.188Z Learning: Applies to test/**/*.test.{ts,tsx} : In tests not using createSelectorTestCase, advance timers after renders with vi.runOnlyPendingTimers()Learnt from: CR Repo: recharts/recharts PR: 0 File: test/README.md:0-0 Timestamp: 2025-10-25T07:35:46.188Z Learning: Applies to test/**/vitest.setup.ts : Ensure vi.useFakeTimers() is configured in the Vitest setup fileLearnt from: CR Repo: recharts/recharts PR: 0 File: test/README.md:0-0 Timestamp: 2025-10-25T07:35:46.188Z Learning: Applies to test/**/*.test.{ts,tsx} : When using user-event, initialize with userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers }) or use test/helper/userEventSetup.tsLearnt from: CR Repo: recharts/recharts PR: 0 File: AGENTS.md:0-0 Timestamp: 2025-10-25T07:36:02.229Z Learning: Only run the full test suite (npm test) when verifying everything, as it can be slowLearnt from: CR Repo: recharts/recharts PR: 0 File: test/README.md:0-0 Timestamp: 2025-10-25T07:35:46.188Z Learning: Applies to test/**/*.test.{ts,tsx} : Use expectLastCalledWith instead of toHaveBeenLastCalledWith for typed last-call assertions
| * Quick Start: | ||
| * 1. Add warning patterns to WARNINGS_AS_ERRORS array below | ||
| * 2. Run tests - they will fail when those warnings occur | ||
| * 3. Fix the issues in source code | ||
| * 4. Tests pass - the warning pattern stays to prevent regressions | ||
| * | ||
| * See ./CONSOLE_WARNINGS.md for detailed documentation and examples. | ||
| */ |
There was a problem hiding this comment.
Doc/code mismatch: references WARNINGS_AS_ERRORS but only IGNORE_WARNINGS exists
Comments instruct adding patterns to “WARNINGS_AS_ERRORS”, but the implementation treats all non-ignored warns/errors as failures and only exposes IGNORE_WARNINGS. Update the header to match behavior to prevent confusion.
Apply:
- * Quick Start:
- * 1. Add warning patterns to WARNINGS_AS_ERRORS array below
- * 2. Run tests - they will fail when those warnings occur
- * 3. Fix the issues in source code
- * 4. Tests pass - the warning pattern stays to prevent regressions
+ * Quick Start:
+ * 1. By default, any console.warn/console.error will fail tests.
+ * 2. Temporarily add benign/known patterns to IGNORE_WARNINGS below to unblock work.
+ * 3. Run tests, fix surfaced warnings in source, and then remove patterns from IGNORE_WARNINGS.
+ * 4. Keep IGNORE_WARNINGS minimal to prevent regressions.📝 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.
| * Quick Start: | |
| * 1. Add warning patterns to WARNINGS_AS_ERRORS array below | |
| * 2. Run tests - they will fail when those warnings occur | |
| * 3. Fix the issues in source code | |
| * 4. Tests pass - the warning pattern stays to prevent regressions | |
| * | |
| * See ./CONSOLE_WARNINGS.md for detailed documentation and examples. | |
| */ | |
| * Quick Start: | |
| * 1. By default, any console.warn/console.error will fail tests. | |
| * 2. Temporarily add benign/known patterns to IGNORE_WARNINGS below to unblock work. | |
| * 3. Run tests, fix surfaced warnings in source, and then remove patterns from IGNORE_WARNINGS. | |
| * 4. Keep IGNORE_WARNINGS minimal to prevent regressions. | |
| * | |
| * See ./CONSOLE_WARNINGS.md for detailed documentation and examples. | |
| */ |
🤖 Prompt for AI Agents
In test/helper/consoleWarningToError.ts around lines 7 to 14, the header/docs
reference a non-existent WARNINGS_AS_ERRORS variable while the code actually
uses IGNORE_WARNINGS and treats any non-ignored warnings as failures; update the
header text to reference IGNORE_WARNINGS instead of WARNINGS_AS_ERRORS and
rewrite the quick-start steps to reflect the real behavior (add patterns to
IGNORE_WARNINGS to suppress known warnings; tests will fail on any warnings not
listed there), and ensure the README/db link remains correct.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6585 +/- ##
==========================================
- Coverage 94.56% 94.56% -0.01%
==========================================
Files 491 491
Lines 41031 41037 +6
Branches 4748 4752 +4
==========================================
+ Hits 38803 38808 +5
- Misses 2223 2224 +1
Partials 5 5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 421 bytes (0.02%) ⬆️. 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:
|
| fillOpacity="20%" | ||
| horizontal={horizontal} | ||
| />, | ||
| <svg> |
There was a problem hiding this comment.
Hmm these need a height and width to render anything
There was a problem hiding this comment.
that's recharts specialty, SVG works without: https://stackblitz.com/edit/mr6fcnxe?file=src%2FExample.tsx
Description
Test script that reads all console.warn and console.error calls and turns them info test failures so that we get a chance to catch them all. Includes ignore list so that we can fix them all one by one (there's 300+ total).
I put the largest ones on the ignore list, and fixed some of the smaller ones to keep the PR contained.
Motivation and Context
These console warnings are sometimes irrelevant (missing jsdom support for this and that method) and sometimes more relevant (duplicate React keys) so let's solve them all.
Summary by CodeRabbit
Bug Fixes
Tests