Merge branch 'cw/review-tab-performance-phase-frontend-move-syntax-highlighting-off-main-thread' into cw-merge-1772822818316

apps/web/src/components/review/highlight-worker.ts

@@ -0,0 +1,39 @@
+import type { ThemedToken } from 'shiki';
+
+export interface HighlightRequest {
+  id: string;
+  filePath: string;
+  language: string; // resolved lang name (e.g. "typescript") or "text"
+  code: string; // full joined content of new-side lines to highlight
+  lineNumbers: number[]; // new-side line numbers to map tokens back to
+}
+
+export interface HighlightResponse {
+  id: string;
+  tokens: Array<{ lineNumber: number; tokens: ThemedToken[] }>;
+  error?: string;
+}
+
+self.addEventListener('message', async (event: MessageEvent<HighlightRequest>) => {
+  const { id, language, code, lineNumbers } = event.data;
+  try {
+    const { codeToTokens } = await import('shiki');
+    const result = await codeToTokens(code, {
+      lang: language as Parameters<typeof codeToTokens>[1]['lang'],
+      theme: 'github-dark-default',
+    });
+    const tokens: HighlightResponse['tokens'] = result.tokens.map((lineTokens, idx) => ({
+      lineNumber: lineNumbers[idx] ?? idx,
+      tokens: lineTokens,
+    }));
+    const response: HighlightResponse = { id, tokens };
+    self.postMessage(response);
+  } catch (err) {
+    const response: HighlightResponse = {
+      id,
+      tokens: [],
+      error: err instanceof Error ? err.message : String(err),
+    };
+    self.postMessage(response);
+  }
+});

apps/web/src/components/review/use-syntax-highlight.fallback.test.ts

