More undefined checks by PavelVanecek · Pull Request #6754 · recharts/recharts

PavelVanecek · 2025-12-09T12:41:23Z

Description

271 errors -> 165

Related Issue

#6645

Summary by CodeRabbit

Release Notes

Bug Fixes
- Enhanced chart rendering stability with improved handling of missing or incomplete data
- Strengthened touch event detection on mobile devices to prevent crashes
- Added defensive checks to gracefully handle edge cases without throwing errors
- Improved tooltip interaction reliability with better state management

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2025-12-09T12:41:35Z

Walkthrough

This PR adds extensive defensive null/undefined checks across Sankey rendering logic, broadens type signatures in selectors to accept optional parameters, tightens type guard constraints, and hardens event handling to safely tolerate missing or undefined data without throwing errors.

Changes

Cohort / File(s)	Change Summary
Sankey rendering hardening `src/chart/Sankey.tsx`	Added defensive null/undefined guards throughout rendering logic (value retrieval, tree construction, depth calculations, layout updates). Updated `buildLinkProps` to return `LinkProps \| undefined` and filter out undefined results in rendering pipeline.
Selector signature updates `src/state/selectors/axisSelectors.ts`, `src/state/selectors/tooltipSelectors.ts`	Broadened type constraints in `itemAxisPredicate` to accept base graphical item settings. Made `combineDotsDomain`, `combineAreasDomain`, and `combineLinesDomain` parameters optional with early returns. Updated selector return types for finer-grained typing.
Event handling defensive checks `src/state/touchEventsMiddleware.ts`	Added null checks for first touch object in both axis and item tooltip paths to safely handle empty touch arrays.
Type guard and utility refinements `src/state/types/StackedGraphicalItem.ts`, `src/util/getActiveCoordinate.ts`	Tightened `isStacked` generic constraint to `GraphicalItemSettings` and added property existence check. Widened `calculateActiveTickIndex` return type to `number \| undefined`.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Multiple public API signature changes across selectors and utilities require careful verification of downstream impact
Defensive guard conditions and optional chaining patterns need validation throughout rendering and selector logic
Type constraint and parameter broadening changes must be verified for correctness in domain aggregation helpers and type predicates
Touch event null checks require verification that they handle edge cases correctly without breaking existing behavior

Possibly related PRs

Use explicit types instead of inference #6710: Modifies exported selector signatures in axisSelectors.ts with type/parameter changes
[Omnidoc] Fix prop comments of Label, LabelList, Text; extend generation #6749: Modifies calculateActiveTickIndex return type (conflicts with this PR's changes)
Turn console warnings into failed tests #6585: Adds guards to touch event handling in touchEventsMiddleware.ts to avoid dereferencing empty touches

Suggested labels

typescript

Suggested reviewers

ckifer

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings, 1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The PR description is largely incomplete; it only provides a metrics summary and issue link but omits most required template sections including Motivation, Testing, and Change Type.	Complete the PR description by filling in Motivation and Context (why these checks are needed), How Has This Been Tested, and the Types of changes checklist.
Docstring Coverage	⚠️ Warning	Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Title check	❓ Inconclusive	The title 'More undefined checks' is vague and uses generic phrasing that doesn't convey the specific purpose or scope of the changes.	Provide a more descriptive title that clarifies the primary objective, such as 'Add defensive null/undefined checks for robustness' or 'Improve type safety with optional parameter handling'.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch ts-strict

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/chart/Sankey.tsx (1)
782-801: Potential index mismatch after filtering undefined links.

In AllSankeyLinkElements, you iterate over links array (line 784) and access modifiedLinks[i] (line 785). However, since modifiedLinks is filtered using .filter(isNotNil) (line 998), its indices no longer align with the original links array.

For example, if buildLinkProps returns undefined for the link at index 2, modifiedLinks will have fewer elements than links, causing modifiedLinks[i] to reference the wrong link or be undefined for valid links at higher indices.

Consider either:

Keep undefined entries in modifiedLinks without filtering (the null check at line 786-788 already handles this), or

