feat(platform): add shiki skipLangs option for analog v2 - #2282
Conversation
Made-with: Cursor
✅ Deploy Preview for analog-blog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for analog-docs 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. |
📝 WalkthroughWalkthroughThis multi-package pull request introduces a Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Review focus areasBreaking changes & API compatibility:
High-complexity changes:
Test coverage:
Monorepo consistency:
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 |
github-actions
Bot
added
scope:content
|
This PR touches multiple package scopes: Please confirm the changes are closely related. |
benpsnyder
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/docs-app/docs/features/routing/content.md (1)
208-231: Mention thatloadMermaidis still required.This snippet only changes build-time highlighting. If readers copy it without the
withMarkdownRenderer({ loadMermaid: ... })setup from the Mermaid section below, they can end up with raw<pre class="mermaid">output at runtime. A short note or cross-reference here would make the setup self-contained. As per coding guidelines,apps/docs-app/**/*.{md,mdx}: Add concise documentation with descriptive sections to the appropriate guides in the docs-app.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/docs-app/docs/features/routing/content.md` around lines 208 - 231, Add a brief note after the config snippet clarifying that this only affects build-time highlighting and that the runtime still requires the loadMermaid setup (e.g., the withMarkdownRenderer({ loadMermaid: ... }) call) to render <pre class="mermaid"> blocks; reference the Mermaid section and mention the need to keep loadMermaid so readers don't end up with raw Mermaid blocks, and refer to the shikiOptions keys (skipLangs/additionalLangs) and loadMermaid/withMarkdownRenderer symbols to guide where to wire it up.packages/platform/src/lib/content/shiki/index.spec.ts (1)
53-95: Add oneskipLangscase for a default language.Current coverage only exercises skipped languages introduced through
additionalLangs. A smallskipLangs: ['ts']or['js']case would protect the default-lang initialization/filter path inpackages/platform/src/lib/content/shiki/index.tstoo. As per coding guidelines,**/*.spec.{ts,tsx}: Keep tests lightweight and targeted to critical functionality testing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/platform/src/lib/content/shiki/index.spec.ts` around lines 53 - 95, Add a new unit test using getShikiHighlighter that sets highlighter.skipLangs to include a default language like 'ts' (or 'js') and assert that the returned extension.highlight for a TS/JS code string returns a plain escaped fenced block (same shape as the existing YAML tests); target the same highlighter.getHighlightExtension() and its highlight(code, lang, props) promise to ensure the default-lang initialization/filter path in packages/platform/src/lib/content/shiki/index.ts is exercised without adding heavy fixtures.packages/platform/src/lib/content/shiki/index.ts (1)
21-53: Normalize into a local copy instead of mutatinghighlighter.
delete highlighter.additionalLangsanddelete highlighter.skipLangsstrip fields from the caller's config object after the first call. Cloning before normalization keeps this helper side-effect free and avoids surprising reuse bugs.♻️ Suggested normalization pattern
- const additionalLangs = highlighter.additionalLangs ?? []; - const skipLangs = highlighter.skipLangs ?? []; + const { + additionalLangs = [], + skipLangs = [], + ...normalizedHighlighter + } = highlighter; const hasMermaidSupport = - highlighter.langs?.includes('mermaid') || + normalizedHighlighter.langs?.includes('mermaid') || additionalLangs.includes('mermaid'); - if (!highlighter.themes) { + if (!normalizedHighlighter.themes) { if (highlight.theme) { - highlighter.themes = [highlight.theme]; + normalizedHighlighter.themes = [highlight.theme]; } else if (highlight.themes && typeof highlight.themes === 'object') { - highlighter.themes = Object.values(highlight.themes) as string[]; + normalizedHighlighter.themes = Object.values(highlight.themes) as string[]; } else { - highlighter.themes = defaultHighlighterOptions.themes; + normalizedHighlighter.themes = defaultHighlighterOptions.themes; } } - if (!highlighter.langs) { - highlighter.langs = [...defaultHighlighterOptions.langs]; + if (!normalizedHighlighter.langs) { + normalizedHighlighter.langs = [...defaultHighlighterOptions.langs]; } if (additionalLangs.length > 0) { - highlighter.langs.push(...additionalLangs); + normalizedHighlighter.langs.push(...additionalLangs); } if (skipLangs.length > 0) { const skipSet = new Set<string>(skipLangs); - highlighter.langs = highlighter.langs.filter( + normalizedHighlighter.langs = normalizedHighlighter.langs.filter( (lang: unknown) => typeof lang !== 'string' || !skipSet.has(lang), ); } highlighterInstance = new ShikiHighlighter( - highlighter as ShikiHighlighterOptions, + normalizedHighlighter as ShikiHighlighterOptions, highlight, container, hasMermaidSupport, skipLangs, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/platform/src/lib/content/shiki/index.ts` around lines 21 - 53, Make a shallow clone of the incoming highlighter config at the top (e.g., const normalizedHighlighter = { ...highlighter }) and perform all normalization steps against normalizedHighlighter (compute additionalLangs/skipLangs from normalizedHighlighter, set normalizedHighlighter.themes/langs, push additionalLangs, filter skipLangs, compute hasMermaidSupport using normalizedHighlighter) instead of mutating the original highlighter; remove the final delete highlighter.additionalLangs / delete highlighter.skipLangs lines so the caller object isn’t modified, and ensure downstream code uses normalizedHighlighter (or return it) where the normalized config is expected.
🤖 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/router/src/lib/i18n/provide-i18n.ts`:
- Around line 117-118: Write a test that verifies ENVIRONMENT_INITIALIZER's
re-read of LOCALE respects a server-provided token: set up a test application
that calls provideServerContext({ locale: '<server-locale>' }) before
bootstrapping, ensure the ENVIRONMENT_INITIALIZER runs, and assert that
injectLocale() within the initializer returns the server locale and that
initI18n(resolved, locale) is invoked with that value; focus the test on the
path exercised by provide-i18n's initializer (the function that returns () =>
initI18n(resolved, locale ?? undefined)) so it proves server-context LOCALE
precedence over client-detected locale.
---
Nitpick comments:
In `@apps/docs-app/docs/features/routing/content.md`:
- Around line 208-231: Add a brief note after the config snippet clarifying that
this only affects build-time highlighting and that the runtime still requires
the loadMermaid setup (e.g., the withMarkdownRenderer({ loadMermaid: ... })
call) to render <pre class="mermaid"> blocks; reference the Mermaid section and
mention the need to keep loadMermaid so readers don't end up with raw Mermaid
blocks, and refer to the shikiOptions keys (skipLangs/additionalLangs) and
loadMermaid/withMarkdownRenderer symbols to guide where to wire it up.
In `@packages/platform/src/lib/content/shiki/index.spec.ts`:
- Around line 53-95: Add a new unit test using getShikiHighlighter that sets
highlighter.skipLangs to include a default language like 'ts' (or 'js') and
assert that the returned extension.highlight for a TS/JS code string returns a
plain escaped fenced block (same shape as the existing YAML tests); target the
same highlighter.getHighlightExtension() and its highlight(code, lang, props)
promise to ensure the default-lang initialization/filter path in
packages/platform/src/lib/content/shiki/index.ts is exercised without adding
heavy fixtures.
In `@packages/platform/src/lib/content/shiki/index.ts`:
- Around line 21-53: Make a shallow clone of the incoming highlighter config at
the top (e.g., const normalizedHighlighter = { ...highlighter }) and perform all
normalization steps against normalizedHighlighter (compute
additionalLangs/skipLangs from normalizedHighlighter, set
normalizedHighlighter.themes/langs, push additionalLangs, filter skipLangs,
compute hasMermaidSupport using normalizedHighlighter) instead of mutating the
original highlighter; remove the final delete highlighter.additionalLangs /
delete highlighter.skipLangs lines so the caller object isn’t modified, and
ensure downstream code uses normalizedHighlighter (or return it) where the
normalized config is expected.
🪄 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: a7e0ef1e-5234-44d9-bd84-7da486424b62
📒 Files selected for processing (11)
apps/docs-app/docs/features/routing/content.mdpackages/content/prism-highlighter/src/lib/prism-highlighter.tspackages/content/resources/src/content-file-resource.tspackages/content/resources/src/content-files-resource.tspackages/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.tspackages/router/project.jsonpackages/router/src/lib/i18n/provide-i18n.tspackages/router/tsconfig.lib.json
| const locale = injectLocale(); | ||
| return () => initI18n(resolved, locale ?? undefined); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether router i18n tests cover server-overridden locale precedence.
# Expected: at least one spec asserting init path uses server-provided LOCALE/injectLocale outcome.
rg -nP --type=ts -C3 '\b(provideI18n|initI18n|injectLocale|LOCALE|provideServerContext)\b'
fd -i 'i18n'Repository: analogjs/analog
Length of output: 19909
🏁 Script executed:
cat packages/router/src/lib/i18n/provide-i18n.spec.tsRepository: analogjs/analog
Length of output: 8147
🏁 Script executed:
fd -i 'provide-server-context\|\.spec\|\.test' packages/router/server/src
cat packages/router/server/src/provide-server-context.ts
rg -A 10 'provideI18n.*provideServerContext|provideServerContext.*provideI18n' --type tsRepository: analogjs/analog
Length of output: 3181
🏁 Script executed:
rg -l 'provideServerContext.*provideI18n|ENVIRONMENT_INITIALIZER' --type ts
find apps -name '*.spec.ts' -o -name '*.e2e.ts' | xargs grep -l 'provideServerContext\|server.*i18n' 2>/dev/null | head -5Repository: analogjs/analog
Length of output: 192
🏁 Script executed:
git log --oneline -n 20 packages/router/src/lib/i18n/provide-i18n.ts
git show HEAD:packages/router/src/lib/i18n/provide-i18n.ts 2>/dev/null | sed -n '110,125p' || echo "HEAD revision unavailable, checking current state only"Repository: analogjs/analog
Length of output: 588
🏁 Script executed:
# Check previous commits to see if this ENVIRONMENT_INITIALIZER pattern is new
git log --all --oneline packages/router/src/lib/i18n/provide-i18n.ts | head -10
git show f68a87bb:packages/router/src/lib/i18n/provide-i18n.ts 2>/dev/null | sed -n '110,125p' || echo "Previous version not available"
git diff f68a87bb~1 f68a87bb packages/router/src/lib/i18n/provide-i18n.ts 2>/dev/null | head -60 || echo "No diff available"Repository: analogjs/analog
Length of output: 647
🏁 Script executed:
# Check the actual diff of the feature PR to see what tests were added
git show 7dbc7dfa packages/router/src/lib/i18n/provide-i18n.spec.ts 2>/dev/null | head -100 || echo "Cannot retrieve feature PR diff"
# Also check if there are any integration tests in the analog-app-e2e or similar
find apps -name '*e2e*' -type d | head -5Repository: analogjs/analog
Length of output: 2915
Add test for server-locale precedence in ENVIRONMENT_INITIALIZER.
Lines 117–118 introduce logic that re-reads the LOCALE token during app initialization to allow server context to override client-detected locale. The code comment explicitly documents this behavior ("Re-read LOCALE in case the server context overrode it"), but the test suite does not verify that injectLocale() correctly picks up a server-provided LOCALE token during the initializer phase. Add a unit or integration test confirming this path works when provideServerContext({ locale }) sets a LOCALE token before the ENVIRONMENT_INITIALIZER runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/router/src/lib/i18n/provide-i18n.ts` around lines 117 - 118, Write a
test that verifies ENVIRONMENT_INITIALIZER's re-read of LOCALE respects a
server-provided token: set up a test application that calls
provideServerContext({ locale: '<server-locale>' }) before bootstrapping, ensure
the ENVIRONMENT_INITIALIZER runs, and assert that injectLocale() within the
initializer returns the server locale and that initI18n(resolved, locale) is
invoked with that value; focus the test on the path exercised by provide-i18n's
initializer (the function that returns () => initI18n(resolved, locale ??
undefined)) so it proves server-context LOCALE precedence over client-detected
locale.
2 participants
No description provided.