fix(charts-core): keep zero-valued cells on their stack baseline in the diverging offset - #70
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan includes up to 4 reviews per rolling hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe default diverging stack offset now preserves zero-valued cells on their series’ running baseline. Tests cover positive, negative, mixed, and all-zero stacks, plus vertical and horizontal materialized rows. A patch changeset documents the correction. ChangesZero-aware diverging stacking
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized fix keeps exact-zero stacked cells on their current baseline and adds coverage for positive, negative, mixed, and all-zero cases; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
…he diverging offset d3's stackOffsetDiverging assigns [0, 0] to a cell whose value is exactly zero, parking it at the axis instead of its stack's running baseline. That is invisible for bars but not for area/line marks, whose paths interpolate between adjacent positions, producing a spike to the axis and back that cuts through the layers below. Replace stackOffsetDiverging with a local zero-aware variant that keeps a zero-valued cell on the baseline of whichever side its own series occupies. A series is treated as negative-side only when it is exclusively negative; everything else resolves to the positive baseline.
686f0a9 to
2f96675
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Problem
A stacked
areaY/areaX/barYwith an exact-zero value renders a spike to the axis instead of a flat segment at the current baseline.stackExtentsdefaults to d3'sstackOffsetDiverging, which assigns the extent[0, 0]to any cell whose value is exactly0. d3 does this on purpose — zero has no side, so it is stacked at zero — and for bar marks that is fine, since a zero-height band is invisible wherever it sits. Area and line marks interpolate between adjacent positions, so the same rule makes the band's edges collapse to the axis and return, cutting through every band below it.Shows a stacked areaY with two series ("Allowed" climbing, "Creating" idle at 0 for one stage); the "Creating" band spikes to the axis and cuts through "Allowed" underneath.
The bug is not confined to the topmost series. It affects any series that is not the first on its side of the axis, in both directions:
A=10, B=5 / A=10, B=0B[0, 0][10, 10]A=-10, B=-5 / A=-10, B=0B[0, 0][-10, -10]A=10, B=-5, C=3 / …, C=0C[0, 0][10, 10]Every stack in the library is affected, because
divergingis the default offset andstack-internal.tsbacksarea,area-x,barandtransform-stackalike. Applications currently have to pre-perturb their data with an epsilon to avoid it.Fix
Replace the
stackOffsetDivergingimport with a local offset that keeps d3's positive/negative split but resolves a zero-valued cell to the baseline of whichever side its own series occupies, rather than to the axis. A series is treated as negative-side only when it is exclusively negative; anything else (positive, mixed, all-zero) resolves to the positive baseline, which matches Observable Plot's stack transform (else if (y >= 0) yp = Y2[i] = (Y1[i] = yp) + y).stackOffsetExpand,stackOffsetSilhouetteandstackOffsetWiggleall delegate tostackOffsetNone, which accumulates zeros correctly, so they need no change.Same dataset as above, after the fix: the "Creating" band pinches flat against "Allowed" at the boundary instead of diving to the axis.
Behaviour change
Only cells whose value is exactly
0move, and only when the running baseline on their side is already nonzero. Verified unchanged:0)NaNextents preserved, so gaps still render as gaps)No public API, type, or option changes.
StackOffsetkeeps its four documented values.Tests
Added to
packages/charts-core/src/stack-internal.test.ts:[10, 10], not[0, 0])[-10, -10])stackRowsX/stackRowsYagree withstackExtentson the aboveChangeset
patchfor@tanstack/charts(fixed release group covers all adapters).Alternative considered
Selecting
stackOffsetNonewhenever the data contains no negative values is a two-line change and fixes the reported case, but leaves the negative-side and mixed-stack variants of the same bug in place. Not worth splitting into two fixes.Summary by CodeRabbit