diff --git a/src/main/github/client.test.ts b/src/main/github/client.test.ts index c0a3c58be37..9e3de8a15c9 100644 --- a/src/main/github/client.test.ts +++ b/src/main/github/client.test.ts @@ -926,7 +926,11 @@ describe('getPRForBranch', () => { expect(gitExecFileAsyncMock).not.toHaveBeenCalled() expect(outcome).toMatchObject({ kind: 'found', - pr: { number: 6012, confirmedContainedHeadOid: 'cccc3333cccc3333' } + pr: { + number: 6012, + headRefName: 'fix-hibernation-wake', + confirmedContainedHeadOid: 'cccc3333cccc3333' + } }) }) diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 0d222957651..0607e88f7bf 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -3127,6 +3127,287 @@ describe('createGitHubSlice.fetchPRForBranch', () => { expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull() }) + it('clears a branch-mismatched linked open PR from an accepted coordinator event', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/current' + const worktreeId = 'wt-coordinator-open-mismatch' + const cacheKey = `${repoId}::${branch}` + const worktree = makePRRefreshWorktree({ + id: worktreeId, + repoId, + branch, + head: 'current-head', + linkedPR: 12 + }) + const updateWorktreeMeta = installLinkedPRClearStub(store, { + repoId, + repoPath, + branch, + worktree + }) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'swr', + aliases: [ + { + cacheKey, + repoPath, + repoId, + branch, + worktreeId, + linkedPRNumber: 12, + currentHeadOid: 'current-head' + } + ], + outcome: { + kind: 'found', + pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }), + fetchedAt: 2 + } + }) + + expect(updateWorktreeMeta).toHaveBeenCalledWith( + worktreeId, + { linkedPR: null }, + { shouldApply: expect.any(Function) } + ) + expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBeNull() + }) + + it('does not scan worktrees for a found coordinator outcome without a durable PR link', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/unlinked' + let worktreeIdReads = 0 + const worktrees = Array.from({ length: 100 }, (_, index) => { + const worktree = makePRRefreshWorktree({ + id: `wt-${index}`, + repoId, + branch: `feature/${index}`, + head: `head-${index}`, + linkedPR: null + }) + Object.defineProperty(worktree, 'id', { + enumerable: true, + get: () => { + worktreeIdReads += 1 + return `wt-${index}` + } + }) + return worktree + }) + store.setState({ + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }], + worktreesByRepo: { [repoId]: worktrees } + } as unknown as Partial) + worktreeIdReads = 0 + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'swr', + aliases: [ + { + cacheKey: `${repoId}::${branch}`, + repoPath, + repoId, + branch, + worktreeId: 'wt-0', + linkedPRNumber: null, + currentHeadOid: 'head-0' + } + ], + outcome: { + kind: 'found', + pr: makePR({ number: 12, state: 'open', headRefName: branch }), + fetchedAt: 2 + } + }) + + expect(worktreeIdReads).toBe(0) + }) + + it('does not unlink from a branch-mismatched outcome rejected by the sequence gate', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/current' + const worktreeId = 'wt-coordinator-stale-sequence' + const cacheKey = `${repoId}::${branch}` + const worktree = makePRRefreshWorktree({ + id: worktreeId, + repoId, + branch, + head: 'current-head', + linkedPR: 12 + }) + const updateWorktreeMeta = installLinkedPRClearStub(store, { + repoId, + repoPath, + branch, + worktree + }) + const aliases = [ + { + cacheKey, + repoPath, + repoId, + branch, + worktreeId, + linkedPRNumber: 12, + currentHeadOid: 'current-head' + } + ] + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 2, + reason: 'swr', + aliases, + outcome: { + kind: 'found', + pr: makePR({ number: 12, state: 'open', headRefName: branch }), + fetchedAt: 3 + } + }) + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'swr', + aliases, + outcome: { + kind: 'found', + pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }), + fetchedAt: 4 + } + }) + + expect(updateWorktreeMeta).not.toHaveBeenCalled() + expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBe(12) + expect(store.getState().prRefreshSequences[cacheKey]).toBe(2) + }) + + it('does not unlink an ambiguous worktree id shared by multiple hosts', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/current' + const worktreeId = 'repo-1::/same/path' + const cacheKey = `${repoId}::${branch}` + const updateWorktreeMeta = vi.fn() + store.setState({ + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }], + worktreesByRepo: { + [repoId]: [ + makePRRefreshWorktree({ + id: worktreeId, + repoId, + branch, + head: 'local-head', + linkedPR: 12, + hostId: 'local' + }), + makePRRefreshWorktree({ + id: worktreeId, + repoId, + branch, + head: 'ssh-head', + linkedPR: 12, + hostId: 'ssh:ssh-1' + }) + ] + }, + updateWorktreeMeta + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'swr', + aliases: [ + { + cacheKey, + repoPath, + repoId, + branch, + worktreeId, + linkedPRNumber: 12, + currentHeadOid: 'local-head' + } + ], + outcome: { + kind: 'found', + pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }), + fetchedAt: 2 + } + }) + + expect(updateWorktreeMeta).not.toHaveBeenCalled() + expect(store.getState().worktreesByRepo[repoId]).toHaveLength(2) + expect( + store.getState().worktreesByRepo[repoId]?.every((worktree) => worktree.linkedPR === 12) + ).toBe(true) + }) + + it('does not let an old-host coordinator event unlink a row now owned by another host', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/current' + const worktreeId = 'repo-1::/same/path' + const cacheKey = `ssh:ssh-1::${repoId}::${branch}` + const updateWorktreeMeta = vi.fn() + store.setState({ + repos: [ + { + id: repoId, + path: repoPath, + name: 'repo', + kind: 'git', + connectionId: 'ssh-2' + } + ], + worktreesByRepo: { + [repoId]: [ + makePRRefreshWorktree({ + id: worktreeId, + repoId, + branch, + head: 'ssh-2-head', + linkedPR: 12, + hostId: 'ssh:ssh-2' + }) + ] + }, + updateWorktreeMeta + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + reason: 'swr', + aliases: [ + { + cacheKey, + repoPath, + repoId, + branch, + worktreeId, + linkedPRNumber: 12, + currentHeadOid: 'old-host-head', + executionHostId: 'ssh:ssh-1' + } + ], + outcome: { + kind: 'found', + pr: makePR({ number: 12, state: 'open', headSha: 'old-head', headRefName: 'feature/old' }), + fetchedAt: 2 + } + }) + + expect(updateWorktreeMeta).not.toHaveBeenCalled() + expect(store.getState().worktreesByRepo[repoId]?.[0]?.linkedPR).toBe(12) + }) + it('does not clear a linked merged PR from a coordinator refresh event without a request head', () => { const store = createTestStore() const repoPath = '/repo' @@ -6555,6 +6836,12 @@ describe('shouldClearBranchMismatchedLinkedOpenPR', () => { expect(shouldClearBranchMismatchedLinkedOpenPR(baseArgs)).toBe(true) }) + it('clears when the mismatched linked PR is a draft', () => { + expect( + shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, pr: basePR({ state: 'draft' }) }) + ).toBe(true) + }) + it('strips refs/heads/ from the worktree branch before comparing', () => { expect( shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, branch: 'refs/heads/feature-one' }) @@ -6582,7 +6869,7 @@ describe('shouldClearBranchMismatchedLinkedOpenPR', () => { ) }) - it('only applies to open PRs', () => { + it('only applies to active PRs', () => { expect( shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, pr: basePR({ state: 'merged' }) }) ).toBe(false) @@ -6600,6 +6887,9 @@ describe('shouldClearBranchMismatchedLinkedOpenPR', () => { ).toBe(false) // Detached HEAD reports an empty branch; there is no mismatch to act on. expect(shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, branch: '' })).toBe(false) + expect(shouldClearBranchMismatchedLinkedOpenPR({ ...baseArgs, requestHeadOid: null })).toBe( + false + ) }) it('ignores lookups that resolved a different PR than the link', () => { diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index fe3c3a70a43..953444934b8 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -994,6 +994,45 @@ function findWorktreeById(state: AppState, worktreeId: string): Worktree | null return null } +function findUniqueWorktreeById( + state: AppState, + worktreeId: string, + executionHostId?: string +): Worktree | null { + let match: Worktree | null = null + for (const worktrees of Object.values(state.worktreesByRepo)) { + for (const worktree of worktrees) { + if (worktree.id !== worktreeId) { + continue + } + if (match) { + // Why: metadata persistence is keyed only by worktree id. If two hosts + // own that id, a destructive linked-PR clear cannot be routed safely. + return null + } + match = worktree + } + } + if (!match || executionHostId === undefined) { + return match + } + const expectedHostId = normalizeExecutionHostId(executionHostId) ?? LOCAL_EXECUTION_HOST_ID + const explicitWorktreeHostId = normalizeExecutionHostId(match.hostId) + if (explicitWorktreeHostId) { + return explicitWorktreeHostId === expectedHostId ? match : null + } + const repoHostIds = new Set( + state.repos + .filter((repo) => repo.id === match.repoId) + .map((repo) => getRepoExecutionHostId(repo)) + ) + // Pre-host persisted rows are safe only when their repo has one unambiguous owner. + if (repoHostIds.size !== 1 || !repoHostIds.has(expectedHostId)) { + return null + } + return match +} + function isStaleExactLinkedPRLookup( state: AppState, worktreeId: string | undefined, @@ -1044,8 +1083,8 @@ function shouldApplyDivergedLinkedPRClear(args: { } // Why: a linked PR is a branch-scoped hint. When a lookup returns the linked -// OPEN PR while the worktree sits on a different branch — and neither the push -// target nor the worktree HEAD still points at the PR head — the link is stale +// open or draft PR while the worktree sits on a different branch — and neither +// the push target nor the worktree HEAD still points at the PR head — the link is stale // (a branch switch whose identity-path clear was missed) and would otherwise // pin Checks to the previous branch's PR on every refresh. export function shouldClearBranchMismatchedLinkedOpenPR(args: { @@ -1061,29 +1100,35 @@ export function shouldClearBranchMismatchedLinkedOpenPR(args: { return ( linkedPRNumber != null && pr?.number === linkedPRNumber && - pr.state === 'open' && + // Draft reviews are open PRs too; their distinct renderer state must not + // leave the same stale durable link permanently wedged after a branch switch. + (pr.state === 'open' || pr.state === 'draft') && + requestHeadOid !== null && headRefName !== '' && currentBranch !== '' && headRefName !== currentBranch && (pushTargetBranch === null || pushTargetBranch !== headRefName) && // A worktree parked on the PR's own head commit is the same line of work // (e.g. a PR checkout under a renamed local branch); keep the link. - !(pr.headSha != null && requestHeadOid !== null && pr.headSha === requestHeadOid) + !(pr.headSha != null && pr.headSha === requestHeadOid) ) } function shouldApplyBranchMismatchedLinkedPRClear(args: { - worktree: Pick | undefined + worktree: Pick | undefined linkedPRNumber: number branch: string + requestHeadOid: string | null }): boolean { - const { worktree, linkedPRNumber, branch } = args + const { worktree, linkedPRNumber, branch, requestHeadOid } = args return ( Boolean(worktree) && + requestHeadOid !== null && worktree?.linkedPR === linkedPRNumber && // Branch-scoped: only clear while the worktree is still on the branch the // mismatch was computed against; a newer switch gets its own validation. worktree.branch.replace(/^refs\/heads\//, '') === branch.replace(/^refs\/heads\//, '') && + worktree.head === requestHeadOid && worktree.isBare !== true && worktree.isArchived !== true ) @@ -1361,17 +1406,23 @@ function shouldPreserveExistingPRForFallbackMiss(args: { fallbackPRNumber?: number | null fallbackPRSource?: GitHubPRFallbackSource | null }): boolean { + if ( + args.nextPR !== null || + args.linkedPRNumber != null || + args.currentPR?.state !== 'merged' || + typeof args.currentPR.headSha !== 'string' || + args.currentPR.headSha.length === 0 + ) { + return false + } + // Why: the common found/non-merged paths do not depend on worktree state. + // Gate the global lookup so batched refresh aliases do not multiply full scans. const worktree = args.worktreeId ? findWorktreeById(args.state, args.worktreeId) : null const worktreeHead = worktree?.head // Why: merged branch PRs are only safe to keep when cached PR metadata still // matches the commit this stored worktree is actually on — exactly, or via a // head confirmed to be part of the merged PR. const preservesMergedPRForCurrentHead = - args.nextPR === null && - args.linkedPRNumber == null && - args.currentPR?.state === 'merged' && - typeof args.currentPR.headSha === 'string' && - args.currentPR.headSha.length > 0 && typeof worktreeHead === 'string' && worktreeHead.length > 0 && (args.currentPR.headSha === worktreeHead || @@ -3179,8 +3230,17 @@ export const createGitHubSlice: StateCreator = (s if (didUpdatePRCache) { debouncedSaveCache(get()) } + const linkedPRWorktree = + options?.worktreeId && linkedPRNumber != null + ? findUniqueWorktreeById( + get(), + options.worktreeId, + repo ? getRepoExecutionHostId(repo) : LOCAL_EXECUTION_HOST_ID + ) + : null if ( options?.worktreeId && + linkedPRWorktree && linkedPRNumber != null && shouldClearDivergedLinkedMergedPR({ pr, linkedPRNumber, requestHeadOid }) ) { @@ -3190,9 +3250,14 @@ export const createGitHubSlice: StateCreator = (s options.worktreeId, { linkedPR: null }, { - shouldApply: (worktree) => + shouldApply: () => shouldApplyDivergedLinkedPRClear({ - worktree, + worktree: + findUniqueWorktreeById( + get(), + options.worktreeId!, + repo ? getRepoExecutionHostId(repo) : LOCAL_EXECUTION_HOST_ID + ) ?? undefined, linkedPRNumber, branch, requestHeadOid @@ -3202,22 +3267,32 @@ export const createGitHubSlice: StateCreator = (s } if ( options?.worktreeId && + linkedPRWorktree && linkedPRNumber != null && shouldClearBranchMismatchedLinkedOpenPR({ pr, linkedPRNumber, branch, requestHeadOid, - pushTargetBranch: - findWorktreeById(get(), options.worktreeId)?.pushTarget?.branchName ?? null + pushTargetBranch: linkedPRWorktree.pushTarget?.branchName ?? null }) ) { void get().updateWorktreeMeta( options.worktreeId, { linkedPR: null }, { - shouldApply: (worktree) => - shouldApplyBranchMismatchedLinkedPRClear({ worktree, linkedPRNumber, branch }) + shouldApply: () => + shouldApplyBranchMismatchedLinkedPRClear({ + worktree: + findUniqueWorktreeById( + get(), + options.worktreeId!, + repo ? getRepoExecutionHostId(repo) : LOCAL_EXECUTION_HOST_ID + ) ?? undefined, + linkedPRNumber, + branch, + requestHeadOid + }) } ) // Re-resolve by branch right away so visible Checks recover on this @@ -3955,48 +4030,15 @@ export const createGitHubSlice: StateCreator = (s linkedPRNumber: number branch: string requestHeadOid: string | null + executionHostId: string }[] = [] const branchMismatchedLinkedPRClears: { worktreeId: string linkedPRNumber: number branch: string + requestHeadOid: string | null + executionHostId: string }[] = [] - if (event.outcome?.kind === 'found') { - const pr = event.outcome.pr - for (const alias of event.aliases) { - const linkedPRNumber = alias.linkedPRNumber ?? null - const requestHeadOid = alias.currentHeadOid ?? null - if (!alias.worktreeId || linkedPRNumber == null) { - continue - } - if (shouldClearDivergedLinkedMergedPR({ pr, linkedPRNumber, requestHeadOid })) { - divergedLinkedPRClears.push({ - worktreeId: alias.worktreeId, - linkedPRNumber, - branch: alias.branch, - requestHeadOid - }) - } else if ( - shouldClearBranchMismatchedLinkedOpenPR({ - pr, - linkedPRNumber, - branch: alias.branch, - requestHeadOid, - pushTargetBranch: - findWorktreeById(get(), alias.worktreeId)?.pushTarget?.branchName ?? null - }) - ) { - // Why: the background PR coordinator also refreshes with the durable - // link; without this clear a stale-linked worktree never re-resolves - // by branch on its own. - branchMismatchedLinkedPRClears.push({ - worktreeId: alias.worktreeId, - linkedPRNumber, - branch: alias.branch - }) - } - } - } let didUpdatePRCache = false set((s) => { const nextSequences = { ...s.prRefreshSequences } @@ -4116,6 +4158,51 @@ export const createGitHubSlice: StateCreator = (s if (isStaleExactLinkedPRLookup(s, alias.worktreeId, alias.linkedPRNumber)) { continue } + if (event.outcome.kind === 'found' && alias.worktreeId) { + const linkedPRNumber = alias.linkedPRNumber ?? null + const requestHeadOid = alias.currentHeadOid ?? null + const worktree = + linkedPRNumber != null + ? findUniqueWorktreeById(s, alias.worktreeId, aliasExecutionHostId) + : null + // Why: only an event that won the sequence gate above owns metadata + // side effects; rejected late outcomes must not unlink a newer PR. + if ( + worktree && + linkedPRNumber != null && + shouldClearDivergedLinkedMergedPR({ + pr: event.outcome.pr, + linkedPRNumber, + requestHeadOid + }) + ) { + divergedLinkedPRClears.push({ + worktreeId: alias.worktreeId, + linkedPRNumber, + branch: alias.branch, + requestHeadOid, + executionHostId: aliasExecutionHostId + }) + } else if ( + worktree && + linkedPRNumber != null && + shouldClearBranchMismatchedLinkedOpenPR({ + pr: event.outcome.pr, + linkedPRNumber, + branch: alias.branch, + requestHeadOid, + pushTargetBranch: worktree.pushTarget?.branchName ?? null + }) + ) { + branchMismatchedLinkedPRClears.push({ + worktreeId: alias.worktreeId, + linkedPRNumber, + branch: alias.branch, + requestHeadOid, + executionHostId: aliasExecutionHostId + }) + } + } const nextCaches = applyGitHubPRResultToCaches({ prCache: nextPRCache, hostedReviewCache: nextHostedReviewCache, @@ -4198,9 +4285,10 @@ export const createGitHubSlice: StateCreator = (s clear.worktreeId, { linkedPR: null }, { - shouldApply: (worktree) => + shouldApply: () => shouldApplyDivergedLinkedPRClear({ - worktree, + worktree: + findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined, linkedPRNumber: clear.linkedPRNumber, branch: clear.branch, requestHeadOid: clear.requestHeadOid @@ -4213,11 +4301,13 @@ export const createGitHubSlice: StateCreator = (s clear.worktreeId, { linkedPR: null }, { - shouldApply: (worktree) => + shouldApply: () => shouldApplyBranchMismatchedLinkedPRClear({ - worktree, + worktree: + findUniqueWorktreeById(get(), clear.worktreeId, clear.executionHostId) ?? undefined, linkedPRNumber: clear.linkedPRNumber, - branch: clear.branch + branch: clear.branch, + requestHeadOid: clear.requestHeadOid }) } ) diff --git a/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts b/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts index ac474870979..0edf89bfb7b 100644 --- a/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts +++ b/src/renderer/src/store/slices/worktree-listing-branch-switch.test.ts @@ -27,14 +27,24 @@ function makeWorktree(overrides: Partial & { id: string }): Worktree { } const hasLinkedPR = (worktree: Worktree): boolean => worktree.linkedPR != null +const matchesAnyHost = (): boolean => true describe('routeListingBranchSwitchesThroughGitIdentity', () => { it('routes a listing-observed branch switch through updateWorktreeGitIdentity', () => { const updateWorktreeGitIdentity = vi.fn() + const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })] routeListingBranchSwitchesThroughGitIdentity({ - current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })], - incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }], + requestStarted: current, + current, + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-two', + head: 'def456' + }) + ], + matchesRefreshHost: matchesAnyHost, hasBranchScopedReviewContext: hasLinkedPR, updateWorktreeGitIdentity }) @@ -48,10 +58,19 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { it('does nothing when the branch is unchanged', () => { const updateWorktreeGitIdentity = vi.fn() + const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })] routeListingBranchSwitchesThroughGitIdentity({ - current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })], - incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-one', head: 'def456' }], + requestStarted: current, + current, + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-one', + head: 'def456' + }) + ], + matchesRefreshHost: matchesAnyHost, hasBranchScopedReviewContext: hasLinkedPR, updateWorktreeGitIdentity }) @@ -61,10 +80,19 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { it('skips entries without branch-scoped review context (stale-refetch protection)', () => { const updateWorktreeGitIdentity = vi.fn() + const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: null })] routeListingBranchSwitchesThroughGitIdentity({ - current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: null })], - incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }], + requestStarted: current, + current, + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-two', + head: 'def456' + }) + ], + matchesRefreshHost: matchesAnyHost, hasBranchScopedReviewContext: hasLinkedPR, updateWorktreeGitIdentity }) @@ -74,10 +102,19 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { it('skips incoming worktrees with no current entry (cold hydration)', () => { const updateWorktreeGitIdentity = vi.fn() + const current = [makeWorktree({ id: 'repo1::/path/other', linkedPR: 101 })] routeListingBranchSwitchesThroughGitIdentity({ - current: [makeWorktree({ id: 'repo1::/path/other', linkedPR: 101 })], - incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }], + requestStarted: current, + current, + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-two', + head: 'def456' + }) + ], + matchesRefreshHost: matchesAnyHost, hasBranchScopedReviewContext: hasLinkedPR, updateWorktreeGitIdentity }) @@ -89,8 +126,16 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { const updateWorktreeGitIdentity = vi.fn() routeListingBranchSwitchesThroughGitIdentity({ + requestStarted: undefined, current: undefined, - incoming: [{ id: 'repo1::/path/wt1', branch: 'refs/heads/feature-two', head: 'def456' }], + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-two', + head: 'def456' + }) + ], + matchesRefreshHost: matchesAnyHost, hasBranchScopedReviewContext: hasLinkedPR, updateWorktreeGitIdentity }) @@ -100,10 +145,13 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { it('maps an empty listing branch to the explicit detached-HEAD signal', () => { const updateWorktreeGitIdentity = vi.fn() + const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })] routeListingBranchSwitchesThroughGitIdentity({ - current: [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })], - incoming: [{ id: 'repo1::/path/wt1', branch: '', head: 'def456' }], + requestStarted: current, + current, + incoming: [makeWorktree({ id: 'repo1::/path/wt1', branch: '', head: 'def456' })], + matchesRefreshHost: matchesAnyHost, hasBranchScopedReviewContext: hasLinkedPR, updateWorktreeGitIdentity }) @@ -113,4 +161,114 @@ describe('routeListingBranchSwitchesThroughGitIdentity', () => { branch: null }) }) + + it('rejects a listing response when the branch changed again after the request started', () => { + const updateWorktreeGitIdentity = vi.fn() + const current = [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-three', + head: 'newest-head', + linkedPR: 303 + }) + ] + + const reconciled = routeListingBranchSwitchesThroughGitIdentity({ + requestStarted: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-one', + linkedPR: 101 + }) + ], + current, + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-two', + head: 'stale-head' + }) + ], + matchesRefreshHost: matchesAnyHost, + hasBranchScopedReviewContext: hasLinkedPR, + updateWorktreeGitIdentity + }) + + expect(updateWorktreeGitIdentity).not.toHaveBeenCalled() + expect(reconciled).toEqual(current) + }) + + it('preserves a manual relink made while the listing request was in flight', () => { + const updateWorktreeGitIdentity = vi.fn() + const requestStarted = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 101 })] + const current = [makeWorktree({ id: 'repo1::/path/wt1', linkedPR: 303 })] + + const reconciled = routeListingBranchSwitchesThroughGitIdentity({ + requestStarted, + current, + incoming: [ + makeWorktree({ + id: 'repo1::/path/wt1', + branch: 'refs/heads/feature-two', + head: 'stale-head', + linkedPR: 101 + }) + ], + matchesRefreshHost: matchesAnyHost, + hasBranchScopedReviewContext: hasLinkedPR, + updateWorktreeGitIdentity + }) + + expect(updateWorktreeGitIdentity).not.toHaveBeenCalled() + expect(reconciled).toEqual(current) + }) + + it('fails closed when the same worktree id belongs to multiple execution hosts', () => { + const updateWorktreeGitIdentity = vi.fn() + const requestStarted = [ + makeWorktree({ id: 'repo1::/same/path', hostId: 'local', linkedPR: 101 }), + makeWorktree({ id: 'repo1::/same/path', hostId: 'ssh:ssh-1', linkedPR: 202 }) + ] + + routeListingBranchSwitchesThroughGitIdentity({ + requestStarted, + current: requestStarted, + incoming: [ + makeWorktree({ + id: 'repo1::/same/path', + branch: 'refs/heads/feature-two', + head: 'remote-head' + }) + ], + matchesRefreshHost: matchesAnyHost, + hasBranchScopedReviewContext: hasLinkedPR, + updateWorktreeGitIdentity + }) + + expect(updateWorktreeGitIdentity).not.toHaveBeenCalled() + }) + + it('does not route an old-host response through a row now owned by another host', () => { + const updateWorktreeGitIdentity = vi.fn() + + routeListingBranchSwitchesThroughGitIdentity({ + requestStarted: [ + makeWorktree({ id: 'repo1::/same/path', hostId: 'ssh:ssh-1', linkedPR: 101 }) + ], + current: [makeWorktree({ id: 'repo1::/same/path', hostId: 'ssh:ssh-2', linkedPR: 202 })], + incoming: [ + makeWorktree({ + id: 'repo1::/same/path', + hostId: 'ssh:ssh-1', + branch: 'refs/heads/feature-two', + head: 'old-host-head' + }) + ], + matchesRefreshHost: (worktree) => worktree.hostId === 'ssh:ssh-1', + hasBranchScopedReviewContext: hasLinkedPR, + updateWorktreeGitIdentity + }) + + expect(updateWorktreeGitIdentity).not.toHaveBeenCalled() + }) }) diff --git a/src/renderer/src/store/slices/worktree-listing-branch-switch.ts b/src/renderer/src/store/slices/worktree-listing-branch-switch.ts index 01d76a248b2..3782463668c 100644 --- a/src/renderer/src/store/slices/worktree-listing-branch-switch.ts +++ b/src/renderer/src/store/slices/worktree-listing-branch-switch.ts @@ -1,5 +1,31 @@ import type { Worktree } from '../../../../shared/types' +function indexUnambiguousWorktrees( + worktrees: readonly Worktree[], + include?: (worktree: Worktree) => boolean +): Map { + const byId = new Map() + for (const worktree of worktrees) { + if (include && !include(worktree)) { + continue + } + byId.set(worktree.id, byId.has(worktree.id) ? null : worktree) + } + return byId +} + +function branchScopedReviewContextMatches(left: Worktree, right: Worktree): boolean { + return ( + left.linkedPR === right.linkedPR && + left.linkedGitLabMR === right.linkedGitLabMR && + left.linkedBitbucketPR === right.linkedBitbucketPR && + left.linkedAzureDevOpsPR === right.linkedAzureDevOpsPR && + left.linkedGiteaPR === right.linkedGiteaPR && + left.pushTarget?.remoteName === right.pushTarget?.remoteName && + left.pushTarget?.branchName === right.pushTarget?.branchName + ) +} + /** * Route branch switches observed by a worktree-listing refresh through * `updateWorktreeGitIdentity` before the listing is merged into the store. @@ -18,22 +44,52 @@ import type { Worktree } from '../../../../shared/types' * listing row must not roll back a newer branch identity. */ export function routeListingBranchSwitchesThroughGitIdentity(args: { + requestStarted: readonly Worktree[] | undefined current: readonly Worktree[] | undefined - incoming: readonly Pick[] + incoming: Worktree[] + matchesRefreshHost: (worktree: Worktree) => boolean hasBranchScopedReviewContext: (worktree: Worktree) => boolean updateWorktreeGitIdentity: ( worktreeId: string, identity: { head?: string; branch?: string | null } ) => void -}): void { - const { current, incoming, hasBranchScopedReviewContext, updateWorktreeGitIdentity } = args - if (!current?.length) { - return +}): Worktree[] { + const { + requestStarted, + current, + incoming, + matchesRefreshHost, + hasBranchScopedReviewContext, + updateWorktreeGitIdentity + } = args + if (!requestStarted?.length || !current?.length) { + return incoming } - const currentById = new Map(current.map((worktree) => [worktree.id, worktree])) - for (const worktree of incoming) { - const existing = currentById.get(worktree.id) + + const allLatestById = indexUnambiguousWorktrees(current) + const startedById = indexUnambiguousWorktrees(requestStarted, matchesRefreshHost) + const latestById = indexUnambiguousWorktrees(current, matchesRefreshHost) + let reconciled: Worktree[] | null = null + for (const [index, worktree] of incoming.entries()) { + const requestStartedWorktree = startedById.get(worktree.id) + const existing = latestById.get(worktree.id) + const isAmbiguousAcrossHosts = allLatestById.get(worktree.id) === null if ( + requestStartedWorktree && + existing && + (existing.branch !== requestStartedWorktree.branch || + existing.head !== requestStartedWorktree.head || + !branchScopedReviewContextMatches(existing, requestStartedWorktree)) + ) { + // Why: rejecting the clear alone is insufficient; preserve the newer row + // so the subsequent listing merge cannot roll its branch and metadata back. + reconciled ??= [...incoming] + reconciled[index] = existing + continue + } + if ( + isAmbiguousAcrossHosts || + !requestStartedWorktree || !existing || existing.branch === worktree.branch || !hasBranchScopedReviewContext(existing) @@ -47,4 +103,5 @@ export function routeListingBranchSwitchesThroughGitIdentity(args: { branch: worktree.branch === '' ? null : worktree.branch }) } + return reconciled ?? incoming } diff --git a/src/renderer/src/store/slices/worktrees.test.ts b/src/renderer/src/store/slices/worktrees.test.ts index dbbd869ff60..d22c15877c8 100644 --- a/src/renderer/src/store/slices/worktrees.test.ts +++ b/src/renderer/src/store/slices/worktrees.test.ts @@ -450,6 +450,47 @@ describe('fetchWorktrees', () => { }) }) + it('does not merge a stale listing row over a newer branch and review link', async () => { + const store = createTestStore() + const worktreeId = 'repo1::/path/wt1' + const requestStarted = makeWorktree({ + id: worktreeId, + repoId: 'repo1', + path: '/path/wt1', + branch: 'refs/heads/feature-one', + head: 'first-head', + linkedPR: 101 + }) + const staleResponse = makeWorktree({ + ...requestStarted, + branch: 'refs/heads/feature-two', + head: 'stale-head', + linkedPR: 202 + }) + let resolveListing!: (worktrees: Worktree[]) => void + const listing = new Promise((resolve) => { + resolveListing = resolve + }) + worktreeListMock.mockReturnValueOnce(listing) + store.setState({ worktreesByRepo: { repo1: [requestStarted] } } as Partial) + + const refresh = store.getState().fetchWorktrees('repo1') + await vi.waitFor(() => expect(worktreeListMock).toHaveBeenCalledTimes(1)) + const latest = makeWorktree({ + ...requestStarted, + branch: 'refs/heads/feature-three', + head: 'latest-head', + linkedPR: 303 + }) + store.setState({ worktreesByRepo: { repo1: [latest] } } as Partial) + resolveListing([staleResponse]) + + await refresh + + expect(store.getState().worktreesByRepo.repo1).toEqual([latest]) + expect(mockApi.worktrees.updateMeta).not.toHaveBeenCalled() + }) + it('updates the repo entry when only the persisted base ref changes', async () => { const store = createTestStore() const existing = makeWorktree({ diff --git a/src/renderer/src/store/slices/worktrees.ts b/src/renderer/src/store/slices/worktrees.ts index 9c68265a7ce..d9cb0d09afe 100644 --- a/src/renderer/src/store/slices/worktrees.ts +++ b/src/renderer/src/store/slices/worktrees.ts @@ -2327,6 +2327,7 @@ export const createWorktreeSlice: StateCreator fetchWorktrees: async (repoId, options) => { try { const ownerState = get() + const requestStartedWorktrees = ownerState.worktreesByRepo[repoId] const hostId = repoHostId(ownerState, repoId) const ownerWasMissingAtStart = !ownerState.repos.some((repo) => repo.id === repoId) const setup = getProjectHostSetupForRepoHost(ownerState, repoId, hostId) @@ -2338,13 +2339,19 @@ export const createWorktreeSlice: StateCreator if (options?.requireAuthoritative && !detected.authoritative) { return false } - const incoming = toVisibleWorktrees(detected, hostId, setup) - routeListingBranchSwitchesThroughGitIdentity({ - current: get().worktreesByRepo[repoId], - incoming, - hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext, - updateWorktreeGitIdentity: get().updateWorktreeGitIdentity - }) + let incoming = toVisibleWorktrees(detected, hostId, setup) + const latestState = get() + if (repoHasExecutionHost(latestState, repoId, hostId, ownerWasMissingAtStart)) { + const matchOptions = worktreeHostMatchOptions(latestState, repoId, hostId) + incoming = routeListingBranchSwitchesThroughGitIdentity({ + requestStarted: requestStartedWorktrees, + current: latestState.worktreesByRepo[repoId], + incoming, + matchesRefreshHost: (worktree) => worktreeMatchesHost(worktree, hostId, matchOptions), + hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext, + updateWorktreeGitIdentity: latestState.updateWorktreeGitIdentity + }) + } const current = get().worktreesByRepo[repoId] const worktrees = sanitizeHostedReviewLinksForBranchClears(incoming, current) const currentMatchOptions = worktreeHostMatchOptions(get(), repoId, hostId) @@ -2482,20 +2489,28 @@ export const createWorktreeSlice: StateCreator if (get().hasHydratedWorktreePurge) { await mapReposForWorktreeRefresh(repos, async (r) => { try { + const requestStartedState = get() + const requestStartedWorktrees = requestStartedState.worktreesByRepo[r.id] const hostId = getRepoExecutionHostId(r) - const setup = getProjectHostSetupForRepoHost(get(), r.id, hostId) - const settings = settingsForKnownRepoOwner(get().settings, r) + const setup = getProjectHostSetupForRepoHost(requestStartedState, r.id, hostId) + const settings = settingsForKnownRepoOwner(requestStartedState.settings, r) const detected = await listDetectedWorktreesForRepoCoalesced(settings, r.id, { executionHostId: hostId, reuseRecentCompatibilityFailure: true }) - const incoming = toVisibleWorktrees(detected, hostId, setup) - routeListingBranchSwitchesThroughGitIdentity({ - current: get().worktreesByRepo[r.id], - incoming, - hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext, - updateWorktreeGitIdentity: get().updateWorktreeGitIdentity - }) + let incoming = toVisibleWorktrees(detected, hostId, setup) + const latestState = get() + if (repoHasExecutionHost(latestState, r.id, hostId, false)) { + const matchOptions = worktreeHostMatchOptions(latestState, r.id, hostId) + incoming = routeListingBranchSwitchesThroughGitIdentity({ + requestStarted: requestStartedWorktrees, + current: latestState.worktreesByRepo[r.id], + incoming, + matchesRefreshHost: (worktree) => worktreeMatchesHost(worktree, hostId, matchOptions), + hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext, + updateWorktreeGitIdentity: latestState.updateWorktreeGitIdentity + }) + } const worktrees = sanitizeHostedReviewLinksForBranchClears( incoming, get().worktreesByRepo[r.id] @@ -2568,20 +2583,28 @@ export const createWorktreeSlice: StateCreator | { repoId: string; ok: false } > => { try { + const requestStartedState = get() + const requestStartedWorktrees = requestStartedState.worktreesByRepo[r.id] const hostId = getRepoExecutionHostId(r) - const setup = getProjectHostSetupForRepoHost(get(), r.id, hostId) + const setup = getProjectHostSetupForRepoHost(requestStartedState, r.id, hostId) const detected = await listDetectedWorktreesForRepoCoalesced( - settingsForKnownRepoOwner(get().settings, r), + settingsForKnownRepoOwner(requestStartedState.settings, r), r.id, { executionHostId: hostId, reuseRecentCompatibilityFailure: true } ) - const incoming = toVisibleWorktrees(detected, hostId, setup) - routeListingBranchSwitchesThroughGitIdentity({ - current: get().worktreesByRepo[r.id], - incoming, - hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext, - updateWorktreeGitIdentity: get().updateWorktreeGitIdentity - }) + let incoming = toVisibleWorktrees(detected, hostId, setup) + const latestState = get() + if (repoHasExecutionHost(latestState, r.id, hostId, false)) { + const matchOptions = worktreeHostMatchOptions(latestState, r.id, hostId) + incoming = routeListingBranchSwitchesThroughGitIdentity({ + requestStarted: requestStartedWorktrees, + current: latestState.worktreesByRepo[r.id], + incoming, + matchesRefreshHost: (worktree) => worktreeMatchesHost(worktree, hostId, matchOptions), + hasBranchScopedReviewContext: hasBranchScopedHostedReviewContext, + updateWorktreeGitIdentity: latestState.updateWorktreeGitIdentity + }) + } const current = get().worktreesByRepo[r.id] const list = sanitizeHostedReviewLinksForBranchClears(incoming, current) const currentMatchOptions = worktreeHostMatchOptions(get(), r.id, hostId)