feat: fix known download anomalies with interpolation#1636
feat: fix known download anomalies with interpolation#1636jycouet wants to merge 4 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! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds configurable download-chart filtering and anomaly mitigation across the UI and data utilities. New smoothing and moving-average functions and a download-anomaly interpolation filter are implemented; settings gain a Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
app/utils/chart-filters.ts (1)
9-10: No-op paths return the original array reference — document the contract.Both
movingAverageandsmoothingreturn the inputdataarray unchanged when the guard condition fires. This means any caller that mutates the returned array will also mutate the original. Current callers in the pipeline are safe, but this is a non-obvious contract that could cause subtle bugs if new callers are added without reading the source.♻️ Suggested: add a clarifying note to the JSDoc
/** * Bidirectional moving average. … * First and last points are preserved. * * `@param` halfWindow - number of points on each side (0 = disabled) + * `@returns` A new array when filtering is applied; the original `data` reference when the guard fires (no-op). */ export function movingAverage<T extends { value: number }>(data: T[], halfWindow: number): T[] {/** * Forward-backward exponential smoothing (zero-phase). … * First and last points are preserved. * * `@param` tau - time constant (0 = disabled, higher = smoother) + * `@returns` A new array when filtering is applied; the original `data` reference when the guard fires (no-op). */ export function smoothing<T extends { value: number }>(data: T[], tau: number): T[] {app/utils/download-anomalies.ts (2)
15-26:Record<string, any>silently hides missing-field mismatches.
getDateStringaccessespoint.day,point.weekStart,point.month, andpoint.yearwithout any guard. If a data point is missing the expected field, the function returnsundefinedcoerced to"undefined", which silently produces a no-match rather than a loud error. A discriminated-union parameter type (or at minimum an intersection with{ value: number }) would surface the issue at the call site.♻️ Suggested: narrow the parameter type
-function getDateString(point: Record<string, any>, granularity: ChartTimeGranularity): string { +function getDateString( + point: Record<string, unknown>, + granularity: ChartTimeGranularity, +): string { switch (granularity) { case 'daily': - return point.day + return String(point.day ?? '') case 'weekly': - return point.weekStart + return String(point.weekStart ?? '') case 'monthly': - return `${point.month}-01` + return `${String(point.month ?? '')}-01` case 'yearly': - return `${point.year}-01-01` + return `${String(point.year ?? '')}-01-01` } }
55-66: Monthly/yearly scaling uses calendar approximations — worth documenting.
Math.round((weeklyValue / 7) * 30)treats every month as exactly 30 days and* 365treats every year as 365 days (ignoring leap years). For anomaly correction purposes these approximations are reasonable, but a brief inline comment would make the intent clear to future contributors who might otherwise reach for a calendar-aware alternative.♻️ Suggested: add clarifying comments
case 'monthly': - return Math.round((weeklyValue / 7) * 30) + return Math.round((weeklyValue / 7) * 30) // approximate: treats all months as 30 days case 'yearly': - return Math.round((weeklyValue / 7) * 365) + return Math.round((weeklyValue / 7) * 365) // approximate: treats all years as 365 daysapp/components/Package/TrendsChart.vue (2)
1614-1615:isDownloadsMetricis declared 670+ lines after its first use — consider hoisting.
isDownloadsMetricis referenced inside theeffectiveDataSingle(line 946) andchartData(line 990) computed properties, but is not declared until line 1614. Vue's lazy computed evaluation means this works at runtime (no TDZ crash), but the declaration order makes the dependency chain very hard to follow and is contrary to the convention in the rest of the file where helpers are declared before their consumers.♻️ Suggested: hoist the declaration above `effectiveDataSingle`
+const isDownloadsMetric = computed(() => selectedMetric.value === 'downloads') +const showFilterControls = shallowRef(false) + const effectiveDataSingle = computed<EvolutionData>(() => {…and remove the duplicate declarations at lines 1614–1615.
946-955: Download-filter logic is duplicated betweeneffectiveDataSingleandchartData— extract a helper.The
anomaliesFixed→applyBlocklistFilter→applyDownloadFilterchain appears verbatim in both computed properties. Any future change to the pipeline (e.g. adding a new filter step) must be applied in both places.♻️ Suggested: extract a shared helper
+function applyDownloadsFilters( + data: EvolutionData, + pkg: string, + granularity: ChartTimeGranularity, +): EvolutionData { + let result = data + if (settings.value.chartFilter.anomaliesFixed) { + result = applyBlocklistFilter(result, pkg, granularity) + } + return applyDownloadFilter( + result as Array<{ value: number }>, + settings.value.chartFilter, + ) as EvolutionData +}Then in
effectiveDataSingle:- if (isDownloadsMetric.value && data.length) { - const pkg = effectivePackageNames.value[0] ?? props.packageName ?? '' - if (settings.value.chartFilter.anomaliesFixed) { - data = applyBlocklistFilter(data, pkg, displayedGranularity.value) - } - return applyDownloadFilter( - data as Array<{ value: number }>, - settings.value.chartFilter, - ) as EvolutionData - } + if (isDownloadsMetric.value && data.length) { + const pkg = effectivePackageNames.value[0] ?? props.packageName ?? '' + return applyDownloadsFilters(data, pkg, displayedGranularity.value) + }And in
chartData:- if (isDownloadsMetric.value && data.length) { - if (settings.value.chartFilter.anomaliesFixed) { - data = applyBlocklistFilter(data, pkg, granularity) - } - data = applyDownloadFilter( - data as Array<{ value: number }>, - settings.value.chartFilter, - ) as EvolutionData - } + if (isDownloadsMetric.value && data.length) { + data = applyDownloadsFilters(data, pkg, granularity) + }Also applies to: 990-999
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/Package/TrendsChart.vueapp/components/Package/WeeklyDownloadStats.vueapp/composables/useSettings.tsapp/utils/chart-filters.tsapp/utils/download-anomalies.data.tsapp/utils/download-anomalies.ts
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
This looks awesome :) A few remarks before I take a look at the code, regarding the labels for the range inputs.
Also, perhaps an info icon with a tooltip could explain what the checkbox does (and only showing the checkbox when the 'database' contains corrections ?) Finally, I could not test on a newly created package that has only one download datapoint. |
There was a problem hiding this comment.
I would suggest renaming this file to something like 'chart-data-correction'
| /** | ||
| * Applies moving average then smoothing in sequence. | ||
| */ | ||
| export function applyDownloadFilter<T extends { value: number }>( |
There was a problem hiding this comment.
I would suggest renaming this function to something like applyDataCorrection
| } | ||
| } | ||
|
|
||
| export function applyBlocklistFilter( |
There was a problem hiding this comment.
I would suggest renaming this function to something like applyBlocklistCorrection
| () => loadWeeklyDownloads(), | ||
| ) | ||
|
|
||
| const filteredDownloads = computed<WeeklyDataPoint[]>(() => { |
There was a problem hiding this comment.
I would suggest renaming this var to something like correctedDownloads
| data: EvolutionData, | ||
| packageName: string, | ||
| granularity: ChartTimeGranularity, | ||
| ): EvolutionData { |
There was a problem hiding this comment.
It would probably be better to have the 3 params as an object, to have call sites more readable
2 participants
🔗 Linked issue
Closes #1241
🧭 Context
Having nicer charts in general without data going off!
Right now, this PR is the Option 4 with these defaults:
FYI, I split commits to be able to go back to other options, if you want to try on some packages :)
📚 Description
1/ Hampel
2026.02.24.-.Hampel.mp4
Was good, be it's a lot of variables... And it's hard to get all situations correct
2/ Outlier Sensitivity
2026.02.24.-.Outlier.Sensitivity.mp4
Same thing... I tried with some linear reduction & some tweaks to keep first an last point. Good for some... not the bast for others...
3/ 2 Passes Avg or Smoothing
2026.02.24.-.2.Passes.avg.or.Smoothing.mp4
Std Average / Smoothing are not so good because they change the start or end. But here in the 2 passes process we minimise this effect.
4/ 2 Passes Avg or Smoothing & Manual tuning
2026.02.24.-.Manu.mp4
This is actually the best! Manually declaring where it's wrong. In the vite example it's obvious, so we can just add it. And the community is soooo into it that we could encourage people to enter things there (we could even offer an API at some point).
Keeping the option for Avg & Smooth to have a good mix.
What do you think?!