mirror of
https://github.com/stablyai/orca.git
synced 2026-10-31 00:57:45 +00:00
fix(checks): hide stale merged PRs after workspace HEAD changes (#7815)
* fix(checks): stop stale-while-revalidate from flashing a stale merged PR The synchronous fresh-return path guards on `staleMergedHeadRefetch` so a worktree that advanced off a merged PR's head does not reuse the now-stale merged review (added in #7277). The stale-while-revalidate fast path was missing that same guard, so it served the stale merged card synchronously (and kicked only a background refetch) whenever the parent-PR-checks overview polled with `{ staleWhileRevalidate: true, currentHeadOid }`. Result: a worktree that commits new work on top of a merged branch keeps showing the old "Merged" card instead of going branch-only — persistently while the background revalidation keeps failing (rate limit / gh error). Add `!staleMergedHeadRefetch` to the SWR condition so the read revalidates synchronously, matching the fresh-return path. Adds a regression test that fails without the guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * chore(review): clarify stale review revalidation comments * fix: avoid reusing hosted review requests from a different head * fix(checks): track concurrent review lookups by Git head * fix(hosted-review): narrow per-HEAD request fencing to a join guard Replace the HEAD-keyed in-flight map with the original single slot per request key, and skip joining an in-flight lookup only when it was started for a different known HEAD. Keep the stale-merged guard on the stale-while-revalidate path and the merged-HEAD guards on the readers. --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Neil <4138956+nwparker@users.noreply.github.com> Co-authored-by: Neil <neil@stably.ai>
This commit is contained in:
co-authored by
Claude Opus 4.8
Neil
Neil
parent
36f093a08f
commit
55ed5f8757
+5
-2
@@ -4,6 +4,7 @@ import { useNow } from '@/hooks/use-now'
|
||||
import { isFolderRepo } from '../../../../../shared/repo-kind'
|
||||
import { getGitHubPRCacheKey } from '../../../../../shared/github/github-cache-key'
|
||||
import { getHostedReviewCacheKey } from '@/store/slices/hosted-review-cache-identity'
|
||||
import { isStaleMergedGitHubReviewForHead } from '@/store/slices/hosted-review-cache-state'
|
||||
import { selectReviewCacheEntry } from '../review-cache-entry-selection'
|
||||
import { selectChecksPanelReview, type ChecksPanelReview } from '../checks-panel-review'
|
||||
import { isGitLabChecksPanelReview } from './gitlab-review-client'
|
||||
@@ -198,8 +199,10 @@ export function useChecksPanelContextState(model: ChecksPanelContextStateInput)
|
||||
}
|
||||
// Why: background PR refreshes replace the cache map; Checks only renders the entry for the active repo and branch.
|
||||
const prCacheEntry = useAppStore((s) => selectReviewCacheEntry(s.prCache, prCacheKey || null))
|
||||
const pr: PRInfo | null = prCacheEntry?.data ?? null
|
||||
const prCachedHasPR = prCacheEntry ? prCacheEntry.data !== null : null
|
||||
const pr: PRInfo | null = isStaleMergedGitHubReviewForHead(prCacheEntry, activeWorktree?.head)
|
||||
? null
|
||||
: (prCacheEntry?.data ?? null)
|
||||
const prCachedHasPR = prCacheEntry ? pr !== null : null
|
||||
const hostedReview = useAppStore((s) =>
|
||||
hostedReviewCacheKey ? (s.hostedReviewCache[hostedReviewCacheKey]?.data ?? null) : null
|
||||
)
|
||||
|
||||
@@ -115,6 +115,25 @@ function makeProjection({
|
||||
}
|
||||
|
||||
describe('buildParentPrChecksProjection', () => {
|
||||
it('drops a completed merged lookup when its worktree has moved to another head', () => {
|
||||
const repo = makeRepo()
|
||||
const worktree = makeWorktree({ id: 'repo-1::/feature', head: 'merged-head' })
|
||||
const identity = getParentPrChecksRefreshIdentity(worktree, repo, 'feature')
|
||||
const review = makeReview({ state: 'merged', headSha: 'merged-head' })
|
||||
const refreshOutcomes = new Map<string, ParentPrChecksRefreshOutcome>([
|
||||
[identity, { kind: 'found', review }]
|
||||
])
|
||||
|
||||
expect(makeProjection({ worktree, repo, refreshOutcomes }).rows[0].reviewNumber).toBe(12)
|
||||
expect(
|
||||
makeProjection({
|
||||
worktree: { ...worktree, head: 'new-work-head' },
|
||||
repo,
|
||||
refreshOutcomes
|
||||
}).rows[0].reviewNumber
|
||||
).toBeNull()
|
||||
})
|
||||
|
||||
it('classifies known review states into compact row groups', () => {
|
||||
const repo = makeRepo()
|
||||
const worktree = makeWorktree({ id: 'repo-1::/feature' })
|
||||
|
||||
@@ -13,6 +13,7 @@ import {
|
||||
linkedReviewHintKey
|
||||
} from '@/store/slices/hosted-review-cache-identity'
|
||||
import { prChecksCacheSuffix } from '@/store/github/cache-identity'
|
||||
import { isStaleMergedGitHubReviewForHead } from '@/store/slices/hosted-review-cache-state'
|
||||
import {
|
||||
PARENT_PR_CHECKS_GROUP_LABELS,
|
||||
PARENT_PR_CHECKS_GROUP_ORDER,
|
||||
@@ -158,6 +159,7 @@ function getReviewSnapshot(
|
||||
): { review: HostedReviewInfo | null | undefined; hasCacheEntry: boolean } {
|
||||
if (
|
||||
outcome?.kind === 'found' &&
|
||||
!isStaleMergedGitHubReviewForHead({ data: outcome.review }, args.worktree.head) &&
|
||||
(outcome.review.provider !== 'github' ||
|
||||
!isGitHubPRSuppressed(args.worktree, outcome.review.number))
|
||||
) {
|
||||
|
||||
+68
@@ -23,11 +23,13 @@ function makePR(number = 42): PRInfo {
|
||||
function renderState({
|
||||
pr = makePR(),
|
||||
hostedReviewEntryData = null,
|
||||
currentHeadOid,
|
||||
linkedPR = null,
|
||||
suppressedGitHubPR = null
|
||||
}: {
|
||||
pr?: PRInfo | null
|
||||
hostedReviewEntryData?: HostedReviewInfo | null
|
||||
currentHeadOid?: string | null
|
||||
linkedPR?: number | null
|
||||
suppressedGitHubPR?: number | null
|
||||
} = {}) {
|
||||
@@ -39,6 +41,7 @@ function renderState({
|
||||
branchName: 'feature',
|
||||
hostedReviewCacheKey: 'cache-key',
|
||||
hostedReviewEntryData,
|
||||
currentHeadOid,
|
||||
linkedPR,
|
||||
suppressedGitHubPR
|
||||
})
|
||||
@@ -46,6 +49,71 @@ function renderState({
|
||||
}
|
||||
|
||||
describe('useSourceControlHostedReviewState', () => {
|
||||
it.each(['queue', 'hosted'] as const)(
|
||||
'hides a merged %s review as the worktree moves to another head',
|
||||
(source) => {
|
||||
const pr = { ...makePR(), state: 'merged' as const, headSha: 'merged-head' }
|
||||
const review: HostedReviewInfo = { provider: 'github', ...pr, status: pr.checksStatus }
|
||||
const { result, rerender } = renderHook(
|
||||
({ head }) =>
|
||||
useSourceControlHostedReviewState({
|
||||
activePrFromQueue: source === 'queue' ? pr : null,
|
||||
activeRepoId: 'repo-1',
|
||||
activeWorktreeId: 'wt-1',
|
||||
branchName: 'feature',
|
||||
hostedReviewCacheKey: 'cache-key',
|
||||
hostedReviewEntryData: source === 'hosted' ? review : null,
|
||||
currentHeadOid: head,
|
||||
linkedPR: null,
|
||||
suppressedGitHubPR: null
|
||||
}),
|
||||
{ initialProps: { head: 'merged-head' } }
|
||||
)
|
||||
|
||||
expect(result.current.hostedReview?.number).toBe(42)
|
||||
rerender({ head: 'new-work-head' })
|
||||
expect(result.current.hostedReview).toBeNull()
|
||||
expect(result.current.currentPrFromQueue).toBeNull()
|
||||
rerender({ head: 'merged-head' })
|
||||
expect(result.current.hostedReview?.number).toBe(42)
|
||||
}
|
||||
)
|
||||
|
||||
it.each([undefined, null, '', ' ', 'merged-head', 'contained-head'])(
|
||||
'preserves a merged review for an unknown or owned head %s',
|
||||
(currentHeadOid) => {
|
||||
const pr = {
|
||||
...makePR(),
|
||||
state: 'merged' as const,
|
||||
headSha: 'merged-head',
|
||||
confirmedContainedHeadOid: 'contained-head'
|
||||
}
|
||||
expect(renderState({ pr, currentHeadOid }).hostedReview?.number).toBe(42)
|
||||
}
|
||||
)
|
||||
|
||||
it('keeps a valid hosted review while an older queued PR belongs to another head', () => {
|
||||
const currentReview: HostedReviewInfo = {
|
||||
provider: 'github',
|
||||
...makePR(43),
|
||||
status: 'pending',
|
||||
headSha: 'new-work-head'
|
||||
}
|
||||
const state = renderState({
|
||||
pr: { ...makePR(), state: 'merged', headSha: 'merged-head' },
|
||||
hostedReviewEntryData: currentReview,
|
||||
currentHeadOid: 'new-work-head'
|
||||
})
|
||||
|
||||
expect(state.hostedReview).toBe(currentReview)
|
||||
expect(state.currentPrFromQueue).toBeNull()
|
||||
expect(renderState({ currentHeadOid: 'new-work-head' }).hostedReview?.number).toBe(42)
|
||||
expect(
|
||||
renderState({ pr: { ...makePR(), state: 'merged' }, currentHeadOid: 'new-work-head' })
|
||||
.hostedReview?.number
|
||||
).toBe(42)
|
||||
})
|
||||
|
||||
it('uses MR header copy while eligibility loads for a GitLab SSH-over-443 remote', () => {
|
||||
const { result } = renderHook(() => {
|
||||
const state = useSourceControlHostedReviewState({
|
||||
|
||||
+20
-4
@@ -3,6 +3,7 @@ import { localizedHostedReviewCopy } from '@/i18n/hosted-review-localized-copy'
|
||||
import type { HostedReviewInfo } from '../../../../../../shared/hosted-review'
|
||||
import { isGitHubPRSuppressed } from '../../../../../../shared/worktree/github-pr-suppression'
|
||||
import { resolveHostedReviewCreationProvider } from '../../../../../../shared/hosted-review-creation-providers'
|
||||
import { isStaleMergedGitHubReviewForHead } from '@/store/slices/hosted-review-cache-state'
|
||||
import type { SourceControlWorktreeContext } from '../listing/use-worktree-context'
|
||||
import type {
|
||||
HostedReviewCreationProviderHint,
|
||||
@@ -19,6 +20,7 @@ export function useSourceControlHostedReviewState({
|
||||
activeRepoId,
|
||||
activeWorktreeId,
|
||||
branchName,
|
||||
currentHeadOid,
|
||||
hostedReviewCacheKey,
|
||||
hostedReviewEntryData,
|
||||
linkedPR,
|
||||
@@ -28,6 +30,7 @@ export function useSourceControlHostedReviewState({
|
||||
activeRepoId: string | null
|
||||
activeWorktreeId: string | null
|
||||
branchName: string
|
||||
currentHeadOid?: string | null
|
||||
hostedReviewCacheKey: string | null
|
||||
hostedReviewEntryData: SourceControlWorktreeContext['hostedReviewEntryData']
|
||||
linkedPR: number | null
|
||||
@@ -51,15 +54,27 @@ export function useSourceControlHostedReviewState({
|
||||
branchName === hostedReviewCreationState.branch
|
||||
? hostedReviewCreationState.data
|
||||
: null
|
||||
const currentPrFromQueue = isStaleMergedGitHubReviewForHead(
|
||||
{ data: activePrFromQueue },
|
||||
currentHeadOid
|
||||
)
|
||||
? null
|
||||
: activePrFromQueue
|
||||
const currentHostedReview = isStaleMergedGitHubReviewForHead(
|
||||
{ data: hostedReviewEntryData },
|
||||
currentHeadOid
|
||||
)
|
||||
? null
|
||||
: hostedReviewEntryData
|
||||
const rawHostedReview: HostedReviewInfo | null = useMemo(() => {
|
||||
if (!hostedReviewCacheKey) {
|
||||
return null
|
||||
}
|
||||
if (activePrFromQueue) {
|
||||
return { provider: 'github', ...activePrFromQueue, status: activePrFromQueue.checksStatus }
|
||||
if (currentPrFromQueue) {
|
||||
return { provider: 'github', ...currentPrFromQueue, status: currentPrFromQueue.checksStatus }
|
||||
}
|
||||
return hostedReviewEntryData
|
||||
}, [activePrFromQueue, hostedReviewCacheKey, hostedReviewEntryData])
|
||||
return currentHostedReview
|
||||
}, [currentPrFromQueue, hostedReviewCacheKey, currentHostedReview])
|
||||
const hasSuppressedGitHubPR =
|
||||
rawHostedReview?.provider === 'github' &&
|
||||
isGitHubPRSuppressed({ linkedPR, suppressedGitHubPR }, rawHostedReview.number)
|
||||
@@ -81,6 +96,7 @@ export function useSourceControlHostedReviewState({
|
||||
const hostedReviewCreateCopy = localizedHostedReviewCopy(hostedReviewCreateProvider)
|
||||
|
||||
return {
|
||||
currentPrFromQueue,
|
||||
hasSuppressedGitHubPR,
|
||||
hostedReview,
|
||||
hostedReviewCreateCopy,
|
||||
|
||||
+3
-1
@@ -44,10 +44,12 @@ export function useSourceControlReviewContext(panelState: SourceControlPanelStat
|
||||
branchName,
|
||||
hostedReviewCacheKey,
|
||||
hostedReviewEntryData,
|
||||
currentHeadOid: activeWorktree?.head,
|
||||
linkedPR: activeWorktree?.linkedPR ?? null,
|
||||
suppressedGitHubPR: activeWorktree?.suppressedGitHubPR ?? null
|
||||
})
|
||||
const {
|
||||
currentPrFromQueue,
|
||||
hasSuppressedGitHubPR,
|
||||
hostedReview,
|
||||
hostedReviewCreation,
|
||||
@@ -88,7 +90,7 @@ export function useSourceControlReviewContext(panelState: SourceControlPanelStat
|
||||
worktreePath
|
||||
})
|
||||
const linkedReviews = useSourceControlLinkedReviews({
|
||||
activePrFromQueue: hasSuppressedGitHubPR ? null : activePrFromQueue,
|
||||
activePrFromQueue: hasSuppressedGitHubPR ? null : currentPrFromQueue,
|
||||
activeRepo,
|
||||
activeWorktree,
|
||||
branchName,
|
||||
|
||||
@@ -6,6 +6,7 @@ import type {
|
||||
HostedReviewInfo
|
||||
} from '../../../../shared/hosted-review'
|
||||
import type { Repo } from '../../../../shared/repo-types'
|
||||
import type { PRInfo } from '../../../../shared/github/pull-request-types'
|
||||
import { getRepoExecutionHostId } from '../../../../shared/execution-host'
|
||||
import { ipcOrRuntimeTargetForHostId } from '@/runtime/runtime-client-target'
|
||||
import type { RuntimeClientTarget } from '../../../../shared/runtime-client-target'
|
||||
@@ -148,7 +149,7 @@ export function shouldRefetchGitHubScopedResultForNoHint(
|
||||
}
|
||||
|
||||
export function isStaleMergedGitHubReviewForHead(
|
||||
cached: HostedReviewCacheEntry<HostedReviewInfo> | undefined,
|
||||
cached: { data: HostedReviewInfo | PRInfo | null } | undefined,
|
||||
currentHeadOid: string | null | undefined
|
||||
): boolean {
|
||||
// Why: a merged GitHub PR is only shown when the worktree sits on its head
|
||||
@@ -161,7 +162,9 @@ export function isStaleMergedGitHubReviewForHead(
|
||||
}
|
||||
const data = cached?.data
|
||||
return (
|
||||
data?.provider === 'github' &&
|
||||
data !== null &&
|
||||
data !== undefined &&
|
||||
(!('provider' in data) || data.provider === 'github') &&
|
||||
data.state === 'merged' &&
|
||||
typeof data.headSha === 'string' &&
|
||||
data.headSha.length > 0 &&
|
||||
|
||||
@@ -9,6 +9,7 @@ export const inflightHostedReviewRequests = new Map<
|
||||
{
|
||||
promise: Promise<HostedReviewInfo | null>
|
||||
force: boolean
|
||||
currentHeadOid: string | null
|
||||
generation: number
|
||||
startedAt: number
|
||||
}
|
||||
@@ -177,6 +178,7 @@ export function registerInflightHostedReviewRequest(
|
||||
entry: {
|
||||
promise: Promise<HostedReviewInfo | null>
|
||||
force: boolean
|
||||
currentHeadOid: string | null
|
||||
generation: number
|
||||
startedAt: number
|
||||
}
|
||||
|
||||
@@ -712,6 +712,77 @@ describe('hosted review slice', () => {
|
||||
expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('revalidates instead of serving a stale merged review on the stale-while-revalidate path', async () => {
|
||||
const mergedAtHead: HostedReviewInfo = {
|
||||
provider: 'github',
|
||||
number: 7,
|
||||
title: 'Merged at head',
|
||||
state: 'merged',
|
||||
url: 'https://github.com/acme/orca/pull/7',
|
||||
status: 'success',
|
||||
updatedAt: '2026-05-10T00:00:00.000Z',
|
||||
mergeable: 'MERGEABLE',
|
||||
headSha: 'aaaaaaa'
|
||||
}
|
||||
mockApi.hostedReview.forBranch.mockResolvedValueOnce(mergedAtHead).mockResolvedValueOnce(null)
|
||||
const store = makeStore()
|
||||
|
||||
await expect(
|
||||
store.getState().fetchHostedReviewForBranch('/repo', 'feature/merged', {
|
||||
currentHeadOid: 'aaaaaaa'
|
||||
})
|
||||
).resolves.toEqual(mergedAtHead)
|
||||
|
||||
// Advancing the branch must bypass even the fresh stale-while-revalidate cache.
|
||||
await expect(
|
||||
store.getState().fetchHostedReviewForBranch('/repo', 'feature/merged', {
|
||||
staleWhileRevalidate: true,
|
||||
currentHeadOid: 'bbbbbbb'
|
||||
})
|
||||
).resolves.toBeNull()
|
||||
expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(2)
|
||||
})
|
||||
|
||||
it('does not reuse an old-head request after the branch advances', async () => {
|
||||
const mergedAtHead: HostedReviewInfo = {
|
||||
...githubReview,
|
||||
state: 'merged',
|
||||
headSha: 'aaaaaaa'
|
||||
}
|
||||
let resolveOldHead!: (value: HostedReviewInfo | null) => void
|
||||
mockApi.hostedReview.forBranch
|
||||
.mockResolvedValueOnce(mergedAtHead)
|
||||
.mockImplementationOnce(
|
||||
() =>
|
||||
new Promise<HostedReviewInfo | null>((resolve) => {
|
||||
resolveOldHead = resolve
|
||||
})
|
||||
)
|
||||
.mockResolvedValueOnce(null)
|
||||
const store = makeStore()
|
||||
const fetch = store.getState().fetchHostedReviewForBranch
|
||||
await fetch('/repo', 'feature/inflight-head', { currentHeadOid: 'aaaaaaa' })
|
||||
const oldFetch = fetch('/repo', 'feature/inflight-head', {
|
||||
force: true,
|
||||
currentHeadOid: 'aaaaaaa'
|
||||
})
|
||||
const newFetch = fetch('/repo', 'feature/inflight-head', {
|
||||
staleWhileRevalidate: true,
|
||||
currentHeadOid: 'bbbbbbb'
|
||||
})
|
||||
resolveOldHead(mergedAtHead)
|
||||
|
||||
await expect(newFetch).resolves.toBeNull()
|
||||
await expect(oldFetch).resolves.toEqual(mergedAtHead)
|
||||
expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(3)
|
||||
await expect(
|
||||
fetch('/repo', 'feature/inflight-head', {
|
||||
currentHeadOid: 'bbbbbbb'
|
||||
})
|
||||
).resolves.toBeNull()
|
||||
expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(3)
|
||||
})
|
||||
|
||||
it('serves a cached merged review whose confirmed contained head matches the worktree', async () => {
|
||||
const mergedBehindHead: HostedReviewInfo = {
|
||||
provider: 'github',
|
||||
|
||||
@@ -181,7 +181,15 @@ export const createHostedReviewSlice: StateCreator<AppState, [], [], HostedRevie
|
||||
return cached.data
|
||||
}
|
||||
|
||||
const currentHeadOid = options?.currentHeadOid ?? null
|
||||
const inflightRequest = inflightHostedReviewRequests.get(requestKey)
|
||||
// A lookup started at another HEAD may return a merged PR this worktree has moved past.
|
||||
const joinableInflightRequest =
|
||||
currentHeadOid !== null &&
|
||||
inflightRequest?.currentHeadOid != null &&
|
||||
inflightRequest.currentHeadOid !== currentHeadOid
|
||||
? undefined
|
||||
: inflightRequest
|
||||
const startRequest = (): Promise<HostedReviewInfo | null> => {
|
||||
const generation = nextLookupGeneration()
|
||||
const requestStartedAt = Date.now()
|
||||
@@ -287,6 +295,7 @@ export const createHostedReviewSlice: StateCreator<AppState, [], [], HostedRevie
|
||||
registerInflightHostedReviewRequest(requestKey, {
|
||||
promise: request,
|
||||
force: Boolean(options?.force),
|
||||
currentHeadOid,
|
||||
generation,
|
||||
startedAt: requestStartedAt
|
||||
})
|
||||
@@ -297,6 +306,7 @@ export const createHostedReviewSlice: StateCreator<AppState, [], [], HostedRevie
|
||||
!options?.force &&
|
||||
!linkedRefetch &&
|
||||
!scopedResultRefetch &&
|
||||
!staleMergedHeadRefetch &&
|
||||
options?.staleWhileRevalidate &&
|
||||
cached !== undefined &&
|
||||
cached.data !== null
|
||||
@@ -307,8 +317,8 @@ export const createHostedReviewSlice: StateCreator<AppState, [], [], HostedRevie
|
||||
return cached.data
|
||||
}
|
||||
|
||||
if (inflightRequest && (!options?.force || inflightRequest.force)) {
|
||||
return inflightRequest.promise
|
||||
if (joinableInflightRequest && (!options?.force || joinableInflightRequest.force)) {
|
||||
return joinableInflightRequest.promise
|
||||
}
|
||||
|
||||
return startRequest()
|
||||
|
||||
Reference in New Issue
Block a user