From 8cf39f3a2cea3472a4d425e3cb7c5b8ed3ac5bc1 Mon Sep 17 00:00:00 2001 From: Jinwoo Hong <73622457+Jinwoo-H@users.noreply.github.com> Date: Thu, 21 May 2026 02:38:32 -0400 Subject: [PATCH] Fix stale GitHub status cache updates (#2483) Co-authored-by: Orca --- .../github/pr-refresh-coordinator.test.ts | 64 + src/main/github/pr-refresh-coordinator.ts | 24 +- src/main/source-control/hosted-review.ts | 14 +- .../components/right-sidebar/ChecksPanel.tsx | 117 +- .../right-sidebar/SourceControl.tsx | 20 +- .../active-checks-status.test.ts | 40 + .../right-sidebar/active-checks-status.ts | 44 + .../checks-panel-async-result-key.test.ts | 12 +- .../checks-panel-async-result-key.ts | 7 +- .../src/components/right-sidebar/index.tsx | 30 +- .../src/components/sidebar/WorktreeCard.tsx | 4 +- .../src/components/sidebar/WorktreeList.tsx | 20 +- .../sidebar/worktree-list-groups.test.ts | 54 +- .../sidebar/worktree-list-groups.ts | 43 +- .../src/store/slices/github-cache-key.ts | 45 + .../src/store/slices/github-checks.ts | 7 +- src/renderer/src/store/slices/github.test.ts | 1029 ++++++++++++++++- src/renderer/src/store/slices/github.ts | 487 +++++++- .../slices/hosted-review-cache-identity.ts | 42 + .../slices/hosted-review-cache-race.test.ts | 257 ++++ .../src/store/slices/hosted-review.test.ts | 65 ++ .../src/store/slices/hosted-review.ts | 167 ++- src/renderer/src/store/slices/worktrees.ts | 8 +- src/shared/hosted-review-github.test.ts | 19 +- src/shared/hosted-review-github.ts | 17 +- src/shared/types.ts | 2 + 26 files changed, 2438 insertions(+), 200 deletions(-) create mode 100644 src/renderer/src/components/right-sidebar/active-checks-status.test.ts create mode 100644 src/renderer/src/components/right-sidebar/active-checks-status.ts create mode 100644 src/renderer/src/store/slices/github-cache-key.ts create mode 100644 src/renderer/src/store/slices/hosted-review-cache-identity.ts create mode 100644 src/renderer/src/store/slices/hosted-review-cache-race.test.ts diff --git a/src/main/github/pr-refresh-coordinator.test.ts b/src/main/github/pr-refresh-coordinator.test.ts index 726784bae..f7bff96cb 100644 --- a/src/main/github/pr-refresh-coordinator.test.ts +++ b/src/main/github/pr-refresh-coordinator.test.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines -- Why: coordinator tests cover queueing, coalescing, +request timestamps, and follow-up scheduling against shared module state. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import type { GitHubPRRefreshCandidate, PRInfo } from '../../shared/types' @@ -289,6 +291,68 @@ describe('pr-refresh-coordinator', () => { expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) }) + it('includes request start time on manual refresh events', async () => { + const { refreshPRNow } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock.mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ checksStatus: 'success' }), + fetchedAt: Date.now() + 5 + }) + + await refreshPRNow(makeCandidate()) + + const events = sendMock.mock.calls.map(([, event]) => event) + const inFlight = events.find((event) => event.status === 'in-flight') + const outcome = events.find((event) => event.outcome) + expect(inFlight?.requestStartedAt).toBe(1_000) + expect(outcome?.requestStartedAt).toBe(1_000) + expect(outcome?.sequence).toBe(inFlight?.sequence) + }) + + it('does not coalesce local and SSH refreshes for the same branch', async () => { + const { enqueuePRRefresh } = await import('./pr-refresh-coordinator') + getPRForBranchOutcomeMock + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ number: 12 }), + fetchedAt: Date.now() + }) + .mockResolvedValueOnce({ + kind: 'found', + pr: makePR({ number: 44 }), + fetchedAt: Date.now() + }) + + enqueuePRRefresh(makeCandidate({ cacheKey: 'local::repo-1::feature/test' }), 'active', 80, 1) + enqueuePRRefresh( + makeCandidate({ + cacheKey: 'ssh:ssh-1::repo-1::feature/test', + connectionId: 'ssh-1' + }), + 'active', + 80, + 1 + ) + await vi.runOnlyPendingTimersAsync() + await vi.runOnlyPendingTimersAsync() + + expect(getPRForBranchOutcomeMock).toHaveBeenCalledTimes(2) + expect(getPRForBranchOutcomeMock).toHaveBeenNthCalledWith( + 1, + '/repo', + 'feature/test', + null, + null + ) + expect(getPRForBranchOutcomeMock).toHaveBeenNthCalledWith( + 2, + '/repo', + 'feature/test', + null, + 'ssh-1' + ) + }) + it('preserves coalesced aliases across visible follow-up refreshes', async () => { const { reportVisiblePRRefreshCandidates } = await import('./pr-refresh-coordinator') getPRForBranchOutcomeMock diff --git a/src/main/github/pr-refresh-coordinator.ts b/src/main/github/pr-refresh-coordinator.ts index 75f883067..cb7a02728 100644 --- a/src/main/github/pr-refresh-coordinator.ts +++ b/src/main/github/pr-refresh-coordinator.ts @@ -65,10 +65,11 @@ function broadcast(event: Omit, sequenceOverri } function refreshKey(candidate: GitHubPRRefreshCandidate): string { + const connectionScope = candidate.connectionId ?? 'local' if (typeof candidate.linkedPRNumber === 'number') { - return `${candidate.repoPath}::pr::${candidate.linkedPRNumber}` + return `${connectionScope}::${candidate.repoPath}::pr::${candidate.linkedPRNumber}` } - return `${candidate.repoPath}::branch::${candidate.branch}` + return `${connectionScope}::${candidate.repoPath}::branch::${candidate.branch}` } function isVisibleKey(key: string): boolean { @@ -381,7 +382,11 @@ async function drainQueue(): Promise { continue } const requestSequence = nextSequence() - broadcast({ aliases, reason: next.reason, status: 'in-flight' }, requestSequence) + const requestStartedAt = Date.now() + broadcast( + { aliases, reason: next.reason, status: 'in-flight', requestStartedAt }, + requestSequence + ) if (isBackground(next.reason)) { const rateLimit = await getRateLimit() @@ -430,7 +435,7 @@ async function drainQueue(): Promise { next.candidate.connectionId ?? null ) outcomeObserver?.(next.candidate, outcome) - broadcast({ aliases, reason: next.reason, outcome }, requestSequence) + broadcast({ aliases, reason: next.reason, outcome, requestStartedAt }, requestSequence) scheduleVisibleFollowUp( next.key, next.candidate, @@ -456,7 +461,8 @@ export function enqueuePRRefresh( repoId: candidate.repoId, repoPath: candidate.repoPath, branch: candidate.branch, - worktreeId: candidate.worktreeId + worktreeId: candidate.worktreeId, + connectionId: candidate.connectionId ?? null } const key = refreshKey(candidate) const skippedReason = validateCandidate(candidate) @@ -539,7 +545,8 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise repoId: candidate.repoId, repoPath: candidate.repoPath, branch: candidate.branch, - worktreeId: candidate.worktreeId + worktreeId: candidate.worktreeId, + connectionId: candidate.connectionId ?? null } const key = refreshKey(candidate) const existing = queue.get(key) @@ -562,7 +569,8 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise queue.delete(key) const requestSequence = nextSequence() - broadcast({ aliases, reason: 'manual', status: 'in-flight' }, requestSequence) + const requestStartedAt = Date.now() + broadcast({ aliases, reason: 'manual', status: 'in-flight', requestStartedAt }, requestSequence) const outcome = await getPRForBranchOutcome( candidate.repoPath, candidate.branch, @@ -570,7 +578,7 @@ export async function refreshPRNow(candidate: GitHubPRRefreshCandidate): Promise candidate.connectionId ?? null ) outcomeObserver?.(candidate, outcome) - broadcast({ aliases, reason: 'manual', outcome }, requestSequence) + broadcast({ aliases, reason: 'manual', outcome, requestStartedAt }, requestSequence) scheduleVisibleFollowUp(key, candidate, outcome, 40, aliases) return outcome } diff --git a/src/main/source-control/hosted-review.ts b/src/main/source-control/hosted-review.ts index c76da7c48..55ad105bf 100644 --- a/src/main/source-control/hosted-review.ts +++ b/src/main/source-control/hosted-review.ts @@ -1,5 +1,6 @@ import type { HostedReviewInfo } from '../../shared/hosted-review' import type { MRInfo, PRInfo } from '../../shared/types' +import { hostedReviewInfoFromGitHubPRInfo } from '../../shared/hosted-review-github' import { getAzureDevOpsPullRequest, getAzureDevOpsPullRequestForBranch, @@ -22,18 +23,7 @@ import { getPRForBranch, getRepoSlug } from '../github/client' import { getMergeRequest, getMergeRequestForBranch, getProjectSlug } from '../gitlab/client' function mapGitHubReview(pr: PRInfo): HostedReviewInfo { - return { - provider: 'github', - number: pr.number, - title: pr.title, - state: pr.state, - url: pr.url, - status: pr.checksStatus, - updatedAt: pr.updatedAt, - mergeable: pr.mergeable, - ...(pr.headSha ? { headSha: pr.headSha } : {}), - ...(pr.conflictSummary ? { conflictSummary: pr.conflictSummary } : {}) - } + return hostedReviewInfoFromGitHubPRInfo(pr) } function mapGitLabReviewState(state: MRInfo['state']): HostedReviewInfo['state'] { diff --git a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx index ab339e937..958cb9ca3 100644 --- a/src/renderer/src/components/right-sidebar/ChecksPanel.tsx +++ b/src/renderer/src/components/right-sidebar/ChecksPanel.tsx @@ -4,6 +4,7 @@ import React, { useCallback, useEffect, useRef, useState } from 'react' import { LoaderCircle, ExternalLink, RefreshCw, Check, X, Pencil } from 'lucide-react' import { useAppStore } from '@/store' import { prChecksCacheSuffix, prCommentsCacheSuffix } from '@/store/slices/github' +import { getGitHubPRCacheKey, getGitHubRepoCacheKey } from '@/store/slices/github-cache-key' import { useActiveWorktree, useRepoById } from '@/store/selectors' import { cn } from '@/lib/utils' import { Button } from '@/components/ui/button' @@ -42,6 +43,7 @@ export default function ChecksPanel(): React.JSX.Element { const activeWorktree = useActiveWorktree() const activeWorktreeId = useAppStore((s) => s.activeWorktreeId) const repo = useRepoById(activeWorktree?.repoId ?? null) + const settings = useAppStore((s) => s.settings) const prCache = useAppStore((s) => s.prCache) const fetchPRForBranch = useAppStore((s) => s.fetchPRForBranch) const fetchHostedReviewForBranch = useAppStore((s) => s.fetchHostedReviewForBranch) @@ -118,8 +120,11 @@ export default function ChecksPanel(): React.JSX.Element { // Find active worktree and repo const branch = activeWorktree ? activeWorktree.branch.replace(/^refs\/heads\//, '') : '' const isFolder = repo ? isFolderRepo(repo) : false - const prCacheKey = repo && branch ? `${repo.id}::${branch}` : '' - const refreshContextKey = `${activeWorktreeId ?? ''}::${repo?.id ?? ''}::${branch}` + const prCacheKey = + repo && branch + ? getGitHubPRCacheKey(repo.path, repo.id, branch, settings, repo.connectionId) + : '' + const refreshContextKey = `${activeWorktreeId ?? ''}::${prCacheKey}::${branch}` if (refreshContextKey !== refreshContextKeyRef.current) { refreshContextKeyRef.current = refreshContextKey refreshRequestKeyRef.current = null @@ -144,9 +149,25 @@ export default function ChecksPanel(): React.JSX.Element { prCacheKey ? s.prCache[prCacheKey]?.fetchedAt : undefined ) const checksCacheKey = - repo && prNumber ? `${repo.id}::${prChecksCacheSuffix(prNumber, pr?.prRepo)}` : '' + repo && prNumber + ? getGitHubRepoCacheKey( + repo.path, + repo.id, + prChecksCacheSuffix(prNumber, pr?.prRepo), + settings, + repo.connectionId + ) + : '' const commentsCacheKey = - repo && prNumber ? `${repo.id}::${prCommentsCacheSuffix(prNumber, pr?.prRepo)}` : '' + repo && prNumber + ? getGitHubRepoCacheKey( + repo.path, + repo.id, + prCommentsCacheSuffix(prNumber, pr?.prRepo), + settings, + repo.connectionId + ) + : '' const checksFetchedAt = useAppStore((s) => checksCacheKey ? s.checksCache[checksCacheKey]?.fetchedAt : undefined ) @@ -161,7 +182,9 @@ export default function ChecksPanel(): React.JSX.Element { const linkedGitLabMR = activeWorktree?.linkedGitLabMR ?? null const activeWorktreePath = activeWorktree?.path ?? null const stateRequestKey = - repo && branch ? checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) : '' + repo && branch + ? checksPanelAsyncResultKey(prCacheKey, branch, prNumber, pr?.prRepo, pr?.headSha) + : '' asyncResultKeyRef.current = stateRequestKey const isCurrentAsyncResult = useCallback( @@ -235,7 +258,7 @@ export default function ChecksPanel(): React.JSX.Element { return } - const refreshKey = `${repo.path}::${branch}::${pr.number}` + const refreshKey = `${prCacheKey}::${branch}::${pr.number}` if (conflictSummaryRefreshKeyRef.current === refreshKey) { return } @@ -258,7 +281,7 @@ export default function ChecksPanel(): React.JSX.Element { setConflictDetailsRefreshing(false) } }) - }, [repo, isFolder, branch, pr, activeWorktreeId, linkedPR, fetchPRForBranch]) + }, [repo, isFolder, branch, pr, prCacheKey, activeWorktreeId, linkedPR, fetchPRForBranch]) // Fetch checks via cached store method const fetchChecks = useCallback( @@ -272,7 +295,13 @@ export default function ChecksPanel(): React.JSX.Element { } setChecksLoading(true) try { - const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, pr?.prRepo) + const requestKey = checksPanelAsyncResultKey( + prCacheKey, + branch, + targetPRNumber, + pr?.prRepo, + pr?.headSha + ) const result = await fetchPRChecks( repo.path, targetPRNumber, @@ -300,7 +329,7 @@ export default function ChecksPanel(): React.JSX.Element { } catch (err) { if ( !isCurrentAsyncResult( - checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, pr?.prRepo) + checksPanelAsyncResultKey(prCacheKey, branch, targetPRNumber, pr?.prRepo, pr?.headSha) ) ) { return @@ -310,14 +339,23 @@ export default function ChecksPanel(): React.JSX.Element { } finally { if ( isCurrentAsyncResult( - checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, pr?.prRepo) + checksPanelAsyncResultKey(prCacheKey, branch, targetPRNumber, pr?.prRepo, pr?.headSha) ) ) { setChecksLoading(false) } } }, - [repo, prNumber, branch, pr?.headSha, pr?.prRepo, fetchPRChecks, isCurrentAsyncResult] + [ + repo, + prNumber, + branch, + pr?.headSha, + pr?.prRepo, + prCacheKey, + fetchPRChecks, + isCurrentAsyncResult + ] ) // Fetch checks on mount + poll with exponential backoff @@ -372,7 +410,13 @@ export default function ChecksPanel(): React.JSX.Element { } setCommentsLoading(true) try { - const requestKey = checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, targetPRRepo) + const requestKey = checksPanelAsyncResultKey( + prCacheKey, + branch, + targetPRNumber, + targetPRRepo, + pr?.headSha + ) const result = await fetchPRComments(repo.path, targetPRNumber, { force, repoId: repo.id, @@ -385,7 +429,7 @@ export default function ChecksPanel(): React.JSX.Element { } catch (err) { if ( !isCurrentAsyncResult( - checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, targetPRRepo) + checksPanelAsyncResultKey(prCacheKey, branch, targetPRNumber, targetPRRepo, pr?.headSha) ) ) { return @@ -395,14 +439,23 @@ export default function ChecksPanel(): React.JSX.Element { } finally { if ( isCurrentAsyncResult( - checksPanelAsyncResultKey(repo.id, branch, targetPRNumber, targetPRRepo) + checksPanelAsyncResultKey(prCacheKey, branch, targetPRNumber, targetPRRepo, pr?.headSha) ) ) { setCommentsLoading(false) } } }, - [repo, prNumber, pr?.prRepo, fetchPRComments, branch, isCurrentAsyncResult] + [ + repo, + prNumber, + pr?.headSha, + pr?.prRepo, + prCacheKey, + fetchPRComments, + branch, + isCurrentAsyncResult + ] ) useEffect(() => { @@ -431,14 +484,20 @@ export default function ChecksPanel(): React.JSX.Element { return () => { cancelled = true } - }, [repo, prNumber, pr?.prRepo, isPanelVisible, fetchPRComments]) + }, [repo, prNumber, pr?.prRepo, prCacheKey, isPanelVisible, fetchPRComments]) const handleRefresh = useCallback(async () => { if (!repo || !branch) { return } - const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) - const refreshRequestKey = `${activeWorktreeId ?? ''}::${repo.id}::${branch}::${Date.now()}::${Math.random()}` + const initialRequestKey = checksPanelAsyncResultKey( + prCacheKey, + branch, + prNumber, + pr?.prRepo, + pr?.headSha + ) + const refreshRequestKey = `${activeWorktreeId ?? ''}::${prCacheKey}::${branch}::${Date.now()}::${Math.random()}` refreshRequestKeyRef.current = refreshRequestKey const isCurrentRequest = (): boolean => refreshRequestKeyRef.current === refreshRequestKey setIsRefreshing(true) @@ -463,10 +522,11 @@ export default function ChecksPanel(): React.JSX.Element { } if (refreshedPR) { const prRequestKey = checksPanelAsyncResultKey( - repo.id, + prCacheKey, branch, refreshedPR.number, - refreshedPR.prRepo + refreshedPR.prRepo, + refreshedPR.headSha ) if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentRequest()) { return @@ -554,7 +614,9 @@ export default function ChecksPanel(): React.JSX.Element { branch, activeWorktreeId, prNumber, + pr?.headSha, pr?.prRepo, + prCacheKey, linkedPR, linkedGitLabMR, fetchPRForBranch, @@ -868,7 +930,13 @@ export default function ChecksPanel(): React.JSX.Element { if (!repo || !branch) { return } - const initialRequestKey = checksPanelAsyncResultKey(repo.id, branch, prNumber, pr?.prRepo) + const initialRequestKey = checksPanelAsyncResultKey( + prCacheKey, + branch, + prNumber, + pr?.prRepo, + pr?.headSha + ) setRightSidebarOpen(true) setRightSidebarTab('checks') try { @@ -886,10 +954,11 @@ export default function ChecksPanel(): React.JSX.Element { }) if (refreshedPR) { const requestKey = checksPanelAsyncResultKey( - repo.id, + prCacheKey, branch, refreshedPR.number, - refreshedPR.prRepo + refreshedPR.prRepo, + refreshedPR.headSha ) if (!isCurrentAsyncResult(initialRequestKey) && !isCurrentAsyncResult(requestKey)) { return @@ -934,7 +1003,9 @@ export default function ChecksPanel(): React.JSX.Element { fetchPRForBranch, isCurrentAsyncResult, linkedGitLabMR, + prCacheKey, prNumber, + pr?.headSha, pr?.prRepo, repo, setRightSidebarOpen, diff --git a/src/renderer/src/components/right-sidebar/SourceControl.tsx b/src/renderer/src/components/right-sidebar/SourceControl.tsx index 80bd71611..89ef27904 100644 --- a/src/renderer/src/components/right-sidebar/SourceControl.tsx +++ b/src/renderer/src/components/right-sidebar/SourceControl.tsx @@ -35,6 +35,7 @@ import { useAppStore } from '@/store' import { resolveRemoteOperationErrorMessage } from '@/store/slices/editor' import { useActiveWorktree, useRepoById, useWorktreeMap } from '@/store/selectors' import { getHostedReviewCacheKey } from '@/store/slices/hosted-review' +import { getGitHubPRCacheKey } from '@/store/slices/github-cache-key' import { detectLanguage } from '@/lib/language-detect' import { basename, dirname, joinPath } from '@/lib/path' import { cn } from '@/lib/utils' @@ -926,12 +927,27 @@ function SourceControlInner(): React.JSX.Element { const branchName = activeWorktree?.branch.replace(/^refs\/heads\//, '') ?? 'HEAD' const hostedReviewCacheKey = activeRepo && branchName - ? getHostedReviewCacheKey(activeRepo.path, branchName, settings, activeRepo.id) + ? getHostedReviewCacheKey( + activeRepo.path, + branchName, + settings, + activeRepo.id, + activeRepo.connectionId + ) : null const hostedReviewEntry = hostedReviewCacheKey ? hostedReviewCache[hostedReviewCacheKey] : undefined - const activePrCacheKey = activeRepo && branchName ? `${activeRepo.id}::${branchName}` : null + const activePrCacheKey = + activeRepo && branchName + ? getGitHubPRCacheKey( + activeRepo.path, + activeRepo.id, + branchName, + settings, + activeRepo.connectionId + ) + : null const activePrFromQueue = activePrCacheKey ? (prCache[activePrCacheKey]?.data ?? null) : null const hostedReview: HostedReviewInfo | null = hostedReviewCacheKey ? activePrFromQueue diff --git a/src/renderer/src/components/right-sidebar/active-checks-status.test.ts b/src/renderer/src/components/right-sidebar/active-checks-status.test.ts new file mode 100644 index 000000000..9a196b3bc --- /dev/null +++ b/src/renderer/src/components/right-sidebar/active-checks-status.test.ts @@ -0,0 +1,40 @@ +import { describe, expect, it } from 'vitest' +import { getActiveChecksStatus } from './active-checks-status' +import type { AppState } from '../../store/types' +import type { PRInfo } from '../../../../shared/types' + +function makePR(status: PRInfo['checksStatus']): PRInfo { + return { + number: 12, + title: 'Test PR', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + checksStatus: status, + updatedAt: '2026-05-20T00:00:00Z', + mergeable: 'MERGEABLE' + } +} + +describe('getActiveChecksStatus', () => { + it('prefers repo-id scoped status over stale path-scoped status for the active worktree', () => { + const state = { + activeWorktreeId: 'wt-1', + repos: [{ id: 'repo-1', path: '/repo' }], + worktreesByRepo: { + 'repo-1': [ + { + id: 'wt-1', + repoId: 'repo-1', + branch: 'refs/heads/feature/test' + } + ] + }, + prCache: { + 'repo-1::feature/test': { data: makePR('success'), fetchedAt: 2 }, + '/repo::feature/test': { data: makePR('failure'), fetchedAt: 999 } + } + } as unknown as Pick + + expect(getActiveChecksStatus(state)).toBe('success') + }) +}) diff --git a/src/renderer/src/components/right-sidebar/active-checks-status.ts b/src/renderer/src/components/right-sidebar/active-checks-status.ts new file mode 100644 index 000000000..eaadb1782 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/active-checks-status.ts @@ -0,0 +1,44 @@ +import type { AppState } from '../../store/types' +import { getRepoMapFromState, getWorktreeMapFromState } from '../../store/selectors' +import type { CheckStatus } from '../../../../shared/types' +import { getGitHubPRCacheKey } from '../../store/slices/github-cache-key' + +type ActiveChecksStatusState = Pick< + AppState, + 'activeWorktreeId' | 'worktreesByRepo' | 'repos' | 'prCache' +> & + Partial> + +function branchDisplayName(branch: string): string { + return branch.replace(/^refs\/heads\//, '') +} + +export function getActiveChecksStatus(state: ActiveChecksStatusState): CheckStatus | null { + const activeWorktree = state.activeWorktreeId + ? (getWorktreeMapFromState(state).get(state.activeWorktreeId) ?? null) + : null + if (!activeWorktree) { + return null + } + + const activeRepo = getRepoMapFromState(state).get(activeWorktree.repoId) + if (!activeRepo) { + return null + } + + const branch = branchDisplayName(activeWorktree.branch) + if (!branch) { + return null + } + + // Why: PR refreshes are written under repo-id scoped keys so repo path + // changes and legacy duplicates cannot leave the activity indicator stale. + const prCacheKey = getGitHubPRCacheKey( + activeRepo.path, + activeRepo.id, + branch, + state.settings, + activeRepo.connectionId + ) + return state.prCache[prCacheKey]?.data?.checksStatus ?? null +} diff --git a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts index 5efdfabad..d6877ed42 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.test.ts @@ -7,13 +7,13 @@ import { describe('checksPanelAsyncResultKey', () => { it('builds a stable repo-scoped key', () => { expect(checksPanelAsyncResultKey('repo-id', 'feature/test', 12)).toBe( - 'repo-id::feature/test::none::12' + 'repo-id::feature/test::none::12::none' ) }) it('uses explicit none marker when PR is absent', () => { expect(checksPanelAsyncResultKey('repo-id', 'feature/test', null)).toBe( - 'repo-id::feature/test::none::none' + 'repo-id::feature/test::none::none::none' ) }) @@ -23,7 +23,13 @@ describe('checksPanelAsyncResultKey', () => { owner: 'Acme', repo: 'Widgets' }) - ).toBe('repo-id::feature/test::acme/widgets::12') + ).toBe('repo-id::feature/test::acme/widgets::12::none') + }) + + it('includes PR head SHA so stale checks cannot commit after a new head is discovered', () => { + expect(checksPanelAsyncResultKey('repo-id', 'feature/test', 12, null, 'head-a')).toBe( + 'repo-id::feature/test::none::12::head-a' + ) }) }) diff --git a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts index fc395aae9..470020680 100644 --- a/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts +++ b/src/renderer/src/components/right-sidebar/checks-panel-async-result-key.ts @@ -11,9 +11,12 @@ export function checksPanelAsyncResultKey( repoId: string, branch: string, prNumber: number | null, - prRepo?: GitHubOwnerRepo | null + prRepo?: GitHubOwnerRepo | null, + headSha?: string | null ): string { - return `${repoId}::${branch}::${normalizedPRRepoIdentity(prRepo)}::${prNumber ?? 'none'}` + return `${repoId}::${branch}::${normalizedPRRepoIdentity(prRepo)}::${prNumber ?? 'none'}::${ + headSha ?? 'none' + }` } export function shouldCommitChecksPanelAsyncResult( diff --git a/src/renderer/src/components/right-sidebar/index.tsx b/src/renderer/src/components/right-sidebar/index.tsx index 1927c725d..4aa16813a 100644 --- a/src/renderer/src/components/right-sidebar/index.tsx +++ b/src/renderer/src/components/right-sidebar/index.tsx @@ -1,13 +1,11 @@ import React, { useEffect, useMemo, useState } from 'react' import { Files, Search, GitBranch, ListChecks, PanelRight } from 'lucide-react' import { useAppStore } from '@/store' -import { getRepoMapFromState, useActiveWorktree, useRepoById } from '@/store/selectors' +import { useActiveWorktree, useRepoById } from '@/store/selectors' import { cn } from '@/lib/utils' import { useSidebarResize } from '@/hooks/useSidebarResize' import type { ActivityBarPosition } from '@/store/slices/editor' -import type { CheckStatus } from '../../../../shared/types' import { isFolderRepo } from '../../../../shared/repo-kind' -import { findWorktreeById } from '@/store/slices/worktree-helpers' import { Tooltip, TooltipTrigger, TooltipContent, TooltipProvider } from '@/components/ui/tooltip' import { ContextMenu, @@ -27,6 +25,7 @@ import { TopActivityOverflowMenu, type ActivityBarItem } from './activity-bar-buttons' +import { getActiveChecksStatus } from './active-checks-status' const MIN_WIDTH = 220 // Why: long file names (e.g. construction drawing sheets, multi-part document @@ -38,31 +37,6 @@ const MIN_NON_SIDEBAR_AREA = 320 const ABSOLUTE_FALLBACK_MAX_WIDTH = 2000 const ACTIVITY_BAR_SIDE_WIDTH = 40 -function branchDisplayName(branch: string): string { - return branch.replace(/^refs\/heads\//, '') -} - -function getActiveChecksStatus(state: ReturnType): CheckStatus | null { - const activeWorktree = state.activeWorktreeId - ? findWorktreeById(state.worktreesByRepo, state.activeWorktreeId) - : null - if (!activeWorktree) { - return null - } - - const activeRepo = getRepoMapFromState(state).get(activeWorktree.repoId) - if (!activeRepo) { - return null - } - - const branch = branchDisplayName(activeWorktree.branch) - if (!branch) { - return null - } - - const prCacheKey = `${activeRepo.path}::${branch}` - return state.prCache[prCacheKey]?.data?.checksStatus ?? null -} const isMac = typeof navigator !== 'undefined' && navigator.userAgent.includes('Mac') const isWindows = diff --git a/src/renderer/src/components/sidebar/WorktreeCard.tsx b/src/renderer/src/components/sidebar/WorktreeCard.tsx index 0edd6e3dd..ad63917ec 100644 --- a/src/renderer/src/components/sidebar/WorktreeCard.tsx +++ b/src/renderer/src/components/sidebar/WorktreeCard.tsx @@ -172,7 +172,9 @@ const WorktreeCard = React.memo(function WorktreeCard({ const branch = branchDisplayName(worktree.branch) const isFolder = repo ? isFolderRepo(repo) : false const hostedReviewCacheKey = - repo && branch ? getHostedReviewCacheKey(repo.path, branch, settings, repo.id) : '' + repo && branch + ? getHostedReviewCacheKey(repo.path, branch, settings, repo.id, repo.connectionId) + : '' const issueCacheKey = repo && worktree.linkedIssue ? `${repo.id}::${worktree.linkedIssue}` : '' const linearIssueCacheKey = worktree.linkedLinearIssue ? `selected::${worktree.linkedLinearIssue}` diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index b1dca0178..d41d9b730 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -521,6 +521,7 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp const cardProps = useAppStore((s) => s.worktreeCardProperties) const sshConnectedGeneration = useAppStore((s) => s.sshConnectedGeneration) const prVisibleRefreshGeneration = useAppStore((s) => s.prVisibleRefreshGeneration) + const settings = useAppStore((s) => s.settings) // Drag is only meaningful when repo headers are using manual order. The // controller is still constructed for hook order stability when inert. @@ -830,7 +831,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp targetWorktree, repoMap, prCache, - workspaceStatuses + workspaceStatuses, + settings ) if (groupKey && collapsedGroups.has(groupKey)) { toggleGroup(groupKey) @@ -877,7 +879,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp clearPendingRevealWorktreeId, toggleGroup, collapsedGroups, - workspaceStatuses + workspaceStatuses, + settings ]) const prCacheLen = useAppStore((s) => Object.keys(s.prCache).length) @@ -964,7 +967,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp repoGroupOrdering, worktreeLineageById, worktreeMap, - true + true, + settings ).filter((r): r is Extract => r.type === 'item') if (worktreeRows.length === 0) { return @@ -1009,7 +1013,8 @@ const VirtualizedWorktreeViewport = React.memo(function VirtualizedWorktreeViewp repoOrder, workspaceStatuses, worktreeLineageById, - worktreeMap + worktreeMap, + settings ] ) @@ -2282,6 +2287,7 @@ const WorktreeList = React.memo(function WorktreeList({ const prCache = useAppStore((s) => groupBy === 'pr-status' || cardProps.includes('pr') ? s.prCache : null ) + const settings = useAppStore((s) => s.settings) const sortEpoch = useAppStore((s) => s.sortEpoch) @@ -2581,7 +2587,8 @@ const WorktreeList = React.memo(function WorktreeList({ repoGroupOrdering, worktreeLineageById, worktreeMap, - true + true, + settings ), [ groupBy, @@ -2593,7 +2600,8 @@ const WorktreeList = React.memo(function WorktreeList({ workspaceStatuses, repoGroupOrdering, worktreeLineageById, - worktreeMap + worktreeMap, + settings ] ) // Why: header/mode changes can shift entire groups, so remount the diff --git a/src/renderer/src/components/sidebar/worktree-list-groups.test.ts b/src/renderer/src/components/sidebar/worktree-list-groups.test.ts index 43863cf02..698350718 100644 --- a/src/renderer/src/components/sidebar/worktree-list-groups.test.ts +++ b/src/renderer/src/components/sidebar/worktree-list-groups.test.ts @@ -46,13 +46,65 @@ const repoMap = new Map([[repo.id, repo]]) describe('getPRGroupKey', () => { it('puts merged PRs in the done group', () => { const prCache = { - '/tmp/orca::feature/super-critical': { + 'repo-1::feature/super-critical': { data: { state: 'merged' } } } expect(getPRGroupKey(worktree, repoMap, prCache)).toBe('done') }) + + it('prefers repo-scoped PR status over stale legacy path-scoped status', () => { + const prCache = { + '/tmp/orca::feature/super-critical': { + data: { state: 'closed' } + }, + 'repo-1::feature/super-critical': { + data: { state: 'merged' } + } + } + + expect(getPRGroupKey(worktree, repoMap, prCache)).toBe('done') + }) + + it('falls back to legacy path-scoped PR status when no repo-scoped entry exists', () => { + const prCache = { + '/tmp/orca::feature/super-critical': { + data: { state: 'closed' } + } + } + + expect(getPRGroupKey(worktree, repoMap, prCache)).toBe('closed') + }) + + it('does not fall back to local PR cache while runtime scoped data is loading', () => { + const prCache = { + 'repo-1::feature/super-critical': { + data: { state: 'merged' } + } + } + + expect( + getPRGroupKey(worktree, repoMap, prCache, { + activeRuntimeEnvironmentId: 'env-1' + } as never) + ).toBe('in-progress') + }) + + it('uses SSH-scoped PR cache entries instead of local entries for SSH repos', () => { + const sshRepo = { ...repo, connectionId: 'ssh-1' } + const sshRepoMap = new Map([[sshRepo.id, sshRepo]]) + const prCache = { + 'repo-1::feature/super-critical': { + data: { state: 'merged' } + }, + 'ssh:ssh-1::repo-1::feature/super-critical': { + data: { state: 'closed' } + } + } + + expect(getPRGroupKey(worktree, sshRepoMap, prCache)).toBe('closed') + }) }) describe('getGroupKeyForWorktree', () => { diff --git a/src/renderer/src/components/sidebar/worktree-list-groups.ts b/src/renderer/src/components/sidebar/worktree-list-groups.ts index c31fe6fb0..dd7bca443 100644 --- a/src/renderer/src/components/sidebar/worktree-list-groups.ts +++ b/src/renderer/src/components/sidebar/worktree-list-groups.ts @@ -21,6 +21,8 @@ import { } from './workspace-status-icons' import { cloneDefaultWorkspaceStatuses } from '../../../../shared/workspace-statuses' import type { SortBy } from './smart-sort' +import type { AppState } from '@/store/types' +import { getGitHubPRCacheKey, getLegacyGitHubPRCacheKey } from '@/store/slices/github-cache-key' export { branchName } @@ -144,15 +146,34 @@ export function getLineageRenderInfo( export function getPRGroupKey( worktree: Worktree, repoMap: Map, - prCache: Record | null + prCache: Record | null, + settings?: AppState['settings'] ): PRGroupKey { const repo = repoMap.get(worktree.repoId) const branch = branchName(worktree.branch) - const cacheKey = repo && branch ? `${repo.path}::${branch}` : '' - const prEntry = - cacheKey && prCache - ? (prCache[cacheKey] as { data?: { state?: string } } | undefined) - : undefined + const repoScopedCacheKey = + repo && branch + ? getGitHubPRCacheKey(repo.path, repo.id, branch, settings, repo.connectionId) + : '' + const canUseLegacyPRCache = + repo !== undefined && !settings?.activeRuntimeEnvironmentId?.trim() && !repo.connectionId + const legacyRepoScopedCacheKey = + canUseLegacyPRCache && branch ? getLegacyGitHubPRCacheKey(repo.path, repo.id, branch) : '' + const legacyPathScopedCacheKey = + canUseLegacyPRCache && branch ? getLegacyGitHubPRCacheKey(repo.path, undefined, branch) : '' + // Why: PR refreshes now write repo-id scoped entries; legacy path entries may + // still exist from persisted cache, but must not override fresher repo data. + const prEntry = prCache + ? ((repoScopedCacheKey + ? (prCache[repoScopedCacheKey] as { data?: { state?: string } } | undefined) + : undefined) ?? + (legacyRepoScopedCacheKey + ? (prCache[legacyRepoScopedCacheKey] as { data?: { state?: string } } | undefined) + : undefined) ?? + (legacyPathScopedCacheKey + ? (prCache[legacyPathScopedCacheKey] as { data?: { state?: string } } | undefined) + : undefined)) + : undefined const pr = prEntry?.data if (!pr) { @@ -329,7 +350,8 @@ export function buildRows( worktreeMap: Map = new Map( worktrees.map((worktree) => [worktree.id, worktree]) ), - nestLineage = false + nestLineage = false, + settings?: AppState['settings'] ): Row[] { const result: Row[] = [] @@ -378,7 +400,7 @@ export function buildRows( label = workspaceStatuses.find((status) => status.id === workspaceStatus)?.label ?? workspaceStatus } else { - const prGroup = getPRGroupKey(w, repoMap, prCache) + const prGroup = getPRGroupKey(w, repoMap, prCache, settings) key = `pr:${prGroup}` label = PR_GROUP_META[prGroup].label } @@ -491,7 +513,8 @@ export function getGroupKeyForWorktree( worktree: Worktree, repoMap: Map, prCache: Record | null, - workspaceStatuses: readonly WorkspaceStatusDefinition[] = cloneDefaultWorkspaceStatuses() + workspaceStatuses: readonly WorkspaceStatusDefinition[] = cloneDefaultWorkspaceStatuses(), + settings?: AppState['settings'] ): string | null { if (groupBy === 'none') { return ALL_GROUP_KEY @@ -502,5 +525,5 @@ export function getGroupKeyForWorktree( if (groupBy === 'repo') { return `repo:${worktree.repoId}` } - return `pr:${getPRGroupKey(worktree, repoMap, prCache)}` + return `pr:${getPRGroupKey(worktree, repoMap, prCache, settings)}` } diff --git a/src/renderer/src/store/slices/github-cache-key.ts b/src/renderer/src/store/slices/github-cache-key.ts new file mode 100644 index 000000000..4d74acbe3 --- /dev/null +++ b/src/renderer/src/store/slices/github-cache-key.ts @@ -0,0 +1,45 @@ +import type { AppState } from '../types' + +export function getGitHubRepoCacheKey( + repoPath: string, + repoId: string | undefined, + suffix: string, + settings?: AppState['settings'], + connectionId?: string | null +): string { + const runtimeEnvironmentId = settings?.activeRuntimeEnvironmentId?.trim() + const owner = repoId ?? repoPath + // Why: runtime/SSH lookups can observe different remotes than the local repo + // path, so cache keys include the active remote execution boundary. + if (runtimeEnvironmentId) { + return `runtime:${runtimeEnvironmentId}::${owner}::${suffix}` + } + const sshConnectionId = connectionId?.trim() + return sshConnectionId ? `ssh:${sshConnectionId}::${owner}::${suffix}` : `${owner}::${suffix}` +} + +export function getLegacyGitHubRepoCacheKey( + repoPath: string, + repoId: string | undefined, + suffix: string +): string { + return `${repoId ?? repoPath}::${suffix}` +} + +export function getGitHubPRCacheKey( + repoPath: string, + repoId: string | undefined, + branch: string, + settings?: AppState['settings'], + connectionId?: string | null +): string { + return getGitHubRepoCacheKey(repoPath, repoId, branch, settings, connectionId) +} + +export function getLegacyGitHubPRCacheKey( + repoPath: string, + repoId: string | undefined, + branch: string +): string { + return getLegacyGitHubRepoCacheKey(repoPath, repoId, branch) +} diff --git a/src/renderer/src/store/slices/github-checks.ts b/src/renderer/src/store/slices/github-checks.ts index c0fd035a2..3149f280d 100644 --- a/src/renderer/src/store/slices/github-checks.ts +++ b/src/renderer/src/store/slices/github-checks.ts @@ -1,5 +1,6 @@ import type { AppState } from '../types' import type { PRCheckDetail, CheckStatus, GitHubOwnerRepo } from '../../../../shared/types' +import { getGitHubPRCacheKey } from './github-cache-key' export function normalizeBranchName(branch: string): string { return branch.replace(/^refs\/heads\//, '') @@ -40,14 +41,16 @@ export function syncPRChecksStatus( branch: string | undefined, checks: PRCheckDetail[], headSha?: string, - prRepo?: GitHubOwnerRepo | null + prRepo?: GitHubOwnerRepo | null, + settings?: AppState['settings'], + connectionId?: string | null ): Partial | null { const normalized = branch ? normalizeBranchName(branch) : '' if (!normalized) { return null } - const prCacheKey = `${repoId ?? repoPath}::${normalized}` + const prCacheKey = getGitHubPRCacheKey(repoPath, repoId, normalized, settings, connectionId) const prEntry = state.prCache[prCacheKey] if (!prEntry?.data) { return null diff --git a/src/renderer/src/store/slices/github.test.ts b/src/renderer/src/store/slices/github.test.ts index 485ebd534..7a241f530 100644 --- a/src/renderer/src/store/slices/github.test.ts +++ b/src/renderer/src/store/slices/github.test.ts @@ -4,13 +4,16 @@ GitHub slice's cross-cutting invariants verifiable in one place. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { create } from 'zustand' import { createGitHubSlice, prChecksCacheSuffix, workItemsCacheKey } from './github' +import { createHostedReviewSlice } from './hosted-review' import type { AppState } from '../types' import type { GitHubWorkItem, PRInfo } from '../../../../shared/types' +import type { HostedReviewInfo } from '../../../../shared/hosted-review' import { createCompatibleRuntimeStatusResponseIfNeeded, type RuntimeEnvironmentCallRequest } from '../../runtime/runtime-compatibility-test-fixture' import { clearRuntimeCompatibilityCacheForTests } from '../../runtime/runtime-rpc-client' +import { getHostedReviewCacheKey } from './hosted-review-cache-identity' const runtimeEnvironmentCall = vi.fn() const runtimeEnvironmentTransportCall = vi.fn() @@ -26,6 +29,11 @@ const mockApi = { listWorkItems: vi.fn(), getProjectViewTable: vi.fn() }, + hostedReview: { + forBranch: vi.fn().mockResolvedValue(null), + getCreationEligibility: vi.fn(), + create: vi.fn() + }, runtimeEnvironments: { call: runtimeEnvironmentTransportCall }, @@ -51,7 +59,8 @@ function createTestStore() { return create()( (...a) => ({ - ...createGitHubSlice(...a) + ...createGitHubSlice(...a), + ...createHostedReviewSlice(...a) }) as AppState ) } @@ -247,6 +256,49 @@ describe('createGitHubSlice.fetchPRChecks', () => { expect(store.getState().prCache[prCacheKey]?.data?.checksStatus).toBe('success') }) + it('stores runtime checks under runtime-scoped cache keys', async () => { + runtimeEnvironmentCall.mockResolvedValueOnce({ + id: 'rpc-checks', + ok: true, + result: [{ name: 'build', status: 'completed', conclusion: 'success', url: null }], + _meta: { runtimeId: 'remote-runtime' } + }) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + const branch = 'feature/runtime-checks' + const runtimePrCacheKey = `runtime:env-1::${repoId}::${branch}` + const runtimeChecksCacheKey = `runtime:env-1::${repoId}::pr-checks::12` + const localChecksCacheKey = `${repoId}::pr-checks::12` + + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }], + prCache: { + [runtimePrCacheKey]: { + data: makePR({ checksStatus: 'pending' }), + fetchedAt: 1 + } + } + } as unknown as Partial) + + await store + .getState() + .fetchPRChecks(repoPath, 12, branch, undefined, null, { force: true, repoId }) + + expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'github.prChecks', + params: { repo: repoId, prNumber: 12, headSha: undefined, prRepo: null, noCache: true }, + timeoutMs: 30_000 + }) + expect(store.getState().checksCache[runtimeChecksCacheKey]?.data).toEqual([ + { name: 'build', status: 'completed', conclusion: 'success', url: null } + ]) + expect(store.getState().checksCache[localChecksCacheKey]).toBeUndefined() + expect(store.getState().prCache[runtimePrCacheKey]?.data?.checksStatus).toBe('success') + }) + it('marks the PR cache entry as failure when any check fails', async () => { const store = createTestStore() const repoPath = '/repo' @@ -576,6 +628,48 @@ describe('createGitHubSlice.fetchPRComments', () => { }) }) + it('stores runtime PR comments under runtime-scoped cache keys', async () => { + runtimeEnvironmentCall.mockResolvedValueOnce({ + id: 'rpc-comments', + ok: true, + result: [{ id: 1, author: 'remote', authorAvatarUrl: '', body: '', createdAt: '', url: '' }], + _meta: { runtimeId: 'remote-runtime' } + }) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-id' + + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }] + } as unknown as Partial) + + await store.getState().fetchPRComments(repoPath, 12, { + force: true, + repoId, + prRepo: { owner: 'Acme', repo: 'Widgets' } + }) + + expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'github.prComments', + params: { + repo: repoId, + prNumber: 12, + prRepo: { owner: 'Acme', repo: 'Widgets' }, + noCache: true + }, + timeoutMs: 30_000 + }) + expect( + store.getState().commentsCache[`runtime:env-1::${repoId}::pr-comments::acme/widgets::12`] + ?.data?.[0].author + ).toBe('remote') + expect( + store.getState().commentsCache[`${repoId}::pr-comments::acme/widgets::12`] + ).toBeUndefined() + }) + it('preserves cached checks when the checks IPC fails', async () => { const store = createTestStore() const repoPath = '/repo' @@ -640,6 +734,7 @@ describe('createGitHubSlice.fetchPRForBranch', () => { mockApi.gh.prForBranch.mockResolvedValue(null) mockApi.gh.refreshPRNow.mockReset() mockApi.gh.refreshPRNow.mockResolvedValue({ kind: 'no-pr', fetchedAt: Date.now() }) + mockApi.hostedReview.forBranch.mockResolvedValue(null) }) it('lets a forced refresh bypass a non-forced inflight request and keeps the newer result', async () => { @@ -714,12 +809,218 @@ describe('createGitHubSlice.fetchPRForBranch', () => { repoId: 'repo-1', repoPath, branch, - cacheKey: `repo-1::${branch}`, + cacheKey: `ssh:ssh-1::repo-1::${branch}`, connectionId: 'ssh-1' }) }) }) + it('does not reuse local fresh PR cache for SSH-backed repos', async () => { + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/test' + const pr = makePR({ number: 44 }) + + store.setState({ + repos: [ + { + id: 'repo-1', + path: repoPath, + name: 'repo', + kind: 'git', + connectionId: 'ssh-1' + } + ], + prCache: { + [`repo-1::${branch}`]: { + data: makePR({ number: 12, title: 'Local stale PR' }), + fetchedAt: Date.now() + } + } + } as unknown as Partial) + mockApi.gh.refreshPRNow.mockResolvedValueOnce({ + kind: 'found', + pr, + fetchedAt: Date.now() + }) + + await expect(store.getState().fetchPRForBranch(repoPath, branch)).resolves.toMatchObject({ + number: 44 + }) + + expect(mockApi.gh.refreshPRNow).toHaveBeenCalled() + expect(store.getState().prCache[`ssh:ssh-1::repo-1::${branch}`]?.data).toMatchObject({ + number: 44 + }) + expect(store.getState().prCache[`repo-1::${branch}`]?.data).toMatchObject({ + title: 'Local stale PR' + }) + }) + + it('writes direct PR refresh results to the hosted-review scope captured at request start', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/scope-switch' + const localHostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const runtimeHostedReviewCacheKey = getHostedReviewCacheKey( + repoPath, + branch, + { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repoId + ) + let resolveRefresh: ( + value: Awaited> + ) => void = () => {} + const refresh = new Promise>>((resolve) => { + resolveRefresh = resolve + }) + mockApi.gh.refreshPRNow.mockReturnValueOnce(refresh) + + store.setState({ + settings: null, + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }] + } as unknown as Partial) + + const request = store.getState().fetchPRForBranch(repoPath, branch, { + force: true, + repoId + }) + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'] + } as Partial) + resolveRefresh({ + kind: 'found', + pr: makePR({ number: 12, title: 'Local request result' }), + fetchedAt: 2 + }) + + await expect(request).resolves.toMatchObject({ title: 'Local request result' }) + expect(store.getState().hostedReviewCache[localHostedReviewCacheKey]).toMatchObject({ + data: expect.objectContaining({ provider: 'github', title: 'Local request result' }), + linkedReviewHintKey: 'github:12' + }) + expect(store.getState().hostedReviewCache[runtimeHostedReviewCacheKey]).toBeUndefined() + }) + + it('does not let an older direct PR refresh overwrite a newer hosted-review cache entry', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/newer-hosted-review' + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const newerReview: HostedReviewInfo = { + provider: 'github', + number: 12, + title: 'Newer hosted review status', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'success', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' + } + let resolveRefresh: ( + value: Awaited> + ) => void = () => {} + const refresh = new Promise>>((resolve) => { + resolveRefresh = resolve + }) + mockApi.gh.refreshPRNow.mockReturnValueOnce(refresh) + + store.setState({ + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }] + } as unknown as Partial) + + const request = store.getState().fetchPRForBranch(repoPath, branch, { + force: true, + repoId + }) + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: newerReview, + fetchedAt: Date.now() + 1_000, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + resolveRefresh({ + kind: 'found', + pr: makePR({ number: 12, title: 'Older direct PR refresh' }), + fetchedAt: Date.now() + 2_000 + }) + + await expect(request).resolves.toMatchObject({ title: 'Older direct PR refresh' }) + expect(store.getState().prCache[`${repoId}::${branch}`]).toBeUndefined() + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: newerReview, + fetchedAt: expect.any(Number), + linkedReviewHintKey: 'github:12' + }) + }) + + it('does not let a same-millisecond direct PR refresh overwrite an external hosted-review write', async () => { + vi.useFakeTimers() + vi.setSystemTime(100) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/same-ms-hosted-review' + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const externalReview: HostedReviewInfo = { + provider: 'github', + number: 12, + title: 'Same-ms external hosted review status', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'success', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' + } + let resolveRefresh: ( + value: Awaited> + ) => void = () => {} + const refresh = new Promise>>((resolve) => { + resolveRefresh = resolve + }) + mockApi.gh.refreshPRNow.mockReturnValueOnce(refresh) + + try { + store.setState({ + repos: [{ id: repoId, path: repoPath, name: 'repo', kind: 'git' }] + } as unknown as Partial) + + const request = store.getState().fetchPRForBranch(repoPath, branch, { + force: true, + repoId + }) + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: externalReview, + fetchedAt: Date.now(), + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + resolveRefresh({ + kind: 'found', + pr: makePR({ number: 12, title: 'Same-ms direct PR refresh' }), + fetchedAt: Date.now() + }) + + await expect(request).resolves.toMatchObject({ title: 'Same-ms direct PR refresh' }) + expect(store.getState().prCache[`${repoId}::${branch}`]).toBeUndefined() + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: externalReview, + fetchedAt: 100, + linkedReviewHintKey: 'github:12' + }) + } finally { + vi.useRealTimers() + } + }) + it('preserves cached PR data when a forced coordinator refresh errors', async () => { const store = createTestStore() const repoPath = '/repo' @@ -783,6 +1084,567 @@ describe('createGitHubSlice.fetchPRForBranch', () => { message: 'network unavailable' }) }) + + it('updates hosted review cache from GitHub PR refresh events', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/test' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: { + provider: 'github', + number: 12, + title: 'Old PR status', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN' + }, + fetchedAt: 1, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ + number: 12, + title: 'Fresh PR status', + checksStatus: 'success', + mergeable: 'MERGEABLE' + }), + fetchedAt: 2 + } + }) + + expect(store.getState().prCache[cacheKey]).toMatchObject({ + data: expect.objectContaining({ title: 'Fresh PR status', checksStatus: 'success' }), + fetchedAt: 2 + }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toMatchObject({ + data: expect.objectContaining({ + provider: 'github', + title: 'Fresh PR status', + status: 'success', + mergeable: 'MERGEABLE' + }), + fetchedAt: 2, + linkedReviewHintKey: 'github:12' + }) + }) + + it('does not let an older GitHub PR refresh event overwrite a newer hosted-review cache entry', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/event-race' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const newerReview: HostedReviewInfo = { + provider: 'github', + number: 12, + title: 'Newer hosted review status', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'success', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' + } + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: newerReview, + fetchedAt: 3, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ number: 12, title: 'Older event PR status' }), + fetchedAt: 2 + } + }) + + expect(store.getState().prCache[cacheKey]).toBeUndefined() + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: newerReview, + fetchedAt: 3, + linkedReviewHintKey: 'github:12' + }) + }) + + it('uses event request start time to reject older PR refreshes that finish later', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/start-race' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const newerReview: HostedReviewInfo = { + provider: 'github', + number: 12, + title: 'Newer hosted review status', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'success', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' + } + const stalePR = makePR({ number: 12, title: 'Stale PR status' }) + + store.setState({ + prCache: { + [cacheKey]: { + data: stalePR, + fetchedAt: 1 + } + }, + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: newerReview, + fetchedAt: 3, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + requestStartedAt: 2, + outcome: { + kind: 'found', + pr: makePR({ number: 12, title: 'Older request finished late' }), + fetchedAt: 4 + } + }) + + expect(store.getState().prCache[cacheKey]).toBeUndefined() + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: newerReview, + fetchedAt: 3, + linkedReviewHintKey: 'github:12' + }) + }) + + it('uses the in-flight event entry to allow same-millisecond coordinator refreshes', () => { + vi.useFakeTimers() + vi.setSystemTime(100) + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/event-same-ms' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const existingReview: HostedReviewInfo = { + provider: 'github', + number: 12, + title: 'Existing same-ms hosted review status', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN' + } + + try { + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: existingReview, + fetchedAt: 100, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + requestStartedAt: 100, + status: 'in-flight' + }) + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + requestStartedAt: 100, + outcome: { + kind: 'found', + pr: makePR({ number: 12, title: 'Fresh same-ms event PR status' }), + fetchedAt: 100 + } + }) + + expect(store.getState().prCache[cacheKey]?.data).toMatchObject({ + title: 'Fresh same-ms event PR status' + }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toMatchObject({ + data: expect.objectContaining({ title: 'Fresh same-ms event PR status' }), + fetchedAt: 100 + }) + } finally { + vi.useRealTimers() + } + }) + + it('does not overwrite a non-GitHub hosted review from GitHub PR refresh events', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/gitlab-review' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const gitlabReview: HostedReviewInfo = { + provider: 'gitlab', + number: 5, + title: 'GitLab MR', + state: 'open', + url: 'https://gitlab.com/acme/orca/-/merge_requests/5', + status: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN' + } + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: gitlabReview, + fetchedAt: 1, + linkedReviewHintKey: 'gitlab:5' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ number: 12, title: 'GitHub PR status' }), + fetchedAt: 2 + } + }) + + expect(store.getState().prCache[cacheKey]).toBeUndefined() + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: gitlabReview, + fetchedAt: 1, + linkedReviewHintKey: 'gitlab:5' + }) + }) + + it('does not apply local GitHub PR refresh events while a runtime is active', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/runtime' + const cacheKey = `${repoId}::${branch}` + const settings = { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'] + const runtimeHostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, settings, repoId) + + store.setState({ settings } as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ number: 12, title: 'Local PR status' }), + fetchedAt: 2 + } + }) + + expect(store.getState().prCache[cacheKey]).toBeUndefined() + expect(store.getState().prRefreshSequences[cacheKey]).toBeUndefined() + expect(store.getState().hostedReviewCache[runtimeHostedReviewCacheKey]).toBeUndefined() + }) + + it('does not create hosted review cache entries from GitHub no-PR refreshes', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/missing' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { kind: 'no-pr', fetchedAt: 2 } + }) + + expect(store.getState().prCache[cacheKey]).toEqual({ data: null, fetchedAt: 2 }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toBeUndefined() + }) + + it('does not refresh provider-neutral null hosted review cache on a GitHub no-PR refresh', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/neutral' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: null, + fetchedAt: 1 + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { kind: 'no-pr', fetchedAt: 2 } + }) + + expect(store.getState().prCache[cacheKey]).toEqual({ data: null, fetchedAt: 2 }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: null, + fetchedAt: 1 + }) + }) + + it('clears GitHub-scoped null hosted review cache on a GitHub no-PR refresh', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/github-null' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: null, + fetchedAt: 1, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { kind: 'no-pr', fetchedAt: 2 } + }) + + expect(store.getState().prCache[cacheKey]).toEqual({ data: null, fetchedAt: 2 }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: null, + fetchedAt: 2, + linkedReviewHintKey: 'github:12' + }) + }) + + it('does not reuse a GitHub-scoped null hosted review cache for neutral discovery', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/github-null-then-gitlab' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const gitlabReview: HostedReviewInfo = { + provider: 'gitlab', + number: 5, + title: 'GitLab MR', + state: 'open', + url: 'https://gitlab.com/acme/orca/-/merge_requests/5', + status: 'success', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' + } + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: null, + fetchedAt: 1, + linkedReviewHintKey: 'github:12' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { kind: 'no-pr', fetchedAt: 2 } + }) + mockApi.hostedReview.forBranch.mockResolvedValueOnce(gitlabReview) + + await expect( + store.getState().fetchHostedReviewForBranch(repoPath, branch, { repoId }) + ).resolves.toEqual(gitlabReview) + expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(1) + expect(mockApi.hostedReview.forBranch).toHaveBeenCalledWith({ + branch, + linkedAzureDevOpsPR: null, + linkedBitbucketPR: null, + linkedGitHubPR: null, + linkedGitLabMR: null, + linkedGiteaPR: null, + repoId, + repoPath + }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: gitlabReview, + fetchedAt: expect.any(Number), + linkedReviewHintKey: '' + }) + }) + + it('does not reuse a GitHub-scoped PR hit for neutral hosted review discovery', async () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/github-hit-then-gitlab' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const gitlabReview: HostedReviewInfo = { + provider: 'gitlab', + number: 5, + title: 'GitLab MR', + state: 'open', + url: 'https://gitlab.com/acme/orca/-/merge_requests/5', + status: 'success', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' + } + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { + kind: 'found', + pr: makePR({ number: 12, title: 'GitHub PR status' }), + fetchedAt: 2 + } + }) + mockApi.hostedReview.forBranch.mockResolvedValueOnce(gitlabReview) + + await expect( + store.getState().fetchHostedReviewForBranch(repoPath, branch, { repoId }) + ).resolves.toEqual(gitlabReview) + expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(1) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: gitlabReview, + fetchedAt: expect.any(Number), + linkedReviewHintKey: '' + }) + }) + + it('keeps cleared GitHub hosted review data scoped to GitHub PR discovery', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/github-data' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: { + provider: 'github', + number: 12, + title: 'Old GitHub PR', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + status: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN' + }, + fetchedAt: 1 + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { kind: 'no-pr', fetchedAt: 2 } + }) + + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: null, + fetchedAt: 2, + linkedReviewHintKey: 'github:12' + }) + }) + + it('does not clear non-GitHub hosted review cache on a GitHub no-PR refresh', () => { + const store = createTestStore() + const repoPath = '/repo' + const repoId = 'repo-1' + const branch = 'feature/gitlab' + const cacheKey = `${repoId}::${branch}` + const hostedReviewCacheKey = getHostedReviewCacheKey(repoPath, branch, null, repoId) + const gitlabReview = { + provider: 'gitlab' as const, + number: 5, + title: 'GitLab MR', + state: 'open' as const, + url: 'https://gitlab.com/acme/orca/-/merge_requests/5', + status: 'success' as const, + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'MERGEABLE' as const + } + + store.setState({ + hostedReviewCache: { + [hostedReviewCacheKey]: { + data: gitlabReview, + fetchedAt: 1, + linkedReviewHintKey: 'gitlab:5' + } + } + } as unknown as Partial) + + store.getState().applyGitHubPRRefreshEvent({ + sequence: 1, + aliases: [{ cacheKey, repoId, repoPath, branch }], + reason: 'visible', + outcome: { kind: 'no-pr', fetchedAt: 2 } + }) + + expect(store.getState().prCache[cacheKey]).toBeUndefined() + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toEqual({ + data: gitlabReview, + fetchedAt: 1, + linkedReviewHintKey: 'gitlab:5' + }) + }) }) describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => { @@ -952,6 +1814,72 @@ describe('createGitHubSlice.refreshGitHubForWorktreeIfStale', () => { priority: 80 }) }) + + it('fetches PR through the runtime when activating a runtime workspace', async () => { + resetRemoteRuntimeMocks() + runtimeEnvironmentCall.mockResolvedValueOnce({ + id: 'rpc-1', + ok: true, + result: makePR({ number: 12 }), + _meta: { runtimeId: 'remote-runtime' } + }) + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/runtime' + const worktreeId = 'wt-runtime' + const hostedReviewCacheKey = getHostedReviewCacheKey( + repoPath, + branch, + { + activeRuntimeEnvironmentId: 'env-1' + } as AppState['settings'], + 'repo-1' + ) + + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + groupBy: 'pr-status', + worktreeCardProperties: ['pr'], + worktreesByRepo: { + 'repo-1': [ + { + id: worktreeId, + repoId: 'repo-1', + path: '/repo/worktrees/runtime', + branch, + displayName: 'runtime', + isMainWorktree: false, + isBare: false, + isArchived: false, + linkedPR: 12 + } + ] + } + } as unknown as Partial) + + store.getState().refreshGitHubForWorktreeIfStale(worktreeId) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled() + expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'github.prForBranch', + params: { repo: 'repo-1', branch, linkedPRNumber: 12 }, + timeoutMs: 30_000 + }) + expect(store.getState().hostedReviewCache[hostedReviewCacheKey]).toMatchObject({ + data: expect.objectContaining({ + provider: 'github', + number: 12 + }), + linkedReviewHintKey: 'github:12' + }) + expect(store.getState().prCache[`runtime:env-1::repo-1::${branch}`]?.data).toMatchObject({ + number: 12 + }) + expect(store.getState().prCache[`repo-1::${branch}`]).toBeUndefined() + }) }) describe('createGitHubSlice.refreshAllGitHub', () => { @@ -995,6 +1923,103 @@ describe('createGitHubSlice.refreshAllGitHub', () => { priority: 10 }) }) + + it('refreshes runtime PR data directly instead of enqueueing local coordinator work', async () => { + runtimeEnvironmentCall.mockResolvedValueOnce({ + id: 'rpc-1', + ok: true, + result: makePR({ number: 12 }), + _meta: { runtimeId: 'remote-runtime' } + }) + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/runtime' + + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + groupBy: 'repo', + worktreeCardProperties: ['comment'], + rightSidebarOpen: true, + rightSidebarTab: 'source-control', + worktreesByRepo: { + 'repo-1': [ + { + id: 'wt-1', + repoId: 'repo-1', + path: '/repo/worktrees/runtime', + branch, + displayName: 'runtime', + isMainWorktree: false, + isBare: false, + isArchived: false, + lastActivityAt: 1 + } + ] + } + } as unknown as Partial) + + store.getState().refreshAllGitHub() + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled() + expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'github.prForBranch', + params: { repo: 'repo-1', branch, linkedPRNumber: null }, + timeoutMs: 30_000 + }) + }) +}) + +describe('createGitHubSlice.refreshGitHubForWorktree', () => { + beforeEach(() => { + vi.clearAllMocks() + resetRemoteRuntimeMocks() + }) + + it('refreshes runtime PR data directly after invalidating a worktree', async () => { + runtimeEnvironmentCall.mockResolvedValueOnce({ + id: 'rpc-1', + ok: true, + result: makePR({ number: 12 }), + _meta: { runtimeId: 'remote-runtime' } + }) + const store = createTestStore() + const repoPath = '/repo' + const branch = 'feature/runtime' + const worktreeId = 'wt-runtime' + + store.setState({ + settings: { activeRuntimeEnvironmentId: 'env-1' } as AppState['settings'], + repos: [{ id: 'repo-1', path: repoPath, name: 'repo', kind: 'git' }], + worktreesByRepo: { + 'repo-1': [ + { + id: worktreeId, + repoId: 'repo-1', + path: '/repo/worktrees/runtime', + branch, + displayName: 'runtime', + isMainWorktree: false, + isBare: false, + isArchived: false + } + ] + } + } as unknown as Partial) + + store.getState().refreshGitHubForWorktree(worktreeId) + await new Promise((resolve) => setTimeout(resolve, 0)) + + expect(mockApi.gh.enqueuePRRefresh).not.toHaveBeenCalled() + expect(runtimeEnvironmentCall).toHaveBeenCalledWith({ + selector: 'env-1', + method: 'github.prForBranch', + params: { repo: 'repo-1', branch, linkedPRNumber: null }, + timeoutMs: 30_000 + }) + }) }) describe('createGitHubSlice.fetchWorkItems source/error envelope', () => { diff --git a/src/renderer/src/store/slices/github.ts b/src/renderer/src/store/slices/github.ts index 25f7d45f3..fa9702a32 100644 --- a/src/renderer/src/store/slices/github.ts +++ b/src/renderer/src/store/slices/github.ts @@ -30,6 +30,9 @@ import type { import { sortWorkItemsByUpdatedAt, PER_REPO_FETCH_LIMIT } from '../../../../shared/work-items' import { deriveCheckStatusFromChecks, syncPRChecksStatus } from './github-checks' import { callRuntimeRpc, getActiveRuntimeTarget } from '../../runtime/runtime-rpc-client' +import { hostedReviewInfoFromGitHubPRInfo } from '../../../../shared/hosted-review-github' +import { getHostedReviewCacheKey, linkedReviewHintKey } from './hosted-review-cache-identity' +import { getGitHubPRCacheKey, getGitHubRepoCacheKey } from './github-cache-key' // ─── ProjectV2 cache types ──────────────────────────────────────────── // Why: declared separately from CacheEntry (not a generified E parameter) @@ -84,9 +87,10 @@ function queryOverrideKeyPart(queryOverride: string | undefined): string { function getRuntimeRepoTarget( state: AppState, - repoPath: string + repoPath: string, + settings: AppState['settings'] = state.settings ): { target: { kind: 'environment'; environmentId: string }; repo: Repo } | null { - const target = getActiveRuntimeTarget(state.settings) + const target = getActiveRuntimeTarget(settings) if (target.kind !== 'environment') { return null } @@ -346,6 +350,10 @@ type InflightWorkItems = { } const inflightWorkItemsRequests = new Map() const prRequestGenerations = new Map() +const prRefreshStartedHostedReviewEntries = new Map< + string, + AppState['hostedReviewCache'][string] | undefined +>() // Why: cap in-flight cross-repo fan-out and hover-prefetches at the renderer // boundary — the main-side gate is behind the IPC queue, so it can't see a @@ -384,6 +392,26 @@ function repoScopedCacheKey(repoPath: string, repoId: string | undefined, suffix return `${repoId ?? repoPath}::${suffix}` } +function runtimeScopedRepoCacheKey( + repoPath: string, + repoId: string | undefined, + suffix: string, + settings?: AppState['settings'], + connectionId?: string | null +): string { + return getGitHubRepoCacheKey(repoPath, repoId, suffix, settings, connectionId) +} + +function prCacheKey( + repoPath: string, + repoId: string | undefined, + branch: string, + settings?: AppState['settings'], + connectionId?: string | null +): string { + return getGitHubPRCacheKey(repoPath, repoId, branch, settings, connectionId) +} + function repoCacheKeyPrefixes(repoId: string, repoPath?: string): string[] { const prefixes = [`${repoId}::`] if (repoPath && repoPath !== repoId) { @@ -479,7 +507,13 @@ function buildPRRefreshCandidate( return null } const branch = worktree.branch.replace(/^refs\/heads\//, '') - const cacheKey = repoScopedCacheKey(repoPath ?? repo.path, repo.id, branch) + const cacheKey = prCacheKey( + repoPath ?? repo.path, + repo.id, + branch, + state.settings, + repo.connectionId + ) const sshStatus = repo.connectionId ? state.sshConnectionStates.get(repo.connectionId)?.status : null @@ -506,6 +540,224 @@ function buildPRRefreshCandidate( } } +function shouldClearHostedReviewForNoGitHubPR( + entry: AppState['hostedReviewCache'][string] | undefined +): boolean { + // Why: a GitHub-only miss should not create or refresh provider-neutral + // branch misses that suppress discovery for GitLab/other hosted reviews. + if (!entry) { + return false + } + if (entry.data?.provider === 'github') { + return true + } + return entry.data === null && isGitHubLinkedReviewHintKey(entry.linkedReviewHintKey) +} + +function isGitHubLinkedReviewHintKey(hintKey: string | undefined): boolean { + return hintKey?.split('|').some((key) => key.startsWith('github:')) ?? false +} + +function linkedReviewHintKeyForNoGitHubPR( + entry: AppState['hostedReviewCache'][string] | undefined +): string | undefined { + if (entry?.data?.provider === 'github') { + return isGitHubLinkedReviewHintKey(entry.linkedReviewHintKey) + ? entry.linkedReviewHintKey + : linkedReviewHintKey({ linkedGitHubPR: entry.data.number }) + } + return entry?.linkedReviewHintKey +} + +function hasNewerHostedReviewCacheEntry( + cache: AppState['hostedReviewCache'], + cacheKey: string, + requestStartedAt: number, + requestStartedEntry: AppState['hostedReviewCache'][string] | undefined +): boolean { + const entry = cache[cacheKey] + return ( + entry !== undefined && + (entry.fetchedAt > requestStartedAt || + (entry.fetchedAt === requestStartedAt && entry !== requestStartedEntry)) + ) +} + +function syncHostedReviewCacheFromGitHubPRResult(args: { + cache: AppState['hostedReviewCache'] + repoPath: string + branch: string + settings: AppState['settings'] + repoId?: string + connectionId?: string | null + pr: PRInfo | null + fetchedAt: number + requestStartedAt?: number + requestStartedEntry?: AppState['hostedReviewCache'][string] +}): { cache: AppState['hostedReviewCache']; accepted: boolean } { + const hostedReviewCacheKey = getHostedReviewCacheKey( + args.repoPath, + args.branch, + args.settings, + args.repoId, + args.connectionId + ) + if ( + args.requestStartedAt !== undefined && + hasNewerHostedReviewCacheEntry( + args.cache, + hostedReviewCacheKey, + args.requestStartedAt, + args.requestStartedEntry + ) + ) { + return { cache: args.cache, accepted: false } + } + const hostedReviewEntry = args.cache[hostedReviewCacheKey] + if ( + args.requestStartedAt === undefined && + hostedReviewEntry !== undefined && + hostedReviewEntry.fetchedAt >= args.fetchedAt + ) { + return { cache: args.cache, accepted: false } + } + if (args.pr && hostedReviewEntry?.data && hostedReviewEntry.data.provider !== 'github') { + return { cache: args.cache, accepted: false } + } + if (!args.pr && !shouldClearHostedReviewForNoGitHubPR(hostedReviewEntry)) { + return { cache: args.cache, accepted: hostedReviewEntry?.data == null } + } + return { + cache: { + ...args.cache, + [hostedReviewCacheKey]: { + data: args.pr ? hostedReviewInfoFromGitHubPRInfo(args.pr) : null, + fetchedAt: args.fetchedAt, + linkedReviewHintKey: args.pr + ? linkedReviewHintKey({ linkedGitHubPR: args.pr.number }) + : linkedReviewHintKeyForNoGitHubPR(hostedReviewEntry) + } + }, + accepted: true + } +} + +function shouldWritePRCacheForHostedReviewSync(args: { + hostedReviewSyncAccepted: boolean +}): boolean { + // Why: PR-status grouping reads prCache while cards read hostedReviewCache. + // If a GitHub PR result was rejected for the card, don't let grouping drift. + return args.hostedReviewSyncAccepted +} + +function applyPRCacheResult( + cache: AppState['prCache'], + cacheKey: string, + pr: PRInfo | null, + fetchedAt: number, + accepted: boolean +): AppState['prCache'] { + if (accepted) { + return { ...cache, [cacheKey]: { data: pr, fetchedAt } } + } + if (!cache[cacheKey]) { + return cache + } + const next = { ...cache } + delete next[cacheKey] + return next +} + +function prRefreshStartedEntryKey(sequence: number, cacheKey: string): string { + return `${sequence}::${cacheKey}` +} + +function setGitHubPRResultCaches( + state: AppState, + args: { + prCacheKey: string + repoPath: string + branch: string + settings: AppState['settings'] + repoId?: string + connectionId?: string | null + pr: PRInfo | null + fetchedAt: number + requestStartedAt?: number + requestStartedEntry?: AppState['hostedReviewCache'][string] + } +): Partial { + const hostedReviewSync = syncHostedReviewCacheFromGitHubPRResult({ + cache: state.hostedReviewCache, + repoPath: args.repoPath, + branch: args.branch, + settings: args.settings, + repoId: args.repoId, + connectionId: args.connectionId, + pr: args.pr, + fetchedAt: args.fetchedAt, + requestStartedAt: args.requestStartedAt, + requestStartedEntry: args.requestStartedEntry + }) + return { + prCache: applyPRCacheResult( + state.prCache, + args.prCacheKey, + args.pr, + args.fetchedAt, + shouldWritePRCacheForHostedReviewSync({ + hostedReviewSyncAccepted: hostedReviewSync.accepted + }) + ), + ...(hostedReviewSync.cache === state.hostedReviewCache + ? {} + : { hostedReviewCache: hostedReviewSync.cache }) + } +} + +function applyGitHubPRResultToCaches(args: { + prCache: AppState['prCache'] + hostedReviewCache: AppState['hostedReviewCache'] + prCacheKey: string + repoPath: string + branch: string + settings: AppState['settings'] + repoId?: string + connectionId?: string | null + pr: PRInfo | null + fetchedAt: number + requestStartedAt?: number + requestStartedEntry?: AppState['hostedReviewCache'][string] +}): { + prCache: AppState['prCache'] + hostedReviewCache: AppState['hostedReviewCache'] +} { + const hostedReviewSync = syncHostedReviewCacheFromGitHubPRResult({ + cache: args.hostedReviewCache, + repoPath: args.repoPath, + branch: args.branch, + settings: args.settings, + repoId: args.repoId, + connectionId: args.connectionId, + pr: args.pr, + fetchedAt: args.fetchedAt, + requestStartedAt: args.requestStartedAt, + requestStartedEntry: args.requestStartedEntry + }) + return { + prCache: applyPRCacheResult( + args.prCache, + args.prCacheKey, + args.pr, + args.fetchedAt, + shouldWritePRCacheForHostedReviewSync({ + hostedReviewSyncAccepted: hostedReviewSync.accepted + }) + ), + hostedReviewCache: hostedReviewSync.cache + } +} + /** * Evict the oldest entries from a cache record when it exceeds the max size. * Returns a pruned copy, or the original reference if no eviction was needed. @@ -1427,8 +1679,16 @@ export const createGitHubSlice: StateCreator = (s options?.repoId ? candidate.id === options.repoId : candidate.path === repoPath ) const repoId = options?.repoId ?? repo?.id - const cacheKey = repoScopedCacheKey(repoPath, repoId, branch) + const requestSettings = get().settings + const cacheKey = prCacheKey(repoPath, repoId, branch, requestSettings, repo?.connectionId) const cached = get().prCache[cacheKey] + const hostedReviewCacheKey = getHostedReviewCacheKey( + repoPath, + branch, + requestSettings, + repoId, + repo?.connectionId + ) // Why: if a prior caller without a linkedPR cached `null` for this branch, // the worktree-card lookup (which has a linked PR fallback) would otherwise // return null forever. Refetch when the cached miss could now resolve via @@ -1444,12 +1704,14 @@ export const createGitHubSlice: StateCreator = (s } const generation = (prRequestGenerations.get(cacheKey) ?? 0) + 1 + const requestStartedAt = Date.now() + const requestStartedHostedReviewEntry = get().hostedReviewCache[hostedReviewCacheKey] prRequestGenerations.set(cacheKey, generation) const linkedPRNumber = options?.linkedPRNumber ?? null const request = (async () => { try { - const runtimeRepo = getRuntimeRepoTarget(get(), repoPath) + const runtimeRepo = getRuntimeRepoTarget(get(), repoPath, requestSettings) const outcome = runtimeRepo ? await callRuntimeRpc( runtimeRepo.target, @@ -1488,9 +1750,20 @@ export const createGitHubSlice: StateCreator = (s return cached?.data ?? null } if (prRequestGenerations.get(cacheKey) === generation) { - set((s) => ({ - prCache: { ...s.prCache, [cacheKey]: { data: pr, fetchedAt: outcome.fetchedAt } } - })) + set((s) => + setGitHubPRResultCaches(s, { + prCacheKey: cacheKey, + repoPath, + branch, + settings: requestSettings, + repoId, + connectionId: repo?.connectionId, + pr, + fetchedAt: outcome.fetchedAt, + requestStartedAt, + requestStartedEntry: requestStartedHostedReviewEntry + }) + ) debouncedSaveCache(get()) } return pr ?? null @@ -1558,14 +1831,26 @@ export const createGitHubSlice: StateCreator = (s prRepo, options ): Promise => { - const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id - const cacheKey = repoScopedCacheKey( + const repo = get().repos?.find((candidate) => + options?.repoId ? candidate.id === options.repoId : candidate.path === repoPath + ) + const repoId = options?.repoId ?? repo?.id + const requestSettings = get().settings + const cacheKey = runtimeScopedRepoCacheKey( repoPath, repoId, - prChecksCacheSuffix(prNumber, prRepo, headSha) + prChecksCacheSuffix(prNumber, prRepo, headSha), + requestSettings, + repo?.connectionId ) const legacyCacheKey = headSha - ? repoScopedCacheKey(repoPath, repoId, prChecksCacheSuffix(prNumber, prRepo)) + ? runtimeScopedRepoCacheKey( + repoPath, + repoId, + prChecksCacheSuffix(prNumber, prRepo), + requestSettings, + repo?.connectionId + ) : cacheKey const inflightKey = cacheKey const cached = get().checksCache[cacheKey] ?? get().checksCache[legacyCacheKey] @@ -1582,7 +1867,9 @@ export const createGitHubSlice: StateCreator = (s branch, cachedChecks, cached.headSha, - prRepo + prRepo, + requestSettings, + repo?.connectionId ) if (prStatusUpdate) { set(prStatusUpdate) @@ -1598,7 +1885,7 @@ export const createGitHubSlice: StateCreator = (s const request = (async () => { try { - const runtimeRepo = getRuntimeRepoTarget(get(), repoPath) + const runtimeRepo = getRuntimeRepoTarget(get(), repoPath, requestSettings) const checks = runtimeRepo ? await callRuntimeRpc( runtimeRepo.target, @@ -1635,7 +1922,9 @@ export const createGitHubSlice: StateCreator = (s branch, checks, headSha, - prRepo + prRepo, + requestSettings, + repo?.connectionId ) if (prStatusUpdate?.prCache) { nextState.prCache = prStatusUpdate.prCache @@ -1662,11 +1951,17 @@ export const createGitHubSlice: StateCreator = (s }, fetchPRComments: async (repoPath, prNumber, options): Promise => { - const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id - const cacheKey = repoScopedCacheKey( + const repo = get().repos?.find((candidate) => + options?.repoId ? candidate.id === options.repoId : candidate.path === repoPath + ) + const repoId = options?.repoId ?? repo?.id + const requestSettings = get().settings + const cacheKey = runtimeScopedRepoCacheKey( repoPath, repoId, - prCommentsCacheSuffix(prNumber, options?.prRepo) + prCommentsCacheSuffix(prNumber, options?.prRepo), + requestSettings, + repo?.connectionId ) const cached = get().commentsCache[cacheKey] if (!options?.force && isFresh(cached)) { @@ -1680,13 +1975,26 @@ export const createGitHubSlice: StateCreator = (s const request = (async () => { try { - const comments = (await window.api.gh.prComments({ - repoPath, - repoId, - prNumber, - prRepo: options?.prRepo ?? null, - noCache: options?.force - })) as PRComment[] + const runtimeRepo = getRuntimeRepoTarget(get(), repoPath, requestSettings) + const comments = runtimeRepo + ? await callRuntimeRpc( + runtimeRepo.target, + 'github.prComments', + { + repo: runtimeRepo.repo.id, + prNumber, + prRepo: options?.prRepo ?? null, + noCache: options?.force + }, + { timeoutMs: 30_000 } + ) + : ((await window.api.gh.prComments({ + repoPath, + repoId, + prNumber, + prRepo: options?.prRepo ?? null, + noCache: options?.force + })) as PRComment[]) set((s) => ({ commentsCache: { ...s.commentsCache, @@ -1707,11 +2015,17 @@ export const createGitHubSlice: StateCreator = (s }, resolveReviewThread: async (repoPath, prNumber, threadId, resolve, options) => { - const repoId = options?.repoId ?? get().repos?.find((repo) => repo.path === repoPath)?.id - const cacheKey = repoScopedCacheKey( + const repo = get().repos?.find((candidate) => + options?.repoId ? candidate.id === options.repoId : candidate.path === repoPath + ) + const repoId = options?.repoId ?? repo?.id + const requestSettings = get().settings + const cacheKey = runtimeScopedRepoCacheKey( repoPath, repoId, - prCommentsCacheSuffix(prNumber, options?.prRepo) + prCommentsCacheSuffix(prNumber, options?.prRepo), + requestSettings, + repo?.connectionId ) // Optimistic update: toggle isResolved on all comments in this thread immediately @@ -1729,7 +2043,15 @@ export const createGitHubSlice: StateCreator = (s })) } - const ok = await window.api.gh.resolveReviewThread({ repoPath, repoId, threadId, resolve }) + const runtimeRepo = getRuntimeRepoTarget(get(), repoPath, requestSettings) + const ok = runtimeRepo + ? await callRuntimeRpc( + runtimeRepo.target, + 'github.resolveReviewThread', + { repo: runtimeRepo.repo.id, threadId, resolve }, + { timeoutMs: 30_000 } + ) + : await window.api.gh.resolveReviewThread({ repoPath, repoId, threadId, resolve }) if (!ok && prev) { // Revert optimistic update on failure set((s) => ({ @@ -1749,6 +2071,14 @@ export const createGitHubSlice: StateCreator = (s if (!candidate) { return } + if (getRuntimeRepoTarget(state, candidate.repoPath)) { + void get().fetchPRForBranch(candidate.repoPath, candidate.branch, { + force: bypassesGitHubPRRefreshFreshness(reason), + repoId: candidate.repoId, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + return + } const enqueue = window.api.gh.enqueuePRRefresh if (enqueue) { void enqueue({ candidate, reason, priority }) @@ -1776,6 +2106,15 @@ export const createGitHubSlice: StateCreator = (s return worktree ? buildPRRefreshCandidate(state, worktree) : null }) .filter((candidate): candidate is GitHubPRRefreshCandidate => candidate !== null) + if (getActiveRuntimeTarget(state.settings).kind === 'environment') { + for (const candidate of candidates) { + void get().fetchPRForBranch(candidate.repoPath, candidate.branch, { + repoId: candidate.repoId, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + } + return + } const reportVisible = window.api.gh.reportVisiblePRRefreshCandidates if (reportVisible) { void reportVisible({ candidates, generation }).catch((err) => { @@ -1790,9 +2129,15 @@ export const createGitHubSlice: StateCreator = (s applyGitHubPRRefreshEvent: (event) => { set((s) => { + // Why: local main-process refresh events are keyed only by repo/branch; + // applying them while a runtime is active can leak local PR state into SSH. + if (getActiveRuntimeTarget(s.settings).kind === 'environment') { + return {} + } const nextSequences = { ...s.prRefreshSequences } const nextStates = { ...s.prRefreshStates } let nextPRCache = s.prCache + let nextHostedReviewCache = s.hostedReviewCache ?? {} let changed = false for (const alias of event.aliases) { @@ -1806,6 +2151,9 @@ export const createGitHubSlice: StateCreator = (s changed = true if (event.outcome) { + const startedEntryKey = prRefreshStartedEntryKey(event.sequence, alias.cacheKey) + const requestStartedEntry = prRefreshStartedHostedReviewEntries.get(startedEntryKey) + prRefreshStartedHostedReviewEntries.delete(startedEntryKey) delete nextStates[alias.cacheKey] if (event.outcome.kind === 'upstream-error') { nextStates[alias.cacheKey] = { @@ -1823,17 +2171,21 @@ export const createGitHubSlice: StateCreator = (s const checksCacheKeys = [ ...(alias.repoId ? [ - repoScopedCacheKey( + runtimeScopedRepoCacheKey( alias.repoPath, alias.repoId, - prChecksCacheSuffix(pr.number, pr.prRepo) + prChecksCacheSuffix(pr.number, pr.prRepo), + s.settings, + alias.connectionId ) ] : []), - repoScopedCacheKey( + runtimeScopedRepoCacheKey( alias.repoPath, undefined, - prChecksCacheSuffix(pr.number, pr.prRepo) + prChecksCacheSuffix(pr.number, pr.prRepo), + s.settings, + alias.connectionId ), `${alias.repoPath}::pr-checks::${pr.number}` ] @@ -1852,14 +2204,39 @@ export const createGitHubSlice: StateCreator = (s return pr })() : null - nextPRCache = { - ...nextPRCache, - [alias.cacheKey]: { data, fetchedAt: event.outcome.fetchedAt } - } + const nextCaches = applyGitHubPRResultToCaches({ + prCache: nextPRCache, + hostedReviewCache: nextHostedReviewCache, + prCacheKey: alias.cacheKey, + repoPath: alias.repoPath, + branch: alias.branch, + settings: s.settings, + repoId: alias.repoId, + connectionId: alias.connectionId, + pr: data, + fetchedAt: event.outcome.fetchedAt, + requestStartedAt: event.requestStartedAt, + requestStartedEntry + }) + nextPRCache = nextCaches.prCache + nextHostedReviewCache = nextCaches.hostedReviewCache continue } if (event.status) { + if (event.status === 'in-flight' && event.requestStartedAt !== undefined) { + const hostedReviewCacheKey = getHostedReviewCacheKey( + alias.repoPath, + alias.branch, + s.settings, + alias.repoId, + alias.connectionId + ) + prRefreshStartedHostedReviewEntries.set( + prRefreshStartedEntryKey(event.sequence, alias.cacheKey), + s.hostedReviewCache[hostedReviewCacheKey] + ) + } nextStates[alias.cacheKey] = { status: event.status, reason: event.reason, @@ -1873,7 +2250,8 @@ export const createGitHubSlice: StateCreator = (s ? { prRefreshSequences: nextSequences, prRefreshStates: nextStates, - prCache: nextPRCache + prCache: nextPRCache, + hostedReviewCache: nextHostedReviewCache } : {} }) @@ -1924,7 +2302,7 @@ export const createGitHubSlice: StateCreator = (s const branch = wt.branch.replace(/^refs\/heads\//, '') if (shouldRefreshPRs && !wt.isBare && branch) { - const prKey = repoScopedCacheKey(repo.path, repo.id, branch) + const prKey = prCacheKey(repo.path, repo.id, branch, state.settings, repo.connectionId) const prEntry = state.prCache[prKey] if (!prEntry || now - prEntry.fetchedAt >= CACHE_TTL) { const candidate = buildPRRefreshCandidate(state, wt) @@ -1951,7 +2329,14 @@ export const createGitHubSlice: StateCreator = (s .sort((a, b) => b.score - a.score) .slice(0, isPRStatusGrouping ? stalePRCandidates.length : 5) for (const { candidate } of candidatesToRefresh) { - void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'swr', priority: 10 }) + if (getRuntimeRepoTarget(state, candidate.repoPath)) { + void get().fetchPRForBranch(candidate.repoPath, candidate.branch, { + repoId: candidate.repoId, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + } else { + void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'swr', priority: 10 }) + } } }, @@ -1975,7 +2360,7 @@ export const createGitHubSlice: StateCreator = (s // Invalidate this worktree's cache entries const branch = worktree.branch.replace(/^refs\/heads\//, '') - const prKey = repoScopedCacheKey(repo.path, repo.id, branch) + const prKey = prCacheKey(repo.path, repo.id, branch, state.settings, repo.connectionId) const issueKey = worktree.linkedIssue ? repoScopedCacheKey(repo.path, repo.id, String(worktree.linkedIssue)) : '' @@ -1998,7 +2383,15 @@ export const createGitHubSlice: StateCreator = (s if (!worktree.isBare && branch) { const candidate = buildPRRefreshCandidate(get(), worktree) if (candidate) { - void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'post-push', priority: 100 }) + if (getRuntimeRepoTarget(get(), candidate.repoPath)) { + void get().fetchPRForBranch(candidate.repoPath, candidate.branch, { + force: true, + repoId: candidate.repoId, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + } else { + void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'post-push', priority: 100 }) + } } } if (worktree.linkedIssue) { @@ -2170,7 +2563,15 @@ export const createGitHubSlice: StateCreator = (s if (shouldRefreshPR && !worktree.isBare && branch) { const candidate = buildPRRefreshCandidate(state, worktree) if (candidate) { - void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'active', priority: 80 }) + if (getRuntimeRepoTarget(state, candidate.repoPath)) { + void get().fetchPRForBranch(candidate.repoPath, candidate.branch, { + force: true, + repoId: candidate.repoId, + linkedPRNumber: candidate.linkedPRNumber ?? null + }) + } else { + void window.api.gh.enqueuePRRefresh?.({ candidate, reason: 'active', priority: 80 }) + } } } diff --git a/src/renderer/src/store/slices/hosted-review-cache-identity.ts b/src/renderer/src/store/slices/hosted-review-cache-identity.ts new file mode 100644 index 000000000..0ffad6175 --- /dev/null +++ b/src/renderer/src/store/slices/hosted-review-cache-identity.ts @@ -0,0 +1,42 @@ +import type { GlobalSettings } from '../../../../shared/types' + +export type LinkedReviewHints = { + linkedGitHubPR?: number | null + linkedGitLabMR?: number | null + linkedBitbucketPR?: number | null + linkedAzureDevOpsPR?: number | null + linkedGiteaPR?: number | null +} + +export function getHostedReviewCacheKey( + repoPath: string, + branch: string, + settings?: Pick | null, + repoId?: string | null, + connectionId?: string | null +): string { + const environmentId = settings?.activeRuntimeEnvironmentId?.trim() + const sshConnectionId = connectionId?.trim() + const scope = environmentId + ? `runtime:${environmentId}` + : sshConnectionId + ? `ssh:${sshConnectionId}` + : 'local' + return `${scope}::${repoId ?? repoPath}::${branch}` +} + +// Why: a branch-keyed lookup can describe a different PR than the persisted +// linked review number. Track that distinction without changing the cache key. +export function linkedReviewHintKey(options?: LinkedReviewHints): string { + const hints = [ + ['github', options?.linkedGitHubPR ?? null], + ['gitlab', options?.linkedGitLabMR ?? null], + ['bitbucket', options?.linkedBitbucketPR ?? null], + ['azure-devops', options?.linkedAzureDevOpsPR ?? null], + ['gitea', options?.linkedGiteaPR ?? null] + ] as const + return hints + .filter(([, number]) => number !== null) + .map(([provider, number]) => `${provider}:${number}`) + .join('|') +} diff --git a/src/renderer/src/store/slices/hosted-review-cache-race.test.ts b/src/renderer/src/store/slices/hosted-review-cache-race.test.ts new file mode 100644 index 000000000..d91951aec --- /dev/null +++ b/src/renderer/src/store/slices/hosted-review-cache-race.test.ts @@ -0,0 +1,257 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' +import { create } from 'zustand' +import type { AppState } from '../types' +import { createHostedReviewSlice, getHostedReviewCacheKey } from './hosted-review' +import type { HostedReviewInfo } from '../../../../shared/hosted-review' + +const runtimeRpc = vi.hoisted(() => ({ + callRuntimeRpc: vi.fn() +})) + +vi.mock('@/runtime/runtime-rpc-client', () => ({ + callRuntimeRpc: runtimeRpc.callRuntimeRpc, + getActiveRuntimeTarget: ( + settings: { activeRuntimeEnvironmentId?: string | null } | null | undefined + ) => { + const environmentId = settings?.activeRuntimeEnvironmentId?.trim() + return environmentId ? { kind: 'environment', environmentId } : { kind: 'local' } + } +})) + +const mockApi = { + hostedReview: { + forBranch: vi.fn(), + getCreationEligibility: vi.fn(), + create: vi.fn() + } +} + +globalThis.window = { api: mockApi } as never + +function makeStore(settings: AppState['settings'] = null) { + return create< + Pick< + AppState, + | 'hostedReviewCache' + | 'fetchHostedReviewForBranch' + | 'getHostedReviewCreationEligibility' + | 'createHostedReview' + | 'settings' + | 'repos' + > + >()((...args) => ({ + settings, + repos: [{ id: 'repo-1', path: '/repo', connectionId: null } as AppState['repos'][number]], + ...createHostedReviewSlice(...(args as Parameters)) + })) +} + +const review: HostedReviewInfo = { + provider: 'gitlab', + number: 5, + title: 'Shared MR status', + state: 'open', + url: 'https://gitlab.com/g/p/-/merge_requests/5', + status: 'success', + updatedAt: '2026-05-10T00:00:00.000Z', + mergeable: 'MERGEABLE' +} + +function makeGitHubReview(title: string): HostedReviewInfo { + return { + ...review, + provider: 'github', + number: 42, + title, + url: 'https://github.com/acme/orca/pull/42' + } +} + +describe('hosted review cache race protection', () => { + beforeEach(() => { + mockApi.hostedReview.forBranch.mockReset() + mockApi.hostedReview.getCreationEligibility.mockReset() + mockApi.hostedReview.create.mockReset() + runtimeRpc.callRuntimeRpc.mockReset() + }) + + afterEach(() => { + vi.useRealTimers() + }) + + it('does not let an older successful fetch overwrite a newer external cache write', async () => { + vi.useFakeTimers() + vi.setSystemTime(100) + const olderReview: HostedReviewInfo = { ...review, title: 'Older hosted review status' } + const newerReview = makeGitHubReview('Newer GitHub refresh status') + let resolveFetch: (value: HostedReviewInfo) => void = () => {} + const fetch = new Promise((resolve) => { + resolveFetch = resolve + }) + mockApi.hostedReview.forBranch.mockReturnValueOnce(fetch) + const store = makeStore() + const cacheKey = getHostedReviewCacheKey('/repo', 'feature/race') + + const request = store.getState().fetchHostedReviewForBranch('/repo', 'feature/race') + vi.setSystemTime(200) + store.setState({ + hostedReviewCache: { + [cacheKey]: { + data: newerReview, + fetchedAt: Date.now(), + linkedReviewHintKey: 'github:42' + } + } + }) + vi.setSystemTime(300) + resolveFetch(olderReview) + + await expect(request).resolves.toEqual(olderReview) + expect(store.getState().hostedReviewCache[cacheKey]).toEqual({ + data: newerReview, + fetchedAt: 200, + linkedReviewHintKey: 'github:42' + }) + }) + + it('does not let an older failed fetch overwrite a newer external cache write', async () => { + vi.useFakeTimers() + vi.setSystemTime(100) + const consoleError = vi.spyOn(console, 'error').mockImplementation(() => {}) + const newerReview = makeGitHubReview('Newer GitHub refresh status') + let rejectFetch: (error: Error) => void = () => {} + const fetch = new Promise((_resolve, reject) => { + rejectFetch = reject + }) + mockApi.hostedReview.forBranch.mockReturnValueOnce(fetch) + const store = makeStore() + const cacheKey = getHostedReviewCacheKey('/repo', 'feature/error-race') + + try { + const request = store.getState().fetchHostedReviewForBranch('/repo', 'feature/error-race') + vi.setSystemTime(200) + store.setState({ + hostedReviewCache: { + [cacheKey]: { + data: newerReview, + fetchedAt: Date.now(), + linkedReviewHintKey: 'github:42' + } + } + }) + vi.setSystemTime(300) + rejectFetch(new Error('older lookup failed')) + + await expect(request).resolves.toBeNull() + expect(store.getState().hostedReviewCache[cacheKey]).toEqual({ + data: newerReview, + fetchedAt: 200, + linkedReviewHintKey: 'github:42' + }) + } finally { + consoleError.mockRestore() + } + }) + + it('does not let a same-millisecond external cache write after request start be overwritten', async () => { + vi.useFakeTimers() + vi.setSystemTime(100) + const olderReview: HostedReviewInfo = { + ...review, + title: 'Older same-ms hosted review status' + } + const newerReview = makeGitHubReview('Newer same-ms GitHub refresh status') + let resolveFetch: (value: HostedReviewInfo) => void = () => {} + const fetch = new Promise((resolve) => { + resolveFetch = resolve + }) + mockApi.hostedReview.forBranch.mockReturnValueOnce(fetch) + const store = makeStore() + const cacheKey = getHostedReviewCacheKey('/repo', 'feature/same-ms-race') + + const request = store.getState().fetchHostedReviewForBranch('/repo', 'feature/same-ms-race') + store.setState({ + hostedReviewCache: { + [cacheKey]: { + data: newerReview, + fetchedAt: Date.now(), + linkedReviewHintKey: 'github:42' + } + } + }) + resolveFetch(olderReview) + + await expect(request).resolves.toEqual(olderReview) + expect(store.getState().hostedReviewCache[cacheKey]).toEqual({ + data: newerReview, + fetchedAt: 100, + linkedReviewHintKey: 'github:42' + }) + }) + + it('does not block a pre-existing same-millisecond cache entry from being refreshed', async () => { + vi.useFakeTimers() + vi.setSystemTime(100) + const staleReview: HostedReviewInfo = { + ...review, + title: 'Pre-existing same-ms hosted review status' + } + const freshReview: HostedReviewInfo = { + ...review, + title: 'Fresh same-ms hosted review status' + } + mockApi.hostedReview.forBranch.mockResolvedValueOnce(freshReview) + const store = makeStore() + const cacheKey = getHostedReviewCacheKey('/repo', 'feature/same-ms-existing') + + store.setState({ + hostedReviewCache: { + [cacheKey]: { + data: staleReview, + fetchedAt: Date.now() + } + } + }) + + await expect( + store + .getState() + .fetchHostedReviewForBranch('/repo', 'feature/same-ms-existing', { force: true }) + ).resolves.toEqual(freshReview) + expect(store.getState().hostedReviewCache[cacheKey]).toEqual({ + data: freshReview, + fetchedAt: 100, + linkedReviewHintKey: '' + }) + }) + + it('does not reuse a provider-scoped inflight request for neutral discovery', async () => { + const githubReview = makeGitHubReview('Linked GitHub PR status') + let resolveGitHubLookup: (value: HostedReviewInfo | null) => void = () => {} + const githubLookup = new Promise((resolve) => { + resolveGitHubLookup = resolve + }) + mockApi.hostedReview.forBranch.mockReturnValueOnce(githubLookup).mockResolvedValueOnce(review) + const store = makeStore() + + const linkedRequest = store.getState().fetchHostedReviewForBranch('/repo', 'feature/inflight', { + linkedGitHubPR: 42 + }) + const neutralRequest = store.getState().fetchHostedReviewForBranch('/repo', 'feature/inflight') + + expect(mockApi.hostedReview.forBranch).toHaveBeenCalledTimes(2) + expect(mockApi.hostedReview.forBranch).toHaveBeenNthCalledWith(2, { + branch: 'feature/inflight', + linkedAzureDevOpsPR: null, + linkedBitbucketPR: null, + linkedGitHubPR: null, + linkedGitLabMR: null, + linkedGiteaPR: null, + repoPath: '/repo' + }) + resolveGitHubLookup(githubReview) + + await expect(linkedRequest).resolves.toEqual(githubReview) + await expect(neutralRequest).resolves.toEqual(review) + }) +}) diff --git a/src/renderer/src/store/slices/hosted-review.test.ts b/src/renderer/src/store/slices/hosted-review.test.ts index 0086cddaa..25268aefb 100644 --- a/src/renderer/src/store/slices/hosted-review.test.ts +++ b/src/renderer/src/store/slices/hosted-review.test.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines -- Why: hosted-review tests cover runtime routing, +hinted cache revalidation, provider discovery, and PR cache reconciliation. */ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' import { create } from 'zustand' import type { AppState } from '../types' @@ -38,10 +40,12 @@ function makeStore(settings: AppState['settings'] = null) { | 'createHostedReview' | 'settings' | 'repos' + | 'prCache' > >()((...args) => ({ settings, repos: [{ id: 'repo-1', path: '/repo', connectionId: null } as AppState['repos'][number]], + prCache: {}, ...createHostedReviewSlice(...(args as Parameters)) })) } @@ -94,6 +98,67 @@ describe('hosted review slice', () => { }) }) + it('clears stale GitHub PR cache when branch review lookup finds a non-GitHub review', async () => { + mockApi.hostedReview.forBranch.mockResolvedValueOnce(review) + const store = makeStore() + store.setState({ + prCache: { + 'repo-1::feature/gitlab': { + data: { + number: 12, + title: 'Old GitHub PR', + state: 'open', + url: 'https://github.com/acme/orca/pull/12', + checksStatus: 'pending', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN', + headSha: 'head-oid' + }, + fetchedAt: 1 + }, + '/repo::feature/gitlab': { + data: { + number: 99, + title: 'Old path-scoped GitHub PR', + state: 'closed', + url: 'https://github.com/acme/orca/pull/99', + checksStatus: 'failure', + updatedAt: '2026-03-28T00:00:00Z', + mergeable: 'UNKNOWN', + headSha: 'old-head-oid' + }, + fetchedAt: 1 + } + } + } as unknown as Partial) + + await expect( + store.getState().fetchHostedReviewForBranch('/repo', 'feature/gitlab') + ).resolves.toEqual(review) + + expect(store.getState().prCache['repo-1::feature/gitlab']).toBeUndefined() + expect(store.getState().prCache['/repo::feature/gitlab']).toBeUndefined() + }) + + it('uses SSH-scoped hosted review cache entries for SSH-backed repos', async () => { + mockApi.hostedReview.forBranch.mockResolvedValueOnce(review) + const store = makeStore() + store.setState({ + repos: [{ id: 'repo-1', path: '/repo', connectionId: 'ssh-1' } as AppState['repos'][number]] + } as Partial) + + await expect( + store.getState().fetchHostedReviewForBranch('/repo', 'feature/gitlab', { + repoId: 'repo-1' + }) + ).resolves.toEqual(review) + + expect(store.getState().hostedReviewCache['ssh:ssh-1::repo-1::feature/gitlab']).toMatchObject({ + data: review + }) + expect(store.getState().hostedReviewCache['local::repo-1::feature/gitlab']).toBeUndefined() + }) + it('routes active runtime review lookups through runtime RPC', async () => { runtimeRpc.callRuntimeRpc.mockResolvedValueOnce(review) const store = makeStore({ diff --git a/src/renderer/src/store/slices/hosted-review.ts b/src/renderer/src/store/slices/hosted-review.ts index 1a5dd5f84..ad133db21 100644 --- a/src/renderer/src/store/slices/hosted-review.ts +++ b/src/renderer/src/store/slices/hosted-review.ts @@ -1,3 +1,5 @@ +/* eslint-disable max-lines -- Why: hosted-review cache identity, runtime dispatch, +and race protection are kept together so branch review lookup invariants stay testable. */ import type { StateCreator } from 'zustand' import type { CreateHostedReviewInput, @@ -6,19 +8,19 @@ import type { HostedReviewCreationEligibilityArgs, HostedReviewInfo } from '../../../../shared/hosted-review' -import type { GlobalSettings } from '../../../../shared/types' import { callRuntimeRpc, getActiveRuntimeTarget } from '@/runtime/runtime-rpc-client' import type { AppState } from '../types' +import { + getHostedReviewCacheKey, + linkedReviewHintKey, + type LinkedReviewHints +} from './hosted-review-cache-identity' +import { getGitHubPRCacheKey, getLegacyGitHubPRCacheKey } from './github-cache-key' + +export { getHostedReviewCacheKey, linkedReviewHintKey } from './hosted-review-cache-identity' type CacheEntry = { data: T | null; fetchedAt: number; linkedReviewHintKey?: string } type FetchOptions = { force?: boolean; repoId?: string; staleWhileRevalidate?: boolean } -type LinkedReviewHints = { - linkedGitHubPR?: number | null - linkedGitLabMR?: number | null - linkedBitbucketPR?: number | null - linkedAzureDevOpsPR?: number | null - linkedGiteaPR?: number | null -} const CACHE_TTL_MS = 60_000 @@ -37,22 +39,6 @@ function isFresh(entry: CacheEntry | undefined): entry is CacheEntry { return entry !== undefined && Date.now() - entry.fetchedAt < CACHE_TTL_MS } -// Why: a branch-keyed lookup can describe a different PR than the persisted -// linked review number. Track that distinction without changing the cache key. -function linkedReviewHintKey(options?: LinkedReviewHints): string { - const hints = [ - ['github', options?.linkedGitHubPR ?? null], - ['gitlab', options?.linkedGitLabMR ?? null], - ['bitbucket', options?.linkedBitbucketPR ?? null], - ['azure-devops', options?.linkedAzureDevOpsPR ?? null], - ['gitea', options?.linkedGiteaPR ?? null] - ] as const - return hints - .filter(([, number]) => number !== null) - .map(([provider, number]) => `${provider}:${number}`) - .join('|') -} - function shouldRefetchForLinkedHint( cached: CacheEntry | undefined, hintKey: string @@ -60,19 +46,41 @@ function shouldRefetchForLinkedHint( return cached !== undefined && hintKey !== '' && (cached.linkedReviewHintKey ?? '') !== hintKey } -function canReuseInflightHint(inflightHintKey: string, nextHintKey: string): boolean { - return nextHintKey === '' || inflightHintKey === nextHintKey +function isGitHubLinkedReviewHintKey(hintKey: string | undefined): boolean { + return hintKey?.split('|').some((key) => key.startsWith('github:')) ?? false } -export function getHostedReviewCacheKey( - repoPath: string, - branch: string, - settings?: Pick | null, - repoId?: string | null -): string { - const target = getActiveRuntimeTarget(settings) - const scope = target.kind === 'environment' ? `runtime:${target.environmentId}` : 'local' - return `${scope}::${repoId ?? repoPath}::${branch}` +function shouldRefetchGitHubScopedResultForNoHint( + cached: CacheEntry | undefined, + hintKey: string +): boolean { + // Why: a GitHub-scoped result does not prove the branch's publishing remote + // has no GitLab/other review for neutral lookup. + return ( + cached !== undefined && + hintKey === '' && + isGitHubLinkedReviewHintKey(cached.linkedReviewHintKey) + ) +} + +function canReuseInflightHint(inflightHintKey: string, nextHintKey: string): boolean { + return inflightHintKey === nextHintKey +} + +function hasNewerHostedReviewCacheEntry( + cache: HostedReviewSlice['hostedReviewCache'], + cacheKey: string, + requestStartedAt: number, + requestStartedEntry: CacheEntry | undefined +): boolean { + // Why: GitHub refresh events can update this shared cache while a branch + // lookup is in flight; older lookups must not resurrect stale results. + const entry = cache[cacheKey] + return ( + entry !== undefined && + (entry.fetchedAt > requestStartedAt || + (entry.fetchedAt === requestStartedAt && entry !== requestStartedEntry)) + ) } export type HostedReviewSlice = { @@ -87,13 +95,7 @@ export type HostedReviewSlice = { fetchHostedReviewForBranch: ( repoPath: string, branch: string, - options?: FetchOptions & { - linkedGitHubPR?: number | null - linkedGitLabMR?: number | null - linkedBitbucketPR?: number | null - linkedAzureDevOpsPR?: number | null - linkedGiteaPR?: number | null - } + options?: FetchOptions & LinkedReviewHints ) => Promise } @@ -186,11 +188,22 @@ export const createHostedReviewSlice: StateCreator => { const settings = get().settings const target = getActiveRuntimeTarget(settings) - const cacheKey = getHostedReviewCacheKey(repoPath, branch, settings, options?.repoId) + const repo = get().repos?.find((candidate) => + options?.repoId ? candidate.id === options.repoId : candidate.path === repoPath + ) + const repoId = options?.repoId ?? repo?.id + const cacheKey = getHostedReviewCacheKey( + repoPath, + branch, + settings, + options?.repoId, + repo?.connectionId + ) const cached = get().hostedReviewCache[cacheKey] const hintKey = linkedReviewHintKey(options) const linkedRefetch = shouldRefetchForLinkedHint(cached, hintKey) - if (!options?.force && !linkedRefetch && isFresh(cached)) { + const scopedResultRefetch = shouldRefetchGitHubScopedResultForNoHint(cached, hintKey) + if (!options?.force && !linkedRefetch && !scopedResultRefetch && isFresh(cached)) { return cached.data } @@ -200,6 +213,8 @@ export const createHostedReviewSlice: StateCreator => { const generation = (requestGenerations.get(cacheKey) ?? 0) + 1 + const requestStartedAt = Date.now() + const requestStartedEntry = get().hostedReviewCache[cacheKey] requestGenerations.set(cacheKey, generation) const request = (async () => { try { @@ -226,23 +241,66 @@ export const createHostedReviewSlice: StateCreator ({ - hostedReviewCache: { - ...state.hostedReviewCache, - [cacheKey]: { data: review, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } + set((state) => { + if ( + hasNewerHostedReviewCacheEntry( + state.hostedReviewCache, + cacheKey, + requestStartedAt, + requestStartedEntry + ) + ) { + return {} } - })) + const prCacheKeys = [ + getGitHubPRCacheKey(repoPath, repoId, branch, settings, repo?.connectionId), + getLegacyGitHubPRCacheKey(repoPath, repoId, branch), + getLegacyGitHubPRCacheKey(repoPath, undefined, branch) + ] + const currentPRCache = state.prCache ?? {} + const prCache = + review && + review.provider !== 'github' && + prCacheKeys.some((key) => currentPRCache[key]) + ? (() => { + const next = { ...currentPRCache } + for (const key of prCacheKeys) { + delete next[key] + } + return next + })() + : currentPRCache + return { + ...(prCache === currentPRCache ? {} : { prCache }), + hostedReviewCache: { + ...state.hostedReviewCache, + [cacheKey]: { data: review, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } + } + } + }) } return review } catch (error) { console.error('Failed to fetch hosted review:', error) if (requestGenerations.get(cacheKey) === generation) { - set((state) => ({ - hostedReviewCache: { - ...state.hostedReviewCache, - [cacheKey]: { data: null, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } + set((state) => { + if ( + hasNewerHostedReviewCacheEntry( + state.hostedReviewCache, + cacheKey, + requestStartedAt, + requestStartedEntry + ) + ) { + return {} } - })) + return { + hostedReviewCache: { + ...state.hostedReviewCache, + [cacheKey]: { data: null, fetchedAt: Date.now(), linkedReviewHintKey: hintKey } + } + } + }) } return null } finally { @@ -265,6 +323,7 @@ export const createHostedReviewSlice: StateCreator const nextWorktrees = applyWorktreeUpdates(s.worktreesByRepo, worktreeId, enriched) const cacheKey = reviewRepo && reviewBranch - ? getHostedReviewCacheKey(reviewRepo.path, reviewBranch, s.settings, reviewRepo.id) + ? getHostedReviewCacheKey( + reviewRepo.path, + reviewBranch, + s.settings, + reviewRepo.id, + reviewRepo.connectionId + ) : null const hostedReviewCache = s.hostedReviewCache ?? {} if (nextWorktrees === s.worktreesByRepo && !cacheKey) { diff --git a/src/shared/hosted-review-github.test.ts b/src/shared/hosted-review-github.test.ts index 6ab4dac01..744424025 100644 --- a/src/shared/hosted-review-github.test.ts +++ b/src/shared/hosted-review-github.test.ts @@ -1,5 +1,8 @@ import { describe, expect, it } from 'vitest' -import { hostedReviewSummaryFromGitHubPRInfo } from './hosted-review-github' +import { + hostedReviewInfoFromGitHubPRInfo, + hostedReviewSummaryFromGitHubPRInfo +} from './hosted-review-github' import type { PRInfo } from './types' const pr: PRInfo = { @@ -95,4 +98,18 @@ describe('hostedReviewSummaryFromGitHubPRInfo', () => { }).threadSummary ).toEqual({ unresolvedCount: 0, dataCompleteness: 'partial' }) }) + + it('maps PRInfo into sidebar hosted review metadata', () => { + const review = hostedReviewInfoFromGitHubPRInfo(pr) + + expect(review).toMatchObject({ + provider: 'github', + number: 12, + title: 'Add queue badges', + state: 'open', + status: 'pending', + mergeable: 'MERGEABLE', + headSha: 'abc123' + }) + }) }) diff --git a/src/shared/hosted-review-github.ts b/src/shared/hosted-review-github.ts index ea733a59d..19e7b4be2 100644 --- a/src/shared/hosted-review-github.ts +++ b/src/shared/hosted-review-github.ts @@ -1,5 +1,5 @@ import type { PRCheckDetail, PRComment, PRInfo } from './types' -import type { HostedReviewQueueSummary } from './hosted-review' +import type { HostedReviewInfo, HostedReviewQueueSummary } from './hosted-review' export type HostedReviewFromGitHubPRInfoArgs = { pr: PRInfo @@ -86,3 +86,18 @@ export function hostedReviewSummaryFromGitHubPRInfo( draft: args.pr.state === 'draft' } } + +export function hostedReviewInfoFromGitHubPRInfo(pr: PRInfo): HostedReviewInfo { + return { + provider: 'github', + number: pr.number, + title: pr.title, + state: pr.state, + url: pr.url, + status: pr.checksStatus, + updatedAt: pr.updatedAt, + mergeable: pr.mergeable, + ...(pr.headSha ? { headSha: pr.headSha } : {}), + ...(pr.conflictSummary ? { conflictSummary: pr.conflictSummary } : {}) + } +} diff --git a/src/shared/types.ts b/src/shared/types.ts index dd553236d..b90bbf3bd 100644 --- a/src/shared/types.ts +++ b/src/shared/types.ts @@ -641,6 +641,7 @@ export type GitHubPRRefreshAlias = { repoPath: string branch: string worktreeId?: string + connectionId?: string | null } export type GitHubPRRefreshCandidate = GitHubPRRefreshAlias & { @@ -670,6 +671,7 @@ type GitHubPRRefreshEventBase = { sequence: number reason: GitHubPRRefreshReason aliases: GitHubPRRefreshAlias[] + requestStartedAt?: number } export type GitHubPRRefreshEvent =