feat: fix known download anomalies with interpolation#1636
feat: fix known download anomalies with interpolation#1636graphieros merged 19 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! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request introduces infrastructure for cleaning npm download data displayed in charts. New utilities detect and correct known download anomalies through linear interpolation, and apply data smoothing via moving averages and exponential filtering. The TrendsChart and WeeklyDownloadStats components integrate blocklist-based and general corrections into their data pipelines. Settings are extended to include correction parameters (averageWindow, smoothingTau, anomaliesFixed), and corresponding UI controls are added to the downloads metric view. Translations support the new interface elements across localisation files. 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.
🧹 Nitpick comments (2)
app/components/Package/TrendsChart.vue (2)
1720-1734: Add ARIA attributes for accessible collapsible section.The toggle button should include
aria-expandedandaria-controlsattributes for screen reader users to understand the collapsible state and relationship.♻️ Suggested accessibility improvement
<button type="button" class="self-start flex items-center gap-1 text-2xs font-mono text-fg-subtle hover:text-fg transition-colors" + :aria-expanded="showFilterControls" + aria-controls="download-filter-controls" `@click`="showFilterControls = !showFilterControls" > <span class="w-3.5 h-3.5 transition-transform" :class="showFilterControls ? 'i-lucide:chevron-down' : 'i-lucide:chevron-right'" aria-hidden="true" /> {{ $t('package.trends.filters') }} </button> - <div v-if="showFilterControls" class="flex items-end gap-3"> + <div v-if="showFilterControls" id="download-filter-controls" class="flex items-end gap-3">
1763-1772: Consider UX feedback from PR discussion.Per the PR comments, graphieros suggested adding an info icon with a tooltip explaining the "Anomalies fixed" checkbox behaviour, and potentially showing this checkbox only when the database contains corrections for the current package. This would improve clarity for users unfamiliar with the feature.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
app/components/Package/TrendsChart.vueapp/components/Package/WeeklyDownloadStats.vuei18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.json
🚧 Files skipped from review as they are similar to previous changes (5)
- app/components/Package/WeeklyDownloadStats.vue
- i18n/schema.json
- lunaria/files/en-GB.json
- lunaria/files/en-US.json
- i18n/locales/en.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lunaria/files/en-GB.json (1)
381-384: Consider more descriptive labels per PR feedback.The translations are functional, but the PR comments from graphieros suggest more explicit labels that could improve user understanding:
Current Suggested "Filters" "Data correction controls" "Average" "Average window" "Smoothing" "Noise reduction" These suggestions aim to make the controls' purpose clearer to users unfamiliar with the chart filtering feature. Consider whether adopting some or all of these would benefit the UX.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
i18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.json
🚧 Files skipped from review as they are similar to previous changes (2)
- lunaria/files/en-US.json
- i18n/locales/en.json
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
this looks amazing!
@graphieros I'll let you press the merge button when you're happy 🙏
|
I'll try to address comments tomorrow morning 🤞 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
app/utils/download-anomalies.ts (1)
15-26: Avoidanyin the anomaly correction path.
Record<string, any>and broad casts here bypass type safety exactly where granularity-specific date fields are resolved.Typed alternative (outline)
-import type { ChartTimeGranularity, EvolutionData } from '~/types/chart' +import type { + ChartTimeGranularity, + DailyDataPoint, + EvolutionData, + MonthlyDataPoint, + WeeklyDataPoint, + YearlyDataPoint, +} from '~/types/chart' + +type EvolutionPoint = DailyDataPoint | WeeklyDataPoint | MonthlyDataPoint | YearlyDataPoint -function getDateString(point: Record<string, any>, granularity: ChartTimeGranularity): string { +function getDateString(point: EvolutionPoint, granularity: ChartTimeGranularity): string { switch (granularity) { case 'daily': - return point.day + return 'day' in point ? point.day : '' case 'weekly': - return point.weekStart + return 'weekStart' in point ? point.weekStart : '' case 'monthly': - return `${point.month}-01` + return 'month' in point ? `${point.month}-01` : '' case 'yearly': - return `${point.year}-01-01` + return 'year' in point ? `${point.year}-01-01` : '' } }As per coding guidelines, "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
Also applies to: 88-89
app/utils/chart-data-correction.ts (1)
9-53: Add finite-value guards to chart filter parameters.The
movingAverageandsmoothingfunctions lack validation for non-finite inputs (NaN, Infinity). IfhalfWindowortaubecome non-finite via corrupted localStorage or an upstream bug, they propagate NaN through the entire corrected series, silently corrupting chart output. The default values are safe, but there is no validation in the settings chain to prevent this edge case.Suggested hardening
export function movingAverage<T extends { value: number }>(data: T[], halfWindow: number): T[] { - if (halfWindow <= 0 || data.length < 3) return data + const safeHalfWindow = Number.isFinite(halfWindow) ? Math.max(0, Math.floor(halfWindow)) : 0 + if (safeHalfWindow <= 0 || data.length < 3) return data const n = data.length @@ for (let i = 0; i < n; i++) { - const lo = Math.max(0, i - halfWindow) + const lo = Math.max(0, i - safeHalfWindow) @@ for (let i = 0; i < n; i++) { - const hi = Math.min(n - 1, i + halfWindow) + const hi = Math.min(n - 1, i + safeHalfWindow)export function smoothing<T extends { value: number }>(data: T[], tau: number): T[] { - if (tau <= 0 || data.length < 3) return data + const safeTau = Number.isFinite(tau) ? Math.max(0, tau) : 0 + if (safeTau <= 0 || data.length < 3) return data - const alpha = 1 / (1 + tau) + const alpha = 1 / (1 + safeTau)
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/Package/TrendsChart.vueapp/components/Package/WeeklyDownloadStats.vueapp/utils/chart-data-correction.tsapp/utils/download-anomalies.tsi18n/locales/en.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.json
🚧 Files skipped from review as they are similar to previous changes (1)
- lunaria/files/en-US.json
graphieros
left a comment
graphieros
left a comment
There was a problem hiding this comment.
Looks awesome to me !
You can probably address the coderabbit critical notification.
One last detail, regarding the tooltip content when the chart is used for multiple series in the compare page, the translation could use plural management.
haha, I didn't know this chart! I'll look at the rabbit as well. (I have to say that this is a bit overwhelming) |
3 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?!