Conversation
There was a problem hiding this comment.
Pull Request Overview
Stricter nullability and tooltip refactor to split cartesian/polar paths and remove optional/undefined-heavy props.
- Introduces util/getActiveCoordinate.ts and refactors tooltip active coordinate and index calculations into separate cartesian and polar paths.
- Tightens types across grid/axis/tick utilities and selectors (undefined vs null, required vs optional), and updates tests accordingly.
- Simplifies getEveryNthWithCondition API by removing the predicate parameter and undefined return.
Reviewed Changes
Copilot reviewed 24 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| www/src/docs/exampleComponents/PieChart/index.tsx | Moves flexbox explanatory content out of the component into index wrapper. |
| www/src/docs/exampleComponents/PieChart/PieChartInFlexbox.tsx | Removes wrapper/paragraph to avoid duplication; component now only renders the flex container and chart. |
| test/util/ChartUtils.spec.tsx | Updates import path for calculateActiveTickIndex to new util/getActiveCoordinate. |
| test/cartesian/CartesianGrid.spec.tsx | Types tests’ generators as Vitest mocks and adjusts expectations for syncWithTicks (undefined → false). Adds expectLastCalledWith helper. |
| src/util/types.ts | Redefines RangeObj to be PolarViewBoxRequired plus required angle/radius, removing cartesian fields. |
| src/util/getEveryNthWithCondition.ts | Removes predicate and undefined return; always returns a stepped subset. |
| src/util/getActiveCoordinate.ts | New module: cartesian/polar active coordinate helpers, in-range check, and calculateActiveTickIndex. |
| src/util/YAxisUtils.tsx | Changes ticks arg type to null instead of undefined. |
| src/util/TickUtils.ts | getAngledTickWidth accepts angle: number |
| src/util/PolarUtils.ts | narrows exports; inRangeOfSector now accepts ChartPointer instead of raw coordinates. |
| src/util/ChartUtils.ts | Removes inRange/getActiveCoordinate/calculateActiveTickIndex; adds calculateCartesianTooltipPos/calculatePolarTooltipPos. |
| src/state/selectors/tooltipSelectors.ts | selectDomainOfAllAppliedNumericalValuesIncludingErrorValues can return undefined. |
| src/state/selectors/selectors.ts | Refactors active tooltip prop combination into separate cartesian/polar code paths; updates selector arg types to allow undefined. |
| src/state/selectors/combiners/combineTooltipPayload.ts | tooltipEventType argument is optional. |
| src/state/selectors/axisSelectors.ts | Returns undefined (not null) for missing axis props. |
| src/component/Tooltip.tsx | Safely handles possibly undefined selector results; finalIsActive defaults to false. |
| src/component/Label.tsx | labelRef types accept null. |
| src/cartesian/getTicks.ts | Loosens/optionalizes several GetTicksInput properties for strict null checks. |
| src/cartesian/CartesianGrid.tsx | Adds defaultCartesianGridProps, positive-number validation, undefined-safe generator inputs, and width/height fallbacks. |
| scripts/snapshots/*Files.txt | Adds getActiveCoordinate outputs to published bundles. |
| eslint.config.mjs | Lints: ignore docs index.tsx files to allow default exports. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| @@ -1,5 +1,5 @@ | |||
| import React from 'react'; | |||
| import { describe, test, it, expect, vi } from 'vitest'; | |||
| import { describe, test, it, expect, vi, Mock } from 'vitest'; | |||
There was a problem hiding this comment.
Mock is a type in Vitest and should be imported as a type-only import; importing it as a value can cause a runtime error ('does not provide an export named Mock'). Replace with a type-only import.
| import { describe, test, it, expect, vi, Mock } from 'vitest'; | |
| import { describe, test, it, expect, vi } from 'vitest'; | |
| import type { Mock } from 'vitest'; |
|
|
||
| it('should pass props to the generator', () => { | ||
| const horizontalCoordinatesGenerator: HorizontalCoordinatesGenerator = vi.fn().mockReturnValue([]); | ||
| const horizontalCoordinatesGenerator: Mock<HorizontalCoordinatesGenerator> = vi.fn().mockReturnValue([]); |
There was a problem hiding this comment.
Incorrect Mock typing: Vitest's Mock expects argument and return generics, not a function type. Use MockedFunction (or Mock<Parameters, ReturnType>) and import it as a type. Example: import type { MockedFunction } from 'vitest'; const horizontalCoordinatesGenerator: MockedFunction = vi.fn().mockReturnValue([]);
| export function getEveryNthWithCondition<Type>(array: ReadonlyArray<Type>, n: number): ReadonlyArray<Type> { | ||
| if (n < 1) { | ||
| return []; | ||
| } | ||
| if (n === 1 && isValid === undefined) { | ||
| if (n === 1) { | ||
| return array; | ||
| } | ||
| const result: Type[] = []; | ||
| for (let i = 0; i < array.length; i += n) { | ||
| if (isValid === undefined || isValid(array[i]) === true) { | ||
| result.push(array[i]); | ||
| } else { | ||
| return undefined; | ||
| } | ||
| result.push(array[i]); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
This change removes the isValid predicate parameter and the undefined return case, altering the exported function's signature and behavior. Since getEveryNthWithCondition is part of the published types (see snapshots), this is a breaking change for consumers. To preserve backwards compatibility, consider adding an overloaded signature that still accepts the optional predicate (ignoring it internally), or introduce a new function (e.g., getEveryNth) while keeping the old API deprecated but intact.
There was a problem hiding this comment.
This is exported for internal use but it's not a public API (not exported from index.ts). This parameter is never used internally.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6446 +/- ##
=======================================
Coverage 93.83% 93.83%
=======================================
Files 417 418 +1
Lines 38423 38488 +65
Branches 4502 4503 +1
=======================================
+ Hits 36053 36114 +61
- Misses 2353 2357 +4
Partials 17 17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 9.35kB (0.37%) ⬆️. 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:
|
Description
Split the inRange method & friends to two code paths, got rid of undefined and optional properties.
Related Issue
#5768
333 errors -> 294