fix: address review findings (#2029)

This commit is contained in:
Jinjing 2026-05-15 19:54:02 -07:00 committed by GitHub
parent ccd6350868
commit 7b7f82f763
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 273 additions and 37 deletions

View File

@ -21,12 +21,17 @@ import type { PRInfo, PRCheckDetail, PRComment } from '../../../../shared/types'
import { getConnectionId } from '@/lib/connection-context'
import { CreatePullRequestDialog } from './CreatePullRequestDialog'
import type { HostedReviewCreationEligibility } from '../../../../shared/hosted-review'
import { refreshHostedReviewCard } from '@/store/slices/hosted-review'
import { toast } from 'sonner'
import {
classifyHostedReview,
type HostedReviewClassificationOptions
} from '../../../../shared/hosted-review-queue'
import { hostedReviewSummaryFromGitHubPRInfo } from '../../../../shared/hosted-review-github'
import {
checksPanelAsyncResultKey,
shouldCommitChecksPanelAsyncResult
} from './checks-panel-async-result-key'
export default function ChecksPanel(): React.JSX.Element {
const activeWorktree = useActiveWorktree()
@ -76,6 +81,7 @@ export default function ChecksPanel(): React.JSX.Element {
const pollIntervalRef = useRef(30_000) // start at 30s, backs off to 120s
const prevChecksRef = useRef<string>('')
const conflictSummaryRefreshKeyRef = useRef<string | null>(null)
const asyncResultKeyRef = useRef<string>('')
// Why: the sidebar no longer uses key={activeWorktreeId} to force a full
// remount on worktree switch (that caused an IPC storm on Windows).
@ -131,6 +137,15 @@ export default function ChecksPanel(): React.JSX.Element {
// Why: pass linkedPR so worktrees created from a PR (whose new local branch
// differs from the PR's head ref) resolve via the number-based fallback.
const linkedPR = activeWorktree?.linkedPR ?? null
const linkedGitLabMR = activeWorktree?.linkedGitLabMR ?? null
const stateRequestKey = repo && branch ? checksPanelAsyncResultKey(repo.id, branch, prNumber) : ''
asyncResultKeyRef.current = stateRequestKey
const isCurrentAsyncResult = useCallback(
(requestKey: string) =>
shouldCommitChecksPanelAsyncResult(asyncResultKeyRef.current, requestKey),
[]
)
useEffect(() => {
if (repo && !isFolder && branch) {
void fetchPRForBranch(repo.path, branch, { repoId: repo.id, linkedPRNumber: linkedPR })
@ -223,10 +238,14 @@ export default function ChecksPanel(): React.JSX.Element {
}
setChecksLoading(true)
try {
const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber)
const result = await fetchPRChecks(repo.path, targetPRNumber, branch, pr?.headSha, {
force,
repoId: repo.id
})
if (!isCurrentAsyncResult(requestKey)) {
return
}
setChecks(result)
// Exponential backoff: if checks haven't changed, double the interval (cap 120s).
@ -238,13 +257,18 @@ export default function ChecksPanel(): React.JSX.Element {
: 30_000
prevChecksRef.current = signature
} catch (err) {
if (!isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) {
return
}
console.warn('Failed to fetch PR checks:', err)
setChecks([])
} finally {
setChecksLoading(false)
if (isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) {
setChecksLoading(false)
}
}
},
[repo, prNumber, branch, pr?.headSha, fetchPRChecks]
[repo, prNumber, branch, pr?.headSha, fetchPRChecks, isCurrentAsyncResult]
)
// Fetch checks on mount + poll with exponential backoff
@ -293,16 +317,25 @@ export default function ChecksPanel(): React.JSX.Element {
}
setCommentsLoading(true)
try {
const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber)
const result = await fetchPRComments(repo.path, targetPRNumber, { force, repoId: repo.id })
if (!isCurrentAsyncResult(requestKey)) {
return
}
setComments(result)
} catch (err) {
if (!isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) {
return
}
console.warn('Failed to fetch PR comments:', err)
setComments([])
} finally {
setCommentsLoading(false)
if (isCurrentAsyncResult(checksPanelAsyncResultKey(repo.id, branch, targetPRNumber))) {
setCommentsLoading(false)
}
}
},
[repo, prNumber, fetchPRComments]
[repo, prNumber, fetchPRComments, branch, isCurrentAsyncResult]
)
useEffect(() => {
@ -337,6 +370,8 @@ export default function ChecksPanel(): React.JSX.Element {
if (!repo || !branch) {
return
}
const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber)
let activeRefreshKey = initialRequestKey
setIsRefreshing(true)
try {
const refreshedPR = await fetchPRForBranch(repo.path, branch, {
@ -344,7 +379,22 @@ export default function ChecksPanel(): React.JSX.Element {
repoId: repo.id,
linkedPRNumber: linkedPR
})
await refreshHostedReviewCard(fetchHostedReviewForBranch, {
repoPath: repo.path,
repoId: repo.id,
branch,
linkedGitHubPR: refreshedPR?.number ?? linkedPR,
linkedGitLabMR
})
if (refreshedPR) {
const requestKey = checksPanelAsyncResultKey(repo.id, branch, refreshedPR.number)
if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentAsyncResult(requestKey)) {
return
}
// Why: a forced PR refresh can discover the PR number before React has
// repainted from prCache; make this refresh's follow-up checks current.
asyncResultKeyRef.current = requestKey
activeRefreshKey = requestKey
// Why: call fetchPRChecks directly with the refreshed PR's headSha so
// we don't pass the stale headSha captured by `fetchChecks`'s closure
// before the PR refresh completed (covers external force-pushes and
@ -357,6 +407,9 @@ export default function ChecksPanel(): React.JSX.Element {
{ force: true, repoId: repo.id }
).then(
(result) => {
if (!isCurrentAsyncResult(requestKey)) {
return
}
setChecks(result)
const signature = JSON.stringify(
result.map((c) => `${c.name}:${c.status}:${c.conclusion}`)
@ -368,6 +421,9 @@ export default function ChecksPanel(): React.JSX.Element {
prevChecksRef.current = signature
},
(err) => {
if (!isCurrentAsyncResult(requestKey)) {
return
}
console.warn('Failed to fetch PR checks:', err)
setChecks([])
}
@ -378,17 +434,34 @@ export default function ChecksPanel(): React.JSX.Element {
prNumberOverride: refreshedPR.number
})
await Promise.all([
refreshedChecks.finally(() => setChecksLoading(false)),
refreshedChecks.finally(() => {
if (isCurrentAsyncResult(requestKey)) {
setChecksLoading(false)
}
}),
refreshedComments
])
} else {
} else if (isCurrentAsyncResult(initialRequestKey)) {
setChecks([])
setComments([])
}
} finally {
setIsRefreshing(false)
if (isCurrentAsyncResult(activeRefreshKey)) {
setIsRefreshing(false)
}
}
}, [repo, branch, linkedPR, fetchPRForBranch, fetchPRChecks, fetchComments])
}, [
repo,
branch,
prNumber,
linkedPR,
linkedGitLabMR,
fetchPRForBranch,
fetchPRChecks,
fetchComments,
fetchHostedReviewForBranch,
isCurrentAsyncResult
])
// Why: force a freshness check on each "entry" into the Checks tab so PRs
// opened outside Orca, externally force-pushed heads, and stale checks/comments
@ -509,13 +582,20 @@ export default function ChecksPanel(): React.JSX.Element {
// Refresh PR (passed to PRActions)
const handleRefreshPR = useCallback(async () => {
if (repo && branch) {
await fetchPRForBranch(repo.path, branch, {
const refreshedPR = await fetchPRForBranch(repo.path, branch, {
force: true,
repoId: repo.id,
linkedPRNumber: linkedPR
})
await refreshHostedReviewCard(fetchHostedReviewForBranch, {
repoPath: repo.path,
repoId: repo.id,
branch,
linkedGitHubPR: refreshedPR?.number ?? linkedPR,
linkedGitLabMR
})
}
}, [repo, branch, linkedPR, fetchPRForBranch])
}, [repo, branch, linkedPR, linkedGitLabMR, fetchPRForBranch, fetchHostedReviewForBranch])
// Open PR in browser
const handleOpenPR = useCallback(() => {
@ -549,23 +629,45 @@ export default function ChecksPanel(): React.JSX.Element {
if (!repo || !branch) {
return
}
const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber)
setRightSidebarOpen(true)
setRightSidebarTab('checks')
try {
const refreshedPR = await fetchPRForBranch(repo.path, branch, {
force: true,
repoId: repo.id,
linkedPRNumber: result.number
})
await fetchHostedReviewForBranch(repo.path, branch, {
force: true,
linkedGitHubPR: result.number
await refreshHostedReviewCard(fetchHostedReviewForBranch, {
repoPath: repo.path,
repoId: repo.id,
branch,
linkedGitHubPR: result.number,
linkedGitLabMR
})
if (refreshedPR) {
const requestKey = checksPanelAsyncResultKey(repo.id, branch, refreshedPR.number)
if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentAsyncResult(requestKey)) {
return
}
asyncResultKeyRef.current = requestKey
await Promise.all([
fetchPRChecks(repo.path, refreshedPR.number, branch, refreshedPR.headSha, {
force: true
}).then(setChecks),
fetchPRComments(repo.path, refreshedPR.number, { force: true }).then(setComments)
force: true,
repoId: repo.id
}).then((result) => {
if (isCurrentAsyncResult(requestKey)) {
setChecks(result)
}
}),
fetchPRComments(repo.path, refreshedPR.number, {
force: true,
repoId: repo.id
}).then((result) => {
if (isCurrentAsyncResult(requestKey)) {
setComments(result)
}
})
])
}
} catch {
@ -578,6 +680,9 @@ export default function ChecksPanel(): React.JSX.Element {
fetchPRChecks,
fetchPRComments,
fetchPRForBranch,
isCurrentAsyncResult,
linkedGitLabMR,
prNumber,
repo,
setRightSidebarOpen,
setRightSidebarTab

View File

@ -1177,11 +1177,13 @@ function SourceControlInner(): React.JSX.Element {
await Promise.all([
fetchHostedReviewForBranch(activeRepo.path, branchName, {
force: true,
repoId: activeRepo.id,
linkedGitHubPR: result.number,
linkedGitLabMR
}),
fetchPRForBranch(activeRepo.path, branchName, {
force: true,
repoId: activeRepo.id,
linkedPRNumber: result.number
})
])

View File

@ -0,0 +1,30 @@
import { describe, expect, it } from 'vitest'
import {
checksPanelAsyncResultKey,
shouldCommitChecksPanelAsyncResult
} from './checks-panel-async-result-key'
describe('checksPanelAsyncResultKey', () => {
it('builds a stable repo-scoped key', () => {
expect(checksPanelAsyncResultKey('repo-id', 'feature/test', 12)).toBe(
'repo-id::feature/test::12'
)
})
it('uses explicit none marker when PR is absent', () => {
expect(checksPanelAsyncResultKey('repo-id', 'feature/test', null)).toBe(
'repo-id::feature/test::none'
)
})
})
describe('shouldCommitChecksPanelAsyncResult', () => {
it('suppresses stale async completions', () => {
expect(
shouldCommitChecksPanelAsyncResult(
checksPanelAsyncResultKey('repo-id', 'feature/new', 99),
checksPanelAsyncResultKey('repo-id', 'feature/old', 12)
)
).toBe(false)
})
})

View File

@ -0,0 +1,14 @@
export function checksPanelAsyncResultKey(
repoId: string,
branch: string,
prNumber: number | null
): string {
return `${repoId}::${branch}::${prNumber ?? 'none'}`
}
export function shouldCommitChecksPanelAsyncResult(
currentKey: string,
requestKey: string
): boolean {
return currentKey === requestKey
}

View File

@ -19,7 +19,7 @@ describe('normalizeBranchName', () => {
describe('syncPRChecksStatus', () => {
const baseState = {
prCache: {
'/repo::main': {
'repo-id::main': {
fetchedAt: 0,
data: { checksStatus: 'neutral' as const }
}
@ -27,14 +27,21 @@ describe('syncPRChecksStatus', () => {
} as unknown as AppState
it('returns null for undefined branch', () => {
expect(syncPRChecksStatus(baseState, '/repo', undefined, [])).toBeNull()
expect(syncPRChecksStatus(baseState, '/repo', 'repo-id', undefined, [])).toBeNull()
})
it('returns null for empty string branch', () => {
expect(syncPRChecksStatus(baseState, '/repo', '', [])).toBeNull()
expect(syncPRChecksStatus(baseState, '/repo', 'repo-id', '', [])).toBeNull()
})
it('returns null for refs/heads/ only (normalizes to empty)', () => {
expect(syncPRChecksStatus(baseState, '/repo', 'refs/heads/', [])).toBeNull()
expect(syncPRChecksStatus(baseState, '/repo', 'repo-id', 'refs/heads/', [])).toBeNull()
})
it('uses repoId-scoped key when syncing status', () => {
const result = syncPRChecksStatus(baseState, '/repo', 'repo-id', 'main', [
{ name: 'build', status: 'completed', conclusion: 'success', url: null }
])
expect(result?.prCache?.['repo-id::main']?.data?.checksStatus).toBe('success')
})
})

View File

@ -36,6 +36,7 @@ export function deriveCheckStatusFromChecks(checks: PRCheckDetail[]): CheckStatu
export function syncPRChecksStatus(
state: AppState,
repoPath: string,
repoId: string | undefined,
branch: string | undefined,
checks: PRCheckDetail[]
): Partial<AppState> | null {
@ -44,7 +45,7 @@ export function syncPRChecksStatus(
return null
}
const prCacheKey = `${repoPath}::${normalized}`
const prCacheKey = `${repoId ?? repoPath}::${normalized}`
const prEntry = state.prCache[prCacheKey]
if (!prEntry?.data) {
return null

View File

@ -81,8 +81,9 @@ describe('createGitHubSlice.fetchPRChecks', () => {
it('updates the matching PR cache entry with derived check status', async () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const prCacheKey = `${repoPath}::${branch}`
const prCacheKey = `${repoId}::${branch}`
store.setState({
prCache: {
@ -98,7 +99,7 @@ describe('createGitHubSlice.fetchPRChecks', () => {
{ name: 'lint', status: 'completed', conclusion: 'success', url: null }
])
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true })
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId })
expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success')
})
@ -106,8 +107,9 @@ describe('createGitHubSlice.fetchPRChecks', () => {
it('marks the PR cache entry as failure when any check fails', async () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const prCacheKey = `${repoPath}::${branch}`
const prCacheKey = `${repoId}::${branch}`
store.setState({
prCache: {
@ -123,7 +125,7 @@ describe('createGitHubSlice.fetchPRChecks', () => {
{ name: 'integration', status: 'completed', conclusion: 'failure', url: null }
])
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true })
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId })
expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('failure')
})
@ -131,8 +133,9 @@ describe('createGitHubSlice.fetchPRChecks', () => {
it('normalizes refs/heads branch names before updating PR cache status', async () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const prCacheKey = `${repoPath}::${branch}`
const prCacheKey = `${repoId}::${branch}`
store.setState({
prCache: {
@ -149,7 +152,7 @@ describe('createGitHubSlice.fetchPRChecks', () => {
await store
.getState()
.fetchPRChecks(repoPath, 12, `refs/heads/${branch}`, undefined, { force: true })
.fetchPRChecks(repoPath, 12, `refs/heads/${branch}`, undefined, { force: true, repoId })
expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success')
})
@ -159,8 +162,9 @@ describe('createGitHubSlice.fetchPRChecks', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const prCacheKey = `${repoPath}::${branch}`
const prCacheKey = `${repoId}::${branch}`
store.setState({
prCache: {
@ -175,7 +179,7 @@ describe('createGitHubSlice.fetchPRChecks', () => {
{ name: 'build', status: 'completed', conclusion: 'success', url: null }
])
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true })
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId })
await vi.advanceTimersByTimeAsync(1000)
expect(mockApi.cache.setGitHub).toHaveBeenCalledWith({
@ -191,9 +195,10 @@ describe('createGitHubSlice.fetchPRChecks', () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const prCacheKey = `${repoPath}::${branch}`
const checksCacheKey = `${repoPath}::pr-checks::12`
const prCacheKey = `${repoId}::${branch}`
const checksCacheKey = `${repoId}::pr-checks::12`
store.setState({
prCache: {
@ -210,7 +215,7 @@ describe('createGitHubSlice.fetchPRChecks', () => {
}
})
await store.getState().fetchPRChecks(repoPath, 12, branch)
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { repoId })
await vi.advanceTimersByTimeAsync(1000)
expect(mockApi.gh.prChecks).not.toHaveBeenCalled()
@ -226,8 +231,9 @@ describe('createGitHubSlice.fetchPRChecks', () => {
it('passes the cached PR head SHA to the checks IPC request', async () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const prCacheKey = `${repoPath}::${branch}`
const prCacheKey = `${repoId}::${branch}`
store.setState({
prCache: {
@ -238,15 +244,43 @@ describe('createGitHubSlice.fetchPRChecks', () => {
}
})
await store.getState().fetchPRChecks(repoPath, 12, branch, 'abc123head', { force: true })
await store
.getState()
.fetchPRChecks(repoPath, 12, branch, 'abc123head', { force: true, repoId })
expect(mockApi.gh.prChecks).toHaveBeenCalledWith({
repoPath,
repoId,
prNumber: 12,
headSha: 'abc123head',
noCache: true
})
})
it('updates repo-scoped PR cache entry instead of repoPath fallback key', async () => {
const store = createTestStore()
const repoPath = '/repo'
const repoId = 'repo-id'
const branch = 'feature/test'
const repoScopedKey = `${repoId}::${branch}`
const pathScopedKey = `${repoPath}::${branch}`
store.setState({
prCache: {
[repoScopedKey]: { data: makePR({ checksStatus: 'pending' }), fetchedAt: 1 },
[pathScopedKey]: { data: makePR({ checksStatus: 'pending' }), fetchedAt: 1 }
}
})
mockApi.gh.prChecks.mockResolvedValue([
{ name: 'build', status: 'completed', conclusion: 'success', url: null }
])
await store.getState().fetchPRChecks(repoPath, 12, branch, undefined, { force: true, repoId })
expect(store.getState().prCache[repoScopedKey]?.data?.checksStatus).toBe('success')
expect(store.getState().prCache[pathScopedKey]?.data?.checksStatus).toBe('pending')
})
})
describe('createGitHubSlice.fetchPRForBranch', () => {

View File

@ -1364,7 +1364,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
const cached = get().checksCache[cacheKey]
if (!options?.force && isFresh(cached, CHECKS_CACHE_TTL)) {
const cachedChecks = cached.data ?? []
const prStatusUpdate = syncPRChecksStatus(get(), repoPath, branch, cachedChecks)
const prStatusUpdate = syncPRChecksStatus(get(), repoPath, repoId, branch, cachedChecks)
if (prStatusUpdate) {
set(prStatusUpdate)
debouncedSaveCache(get())
@ -1391,7 +1391,7 @@ export const createGitHubSlice: StateCreator<AppState, [], [], GitHubSlice> = (s
checksCache: { ...s.checksCache, [cacheKey]: { data: checks, fetchedAt: Date.now() } }
}
const prStatusUpdate = syncPRChecksStatus(s, repoPath, branch, checks)
const prStatusUpdate = syncPRChecksStatus(s, repoPath, repoId, branch, checks)
if (prStatusUpdate?.prCache) {
nextState.prCache = prStatusUpdate.prCache
}

View File

@ -1,7 +1,7 @@
import { beforeEach, describe, expect, it, vi } from 'vitest'
import { create } from 'zustand'
import type { AppState } from '../types'
import { createHostedReviewSlice } from './hosted-review'
import { createHostedReviewSlice, refreshHostedReviewCard } from './hosted-review'
import type { HostedReviewInfo } from '../../../../shared/hosted-review'
const runtimeRpc = vi.hoisted(() => ({
@ -104,4 +104,23 @@ describe('hosted review slice', () => {
{ timeoutMs: 30_000 }
)
})
it('forces card refresh with repo-scoped identity and linked review ids', async () => {
const fetchHostedReviewForBranch = vi.fn().mockResolvedValue(null)
await refreshHostedReviewCard(fetchHostedReviewForBranch, {
repoPath: '/repo',
repoId: 'repo-id',
branch: 'feature/test',
linkedGitHubPR: null,
linkedGitLabMR: 33
})
expect(fetchHostedReviewForBranch).toHaveBeenCalledWith('/repo', 'feature/test', {
force: true,
repoId: 'repo-id',
linkedGitHubPR: null,
linkedGitLabMR: 33,
linkedBitbucketPR: null,
linkedGiteaPR: null
})
})
})

View File

@ -57,6 +57,30 @@ export type HostedReviewSlice = {
) => Promise<HostedReviewInfo | null>
}
type RefreshHostedReviewCardArgs = {
repoPath: string
repoId: string
branch: string
linkedGitHubPR?: number | null
linkedGitLabMR?: number | null
linkedBitbucketPR?: number | null
linkedGiteaPR?: number | null
}
export function refreshHostedReviewCard(
fetchHostedReviewForBranch: HostedReviewSlice['fetchHostedReviewForBranch'],
args: RefreshHostedReviewCardArgs
): Promise<HostedReviewInfo | null> {
return fetchHostedReviewForBranch(args.repoPath, args.branch, {
force: true,
repoId: args.repoId,
linkedGitHubPR: args.linkedGitHubPR ?? null,
linkedGitLabMR: args.linkedGitLabMR ?? null,
linkedBitbucketPR: args.linkedBitbucketPR ?? null,
linkedGiteaPR: args.linkedGiteaPR ?? null
})
}
export const createHostedReviewSlice: StateCreator<AppState, [], [], HostedReviewSlice> = (
set,
get