fix(Area): connectNulls with stackId correctly handles all-null data points#7085
Closed
Harikrushn9118 wants to merge 5 commits into
Closed
fix(Area): connectNulls with stackId correctly handles all-null data points#7085Harikrushn9118 wants to merge 5 commits into
Harikrushn9118 wants to merge 5 commits into
Conversation
…ies are null Fixes recharts#6985. PR recharts#7041 removed the !connectNulls guard entirely, which caused a regression (recharts#7070) where stacked areas overlapped when connectNulls={true} and only some series had null values. This v2 fix preserves the original !connectNulls guard and adds a new allStackedSeriesNull check: when connectNulls is true and a stacked chart has a data point where ALL series in the stack are null, only then treat it as a breakpoint. If only some series are null, they are connected through as 0 (the correct stacking behavior). Changes: - Added selectStackDataKeys selector to retrieve all dataKeys in a stack group - Pass stackDataKeys into computeArea - isBreakPoint now has three conditions: 1. value1 == null (basic null check) 2. hasStack && !connectNulls && rawValue == null (original guard, preserved) 3. allStackedSeriesNull (new: all series null → break even with connectNulls)
Contributor
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (3)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughAdds stack-aware null handling to area layout: Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cartesian/Area.tsx (1)
1047-1047: Avoid duplicategetValueByDataKeylookup per datum.Line 1047 recalculates
rawValue, while non-stacked flow already computed it at Line 1035. Reusing one value avoids repeated accessor/function execution inside this loop.Proposed change
- if (hasStack) { + const rawValue = getValueByDataKey(entry, dataKey); + if (hasStack) { valueAsArray = stackedData[dataStartIndex + index]; } else { - const rawValue = getValueByDataKey(entry, dataKey); - if (!Array.isArray(rawValue)) { valueAsArray = [baseValue, rawValue]; } else { valueAsArray = rawValue; isRange = true; } } @@ - const rawValue = getValueByDataKey(entry, dataKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cartesian/Area.tsx` at line 1047, The loop in Area.tsx is calling getValueByDataKey twice for the same datum; reuse the previously computed rawValue instead of recalculating it. Modify the loop so the rawValue computed earlier (from getValueByDataKey(entry, dataKey)) is stored in a variable visible to both the non-stacked and stacked branches and remove the second getValueByDataKey call, ensuring you still use entry and dataKey where needed and preserve any variable renaming (e.g., rawValue) used by the surrounding code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/selectors/areaSelectors.ts`:
- Line 151: The current expression in areaSelectors.ts that builds dataKeys —
group.graphicalItems.map(item => item.dataKey).filter(Boolean) — incorrectly
drops valid falsy keys like 0 or ''; update the filter to only remove
null/undefined (e.g., .filter(k => k !== undefined && k !== null)) so
allStackedSeriesNull and any logic using the dataKey list preserve numeric-zero
and empty-string keys while still excluding missing values.
---
Nitpick comments:
In `@src/cartesian/Area.tsx`:
- Line 1047: The loop in Area.tsx is calling getValueByDataKey twice for the
same datum; reuse the previously computed rawValue instead of recalculating it.
Modify the loop so the rawValue computed earlier (from getValueByDataKey(entry,
dataKey)) is stored in a variable visible to both the non-stacked and stacked
branches and remove the second getValueByDataKey call, ensuring you still use
entry and dataKey where needed and preserve any variable renaming (e.g.,
rawValue) used by the surrounding code paths.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7085 +/- ##
==========================================
- Coverage 89.81% 89.65% -0.17%
==========================================
Files 529 536 +7
Lines 39956 40561 +605
Branches 5452 5538 +86
==========================================
+ Hits 35888 36363 +475
- Misses 4059 4190 +131
+ Partials 9 8 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state/selectors/areaSelectors.ts`:
- Around line 134-154: Add a unit test that exercises the "all-null
stacked-null" branch by creating area settings with stackId and
connectNulls={true}, a stack group (selectNumericalAxisStackGroups) whose
graphicalItems all have null values at the same index, and assert that
selectStackDataKeys flows into computeArea producing the expected all-null
handling; specifically, write a focused test under Area tests that constructs
the RechartsRootState slices used by selectSynchronisedAreaSettings and
selectNumericalAxisStackGroups, calls the selector/selectors
(selectStackDataKeys) and then computeArea (or the public component behavior
relying on it), and asserts the behavior for the all-null breakpoint so this new
production path is covered.
In `@www/src/docs/exampleComponents/AreaChart/AreaChartConnectNulls.tsx`:
- Around line 68-70: The sample data in the AreaChartConnectNulls component
contains a duplicated category label ("Page D") for the last two points; update
the data array in AreaChartConnectNulls.tsx by renaming the final object's name
field to a distinct label (e.g., change the last "Page D" to "Page E") so the
categorical axis and connectNulls behavior are clear and not confusing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7b7cf8b-fee4-4a85-b870-ff48033933fc
⛔ Files ignored due to path filters (3)
test-vr/__snapshots__/tests/www/AreaChartApiExamples.spec-vr.tsx-snapshots/AreaChartConnectNulls-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/AreaChartApiExamples.spec-vr.tsx-snapshots/AreaChartConnectNulls-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/AreaChartApiExamples.spec-vr.tsx-snapshots/AreaChartConnectNulls-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (2)
src/state/selectors/areaSelectors.tswww/src/docs/exampleComponents/AreaChart/AreaChartConnectNulls.tsx
Comment on lines
+134
to
+154
| const selectStackDataKeys: ( | ||
| state: RechartsRootState, | ||
| id: GraphicalItemId, | ||
| isPanorama: boolean, | ||
| ) => ReadonlyArray<DataKey<any>> | undefined = createSelector( | ||
| [selectSynchronisedAreaSettings, selectNumericalAxisStackGroups], | ||
| (areaSettings: AreaSettings | undefined, stackGroups: Record<StackId, StackGroup> | undefined) => { | ||
| if (areaSettings == null || stackGroups == null) { | ||
| return undefined; | ||
| } | ||
| const { stackId } = areaSettings; | ||
| if (stackId == null) { | ||
| return undefined; | ||
| } | ||
| const group: StackGroup | undefined = stackGroups[stackId]; | ||
| if (group == null) { | ||
| return undefined; | ||
| } | ||
| return group.graphicalItems.map(item => item.dataKey).filter(isNotNil); | ||
| }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Add a unit test for the new all-null stacked-null branch.
This wires a new production path for connectNulls={true} when every sibling in the stack is null at the same index, but the PR context only mentions existing coverage for the partial-null case. Please add a focused Area test for the all-null breakpoint behavior so this selector-to-computeArea() path stays pinned.
As per coding guidelines, "Aim for 100% unit test code coverage when writing new code".
Also applies to: 172-225
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/selectors/areaSelectors.ts` around lines 134 - 154, Add a unit test
that exercises the "all-null stacked-null" branch by creating area settings with
stackId and connectNulls={true}, a stack group (selectNumericalAxisStackGroups)
whose graphicalItems all have null values at the same index, and assert that
selectStackDataKeys flows into computeArea producing the expected all-null
handling; specifically, write a focused test under Area tests that constructs
the RechartsRootState slices used by selectSynchronisedAreaSettings and
selectNumericalAxisStackGroups, calls the selector/selectors
(selectStackDataKeys) and then computeArea (or the public component behavior
relying on it), and asserts the behavior for the all-null breakpoint so this new
production path is covered.
Bundle ReportChanges will increase total bundle size by 88.42kB (1.77%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-treemapAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
Collaborator
|
Here is the chart before & after, is this how it's meant to look like? #6985 (comment) |
PavelVanecek
reviewed
Mar 7, 2026
Contributor
Author
There was a problem hiding this comment.
Thanks for adding the example and the isNotNil cleanup! The approach here is to only break when all stacked series are null at a data point — partial nulls are still treated as 0 in the stack so stacking stays consistent.
…ent connectNulls on both areas, remove stale VR snapshots
Contributor
Author
|
@PavelVanecek Fixed! The 5th chart had a duplicate "Page D" label and only one area had connectNulls — that's why it looked off. Now both areas use connectNulls and the labels are unique. Deleted the VR snapshots so CI can regenerate them. The core logic is same — partial nulls still treated as 0 in the stack, only all-null entries become breakpoints. |
Collaborator
|
Our CI doesn't generate snapshots. You need to commit them. |
PavelVanecek
pushed a commit
that referenced
this pull request
Jul 20, 2026
…ries is null (#7566) Fixes #6985.  Top chart is master, bottom is this PR. The all-null point (Page C) connects across; the partial-null point (Page F, only `uv` missing) still renders as 0, so the #7073 regression does not return. ## Problem In a stacked AreaChart with `connectNulls`, a data point where every series is null renders as a drop to 0 instead of connecting across the gap. d3-stack defaults nulls to 0, so `value1` is never null for stacked data, and the `!connectNulls` guard is the only thing controlling null behavior there. ## Change `computeArea` in `src/cartesian/Area.tsx` marks a point as a break point when every dataKey in the stack group is null (`wholeStackIsNull`). The stack dataKeys are supplied by a new `selectStackDataKeys` selector in `src/state/selectors/areaSelectors.ts`. The `!connectNulls && rawValue == null` guard is unchanged, so a point where only some series are null still renders as 0 in the stack. - Every series null at a point: connects across (new behavior). - Some series null at a point: unchanged, renders as 0 to preserve stack offsets. ## Prior art This is the design proposed by @Harikrushn9118 in #7085: keep the existing guard, add a separate all-null check, and pass the stack dataKeys into `computeArea`. #7085 was closed by its author before completion. This PR implements that design and commits the VR snapshots #7085 was missing. ## History #7041 removed the `!connectNulls` guard and was merged, then reverted in #7073 because it made stacked charts with a single null series render incorrectly. This PR keeps that guard and scopes the new behavior to the all-null case, so the #7073 regression does not return. ## Tests - `test/chart/AreaChart.spec.tsx`: two regression tests, "connects across points where every stacked series is null if connectNulls is true" and "breaks at points where every stacked series is null if connectNulls is false". Both fail without the source change. The existing "Renders null points as 0 if stacked and connectNulls is true" test covers the partial-null case that must stay unchanged. - `www` example `AreaChartConnectNulls.tsx`: adds a `dataWithGaps` dataset with an all-null point at Page C and a partial-null point at Page F. - VR snapshots committed for chromium, firefox, and webkit. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved stacked area gap/break detection when an entire stacked point has all series values missing, so gaps are handled correctly. * `connectNulls` now reliably bridges fully missing stacked points, while keeping breaks when `connectNulls` is disabled. * Partial missing values still render with correct stacked contributions. * **Documentation** * Added stacked area examples showing how gaps are rendered with and without `connectNulls`. * **Tests** * Added coverage validating `connectNulls` behavior for all-missing stacked points in area charts. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #6985. Follow-up to reverted PR #7041.
Changes
PR #7041 removed the
!connectNullsguard entirely, which brokeconnectNulls={true}for stacked charts — null points in individual series became break points even when the user opted into connecting them, causing visible overlap artifacts (#7070).This v2 fix preserves the original
!connectNulls &&guard and adds a targetedallStackedSeriesNullcheck:connectNulls={true}and a stacked chart has a data point where only some series are null → connect through (treat as 0) ✅connectNulls={true}and ALL series in the stack are null for a data point → treat as breakpoint ✅ (avoids overlap regression)connectNulls={false}→ unchanged behavior ✅Implementation
selectStackDataKeysinareaSelectors.ts— retrieves alldataKeys from sibling graphical items in the same stack groupstackDataKeysintocomputeArea()isBreakPointlogic with three conditions:value1 == null— basic null check (unchanged)hasStack && !connectNulls && rawValue == null— original guard (preserved)allStackedSeriesNull— new: only breaks when ALL stacked series are nullRoot cause
d3-stackdefaults null values to 0 in stacked output, sovalue1(fromstackedData) is nevernullfor stacked charts. The!connectNullsguard on the second condition was the only thing controlling null behavior in stacks. Removing it entirely (PR #7041) was too aggressive.Related Issue
Fixes #6985
Fixes #7070
How Has This Been Tested?
'Renders null points as 0 if stacked and connectNulls is true'covers the partial-null scenarioTypes of changes
Checklist:
Summary by CodeRabbit