Keep layout and wrapping preferences across all reviews

README.md

@@ -48,7 +48,9 @@ The review sidebar has separate **Tour**, **Questions**, and **Comments** tabs,
 with drafts and reading positions retained when you switch between them.
 
 Briefing shares the Shiki/Monaco, Split/Unified, and Scroll/Wrap preferences with
-Changes. Resize or collapse the evidence pane, choose **Wide briefing**, and
+Changes. These choices follow you across repositories and resumed reviews, with
+separate layout and wrapping preferences for desktop and mobile.
+Resize or collapse the evidence pane, choose **Wide briefing**, and
 export an offline HTML reader with its diagrams and saved evidence.
 
 See the [Briefing guide](docs/briefing.md) for saved reading state, regeneration,

public/app.js

@@ -1249,7 +1249,8 @@ function diffLayoutPreferenceKey() {
 function preferredDiffLayout() {
   const sessionPreference = typeof state !== "undefined" ? state.session?.diffPreferences?.[narrowFilePanelLayout() ? "mobile" : "desktop"]?.layout : undefined;
   if (["unified", "split"].includes(sessionPreference)) return sessionPreference;
-  const saved = window.localStorage.getItem(diffLayoutPreferenceKey());
+  let saved;
+  try { saved = window.localStorage.getItem(diffLayoutPreferenceKey()); } catch {}
   if (["unified", "split"].includes(saved)) return saved;
   return narrowFilePanelLayout() ? "unified" : state.session.preferences?.diffLayout ?? "split";
 }
@@ -1261,7 +1262,8 @@ function diffWrapPreferenceKey() {
 function preferredDiffWrap() {
   const sessionPreference = typeof state !== "undefined" ? state.session?.diffPreferences?.[narrowFilePanelLayout() ? "mobile" : "desktop"]?.wrap : undefined;
   if (["scroll", "wrap"].includes(sessionPreference)) return sessionPreference;
-  const saved = window.localStorage.getItem(diffWrapPreferenceKey());
+  let saved;
+  try { saved = window.localStorage.getItem(diffWrapPreferenceKey()); } catch {}
   return ["scroll", "wrap"].includes(saved) ? saved : "scroll";
 }
 
@@ -1776,8 +1778,10 @@ function changeDiffView(layout, wrap, persist = true) {
   state.diffLayout = layout;
   state.diffWrap = wrap;
   if (persist) {
-    window.localStorage.setItem(diffLayoutPreferenceKey(), state.diffLayout);
-    window.localStorage.setItem(diffWrapPreferenceKey(), state.diffWrap);
+    try {
+      window.localStorage.setItem(diffLayoutPreferenceKey(), state.diffLayout);
+      window.localStorage.setItem(diffWrapPreferenceKey(), state.diffWrap);
+    } catch {}
     if (typeof persistDiffPreferences === "function") persistDiffPreferences();
   }
   if (rerender) {

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", "monaco-click-focus", "media", "file-header", "monaco-edits", "monaco-edits-syntax", "monaco-themes", "renderer-preference", "monaco-gutter", "monaco-wrapping", "markdown-preview", "monaco-semantic", "monaco-mount", "monaco-paged-draft"] },
+  local: { samplePr: false, scripts: ["monaco-foundation", "monaco-scroll", "monaco-click-focus", "media", "file-header", "monaco-edits", "monaco-edits-syntax", "monaco-themes", "renderer-preference", "diff-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: ["review-pane", "renderer-comments", "monaco-context", "monaco-edits", "monaco-edits-syntax", "monaco-discussions"] },
   before: { samplePr: true, beforeTour: true, scripts: ["monaco-before-tour"] },

scripts/diff-preference-browser.mjs

@@ -0,0 +1,99 @@
+// Reviewer view preferences survive different repositories, ports and stale sessions.
+import assert from 'node:assert/strict';
+import { cp } 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';
+import { loadPersistedSession, savePersistedSession } from '../src/store.ts';
+
+const [playwrightPath, url, repository] = process.argv.slice(2);
+assert.match(repository, /diffcompass-monaco-production-[^/]+\/fixture\/repository$/);
+const { chromium } = await import(pathToFileURL(playwrightPath).href);
+const browser = await chromium.launch({ channel: process.env.CI ? undefined : 'chrome', headless: true });
+const page = await browser.newPage({ viewport: { width: 1600, height: 1000 }, reducedMotion: 'reduce' });
+page.setDefaultTimeout(20000);
+const errors = [];
+page.on('pageerror', error => errors.push(error.message));
+let session, server;
+const ready = () => page.locator('#diff-renderer:enabled').waitFor({ state: 'attached' });
+const selected = async (layout, wrap) => {
+  await page.locator(`[data-diff-layout="${layout}"][aria-pressed="true"]`).waitFor({ state: 'attached' });
+  await page.locator(`[data-diff-wrap="${wrap}"][aria-pressed="true"]`).waitFor({ state: 'attached' });
+};
+async function choose(kind, value, mobile = false) {
+  const field = kind === 'layout' ? 'diffLayout' : 'diffWrap';
+  const [response] = await Promise.all([
+    page.waitForResponse(response => response.url().endsWith('/api/preferences')
+      && response.request().postDataJSON()?.[field] === value
+      && response.request().postDataJSON()?.mobile === mobile),
+    page.locator(`[data-diff-${kind}="${value}"]`).click(),
+  ]);
+  assert.equal(response.status(), 200);
+}
+try {
+  await page.goto(url); await ready();
+  await choose('layout', 'unified');
+  await choose('wrap', 'wrap');
+  await selected('unified', 'wrap');
+
+  await page.setViewportSize({ width: 390, height: 1000 });
+  await choose('layout', 'split', true);
+  await choose('wrap', 'wrap', true);
+  await selected('split', 'wrap');
+
+  const directory = dirname(repository);
+  const otherRepository = join(directory, 'other-repository');
+  await cp(repository, otherRepository, { recursive: true });
+  const stateFile = join(directory, 'diff-preference-session.json');
+  const saved = await loadPersistedSession(join(directory, 'session.json'));
+  saved.repositoryRoot = otherRepository;
+  saved.diffPreferences = {
+    desktop: { layout: 'split', wrap: 'scroll' },
+    mobile: { layout: 'unified', wrap: 'scroll' },
+  };
+  await savePersistedSession(stateFile, saved);
+  session = await ReviewSession.create(otherRepository, 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.addInitScript(() => { window.initialStorageCount = localStorage.length; });
+  await page.setViewportSize({ width: 1600, height: 1000 });
+  await page.goto(nextUrl); await ready();
+  assert.equal(await page.evaluate(() => window.initialStorageCount), 0, 'New port has no browser settings');
+  await selected('unified', 'wrap');
+  await page.setViewportSize({ width: 390, height: 1000 });
+  await selected('split', 'wrap');
+  console.log('PASS global layout/wrap across repositories and ports; stale sessions cannot override desktop/mobile choices');
+
+  // Saving must still reach the server if the optional browser cache rejects it.
+  await page.addInitScript(() => {
+    for (const method of ['getItem', 'setItem']) {
+      const original = Storage.prototype[method];
+      Storage.prototype[method] = function(key, ...args) {
+        if (/:(mobile-)?diff-(layout|wrap)$/.test(key)) throw Error('View preference storage unavailable');
+        return original.call(this, key, ...args);
+      };
+    }
+  });
+  await page.setViewportSize({ width: 1600, height: 1000 });
+  await page.reload(); await ready();
+  await choose('layout', 'split');
+  await choose('wrap', 'scroll');
+  await page.reload(); await ready();
+  await selected('split', 'scroll');
+  await page.setViewportSize({ width: 390, height: 1000 });
+  await selected('split', 'wrap');
+  assert.deepEqual(errors, []);
+  console.log('PASS layout/wrap saves and reloads with browser preference storage unavailable');
+} catch (error) {
+  await page.screenshot({ path: '/tmp/dc-diff-preferences-failure.png', timeout: 5000 }).catch(() => {});
+  throw error;
+} finally {
+  await browser.close();
+  await server?.stop(true);
+  await session?.dispose();
+}

src/diff-preferences.ts

@@ -11,6 +11,9 @@ interface DiffPreferenceStore {
 }
 
 const STORE_NAME = ".diff-preferences.json";
+// A non-path key keeps reviewer defaults intact when an older running review
+// rewrites this store: older readers preserve this map but drop new root fields.
+const GLOBAL_DIFF_PREFERENCES_KEY = "*";
 let updateQueue: Promise<void> = Promise.resolve();
 
 function validPreferences(value: unknown): value is DiffViewPreferences {
@@ -96,11 +99,29 @@ export function diffPreferenceStoreDirectory(stateFile?: string): string | undef
   return stateFile ? dirname(stateFile) : undefined;
 }
 
-export async function loadGlobalDiffPreferences(directory: string | undefined, repositoryRoot: string): Promise<SessionDiffPreferences | undefined> {
+export async function loadGlobalDiffPreferences(
+  directory: string | undefined,
+  repositoryRoot: string,
+  sessionPreferences?: SessionDiffPreferences,
+): Promise<SessionDiffPreferences | undefined> {
   if (!directory) return undefined;
   const store = await loadStore(directory);
-  const preferences = store.repositories[repositoryKey(repositoryRoot)];
-  return preferences ? structuredClone(preferences) : undefined;
+  const key = repositoryKey(repositoryRoot);
+  const global = store.repositories[GLOBAL_DIFF_PREFERENCES_KEY];
+  const legacy = { ...sessionPreferences, ...store.repositories[key] };
+  const needsMigration = (["desktop", "mobile"] as const).some((scope) => !global?.[scope] && legacy?.[scope]);
+  if (!needsMigration) return global ? structuredClone(global) : undefined;
+  // Carry existing choices forward when their review is first opened. Never
+  // guess between other repositories' conflicting legacy choices.
+  return updateStore(directory, (current) => {
+    const preferences = {
+      ...sessionPreferences,
+      ...current.repositories[key],
+      ...current.repositories[GLOBAL_DIFF_PREFERENCES_KEY],
+    };
+    current.repositories[GLOBAL_DIFF_PREFERENCES_KEY] = preferences;
+    return structuredClone(preferences);
+  });
 }
 
 export async function loadGlobalBriefingPreferences(directory: string | undefined): Promise<BriefingPreferences | undefined> {
@@ -142,13 +163,12 @@ export async function restoreGlobalBriefingPreferences(directory: string | undef
 
 export async function updateGlobalDiffPreferences(
   directory: string | undefined,
-  repositoryRoot: string,
   scope: "desktop" | "mobile",
   preferences: DiffViewPreferences,
 ): Promise<SessionDiffPreferences | undefined> {
   if (!directory) return undefined;
   return updateStore(directory, (store) => {
-    const key = repositoryKey(repositoryRoot);
+    const key = GLOBAL_DIFF_PREFERENCES_KEY;
     const previous = structuredClone(store.repositories[key]);
     store.repositories[key] = { ...store.repositories[key], [scope]: structuredClone(preferences) };
     return previous;
@@ -157,13 +177,19 @@ export async function updateGlobalDiffPreferences(
 
 export async function restoreGlobalDiffPreferences(
   directory: string | undefined,
-  repositoryRoot: string,
+  scope: "desktop" | "mobile",
   previous: SessionDiffPreferences | undefined,
+  attempted: DiffViewPreferences,
 ): Promise<void> {
   if (!directory) return;
   await updateStore(directory, (store) => {
-    const key = repositoryKey(repositoryRoot);
-    if (previous) store.repositories[key] = structuredClone(previous);
+    const key = GLOBAL_DIFF_PREFERENCES_KEY;
+    const current = store.repositories[key];
+    if (current?.[scope]?.layout !== attempted.layout || current?.[scope]?.wrap !== attempted.wrap) return;
+    const restored = { ...current };
+    if (previous?.[scope]) restored[scope] = structuredClone(previous[scope]);
+    else delete restored[scope];
+    if (Object.keys(restored).length) store.repositories[key] = restored;
     else delete store.repositories[key];
   });
 }

src/session.ts

@@ -200,7 +200,9 @@ export class ReviewSession {
       ...(defaults.preferences ?? {}),
       ...(persisted?.preferences ?? {}),
     });
-    this.diffPreferences = structuredClone(persisted?.diffPreferences ?? initialDiffPreferences);
+    this.diffPreferences = initialDiffPreferences || persisted?.diffPreferences
+      ? structuredClone({ ...persisted?.diffPreferences, ...initialDiffPreferences })
+      : undefined;
     const configuredReviewPolicy = structuredClone(persisted?.reviewPolicy ?? defaults.reviewPolicy ?? DEFAULT_REVIEW_POLICY);
     const pullRequestReview = Boolean(persisted?.pullRequest ?? defaults.pullRequest);
     this.reviewPolicy = pullRequestReview && defaults.includeFindings !== false
@@ -864,7 +866,7 @@ export class ReviewSession {
     )) throw new ReviewError("The persisted review does not match the requested repository comparison.");
     const adapters = structuredClone(defaults.agentAdapters ?? await discoverAgentAdapters(comparison.repositoryRoot));
     const preferenceRepositoryRoot = persisted?.pullRequest?.sourceRepositoryRoot ?? defaults.pullRequest?.sourceRepositoryRoot ?? comparison.repositoryRoot;
-    const globalDiffPreferences = await loadGlobalDiffPreferences(preferenceDirectory, preferenceRepositoryRoot);
+    const globalDiffPreferences = await loadGlobalDiffPreferences(preferenceDirectory, preferenceRepositoryRoot, persisted?.diffPreferences);
     const attached = defaults.attached || Boolean(persisted?.bridgeToken);
     const bridge = attached ? new AttachedAgentBridge(
       persisted?.bridgeToken,
@@ -1589,10 +1591,9 @@ export class ReviewSession {
         throw new ReviewError("Diff preferences are invalid.");
       }
       const previous = structuredClone(this.diffPreferences);
-      const preferenceRepositoryRoot = this.pullRequest?.sourceRepositoryRoot ?? this.comparison.repositoryRoot;
       const preferenceDirectory = this.defaults.diffPreferenceDirectory ?? diffPreferenceStoreDirectory(this.defaults.stateFile);
-      const current = this.diffPreferences?.[scope];
-      if (current?.layout === preferences.layout && current.wrap === preferences.wrap) return this.snapshot();
+      // Another open review may have changed the global default since this
+      // session loaded, so even reselecting its current choice must be saved.
       this.diffPreferences = {
         ...this.diffPreferences,
         [scope]: structuredClone(preferences),
@@ -1602,13 +1603,13 @@ export class ReviewSession {
       let globalUpdated = false;
       let previousGlobal: SessionDiffPreferences | undefined;
       try {
-        previousGlobal = await updateGlobalDiffPreferences(preferenceDirectory, preferenceRepositoryRoot, scope, preferences);
+        previousGlobal = await updateGlobalDiffPreferences(preferenceDirectory, scope, preferences);
         globalUpdated = preferenceDirectory !== undefined;
         await this.persist();
       } catch (error) {
         this.diffPreferences = previous;
         this.revision = previousRevision;
-        if (globalUpdated) await restoreGlobalDiffPreferences(preferenceDirectory, preferenceRepositoryRoot, previousGlobal).catch(() => undefined);
+        if (globalUpdated) await restoreGlobalDiffPreferences(preferenceDirectory, scope, previousGlobal, preferences).catch(() => undefined);
         throw error;
       }
       return this.snapshot();