Fix stackOffset=sign for charts with 3 or more positive values in one series by PavelVanecek · Pull Request #6807 · recharts/recharts

PavelVanecek · 2025-12-19T06:13:51Z

Description

Caused by an unfortunate typescript refactor.

Related Issue

Fixes #6803

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)

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

Tests
- Added comprehensive test suite validating bar chart stack offset behavior across multiple configuration modes.
Refactor
- Optimized internal offset calculation logic in bar chart rendering for improved accuracy.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

… series

coderabbitai · 2025-12-19T06:14:01Z

Walkthrough

This PR exports a previously internal selector (selectStackedDataOfItem), fixes stack offset calculation logic in ChartUtils.ts for the offsetSign and offsetPositive functions, exports the ExpectedBar test utility type, and introduces a comprehensive test suite validating bar chart rendering and selector behavior across stack offset modes.

Changes

Cohort / File(s)	Summary
Selector Export `src/state/selectors/barSelectors.ts`	Export `selectStackedDataOfItem` selector by adding `export` keyword; no logic changes
Stack Offset Logic Fix `src/util/ChartUtils.ts`	Refactor `offsetSign` and `offsetPositive` to update running totals before assignment; changes accumulator update pattern but maintains behavior
Test Utility Export `test/helper/expectBars.ts`	Export `ExpectedBar` type alias for public use; no behavioral changes
Stack Offset Test Suite `test/state/selectors/selectBarRectangles.stackOffset.spec.tsx`	Add comprehensive test suite for bar chart stack offset behavior across `none`, `sign`, and `positive` modes; validates rendering, DOM rectangles, and related selectors

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

ChartUtils.ts: Offset calculation refactoring requires careful verification that the updated accumulator pattern (updating totals before assignment) produces identical output to the previous approach, particularly for offsetSign which addresses the reported issue
selectBarRectangles.stackOffset.spec.tsx: Large test file spanning three stack offset modes with multiple test cases; verify test expectations are correct and align with the fix logic
barSelectors.ts & expectBars.ts: Straightforward exports requiring minimal review

Possibly related issues

#6803: The stack offset calculation fixes in ChartUtils.ts directly address the reported bug where stackOffset = 'sign' caused incorrect bar sizing and Y-axis total misalignment
#6802: Related to the same stackOffset sign-offset rendering issue

Possibly related PRs

Ts strict #6750: Modifies the same stacking logic in ChartUtils.ts (offsetSign/offsetPositive) and affects stacked-data selectors
Refactor barSelectors so that they read axis IDs from state instead of props #6723: Refactors selectStackedDataOfItem which this PR also exports

Suggested labels

refactor

Suggested reviewers

ckifer

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the bug fix: correcting stackOffset=sign for charts with 3+ positive values, which directly addresses the linked issue.
Description check	✅ Passed	The description includes the required sections: a brief explanation, related issue link, types of changes, and checklist. Most sections are complete; motivation/context is minimal but the linked issue provides full context.
Linked Issues check	✅ Passed	The PR addresses the core requirement from issue #6803: fixing stackOffset='sign' behavior for correct bar stacking and Y-axis alignment. Code changes in ChartUtils fix the offset calculation logic, and new tests verify correct behavior.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing the stackOffset='sign' issue: core offset logic fixes, necessary exports for testing, and comprehensive test coverage. No unrelated modifications detected.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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 stackOffset-sign

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 and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx (1)

1-480: Comprehensive test coverage for the stack offset bug fix with best practices applied!

The test suite thoroughly validates the fix across three stack offset modes (none, sign, positive) with well-documented expected behavior. It properly uses createSelectorTestCase and expectLastCalledWith helpers as per guidelines.

To enhance performance verification, consider adding assertions to check the number of selector calls using spy.mock.calls.length for each test. This helps identify unnecessary re-renders or redundant computations in the selectors.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b4948a8 and 95052d7.

📒 Files selected for processing (4)

