Conversation
Do the hard work in the api, fake paging just for sake of making the page nicer.
Aw yeah!
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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
|
|
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:
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 selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a package timeline feature: a new page at /package-timeline/:org?/:packageName/v/:version with SSR-cached API pagination and client-side "load more", per-version computed sub-events (install size, dependency deltas, license, ESM/Typescript/trusted-publisher/provenance changes), best-effort per-version install-size fetches and caching, and UI including PackageHeader timeline tab and keyboard shortcut. Normalises license/type fields in packument handling, extends SlimVersion, adds i18n keys/schema for timeline and shortcut, and includes unit tests for the timeline API endpoint. Possibly related issues
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7fe43926-024b-4e21-9ef6-4a2a338bd6b7
📒 Files selected for processing (7)
app/components/Package/Header.vueapp/composables/npm/usePackage.tsapp/pages/package-timeline/[[org]]/[packageName].vuei18n/locales/en.jsoni18n/schema.jsonserver/api/registry/timeline/[...pkg].get.tsshared/types/npm-registry.ts
| const versionSubEvents = computed(() => { | ||
| const result = new Map<string, SubEvent[]>() | ||
| const entries = timelineEntries.value | ||
|
|
||
| // Sort by semver to find each version's true predecessor | ||
| const semverSorted = [...entries].sort((a, b) => compare(b.version, a.version)) | ||
| const prevBySemver = new Map<string, TimelineVersion>() | ||
| for (let i = 0; i < semverSorted.length - 1; i++) { | ||
| prevBySemver.set(semverSorted[i]!.version, semverSorted[i + 1]!) | ||
| } |
There was a problem hiding this comment.
Don't infer the semver predecessor from a paged slice.
timelineEntries only contains the current publish-time page. Sorting that slice by semver and taking semverSorted[i + 1] does not guarantee the true predecessor, so maintenance releases on older branches can end up compared against unrelated versions and produce false sub-events. This needs predecessor data from the full version set, or the comparison should be suppressed until the real predecessor is loaded.
|
Super cool feature! I took a quick look at it, and there are a couple of UI/UX aspects that aren't very clear What happens when you click on each version?I know the "diff" link appears and lets you check differences between versions, but it’s not obvious that clicking the version itself opens that feature. Maybe we can improve discoverability and the link's purpose? Re-renderingEvery time I click a version, it does all the requests again (cached), but it seems to be causing a lot of repainting |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/server/api/registry/timeline/pkg.get.spec.ts (2)
7-7: AvoidFunctionin the cached-handler stub.Using
Functionremoves parameter/return checks and can hide handler-signature drift; prefer an explicit typed callback signature.♻️ Proposed refactor
-vi.stubGlobal('defineCachedEventHandler', (fn: Function) => fn) +vi.stubGlobal( + 'defineCachedEventHandler', + <T>(fn: (event: H3Event) => T, _opts?: unknown) => fn, +)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".
189-190: Removeanyfrom version lookup predicates.The predicate type can be inferred from
result.versions;anyweakens test type guarantees.♻️ Proposed refactor
- const latest = result.versions.find((v: any) => v.version === '1.0.0') - const next = result.versions.find((v: any) => v.version === '2.0.0-beta.1') + const latest = result.versions.find(v => v.version === '1.0.0') + const next = result.versions.find(v => v.version === '2.0.0-beta.1')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".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 73024f04-51f6-4e72-aa61-63bf62b35d6a
📒 Files selected for processing (1)
test/unit/server/api/registry/timeline/pkg.get.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/pages/package-timeline/[[org]]/[packageName].vue (1)
390-392:⚠️ Potential issue | 🟡 MinorEmpty state does not distinguish between loading and initial fetch failure.
If the initial
useAsyncDatarequest fails,timelineEntriesremains empty and the spinner displays indefinitely. Consider adding an error state to inform users when the initial load has failed.Suggested approach
Capture the error from
useAsyncData:-const { data: initialTimeline } = await useAsyncData(`timeline:${packageName.value}`, () => +const { data: initialTimeline, error: initialError } = await useAsyncData(`timeline:${packageName.value}`, () => fetchTimeline(0), )Then render an error state in the template:
<!-- Empty state --> - <div v-else-if="!timelineEntries.length" class="py-20 text-center"> + <div v-else-if="initialError" class="py-20 text-center text-red-600 dark:text-red-400"> + {{ $t('package.timeline.initial_load_error') }} + </div> + <div v-else-if="!timelineEntries.length" class="py-20 text-center"> <span class="i-svg-spinners:ring-resize w-5 h-5 text-fg-subtle" /> </div>
🧹 Nitpick comments (2)
app/pages/package-timeline/[[org]]/[packageName].vue (2)
14-14: Prefer auto-imported$t()over destructuring fromuseI18n().The project pattern uses the globally auto-imported
$t()in<script setup>rather than destructuringtfromuseI18n(). The$t()global works inside computed properties and callbacks in this Nuxt i18n setup.Suggested change
-const { t } = useI18n()Then replace all
t(...)calls with$t(...)in the computed property (lines 175, 179, 193, 194, 207, 219, 226, 236, 243, 253, 260, 270, 277).Based on learnings: "In this Nuxt 4 project with nuxtjs/i18n v10, $t() and other globals like $n, $d are exposed in <script setup> and work inside callbacks... Do not destructure t from useI18n(); rely on the global provided by Nuxt i18n in script setup."
74-74: BlockingloadMorewhile sizes are fetching may degrade UX.
sizesLoading.valueprevents loading more versions whenever any install-size request is in flight. Since size fetches are best-effort and don't affect pagination correctness, this coupling unnecessarily delays content loading for users scrolling through the timeline.Consider decoupling size loading from pagination:
Suggested change
async function loadMore() { - if (loadingMore.value || sizesLoading.value) return + if (loadingMore.value) return
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2dd7a2a9-64d2-4901-b5e6-487ee0837186
📒 Files selected for processing (3)
app/components/AppFooter.vueapp/pages/package-timeline/[[org]]/[packageName].vuei18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (1)
- i18n/locales/en.json
There was a problem hiding this comment.
♻️ Duplicate comments (1)
app/pages/package-timeline/[[org]]/[packageName].vue (1)
148-157:⚠️ Potential issue | 🟠 MajorSemver predecessor inference remains unreliable with paginated data.
As noted in previous reviews,
timelineEntriescontains only the current publish-time page, not the full version set. Sorting this partial slice by semver and usingsemverSorted[i + 1]as the predecessor can produce incorrect comparisons:
- If the page contains
[2.0.0, 1.5.0]but1.9.0is on an unloaded page,2.0.0will be incorrectly compared against1.5.0- Maintenance releases on older branches may be compared against unrelated versions
Consider either:
- Suppress sub-events until predecessors are confirmed (e.g., track known predecessor gaps)
- Fetch predecessor data from the server alongside each version
- Accept this limitation and document it in the UI (e.g., "Based on loaded versions")
,
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a500347-492f-446d-b72f-6f1beba86091
📒 Files selected for processing (4)
app/pages/package-timeline/[[org]]/[packageName].vuei18n/locales/en.jsoni18n/schema.jsontest/unit/server/api/registry/timeline/pkg.get.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test/unit/server/api/registry/timeline/pkg.get.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- i18n/locales/en.json
- i18n/schema.json
That's because its a new page load really. isn't this how the code/docs/etc tabs work too? if not, whatever "make it work" config those have, we need the same here. any help with that would be nice |
| packageName = decodeURIComponent(pkgParam) | ||
| } catch { | ||
| throw createError({ statusCode: 400, message: 'Invalid package name encoding' }) | ||
| } |
There was a problem hiding this comment.
we don't try/catch this anywhere else from what I can tell, should we be?
| tags: tagsByVersion.get(v) ?? [], | ||
| } | ||
| }) | ||
| .sort((a, b) => new Date(b.time).getTime() - new Date(a.time).getTime()) |
There was a problem hiding this comment.
| .sort((a, b) => new Date(b.time).getTime() - new Date(a.time).getTime()) | |
| .sort((a, b) => Date.parse(b.time) - Date.parse(a.time)) |
| time: packument.time[v]!, | ||
| license: typeof license === 'string' ? license : undefined, | ||
| type: typeof version.type === 'string' ? version.type : undefined, | ||
| hasTypes: hasBuiltInTypes(version) || undefined, |
There was a problem hiding this comment.
| hasTypes: hasBuiltInTypes(version) || undefined, | |
| hasTypes: hasBuiltInTypes(version), |
could we just do that?
|
@43081j, I didn't realize it was actually linking me to the package version. |
|
I was also wondering if it might be a good idea to merge this page's functionalities with the existing Version history feature. They seem similar enough to be combined into a single page. 🤔 |
howwohmm
left a comment
howwohmm
left a comment
There was a problem hiding this comment.
this is excellent work — the semver-ordered predecessor comparison is a particularly smart choice to avoid false diffs from interleaved backport releases. a few things from reading the full diff:
-
N+1 install-size fetches with no concurrency limit. the watcher fires
fetchSize()for all 25 versions simultaneously on page load. consider a concurrency limiter (e.g.p-limit(5)) to avoid overwhelming the browser's connection pool on slow networks. -
sizesLoadinggatesloadMore()too aggressively. if any size fetch is in-flight, the user can't paginate. since sizes are best-effort annotations, decouple them — guardloadMoreonly onloadingMoreitself, not onsizesLoading. -
page-boundary sub-event pop-in.
versionSubEventscomputes predecessors only from loaded entries. the last version on each page initially shows no sub-events, then retroactively gains them when the next page loads (bringing its semver predecessor into scope). one fix: fetchoffset - 1from the server to ensure the boundary predecessor is always available. -
useAsyncDatakey collision. the key`timeline:${packageName.value}`is evaluated once at setup. if the component instance handles navigation between packages (possible via address bar), the cache key won't distinguish them. using a getter function (Nuxt 3.8+) or including a version hash would be safer.
the cloneDeep on sub-event comparison data and the paginated server endpoint design are both well done.
4 participants
🔗 Linked issue
#2244
🧭 Context
A timeline will help show how a package has changed over time in good and bad ways.
📚 Description
This is basically a timeline which shows the published versions of a package.
Alongside each version, it shows significant changes.
Positive changes flagged:
Negatives flagged:
Neutrals flagged: