fix: parse hyphenated example fence languages#2734
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
Hello! Thank you for opening your first PR to npmx, @lisiqi1983! 🚀 Here’s what will happen next:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR updates JSDoc ChangesHyphenated JSDoc Example Language Support
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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 |
gameroman
added
the
needs review
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/server/utils/docs/render.spec.ts (1)
214-214: ⚡ Quick winMake the
-tsassertion less brittle.
expect(html).not.toContain('-ts')is too broad and can fail on valid highlighted HTML changes unrelated to this regression.Proposed test assertion tightening
- expect(html).not.toContain('-ts') + // Ensure the raw leaked suffix token is not rendered as standalone content. + expect(html).not.toMatch(/(^|[>\s])-ts([<\s]|$)/)🤖 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 `@test/unit/server/utils/docs/render.spec.ts` at line 214, The current assertion expect(html).not.toContain('-ts') is too broad; tighten it to only fail if the literal "-ts" appears inside rendered code/inline-code elements (the html variable in render.spec.ts). Replace the broad contains check with an assertion that searches html for "-ts" specifically within <code> or <pre> elements (or the renderer's code wrapper class used in the test) — e.g., assert that a regex matching "-ts" inside code tags does not match — so unrelated highlighted HTML changes won't trigger the test; target the html variable and the test surrounding this expect to implement the narrower check.
🤖 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.
Nitpick comments:
In `@test/unit/server/utils/docs/render.spec.ts`:
- Line 214: The current assertion expect(html).not.toContain('-ts') is too
broad; tighten it to only fail if the literal "-ts" appears inside rendered
code/inline-code elements (the html variable in render.spec.ts). Replace the
broad contains check with an assertion that searches html for "-ts" specifically
within <code> or <pre> elements (or the renderer's code wrapper class used in
the test) — e.g., assert that a regex matching "-ts" inside code tags does not
match — so unrelated highlighted HTML changes won't trigger the test; target the
html variable and the test surrounding this expect to implement the narrower
check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1b98042a-5581-4613-a61d-c512ce450800
📒 Files selected for processing (2)
server/utils/docs/render.tstest/unit/server/utils/docs/render.spec.ts
| const langMatch = example.doc.match(/```(\w+)?/) | ||
| const langMatch = example.doc.match(/```[ \t]*([^` \t\r\n]*)/) | ||
| const lang = langMatch?.[1] || 'typescript' | ||
| const code = example.doc.replace(/```\w*\n?/g, '').trim() | ||
| const code = example.doc.replace(/```[ \t]*[^` \t\r\n]*[ \t]*(?:\r\n|\r|\n)?/g, '').trim() |
There was a problem hiding this comment.
It seems to me that the new exp allows extra characters
If the previous one didn't include only the dash, I would suggest adding only it
([\w\-]+)Or
([\w\-_]+)There was a problem hiding this comment.
Thanks, good catch. I narrowed the fence language matching back to the original \w set plus -, so it no longer accepts arbitrary non-whitespace characters. I also tightened the regression assertion so it only checks for a leaked standalone -ts token instead of rejecting any incidental -ts substring.
Validated locally:
STORYBOOK=true pnpm vp test --project unit test/unit/server/utils/docs/render.spec.tspnpm vp lint server/utils/docs/render.ts test/unit/server/utils/docs/render.spec.tspnpm vp fmt --check server/utils/docs/render.ts test/unit/server/utils/docs/render.spec.tsgit diff --check -- server/utils/docs/render.ts test/unit/server/utils/docs/render.spec.ts
lisiqi1983
force-pushed
the
fix/example-fence-language
branch
from
4fb8e09 to
59d245d
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/utils/docs/render.ts (1)
194-196: 💤 Low valueConsider escaping hyphens in character classes for clarity.
The character class
[\w-]is valid (hyphen at the end is treated as literal), but explicitly escaping it as[\w\-]would be clearer and addresses the previous reviewer's suggestion. This applies to both the language-matching regex (line 194) and the fence-removal regex (line 196).📝 Proposed refinement
- const langMatch = example.doc.match(/```[ \t]*([\w-]+)?/) + const langMatch = example.doc.match(/```[ \t]*([\w\-]+)?/) const lang = langMatch?.[1] || 'typescript' - const code = example.doc.replace(/```[ \t]*[\w-]*[ \t]*(?:\r\n|\r|\n)?/g, '').trim() + const code = example.doc.replace(/```[ \t]*[\w\-]*[ \t]*(?:\r\n|\r|\n)?/g, '').trim()🤖 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/docs/render.ts` around lines 194 - 196, Update the two regexes that parse code fences to escape the hyphen for clarity: replace occurrences of [\w-] in the language capture regex used to compute langMatch (the expression that sets langMatch from example.doc) and in the fence-removal regex used to compute code (the expression that calls example.doc.replace(...)). Change [\w-] to [\w\-] in both places so the character class explicitly treats the hyphen as a literal.
🤖 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.
Nitpick comments:
In `@server/utils/docs/render.ts`:
- Around line 194-196: Update the two regexes that parse code fences to escape
the hyphen for clarity: replace occurrences of [\w-] in the language capture
regex used to compute langMatch (the expression that sets langMatch from
example.doc) and in the fence-removal regex used to compute code (the expression
that calls example.doc.replace(...)). Change [\w-] to [\w\-] in both places so
the character class explicitly treats the hyphen as a literal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 15a4043b-7718-4d85-9c68-399c7d9e9048
📒 Files selected for processing (2)
server/utils/docs/render.tstest/unit/server/utils/docs/render.spec.ts
| expect(html).toContain('<h4>Example</h4>') | ||
| expect(html).toContain('shiki') | ||
| expect(html).toContain('greeting') | ||
| expect(html).not.toMatch(/(^|[>\s])-ts([<\s]|$)/) |
There was a problem hiding this comment.
You can probably keep both tests? Or that wouldn't work?
| expect(html).not.toMatch(/(^|[>\s])-ts([<\s]|$)/) | |
| expect(html).not.toMatch(/(^|[>\s])-ts([<\s]|$)/) | |
| expect(html).not.toContain('-ts') |
3 participants
Summary
@examplefence languages up to whitespace/backticks so hyphenated Shiki language ids are preservedglimmer-tsfenced exampleTesting
STORYBOOK=true pnpm vp test --project unit test/unit/server/utils/docs/render.spec.tspnpm vp lint server/utils/docs/render.ts test/unit/server/utils/docs/render.spec.tspnpm vp fmt --check server/utils/docs/render.ts test/unit/server/utils/docs/render.spec.tsFixes #2437