src/state/selectors/barSelectors.ts (1 hunks)
src/util/ChartUtils.ts (2 hunks)
test/helper/expectBars.ts (1 hunks)
test/state/selectors/selectBarRectangles.stackOffset.spec.tsx (1 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

**/*.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

**/*.{ts,tsx}: Never use any type (implicit or explicit) in TypeScript code
Prefer unknown over any and refine the type in TypeScript
Type function parameters and return values explicitly in TypeScript, do not rely on implicit any or inference; exceptions are React components and trivial functions
Do not use as type assertions in TypeScript; the only exception is as const

Files:

test/helper/expectBars.ts
src/state/selectors/barSelectors.ts
src/util/ChartUtils.ts
test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

**/*.{js,ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Ensure code lints by running npm run lint and follows Airbnb's Style Guide

Files:

test/helper/expectBars.ts
src/state/selectors/barSelectors.ts
src/util/ChartUtils.ts
test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

**/*.{ts,tsx,js,jsx}

📄 CodeRabbit inference engine (DEVELOPING.md)

All imports from recharts must use the public API entry point; imports from internal paths like recharts/types/* or recharts/src/* are not allowed

Files:

test/helper/expectBars.ts
src/state/selectors/barSelectors.ts
src/util/ChartUtils.ts
test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Do not hardcode any strings or formatting choices in library code; users should provide localized strings as desired

Files:

src/state/selectors/barSelectors.ts
src/util/ChartUtils.ts

test/**/*.spec.{ts,tsx}

📄 CodeRabbit inference engine (CONTRIBUTING.md)

Aim for 100% unit test code coverage when writing new code

Files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

test/**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (test/README.md)

test/**/*.{test,spec}.{ts,tsx}: Aim for 100% unit test code coverage when writing new code
Prefer to use the createSelectorTestCase helper function when writing or modifying tests
Use the expectLastCalledWith helper function instead of expect(spy).toHaveBeenLastCalledWith(...) for better typing and autocompletion
Verify the number of selector calls using the spy object from createSelectorTestCase to spot unnecessary re-renders and improve performance
Mock getBoundingClientRect in tests using the helper function provided in test/helper/MockGetBoundingClientRect.ts
Use vi.useFakeTimers() in all tests due to Redux autoBatchEnhancer dependency on timers and requestAnimationFrame
Call vi.runOnlyPendingTimers() to advance timers after renders when not using createSelectorTestCase helper, and avoid vi.runAllTimers() to prevent infinite loops
Use userEvent.setup({ advanceTimers: vi.runOnlyPendingTimers }) or the userEventSetup helper function from test/helper/userEventSetup.ts when creating userEvent instances
When testing tooltips on hover, use vi.runOnlyPendingTimers() after each userEvent.hover() call or use the showTooltip helper function from tooltipTestHelpers.ts to account for requestAnimationFrame delays

Files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

**/*.spec.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

When running unit tests, prefer to run a single test file using npm run test -- path/to/TestFile.spec.tsx rather than running all tests with npm test

Files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

🧠 Learnings (10)

📓 Common learnings

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:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to 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} : Prefer to use the `createSelectorTestCase` helper function when writing or modifying tests

📚 Learning: 2025-11-25T01:23:08.250Z

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 the `expectLastCalledWith` helper function instead of `expect(spy).toHaveBeenLastCalledWith(...)` for better typing and autocompletion

Applied to files:

test/helper/expectBars.ts

📚 Learning: 2025-11-23T13:30:10.395Z

Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6669
File: www/src/docs/exampleComponents/ScatterChart/ScatterChartWithLabels.tsx:2-2
Timestamp: 2025-11-23T13:30:10.395Z
Learning: The `TooltipIndex` type from recharts is defined in `src/state/tooltipSlice.ts` but is not currently exported from the public API entry points. It should not be imported from `recharts/types/state/tooltipSlice` as this is an internal implementation path. An ESLint rule is needed to prevent regressions.

Applied to files:

test/helper/expectBars.ts
src/state/selectors/barSelectors.ts

📚 Learning: 2025-12-16T08:12:13.355Z

Learnt from: PavelVanecek
Repo: recharts/recharts PR: 6783
File: test/util/ChartUtils.spec.tsx:15-16
Timestamp: 2025-12-16T08:12:13.355Z
Learning: In the recharts codebase, files in the `test` folder are allowed to import from internal paths (e.g., `../../src/state/cartesianAxisSlice`) and do not need to use the public API entry point (`src/index.ts`). The public API import restriction applies only to non-test code.

Applied to files:

test/helper/expectBars.ts

📚 Learning: 2025-11-25T01:23:08.250Z

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

Applied to files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

📚 Learning: 2025-11-25T01:23:08.250Z

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

Applied to files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

📚 Learning: 2025-12-16T08:12:06.809Z

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.

Applied to files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

📚 Learning: 2025-11-25T01:23:08.250Z

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`

Applied to files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

📚 Learning: 2025-11-25T01:23:08.250Z

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

Applied to files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

📚 Learning: 2025-11-25T01:22:59.729Z

Learnt from: CR
Repo: recharts/recharts PR: 0
File: CONTRIBUTING.md:0-0
Timestamp: 2025-11-25T01:22:59.729Z
Learning: Applies to test/component/**/*.spec.tsx : Use React Testing Library for testing component interactions and behavior upon rendering

Applied to files:

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx

🧬 Code graph analysis (1)

test/state/selectors/selectBarRectangles.stackOffset.spec.tsx (7)

test/helper/expectBars.ts (2)

ExpectedBar (4-11)

expectBars (21-33)

src/util/ChartUtils.ts (1)

BarPositionPosition (60-74)

test/_data.ts (1)

PageData (4-11)

src/state/selectors/barSelectors.ts (3)

selectBarRectangles (264-343)

selectBarPosition (234-253)

selectStackedDataOfItem (255-262)

test/helper/expectLastCalledWith.ts (1)

expectLastCalledWith (14-34)

src/state/selectors/axisSelectors.ts (1)

selectDisplayedStackedData (572-580)

src/state/cartesianAxisSlice.ts (1)

defaultAxisId (12-12)

⏰ 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 (4)

src/util/ChartUtils.ts (2)

372-380: LGTM! Correct fix for stackOffset='sign' bug.

The updated accumulation logic correctly increments the running totals before assigning to col[1], ensuring proper stacking behavior. This fix addresses the issue where stacked bar sums were incorrect with three or more positive values.

414-421: LGTM! Consistent fix for stackOffset='positive'.

The same correct accumulation pattern is applied here, ensuring consistency across stack offset modes.

test/helper/expectBars.ts (1)

4-11: LGTM! Helpful test utility export.

Exporting the ExpectedBar type makes it reusable across test files, improving test maintainability.

src/state/selectors/barSelectors.ts (1)

255-262: Verify if this selector should be part of the public API.

The selectStackedDataOfItem selector is now exported. While tests can import from internal paths (per coding guidelines), exposing this selector increases the public API surface. Confirm whether this is intentional or if it's only needed for testing purposes.

codecov · 2025-12-19T06:23:52Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.83%. Comparing base (b4948a8) to head (95052d7).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6807   +/-   ##
=======================================
  Coverage   93.83%   93.83%           
=======================================
  Files         518      518           
  Lines       44781    44781           
  Branches     5037     5037           
=======================================
  Hits        42021    42021           
  Misses       2753     2753           
  Partials        7        7

☔ 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 · 2025-12-19T06:25:58Z

Bundle Report

Changes will decrease total bundle size by 97 bytes (-0.0%) ⬇️. This is within the configured threshold ✅

Detailed changes

Bundle name	Size	Change
recharts/bundle-cjs	1.18MB	-15 bytes (-0.0%) ⬇️
recharts/bundle-es6	1.02MB	-76 bytes (-0.01%) ⬇️
recharts/bundle-umd	523.0kB	-6 bytes (-0.0%) ⬇️

Affected Assets, Files, and Routes:

view changes for bundle: recharts/bundle-es6

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`util/ChartUtils.js`	-83 bytes	18.27kB	-0.45%
`state/selectors/barSelectors.js`	7 bytes	8.14kB	0.09%

view changes for bundle: recharts/bundle-cjs

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`util/ChartUtils.js`	-83 bytes	20.36kB	-0.41%
`state/selectors/barSelectors.js`	68 bytes	9.84kB	0.7%

view changes for bundle: recharts/bundle-umd

Assets Changed:

Asset Name	Size Change	Total Size	Change (%)
`Recharts.js`	-6 bytes	523.0kB	-0.0%

ckifer

😅

PavelVanecek · 2025-12-19T06:39:39Z

Variable assignment is hard okay

github-actions · 2025-12-19T06:43:59Z

Staging Deployment Details

These deployments will remain available for 30 days.

To update snapshots: Comment /update-snapshots on this PR to automatically update the baseline screenshots.

PavelVanecek added 2 commits December 19, 2025 14:46

Add unit test to reproduce 6803

a3128fc

Fix stackOffset=sign for charts with 3 or more positive values in one…

95052d7

… series

coderabbitai Bot reviewed Dec 19, 2025

View reviewed changes

ckifer approved these changes Dec 19, 2025

View reviewed changes

ckifer merged commit 381f256 into main Dec 19, 2025
44 checks passed

ckifer deleted the stackOffset-sign branch December 19, 2025 07:01

PavelVanecek mentioned this pull request Jan 17, 2026

BarStack bars don't show when they have negative values #6893

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Fix stackOffset=sign for charts with 3 or more positive values in one series#6807

Fix stackOffset=sign for charts with 3 or more positive values in one series#6807
ckifer merged 2 commits into
mainfrom
stackOffset-sign

PavelVanecek commented Dec 19, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

codecov Bot commented Dec 19, 2025 •

edited

Loading

Uh oh!

codecov Bot commented Dec 19, 2025

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

ckifer left a comment

Uh oh!

PavelVanecek commented Dec 19, 2025

Uh oh!

github-actions Bot commented Dec 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Uh oh!

Conversation

PavelVanecek commented Dec 19, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Types of changes

Checklist:

Summary by CodeRabbit

Release Notes

Uh oh!

coderabbitai Bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related issues

Possibly related PRs

Suggested labels

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Dec 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

codecov Bot commented Dec 19, 2025

Bundle Report

Affected Assets, Files, and Routes:

Assets Changed:

Assets Changed:

Assets Changed:

Uh oh!

ckifer left a comment

Choose a reason for hiding this comment

Uh oh!

PavelVanecek commented Dec 19, 2025

Uh oh!

github-actions Bot commented Dec 19, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

PavelVanecek commented Dec 19, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Dec 19, 2025 •

edited

Loading

codecov Bot commented Dec 19, 2025 •

edited

Loading