diff --git a/README.md b/README.md index 081717b..eaeed47 100644 --- a/README.md +++ b/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, diff --git a/public/app.js b/public/app.js index 0b53fb9..50e97c5 100644 --- a/public/app.js +++ b/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) { diff --git a/scripts/check-monaco.ts b/scripts/check-monaco.ts index 2d17b26..3b40e68 100644 --- a/scripts/check-monaco.ts +++ b/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"] }, diff --git a/scripts/diff-preference-browser.mjs b/scripts/diff-preference-browser.mjs new file mode 100644 index 0000000..badb1fa --- /dev/null +++ b/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(); +} diff --git a/src/diff-preferences.ts b/src/diff-preferences.ts index 2f06db0..9cab80a 100644 --- a/src/diff-preferences.ts +++ b/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 = 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 { +export async function loadGlobalDiffPreferences( + directory: string | undefined, + repositoryRoot: string, + sessionPreferences?: SessionDiffPreferences, +): Promise { 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 { @@ -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 { 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 { 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]; }); } diff --git a/src/session.ts b/src/session.ts index 76e4230..02b9d2a 100644 --- a/src/session.ts +++ b/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(); diff --git a/tests/lifecycle.test.ts b/tests/lifecycle.test.ts index 26774fd..addefbc 100644 --- a/tests/lifecycle.test.ts +++ b/tests/lifecycle.test.ts @@ -7,6 +7,7 @@ import { loadComparison } from "../src/git.ts"; import { ReviewSession } from "../src/session.ts"; import { loadPersistedSession, savePersistedSession } from "../src/store.ts"; import { createRequestHandler } from "../src/server.ts"; +import { loadGlobalDiffPreferences, restoreGlobalDiffPreferences, updateGlobalDiffPreferences } from "../src/diff-preferences.ts"; import { briefingFingerprint } from "../src/briefing.ts"; import { exportReview } from "../src/export.ts"; import type { PersistedReviewSession } from "../src/types.ts"; @@ -332,15 +333,16 @@ test("selected diff preferences survive an existing session resume", async () => }); }); -test("new sessions inherit the last repository-scoped diff preferences", async () => { +test("new sessions inherit global diff preferences across repositories", async () => { const { directory, repository, stateFile } = await repositoryFixture(); + const other = await repositoryFixture(); const first = await ReviewSession.create(repository, undefined, { ...noAdapters, stateFile }); const nextStateFile = join(directory, "next-session.json"); try { await first.setDiffPreferences("desktop", { layout: "unified", wrap: "wrap" }); await first.setDiffPreferences("mobile", { layout: "split", wrap: "scroll" }); - const next = await ReviewSession.create(repository, undefined, { ...noAdapters, stateFile: nextStateFile }); + const next = await ReviewSession.create(other.repository, undefined, { ...noAdapters, stateFile: nextStateFile }); try { expect(next.snapshot().diffPreferences).toEqual({ desktop: { layout: "unified", wrap: "wrap" }, @@ -350,6 +352,117 @@ test("new sessions inherit the last repository-scoped diff preferences", async ( } finally { await first.dispose(); } }); +test("resuming an older review uses the latest global diff preferences for each device", async () => { + const first = await repositoryFixture(); + const other = await repositoryFixture(); + const defaults = { ...noAdapters, diffPreferenceDirectory: first.directory }; + const sessions: ReviewSession[] = []; + const open = async (repository: string, stateFile: string, resume = false) => { + const session = await ReviewSession.create(repository, undefined, { ...defaults, stateFile, resume }); + sessions.push(session); + return session; + }; + try { + const original = await open(first.repository, first.stateFile); + await original.setDiffPreferences("desktop", { layout: "split", wrap: "scroll" }); + const next = await open(other.repository, other.stateFile); + await next.setDiffPreferences("desktop", { layout: "unified", wrap: "wrap" }); + await next.setDiffPreferences("mobile", { layout: "split", wrap: "wrap" }); + const resumed = await open(first.repository, first.stateFile, true); + expect(resumed.snapshot().diffPreferences).toEqual({ + desktop: { layout: "unified", wrap: "wrap" }, + mobile: { layout: "split", wrap: "wrap" }, + }); + + // A choice from an already open review still changes the global default, + // even when that review already displays the requested values. + await original.setDiffPreferences("desktop", { layout: "split", wrap: "scroll" }); + const again = await open(other.repository, other.stateFile, true); + expect(again.snapshot().diffPreferences).toEqual({ + desktop: { layout: "split", wrap: "scroll" }, + mobile: { layout: "split", wrap: "wrap" }, + }); + } finally { + for (const session of sessions) await session.dispose(); + } +}); + +test("legacy diff preferences migrate without an older settings writer erasing global choices", async () => { + const first = await repositoryFixture(); + const other = await repositoryFixture(); + const storePath = join(first.directory, ".diff-preferences.json"); + await writeFile(storePath, JSON.stringify({ schemaVersion: 1, diffRenderer: "monaco", repositories: { + [first.repository]: { desktop: { layout: "unified", wrap: "wrap" } }, + } })); + const original = await ReviewSession.create(first.repository, undefined, { ...noAdapters, stateFile: first.stateFile }); + try { + expect(original.snapshot().diffPreferences?.desktop).toEqual({ layout: "unified", wrap: "wrap" }); + // Previous binaries retain the repository map but discard unknown top-level + // fields when saving renderer, briefing, or repository-scoped preferences. + const saved = JSON.parse(await readFile(storePath, "utf8")); + await writeFile(storePath, JSON.stringify({ schemaVersion: 1, repositories: saved.repositories, diffRenderer: "current" })); + const next = await ReviewSession.create(other.repository, undefined, { ...noAdapters, stateFile: other.stateFile, diffPreferenceDirectory: first.directory }); + try { + expect(next.snapshot().diffPreferences?.desktop).toEqual({ layout: "unified", wrap: "wrap" }); + expect(next.snapshot().diffRenderer).toBe("current"); + } finally { await next.dispose(); } + } finally { await original.dispose(); } +}); + +test("legacy session diff preferences fill missing global devices without replacing newer choices", async () => { + const { directory, repository, stateFile } = await repositoryFixture(); + const original = await ReviewSession.create(repository, undefined, { ...noAdapters, stateFile }); + await original.setDiffPreferences("desktop", { layout: "split", wrap: "scroll" }); + await original.setDiffPreferences("mobile", { layout: "split", wrap: "wrap" }); + await original.dispose(); + await writeFile(join(directory, ".diff-preferences.json"), JSON.stringify({ schemaVersion: 1, repositories: { + [repository]: { desktop: { layout: "split", wrap: "scroll" } }, + } })); + await updateGlobalDiffPreferences(directory, "desktop", { layout: "unified", wrap: "wrap" }); + const resumed = await ReviewSession.create(repository, undefined, { ...noAdapters, stateFile, resume: true }); + try { + const expected = { desktop: { layout: "unified", wrap: "wrap" }, mobile: { layout: "split", wrap: "wrap" } } as const; + expect(resumed.snapshot().diffPreferences).toEqual(expected); + const other = await repositoryFixture(); + expect(await loadGlobalDiffPreferences(directory, other.repository)).toEqual(expected); + } finally { await resumed.dispose(); } +}); + +test("failed diff preference saves roll back global choices", async () => { + const { repository, stateFile, directory } = await repositoryFixture(); + const session = await ReviewSession.create(repository, undefined, { ...noAdapters, stateFile }); + try { + await session.setDiffPreferences("desktop", { layout: "unified", wrap: "wrap" }); + await session.setDiffRenderer("monaco"); + await rm(stateFile); + await mkdir(stateFile); + await expect(session.setDiffPreferences("desktop", { layout: "split", wrap: "scroll" })).rejects.toThrow(); + const other = await repositoryFixture(); + const next = await ReviewSession.create(other.repository, undefined, { ...noAdapters, stateFile: other.stateFile, diffPreferenceDirectory: directory }); + try { + expect(next.snapshot().diffPreferences?.desktop).toEqual({ layout: "unified", wrap: "wrap" }); + expect(next.snapshot().diffRenderer).toBe("monaco"); + } finally { await next.dispose(); } + } finally { await session.dispose(); } +}); + +test("rolling back diff preferences preserves choices saved by another review", async () => { + const { directory, repository } = await repositoryFixture(); + const original = { layout: "split", wrap: "scroll" } as const; + const attempted = { layout: "unified", wrap: "wrap" } as const; + await updateGlobalDiffPreferences(directory, "desktop", original); + const previous = await updateGlobalDiffPreferences(directory, "desktop", attempted); + await updateGlobalDiffPreferences(directory, "mobile", attempted); + await restoreGlobalDiffPreferences(directory, "desktop", previous, attempted); + expect(await loadGlobalDiffPreferences(directory, repository)).toEqual({ desktop: original, mobile: attempted }); + + await updateGlobalDiffPreferences(directory, "desktop", attempted); + const newer = { layout: "split", wrap: "wrap" } as const; + await updateGlobalDiffPreferences(directory, "desktop", newer); + await restoreGlobalDiffPreferences(directory, "desktop", previous, attempted); + expect(await loadGlobalDiffPreferences(directory, repository)).toEqual({ desktop: newer, mobile: attempted }); +}); + test("renderer choice follows the reviewer across repositories and resumed reviews", async () => { const first = await repositoryFixture(); const second = await repositoryFixture(); @@ -368,7 +481,7 @@ test("renderer choice follows the reviewer across repositories and resumed revie const next = await open(second.repository, second.stateFile); expect(next.snapshot().diffRenderer).toBe("monaco"); - expect(next.snapshot().diffPreferences).toBeUndefined(); + expect(next.snapshot().diffPreferences?.desktop).toEqual({ layout: "unified", wrap: "wrap" }); // Updating other saved preferences must retain the global renderer. await next.setDiffPreferences("mobile", { layout: "split", wrap: "scroll" }); const resumed = await open(first.repository, first.stateFile, true);