Skip to content

Use shared Shiki highlighting for chat markdown code fences - #120

Merged
juliusmarminge merged 9 commits into
mainfrom
codething/dfc16aa0
Feb 28, 2026
Merged

juliusmarminge merged 9 commits into
mainfrom
codething/dfc16aa0

Conversation

@juliusmarminge

@juliusmarminge juliusmarminge commented Feb 28, 2026 •

Copy link
Copy Markdown
Member

Summary

  • Replace rehype-highlight/highlight.js in chat markdown with shared Shiki highlighting via @pierre/diffs.
  • Add async code-fence rendering in ChatMarkdown with in-flight deduping plus LRU-based caching, and bypass cache while messages are streaming.
  • Wire chat rendering to streaming state (isStreaming) so partial responses render safely before final cached highlight.
  • Extract shared diff theme resolution into src/lib/diffThemes.ts and reuse it in diff panel/worker theme sync.
  • Add unit tests for the new LRUCache utility 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.
  • Project lint: Not run.
  • Full test suite: Not run.

Note

Medium Risk
Changes markdown code-block rendering to async dangerouslySetInnerHTML output 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 injected unsafeCSS, which may cause visual regressions.

Overview
Chat markdown code fences now use shared Shiki highlighting instead of rehype-highlight/highlight.js. ChatMarkdown switches 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/diffRendering and reused by the diff panel and worker pool; the diff panel also injects new unsafeCSS plus minor surface/background class changes to better match the app palette.

Cleanup and tests. Removes highlight.js/rehype-highlight dependencies and CSS import, and adds unit tests for the new LRUCache utility.

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/diffs and remove highlight.js/rehype-highlight from the web app

Replace 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 from ChatView. Update diff panel theming and remove legacy highlight assets.

📍Where to Start

Start with ChatMarkdown rendering and Shiki integration in ChatMarkdown.tsx.

📊 Macroscope summarized c832b7c. 6 files reviewed, 6 issues evaluated, 0 issues filtered, 2 comments posted

🗂️ Filtered Issues

- 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
@coderabbitai

coderabbitai Bot commented Feb 28, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch codething/dfc16aa0

Comment @coderabbitai help to get the list of available commands and usage tips.

Comment thread apps/web/src/components/ChatMarkdown.tsx
Comment thread apps/web/src/components/ChatMarkdown.tsx Outdated
Comment thread apps/web/src/components/ChatMarkdown.tsx Outdated
- 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
Comment thread apps/web/src/components/ChatMarkdown.tsx Outdated
- 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
@juliusmarminge
juliusmarminge merged commit 206eb6a into main Feb 28, 2026
4 checks passed
themeName,
isStreaming,
}: SuspenseShikiCodeBlockProps) {
const language = extractFenceLanguage(className);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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>>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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>>();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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.

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}
/>
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Fix in Cursor Fix in Web

TomCohenDev pushed a commit to TomCohenDev/t3code that referenced this pull request Oct 18, 2026
…pu-pressure

feat(machine-map): CPU alert means starved, not busy
HaukeSchnau added a commit to HaukeSchnau/t3code that referenced this pull request Oct 20, 2026
…refresh them in a second' (pingdotgg#120) from fix/usage-limits-refresh into main
HaukeSchnau added a commit to HaukeSchnau/t3code that referenced this pull request Oct 20, 2026
Merges 2,459 upstream commits (e9298af..069e63b, Oct 2 to Oct 19) into
fork main at pingdotgg#120. Upstream moved providers into packages/provider-*, dropped the
server's Services/Layers folders, went to stable Effect 4.0.5, switched mobile
Markdown to Enriched, split source control into packages, moved typecheck to
tsc-rs and pruned narration comments repo-wide. The merge had 254 conflicted
files, resolved hunk by hunk.