@@ -0,0 +1,131 @@
+// @vitest-environment happy-dom
+// This file tests the chunked main-thread fallback path when Worker
+// construction is blocked (e.g. by CSP). It runs in isolation from the
+// worker-path tests so that module-level state (workersInitialized, workers)
+// starts clean.
+import '@testing-library/jest-dom/vitest'
+import { renderHook, waitFor } from '@testing-library/react'
+import { vi, describe, it, expect, beforeAll, afterAll } from 'vitest'
+
+const MOCK_TOKEN_A = { content: 'const', color: '#569cd6', offset: 0 }
+const MOCK_TOKEN_B = { content: 'x', color: '#9cdcfe', offset: 0 }
+
+// Mock shiki's createHighlighter for the fallback path
+const mockCodeToTokens = vi.fn()
+
+vi.mock('shiki', () => ({
+  createHighlighter: vi.fn().mockResolvedValue({
+    codeToTokens: mockCodeToTokens,
+  }),
+}))
+
+// Stub Worker to throw (simulating CSP) BEFORE the hook module is loaded.
+// initWorkers() catches the exception and leaves workers = [].
+beforeAll(() => {
+  // Use a class so Vitest doesn't warn about constructing vi.fn() without a class impl
+  class BlockedWorker {
+    constructor() {
+      throw new Error('CSP blocks workers')
+    }
+  }
+  vi.stubGlobal('Worker', BlockedWorker)
+
+  mockCodeToTokens.mockReturnValue({
+    tokens: [[MOCK_TOKEN_A], [MOCK_TOKEN_B]],
+  })
+})
+
+afterAll(() => {
+  vi.unstubAllGlobals()
+})
+
+// Dynamic import ensures this file's module instance is fresh (workersInitialized = false).
+// We import inside tests below rather than at the top level.
+
+describe('useHighlightedFile — fallback path (Worker unavailable)', () => {
+  it('falls back to chunked main-thread highlighting when Worker construction throws', async () => {
+    const { useHighlightedFile } = await import('./use-syntax-highlight')
+
+    const lines = [
+      { content: 'const x = 1', newLineNumber: 1, type: 'added' as const },
+      { content: 'let y = 2', newLineNumber: 2, type: 'context' as const },
+    ]
+
+    const { result } = renderHook(() => useHighlightedFile('app.ts', lines))
+
+    // Initially null while chunked highlighting runs
+    expect(result.current).toBeNull()
+
+    // Fallback createHighlighter path eventually resolves tokens
+    await waitFor(
+      () => {
+        expect(result.current).not.toBeNull()
+      },
+      { timeout: 5000 },
+    )
+
+    expect(result.current?.get(1)).toEqual([MOCK_TOKEN_A])
+    expect(result.current?.get(2)).toEqual([MOCK_TOKEN_B])
+  })
+
+  it('returns a complete token map with no lines missing for ≤200-line input (single-chunk equivalence)', async () => {
+    const { useHighlightedFile } = await import('./use-syntax-highlight')
+
+    // 5 lines — well within the 200-line chunk size, so a single codeToTokens call handles all
+    const MOCK_TOKENS = [
+      [{ content: 'line1', color: '#fff', offset: 0 }],
+      [{ content: 'line2', color: '#fff', offset: 0 }],
+      [{ content: 'line3', color: '#fff', offset: 0 }],
+      [{ content: 'line4', color: '#fff', offset: 0 }],
+      [{ content: 'line5', color: '#fff', offset: 0 }],
+    ]
+    mockCodeToTokens.mockReturnValueOnce({ tokens: MOCK_TOKENS })
+
+    const lines = [1, 2, 3, 4, 5].map((n) => ({
+      content: `line${n}`,
+      newLineNumber: n,
+      type: 'context' as const,
+    }))
+
+    const { result } = renderHook(() => useHighlightedFile('src/bar.ts', lines))
+
+    await waitFor(
+      () => {
+        expect(result.current).not.toBeNull()
+      },
+      { timeout: 5000 },
+    )
+
+    // All 5 line numbers must be present — no lines missing
+    expect(result.current!.size).toBe(5)
+    for (let n = 1; n <= 5; n++) {
+      expect(result.current!.get(n)).toEqual(MOCK_TOKENS[n - 1])
+    }
+  })
+
+  it('calls AbortController.abort() when component unmounts during chunked fallback', async () => {
+    const { useHighlightedFile } = await import('./use-syntax-highlight')
+
+    const abortSpy = vi.spyOn(AbortController.prototype, 'abort')
+
+    // Delay the mock so the hook is still in-flight when we unmount
+    mockCodeToTokens.mockImplementationOnce(
+      () =>
+        new Promise((resolve) =>
+          setTimeout(() => resolve({ tokens: [[MOCK_TOKEN_A]] }), 500),
+        ),
+    )
+
+    const lines = [{ content: 'const x = 1', newLineNumber: 1, type: 'added' as const }]
+
+    const { unmount } = renderHook(() => useHighlightedFile('unmount.ts', lines))
+
+    // Unmount while the async chunked highlight is still pending
+    unmount()
+
+    // The cleanup function calls abortController.abort()
+    expect(abortSpy).toHaveBeenCalled()
+
+    abortSpy.mockRestore()
+  })
+})

apps/web/src/components/review/use-syntax-highlight.test.ts

