diff --git a/src/renderer/src/app-shell/use-persisted-ui-writer.ts b/src/renderer/src/app-shell/use-persisted-ui-writer.ts index 7d21726619e..857b55fc8a4 100644 --- a/src/renderer/src/app-shell/use-persisted-ui-writer.ts +++ b/src/renderer/src/app-shell/use-persisted-ui-writer.ts @@ -1,4 +1,4 @@ -import { useEffect } from 'react' +import { useEffect, useMemo } from 'react' import { useShallow } from 'zustand/react/shallow' import { useAppStore } from '../store' import { @@ -19,65 +19,111 @@ import { * next change (no automatic retry loop) — which is why this sends through * setWithAck: the web preload's plain set swallows transport failures. */ -function sendPersistedUIWrite(changed: Partial): void { - const fields = Object.keys(changed) as (keyof PersistedUIWriteBaseline)[] - const state = useAppStore.getState() - const sentAtGeneration = state.persistedUIWriteBaselineGeneration - state.notePersistedUIWriteStarted(fields) - let request: Promise - try { - // setWithAck rejects when the host did not apply the patch (web's plain set - // swallows transport failures); older preloads without it fall back to set. - const send = window.api.ui.setWithAck ?? window.api.ui.set - request = send(persistedUIWriteFieldsToWireUpdate(changed)) - } catch { - // A synchronous throw (e.g. a non-cloneable value) must still settle the - // in-flight marker, or the field stays pinned against hydration forever. - useAppStore.getState().notePersistedUIWriteSettled(fields, null) - scheduleTrailingPersistedUIFlush() - return - } - // Two-arg then: the rejection handler must not catch throws from the ack - // handler, which would double-settle these fields and leak the trailing ones. - request.then( - () => { - useAppStore.getState().notePersistedUIWriteSettled(fields, changed, { sentAtGeneration }) - scheduleTrailingPersistedUIFlush() - }, - () => { - useAppStore.getState().notePersistedUIWriteSettled(fields, null) - // A trailing pass skipped because THIS write was in flight must still - // happen — a rejection schedules nothing on its own, and a pending - // flip-back would otherwise be stranded until the next edit. - scheduleTrailingPersistedUIFlush() - } - ) +type PersistedUIWriteController = { + activate: () => void + send: (changed: Partial) => void + scheduleTrailing: () => void + dispose: () => void } -/** - * Debounced (never inline at ack rate — one in-flight write must not turn a - * drag into one ui.set per IPC round trip) re-diff of the mirror against the - * settled baseline. Skips while any write is still in flight: that write's - * own ack schedules the next pass, so overlapping timers can't double-send. - * Termination invariant: each acked write folds exactly the values it diffed - * (or a hydration advances the baseline), so the trailing diff shrinks to - * empty; without the fold this would loop. - */ -function scheduleTrailingPersistedUIFlush(): void { - window.setTimeout(() => { +function createPersistedUIWriteController(): PersistedUIWriteController { + let disposed = false + let trailingTimer: number | null = null + + const scheduleTrailing = (): void => { + if (disposed) { + return + } + if (trailingTimer !== null) { + window.clearTimeout(trailingTimer) + } + trailingTimer = window.setTimeout(() => { + trailingTimer = null + if (disposed) { + return + } + const state = useAppStore.getState() + if (Object.keys(state.persistedUIWriteInFlightCounts).length > 0) { + return + } + const baseline = state.persistedUIWriteBaseline + if (!baseline) { + return + } + const trailing = diffPersistedUIWriteFields(capturePersistedUIWriteBaseline(state), baseline) + if (Object.keys(trailing).length > 0) { + controller.send(trailing) + } + }, 150) + } + + const send = (changed: Partial): void => { + if (disposed) { + return + } + const fields = Object.keys(changed) as (keyof PersistedUIWriteBaseline)[] const state = useAppStore.getState() - if (Object.keys(state.persistedUIWriteInFlightCounts).length > 0) { + const sentAtGeneration = state.persistedUIWriteBaselineGeneration + state.notePersistedUIWriteStarted(fields) + let request: Promise + try { + // setWithAck rejects when the host did not apply the patch (web's plain set + // swallows transport failures); older preloads without it fall back to set. + const send = window.api.ui.setWithAck ?? window.api.ui.set + request = send(persistedUIWriteFieldsToWireUpdate(changed)) + } catch { + // A synchronous throw (e.g. a non-cloneable value) must still settle the + // in-flight marker, or the field stays pinned against hydration forever. + useAppStore.getState().notePersistedUIWriteSettled(fields, null) return } - const baseline = state.persistedUIWriteBaseline - if (!baseline) { - return + // Two-arg then: the rejection handler must not catch throws from the ack + // handler, which would double-settle these fields and leak the trailing ones. + request.then( + () => { + useAppStore.getState().notePersistedUIWriteSettled(fields, changed, { sentAtGeneration }) + scheduleTrailing() + }, + () => { + useAppStore.getState().notePersistedUIWriteSettled(fields, null) + // Preserve a pending edit made during this round trip, but don't retry the + // rejected patch itself: a terminal transport failure must not loop. + const state = useAppStore.getState() + const baseline = state.persistedUIWriteBaseline + const dirty = baseline + ? diffPersistedUIWriteFields(capturePersistedUIWriteBaseline(state), baseline) + : {} + const current = capturePersistedUIWriteBaseline(state) + const changedDuringFlight = fields.some( + (field) => !Object.is(current[field], changed[field]) + ) + const shouldRetry = + changedDuringFlight || + Object.keys(dirty).some( + (field) => !fields.includes(field as keyof PersistedUIWriteBaseline) + ) + if (shouldRetry) { + scheduleTrailing() + } + } + ) + } + + const controller: PersistedUIWriteController = { + activate: () => { + disposed = false + }, + send, + scheduleTrailing, + dispose: () => { + disposed = true + if (trailingTimer !== null) { + window.clearTimeout(trailingTimer) + trailingTimer = null + } } - const trailing = diffPersistedUIWriteFields(capturePersistedUIWriteBaseline(state), baseline) - if (Object.keys(trailing).length > 0) { - sendPersistedUIWrite(trailing) - } - }, 150) + } + return controller } /** @@ -93,6 +139,7 @@ function scheduleTrailingPersistedUIFlush(): void { * same 150ms save (#8265), so every top-level view switch scheduled a full durable-state write. */ export function usePersistedUIWriter(): void { + const controller = useMemo(() => createPersistedUIWriteController(), []) const persistedUIReady = useAppStore((s) => s.persistedUIReady) const activeView = useAppStore((s) => s.activeView) const ui = useAppStore( @@ -123,6 +170,10 @@ export function usePersistedUIWriter(): void { acknowledgedAgentsByPaneKey: s.acknowledgedAgentsByPaneKey })) ) + useEffect(() => { + controller.activate() + return () => controller.dispose() + }, [controller]) useEffect(() => { // The baseline holds the values this client last saw persisted (newest // hydration from main, overlaid with this client's flushed writes); fields @@ -150,11 +201,11 @@ export function usePersistedUIWriter(): void { if (Object.keys(changed).length === 0) { return } - sendPersistedUIWrite(changed) + controller.send(changed) }, 150) return () => window.clearTimeout(timer) - }, [persistedUIReady, ui]) + }, [controller, persistedUIReady, ui]) // Why (#9002): activeView has its own tiny profile preference, so it can track // every switch without scheduling the multi-MB durable-state writer. diff --git a/src/renderer/src/app-shell/workspace-view-cross-client-sync.test.tsx b/src/renderer/src/app-shell/workspace-view-cross-client-sync.test.tsx index 8049890b968..314977a17aa 100644 --- a/src/renderer/src/app-shell/workspace-view-cross-client-sync.test.tsx +++ b/src/renderer/src/app-shell/workspace-view-cross-client-sync.test.tsx @@ -31,6 +31,10 @@ import { } from '../../../main/persistence/applying-settings/ui-state-update' import type { AppState } from '../store/types' import { createUIStore } from '../store/slices/ui-slice-test-harness' +import { + capturePersistedUIWriteBaseline, + diffPersistedUIWriteFields +} from '../store/slices/persisted-ui-write-baseline' import { usePersistedUIWriter } from './use-persisted-ui-writer' const storeRef = vi.hoisted(() => ({ @@ -197,8 +201,10 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () = let pendingBroadcasts: PersistedUIState[] let holdAcks: boolean let rejectSets: boolean + let rejectNextSet: boolean let setCallCount: number let pendingAcks: (() => void)[] + let pendingRejects: ((reason?: unknown) => void)[] async function resolveAcks() { await act(async () => { @@ -249,8 +255,10 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () = storeRef.current = store as unknown as typeof storeRef.current holdAcks = false rejectSets = false + rejectNextSet = false setCallCount = 0 pendingAcks = [] + pendingRejects = [] ;(window as unknown as { api: unknown }).api = { ui: { set: (updates: Partial) => { @@ -259,6 +267,10 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () = if (rejectSets) { return Promise.reject(new Error('transport failure')) } + if (rejectNextSet) { + rejectNextSet = false + return new Promise((_, reject) => pendingRejects.push(reject)) + } // Like the real IPC: main applies the update before the renderer's // promise resolves; holdAcks models the in-flight round-trip window. authority.set(updates) @@ -516,7 +528,132 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () = expect(authority.get().hideCliCreatedWorkspaces).toBe(true) }) - it('a synchronously throwing ui.set still settles the marker and reschedules', async () => { + it('a terminal rejection leaves one dirty field without an automatic retry loop', async () => { + // A rejected transport write is terminal for this attempt. The mirror must + // stay dirty for an explicit later edit, but the rejection itself must not + // keep scheduling 150ms trailing writes forever. + rejectSets = true + setCallCount = 0 + act(() => { + store.getState().setHideDefaultBranchWorkspace(true) + }) + + vi.advanceTimersByTime(150) + // Let the controlled rejection settle and (on the buggy writer) arm its + // next trailing timer, without allowing that timer to fire in this tick. + await Promise.resolve() + + // Independent signals: exactly one transport call and no trailing timer + // remain after its rejection settles; the failed field is still dirty. + expect(setCallCount).toBe(1) + expect(store.getState().persistedUIWriteInFlightCounts).toEqual({}) + const state = store.getState() + expect( + diffPersistedUIWriteFields( + capturePersistedUIWriteBaseline(state), + state.persistedUIWriteBaseline! + ) + ).toEqual({ hideDefaultBranchWorkspace: true }) + expect(vi.getTimerCount()).toBe(0) + + // Recovery is explicit: the next user edit arms one debounce and flushes + // both the old dirty field and the new edit once transport recovers. + rejectSets = false + act(() => { + store.getState().setHideCliCreatedWorkspaces(true) + }) + vi.advanceTimersByTime(150) + await Promise.resolve() + expect(setCallCount).toBe(2) + expect(authority.get().hideDefaultBranchWorkspace).toBe(true) + expect(authority.get().hideCliCreatedWorkspaces).toBe(true) + expect(store.getState().persistedUIWriteInFlightCounts).toEqual({}) + + // Unmount cleanup must cancel any delayed work and prevent a post-close + // write when the store changes later. + act(() => { + root.unmount() + }) + expect(vi.getTimerCount()).toBe(0) + store.getState().setHideDetachedHeadWorkspaces(true) + vi.advanceTimersByTime(300) + expect(setCallCount).toBe(2) + expect(vi.getTimerCount()).toBe(0) + }) + + it('a transient rejection still flushes a pending flip-back exactly once', async () => { + // A trailing pass that was already needed for an edit made while a write + // was in flight must survive that write's rejection. This is distinct from + // a terminal rejection with no newer edit (covered above). + holdAcks = true + rejectNextSet = true + setCallCount = 0 + act(() => { + store.getState().setHideDefaultBranchWorkspace(true) + }) + vi.advanceTimersByTime(150) + expect(setCallCount).toBe(1) + expect(pendingRejects).toHaveLength(1) + + // Flip the first field back and edit a second field while write #1 is in + // flight. The failed write must not be retried; only the pending edit is + // eligible for the one trailing flush. + act(() => { + store.getState().setHideDefaultBranchWorkspace(false) + store.getState().setHideCliCreatedWorkspaces(true) + }) + holdAcks = false + pendingRejects.splice(0).forEach((reject) => reject(new Error('transient transport failure'))) + expect(pendingRejects).toHaveLength(0) + await Promise.resolve() + vi.advanceTimersByTime(150) + await Promise.resolve() + + expect(setCallCount).toBe(2) + expect(authority.get().hideDefaultBranchWorkspace).toBe(false) + expect(authority.get().hideCliCreatedWorkspaces).toBe(true) + expect(store.getState().persistedUIWriteInFlightCounts).toEqual({}) + + // Once the pending edit is acknowledged, no rejection-induced timer may + // remain to send another copy. + vi.advanceTimersByTime(150) + await Promise.resolve() + expect(setCallCount).toBe(2) + expect(vi.getTimerCount()).toBe(0) + }) + + it('a rejected trailing write does not inherit automatic retry permission', async () => { + // A successful write's trailing pass is still a normal persistence attempt: + // if that pass is rejected with no newer edit, it must be terminal too. + holdAcks = true + setCallCount = 0 + act(() => { + store.getState().setHideDefaultBranchWorkspace(true) + }) + vi.advanceTimersByTime(150) + expect(setCallCount).toBe(1) + + act(() => { + store.getState().setHideCliCreatedWorkspaces(true) + }) + await resolveAcks() + holdAcks = false + rejectSets = true + + // The edit's debounce and the successful ack's trailing pass may overlap; + // either way only one rejected trailing attempt is allowed. + vi.advanceTimersByTime(150) + await Promise.resolve() + expect(setCallCount).toBe(2) + expect(store.getState().persistedUIWriteInFlightCounts).toEqual({}) + + vi.advanceTimersByTime(150) + await Promise.resolve() + expect(setCallCount).toBe(2) + expect(vi.getTimerCount()).toBe(0) + }) + + it('a synchronously throwing ui.set settles without a retry loop', async () => { const api = ( window as unknown as { api: { ui: { set: (u: Partial) => Promise } } } ).api.ui @@ -532,42 +669,13 @@ describe('workspace view preferences: cross-client persistence (STA-5781)', () = // A leaked marker would pin the field against hydration for the renderer's life. expect(store.getState().persistedUIWriteInFlightCounts).toEqual({}) - // The throw must also reschedule the trailing pass: once the transport - // recovers, the dirty field flushes without waiting for another edit. + // Recovery is explicit: a later edit flushes the still-dirty field. api.set = workingSet - await flushDesktopDebounce() - expect(authority.get().hideDefaultBranchWorkspace).toBe(true) - }) - - it('a rejection re-schedules the trailing pass it caused to be skipped', async () => { - // Round-3 verification: a trailing pass that bails because a write is in - // flight relies on that write's settle to reschedule — including rejection, - // or a pending flip-back is stranded until the next unrelated edit. - holdAcks = true - act(() => { - store.getState().setHideDefaultBranchWorkspace(true) - }) - await flushDesktopDebounce() - // Flip back while write #1 is in flight: only a trailing flush carries it. - act(() => { - store.getState().setHideDefaultBranchWorkspace(false) - }) - await resolveAcks() - - // Before the trailing pass fires, a different field's write goes out and - // is REJECTED while in flight when the trailing pass checks. - rejectSets = true act(() => { store.getState().setHideCliCreatedWorkspaces(true) }) await flushDesktopDebounce() - - rejectSets = false - holdAcks = false - await flushDesktopDebounce() - await flushDesktopDebounce() - expect(authority.get().hideDefaultBranchWorkspace).toBe(false) - expect(store.getState().hideDefaultBranchWorkspace).toBe(false) + expect(authority.get().hideDefaultBranchWorkspace).toBe(true) }) it('overlapping in-flight writes on one field decrement, not clear, the marker', () => {