Decided with Hauke:
- Agent scopes: upstream AgentScope (pingdotgg#17662) replaces the fork's provider
  systemd scopes. agent-exec routing for separate projects runs inside the
  scope. Watch commands use it too. Needs the matching infra change.
- Find in thread: upstream's find (pingdotgg#10439) replaces the fork's on web and
  desktop. Mobile find is ported onto upstream's searchThread RPC
  (patches/mobile-thread-find.md). Saved thread.find bindings migrate to
  chat.find.
- MCP search: upstream's cross-project thread tools (pingdotgg#15219) replace the
  fork wrapper; t3_thread_search without projectId still searches every
  project.
- Web browser tests (pingdotgg#20905) run on Calder with nixpkgs Chromium.

Superseded by upstream and removed: Mermaid, the unknown-remote refinement
cache, the SSH ControlMaster part of the device tunnel fix, the runtime-state
ownership fix, reasoningEffort.cycle (now composer.increaseEffort on
mod+shift+.), the fork's SQLite client copy (upstream's shared client gets a
timing hook for the fork's SQL metrics), and the claude auth status fallback.

Re-homed onto upstream's structure: jj and skill-pack environment for the
packaged adapters (ProviderThreadEnvironment in provider-core), Z.AI limits
(provider-opencode), automatic title refresh (provider-core title
generation), metadata-only provider event logs (now upstream's single
events.log), watches and jj MCP tools (McpToolAccess), artifact previews,
inline replies, inline-code links (Enriched on mobile), accountless agent
awareness, the desktop t3code://open deep link, pingdotgg#120's usage-limit refresh
(provider-core usageLimits and managedProvider), and the fork's dev-runner,
setup and Nix packaging changes. The Nix runtime dependency fetch now takes the
server's whole workspace closure, since the deploy injects upstream's new
provider and source-control packages.

Migrations: upstream's 10 new migrations run as fork ids 82-91. Ids 44, 45
and 62 point at upstream's original filenames again; ids and names are
unchanged. assertV2MigrationsCanRun keys on ORCHESTRATION_V2_MIGRATION_ID.

Verification: all 32 workspace packages typecheck with tsc-rs; lint has no
errors and formatting is clean on the fork delta; contracts and shared suites
pass; focused suites per area pass (server orchestration 979 + replays 244,
providers 344 package + 680 server, platform ~900, web 998 unit + 456/460
browser, mobile 1,833). Remaining host-only failures match upstream's code on
srv-2 (German locale and git log.decorate, inherited production env vars,
load-sensitive timeouts).
HaukeSchnau added a commit to HaukeSchnau/t3code that referenced this pull request Oct 20, 2026
Merges 2,459 upstream commits (e9298af..069e63b, Oct 2 to Oct 19) into
fork main at pingdotgg#122. Upstream moved providers into packages/provider-*, dropped the
server's Services/Layers folders, went to stable Effect 4.0.5, switched mobile
Markdown to Enriched, split source control into packages, moved typecheck to
tsc-rs and pruned narration comments repo-wide. The merge had 254 conflicted
files, resolved hunk by hunk.

Decided with Hauke:
- Agent scopes: upstream AgentScope (pingdotgg#17662) replaces the fork's provider
  systemd scopes. agent-exec routing for separate projects runs inside the
  scope. Watch commands use it too. Needs the matching infra change.
- Find in thread: upstream's find (pingdotgg#10439) replaces the fork's on web and
  desktop. Mobile find is ported onto upstream's searchThread RPC
  (patches/mobile-thread-find.md). Saved thread.find bindings migrate to
  chat.find.
- MCP search: upstream's cross-project thread tools (pingdotgg#15219) replace the
  fork wrapper; t3_thread_search without projectId still searches every
  project.
- Web browser tests (pingdotgg#20905) run on Calder with nixpkgs Chromium.

Superseded by upstream and removed: Mermaid, the unknown-remote refinement
cache, the SSH ControlMaster part of the device tunnel fix, the runtime-state
ownership fix, reasoningEffort.cycle (now composer.increaseEffort on
mod+shift+.), the fork's SQLite client copy (upstream's shared client gets a
timing hook for the fork's SQL metrics), and the claude auth status fallback.

Re-homed onto upstream's structure: jj and skill-pack environment for the
packaged adapters (ProviderThreadEnvironment in provider-core), Z.AI limits
(provider-opencode), automatic title refresh (provider-core title
generation), metadata-only provider event logs (now upstream's single
events.log), watches and jj MCP tools (McpToolAccess), artifact previews,
inline replies, inline-code links (Enriched on mobile), accountless agent
awareness, the desktop t3code://open deep link, pingdotgg#120's usage-limit refresh
(provider-core usageLimits and managedProvider; pingdotgg#122's connection-based
schedule), and the fork's dev-runner,
setup and Nix packaging changes. The Nix runtime dependency fetch now takes the
server's whole workspace closure, since the deploy injects upstream's new
provider and source-control packages.

Migrations: upstream's 10 new migrations run as fork ids 82-91. Ids 44, 45
and 62 point at upstream's original filenames again; ids and names are
unchanged. assertV2MigrationsCanRun keys on ORCHESTRATION_V2_MIGRATION_ID.

Verification: all 32 workspace packages typecheck with tsc-rs; lint has no
errors and formatting is clean on the fork delta; contracts and shared suites
pass; focused suites per area pass (server orchestration 979 + replays 244,
providers 344 package + 680 server, platform ~900, web 998 unit + 456/460
browser, mobile 1,833). Remaining host-only failures match upstream's code on
srv-2 (German locale and git log.decorate, inherited production env vars,
load-sensitive timeouts).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant