Skip to content

refactor: extract common Dots component from Area, Line, and Radar#6508

Merged
PavelVanecek merged 1 commit intomainfrom
dot
Oct 27, 2025
Merged

refactor: extract common Dots component from Area, Line, and Radar#6508
PavelVanecek merged 1 commit intomainfrom
dot

Conversation

@PavelVanecek
Copy link
Collaborator

@PavelVanecek PavelVanecek commented Oct 26, 2025

Description

I noticed that we have three identical Dots implementations so I moved them all to a new file. It will help with the zIndex changes and it's easy to split in its own PR.

I also fixed some of the types while here.

Motivation and Context

DRY

How Has This Been Tested?

All tests passing

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactor

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • I have added a storybook story or VR test, or extended an existing story or VR test to show my changes

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced new public types DotItemDotProps and DotType for consistent dot customization across all chart components.
    • New unified dot rendering system provides a standardized interface for customizing data point visualization.
  • Refactor

    • Streamlined dot rendering logic across Area, Line, and Radar charts for improved consistency and maintainability.
    • Examples updated to reflect the new dot customization API.

- Created shared Dots.tsx component that consolidates dot rendering logic
- Converted renderDotItem function to DotItem React component
- Added wrapper components (AreaDotsWrapper, LineDotsWrapper, RadarDotsWrapper)
  to handle component-specific props
- Introduced DotPoint interface as common contract for all point types
- Reduced code duplication across three components
- Maintained backward compatibility and all existing functionality
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 26, 2025

Walkthrough

This PR introduces a centralized Dots component that unifies dot rendering across Area, Line, and Radar charts, replacing per-component custom dot rendering logic with a unified wrapper pattern. The change includes new public types (DotType, DotItemDotProps), broadened utility function signatures, and updated exports across the type system.

Changes

Cohort / File(s) Summary
Snapshot registrations
scripts/snapshots/es6Files.txt, scripts/snapshots/libFiles.txt, scripts/snapshots/typesFiles.txt
Added snapshot entries for new Dots component files across ES6, lib, and types directories.
New Dots component
src/component/Dots.tsx
Introduced new React component that renders a collection of dots for data points. Exports DotPoint interface, DotsDotProps type, and Dots component. Includes internal DotItem component and shouldRenderDots helper function for managing dot rendering with clipping support.
Cartesian charts refactoring
src/cartesian/Area.tsx, src/cartesian/Line.tsx
Replaced in-component custom dot rendering logic (renderDotItem, shouldRenderDots, manual dot creation) with AreaDotsWrapper and LineDotsWrapper that delegate to the Dots component. Updated dot prop type from ActiveDotType to DotType in both public and internal props. Adjusted imports to use Dots component.
Polar chart refactoring
src/polar/Radar.tsx
Replaced per-dot rendering logic with RadarDotsWrapper delegating to Dots component. Updated RadarProps.dot type from RadarDot union to DotType. Removed local RadarDot type alias and adjusted imports.
Component utilities
src/component/ActivePoints.tsx
Restructured dotProps construction: now uses intermediate dotPropsTyped (ActiveDotProps) merged with svgPropertiesNoEventsFromUnknown result for expanded SVG property support. Updated typing note in element cloning path.
Utility function signature updates
src/util/ReactUtils.ts, src/util/getRadiusAndStrokeWidthFromDot.tsx, src/util/svgPropertiesNoEvents.ts
Broadened function signatures: isClipDot and getRadiusAndStrokeWidthFromDot now accept ActiveDotType | DotType. Updated svgPropertiesNoEvents return type from SVGPropsNoEvents<T> | null to SVGPropsNoEvents<T> (non-null).
Type system extensions
src/util/types.ts, src/index.ts
Added new public types DotItemDotProps and DotType for dot rendering. Updated DOMAttributesWithProps to accept string | TrustedHTML for dangerouslySetInnerHTML.__html. Added DotType export to public API via src/index.ts.
Examples and tests
storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx, storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx, test/cartesian/Area.spec.tsx, test/polar/Radar.spec.tsx, www/src/docs/exampleComponents/LineChart/CustomizedDotLineChart.tsx
Updated renderDot function signatures to use DotItemDotProps instead of local inline types. Added imports of DotItemDotProps. Updated Radar test Point type to allow string or number coordinates.

Sequence Diagram(s)

sequenceDiagram
    participant Chart as Area/Line/Radar
    participant Wrapper as DotsWrapper
    participant Dots as Dots Component
    participant DotItem as DotItem

    Chart->>Wrapper: Render with dot config & points
    Wrapper->>Dots: Pass points, dot type, baseProps
    Dots->>Dots: Check shouldRenderDots()
    alt Should Render
        Dots->>Dots: Extract SVG properties from dot
        Dots->>Dots: Build clipPath if needed
        loop For each point
            Dots->>DotItem: Create DotItem with dotProps
            DotItem->>DotItem: Resolve dot (element/function/object)
            DotItem->>DotItem: Render Dot or custom element
        end
        Dots->>Dots: Render Layer with all DotItems
    else Should Not Render
        Dots->>Dots: Return null
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20–30 minutes

  • Multiple affected components across different chart types (Area, Line, Radar) with a consistent refactoring pattern (custom dot logic → Dots wrapper delegation)
  • New component introduction (Dots.tsx) with internal helpers and public exports that requires careful validation of logic and prop handling
  • Type system changes (DotType, DotItemDotProps union types) and signature broadening across utility functions need consistency verification
  • Snapshot registrations are straightforward additions
  • Example and test file updates follow straightforward type name changes

Areas requiring extra attention:

  • Logic density in new Dots component, particularly shouldRenderDots, clipping logic, and prop merging
  • Consistency of DotProps handling across Area/Line/Radar DotsWrapper implementations
  • Verification that type broadening in utilities (ActiveDotType | DotType) maintains backward compatibility
  • Ensure clipPath handling and SVG property extraction work correctly across all chart types

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 12.50% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The pull request title "refactor: extract common Dots component from Area, Line, and Radar" directly and specifically describes the main change in the changeset. The title is concise, avoids unnecessary noise, and clearly communicates that the PR consolidates duplicate Dots implementations from three components into a single extracted component. This aligns perfectly with the raw summary showing the creation of src/component/Dots.tsx and subsequent updates to Area.tsx, Line.tsx, and Radar.tsx to use this new component.
Description Check ✅ Passed The pull request description addresses most sections of the template with substantive content. The Description section explains the extraction of three identical Dots implementations and mentions type fixes; the Motivation section states "DRY"; the Testing section confirms "All tests passing"; and the Types of changes section identifies this as a Refactor. While the Motivation and Context and How Has This Been Tested sections are quite brief, they are present and meaningful. The Related Issue section is notably absent, but this may be acceptable for a refactoring effort not tied to a specific issue. Overall, the description is mostly complete despite some non-critical sections being minimal or missing.
✨ 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 dot

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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
src/index.ts (1)

42-42: Consider using generics instead of any in DotItemDotProps.

The new public export DotItemDotProps uses any in multiple locations (payload: any, value: any, DataKey<any>), which violates the coding guideline: "Never use the TypeScript any type (implicit or explicit)." Consider making DotItemDotProps generic to allow consumers to specify their data types more precisely:

As per coding guidelines

export type DotItemDotProps<TPayload = unknown, TValue = unknown> = SVGPropsNoEvents<Omit<DotProps, 'points' | 'ref'>> & {
  points: ReadonlyArray<DotPoint>;
  index: number;
  payload: TPayload;
  dataKey: DataKey<TPayload> | undefined;
  value: TValue;
};

This would provide better type safety for consumers while maintaining backward compatibility by defaulting to unknown.

src/component/Dots.tsx (1)

23-36: Minor: Unnecessary nullish coalescing.

Line 34 includes ?? {} but dotProps is a required parameter, so it will never be nullish. The fallback is harmless but unnecessary.

Consider simplifying:

-  const { points, ...props } = dotProps ?? {};
+  const { points, ...props } = dotProps;
src/cartesian/Area.tsx (1)

196-204: Add explicit return type to AreaDotsWrapper.

Per guidelines, specify return types for functions. This returns a JSX element.

Apply:

-function AreaDotsWrapper({
+function AreaDotsWrapper({
   clipPathId,
   points,
   props,
 }: {
   clipPathId: string;
   points: ReadonlyArray<AreaPointItem>;
   props: WithoutId<InternalProps>;
-}) {
+}): JSX.Element {
src/util/types.ts (2)

187-193: Unify dangerouslySetInnerHTML typing.

You updated DOMAttributesWithProps to accept string | TrustedHTML. Do the same in DOMAttributesAdaptChildEvent for consistency.

Apply:

 interface DOMAttributesWithProps<P, T> {
   children?: ReactNode;
   dangerouslySetInnerHTML?: {
-    __html: string | TrustedHTML;
+    __html: string | TrustedHTML;
   };
 }
@@
 export type DOMAttributesAdaptChildEvent<P, T> = {
   children?: ReactNode;
   dangerouslySetInnerHTML?: {
-    __html: string;
+    __html: string | TrustedHTML;
   };

Also applies to: 403-407


990-1000: Prefer unknown over any in new public types.

Replace any with unknown and tighten DataKey.

Apply:

-export type DotItemDotProps = SVGPropsNoEvents<Omit<DotProps, 'points' | 'ref'>> & {
+export type DotItemDotProps = SVGPropsNoEvents<Omit<DotProps, 'points' | 'ref'>> & {
   points: ReadonlyArray<DotPoint>;
   index: number;
-  payload: any;
-  dataKey: DataKey<any> | undefined;
-  value: any;
+  payload: unknown;
+  dataKey: DataKey<unknown> | undefined;
+  value: unknown;
 };

As per coding guidelines.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 05495b5 and f4a8435.

📒 Files selected for processing (18)
  • scripts/snapshots/es6Files.txt (1 hunks)
  • scripts/snapshots/libFiles.txt (1 hunks)
  • scripts/snapshots/typesFiles.txt (1 hunks)
  • src/cartesian/Area.tsx (8 hunks)
  • src/cartesian/Line.tsx (7 hunks)
  • src/component/ActivePoints.tsx (1 hunks)
  • src/component/Dots.tsx (1 hunks)
  • src/index.ts (1 hunks)
  • src/polar/Radar.tsx (6 hunks)
  • src/util/ReactUtils.ts (2 hunks)
  • src/util/getRadiusAndStrokeWidthFromDot.tsx (1 hunks)
  • src/util/svgPropertiesNoEvents.ts (1 hunks)
  • src/util/types.ts (4 hunks)
  • storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx (1 hunks)
  • storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx (1 hunks)
  • test/cartesian/Area.spec.tsx (2 hunks)
  • test/polar/Radar.spec.tsx (2 hunks)
  • www/src/docs/exampleComponents/LineChart/CustomizedDotLineChart.tsx (2 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{ts,tsx}: Ensure code lints cleanly before submitting PRs (npm run lint)
Never use the TypeScript any type (implicit or explicit)
Prefer unknown over any and refine types appropriately
Explicitly type all function parameters and return values; do not rely on implicit any or inference
Do not use as type assertions; the only exception is as const

Files:

  • src/util/svgPropertiesNoEvents.ts
  • src/index.ts
  • test/cartesian/Area.spec.tsx
  • src/util/getRadiusAndStrokeWidthFromDot.tsx
  • src/component/Dots.tsx
  • src/component/ActivePoints.tsx
  • www/src/docs/exampleComponents/LineChart/CustomizedDotLineChart.tsx
  • src/util/ReactUtils.ts
  • test/polar/Radar.spec.tsx
  • src/util/types.ts
  • storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx
  • src/cartesian/Area.tsx
  • storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx
  • src/cartesian/Line.tsx
  • src/polar/Radar.tsx
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/util/svgPropertiesNoEvents.ts
  • src/index.ts
  • src/util/getRadiusAndStrokeWidthFromDot.tsx
  • src/component/Dots.tsx
  • src/component/ActivePoints.tsx
  • src/util/ReactUtils.ts
  • src/util/types.ts
  • src/cartesian/Area.tsx
  • src/cartesian/Line.tsx
  • src/polar/Radar.tsx
{test,www/test}/**

📄 CodeRabbit inference engine (DEVELOPING.md)

Place unit tests in the test directory; some tests may also live in www/test

Files:

  • test/cartesian/Area.spec.tsx
  • test/polar/Radar.spec.tsx
test/**/*.spec.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

test/**/*.spec.{ts,tsx}: When writing new code, aim for 100% unit test code coverage
When adding or changing functionality, add appropriate tests (prefer unit tests; use RTL if rendering is involved)

Files:

  • test/cartesian/Area.spec.tsx
  • test/polar/Radar.spec.tsx
www/**

📄 CodeRabbit inference engine (DEVELOPING.md)

Use the www directory to add and commit examples for the documentation website (recharts.github.io)

Files:

  • www/src/docs/exampleComponents/LineChart/CustomizedDotLineChart.tsx
storybook/**

📄 CodeRabbit inference engine (DEVELOPING.md)

Use Storybook for high-fidelity examples intended for publication; prefer unit or VR tests for low-fidelity cases due to Chromatic limits

Files:

  • storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx
  • storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx
storybook/stories/**/*.stories.tsx

📄 CodeRabbit inference engine (CONTRIBUTING.md)

storybook/stories/**/*.stories.tsx: Use Storybook stories as smoke tests and add play-function tests with assertions when appropriate
If APIs change, ensure all Storybook stories still work as expected

Files:

  • storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx
  • storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx
🧬 Code graph analysis (13)
test/cartesian/Area.spec.tsx (2)
src/index.ts (1)
  • DotItemDotProps (42-42)
src/util/types.ts (1)
  • DotItemDotProps (994-1000)
src/util/getRadiusAndStrokeWidthFromDot.tsx (1)
src/util/types.ts (2)
  • ActiveDotType (964-988)
  • DotType (1008-1035)
src/component/Dots.tsx (3)
src/util/types.ts (3)
  • DotType (1008-1035)
  • DotItemDotProps (994-1000)
  • DataKey (58-58)
src/util/ReactUtils.ts (1)
  • isClipDot (107-112)
src/util/svgPropertiesAndEvents.ts (1)
  • svgPropertiesAndEventsFromUnknown (41-56)
src/component/ActivePoints.tsx (2)
src/util/types.ts (2)
  • ActiveDotProps (945-956)
  • adaptEventHandlers (838-865)
src/util/svgPropertiesNoEvents.ts (1)
  • svgPropertiesNoEventsFromUnknown (362-379)
www/src/docs/exampleComponents/LineChart/CustomizedDotLineChart.tsx (2)
src/index.ts (1)
  • DotItemDotProps (42-42)
src/util/types.ts (1)
  • DotItemDotProps (994-1000)
src/util/ReactUtils.ts (1)
src/util/types.ts (2)
  • ActiveDotType (964-988)
  • DotType (1008-1035)
test/polar/Radar.spec.tsx (1)
storybook/stories/Examples/cartesian/Bar/CustomizedShape.stories.tsx (1)
  • CustomizedShape (10-61)
src/util/types.ts (2)
src/util/svgPropertiesNoEvents.ts (1)
  • SVGPropsNoEvents (326-326)
src/component/Dots.tsx (1)
  • DotPoint (10-15)
storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx (2)
src/index.ts (1)
  • DotItemDotProps (42-42)
src/util/types.ts (1)
  • DotItemDotProps (994-1000)
src/cartesian/Area.tsx (3)
src/util/types.ts (1)
  • DotType (1008-1035)
src/component/Dots.tsx (2)
  • DotsDotProps (48-48)
  • Dots (86-121)
src/util/svgPropertiesNoEvents.ts (1)
  • svgPropertiesNoEvents (342-345)
storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx (2)
src/index.ts (1)
  • DotItemDotProps (42-42)
src/util/types.ts (1)
  • DotItemDotProps (994-1000)
src/cartesian/Line.tsx (2)
src/util/types.ts (1)
  • DotType (1008-1035)
src/component/Dots.tsx (1)
  • Dots (86-121)
src/polar/Radar.tsx (3)
src/util/types.ts (1)
  • DotType (1008-1035)
src/util/svgPropertiesNoEvents.ts (1)
  • svgPropertiesNoEvents (342-345)
src/component/Dots.tsx (1)
  • Dots (86-121)
⏰ 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 (24)
scripts/snapshots/es6Files.txt (1)

56-56: LGTM!

Expected snapshot update for the new Dots component.

scripts/snapshots/libFiles.txt (1)

56-56: LGTM!

Expected snapshot update for the new Dots component.

test/cartesian/Area.spec.tsx (1)

24-24: LGTM!

Good refactor replacing the ad-hoc inline type with the proper exported DotItemDotProps type. This improves type safety and consistency across the codebase.

Also applies to: 101-101

scripts/snapshots/typesFiles.txt (1)

56-56: LGTM!

Expected snapshot update for the new Dots component type declarations.

www/src/docs/exampleComponents/LineChart/CustomizedDotLineChart.tsx (1)

2-2: LGTM!

Documentation correctly updated to use the new DotItemDotProps type. The properties accessed in the component (cx, cy, value) are all available in the new type.

Also applies to: 51-51

test/polar/Radar.spec.tsx (1)

15-16: LGTM!

Excellent refactor replacing any types with a proper Point type. This improves type safety and aligns with the coding guideline to avoid using any.

Also applies to: 29-29

src/util/ReactUtils.ts (1)

7-7: LGTM! Type signature broadened correctly.

The function signature correctly accepts the union type ActiveDotType | DotType, aligning with the centralized dot rendering approach introduced in this PR.

Also applies to: 107-112

storybook/stories/Examples/cartesian/Line/LineWithCustomDot.stories.tsx (1)

5-5: LGTM! Story updated to use public DotItemDotProps type.

The type change aligns with the new centralized dot rendering API and makes the story consistent with the refactored implementation.

Also applies to: 13-13

storybook/stories/Examples/cartesian/Area/AreaWithCustomDot.stories.tsx (1)

5-5: LGTM! Consistent type usage across stories.

The change mirrors the Line story update and correctly adopts the public DotItemDotProps type.

Also applies to: 13-13

src/component/ActivePoints.tsx (2)

39-57: LGTM! Props construction clarified.

The two-step approach (creating typed props, then merging SVG properties) improves clarity and aligns with the centralized prop handling pattern.


62-62: Comment updated appropriately.

The updated comment more accurately describes the type limitation.

src/util/getRadiusAndStrokeWidthFromDot.tsx (1)

1-1: LGTM! Function signature broadened consistently.

The union type ActiveDotType | DotType aligns with the centralized dot type system and maintains backward compatibility.

Also applies to: 4-4

src/polar/Radar.tsx (3)

17-17: LGTM! Imports and type signature updated for centralized Dots.

The changes align Radar with the centralized dot rendering approach while updating the public API to use DotType.

Also applies to: 23-23, 41-41, 67-67


146-162: LGTM! RadarDotsWrapper correctly delegates to Dots.

The wrapper follows the established pattern:

  • Extracts id to prevent duplicate IDs on multiple dots
  • Derives baseProps via svgPropertiesNoEvents
  • Passes appropriate props to the centralized Dots component

Note that needClip and clipPathId are not passed, which is correct for polar charts.


337-337: LGTM! Dot rendering centralized.

The RadarDotsWrapper usage replaces inline dot rendering logic, achieving the DRY goal of this refactor.

src/cartesian/Line.tsx (3)

13-13: LGTM! Imports and type signatures updated consistently.

The changes align Line with the centralized dot rendering approach and update both internal and public API to use DotType.

Also applies to: 24-24, 51-51, 79-79, 118-118


220-251: LGTM! LineDotsWrapper correctly handles cartesian clipping.

The wrapper follows the same pattern as RadarDotsWrapper but includes needClip and clipPathId props, which are necessary for cartesian coordinate systems. The id is correctly excluded to prevent duplicate IDs.


322-322: LGTM! Dot rendering centralized.

The LineDotsWrapper usage replaces inline dot rendering logic, achieving the DRY goal stated in the PR objectives.

src/component/Dots.tsx (4)

10-15: LGTM! DotPoint interface well-defined.

The exported interface correctly represents a data point with optional nullability for x/y coordinates.


38-46: LGTM! shouldRenderDots logic handles edge cases.

The function correctly handles the special case of always rendering a single point (Line 45), which ensures visibility when there's only one data point.


48-48: LGTM! DotsDotProps correctly omits managed properties.

The type correctly excludes properties that the Dots component manages internally (cx, cy, key, index, dataKey, value, payload).


86-121: LGTM! Dots component centralizes dot rendering logic.

The component successfully consolidates the previously duplicated dot rendering logic from Area, Line, and Radar. The implementation correctly:

  • Handles early return for no-render cases
  • Determines clipping behavior
  • Merges base props with custom props
  • Builds appropriate clipPath URLs
  • Maps points to DotItem elements with proper props

This achieves the DRY goal stated in the PR objectives.

src/cartesian/Area.tsx (2)

66-107: No ecosystem updates needed — DotItemDotProps already in use throughout.

The type change to DotType is correctly implemented and the ecosystem is already aligned. DotItemDotProps is exported from the public API, and all current usages (tests, documentation, storybook stories, and internal components) already reference it correctly. No further action required.


209-218: All clipPath ID patterns verified and consistent.

The verification confirms the centralized Dots integration works correctly:

  • Main area clipping (line 279): url(#clipPath-${clipPathId})
  • Dots-specific clipping (line 629 definition, applied in Dots.tsx line 113): url(#clipPath-dots-${clipPathId}) when !clipDot, or url(#clipPath-${clipPathId}) when clipDot
  • Animation clipping (lines 551, 562): url(#animationClipPath-${clipPathId})

The Dots component (line 217) correctly receives and applies clipPathId and needClip parameters. Base props filtering prevents event leak. GraphicalItemClipPath handles axis-aligned clipping. DRY refactor consolidates dots rendering logic successfully.

Comment on lines +342 to 345
export function svgPropertiesNoEvents<T extends Record<PropertyKey, any>>(obj: T | boolean): SVGPropsNoEvents<T> {
const filteredEntries = Object.entries(obj).filter(([key]) => isSvgElementPropKey(key) || isDataAttribute(key));
return Object.fromEntries(filteredEntries) as SVGPropsNoEvents<T>;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

Verify if | boolean parameter type is necessary.

The return type change from SVGPropsNoEvents<T> | null to SVGPropsNoEvents<T> is correct since the function never actually returns null. However, the parameter type obj: T | boolean seems unusual for a function that filters SVG properties. If a boolean is passed, Object.entries(boolean) returns [], and the function returns an empty object {}.

Can you confirm whether the | boolean parameter type serves a specific purpose, or should it be removed to simplify the type signature?


🏁 Script executed:

#!/bin/bash
# Search for calls to svgPropertiesNoEvents with boolean arguments
rg -n -C3 'svgPropertiesNoEvents\s*\(\s*(true|false)\s*\)'

Length of output: 0


🏁 Script executed:

#!/bin/bash
# Search for all calls to svgPropertiesNoEvents (broader search)
echo "=== Calls to svgPropertiesNoEvents ==="
rg -n -C3 'svgPropertiesNoEvents\s*\(' --type ts --type tsx

echo ""
echo "=== Type references or interface definitions ==="
rg -n 'svgPropertiesNoEvents' --type ts --type tsx -B2 -A2

echo ""
echo "=== Function definition and exports ==="
fd -e ts -e tsx | xargs grep -l 'svgPropertiesNoEvents' | head -5

Length of output: 318


🏁 Script executed:

#!/bin/bash
# Search for all calls to svgPropertiesNoEvents without type restrictions
echo "=== All references to svgPropertiesNoEvents ==="
rg -n 'svgPropertiesNoEvents' -C2 | head -60

echo ""
echo "=== Check the function definition in svgPropertiesNoEvents.ts ==="
cat src/util/svgPropertiesNoEvents.ts | head -50

echo ""
echo "=== Look for export statement ==="
rg -n 'export.*svgPropertiesNoEvents' -C1

Length of output: 5963


Remove | boolean from parameter type; it contradicts the generic constraint and is never used.

The generic constraint T extends Record<PropertyKey, any> requires T to be a record type, yet the parameter allows T | boolean. All call sites in the codebase pass objects—including the wrapper function svgPropertiesNoEventsFromUnknown at lines 371 and 375. Passing a boolean would result in an empty object {} due to Object.entries(boolean) returning [], which is likely unintended behavior. The parameter type should be obj: T to align with the generic constraint.

🤖 Prompt for AI Agents
In src/util/svgPropertiesNoEvents.ts around lines 342 to 345, the function
signature currently allows obj: T | boolean which contradicts the generic
constraint T extends Record<PropertyKey, any> and permits unreachable/incorrect
boolean inputs; change the parameter type to obj: T only, update any related
JSDoc/types if present, and run typechecks to ensure callers (like
svgPropertiesNoEventsFromUnknown) still pass object values and no boolean
handling is needed.

Comment on lines +32 to 34
import { DotPoint } from '../component/Dots';
import { SVGPropsNoEvents } from './svgPropertiesNoEvents';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Avoid util → component import; move DotPoint into util/types.

types.ts should not depend on component/Dots.tsx. It risks cycles and complicates layering.

Apply in this file:

-import { DotPoint } from '../component/Dots';
+export interface DotPoint {
+  readonly x: number | null;
+  readonly y: number | null;
+  readonly value?: unknown;
+  readonly payload?: unknown;
+}

Then in src/component/Dots.tsx, replace:

- export interface DotPoint { ... }
+ import type { DotPoint } from '../util/types';
🤖 Prompt for AI Agents
In src/util/types.ts around lines 32 to 34, remove the import from
'../component/Dots' and add the DotPoint type definition into this file
(exported) so util no longer depends on the component layer; then update
src/component/Dots.tsx to stop exporting DotPoint and instead import it with a
type-only import from '../util/types'. Ensure the moved type matches the
original shape, export it as a named type, and update any local references and
imports in other files to point to util/types.

Comment on lines +1008 to +1035
export type DotType =
/**
* true | false will turn the default dot on and off, respectively
*/
| boolean
/**
* dot can be a custom React Component.
* It should return an SVG element because we are in SVG context - HTML won't work here.
* Unfortunately, if you write a regular old functional component and have it return SVG element,
* its default, inferred return type is `JSX.Element` and so if this return type was `SVGElement`
* then it would look like a type error (even when doing the right thing).
* So instead here we have ReactNode return type which is invalid in runtime
* (remember, we are in SVG context so HTML elements won't work, we need SVGElement).
* But better than forcing everyone to re-type their components I guess.
*
* Not that when a function, or a component is used, the props received are {@link DotItemDotProps}
* which contain some extra information compared to {@link DotProps}.
*/
| ((props: DotItemDotProps) => ReactNode)
/**
* dot can be an object; props from here will be appended to the default dot
*/
| Partial<DotProps>
/**
* dot can be an element; it will get cloned and will receive new extra props.
* I do not recommend this way! Use React component instead, that way you get more predictable props.
*/
| ReactElement<SVGProps<SVGElement>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

DotType addition: LGTM. Minor JSDoc nit.

Great consolidation of the dot API. Fix typo “Not that” → “Note that”.

Apply:

-   * Not that when a function, or a component is used, the props received are {@link DotItemDotProps}
+   * Note that when a function, or a component is used, the props received are {@link DotItemDotProps}
📝 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.

Suggested change
export type DotType =
/**
* true | false will turn the default dot on and off, respectively
*/
| boolean
/**
* dot can be a custom React Component.
* It should return an SVG element because we are in SVG context - HTML won't work here.
* Unfortunately, if you write a regular old functional component and have it return SVG element,
* its default, inferred return type is `JSX.Element` and so if this return type was `SVGElement`
* then it would look like a type error (even when doing the right thing).
* So instead here we have ReactNode return type which is invalid in runtime
* (remember, we are in SVG context so HTML elements won't work, we need SVGElement).
* But better than forcing everyone to re-type their components I guess.
*
* Not that when a function, or a component is used, the props received are {@link DotItemDotProps}
* which contain some extra information compared to {@link DotProps}.
*/
| ((props: DotItemDotProps) => ReactNode)
/**
* dot can be an object; props from here will be appended to the default dot
*/
| Partial<DotProps>
/**
* dot can be an element; it will get cloned and will receive new extra props.
* I do not recommend this way! Use React component instead, that way you get more predictable props.
*/
| ReactElement<SVGProps<SVGElement>>;
export type DotType =
/**
* true | false will turn the default dot on and off, respectively
*/
| boolean
/**
* dot can be a custom React Component.
* It should return an SVG element because we are in SVG context - HTML won't work here.
* Unfortunately, if you write a regular old functional component and have it return SVG element,
* its default, inferred return type is `JSX.Element` and so if this return type was `SVGElement`
* then it would look like a type error (even when doing the right thing).
* So instead here we have ReactNode return type which is invalid in runtime
* (remember, we are in SVG context so HTML elements won't work, we need SVGElement).
* But better than forcing everyone to re-type their components I guess.
*
* Note that when a function, or a component is used, the props received are {@link DotItemDotProps}
* which contain some extra information compared to {@link DotProps}.
*/
| ((props: DotItemDotProps) => ReactNode)
/**
* dot can be an object; props from here will be appended to the default dot
*/
| Partial<DotProps>
/**
* dot can be an element; it will get cloned and will receive new extra props.
* I do not recommend this way! Use React component instead, that way you get more predictable props.
*/
| ReactElement<SVGProps<SVGElement>>;
🤖 Prompt for AI Agents
In src/util/types.ts around lines 1008 to 1035, there's a JSDoc typo: change the
phrase "Not that when a function, or a component is used..." to "Note that when
a function, or a component is used..." — update that comment line only (preserve
rest of the paragraph and formatting).

@codecov
Copy link

codecov bot commented Oct 26, 2025

Codecov Report

❌ Patch coverage is 98.18182% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.49%. Comparing base (05495b5) to head (f4a8435).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/component/Dots.tsx 96.42% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6508      +/-   ##
==========================================
- Coverage   93.49%   93.49%   -0.01%     
==========================================
  Files         431      432       +1     
  Lines       39173    39132      -41     
  Branches     4554     4533      -21     
==========================================
- Hits        36625    36585      -40     
+ Misses       2531     2530       -1     
  Partials       17       17              

☔ 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.

@codecov
Copy link

codecov bot commented Oct 26, 2025

Bundle Report

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

Detailed changes
Bundle name Size Change
recharts/bundle-cjs 1.09MB 1.34kB (0.12%) ⬆️
recharts/bundle-es6 940.36kB 602 bytes (0.06%) ⬆️
recharts/bundle-umd 500.93kB 1.16kB (0.23%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name Size Change Total Size Change (%)
cartesian/Area.js -1.54kB 26.68kB -5.44%
cartesian/Line.js -1.55kB 24.15kB -6.04%
polar/Radar.js -933 bytes 16.8kB -5.26%
util/types.js 254 bytes 6.61kB 4.0%
component/Dots.js (New) 5.35kB 5.35kB 100.0% 🚀
component/ActivePoints.js -236 bytes 4.42kB -5.07%
view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name Size Change Total Size Change (%)
cartesian/Area.js -1.51kB 24.91kB -5.71%
cartesian/Line.js -1.52kB 22.68kB -6.29%
polar/Radar.js -922 bytes 15.41kB -5.65%
util/types.js 254 bytes 6.19kB 4.28%
component/Dots.js (New) 4.54kB 4.54kB 100.0% 🚀
component/ActivePoints.js -236 bytes 3.6kB -6.16%
view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name Size Change Total Size Change (%)
Recharts.js 1.16kB 500.93kB 0.23%

@PavelVanecek
Copy link
Collaborator Author

Hah I expected the bundle size to go down, not up.

@PavelVanecek PavelVanecek merged commit fa6085f into main Oct 27, 2025
27 of 29 checks passed
@PavelVanecek PavelVanecek deleted the dot branch October 27, 2025 00:20
@coderabbitai coderabbitai bot mentioned this pull request Oct 27, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants