fix: improve anchors parsing in readmes#2486
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR addresses two separate issues: removing a class binding from the PackageHeader component and refactoring README heading and link rendering logic to prevent anchor-within-anchor HTML hydration problems whilst handling anchor-wrapped headings. Changes
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/utils/readme.ts`:
- Around line 552-555: The issue is that htmlAnchorRe (global /gi) is used with
.test() inside processHeading which mutates lastIndex and interferes with the
later .replace() that uses the same regex; fix by using a non-global regex when
testing or cloning the regex before use: inside processHeading, replace uses of
htmlAnchorRe.test(displayHtml) with testing against a non-global instance (e.g.
new RegExp(htmlAnchorRe.source, htmlAnchorRe.flags.replace('g','')) or simply
create /i-only version) or otherwise ensure lastIndex is reset
(htmlAnchorRe.lastIndex = 0) before the subsequent .replace(); keep the
.replace() usage unchanged but ensure it receives an unmutated/global-intended
regex.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: de042ff2-6a07-479e-aae8-49e64494082d
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name].vueserver/utils/readme.ts
💤 Files with no reviewable changes (1)
- app/pages/package/[[org]]/[name].vue
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
would it be possible to add some test coverage for this? it's pretty complex parsing logic!
| // The browser doesn't support anchors within anchors and automatically extracts them from each other, | ||
| // causing a hydration error. To prevent this from happening in such cases, we use the anchor separately | ||
| if (htmlAnchorRe.test(displayHtml)) { | ||
| return `<h${semanticLevel} id="${id}" data-level="${depth}"${preservedAttrs}>${displayHtml}<a href="#${id}"></a></h${semanticLevel}>\n` |
There was a problem hiding this comment.
Could we just always do it this way rather than conditionally? 🤔
There was a problem hiding this comment.
Probably more UX question - is it appropriate for us to only be able to click the anchor by hovering over that side area, not the entire header? I didn't want to break the current UX, so I just added a condition for this rare, specific case with anchor inside heading
|
|
||
| renderer.heading = function ({ tokens, depth }: Tokens.Heading) { | ||
| const displayHtml = this.parser.parseInline(tokens) | ||
| const anchorTokenRegex = /^<a(\s.+)?\/?>$/ |
There was a problem hiding this comment.
perf nit: we could define this constant regex outside this renderer.heading so it doesn't get recompiled on every heading call.
2 participants
🧭 Context
Some packages (like nuxt) add anchors to headers themselves to control them in other services. This, coupled with the fact that we wrapped all header content in a link, caused hydration errors and wrong rendering
Added processing for a few common cases
The class from the other file does not exist, but it caused warning with several thousand lines description and was terribly interfering