fix(github): keep paired-host refresh owner consistent

This commit is contained in:
Brennan Benson
2026-07-15 18:08:59 -07:00
parent 76e8e8d799
commit cf1554351b
2 changed files with 304 additions and 22 deletions
@@ -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<AppState>()(
(...a) =>
({
...createGitHubSlice(...a),
...createHostedReviewSlice(...a)
}) as AppState
)
}
function makePR(overrides: Partial<PRInfo> = {}): 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<Repo> & Pick<Repo, 'id' | 'path'>): 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<typeof createTestStore>,
state: Pick<AppState, 'repos' | 'worktreesByRepo'> & Partial<AppState>
): 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<AppState>)
}
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<typeof fetchPRForBranch>) =>
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({})
})
})
+54 -22
View File
@@ -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<AppState, 'settings'> & Partial<Pick<AppState, 'repos'>>,
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<AppState, [], [], GitHubSlice> = (s
fetchPRForBranch: async (repoPath, branch, options): Promise<PRInfo | null> => {
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<AppState, [], [], GitHubSlice> = (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<AppState, [], [], GitHubSlice> = (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<AppState, [], [], GitHubSlice> = (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,