Conversation
When a chart has items with own data (e.g. Scatter with outliers) alongside items that use chart-level data (e.g. Bar), the numerical axis domain was only covering the items-with-own-data. The root cause was that combineDomainOfAllAppliedNumericalValuesIncludingErrorValues received selectDisplayedData which returns only graphicalItemsData when any item has own data, ignoring chart-level data entirely. Fix: restructure the iteration to be item-first. For each item, use item.data when present, falling back to the chart-level data slice. This ensures Bar (chart-level) and Scatter (own data) both contribute to the domain. The chartDataSlice is passed as a new input to the domain selector via selectChartDataSliceIfNotInPanorama, a content-memoized selector that returns a stable reference when the slice content hasn't changed. This prevents spurious recomputes but introduces one additional render in ScatterChart and ComposedChart (consistent with existing behaviour in horizontal LineChart, which already expected 4 renders). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
WalkthroughThe PR introduces mixed data-source domain calculation by adding selectors to detect when Cartesian chart items rely on chart-level data rather than their own data prop. It extends the domain computation pipeline to handle cases with no items by falling back to chart data when an axis has a dataKey, and updates error domain calculations to account for ranged bar values and whisker ranges. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant AxisDomain as Domain Calculation
participant DataDetection as Data Detection
participant DataSelection as Data Selection
participant ErrorBars as Error Bar Handler
User->>AxisDomain: Render chart (Bar + ErrorBar ± Scatter)
AxisDomain->>DataDetection: Check if any item uses chart data
alt Item has own data
DataDetection-->>AxisDomain: anyItemUsesChartData = false
AxisDomain->>DataSelection: Use item-level data only
else Item lacks data prop
DataDetection-->>AxisDomain: anyItemUsesChartData = true
AxisDomain->>DataSelection: Merge chart data + item data
end
DataSelection-->>AxisDomain: Return combined data
AxisDomain->>ErrorBars: Compute domain with error ranges
ErrorBars->>ErrorBars: Extract numeric values from ranged data
ErrorBars-->>AxisDomain: Extended domain (base ± errors)
AxisDomain-->>User: Render axis with full domain
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7146 +/- ##
==========================================
+ Coverage 89.59% 89.61% +0.02%
==========================================
Files 536 536
Lines 40401 40479 +78
Branches 5498 5520 +22
==========================================
+ Hits 36197 36275 +78
Misses 4196 4196
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Bundle ReportChanges will increase total bundle size by 35.68kB (0.71%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: recharts/bundle-treeshaking-sunburstAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-polarAssets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-sankeyAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-treemapAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/cartesian/XAxis/XAxis.categorydomain.spec.tsx (1)
595-611: Please pin the recompute budget in these new selector tests.They currently only assert the final domain, so an unnecessary extra selector pass will still go green.
createSelectorTestCasealready gives you the spy—addtoHaveBeenCalledTimes(...)here as well. As per coding guidelines "Verify the number of selector calls using the spy object fromcreateSelectorTestCaseto spot unnecessary re-renders and improve performance".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/cartesian/XAxis/XAxis.categorydomain.spec.tsx` around lines 595 - 611, The tests created with createSelectorTestCase (renderTestCase) are missing assertions on selector call counts; after obtaining the spy from renderTestCase(useXAxisDomain) and renderTestCase(selectTooltipAxisDomain) add an assertion like expect(spy).toHaveBeenCalledTimes(1) to pin the recompute budget and ensure only one selector pass occurs; update the two it blocks that call useXAxisDomain and selectTooltipAxisDomain to include this call-count assertion immediately after the existing expectLastCalledWith(spy, [...]) checks.
🤖 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/dataSelectors.ts`:
- Around line 60-97: The selectors selectChartDataSliceIfNotInPanorama,
selectChartDataSliceIgnoringIndexes, and selectChartDataSliceWithIndexes
currently always return chartData.slice(...) which produces a new array
reference even when contents are identical; change each selector so it returns a
stable array instance when the slice content hasn’t changed by caching the last
returned slice and the last inputs (e.g., last chartData reference, last
dataStartIndex, last dataEndIndex) inside the selector projector (or create a
createSelectorCreator wrapper using defaultMemoize + deep equality) and, before
creating a new slice, compare start/end and either the chartData reference or
deep-equal the candidate slice to the cached slice and return the cached slice
when equal; update the projector bodies of selectChartDataSliceIfNotInPanorama,
selectChartDataSliceIgnoringIndexes, and selectChartDataSliceWithIndexes to use
that cache logic.
---
Nitpick comments:
In `@test/cartesian/XAxis/XAxis.categorydomain.spec.tsx`:
- Around line 595-611: The tests created with createSelectorTestCase
(renderTestCase) are missing assertions on selector call counts; after obtaining
the spy from renderTestCase(useXAxisDomain) and
renderTestCase(selectTooltipAxisDomain) add an assertion like
expect(spy).toHaveBeenCalledTimes(1) to pin the recompute budget and ensure only
one selector pass occurs; update the two it blocks that call useXAxisDomain and
selectTooltipAxisDomain to include this call-count assertion immediately after
the existing expectLastCalledWith(spy, [...]) checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ecc5c928-ba85-4aa8-b7ac-9b0e1140caf2
⛔ Files ignored due to path filters (6)
test-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/BoxPlotExample-1-webkit-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/CandlestickExample-1-chromium-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/CandlestickExample-1-firefox-linux.pngis excluded by!**/*.pngtest-vr/__snapshots__/tests/www/BarChartApiExamples.spec-vr.tsx-snapshots/CandlestickExample-1-webkit-linux.pngis excluded by!**/*.png
📒 Files selected for processing (8)
src/state/selectors/axisSelectors.tssrc/state/selectors/dataSelectors.tssrc/state/selectors/polarSelectors.tssrc/state/selectors/tooltipSelectors.tstest/cartesian/ErrorBar.spec.tsxtest/cartesian/XAxis/XAxis.categorydomain.spec.tsxtest/chart/LineChart.spec.tsxtest/state/selectors/axisSelectors.spec.tsx
Comment on lines
+60
to
+97
| /** | ||
| * Returns the chart-level data slice (respecting Brush indexes), memoized by content so that | ||
| * spurious Immer reference changes (e.g. dispatching `setChartData(undefined)` when data is | ||
| * already `undefined`) do not propagate to downstream selectors. | ||
| * | ||
| * Used when a selector needs chart-level data but must avoid extra recomputes when the | ||
| * data content has not actually changed. | ||
| */ | ||
| export const selectChartDataSliceIfNotInPanorama: ( | ||
| state: RechartsRootState, | ||
| _unused1: unknown, | ||
| _unused2: unknown, | ||
| isPanorama: boolean, | ||
| ) => ChartData = createSelector( | ||
| [selectChartDataWithIndexesIfNotInPanoramaPosition4], | ||
| ({ chartData, dataStartIndex, dataEndIndex }: ChartDataState): ChartData => | ||
| chartData != null ? chartData.slice(dataStartIndex, dataEndIndex + 1) : [], | ||
| ); | ||
|
|
||
| /** | ||
| * Returns the chart-level data slice (ignoring Brush indexes), memoized by content. | ||
| * Used in tooltip and polar selectors that always need the full data range. | ||
| */ | ||
| export const selectChartDataSliceIgnoringIndexes: (state: RechartsRootState) => ChartData = createSelector( | ||
| [selectChartDataAndAlwaysIgnoreIndexes], | ||
| ({ chartData, dataStartIndex, dataEndIndex }: ChartDataState): ChartData => | ||
| chartData != null ? chartData.slice(dataStartIndex, dataEndIndex + 1) : [], | ||
| ); | ||
|
|
||
| /** | ||
| * Returns the chart-level data slice (with Brush indexes applied), memoized by content. | ||
| * Used in tooltip selectors. | ||
| */ | ||
| export const selectChartDataSliceWithIndexes: (state: RechartsRootState) => ChartData = createSelector( | ||
| [selectChartDataWithIndexes], | ||
| ({ chartData, dataStartIndex, dataEndIndex }: ChartDataState): ChartData => | ||
| chartData != null ? chartData.slice(dataStartIndex, dataEndIndex + 1) : [], | ||
| ); |
Contributor
There was a problem hiding this comment.
The “memoized by content” guarantee is not implemented yet.
These selectors still emit a fresh slice(...) array whenever the surrounding ChartDataState object gets a new reference, so unchanged slices keep invalidating downstream selectors. The 3→4 call-count bump in test/state/selectors/axisSelectors.spec.tsx is already one symptom of that. Please stabilize equal slices here instead of teaching callers to expect the extra recompute. Based on learnings "Value consistency, usability, and performance in Recharts implementation".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state/selectors/dataSelectors.ts` around lines 60 - 97, The selectors
selectChartDataSliceIfNotInPanorama, selectChartDataSliceIgnoringIndexes, and
selectChartDataSliceWithIndexes currently always return chartData.slice(...)
which produces a new array reference even when contents are identical; change
each selector so it returns a stable array instance when the slice content
hasn’t changed by caching the last returned slice and the last inputs (e.g.,
last chartData reference, last dataStartIndex, last dataEndIndex) inside the
selector projector (or create a createSelectorCreator wrapper using
defaultMemoize + deep equality) and, before creating a new slice, compare
start/end and either the chartData reference or deep-equal the candidate slice
to the cached slice and return the cached slice when equal; update the projector
bodies of selectChartDataSliceIfNotInPanorama,
selectChartDataSliceIgnoringIndexes, and selectChartDataSliceWithIndexes to use
that cache logic.
Contributor
|
Staging Deployment Details
These deployments will remain available for 30 days. To update snapshots: Comment |
Member
lol |
ckifer
approved these changes
Mar 17, 2026
Merged
PavelVanecek
added a commit
that referenced
this pull request
Mar 22, 2026
## Description #7146 fixed a bug where the XAxis was ignoring data defined in both chart and graphical item. One of our stories did just that: https://www.chromatic.com/test?appId=63da8268a0da9970db6992aa&id=69bf5ed7116eb6d0d637ed0d so here is a fix. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Refreshed sample datasets in the Tooltip story to showcase updated data structures. * Enhanced axis configuration examples in tooltip documentation. <!-- 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.
Description
Fixes two bugs that were affecting the new boxplot example - now we don't need to be embarassed to release it.
Related Issue
Fixes #7134
Fixes #7138
Screenshots (if appropriate):
Notice the XAxis domain (before: ABDE, after: ABCDE) and notice the YAxis domain (now it correctly extends based on the error bar values).
Types of changes
Checklist:
Summary by CodeRabbit