fix: 回应 Codex 审查的 4 条意见(2×P1 + 2×P2)
- P1 state.ts reduceFor:恢复共享 uid 计数器到调用前水平(目标 session 的 max 可能低于激活 session 已用 id,否则下次 mint 撞 id 损坏布局); 回归测试实证:无修复时 split 产生重复 pane id - P1 service.ts 生命周期分类:dedupe 聚焦判定改为基于输入态的存在性 (dedupeKey 匹配或 id 匹配都算聚焦)——按 path 去重 + 新 id 打开时 不再对从未插入的幽灵 tab 触发 onOpen(资源泄漏源) - P2 SideCardSection:乐观 prefs ref(optimisticRef)驱动嵌套 map 合并 (tabsEnabled/viewersEnabled/pluginSettings),同 tick 多次写入不再 互相丢弃;mergePluginSetting 抽纯函数 + 单测 - P2 state.ts schedulePersist:每 session 独立防抖定时器(persistTimers Map),定向打开不再取消其他 session 的待写;单测验证双写都落盘
This commit is contained in:
@@ -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<string, Record<string, unknown>>,
|
||||
descriptorId: string,
|
||||
key: string,
|
||||
value: unknown,
|
||||
): Record<string, Record<string, unknown>> {
|
||||
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<string | null>(null)
|
||||
// Which feature's secondary settings popup is open (null = closed).
|
||||
const [settingsFor, setSettingsFor] = useState<TabDescriptor | FileViewerDescriptor | null>(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<string, unknown>): 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). */
|
||||
|
||||
+16
-5
@@ -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)
|
||||
|
||||
+21
-3
@@ -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<string, number>()
|
||||
/** 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 {
|
||||
|
||||
@@ -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'])
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, Record<string, unknown>> = {}
|
||||
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' })
|
||||
})
|
||||
})
|
||||
|
||||
@@ -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<string, unknown>
|
||||
let seq = 0
|
||||
const timers = new Map<number, () => 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)', () => {
|
||||
|
||||
Reference in New Issue
Block a user