Open
Conversation
# Conflicts: # nuxt.config.ts # pnpm-lock.yaml
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
❌ 37 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
Contributor
|
Great work! I kinda miss the circle in the background 🔵 |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (14)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis pull request performs a comprehensive migration of the OG image generation system from Nuxt OG Image v5 to v6. The migration introduces a new API ( Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
app/components/OgImage/Page.takumi.vue (1)
14-19: Consider providing a fallback fortitlewhen undefined.The
titleprop is optional, but it's rendered directly without a fallback. Iftitleis undefined, this will render an empty div. Consider whether a default value or conditional rendering would be more appropriate.💡 Optional: Add fallback or conditional rendering
- <div - class="lg:text-7xl text-5xl tracking-tighter font-mono leading-none" - :style="{ lineClamp: 1, textOverflow: 'ellipsis' }" - > - {{ title }} - </div> + <div + v-if="title" + class="lg:text-7xl text-5xl tracking-tighter font-mono leading-none" + :style="{ lineClamp: 1, textOverflow: 'ellipsis' }" + > + {{ title }} + </div>app/components/OgImage/Profile.takumi.vue (1)
18-24: Consider filtering empty strings for robustness.The
getInitialsfunction works correctly for typical input, but could be more explicit when handling edge cases with multiple spaces or empty segments.♻️ Optional improvement for clarity
const getInitials = (name: string) => name .split(' ') + .filter(Boolean) .map(n => n[0]) .join('') .toUpperCase() .slice(0, 2)app/components/OgImage/Package.takumi.vue (2)
109-114: Consider extractingsortedhelper to module scope.The linter flags that
sorteddoesn't capture variables fromfetchCodeTree. Extracting it to the module-level<script lang="ts">block (alongsideKIND_ICONS) would address the warning and make it reusable.♻️ Proposed refactor
Add to the first
<script lang="ts">block:function sortedNodes(nodes: JsDelivrFileNode[]) { return [...nodes].sort((a, b) => { if (a.type !== b.type) return a.type === 'directory' ? -1 : 1 return a.name.localeCompare(b.name) }) }Then use
sortedNodesinfetchCodeTreeinstead of the localsorted.
344-375: Consider using dynamic component for icon rendering.The repeated
v-if/v-else-ifchain for icons is verbose. Since the icons are already stored as class strings inrow.icon, you could use a single element with a dynamic class binding.♻️ Proposed simplification
<span v-if="row.kind === 'symbol' || row.icon" class="w-5 h-5 shrink-0 text-fg-muted force-mr-1.5" :class="row.icon || 'i-lucide:code'" />This replaces the entire v-if/v-else-if chain with a single element that applies the icon class dynamically.
app/pages/~[username]/orgs.vue (1)
111-124: Consider using i18n for OG image description.The description callback uses hardcoded English strings ("organization"/"organizations") while the rest of the component uses
$t(). For consistency, consider using i18n keys with pluralisation.♻️ Proposed change
defineOgImage( 'Page.takumi', { title: () => `@${username.value}`, description: () => { - if (isLoading.value) return 'npm organizations' - if (orgs.value.length === 0) return 'No organizations found' - - const count = orgs.value.length - return `${count} ${count === 1 ? 'organization' : 'organizations'}` + if (isLoading.value) return $t('user.orgs_page.og_loading') + if (orgs.value.length === 0) return $t('user.orgs_page.og_empty') + return $t('user.orgs_page.og_count', { count: orgs.value.length }, orgs.value.length) }, }, { alt: () => `Organizations for @${username.value} on npmx` }, )This requires adding the corresponding i18n keys.
test/fixtures/mock-routes.cjs (1)
184-206: Well-implemented deterministic download data generation.The mock correctly generates realistic-looking sparkline data using a sine wave pattern with noise. The seed derived from the package name ensures consistent test results.
Regarding the static analysis warnings about
dandendDatenot being modified: these are false positives. Thed.setUTCDate(d.getUTCDate() + 1)call mutates theDateobject in place, which the linter doesn't recognise as a modification. The loop terminates correctly.To silence the linter without changing behaviour, you could restructure the loop:
Optional: Alternative loop structure to satisfy linter
- for (let d = new Date(startDate); d <= endDate; d.setUTCDate(d.getUTCDate() + 1)) { - const day = d.toISOString().slice(0, 10) + let d = new Date(startDate) + while (d <= endDate) { + const day = d.toISOString().slice(0, 10) // Sine wave + noise for a realistic-looking sparkline const dayIndex = downloads.length const wave = Math.sin(dayIndex / 30) * base * 0.3 const noise = Math.sin(dayIndex * 7 + seed) * base * 0.1 downloads.push({ day, downloads: Math.max(0, Math.round(base + wave + noise)) }) + d.setUTCDate(d.getUTCDate() + 1) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1e526e7b-4ffe-427c-8b31-b7a436c2637a
⛔ Files ignored due to path filters (15)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yamlpublic/favicon.svgis excluded by!**/*.svgpublic/logo-icon.svgis excluded by!**/*.svgtest/e2e/og-image.spec.ts-snapshots/og-image-accessibility.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-blog-alpha-release.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-for--.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-for--package-nuxt-v-3-20-2.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-home.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package--babel-plugin-transform-exponentiation-operator.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package--nuxt-kit.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package--tanstack-react-query.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package-code-vue-v-3-5-27.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package-docs-ufo-v-1-6-3.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package-nuxt-v-4-3-1.pngis excluded by!**/*.pngtest/e2e/og-image.spec.ts-snapshots/og-image-package-vue.pngis excluded by!**/*.png
📒 Files selected for processing (51)
.github/workflows/ci.yml.storybook/preview.tsapp/app.vueapp/components/OgBrand.vueapp/components/OgImage/BlogPost.d.vue.tsapp/components/OgImage/BlogPost.takumi.vueapp/components/OgImage/BlogPost.vueapp/components/OgImage/Default.vueapp/components/OgImage/Package.d.vue.tsapp/components/OgImage/Package.takumi.vueapp/components/OgImage/Package.vueapp/components/OgImage/Page.takumi.vueapp/components/OgImage/Profile.takumi.vueapp/components/OgImage/Splash.takumi.vueapp/components/OgLayout.vueapp/components/global/BlogPostWrapper.vueapp/composables/useCharts.tsapp/pages/about.vueapp/pages/accessibility.vueapp/pages/index.vueapp/pages/org/[org].vueapp/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vueapp/pages/package-docs/[...path].vueapp/pages/package/[[org]]/[name].vueapp/pages/pds.vueapp/pages/privacy.vueapp/pages/profile/[identity]/index.vueapp/pages/recharging.vueapp/pages/search.vueapp/pages/settings.vueapp/pages/~[username]/index.vueapp/pages/~[username]/orgs.vuemodules/og-image.tsmodules/runtime/server/cache.tsnuxt.config.tspackage.jsonpnpm-workspace.yamlscripts/generate-fixtures.tstest/e2e/og-image.spec.tstest/fixtures/mock-routes.cjstest/fixtures/npm-api/downloads/@anthropic-ai/claude-code.jsontest/fixtures/npm-api/downloads/@babel/plugin-transform-exponentiation-operator.jsontest/fixtures/npm-api/downloads/@tanstack/react-query.jsontest/fixtures/npm-registry/packuments/@anthropic-ai/claude-code.jsontest/fixtures/npm-registry/packuments/@babel/plugin-transform-exponentiation-operator.jsontest/fixtures/npm-registry/packuments/@tanstack/react-query.jsontest/mocks/site-config.tstest/nuxt/components/OgImagePackage.spec.tstest/unit/a11y-component-coverage.spec.tsuno.config.tsvitest.config.ts
💤 Files with no reviewable changes (7)
- app/components/OgImage/Package.d.vue.ts
- app/components/OgImage/BlogPost.vue
- pnpm-workspace.yaml
- app/components/OgImage/Package.vue
- app/components/OgImage/BlogPost.d.vue.ts
- modules/og-image.ts
- app/components/OgImage/Default.vue
Comment on lines
+18
to
+20
| :width="width" | ||
| :height="height" | ||
| :style="{ width: `${width}px`, height: `${height}px` }" |
Contributor
There was a problem hiding this comment.
height is not defined in scope – use props.height.
On line 19, :height="height" references height which isn't directly available in scope. Since props is the return value of withDefaults, you need to access props.height.
🐛 Proposed fix
:width="width"
- :height="height"
- :style="{ width: `${width}px`, height: `${height}px` }"
+ :height="props.height"
+ :style="{ width: `${width}px`, height: `${props.height}px` }"📝 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.
Suggested change
| :width="width" | |
| :height="height" | |
| :style="{ width: `${width}px`, height: `${height}px` }" | |
| :width="width" | |
| :height="props.height" | |
| :style="{ width: `${width}px`, height: `${props.height}px` }" |
Comment on lines
+29
to
+35
| const getInitials = (name: string) => | ||
| name | ||
| .split(' ') | ||
| .map(n => n[0]) | ||
| .join('') | ||
| .toUpperCase() | ||
| .slice(0, 2) |
Contributor
There was a problem hiding this comment.
Potential runtime error if name contains consecutive spaces.
If name contains consecutive spaces (e.g., "John Doe"), split(' ') produces empty strings, and accessing n[0] on an empty string returns undefined. Calling .toUpperCase() on undefined throws a TypeError.
🛡️ Proposed fix to filter empty parts
const getInitials = (name: string) =>
name
.split(' ')
+ .filter(n => n.length > 0)
.map(n => n[0])
.join('')
.toUpperCase()
.slice(0, 2)📝 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.
Suggested change
| const getInitials = (name: string) => | |
| name | |
| .split(' ') | |
| .map(n => n[0]) | |
| .join('') | |
| .toUpperCase() | |
| .slice(0, 2) | |
| const getInitials = (name: string) => | |
| name | |
| .split(' ') | |
| .filter(n => n.length > 0) | |
| .map(n => n[0]) | |
| .join('') | |
| .toUpperCase() | |
| .slice(0, 2) |
Comment on lines
+70
to
+77
| <!-- Description --> | ||
| <div | ||
| v-if="description" | ||
| class="text-3xl text-fg-muted opacity-70" | ||
| :style="{ lineClamp: 2, textOverflow: 'ellipsis' }" | ||
| > | ||
| {{ description }} | ||
| </div> |
Contributor
There was a problem hiding this comment.
CSS lineClamp property requires vendor prefix or standard syntax.
The inline style uses lineClamp which is not a standard CSS property. The correct property is -webkit-line-clamp (with display: -webkit-box and -webkit-box-orient: vertical), or you could use a UnoCSS utility class like line-clamp-2 instead.
🐛 Proposed fix using UnoCSS utility
<div
v-if="description"
- class="text-3xl text-fg-muted opacity-70"
- :style="{ lineClamp: 2, textOverflow: 'ellipsis' }"
+ class="text-3xl text-fg-muted opacity-70 line-clamp-2"
>📝 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.
Suggested change
| <!-- Description --> | |
| <div | |
| v-if="description" | |
| class="text-3xl text-fg-muted opacity-70" | |
| :style="{ lineClamp: 2, textOverflow: 'ellipsis' }" | |
| > | |
| {{ description }} | |
| </div> | |
| <!-- Description --> | |
| <div | |
| v-if="description" | |
| class="text-3xl text-fg-muted opacity-70 line-clamp-2" | |
| > | |
| {{ description }} | |
| </div> |
app/composables/useCharts.ts
Outdated
| .slice() | ||
| .sort((a, b) => a.day.localeCompare(b.day)) | ||
| .map(item => { | ||
| const dayDate = parseIsoDateOnly(item.day) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for parseIsoDateOnly definition in the codebase
echo "=== Searching for parseIsoDateOnly definition ==="
rg -n "function parseIsoDateOnly|const parseIsoDateOnly|export.*parseIsoDateOnly" --type ts
echo ""
echo "=== Searching for parseIsoDate definition ==="
rg -n "function parseIsoDate|const parseIsoDate|export.*parseIsoDate" --type tsRepository: npmx-dev/npmx.dev
Length of output: 229
🏁 Script executed:
# Check the imports and beginning of the file
head -40 app/composables/useCharts.ts
echo ""
echo "=== Check if parseIsoDateOnly is defined anywhere in the file ==="
rg -n "parseIsoDateOnly" app/composables/useCharts.ts
echo ""
echo "=== Check the full definition of parseIsoDate ==="
cat app/utils/date.tsRepository: npmx-dev/npmx.dev
Length of output: 2363
🏁 Script executed:
# Check the first 25 lines to see all imports
head -25 app/composables/useCharts.ts
echo ""
echo "=== Search for parseIsoDate import in the file ==="
rg -n "import.*parseIsoDate|from.*date" app/composables/useCharts.ts | head -20Repository: npmx-dev/npmx.dev
Length of output: 774
🏁 Script executed:
# Count total lines
wc -l app/composables/useCharts.ts
echo ""
echo "=== Display the entire file ==="
cat app/composables/useCharts.tsRepository: npmx-dev/npmx.dev
Length of output: 20834
Import missing date utilities from ~/utils/date.
The file uses parseIsoDateOnly at lines 73, 86, 87, 92, 176, and 194, but this function does not exist in the codebase. Additionally, parseIsoDate, toIsoDate, and addDays are called without being imported.
Add the missing import statement:
import { parseIsoDate, toIsoDate, addDays } from '~/utils/date'If parseIsoDateOnly is meant to parse only the date portion (ignoring time), use parseIsoDate instead, as it already provides this behaviour. Otherwise, define parseIsoDateOnly in the date utilities module.
🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 73-73:
Cannot find name 'parseIsoDateOnly'. Did you mean 'parseIsoDate'?
Comment on lines
+18
to
+25
| defineOgImage( | ||
| 'Page.takumi', | ||
| { | ||
| title: () => `${$t('about.title')}`, | ||
| description: 'a fast, modern browser for the npm registry', | ||
| }, | ||
| { alt: () => `${$t('about.title')} — npmx` }, | ||
| ) |
Contributor
There was a problem hiding this comment.
Inconsistent internationalisation for OG image description.
The title and alt use $t() for localisation, but description is hardcoded as 'a fast, modern browser for the npm registry'. In contrast, useSeoMeta on line 13 uses $t('about.meta_description'). If consistent localisation is desired for OG images, consider using the translation function here as well.
🌐 Proposed fix for consistency
defineOgImage(
'Page.takumi',
{
title: () => `${$t('about.title')}`,
- description: 'a fast, modern browser for the npm registry',
+ description: () => $t('about.meta_description'),
},
{ alt: () => `${$t('about.title')} — npmx` },
)📝 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.
Suggested change
| defineOgImage( | |
| 'Page.takumi', | |
| { | |
| title: () => `${$t('about.title')}`, | |
| description: 'a fast, modern browser for the npm registry', | |
| }, | |
| { alt: () => `${$t('about.title')} — npmx` }, | |
| ) | |
| defineOgImage( | |
| 'Page.takumi', | |
| { | |
| title: () => `${$t('about.title')}`, | |
| description: () => $t('about.meta_description'), | |
| }, | |
| { alt: () => `${$t('about.title')} — npmx` }, | |
| ) |
Comment on lines
+246
to
+265
| if (rangeMatch?.[1] && rangeMatch[2]) { | ||
| const [startDate, endDate] = rangeMatch[1].split(':') | ||
| const packageName = rangeMatch[2] | ||
| // Simple hash seeded by package name for deterministic but varied curves | ||
| let h = 0 | ||
| for (const c of packageName) h = ((h << 5) - h + c.charCodeAt(0)) | 0 | ||
| const s = Math.abs(h) | ||
|
|
||
| const base = (s % 40_000) + 500 | ||
| // Trend: some packages grow, some shrink, some flat | ||
| const trendSlope = (((s >> 4) % 200) - 100) / 100_000 // -0.001 .. +0.001 per day | ||
| // Wave period varies per package (20-60 days) | ||
| const wavePeriod = 20 + ((s >> 8) % 40) | ||
| const waveAmp = 0.1 + ((s >> 12) % 30) / 100 // 0.10 .. 0.40 | ||
| // Weekend dip intensity | ||
| const weekendDip = 0.3 + ((s >> 16) % 40) / 100 // 0.30 .. 0.70 | ||
|
|
||
| const downloads: { day: string; downloads: number }[] = [] | ||
| const start = new Date(startDate!) | ||
| const end = new Date(endDate!) |
Contributor
There was a problem hiding this comment.
Add defensive checks for date parsing to avoid runtime errors.
The code assumes rangeMatch[1] contains a colon and produces valid dates. If the input format is malformed (e.g., missing colon or invalid date strings), startDate or endDate could be undefined, and new Date() could produce Invalid Date.
Note: The static analysis warnings about d and end not being modified are false positives – d.setUTCDate() correctly mutates the Date object in place.
🛡️ Proposed defensive fix
if (rangeMatch?.[1] && rangeMatch[2]) {
const [startDate, endDate] = rangeMatch[1].split(':')
const packageName = rangeMatch[2]
+
+ // Validate date format
+ if (!startDate || !endDate) {
+ return null
+ }
+
// Simple hash seeded by package name for deterministic but varied curves
let h = 0
for (const c of packageName) h = ((h << 5) - h + c.charCodeAt(0)) | 0
const s = Math.abs(h)
// ... existing code ...
const downloads: { day: string; downloads: number }[] = []
- const start = new Date(startDate!)
- const end = new Date(endDate!)
+ const start = new Date(startDate)
+ const end = new Date(endDate)
+
+ // Validate parsed dates
+ if (Number.isNaN(start.getTime()) || Number.isNaN(end.getTime())) {
+ return null
+ }
+
for (let d = new Date(start); d <= end; d.setUTCDate(d.getUTCDate() + 1)) {
Contributor
Author
Thanks :) Happy to re-add if there's a preference for that. My focus was on getting the og images to match the current site branding, as they seemed to have skewed a bit and good opportunity to re-do them given the new v6 capabilities (we can re-use the unocss tokens). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/unit/a11y-component-coverage.spec.ts (1)
201-204: Prefer one template literal over concatenation.Line 201 to Line 204 still concatenate two template literals; a single template literal is easier to maintain.
♻️ Suggested tidy-up
- return ( - `Missing a11y tests for ${missingTests.length} component(s):\n${missingTests - .map(c => ` - ${c}`) - .join('\n')}\n\nTo fix: Add tests in test/nuxt/a11y.spec.ts or add to SKIPPED_COMPONENTS ` + - `in test/unit/a11y-component-coverage.spec.ts with justification.` - ) + return `Missing a11y tests for ${missingTests.length} component(s):\n${missingTests + .map(c => ` - ${c}`) + .join('\n')}\n\nTo fix: Add tests in test/nuxt/a11y.spec.ts or add to SKIPPED_COMPONENTS in test/unit/a11y-component-coverage.spec.ts with justification.`app/composables/useCharts.ts (1)
165-166: Drop redundant pre-sort before month/year grouping.The initial
daily.slice().sort(...)in both builders is unnecessary since you sort grouped entries before returning. Removing it cuts avoidableO(n log n)work.Proposed refactor
export function buildMonthlyEvolutionFromDaily(daily: DailyRawPoint[]): MonthlyDataPoint[] { - const sorted = daily.slice().sort((a, b) => a.day.localeCompare(b.day)) const valuesByMonth = new Map<string, number>() - for (const item of sorted) { + for (const item of daily) { const month = item.day.slice(0, 7) valuesByMonth.set(month, (valuesByMonth.get(month) ?? 0) + item.value) } @@ export function buildYearlyEvolutionFromDaily(daily: DailyRawPoint[]): YearlyDataPoint[] { - const sorted = daily.slice().sort((a, b) => a.day.localeCompare(b.day)) const valuesByYear = new Map<string, number>() - for (const item of sorted) { + for (const item of daily) { const year = item.day.slice(0, 4) valuesByYear.set(year, (valuesByYear.get(year) ?? 0) + item.value) }Also applies to: 183-184
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3be127c1-5d4f-4b84-bb4d-c5bcc42bdd45
📒 Files selected for processing (3)
app/composables/useCharts.tsknip.tstest/unit/a11y-component-coverage.spec.ts
💤 Files with no reviewable changes (1)
- knip.ts
Comment on lines
+80
to
+125
| export function buildRollingWeeklyEvolutionFromDaily( | ||
| daily: DailyRawPoint[], | ||
| rangeStartIso: string, | ||
| rangeEndIso: string, | ||
| ): WeeklyDataPoint[] { | ||
| const sorted = daily.slice().sort((a, b) => a.day.localeCompare(b.day)) | ||
| const rangeStartDate = parseIsoDate(rangeStartIso) | ||
| const rangeEndDate = parseIsoDate(rangeEndIso) | ||
|
|
||
| const groupedByIndex = new Map<number, number>() | ||
|
|
||
| for (const item of sorted) { | ||
| const itemDate = parseIsoDate(item.day) | ||
| const dayOffset = Math.floor((itemDate.getTime() - rangeStartDate.getTime()) / 86400000) | ||
| if (dayOffset < 0) continue | ||
|
|
||
| const weekIndex = Math.floor(dayOffset / 7) | ||
| groupedByIndex.set(weekIndex, (groupedByIndex.get(weekIndex) ?? 0) + item.value) | ||
| } | ||
|
|
||
| return Array.from(groupedByIndex.entries()) | ||
| .sort(([a], [b]) => a - b) | ||
| .map(([weekIndex, value]) => { | ||
| const weekStartDate = addDays(rangeStartDate, weekIndex * 7) | ||
| const weekEndDate = addDays(weekStartDate, 6) | ||
|
|
||
| // Clamp weekEnd to the actual data range end date | ||
| const clampedWeekEndDate = | ||
| weekEndDate.getTime() > rangeEndDate.getTime() ? rangeEndDate : weekEndDate | ||
|
|
||
| const weekStartIso = toIsoDateString(weekStartDate) | ||
| const weekEndIso = toIsoDateString(clampedWeekEndDate) | ||
|
|
||
| const timestampStart = weekStartDate.getTime() | ||
| const timestampEnd = clampedWeekEndDate.getTime() | ||
|
|
||
| return { | ||
| value, | ||
| weekKey: `${weekStartIso}_${weekEndIso}`, | ||
| weekStart: weekStartIso, | ||
| weekEnd: weekEndIso, | ||
| timestampStart, | ||
| timestampEnd, | ||
| } | ||
| }) | ||
| } |
Contributor
There was a problem hiding this comment.
Exclude points after rangeEndIso in weekly aggregation.
Line 93 only guards the lower bound. Points after the requested end range are still counted, which can skew values and produce invalid week windows.
Proposed fix
export function buildRollingWeeklyEvolutionFromDaily(
daily: DailyRawPoint[],
rangeStartIso: string,
rangeEndIso: string,
): WeeklyDataPoint[] {
const sorted = daily.slice().sort((a, b) => a.day.localeCompare(b.day))
const rangeStartDate = parseIsoDate(rangeStartIso)
const rangeEndDate = parseIsoDate(rangeEndIso)
+ const rangeStartTime = rangeStartDate.getTime()
+ const rangeEndTime = rangeEndDate.getTime()
const groupedByIndex = new Map<number, number>()
for (const item of sorted) {
const itemDate = parseIsoDate(item.day)
- const dayOffset = Math.floor((itemDate.getTime() - rangeStartDate.getTime()) / 86400000)
- if (dayOffset < 0) continue
+ const itemTime = itemDate.getTime()
+ if (itemTime < rangeStartTime || itemTime > rangeEndTime) continue
+ const dayOffset = Math.floor((itemTime - rangeStartTime) / 86400000)
const weekIndex = Math.floor(dayOffset / 7)
groupedByIndex.set(weekIndex, (groupedByIndex.get(weekIndex) ?? 0) + item.value)
}
Comment on lines
+128
to
+133
| export function smoothPath(pts: { x: number; y: number }[]): string { | ||
| if (pts.length < 2) return '0,0' | ||
| const n = pts.length - 1 | ||
| const r = (v: number) => Math.round(v * 100) / 100 | ||
| const out = [`${r(pts[0]!.x)},${r(pts[0]!.y)}`] | ||
| const dx: number[] = [] |
Contributor
There was a problem hiding this comment.
Handle single-point paths with the actual coordinate.
Line 129 returns '0,0' for both empty and single-point series. For one point, this should return that point’s coordinate; otherwise charts can render at the origin incorrectly.
Proposed fix
export function smoothPath(pts: { x: number; y: number }[]): string {
- if (pts.length < 2) return '0,0'
+ if (pts.length === 0) return '0,0'
const n = pts.length - 1
const r = (v: number) => Math.round(v * 100) / 100
- const out = [`${r(pts[0]!.x)},${r(pts[0]!.y)}`]
+ if (pts.length === 1) return `${r(pts[0]!.x)},${r(pts[0]!.y)}`
+ const out = [`${r(pts[0]!.x)},${r(pts[0]!.y)}`]📝 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.
Suggested change
| export function smoothPath(pts: { x: number; y: number }[]): string { | |
| if (pts.length < 2) return '0,0' | |
| const n = pts.length - 1 | |
| const r = (v: number) => Math.round(v * 100) / 100 | |
| const out = [`${r(pts[0]!.x)},${r(pts[0]!.y)}`] | |
| const dx: number[] = [] | |
| export function smoothPath(pts: { x: number; y: number }[]): string { | |
| if (pts.length === 0) return '0,0' | |
| const n = pts.length - 1 | |
| const r = (v: number) => Math.round(v * 100) / 100 | |
| if (pts.length === 1) return `${r(pts[0]!.x)},${r(pts[0]!.y)}` | |
| const out = [`${r(pts[0]!.x)},${r(pts[0]!.y)}`] | |
| const dx: number[] = [] |
🧰 Tools
🪛 GitHub Check: 🔠 Lint project
[warning] 128-128: eslint-plugin-unicorn(consistent-function-scoping)
Function r does not capture any variables from its parent scope
🪛 GitHub Check: 🤖 Autofix code
[warning] 128-128: eslint-plugin-unicorn(consistent-function-scoping)
Function r does not capture any variables from its parent scope
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
#1654
🧭 Context
The previous PR was rolled back due to some Takumi stability issues. These are now solved afaik and OG Image got a v6 stable release so it's a good time to reintroduce it. (Takumi is v1 beta now so also likely stable very soon)
📚 Description
Same as before, I've resynced the UI to match recent branding changes.
sample - check PR files for more
https://npmx.dev/
https://npmx.dev/accessibility
https://npmx.dev/blog/alpha-release
https://npmx.dev/package/@nuxt/kit
https://npmx.dev/package-docs/ufo/v/1.6.3
