fix: align weekly chart buckets from end to match npm downloads#2052
fix: align weekly chart buckets from end to match npm downloads#2052graphieros merged 7 commits intonpmx-dev:mainfrom
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 |
![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/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.
| 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 } | |
| const endDateOnly = toDateOnly(evolutionOptions.endDate) | |
| const parsedEnd = endDateOnly ? parseIsoDate(endDateOnly) : null | |
| const end = parsedEnd && !Number.isNaN(parsedEnd.getTime()) ? parsedEnd : yesterday | |
| const startDateOnly = toDateOnly(evolutionOptions.startDate) | |
| if (startDateOnly) { | |
| const parsedStart = parseIsoDate(startDateOnly) | |
| if (!Number.isNaN(parsedStart.getTime())) { | |
| return { start: parsedStart, end } | |
| } | |
| } |
graphieros
left a comment
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 (?).
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' ? |
|
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 ;) |
) <- 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 |
Yes, you must let know the next time you're in Paris ^^ |
|
I'm sorry @jycouet I edited your previous message instead of answering it. |
…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)
2 participants
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).