diff --git a/src/main/index.ts b/src/main/index.ts index 11e222e9..2ea4ad9f 100644 --- a/src/main/index.ts +++ b/src/main/index.ts @@ -36,6 +36,7 @@ import type { ServerTransport } from '../shared/transport/transport' import { detectRuntime } from './paths' import { fixPathFromLoginShell } from './path-fix' import { ChatStartGate } from './chat-start-gate' +import { createSingleFlight } from './single-flight' import { knownClaudeConfigDirs, onShellEnvCaptured, @@ -1973,8 +1974,17 @@ function registerIpcHandlers(): void { } ) + // Identical concurrent asks share one `git status`. A client that re-sends + // the same sweep on every render (the 2.18 Cleanup modal did, once per + // streamed token) otherwise queues thousands of reads, and each spawn + // blocks the event loop long enough that every other request — tab + // switches included — waits minutes behind them. + // Keyed on priority too, so a bulk sweep never hands its queued answer + // to an interactive caller. + const dirtyFlight = createSingleFlight() transport.onRequest('worktree:isDirty', async (_ctx, path: string, opts?: { bulk?: boolean }) => { - const git = await isWorktreeDirty(path, opts?.bulk ? 'bulk' : 'interactive') + const priority = opts?.bulk ? 'bulk' : 'interactive' + const git = await dirtyFlight(`${priority}\0${path}`, () => isWorktreeDirty(path, priority)) const scratchpad = hasScratchpadNote(store.getSnapshot().state.scratchpad, path) return { git, scratchpad } }) diff --git a/src/main/single-flight.test.ts b/src/main/single-flight.test.ts new file mode 100644 index 00000000..1deb0f1c --- /dev/null +++ b/src/main/single-flight.test.ts @@ -0,0 +1,55 @@ +import { describe, it, expect, vi } from 'vitest' +import { createSingleFlight } from './single-flight' + +function deferred(): { promise: Promise; resolve: (v: T) => void; reject: (e: unknown) => void } { + let resolve!: (v: T) => void + let reject!: (e: unknown) => void + const promise = new Promise((res, rej) => { + resolve = res + reject = rej + }) + return { promise, resolve, reject } +} + +describe('createSingleFlight', () => { + it('shares one underlying call between concurrent callers with the same key', async () => { + const flight = createSingleFlight() + const d = deferred() + const fn = vi.fn(() => d.promise) + + const calls = Array.from({ length: 50 }, () => flight('a', fn)) + expect(fn).toHaveBeenCalledTimes(1) + + d.resolve(true) + expect(await Promise.all(calls)).toEqual(Array(50).fill(true)) + }) + + it('keeps different keys separate', () => { + const flight = createSingleFlight() + const fn = vi.fn(() => new Promise(() => {})) + void flight('a', fn) + void flight('b', fn) + expect(fn).toHaveBeenCalledTimes(2) + }) + + it('runs fresh after the shared call resolves', async () => { + const flight = createSingleFlight() + let n = 0 + const fn = vi.fn(async () => ++n) + expect(await flight('a', fn)).toBe(1) + expect(await flight('a', fn)).toBe(2) + expect(fn).toHaveBeenCalledTimes(2) + }) + + it('clears the entry on reject, so the next call retries', async () => { + const flight = createSingleFlight() + const failing = vi.fn(() => Promise.reject(new Error('boom'))) + const a = flight('a', failing) + const b = flight('a', failing) + await expect(a).rejects.toThrow('boom') + await expect(b).rejects.toThrow('boom') + expect(failing).toHaveBeenCalledTimes(1) + + expect(await flight('a', async () => 7)).toBe(7) + }) +}) diff --git a/src/main/single-flight.ts b/src/main/single-flight.ts new file mode 100644 index 00000000..1ce02471 --- /dev/null +++ b/src/main/single-flight.ts @@ -0,0 +1,14 @@ +/** Collapses concurrent calls with the same key onto one in-flight promise. + * The entry is dropped as soon as that promise settles, either way, so a + * call made afterwards always runs fresh — this dedupes a burst, it never + * caches an answer. */ +export function createSingleFlight(): (key: string, fn: () => Promise) => Promise { + const inflight = new Map>() + return (key, fn) => { + const pending = inflight.get(key) + if (pending) return pending + const started = fn().finally(() => inflight.delete(key)) + inflight.set(key, started) + return started + } +} diff --git a/src/renderer/App.tsx b/src/renderer/App.tsx index ab5de3e8..c98c897d 100644 --- a/src/renderer/App.tsx +++ b/src/renderer/App.tsx @@ -1962,6 +1962,7 @@ const setQuestStep = useCallback((next: QuestStep) => { prStatuses={prStatuses} mergedPaths={mergedPaths} lastActive={lastActive} + backendFor={backendFor} onBulkDelete={handleBulkDeleteWorktrees} /> diff --git a/src/renderer/components/Cleanup.tsx b/src/renderer/components/Cleanup.tsx index 72bcdcdf..325e0689 100644 --- a/src/renderer/components/Cleanup.tsx +++ b/src/renderer/components/Cleanup.tsx @@ -1,6 +1,6 @@ -import { useEffect, useMemo, useState } from 'react' +import { useEffect, useMemo, useRef, useState } from 'react' import { ArrowLeft, Trash2, AlertTriangle, GitPullRequest, CheckCircle2, Loader2, ChevronDown, ChevronRight, ExternalLink } from 'lucide-react' -import type { Worktree, PRStatus, ActivityLog, BranchCommit } from '../types' +import type { Worktree, PRStatus, ActivityLog, BranchCommit, ElectronAPI } from '../types' import { isPRMerged } from '../../shared/state/prs' import { useBackend } from '../backend' import { useAliases } from '../store' @@ -11,6 +11,10 @@ interface CleanupProps { prStatuses: Record mergedPaths: Record lastActive: Record + /** The backend hosting each worktree. The list spans every backend in + * fleet view, and asking the active one about a remote path gets a + * wrong answer from the wrong machine. */ + backendFor: (worktreePath: string) => ElectronAPI onBulkDelete: ( paths: string[], force: boolean, @@ -64,6 +68,7 @@ export function Cleanup({ prStatuses, mergedPaths, lastActive, + backendFor, onBulkDelete }: CleanupProps): JSX.Element { const backend = useBackend() @@ -74,7 +79,8 @@ export function Cleanup({ const [includeDirty, setIncludeDirty] = useState(false) // Absent key = not scanned yet. Distinct from `false` (scanned, clean). const [dirtyMap, setDirtyMap] = useState>({}) - const [scanningDirty, setScanningDirty] = useState(true) + // Answered with an error: no longer pending, but still not known clean. + const [dirtyFailed, setDirtyFailed] = useState>({}) const [activityLastTs, setActivityLastTs] = useState>({}) const [selected, setSelected] = useState>({}) // Paths the user has explicitly clicked; their selection state must survive @@ -108,61 +114,83 @@ export function Cleanup({ // A path missing from dirtyMap means "not scanned yet", which is distinct // from "clean" — see the selection defaults below, which must not offer up // an unscanned worktree for deletion. + // + // Each path is asked about once per open, never per `worktrees` change. + // The list legitimately changes while the modal is open — every deletion + // from it is one — and rescanning the whole set each time multiplied the + // sweep by the number of deletions. Requests can't be cancelled once sent, + // so a re-run only ever adds the paths it hasn't asked about yet. + const requestedDirty = useRef(new Set()) + const mounted = useRef(true) + useEffect(() => { + mounted.current = true + return () => { + mounted.current = false + } + }, []) useEffect(() => { - let cancelled = false - setScanningDirty(true) - let outstanding = eligible.length - if (outstanding === 0) setScanningDirty(false) for (const w of eligible) { - void backend + if (requestedDirty.current.has(w.path)) continue + requestedDirty.current.add(w.path) + void backendFor(w.path) .isWorktreeDirty(w.path, { bulk: true }) .then((d) => { - if (cancelled) return + if (!mounted.current) return setDirtyMap((prev) => ({ ...prev, [w.path]: d.git || d.scratchpad })) }) - .catch(() => {}) - .finally(() => { - if (cancelled) return - if (--outstanding === 0) setScanningDirty(false) + .catch(() => { + if (!mounted.current) return + setDirtyFailed((prev) => ({ ...prev, [w.path]: true })) }) } - return () => { - cancelled = true - } - }, [eligible, backend]) + }, [eligible, backendFor]) + const scanningDirty = eligible.some( + (w) => dirtyMap[w.path] === undefined && !dirtyFailed[w.path] + ) + // Each backend keeps its own activity log, and in fleet view the list + // spans several, so the log is fetched once from every backend that owns + // an eligible worktree. Like the dirty sweep, a backend is asked once per + // open: a deletion changes `eligible` but not anyone's history. + // `backendFor` hands back one cached object per backend, so it is the key. + const requestedLogs = useRef(new Set()) + const logsInFlight = useRef(0) useEffect(() => { - let cancelled = false - ;(async () => { - setLoading(true) - try { - const log = await backend.getActivityLog() - if (cancelled) return - const lastTs: Record = {} - for (const [path, rec] of Object.entries(log as ActivityLog)) { - // Only 'processing' proves the agent actually ran here. Waking a - // chat session derives 'waiting' and auto-sleep derives 'idle', - // so using the newest event of any kind resets a worktree's age - // just from the user opening it. - let ts: number | undefined - for (let i = rec.events.length - 1; i >= 0; i--) { - if (rec.events[i].s === 'processing') { - ts = rec.events[i].t - break + for (const w of eligible) { + const owner = backendFor(w.path) + if (requestedLogs.current.has(owner)) continue + requestedLogs.current.add(owner) + logsInFlight.current++ + void owner + .getActivityLog() + .then((log) => { + if (!mounted.current) return + const lastTs: Record = {} + for (const [path, rec] of Object.entries(log as ActivityLog)) { + if (backendFor(path) !== owner) continue + // Only 'processing' proves the agent actually ran here. Waking a + // chat session derives 'waiting' and auto-sleep derives 'idle', + // so using the newest event of any kind resets a worktree's age + // just from the user opening it. + let ts: number | undefined + for (let i = rec.events.length - 1; i >= 0; i--) { + if (rec.events[i].s === 'processing') { + ts = rec.events[i].t + break + } } + ts ??= rec.createdAt + if (ts !== undefined) lastTs[path] = ts } - ts ??= rec.createdAt - if (ts !== undefined) lastTs[path] = ts - } - setActivityLastTs(lastTs) - } finally { - if (!cancelled) setLoading(false) - } - })() - return () => { - cancelled = true + setActivityLastTs((prev) => ({ ...prev, ...lastTs })) + }) + .catch(() => {}) + .finally(() => { + if (--logsInFlight.current === 0 && mounted.current) setLoading(false) + }) } - }, [eligible]) + if (logsInFlight.current === 0) setLoading(false) + }, [eligible, backendFor]) const ageMs = AGES.find((a) => a.id === ageKey)!.ms @@ -240,7 +268,7 @@ export function Cleanup({ }) if (!commitsByPath[path]) { setCommitsByPath((prev) => ({ ...prev, [path]: 'loading' })) - backend + backendFor(path) .getBranchCommits(path) .then((commits) => { setCommitsByPath((prev) => ({ ...prev, [path]: commits })) diff --git a/src/renderer/merge-backend-states.test.ts b/src/renderer/merge-backend-states.test.ts index be55c717..9b062a18 100644 --- a/src/renderer/merge-backend-states.test.ts +++ b/src/renderer/merge-backend-states.test.ts @@ -163,4 +163,76 @@ describe('getMergedWorktrees (shared cache)', () => { expect(second.statuses.t1).toBe('processing') expect(second.tabsByWorktree['/w']).toBe(first.tabsByWorktree['/w']) // but not the tab walk }) + + it('keeps unchanged outputs by reference when another field changes', () => { + const list = [wt('/w', '/r')] + const first = getMergedWorktrees( + [backend('local', { kind: 'local', worktrees: list, statuses: { t1: 'idle' } })], + 'merged', + 'local' + ) + const second = getMergedWorktrees( + [backend('local', { kind: 'local', worktrees: list, statuses: { t1: 'processing' } })], + 'merged', + 'local' + ) + expect(second.statuses).not.toBe(first.statuses) + expect(second.flatWorktrees).toBe(first.flatWorktrees) + expect(second.worktrees).toBe(first.worktrees) + expect(second.pathBackendId).toBe(first.pathBackendId) + expect(second.backendMeta).toBe(first.backendMeta) + }) + + it('keeps every output stable in single mode when a hidden backend streams', () => { + // Single mode merges only the active backend, but the cache key spans + // all of them — a token on a hidden remote still reruns the merge. + const local = backend('local', { + kind: 'local', + worktrees: [wt('/l/a', '/l')], + repoRoots: ['/l'], + pending: [pending('p1', '/l')], + statuses: { t1: 'idle' } + }) + const first = getMergedWorktrees( + [local, backend('beta', { worktrees: [wt('/b/a', '/b')], statuses: { b1: 'idle' } })], + 'single', + 'local' + ) + const second = getMergedWorktrees( + [local, backend('beta', { worktrees: [wt('/b/a', '/b')], statuses: { b1: 'processing' } })], + 'single', + 'local' + ) + expect(second).toBe(first) + for (const key of Object.keys(first) as (keyof typeof first)[]) { + expect(second[key], key).toBe(first[key]) + } + }) + + it('returns the previous result when an event changed nothing it reads', () => { + const list = [wt('/w', '/r')] + const first = getMergedWorktrees( + [backend('local', { kind: 'local', worktrees: list })], + 'merged', + 'local' + ) + const second = getMergedWorktrees( + [backend('local', { kind: 'local', worktrees: list })], + 'merged', + 'local' + ) + expect(second).toBe(first) + }) + + it('hands out a new worktree list when a worktree is added', () => { + const a = wt('/a', '/r') + const first = getMergedWorktrees([backend('local', { kind: 'local', worktrees: [a] })], 'merged', 'local') + const second = getMergedWorktrees( + [backend('local', { kind: 'local', worktrees: [a, wt('/b', '/r')] })], + 'merged', + 'local' + ) + expect(second.flatWorktrees).not.toBe(first.flatWorktrees) + expect(second.flatWorktrees.map((w) => w.path)).toEqual(['/a', '/b']) + }) }) diff --git a/src/renderer/merge-backend-states.ts b/src/renderer/merge-backend-states.ts index e12e7480..79b47a99 100644 --- a/src/renderer/merge-backend-states.ts +++ b/src/renderer/merge-backend-states.ts @@ -230,15 +230,71 @@ export function getMergedWorktrees( // evicts a backend's cached tab walk. const segments = tabsSelector.select(entries.map((e) => ({ id: e.backendId, state: e.state }))) const tabsById = new Map(segments.map((s) => [s.backendId, s.value])) - const result = mergeBackendStates(entries, { + const fresh = mergeBackendStates(entries, { viewMode, activeId, tabsFor: (backendId) => tabsById.get(backendId) ?? {} }) + const result = cache ? reuseUnchanged(cache.result, fresh) : fresh cache = { entries, viewMode, activeId, result } return result } +/** The cache key above is each backend's whole AppState, which is a new + * object after every event on that backend — a streamed token included. + * So the merge reruns constantly, and before this every output was a new + * array or map each time even when its contents were identical. Anything + * keyed on one of them re-fired per event: the Cleanup modal's dirty sweep + * re-queued a `git status` for every worktree on every token, which buried + * a remote host under thousands of queued reads and stalled tab switching + * behind them. Each field keeps its previous reference unless its contents + * actually changed, and the whole result does too when no field did. */ +function reuseUnchanged(prev: MergedWorktreeData, next: MergedWorktreeData): MergedWorktreeData { + let changed = false + const out = {} as Record + for (const key of Object.keys(next) as (keyof MergedWorktreeData)[]) { + if (sameContents(prev[key], next[key])) { + out[key] = prev[key] + } else { + out[key] = next[key] + changed = true + } + } + return changed ? (out as unknown as MergedWorktreeData) : prev +} + +function sameContents(a: unknown, b: unknown): boolean { + if (a === b) return true + if (Array.isArray(a) && Array.isArray(b)) { + if (a.length !== b.length) return false + for (let i = 0; i < a.length; i++) if (!sameEntry(a[i], b[i])) return false + return true + } + if (!isRecord(a) || !isRecord(b)) return false + const aKeys = Object.keys(a) + if (aKeys.length !== Object.keys(b).length) return false + for (const k of aKeys) { + if (!(k in b) || !sameEntry(a[k], b[k])) return false + } + return true +} + +/** One level deeper than reference equality, for the small wrappers the + * merge allocates per element (`MergedWorktree`, `backendMeta` values). + * Their fields are slice values the reducers already keep stable. */ +function sameEntry(a: unknown, b: unknown): boolean { + if (a === b) return true + if (!isRecord(a) || !isRecord(b) || Array.isArray(a) || Array.isArray(b)) return false + const aKeys = Object.keys(a) + if (aKeys.length !== Object.keys(b).length) return false + for (const k of aKeys) if (a[k] !== b[k]) return false + return true +} + +function isRecord(v: unknown): v is Record { + return typeof v === 'object' && v !== null +} + /** Test-only: forget the shared cache between cases. */ export function resetMergedWorktreesCache(): void { cache = null