test/cartesian/Bar.zeroDimensionFiltering.spec.tsx (1)

66-69: Consider improving type annotation.

The inline type annotation could be replaced with proper typing from the selector's return type for better type safety and maintainability.

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6659
File: www/src/components/GuideView/Performance/index.tsx:232-250
Timestamp: 2025-11-19T14:08:01.728Z
Learning: In Recharts version 3.4.2, object-as-prop optimizations were introduced to reduce unnecessary re-renders when new object references are passed as props. This affects the recommendation for the `react-perf/jsx-no-new-object-as-prop` ESLint rule.

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

Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:06.809Z
Learning: In tests (files under any test directory, e.g., test/, __tests__/, etc.), allow importing internal module paths (e.g., ../../src/...) and do not require imports to use the public API entry point (src/index.ts). The public API import restriction should apply only to non-test code. During reviews, accept internal imports in test files and enforce the public API pattern only for non-test code.

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} : Mock `getBoundingClientRect` in tests using the helper function provided in `test/helper/MockGetBoundingClientRect.ts`

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} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests

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} : Use `vi.useFakeTimers()` in all tests due to Redux autoBatchEnhancer dependency on timers and `requestAnimationFrame`

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} : Call `vi.runOnlyPendingTimers()` to advance timers after renders when not using `createSelectorTestCase` helper, and avoid `vi.runAllTimers()` to prevent infinite loops

src/cartesian/Bar.tsx (1)

1183-1183: Nit: isInBarStack ? true : undefined can be simplified.

