Uh oh!
There was an error while loading. Please reload this page.
fix: align weekly chart buckets from end to match npm downloads - #2052
Conversation
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThis pull request extracts date utilities into Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 Migrating from UI to YAML configuration.Use the |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/utils/chart-data-buckets.ts (1)
94-107: Consider defensive handling for malformed date strings.The type assertion
as [number, number]on line 95 assumes the split will always yield two numeric elements. While this is safe for well-formed ISO dates from internal sources, a malformed string would produceNaNvalues.Given this is internal utility code processing known-format npm API data, this is acceptable, but worth noting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 534a7ede-1f37-4844-abb4-17238285f660
📒 Files selected for processing (8)
app/components/Package/TrendsChart.vueapp/composables/useCharts.tsapp/utils/chart-data-buckets.tsapp/utils/date.tstest/unit/app/composables/use-charts.spec.tstest/unit/app/utils/chart-data-buckets.spec.tstest/unit/app/utils/date.spec.tsvitest.config.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 74616625-2d65-438e-8ed0-2a30b6a6275a
📒 Files selected for processing (2)
app/components/Package/TrendsChart.vueapp/composables/useCharts.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/components/Package/TrendsChart.vue
| const endDateOnly = toDateOnly(evolutionOptions.endDate) | ||
| const end = endDateOnly ? parseIsoDateOnly(endDateOnly) : yesterday | ||
| const end = endDateOnly ? parseIsoDate(endDateOnly) : yesterday | ||
| const startDateOnly = toDateOnly(evolutionOptions.startDate) | ||
| if (startDateOnly) { | ||
| const start = parseIsoDateOnly(startDateOnly) | ||
| const start = parseIsoDate(startDateOnly) | ||
| return { start, end } |
There was a problem hiding this comment.
Handle invalid calendar dates before using parsed values.
toDateOnly checks format only. A value like 2026-02-31 still passes, then parseIsoDate can produce an invalid date and propagate NaN behaviour. Please guard parsed dates and fall back to defaults.
Proposed fix
const endDateOnly = toDateOnly(evolutionOptions.endDate)
- const end = endDateOnly ? parseIsoDate(endDateOnly) : yesterday+ const parsedEnd = endDateOnly ? parseIsoDate(endDateOnly) : null+ const end = parsedEnd && !Number.isNaN(parsedEnd.getTime()) ? parsedEnd : yesterday
const startDateOnly = toDateOnly(evolutionOptions.startDate)
if (startDateOnly) {
- const start = parseIsoDate(startDateOnly)- return { start, end }+ const parsedStart = parseIsoDate(startDateOnly)+ if (!Number.isNaN(parsedStart.getTime())) {+ return { start: parsedStart, end }+ }
}As per coding guidelines "Use error handling patterns consistently".
📝 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.
| constendDateOnly=toDateOnly(evolutionOptions.endDate) | |
| constend=endDateOnly ? parseIsoDateOnly(endDateOnly) : yesterday | |
| constend=endDateOnly ? parseIsoDate(endDateOnly) : yesterday | |
| conststartDateOnly=toDateOnly(evolutionOptions.startDate) | |
| if(startDateOnly){ | |
| conststart=parseIsoDateOnly(startDateOnly) | |
| conststart=parseIsoDate(startDateOnly) | |
| return{ start, end } | |
| constendDateOnly=toDateOnly(evolutionOptions.endDate) | |
| constparsedEnd=endDateOnly ? parseIsoDate(endDateOnly) : null | |
| constend=parsedEnd&&!Number.isNaN(parsedEnd.getTime()) ? parsedEnd : yesterday | |
| conststartDateOnly=toDateOnly(evolutionOptions.startDate) | |
| if(startDateOnly){ | |
| constparsedStart=parseIsoDate(startDateOnly) | |
| if(!Number.isNaN(parsedStart.getTime())){ | |
| return{start: parsedStart, end } | |
| } | |
| } |
graphieros
left a comment
There was a problem hiding this comment.
This is great !
Just one thing, the Known Anomalies checkbox is now togglable when there are no recorded corrections. I think this might be a regression (?).
jycouet
commented
Mar 13, 2026
Haaa, yes I did this change. It's actually bugging me that sometime I can click and sometime not. We have the tooltip on top to know if it will have wrong data, so having this conditional doesn't feel correct. It's a bit like "disabling smooting if if's the same value" What do you think ? |
As a user, if I can click but it does not change anything, it might feel as a bug. I wonder if the tooltip, however, could also say: 'No recorded anomalies' ? |
jycouet
commented
Mar 13, 2026
On a flat chart (a lot of small libraries with almost no download) all sliders have also no affect on the trend. I have a half strong opinion on this 😜 |
Yeah, you're right about the sliders... (And I prefer that we decide together ;) |
jycouet
commented
Mar 13, 2026
) <- closing your open parentesis ;) Me too, I would like to decide together, just wanted to say that we could remove it. I would not have hard feelings, you will still be a great OSS friend 😉 BTW we have to meet one day !🧡 Your suggestion just before looks perfect to me. I'll update that after coffee |
graphieros
commented
Mar 13, 2026
Yes, you must let know the next time you're in Paris ^^ |
graphieros
commented
Mar 13, 2026
I'm sorry @jycouet I edited your previous message instead of answering it. |
Uh oh!
There was an error while loading. Please reload this page.
…ix/issue-2044 * 'fix/issue-2044' of github.com:eryue0220/npmx.dev: fix: align weekly chart buckets from end to match npm downloads (npmx-dev#2052)
Closes#2041
Problem
Weekly buckets were aligned from the start of the date range, so the last bucket often covered a different 7-day window than the npm rolling week.
Solution
app/utils/chart-data-buckets.tswithfillPartialBucketfor the (now partial) first bucketapp/utils/date.ts(parseIsoDate,toIsoDate,addDays, etc.)Verification
On
/compare?packages=svelte,@sveltejs/kit, table and chart now show identical values (3.3M / 1.4M).