From cf1554351b56ce0b7a2ced5f38544565a52c494a Mon Sep 17 00:00:00 2001 From: Brennan Benson <79079362+brennanb2025@users.noreply.github.com> Date: Wed, 15 Jul 2026 18:08:59 -0700 Subject: [PATCH] fix(github): keep paired-host refresh owner consistent --- ...ithub-pr-refresh-owner-consistency.test.ts | 250 ++++++++++++++++++ src/renderer/src/store/slices/github.ts | 76 ++++-- 2 files changed, 304 insertions(+), 22 deletions(-) create mode 100644 src/renderer/src/store/slices/github-pr-refresh-owner-consistency.test.ts diff --git a/src/renderer/src/store/slices/github-pr-refresh-owner-consistency.test.ts b/src/renderer/src/store/slices/github-pr-refresh-owner-consistency.test.ts new file mode 100644 index 00000000000..a69b388a7e6 --- /dev/null +++ b/src/renderer/src/store/slices/github-pr-refresh-owner-consistency.test.ts @@ -0,0 +1,250 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { create } from 'zustand' +import { createGitHubSlice } from './github' +import { createHostedReviewSlice } from './hosted-review' +import type { AppState } from '../types' +import type { PRInfo, Repo, Worktree } from '../../../../shared/types' +import { + createCompatibleRuntimeStatusResponseIfNeeded, + type RuntimeEnvironmentCallRequest +} from '../../runtime/runtime-compatibility-test-fixture' +import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' + +const runtimeEnvironmentCall = vi.fn() +const runtimeEnvironmentTransportCall = vi.fn() +const enqueuePRRefresh = vi.fn().mockResolvedValue(undefined) + +const mockApi = { + gh: { + prForBranch: vi.fn().mockResolvedValue(null), + refreshPRNow: vi.fn().mockResolvedValue({ kind: 'no-pr', fetchedAt: 1 }), + enqueuePRRefresh, + issue: vi.fn().mockResolvedValue(null) + }, + hostedReview: { forBranch: vi.fn().mockResolvedValue(null) }, + runtimeEnvironments: { call: runtimeEnvironmentTransportCall }, + cache: { + getGitHub: vi.fn().mockResolvedValue(null), + setGitHub: vi.fn().mockResolvedValue(undefined) + } +} + +// @ts-expect-error test window mock +globalThis.window = { api: mockApi } + +function createTestStore() { + return create()( + (...a) => + ({ + ...createGitHubSlice(...a), + ...createHostedReviewSlice(...a) + }) as AppState + ) +} + +function makePR(overrides: Partial = {}): PRInfo { + return { + number: 12, + title: 'Test PR', + state: 'open', + url: 'https://example.com/pr/12', + checksStatus: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN', + headSha: 'head-oid', + ...overrides + } +} + +function makeRepo(overrides: Partial & Pick): Repo { + return { + displayName: overrides.id, + badgeColor: 'blue', + addedAt: 1, + kind: 'git', + ...overrides + } +} + +function makeRuntimeWorktree(repoId: string, branch: string): Worktree { + return { + id: 'wt-runtime', + repoId, + path: '/runtime/repo/worktrees/feature', + head: 'head-oid', + branch, + displayName: branch, + comment: '', + linkedIssue: null, + linkedPR: null, + linkedLinearIssue: null, + linkedLinearIssueWorkspaceId: null, + linkedLinearIssueOrganizationUrlKey: null, + isMainWorktree: false, + isBare: false, + isArchived: false, + isUnread: false, + isPinned: false, + sortOrder: 1, + lastActivityAt: 1, + hostId: 'runtime:env-1' + } +} + +function seed( + store: ReturnType, + state: Pick & Partial +): void { + store.setState({ + settings: { activeRuntimeEnvironmentId: null } as AppState['settings'], + groupBy: 'pr-status', + worktreeCardProperties: ['status'], + prCache: {}, + issueCache: {}, + hostedReviewCache: {}, + commentsCache: {}, + sshConnectionStates: new Map(), + ...state + } as unknown as Partial) +} + +describe('GitHub PR refresh owner consistency', () => { + beforeEach(() => { + vi.clearAllMocks() + clearRuntimeCompatibilityCacheForTests() + runtimeEnvironmentTransportCall.mockImplementation((args: RuntimeEnvironmentCallRequest) => { + return createCompatibleRuntimeStatusResponseIfNeeded(args) ?? runtimeEnvironmentCall(args) + }) + }) + + it('refreshes stale runtime PR data even when the same-id local cache is fresh', async () => { + runtimeEnvironmentCall.mockResolvedValueOnce({ + id: 'rpc-1', + ok: true, + result: makePR({ number: 31 }), + _meta: { runtimeId: 'remote-runtime' } + }) + const store = createTestStore() + const repoId = 'repo-paired' + const branch = 'feature/runtime-stale' + const fetchPRForBranch = store.getState().fetchPRForBranch + const fetchPRForBranchSpy = vi.fn((...args: Parameters) => + fetchPRForBranch(...args) + ) + store.setState({ fetchPRForBranch: fetchPRForBranchSpy }) + seed(store, { + repos: [ + makeRepo({ id: repoId, path: '/local/repo' }), + makeRepo({ + id: repoId, + path: '/runtime/repo', + executionHostId: 'runtime:env-1' + }) + ], + worktreesByRepo: { [repoId]: [makeRuntimeWorktree(repoId, branch)] }, + prCache: { + [`${repoId}::${branch}`]: { + data: makePR({ number: 7 }), + fetchedAt: Date.now() + } + } + }) + + store.getState().refreshAllGitHub() + + await vi.waitFor(() => expect(runtimeEnvironmentCall).toHaveBeenCalledTimes(1)) + expect(fetchPRForBranchSpy).toHaveBeenCalledWith('/runtime/repo', branch, { + repoId, + hostId: 'runtime:env-1', + worktreeId: 'wt-runtime', + linkedPRNumber: null, + fallbackPRNumber: null, + fallbackPRSource: null + }) + expect(enqueuePRRefresh).not.toHaveBeenCalled() + expect(store.getState().prCache[`runtime:env-1::${repoId}::${branch}`]?.data).toMatchObject({ + number: 31 + }) + }) + + it('does not dispatch a refresh when the owner cache is fresh', () => { + const store = createTestStore() + const repoId = 'repo-paired' + const branch = 'feature/runtime-fresh' + const fetchPRForBranchSpy = vi.fn(store.getState().fetchPRForBranch) + store.setState({ fetchPRForBranch: fetchPRForBranchSpy }) + seed(store, { + repos: [ + makeRepo({ id: repoId, path: '/local/repo' }), + makeRepo({ + id: repoId, + path: '/runtime/repo', + executionHostId: 'runtime:env-1' + }) + ], + worktreesByRepo: { [repoId]: [makeRuntimeWorktree(repoId, branch)] }, + prCache: { + [`runtime:env-1::${repoId}::${branch}`]: { + data: makePR({ number: 32 }), + fetchedAt: Date.now() + } + } + }) + + store.getState().refreshAllGitHub() + + expect(fetchPRForBranchSpy).not.toHaveBeenCalled() + expect(runtimeEnvironmentCall).not.toHaveBeenCalled() + expect(enqueuePRRefresh).not.toHaveBeenCalled() + }) + + it('drops an in-flight result when the explicit owner path changes', async () => { + let resolveRuntimeCall: (response: unknown) => void = () => {} + runtimeEnvironmentCall.mockImplementationOnce( + () => + new Promise((resolve) => { + resolveRuntimeCall = resolve + }) + ) + const store = createTestStore() + const repoId = 'repo-paired' + const branch = 'feature/owner-moved' + seed(store, { + repos: [ + makeRepo({ + id: repoId, + path: '/runtime/repo', + executionHostId: 'runtime:env-1' + }) + ], + worktreesByRepo: { [repoId]: [makeRuntimeWorktree(repoId, branch)] } + }) + + const request = store.getState().fetchPRForBranch('/runtime/repo', branch, { + force: true, + repoId, + hostId: 'runtime:env-1', + worktreeId: 'wt-runtime' + }) + await vi.waitFor(() => expect(runtimeEnvironmentCall).toHaveBeenCalledTimes(1)) + store.setState({ + repos: [ + makeRepo({ + id: repoId, + path: '/runtime/repo-moved', + executionHostId: 'runtime:env-1' + }) + ] + }) + resolveRuntimeCall({ + id: 'rpc-1', + ok: true, + result: makePR({ number: 33 }), + _meta: { runtimeId: 'remote-runtime' } + }) + + await expect(request).resolves.toBeNull() + expect(store.getState().prCache[`runtime:env-1::${repoId}::${branch}`]).toBeUndefined() + expect(store.getState().hostedReviewCache).toEqual({}) + }) +}) diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index b6b2dc0b833..3a497260f87 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -1164,18 +1164,18 @@ function shouldApplyBranchMismatchedLinkedPRClear(args: { function buildPRRefreshCandidate( state: AppState, worktree: Worktree, - repoPath?: string + ownerRepo?: Repo ): GitHubPRRefreshCandidate | null { - const repo = findWorktreeOwnerRepo(state, worktree) + const repo = ownerRepo ?? findWorktreeOwnerRepo(state, worktree) if (!repo) { return null } - if (isMacAppDataPath(repoPath ?? repo.path)) { + if (isMacAppDataPath(repo.path)) { return null } const branch = worktree.branch.replace(/^refs\/heads\//, '') const cacheKey = prCacheKey( - repoPath ?? repo.path, + repo.path, repo.id, branch, settingsForGitHubRepoOwner(state.settings, repo), @@ -1186,7 +1186,7 @@ function buildPRRefreshCandidate( const cachedPR = state.prCache[cacheKey]?.data ?? null const hostedReviewFallbackPRNumber = githubHostedReviewFallbackPRNumber( state, - repoPath ?? repo.path, + repo.path, repo.id, branch, repo.connectionId, @@ -1217,7 +1217,7 @@ function buildPRRefreshCandidate( : null return { repoId: repo.id, - repoPath: repoPath ?? repo.path, + repoPath: repo.path, repoKind: repo.kind ?? 'git', branch, cacheKey, @@ -1258,6 +1258,43 @@ function findWorktreeOwnerRepo( }) as Repo | null } +function findPRFetchRepoOwner( + state: Pick & Partial>, + repoPath: string, + repoId?: string, + hostId?: ExecutionHostId +): Repo | undefined { + const repos = state.repos ?? [] + if (hostId) { + // Why: refresh candidates carry the complete owner identity. Resolve it in + // one pass and fail closed instead of consulting focus or another same-id row. + let match: Repo | undefined + for (const candidate of repos) { + if ( + candidate.path !== repoPath || + (repoId && candidate.id !== repoId) || + getRepoExecutionHostId(candidate) !== hostId + ) { + continue + } + if (match) { + return undefined + } + match = candidate as Repo + } + return match + } + if (!repoId) { + return repos.find((candidate) => candidate.path === repoPath) as Repo | undefined + } + const reposAtPath = repos.filter((candidate) => candidate.path === repoPath) + return ( + (findRepoForHost(reposAtPath, repoId, { + settings: state.settings + }) as Repo | null) ?? undefined + ) +} + function githubHostedReviewFallbackPRNumber( state: AppState, repoPath: string, @@ -3057,20 +3094,10 @@ export const createGitHubSlice: StateCreator = (s fetchPRForBranch: async (repoPath, branch, options): Promise => { const requestState = get() - const reposAtPath = (requestState.repos ?? []).filter( - (candidate) => candidate.path === repoPath - ) - const repo = ( - options?.repoId - ? findRepoForHost(reposAtPath, options.repoId, { - hostId: options.hostId, - settings: requestState.settings - }) - : reposAtPath[0] - ) as Repo | null | undefined + const repo = findPRFetchRepoOwner(requestState, repoPath, options?.repoId, options?.hostId) // Why: an explicit refresh owner must not silently fall back to another // same-id repo if its catalog row or path disappears mid-request. - if (options?.hostId && repo?.path !== repoPath) { + if (options?.hostId && !repo) { return null } const repoId = options?.repoId ?? repo?.id @@ -3251,6 +3278,11 @@ export const createGitHubSlice: StateCreator = (s })() const pr: PRInfo | null = outcome.kind === 'found' ? outcome.pr : outcome.kind === 'no-pr' ? null : null + // Why: repo ids and cache keys survive path changes; an old response + // must not become the replacement owner's PR data. + if (options?.hostId && !findPRFetchRepoOwner(get(), repoPath, repoId, options.hostId)) { + return null + } if (outcome.kind === 'upstream-error') { return cached?.data ?? null } @@ -4432,11 +4464,10 @@ export const createGitHubSlice: StateCreator = (s for (const worktrees of Object.values(state.worktreesByRepo)) { for (const wt of worktrees) { - const repo = state.repos.find((r) => r.id === wt.repoId) + const repo = findWorktreeOwnerRepo(state, wt) if (!repo) { continue } - const branch = wt.branch.replace(/^refs\/heads\//, '') if (shouldRefreshPRs && !wt.isBare && branch) { const ownerSettings = settingsForGitHubRepoOwner(state.settings, repo) @@ -4446,11 +4477,12 @@ export const createGitHubSlice: StateCreator = (s branch, ownerSettings, repo.connectionId, - repo.executionHostId + repo.executionHostId, + true ) const prEntry = state.prCache[prKey] if (!prEntry || now - prEntry.fetchedAt >= CACHE_TTL) { - const candidate = buildPRRefreshCandidate(state, wt) + const candidate = buildPRRefreshCandidate(state, wt, repo) if (candidate) { stalePRCandidates.push({ candidate,