ReviewOS

stacks/stx

highlighter: the theme prop is computed, returned, and never applied — everything renders github-light

#2015
Closed glennmichael123 opened this 9 days ago ·
9 days ago

highlight() computes the requested theme, returns it in its result, and never uses it. Every CodeBlock in every app renders github-light, so the component is unusable on a dark code surface.

The defect

src/utils/highlighter.ts:

export async function getHighlighter(): Promise<TSHighlighter> {
  if (!highlighterInstance) {
    highlighterInstance = await createHighlighter({
      theme: 'github-light',          // <- singleton, hardcoded
    })
  }
  return highlighterInstance
}
const effectiveTheme = theme === 'auto'
  ? (globalThis.matchMedia?.('(prefers-color-scheme: dark)').matches ? 'github-dark' : 'github-light')
  : theme === 'dark' ? 'github-dark' : 'github-light'

result = await highlighter.highlight(code, language)   // <- effectiveTheme not passed

return { html: highlighted.html, css: highlighted.css, language, theme: effectiveTheme }

effectiveTheme reaches only the returned metadata. The CSS comes from the singleton, which is always github-light.

Observable

for (const theme of ['auto', 'dark']) {
  const r = await highlight('bun add x', { language: 'bash', theme, lineNumbers: false })
  // both: syntax bg=#ffffff, no dark variant, css length 2777 — byte identical
}

The emitted palette is GitHub Light: #24292f, #1a7f37, #cf222e, #0969da, #9a6700. Four of its seven colours have luminance below 0.4, so on a dark code surface the highlighted code is unreadable. .syntax also hardcodes background-color: #ffffff with no prefers-color-scheme block, so it paints a white panel into a dark page.

The returned theme field actively misleads: it reports github-dark while the CSS is github-light.

A second problem with auto

theme: 'auto' reads globalThis.matchMedia, which does not exist on the server. highlight() is called from <script server>, so auto resolves to github-light during SSR unconditionally. Even with the theme wired through, server rendering could never select dark, and a viewer's preference is not knowable at render time anyway.

For a server-rendered component the honest fix is to emit both palettes and let CSS choose, the way the rest of the library handles dark mode: ship the dark rules under @media (prefers-color-scheme: dark) (and a .dark selector for apps with a manual toggle) rather than branching in JS.

Suggested

  1. Pass the resolved theme into highlighting instead of discarding it. The singleton has to go, or be keyed by theme.
  2. For auto, emit both palettes with the dark set behind prefers-color-scheme and .dark, rather than probing matchMedia on the server.
  3. Until 1 is fixed, do not return theme: effectiveTheme, because it reports something untrue.

Context

Two apps tried to adopt <CodeBlock> today and both stopped here. Both have dark code surfaces and light/dark modes, so a hardcoded light palette is not something className can paper over: the token colours themselves are wrong, not just the panel background.

Related: stacksjs/stx#2014 (the code prop reaches a client script even when copyable is false).

9 days ago

Ask 1 and ask 3 are fixed in 202d5bc28d. Ask 2 is not, and I think it needs upstream rather than us.

The theme now reaches the highlighter

RenderOptions.theme overrides the instance per call, so one shared highlighter serves both palettes and the singleton does not have to go:

requested light -> css 2777  bg #ffffff
requested dark  -> css 2785  bg #0d1117   (was 2777 / #ffffff)

Dark now emits #0d1117, #161b22, #3fb950, #58a6ff, #c9d1d9 instead of the GitHub Light set.

And ask 3, which turned out to have a second instance

You asked me not to return theme: effectiveTheme while it was untrue. Fixed by making it true, but there was a second untruth on the failure path: the load guard returned a hardcoded github-light, so a caller asking for dark and getting the plain-text floor was also told it got light. The resolved theme now sits above that guard, so every exit reports what was asked for.

Ask 2: both palettes, and why not yet

You are right that this cannot be answered in JS. highlight() runs in <script server>, globalThis.matchMedia does not exist there, and a viewer's preference is not knowable at render time anyway. So auto still resolves light, and I have left it doing that rather than pretending.

What stops the honest fix is narrower than the design, though. This highlighter writes token colours as inline style attributes on each span, not through its token classes:

<span class="token storage-type-ts" style="color: #cf222e">const</span>

so no @media (prefers-color-scheme: dark) block and no .dark selector can override them. Emitting both palettes needs the renderer to stop inlining colours, or to emit both itself.

