diff --git a/src/main/github.test.ts b/src/main/github.test.ts index a059f2c7..fb5d01ed 100644 --- a/src/main/github.test.ts +++ b/src/main/github.test.ts @@ -279,7 +279,7 @@ describe('fetchPRStatusesForRepo matcher', () => { vi.clearAllMocks() }) - it('emits both prBr and prSha aliases when worktree HEAD is a valid SHA', async () => { + it('emits both prBr and prSearch aliases when worktree HEAD is a valid SHA', async () => { const sha = 'b'.repeat(40) fetchSpy.mockResolvedValueOnce( mockResponse(200, { @@ -287,9 +287,9 @@ describe('fetchPRStatusesForRepo matcher', () => { repository: { defaultBranchRef: { name: 'main' }, milestones: { totalCount: 0 }, - prBr0: { nodes: [] }, - prSha0: { associatedPullRequests: { nodes: [] } } - } + prBr0: { nodes: [] } + }, + prSearch0: { nodes: [] } } }) ) @@ -299,11 +299,14 @@ describe('fetchPRStatusesForRepo matcher', () => { ) const body = JSON.parse((fetchSpy.mock.calls[0][1] as RequestInit).body as string) expect(body.query).toContain('prBr0: pullRequests(headRefName: $branch0') - expect(body.query).toContain('prSha0: object(oid: $sha0)') - expect(body.variables).toMatchObject({ branch0: 'feature', sha0: sha }) + expect(body.query).toContain('prSearch0: search(query: $q0, type: ISSUE') + expect(body.variables).toMatchObject({ + branch0: 'feature', + q0: `type:pr repo:o/r ${sha}` + }) }) - it('omits the prSha alias when worktree HEAD is not a 40-char SHA', async () => { + it('omits the prSearch alias when worktree HEAD is not a 40-char SHA', async () => { fetchSpy.mockResolvedValueOnce( mockResponse(200, { data: { @@ -321,11 +324,11 @@ describe('fetchPRStatusesForRepo matcher', () => { ) const body = JSON.parse((fetchSpy.mock.calls[0][1] as RequestInit).body as string) expect(body.query).toContain('prBr0:') - expect(body.query).not.toContain('prSha0:') - expect(body.variables).not.toHaveProperty('sha0') + expect(body.query).not.toContain('prSearch0:') + expect(body.variables).not.toHaveProperty('q0') }) - it('finds the PR via the SHA fallback when the local branch name does not match the PR head ref', async () => { + it('finds the PR via the SHA search fallback when the local branch name does not match the PR head ref', async () => { // Mirrors `gh pr checkout 37320` against gradle/gradle: local branch // is `pr-37320-strictly-doc-update` but the PR's head.ref on the // upstream is `lkasso/documentation/strictly--doc-update`. @@ -336,12 +339,10 @@ describe('fetchPRStatusesForRepo matcher', () => { repository: { defaultBranchRef: { name: 'main' }, milestones: { totalCount: 0 }, - prBr0: { nodes: [] }, // branch-name lookup misses - prSha0: { - associatedPullRequests: { - nodes: [gqlPR({ number: 37320, title: 'docs: strictly', headRefOid: sha })] - } - } + prBr0: { nodes: [] } // branch-name lookup misses + }, + prSearch0: { + nodes: [gqlPR({ number: 37320, title: 'docs: strictly', headRefOid: sha })] } } }) @@ -354,9 +355,67 @@ describe('fetchPRStatusesForRepo matcher', () => { expect(result.get('/wt')?.title).toBe('docs: strictly') }) - it('prefers a SHA-matched PR when branch and SHA lookups return different PRs', async () => { - // Two branches named the same on different forks both produce PRs; - // SHA disambiguates to the one whose head commit matches the worktree. + it('finds a cross-fork PR via the SHA search fallback', async () => { + // Mirrors gradle PR #32046 from MattAlp/gradle: associatedPullRequests + // against the upstream is empty for cross-fork PRs, but search by SHA + // returns the PR. + const sha = 'c'.repeat(40) + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { + repository: { + defaultBranchRef: { name: 'main' }, + milestones: { totalCount: 0 }, + prBr0: { nodes: [] } + }, + prSearch0: { + nodes: [ + gqlPR({ + number: 32046, + title: 'Graduate aarch64', + headRefOid: sha, + headRepository: { nameWithOwner: 'MattAlp/gradle' } + }) + ] + } + } + }) + ) + const result = await fetchPRStatusesForRepo( + { origin: { owner: 'gradle', repo: 'gradle' }, upstream: { owner: 'gradle', repo: 'gradle' } }, + [{ worktreePath: '/wt', branch: 'sg/pr/32046', headSha: sha }] + ) + expect(result.get('/wt')?.number).toBe(32046) + }) + + it('ignores non-PR search results (issues without a number field)', async () => { + const sha = 'd'.repeat(40) + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { + repository: { + defaultBranchRef: { name: 'main' }, + milestones: { totalCount: 0 }, + prBr0: { nodes: [] } + }, + prSearch0: { + // Empty inline fragment for non-PR Issue + a real PR + nodes: [ + {}, + gqlPR({ number: 99, headRefOid: sha }) + ] + } + } + }) + ) + const result = await fetchPRStatusesForRepo( + { origin: { owner: 'o', repo: 'r' }, upstream: { owner: 'o', repo: 'r' } }, + [{ worktreePath: '/wt', branch: 'feature', headSha: sha }] + ) + expect(result.get('/wt')?.number).toBe(99) + }) + + it('prefers a SHA-matched PR when branch and search return different PRs', async () => { const sha = 'd'.repeat(40) fetchSpy.mockResolvedValueOnce( mockResponse(200, { @@ -372,11 +431,9 @@ describe('fetchPRStatusesForRepo matcher', () => { headRepository: { nameWithOwner: 'someoneelse/r' } }) ] - }, - prSha0: { - associatedPullRequests: { nodes: [gqlPR({ number: 200, headRefOid: sha })] } } - } + }, + prSearch0: { nodes: [gqlPR({ number: 200, headRefOid: sha })] } } }) ) @@ -387,6 +444,104 @@ describe('fetchPRStatusesForRepo matcher', () => { expect(result.get('/wt')?.number).toBe(200) }) + it('does not claim a PR for a worktree on the repo default branch (main)', async () => { + // A worktree sitting on main shouldn't be reported as "this PR" just + // because the search index for main's HEAD SHA happens to surface the + // most-recently-squashed PR. Same for master. + const mainSha = 'a'.repeat(40) + const mergedPRHeadSha = 'b'.repeat(40) + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { + repository: { + defaultBranchRef: { name: 'main' }, + milestones: { totalCount: 0 }, + prBr0: { nodes: [] } + }, + // GitHub's search index returns the just-merged PR when you + // search the squash commit's SHA — its headRefOid is the + // original PR's head, not main's HEAD. + prSearch0: { + nodes: [ + gqlPR({ + number: 999, + title: 'Some merged feature', + state: 'MERGED', + headRefOid: mergedPRHeadSha + }) + ] + } + } + }) + ) + const result = await fetchPRStatusesForRepo( + { origin: { owner: 'o', repo: 'r' }, upstream: { owner: 'o', repo: 'r' } }, + [{ worktreePath: '/wt', branch: 'main', headSha: mainSha }] + ) + expect(result.get('/wt')).toBeNull() + }) + + it('does not claim a PR for a worktree on a non-default merge-point branch (develop)', async () => { + // 'develop' isn't the default branch but PR #50 targets it as base. + // A worktree sitting on develop shouldn't be credited with a PR even + // if search-by-SHA surfaces a recently squashed candidate. + const developSha = 'a'.repeat(40) + const featureSha = 'b'.repeat(40) + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { + repository: { + defaultBranchRef: { name: 'main' }, + milestones: { totalCount: 0 }, + // Worktree on develop: search returns the most-recently-squashed + // PR which still claims our origin via sameRepo fallback if we + // weren't already excluding it. + prBr0: { nodes: [] }, + // Worktree on feature: legitimately ours, baseRefName=develop. + prBr1: { nodes: [gqlPR({ number: 50, baseRefName: 'develop', headRefOid: featureSha })] } + }, + prSearch0: { + nodes: [gqlPR({ number: 99, state: 'MERGED', headRefOid: 'c'.repeat(40) })] + }, + prSearch1: { nodes: [] } + } + }) + ) + const result = await fetchPRStatusesForRepo( + { origin: { owner: 'o', repo: 'r' }, upstream: { owner: 'o', repo: 'r' } }, + [ + { worktreePath: '/wt-dev', branch: 'develop', headSha: developSha }, + { worktreePath: '/wt-feat', branch: 'feature', headSha: featureSha } + ] + ) + expect(result.get('/wt-dev')).toBeNull() + expect(result.get('/wt-feat')?.number).toBe(50) + }) + + it('does not claim a PR for a worktree on master when that is the default branch', async () => { + const masterSha = 'c'.repeat(40) + const mergedPRHeadSha = 'd'.repeat(40) + fetchSpy.mockResolvedValueOnce( + mockResponse(200, { + data: { + repository: { + defaultBranchRef: { name: 'master' }, + milestones: { totalCount: 0 }, + prBr0: { nodes: [] } + }, + prSearch0: { + nodes: [gqlPR({ number: 999, state: 'MERGED', headRefOid: mergedPRHeadSha })] + } + } + }) + ) + const result = await fetchPRStatusesForRepo( + { origin: { owner: 'o', repo: 'r' }, upstream: { owner: 'o', repo: 'r' } }, + [{ worktreePath: '/wt', branch: 'master', headSha: masterSha }] + ) + expect(result.get('/wt')).toBeNull() + }) + it('dedupes when both lookups return the same PR', async () => { const sha = 'f'.repeat(40) fetchSpy.mockResolvedValueOnce( @@ -395,11 +550,9 @@ describe('fetchPRStatusesForRepo matcher', () => { repository: { defaultBranchRef: { name: 'main' }, milestones: { totalCount: 0 }, - prBr0: { nodes: [gqlPR({ number: 42, headRefOid: sha })] }, - prSha0: { - associatedPullRequests: { nodes: [gqlPR({ number: 42, headRefOid: sha })] } - } - } + prBr0: { nodes: [gqlPR({ number: 42, headRefOid: sha })] } + }, + prSearch0: { nodes: [gqlPR({ number: 42, headRefOid: sha })] } } }) ) diff --git a/src/main/github.ts b/src/main/github.ts index 8244a446..3de4d6bc 100644 --- a/src/main/github.ts +++ b/src/main/github.ts @@ -271,19 +271,16 @@ type GraphQLCheckContext = } interface GraphQLBatchResponse { - data?: { - repository?: - | ({ - defaultBranchRef: { name: string } | null - milestones: { totalCount: number } | null - } & Record< - string, - | { nodes: GraphQLPR[] | null } - | { associatedPullRequests: { nodes: GraphQLPR[] | null } | null } + data?: + | ({ + repository?: + | ({ + defaultBranchRef: { name: string } | null + milestones: { totalCount: number } | null + } & Record) | null - >) - | null - } | null + } & Record | null } | null>) + | null errors?: Array<{ message: string }> | null } @@ -390,23 +387,25 @@ export async function fetchPRStatusesForRepo( const originFull = `${ctx.origin.owner}/${ctx.origin.repo}` const varDefs = ['$owner:String!', '$name:String!'] - const aliasParts: string[] = [] + const repoAliasParts: string[] = [] + const topAliasParts: string[] = [] const variables: Record = { owner, name: repo } - // Branch-name lookup handles the common case. SHA lookup via - // object(oid).associatedPullRequests handles `gh pr checkout`-style - // synthetic local branches whose name doesn't match the PR's head.ref. - // Both fire in the same request so the fallback adds no extra round-trip. + // Branch-name lookup handles the common case. SHA-via-search handles + // both cross-fork PRs (whose commits aren't linked from the upstream's + // associatedPullRequests index) and `gh pr checkout`-style synthetic + // local branches whose name doesn't match the PR's head.ref. Both fire + // in the same request so the fallback adds no extra round-trip. queryable.forEach((req, i) => { varDefs.push(`$branch${i}:String!`) variables[`branch${i}`] = req.branch - aliasParts.push( + repoAliasParts.push( `prBr${i}: pullRequests(headRefName: $branch${i}, first: 5, orderBy: {field: UPDATED_AT, direction: DESC}) { nodes { ...PR } }` ) if (/^[0-9a-f]{40}$/i.test(req.headSha)) { - varDefs.push(`$sha${i}:GitObjectID!`) - variables[`sha${i}`] = req.headSha - aliasParts.push( - `prSha${i}: object(oid: $sha${i}) { ... on Commit { associatedPullRequests(first: 5, orderBy: {field: UPDATED_AT, direction: DESC}) { nodes { ...PR } } } }` + varDefs.push(`$q${i}:String!`) + variables[`q${i}`] = `type:pr repo:${owner}/${repo} ${req.headSha}` + topAliasParts.push( + `prSearch${i}: search(query: $q${i}, type: ISSUE, first: 5) { nodes { ... on PullRequest { ...PR } } }` ) } }) @@ -414,8 +413,9 @@ export async function fetchPRStatusesForRepo( repository(owner: $owner, name: $name) { defaultBranchRef { name } milestones(first: 1) { totalCount } - ${aliasParts.join('\n ')} + ${repoAliasParts.join('\n ')} } + ${topAliasParts.join('\n ')} } ${PR_FRAGMENT}` @@ -440,22 +440,33 @@ ${PR_FRAGMENT}` if (!repoData) { throw new Error(`GitHub GraphQL: empty repository response for ${owner}/${repo}`) } + const topData = json.data ?? {} const hasMilestones = (repoData.milestones?.totalCount ?? 0) > 0 + const defaultBranchName = repoData.defaultBranchRef?.name ?? '' + // Resolve per-request, then fetch behind_by + first-release-tag in parallel. const built = await Promise.all( queryable.map(async (req, i) => { + // A worktree sitting on the repo's default branch (main/master) is + // not the head of any PR — skip the resolution entirely to avoid + // misattributing the latest squash-merged PR's status to it. + if (defaultBranchName && req.branch === defaultBranchName) { + return { worktreePath: req.worktreePath, branch: req.branch, status: null as PRStatus | null } + } const brAlias = repoData[`prBr${i}`] as { nodes: GraphQLPR[] | null } | null | undefined - const shaAlias = repoData[`prSha${i}`] as - | { associatedPullRequests?: { nodes: GraphQLPR[] | null } | null } + const searchAlias = topData[`prSearch${i}`] as + | { nodes: Array | null } | null | undefined const branchNodes = brAlias?.nodes ?? [] - const shaNodes = shaAlias?.associatedPullRequests?.nodes ?? [] - const nodes = dedupePRsByNumber([...branchNodes, ...shaNodes]) - const pr = pickPRBySha(nodes, req.headSha, originFull) - if (!pr) return { worktreePath: req.worktreePath, status: null as PRStatus | null } + // search returns Issue | PullRequest; filter to PR-shaped nodes only. + const searchNodes = (searchAlias?.nodes ?? []).filter( + (n): n is GraphQLPR => !!n && typeof (n as GraphQLPR).number === 'number' + ) + const pr = resolvePRForWorktree(branchNodes, searchNodes, req.headSha, originFull) + if (!pr) return { worktreePath: req.worktreePath, branch: req.branch, status: null as PRStatus | null } const [behindBy, firstReleaseTag] = await Promise.all([ pr.state === 'MERGED' || pr.state === 'CLOSED' ? Promise.resolve(null) @@ -465,40 +476,54 @@ ${PR_FRAGMENT}` : Promise.resolve(null) ]) const status = buildPRStatus(pr, req.branch, behindBy, firstReleaseTag, hasMilestones) - return { worktreePath: req.worktreePath, status } + return { worktreePath: req.worktreePath, branch: req.branch, status } }) ) + + // Any branch that some PR is targeting as base (develop / integration / + // release/*, etc.) is a merge point, not a PR head. Null out attributions + // for worktrees sitting on one of those. + const baseBranches = new Set() + for (const b of built) if (b.status) baseBranches.add(b.status.baseBranch) + for (const b of built) { + if (b.status && baseBranches.has(b.branch)) b.status = null + } + for (const b of built) result.set(b.worktreePath, b.status) return result } -/** Combine PR nodes from the branch-name and SHA-based lookups, keeping - * the first occurrence per PR number. */ -function dedupePRsByNumber(nodes: GraphQLPR[]): GraphQLPR[] { - const seen = new Set() - const out: GraphQLPR[] = [] - for (const n of nodes) { - if (seen.has(n.number)) continue - seen.add(n.number) - out.push(n) - } - return out -} - -/** Among the up-to-5 PR nodes returned for a single headRefName lookup, - * pick the one most likely to belong to this worktree: - * 1. exact SHA match against the worktree's HEAD, - * 2. same-origin head (PR opened from this repo, not a fork), - * 3. fall back to the most-recently-updated (first node). */ -function pickPRBySha(nodes: GraphQLPR[], headSha: string, originFull: string): GraphQLPR | null { - if (nodes.length === 0) return null +/** Resolve which PR (if any) belongs to a given worktree. + * + * Branch-name nodes come from `pullRequests(headRefName: $branch)` — the + * filter is by ref, so any same-origin hit is a legitimate match for + * this branch. We prefer an exact SHA match if available, then + * same-origin, then the most-recently-updated. + * + * Search nodes come from `search("type:pr repo:o/n ")` — the index + * matches the SHA appearing anywhere in the PR's commit history or + * body, including squash-merge commits that land on the default branch. + * That's too loose to trust on its own, so we require the candidate's + * `headRefOid` to equal the worktree's HEAD SHA before accepting it. */ +function resolvePRForWorktree( + branchNodes: GraphQLPR[], + searchNodes: GraphQLPR[], + headSha: string, + originFull: string +): GraphQLPR | null { if (headSha) { - const bySha = nodes.find((n) => n.headRefOid === headSha) + const bySha = + branchNodes.find((n) => n.headRefOid === headSha) ?? + searchNodes.find((n) => n.headRefOid === headSha) if (bySha) return bySha } - const sameRepo = nodes.find((n) => n.headRepository?.nameWithOwner === originFull) + // No SHA match. Same-origin branch-name hit is still a legitimate + // match (worktree slightly behind the PR head, or PR force-pushed). + // Cross-fork branch-name hits are ignored — `feature/foo` on someone + // else's fork is not our PR. + const sameRepo = branchNodes.find((n) => n.headRepository?.nameWithOwner === originFull) if (sameRepo) return sameRepo - return nodes[0] + return null } function buildPRStatus( diff --git a/src/renderer/worktree-sort.test.ts b/src/renderer/worktree-sort.test.ts index e9c1ad23..a430c906 100644 --- a/src/renderer/worktree-sort.test.ts +++ b/src/renderer/worktree-sort.test.ts @@ -155,4 +155,26 @@ describe('worktree-sort snoozed group', () => { it('getGroupKey returns snoozed when isSnoozed regardless of merged', () => { expect(getGroupKey(wt('/a'), mergedPR, true, true)).toBe('snoozed') }) + + it('no-pr group lists merge-point worktrees (PR bases) above feature worktrees', () => { + const main = stubWorktree({ path: '/main', branch: 'main', isMain: true, createdAt: 100 }) + const develop = stubWorktree({ path: '/develop', branch: 'develop', createdAt: 50 }) + const feature = stubWorktree({ path: '/feat', branch: 'feature/x', createdAt: 200 }) + const featureWithPR = stubWorktree({ path: '/other', branch: 'feature/y', createdAt: 300 }) + // Active PR targets develop — so develop counts as a base branch. + const groups = groupWorktrees( + [feature, develop, main, featureWithPR], + { + '/main': null, + '/develop': null, + '/feat': null, + '/other': stubPRStatus({ branch: 'feature/y', baseBranch: 'develop' }) + }, + {}, + {} + ) + const noPR = groups.find((g) => g.key === 'no-pr')! + // main pinned to top, then develop (a base branch), then features by createdAt desc. + expect(noPR.worktrees.map((w) => w.path)).toEqual(['/main', '/develop', '/feat']) + }) }) diff --git a/src/renderer/worktree-sort.ts b/src/renderer/worktree-sort.ts index 114ed64b..ffc248ae 100644 --- a/src/renderer/worktree-sort.ts +++ b/src/renderer/worktree-sort.ts @@ -49,6 +49,27 @@ function sortByCreatedAt(worktrees: Worktree[]): Worktree[] { }) } +/** Sort the no-PR / Active group: main first, then any worktree whose + * branch is being targeted by an open PR (merge points like + * develop/integration/release), then everything else by createdAt desc. */ +function sortNoPRGroup(worktrees: Worktree[], baseBranches: Set): Worktree[] { + return [...worktrees].sort((a, b) => { + if (a.isMain !== b.isMain) return a.isMain ? -1 : 1 + const aBase = baseBranches.has(a.branch) ? 1 : 0 + const bBase = baseBranches.has(b.branch) ? 1 : 0 + if (aBase !== bBase) return bBase - aBase + return (b.createdAt || 0) - (a.createdAt || 0) + }) +} + +function collectBaseBranches(prStatuses: Record): Set { + const out = new Set() + for (const status of Object.values(prStatuses)) { + if (status?.baseBranch) out.add(status.baseBranch) + } + return out +} + /** Group worktrees by PR status, sorted by creation time within each group */ export function groupWorktrees( worktrees: Worktree[], @@ -77,12 +98,16 @@ export function groupWorktrees( grouped[key].push(wt) } + const baseBranches = collectBaseBranches(prStatuses) return GROUP_ORDER .filter((key) => grouped[key].length > 0) .map((key) => ({ key, label: GROUP_LABELS[key], - worktrees: sortByCreatedAt(grouped[key]) + worktrees: + key === 'no-pr' + ? sortNoPRGroup(grouped[key], baseBranches) + : sortByCreatedAt(grouped[key]) })) }