Repository navigation
Use shared Shiki highlighting for chat markdown code fences - #120
Conversation
- Replace `rehype-highlight`/`highlight.js` with `@pierre/diffs` highlighter in `ChatMarkdown` - Add LRU caching and in-flight dedupe for highlighted code, with streaming-aware behavior - Extract shared diff theme resolution utility and add unit tests for new LRU cache
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- add diff-specific wrapper classes in `DiffPanel` for targeted theming - override diff renderer color variables to match card/background palette - soften chat markdown Shiki code-block background for consistency
- Inject diff palette overrides via `unsafeCSS` in `DiffPanel` - Add `diff-panel-viewport` background and keep file container border/background styling in `index.css` - Switch panel root from `bg-card` to `bg-background` for consistent surface tone
- Reuse in-flight highlight requests and allow later non-streaming renders to cache results - Stabilize code block highlight state updates to avoid stale or redundant rerenders - Export and reuse `fnv1a32` from `diffRendering` for shared cache key hashing
- Wrap rendered markdown code blocks with a copy action button - Show copy/check icons with transient "Copied" state and cleanup - Style button to appear on hover/focus without shifting code layout
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| themeName, | ||
| isStreaming, | ||
| }: SuspenseShikiCodeBlockProps) { | ||
| const language = extractFenceLanguage(className); |
There was a problem hiding this comment.
🟠 High components/ChatMarkdown.tsx:168
Calling useMemo and useEffect after the conditional return on line 172 violates the Rules of Hooks. If a re-render hits the cache after an initial miss, React will throw "Rendered fewer hooks than during the previous render." Consider moving all hooks before the conditional return.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ChatMarkdown.tsx around line 168:
Calling `useMemo` and `useEffect` after the conditional return on line 172 violates the Rules of Hooks. If a re-render hits the cache after an initial miss, React will throw "Rendered fewer hooks than during the previous render." Consider moving all hooks before the conditional return.
Evidence trail:
apps/web/src/components/ChatMarkdown.tsx lines 160-199 at REVIEWED_COMMIT. Line 172 shows conditional return `if (cachedHighlightedHtml != null)`. Lines 180-183 show `useMemo` call after the conditional. Lines 185-193 show `useEffect` call after the conditional. React Rules of Hooks documentation: https://react.dev/reference/rules/rules-of-hooks
| MAX_HIGHLIGHT_CACHE_ENTRIES, | ||
| MAX_HIGHLIGHT_CACHE_MEMORY_BYTES, | ||
| ); | ||
| const highlighterPromiseCache = new Map<string, Promise<DiffsHighlighter>>(); |
There was a problem hiding this comment.
🟡 Medium components/ChatMarkdown.tsx:44
Consider handling rejected promises in getHighlighterPromise — if getSharedHighlighter fails, the rejected promise stays cached forever, breaking that language until page reload. Adding a .catch() to delete the entry on failure would fix this.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ChatMarkdown.tsx around line 44:
Consider handling rejected promises in `getHighlighterPromise` — if `getSharedHighlighter` fails, the rejected promise stays cached forever, breaking that language until page reload. Adding a `.catch()` to delete the entry on failure would fix this.
Evidence trail:
apps/web/src/components/ChatMarkdown.tsx lines 44, 94-105 at REVIEWED_COMMIT: `highlighterPromiseCache` is a Map that caches promises. Line 103 stores the promise immediately via `highlighterPromiseCache.set(language, promise)`. Lines 95-96 return the cached promise without checking rejection state. No `.catch()` handler exists to remove failed promises from the cache.
| MAX_HIGHLIGHT_CACHE_ENTRIES, | ||
| MAX_HIGHLIGHT_CACHE_MEMORY_BYTES, | ||
| ); | ||
| const highlighterPromiseCache = new Map<string, Promise<DiffsHighlighter>>(); |
There was a problem hiding this comment.
🟢 Low components/ChatMarkdown.tsx:44
highlighterPromiseCache can grow unbounded with arbitrary language keys (including partials while streaming). Consider validating language against SupportedLanguages before caching, or replacing the Map with an LRUCache.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file apps/web/src/components/ChatMarkdown.tsx around line 44:
`highlighterPromiseCache` can grow unbounded with arbitrary language keys (including partials while streaming). Consider validating `language` against `SupportedLanguages` before caching, or replacing the `Map` with an `LRUCache`.
Evidence trail:
apps/web/src/components/ChatMarkdown.tsx: Line 44 shows `highlighterPromiseCache = new Map<string, Promise<DiffsHighlighter>>()` (unbounded Map). Lines 38-41 show `highlightedCodeCache = new LRUCache<string>(MAX_HIGHLIGHT_CACHE_ENTRIES, MAX_HIGHLIGHT_CACHE_MEMORY_BYTES)` (bounded). Lines 100-111 show `getHighlighterPromise` adds entries without any cleanup/eviction. Line 108 uses `language as SupportedLanguages` - type assertion only, no runtime validation. apps/web/src/lib/lruCache.ts shows LRUCache implementation with `evictIfNeeded` - this is not used for highlighterPromiseCache.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is ON, but it could not run because the spend limit has been reached. To enable Bugbot Autofix, raise your spend limit in the Cursor dashboard.
| dangerouslySetInnerHTML={{ __html: cachedHighlightedHtml }} | ||
| /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
Conditional return before hooks violates Rules of Hooks
High Severity
SuspenseShikiCodeBlock has a conditional early return before useMemo and useEffect are called. While React 19's use() is allowed conditionally, useMemo and useEffect are not — they must be called in the same order on every render. When a code block transitions from cache-miss (all hooks called) to cache-hit (early return, no hooks called), React will throw "Rendered fewer hooks than expected" and crash the component tree.
Additional Locations (1)
1 participant
Summary
rehype-highlight/highlight.jsin chat markdown with shared Shiki highlighting via@pierre/diffs.ChatMarkdownwith in-flight deduping plus LRU-based caching, and bypass cache while messages are streaming.isStreaming) so partial responses render safely before final cached highlight.src/lib/diffThemes.tsand reuse it in diff panel/worker theme sync.LRUCacheutility and remove obsolete highlight CSS/dependencies.Testing
apps/web/src/lib/lruCache.test.ts: verifies cache miss behavior, max-entry eviction, LRU promotion semantics, and memory-budget eviction.Note
Medium Risk
Changes markdown code-block rendering to async
dangerouslySetInnerHTMLoutput with caching and streaming-aware behavior, which could affect rendering correctness/performance and introduces some XSS/regression risk if highlighting output isn’t properly escaped. Diff UI theming is also adjusted via injectedunsafeCSS, which may cause visual regressions.Overview
Chat markdown code fences now use shared Shiki highlighting instead of
rehype-highlight/highlight.js.ChatMarkdownswitches to a Suspense-based async highlighter from@pierre/diffs, adds a code-block copy button, and introduces an LRU cache (bypassed while messages are streaming) to reuse highlighted HTML.Diff theming/styling is consolidated and adjusted. Theme name resolution is centralized in
lib/diffRenderingand reused by the diff panel and worker pool; the diff panel also injects newunsafeCSSplus minor surface/background class changes to better match the app palette.Cleanup and tests. Removes
highlight.js/rehype-highlightdependencies and CSS import, and adds unit tests for the newLRUCacheutility.Written by Cursor Bugbot for commit c0f8c29. This will update automatically on new commits. Configure here.
Note
Switch chat markdown code fences to shared Shiki highlighting via
@pierre/diffsand removehighlight.js/rehype-highlightfrom the web appReplace rehype-based highlighting with Shiki in
ChatMarkdown, add theme-aware rendering and a copy button for code blocks, introduce an LRU cache for highlighted HTML, and pass a streaming flag fromChatView. Update diff panel theming and remove legacy highlight assets.📍Where to Start
Start with
ChatMarkdownrendering and Shiki integration in ChatMarkdown.tsx.📊 Macroscope summarized c832b7c. 6 files reviewed, 6 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues