feat: add package download button#1586
Conversation
|
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
|
Adebesin-Cell
changed the title
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds a "subtle" variant to the Button component and adjusts its class/border logic. Introduces a new Package DownloadButton Vue component implementing a dropdown to download package tarballs or a dependencies script, with keyboard/accessibility handling and Teleported menu. Integrates the download button into the package detail page. Extracts install-size types to shared/types, adds tarballUrl to dependency/resolved package data and tests, and adds i18n keys and schema entries for download labels. 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)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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: 4
🧹 Nitpick comments (1)
app/pages/package/[[org]]/[name].vue (1)
1144-1154: Consider showing the download button without waiting for install-size.
Right now the button only appears once install-size resolves; if the fetch is slow or fails, users lose direct tarball download. Consider rendering ondisplayVersionand disabling the dependencies action untilinstallSizearrives.Possible tweak
- <PackageDownloadButton - v-if="displayVersion && installSize" + <PackageDownloadButton + v-if="displayVersion" :package-name="pkg.name" :version="displayVersion" - :install-size="installSize" + :install-size="installSize ?? undefined" size="small" />
| class="group gap-x-1 items-center justify-center font-mono border rounded-md transition-all duration-200 disabled:(opacity-40 cursor-not-allowed border-transparent)" | ||
| :class="{ | ||
| 'inline-flex': !block, | ||
| 'flex': block, | ||
| 'text-sm px-4 py-2': size === 'medium', | ||
| 'text-xs px-2 py-0.5': size === 'small', | ||
| 'text-xs px-2 py-0.5': size === 'small' && variant !== 'subtle', | ||
| 'text-xs px-2 py-2': size === 'small' && variant === 'subtle', | ||
| 'border-border': variant !== 'subtle', | ||
| 'border-border-subtle': variant === 'subtle', | ||
| 'bg-transparent text-fg hover:enabled:(bg-fg/10) focus-visible:enabled:(bg-fg/10) aria-pressed:(bg-fg/10 border-fg/20 hover:enabled:(bg-fg/20 text-fg/50))': | ||
| variant === 'secondary', | ||
| 'text-bg bg-fg hover:enabled:(bg-fg/50) focus-visible:enabled:(bg-fg/50) aria-pressed:(bg-fg text-bg border-fg hover:enabled:(text-bg/50))': | ||
| variant === 'primary', | ||
| 'bg-bg-subtle text-fg-muted hover:enabled:(text-fg border-border-hover) focus-visible:enabled:(text-fg border-border-hover) active:enabled:scale-95': | ||
| variant === 'subtle', |
There was a problem hiding this comment.
Avoid per-button focus-visible utilities for the subtle variant.
Buttons already get focus-visible styling globally; adding inline focus-visible classes here diverges from the shared pattern. Consider dropping the inline focus-visible utility (or moving the styling to the global rule).
Suggested change
- 'bg-bg-subtle text-fg-muted hover:enabled:(text-fg border-border-hover) focus-visible:enabled:(text-fg border-border-hover) active:enabled:scale-95':
+ 'bg-bg-subtle text-fg-muted hover:enabled:(text-fg border-border-hover) active:enabled:scale-95':
variant === 'subtle',Based on learnings: focus-visible styling for buttons and selects is applied globally via main.css (button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }); therefore individual buttons should not use inline focus-visible utility classes.
| function downloadPackage() { | ||
| const tarballUrl = props.version.dist.tarball | ||
| if (!tarballUrl) return | ||
|
|
||
| const link = document.createElement('a') | ||
| link.href = tarballUrl | ||
| link.download = `${props.packageName}-${props.version.version}.tgz` | ||
| document.body.appendChild(link) | ||
| link.click() | ||
| document.body.removeChild(link) | ||
| } |
There was a problem hiding this comment.
link.download is ignored for cross-origin URLs — tarball downloads will behave unreliably across browsers.
The HTML download attribute doesn't work with cross-origin URLs due to browser security restrictions implemented in Chrome 65 and other modern browsers; it is ignored for cross-origin URLs to prevent security issues. Since registry.npmjs.org is cross-origin from npmx.dev, browsers will ignore the download for cross-origin resources; as a result, no download is triggered and the resource is simply treated as another navigation destination.
The custom filename set at line 111 will never be applied. Whether the file downloads at all depends entirely on whether the npm registry sends Content-Disposition: attachment, which is outside this code's control.
The reliable fix is a Nuxt server route that proxies the tarball and sets the Content-Disposition: attachment; filename=... header itself, or fetching it as a Blob first:
🔧 Blob-based workaround (client-side, loads entire file into memory)
-function downloadPackage() {
- const tarballUrl = props.version.dist.tarball
- if (!tarballUrl) return
-
- const link = document.createElement('a')
- link.href = tarballUrl
- link.download = `${props.packageName}-${props.version.version}.tgz`
- document.body.appendChild(link)
- link.click()
- document.body.removeChild(link)
-}
+async function downloadPackage() {
+ const tarballUrl = props.version.dist.tarball
+ if (!tarballUrl) return
+
+ const response = await fetch(tarballUrl)
+ const blob = await response.blob()
+ const blobUrl = URL.createObjectURL(blob)
+ const link = document.createElement('a')
+ link.href = blobUrl
+ link.download = `${props.packageName.replace(/\//g, '__')}-${props.version.version}.tgz`
+ document.body.appendChild(link)
+ link.click()
+ document.body.removeChild(link)
+ URL.revokeObjectURL(blobUrl)
+}Note the blob-based fix also applies the / → __ replacement for scoped packages (e.g. @types/react), keeping the filename consistent with downloadDependenciesScript at lines 132 and 139, which already does this. The current line 111 is missing that replacement.
| ] | ||
|
|
||
| // Add root package | ||
| const rootTarball = (props.version.dist as any).tarball |
There was a problem hiding this comment.
Remove the unnecessary as any cast — dist.tarball is already typed on SlimPackumentVersion.
downloadPackage() on line 106 accesses props.version.dist.tarball directly without any cast, proving the field is properly typed on SlimPackumentVersion. The as any here silently bypasses TypeScript for no reason. As per the coding guidelines, strictly type-safe code is required.
🔧 Proposed fix
- const rootTarball = (props.version.dist as any).tarball
+ const rootTarball = props.version.dist.tarball📝 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.
| const rootTarball = (props.version.dist as any).tarball | |
| const rootTarball = props.version.dist.tarball |
| // Add dependencies | ||
| props.installSize.dependencies.forEach((dep: any) => { | ||
| lines.push(`# ${dep.name}@${dep.version}`) | ||
| lines.push(`curl -L ${dep.tarballUrl} -o ${dep.name.replace(/\//g, '__')}-${dep.version}.tgz`) | ||
| }) |
There was a problem hiding this comment.
Replace dep: any with DependencySize; add a guard for empty tarballUrl; quote URLs in the curl commands.
Three related issues in this block:
-
dep: any(line 137) —installSize.dependenciesis typed asDependencySize[], sodepis already statically typed. Annotating it asanydefeats type checking ondep.tarballUrl,dep.name, anddep.version. As per the coding guidelines, strictly type-safe code is required. -
Missing
tarballUrlguard (line 139) —DependencySize.tarballUrlis astring(not optional), but an empty string is valid at the type level. Ifdep.tarballUrlis"", the generatedcurlcommand becomescurl -L -o …which silently fails. -
Unquoted URL in
curl(lines 132, 139) — interpolating an unquoted URL into a shell command is fragile. Quoting removes any ambiguity.
🔧 Proposed fix
- props.installSize.dependencies.forEach((dep: any) => {
- lines.push(`# ${dep.name}@${dep.version}`)
- lines.push(`curl -L ${dep.tarballUrl} -o ${dep.name.replace(/\//g, '__')}-${dep.version}.tgz`)
- })
+ props.installSize.dependencies.forEach((dep) => {
+ if (!dep.tarballUrl) return
+ lines.push(`# ${dep.name}@${dep.version}`)
+ lines.push(`curl -L "${dep.tarballUrl}" -o ${dep.name.replace(/\//g, '__')}-${dep.version}.tgz`)
+ })Apply the same quoting fix to the root package curl command (line 132):
- lines.push(
- `curl -L ${rootTarball} -o ${props.packageName.replace(/\//g, '__')}-${props.version.version}.tgz`,
- )
+ lines.push(
+ `curl -L "${rootTarball}" -o ${props.packageName.replace(/\//g, '__')}-${props.version.version}.tgz`,
+ )📝 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.
| // Add dependencies | |
| props.installSize.dependencies.forEach((dep: any) => { | |
| lines.push(`# ${dep.name}@${dep.version}`) | |
| lines.push(`curl -L ${dep.tarballUrl} -o ${dep.name.replace(/\//g, '__')}-${dep.version}.tgz`) | |
| }) | |
| // Add dependencies | |
| props.installSize.dependencies.forEach((dep) => { | |
| if (!dep.tarballUrl) return | |
| lines.push(`# ${dep.name}@${dep.version}`) | |
| lines.push(`curl -L "${dep.tarballUrl}" -o ${dep.name.replace(/\//g, '__')}-${dep.version}.tgz`) | |
| }) |
1 participant
🔗 Linked issue
resolves #1528
🧭 Context
There was previously no way to directly download a package tarball or fetch all dependencies from the package detail page.
This PR introduces a Download button to make that happen.
📚 Description
This change adds a new Download button to the package detail page. The button includes a dropdown menu with two options:
.tgztarball directly..shscript to fetch all dependencies.Screenshot