fix: change readme md renderer behavior#1776
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughRefactors README rendering into a single-pass pipeline. Adds user-content naming utilities (USER_CONTENT_PREFIX, withUserContentPrefix, toUserContentId, toUserContentHash) and normalizePreservedAnchorAttrs. Centralises heading processing (semantic level, slug generation, TOC collection) and unifies Markdown and raw-HTML heading handling so IDs use user-content prefixes and duplicate slugs reflect document order. Consolidates URL resolution, playground-link collection, and security attribute application via processLink/resolveUrl with de-duplication. Renderer changes intercept raw HTML headings and anchors, normalise preserved attrs, ensure idempotent anchor/hash prefixing, and update transformTags to preserve data-level semantics. Tests expanded for TOC ordering, slug collisions, ID/anchor consistency, link resolution and single-pass behaviour. 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: 4
🧹 Nitpick comments (1)
test/unit/server/utils/readme.spec.ts (1)
579-584: Add a regression case foruser-content-slug collisions.This duplicate-slug test only covers identical heading text. Please add a case like
# Titleplus# user-content-titleto ensure generated IDs remain unique.🧪 Suggested additional test
it('handles duplicate raw HTML heading slugs', async () => { const md = '<h2>API</h2>\n\n<h2>API</h2>' const result = await renderReadmeHtml(md, 'test-pkg') expect(result.html).toContain('id="user-content-api"') expect(result.html).toContain('id="user-content-api-1"') }) + + it('keeps IDs unique when a heading slug already starts with user-content-', async () => { + const md = '# Title\n\n# user-content-title' + const result = await renderReadmeHtml(md, 'test-pkg') + const ids = result.toc.map(t => t.id) + expect(new Set(ids).size).toBe(ids.length) + }) })
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
server/utils/readme.ts (1)
490-495:⚠️ Potential issue | 🟠 MajorPreserve explicit heading ids when normalising raw HTML headings.
Lines [490-495] only carry
alignforward. A heading like<h2 id="custom-anchor">Install</h2>is rewritten with a generated id based onInstall, whileresolveUrl('#custom-anchor')still points links at#user-content-custom-anchor. That leaves no matching target and breaks existing in-document links. If you keep custom ids here, make sure they also participate in the duplicate-id tracking so later generated slugs still suffix in document order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cedcc33-23a5-4b86-88f3-9a02da10ce8d
📒 Files selected for processing (1)
server/utils/readme.ts
| if (url.startsWith('#')) { | ||
| // Prefix anchor links to match heading IDs (avoids collision with page IDs) | ||
| return `#user-content-${url.slice(1)}` | ||
| // Idempotent: don't double-prefix if already prefixed | ||
| return toUserContentHash(url.slice(1)) | ||
| } | ||
| // Absolute paths (e.g. /package/foo from a previous npmjs redirect) are already resolved | ||
| if (url.startsWith('/')) return url |
There was a problem hiding this comment.
Don’t fold # and //… into the new fast-path.
Lines [323-329] now turn href="#" into #user-content-, and protocol-relative URLs like //www.npmjs.com/package/foo return early as if they were root-relative. That breaks valid README links and skips the npmjs normalisation branch for //… URLs.
Possible fix
function resolveUrl(url: string, packageName: string, repoInfo?: RepositoryInfo): string {
if (!url) return url
+ if (url === '#') return url
if (url.startsWith('#')) {
// Prefix anchor links to match heading IDs (avoids collision with page IDs)
// Idempotent: don't double-prefix if already prefixed
return toUserContentHash(url.slice(1))
}
// Absolute paths (e.g. /package/foo from a previous npmjs redirect) are already resolved
- if (url.startsWith('/')) return url
+ if (url.startsWith('/') && !url.startsWith('//')) return url
if (hasProtocol(url, { acceptRelative: true })) {1 participant
🔗 Linked issue
resolves #1323
🧭 Context
📚 Description
This PR fixes README rendering issues caused by split processing between
markedandsanitizeHtml, and makes heading/link handling more consistent in mixed markdown + raw HTML content.What changed:
markedrendering.user-content-.align)title, even when placed beforehref)hrefand enforce saferel/targetwithout duplicates