fix(sync): paginate compare and resolve real head SHA

This commit is contained in:
Gabriel Brown
2026-07-11 13:44:10 -04:00
parent c8200a6747
commit 4684617fed
3 changed files with 128 additions and 12 deletions
+34 -9
View File
@@ -117,6 +117,9 @@ const normalizeCompareCommit = (
htmlUrl: commit.html_url, htmlUrl: commit.html_url,
}); });
const COMPARE_PER_PAGE = 100;
const COMPARE_MAX_PAGES = 30; // bound: up to 3000 commits collected
export const compareAcrossForkNetwork = async ( export const compareAcrossForkNetwork = async (
octokit: Octokit, octokit: Octokit,
args: { args: {
@@ -125,24 +128,46 @@ export const compareAcrossForkNetwork = async (
baseOwner: string; baseOwner: string;
baseBranch: string; baseBranch: string;
headOwner: string; headOwner: string;
headRepo: string;
headBranch: string; headBranch: string;
}, },
): Promise<GitHubCompareSummary> => { ): Promise<GitHubCompareSummary> => {
const basehead = `${args.baseOwner}:${args.baseBranch}...${args.headOwner}:${args.headBranch}`; const basehead = `${args.baseOwner}:${args.baseBranch}...${args.headOwner}:${args.headBranch}`;
const result = await octokit.rest.repos.compareCommitsWithBasehead({ const first = await octokit.rest.repos.compareCommitsWithBasehead({
owner: args.owner, owner: args.owner,
repo: args.repo, repo: args.repo,
basehead, basehead,
per_page: 100, per_page: COMPARE_PER_PAGE,
page: 1,
}); });
const commits = result.data.commits.map(normalizeCompareCommit); const rawCommits = [...first.data.commits];
const totalCommits = first.data.total_commits;
let page = 2;
while (rawCommits.length < totalCommits && page <= COMPARE_MAX_PAGES) {
const next = await octokit.rest.repos.compareCommitsWithBasehead({
owner: args.owner,
repo: args.repo,
basehead,
per_page: COMPARE_PER_PAGE,
page,
});
if (next.data.commits.length === 0) break;
rawCommits.push(...next.data.commits);
page += 1;
}
const head = await resolveBranchHead(
octokit,
args.headOwner,
args.headRepo,
args.headBranch,
);
return { return {
aheadBy: result.data.ahead_by, aheadBy: first.data.ahead_by,
mergeBaseSha: result.data.merge_base_commit.sha, mergeBaseSha: first.data.merge_base_commit.sha,
headSha: commits[commits.length - 1]?.sha, headSha: head.sha,
baseSha: result.data.base_commit.sha, baseSha: first.data.base_commit.sha,
htmlUrl: result.data.html_url, htmlUrl: first.data.html_url,
commits, commits: rawCommits.map(normalizeCompareCommit),
}; };
}; };
+9 -2
View File
@@ -32,8 +32,13 @@ const toStatus = (upstreamAheadBy: number, forkAheadBy: number) => {
return 'unknown' as const; return 'unknown' as const;
}; };
const getLastCommitAt = (compare: GitHubCompareSummary) => const getLastCommitAt = (compare: GitHubCompareSummary) => {
compare.commits[compare.commits.length - 1]?.committedAt; const head = compare.commits.find((c) => c.sha === compare.headSha);
return (
head?.committedAt ??
compare.commits[compare.commits.length - 1]?.committedAt
);
};
const ensureForkMetadata = (spoon: Doc<'spoons'>) => { const ensureForkMetadata = (spoon: Doc<'spoons'>) => {
if (!spoon.forkOwner || !spoon.forkRepo) { if (!spoon.forkOwner || !spoon.forkRepo) {
@@ -121,6 +126,7 @@ const refreshOwnedSpoon = async (
baseOwner: forkOwner, baseOwner: forkOwner,
baseBranch: resolvedForkBranch, baseBranch: resolvedForkBranch,
headOwner: spoon.upstreamOwner, headOwner: spoon.upstreamOwner,
headRepo: spoon.upstreamRepo,
headBranch: upstreamBranch, headBranch: upstreamBranch,
}), }),
compareAcrossForkNetwork(octokit, { compareAcrossForkNetwork(octokit, {
@@ -129,6 +135,7 @@ const refreshOwnedSpoon = async (
baseOwner: spoon.upstreamOwner, baseOwner: spoon.upstreamOwner,
baseBranch: upstreamBranch, baseBranch: upstreamBranch,
headOwner: forkOwner, headOwner: forkOwner,
headRepo: forkRepo,
headBranch: resolvedForkBranch, headBranch: resolvedForkBranch,
}), }),
listPullRequests(octokit, { owner: forkOwner, repo: forkRepo }), listPullRequests(octokit, { owner: forkOwner, repo: forkRepo }),
@@ -1,7 +1,10 @@
import type { Octokit } from '@octokit/rest'; import type { Octokit } from '@octokit/rest';
import { describe, expect, it, vi } from 'vitest'; import { describe, expect, it, vi } from 'vitest';
import { resolveBranchHead } from '../../convex/githubClient'; import {
compareAcrossForkNetwork,
resolveBranchHead,
} from '../../convex/githubClient';
describe('resolveBranchHead', () => { describe('resolveBranchHead', () => {
it('returns the branch tip sha from the branch ref', async () => { it('returns the branch tip sha from the branch ref', async () => {
@@ -22,3 +25,84 @@ describe('resolveBranchHead', () => {
}); });
}); });
}); });
const makeCommit = (sha: string) => ({
sha,
commit: {
message: `commit ${sha}`,
author: { name: 'Dev', email: 'dev@example.com', date: '2026-01-01T00:00:00Z' },
committer: { name: 'Dev', email: 'dev@example.com', date: '2026-01-01T00:00:00Z' },
},
author: { login: 'dev' },
html_url: `https://github.com/o/r/commit/${sha}`,
});
describe('compareAcrossForkNetwork', () => {
it('paginates the compare and takes headSha from the branch tip, not the last commit', async () => {
const page1 = Array.from({ length: 100 }, (_, i) => makeCommit(`c${i}`));
// Last commit in the collected list is a DIFFERENT sha than the branch tip.
const page2 = Array.from({ length: 50 }, (_, i) =>
makeCommit(i === 49 ? 'LAST_COMMIT_NOT_TIP' : `c${100 + i}`),
);
const compareCommitsWithBasehead = vi.fn(
async ({ page }: { page?: number }) => {
const commits = page === 2 ? page2 : page1;
return {
data: {
total_commits: 150,
ahead_by: 150,
merge_base_commit: { sha: 'MERGE_BASE' },
base_commit: { sha: 'BASE' },
html_url: 'https://github.com/o/r/compare/base...head',
commits,
},
};
},
);
const getBranch = vi.fn(async () => ({
data: { commit: { sha: 'TIP' } },
}));
const octokit = {
rest: { repos: { compareCommitsWithBasehead, getBranch } },
} as unknown as Octokit;
const result = await compareAcrossForkNetwork(octokit, {
owner: 'up',
repo: 'upstream-repo',
baseOwner: 'fork',
baseBranch: 'main',
headOwner: 'up',
headRepo: 'upstream-repo',
headBranch: 'main',
});
// Core bug: head is the branch tip, NOT the last commit in the compare list.
expect(result.headSha).toBe('TIP');
expect(result.commits[result.commits.length - 1]?.sha).toBe(
'LAST_COMMIT_NOT_TIP',
);
expect(result.headSha).not.toBe('LAST_COMMIT_NOT_TIP');
// ahead_by / merge base / base come from the first compare page.
expect(result.aheadBy).toBe(150);
expect(result.mergeBaseSha).toBe('MERGE_BASE');
expect(result.baseSha).toBe('BASE');
// All commits collected across both pages.
expect(result.commits).toHaveLength(150);
// Pagination is bounded and the tip is resolved via a single branch ref call.
expect(getBranch).toHaveBeenCalledTimes(1);
expect(getBranch).toHaveBeenCalledWith({
owner: 'up',
repo: 'upstream-repo',
branch: 'main',
});
expect(compareCommitsWithBasehead).toHaveBeenCalledTimes(2);
expect(compareCommitsWithBasehead.mock.calls[0]?.[0]).toMatchObject({
per_page: 100,
page: 1,
});
});
});