diff --git a/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.test.tsx b/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.test.tsx index 2cf19599bc..de88bd8454 100644 --- a/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.test.tsx +++ b/src/renderer/src/components/right-sidebar/FolderWorkspacePrChecksRow.test.tsx @@ -95,9 +95,11 @@ function makeRow( group: 'passing', checkTone: 'success', title: 'Review title', + reviewNumber: 12, reviewLabel: '#12', reviewUrl: 'https://example.test/pr/12', reviewState: 'open', + reviewStatus: 'success', provider: 'github', summary: 'Checks passing', detailNames: [], diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-github-pr-cache.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-github-pr-cache.ts new file mode 100644 index 0000000000..8487c6619e --- /dev/null +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-github-pr-cache.ts @@ -0,0 +1,90 @@ +import type { HostedReviewInfo } from '../../../../shared/hosted-review' +import type { PRInfo, Repo, Worktree } from '../../../../shared/types' +import { + LOCAL_EXECUTION_HOST_ID, + normalizeExecutionHostId +} from '../../../../shared/execution-host' +import { isCachedMergedBranchPRCurrentForWorktree } from '@/components/sidebar/worktree-card-pr-display' +import type { AppState } from '@/store/types' +import { getGitHubPRCacheKey, getLegacyGitHubPRCacheKey } from '@/store/slices/github-cache-key' +import type { ParentPrChecksCacheEntry } from './parent-pr-checks-row-types' + +export function canUseParentPrChecksGitHubPRCacheEntry( + worktree: Worktree, + prEntry: ParentPrChecksCacheEntry | undefined, + hostedReviewEntry: ParentPrChecksCacheEntry | undefined +): prEntry is ParentPrChecksCacheEntry & { + data: NonNullable['data']> +} { + const pr = prEntry?.data + if (!pr) { + return false + } + const prFetchedAt = prEntry.fetchedAt + const hasLinkedGitHubPR = worktree.linkedPR !== null + if (hasLinkedGitHubPR && pr.number !== worktree.linkedPR) { + return false + } + if (!hasLinkedGitHubPR && hasNonGitHubLinkedReview(worktree)) { + return false + } + const mergedPrMatchesCurrentHead = isCachedMergedBranchPRCurrentForWorktree(pr, worktree) + if (pr.state === 'merged' && !mergedPrMatchesCurrentHead) { + return false + } + // Why: a newer hosted-review miss should suppress older branch PR cache unless + // a merged PR is proven to still describe the checked-out worktree head. + if ( + hostedReviewEntry?.data === null && + !mergedPrMatchesCurrentHead && + prFetchedAt <= hostedReviewEntry.fetchedAt + ) { + return false + } + return true +} + +export function getParentPrChecksGitHubPRCacheEntry({ + prCache, + repo, + branch, + settings +}: { + prCache: Record> + repo: Repo + branch: string + settings: AppState['settings'] +}): ParentPrChecksCacheEntry | undefined { + const currentKey = getGitHubPRCacheKey( + repo.path, + repo.id, + branch, + settings, + repo.connectionId, + repo.executionHostId, + true + ) + const executionHostId = normalizeExecutionHostId(repo.executionHostId) + const canUseLegacyPRCache = + !repo.connectionId && (!executionHostId || executionHostId === LOCAL_EXECUTION_HOST_ID) + const legacyRepoKey = canUseLegacyPRCache + ? getLegacyGitHubPRCacheKey(repo.path, repo.id, branch) + : '' + const legacyPathKey = canUseLegacyPRCache + ? getLegacyGitHubPRCacheKey(repo.path, undefined, branch) + : '' + return ( + prCache[currentKey] ?? + (legacyRepoKey ? prCache[legacyRepoKey] : undefined) ?? + (legacyPathKey ? prCache[legacyPathKey] : undefined) + ) +} + +function hasNonGitHubLinkedReview(worktree: Worktree): boolean { + return ( + worktree.linkedGitLabMR != null || + worktree.linkedBitbucketPR != null || + worktree.linkedAzureDevOpsPR != null || + worktree.linkedGiteaPR != null + ) +} diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-hosted-review-cache.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-hosted-review-cache.ts new file mode 100644 index 0000000000..e6db09a273 --- /dev/null +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-hosted-review-cache.ts @@ -0,0 +1,83 @@ +import type { HostedReviewInfo } from '../../../../shared/hosted-review' +import type { PRInfo, Worktree } from '../../../../shared/types' +import { + getWorktreeCardPrDisplay, + isCachedMergedBranchPRCurrentForWorktree +} from '@/components/sidebar/worktree-card-pr-display' +import type { ParentPrChecksCacheEntry } from './parent-pr-checks-row-types' + +export function canUseParentPrChecksHostedReviewCacheEntry( + worktree: Worktree, + review: HostedReviewInfo, + entry: ParentPrChecksCacheEntry +): boolean { + if (review.state === 'merged' && !mergedReviewMatchesHead(review, worktree)) { + return false + } + const linkedReviewNumber = getLinkedReviewNumberForProvider(worktree, review.provider) + if (hasLinkedReview(worktree)) { + return linkedReviewNumber === review.number + } + if ((entry.linkedReviewHintKey ?? '') !== '') { + return false + } + const display = getWorktreeCardPrDisplay( + review, + worktree.linkedPR, + worktree.linkedGitLabMR ?? null, + worktree.linkedBitbucketPR ?? null, + worktree.linkedAzureDevOpsPR ?? null, + worktree.linkedGiteaPR ?? null, + { reviewHintKey: entry.linkedReviewHintKey } + ) + return display?.provider === review.provider && display.number === review.number +} + +function mergedReviewMatchesHead(review: HostedReviewInfo, worktree: Worktree): boolean { + return isCachedMergedBranchPRCurrentForWorktree( + { + number: review.number, + title: review.title, + state: review.state, + url: review.url, + checksStatus: review.status, + updatedAt: review.updatedAt, + mergeable: review.mergeable, + ...(review.headSha ? { headSha: review.headSha } : {}), + ...(review.confirmedContainedHeadOid + ? { confirmedContainedHeadOid: review.confirmedContainedHeadOid } + : {}) + } satisfies PRInfo, + worktree + ) +} + +function getLinkedReviewNumberForProvider( + worktree: Worktree, + provider: HostedReviewInfo['provider'] +): number | null { + switch (provider) { + case 'github': + return worktree.linkedPR + case 'gitlab': + return worktree.linkedGitLabMR ?? null + case 'bitbucket': + return worktree.linkedBitbucketPR ?? null + case 'azure-devops': + return worktree.linkedAzureDevOpsPR ?? null + case 'gitea': + return worktree.linkedGiteaPR ?? null + case 'unsupported': + return null + } +} + +function hasLinkedReview(worktree: Worktree): boolean { + return ( + worktree.linkedPR != null || + worktree.linkedGitLabMR != null || + worktree.linkedBitbucketPR != null || + worktree.linkedAzureDevOpsPR != null || + worktree.linkedGiteaPR != null + ) +} diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts index 28730809af..2b9e5dcbe3 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.test.ts @@ -147,6 +147,7 @@ describe('parent PR checks refresh', () => { linkedBitbucketPR: 10, linkedAzureDevOpsPR: 11, linkedGiteaPR: 12, + currentHeadOid: 'abc', staleWhileRevalidate: true } ]) diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts index e09caf6a44..545f2b6fec 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-refresh.ts @@ -24,6 +24,7 @@ type FetchHostedReview = ( linkedBitbucketPR?: number | null linkedAzureDevOpsPR?: number | null linkedGiteaPR?: number | null + currentHeadOid?: string | null } ) => Promise @@ -134,6 +135,7 @@ async function refreshParentPrChecksCandidate( linkedBitbucketPR: candidate.worktree.linkedBitbucketPR ?? null, linkedAzureDevOpsPR: candidate.worktree.linkedAzureDevOpsPR ?? null, linkedGiteaPR: candidate.worktree.linkedGiteaPR ?? null, + currentHeadOid: candidate.worktree.head ?? null, staleWhileRevalidate: true }) if (!review) { diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts index 8aa9e72150..9e6f3c2f78 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-row-types.ts @@ -7,6 +7,7 @@ export type ParentPrChecksCacheEntry = { data: T | null fetchedAt: number headSha?: string + linkedReviewHintKey?: string } export type ParentPrChecksRefreshOutcome = @@ -52,9 +53,11 @@ export type ParentPrChecksRow = { group: ParentPrChecksGroupKey checkTone: CheckStatus title: string + reviewNumber: number | null reviewLabel: string | null reviewUrl: string | null reviewState: HostedReviewInfo['state'] | null + reviewStatus: HostedReviewInfo['status'] | null provider: HostedReviewInfo['provider'] | null summary: string detailNames: string[] diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts index 5471b89a59..d261df5851 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.test.ts @@ -2,7 +2,11 @@ import { describe, expect, it } from 'vitest' import type { PRCheckDetail, PRInfo, Repo, Worktree } from '../../../../shared/types' import type { HostedReviewInfo } from '../../../../shared/hosted-review' import { getHostedReviewCacheKey } from '@/store/slices/hosted-review' -import { getGitHubRepoCacheKey } from '@/store/slices/github-cache-key' +import { + getGitHubPRCacheKey, + getGitHubRepoCacheKey, + getLegacyGitHubPRCacheKey +} from '@/store/slices/github-cache-key' import { prChecksCacheSuffix } from '@/store/slices/github' import { buildParentPrChecksProjection, @@ -62,6 +66,19 @@ function makeReview(overrides: Partial = {}): HostedReviewInfo } } +function makePRInfo(overrides: Partial = {}): PRInfo { + return { + number: 42, + title: 'Branch PR', + state: 'open', + url: 'https://example.test/pull/42', + checksStatus: 'success', + updatedAt: '2026-01-01T00:00:00.000Z', + mergeable: 'MERGEABLE', + ...overrides + } +} + function makeProjection({ worktree = makeWorktree({ id: 'repo-1::/feature' }), repo = makeRepo(), @@ -72,7 +89,10 @@ function makeProjection({ }: { worktree?: Worktree repo?: Repo - hostedReviewCache?: Record + hostedReviewCache?: Record< + string, + { data: HostedReviewInfo | null; fetchedAt: number; linkedReviewHintKey?: string } + > prCache?: Record checksCache?: Record< string, @@ -102,7 +122,11 @@ describe('buildParentPrChecksProjection', () => { worktree, repo, hostedReviewCache: { - [cacheKey]: { data: makeReview({ status: 'failure' }), fetchedAt: 1 } + [cacheKey]: { + data: makeReview({ status: 'failure' }), + fetchedAt: 1, + linkedReviewHintKey: '' + } } }).rows[0] ).toMatchObject({ @@ -116,7 +140,11 @@ describe('buildParentPrChecksProjection', () => { worktree, repo, hostedReviewCache: { - [cacheKey]: { data: makeReview({ status: 'pending' }), fetchedAt: 1 } + [cacheKey]: { + data: makeReview({ status: 'pending' }), + fetchedAt: 1, + linkedReviewHintKey: '' + } } }).rows[0] ).toMatchObject({ status: 'pending', group: 'pending' }) @@ -126,7 +154,11 @@ describe('buildParentPrChecksProjection', () => { worktree, repo, hostedReviewCache: { - [cacheKey]: { data: makeReview({ state: 'merged' }), fetchedAt: 1 } + [cacheKey]: { + data: makeReview({ state: 'merged', headSha: 'abc' }), + fetchedAt: 1, + linkedReviewHintKey: '' + } } }).rows[0] ).toMatchObject({ status: 'merged', group: 'merged' }) @@ -138,7 +170,8 @@ describe('buildParentPrChecksProjection', () => { hostedReviewCache: { [cacheKey]: { data: makeReview({ mergeable: 'CONFLICTING', status: 'success' }), - fetchedAt: 1 + fetchedAt: 1, + linkedReviewHintKey: '' } } }).rows[0] @@ -168,6 +201,325 @@ describe('buildParentPrChecksProjection', () => { expect(provenNoReview.summary.noPr).toBe(1) }) + it('uses branch-discovered PR cache for unlinked worktrees', () => { + const repo = makeRepo() + const worktree = makeWorktree({ id: 'repo-1::/feature', linkedPR: null }) + const cacheKey = getGitHubPRCacheKey(repo.path, repo.id, 'feature', settings) + + expect( + makeProjection({ + worktree, + repo, + prCache: { + [cacheKey]: { data: makePRInfo({ number: 99 }), fetchedAt: 2 } + } + }).rows[0] + ).toMatchObject({ + status: 'success', + group: 'passing', + reviewNumber: 99, + reviewLabel: '#99' + }) + }) + + it('uses legacy path-scoped PR cache for local persisted entries', () => { + const repo = makeRepo() + const worktree = makeWorktree({ id: 'repo-1::/feature', linkedPR: 99 }) + const cacheKey = getLegacyGitHubPRCacheKey(repo.path, undefined, 'feature') + + expect( + makeProjection({ + worktree, + repo, + prCache: { + [cacheKey]: { data: makePRInfo({ number: 99, checksStatus: 'success' }), fetchedAt: 2 } + } + }).rows[0] + ).toMatchObject({ + status: 'success', + reviewNumber: 99, + reviewLabel: '#99' + }) + }) + + it('uses legacy path-scoped PR cache for explicit local repos', () => { + const repo = makeRepo({ executionHostId: 'local' }) + const worktree = makeWorktree({ id: 'repo-1::/feature', linkedPR: 99 }) + const cacheKey = getLegacyGitHubPRCacheKey(repo.path, undefined, 'feature') + + expect( + makeProjection({ + worktree, + repo, + prCache: { + [cacheKey]: { data: makePRInfo({ number: 99, checksStatus: 'success' }), fetchedAt: 2 } + } + }).rows[0] + ).toMatchObject({ + status: 'success', + reviewNumber: 99, + reviewLabel: '#99' + }) + }) + + it('does not use stale merged branch PR cache after the worktree advances', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedPR: null, + head: 'new-head' + }) + const cacheKey = getGitHubPRCacheKey(repo.path, repo.id, 'feature', settings) + + expect( + makeProjection({ + worktree, + repo, + prCache: { + [cacheKey]: { + data: makePRInfo({ + number: 99, + state: 'merged', + headSha: 'merged-head' + }), + fetchedAt: 2 + } + } + }).rows[0] + ).toMatchObject({ + status: 'notFetched', + reviewLabel: null + }) + }) + + it('does not use stale merged linked PR cache after the worktree advances', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedPR: 99, + head: 'new-head' + }) + const cacheKey = getGitHubPRCacheKey(repo.path, repo.id, 'feature', settings) + + expect( + makeProjection({ + worktree, + repo, + prCache: { + [cacheKey]: { + data: makePRInfo({ + number: 99, + title: 'Stale merged linked PR', + state: 'merged', + headSha: 'merged-head' + }), + fetchedAt: 2 + } + } + }).rows[0] + ).toMatchObject({ + status: 'linkedDetailsUnavailable', + reviewNumber: 99, + reviewLabel: '#99', + title: 'Loading PR...' + }) + }) + + it('does not let older linked PR cache override a newer hosted-review miss', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedPR: 99 + }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + const prKey = getGitHubPRCacheKey(repo.path, repo.id, 'feature', settings) + + expect( + makeProjection({ + worktree, + repo, + hostedReviewCache: { + [hostedKey]: { data: null, fetchedAt: 3 } + }, + prCache: { + [prKey]: { + data: makePRInfo({ + number: 99, + title: 'Older linked PR cache' + }), + fetchedAt: 2 + } + } + }).rows[0] + ).toMatchObject({ + status: 'linkedDetailsUnavailable', + reviewNumber: 99, + reviewLabel: '#99', + title: 'PR details unavailable' + }) + }) + + it('does not let mismatched hosted-review cache override linked PR metadata', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedPR: 99 + }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + + expect( + makeProjection({ + worktree, + repo, + hostedReviewCache: { + [hostedKey]: { + data: makeReview({ + number: 12, + title: 'Stale branch review', + status: 'failure' + }), + fetchedAt: 2, + linkedReviewHintKey: 'github:12' + } + } + }).rows[0] + ).toMatchObject({ + status: 'linkedDetailsUnavailable', + reviewNumber: 99, + reviewLabel: '#99', + title: 'Loading PR...' + }) + }) + + it('does not use stale merged hosted-review cache after the worktree advances', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedPR: null, + head: 'new-head' + }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + + expect( + makeProjection({ + worktree, + repo, + hostedReviewCache: { + [hostedKey]: { + data: makeReview({ + number: 99, + title: 'Stale merged hosted review', + state: 'merged', + headSha: 'merged-head' + }), + fetchedAt: 2, + linkedReviewHintKey: '' + } + } + }).rows[0] + ).toMatchObject({ + status: 'notFetched', + reviewLabel: null + }) + }) + + it('does not use stale merged non-GitHub hosted-review cache after the worktree advances', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedBitbucketPR: null, + head: 'new-head' + }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + + expect( + makeProjection({ + worktree, + repo, + hostedReviewCache: { + [hostedKey]: { + data: makeReview({ + provider: 'bitbucket', + number: 77, + title: 'Stale merged Bitbucket PR', + state: 'merged', + headSha: 'merged-head' + }), + fetchedAt: 2, + linkedReviewHintKey: '' + } + } + }).rows[0] + ).toMatchObject({ + status: 'notFetched', + reviewLabel: null + }) + }) + + it('does not let same-number hosted-review cache from another provider override linked PR metadata', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedPR: 99 + }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + + expect( + makeProjection({ + worktree, + repo, + hostedReviewCache: { + [hostedKey]: { + data: makeReview({ + provider: 'gitlab', + number: 99, + title: 'Wrong provider same number', + status: 'failure' + }), + fetchedAt: 2, + linkedReviewHintKey: '' + } + } + }).rows[0] + ).toMatchObject({ + status: 'linkedDetailsUnavailable', + provider: 'github', + reviewNumber: 99, + reviewLabel: '#99', + title: 'Loading PR...' + }) + }) + + it('does not use linked-hint hosted-review cache after a non-GitHub link is removed', () => { + const repo = makeRepo() + const worktree = makeWorktree({ + id: 'repo-1::/feature', + linkedBitbucketPR: null + }) + const hostedKey = getHostedReviewCacheKey(repo.path, 'feature', settings, repo.id) + + expect( + makeProjection({ + worktree, + repo, + hostedReviewCache: { + [hostedKey]: { + data: makeReview({ + provider: 'bitbucket', + number: 77, + title: 'Removed linked Bitbucket PR' + }), + fetchedAt: 2, + linkedReviewHintKey: 'bitbucket:77' + } + } + }).rows[0] + ).toMatchObject({ + status: 'notFetched', + reviewLabel: null + }) + }) + it('classifies completed unavailable refreshes as unavailable instead of not fetched', () => { const repo = makeRepo() const worktree = makeWorktree({ id: 'repo-1::/feature' }) @@ -218,7 +570,11 @@ describe('buildParentPrChecksProjection', () => { worktree, repo, hostedReviewCache: { - [cacheKey]: { data: makeReview({ status: 'success' }), fetchedAt: 1 } + [cacheKey]: { + data: makeReview({ status: 'success' }), + fetchedAt: 1, + linkedReviewHintKey: '' + } }, refreshOutcomes: new Map([[identity, { kind: 'error' }]]) }) @@ -253,7 +609,7 @@ describe('buildParentPrChecksProjection', () => { const projection = makeProjection({ worktree, repo, - hostedReviewCache: { [hostedKey]: { data: review, fetchedAt: 1 } }, + hostedReviewCache: { [hostedKey]: { data: review, fetchedAt: 1, linkedReviewHintKey: '' } }, checksCache: { [checksKey]: { data: [ @@ -289,7 +645,7 @@ describe('buildParentPrChecksProjection', () => { const projection = makeProjection({ worktree, repo, - hostedReviewCache: { [hostedKey]: { data: review, fetchedAt: 1 } }, + hostedReviewCache: { [hostedKey]: { data: review, fetchedAt: 1, linkedReviewHintKey: '' } }, checksCache: { [checksKey]: { data: [ diff --git a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts index be8f3bdb2c..6330c3bbee 100644 --- a/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts +++ b/src/renderer/src/components/right-sidebar/parent-pr-checks-rows.ts @@ -4,7 +4,7 @@ import { hostedReviewInfoFromGitHubPRInfo } from '../../../../shared/hosted-revi import { isFolderRepo } from '../../../../shared/repo-kind' import { getWorktreeCardPrDisplay } from '@/components/sidebar/worktree-card-pr-display' import { getWorktreeGitIdentityDisplay } from '@/lib/worktree-git-identity-display' -import { getGitHubPRCacheKey, getGitHubRepoCacheKey } from '@/store/slices/github-cache-key' +import { getGitHubRepoCacheKey } from '@/store/slices/github-cache-key' import { getHostedReviewCacheKey, linkedReviewHintKey } from '@/store/slices/hosted-review' import { prChecksCacheSuffix } from '@/store/slices/github' import { @@ -23,6 +23,13 @@ import { getRowSummary, groupForRowStatus } from './parent-pr-checks-row-status' +import { + canUseParentPrChecksGitHubPRCacheEntry, + getParentPrChecksGitHubPRCacheEntry +} from './parent-pr-checks-github-pr-cache' +import { canUseParentPrChecksHostedReviewCacheEntry } from './parent-pr-checks-hosted-review-cache' + +type ParentPrChecksRowSourceArgs = Omit export type { ParentPrChecksGroupKey, @@ -37,13 +44,10 @@ export function buildParentPrChecksProjection( args: BuildParentPrChecksRowsArgs ): ParentPrChecksProjection { const repoById = new Map(args.repos.map((repo) => [repo.id, repo])) - const rows = args.worktrees.map((worktree) => - buildParentPrChecksRow({ - ...args, - worktree, - repo: repoById.get(worktree.repoId) ?? null - }) - ) + const rows = buildParentPrChecksRows({ + ...args, + repoById + }) const groups = PARENT_PR_CHECKS_GROUP_ORDER.map((key) => ({ key, label: PARENT_PR_CHECKS_GROUP_LABELS[key], @@ -52,6 +56,18 @@ export function buildParentPrChecksProjection( return { rows, groups, summary: summarizeParentPrChecksRows(rows) } } +export function buildParentPrChecksRows( + args: ParentPrChecksRowSourceArgs & { repoById: ReadonlyMap } +): ParentPrChecksRow[] { + return args.worktrees.map((worktree) => + buildParentPrChecksRow({ + ...args, + worktree, + repo: args.repoById.get(worktree.repoId) ?? null + }) + ) +} + export function summarizeParentPrChecksRows( rows: readonly ParentPrChecksRow[] ): ParentPrChecksSummary { @@ -90,7 +106,7 @@ export function getParentPrChecksRefreshIdentity( } function buildParentPrChecksRow( - args: BuildParentPrChecksRowsArgs & { worktree: Worktree; repo: Repo | null } + args: ParentPrChecksRowSourceArgs & { worktree: Worktree; repo: Repo | null } ): ParentPrChecksRow { const branch = getBranchName(args.worktree) const refreshIdentity = getParentPrChecksRefreshIdentity(args.worktree, args.repo, branch) @@ -125,9 +141,11 @@ function buildParentPrChecksRow( group: groupForRowStatus(status), checkTone: getRowCheckTone(status, review), title: getRowTitle(args.worktree, branch, review, fallbackDisplay?.title), + reviewNumber: review?.number ?? fallbackDisplay?.number ?? null, reviewLabel: getReviewLabel(review, fallbackDisplay), reviewUrl: review?.url ?? fallbackDisplay?.url ?? null, reviewState: review?.state ?? fallbackDisplay?.state ?? null, + reviewStatus: review?.status ?? fallbackDisplay?.status ?? null, provider: review?.provider ?? fallbackDisplay?.provider ?? null, summary: getRowSummary(status, review, detailNames), detailNames, @@ -138,7 +156,7 @@ function buildParentPrChecksRow( } function getReviewSnapshot( - args: BuildParentPrChecksRowsArgs & { worktree: Worktree; repo: Repo | null }, + args: ParentPrChecksRowSourceArgs & { worktree: Worktree; repo: Repo | null }, branch: string | null, outcome: ParentPrChecksRefreshOutcome | undefined ): { review: HostedReviewInfo | null | undefined; hasCacheEntry: boolean } { @@ -150,18 +168,30 @@ function getReviewSnapshot( } const scopedArgs = { ...args, repo: args.repo } const hostedReviewEntry = args.hostedReviewCache[getHostedReviewKey(scopedArgs, branch)] - if (hostedReviewEntry?.data) { + if ( + hostedReviewEntry?.data && + canUseParentPrChecksHostedReviewCacheEntry( + args.worktree, + hostedReviewEntry.data, + hostedReviewEntry + ) + ) { return { review: hostedReviewEntry.data, hasCacheEntry: true } } - const prEntry = args.prCache[getPRKey(scopedArgs, branch)] - if (prEntry?.data) { + const prEntry = getParentPrChecksGitHubPRCacheEntry({ + prCache: args.prCache, + repo: args.repo, + branch, + settings: args.settings + }) + if (canUseParentPrChecksGitHubPRCacheEntry(args.worktree, prEntry, hostedReviewEntry)) { return { review: hostedReviewInfoFromGitHubPRInfo(prEntry.data), hasCacheEntry: true } } return { - review: hostedReviewEntry?.data, + review: hostedReviewEntry?.data === null ? null : undefined, hasCacheEntry: hostedReviewEntry !== undefined } } @@ -188,7 +218,7 @@ function getReviewLabel( } function getCheckDetails( - args: BuildParentPrChecksRowsArgs & { repo: Repo | null }, + args: ParentPrChecksRowSourceArgs & { repo: Repo | null }, review: HostedReviewInfo | null | undefined, branch: string | null ): PRCheckDetail[] { @@ -218,7 +248,7 @@ function getCheckDetailNames(checks: readonly PRCheckDetail[]): string[] { } function getGitHubChecksEntry( - args: BuildParentPrChecksRowsArgs & { repo: Repo }, + args: ParentPrChecksRowSourceArgs & { repo: Repo }, review: HostedReviewInfo ): ParentPrChecksCacheEntry | undefined { const prRepo = null @@ -244,7 +274,7 @@ function getGitHubChecksEntry( } function getHostedReviewKey( - args: BuildParentPrChecksRowsArgs & { repo: Repo }, + args: ParentPrChecksRowSourceArgs & { repo: Repo }, branch: string ): string { return getHostedReviewCacheKey( @@ -258,18 +288,6 @@ function getHostedReviewKey( ) } -function getPRKey(args: BuildParentPrChecksRowsArgs & { repo: Repo }, branch: string): string { - return getGitHubPRCacheKey( - args.repo.path, - args.repo.id, - branch, - args.settings, - args.repo.connectionId, - args.repo.executionHostId, - true - ) -} - function getBranchName(worktree: Worktree): string | null { const identity = getWorktreeGitIdentityDisplay(worktree) return identity?.kind === 'branch' ? identity.branchName : null diff --git a/src/renderer/src/components/sidebar/WorktreeList.tsx b/src/renderer/src/components/sidebar/WorktreeList.tsx index dbf77ecf0f..adcb4bd43f 100644 --- a/src/renderer/src/components/sidebar/WorktreeList.tsx +++ b/src/renderer/src/components/sidebar/WorktreeList.tsx @@ -675,8 +675,8 @@ type VirtualizedWorktreeViewportProps = { allRepoIds: string[] onReorderHostSections: (orderedHostIds: ExecutionHostId[]) => void onHostDragActiveChange: (active: boolean) => void - prCache: Record | null - hostedReviewCache: Record | null + prCache: AppState['prCache'] | null + hostedReviewCache: AppState['hostedReviewCache'] | null workspaceStatuses: readonly WorkspaceStatusDefinition[] projectGrouping?: ProjectGroupingModel projectGroups?: readonly ProjectGroup[] diff --git a/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.test.ts b/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.test.ts index 6ca326b3c6..0b823f62c8 100644 --- a/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.test.ts +++ b/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from 'vitest' import type { CheckStatus, + PRInfo, Repo, Worktree, WorktreeLineage, @@ -17,6 +18,12 @@ const repo: Repo = { addedAt: 1 } +class LookupOnlyRepoMap extends Map { + override values(): MapIterator { + throw new Error('Folder card PR display should use repo lookup instead of repo enumeration') + } +} + function makeWorktree(overrides: Partial & { id: string }): Worktree { const { id, ...rest } = overrides return { @@ -68,6 +75,24 @@ function makeWorktreeLineage(child: Worktree, parent: Worktree): WorktreeLineage } describe('getFolderWorkspaceCardPrDisplay', () => { + it('uses the existing repo lookup without enumerating all repos', () => { + const worktree = makeWorktree({ id: 'lookup-only', linkedPR: 8 }) + + const display = getFolderWorkspaceCardPrDisplay({ + folderWorkspaceId: 'folder-1', + workspaceLineageByChildKey: { [worktree.id]: makeWorkspaceLineage(worktree) }, + worktreeLineageById: {}, + worktreeMap: new Map([[worktree.id, worktree]]), + repoMap: new LookupOnlyRepoMap([[repo.id, repo]]), + hostedReviewCache: null, + prCache: { + 'repo-1::lookup-only': makePrEntry(8, 'success') + } + }) + + expect(display).toMatchObject({ number: 8, status: 'success' }) + }) + it('uses failing attached PR status ahead of pending and passing PRs', () => { const passing = makeWorktree({ id: 'passing', linkedPR: 1 }) const pending = makeWorktree({ id: 'pending', linkedPR: 2 }) @@ -89,9 +114,9 @@ describe('getFolderWorkspaceCardPrDisplay', () => { repoMap: new Map([[repo.id, repo]]), hostedReviewCache: null, prCache: { - 'repo-1::passing': { data: makePr(1, 'success') }, - 'repo-1::pending': { data: makePr(2, 'pending') }, - 'repo-1::failing': { data: makePr(3, 'failure') } + 'repo-1::passing': makePrEntry(1, 'success'), + 'repo-1::pending': makePrEntry(2, 'pending'), + 'repo-1::failing': makePrEntry(3, 'failure') } }) @@ -116,8 +141,8 @@ describe('getFolderWorkspaceCardPrDisplay', () => { repoMap: new Map([[repo.id, repo]]), hostedReviewCache: null, prCache: { - 'repo-1::passing': { data: makePr(1, 'success') }, - 'repo-1::pending': { data: makePr(2, 'pending') } + 'repo-1::passing': makePrEntry(1, 'success'), + 'repo-1::pending': makePrEntry(2, 'pending') } }) @@ -139,15 +164,75 @@ describe('getFolderWorkspaceCardPrDisplay', () => { repoMap: new Map([[repo.id, repo]]), hostedReviewCache: null, prCache: { - 'repo-1::nested': { data: makePr(4, 'success') } + 'repo-1::nested': makePrEntry(4, 'success') } }) expect(display).toMatchObject({ number: 4, status: 'success' }) }) + + it('uses branch-discovered PR cache for unlinked attached worktrees', () => { + const worktree = makeWorktree({ id: 'branch-discovered', linkedPR: null }) + + const display = getFolderWorkspaceCardPrDisplay({ + folderWorkspaceId: 'folder-1', + workspaceLineageByChildKey: { [worktree.id]: makeWorkspaceLineage(worktree) }, + worktreeLineageById: {}, + worktreeMap: new Map([[worktree.id, worktree]]), + repoMap: new Map([[repo.id, repo]]), + hostedReviewCache: null, + prCache: { + 'repo-1::branch-discovered': { data: makePr(9, 'success'), fetchedAt: 2 } + } + }) + + expect(display).toMatchObject({ number: 9, status: 'success' }) + }) + + it('uses the right-sidebar classified status for conflicting PRs', () => { + const worktree = makeWorktree({ id: 'conflicting-pr', linkedPR: null }) + + const display = getFolderWorkspaceCardPrDisplay({ + folderWorkspaceId: 'folder-1', + workspaceLineageByChildKey: { [worktree.id]: makeWorkspaceLineage(worktree) }, + worktreeLineageById: {}, + worktreeMap: new Map([[worktree.id, worktree]]), + repoMap: new Map([[repo.id, repo]]), + hostedReviewCache: null, + prCache: { + 'repo-1::conflicting-pr': { + data: { ...makePr(11, 'success'), mergeable: 'CONFLICTING' }, + fetchedAt: 2 + } + } + }) + + expect(display).toMatchObject({ number: 11, status: 'failure' }) + }) + + it('does not use stale merged branch PR cache after the worktree advances', () => { + const worktree = makeWorktree({ id: 'advanced-after-merge', linkedPR: null, head: 'new-head' }) + + const display = getFolderWorkspaceCardPrDisplay({ + folderWorkspaceId: 'folder-1', + workspaceLineageByChildKey: { [worktree.id]: makeWorkspaceLineage(worktree) }, + worktreeLineageById: {}, + worktreeMap: new Map([[worktree.id, worktree]]), + repoMap: new Map([[repo.id, repo]]), + hostedReviewCache: null, + prCache: { + 'repo-1::advanced-after-merge': { + data: { ...makePr(10, 'success'), state: 'merged', headSha: 'merged-head' }, + fetchedAt: 2 + } + } + }) + + expect(display).toBeNull() + }) }) -function makePr(number: number, checksStatus: CheckStatus) { +function makePr(number: number, checksStatus: CheckStatus): PRInfo { return { number, title: `PR ${number}`, @@ -158,3 +243,10 @@ function makePr(number: number, checksStatus: CheckStatus) { mergeable: 'UNKNOWN' } } + +function makePrEntry( + number: number, + checksStatus: CheckStatus +): { data: PRInfo; fetchedAt: number } { + return { data: makePr(number, checksStatus), fetchedAt: 2 } +} diff --git a/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.ts b/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.ts index fb43b506dc..bbd6711768 100644 --- a/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.ts +++ b/src/renderer/src/components/sidebar/folder-workspace-card-pr-display.ts @@ -1,26 +1,11 @@ import type { AppState } from '@/store/types' -import { getHostedReviewCacheKey } from '@/store/slices/hosted-review' -import { getGitHubPRCacheKey, getLegacyGitHubPRCacheKey } from '@/store/slices/github-cache-key' -import { branchName } from '@/lib/git-utils' -import type { HostedReviewInfo } from '../../../../shared/hosted-review' -import type { - PRInfo, - Repo, - Worktree, - WorktreeLineage, - WorkspaceLineage -} from '../../../../shared/types' +import type { Repo, Worktree, WorktreeLineage, WorkspaceLineage } from '../../../../shared/types' import { folderWorkspaceKey, parseWorkspaceKey } from '../../../../shared/workspace-scope' -import { getWorktreeCardPrDisplay, type WorktreeCardPrDisplay } from './worktree-card-pr-display' - -type HostedReviewCacheEntry = { - data?: HostedReviewInfo | null - linkedReviewHintKey?: string -} - -type PrCacheEntry = { - data?: PRInfo | null -} +import { + buildParentPrChecksRows, + type ParentPrChecksRow +} from '@/components/right-sidebar/parent-pr-checks-rows' +import type { WorktreeCardPrDisplay } from './worktree-card-pr-display' type FolderWorkspaceCardPrDisplayArgs = { folderWorkspaceId: string @@ -28,8 +13,8 @@ type FolderWorkspaceCardPrDisplayArgs = { worktreeLineageById: Record | null | undefined worktreeMap: ReadonlyMap repoMap: ReadonlyMap - hostedReviewCache: Record | null - prCache: Record | null + hostedReviewCache: AppState['hostedReviewCache'] | null + prCache: AppState['prCache'] | null settings?: AppState['settings'] } @@ -50,21 +35,23 @@ export function getFolderWorkspaceCardPrDisplay({ prCache, settings }: FolderWorkspaceCardPrDisplayArgs): WorktreeCardPrDisplay | null { - const reviews = getAttachedWorktreesForFolderWorkspaceCard({ + const attachedWorktrees = getAttachedWorktreesForFolderWorkspaceCard({ folderWorkspaceId, workspaceLineageByChildKey, worktreeLineageById, worktreeMap }) - .map((worktree) => - getAttachedWorktreePrDisplay({ - worktree, - repo: repoMap.get(worktree.repoId), - hostedReviewCache, - prCache, - settings - }) - ) + + const reviews = buildParentPrChecksRows({ + worktrees: attachedWorktrees, + repoById: repoMap, + settings: settings ?? null, + hostedReviewCache: hostedReviewCache ?? {}, + prCache: prCache ?? {}, + // Folder cards only need the compact status icon; avoid check-detail cache fanout. + checksCache: {} + }) + .map(parentPrChecksRowToCardDisplay) .filter((review): review is WorktreeCardPrDisplay => review !== null) if (reviews.length === 0) { @@ -111,6 +98,20 @@ function getAttachedWorktreesForFolderWorkspaceCard({ return [...included.values()] } +function parentPrChecksRowToCardDisplay(row: ParentPrChecksRow): WorktreeCardPrDisplay | null { + if (!row.provider || row.provider === 'unsupported' || row.reviewNumber === null) { + return null + } + return { + provider: row.provider, + number: row.reviewNumber, + title: row.title, + ...(row.reviewState ? { state: row.reviewState } : {}), + ...(row.reviewUrl ? { url: row.reviewUrl } : {}), + status: row.checkTone + } +} + function getWorkspaceLineageChild( lineage: WorkspaceLineage, worktreeMap: ReadonlyMap @@ -144,126 +145,6 @@ function isCurrentLineagePair( ) } -function getAttachedWorktreePrDisplay({ - worktree, - repo, - hostedReviewCache, - prCache, - settings -}: { - worktree: Worktree - repo: Repo | undefined - hostedReviewCache: Record | null - prCache: Record | null - settings?: AppState['settings'] -}): WorktreeCardPrDisplay | null { - if (!repo) { - return null - } - - const branch = branchName(worktree.branch).trim() - if (!branch) { - return getLinkedReviewDisplay(worktree) - } - - const hostedReviewCacheKey = getHostedReviewCacheKey( - repo.path, - branch, - settings, - repo.id, - repo.connectionId, - repo.executionHostId, - true - ) - const hostedReviewEntry = hostedReviewCache?.[hostedReviewCacheKey] as - | HostedReviewCacheEntry - | undefined - const hostedReviewDisplay = hostedReviewEntry - ? getWorktreeCardPrDisplay( - hostedReviewEntry.data, - worktree.linkedPR, - worktree.linkedGitLabMR ?? null, - worktree.linkedBitbucketPR ?? null, - worktree.linkedAzureDevOpsPR ?? null, - worktree.linkedGiteaPR ?? null, - { reviewHintKey: hostedReviewEntry.linkedReviewHintKey } - ) - : null - if (hostedReviewDisplay) { - return hostedReviewDisplay - } - - const cachedGitHubPr = getCachedGitHubPr({ worktree, repo, branch, prCache, settings }) - if (cachedGitHubPr) { - return { - provider: 'github', - number: cachedGitHubPr.number, - title: cachedGitHubPr.title, - state: cachedGitHubPr.state, - url: cachedGitHubPr.url, - status: cachedGitHubPr.checksStatus - } - } - - return getLinkedReviewDisplay(worktree) -} - -function getLinkedReviewDisplay(worktree: Worktree): WorktreeCardPrDisplay | null { - return getWorktreeCardPrDisplay( - undefined, - worktree.linkedPR, - worktree.linkedGitLabMR ?? null, - worktree.linkedBitbucketPR ?? null, - worktree.linkedAzureDevOpsPR ?? null, - worktree.linkedGiteaPR ?? null - ) -} - -function getCachedGitHubPr({ - worktree, - repo, - branch, - prCache, - settings -}: { - worktree: Worktree - repo: Repo - branch: string - prCache: Record | null - settings?: AppState['settings'] -}): PRInfo | null { - if (!prCache || worktree.linkedPR === null) { - return null - } - - const cacheKey = getGitHubPRCacheKey( - repo.path, - repo.id, - branch, - settings, - repo.connectionId, - repo.executionHostId, - true - ) - const canUseLegacyPRCache = !repo.connectionId && !repo.executionHostId - const legacyRepoScopedCacheKey = canUseLegacyPRCache - ? getLegacyGitHubPRCacheKey(repo.path, repo.id, branch) - : '' - const legacyPathScopedCacheKey = canUseLegacyPRCache - ? getLegacyGitHubPRCacheKey(repo.path, undefined, branch) - : '' - const entry = - (prCache[cacheKey] as PrCacheEntry | undefined) ?? - (legacyRepoScopedCacheKey - ? (prCache[legacyRepoScopedCacheKey] as PrCacheEntry | undefined) - : undefined) ?? - (legacyPathScopedCacheKey - ? (prCache[legacyPathScopedCacheKey] as PrCacheEntry | undefined) - : undefined) - const pr = entry?.data ?? null - return pr?.number === worktree.linkedPR ? pr : null -} - function compareReviewDisplays(left: WorktreeCardPrDisplay, right: WorktreeCardPrDisplay): number { return getReviewDisplayPriority(left) - getReviewDisplayPriority(right) }