mirror of
https://github.com/stablyai/orca.git
synced 2026-10-15 16:02:49 +00:00
Prevent stale PR refreshes from restoring unlinked review state (#5180)
- Pass `worktreeId` to `fetchPRForBranch` to track active worktree context - Ignore inflight or queued PR fetches if the worktree has been unlinked - Include linked PR/MR metadata in the checks panel snapshot key to trigger updates immediately on link/unlink events
This commit is contained in:
@@ -445,6 +445,8 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
worktreeId: activeWorktreeId,
|
||||
worktreePath: activeWorktreePath,
|
||||
branch,
|
||||
linkedGitHubPR: activeWorktree?.linkedPR ?? null,
|
||||
linkedGitLabMR: activeWorktree?.linkedGitLabMR ?? null,
|
||||
runtimeEnvironmentId,
|
||||
repoConnectionId,
|
||||
pushTarget: activeWorktreePushTarget
|
||||
@@ -948,6 +950,7 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
void fetchPRForBranch(repo.path, branch, {
|
||||
force: true,
|
||||
repoId: repo.id,
|
||||
worktreeId: activeWorktreeId ?? undefined,
|
||||
linkedPRNumber: linkedPR,
|
||||
fallbackPRNumber: fallbackGitHubPRNumber ?? pr.number
|
||||
}).finally(() => {
|
||||
@@ -1345,6 +1348,7 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
const refreshedPR = await fetchPRForBranch(repo.path, branch, {
|
||||
force: true,
|
||||
repoId: repo.id,
|
||||
worktreeId: activeWorktreeId ?? undefined,
|
||||
linkedPRNumber: linkedPR,
|
||||
fallbackPRNumber: fallbackGitHubPRNumber
|
||||
})
|
||||
@@ -1594,6 +1598,7 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
const refreshedPR = await fetchPRForBranch(repo.path, branch, {
|
||||
force: true,
|
||||
repoId: repo.id,
|
||||
worktreeId: activeWorktreeId ?? undefined,
|
||||
linkedPRNumber: linkedPR,
|
||||
fallbackPRNumber: fallbackGitHubPRNumber
|
||||
})
|
||||
@@ -1608,6 +1613,7 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
}, [
|
||||
activeGitLabReview,
|
||||
activeReview?.provider,
|
||||
activeWorktreeId,
|
||||
branch,
|
||||
fallbackGitHubPRNumber,
|
||||
fetchGitLabDetails,
|
||||
@@ -2068,6 +2074,7 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
const refreshedPR = await fetchPRForBranch(repo.path, branch, {
|
||||
force: true,
|
||||
repoId: repo.id,
|
||||
worktreeId: activeWorktreeId ?? undefined,
|
||||
linkedPRNumber
|
||||
})
|
||||
if (!isCurrentRequestContext()) {
|
||||
@@ -2175,6 +2182,7 @@ export default function ChecksPanel(): React.JSX.Element {
|
||||
}
|
||||
},
|
||||
[
|
||||
activeWorktreeId,
|
||||
branch,
|
||||
fetchHostedReviewForBranch,
|
||||
fetchPRChecks,
|
||||
|
||||
@@ -2033,6 +2033,7 @@ function SourceControlInner(): React.JSX.Element {
|
||||
fetchPRForBranch(activeRepo.path, branchName, {
|
||||
force: true,
|
||||
repoId: activeRepo.id,
|
||||
worktreeId: activeWorktreeId ?? undefined,
|
||||
linkedPRNumber: result.number
|
||||
})
|
||||
])
|
||||
|
||||
@@ -43,6 +43,32 @@ describe('buildChecksPanelGitStatusContextKey', () => {
|
||||
})
|
||||
)
|
||||
})
|
||||
|
||||
it('changes when linked hosted review metadata changes', () => {
|
||||
const base = {
|
||||
repoId: 'repo-1',
|
||||
worktreeId: 'worktree-1',
|
||||
worktreePath: 'repo-worktree',
|
||||
branch: 'feature/checks',
|
||||
runtimeEnvironmentId: 'runtime-1',
|
||||
repoConnectionId: 'ssh-1',
|
||||
pushTarget: null
|
||||
}
|
||||
|
||||
expect(
|
||||
buildChecksPanelGitStatusContextKey({
|
||||
...base,
|
||||
linkedGitHubPR: 12,
|
||||
linkedGitLabMR: null
|
||||
})
|
||||
).not.toBe(
|
||||
buildChecksPanelGitStatusContextKey({
|
||||
...base,
|
||||
linkedGitHubPR: null,
|
||||
linkedGitLabMR: null
|
||||
})
|
||||
)
|
||||
})
|
||||
})
|
||||
|
||||
describe('readChecksPanelGitStatusSnapshot', () => {
|
||||
|
||||
@@ -5,6 +5,8 @@ export type ChecksPanelGitStatusContextInput = {
|
||||
worktreeId: string | null | undefined
|
||||
worktreePath: string | null | undefined
|
||||
branch: string
|
||||
linkedGitHubPR?: number | null
|
||||
linkedGitLabMR?: number | null
|
||||
runtimeEnvironmentId: string | null
|
||||
repoConnectionId: string | null
|
||||
pushTarget: GitPushTarget | null | undefined
|
||||
@@ -29,6 +31,10 @@ export function buildChecksPanelGitStatusContextKey(
|
||||
worktreeId: input.worktreeId ?? '',
|
||||
worktreePath: input.worktreePath ?? '',
|
||||
branch: input.branch,
|
||||
// Why: this key gates right-sidebar async commits too; link/unlink must
|
||||
// make pre-change PR refreshes stale even when repo/branch are unchanged.
|
||||
linkedGitHubPR: input.linkedGitHubPR ?? null,
|
||||
linkedGitLabMR: input.linkedGitLabMR ?? null,
|
||||
runtimeEnvironmentId: input.runtimeEnvironmentId ?? '',
|
||||
repoConnectionId: input.repoConnectionId ?? '',
|
||||
pushTarget: input.pushTarget
|
||||
|
||||
@@ -1666,6 +1666,76 @@ describe('createGitHubSlice.fetchPRForBranch', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('ignores a direct exact linked PR refresh after the worktree was unlinked', async () => {
|
||||
const store = createTestStore()
|
||||
const repoPath = '/repo'
|
||||
const repoId = 'repo-1'
|
||||
const branch = 'feature/unlinked-direct-pr'
|
||||
const worktreeId = 'wt-unlinked-direct-pr'
|
||||
const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId)
|
||||
let resolveRefresh: (
|
||||
value: Awaited<ReturnType<typeof mockApi.gh.refreshPRNow>>
|
||||
) => void = () => {}
|
||||
const refresh = new Promise<Awaited<ReturnType<typeof mockApi.gh.refreshPRNow>>>((resolve) => {
|
||||
resolveRefresh = resolve
|
||||
})
|
||||
mockApi.gh.refreshPRNow.mockReturnValueOnce(refresh)
|
||||
|
||||
store.setState({
|
||||
repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }],
|
||||
worktreesByRepo: {
|
||||
[repoId]: [
|
||||
{
|
||||
id: worktreeId,
|
||||
repoId,
|
||||
path: '/repo/worktrees/unlinked-direct-pr',
|
||||
branch,
|
||||
displayName: 'unlinked-direct-pr',
|
||||
isMainWorktree: false,
|
||||
isBare: false,
|
||||
isArchived: false,
|
||||
linkedPR: 12
|
||||
}
|
||||
]
|
||||
}
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
const request = store.getState().fetchPRForBranch(repoPath, branch, {
|
||||
force: true,
|
||||
repoId,
|
||||
worktreeId,
|
||||
linkedPRNumber: 12
|
||||
})
|
||||
store.setState({
|
||||
worktreesByRepo: {
|
||||
[repoId]: [
|
||||
{
|
||||
id: worktreeId,
|
||||
repoId,
|
||||
path: '/repo/worktrees/unlinked-direct-pr',
|
||||
branch,
|
||||
displayName: 'unlinked-direct-pr',
|
||||
isMainWorktree: false,
|
||||
isBare: false,
|
||||
isArchived: false,
|
||||
linkedPR: null
|
||||
}
|
||||
]
|
||||
},
|
||||
hostedReviewCache: {},
|
||||
prCache: {}
|
||||
} as unknown as Partial<AppState>)
|
||||
resolveRefresh({
|
||||
kind: 'found',
|
||||
pr: makePR({ number: 12, title: 'Stale exact linked PR' }),
|
||||
fetchedAt: Date.now()
|
||||
})
|
||||
|
||||
await expect(request).resolves.toBeNull()
|
||||
expect(store.getState().prCache[`${repoId}::${branch}`]).toBeUndefined()
|
||||
expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toBeUndefined()
|
||||
})
|
||||
|
||||
it('preserves cached PR data when a forced coordinator refresh errors', async () => {
|
||||
const store = createTestStore()
|
||||
const repoPath = '/repo'
|
||||
@@ -2097,6 +2167,51 @@ describe('createGitHubSlice.fetchPRForBranch', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('ignores a queued exact linked PR refresh after the worktree was unlinked', () => {
|
||||
const store = createTestStore()
|
||||
const repoPath = '/repo'
|
||||
const repoId = 'repo-1'
|
||||
const branch = 'feature/unlinked-event-pr'
|
||||
const cacheKey = `${repoId}::${branch}`
|
||||
const worktreeId = 'wt-unlinked-event-pr'
|
||||
const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId)
|
||||
|
||||
store.setState({
|
||||
worktreesByRepo: {
|
||||
[repoId]: [
|
||||
{
|
||||
id: worktreeId,
|
||||
repoId,
|
||||
path: '/repo/worktrees/unlinked-event-pr',
|
||||
branch,
|
||||
displayName: 'unlinked-event-pr',
|
||||
isMainWorktree: false,
|
||||
isBare: false,
|
||||
isArchived: false,
|
||||
linkedPR: null
|
||||
}
|
||||
]
|
||||
},
|
||||
hostedReviewCache: {},
|
||||
prCache: {}
|
||||
} as unknown as Partial<AppState>)
|
||||
|
||||
store.getState().applyGitHubPRRefreshEvent({
|
||||
sequence: 1,
|
||||
aliases: [{ cacheKey, repoId, repoPath, branch, worktreeId, linkedPRNumber: 12 }],
|
||||
reason: 'visible',
|
||||
requestStartedAt: Date.now() - 1_000,
|
||||
outcome: {
|
||||
kind: 'found',
|
||||
pr: makePR({ number: 12, title: 'Stale queued linked PR' }),
|
||||
fetchedAt: Date.now()
|
||||
}
|
||||
})
|
||||
|
||||
expect(store.getState().prCache[cacheKey]).toBeUndefined()
|
||||
expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toBeUndefined()
|
||||
})
|
||||
|
||||
it('uses the in-flight event entry to allow same-millisecond coordinator refreshes', () => {
|
||||
vi.useFakeTimers()
|
||||
vi.setSystemTime(100)
|
||||
|
||||
@@ -758,6 +758,17 @@ function findWorktreeById(state: AppState, worktreeId: string): Worktree | null
|
||||
return null
|
||||
}
|
||||
|
||||
function isStaleExactLinkedPRLookup(
|
||||
state: AppState,
|
||||
worktreeId: string | undefined,
|
||||
linkedPRNumber: number | null | undefined
|
||||
): boolean {
|
||||
if (!worktreeId || linkedPRNumber == null) {
|
||||
return false
|
||||
}
|
||||
return findWorktreeById(state, worktreeId)?.linkedPR !== linkedPRNumber
|
||||
}
|
||||
|
||||
function buildPRRefreshCandidate(
|
||||
state: AppState,
|
||||
worktree: Worktree,
|
||||
@@ -1275,6 +1286,7 @@ export type GitHubSlice = {
|
||||
repoPath: string,
|
||||
branch: string,
|
||||
options?: RepoScopedFetchOptions & {
|
||||
worktreeId?: string
|
||||
linkedPRNumber?: number | null
|
||||
fallbackPRNumber?: number | null
|
||||
fallbackPRSource?: GitHubPRFallbackSource | null
|
||||
@@ -2363,6 +2375,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
repoKind: repo?.kind ?? 'git',
|
||||
branch,
|
||||
cacheKey,
|
||||
worktreeId: options?.worktreeId,
|
||||
linkedPRNumber,
|
||||
fallbackPRNumber,
|
||||
fallbackPRSource,
|
||||
@@ -2391,8 +2404,15 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
return cached?.data ?? null
|
||||
}
|
||||
if (prRequestGenerations.get(cacheKey) === generation) {
|
||||
set((s) =>
|
||||
setGitHubPRResultCaches(s, {
|
||||
let skippedStaleLinkedPRLookup = false
|
||||
set((s) => {
|
||||
// Why: unlinking a PR while an exact linked-PR lookup is in flight
|
||||
// must prevent that older result from restoring the manual link UI.
|
||||
if (isStaleExactLinkedPRLookup(s, options?.worktreeId, linkedPRNumber)) {
|
||||
skippedStaleLinkedPRLookup = true
|
||||
return {}
|
||||
}
|
||||
return setGitHubPRResultCaches(s, {
|
||||
prCacheKey: cacheKey,
|
||||
repoPath,
|
||||
branch,
|
||||
@@ -2408,7 +2428,10 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
requestStartedAt,
|
||||
requestStartedEntry: requestStartedHostedReviewEntry
|
||||
})
|
||||
)
|
||||
})
|
||||
if (skippedStaleLinkedPRLookup) {
|
||||
return null
|
||||
}
|
||||
debouncedSaveCache(get())
|
||||
}
|
||||
if (
|
||||
@@ -2945,6 +2968,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
void get().fetchPRForBranch(candidate.repoPath, candidate.branch, {
|
||||
force: bypassesGitHubPRRefreshFreshness(reason),
|
||||
repoId: candidate.repoId,
|
||||
worktreeId: candidate.worktreeId,
|
||||
linkedPRNumber: candidate.linkedPRNumber ?? null,
|
||||
fallbackPRNumber: candidate.fallbackPRNumber ?? null,
|
||||
fallbackPRSource: candidate.fallbackPRSource ?? null
|
||||
@@ -2959,6 +2983,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
return get().fetchPRForBranch(candidate.repoPath, candidate.branch, {
|
||||
force: bypassesGitHubPRRefreshFreshness(reason),
|
||||
repoId: candidate.repoId,
|
||||
worktreeId: candidate.worktreeId,
|
||||
linkedPRNumber: candidate.linkedPRNumber ?? null,
|
||||
fallbackPRNumber: candidate.fallbackPRNumber ?? null,
|
||||
fallbackPRSource: candidate.fallbackPRSource ?? null
|
||||
@@ -2984,6 +3009,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
for (const candidate of candidates) {
|
||||
void get().fetchPRForBranch(candidate.repoPath, candidate.branch, {
|
||||
repoId: candidate.repoId,
|
||||
worktreeId: candidate.worktreeId,
|
||||
linkedPRNumber: candidate.linkedPRNumber ?? null,
|
||||
fallbackPRNumber: candidate.fallbackPRNumber ?? null,
|
||||
fallbackPRSource: candidate.fallbackPRSource ?? null
|
||||
@@ -3084,6 +3110,11 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
return pr
|
||||
})()
|
||||
: null
|
||||
// Why: queued local refreshes may finish after the user unlinks an
|
||||
// exact PR; those older results must not restore the manual-link UI.
|
||||
if (isStaleExactLinkedPRLookup(s, alias.worktreeId, alias.linkedPRNumber)) {
|
||||
continue
|
||||
}
|
||||
const nextCaches = applyGitHubPRResultToCaches({
|
||||
prCache: nextPRCache,
|
||||
hostedReviewCache: nextHostedReviewCache,
|
||||
@@ -3243,6 +3274,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
if (getRuntimeRepoTarget(state, candidate.repoPath, candidateSettings)) {
|
||||
void get().fetchPRForBranch(candidate.repoPath, candidate.branch, {
|
||||
repoId: candidate.repoId,
|
||||
worktreeId: candidate.worktreeId,
|
||||
linkedPRNumber: candidate.linkedPRNumber ?? null,
|
||||
fallbackPRNumber: candidate.fallbackPRNumber ?? null,
|
||||
fallbackPRSource: candidate.fallbackPRSource ?? null
|
||||
@@ -3315,6 +3347,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
void get().fetchPRForBranch(candidate.repoPath, candidate.branch, {
|
||||
force: true,
|
||||
repoId: candidate.repoId,
|
||||
worktreeId: candidate.worktreeId,
|
||||
linkedPRNumber: candidate.linkedPRNumber ?? null,
|
||||
fallbackPRNumber: candidate.fallbackPRNumber ?? null,
|
||||
fallbackPRSource: candidate.fallbackPRSource ?? null
|
||||
@@ -3501,6 +3534,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
|
||||
void get().fetchPRForBranch(candidate.repoPath, candidate.branch, {
|
||||
force: true,
|
||||
repoId: candidate.repoId,
|
||||
worktreeId: candidate.worktreeId,
|
||||
linkedPRNumber: candidate.linkedPRNumber ?? null,
|
||||
fallbackPRNumber: candidate.fallbackPRNumber ?? null,
|
||||
fallbackPRSource: candidate.fallbackPRSource ?? null
|
||||
|
||||
@@ -2501,6 +2501,7 @@ describe('worktree remote runtime mutations', () => {
|
||||
expect(fetchPRForBranch).toHaveBeenCalledWith('/repos/orca', 'feature/pr-link', {
|
||||
force: true,
|
||||
repoId: 'repo1',
|
||||
worktreeId: wt.id,
|
||||
linkedPRNumber: null,
|
||||
fallbackPRNumber: null,
|
||||
fallbackPRSource: 'explicit'
|
||||
@@ -2546,6 +2547,7 @@ describe('worktree remote runtime mutations', () => {
|
||||
expect(fetchPRForBranch).toHaveBeenCalledWith('/repos/orca', 'feature/pr-link', {
|
||||
force: true,
|
||||
repoId: 'repo1',
|
||||
worktreeId: wt.id,
|
||||
linkedPRNumber: null,
|
||||
fallbackPRNumber: null,
|
||||
fallbackPRSource: 'explicit'
|
||||
@@ -2735,6 +2737,7 @@ describe('worktree remote runtime mutations', () => {
|
||||
expect(fetchPRForBranch).toHaveBeenCalledWith('/repos/orca', 'feature/pr-link', {
|
||||
force: true,
|
||||
repoId: 'repo1',
|
||||
worktreeId: wt.id,
|
||||
linkedPRNumber: null,
|
||||
fallbackPRNumber: null,
|
||||
fallbackPRSource: 'explicit'
|
||||
|
||||
@@ -2017,6 +2017,7 @@ export const createWorktreeSlice: StateCreator<AppState, [], [], WorktreeSlice>
|
||||
void fetchPRForBranch(repo.path, branch, {
|
||||
force: true,
|
||||
repoId: repo.id,
|
||||
worktreeId,
|
||||
linkedPRNumber: alreadyLinked ? link.number : null,
|
||||
fallbackPRNumber: null,
|
||||
fallbackPRSource: alreadyLinked ? null : 'explicit'
|
||||
|
||||
Reference in New Issue
Block a user