fix(ui): resolve root-relative README markdown links to repo blob URLs - #2929
BittuBarnwal7479 wants to merge 13 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
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:
📝 WalkthroughWalkthroughUpdates ChangesRoot-relative URL resolution fix
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant resolveUrl as resolveUrl()
participant toLocalNpmxRedirect as toLocalNpmxRedirect()
participant RepoInfo as repoInfo
Caller->>resolveUrl: resolveUrl(url, repoInfo)
alt starts with $npmx-local:
resolveUrl-->>Caller: strip prefix, return path unchanged
else absolute path (/...)
resolveUrl->>RepoInfo: check rawBaseUrl
alt rawBaseUrl exists
resolveUrl-->>Caller: prefix with blobBaseUrl (.md) or rawBaseUrl (other)
else no rawBaseUrl
resolveUrl-->>Caller: return path unchanged
end
else redirectable npmjs URL
resolveUrl->>toLocalNpmxRedirect: wrap path+search+hash
toLocalNpmxRedirect-->>resolveUrl: $npmx-local:-prefixed path
resolveUrl-->>Caller: return local redirect
end
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ 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 |
| const result = await renderReadmeHtml(markdown, 'test-pkg', repoInfo) | ||
|
|
||
| expect(result.html).toContain('href="/package/test-pkg"') | ||
| }) |
There was a problem hiding this comment.
Is there a reason why we want this behaviour for non-markdown links?
Why not something similar to what is done for this?
npmx.dev/test/unit/server/utils/readme.spec.ts
Lines 219 to 227 in 46e7c59
There was a problem hiding this comment.
Good catch. I updated this to resolve root-relative non-markdown links via rawBaseUrl, consistent with existing relative-link handling. Markdown files still resolve via blobBaseUrl.
Local npmx routes (/package, /org, /search, etc.) are preserved separately, and I added regression tests covering both behaviors.
There was a problem hiding this comment.
Is there a reason / place where preserving Local npmx routes would be useful instead of them resolving to rawBaseUrl too?
There was a problem hiding this comment.
yes. the main case is npmjs links that the README renderer intentionally converts to local npmx routes. For example, https://www.npmjs.com/package/test-pkg becomes /package/test-pkg.
If we treated every root-relative path as a repo file, that converted route would incorrectly become rawBaseUrl/package/test-pkg.
So the updated logic preserves known npmx routes separately, while root-relative repo files like /CONTRIBUTING.md and /assets/logo.png resolve to blobBaseUrl / rawBaseUrl.
There was a problem hiding this comment.
Hmm, is there no way to differentiate a /package that was because of https://www.npmjs.com/package/test-pkg from a /package that someone wrote in their readme?
There was a problem hiding this comment.
good point. i will update this to preserve npmjs-originated links via an internal marker rather than path matching. README-authored root-relative paths now resolve normally against the repository. Added regression tests for both cases.
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)
352-358:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep protocol-relative URLs out of the root-relative rewrite.
//cdn.example.com/file.cssalso satisfiesurl.startsWith('/'), so with repository info it becomeshttps://github.com/ghraw/.../HEAD//cdn.example.com/file.cssinstead of staying external. The later protocol-relative handling never gets a chance to run.Proposed fix
- if (url.startsWith('/')) { + if (url.startsWith('/') && !url.startsWith('//')) { if (!repoInfo?.rawBaseUrl) { return url } const baseUrl = isMarkdownFile ? repoInfo.blobBaseUrl : repoInfo.rawBaseUrl return `${baseUrl}${url}` }Please add a regression case such as
[CDN](//cdn.example.com/file.css)withrepoInfoand assert the href remains protocol-relative.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/utils/readme.ts` around lines 352 - 358, The root-relative URL check at the beginning of the conditional block (url.startsWith('/')) is incorrectly matching protocol-relative URLs that start with //, causing them to be rewritten as repository URLs instead of remaining external. Add an additional condition to explicitly exclude protocol-relative URLs (those starting with //) before processing the root-relative URL case in the repoInfo block. Additionally, add a regression test case that verifies a markdown link with a protocol-relative URL like [CDN](//cdn.example.com/file.css) correctly preserves the protocol-relative href when repoInfo is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@server/utils/readme.ts`:
- Around line 352-358: The root-relative URL check at the beginning of the
conditional block (url.startsWith('/')) is incorrectly matching
protocol-relative URLs that start with //, causing them to be rewritten as
repository URLs instead of remaining external. Add an additional condition to
explicitly exclude protocol-relative URLs (those starting with //) before
processing the root-relative URL case in the repoInfo block. Additionally, add a
regression test case that verifies a markdown link with a protocol-relative URL
like [CDN](//cdn.example.com/file.css) correctly preserves the protocol-relative
href when repoInfo is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e84a9795-dd1d-41b7-b64c-33fc7a943e87
📒 Files selected for processing (2)
server/utils/readme.tstest/unit/server/utils/readme.spec.ts
|
@CodeRabbit review again. |
|
✅ Action performedReview finished.
|
| const npmJsHosts = new Set(['www.npmjs.com', 'npmjs.com', 'www.npmjs.org', 'npmjs.org']) | ||
|
|
||
| const USER_CONTENT_PREFIX = 'user-content-' | ||
| const LOCAL_NPMX_REDIRECT_PREFIX = '$npmx-local:' |
There was a problem hiding this comment.
is this because resolveUrl is being called on the same link twice?
There was a problem hiding this comment.
I do think it is, for changelog I prefix relative links for npmx with $ to prevent the resolveUrl/processUrl turning them into links for the git repo
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
|
9d5d82c to
e7c9284
Compare
# Conflicts: # server/utils/readme.ts
|
@ghostdevv Could you please review and merge this PR before new conflicts arise? thanks! Edit: I initially used Codex to merge it, but that failed. I've now resolved the conflicts manually :-) |
@BittuBarnwal7479 a maintainer will review your PR whenever they get time. Telling a maintainer to merge a PR will not make that happen faster |
Understood, thanks for the clarification. I won't ping maintainers for merges going forward. |
|
Hi @BittuBarnwal7479 👋 There seems to be an unresolved thread where you have not answered yet: #2929 (comment). Could you please explain your thoughts on that and verify Willow's assumptions? Also, I am not sure if this was addressed yet: #2929 (comment). The force commit makes it a little hard to follow. Can you make sure that all threads are resolved? That would make reviewing much easier 👍 |
|
Yes @WilcoSp , that is the reason. without an internal marker, the second pass cannot tell whether The marker is only internal and is stripped before output. Authored root-relative README links like Btw sorry for the late reply. |
|
I’ve addressed all reviewer comments from my side. If anything still needs clarification or another change, please let me know and I’ll update it. Thanks! |
ghostdevv
left a comment
There was a problem hiding this comment.
I pushed cc1accc (this PR) to align closer with the changelog renderer, but it should behave the same
npmx.dev/server/utils/changelog/markdown.ts
Lines 129 to 130 in cc1accc
WilcoSp
left a comment
There was a problem hiding this comment.
looks good, I did have a comment for the isMarkdownFileUrl function but it's not a stopper for me.
| function isMarkdownFileUrl(url: string): boolean { | ||
| return /\.(?:md|markdown)$/i.test(url.split('?')[0]?.split('#')[0] ?? '') | ||
| } | ||
|
|
There was a problem hiding this comment.
the isMarkdownFileUrl function could be moved to mdkit because changelog also needs to check whether an url is for markdown or not
There was a problem hiding this comment.
Would also be good to use URL.parse too, something like URL.parse(url)?.pathname.endsWith...
Thanks for opening this pull request! 🎉We really appreciate you taking the time to contribute, @BittuBarnwal7479. A maintainer will take a look as soon as they can. In the meantime, please make sure that:
If anything needs adjusting we'll leave comments here. Thanks again! |
🔗 Linked issue
Closes #2928
🧭 Context
Steps to Reproduce
Recording.2026-06-17.174239.mp4
📚 Description
On package readme.ts page, root-relative markdown links such as /CONTRIBUTING.md are resolved as local npmx routes instead of repository files.