Since isInBarStack is already a boolean (from the hook's return type), and computeBarRectangles defaults it to false, you can pass the value directly:

-            isInBarStack={isInBarStack ? true : undefined}
+            isInBarStack={isInBarStack || undefined}

Both are equivalent, but the shorter form is more idiomatic. If the intent is specifically to avoid storing false in Redux state, a brief comment would clarify that.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cartesian/Bar.tsx` at line 1183, Replace the verbose ternary prop
assignment isInBarStack={isInBarStack ? true : undefined} with the direct
boolean prop isInBarStack={isInBarStack} in Bar.tsx; update the call site that
uses the hook-returned isInBarStack (and note computeBarRectangles defaults it
to false) — if the original intent was to avoid storing false in Redux, add a
one-line comment near the prop or hook call explaining that behavior instead of
the ternary.

src/cartesian/BarStack.tsx (1)

69-70: Add JSDoc for useIsInBarStack consistent with sibling hooks.

Other exported hooks in this file (e.g., useStackId at Line 51) have JSDoc comments. Adding one here keeps the file consistent and supports the auto-generated API docs.

Suggested doc

+/**
+ * Returns true when the component is rendered inside a BarStack context, false otherwise.
+ */
 export const useIsInBarStack = (): boolean => useContext(BarStackContext) != null;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cartesian/BarStack.tsx` around lines 69 - 70, Add a JSDoc comment for the
exported hook useIsInBarStack to match the style of sibling hooks (e.g.,
useStackId) in this file: include a short one-line description stating it
returns whether the current component is inside a BarStack, an `@returns` tag
describing the boolean result, and any relevant tags used by other hooks in this
module so the auto-generated API docs remain consistent with the rest of the
file.

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

10-10: Missing JSDoc for isInBarStack.

Per the project's coding guidelines, TypeScript definitions in source files should be documented as they feed the auto-generated API docs. A brief JSDoc comment clarifying the purpose and default behavior would help.

Suggested doc

+    /**
+     * Whether this bar is rendered inside a BarStack context.
+     * When true, zero-dimension rectangles are preserved for clip-path calculations.
+     * `@default` false
+     */
     isInBarStack?: boolean;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/state/types/BarSettings.ts` at line 10, Add a JSDoc comment for the
isInBarStack property on the BarSettings interface explaining that it is a
boolean flag indicating whether the item is part of the "bar stack" and
documenting the default behavior (e.g., when omitted it is false), so the
auto-generated API docs convey its purpose and default. Ensure the JSDoc is
placed immediately above the isInBarStack declaration and references the
property name exactly.

src/cartesian/Bar.tsx (1)

1183-1184: isInBarStack || undefined is non-idiomatic; prefer isInBarStack ? true : undefined.

false || undefined evaluates to undefined, so the expression works, but it reads as a boolean OR rather than a conditional assignment. The inline comment clarifies intent but the ternary form makes it self-evident:

-            // false fallback is intentional, otherwise we need to update 10 test files with the new prop
-            isInBarStack={isInBarStack || undefined}
+            isInBarStack={isInBarStack ? true : undefined}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cartesian/Bar.tsx` around lines 1183 - 1184, Replace the non-idiomatic
fallback expression used when passing the isInBarStack prop (currently written
as isInBarStack || undefined) with an explicit conditional so it reads as
intended; update the prop assignment in Bar (the isInBarStack prop passed in the
Bar component) to use isInBarStack ? true : undefined to make the
boolean-to-undefined conversion explicit and self-documenting.

test/state/selectors/barStackSelectors.spec.tsx (1)

219-221: Consider using expectLastCalledWith for the length/index assertions where possible.

Lines 219–221 access spy.mock.lastCall?.[0] directly. The cross-comparison on line 230 legitimately requires a local reference, but the individual property assertions (toHaveLength, toBeUndefined, objectContaining) could use expectLastCalledWith for type safety. As per coding guidelines, prefer expectLastCalledWith over direct mock access.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@test/state/selectors/barStackSelectors.spec.tsx` around lines 219 - 221,
Replace direct access to spy.mock.lastCall?.[0] for individual property
assertions by using Jest's expectLastCalledWith on the relevant spies: use
expect(stackSpy).lastCalledWith(...) / expect(firstBarSpy).lastCalledWith(...) /
expect(secondBarSpy).lastCalledWith(...) (or
expect(spy).toHaveBeenLastCalledWith) to assert lengths and properties (the
assertions currently using toHaveLength, toBeUndefined, and objectContaining)
for better type-safety; retain the local stackRects variable (from
stackSpy.mock.lastCall?.[0]) only where you need to cross-compare arrays between
spies (the cross-comparison logic must keep the local reference), but convert
the individual assertions against firstBarSpy and secondBarSpy to
expect(...).toHaveBeenLastCalledWith(...) patterns.

src/cartesian/Bar.tsx (1)

723-757: BarStackClipLayer clip-path index is correct; animation frame interpolation has a related gap.

Line 726 correctly uses entry.originalDataIndex for clip-path lookup — the core of the index-stability fix. LGTM.

However, RectanglesWithAnimation at line ~817 uses prevData[index] where index is the current filtered-array position, not originalDataIndex. If the filtered set changes between renders (e.g., a bar transitions from non-zero to zero), prevData[0] may correspond to a logically different entry, causing the bar to animate from a wrong previous position.

♻️ Suggested fix for animation interpolation in `RectanglesWithAnimation`

Build a lookup map from originalDataIndex → prev rect before the animation step, so each entry interpolates from its own previous frame regardless of filtering changes:

+  const prevDataMap = React.useMemo(
+    () => new Map(prevData?.map(r => [r.originalDataIndex, r])),
+    [prevData],
+  );

  const stepData =
    t === 1
      ? data
      : data?.map((entry: BarRectangleItem, index: number): BarRectangleItem => {
-         const prev = prevData && prevData[index];
+         const prev = prevDataMap?.get(entry.originalDataIndex);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/cartesian/Bar.tsx` around lines 723 - 757, The animation uses
prevData[index] which is the filtered-array position and can mismatch entries
when filtering changes; in RectanglesWithAnimation, build a lookup map keyed by
each rectangle's originalDataIndex (e.g., const prevMap = new Map(prevData.map(r
=> [r.originalDataIndex, r]))) before interpolation, and then replace uses of
prevData[index] with prevMap.get(entry.originalDataIndex) (falling back to a
sensible default if missing) so each entry interpolates from its true previous
frame; keep BarStackClipLayer usage as-is.