src/cartesian/GraphicalItemClipPath.tsx (1)

16-16: Consider adding a JSDoc comment for the forceClip prop.

Since GraphicalItemClipPath is part of the public type surface (listed in typesFiles.txt), the new forceClip prop would benefit from a brief JSDoc description to support auto-generated API docs.

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'."

src/animation/pointMatching.ts (3)

10-15: Clarify JSDoc for 'x' strategy — it matches by payload key, not geometric coordinate.

The JSDoc says 'x' matches by coordinate, but tracing through getPointKey, when strategy is 'x', it falls through to the DataKey branch and looks up payload.x via getValueByDataKey — not the point's geometric x property. While these often coincide, the distinction matters when they diverge (e.g., derived/computed coordinates). Consider clarifying the JSDoc:

-/** 'index' preserves legacy behavior; 'x' matches by coordinate; a DataKey or function matches by identity. */
+/** 'index' preserves legacy behavior; 'x' matches by the 'x' key in the data payload; a DataKey or function matches by identity. */

---

262-322: interpolateMatchedPoints — solid interpolation logic with a few observations.

The function handles three cases well: matched points (interpolated), new points (entry from edge), and removed points (slide out). The early-return at t === 1 is a good optimization.

Two minor notes:

1. Line 251: avgSpacing defaults to 100 when there's only one point — consider documenting or extracting this magic number.
2. Line 302: new points don't interpolate y — they appear at their final y-position immediately. This is intentional for horizontal line charts but may look odd for vertical layouts. Worth a comment explaining the design choice.

---

50-68: buildPreviousPointsMap — first-occurrence-wins on duplicate keys.

Line 61's if (!map.has(key)) means only the first occurrence of a duplicate key is kept. This is a valid design choice but might surprise users with non-unique keys. Consider adding a brief JSDoc note about this behavior.

src/cartesian/ReferenceArea.tsx (2)

197-199: lerp helper is duplicated between ReferenceArea.tsx and ReferenceLine.tsx.

Both files define identical lerp functions. Consider extracting this to a shared utility (e.g., src/util/DataUtils.ts where interpolate already lives, or a new animation utility).

---

261-287: Interrupted animations will jump to the previous target position.

When a new rect arrives while an animation is in progress, previousRectRef.current has already been set to the target of the in-flight animation (line 267), not its current interpolated position. This means a mid-animation data change will cause a visual discontinuity — the new animation starts from where the old one was heading, not from where it currently is.

Compare with Line.tsx's approach where previousPointsRef.current is updated inside the animation callback (line 706) with the interpolated stepData, enabling smooth interruption.

This is likely acceptable for ReferenceArea (which changes less frequently), but worth noting if smooth interruption is desired.

src/cartesian/ReferenceLine.tsx (3)

5-5: Same useId import concern as ReferenceArea.tsx.

Consider using the custom useId from src/util/useId.ts for backward compatibility with React < 18.

---

286-290: lerp is duplicated (also in ReferenceArea.tsx) with a slightly different signature.

This version accepts number | null | undefined while ReferenceArea's accepts number | undefined. Consider extracting a single lerp utility with the broader signature.

---

130-138: Animation props use inline union type instead of AnimationTiming.

Same as the ReferenceArea comment — animationEasing is typed as 'ease' | 'ease-in' | 'ease-out' | 'ease-in-out' | 'linear' rather than reusing AnimationTiming from src/util/types.ts. This creates a maintenance risk if the timing options are extended.

Proposed fix

+ import { AnimationTiming } from '../util/types';

  // In ReferenceLineProps:
- animationEasing?: 'ease' | 'ease-in' | 'ease-out' | 'ease-in-out' | 'linear';
+ animationEasing?: AnimationTiming;

Apply the same change in ReferenceArea.tsx.

Also applies to: 412-415

src/cartesian/GraphicalItemClipPath.tsx (1)

16-16: Consider adding a JSDoc comment for the forceClip prop.

The inline comment on line 41 explains the intent well, but a brief JSDoc on the prop itself would help discoverability — especially since the coding guidelines note that JSDoc is used for auto-generated documentation. That said, this is an internal component, so it's a nice-to-have.

src/cartesian/ReferenceLine.tsx (3)

296-300: Duplicate lerp helper — also defined in ReferenceArea.tsx.

Both ReferenceLine.tsx and ReferenceArea.tsx define their own lerp function with slightly different signatures (number | null | undefined here vs number | undefined in ReferenceArea). Consider extracting this into a shared utility (e.g., alongside the animation helpers in src/animation/) to reduce duplication and keep the signatures consistent.

---

383-389: Ref mutation during render — works but worth noting.

