Skip to content

fix(pr): show a branch's own open PR, not a newer closed or same-named one #29330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
AmoabaKelvin wants to merge 1 commit into from
Closed

fix(pr): show a branch's own open PR, not a newer closed or same-named one #29330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
148 changes: 104 additions & 44 deletions src/main/azure-devops/client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ describe('Azure DevOps client', () => {
})

afterEach(() => {
vi.unstubAllGlobals()
process.env = OLD_ENV
globalThis.fetch = OLD_FETCH
_resetAzureDevOpsRepoRefCache()
Expand Down Expand Up @@ -200,60 +201,119 @@ describe('Azure DevOps client', () => {
).rejects.toThrow(/Azure DevOps request failed/)
})

it('uses the most recent branch PR instead of preferring stale active PRs', async () => {
it('shows the active branch PR over a newer completed one (#28324)', async () => {
gitExecFileAsyncMock.mockResolvedValue({
stdout: 'https://dev.azure.com/acme/Project/_git/repo\n'
})
globalThis.fetch = vi.fn(async (input: string | URL | Request) => {
const url = new URL(String(input))
const response = (body: unknown): Response =>
new Response(JSON.stringify(body), {
status: 200,
headers: { 'Content-Type': 'application/json' }
})
vi.stubGlobal(
'fetch',
vi.fn(async (input: string | URL | Request) => {
const url = new URL(String(input))
const response = (body: unknown): Response =>
new Response(JSON.stringify(body), {
status: 200,
headers: { 'Content-Type': 'application/json' }
})

if (url.pathname === '/acme/Project/_apis/git/repositories/repo') {
return response({ id: 'repo-guid' })
}
if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') {
expect(url.searchParams.get('searchCriteria.sourceRefName')).toBe(
'refs/heads/feature/azure'
)
expect(url.searchParams.get('searchCriteria.sourceRepositoryId')).toBe('repo-guid')
expect(url.searchParams.get('searchCriteria.status')).toBe('all')
return response({
value: [
{
pullRequestId: 20,
title: 'Open PR',
status: 'active',
creationDate: '2026-05-10T00:00:00Z',
mergeStatus: 'succeeded',
lastMergeSourceCommit: { commitId: 'oldsha' }
},
{
pullRequestId: 21,
title: 'Latest completed PR',
status: 'completed',
creationDate: '2026-05-15T00:00:00Z',
closedDate: '2026-05-16T00:00:00Z',
mergeStatus: 'succeeded',
lastMergeSourceCommit: { commitId: 'newsha' }
}
]
})
}
if (
url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/20/statuses'
) {
return response({ value: [{ state: 'succeeded' }] })
}
return new Response(JSON.stringify({ message: 'not found' }), { status: 404 })
})
)

await expect(
getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/feature/azure')
).resolves.toMatchObject({
number: 20,
title: 'Open PR',
state: 'open',
headSha: 'oldsha'
})
})

it('shows the linked PR without consulting the branch', async () => {
gitExecFileAsyncMock.mockResolvedValue({
stdout: 'https://dev.azure.com/acme/Project/_git/repo\n'
})
const fetchMock = vi.fn(async (input: string | URL | Request) => {
const url = new URL(String(input))
if (url.pathname === '/acme/Project/_apis/git/repositories/repo') {
return response({ id: 'repo-guid' })
}
if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests') {
return response({
value: [
{
pullRequestId: 20,
title: 'Stale active PR',
status: 'active',
creationDate: '2026-05-10T00:00:00Z',
mergeStatus: 'succeeded',
lastMergeSourceCommit: { commitId: 'oldsha' }
},
{
pullRequestId: 21,
title: 'Latest completed PR',
status: 'completed',
creationDate: '2026-05-15T00:00:00Z',
closedDate: '2026-05-16T00:00:00Z',
mergeStatus: 'succeeded',
lastMergeSourceCommit: { commitId: 'newsha' }
}
]
})
return Response.json({ id: 'repo-guid' })
}
if (
url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/21/statuses'
) {
return response({ value: [{ state: 'succeeded' }] })
if (url.pathname === '/acme/Project/_apis/git/repositories/repo-guid/pullRequests/42') {
return Response.json({ pullRequestId: 42, title: 'Linked PR', status: 'active' })
}
return new Response(JSON.stringify({ message: 'not found' }), { status: 404 })
}) as never
return Response.json({ value: [] })
})
vi.stubGlobal('fetch', fetchMock)

await expect(
getAzureDevOpsPullRequestForBranch('/repo', 'refs/heads/feature/azure')
).resolves.toMatchObject({
number: 21,
title: 'Latest completed PR',
state: 'merged',
headSha: 'newsha'
getAzureDevOpsPullRequestForBranch('/repo', 'feature/azure', 42)
).resolves.toMatchObject({ number: 42, title: 'Linked PR' })
expect(
fetchMock.mock.calls.some(([input]) =>
new URL(String(input)).pathname.endsWith('/pullRequests')
)
).toBe(false)
})

it('ignores a same-named branch from a fork', async () => {
gitExecFileAsyncMock.mockResolvedValue({
stdout: 'https://dev.azure.com/acme/Project/_git/repo\n'
})
vi.stubGlobal(
'fetch',
vi.fn(async (input: string | URL | Request) =>
new URL(String(input)).pathname === '/acme/Project/_apis/git/repositories/repo'
? Response.json({ id: 'repo-guid' })
: Response.json({
value: [
{
pullRequestId: 5,
title: 'Fork PR',
status: 'active',
forkSource: { repository: { id: 'fork-guid' } }
}
]
})
)
)

await expect(getAzureDevOpsPullRequestForBranch('/repo', 'feature/azure')).resolves.toBeNull()
})

it('hides a stale completed/abandoned PR whose source branch is the repo default branch (#9171)', async () => {
Expand Down
137 changes: 78 additions & 59 deletions src/main/azure-devops/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,16 @@ import {
type RawAzureDevOpsStatus
} from './pull-request-mappers'
import { shouldHideNonOpenReviewOnDefaultBranch } from '../source-control/repo-default-branch'
import { getAzureDevOpsRepoRef, type AzureDevOpsRepoRef } from './repository-ref'
import {
BRANCH_REVIEW_PAGE_SIZE,
selectReviewForHead
} from '../source-control/branch-review-selection'
import { resolveBranchReviewTarget } from '../source-control/review-head'
import {
getAzureDevOpsRepoRef,
getAzureDevOpsRepoRefForRemote,
type AzureDevOpsRepoRef
} from './repository-ref'
import {
getHostedReviewLocalGitOptions,
type HostedReviewExecutionOptions
Expand Down Expand Up @@ -111,25 +120,6 @@ async function normalizePullRequest(
return mapAzureDevOpsPullRequest(raw, deriveAzureDevOpsStatus(statuses), webBaseUrl)
}

function sortPullRequestsForBranch(
left: RawAzureDevOpsPullRequest,
right: RawAzureDevOpsPullRequest
): number {
const leftStatus = left.status?.trim().toLowerCase()
const rightStatus = right.status?.trim().toLowerCase()
const abandonedOrder = Number(leftStatus === 'abandoned') - Number(rightStatus === 'abandoned')
// Why: abandoned PRs can have newer close dates, but should not hide a usable branch PR.
if (abandonedOrder !== 0) {
return abandonedOrder
}
const leftTime = Date.parse(left.closedDate ?? left.creationDate ?? '') || 0
const rightTime = Date.parse(right.closedDate ?? right.creationDate ?? '') || 0
if (leftTime !== rightTime) {
return rightTime - leftTime
}
return Number(rightStatus === 'active') - Number(leftStatus === 'active')
}

export async function getAzureDevOpsAuthStatus(): Promise<AzureDevOpsAuthStatus> {
const config = getAzureDevOpsAuthConfig()
const baseUrl = config.apiBaseUrl ? normalizeAzureDevOpsApiBaseUrl(config.apiBaseUrl) : null
Expand Down Expand Up @@ -207,60 +197,89 @@ export async function getAzureDevOpsPullRequestForBranch(
return null
}

const repo = await getAzureDevOpsRepoRef(
repoPath,
connectionId,
getHostedReviewLocalGitOptions(options)
)
const localGitOptions = getHostedReviewLocalGitOptions(options)
const repo = await getAzureDevOpsRepoRef(repoPath, connectionId, localGitOptions)
const repository = repo ? await getRepository(repo) : null
if (!repo || !repository) {
return null
}
const { linkedNumber, head } = await resolveBranchReviewTarget(
repoPath,
branchName,
linkedPRNumber,
connectionId,
localGitOptions
)

if (branchName) {
const list = await requestAzureDevOpsJson<{ value?: RawAzureDevOpsPullRequest[] }>(
if (linkedNumber !== null) {
const raw = await requestAzureDevOpsJson<RawAzureDevOpsPullRequest>(
repo,
`/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests`,
{
searchParams: {
'searchCriteria.sourceRefName': `refs/heads/${branchName}`,
'searchCriteria.status': 'all',
$top: 10
}
},
`/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests/${encodePathSegment(
String(linkedNumber)
)}`,
{},
throwOnFailure
)
const raw = (list?.value ?? []).sort(sortPullRequestsForBranch)[0]
if (raw) {
// Why (#9171): discard a non-open implicit branch match on the repo
// default branch and fall through to the linked-number fallback below.
const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({
state: mapAzureDevOpsPullRequestState(raw),
reviewNumber: raw.pullRequestId ?? null,
linkedReviewNumber: linkedPRNumber,
branchName,
repoPath,
connectionId,
localGitOptions: getHostedReviewLocalGitOptions(options)
})
if (!hideOnDefaultBranch) {
return normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw)
}
return normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw)
}
}

if (typeof linkedPRNumber !== 'number') {
if (!head) {
return null
}
const raw = await requestAzureDevOpsJson<RawAzureDevOpsPullRequest>(
repo,
`/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests/${encodePathSegment(
String(linkedPRNumber)
)}`,
{},
throwOnFailure
const headRepositoryId =
head.remote === 'origin'
? null
: await getAzureDevOpsRepoRefForRemote(repoPath, head.remote, connectionId, localGitOptions)
.then((headRepo) => (headRepo ? getRepository(headRepo) : null))
.then((headRepository) => headRepository?.idOrName ?? null)
if (head.remote !== 'origin' && !headRepositoryId) {
return null
}
const isFromHead = (pr: RawAzureDevOpsPullRequest): boolean =>
headRepositoryId === null ? !pr.forkSource : pr.forkSource?.repository?.id === headRepositoryId
const raw = await selectReviewForHead(
async (states) => {
const list = await requestAzureDevOpsJson<{ value?: RawAzureDevOpsPullRequest[] }>(
repo,
`/_apis/git/repositories/${encodePathSegment(repository.idOrName)}/pullRequests`,
{
searchParams: {
'searchCriteria.sourceRefName': `refs/heads/${head.branch}`,
'searchCriteria.sourceRepositoryId': headRepositoryId ?? repository.idOrName,
'searchCriteria.status': states === 'open' ? 'active' : 'all',
$top: BRANCH_REVIEW_PAGE_SIZE
}
},
throwOnFailure
)
// Why: Azure doesn't document its order, so sort newest first here.
return (list?.value ?? [])
.filter(isFromHead)
.sort(
(left, right) =>
(Date.parse(right.creationDate ?? '') || 0) - (Date.parse(left.creationDate ?? '') || 0)
)
},
(pr) => pr.status?.trim().toLowerCase() === 'active'
)
return raw ? normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw) : null
if (!raw) {
return null
}
// Why (#9171): discard a non-open implicit branch match on the repo default branch.
const hideOnDefaultBranch = await shouldHideNonOpenReviewOnDefaultBranch({
state: mapAzureDevOpsPullRequestState(raw),
reviewNumber: raw.pullRequestId ?? null,
linkedReviewNumber: linkedPRNumber,
branchName,
repoPath,
connectionId,
localGitOptions
})
return hideOnDefaultBranch
? null
: normalizePullRequest(repo, repository.idOrName, repository.webBaseUrl, raw)
}

/**
Expand Down
1 change: 1 addition & 0 deletions src/main/azure-devops/pull-request-mappers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export type RawAzureDevOpsPullRequest = {
closedDate?: string | null
mergeStatus?: string | null
sourceRefName?: string | null
forkSource?: { repository?: { id?: string | null } | null } | null
lastMergeSourceCommit?: {
commitId?: string | null
} | null
Expand Down
Loading
Loading