Iterate over modifiedLinks directly instead of links:
-      {links.map((link: SankeyLink, i: number) => {
-        const linkProps = modifiedLinks[i];
-        if (linkProps == null) {
-          return null;
-        }
+      {modifiedLinks.map((linkProps: LinkProps) => {
+        const link = linkProps.payload;
         return (
           <SankeyLinkElement
-            key={`link-${link.source}-${link.target}-${link.value}`}
+            key={`link-${linkProps.index}`}
             props={linkProps}
             linkContent={linkContent}
-            i={i}
+            i={linkProps.index}
             onMouseEnter={onMouseEnter}
             onMouseLeave={onMouseLeave}
             onClick={onClick}
             dataKey={dataKey}
           />
         );
       })}

🧹 Nitpick comments (1)

src/state/selectors/axisSelectors.ts (1)

58-62: itemAxisPredicate typing/doc now cover more than just cartesian items

Broadening the predicate parameter to BaseCartesianGraphicalItemSettings | BasePolarGraphicalItemSettings makes sense for reusing this helper across both cartesian and polar items; the runtime checks using 'xAxisId' in item, 'angleAxisId' in item, etc. are safe.

Two small follow‑ups you might consider:

The JSDoc above still says “Filters CartesianGraphicalItemSettings…”, which is no longer accurate given the new Base* imports. It would be clearer to update it to “graphical item settings (cartesian or polar)” or similar.

selectAxisPredicate is annotated as returning (item: CartesianGraphicalItemSettings) => boolean but is implemented via itemAxisPredicate, which returns a predicate over the union of base cartesian + base polar settings. This works today, but the exposed type suggests cartesian‑only. Either widening that callback type to the same union or introducing a shared alias (e.g. AxisRelevantGraphicalItemSettings) would keep the types and implementation aligned and avoid future confusion.

Also applies to: 293-317

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 670ebbe and b8224be.

📒 Files selected for processing (6)

src/chart/Sankey.tsx (21 hunks)
src/state/selectors/axisSelectors.ts (5 hunks)
src/state/selectors/tooltipSelectors.ts (7 hunks)
src/state/touchEventsMiddleware.ts (2 hunks)
src/state/types/StackedGraphicalItem.ts (1 hunks)
src/util/getActiveCoordinate.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{ts,tsx}: Never use any type (implicit or explicit) in TypeScript code
Prefer unknown over any and 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 use as type assertions in TypeScript; the only exception is as const

Files:

src/state/touchEventsMiddleware.ts
src/state/types/StackedGraphicalItem.ts
src/state/selectors/axisSelectors.ts
src/util/getActiveCoordinate.ts
src/chart/Sankey.tsx
src/state/selectors/tooltipSelectors.ts

**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Ensure code lints by running npm run lint and follows Airbnb's Style Guide

Files:

src/state/touchEventsMiddleware.ts
src/state/types/StackedGraphicalItem.ts
src/state/selectors/axisSelectors.ts
src/util/getActiveCoordinate.ts
src/chart/Sankey.tsx
src/state/selectors/tooltipSelectors.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/state/touchEventsMiddleware.ts
src/state/types/StackedGraphicalItem.ts
src/state/selectors/axisSelectors.ts
src/util/getActiveCoordinate.ts
src/chart/Sankey.tsx
src/state/selectors/tooltipSelectors.ts

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (DEVELOPING.md)

All imports from recharts must use the public API entry point; imports from internal paths like recharts/types/* or recharts/src/* are not allowed

Files:

src/state/touchEventsMiddleware.ts
src/state/types/StackedGraphicalItem.ts
src/state/selectors/axisSelectors.ts
src/util/getActiveCoordinate.ts
src/chart/Sankey.tsx
src/state/selectors/tooltipSelectors.ts

🧠 Learnings (4)

📚 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:

src/state/touchEventsMiddleware.ts
src/state/selectors/tooltipSelectors.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} : 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/touchEventsMiddleware.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} : Verify the number of selector calls using the spy object from `createSelectorTestCase` to spot unnecessary re-renders and improve performance

Applied to files:

src/state/selectors/tooltipSelectors.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:

src/state/selectors/tooltipSelectors.ts

🧬 Code graph analysis (5)

src/state/touchEventsMiddleware.ts (2)

src/state/selectors/selectActivePropsFromChartPointer.ts (1)

selectActivePropsFromChartPointer (14-29)

src/util/getChartPointer.ts (1)

getChartPointer (16-34)

src/state/types/StackedGraphicalItem.ts (1)

src/state/graphicalItemsSlice.ts (1)

GraphicalItemSettings (21-41)

src/state/selectors/axisSelectors.ts (4)

src/state/graphicalItemsSlice.ts (2)

BaseCartesianGraphicalItemSettings (43-57)

BasePolarGraphicalItemSettings (61-64)

src/state/referenceElementsSlice.ts (3)

ReferenceDotSettings (13-17)

ReferenceAreaSettings (19-24)

ReferenceLineSettings (26-30)

src/state/selectors/selectTooltipAxisType.ts (1)

RenderableAxisType (12-12)

src/util/types.ts (1)

NumberDomain (708-708)

src/chart/Sankey.tsx (2)

src/util/types.ts (1)

SankeyNode (957-971)

src/util/DataUtils.ts (1)

isNotNil (202-204)

src/state/selectors/tooltipSelectors.ts (6)

src/state/graphicalItemsSlice.ts (3)

CartesianGraphicalItemSettings (59-59)

PolarGraphicalItemSettings (66-66)

GraphicalItemSettings (21-41)

src/state/selectors/selectTooltipAxisType.ts (1)

selectTooltipAxisType (14-30)

src/state/selectors/axisSelectors.ts (2)

itemAxisPredicate (299-317)

selectTooltipAxis (595-599)

src/state/selectors/arrayEqualityCheck.ts (1)

emptyArraysAreEqualCheck (7-13)

src/state/tooltipSlice.ts (1)

TooltipInteractionState (118-162)

src/util/types.ts (2)

DataKey (60-60)

Coordinate (88-91)

⏰ 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 (14)

src/chart/Sankey.tsx (4)

40-40: Defensive null check for entry value.

The updated getValue handles undefined entries safely by returning 0 as a fallback.

355-380: Null-safe sorting comparators.

Returning 0 when encountering null values preserves the relative order of elements with missing data. This is a reasonable defensive approach for handling potentially sparse data.

52-78: Consistent null guards in weighted sum calculations.

The added null checks in getSumWithWeightedSource and getSumWithWeightedTarget prevent crashes when links or nodes are missing, returning the accumulated result safely.

678-684: buildLinkProps now returns undefined for invalid node references.

The change correctly guards against missing source or target nodes, aligning with the updated return type LinkProps | undefined.

src/state/touchEventsMiddleware.ts (2)

29-39: Defensive null check for touch event.

While line 23 already verifies touchEvent.touches.length !== 0, the explicit null check at lines 30-32 provides additional type safety for TypeScript's strict mode. Using a local touch variable also improves readability.

51-54: Combined null guards for item tooltip path.

The combined check for document.elementFromPoint == null || touch == null correctly handles both edge cases before attempting to use the touch coordinates.

src/state/types/StackedGraphicalItem.ts (1)

27-31: Broadened type guard to accept base GraphicalItemSettings.

The updated type guard correctly:

Accepts any GraphicalItemSettings (not just MaybeStackedGraphicalItem)

Uses 'stackId' in graphicalItem to verify the property exists before checking its value

Narrows to T & DefinitelyStackedGraphicalItem which ensures both stackId and dataKey are non-null

This is a sound type narrowing pattern that aligns with the broader type flexibility introduced in selectors.

src/state/selectors/tooltipSelectors.ts (5)

52-56: Explicit type imports for graphical item settings.

The updated imports provide clearer type definitions for the selectors that work with graphical items.

98-119: Explicit return types for graphical item selectors.

The explicit return type annotations for selectAllUnfilteredGraphicalItems and selectAllGraphicalItemsSettings improve code clarity and ensure type consistency across the selector chain.

324-324: Parameter type accepts undefined for realScaleType.

This aligns with the upstream selector selectTooltipAxisRealScaleType which returns string | undefined. The function body at line 342 safely handles the undefined case with a comparison check.

415-432: Undefined-safe tooltip interaction state handling.

Both selectActiveTooltipDataKey and selectActiveTooltipGraphicalItemId now explicitly accept TooltipInteractionState | undefined and guard against undefined before accessing properties.

456-471: Consistent undefined handling in tooltip coordinate and active state selectors.

selectActiveTooltipCoordinate uses optional chaining (tooltipInteractionState?.coordinate)

selectIsTooltipActive uses nullish coalescing (?? false) for a clean boolean default

Both patterns are idiomatic TypeScript for handling potentially undefined values.

src/util/getActiveCoordinate.ts (1)

113-113: Return type broadening to number | undefined is correct.

The change is appropriate since the function uses optional chaining (unsortedTicks[i]?.index) at lines 157 and 164, which can return undefined. Callers in src/state/selectors/selectors.ts (lines 217 and 248) properly handle the undefined return value by passing it to getActiveCartesianCoordinate and getActivePolarCoordinate, both of which accept number | undefined as their activeIndex parameter.

src/state/selectors/axisSelectors.ts (1)

936-948: Undefined‑tolerant reference domains look correct

The added | undefined on dots/areas/lines and the early if (… == null) return undefined; guards in combineDotsDomain, combineAreasDomain, and combineLinesDomain safely handle optional inputs without changing behavior for existing callers that pass arrays (empty or not). The downstream logic still correctly returns undefined when there are no numeric coordinates.

No issues from a correctness or typings perspective; this is a solid defensive improvement.

Also applies to: 952-966, 992-1006

codecov · 2025-12-09T12:52:26Z

Codecov Report

❌ Patch coverage is 61.87500% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.79%. Comparing base (670ebbe) to head (b8224be).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
src/chart/Sankey.tsx	51.81%	53 Missing ⚠️
src/state/selectors/axisSelectors.ts	53.84%	6 Missing ⚠️
src/state/touchEventsMiddleware.ts	71.42%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6754      +/-   ##
==========================================
- Coverage   93.91%   93.79%   -0.13%     
==========================================
  Files         514      514              
  Lines       42833    42947     +114     
  Branches     4991     5020      +29     
==========================================
+ Hits        40227    40280      +53     
- Misses       2600     2661      +61     
  Partials        6        6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-12-09T13:11:23Z

Staging Deployment Details

Website: https://recharts.github.io/recharts/pr-6754/www/
Test Report: https://recharts.github.io/recharts/pr-6754/vr-tests/

These deployments will remain available for 30 days.

To update snapshots: Comment /update-snapshots on this PR to automatically update the baseline screenshots.

codecov · 2025-12-10T02:37:14Z

Bundle Report

Changes will increase total bundle size by 7.5kB (0.28%) ⬆️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
recharts/bundle-cjs	1.16MB	3.26kB (0.28%) ⬆️
recharts/bundle-es6	1.01MB	3.25kB (0.32%) ⬆️
recharts/bundle-umd	521.0kB	994 bytes (0.19%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`state/selectors/axisSelectors.js`	146 bytes	54.08kB	0.27%
`chart/Sankey.js`	2.76kB	28.49kB	10.73% ⚠️
`state/selectors/tooltipSelectors.js`	228 bytes	13.54kB	1.71%
`state/touchEventsMiddleware.js`	81 bytes	2.6kB	3.22%
`state/types/StackedGraphicalItem.js`	30 bytes	454 bytes	7.08% ⚠️

view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`state/selectors/axisSelectors.js`	146 bytes	63.93kB	0.23%
`chart/Sankey.js`	2.77kB	30.18kB	10.11% ⚠️
`state/selectors/tooltipSelectors.js`	228 bytes	16.95kB	1.36%
`state/touchEventsMiddleware.js`	81 bytes	2.85kB	2.92%
`state/types/StackedGraphicalItem.js`	30 bytes	558 bytes	5.68% ⚠️

view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`Recharts.js`	994 bytes	521.0kB	0.19%

PavelVanecek added 2 commits December 9, 2025 20:34

271 errors -> 182

64cfd87

182 errors -> 165

b8224be

coderabbitai bot reviewed Dec 9, 2025

View reviewed changes

ckifer approved these changes Dec 9, 2025

View reviewed changes

ckifer merged commit 6b473f2 into main Dec 9, 2025
36 of 39 checks passed

ckifer deleted the ts-strict branch December 9, 2025 16:45

This was referenced Dec 27, 2025

More undefined checks, Polygon tests #6830

Merged

Strict tsconfig #6842

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

More undefined checks#6754

More undefined checks#6754
ckifer merged 2 commits intomainfrom
ts-strict

PavelVanecek commented Dec 9, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

codecov bot commented Dec 9, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Dec 9, 2025

Uh oh!

Uh oh!

codecov bot commented Dec 10, 2025

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

PavelVanecek commented Dec 9, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Dec 9, 2025

Uh oh!

Uh oh!

codecov bot commented Dec 10, 2025

Bundle Report

Affected Assets, Files, and Routes:

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PavelVanecek commented Dec 9, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Dec 9, 2025 •

edited

Loading

codecov bot commented Dec 9, 2025 •

edited

Loading