previousEndPointsRef.current is read and written during the render phase (lines 384 and 389). While this is a common pattern for "track previous value" and works correctly today, React's documentation advises against side effects during render. If strict mode double-rendering or future concurrent features cause issues, consider moving the ref update into a useEffect. For now this is fine in practice.

---

392-398: Nullish coalescing fallbacks are redundant with resolveDefaultProps.

animationBegin, animationDuration, and animationEasing are destructured from PropsWithDefaults, which already guarantees values via referenceLineDefaultProps. The ?? 0 / ?? 300 / ?? 'ease' fallbacks are dead code.

Not harmful, but they obscure the fact that defaults are handled upstream.

Proposed cleanup

     <JavascriptAnimate
       animationId={`${animationId}-${JSON.stringify(currentEndPoints)}`}
-      begin={animationBegin ?? 0}
-      duration={animationDuration ?? 300}
+      begin={animationBegin}
+      duration={animationDuration}
       isActive
-      easing={animationEasing ?? 'ease'}
+      easing={animationEasing}
     >

src/cartesian/ReferenceArea.tsx (2)

207-209: Duplicate lerp — same function exists in ReferenceLine.tsx.

This is the same helper noted in ReferenceLine.tsx (with a slightly narrower type: number | undefined vs number | null | undefined). Worth extracting to a shared location to keep the signatures consistent and avoid duplication.

---

279-297: Redundant nullish coalescing and ref mutation during render — same patterns as ReferenceLine.

Same observations as in ReferenceLine.tsx:

• animationBegin ?? 0, animationDuration ?? 300, animationEasing ?? 'ease' are redundant since resolveDefaultProps already guarantees values.
• previousRectRef.current is written during render (lines 272, 277), which works in practice but is technically a render-phase side effect.

Both are minor nits, not blockers.

src/animation/pointMatching.ts (3)

267-273: from parameter typed as unknown is unnecessarily broad.

Every call site passes number | null (e.g., previous.x, previous.y, removed.x). Narrowing the signature to (from: number | null, to: number | null, t: number) => number | null would catch misuse at compile time and align with the coding guideline to type parameters explicitly.

