fix(Brush): keep controlled startIndex/endIndex on data updates by momomuchu · Pull Request #7530 · recharts/recharts

momomuchu · 2026-07-05T01:16:12Z

Description

Controlled startIndex/endIndex on <Brush> get silently overwritten when the chart data array changes. The sync effect that copies the props into Redux only depended on the index props themselves, so once chartDataSlice reset dataStartIndex/dataEndIndex on a data update, nothing re-applied the controlled values. Adding chartData to that effect's dependency array makes it re-assert the controlled indices any time the data changes, not just when the index props change.

This does not touch chartDataSlice.ts or the reducer's reset behavior at all, and does not affect uncontrolled Brush usage in any way (setDataStartEndIndexes is a no-op when both indices are undefined). It's unrelated to the referential-equality discussion in #7021 - that was about whether Brush should treat a same-length new array reference as "unchanged" for internally-managed selection state, which is a separate question from what a controlled component does with props it was explicitly given. This PR doesn't reopen that; it just makes controlled props actually win, the same way a controlled <input value> would.

Related Issue

Fixes #7462

Motivation and Context

Bug fix. Controlled Brush is a documented pattern and currently doesn't work reliably across data updates.

How Has This Been Tested?

Added a unit test in test/cartesian/Brush.spec.tsx that renders a controlled Brush, changes the data array to a new reference of the same length, and asserts the controlled indices are preserved. Ran the full unit suite locally, all green.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

My change requires a change to the documentation.
I have added tests to cover my changes.

Summary by CodeRabbit

Bug Fixes
- Brush start/end range controls now remain consistent when the chart updates its underlying data, avoiding unintended resets of the selected brush positions.
- Controlled brush indices are preserved across data re-renders, even when the chart receives a new data array instance.
Tests
- Added coverage to verify controlled Brush indices stay stable after parent chart re-renders with updated data identity.

When startIndex/endIndex are passed as props, a subsequent data update could still reset the Brush selection to the full range. chartDataSlice resets dataStartIndex/dataEndIndex whenever the new data does not match the previous end index, and nothing re-applied the controlled values afterwards since the syncing effect only depended on the index props. Add chartData as a dependency of that effect so a controlled selection is re-applied any time the data changes, not just when the index props themselves change. Fixes recharts#7462

coderabbitai · 2026-07-05T01:16:41Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f2b050c0-826e-4af1-933a-622622da5877

📥 Commits

Reviewing files that changed from the base of the PR and between 54b5680 and f99b6fc.

📒 Files selected for processing (1)

test/cartesian/Brush.spec.tsx

🚧 Files skipped from review as they are similar to previous changes (1)

test/cartesian/Brush.spec.tsx

Walkthrough

BrushInternal now re-runs its controlled index sync when chartData changes, and the test suite adds coverage for preserving startIndex/endIndex across a new data array identity.

Changes

Brush Controlled Indices Fix

Layer / File(s)	Summary
Re-apply controlled indices on data change `src/cartesian/Brush.tsx`	Adds `chartData` to the `useEffect` dependency array so `setDataStartEndIndexes` re-runs when chart data changes, with a comment explaining the re-sync behavior.
Tests for stable indices across data updates `test/cartesian/Brush.spec.tsx`	Imports `act`, adds a stateful wrapper that swaps in a cloned `data` array, and asserts `dataStartIndex`/`dataEndIndex` stay unchanged while `chartData` identity updates.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

Brush component displays all bars instead of selected range when using start/end indices with many bars #6258: Concerns Brush losing controlled startIndex/endIndex when data changes, which this PR addresses.
Brush component displays all bars instead of selected range when using start/end indices with many bars #6984: Also concerns preserving controlled Brush indices across data changes, directly related to this fix.

Possibly related PRs

recharts/recharts#7020: Touches brush/chart synchronization plumbing in a closely related area, with overlapping control-flow around chart state updates.

Suggested reviewers: PavelVanecek

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title is concise and clearly states the Brush controlled-index fix on data updates.
Description check	✅ Passed	The description covers the problem, related issue, motivation, testing, and change type sections well.
Linked Issues check	✅ Passed	The PR directly addresses `#7462` by preserving controlled Brush indices across chart data updates.
Out of Scope Changes check	✅ Passed	The changes stay focused on the Brush sync fix and its test, with no obvious unrelated additions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

test/cartesian/Brush.spec.tsx (1)
696-716: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider asserting selector call count to catch redundant dispatches.

