fix: round chart numbers in applyDataCorrection for consistent rounding#2556
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughChart data values are now ceiling-rounded after smoothing; several components stopped forcing zero fractional digits in number formatting, and a tooltip formatter was changed to use the raw numeric value (no Math.round). Changes
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
|
Hello! Thank you for opening your first PR to npmx, @ulrichstark! 🚀 Here’s what will happen next:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Package/WeeklyDownloadStats.vue (1)
204-204: ConsiderMath.roundinstead ofMath.ceilfor display values.Using
Math.ceilsystematically biases displayed values upwards (e.g.,34,803,664.957→34,803,665happens to match, but34,803,664.1would also become34,803,665). Since the goal is to hide fractional artefacts from smoothing/moving-average post-processing rather than to report an upper bound,Math.roundwould be more faithful to the underlying data.♻️ Proposed refinement
- value: Math.ceil(d?.value ?? 0), + value: Math.round(d?.value ?? 0),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/components/Package/WeeklyDownloadStats.vue` at line 204, In WeeklyDownloadStats.vue locate the mapping that sets value: Math.ceil(d?.value ?? 0) and replace Math.ceil with Math.round so the display values use rounding instead of always rounding up (i.e., change to value: Math.round(d?.value ?? 0)); keep the nullish fallback (d?.value ?? 0) intact so only the rounding strategy changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/components/Package/WeeklyDownloadStats.vue`:
- Line 204: In WeeklyDownloadStats.vue locate the mapping that sets value:
Math.ceil(d?.value ?? 0) and replace Math.ceil with Math.round so the display
values use rounding instead of always rounding up (i.e., change to value:
Math.round(d?.value ?? 0)); keep the nullish fallback (d?.value ?? 0) intact so
only the rounding strategy changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e9bbf48e-3e7e-4aff-84b5-65d5d31cf4e3
📒 Files selected for processing (1)
app/components/Package/WeeklyDownloadStats.vue
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…ing WeeklyDownloadStats" This reverts commit 47bd16e.
|
Hey @graphieros, thanks for the prompt review! Just reverted my first commit and made the change in |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/utils/chart-data-correction.ts (2)
85-95: Document the new rounding step in the function contract.
applyDataCorrectionnow also ceilings values, but the JSDoc still describes only moving average and smoothing. Please update it so callers know the returned values are integer-rounded.Suggested documentation update
/** - * Applies moving average then smoothing in sequence. + * Applies moving average, smoothing, then ceiling-rounds values in sequence. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/chart-data-correction.ts` around lines 85 - 95, Update the JSDoc for applyDataCorrection to reflect the new final rounding step: mention that the function applies a moving average (movingAverage), then smoothing (smoothing), and finally ceilings values to integers (Math.ceil) on the value field of each returned item; state that the returned array preserves items of type T but with value rounded up to an integer. Edit the comment block immediately above the export function applyDataCorrection to include this behaviour and an explicit note that callers should expect integer-rounded values.
95-95: Add or update a test that covers the final ceiling step.The new behaviour is central to this fix, but the related
applyDataCorrectionmock intest/unit/app/utils/chart-data-prediction.spec.tsreturns data unchanged, so pipeline tests will not catch regressions in this integer-output contract.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/utils/chart-data-correction.ts` at line 95, The final Math.ceil rounding step (result = result.map(d => ({ ...d, value: Math.ceil(d.value) }))) isn't covered by tests because the applyDataCorrection mock in test/unit/app/utils/chart-data-prediction.spec.ts returns data unchanged; add or update a unit test to call applyDataCorrection (or run the full pipeline that uses applyDataCorrection) and assert that output values are integers (use Math.floor/=== or Number.isInteger) and reflect ceiling behaviour for fractional inputs, or change the mock to mimic the ceiling transformation so pipeline tests will fail if the ceiling step is removed; reference applyDataCorrection and the mapping line to locate where to assert the integer-output contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/utils/chart-data-correction.ts`:
- Around line 85-95: Update the JSDoc for applyDataCorrection to reflect the new
final rounding step: mention that the function applies a moving average
(movingAverage), then smoothing (smoothing), and finally ceilings values to
integers (Math.ceil) on the value field of each returned item; state that the
returned array preserves items of type T but with value rounded up to an
integer. Edit the comment block immediately above the export function
applyDataCorrection to include this behaviour and an explicit note that callers
should expect integer-rounded values.
- Line 95: The final Math.ceil rounding step (result = result.map(d => ({ ...d,
value: Math.ceil(d.value) }))) isn't covered by tests because the
applyDataCorrection mock in test/unit/app/utils/chart-data-prediction.spec.ts
returns data unchanged; add or update a unit test to call applyDataCorrection
(or run the full pipeline that uses applyDataCorrection) and assert that output
values are integers (use Math.floor/=== or Number.isInteger) and reflect ceiling
behaviour for fractional inputs, or change the mock to mimic the ceiling
transformation so pipeline tests will fail if the ceiling step is removed;
reference applyDataCorrection and the mapping line to locate where to assert the
integer-output contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9631dd62-a3c8-4154-8fab-a6551591f68d
📒 Files selected for processing (1)
app/utils/chart-data-correction.ts
There was a problem hiding this comment.
Perfect.
Prior changes applied higher up need to be reverted / adapted, now that the rounding is rooted:
- TrendsChart.vue : https://github.com/npmx-dev/npmx.dev/pull/2550/changes : the rounding can be removed
- SplitSparkline.vue : the
maximumFractionDigitsparam can be removed from the composable call - WeeklyDownloadStats.vue: the
maximumFractionDigitsparam can be removed from the composable call
ulrichstark
changed the title
applyDataCorrection for consistent rounding
Thanks for assisting on this change! Does this mean the original issue way already fixed but not yet released to production? This would also explain that I failed to reproduce it locally, but could on production and therefore could only proof it using the devtools on the production site. |
ulrichstark
deleted the
fix--round-downloads-to-avoid-fractions-showing-up-when-hovering-WeeklyDownloadStats
branch
Yes, it will be in production with the next release |
Ahhh perfect. Thank you very much once again!! |
2 participants
🔗 Linked issue
There's not.
This is a pretty straightforward and tiny fix.
🧭 Context
The weekly download charts shows numbers with fraction when hovering over it and smoothing is enabled:
Here you can see the exact number of the screenshot coming in to the code location I'm changing with this PR:
📚 Description
Download numbers are now rounded up before showing them to the user.
I can also do the change in
applyDataCorrectioninstead if that's preferred.