feat: adding gh changelog/releases within npmx#1233
feat: adding gh changelog/releases within npmx#1233WilcoSp wants to merge 54 commits intonpmx-dev:mainfrom
Conversation
…md -> html does need to change)
|
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
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
excludes changelog releases due to needing api calls
WilcoSp
changed the title
|
After a lot of work, this pr is now ready for review. if there is something that needs to be changed in this pr let me know. |
WilcoSp
changed the title
|
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:
📝 WalkthroughWalkthroughAdds a changelog feature and supporting infrastructure: new Vue components under app/components/Changelog (Card.vue, Markdown.vue, Releases.vue); composables usePackageChangelog and useProviderIcon; a new page app/pages/package-changes/[...path].vue and package page changes to surface changelogs; server APIs for changelog info, markdown and releases (server/api/changelog/{info,md,releases}); markdown rendering/sanitisation and detection utilities (server/utils/changelog/{markdown.ts,detectChangelog.ts}); adjustments to server/utils/readme.ts exports; new types and schemas (shared/types/changelog.ts, shared/schemas/changelog/release.ts); wide i18n key rename from common.view_on_npm → common.view_on; tests and a11y test updates. Possibly related PRs
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}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
server/utils/readme.ts (1)
135-195: Harden exported sanitiser allow-lists against accidental mutation.Now that these are exported, freezing/readonly typing would prevent downstream code from mutating sanitisation policy at runtime.
🔒 Suggested hardening
-export const ALLOWED_TAGS = [ +export const ALLOWED_TAGS: readonly string[] = Object.freeze([ 'h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'br', 'hr', 'ul', 'ol', 'li', 'blockquote', 'pre', 'code', 'a', 'strong', 'em', 'del', 's', 'table', 'thead', 'tbody', 'tr', 'th', 'td', 'img', 'picture', 'source', 'details', 'summary', 'div', 'span', 'sup', 'sub', 'kbd', 'mark', 'button', -] +]) -export const ALLOWED_ATTR: Record<string, string[]> = { +export const ALLOWED_ATTR: Readonly<Record<string, readonly string[]>> = Object.freeze({ '*': ['id'], // Allow id on all tags 'a': ['href', 'title', 'target', 'rel', 'content-none'], 'img': ['src', 'alt', 'title', 'width', 'height', 'align'], 'source': ['src', 'srcset', 'type', 'media'], 'button': ['class', 'title', 'type', 'aria-label', 'data-copy'], 'th': ['colspan', 'rowspan', 'align', 'valign', 'width'], 'td': ['colspan', 'rowspan', 'align', 'valign', 'width'], 'h3': ['data-level', 'align'], 'h4': ['data-level', 'align'], 'h5': ['data-level', 'align'], 'h6': ['data-level', 'align'], 'blockquote': ['data-callout'], 'details': ['open'], 'code': ['class'], 'pre': ['class', 'style'], 'span': ['class', 'style'], 'div': ['class', 'style', 'align'], 'p': ['align'], -} +})shared/schemas/changelog/release.ts (1)
3-18: Fix the exportedSchamatypo before this API spreads further.The misspelling is now part of exported symbols, so it will propagate to every consumer.
As per coding guidelines "Use clear, descriptive variable and function names".Suggested refactor
-export const GithubReleaseSchama = v.object({ +export const GithubReleaseSchema = v.object({ id: v.pipe(v.number(), v.integer()), name: v.nullable(v.string()), tag: v.string(), draft: v.boolean(), prerelease: v.boolean(), - markdown: v.nullable(v.string()), // can be null if no descroption was made + markdown: v.nullable(v.string()), // can be null if no description was made publishedAt: v.pipe(v.string(), v.isoTimestamp()), }) -export const GithubReleaseCollectionSchama = v.object({ - releases: v.array(GithubReleaseSchama), +export const GithubReleaseCollectionSchema = v.object({ + releases: v.array(GithubReleaseSchema), }) -export type GithubRelease = v.InferOutput<typeof GithubReleaseSchama> -export type GithubReleaseCollection = v.InferOutput<typeof GithubReleaseCollectionSchama> +export type GithubRelease = v.InferOutput<typeof GithubReleaseSchema> +export type GithubReleaseCollection = v.InferOutput<typeof GithubReleaseCollectionSchema>server/api/changelog/md/[provider]/[owner]/[repo]/[...path].get.ts (1)
22-23: Remove request-time debug logging.
console.log({ provider })will spam logs in normal traffic and does not add operational value here.Suggested fix
- console.log({ provider }) - switch (provider as ProviderId) {server/utils/changelog/detectChangelog.ts (2)
129-149: Avoid sequential HEAD probes for changelog filenames.This loop performs one network request at a time, so miss-heavy repos pay the full cumulative latency.
Suggested refactor
- for (const fileName of CHANGELOG_FILENAMES) { - const exists = await fetch(`${baseUrl.raw}/${fileName}`, { - headers: { - // GitHub API requires User-Agent - 'User-Agent': 'npmx.dev', - }, - method: 'HEAD', // we just need to know if it exists or not - }) - .then(r => r.ok) - .catch(() => false) - if (exists) { - return { - type: 'md', - provider: ref.provider, - path: fileName, - repo: `${ref.owner}/${ref.repo}`, - link: `${baseUrl.blob}/${fileName}`, - } satisfies ChangelogMarkdownInfo - } - } - return false + const checks = await Promise.all( + CHANGELOG_FILENAMES.map(async fileName => { + const exists = await fetch(`${baseUrl.raw}/${fileName}`, { + headers: { + 'User-Agent': 'npmx.dev', + }, + method: 'HEAD', + }) + .then(r => r.ok) + .catch(() => false) + return { fileName, exists } + }), + ) + + const match = checks.find(c => c.exists)?.fileName + if (!match) { + return false + } + + return { + type: 'md', + provider: ref.provider, + path: match, + repo: `${ref.owner}/${ref.repo}`, + link: `${baseUrl.blob}/${match}`, + } satisfies ChangelogMarkdownInfo }
19-20: Remove stale commented-out code.These commented sections make the flow harder to scan and maintain.
As per coding guidelines "Add comments only to explain complex logic or non-obvious implementations".
Also applies to: 54-73
app/components/Changelog/Card.vue (1)
38-42: Unnecessary optional chaining on required prop.
releaseis a required prop (non-optional indefineProps), sorelease?.toccan be simplified torelease.toc.✏️ Suggested simplification
<ReadmeTocDropdown - v-if="release?.toc && release.toc.length > 1" + v-if="release.toc && release.toc.length > 1" :toc="release.toc" class="ms-auto" />app/pages/package-changes/[...path].vue (1)
112-114: Use strict equality for type comparisons.Consider using
===instead of==for string comparisons to maintain consistency with TypeScript best practices.✏️ Suggested change
- <LazyChangelogReleases v-if="changelog.type == 'release'" :info="changelog" /> + <LazyChangelogReleases v-if="changelog.type === 'release'" :info="changelog" /> <LazyChangelogMarkdown - v-else-if="changelog.type == 'md'" + v-else-if="changelog.type === 'md'"app/pages/package/[[org]]/[name].vue (1)
974-981: Simplify the truthiness check.The double negation
!!changelogis unnecessary in av-ifdirective since Vue already evaluates truthiness.✏️ Suggested simplification
- <li v-if="!!changelog && resolvedVersion"> + <li v-if="changelog && resolvedVersion">
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (74)
app/components/Changelog/Card.vueapp/components/Changelog/Markdown.vueapp/components/Changelog/Releases.vueapp/components/Readme.vueapp/composables/usePackageChangelog.tsapp/composables/useProviderIcon.tsapp/pages/org/[org].vueapp/pages/package-changes/[...path].vueapp/pages/package/[[org]]/[name].vueapp/pages/~[username]/index.vuei18n/locales/ar.jsoni18n/locales/az-AZ.jsoni18n/locales/bg-BG.jsoni18n/locales/bn-IN.jsoni18n/locales/cs-CZ.jsoni18n/locales/de-DE.jsoni18n/locales/en.jsoni18n/locales/es.jsoni18n/locales/fr-FR.jsoni18n/locales/hi-IN.jsoni18n/locales/hu-HU.jsoni18n/locales/id-ID.jsoni18n/locales/it-IT.jsoni18n/locales/ja-JP.jsoni18n/locales/mr-IN.jsoni18n/locales/nb-NO.jsoni18n/locales/ne-NP.jsoni18n/locales/pl-PL.jsoni18n/locales/pt-BR.jsoni18n/locales/ru-RU.jsoni18n/locales/ta-IN.jsoni18n/locales/te-IN.jsoni18n/locales/uk-UA.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.jsonlunaria/files/ar-EG.jsonlunaria/files/az-AZ.jsonlunaria/files/bg-BG.jsonlunaria/files/bn-IN.jsonlunaria/files/cs-CZ.jsonlunaria/files/de-DE.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/es-419.jsonlunaria/files/es-ES.jsonlunaria/files/fr-FR.jsonlunaria/files/hi-IN.jsonlunaria/files/hu-HU.jsonlunaria/files/id-ID.jsonlunaria/files/it-IT.jsonlunaria/files/ja-JP.jsonlunaria/files/mr-IN.jsonlunaria/files/nb-NO.jsonlunaria/files/ne-NP.jsonlunaria/files/pl-PL.jsonlunaria/files/pt-BR.jsonlunaria/files/ru-RU.jsonlunaria/files/ta-IN.jsonlunaria/files/te-IN.jsonlunaria/files/uk-UA.jsonlunaria/files/zh-CN.jsonlunaria/files/zh-TW.jsonserver/api/changelog/info/[...pkg].get.tsserver/api/changelog/md/[provider]/[owner]/[repo]/[...path].get.tsserver/api/changelog/releases/[provider]/[owner]/[repo].get.tsserver/utils/changelog/detectChangelog.tsserver/utils/changelog/markdown.tsserver/utils/readme.tsshared/schemas/changelog/release.tsshared/types/changelog.tsshared/utils/constants.tstest/nuxt/a11y.spec.tstest/unit/a11y-component-coverage.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/utils/changelog/markdown.ts (1)
32-34: Simplify the template literal.The template literal on line 32 has redundant string concatenation with an empty string.
♻️ Suggested simplification
- const intermediateTitleAttr = `${` data-title-intermediate="${plainText || title}"`}` + const intermediateTitleAttr = ` data-title-intermediate="${plainText || title}"`
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/components/Changelog/Card.vueapp/components/Changelog/Markdown.vueapp/composables/useProviderIcon.tsapp/pages/package-changes/[...path].vuei18n/locales/en.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonserver/utils/changelog/markdown.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- app/composables/useProviderIcon.ts
- i18n/locales/en.json
- lunaria/files/en-GB.json
- app/components/Changelog/Markdown.vue
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/pages/package-changes/[...path].vue (2)
44-44: Consider removing commented-out code.The commented-out
filePathOrigcomputed property appears unused. If it's not needed for upcoming work, consider removing it to reduce noise.
95-102: Consider adding optional chaining forchangelog.provider.The
v-ifguards onchangelog?.link, butchangelog.provideris accessed without optional chaining on lines 99 and 101. If the type guaranteesproviderexists wheneverlinkdoes, this is fine; otherwise, addingchangelog?.providerwould be more defensive.🛡️ Suggested defensive change
<LinkBase v-if="changelog?.link" :to="changelog?.link" :classicon="repoProviderIcon" - :title="$t('common.view_on', { site: changelog.provider })" + :title="$t('common.view_on', { site: changelog?.provider })" > - {{ changelog.provider }} + {{ changelog?.provider }} </LinkBase>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
server/utils/changelog/markdown.ts (1)
263-266: Remove commented-out code.This commented code is superseded by the cleaner implementation at lines 274-277. Dead comments reduce readability.
🧹 Proposed fix
const baseUrl = isMarkdownFile ? repoInfo.blobBaseUrl : repoInfo.rawBaseUrl - // if (url.startsWith('./') || url.startsWith('../')) { - // // url constructor handles relative paths - // return checkResolvedUrl(new URL(url, `${baseUrl}/${repoInfo.path ?? ''}`).href, baseUrl) - // } - if (url.startsWith('/')) {
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/pages/package-changes/[...path].vueserver/utils/changelog/markdown.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/pages/package-changes/[...path].vue
| renderer.link = function ({ href, title, tokens }: Tokens.Link) { | ||
| const text = this.parser.parseInline(tokens) | ||
| const titleAttr = title ? ` title="${title}"` : '' | ||
| const plainText = text.replace(/<[^>]*>/g, '').trim() | ||
|
|
||
| if (href.startsWith('mailto:') && !EMAIL_REGEX.test(plainText)) { | ||
| return text | ||
| } | ||
|
|
||
| const intermediateTitleAttr = `data-title-intermediate="${plainText || title}"` | ||
|
|
||
| return `<a href="${href}"${titleAttr}${intermediateTitleAttr} target="_blank">${text}</a>` | ||
| } |
There was a problem hiding this comment.
Anchor links will incorrectly open in a new tab.
The renderer unconditionally adds target="_blank" to all links, including anchor links like #heading. This breaks same-page navigation — clicking an anchor should scroll to that heading, not open a new tab.
🐛 Proposed fix to conditionally apply target
renderer.link = function ({ href, title, tokens }: Tokens.Link) {
const text = this.parser.parseInline(tokens)
const titleAttr = title ? ` title="${title}"` : ''
const plainText = text.replace(/<[^>]*>/g, '').trim()
if (href.startsWith('mailto:') && !EMAIL_REGEX.test(plainText)) {
return text
}
- const intermediateTitleAttr = `data-title-intermediate="${plainText || title}"`
+ // Only open external links in new tab; anchor links should navigate in-page
+ const targetAttr = href.startsWith('#') ? '' : ' target="_blank"'
- return `<a href="${href}"${titleAttr}${intermediateTitleAttr} target="_blank">${text}</a>`
+ return `<a href="${href}"${titleAttr}${targetAttr}>${text}</a>`
}Note: The data-title-intermediate attribute is stripped by sanitize-html since it's not in ALLOWED_ATTR. If this attribute is needed downstream, add it to the allowed list; otherwise, it can be removed from the renderer.
| div: prefixId, | ||
| p: prefixId, | ||
| span: prefixId, | ||
| section: prefixId, | ||
| article: prefixId, | ||
| }, |
There was a problem hiding this comment.
Dead code: section and article transforms will never apply.
The transformTags entries for section and article are never executed because neither tag is included in ALLOWED_TAGS. sanitize-html removes disallowed tags before transforms run.
🧹 Proposed fix to remove dead code
div: prefixId,
p: prefixId,
span: prefixId,
- section: prefixId,
- article: prefixId,
},
})If these tags should be supported, add them to ALLOWED_TAGS in server/utils/readme.ts instead.
📝 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.
| div: prefixId, | |
| p: prefixId, | |
| span: prefixId, | |
| section: prefixId, | |
| article: prefixId, | |
| }, | |
| div: prefixId, | |
| p: prefixId, | |
| span: prefixId, | |
| }, |
There was a problem hiding this comment.
rather keep the dead code in case at the readme.ts then allow those tags
There was a problem hiding this comment.
@WilcoSp, understood! Keeping those transforms in place for future use is a reasonable approach if section and article might be added to ALLOWED_TAGS later.
You might consider adding a brief comment above those lines to clarify they're intentionally included for future compatibility, which would help other reviewers understand the intent:
div: prefixId,
p: prefixId,
span: prefixId,
// Reserved for future use if section/article are added to ALLOWED_TAGS
section: prefixId,
article: prefixId,This way the code documents the architectural decision. But it's entirely optional—your call!
🧠 Learnings used
Learnt from: CR
Repo: npmx-dev/npmx.dev PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-02-03T10:09:38.777Z
Learning: Applies to **/*.{ts,tsx,vue} : Add comments only to explain complex logic or non-obvious implementations
There was a problem hiding this comment.
♻️ Duplicate comments (1)
server/utils/changelog/markdown.ts (1)
23-35:⚠️ Potential issue | 🟠 MajorAnchor links will incorrectly open in a new tab.
The renderer unconditionally adds
target="_blank"to all links at line 34, including anchor links like#heading. This breaks same-page navigation — clicking an anchor should scroll to that heading, not open a new tab. This was flagged in a previous review but appears unaddressed.🐛 Proposed fix to conditionally apply target
renderer.link = function ({ href, title, tokens }: Tokens.Link) { const text = this.parser.parseInline(tokens) const titleAttr = title ? ` title="${title}"` : '' const plainText = text.replace(/<[^>]*>/g, '').trim() if (href.startsWith('mailto:') && !EMAIL_REGEX.test(plainText)) { return text } - const intermediateTitleAttr = `data-title-intermediate="${plainText || title}"` - - return `<a href="${href}"${titleAttr}${intermediateTitleAttr} target="_blank">${text}</a>` + // Only open external links in new tab; anchor links should navigate in-page + const targetAttr = href.startsWith('#') ? '' : ' target="_blank"' + + return `<a href="${href}"${titleAttr}${targetAttr}>${text}</a>` }Note: The
data-title-intermediateattribute will be stripped bysanitize-htmlunless it's added toALLOWED_ATTR. If this attribute is needed downstream, add it to the allowed list; otherwise, it can be removed from the renderer as shown above.
🧹 Nitpick comments (3)
shared/utils/constants.ts (2)
21-21: Inconsistent message casing and grammar.This message starts with lowercase 'failed' whereas all other error messages in this file use sentence case (e.g., 'Failed to fetch...', 'Package name...'). Additionally, the phrasing 'detect package has changelog' reads awkwardly.
✏️ Suggested fix for consistency
-export const ERROR_PACKAGE_DETECT_CHANGELOG = 'failed to detect package has changelog' +export const ERROR_PACKAGE_DETECT_CHANGELOG = 'Failed to detect changelog for package.'
39-43: Consider grouping changelog constants and aligning naming pattern.Two minor observations:
Organisation:
ERROR_PACKAGE_DETECT_CHANGELOG(line 21) is separated from the other changelog-related constants here. Consider grouping them together for maintainability.Naming:
ERROR_THROW_INCOMPLETE_PARAMuses "THROW" which is an implementation detail. Other constants follow noun-phrase patterns likeERROR_*_FAILEDorERROR_*_NOT_FOUND. A name likeERROR_INCOMPLETE_PARAMETERSorERROR_MISSING_PARAMETERSwould be more consistent.✏️ Suggested rename
-export const ERROR_THROW_INCOMPLETE_PARAM = "Couldn't do request due to incomplete parameters" +export const ERROR_INCOMPLETE_PARAMETERS = "Couldn't complete request due to incomplete parameters"server/utils/changelog/markdown.ts (1)
214-215: Consider adding a comment to document the intentional dead code.As discussed in a previous review,
sectionandarticletransforms are currently dead code since these tags aren't inALLOWED_TAGS. Since you've chosen to keep them for future compatibility, a brief comment would help future reviewers understand this is intentional.📝 Suggested documentation
div: prefixId, p: prefixId, span: prefixId, + // Reserved for future use if section/article are added to ALLOWED_TAGS section: prefixId, article: prefixId,
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/pages/package/[[org]]/[name].vuei18n/locales/de-DE.jsoni18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/locales/kn-IN.jsoni18n/schema.jsonlunaria/files/de-DE.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/fr-FR.jsonlunaria/files/kn-IN.jsonserver/utils/changelog/markdown.tsshared/utils/constants.tstest/nuxt/a11y.spec.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- i18n/locales/de-DE.json
- test/nuxt/a11y.spec.ts
- i18n/locales/fr-FR.json
- lunaria/files/de-DE.json
- lunaria/files/en-US.json
- lunaria/files/en-GB.json
- lunaria/files/fr-FR.json
2 participants
This pr will add the possibility to view the changelog.md & releases from a package's github repo within npmx.
This will make it easier to see the changelogs while not needing to leave npmx and allowing quicker access.
This pr is the first pr of #501
Preview here
While I was making this PR antfu also made this pr #1368 and here my comment from his pr