From 07b207c5fa6807dd31c10d4d3faba4af6d255b85 Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:58:33 -0700 Subject: [PATCH] refactor(worktree): put the retired-name cache rules on one implementation The desktop and mobile hooks that fetch retired names had already drifted four ways. The transports genuinely differ (IPC vs RPC), but the caching rules must not, and mobile's copy reset to [] on any error -- which un-retires every name for the rest of the sheet session, the one outcome retirement exists to prevent. Moves the rules into src/shared/worktree/retired-name-cache: response normalization, the never-leak-across-repos rule, and the hold-previous-on- failure rule. Pure, no React, because src/shared is on the main process's import graph. Each platform keeps its own transport and effect. Mobile moves up to desktop's behavior: it now holds the previous answer through a failed refresh, and refetches when the workspace list changes instead of never refetching after mount. Also drops the unused `loading` return. Neither platform consumed it; its only consumer was the Create-button gate reviewed out earlier, and removing it makes that regression unexpressible. --- mobile/src/components/NewWorktreeModal.tsx | 14 ++- .../use-retired-worktree-names.test.tsx | 116 ++++++++++++++++++ .../worktree/use-retired-worktree-names.ts | 63 +++++----- .../sidebar/worktree-name-suggestions.ts | 8 +- src/renderer/src/hooks/useComposerState.ts | 7 +- .../src/hooks/useRetiredWorktreeNames.test.ts | 37 ++++-- .../src/hooks/useRetiredWorktreeNames.ts | 55 +++------ src/renderer/src/web/web-preload-api.ts | 9 +- .../worktree/retired-name-cache.test.ts | 76 ++++++++++++ src/shared/worktree/retired-name-cache.ts | 54 ++++++++ 10 files changed, 343 insertions(+), 96 deletions(-) create mode 100644 mobile/src/worktree/use-retired-worktree-names.test.tsx create mode 100644 src/shared/worktree/retired-name-cache.test.ts create mode 100644 src/shared/worktree/retired-name-cache.ts diff --git a/mobile/src/components/NewWorktreeModal.tsx b/mobile/src/components/NewWorktreeModal.tsx index 0da42f35f21..f10a96f17c0 100644 --- a/mobile/src/components/NewWorktreeModal.tsx +++ b/mobile/src/components/NewWorktreeModal.tsx @@ -189,8 +189,16 @@ function NewWorktreeModalContent({ const [selectedRepo, setSelectedRepo] = useState(null) // Why: a deleted workspace's directory can still hold agent conversation state keyed by cwd, so // its name must never be suggested again. Fetched per selected repo while the sheet is open. - // Create is never gated on it: the host skips retired candidates before doing any git work. - const { names: retiredWorktreeNames } = useRetiredWorktreeNames(client, selectedRepo?.id) + // Keyed on the path set rather than the array so a poll that changes nothing does not refetch. + const retiredNamesRefreshKey = useMemo( + () => [...(existingWorktreePaths ?? [])].sort().join('\0'), + [existingWorktreePaths] + ) + const retiredWorktreeNames = useRetiredWorktreeNames( + client, + selectedRepo?.id, + retiredNamesRefreshKey + ) const { drawerView, formSheetVisible, formSheetInteractive, transitionDrawer, openSourceDrawer } = useNewWorktreeDrawerNavigation(visible) const createInFlightRef = useRef(false) @@ -662,6 +670,8 @@ function NewWorktreeModalContent({ client, repoId: selectedRepo.id, baseName, + // `baseName` is the suggestion exactly when the user typed nothing, so no identity check + // is needed — desktop's seeded composer needs one (useComposerState `nameWasGenerated`). nameWasGenerated: !trimmedName, createdWithAgentId, comment: trimmedNote, diff --git a/mobile/src/worktree/use-retired-worktree-names.test.tsx b/mobile/src/worktree/use-retired-worktree-names.test.tsx new file mode 100644 index 00000000000..34fbe9a6506 --- /dev/null +++ b/mobile/src/worktree/use-retired-worktree-names.test.tsx @@ -0,0 +1,116 @@ +import { createElement } from 'react' +import { act, create } from 'react-test-renderer' +import { describe, expect, it } from 'vitest' +import type { RpcClient } from '../transport/rpc-client' +import { useRetiredWorktreeNames } from './use-retired-worktree-names' + +type Pending = { resolve: (response: unknown) => void; reject: (err: Error) => void } + +/** Renders the hook against a deferred RPC client so a test decides exactly when each + * `worktree.listRetiredNames` lands, and for which repo. */ +function mountNames() { + const pending: Pending[] = [] + const requests: { method: string; params: unknown }[] = [] + let latest: readonly string[] = [] + + const client = { + sendRequest: (method: string, params: unknown) => { + requests.push({ method, params }) + return new Promise((resolve, reject) => pending.push({ resolve, reject })) + } + } as unknown as RpcClient + + function Probe({ repoId, refreshKey }: { repoId: string | null; refreshKey: string }) { + latest = useRetiredWorktreeNames(client, repoId, refreshKey) + return null + } + + let renderer!: ReturnType + act(() => { + renderer = create(createElement(Probe, { repoId: 'repo-1', refreshKey: 'a' })) + }) + return { + get names(): readonly string[] { + return latest + }, + requests, + rerender(props: { repoId: string | null; refreshKey: string }) { + act(() => { + renderer.update(createElement(Probe, props)) + }) + }, + async settle(index: number, retiredNamesByRepo: Record) { + await act(async () => { + pending[index]!.resolve({ result: { retiredNamesByRepo } }) + await Promise.resolve() + }) + }, + async fail(index: number) { + await act(async () => { + pending[index]!.reject(new Error('host unreachable')) + await Promise.resolve() + }) + } + } +} + +describe('useRetiredWorktreeNames', () => { + it('reads the selected repo out of the response envelope', async () => { + const probe = mountNames() + expect(probe.requests[0]).toEqual({ + method: 'worktree.listRetiredNames', + params: { repo: 'id:repo-1' } + }) + await probe.settle(0, { 'repo-1': ['nautilus'], 'repo-2': ['seahorse'] }) + expect(probe.names).toEqual(['nautilus']) + }) + + it('answers empty for a host that omits the field', async () => { + const probe = mountNames() + await probe.settle(0, {}) + expect(probe.names).toEqual([]) + }) + + // Regression: this hook used to reset to [] on any error, un-retiring every name for the rest of + // the sheet session. Desktop always held the previous answer; both now share that rule. + it('keeps previously loaded names when a refresh fails', async () => { + const probe = mountNames() + await probe.settle(0, { 'repo-1': ['nautilus'] }) + expect(probe.names).toEqual(['nautilus']) + + probe.rerender({ repoId: 'repo-1', refreshKey: 'b' }) + await probe.fail(1) + + expect(probe.names).toEqual(['nautilus']) + }) + + // Regression: the effect used to depend on [client, repoId] only, so a workspace created while + // the sheet was open never made it into the pool. + it('refetches when the workspace list changes', async () => { + const probe = mountNames() + await probe.settle(0, { 'repo-1': ['nautilus'] }) + + probe.rerender({ repoId: 'repo-1', refreshKey: 'b' }) + expect(probe.requests).toHaveLength(2) + // The previous answer stays visible while the refetch is in flight. + expect(probe.names).toEqual(['nautilus']) + + await probe.settle(1, { 'repo-1': ['nautilus', 'seahorse'] }) + expect(probe.names).toEqual(['nautilus', 'seahorse']) + }) + + it('drops names when the repo changes rather than showing another repo pool', async () => { + const probe = mountNames() + await probe.settle(0, { 'repo-1': ['nautilus'] }) + + probe.rerender({ repoId: 'repo-2', refreshKey: 'a' }) + expect(probe.names).toEqual([]) + }) + + it('reports nothing without a selected repo, and asks the host nothing', () => { + const probe = mountNames() + probe.rerender({ repoId: null, refreshKey: 'a' }) + expect(probe.names).toEqual([]) + expect(probe.requests).toHaveLength(1) + }) +}) diff --git a/mobile/src/worktree/use-retired-worktree-names.ts b/mobile/src/worktree/use-retired-worktree-names.ts index a641b3f9d4b..ee3872a6948 100644 --- a/mobile/src/worktree/use-retired-worktree-names.ts +++ b/mobile/src/worktree/use-retired-worktree-names.ts @@ -1,57 +1,50 @@ import { useEffect, useState } from 'react' +import { + readRetiredNamesForRepo, + retiredNamesAfterRefresh, + selectRetiredNames, + type RetiredNamesLoad +} from '../../../src/shared/worktree/retired-name-cache' import type { RpcClient } from '../transport/rpc-client' /** Names already spent in a repo, including workspaces that have since been deleted. * * Why a targeted request rather than the workspace catalog: the catalog is served by `worktree.ps`, * which carries rows only. Retired names are needed just while the create sheet is open and only - * for one repo, so this mirrors the desktop hook and asks for exactly that. + * for one repo, so this asks for exactly that. * - * Hosts predating the field omit it, and any failure yields an empty list, so this degrades to - * deduping against live workspaces alone — the behavior before retirement existed. */ + * `refreshKey` must change on every workspace-list mutation. Caching rules live in + * `retired-name-cache` so this and the desktop hook cannot drift on what a failure means, and no + * loading state is reported because create is never gated on this fetch. */ export function useRetiredWorktreeNames( client: RpcClient | null | undefined, - repoId: string | null | undefined -): { names: readonly string[]; loading: boolean } { - const [loaded, setLoaded] = useState<{ - repoId: string - names: readonly string[] - } | null>(null) + repoId: string | null | undefined, + refreshKey: unknown +): readonly string[] { + const [loaded, setLoaded] = useState(null) + const activeRepoId = client && repoId ? repoId : null useEffect(() => { - if (!client || !repoId) { + if (!client || !activeRepoId) { setLoaded(null) return } let cancelled = false + const settle = (names: readonly string[] | null): void => { + if (!cancelled) { + setLoaded((previous) => retiredNamesAfterRefresh(previous, activeRepoId, names)) + } + } void client - .sendRequest('worktree.listRetiredNames', { repo: `id:${repoId}` }) - .then((response) => { - if (cancelled) { - return - } - const result = (response as { result?: unknown }).result as - | { retiredNamesByRepo?: Record } - | undefined - const names = result?.retiredNamesByRepo?.[repoId] - setLoaded({ - repoId, - // A host can answer with a malformed row; a non-string would throw when normalized. - names: Array.isArray(names) ? names.filter((name) => typeof name === 'string') : [] - }) - }) - .catch(() => { - if (!cancelled) { - setLoaded({ repoId, names: [] }) - } - }) + .sendRequest('worktree.listRetiredNames', { repo: `id:${activeRepoId}` }) + .then((response) => + settle(readRetiredNamesForRepo((response as { result?: unknown }).result, activeRepoId)) + ) + .catch(() => settle(null)) return () => { cancelled = true } - }, [client, repoId]) + }, [activeRepoId, client, refreshKey]) - return { - names: loaded && loaded.repoId === repoId ? loaded.names : [], - loading: Boolean(client && repoId) && loaded?.repoId !== repoId - } + return selectRetiredNames(loaded, activeRepoId) } diff --git a/src/renderer/src/components/sidebar/worktree-name-suggestions.ts b/src/renderer/src/components/sidebar/worktree-name-suggestions.ts index bf1ebebe5fa..64827392bb7 100644 --- a/src/renderer/src/components/sidebar/worktree-name-suggestions.ts +++ b/src/renderer/src/components/sidebar/worktree-name-suggestions.ts @@ -1,7 +1,7 @@ -import { basename } from '@/lib/path' import { normalizeSuggestedName, - selectSuggestedCreatureName + selectSuggestedCreatureName, + suggestionPathBasename } from '../../../../shared/worktree-name-suggestion' type WorktreePathLike = { @@ -14,7 +14,9 @@ function collectUsedNames(worktreesByRepo: Record): const usedNames = new Set() for (const worktrees of Object.values(worktreesByRepo)) { for (const worktree of worktrees) { - usedNames.add(normalizeSuggestedName(basename(worktree.path))) + // Shared basename, not `@/lib/path`'s: mobile keys collisions the same way, and two copies of + // the key could drift into suggesting a name that is in fact taken. + usedNames.add(normalizeSuggestedName(suggestionPathBasename(worktree.path))) } } return usedNames diff --git a/src/renderer/src/hooks/useComposerState.ts b/src/renderer/src/hooks/useComposerState.ts index 8f9f56b5573..8356f379a84 100644 --- a/src/renderer/src/hooks/useComposerState.ts +++ b/src/renderer/src/hooks/useComposerState.ts @@ -1601,9 +1601,7 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS .join('\0'), [repoId, worktreesByRepo] ) - // Create is never gated on this load: the host skips retired candidates before any git work, so - // a pending fetch can only cost a suggestion, never correctness. - const { names: retiredWorktreeNames } = useRetiredWorktreeNames(repoId, retiredNamesRefreshKey) + const retiredWorktreeNames = useRetiredWorktreeNames(repoId, retiredNamesRefreshKey) const fallbackCreatureName = useMemo( () => getSuggestedCreatureName(worktreesByRepo, undefined, retiredWorktreeNames), [worktreesByRepo, retiredWorktreeNames] @@ -3650,6 +3648,9 @@ export function useComposerState(options: UseComposerStateOptions): UseComposerS } // Why: only a name Orca generated may be retired — the creature pool contains ordinary words // ("orca", "runner", "molly") a user can type deliberately and expect to reuse. + // The identity check is what a linked PR/issue seed makes necessary here; mobile's blank-create + // path (NewWorktreeModal, `nameWasGenerated: !trimmedName`) has no other seed, so it can't + // share this expression. Same rule, two submit paths — change both together. const nameWasGenerated = !name.trim() && workspaceName === fallbackCreatureName const submitBaseBranch = smartGitHubResolution.kind === 'pr-start-point' diff --git a/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts b/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts index 452771caf14..2c795a7c536 100644 --- a/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts +++ b/src/renderer/src/hooks/useRetiredWorktreeNames.test.ts @@ -20,7 +20,7 @@ describe('useRetiredWorktreeNames', () => { ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), { initialProps: { key: 'a' } } ) - await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + await waitFor(() => expect(result.current).toEqual(['nautilus'])) let resolveSecond: (names: string[]) => void = () => {} listRetiredNames.mockReturnValueOnce( @@ -30,11 +30,10 @@ describe('useRetiredWorktreeNames', () => { ) rerender({ key: 'b' }) - expect(result.current.names).toEqual(['nautilus']) - expect(result.current.loading).toBe(true) + expect(result.current).toEqual(['nautilus']) resolveSecond(['nautilus', 'seahorse']) - await waitFor(() => expect(result.current.names).toEqual(['nautilus', 'seahorse'])) + await waitFor(() => expect(result.current).toEqual(['nautilus', 'seahorse'])) }) it('returns a referentially stable array across refreshes that change nothing', async () => { @@ -45,12 +44,12 @@ describe('useRetiredWorktreeNames', () => { ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), { initialProps: { key: 'a' } } ) - await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) - const first = result.current.names + await waitFor(() => expect(result.current).toEqual(['nautilus'])) + const first = result.current rerender({ key: 'b' }) - expect(result.current.names).toBe(first) + expect(result.current).toBe(first) }) it('drops names when the repo changes rather than showing another repo pool', async () => { @@ -59,12 +58,12 @@ describe('useRetiredWorktreeNames', () => { ({ repoId }: { repoId: string }) => useRetiredWorktreeNames(repoId, 'key'), { initialProps: { repoId: 'repo-1' } } ) - await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + await waitFor(() => expect(result.current).toEqual(['nautilus'])) listRetiredNames.mockReturnValueOnce(new Promise(() => {})) rerender({ repoId: 'repo-2' }) - expect(result.current.names).toEqual([]) + expect(result.current).toEqual([]) }) it('keeps previously loaded names when a refresh fails', async () => { @@ -73,13 +72,25 @@ describe('useRetiredWorktreeNames', () => { ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), { initialProps: { key: 'a' } } ) - await waitFor(() => expect(result.current.names).toEqual(['nautilus'])) + await waitFor(() => expect(result.current).toEqual(['nautilus'])) - vi.spyOn(console, 'warn').mockImplementation(() => {}) + const warn = 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']) + await waitFor(() => expect(warn).toHaveBeenCalled()) + expect(result.current).toEqual(['nautilus']) + }) + + it('refetches when the refresh key changes', async () => { + listRetiredNames.mockResolvedValue(['nautilus']) + const { rerender } = renderHook( + ({ key }: { key: string }) => useRetiredWorktreeNames('repo-1', key), + { initialProps: { key: 'a' } } + ) + await waitFor(() => expect(listRetiredNames).toHaveBeenCalledTimes(1)) + + rerender({ key: 'b' }) + await waitFor(() => expect(listRetiredNames).toHaveBeenCalledTimes(2)) }) }) diff --git a/src/renderer/src/hooks/useRetiredWorktreeNames.ts b/src/renderer/src/hooks/useRetiredWorktreeNames.ts index f68c5c91e1b..c4a7e948842 100644 --- a/src/renderer/src/hooks/useRetiredWorktreeNames.ts +++ b/src/renderer/src/hooks/useRetiredWorktreeNames.ts @@ -1,6 +1,9 @@ import { useEffect, useState } from 'react' - -const NO_RETIRED_NAMES: readonly string[] = [] +import { + retiredNamesAfterRefresh, + selectRetiredNames, + type RetiredNamesLoad +} from '../../../shared/worktree/retired-name-cache' /** Names already spent in a repo, including workspaces that have since been deleted. * @@ -8,26 +11,17 @@ const NO_RETIRED_NAMES: readonly string[] = [] * 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. + * `refreshKey` must change on every workspace-list mutation, so create-multiple refetches after + * each create. Caching rules live in `retired-name-cache` because the mobile hook obeys the same + * ones over a different transport. * - * Failure returns an empty set, which degrades to the pre-existing behavior (a spent name can be - * suggested) rather than blocking workspace creation. */ + * Deliberately reports no loading state: create is never gated on this fetch — the host skips + * retired candidates before any git work, so a pending fetch can only cost a suggestion. */ export function useRetiredWorktreeNames( repoId: string | null | undefined, refreshKey: unknown -): { - names: readonly string[] - loading: boolean -} { - const [loaded, setLoaded] = useState<{ - repoId: string - refreshKey: unknown - names: readonly string[] - } | null>(null) +): readonly string[] { + const [loaded, setLoaded] = useState(null) useEffect(() => { if (!repoId) { @@ -35,22 +29,16 @@ export function useRetiredWorktreeNames( return } let cancelled = false + const settle = (names: readonly string[] | null): void => { + if (!cancelled) { + setLoaded((previous) => retiredNamesAfterRefresh(previous, repoId, names)) + } + } void window.api.worktrees .listRetiredNames({ repoId }) - .then((names) => { - if (!cancelled) { - setLoaded({ repoId, refreshKey, names }) - } - }) + .then(settle) .catch((err) => { - if (!cancelled) { - // 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 - })) - } + settle(null) console.warn(`Failed to load retired workspace names for repo ${repoId}:`, err) }) return () => { @@ -58,8 +46,5 @@ export function useRetiredWorktreeNames( } }, [refreshKey, repoId]) - return { - names: loaded && loaded.repoId === repoId ? loaded.names : NO_RETIRED_NAMES, - loading: Boolean(repoId) && (loaded?.repoId !== repoId || loaded?.refreshKey !== refreshKey) - } + return selectRetiredNames(loaded, repoId) } diff --git a/src/renderer/src/web/web-preload-api.ts b/src/renderer/src/web/web-preload-api.ts index 8bd5e98c7b6..2b243aafac3 100644 --- a/src/renderer/src/web/web-preload-api.ts +++ b/src/renderer/src/web/web-preload-api.ts @@ -63,6 +63,7 @@ import { legacyBaseRefSearchResult } from '../../../shared/base-ref-search-resul import { EMPTY_PTY_MAIN_DELIVERY_DIAGNOSTICS } from '../../../shared/pty-delivery-diagnostics' import { createE2EConfig } from '../../../shared/e2e-config' import { relativePathInsideRoot } from '../../../shared/cross-platform-path' +import { readRetiredNamesForRepo } from '../../../shared/worktree/retired-name-cache' import { applyPRBotAuthorOverride, normalizePRBotAuthorOverrides @@ -1776,12 +1777,10 @@ function createWorktreesApi(): NonNullable['worktrees']> { // degrades the suggestion to the pre-existing behavior rather than blocking workspace create. listRetiredNames: async ({ repoId }) => { try { - const result = await callRuntimeResult<{ retiredNamesByRepo?: Record }>( - 'worktree.listRetiredNames', - { repo: repoId } + return readRetiredNamesForRepo( + await callRuntimeResult('worktree.listRetiredNames', { repo: repoId }), + repoId ) - const names = result.retiredNamesByRepo?.[repoId] - return Array.isArray(names) ? names.filter((name) => typeof name === 'string') : [] } catch { return [] } diff --git a/src/shared/worktree/retired-name-cache.test.ts b/src/shared/worktree/retired-name-cache.test.ts new file mode 100644 index 00000000000..dacc3f0a58b --- /dev/null +++ b/src/shared/worktree/retired-name-cache.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from 'vitest' +import { + NO_RETIRED_NAMES, + readRetiredNamesForRepo, + retiredNamesAfterRefresh, + selectRetiredNames +} from './retired-name-cache' + +describe('readRetiredNamesForRepo', () => { + it('reads the requested repo only', () => { + const result = { retiredNamesByRepo: { 'repo-1': ['nautilus'], 'repo-2': ['seahorse'] } } + expect(readRetiredNamesForRepo(result, 'repo-1')).toEqual(['nautilus']) + }) + + it.each([ + ['a host predating the method', {}], + ['a repo with no entry', { retiredNamesByRepo: { other: ['nautilus'] } }], + ['a non-array row', { retiredNamesByRepo: { 'repo-1': 'nautilus' } }], + ['a null result', null], + ['a non-object result', 'nope'] + ])('answers empty for %s', (_label, result) => { + expect(readRetiredNamesForRepo(result, 'repo-1')).toEqual([]) + }) + + it('drops non-string elements, which would throw when normalized', () => { + const result = { retiredNamesByRepo: { 'repo-1': ['nautilus', 42, null, 'seahorse'] } } + expect(readRetiredNamesForRepo(result, 'repo-1')).toEqual(['nautilus', 'seahorse']) + }) +}) + +describe('retiredNamesAfterRefresh', () => { + it('takes the new names on success', () => { + const previous = { repoId: 'repo-1', names: ['nautilus'] } + expect(retiredNamesAfterRefresh(previous, 'repo-1', ['nautilus', 'seahorse'])).toEqual({ + repoId: 'repo-1', + names: ['nautilus', 'seahorse'] + }) + }) + + // The divergence this module exists to close: mobile used to reset to [] here, which un-retires + // every name in the window where the create form is asking for a suggestion. + it('holds the previous names when the refresh fails', () => { + const previous = { repoId: 'repo-1', names: ['nautilus'] } + expect(retiredNamesAfterRefresh(previous, 'repo-1', null)).toEqual({ + repoId: 'repo-1', + names: ['nautilus'] + }) + }) + + it('does not carry another repo forward through a failure', () => { + const previous = { repoId: 'repo-1', names: ['nautilus'] } + expect(retiredNamesAfterRefresh(previous, 'repo-2', null)).toEqual({ + repoId: 'repo-2', + names: NO_RETIRED_NAMES + }) + }) + + it('answers empty when the first fetch for a repo fails', () => { + expect(retiredNamesAfterRefresh(null, 'repo-1', null).names).toEqual([]) + }) +}) + +describe('selectRetiredNames', () => { + it('serves a load only to the repo it answered for', () => { + const loaded = { repoId: 'repo-1', names: ['nautilus'] } + expect(selectRetiredNames(loaded, 'repo-1')).toEqual(['nautilus']) + expect(selectRetiredNames(loaded, 'repo-2')).toEqual([]) + expect(selectRetiredNames(loaded, null)).toEqual([]) + expect(selectRetiredNames(null, 'repo-1')).toEqual([]) + }) + + it('returns the stored array itself so downstream memos do not rerun', () => { + const names = ['nautilus'] + expect(selectRetiredNames({ repoId: 'repo-1', names }, 'repo-1')).toBe(names) + }) +}) diff --git a/src/shared/worktree/retired-name-cache.ts b/src/shared/worktree/retired-name-cache.ts new file mode 100644 index 00000000000..aafdc65d268 --- /dev/null +++ b/src/shared/worktree/retired-name-cache.ts @@ -0,0 +1,54 @@ +/** Client-side caching rules for retired workspace names, shared by the desktop hook (IPC) and the + * mobile hook (RPC). The transports differ, but what a refresh and a failure *mean* must not: the + * two hooks drifted, and mobile's copy blanked the list on any error — which un-retires names and + * is the one outcome retirement exists to prevent. + * + * Pure by construction: `src/shared` is on the main process's import graph, so no React here. */ + +export const NO_RETIRED_NAMES: readonly string[] = [] + +/** The last answer, tagged with the repo it answered for. */ +export type RetiredNamesLoad = { + repoId: string + names: readonly string[] +} + +/** Reads one repo's names out of a `worktree.listRetiredNames` result. A host predating the method + * omits the field, and any host can answer with a malformed row that would throw when normalized. */ +export function readRetiredNamesForRepo(result: unknown, repoId: string): string[] { + const names = (result as { retiredNamesByRepo?: Record } | null | undefined) + ?.retiredNamesByRepo?.[repoId] + return Array.isArray(names) + ? names.filter((name): name is string => typeof name === 'string') + : [] +} + +/** Next state once a refresh settles; `names === null` means it failed. Belongs in a `setState` + * updater rather than being applied at call time, so overlapping refreshes fold onto whatever + * actually landed last. + * + * A failure holds the same repo's previous answer: a transient failure must not un-retire names, + * and a refresh fires on every workspace-list mutation, so blanking would suggest a spent name in + * exactly the window where the create form is asking for one. */ +export function retiredNamesAfterRefresh( + previous: RetiredNamesLoad | null, + repoId: string, + names: readonly string[] | null +): RetiredNamesLoad { + return { + repoId, + names: names ?? (previous?.repoId === repoId ? previous.names : NO_RETIRED_NAMES) + } +} + +/** Stale-while-revalidate view: a load answers only for its own repo, so names never leak across a + * repo switch, and a refetch in flight keeps serving the previous answer rather than emptying. + * + * Returns the stored array itself, not a copy, so it stays referentially stable and the suggestion + * memo downstream does not rerun on every refetch. */ +export function selectRetiredNames( + loaded: RetiredNamesLoad | null, + repoId: string | null | undefined +): readonly string[] { + return loaded && loaded.repoId === repoId ? loaded.names : NO_RETIRED_NAMES +}