feat(platform): add shiki skipLangs option for analog v3 - #2239
Conversation
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-app ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
I checked this against analogjs/analog#2029, the issue discussion, and the current implementation in this branch.
I did verify that the targeted tests and build pass locally:
So this looks close, but I don't think it fully closes analogjs/analog#2029 yet without the behavior/docs follow-up above. |
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 `@packages/platform/src/lib/content/shiki/shiki-highlighter.ts`:
- Around line 57-59: The skip-branch in the highlight callback returns raw code
into HTML when this.skipLangs includes(lang), which allows unescaped HTML to be
rendered; update the highlight callback in shiki-highlighter (the place using
this.skipLangs and returning `<pre
class="language-${lang}">...${code}...</pre>`) to HTML-escape the code before
interpolation (e.g., replace & < > " ' with entities or use an existing escape
utility) so skipped languages render as safe text; ensure the escaped string is
used only for the skip branch while keeping normal codeToHtml handling for other
languages.
🪄 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: 61aefa5a-b3af-4e79-a4e4-3821b6217c6a
📒 Files selected for processing (4)
packages/platform/src/lib/content/shiki/index.spec.tspackages/platform/src/lib/content/shiki/index.tspackages/platform/src/lib/content/shiki/options.tspackages/platform/src/lib/content/shiki/shiki-highlighter.ts
|
Follow-up pushed on Addressed the skipped-language HTML injection path in Also added a regression test in Verification re-run:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/platform/src/lib/content/shiki/shiki-highlighter.ts (1)
66-68: Pre-existing: Mermaid path uses unescaped code.The Mermaid rendering path at line 67 interpolates raw
codewithout escaping. This is existing behavior (not introduced by this PR) and intentionally preserved per the PR objectives. However, for completeness, Mermaid diagram syntax can include user-defined labels that might contain HTML entities.Consider addressing this in a follow-up if Mermaid content could be user-supplied:
if (this.hasLoadMermaid && lang === 'mermaid') { - return `<pre class="mermaid">${code}</pre>`; + return `<pre class="mermaid">${escapeHtml(code)}</pre>`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/platform/src/lib/content/shiki/shiki-highlighter.ts` around lines 66 - 68, The Mermaid branch in the ShikiHighlighter unconditionally interpolates raw code (the check this.hasLoadMermaid and branch returning `<pre class="mermaid">${code}</pre>`), which can inject HTML entities; modify the branch to HTML-escape or sanitize the `code` value before interpolation (e.g., call an existing escapeHtml / sanitizeMarkdown function or add a small utility that replaces &, <, >, " and '), and use the escaped/sanitized string in the returned `<pre class="mermaid">...</pre>` so user-supplied Mermaid labels cannot introduce HTML markup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/platform/src/lib/content/shiki/shiki-highlighter.ts`:
- Around line 66-68: The Mermaid branch in the ShikiHighlighter unconditionally
interpolates raw code (the check this.hasLoadMermaid and branch returning `<pre
class="mermaid">${code}</pre>`), which can inject HTML entities; modify the
branch to HTML-escape or sanitize the `code` value before interpolation (e.g.,
call an existing escapeHtml / sanitizeMarkdown function or add a small utility
that replaces &, <, >, " and '), and use the escaped/sanitized string in the
returned `<pre class="mermaid">...</pre>` so user-supplied Mermaid labels cannot
introduce HTML markup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 915e1907-d172-407d-815c-b70224c1497f
📒 Files selected for processing (4)
apps/docs-app/docs/features/routing/content.mdpackages/platform/src/lib/content/shiki/index.spec.tspackages/platform/src/lib/content/shiki/index.tspackages/platform/src/lib/content/shiki/shiki-highlighter.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/platform/src/lib/content/shiki/index.spec.ts
- packages/platform/src/lib/content/shiki/index.ts
|
This needs to be validated against the reproduction that it actually fixes the OOM issue. |
|
Beta branch |
|
[On alpha] Validated against the reproduction at dmorfav/analyzing_oom (linked from analogjs/analog#2029 (comment)). Test harnessExercised
Results
Baseline loaded langs included What this proves
Full Vite build on the reproduction completes successfully with the fix applied. |
3e984df to
badafac
Compare
✅ Deploy Preview for analog-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
6114608 to
8e9de4f
Compare
8e9de4f to
5d9ad4d
Compare
b72e152 to
f68a87b
Compare
f68a87b to
d32b012
Compare
d32b012 to
1e6ea9c
Compare
|
@brandonroberts this got very "un-fun" for |
PR Checklist
Closes #2029
Affected scope
platformRecommended merge strategy for maintainer [optional]
Commit preservation note [optional]
What is the new behavior?
This adds
shikiOptions.highlighter.skipLangsto the platform content Shiki integration.When a fenced code block language is listed in
skipLangs, the block bypasses Shiki and is rendered as a plain fenced code block instead of going through Shiki's highlighter pipeline.That gives users a supported way to exclude languages like
mermaidfrom Shiki while still keeping Shiki enabled for the rest of the markdown pipeline. This follows the maintainer direction in the issue thread thatskipLangslooked like the right option.Test plan
nx format:checkpnpm buildpnpm testCommands run:
pnpm nx test platform --runTestsByPath packages/platform/src/lib/content/shiki/index.spec.tspnpm nx build platformDoes this PR introduce a breaking change?
Other information
The behavior is intentionally narrow:
mermaidis not listed inskipLangs