Since this fix makes the effect re-fire on every chartData change, this is a good spot to also assert spy call counts, per the coding guideline: "Verify the number of selector calls using the spy object from createSelectorTestCase to spot unnecessary re-renders and improve performance." This would catch any regression where the added dependency causes extra redundant dispatches/re-renders.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/cartesian/Brush.spec.tsx` around lines 696 - 716, Add an assertion on
the selector invocation count in this Brush.spec.tsx test to guard against
redundant re-renders caused by the ControlledBrushWithChangingData update. Use
the existing spy from createSelectorTestCase around selectChartDataWithIndexes
and verify the expected number of calls before and after the button-triggered
data identity change, alongside the current dataStartIndex/dataEndIndex
assertions, so regressions from the new chartData dependency are caught.
Source: Coding guidelines

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/cartesian/Brush.spec.tsx`:
- Around line 707-716: The Brush test is reading `spy.mock.calls` before the
Redux update queued by `BrushInternal` has been flushed. After
`fireEvent.click(button)`, advance fake timers with `vi.runOnlyPendingTimers()`
before asserting, since this test bypasses `createSelectorTestCase`’s render
helpers and the `autoBatchEnhancer` dispatch may still be pending. Keep the
existing assertions on `chartData`, `dataStartIndex`, and `dataEndIndex` in
`Brush.spec.tsx` after the timer flush.

---

Nitpick comments:
In `@test/cartesian/Brush.spec.tsx`:
- Around line 696-716: Add an assertion on the selector invocation count in this
Brush.spec.tsx test to guard against redundant re-renders caused by the
ControlledBrushWithChangingData update. Use the existing spy from
createSelectorTestCase around selectChartDataWithIndexes and verify the expected
number of calls before and after the button-triggered data identity change,
alongside the current dataStartIndex/dataEndIndex assertions, so regressions
from the new chartData dependency are caught.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2a9758fc-3371-438a-8624-eb175ebacb17

📥 Commits

Reviewing files that changed from the base of the PR and between 39ef890 and 54b5680.

📒 Files selected for processing (2)

src/cartesian/Brush.tsx
test/cartesian/Brush.spec.tsx

codecov · 2026-07-05T01:46:56Z

Bundle Report

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

Detailed changes

Bundle name	Size	Change
recharts/bundle-cjs	1.45MB	353 bytes (0.02%) ⬆️
recharts/bundle-es6	1.27MB	353 bytes (0.03%) ⬆️
recharts/bundle-umd	586.08kB	2 bytes (0.0%) ⬆️
recharts/bundle-treeshaking-cartesian	725.64kB	353 bytes (0.05%) ⬆️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`cartesian/Brush.js`	353 bytes	31.63kB	1.13%

view changes for bundle: recharts/bundle-treeshaking-cartesian

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`bundle.js`	353 bytes	725.64kB	0.05%

view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`Recharts.js`	2 bytes	586.08kB	0.0%

view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`cartesian/Brush.js`	353 bytes	30.3kB	1.18%

codecov · 2026-07-05T01:52:47Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.17%. Comparing base (39ef890) to head (f99b6fc).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #7530   +/-   ##
=======================================
  Coverage   88.17%   88.17%           
=======================================
  Files         613      613           
  Lines       14246    14246           
  Branches     3580     3580           
=======================================
  Hits        12562    12562           
  Misses       1494     1494           
  Partials      190      190

☔ View full report in Codecov by Harness.
📢 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.

Redux's autoBatchEnhancer queues dispatches behind requestAnimationFrame, and this suite runs with fake timers globally. The controlled-index test read spy.mock.calls right after fireEvent.click without flushing that queue, so the assertion could observe a stale value depending on action ordering.

PavelVanecek · 2026-07-13T07:13:03Z

Sorry @momomuchu I merged #7542 and it appears that caused conflicts or your PR. May I please ask you to resolve

Edit: nevermind sorry it all looks good now

coderabbitai Bot reviewed Jul 5, 2026

View reviewed changes

Comment thread test/cartesian/Brush.spec.tsx

PavelVanecek approved these changes Jul 5, 2026

View reviewed changes

PavelVanecek merged commit f2c3ecb into recharts:main Jul 14, 2026
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(Brush): keep controlled startIndex/endIndex on data updates#7530

fix(Brush): keep controlled startIndex/endIndex on data updates#7530
PavelVanecek merged 2 commits into
recharts:mainfrom
momomuchu:fix/brush-controlled-index-reset-7462

momomuchu commented Jul 5, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

codecov Bot commented Jul 5, 2026 •

edited

Loading

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

codecov Bot commented Jul 5, 2026 •

edited

Loading

Uh oh!

PavelVanecek commented Jul 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

momomuchu commented Jul 5, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bundle Report

Affected Assets, Files, and Routes:

Assets Changed:

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

codecov Bot commented Jul 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PavelVanecek commented Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

momomuchu commented Jul 5, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 5, 2026 •

edited

Loading

codecov Bot commented Jul 5, 2026 •

edited

Loading

codecov Bot commented Jul 5, 2026 •

edited

Loading

PavelVanecek commented Jul 13, 2026 •

edited

Loading