Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
ShroXd
changed the title
ShroXd
force-pushed
the
linkable-version-page
branch
from
dd33ecc to
2873b63
Compare
ShroXd
force-pushed
the
linkable-version-page
branch
from
2873b63 to
5cc0842
Compare
ShroXd
force-pushed
the
linkable-version-page
branch
from
5cc0842 to
5fd65b8
Compare
ShroXd
force-pushed
the
linkable-version-page
branch
from
5fd65b8 to
35bdb74
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
ShroXd
force-pushed
the
linkable-version-page
branch
from
35bdb74 to
03ed3c3
Compare
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
|
ShroXd
force-pushed
the
linkable-version-page
branch
from
03ed3c3 to
bba4380
Compare
📝 WalkthroughWalkthroughThis PR introduces a dedicated page route for viewing package version history. A new page component renders versions with semantic grouping, expandable groups with lazy-loaded metadata, search filtering with semver range support, and virtualization for long lists. The main package page conditionally gates rendering when on the versions route. The Versions component header gains a new "View all versions" link button. Internationalisation strings and schema are extended to support the new UI elements including page title, current tags section, and filter controls. Possibly related PRs
Suggested labels
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)
📝 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 CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/pages/package/[[org]]/[name]/versions.vue (1)
377-387: Consider adding safety check for array access.
item.versions[0]is accessed without a guard. While the grouping logic ensures each group has at least one version, adding a safety check would satisfy strict type-safety guidelines and prevent potential runtime issues if the logic changes.🔧 Suggested fix
<span class="ms-auto flex items-center gap-3 shrink-0"> - <span class="text-xs text-fg-muted" dir="ltr">{{ item.versions[0] }}</span> + <span v-if="item.versions[0]" class="text-xs text-fg-muted" dir="ltr">{{ item.versions[0] }}</span> <DateTime - v-if="getVersionTime(item.versions[0])" - :datetime="getVersionTime(item.versions[0])!" + v-if="item.versions[0] && getVersionTime(item.versions[0])" + :datetime="getVersionTime(item.versions[0])" class="text-xs text-fg-subtle hidden sm:block"As per coding guidelines: "ensure 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: 9477326f-b47e-43c8-81a2-07b1d0698711
📒 Files selected for processing (7)
app/components/Package/Versions.vueapp/pages/package/[[org]]/[name].vueapp/pages/package/[[org]]/[name]/versions.vuei18n/locales/en.jsoni18n/schema.jsontest/nuxt/components/PackageVersions.spec.tstest/nuxt/pages/PackageVersionsPage.spec.ts
| <LinkBase | ||
| :to="versionsPageRoute" | ||
| variant="button-secondary" | ||
| class="text-fg-subtle hover:text-fg transition-colors min-w-6 min-h-6 p-1 rounded" | ||
| :title="$t('package.versions.view_all_versions')" | ||
| classicon="i-lucide:history" | ||
| data-testid="view-all-versions-link" | ||
| > |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate LinkBase implementation"
fd -p 'Base.vue' app/components/Link
LINK_BASE_FILE="$(fd -p 'Base.vue' app/components/Link | head -n 1)"
echo "Inspecting: ${LINK_BASE_FILE}"
echo
echo "=== defineProps / attrs forwarding signals ==="
rg -n "defineProps|\\$attrs|inheritAttrs|v-bind=|data-testid|title" "$LINK_BASE_FILE"
echo
echo "=== file excerpt ==="
sed -n '1,260p' "$LINK_BASE_FILE"Repository: npmx-dev/npmx.dev
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Inspecting LinkBase implementation ==="
cat -n app/components/Link/Base.vueRepository: npmx-dev/npmx.dev
Length of output: 5803
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Versions.vue lines 530-545 ==="
sed -n '530,545p' app/components/Package/Versions.vue
echo
echo "=== Search for data-testid usage in Versions.vue ==="
rg -n 'data-testid' app/components/Package/Versions.vue | head -20
echo
echo "=== Search for tests using view-all-versions-link ==="
rg -r 'view-all-versions-link' --type ts --type tsx --type js --type jsx --type vueRepository: npmx-dev/npmx.dev
Length of output: 1543
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Search for test files referencing view-all-versions-link ==="
fd -e spec -e test | xargs grep -l 'view-all-versions-link' 2>/dev/null || echo "No test files found with this selector"
echo
echo "=== Check for any __tests__ or test directories ==="
fd -type d -name '__tests__' -o -name 'tests' -o -name 'test' | head -10
echo
echo "=== Search all files for view-all-versions-link ==="
rg 'view-all-versions-link' --type-list | head -5
rg 'view-all-versions-link' 2>/dev/null || echo "Pattern not found in other files"Repository: npmx-dev/npmx.dev
Length of output: 405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Verify if LinkBase ever renders disabled state in practice ==="
rg -c 'disabled' app/components/Link/Base.vue
echo
echo "=== Check Versions.vue to see if disabled is ever passed ==="
rg -n 'LinkBase' app/components/Package/Versions.vue | grep -i disabled || echo "No disabled prop passed"
echo
echo "=== Verify title is also not declared (from learnings) ==="
rg -n "title" app/components/Link/Base.vue || echo "title not found in LinkBase"Repository: npmx-dev/npmx.dev
Length of output: 303
data-testid and title attributes are not forwarded by LinkBase.
LinkBase does not declare data-testid or title as props. The component uses a v-if/v-else fragment where the non-disabled branch renders <NuxtLink v-bind="props">. Since v-bind="props" only forwards declared props, these HTML attributes are silently dropped and will not appear on the rendered DOM element, making the test selector ineffective and the title attribute unavailable to assistive technologies.
To fix this, add both data-testid and title to LinkBase's defineProps, and forward them explicitly to the <NuxtLink> (the <span> branch can also receive them if needed for consistency).
| // Gates template rendering only — data fetches intentionally still run. | ||
| const isVersionsRoute = computed(() => route.name === 'package-versions') |
There was a problem hiding this comment.
Versions route still triggers the full package data pipeline.
This gates rendering only, but all heavyweight fetch/composable initialisation still runs on /versions. That adds avoidable load and weakens the intended versions-page optimisation. Please gate expensive package-detail data hooks for this route, or move the versions page into a sibling route shell that does not initialise main-page data.
Also applies to: 521-535
| expandedGroups.has(item.groupKey) | ||
| ? $t('package.versions.collapse', { tag: item.label }) | ||
| : $t('package.versions.expand', { tag: item.label }) |
There was a problem hiding this comment.
Don’t worry about updating these labels. Using aria-expanded is already sufficient for AT users to know what state the button is in and consequently what state it will change to if they activate it. Doing both can be confusing (e.g. screen reader might read Collapse 4.x versions, button expanded or Expand 4.x versions, button collapsed).
This goes for anywhere else in the PR that includes the words “expand” or “collapse” inside of the aria-label.
3 participants
🔗 Linked issue
Resolves #1846
🧭 Context
Like the original issues mentioned, it would be useful to have a full version history page with filtering functionality.
📚 Description
Since it's common for a package to have hundreds of versions, some package like typescript even have 3,700 versions. To provide good performance and user experience, I did some optimization and trade off during developing.
1. Two phase data fetching
Phase 1 (on page load) calls
getVersionsto fetch only a lightweight summary — version strings, dist-tags, and publish times. This is enough to immediately render the "Current Tags" section and all group headers without waiting for full metadata.Phase 2 (lazy, on first group expand) calls
fetchAllPackageVersionsto retrieve full metadata (deprecated status, provenance). The result is cached infullVersionMapas ashallowRef— once loaded it's reused across all subsequent group expansions, never re-fetched.The tradeoff: a short one-time loading delay on first expand, in exchange for a significantly faster initial page load.
2. Virtual scrolling + SSR fallback
Since
WindowVirtualizeris client-only, it's wrapped in<ClientOnly>, with a#fallbackslot providing a static SSR substitute — just the first 20 group headers, no interactivity.The SSR fallback iterates
versionGroupsdirectly, a flat list of group objects, each containing an array of versionsThe client-side
virtualizerconsumesflatItems— a flattened array that interleaves group headers and their expanded version rows into a single indexed list.This follows existing pattern in package list page.
3. Debounced version filter
Since the filtering work is done entirely on the frontend, it can put significant performance pressure on packages with a large number of versions, such as typescript. And to support virtual scrolling, version groups are flattened into a
flatItemsarray — a single indexed list of group headers and version rows thatWindowVirtualizercan render efficiently. Therefore, a debounce on the filter input is necessary to avoid triggering expensive recomputation on every keystroke.Some further optimization options include chunked
flatItemsconstruction withrequestIdleCallbackand moving the filtering work into a Web Worker. However, both require non-trivial design and implementation to handle potential race conditions. We also plan to add changelog support to this page, which will introduce additional matching overhead. For now, the current implementation is "good enough" — once the full feature set is complete, we can do thorough performance profiling to find the best optimization approach.4. Disable prefetching for package page of each version
Per-version links in the history list have prefetching disabled, as expanding a group could generate a lot of simultaneous prefetch requests and unnecessarily strain the network and significantly impact page performance. Although this makes package page navigation slightly slower, considering most user behavior on this page is browsing and filtering, the tradeoff is acceptable.
5. Some interesting cases
📸 Screenshots