fix: preserve backtick spans in markdown#1608
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaces HTML-stripping regexes in app/composables/useMarkdown.ts so they match either backtick-enclosed code spans or HTML (tags and comments); when a backtick span is matched it is preserved, otherwise HTML tags/comments are removed. Adds a regression test suite test/nuxt/composables/use-markdown.spec.ts verifying HTML inside backtick code spans is preserved/escaped and HTML outside backticks (including comments) is stripped. 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.
🧹 Nitpick comments (1)
app/composables/useMarkdown.ts (1)
36-41: LGTM — the alternation correctly prioritises backtick spans over HTML tags.A few confirmatory notes on the implementation:
- Operator choice:
codeSpan ?? ''is the right choice over||— it distinguishes anundefinedcapture (second branch matched) from a genuine empty code span string, and is semantically precise.- Security: backtick-preserved spans still pass through the HTML entity-escaping block (lines 60–64) before output, so
<,>etc. inside code spans are correctly escaped to</>. No XSS surface is introduced.iflag: the flag has no effect on the backtick branch (backtick characters have no case), and correctly makes HTML tag matching case-insensitive for the second branch (<DIV>,<Br/>, etc.).Optional edge case to be aware of: the backtick pattern
`[^`]*`recognises only single-backtick code spans. A double-backtick span such as`` code with <tag> inside ``` would be parsed as two empty single-backtick spans — the HTML tags in between would still be stripped. This is a pre-existing limitation of the simple inline parser and out of scope for this fix, but worth documenting if double-backtick spans are ever encountered in package descriptions.📝 Optional: extend the pattern to handle double-backtick spans
- /(`[^`]*`)|<\/?[a-z][^>]*>/gi, + /(``[^`].*?``|`[^`]*`)|<\/?[a-z][^>]*>/gi,This would match
`` … ``spans first (greedy enough to skip embedded single backticks), then fall back to the single-backtick branch.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/nuxt/composables/use-markdown.spec.ts (1)
354-398: Consider adding a test for an unclosed backtick span containing an HTML tag.No test covers the scenario where a backtick is opened but never closed, e.g.,
'Use `<div> for layout'(missing closing backtick). Depending on the regex inuseMarkdown.ts, the entire trailing content could be swallowed as code rather than having the<div>stripped — a subtle regression surface.💡 Suggested additional test case
+ it('handles unclosed backtick span with HTML tag gracefully', () => { + // No closing backtick — the <div> should still be stripped as bare HTML + const processed = useMarkdown({ text: 'Use `<div> for layout' }) + expect(processed.value).not.toContain('<div>') + })
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/composables/useMarkdown.tstest/nuxt/composables/use-markdown.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- app/composables/useMarkdown.ts
danielroe
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/composables/useMarkdown.ts (1)
38-48: Prefix the unusedmatchparameter with_in both replace callbacks.
matchis never referenced in either callback body. Prefixing it signals intentional non-use and avoids lint warnings.♻️ Proposed fix
stripped = stripped.replace( /(`[^`]*`)|<\/?[a-z][^>]*>/gi, - (match, codeSpan: string | undefined) => codeSpan ?? '', + (_match, codeSpan: string | undefined) => codeSpan ?? '', ) // Strip HTML comments: <!-- ... --> (including unclosed comments from truncation) stripped = stripped.replace( /(`[^`]*`)|<!--[\s\S]*?(-->|$)/g, - (match, codeSpan: string | undefined) => codeSpan ?? '', + (_match, codeSpan: string | undefined) => codeSpan ?? '', )
|
Thanks for your first contribution, @vmrjnvc! 🥳 We'd love to welcome you to the npmx community. Come and say hi on Discord! And once you've joined, visit npmx.wamellow.com to claim the contributor role. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Co-authored-by: Daniel Roe <daniel@roe.dev>
2 participants
🔗 Linked issue
Resolves #1478
📚 Description
This update fixes an issue with package descriptions containing code snippets wrapped in backticks. Previously, the HTML stripping function removed HTML tags no matter if they were wrapped with backticks. Because of that short description of packages was broken when code snippets with html tags were processed.