Add support for line numbers in Markdown code fences #1381
Conversation
coreyasmith
force-pushed
the
markdown-code-fence-line-numbers
branch
from
83e9ea2 to
1e99d12
Compare
WalkthroughThe PR adds a new function Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. 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: 0
🧹 Nitpick comments (1)
packages/kg-markdown-html-renderer/test/markdown-html-renderer.test.js (1)
27-77: Consider adding edge case tests.While the current test coverage is good, consider adding tests for:
- Multiple language attributes
- Invalid
line-numbersplacement (as first word)- Empty info string
Example test cases:
it('handles multiple language attributes correctly', function () { const markdown = '```javascript experimental line-numbers\nconst foo = "bar";\n```'; const result = renderer.render(markdown, {ghostVersion: '4.0'}); result.should.containEql('class="line-numbers language-javascript"'); }); it('treats line-numbers as language when specified first', function () { const markdown = '```line-numbers javascript\nconst foo = "bar";\n```'; const result = renderer.render(markdown, {ghostVersion: '4.0'}); result.should.containEql('class="language-line-numbers"'); result.should.not.containEql('language-javascript'); }); it('handles empty info string correctly', function () { const markdown = '```\nconst foo = "bar";\n```'; const result = renderer.render(markdown, {ghostVersion: '4.0'}); result.should.not.containEql('line-numbers'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kg-markdown-html-renderer/lib/markdown-html-renderer.js(2 hunks)packages/kg-markdown-html-renderer/test/markdown-html-renderer.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Node 22.13.1
- GitHub Check: Node 20.11.1
🔇 Additional comments (4)
packages/kg-markdown-html-renderer/lib/markdown-html-renderer.js (2)
45-72: LGTM! ThefenceLineNumbersfunction implementation is well-structured.The implementation correctly handles:
- Fallback to default behavior when no info string is present
- Proper parsing of language and attributes
- Validation of the
line-numbersattribute position- Graceful addition of the class attribute
The code is properly attributed to its original source.
109-110: LGTM! ThefenceLineNumbersplugin is correctly integrated.The plugin is appropriately added only for Ghost versions 4.x and later, maintaining backward compatibility.
packages/kg-markdown-html-renderer/test/markdown-html-renderer.test.js (2)
27-46: LGTM! The test cases for Ghost v4.x are comprehensive.The tests properly verify:
- Presence of
line-numbersclass when specified- Correct combination with language class
68-77: LGTM! The test case for Ghost <4.x ensures backward compatibility.The test verifies that
line-numbersis not included for older Ghost versions, maintaining backward compatibility.
coreyasmith
force-pushed
the
markdown-code-fence-line-numbers
branch
from
16b6352 to
733f4ef
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1381 +/- ##
=======================================
Coverage 90.87% 90.88%
=======================================
Files 188 188
Lines 17887 17917 +30
Branches 2000 2006 +6
=======================================
+ Hits 16254 16283 +29
- Misses 1622 1623 +1
Partials 11 11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/kg-markdown-html-renderer/lib/markdown-html-renderer.js (1)
63-63: Consider using word boundaries for more precise matching.The current implementation uses
info.split(/(\s+)/g).slice(2).join('')which might not handle all edge cases properly. The existing@jwevans/markdown-it-code-block-line-numbersplugin supports multiple modifiers like "showNumbers, showLineNumbers, show-numbers, show-line-numbers, lineNumbers, or line-numbers", suggesting that a more flexible approach might be valuable.Consider this more robust alternative:
- // line-numbers must come after the first word in the info string to be rendered. - // Example: ```ruby line-numbers - // If line-numbers is specified as the first word, fallback to the default behavior - // (i.e., treat line-numbers as the language). - const langAttrs = info.split(/(\s+)/g).slice(2).join(''); - const attributeRegex = new RegExp(`\\b${attribute}\\b`); - if (!langAttrs || !attributeRegex.test(langAttrs)) { + // Extract everything after the first word (language) + const infoWords = info.trim().split(/\s+/); + if (infoWords.length < 2 || !infoWords.includes(attribute)) { return defaultFence(tokens, idx, options, env, self); }packages/kg-markdown-html-renderer/test/markdown-html-renderer.test.js (1)
33-34: Test assertions could be more specific about class order.The current tests use
containEqlwhich checks for substring presence but doesn't verify the exact class structure. Consider making the assertions more precise to ensure the class attribute is properly formatted.Consider making the test assertions more specific:
- result.should.containEql('class="line-numbers language-javascript"'); + result.should.match(/class="line-numbers language-javascript"/); - result.should.containEql('class="language-javascript"'); - result.should.not.containEql('line-numbers'); + result.should.match(/class="language-javascript"(?!.*line-numbers)/);Also applies to: 43-45
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/kg-markdown-html-renderer/lib/markdown-html-renderer.js(2 hunks)packages/kg-markdown-html-renderer/test/markdown-html-renderer.test.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Cursor Bugbot
- GitHub Check: Node 20.11.1
- GitHub Check: Node 22.13.1
🔇 Additional comments (4)
packages/kg-markdown-html-renderer/lib/markdown-html-renderer.js (2)
45-72: LGTM! Well-implemented fence line numbers plugin.The implementation follows markdown-it plugin conventions correctly and handles edge cases appropriately. The logic ensures that
line-numbersmust come after the language specification and uses proper regex matching to detect the attribute.
109-110: Confirm fenceLineNumbers applied only to Ghost 4.x+
selectRenderer’s<4.xbranch omits.use(fenceLineNumbers)while the>=4.xbranch adds it, so the plugin is correctly isolated.packages/kg-markdown-html-renderer/test/markdown-html-renderer.test.js (2)
27-46: LGTM! Comprehensive test coverage for the new functionality.The tests properly verify both positive and negative cases for the line-numbers feature. The test structure correctly validates that the
line-numbersclass is added when specified and not added when omitted.
68-77: LGTM! Correct verification of backward compatibility.The test properly validates that Ghost versions prior to 4.x do not support the line-numbers functionality, ensuring backward compatibility is maintained as intended.
2 participants
Older versions of Ghost (2 and earlier?) used to support line numbers in Markdown code fences by adding
line-numbersafter the language:This would be rendered as:
The Prism Line Numbers plugin would then render line numbers for that code block.
This pull request adds in a markdown-it plugin that I implemented called markdown-it-fence-line-numbers to restore this functionality to Ghost's Markdown cards.