fix(Brush): slide snaps back to original position on mouseup when start/endIndex are controlled#7542
Conversation
…lled (closes recharts#6279) Co-Authored-By: Claude Sonnet 5 <[email protected]>
WalkthroughModified BrushWithState.getDerivedStateFromProps in Brush.tsx to persist prevStartIndexControlledFromProps and prevEndIndexControlledFromProps when data changes. Added a test suite validating that traveller positions remain unchanged after mouseUp when startIndex/endIndex are controlled via props during drag. ChangesBrush controlled-index fix
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/cartesian/Brush.spec.tsx (1)
588-592: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCall
vi.runOnlyPendingTimers()afterrender.The test does not use
createSelectorTestCaseand doesn't advance timers after rendering. While the Brush component manages its own state synchronously viagetDerivedStateFromProps, the parentBarChartuses Redux withautoBatchEnhancerwhich batches state updates behind timers. Advancing timers after render ensures the chart is fully initialized before interaction.As per coding guidelines: "Call
vi.runOnlyPendingTimers()to advance timers after renders when not usingcreateSelectorTestCasehelper".const { container } = render( <BarChart width={400} height={100} data={data}> <Brush dataKey="value" x={100} y={50} width={400} height={40} startIndex={3} endIndex={6} /> </BarChart>, ); + vi.runOnlyPendingTimers();🤖 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 588 - 592, The Brush test setup renders BarChart without the createSelectorTestCase helper, so the Redux auto-batched initialization may still be pending after render. Update the Brush.spec.tsx test that renders BarChart with Brush to call vi.runOnlyPendingTimers() immediately after render, before any assertions or interactions, so the chart is fully initialized.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.
Nitpick comments:
In `@test/cartesian/Brush.spec.tsx`:
- Around line 588-592: The Brush test setup renders BarChart without the
createSelectorTestCase helper, so the Redux auto-batched initialization may
still be pending after render. Update the Brush.spec.tsx test that renders
BarChart with Brush to call vi.runOnlyPendingTimers() immediately after render,
before any assertions or interactions, so the chart is fully initialized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 487fa94e-654c-40bf-b6aa-b19278fddc34
📒 Files selected for processing (2)
src/cartesian/Brush.tsxtest/cartesian/Brush.spec.tsx
Bundle ReportChanges will increase total bundle size by 502 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7542 +/- ##
=======================================
Coverage 88.17% 88.17%
=======================================
Files 613 613
Lines 14246 14246
Branches 3580 3579 -1
=======================================
Hits 12562 12562
Misses 1494 1494
Partials 190 190 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 participants
Summary
Fixes #6279.
The reported repro ("drag the brush handle past the start/end bound, drag it back in, the position ends up offset") turns out to be a symptom of a more general bug: dragging the Brush's slide and releasing the mouse can snap
startXback to its original, pre-drag position wheneverstartIndex/endIndexare controlled via props - regardless of whether the drag went out of bounds. Any drag that ends withoutonChangehaving changed the controlledstartIndex/endIndexprop (the common case for small mouse movements that don't cross a data-index boundary) triggers it.Root cause
Brush.getDerivedStateFromPropshas a branch that re-syncsstartX/endXfromstartIndexControlledFromProps/endIndexControlledFromPropswhenever those controlled props change, but only while the user is not currently interacting with the brush (so it doesn't fight an active drag). The guard for "not interacting" is only false whileisSlideMoving/isTravellerMoving/etc. are true, so this branch is skipped for the entire duration of a drag.The
prevStartIndexControlledFromProps/prevEndIndexControlledFromPropstracking fields used for the comparison were only ever initialized inside that same skipped branch - never on mount. Since the branch is always skipped while dragging, the first time it runs again is the render right aftermouseup. At that pointprevStartIndexControlledFromPropsis stillundefined, soundefined !== startIndexControlledFromPropsistrueeven though the prop never actually changed - and the code incorrectly treats this as "the controlled prop changed", resettingstartXback toscale(startIndexControlledFromProps)and discarding the user's drag.Fix
Initialize
prevStartIndexControlledFromProps/prevEndIndexControlledFromPropsin the samegetDerivedStateFromPropsbranch that already computes the scale on mount / data change, so the trackers start in sync with the actual prop values instead ofundefined. This closes the false-mismatch window without touching the (correct) re-sync behavior for when the controlled props genuinely change later.Test plan
test/cartesian/Brush.spec.tsx(dragging the slide when start/endIndex are controlled from props) that drags the slide with controlledstartIndex/endIndexand asserts the position right aftermouseupmatches the position while dragging.startXreverts to its pre-drag value) and passes with it.npm run test- full suite passes (16199 passed, same pre-existing 6 expected-fail / unrelated website-build failures as onmain).npm run check-types- passes.npm run lint- passes (0 errors).Summary by CodeRabbit
Bug Fixes
Tests