@@ -0,0 +1,240 @@
+// @vitest-environment happy-dom
+import '@testing-library/jest-dom/vitest'
+import { renderHook, waitFor, act } from '@testing-library/react'
+import { vi, describe, it, expect, beforeAll, beforeEach, afterAll } from 'vitest'
+
+// ── Worker mock infrastructure ─────────────────────────────────────────────
+//
+// We stub Worker BEFORE importing use-syntax-highlight so that initWorkers()
+// (called from useEffect on first render) picks up our mock.
+// Module-level state (workers, pending, workersInitialized) is shared across
+// all tests in this file — we control behaviour through the mock instances.
+
+type WorkerHandler = (event: { data: unknown }) => void
+
+class MockWorker {
+  static instances: MockWorker[] = []
+
+  messageHandler: WorkerHandler | null = null
+  postMessage = vi.fn()
+
+  constructor() {
+    MockWorker.instances.push(this)
+  }
+
+  addEventListener(type: string, handler: WorkerHandler) {
+    if (type === 'message') this.messageHandler = handler
+  }
+
+  /** Simulate a message arriving from the worker thread */
+  simulateResponse(data: unknown) {
+    this.messageHandler?.({ data })
+  }
+}
+
+// Stub Worker before the hook module is loaded.
+// initWorkers() is lazy (called inside useEffect), so the stub is in place
+// by the time any test renders a hook.
+beforeAll(() => {
+  vi.stubGlobal('Worker', MockWorker)
+})
+
+afterAll(() => {
+  vi.unstubAllGlobals()
+})
+
+beforeEach(() => {
+  // Reset call history between tests; keep instances (pool is created once)
+  MockWorker.instances.forEach((w) => w.postMessage.mockClear())
+})
+
+// Import the hook AFTER the beforeAll stub is registered (hoisted evaluation
+// of the module will not call initWorkers() — that happens in useEffect).
+import { useHighlightedFile } from './use-syntax-highlight'
+
+// ── Helpers ────────────────────────────────────────────────────────────────
+
+const MOCK_TOKEN_A = { content: 'const', color: '#569cd6', offset: 0 }
+const MOCK_TOKEN_B = { content: 'x', color: '#9cdcfe', offset: 0 }
+
+function makeLine(
+  content: string,
+  newLineNumber: number,
+  type: 'added' | 'context' | 'removed' = 'added',
+) {
+  return { content, newLineNumber, type } as const
+}
+
+// ── Tests ──────────────────────────────────────────────────────────────────
+
+describe('useHighlightedFile — worker path', () => {
+  // ── Test 1: Correct message format ───────────────────────────────────────
+
+  it('posts a message to a worker with filePath, language, code, and lineNumbers', async () => {
+    const lines = [
+      makeLine('const x = 1', 1, 'added'),
+      makeLine('const y = 2', 2, 'context'),
+    ]
+
+    renderHook(() => useHighlightedFile('src/index.ts', lines))
+
+    // Wait for initWorkers() to fire and postMessage to be called
+    await waitFor(() => {
+      const totalCalls = MockWorker.instances.reduce(
+        (n, w) => n + w.postMessage.mock.calls.length,
+        0,
+      )
+      expect(totalCalls).toBeGreaterThan(0)
+    })
+
+    // Find which worker received the message
+    const calledWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0)
+    expect(calledWorker).toBeDefined()
+    expect(calledWorker!.postMessage).toHaveBeenCalledWith(
+      expect.objectContaining({
+        filePath: 'src/index.ts',
+        language: 'typescript',
+        code: 'const x = 1\nconst y = 2',
+        lineNumbers: [1, 2],
+      }),
+    )
+  })
+
+  // ── Test 2: Response builds token map ─────────────────────────────────────
+
+  it('returns null initially and a LineTokenMap after worker responds', async () => {
+    const lines = [makeLine('const x = 1', 10, 'added')]
+
+    const { result } = renderHook(() => useHighlightedFile('component.ts', lines))
+
+    // Immediately null while worker is pending
+    expect(result.current).toBeNull()
+
+    // Capture the request id from whichever worker received it
+    let requestId = ''
+    let respondingWorker: MockWorker | undefined
+
+    await waitFor(() => {
+      respondingWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0)
+      expect(respondingWorker).toBeDefined()
+      requestId = respondingWorker!.postMessage.mock.calls[0][0].id as string
+      expect(requestId).not.toBe('')
+    })
+
+    // Simulate the worker responding
+    act(() => {
+      respondingWorker!.simulateResponse({
+        id: requestId,
+        tokens: [{ lineNumber: 10, tokens: [MOCK_TOKEN_A] }],
+      })
+    })
+
+    await waitFor(() => {
+      expect(result.current).not.toBeNull()
+      expect(result.current?.get(10)).toEqual([MOCK_TOKEN_A])
+    })
+  })
+
+  // ── Test 3: Worker error response → null ──────────────────────────────────
+
+  it('returns null when worker responds with an error field', async () => {
+    const lines = [makeLine('code here', 1, 'added')]
+
+    const { result } = renderHook(() => useHighlightedFile('bad.ts', lines))
+
+    let requestId = ''
+    let respondingWorker: MockWorker | undefined
+
+    await waitFor(() => {
+      respondingWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0)
+      expect(respondingWorker).toBeDefined()
+      requestId = respondingWorker!.postMessage.mock.calls[0][0].id as string
+    })
+
+    act(() => {
+      respondingWorker!.simulateResponse({
+        id: requestId,
+        tokens: [],
+        error: 'Worker crashed',
+      })
+    })
+
+    // Error → stays null (plain text fallback in the UI)
+    await new Promise<void>((r) => setTimeout(r, 20))
+    expect(result.current).toBeNull()
+  })
+
+  // ── Test 4: Unmount before response — no state update ────────────────────
+
+  it('silently discards a late worker response after unmount', async () => {
+    const lines = [makeLine('const z = 3', 5, 'added')]
+
+    const { result, unmount } = renderHook(() => useHighlightedFile('late.ts', lines))
+
+    let requestId = ''
+    let respondingWorker: MockWorker | undefined
+
+    await waitFor(() => {
+      respondingWorker = MockWorker.instances.find((w) => w.postMessage.mock.calls.length > 0)
+      expect(respondingWorker).toBeDefined()
+      requestId = respondingWorker!.postMessage.mock.calls[0][0].id as string
+    })
+
+    // Unmount before the response arrives
+    unmount()
+
+    // Simulate the late response — should be silently dropped
+    act(() => {
+      respondingWorker!.simulateResponse({
+        id: requestId,
+        tokens: [{ lineNumber: 5, tokens: [MOCK_TOKEN_B] }],
+      })
+    })
+
+    // result.current is frozen at last rendered value (null) — no update fired
+    expect(result.current).toBeNull()
+  })
+
+  // ── Test 5: Round-robin — two simultaneous requests go to different workers
+
+  it('distributes two simultaneous requests across both pool workers', async () => {
+    // Ensure the pool has been initialised (first test may have done this)
+    // and reset call counts for clean measurement.
+    MockWorker.instances.forEach((w) => w.postMessage.mockClear())
+
+    const lines1 = [makeLine('alpha', 1, 'added')]
+    const lines2 = [makeLine('beta', 1, 'added')]
+
+    // Render two hook instances at the same time
+    renderHook(() => useHighlightedFile('file1.ts', lines1))
+    renderHook(() => useHighlightedFile('file2.ts', lines2))
+
+    await waitFor(() => {
+      const total = MockWorker.instances.reduce((n, w) => n + w.postMessage.mock.calls.length, 0)
+      expect(total).toBe(2)
+    })
+
+    // Both pool workers should each have received exactly one request
+    // (round-robin: even requestCount → workers[0], odd → workers[1])
+    const counts = MockWorker.instances.map((w) => w.postMessage.mock.calls.length)
+    // Pool has 2 workers; each should have received 1 of the 2 requests
+    expect(counts[0]).toBe(1)
+    expect(counts[1]).toBe(1)
+  })
+
+  // ── Test 6: Unknown language → no request ────────────────────────────────
+
+  it('returns null immediately for files with no detectable language', async () => {
+    MockWorker.instances.forEach((w) => w.postMessage.mockClear())
+
+    const lines = [makeLine('raw data', 1, 'added')]
+
+    const { result } = renderHook(() => useHighlightedFile('data.xyz', lines))
+
+    await new Promise<void>((r) => setTimeout(r, 50))
+
+    expect(result.current).toBeNull()
+    const total = MockWorker.instances.reduce((n, w) => n + w.postMessage.mock.calls.length, 0)
+    expect(total).toBe(0)
+  })
+})

apps/web/src/components/review/use-syntax-highlight.ts

@@ -1,7 +1,59 @@
 import { useState, useEffect, useMemo } from "react";
 import type { ThemedToken } from "shiki";
+import type { HighlightRequest, HighlightResponse } from "./highlight-worker";
 
-/* ── Lazy singleton highlighter ─────────────────────────── */
+/* ── Worker pool (module-level, shared across all hook instances) ─────── */
+
+type PendingResolve = (response: HighlightResponse) => void;
+
+let workers: Worker[] = [];
+let requestCount = 0;
+const MAX_WORKERS = 2;
+const pending = new Map<string, PendingResolve>();
+
+let workersInitialized = false;
+
+function initWorkers(): void {
+  if (workersInitialized) return;
+  workersInitialized = true;
+  try {
+    workers = Array.from({ length: MAX_WORKERS }, () => {
+      const w = new Worker(
+        new URL("./highlight-worker.ts", import.meta.url),
+        { type: "module" },
+      );
+      w.addEventListener("message", (event: MessageEvent<HighlightResponse>) => {
+        const resolve = pending.get(event.data.id);
+        if (resolve) {
+          pending.delete(event.data.id);
+          resolve(event.data);
+        }
+      });
+      return w;
+    });
+  } catch {
+    // CSP or browser compat — fall back to chunked main-thread highlighting
+    workers = [];
+  }
+}
+
+function highlightWithWorker(
+  id: string,
+  language: string,
+  code: string,
+  lineNumbers: number[],
+  filePath: string,
+): Promise<HighlightResponse> {
+  return new Promise<HighlightResponse>((resolve) => {
+    pending.set(id, resolve);
+    const worker = workers[requestCount % MAX_WORKERS];
+    requestCount++;
+    const req: HighlightRequest = { id, filePath, language, code, lineNumbers };
+    worker.postMessage(req);
+  });
+}
+
+/* ── Lazy singleton highlighter (for main-thread fallback) ───────────── */
 
 let highlighterPromise: Promise<Awaited<
   ReturnType<typeof import("shiki")["createHighlighter"]>
@@ -40,10 +92,59 @@ function getHighlighter() {
   return highlighterPromise;
 }
 
-// Pre-warm on module load (non-blocking)
+/* ── Chunked main-thread fallback ────────────────────────────────────── */
-getHighlighter();
 
-/* ── Language detection ──────────────────────────────────── */
+async function highlightChunked(
+  code: string,
+  language: string,
+  lineNumbers: number[],
+  signal: AbortSignal,
+): Promise<LineTokenMap> {
+  const CHUNK = 200;
+  const result: LineTokenMap = new Map();
+  const lines = code.split("\n");
+  const highlighter = await getHighlighter();
+  if (!highlighter) return result;
+
+  for (let i = 0; i < lines.length; i += CHUNK) {
+    if (signal.aborted) break;
+    const chunkLines = lines.slice(i, i + CHUNK);
+    const chunkCode = chunkLines.join("\n");
+    try {
+      const tokenized = highlighter.codeToTokens(chunkCode, {
+        lang: language as Parameters<typeof highlighter.codeToTokens>[1]["lang"],
+        theme: "github-dark-default",
+      });
+      tokenized.tokens.forEach((lineTokens: ThemedToken[], idx: number) => {
+        const lineNum = lineNumbers[i + idx];
+        if (lineNum !== undefined) result.set(lineNum, lineTokens);
+      });
+    } catch {
+      // Skip unparseable chunk
+    }
+
+    // Yield between chunks to avoid blocking the main thread
+    await new Promise<void>((r) => {
+      if (
+        "scheduler" in globalThis &&
+        "yield" in (globalThis as Record<string, unknown>).scheduler
+      ) {
+        (
+          (globalThis as Record<string, unknown>).scheduler as {
+            yield: () => Promise<void>;
+          }
+        )
+          .yield()
+          .then(r);
+      } else {
+        setTimeout(r, 0);
+      }
+    });
+  }
+  return result;
+}
+
+/* ── Language detection ──────────────────────────────────────────────── */
 
 const EXT_TO_LANG: Record<string, string> = {
   ts: "typescript",
@@ -77,7 +178,7 @@ function detectLang(path: string): string | null {
   return EXT_TO_LANG[ext] ?? null;
 }
 
-/* ── Types ───────────────────────────────────────────────── */
+/* ── Types ───────────────────────────────────────────────────────────── */
 
 export type TokenizedLine = ThemedToken[];
 /** Maps newLineNumber → highlighted tokens for that line */
@@ -89,12 +190,23 @@ interface DiffLineInput {
   type: "added" | "removed" | "context";
 }
 
-/* ── Hook ────────────────────────────────────────────────── */
+/* ── Hook ────────────────────────────────────────────────────────────── */
 
 /**
- * Highlights the "new-side" content of a file diff.
+ * Highlights the "new-side" content of a file diff, returning a map of
- * Returns null until highlighting is ready (progressive enhancement).
+ * line number → syntax tokens.
- * Only context + added lines are highlighted (removed lines fall back to plain text).
+ *
+ * Progressive rendering: returns `null` while highlighting is in progress.
+ * Callers (HunkRows → LineWithComments) render plain text when `null` and
+ * patch in highlighted tokens on re-render once the worker or chunked call
+ * resolves.
+ *
+ * Worker path: uses a module-level pool of 2 Web Workers. Round-robin
+ * assignment. Late responses after unmount are silently discarded.
+ *
+ * Fallback path: if Worker construction fails (CSP, browser compat),
+ * falls back to chunked main-thread highlighting via codeToTokens (200
+ * lines/chunk) with scheduler.yield()/setTimeout(0) between chunks.
  */
 export function useHighlightedFile(
   filePath: string,
@@ -129,32 +241,37 @@ export function useHighlightedFile(
       return;
     }
 
-    let cancelled = false;
+    initWorkers(); // no-op after first call
 
-    getHighlighter().then((highlighter) => {
+    const id = crypto.randomUUID();
-      if (cancelled || !highlighter) return;
+    let unmounted = false;
+    const abortController = new AbortController();
 
-      try {
+    if (workers.length > 0) {
-        const result = highlighter.codeToTokens(code, {
+      highlightWithWorker(id, lang, code, lineNums, filePath).then((response) => {
-          lang: lang as Parameters<typeof highlighter.codeToTokens>[1]["lang"],
+        if (unmounted) return; // ignore late responses after unmount
-          theme: "github-dark-default",
+        if (response.error || response.tokens.length === 0) {
-        });
+          setTokenMap(null);
+          return;
+        }
         const map: LineTokenMap = new Map();
-
+        for (const { lineNumber, tokens } of response.tokens) {
-        result.tokens.forEach((lineTokens: ThemedToken[], idx: number) => {
+          map.set(lineNumber, tokens);
-          if (idx < lineNums.length) {
+        }
-            map.set(lineNums[idx], lineTokens);
+        setTokenMap(map);
-          }
+      });
-        });
+    } else {
-
+      highlightChunked(code, lang, lineNums, abortController.signal).then((map) => {
-        if (!cancelled) setTokenMap(map);
+        if (unmounted) return;
-      } catch {
+        setTokenMap(map.size > 0 ? map : null);
-        // Language not loaded or parse error — no highlighting
+      });
-      }
+    }
-    });
 
     return () => {
-      cancelled = true;
+      unmounted = true;
+      abortController.abort();
+      // Remove pending resolver so a late worker response is silently dropped
+      pending.delete(id);
     };
     // eslint-disable-next-line react-hooks/exhaustive-deps
   }, [cacheKey]);

