fix: show version-specific license and highlight license changes#2215
fix: show version-specific license and highlight license changes#2215thealxlabs wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds an empty-state message to the Dependencies component when there are no dependencies; updates the package page to prefer the modern clipboard API ( Possibly related PRs
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 (2)
test/unit/app/utils/license-change.spec.ts (1)
13-21: Tests validate a local copy of the logic, not the production code.The
normalizeandlicenseChangedfunctions are duplicated here rather than imported from the production code inapp/pages/package/[[org]]/[name].vue(lines 325-333). If the production implementation ever diverges, these tests would continue to pass while the actual feature could be broken.Consider extracting the logic to a shared utility (e.g.,
app/utils/license.ts) and importing it in both the component and this test file. This would ensure the tests validate the actual production code.♻️ Suggested approach
Create a shared utility:
// app/utils/license.ts export type LicenseValue = string | { type?: string } | undefined | null export function normalizeLicense(l: LicenseValue): string { if (!l) return '' return typeof l === 'string' ? l : (l.type ?? '') } export function hasLicenseChanged( currentLicense: LicenseValue, packageLicense: LicenseValue, ): boolean { if (!currentLicense || !packageLicense) return false return normalizeLicense(currentLicense) !== normalizeLicense(packageLicense) }Then import in both the component and tests.
app/pages/package/[[org]]/[name].vue (1)
324-332: Consider extractingnormalizeto module scope.The static analysis tool flags that
normalizedoesn't capture any variables from its parent scope. Moving it outside the computed (or to a shared utility as suggested in the test file review) would address this lint warning and improve reusability.♻️ Suggested refactor
+// Module-level helper for license normalisation +function normalizeLicense(l: unknown): string { + if (!l) return '' + return typeof l === 'string' ? l : ((l as { type?: string })?.type ?? '') +} + // SlimVersion does not carry license, so we compare against the package-level license const licenseChanged = computed(() => { const currentLicense = displayVersion.value?.license const latestLicense = pkg.value?.license if (!currentLicense || !latestLicense) return false - const normalize = (l: unknown): string => - typeof l === 'string' ? l : ((l as { type?: string })?.type ?? '') - return normalize(currentLicense) !== normalize(latestLicense) + return normalizeLicense(currentLicense) !== normalizeLicense(latestLicense) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e835a82f-0997-48e4-986c-6932dc80b5f6
📒 Files selected for processing (4)
app/components/Package/Dependencies.vueapp/pages/package/[[org]]/[name].vuei18n/schema.jsontest/unit/app/utils/license-change.spec.ts
| async function copyReadmeHandler() { | ||
| await fetchReadmeMarkdown() | ||
| // Safari requires navigator.clipboard.write() to be called synchronously within | ||
| // the user gesture, but accepts a Promise inside ClipboardItem. | ||
| // This pattern works in Safari 13.1+, Chrome, and Firefox. | ||
| if (typeof ClipboardItem !== 'undefined' && navigator.clipboard?.write) { | ||
| const blobPromise = (async () => { | ||
| await fetchReadmeMarkdown() | ||
| const markdown = readmeMarkdownData.value?.markdown ?? '' | ||
| return new Blob([markdown], { type: 'text/plain' }) | ||
| })() | ||
| try { | ||
| await navigator.clipboard.write([new ClipboardItem({ 'text/plain': blobPromise })]) | ||
| return | ||
| } catch { | ||
| // Fall through to legacy approach | ||
| } | ||
| } | ||
|
|
||
| // Legacy fallback (non-Safari, or older browsers) | ||
| await fetchReadmeMarkdown() | ||
| const markdown = readmeMarkdownData.value?.markdown | ||
| if (!markdown) return | ||
|
|
||
| await copyReadme(markdown) | ||
| } |
There was a problem hiding this comment.
Missing UI feedback for the modern clipboard path.
When navigator.clipboard.write() succeeds (Safari path), the function returns without updating the copiedReadme state. The user won't see the "Copied" feedback that the legacy path provides via copyReadme().
🐛 Proposed fix
async function copyReadmeHandler() {
// Safari requires navigator.clipboard.write() to be called synchronously within
// the user gesture, but accepts a Promise inside ClipboardItem.
// This pattern works in Safari 13.1+, Chrome, and Firefox.
if (typeof ClipboardItem !== 'undefined' && navigator.clipboard?.write) {
const blobPromise = (async () => {
await fetchReadmeMarkdown()
const markdown = readmeMarkdownData.value?.markdown ?? ''
return new Blob([markdown], { type: 'text/plain' })
})()
try {
await navigator.clipboard.write([new ClipboardItem({ 'text/plain': blobPromise })])
+ // Manually trigger copied state for UI feedback
+ await copyReadme(readmeMarkdownData.value?.markdown ?? '')
return
} catch {
// Fall through to legacy approach
}
}
// Legacy fallback (non-Safari, or older browsers)
await fetchReadmeMarkdown()
const markdown = readmeMarkdownData.value?.markdown
if (!markdown) return
await copyReadme(markdown)
}Alternatively, if calling copyReadme again feels redundant, you could expose the copied ref setter from useClipboard or use a separate shallowRef to track copied state.
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
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
I think this is a duplicate of a couple of PRs unfortunately, and is groups a few too many changes together 😅
2 participants
🔗 Linked issue
resolves #2163
resolves #2168
resolves #2190
resolves #1826
🧭 Context
Several related issues on the package page: (1) the license shown was always from the latest version, not the currently viewed version; (2) when a non-latest version has a different license than the current latest, no visual indicator was shown; (3) the
hasDependenciescomputed was unused causing a lint error; (4) theLicenseDisplayprop was receiving a union type that TypeScript rejected.📚 Description
licenseChangedcomputed to comparedisplayVersion.value?.licenseagainstpkg.value?.license(the package-level license, which reflects the latest) —SlimVersiondoesn't carry license so usinglatestVersion.value?.licensewas always undefined.v-if="licenseChanged"witharia-labeltooltip text listing the latest license.hasDependenciescomputed that was causing a lint/type error.as stringforLicenseDisplay:licenseprop to satisfy TypeScript.changed_badgeandchangedkeys to the i18n schema for thepackage.licenseobject.Package/Dependencies.vueempty-state slots for no-deps display.Unit tests in
test/unit/app/utils/license-change.spec.tsverify thelicenseChangeddetection logic including null handling, object-shaped license normalization, and real-world cases.