diff --git a/src/client/SideCardSection.tsx b/src/client/SideCardSection.tsx index de9279b..9481cf6 100644 --- a/src/client/SideCardSection.tsx +++ b/src/client/SideCardSection.tsx @@ -128,6 +128,24 @@ function featureNameOf(feature: TabDescriptor | FileViewerDescriptor): string { return textOf('title' in feature ? feature.title : undefined) || feature.id } +/** + * Merge one plugin-owned setting into a pluginSettings map (pure, v0.12.0+). + * Sequential merges are additive: each call spreads the map it was GIVEN, + * so building from the latest optimistic map keeps earlier keys intact + * (two same-tick writes must not drop each other). + */ +export function mergePluginSetting( + pluginSettings: Record>, + descriptorId: string, + key: string, + value: unknown, +): Record> { + return { + ...pluginSettings, + [descriptorId]: { ...(pluginSettings[descriptorId] ?? {}), [key]: value }, + } +} + /** * Render a custom settings panel (`settings.render`) with error containment: * a throwing panel shows an inline error line instead of breaking the whole @@ -355,6 +373,14 @@ export function SideCardSection({ store, service }: SideCardSectionProps) { const [error, setError] = useState(null) // Which feature's secondary settings popup is open (null = closed). const [settingsFor, setSettingsFor] = useState(null) + // The LATEST optimistic prefs, kept in sync with the state. Nested-map + // merges (tabsEnabled / viewersEnabled / pluginSettings) MUST build from + // this ref, not from the render-time `prefs`: two same-tick writes (e.g. + // a settings panel updating several plugin keys at once) would otherwise + // both spread the stale map and the later patch would drop the earlier + // key even though the commits are serialized. + const optimisticRef = useRef(prefs) + useEffect(() => { optimisticRef.current = prefs }, [prefs]) // The declarative inventory: the registered tab types and file viewers. // Local state + service.subscribe (registry changes are rare — plugin @@ -425,8 +451,10 @@ export function SideCardSection({ store, service }: SideCardSectionProps) { /** Optimistically apply one pref patch, then commit (revert on failure). */ const applyPref = (patch: Record): void => { - const previous = prefs - setPrefs({ ...previous, ...patch } as SidebarPrefs) + const previous = optimisticRef.current + const next = { ...previous, ...patch } as SidebarPrefs + optimisticRef.current = next + setPrefs(next) setError(null) void commit(patch).then(outcome => applyOutcome(previous, outcome)) } @@ -437,12 +465,12 @@ export function SideCardSection({ store, service }: SideCardSectionProps) { /** Flip one per-tab enable switch (merge into the tabsEnabled map). */ const onToggleTab = (id: string, next: boolean): void => { - applyPref({ tabsEnabled: { ...prefs.tabsEnabled, [id]: next } }) + applyPref({ tabsEnabled: { ...optimisticRef.current.tabsEnabled, [id]: next } }) } /** Flip one per-viewer enable switch (merge into the viewersEnabled map). */ const onToggleViewer = (id: string, next: boolean): void => { - applyPref({ viewersEnabled: { ...prefs.viewersEnabled, [id]: next } }) + applyPref({ viewersEnabled: { ...optimisticRef.current.viewersEnabled, [id]: next } }) } /** Flip one declaratively-declared toggle (a SidebarPrefs boolean field). */ @@ -474,12 +502,7 @@ export function SideCardSection({ store, service }: SideCardSectionProps) { /** Persist one plugin-owned setting of one descriptor (merged into the pluginSettings blob). */ const applyPluginSetting = (descriptorId: string, key: string, value: unknown): void => { - applyPref({ - pluginSettings: { - ...prefs.pluginSettings, - [descriptorId]: { ...(prefs.pluginSettings[descriptorId] ?? {}), [key]: value }, - }, - }) + applyPref({ pluginSettings: mergePluginSetting(optimisticRef.current.pluginSettings, descriptorId, key, value) }) } /** Flip one plugin-owned switch row (same row shape, plugin-scoped key). */ diff --git a/src/client/service.ts b/src/client/service.ts index 3329d07..2ec9c8e 100644 --- a/src/client/service.ts +++ b/src/client/service.ts @@ -543,13 +543,24 @@ export function createBetterSidebarService(store: SidebarStore): BetterSidebarSe landed = next } // Lifecycle capture (before the auto-expand block, which early-returns). - if (!tabOpenIn(state, tab.id)) { + // Classify the landing against the INPUT state: a FOCUS fires + // onActivate with the tab that is active NOW; a real creation fires + // onOpen with the minted tab. Both the dedupeKey match AND the id + // match count as a focus — a descriptor deduping by key (e.g. editor + // by path) can focus an existing tab for a NEW requested id, and + // classifying that as a creation would fire onOpen with a phantom + // tab that never closes. + const dedupeKey = descriptor.dedupeKey ?? (descriptor.single === true ? () => descriptor.id : undefined) + const key = dedupeKey?.(tab) + const inputTabs = allLeaves(state.splits).concat(allLeaves(state.bottomSplits)).flatMap(leaf => leaf.tabs) + const existedByKey = key !== undefined + && inputTabs.some(candidate => candidate.type === tab.type && dedupeKey!(candidate) === key) + const existedById = tabOpenIn(state, tab.id) + if (!existedByKey && !existedById) { created = tab } else { - // A focus happened (dedupeKey or the id safety net): resolve the - // tab that is actually active now and report THAT to onActivate. - const dedupeKey = descriptor.dedupeKey ?? (descriptor.single === true ? () => descriptor.id : undefined) - const key = dedupeKey?.(tab) + // A focus happened: resolve the tab that is actually active now and + // report THAT to onActivate (never the caller's un-inserted seed). const candidates = allLeaves(landed.splits).concat(allLeaves(landed.bottomSplits)).flatMap(leaf => leaf.tabs) activated = key !== undefined ? candidates.find(candidate => candidate.type === tab.type && dedupeKey!(candidate) === key) diff --git a/src/client/state.ts b/src/client/state.ts index 55c2b60..6908ddd 100644 --- a/src/client/state.ts +++ b/src/client/state.ts @@ -938,7 +938,9 @@ export class SidebarStore { prefs: { ...SIDEBAR_PREFS_DEFAULTS }, } private readonly listeners = new Set<() => void>() - private persistTimer: number | undefined + /** Per-session persist debounce timers (v0.12.0+: one per session, so a + * targeted open never cancels another session's pending write). */ + private readonly persistTimers = new Map() /** User-facing side card prefs seeding brand-new session states (defaults until the settings RPC resolves). */ private prefs: SidebarPrefs = { ...SIDEBAR_PREFS_DEFAULTS } @@ -1037,6 +1039,13 @@ export class SidebarStore { * session's layout and is visible whenever the user switches to it. */ reduceFor(sessionId: string, reducer: (state: SidebarState) => SidebarState): void { + // The uid counter is SHARED across sessions, and the ACTIVE session's + // safety requires it to never drop below the ids IT minted. Seeding it + // from the target's max may LOWER it (a cached target older than the + // active session): restoring the pre-call level afterwards keeps the + // active session's next mint collision-free — ids minted for the target + // only need to exceed the target's own max, which the seed guaranteed. + const counterBefore = nextIdCounter let state = this.bySession.get(sessionId) if (state === undefined) { state = loadState(sessionId, this.prefs) @@ -1048,18 +1057,27 @@ export class SidebarStore { } const next = reducer(state) this.bySession.set(sessionId, next) + nextIdCounter = Math.max(nextIdCounter, counterBefore) this.schedulePersist(sessionId, next) } private schedulePersist(sessionId: string, state: SidebarState): void { - window.clearTimeout(this.persistTimer) - this.persistTimer = window.setTimeout(() => { + // Per-session debounce timers: one session's pending write must never + // cancel another's (targeted opens schedule writes for INACTIVE + // sessions while the active session may already have one pending — + // a shared timer would drop the earlier write and the reload would + // lose that session's layout). + const existing = this.persistTimers.get(sessionId) + if (existing !== undefined) window.clearTimeout(existing) + const timer = window.setTimeout(() => { + this.persistTimers.delete(sessionId) try { localStorage.setItem(`${STORAGE_PREFIX}:${sessionId}`, JSON.stringify(state)) } catch { // Storage full or unavailable: layout memory is best-effort. } }, 200) + this.persistTimers.set(sessionId, timer) } private notify(): void { diff --git a/tests/service.spec.ts b/tests/service.spec.ts index a040ed6..88b213d 100644 --- a/tests/service.spec.ts +++ b/tests/service.spec.ts @@ -849,3 +849,51 @@ describe('tab meta (v0.12.0)', () => { expect(tabs[0]?.meta).toBeUndefined() }) }) + +describe('lifecycle classification vs dedupe (codex review fixes)', () => { + it('a key-dedupe focus with a NEW id fires onActivate with the REAL tab (no phantom onOpen)', () => { + const store = createSidebarStore() + const service = createBetterSidebarService(store) + const events: Array<{ kind: string; tabId?: string }> = [] + service.registerTab({ + id: 'doc', + title: 'Doc', + // Dedupe by PATH like the editor builtin — the focused tab's id + // differs from the newly requested id. + dedupeKey: (tab) => tab.path ?? '', + onOpen: (tab) => { events.push({ kind: 'open', tabId: tab.id }) }, + onActivate: (tab) => { events.push({ kind: 'activate', tabId: tab.id }) }, + component: () => null, + }) + store.setSession('s1') + service.openTab({ type: 'doc', title: 'Doc', id: 'doc:1', path: '/a.md' }) + expect(events).toEqual([{ kind: 'open', tabId: 'doc:1' }]) + // Same path, NEW id: the existing tab is focused — onActivate must + // carry the EXISTING tab, and onOpen must NOT fire with doc:2. + service.openTab({ type: 'doc', title: 'Doc', id: 'doc:2', path: '/a.md' }) + expect(events).toEqual([ + { kind: 'open', tabId: 'doc:1' }, + { kind: 'activate', tabId: 'doc:1' }, + ]) + const tabs = allLeaves(store.getSnapshot().state!.splits).flatMap(l => l.tabs).filter(t => t.type === 'doc') + expect(tabs.map(t => t.id)).toEqual(['doc:1']) + }) + + it('an id safety-net focus (same id) fires onActivate, not onOpen', () => { + const store = createSidebarStore() + const service = createBetterSidebarService(store) + const events: string[] = [] + service.registerTab({ + id: 'multi', + title: 'Multi', + onOpen: () => { events.push('open') }, + onActivate: () => { events.push('activate') }, + component: () => null, + }) + store.setSession('s1') + service.openTab({ type: 'multi', title: 'Multi', id: 'm:1' }) + expect(events).toEqual(['open']) + service.openTab({ type: 'multi', title: 'Multi', id: 'm:1' }) + expect(events).toEqual(['open', 'activate']) + }) +}) diff --git a/tests/side-card-section.spec.tsx b/tests/side-card-section.spec.tsx index 40f435a..57efc39 100644 --- a/tests/side-card-section.spec.tsx +++ b/tests/side-card-section.spec.tsx @@ -20,7 +20,7 @@ import { createElement } from 'react' import { createSidebarStore, type SidebarStore } from '../src/client/state.ts' import { createBetterSidebarService, type BetterSidebarService } from '../src/client/service.ts' import { SIDEBAR_PREFS_DEFAULTS } from '../src/prefs-shared.ts' -import { FeatureSettingsRows, SideCardSection, type SideCardSectionProps } from '../src/client/SideCardSection.tsx' +import { FeatureSettingsRows, mergePluginSetting, SideCardSection, type SideCardSectionProps } from '../src/client/SideCardSection.tsx' /** One tab + one viewer + the subagent-style nested toggle under a tab. */ function mount(): { store: SidebarStore; service: BetterSidebarService } { @@ -216,3 +216,23 @@ describe('FeatureSettingsRows (the secondary settings popup body)', () => { expect(html).not.toContain('type="checkbox"') }) }) + +describe('mergePluginSetting (v0.12.0, codex review fix)', () => { + it('sequential merges are additive — a later write never drops an earlier key', () => { + // Simulates two same-tick updatePluginSetting calls: each merge spreads + // the map it was GIVEN, so building from the latest optimistic map + // preserves both keys (the pre-fix code spread the stale render-time + // prefs twice and the second write dropped the first key). + let map: Record> = {} + map = mergePluginSetting(map, 'my-plugin:db', 'pageSize', 25) + map = mergePluginSetting(map, 'my-plugin:db', 'theme', 'dark') + expect(map['my-plugin:db']).toEqual({ pageSize: 25, theme: 'dark' }) + // A second descriptor's blob stays independent. + map = mergePluginSetting(map, 'other:view', 'refresh', true) + expect(map['my-plugin:db']).toEqual({ pageSize: 25, theme: 'dark' }) + expect(map['other:view']).toEqual({ refresh: true }) + // Overwriting one key keeps the sibling keys. + map = mergePluginSetting(map, 'my-plugin:db', 'pageSize', 50) + expect(map['my-plugin:db']).toEqual({ pageSize: 50, theme: 'dark' }) + }) +}) diff --git a/tests/unit.spec.ts b/tests/unit.spec.ts index 6cd1e04..747bcd9 100644 --- a/tests/unit.spec.ts +++ b/tests/unit.spec.ts @@ -1690,6 +1690,57 @@ describe('store.reduceFor (targeted opens, v0.12.0)', () => { expect(store.getSnapshot().state?.panelOpen).toBe(false) expect(store.getSnapshot().state?.splits).toBeDefined() }) + + it('reduceFor never lowers the shared uid counter below the active session needs (no pane-id collision)', () => { + const store = createSidebarStore() + // Session 'b' is cached FIRST with a LOW id range (pane:1 / tab:2). + store.setSession('b') + // Session 'a' then loads and its operations raise the shared counter + // well above b's max (default pane, plus fresh pane ids from splits). + store.setSession('a') + store.reduce(s => splitPane(s, 'row')) + const before = allLeaves(store.getSnapshot().state!.splits).map(leaf => leaf.id).sort() + // A targeted reduce into the OLD, low-id session must not lower the + // counter: the next split in the ACTIVE session would otherwise mint + // an id that already exists (mapLeaf visits both leaves → corruption). + store.reduceFor('b', (state) => state) + store.reduce(s => splitPane(s, 'row')) + const after = allLeaves(store.getSnapshot().state!.splits).map(leaf => leaf.id) + expect(new Set(after).size).toBe(after.length) + // The active session's pre-existing pane ids all survived untouched. + for (const id of before) expect(after).toContain(id) + }) + + it('persists each session independently (per-session debounce timers)', () => { + const g = globalThis as Record + let seq = 0 + const timers = new Map void>() + const writes: string[] = [] + g.window = { + clearTimeout: (id: number) => { timers.delete(id) }, + setTimeout: (fn: () => void) => { const id = ++seq; timers.set(id, fn); return id }, + innerWidth: 1024, + innerHeight: 800, + } + g.localStorage = { + getItem: () => null, + setItem: (key: string) => { writes.push(key) }, + } + try { + const store = createSidebarStore() + store.setSession('a') + store.reduce(s => ({ ...s, expanded: ['/a'] })) // schedules persist(a) + store.reduceFor('b', s => ({ ...s, expanded: ['/b'] })) // schedules persist(b) + // A shared timer would have cancelled persist(a) — with per-session + // timers BOTH writes are pending and both land when they fire. + expect(timers.size).toBe(2) + for (const [, fn] of [...timers]) fn() + expect(writes).toEqual(['dsh-sidebar:v1:a', 'dsh-sidebar:v1:b']) + } finally { + delete g.window + delete g.localStorage + } + }) }) describe('tab meta persistence (v0.12.0)', () => {