refactor: parallelize docs rendering and Shiki setup#2382
refactor: parallelize docs rendering and Shiki setup#2382trivikr wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
- Render doc sections, symbols, markdown blocks, and examples concurrently - Cache in-flight Shiki highlighter initialization to avoid duplicate work - Add ordering tests for parallel docs rendering
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR parallelises documentation rendering: 🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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 (2)
server/utils/docs/text.ts (1)
138-141: Consider usingforEachfor type-safe iteration.The non-null assertion
highlightedCodeBlocks[i]!bypasses TypeScript's type checking. While the loop bounds guarantee valid indices, aforEachcallback would be strictly type-safe without the assertion.♻️ Suggested refactor
- for (let i = 0; i < highlightedCodeBlocks.length; i++) { - const highlighted = highlightedCodeBlocks[i]! - result = result.replace(`__CODE_BLOCK_${i}__`, highlighted) - } + highlightedCodeBlocks.forEach((highlighted, i) => { + result = result.replace(`__CODE_BLOCK_${i}__`, highlighted) + })As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
server/utils/docs/render.ts (1)
138-161: Destructured array elements may benulldespite conditional guards.The variables
highlightedSignature,renderedDescription, andrenderedJsDocTagsare typed asstring | null. The conditionals at lines 144, 154, and 159 mirror the conditions used to create the promises, but TypeScript cannot correlate these checks.The runtime behaviour is correct, but for stricter type safety, consider using truthiness checks directly on the resolved values (which you already do at lines 154 and 159) or adding explicit null checks.
♻️ Minor type-safe adjustment for line 145
if (signatures.length > 0) { - lines.push(`<div class="docs-signature">${highlightedSignature}</div>`) + lines.push(`<div class="docs-signature">${highlightedSignature ?? ''}</div>`)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b64ac82b-b369-41a2-b097-ee16c3a7f693
📒 Files selected for processing (4)
server/utils/docs/render.tsserver/utils/docs/text.tsserver/utils/shiki.tstest/unit/server/utils/docs/render.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bd79fef8-b292-4e38-9c0d-e632936bf7fc
📒 Files selected for processing (2)
server/utils/docs/render.tsserver/utils/docs/text.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/utils/docs/render.ts
| highlightedCodeBlocks.forEach((highlighted, i) => { | ||
| result = result.replace(`__CODE_BLOCK_${i}__`, highlighted) | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript String.prototype.replace, when the search value is a plain string, are replacement patterns like $&, $, and $' interpreted in the replacement string?`
💡 Result:
Yes, in JavaScript String.prototype.replace, when the search value is a plain string, replacement patterns like
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/replace
- 2: https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/String/replace
- 3: https://dbushell.com/2024/02/01/javascript-string-replace/
- 4: https://stackoverflow.com/questions/66492062/is-string-prototype-replace-buggy
- 5: https://stackoverflow.com/questions/31187452/why-abc-replaceb-gives-aac
🏁 Script executed:
cat -n server/utils/docs/text.ts | head -160 | tail -40Repository: npmx-dev/npmx.dev
Length of output: 1718
🏁 Script executed:
# Find the complete function containing lines 138-140
sed -n '100,200p' server/utils/docs/text.tsRepository: npmx-dev/npmx.dev
Length of output: 1639
🏁 Script executed:
# Look for where highlightedCodeBlocks is populated to understand what it contains
rg -n "highlightedCodeBlocks" server/utils/docs/text.ts -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 390
Use a function replacer to prevent $ substitution artefacts in highlighted HTML.
At line 139, passing highlighted directly as the replacement string can cause output mangling when the highlighted HTML incidentally contains special patterns like $&, $, or $'. Although syntax highlighters rarely produce such patterns, using a callback replacer is the defensive approach to ensure the HTML is inserted verbatim without interpretation.
Suggested fix
highlightedCodeBlocks.forEach((highlighted, i) => {
- result = result.replace(`__CODE_BLOCK_${i}__`, highlighted)
+ result = result.replace(`__CODE_BLOCK_${i}__`, () => highlighted)
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| highlightedCodeBlocks.forEach((highlighted, i) => { | |
| result = result.replace(`__CODE_BLOCK_${i}__`, highlighted) | |
| } | |
| }) | |
| highlightedCodeBlocks.forEach((highlighted, i) => { | |
| result = result.replace(`__CODE_BLOCK_${i}__`, () => highlighted) | |
| }) |
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
🔗 Linked issue
N/A
🧭 Context
Docs generation was doing more sequential work than necessary in the API renderer. We were awaiting kind sections one at a time, then symbols one at a time within each section, and each symbol also awaited syntax highlighting and markdown rendering serially.
For packages with large API surfaces, that made total render time closer to the sum of all individual steps instead of allowing independent work to overlap.
📚 Description
This PR parallelizes the independent async work in the docs rendering pipeline while preserving stable output order.
Changes:
renderDocNodesThe rendered HTML order is unchanged:
KIND_DISPLAY_ORDER