From 58e8c7d4810052e848c6cb3f019e7a88d2015e76 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 13 Aug 2026 18:24:41 -0700 Subject: [PATCH] fix(worktree): hold retired names across a refresh instead of blanking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit refreshKey changes on every workspace-list mutation, so create-multiple refetches after each create and the hook returned an empty list until the refetch landed — precisely the window in which resetForNextCreate clears the name field and a fresh suggestion is drawn. Keep the previous answer while revalidating and reset only when the repo changes; a failed refresh keeps what was already loaded rather than un-retiring everything. Also makes the returned array referentially stable, so the suggestion memo downstream stops rerunning on every refetch. --- .../src/hooks/useRetiredWorktreeNames.test.ts | 85 +++++++++++++++++++ .../src/hooks/useRetiredWorktreeNames.ts | 22 +++-- 2 files changed, 102 insertions(+), 5 deletions(-) create mode 100644 src/renderer/src/hooks/useRetiredWorktreeNames.test.ts diff --git a/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts b/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts new file mode 100644 index 00000000000..452771caf14 --- /dev/null +++ b/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts @@ -0,0 +1,85 @@ +// @vitest-environment happy-dom + +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { renderHook, waitFor } from '@testing-library/react' +import { useRetiredWorktreeNames } from './useRetiredWorktreeNames' + +const listRetiredNames = vi.fn() + +beforeEach(() => { + listRetiredNames.mockReset() + Object.assign(window, { api: { worktrees: { listRetiredNames } } }) +}) + +describe('useRetiredWorktreeNames', () => { + it('keeps the previous names while a refresh is in flight', async () => { + // Why: refreshKey changes on every workspace-list mutation, so create-multiple refetches after + // each create. Dropping to empty in between would suggest a spent name in exactly that window. + listRetiredNames.mockResolvedValueOnce(['nautilus']) + const { result, rerender } = renderHook( + ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), + { initialProps: { key: 'a' } } + ) + await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + + let resolveSecond: (names: string[]) => void = () => {} + listRetiredNames.mockReturnValueOnce( + new Promise((resolve) => { + resolveSecond = resolve + }) + ) + rerender({ key: 'b' }) + + expect(result.current.names).toEqual(['nautilus']) + expect(result.current.loading).toBe(true) + + resolveSecond(['nautilus', 'seahorse']) + await waitFor(() => expect(result.current.names).toEqual(['nautilus', 'seahorse'])) + }) + + it('returns a referentially stable array across refreshes that change nothing', async () => { + // The suggestion memo downstream keys on this array; a new identity per refetch reruns it. + const names = ['nautilus'] + listRetiredNames.mockResolvedValue(names) + const { result, rerender } = renderHook( + ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), + { initialProps: { key: 'a' } } + ) + await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + const first = result.current.names + + rerender({ key: 'b' }) + + expect(result.current.names).toBe(first) + }) + + it('drops names when the repo changes rather than showing another repo pool', async () => { + listRetiredNames.mockResolvedValueOnce(['nautilus']) + const { result, rerender } = renderHook( + ({ repoId }: { repoId: string }) => useRetiredWorktreeNames(repoId, 'key'), + { initialProps: { repoId: 'repo-1' } } + ) + await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + + listRetiredNames.mockReturnValueOnce(new Promise(() => {})) + rerender({ repoId: 'repo-2' }) + + expect(result.current.names).toEqual([]) + }) + + it('keeps previously loaded names when a refresh fails', async () => { + listRetiredNames.mockResolvedValueOnce(['nautilus']) + const { result, rerender } = renderHook( + ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), + { initialProps: { key: 'a' } } + ) + await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + + vi.spyOn(console, 'warn').mockImplementation(() => {}) + listRetiredNames.mockRejectedValueOnce(new Error('host unreachable')) + rerender({ key: 'b' }) + + await waitFor(() => expect(result.current.loading).toBe(false)) + expect(result.current.names).toEqual(['nautilus']) + }) +}) diff --git a/src/renderer/src/hooks/useRetiredWorktreeNames.ts b/src/renderer/src/hooks/useRetiredWorktreeNames.ts index 8dfdc0bb592..f68c5c91e1b 100644 --- a/src/renderer/src/hooks/useRetiredWorktreeNames.ts +++ b/src/renderer/src/hooks/useRetiredWorktreeNames.ts @@ -1,24 +1,32 @@ import { useEffect, useState } from 'react' +const NO_RETIRED_NAMES: readonly string[] = [] + /** Names already spent in a repo, including workspaces that have since been deleted. * * Why this is fetched rather than derived from the worktree list: a deleted workspace leaves no * row behind, but its directory may still hold agent conversation state keyed by that path. Only * the host-side registry remembers it, so name suggestions must ask for it explicitly. * + * Stale-while-revalidate across `refreshKey`, reset only when the repo changes. A refresh fires on + * every workspace-list mutation, so create-multiple refetches after each create — dropping to + * empty in between would suggest a spent name in exactly that window, which is when + * `resetForNextCreate` clears the field. Holding the previous answer also keeps the returned array + * referentially stable, so the suggestion memo downstream does not rerun on every refetch. + * * Failure returns an empty set, which degrades to the pre-existing behavior (a spent name can be * suggested) rather than blocking workspace creation. */ export function useRetiredWorktreeNames( repoId: string | null | undefined, refreshKey: unknown ): { - names: string[] + names: readonly string[] loading: boolean } { const [loaded, setLoaded] = useState<{ repoId: string refreshKey: unknown - names: string[] + names: readonly string[] } | null>(null) useEffect(() => { @@ -36,7 +44,12 @@ export function useRetiredWorktreeNames( }) .catch((err) => { if (!cancelled) { - setLoaded({ repoId, refreshKey, names: [] }) + // Keep whatever a previous refresh loaded; a transient failure must not un-retire names. + setLoaded((previous) => ({ + repoId, + refreshKey, + names: previous?.repoId === repoId ? previous.names : NO_RETIRED_NAMES + })) } console.warn(`Failed to load retired workspace names for repo ${repoId}:`, err) }) @@ -46,8 +59,7 @@ export function useRetiredWorktreeNames( }, [refreshKey, repoId]) return { - names: - loaded && loaded.repoId === repoId && loaded.refreshKey === refreshKey ? loaded.names : [], + names: loaded && loaded.repoId === repoId ? loaded.names : NO_RETIRED_NAMES, loading: Boolean(repoId) && (loaded?.repoId !== repoId || loaded?.refreshKey !== refreshKey) } }