Repository navigation
feat: add cache option to reuse highlighted output across remounts - #208
AVGVSTVS96 wants to merge 3 commits into
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UaWsciHPYLqiWN1aQq5WL
🦋 Changeset detectedLatest commit: c4577ae The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UaWsciHPYLqiWN1aQq5WL
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019UaWsciHPYLqiWN1aQq5WL
| type HighlightCache = Pick< | ||
| Map<string, HighlightResultMap[OutputFormat]>, | ||
| 'get' | 'set' | ||
| >; |
There was a problem hiding this comment.
Pick<Map<…>, 'get' | 'set'> carries over Map's set return type, so set must return Map<string, …>. That rejects the "any { get, set } works" case from the PR description: a hand-rolled LRU whose set returns this or void fails assignability (and this is exactly what the motivating Hermes use case wrote). The runtime never uses the return value, so an explicit interface matches intent and is easier to read in hover tooltips:
| type HighlightCache = Pick< | |
| Map<string, HighlightResultMap[OutputFormat]>, | |
| 'get' | 'set' | |
| >; | |
| interface HighlightCache { | |
| get(key: string): HighlightResultMap[OutputFormat] | undefined; | |
| set(key: string, value: HighlightResultMap[OutputFormat]): unknown; | |
| } |
A plain Map still satisfies this (method parameters are bivariant, so Map<string, ReactElement> passes too).
| const cacheScope = useMemo( | ||
| () => | ||
| hasCache | ||
| ? JSON.stringify([ | ||
| stableOpts.outputFormat ?? 'react', | ||
| resolved.shikiOptions, | ||
| stableOpts.startingLineNumber, | ||
| stableOpts.highlightLineNumbers, | ||
| ]) | ||
| : undefined, | ||
| [hasCache, resolved, stableOpts] | ||
| ); | ||
| const cacheKey = cacheScope && `${cacheScope}\n${code}`; |
There was a problem hiding this comment.
Minor, memory: resolved.shikiOptions.theme/themes holds the raw theme input, so when a user passes a custom TextMate theme object the whole theme JSON is serialized into cacheScope, and every key is ${cacheScope}\n${code}. V8 flattens the cons-string when the Map hashes it, so each cached entry carries its own full copy of the theme JSON in its key. A 30–50 KB theme times a few hundred blocks adds megabytes of key strings on top of the output itself.
Cheap fix: a JSON.stringify replacer that swaps theme objects for their name (resolveTheme already requires name or type to recognize one), falling back to the full object only when name is absent. Not blocking; bundled theme names keep the scope at a few hundred bytes.
| }); | ||
| }); | ||
|
|
||
| describe('Cache', () => { |
There was a problem hiding this comment.
The four cases cover the core contract well. Three gaps worth closing, all cheap in this harness:
reactoutput format. Every test useshtml; the default path stores and re-renders a React element tree across mounts, which is the realistic case and the one most likely to surprise if it ever regresses.- Unmount while a newer highlight is in flight. The
shownref deliberately stores the output under the key that produced it, not the current key. A test that rerenders to new code, unmounts before the mock resolves, and asserts the cache holds only the old key would pin that design. - Component pass-through. One assertion that
<ShikiHighlighter cache={cache}>reaches the hook via the...optionsspread, so a future prop refactor can't silently drop it.
|
Review: Verdict: approve with one recommended type change. The design is sound and the implementation is small and well reasoned. Storing on unmount under the key that produced the shown output (rather than the current key) is the right call, and pulling Checklist
Findings (inline comments)
Verification 🤖 Reviewed by Fable 5 |
|
@claude can u fix all review comments |
|
Claude finished @AVGVSTVS96's task in 1m 56s —— View job Review comments: fixed locally, but I couldn't push
The fixes are in local commit Changes1. interface HighlightCache {
get(key: string): HighlightResultMap[OutputFormat] | undefined;
set(key: string, value: HighlightResultMap[OutputFormat]): unknown;
}2. import { isTextmateTheme, resolveTheme } from './theme';
// Custom theme objects can be tens of KB; key them by name so every cache
// key doesn't carry a copy of the theme JSON.
const themeByName = (_key: string, value: unknown) =>
isTextmateTheme(value) && value.name ? `theme:${value.name}` : value;
...
? JSON.stringify(
[
stableOpts.outputFormat ?? 'react',
resolved.shikiOptions,
stableOpts.startingLineNumber,
stableOpts.highlightLineNumbers,
],
themeByName
)A theme object without a 3. 4. Tests
Since I couldn't run anything, please check these with |
Remounting a code block highlights it from scratch today. In chat apps that remount transcripts on thread switch, every block re-tokenizes on the main thread (Hermes Agent hit this in NousResearch/hermes-agent#95595 and wrote its own LRU around shiki to fix it).
What it does
Decisions
{ get, set }works, so a plainMapdoes. The app clears it on real events (thread closed) instead of react-shiki picking a size cap.name.cacheis pulled out before options are deep-compared, so a growing Map never re-triggers highlighting.Tests
4 new hook tests: remount paints synchronously with no extra highlight call, streaming partials aren't stored, different theme misses, switching back to cached code while mounted reuses it. 129/129 pass,
tsc+ biome clean.🤖 Generated with Claude Code
https://claude.ai/code/session_019UaWsciHPYLqiWN1aQq5WL