It can, nearly. ts-syntax-highlighter exports renderDualTheme(tokens, { lightTheme, darkTheme, selector }) and a DualThemeRenderer, which is exactly the shape this wants. Both take TokenLine[], and no public method on Highlighter returns that: highlightFast() yields a different token shape and renderDualTheme throws on it (undefined is not an object), loadTheme() throws outright, and Tokenizer is exported without a documented contract.

So ask 2 wants one of:

  1. a public tokenizer whose output renderDualTheme accepts, or
  2. a dual-theme entry point that takes code and a language the way highlight() does, or
  3. RenderOptions.theme accepting a pair, so one call emits both with the dark set behind prefers-color-scheme and a .dark selector

Any of those makes this a small change here. Happy to raise it on ts-syntax-highlighter if you want, since it is their API surface. Until then theme: 'dark' is the explicit escape and it works, which should unblock both apps with dark code surfaces.

Tests

ts-syntax-highlighter@0.2.17 imports bunfig without declaring it, so it throws under the test runner and highlight() takes its plain-text floor with no CSS at all. The palette assertions are skipIf-ed on that rather than passing vacuously against an empty string, and they run wherever it loads. The metadata contract, where the original lie lived, is checked in both environments and fails without the change.

Leaving this open for ask 2.

9 days ago

Item 2 done in b5a418e4aa. Item 1 landed in 202d5bc28d and item 3 is moot now that 1 is fixed, so this is complete.

auto emits both palettes

theme=auto   css=3636  inline colours=false  prefers-color-scheme=true  .dark=true  #0d1117 present
theme=light  css=2777  inline colours=true   (unchanged)
theme=dark   css=2785  inline colours=true   (unchanged)

Both switches, the way the rest of the library does dark mode: the media query for a viewer following their OS — guarded with :root:not(.light) so a page pinned to light does not get dark code — and .dark for an app with a manual toggle. The panel is in there too, not just the tokens: .syntax carrying background-color: #ffffff is the half a reader notices first, and dark tokens on a white panel is the same bug in reverse.

One correction to a note I inherited

The comment left in the source after item 1 said renderDualTheme "takes TokenLine[], which no public method returns". That is wrong — highlight() returns them in result.tokens, so the method is reachable.

It still does not solve this, for a better reason: its CSS styles the .syntax container for both modes and leaves every token colour inline, where no stylesheet rule can reach it. I checked for per-token-type rules to override instead and there are none, in single or dual mode — the engine emits token colour only as an inline style.

So the colours are read back out of two renders — same code, same tokens, so the spans align exactly — and re-emitted as rules on the .token classes the spans already carry, with the inline styles stripped so the rules apply. Reading them back rather than resolving the themes' TextMate tokenColors scopes keeps the engine's own resolution authoritative instead of reimplementing it.

The returned theme field

Now tracks what was emitted rather than what was asked for: 'auto' when both palettes shipped, a concrete name when only one could (the plain-text floor, when the highlighter cannot load). Reporting auto over a single palette would be the same class of untruth this issue opened on.

Verification

The assertions sit behind the existing skipIf(!highlighterLoaded) guard, because ts-syntax-highlighter@0.2.17 cannot load in this test runner — its dist reaches for a path inside its own source tree via import.meta.require, so bunfig resolution fails. They are skipped rather than passing over an empty string, and the exact command that does exercise them is recorded in the file:

bun -e "import { highlight } from './packages/components/src/utils/highlighter'
const r = await highlight('const a = 1 // n', { language: 'typescript', theme: 'auto' })
console.log(/style=\"color/.test(r.html), /prefers-color-scheme/.test(r.css), r.css.includes('#0d1117'))"
-> false true true

For bughq

<CodeBlock> defaults to theme="auto", so dropping it in should now give you readable code in both modes. One caveat from #2014 that still stands: the component exports highlightCss and does not render it, so you need to include that stylesheet yourself. A component <style> does interpolate now (#1990 closed), but @once does not dedupe across instances — five blocks would ship five copies of the ~3.6KB dual stylesheet. That dedup is the only thing between you and zero plumbing, and it is noted on #2014.

Suite green: 14,180 pass.

9 days ago

Both items are done — item 1 in 202d5bc28d, item 2 in b5a418e4aa (details in the comment above). Item 3 was "until 1 is fixed, do not return theme: effectiveTheme", which is moot now that it is, and the field reports what was emitted rather than what was asked for.

Closing. Reopen if <CodeBlock> still renders unreadably on a dark surface in bughq — the remaining plumbing is that the component exports highlightCss without rendering it, which is tracked on stacksjs/stx#2014 along with the two measurements that block it (a component <style> does interpolate now, but @once does not dedupe across instances, so five blocks would ship five copies of the ~3.6KB stylesheet).

Sign in to comment on this issue.