Proposed fix

 export function interpolateMatchedPoints<T extends AnimatablePoint>(
   matchedPoints: ReadonlyArray<MatchedPoint<T>>,
   removedPoints: ReadonlyArray<T>,
   t: number,
-  interpolateFn: (from: unknown, to: number | null, t: number) => number | null,
+  interpolateFn: (from: number | null, to: number | null, t: number) => number | null,
   animateNewValues: boolean,
 ): T[] {

As per coding guidelines, type function parameters and return values explicitly and prefer unknown only when truly needed.

---

249-257: Magic default avgSpacing = 100 when only one point exists.

When there's a single point in the current data, avgSpacing defaults to 100. This arbitrary value directly controls how far off-screen new/removed points start their entry animation. A very different chart width could produce jarring jumps or barely-visible motion. Consider deriving a fallback from the actual chart dimensions or documenting why 100 is acceptable.

---

313-327: Removed points stay in place when avgShift is zero.

If existing points only change their y-coordinates (same x positions), avgShift will be 0, and removed points interpolate x from removed.x to removed.x - 0, i.e., they never move horizontally. They'll visually persist at their position until t = 1 when they're excluded from the output. This is likely fine for the common sliding-window use case but worth documenting.

src/cartesian/Line.tsx (1)

883-884: Duplicated needClip derivation logic between LineImpl and AreaImpl.

The expression needClipFromAxes || (animationClipOverflow && animationMatchBy !== 'index') is repeated identically in both Line.tsx (Line 884) and Area.tsx (Line 941). Consider extracting this into a shared helper or hook to keep the logic DRY and reduce the risk of divergence.

src/cartesian/Line.tsx (1)

883-884: Clipping is always forced for non-index strategies, even when animation is off.

needClip is true whenever animationClipOverflow && animationMatchBy !== 'index', regardless of isAnimationActive. This means a permanent clip path is applied even if the user has disabled animation. Consider gating on isAnimationActive as well:

- const needClip = needClipFromAxes || (animationClipOverflow && animationMatchBy !== 'index');
+ const needClip = needClipFromAxes || (animationClipOverflow && animationMatchBy !== 'index' && isAnimationActive !== false);

src/cartesian/ReferenceLine.tsx (2)

296-300: lerp silently coerces null/undefined to 0.

The ?? 0 fallback means if either endpoint coordinate is null or undefined, interpolation silently treats it as 0. For reference lines this is unlikely to be a problem since endpoints are calculated from scales, but it hides potential upstream bugs. Consider logging or returning NaN/null for invalid inputs.

---

393-398: animationBegin ?? 0 and animationDuration ?? 300 are redundant.

These props already have defaults via resolveDefaultProps at line 446, so they can never be undefined here. The ?? fallbacks are harmless but misleading.

-      begin={animationBegin ?? 0}
-      duration={animationDuration ?? 300}
+      begin={animationBegin}
+      duration={animationDuration}
       isActive
-      easing={animationEasing ?? 'ease'}
+      easing={animationEasing}

src/animation/pointMatching.ts (2)

10-15: 'x' in PointMatchingStrategy is indistinguishable from a DataKey at runtime.

The 'x' literal is listed as a special strategy in docs ("Use 'x' for time-series sliding windows"), but getPointKey has no special branch for it — it falls through to the DataKey path looking up payload.x. This works if the user's data objects have an x field, but may silently fail to match if they don't (e.g., {date: '...', value: 10}).

Consider either:

1. Adding a dedicated 'x' branch in getPointKey that uses point.x (the pixel coordinate) directly, or
2. Clarifying in the JSDoc that 'x' means "use x as a DataKey on the payload", not "match by x-coordinate."

---

224-261: avgSpacing defaults to 100 — magic number.

When there is only one point, avgSpacing falls back to 100 (Line 256). This arbitrary pixel value may produce unexpected entry animation distances depending on chart dimensions. Consider deriving a fallback from the chart width or documenting why 100 was chosen.

omnidoc/verifyExamples.spec.ts (1)

182-184: New exports correctly registered as needing examples.

These are added to the exportsThatNeedExamples list following the established pattern. Since a Storybook story (AnimationMatchBy.stories.tsx) already demonstrates xMatchLinearInterpolation, you may be able to remove it from this list if the ExampleReader picks up storybook stories as examples.

test/animation/pointMatching.spec.ts (1)

755-804: Consider adding tests for animateNewValues=false and new-point entry extrapolation.

interpolateMatchedPoints has branching logic for animateNewValues (true vs false) and entry position extrapolation (left of leftmost, right of rightmost, between matched). The current tests cover the happy path but don't exercise:

• animateNewValues=false (new points should be returned as-is without entry animation)
• Entry position extrapolation for new points appearing to the left/right of matched endpoints
• The avgShift fallback path (new point between matched endpoints)

These paths are critical for the animation correctness that this PR is solving.

As per coding guidelines: "Aim for 100% unit test code coverage when writing new code"

src/cartesian/Line.tsx (1)

887-888: Unnecessary intermediate variable from removed feature.

needClipFromAxes is immediately assigned to needClip without any transformation. This appears to be a leftover from the removed animationClipOverflow feature. Simplify:

Suggested fix

-  const { needClip: needClipFromAxes } = useNeedsClip(xAxisId, yAxisId);
-  const needClip = needClipFromAxes;
+  const { needClip } = useNeedsClip(xAxisId, yAxisId);

src/animation/pointMatching.ts (2)

24-49: getPointKey with strategy = 'x' silently falls through to the DataKey code path.

When the strategy is the string literal 'x', the function doesn't hit the 'index' guard (Line 29) or the typeof === 'function' guard (Line 33), so it proceeds to getValueByDataKey(point.payload, 'x'). This is intentional but not obvious — a brief comment at the // strategy is a DataKey line (37) noting that 'x' is included here by design would help maintainability.

---

250-262: Magic number 100 for avgSpacing fallback when there's a single point.

When all current points share the same x or there's only one point, avgSpacing defaults to 100 (Line 257). This arbitrary constant affects entry/exit animation positions for new and removed points. Consider extracting it as a named constant with a brief comment explaining the rationale.

Suggested change

+const DEFAULT_SPACING_FALLBACK = 100;
+
 function findMatchedEndpoints<T extends AnimatablePoint>(
   matchedPoints: ReadonlyArray<MatchedPoint<T>>,
 ): MatchedEndpoints<T> {
   ...
-  const avgSpacing = allCurrentX.length > 1 ? (lastX - firstX) / (allCurrentX.length - 1) : 100;
+  const avgSpacing = allCurrentX.length > 1 ? (lastX - firstX) / (allCurrentX.length - 1) : DEFAULT_SPACING_FALLBACK;

src/cartesian/Area.tsx (1)

916-984: animationStepFunction flows through everythingElse spread — correct but implicit.

AreaImpl destructures many props from resolveDefaultProps but animationStepFunction is not listed, so it lands in ...everythingElse and is spread onto <AreaWithState>. This works because InternalAreaProps declares the prop. However, it's less discoverable than the explicitly listed props.