apps/web/vite.config.ts

@@ -10,6 +10,11 @@ export default defineConfig({
       "@": path.resolve(__dirname, "./src"),
     },
   },
+  worker: {
+    // ES module workers are required when the app uses code-splitting (Rollup
+    // can't bundle IIFE workers alongside dynamic imports).
+    format: "es",
+  },
   server: {
     proxy: {
       "/trpc": {

docs/frontend.md

@@ -122,6 +122,17 @@ The initiative detail page has three tabs managed via local state (not URL param
 | `PreviewPanel` | Docker preview status: building/running/failed with start/stop (legacy, now integrated into ReviewHeader) |
 | `ProposalCard` | Individual proposal display |
 
+#### Syntax Highlighting (`use-syntax-highlight.ts` + `highlight-worker.ts`)
+
+`useHighlightedFile(filePath, allLines)` returns `LineTokenMap | null`. Tokenisation runs off the main thread:
+
+- **Worker path** (default): a module-level pool of 2 ES module Web Workers (`highlight-worker.ts`) each import shiki's `codeToTokens` dynamically. Requests are round-robined by `requestCount % 2`. Responses are correlated by UUID. Late responses after unmount are silently discarded via the `pending` Map.
+- **Fallback path** (CSP / browser-compat): if `Worker` construction throws, `createHighlighter` is used on the main thread but processes 200 lines per chunk, yielding between chunks via `scheduler.yield()` or `setTimeout(0)`.
+
+Callers receive `null` while highlighting is in progress and a populated `Map<lineNumber, ThemedToken[]>` once it resolves. `LineWithComments` already renders plain text when `null`, so no caller changes are needed.
+
+Vite must be configured with `worker.format: 'es'` (added to `vite.config.ts`) for the worker chunk to bundle correctly alongside code-split app chunks.
+
 ### UI Primitives (`src/components/ui/`)
 shadcn/ui components: badge (6 status variants + xs size), button, card, dialog, dropdown-menu, input, label, select, sonner, textarea, tooltip.