From dc8c7aa9d71eaa5ac5b9dbdbe2e129eccfac8186 Mon Sep 17 00:00:00 2001 From: FND Date: Sat, 5 Sep 2026 14:36:55 +0200 Subject: [PATCH 1/2] fix(review): preserve fresh-profile setup eligibility Fixes #1463. Keep the review panel default in memory until explicitly persisted, so ordinary settings reads do not impersonate a returning reviewer's choice. --- .../review-editor/utils/reviewSetup.test.ts | 41 ++++++++++++++++--- packages/review-editor/utils/reviewSetup.ts | 2 + .../config/configStore.lazyInit.seam.test.ts | 18 ++++++++ packages/ui/config/configStore.ts | 12 +++--- packages/ui/config/settings.ts | 4 ++ 5 files changed, 66 insertions(+), 11 deletions(-) diff --git a/packages/review-editor/utils/reviewSetup.test.ts b/packages/review-editor/utils/reviewSetup.test.ts index 46a2a3d44..a1487ed8f 100644 --- a/packages/review-editor/utils/reviewSetup.test.ts +++ b/packages/review-editor/utils/reviewSetup.test.ts @@ -3,6 +3,7 @@ import { readFileSync } from 'node:fs'; import { join } from 'node:path'; import { resetStorageBackend, setStorageBackend } from '@plannotator/ui/utils/storage'; import { ConfigStoreForTest } from '../../ui/config/configStore'; +import { setReviewPanelView } from '../../ui/config/reviewView'; import { initializeReviewSetup, needsReviewSetup, @@ -32,15 +33,27 @@ afterEach(() => { }); describe('initializeReviewSetup', () => { - test('a genuinely new reviewer starts with Tree while keeping the since-base diff default', () => { + test('a fresh reviewer reaches setup once after normal settings startup', () => { installMemoryBackend(); const store = makeStore(); + // Rendering reads settings before /api/diff initializes server config. + // Neither step is evidence that the reviewer chose the registry default. + store.get('displayName'); + store.init(); + expect(initializeReviewSetup(store)).toBe(true); expect(store.get('reviewPanelView')).toBe('tree'); expect(store.get('reviewPanelViewLastUsed')).toBe('tree'); expect(store.get('defaultDiffType')).toBe('since-base'); expect(needsReviewSetup()).toBe(false); + + expect(initializeReviewSetup(store)).toBe(false); + const nextSession = makeStore(); + nextSession.init(); + expect(initializeReviewSetup(nextSession)).toBe(false); + expect(nextSession.get('reviewPanelView')).toBe('tree'); + expect(nextSession.get('reviewPanelViewLastUsed')).toBe('tree'); }); test('an unseen reviewer inherits an existing classic diff default', () => { @@ -48,6 +61,8 @@ describe('initializeReviewSetup', () => { 'plannotator-default-diff-type': 'uncommitted', }); const store = makeStore(); + store.get('reviewPanelView'); + store.init(); expect(initializeReviewSetup(store)).toBe(true); expect(store.get('reviewPanelView')).toBe('tree'); @@ -55,15 +70,22 @@ describe('initializeReviewSetup', () => { expect(store.get('defaultDiffType')).toBe('uncommitted'); }); - test('an unseen reviewer inherits a local-vs-remote default', () => { + test('first-run setup preserves the server diff default without writing it back', async () => { installMemoryBackend({ - 'plannotator-default-diff-type': 'local-vs-remote', + 'plannotator-default-diff-type': 'uncommitted', }); const store = makeStore(); + const synced: Record[] = []; + store.setServerSync(payload => { synced.push(payload); }); + store.get('reviewPanelView'); + store.init({ diffOptions: { defaultDiffType: 'local-vs-remote' } }); expect(initializeReviewSetup(store)).toBe(true); expect(store.get('reviewPanelView')).toBe('tree'); expect(store.get('defaultDiffType')).toBe('local-vs-remote'); + + await new Promise(resolve => setTimeout(resolve, 350)); + expect(synced).toEqual([]); }); test('an explicit persisted view survives a session that never tripped the seen gate', () => { @@ -71,13 +93,19 @@ describe('initializeReviewSetup', () => { // initializer, so a reviewer can persist a view from Settings while // "seen" stays unset. The next plain git session must not seed over it. installMemoryBackend({ - 'plannotator-review-panel-view': 'sections', - 'plannotator-default-diff-type': 'since-base', + 'plannotator-review-panel-view-last-used': 'tree', }); + const settingsStore = makeStore(); + settingsStore.get('displayName'); + // Even choosing the built-in default is an explicit choice. + setReviewPanelView('sections', undefined, settingsStore); + const store = makeStore(); + store.init(); expect(initializeReviewSetup(store)).toBe(false); expect(store.get('reviewPanelView')).toBe('sections'); + expect(store.get('reviewPanelViewLastUsed')).toBe('sections'); expect(store.get('defaultDiffType')).toBe('since-base'); // The one-time setup is consumed, so this cannot be re-evaluated later. expect(needsReviewSetup()).toBe(false); @@ -89,6 +117,7 @@ describe('initializeReviewSetup', () => { 'plannotator-review-panel-view-last-used': 'sections', }); const store = makeStore(); + store.init(); expect(initializeReviewSetup(store)).toBe(false); expect(store.get('reviewPanelView')).toBe('tree'); @@ -106,6 +135,8 @@ describe('initializeReviewSetup', () => { 'plannotator-default-diff-type': 'since-base', }); const store = makeStore(); + store.get('reviewPanelView'); + store.init(); expect(initializeReviewSetup(store)).toBe(false); expect(store.get('reviewPanelView')).toBe('sections'); diff --git a/packages/review-editor/utils/reviewSetup.ts b/packages/review-editor/utils/reviewSetup.ts index 8a18e34cd..fe1c5dab9 100644 --- a/packages/review-editor/utils/reviewSetup.ts +++ b/packages/review-editor/utils/reviewSetup.ts @@ -83,6 +83,8 @@ export function initializeReviewSetup(store: typeof configStore = configStore): // let Settings persist a panel view, so a reviewer can hold an explicit // choice while "seen" stays unset. Seeding Tree there would overwrite it. // A persisted view IS the decision: consume the one-time setup and leave it. + // The registry keeps this default in memory only, so a settings read cannot + // manufacture that evidence before this initializer runs. if (getPersistedReviewPanelView() !== undefined) { markReviewSetupSeen(); return false; diff --git a/packages/ui/config/configStore.lazyInit.seam.test.ts b/packages/ui/config/configStore.lazyInit.seam.test.ts index 5b5aa90ed..726beb74f 100644 --- a/packages/ui/config/configStore.lazyInit.seam.test.ts +++ b/packages/ui/config/configStore.lazyInit.seam.test.ts @@ -70,4 +70,22 @@ describe('configStore lazy resolution', () => { expect(second).toBe(first); expect(reads.length).toBe(readsAfterLoad); }); + + test('default seeding leaves review setup undecided on load and backend hydration', () => { + setStorageBackend(hostBackend); + const store = new ConfigStoreForTest(); + const identity = store.get('displayName'); + + expect(store.get('reviewPanelView')).toBe('sections'); + expect(stored.has('plannotator-review-panel-view')).toBe(false); + + // A host may install an empty backend after settings were already read. + stored.clear(); + store.loadFromBackend(); + + // Generated defaults still persist, but a panel default is not a choice. + expect(stored.get('plannotator-identity')).toBe(identity); + expect(stored.has('plannotator-review-panel-view')).toBe(false); + expect(new ConfigStoreForTest().get('displayName')).toBe(identity); + }); }); diff --git a/packages/ui/config/configStore.ts b/packages/ui/config/configStore.ts index 958ce2cf6..aa1b55b27 100644 --- a/packages/ui/config/configStore.ts +++ b/packages/ui/config/configStore.ts @@ -89,7 +89,7 @@ class ConfigStore { * first use — deliberately not in the constructor. The singleton is created * at module import, which for a host app is before configurePlannotatorUI() * can install its StorageBackend; resolving eagerly there would write every - * missing default (including a generated identity) as cookies onto the host's + * eligible missing default (including a generated identity) onto the host's * origin. Deferring to first use means a host that configures at startup gets * its own backend for the initial resolution too — no cookies are ever * written on a configured host. Plannotator is unchanged: same resolution, @@ -105,8 +105,8 @@ class ConfigStore { : def.defaultValue; const resolved = fromCookie ?? defaultVal; this.values.set(name, resolved); - // Persist generated defaults to cookie so the value is stable across calls - if (fromCookie === undefined) { + // Persist defaults for stability unless absence must remain distinguishable. + if (fromCookie === undefined && !('persistDefault' in def && def.persistDefault === false)) { def.toCookie(resolved as never); } } @@ -127,9 +127,9 @@ class ConfigStore { const fromBackend = def.fromCookie(); if (fromBackend !== undefined) { this.values.set(name, fromBackend); - } else { - // Seed the host backend with the resolved default. This matters when - // the store was already resolved BEFORE the host installed its + } else if (!('persistDefault' in def && def.persistDefault === false)) { + // Seed the host backend with the resolved default unless it opts out. + // This matters when the store was resolved BEFORE the host installed its // StorageBackend (e.g. something read a setting pre-configure): those // default-seeding writes went to the earlier backend, not this one. // Without this, a fresh host store is never populated, so generated diff --git a/packages/ui/config/settings.ts b/packages/ui/config/settings.ts index a748ad912..348a8274a 100644 --- a/packages/ui/config/settings.ts +++ b/packages/ui/config/settings.ts @@ -95,6 +95,8 @@ function isDiffLineBgIntensity(v: unknown): v is DiffLineBgIntensity { export interface SettingDef { defaultValue: T | (() => T); + /** Persist missing defaults automatically unless false; explicit writes are unaffected. */ + persistDefault?: boolean; fromCookie: () => T | undefined; toCookie: (value: T) => void; /** If set, this setting syncs to server via POST /api/config */ @@ -223,6 +225,8 @@ export const SETTINGS = { // previously-persisted 'commits' cookie is treated as unset. reviewPanelView: { defaultValue: 'sections' as 'sections' | 'tree', + // An absent cookie means no user choice yet, so first-run setup can choose its default. + persistDefault: false, fromCookie: () => { const v = storage.getItem('plannotator-review-panel-view'); return v === 'tree' || v === 'sections' ? v : undefined; From b274fa773149aed7d25c624322bf330c0c8fb265 Mon Sep 17 00:00:00 2001 From: FND Date: Sat, 5 Sep 2026 14:42:14 +0200 Subject: [PATCH 2/2] chore(viewer): sync manifest after settings initialization fix --- packages/core/guide-viewer-manifest.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/core/guide-viewer-manifest.ts b/packages/core/guide-viewer-manifest.ts index 7cd073456..7b83cddbd 100644 --- a/packages/core/guide-viewer-manifest.ts +++ b/packages/core/guide-viewer-manifest.ts @@ -5,9 +5,9 @@ import type { GuideViewerAssets } from "./guide-format"; export const GUIDE_VIEWER_MANIFEST: Omit = { - js: "viewer.C8JgNbZu.js", + js: "viewer.-z4hZ-DF.js", css: "viewer.KIp-iPxY.css", - jsIntegrity: "sha384-dqRkZ2N5WNwLIuwa+1kdrvswkOVHpkBxrrJ5mTU4YrUWzO/8fRS5MRTlDGV9t7eB", + jsIntegrity: "sha384-0m4lvEvRi8w3vafYuzG3N918AAH78sDPytVzerfzpOBxU63u3VrdyhHpBOVpDT4u", cssIntegrity: "sha384-Ty9hGpag8KIAGMDPg7ImsB4L5RabqvonNVjrj/CnYqSJDqIgPRBwjEMoK9/EvgXm", langs: { "astro": "chunks/astro.BykyiR6i.js",