Persist the preferred diff renderer across reviews

public/app.js

@@ -1741,6 +1741,18 @@ function persistDiffPreferences() {
     .catch((error) => toast(error.message));
 }
 
+function persistDiffRenderer(diffRenderer) {
+  state.diffPreferenceSave = (state.diffPreferenceSave ?? Promise.resolve())
+    .catch(() => undefined)
+    .then(async () => {
+      const session = await api("/api/preferences", {
+        method: "POST", body: JSON.stringify({ diffRenderer }),
+      });
+      if (state.session) state.session.diffRenderer = session.diffRenderer;
+    })
+    .catch((error) => toast(`Could not save renderer preference: ${error.message}`));
+}
+
 function diffViewRow(path, side, line) {
   return elements.diff.querySelector(`.diff-row[data-path="${CSS.escape(path)}"][data-side="${side}"][data-line="${line}"]`)
     ?? elements.diff.querySelector(`.diff-row[data-path="${CSS.escape(path)}"][data-${side}-line="${line}"]`);
@@ -5600,10 +5612,10 @@ function isInPlaceSessionUpdate(previous, latest) {
   // Explanations and comments can be updated without replacing the diff.
   // Only skip the full render when everything else is unchanged, including
   // the route, selected stop, diff, and review lifecycle.
-  // Layout/wrap are applied immediately by changeDiffView. A later polling
+  // Renderer/layout/wrap are applied immediately by their controls. A later polling
   // acknowledgement of their saved preferences must not rebuild the diff,
   // replace comment DOM or steal the reader's position.
-  const structuralSnapshot = ({ revision, agentActivity, activities, comments, messages, outline, pullRequest, diffPreferences, ...session }) => ({
+  const structuralSnapshot = ({ revision, agentActivity, activities, comments, messages, outline, pullRequest, diffPreferences, diffRenderer, ...session }) => ({
     ...session,
     pullRequest: pullRequest ? (({ threads, threadsTruncated, updatedAt, ...metadata }) => metadata)(pullRequest) : pullRequest,
     outline: outline ? {
@@ -5626,7 +5638,7 @@ function isSamePhaseNavigationUpdate(previous, latest, { acrossPhases = false }
   const previousPhase = previous.outline.phases.find((phase) => phase.steps.some((step) => step.id === previousStep?.id));
   const latestPhase = latest.outline.phases.find((phase) => phase.steps.some((step) => step.id === latestStep?.id));
   if (!previousPhase || !latestPhase || (!acrossPhases && previousPhase.id !== latestPhase.id)) return false;
-  const navigationSnapshot = ({ revision, agentActivity, currentStepIndex, outline, diffPreferences, ...session }) => ({
+  const navigationSnapshot = ({ revision, agentActivity, currentStepIndex, outline, diffPreferences, diffRenderer, ...session }) => ({
     ...session,
     outline: outline ? {
       ...outline,
@@ -5695,6 +5707,7 @@ function renderSessionUpdate(previous, options) {
 }
 
 function renderSession() {
+  state.monacoRenderer?.restorePreference(state.session.diffRenderer);
   getBriefingReader().render(state.session);
   const { comparison, comments } = state.session;
   syncDryRunSourceCache();
@@ -8388,6 +8401,7 @@ state.monacoRenderer = createMonacoRenderer({
   actions: {
     notify: toast, tourShortcut: handleTourShortcut, viewportChanged: scheduleCurrentStepReturnUpdate,
     reserveScroll: reserveDiffScrollPosition,
+    saveRendererPreference: persistDiffRenderer,
     review: setReviewed,
     fileSummaryControl: file => globalThis.DiffCompassSemantic?.control(file, false) ?? "",
     async suggestionCreated(result, path) {

public/monaco-controller.mjs

@@ -6,12 +6,15 @@ import { panReviewScroll } from "./monaco/pen-scroll.mjs";
 
 export function createMonacoRenderer({ container, controls, token, snapshot, render, actions }) {
   const preferenceKey = "diffcompass-diff-renderer";
-  let preferred = "current";
-  try { if (localStorage.getItem(preferenceKey) === "monaco") preferred = "monaco"; } catch {}
+  let browserPreference;
+  try { browserPreference = localStorage.getItem(preferenceKey); } catch {}
+  let preferred = browserPreference === "monaco" ? "monaco" : "current";
+  let preferenceRestored = false;
   const label = document.createElement("label"); label.textContent = "Renderer ";
   const select = document.createElement("select"); select.id = "diff-renderer"; select.setAttribute("aria-label", "Diff renderer");
   // Keep the persisted value "current" compatible with existing installations.
   select.add(new Option("Shiki", "current")); select.add(new Option("Monaco", "monaco")); select.value = preferred;
+  select.disabled = true; // Restore the saved reviewer preference before accepting changes.
   label.append(select); controls.append(label);
   const region = document.createElement("section"); region.className = "monaco-review-region"; region.hidden = true;
   const notice = document.createElement("p"); notice.className = "monaco-review-notice"; notice.setAttribute("role", "status");
@@ -355,6 +358,7 @@ export function createMonacoRenderer({ container, controls, token, snapshot, ren
     cancelPhase();
     preferred = select.value; failed = false; pendingFailure = null;
     try { localStorage.setItem(preferenceKey, preferred); } catch {}
+    actions.saveRendererPreference(preferred);
     if (preferred === "current") closeAll();
     render();
   };
@@ -449,6 +453,19 @@ export function createMonacoRenderer({ container, controls, token, snapshot, ren
   }
   const controller = {
     owns, sync, hasDraft, setCommentsVisible, preservePhasePosition,
+    restorePreference(saved) {
+      if (preferenceRestored) return;
+      preferenceRestored = true;
+      if (["current", "monaco"].includes(saved)) {
+        preferred = saved;
+        try { localStorage.setItem(preferenceKey, preferred); } catch {}
+      } else if (["current", "monaco"].includes(browserPreference)) {
+        // Migrate the old port-scoped choice only when no durable choice exists.
+        actions.saveRendererPreference(preferred);
+      }
+      select.value = preferred;
+      select.disabled = false;
+    },
     updateMobileActions: mobileActions.update,
     cancelPhase,
     async preparePhase(session) {

scripts/check-monaco.ts

@@ -23,7 +23,7 @@ try {
 }
 const groups = {
   briefing: { samplePr: false, scripts: ["briefing-entry", "briefing-source", "briefing", "briefing-code", "briefing-diagrams"] },
-  local: { samplePr: false, scripts: ["monaco-foundation", "monaco-scroll", "file-header", "monaco-edits", "monaco-edits-syntax", "monaco-themes", "monaco-gutter", "monaco-wrapping", "markdown-preview", "monaco-semantic", "monaco-mount", "monaco-paged-draft"] },
+  local: { samplePr: false, scripts: ["monaco-foundation", "monaco-scroll", "file-header", "monaco-edits", "monaco-edits-syntax", "monaco-themes", "renderer-preference", "monaco-gutter", "monaco-wrapping", "markdown-preview", "monaco-semantic", "monaco-mount", "monaco-paged-draft"] },
   navigation: { samplePr: true, scripts: ["monaco-phase", "monaco-phase-scroll", "monaco-upward-tour", "monaco-reviewed-scroll", "monaco-sticky", "monaco-file-explorer"] },
   discussions: { samplePr: true, scripts: ["renderer-comments", "monaco-context", "monaco-edits", "monaco-edits-syntax", "monaco-discussions"] },
   before: { samplePr: true, beforeTour: true, scripts: ["monaco-before-tour"] },

scripts/renderer-preference-browser.mjs

@@ -0,0 +1,82 @@
+// Renderer choice must survive a new server/port, where localStorage is empty.
+import assert from 'node:assert/strict';
+import { copyFile } from 'node:fs/promises';
+import { dirname, join } from 'node:path';
+import { pathToFileURL } from 'node:url';
+import { ReviewSession } from '../src/session.ts';
+import { startServer } from '../src/server.ts';
+
+const [playwrightPath, url, repository] = process.argv.slice(2);
+const { chromium } = await import(pathToFileURL(playwrightPath).href);
+const browser = await chromium.launch({ channel: process.env.CI ? undefined : 'chrome', headless: true });
+const context = await browser.newContext({ viewport: { width: 1600, height: 1000 }, reducedMotion: 'reduce' });
+const page = await context.newPage();
+page.setDefaultTimeout(20000);
+const errors = []; page.on('pageerror', error => errors.push(error.message));
+let session, server;
+const key = 'diffcompass-diff-renderer';
+const saved = value => page.waitForResponse(response => response.url().endsWith('/api/preferences')
+  && response.request().postDataJSON()?.diffRenderer === value && response.ok());
+const monacoReady = () => page.waitForFunction(() => [...document.querySelectorAll('.monaco-review-frame')].some(frame =>
+  frame.dataset.monacoPath === 'src/cart.ts' && frame.contentWindow?.diffLab?.views.get('src/cart.ts')?.diff.getLineChanges()));
+try {
+  // Upgrade an existing browser-only preference without another manual switch.
+  await context.addInitScript(({ key, origin }) => {
+    if (location.origin === origin) localStorage.setItem(key, 'monaco');
+    window.initialRendererPreference = localStorage.getItem(key);
+  }, { key, origin: new URL(url).origin });
+  const migrated = saved('monaco');
+  await page.goto(url);
+  await migrated; await monacoReady();
+
+  // Resume a separate review using the shared settings but a different port.
+  const directory = dirname(repository);
+  const stateFile = join(directory, 'renderer-session.json');
+  await copyFile(join(directory, 'session.json'), stateFile);
+  session = await ReviewSession.create(repository, undefined, {
+    stateFile, resume: true, agentAdapters: [],
+    diffPreferenceDirectory: join(dirname(directory), 'preferences'),
+  });
+  server = startServer(session, 0, directory);
+  const nextUrl = `http://127.0.0.1:${server.port}/#token=${session.browserToken}`;
+  assert.notEqual(new URL(nextUrl).origin, new URL(url).origin);
+  await page.goto(nextUrl); await monacoReady();
+  assert.equal(await page.evaluate(() => window.initialRendererPreference), null, 'New port starts without a browser preference');
+  assert.equal(await page.locator('#diff-renderer').inputValue(), 'monaco');
+  await page.locator('#briefing-tab').click();
+  const source = page.locator('#briefing-view [data-evidence-id="subtotal-body"]');
+  await source.scrollIntoViewIfNeeded();
+  await source.locator('.briefing-code-frame:not([hidden])').waitFor();
+
+  // Server settings win over stale storage, and also work with storage blocked.
+  await page.evaluate(key => localStorage.setItem(key, 'current'), key);
+  await page.reload(); await monacoReady();
+  assert.equal(await page.locator('#diff-renderer').inputValue(), 'monaco');
+  await page.close();
+  // Use a fresh context so an explicit Shiki selection cannot rely on any cache.
+  const clean = await browser.newPage({ viewport: { width: 1600, height: 1000 }, reducedMotion: 'reduce' });
+  clean.setDefaultTimeout(20000);
+  clean.on('pageerror', error => errors.push(error.message));
+  await clean.addInitScript(key => {
+    const get = Storage.prototype.getItem, set = Storage.prototype.setItem;
+    Storage.prototype.getItem = function(name) { if (name === key) throw new Error('Storage unavailable'); return get.call(this, name); };
+    Storage.prototype.setItem = function(name, value) { if (name === key) throw new Error('Storage unavailable'); return set.call(this, name, value); };
+  }, key);
+  await clean.route('**/api/session', route => route.fulfill({ status: 503, json: { error: 'Temporary startup failure' } }), { times: 1 });
+  await clean.goto(nextUrl);
+  await clean.waitForFunction(() => !document.getElementById('diff-renderer')?.disabled);
+  assert.equal(await clean.locator('#diff-renderer').inputValue(), 'monaco');
+  const shikiSaved = clean.waitForResponse(response => response.url().endsWith('/api/preferences')
+    && response.request().postDataJSON()?.diffRenderer === 'current' && response.ok());
+  await clean.locator('#diff-renderer').selectOption('current'); await shikiSaved;
+  await clean.reload();
+  await clean.waitForFunction(() => !document.getElementById('diff-renderer')?.disabled);
+  assert.equal(await clean.locator('#diff-renderer').inputValue(), 'current');
+  assert.equal(await clean.locator('.monaco-review-frame').count(), 0);
+  assert.deepEqual(errors, []);
+  console.log('PASS renderer migration, new review/port, Briefing inheritance, stale storage precedence, startup retry, and Shiki persistence without browser storage');
+} finally {
+  await browser.close();
+  await server?.stop(true);
+  await session?.dispose();
+}

src/diff-preferences.ts

@@ -1,12 +1,13 @@
 import { chmod, mkdir, readFile, rename, writeFile } from "node:fs/promises";
 import { dirname, join, resolve } from "node:path";
 import { acquireSessionLock } from "./session-library.ts";
-import { isBriefingSectionOrder, type BriefingPreferences, type DiffViewPreferences, type SessionDiffPreferences } from "./types.ts";
+import { isBriefingSectionOrder, isDiffRenderer, type BriefingPreferences, type DiffRenderer, type DiffViewPreferences, type SessionDiffPreferences } from "./types.ts";
 
 interface DiffPreferenceStore {
   schemaVersion: 1;
   repositories: Record<string, SessionDiffPreferences>;
   briefing?: BriefingPreferences;
+  diffRenderer?: DiffRenderer;
 }
 
 const STORE_NAME = ".diff-preferences.json";
@@ -57,6 +58,7 @@ async function loadStore(directory: string): Promise<DiffPreferenceStore> {
       schemaVersion: 1,
       repositories,
       briefing: validBriefingPreferences(value.briefing) ? structuredClone(value.briefing) : undefined,
+      diffRenderer: isDiffRenderer(value.diffRenderer) ? value.diffRenderer : undefined,
     };
   } catch (error) {
     if ((error as NodeJS.ErrnoException).code === "ENOENT") return { schemaVersion: 1, repositories: {} };
@@ -107,6 +109,20 @@ export async function loadGlobalBriefingPreferences(directory: string | undefine
   return store.briefing ? structuredClone(store.briefing) : undefined;
 }
 
+export async function loadGlobalDiffRenderer(directory: string | undefined): Promise<DiffRenderer | undefined> {
+  return directory ? (await loadStore(directory)).diffRenderer : undefined;
+}
+
+export async function updateGlobalDiffRenderer(directory: string | undefined, renderer: DiffRenderer | undefined): Promise<DiffRenderer | undefined> {
+  if (!directory) return undefined;
+  return updateStore(directory, (store) => {
+    const previous = store.diffRenderer;
+    if (renderer === undefined) delete store.diffRenderer;
+    else store.diffRenderer = renderer;
+    return previous;
+  });
+}
+
 export async function updateGlobalBriefingPreferences(directory: string | undefined, preferences: BriefingPreferences): Promise<BriefingPreferences | undefined> {
   if (!directory) return undefined;
   return updateStore(directory, (store) => {

src/server.ts

@@ -50,7 +50,7 @@ import { resolveSymbolPeek } from "./symbol-peek.ts";
 import { ReviewSession } from "./session.ts";
 import { deleteReviewSession, listReviewSessions, resolveReviewSession, reviewSessionDirectory, updateReviewSessionMetadata } from "./session-library.ts";
 import { createUpdateService, type UpdateService } from "./update.ts";
-import { isBriefingSectionOrder, type BriefingPreferences } from "./types.ts";
+import { isBriefingSectionOrder, isDiffRenderer, type BriefingPreferences } from "./types.ts";
 import {
   enumValue,
   exactKeys,
@@ -283,7 +283,13 @@ export function createRequestHandler(session: ReviewSession, sessionDirectory =
       }
       if (request.method === "POST" && url.pathname === "/api/preferences") {
         const input = await readJsonObject(request);
-        exactKeys(input, ["prefetchSteps", "diffLayout", "diffWrap", "mobile", "showBriefing", "briefingArchitecture", "briefingSequenceDiagrams", "briefingCallFlows", "briefingNeedsAttention", "briefingReviewEffort", "briefingOrder"]);
+        exactKeys(input, ["prefetchSteps", "diffLayout", "diffWrap", "diffRenderer", "mobile", "showBriefing", "briefingArchitecture", "briefingSequenceDiagrams", "briefingCallFlows", "briefingNeedsAttention", "briefingReviewEffort", "briefingOrder"]);
+        if ("diffRenderer" in input) {
+          if (Object.keys(input).length !== 1 || !isDiffRenderer(input.diffRenderer)) {
+            throw new HttpError(422, "Diff renderer must be current or monaco.");
+          }
+          return json(await session.setDiffRenderer(input.diffRenderer));
+        }
         if (input.prefetchSteps !== undefined) {
           if (Object.keys(input).length !== 1 || !Number.isInteger(input.prefetchSteps) || Number(input.prefetchSteps) < 0 || Number(input.prefetchSteps) > 5) {
             throw new HttpError(422, "Prepare ahead must be between 0 and 5 steps.");