fix(bar): use Math.round instead of bitwise truncation for bar positioning#7297
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (102)
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:
WalkthroughBar position/layout rounding in combineAllBarPositions switches from bitwise truncation to Math.round for centring offsets and computed original sizes; many tests across selectors, cartesian, chart, radial, tooltip, and animation suites were updated to match the new rounded geometry. ChangesBar position rounding and dependent tests
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/state/selectors/combiners/combineAllBarPositions.ts`:
- Around line 70-74: The computation of originalSize in
combineAllBarPositions.ts currently uses Math.round which can increase bar
widths so total bars plus gaps may exceed bandSize; replace the Math.round call
on originalSize with Math.floor (or equivalent truncation for positive values)
to ensure len * originalSize + (len−1)*realBarGap + 2*offset never exceeds
bandSize, then keep the subsequent size = isWellBehavedNumber(maxBarSize) ?
Math.min(originalSize, maxBarSize) : originalSize logic unchanged so maxBarSize
still caps the floored value.
- Line 42: Add unit tests for the combineAllBarPositions function to exercise
the new Math.round() behavior: include cases where (bandSize - sum) / 2 has
fractional part > 0.5 (e.g., yields 2.6 -> offset rounds to 3), where offset is
negative when sum > bandSize after fallback, and where originalSize has
fractional part > 0.5 (e.g., 24.8 -> 25); assert the returned positions and
sizes match expected rounded values and the negative-offset handling. Target the
combiners/combineAllBarPositions tests and create focused specs that construct
inputs to hit the offset and originalSize rounding branches and verify outputs.
Ensure tests fail on the old truncation behavior and pass with the current
Math.round() implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0158589e-b5c2-43e6-99ad-821cc7913701
📒 Files selected for processing (1)
src/state/selectors/combiners/combineAllBarPositions.ts
| } | ||
|
|
||
| const offset = ((bandSize - sum) / 2) >> 0; | ||
| const offset = Math.round((bandSize - sum) / 2); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for existing test files covering combineAllBarPositions / getBarPositions
rg -rn --type=ts "combineAllBarPositions\|getBarPositions" --glob "**/*.test.*" -A 2Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Find the file and check its content
fd "combineAllBarPositions" --type fRepository: recharts/recharts
Length of output: 117
🏁 Script executed:
# Look at the file content, particularly lines around 42 and 72
cat -n src/state/selectors/combiners/combineAllBarPositions.ts | head -80Repository: recharts/recharts
Length of output: 3391
🏁 Script executed:
# Search more broadly for test files related to bar positions or combiners
fd --type f -e test.ts -e spec.ts | grep -i "bar\|position\|combiner" | head -20Repository: recharts/recharts
Length of output: 359
🏁 Script executed:
# Search for any tests that import combineAllBarPositions or getBarPositions using different patterns
rg "import.*combineAllBarPositions|import.*getBarPositions" --type tsRepository: recharts/recharts
Length of output: 293
🏁 Script executed:
# Search for test files for barSelectors
fd "barSelectors" --type f | grep -E "\.(test|spec)\.(ts|tsx)"Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Search for test files related to BarChart or bar positioning
fd --type f -e spec.ts -e test.ts | xargs grep -l "BarChart\|combineBarSizeList" 2>/dev/null | head -10Repository: recharts/recharts
Length of output: 411
🏁 Script executed:
# Check if there's a test directory structure for combiners
find test -type d -name "combiners" 2>/dev/nullRepository: recharts/recharts
Length of output: 92
🏁 Script executed:
# Look at the git history to see what changed recently for this file
git log --oneline -n 20 src/state/selectors/combiners/combineAllBarPositions.ts 2>/dev/nullRepository: recharts/recharts
Length of output: 142
🏁 Script executed:
# List all test files in the combiners directory
ls -la test/state/selectors/combiners/Repository: recharts/recharts
Length of output: 499
🏁 Script executed:
# Check if there's a test file for combineAllBarPositions that might not show up in the fd search
ls -la test/state/selectors/combiners/ | grep -i barRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Search for any test content that mentions the getBarPositions function
rg "getBarPositions" test/ --type tsRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check the recent commit to see what changed
git show 12a4fc1 --statRepository: recharts/recharts
Length of output: 50374
🏁 Script executed:
# Check what the actual change was in the commit
git show 12a4fc1 src/state/selectors/combiners/combineAllBarPositions.ts | head -100Repository: recharts/recharts
Length of output: 3601
🏁 Script executed:
# Look for tests that might cover BarChart or bar positioning
find test -name "*.spec.ts" -type f | xargs grep -l "BarChart\|bar position\|bandSize" 2>/dev/null | head -5Repository: recharts/recharts
Length of output: 174
🏁 Script executed:
# Search for any existing tests that use combineAllBarPositions or getBarPositions
find . -name "*.spec.ts" -o -name "*.test.ts" | xargs grep -l "barSize\|getBarPositions\|combineAllBarPositions" 2>/dev/nullRepository: recharts/recharts
Length of output: 201
🏁 Script executed:
# Check if there are any test files at all that might import from barSelectors or radialBarSelectors
rg "combineAllBarPositions" test/ --type tsRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Look at the barSelectors file to see how combineAllBarPositions is used
grep -A 5 -B 5 "combineAllBarPositions" src/state/selectors/barSelectors.ts | head -30Repository: recharts/recharts
Length of output: 1240
🏁 Script executed:
# Count test files in the test directory to understand test coverage structure
find test -name "*.spec.ts" -type f | wc -lRepository: recharts/recharts
Length of output: 64
🏁 Script executed:
# Check if there's a test file for barSelectors
ls -la test/state/selectors/ | grep -i barRepository: recharts/recharts
Length of output: 307
🏁 Script executed:
# Check if the existing bar selector tests cover combineAllBarPositions
cat test/state/selectors/barStackSelectors.spec.tsx | head -50Repository: recharts/recharts
Length of output: 1873
🏁 Script executed:
# Check what the radialBarSelectors test file covers
cat test/state/selectors/radialBarSelectors.spec.tsxRepository: recharts/recharts
Length of output: 1391
🏁 Script executed:
# Look at selectBarRectangles test to see if it covers positioning
head -80 test/state/selectors/selectBarRectangles.stackOffset.spec.tsxRepository: recharts/recharts
Length of output: 2506
🏁 Script executed:
# Check if there are integration tests in any chart test files
find test -name "*BarChart*" -type f | head -5Repository: recharts/recharts
Length of output: 241
🏁 Script executed:
# Check the actual test content of selectBarRectangles.stackOffset.spec.tsx to see if it covers fractional cases
grep -n "Math.round\|fractional\|bandSize\|originalSize" test/state/selectors/selectBarRectangles.stackOffset.spec.tsxRepository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Look for any test that specifically tests the combineBarSizeList or bar positioning logic
rg "combineBarSizeList" test/ --type ts -A 3Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# Check if the new file has a corresponding test
ls -la test/state/selectors/combiners/ | grep -i "combineAllBarPositions\|combineBarSizeList"Repository: recharts/recharts
Length of output: 43
🏁 Script executed:
# See the full combineAllBarPositions file to understand what needs testing
cat -n src/state/selectors/combiners/combineAllBarPositions.tsRepository: recharts/recharts
Length of output: 4771
Add unit tests for combineAllBarPositions.ts covering fractional-rounding edge cases.
This new file lacks unit test coverage, which violates the coding guideline requiring 100% unit test coverage for new code in src/**/*.{ts,tsx}. The behavioral changes at lines 42 and 72—switching from bitwise truncation (>> 0) to Math.round()—need explicit test cases:
- Line 42:
(bandSize - sum) / 2with fractional part > 0.5 (e.g., 2.6 rounds to 3, was truncated to 2) - Line 42: Negative offset (when
sum > bandSizeafter fallback) - Line 72:
originalSizewith fractional part > 0.5 (e.g., 24.8 rounds to 25, was truncated to 24)
Also applies to: 72-72
🤖 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 `@src/state/selectors/combiners/combineAllBarPositions.ts` at line 42, Add unit
tests for the combineAllBarPositions function to exercise the new Math.round()
behavior: include cases where (bandSize - sum) / 2 has fractional part > 0.5
(e.g., yields 2.6 -> offset rounds to 3), where offset is negative when sum >
bandSize after fallback, and where originalSize has fractional part > 0.5 (e.g.,
24.8 -> 25); assert the returned positions and sizes match expected rounded
values and the negative-offset handling. Target the
combiners/combineAllBarPositions tests and create focused specs that construct
inputs to hit the offset and originalSize rounding branches and verify outputs.
Ensure tests fail on the old truncation behavior and pass with the current
Math.round() implementation.
| let originalSize = (bandSize - 2 * offset - (len - 1) * realBarGap) / len; | ||
| if (originalSize > 1) { | ||
| originalSize >>= 0; | ||
| originalSize = Math.round(originalSize); | ||
| } | ||
| const size = isWellBehavedNumber(maxBarSize) ? Math.min(originalSize, maxBarSize) : originalSize; |
There was a problem hiding this comment.
Math.round can make total bar area exceed bandSize.
originalSize is derived so that len × originalSize + (len−1) × realBarGap + 2 × offset = bandSize exactly. When the fractional part exceeds 0.5, Math.round rounds up, which means each bar is rendered slightly wider than its slot allows — total overflow up to 0.5 × len pixels. The old >>= 0 (truncation) guaranteed bars never exceeded their allocated space.
For typical charts this is imperceptible (≤ 0.5 px per bar), but at narrow bandSizes or large len values, adjacent bars may visually overlap. If pixel-perfect containment is needed, Math.floor (equivalent to the old truncation for positive values) would be safer:
🔍 Alternative: use `Math.floor` to preserve no-overflow guarantee while still being explicit
- originalSize = Math.round(originalSize);
+ originalSize = Math.floor(originalSize);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let originalSize = (bandSize - 2 * offset - (len - 1) * realBarGap) / len; | |
| if (originalSize > 1) { | |
| originalSize >>= 0; | |
| originalSize = Math.round(originalSize); | |
| } | |
| const size = isWellBehavedNumber(maxBarSize) ? Math.min(originalSize, maxBarSize) : originalSize; | |
| let originalSize = (bandSize - 2 * offset - (len - 1) * realBarGap) / len; | |
| if (originalSize > 1) { | |
| originalSize = Math.floor(originalSize); | |
| } | |
| const size = isWellBehavedNumber(maxBarSize) ? Math.min(originalSize, maxBarSize) : originalSize; |
🤖 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 `@src/state/selectors/combiners/combineAllBarPositions.ts` around lines 70 -
74, The computation of originalSize in combineAllBarPositions.ts currently uses
Math.round which can increase bar widths so total bars plus gaps may exceed
bandSize; replace the Math.round call on originalSize with Math.floor (or
equivalent truncation for positive values) to ensure len * originalSize +
(len−1)*realBarGap + 2*offset never exceeds bandSize, then keep the subsequent
size = isWellBehavedNumber(maxBarSize) ? Math.min(originalSize, maxBarSize) :
originalSize logic unchanged so maxBarSize still caps the floored value.
|
Do we have reproduction, screenshots perhaps, with before & after? Also could we cover this with an eslint rule to prevent regressions? Or unit test? Edit: judging by the failures I guess the test part is covered. |
Bundle ReportChanges will increase total bundle size by 130 bytes (0.0%) ⬆️. 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-umdAssets Changed:
view changes for bundle: recharts/bundle-treeshaking-cartesianAssets Changed:
view changes for bundle: recharts/bundle-es6Assets Changed:
view changes for bundle: recharts/bundle-cjsAssets Changed:
|
…ed spec The PR's bitwise→Math.round change shifts a number of bar positions and sizes by sub-pixel amounts. Updates the hard-coded expected values in the affected suites (BarChart, Bar, XAxis padding/barSize, RadialBar*, itemSorter, barStackSelectors) and adds a targeted spec that exercises combineAllBarPositions for the issue recharts#6115 alignment scenario.
|
@PavelVanecek thanks for the review. Addressing your questions: Reproduction + before/after screenshotsSide-by-side reproduction of issue #6115 (
Before/after as separate images: before.png · after.png Numeric proof from the same render (delta =
That matches @VIDHITTS's analysis on #6115 exactly: with Unit test coverageAdded a focused spec at
I sanity-checked these on ESLint rule to prevent regressions
Other failing checks
CodeRabbit's
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/polar/RadialBar/RadialBar.animation.spec.tsx (1)
255-285: ⚡ Quick winTrack selector churn in the new data-change animation case.
This scenario already uses
createSelectorTestCase, but it only asserts path snapshots. Since this path combines a data-length change with multiple animation steps, it is also a good place to lock down selector call count so rerender regressions do not slip in with future animation changes.As per coding guidelines "Verify the number of selector calls using the spy object from
createSelectorTestCaseto spot unnecessary re-renders and improve performance".🤖 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/polar/RadialBar/RadialBar.animation.spec.tsx` around lines 255 - 285, The test should also verify selector churn: use the spy returned by createSelectorTestCase (available via renderTestCase — e.g., selectorSpy) and assert its call count after the animation completes; add an assertion like expect(selectorSpy).toHaveBeenCalledTimes(5) (one call per animation frame shown in expectAnimatedRadialBarPaths) immediately after the existing path snapshot assertion to lock down unnecessary selector re-renders for the data-length change scenario.
🤖 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.
Inline comments:
In `@test/state/selectors/combiners/combineAllBarPositions.spec.ts`:
- Around line 20-48: Update the test to assert exact centre alignment with the
Scatter midpoint rather than only checking rounded sizes/offsets: in the
single-bar case (calling combineAllBarPositions) assert that
positions[0].position.offset + positions[0].position.size / 2 === bandSize / 2
(use the bandSize variable 91 used in the test), and in the user-sized two-bar
case assert that the group midpoint (positions[0].position.offset + (sum of
barSizes for the group) / 2) === bandSize / 2; keep the existing checks for
size/offset rounding but add these chart-level centre alignment assertions so
the test ensures bar/group centre is derived from the same midpoint as Scatter
and prevents the partial-improvement regression.
---
Nitpick comments:
In `@test/polar/RadialBar/RadialBar.animation.spec.tsx`:
- Around line 255-285: The test should also verify selector churn: use the spy
returned by createSelectorTestCase (available via renderTestCase — e.g.,
selectorSpy) and assert its call count after the animation completes; add an
assertion like expect(selectorSpy).toHaveBeenCalledTimes(5) (one call per
animation frame shown in expectAnimatedRadialBarPaths) immediately after the
existing path snapshot assertion to lock down unnecessary selector re-renders
for the data-length change scenario.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adab2edd-7504-4ffd-ba9c-989177d899a6
📒 Files selected for processing (9)
test/cartesian/Bar/Bar.spec.tsxtest/cartesian/XAxis/XAxis.barSize.spec.tsxtest/cartesian/XAxis/XAxis.padding.spec.tsxtest/chart/BarChart.spec.tsxtest/chart/RadialBarChart.spec.tsxtest/component/Tooltip/itemSorter.spec.tsxtest/polar/RadialBar/RadialBar.animation.spec.tsxtest/state/selectors/barStackSelectors.spec.tsxtest/state/selectors/combiners/combineAllBarPositions.spec.ts
| * For the issue's repro: bandSize=91, barCategoryGap='10%', single bar. | ||
| * - barCategoryGap => offset = 9.1 | ||
| * - originalSize = (91 - 18.2) / 1 = 72.8 -> Math.round -> 73 | ||
| * - bar centre = offset + originalSize/2 = 9.1 + 36.5 = 45.6 | ||
| * - Scatter centre = bandSize / 2 = 45.5 | ||
| * - alignment error: 0.1px (was 0.4px with truncation) | ||
| */ | ||
| it('rounds fractional originalSize instead of truncating (issue #6115)', () => { | ||
| const sizeList: SizeList = [{ stackId: undefined, dataKeys: ['uv'], barSize: undefined }]; | ||
| const positions = combineAllBarPositions(sizeList, undefined, 0, '10%', 91, 91, undefined); | ||
| expect(positions).toBeDefined(); | ||
| expect(positions).toHaveLength(1); | ||
| // Math.round(72.8) === 73, NOT 72 (which is what `>> 0` produced). | ||
| expect(positions![0].position.size).toBe(73); | ||
| }); | ||
|
|
||
| it('rounds the centring offset of user-specified barSizes instead of truncating', () => { | ||
| // if-branch: barSize set by user. | ||
| // bandSize=91, two bars with barSize=20 each, no gap. | ||
| // sum = 40, (bandSize - sum) / 2 = 25.5 -> Math.round -> 26 (was 25 via `>> 0`) | ||
| const sizeList: SizeList = [ | ||
| { stackId: undefined, dataKeys: ['a'], barSize: 20 }, | ||
| { stackId: undefined, dataKeys: ['b'], barSize: 20 }, | ||
| ]; | ||
| const positions = combineAllBarPositions(sizeList, undefined, 0, 0, 91, 91, undefined); | ||
| expect(positions).toBeDefined(); | ||
| expect(positions).toHaveLength(2); | ||
| expect(positions![0].position.offset).toBe(26); | ||
| }); |
There was a problem hiding this comment.
This regression test still locks in a non-zero Bar/Scatter mismatch.
The first case’s own math leaves the bar centre at 45.6 while Scatter stays at 45.5, and the user-sized case has the same problem (26 + 20 = 46 for a 91px band). So the new assertions codify a partial improvement, not the exact alignment promised by #6115. Please make the bar/group centre derive from the same midpoint as Scatter, and add a chart-level assertion for the repro so the real contract is protected.
🤖 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/state/selectors/combiners/combineAllBarPositions.spec.ts` around lines
20 - 48, Update the test to assert exact centre alignment with the Scatter
midpoint rather than only checking rounded sizes/offsets: in the single-bar case
(calling combineAllBarPositions) assert that positions[0].position.offset +
positions[0].position.size / 2 === bandSize / 2 (use the bandSize variable 91
used in the test), and in the user-sized two-bar case assert that the group
midpoint (positions[0].position.offset + (sum of barSizes for the group) / 2)
=== bandSize / 2; keep the existing checks for size/offset rounding but add
these chart-level centre alignment assertions so the test ensures bar/group
centre is derived from the same midpoint as Scatter and prevents the
partial-improvement regression.
- Add as-cast on selectBarRectangles assertions where the runtime rectangle data spreads original chart data fields (uv/pv/xKey/etc.) that aren't part of the BarRectangleItem type. - Add explicit stackId: undefined to selectAllBarPositions assertions so the literal satisfies BarWithPosition. - Re-run prettier on all touched test files to clear the eslint prettier/prettier errors (whitespace + formatting only).
|
Pushed 3ef336b which clears the Unit Tests + Eslint/Typecheck failures from the previous run.
VR Tests are still red, and I haven't attempted to update the baselines. The diffs are exactly what you'd expect from If the maintainers are happy with the change, |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/state/selectors/barStackSelectors.spec.tsx (1)
48-520: ⚡ Quick winAdd selector call-count assertions in these updated selector tests.
The new assertions validate geometry well, but they still miss recomputation guards. Please also assert
spycall counts for the updatedselectBarRectanglesandselectStackRectscases.As per coding guidelines, "Verify the number of selector calls using the spy object from
createSelectorTestCaseto spot unnecessary re-renders and improve performance."🤖 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/state/selectors/barStackSelectors.spec.tsx` around lines 48 - 520, Add assertions that verify the selector spy call count after the geometry assertions: in the tests that call selectBarRectangles (the two cases using renderTestCase and returning { spy }) and the selectStackRects test, add expect(spy).toHaveBeenCalledTimes(1) (or the appropriate expected count) immediately after expectLastCalledWith to ensure the selector was invoked only once; reference the spy returned from renderTestCase and the selectors selectBarRectangles and selectStackRects when adding these assertions.
🤖 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/state/selectors/barStackSelectors.spec.tsx`:
- Around line 48-520: Add assertions that verify the selector spy call count
after the geometry assertions: in the tests that call selectBarRectangles (the
two cases using renderTestCase and returning { spy }) and the selectStackRects
test, add expect(spy).toHaveBeenCalledTimes(1) (or the appropriate expected
count) immediately after expectLastCalledWith to ensure the selector was invoked
only once; reference the spy returned from renderTestCase and the selectors
selectBarRectangles and selectStackRects when adding these assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6bd964b-2063-4737-bab1-c6563d52a40b
📒 Files selected for processing (7)
test/cartesian/Bar/Bar.spec.tsxtest/cartesian/XAxis/XAxis.barSize.spec.tsxtest/cartesian/XAxis/XAxis.padding.spec.tsxtest/chart/BarChart.spec.tsxtest/chart/RadialBarChart.spec.tsxtest/component/Tooltip/itemSorter.spec.tsxtest/state/selectors/barStackSelectors.spec.tsx
|
Hi @PavelVanecek, the workflow run on commit 3ef336b is sitting at |
|
running |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7297 +/- ##
==========================================
- Coverage 89.05% 89.05% -0.01%
==========================================
Files 541 541
Lines 41101 41100 -1
Branches 5565 5564 -1
==========================================
- Hits 36601 36600 -1
Misses 4492 4492
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for approving the workflows, @ckifer. CI is now running on commit 3ef336b, and the long-tail integration tests (React 16/17/18/19 across npm/pnpm/yarn) are already green; Eslint+Typecheck and Omnidoc passed too. Only Unit Tests, VR Tests, and Website Build are still in flight. @PavelVanecek, the screenshots posted yesterday cover the Math.round case you asked about. I'll keep an eye on the remaining checks and push fixes promptly if any of them surface real issues. Would appreciate a review once CI settles. |
The Math.round change in combineAllBarPositions.ts shifts bar positioning by sub-pixel amounts (1 - 2 px in some places). Refresh the affected VR snapshots so the chromium / firefox / webkit runs pass on CI.
|
Pushed 5f018b0 refreshing the VR snapshots. The Math.round change shifted bar pixels by 1 - 2 px, which affected 102 PNGs across BarChart, RadialBarChart, ComposedChart and a few API-example tests; the remaining 876 snapshots regenerated to byte-identical content (deterministic docker rendering), so the diff is scoped only to bar-related views. @ckifer / @PavelVanecek, this push is sitting at |
3 participants
Problem
In ComposedChart, Bar and Scatter components can have misaligned X coordinates due to inconsistent use of the bitwise OR (>> 0) operator for truncating fractional pixel values.
Root cause
In combineAllBarPositions.ts, two places use >> 0 (bitwise right-shift) to truncate a float to an integer:
` s
// Case 1 – user-set barSize path
const offset = ((bandSize - sum) / 2) >> 0; // truncates toward zero
// Case 2 – auto-size path
if (originalSize > 1) {
originalSize >>= 0; // truncates toward zero
}
`
Fix
Replace both truncations with Math.round(), which rounds to the nearest integer consistently with how other positional values are computed:
` s
// Case 1
const offset = Math.round((bandSize - sum) / 2);
// Case 2
if (originalSize > 1) {
originalSize = Math.round(originalSize);
}
`
Fixes #6115
Summary by CodeRabbit
Bug Fixes
Tests