fix(github): keep merged PRs as branch history instead of hiding them

Branch status resolves an open PR across the whole fork network first, so a
merged fork PR can never hide an open upstream PR for the same head. Only when
no target has an open PR does the branch's newest closed/merged PR come back,
as history.

The panel shows that history as a compact note and offers creating the next PR
below it, instead of either sticking on a terminal PR or going blank after a
merge. Terminal associations stay persisted for reload continuity but are never
treated as authority: they revalidate on the discovery cadence and on focus.

History is looked up only for the branch's own remote and name, and remembered
per repo+branch, so the extra lookup cannot exhaust the route's resolve budget.
The checks summary and merge-permission lookup are skipped for a closed or
merged PR, where neither is actionable.
This commit is contained in:
Bohdan Triapitsyn
2026-08-15 17:56:55 +03:00
parent 52ac367b1e
commit 268f9ea9f2
21 changed files with 405 additions and 190 deletions
@@ -77,7 +77,12 @@
- It skips PR lookup when the current branch matches that repo's default branch.
- It first searches for **open** PRs by likely source owner plus exact head branch.
- If that fails, it falls back to broader GitHub search for open PRs on the branch name.
- Closed/merged PRs are intentionally not associated with branch status; historical PR browsing stays on explicit list/detail endpoints.
- An **open PR from any candidate repo always wins** over a closed/merged one, so a merged fork PR can never hide an open upstream PR for the same head.
- Only when no target has an open PR does it return the branch's newest closed/merged PR, as history.
- History is looked up **only for the ranked-first remote and the branch's own name** — the repo it actually pushes to. Live status is worth searching the whole fork network for; history is not, and asking every target for it multiplies serial GitHub calls until the route hits its `12s` resolve timeout and returns no status at all.
- The history answer is remembered per repo+branch so discovery polls do not re-query it: a found closed/merged record for `6h`, and "no history yet" for `10m`. A found record only changes if a second PR appears on the same head, and while that one is open the open-PR path wins without ever reading this cache.
- Creating, merging, or closing a PR invalidates both the shared repo pull list and that remembered history.
- The route skips the checks summary and the merge-permission lookup for a closed/merged PR: neither is actionable, and both cost extra GitHub calls.
- `403` and `404` during repo lookups are treated as expected gaps, not hard errors.
## Shared client state model
@@ -116,8 +121,8 @@
## Persistence notes for terminal PRs
- Closed/merged branch-status entries are not written to local storage.
- Legacy persisted terminal entries are stripped on hydrate (`pr: null`) and marked unresolved until the next refresh.
- Closed/merged branch associations are persisted like open ones, so a reload still shows that the branch's PR was merged.
- Hydrate resets `lastDiscoveryPollAt` for them, so restored history revalidates on the first watcher tick instead of waiting out a discovery interval.
## Background tracking rules
+136 -33
View File
@@ -332,6 +332,38 @@ const safeListPulls = async (octokit, options) => {
const REPO_PULLS_CACHE_TTL_MS = 45_000;
const repoPullsCache = new Map();
// Remembered answer to "what is the newest closed/merged PR for this head?",
// so discovery polls do not re-ask GitHub every few minutes.
//
// A found record barely ever changes: it would take a second PR on the same
// head, and while that one is open the open-PR path wins and never reads this
// cache at all. "No history yet" is the volatile answer, since closing or
// merging a PR elsewhere flips it, so it expires far sooner. Either way, doing
// it from OpenChamber invalidates the entry immediately.
const HISTORICAL_PR_FOUND_TTL_MS = 6 * 60 * 60 * 1000;
const HISTORICAL_PR_ABSENT_TTL_MS = 10 * 60 * 1000;
const HISTORICAL_PR_CACHE_MAX_ENTRIES = 500;
const _historicalPrCache = new Map();
const isHistoricalPrCacheFresh = (entry) => {
if (!entry) {
return false;
}
const ttl = entry.pr ? HISTORICAL_PR_FOUND_TTL_MS : HISTORICAL_PR_ABSENT_TTL_MS;
return Date.now() - entry.fetchedAt < ttl;
};
const rememberHistoricalPr = (key, pr) => {
_historicalPrCache.delete(key);
_historicalPrCache.set(key, { pr, fetchedAt: Date.now() });
if (_historicalPrCache.size > HISTORICAL_PR_CACHE_MAX_ENTRIES) {
const oldest = _historicalPrCache.keys().next().value;
if (oldest !== undefined) {
_historicalPrCache.delete(oldest);
}
}
};
export const invalidateRepoPullsCache = (owner, repo) => {
const prefix = `${normalizeText(owner)}/${normalizeText(repo)}::`;
for (const key of repoPullsCache.keys()) {
@@ -347,6 +379,13 @@ export const invalidateRepoPullsCache = (owner, repo) => {
_searchMissCache.delete(key);
}
}
// A merge or close changes the branch's PR history, so drop it too.
const historicalPrefix = `${normalizeRepoKey(owner, repo)}::`;
for (const key of _historicalPrCache.keys()) {
if (key.startsWith(historicalPrefix)) {
_historicalPrCache.delete(key);
}
}
};
const getRepoPulls = (octokit, repo, state, { force = false } = {}) => {
@@ -440,8 +479,8 @@ const searchFallbackPr = async ({ octokit, branch, repoNames }) => {
const normalizedRepoNames = new Set(repoNames.map((name) => normalizeLower(name)).filter(Boolean));
// Branch status only discovers open PRs. Closed/merged history belongs to
// explicit PR list/detail workflows, not automatic branch association.
// The Search API has a tiny quota, so it is only spent on live branch status.
// Closed/merged history is resolved by the cheaper per-head repo queries.
let response;
try {
response = await octokit.rest.search.issuesAndPullRequests({
@@ -502,7 +541,22 @@ const searchFallbackPr = async ({ octokit, branch, repoNames }) => {
return null;
};
const findFirstMatchingPr = async ({ octokit, target, branch, sourceCandidates, force = false, coverage = null }) => {
const isTerminalPr = (pr) => Boolean(pr) && (pr.state === 'closed' || Boolean(pr.merged_at));
/**
* Resolve the PRs a branch is associated with in one repo target.
*
* Returns both candidates because they answer different questions:
* `open` is live branch status, `historical` is the last closed/merged PR for
* the same head. The caller must prefer an open PR from ANY target over a
* historical one — otherwise a merged fork PR hides an open upstream PR.
*
* `includeHistory` is off by default and must stay that way for secondary
* targets. Live status is worth searching the whole fork network for; history
* is not, and doing it per target multiplied the serial GitHub calls until the
* route hit its resolve timeout and reported no status at all.
*/
const findBranchPrCandidates = async ({ octokit, target, branch, sourceCandidates, force = false, coverage = null, includeHistory = false }) => {
const matcher = buildSourceMatcher(sourceCandidates);
const sourceOwners = [];
sourceCandidates.forEach((candidate) => pushUnique(sourceOwners, candidate.repo?.owner));
@@ -512,46 +566,71 @@ const findFirstMatchingPr = async ({ octokit, target, branch, sourceCandidates,
.filter((pr) => matcher.matches(pr, target.repo.repo))
.sort((left, right) => matcher.compare(left, right, target.repo.repo))[0] ?? null;
// Branch status associates the current head with an open PR only. Returning a
// closed/merged PR here made the client cache a terminal status that could not
// self-heal until a manual forced refresh. A miss also lets the next repo
// target run, so an open upstream PR wins over a merged fork PR.
let listWasComplete = false;
// The shared repo-level open list answers every branch of the repo within the
// TTL. A miss in a complete list is authoritative: no open PR exists here.
let openListWasComplete = false;
try {
const listEntry = await getRepoPulls(octokit, target.repo, 'open', { force });
const fromList = pickPreferred(listEntry.prs);
if (fromList) {
return fromList;
return { open: fromList, historical: null };
}
listWasComplete = listEntry.complete;
openListWasComplete = listEntry.complete;
} catch {
// fall through to the precise per-branch queries
// fall through to the precise per-head queries
}
if (!listWasComplete) {
if (coverage) {
coverage.authoritative = false;
}
for (const owner of sourceOwners) {
const directCandidates = await safeListPulls(octokit, {
owner: target.repo.owner,
repo: target.repo.repo,
state: 'open',
head: `${owner}:${branch}`,
per_page: 100,
});
const direct = pickPreferred(directCandidates);
if (direct) {
return direct;
}
if (!openListWasComplete && coverage) {
coverage.authoritative = false;
}
// A complete open list already proved there is no open PR in this repo. With
// no history to look up there is nothing left to ask GitHub.
if (openListWasComplete && !includeHistory) {
return { open: null, historical: null };
}
const historicalKey = `${normalizeRepoKey(target.repo?.owner, target.repo?.repo)}::${branch}`;
if (includeHistory && !force && openListWasComplete) {
const cached = _historicalPrCache.get(historicalKey);
if (isHistoricalPrCacheFresh(cached)) {
return { open: null, historical: cached.pr };
}
}
return null;
// One query per source owner. With history enabled `state: 'all'` answers
// both questions at once, so asking for history never costs an extra call.
let historical = null;
for (const owner of sourceOwners) {
const directCandidates = await safeListPulls(octokit, {
owner: target.repo.owner,
repo: target.repo.repo,
state: includeHistory ? 'all' : 'open',
head: `${owner}:${branch}`,
per_page: 100,
});
const openMatch = pickPreferred(directCandidates.filter((pr) => !isTerminalPr(pr)));
if (openMatch) {
return { open: openMatch, historical: null };
}
if (includeHistory && !historical) {
// Among past PRs for the same head the newest one is the relevant record.
historical = directCandidates
.filter((pr) => normalizeText(pr?.head?.ref) === branch)
.filter((pr) => matcher.matches(pr, target.repo.repo))
.filter(isTerminalPr)
.sort((left, right) => (right?.number ?? 0) - (left?.number ?? 0))[0] ?? null;
}
}
if (includeHistory) {
rememberHistoricalPr(historicalKey, historical);
}
return { open: null, historical };
};
// Exported for focused unit tests of open-only branch matching.
export { findFirstMatchingPr };
// Exported for focused unit tests of open-versus-historical branch matching.
export { findBranchPrCandidates };
export async function resolveGitHubPrStatus({ octokit, directory, branch, remoteName, force = false }) {
// A deleted worktree can still have a session in the sidebar that keeps
@@ -604,6 +683,11 @@ export async function resolveGitHubPrStatus({ octokit, directory, branch, remote
let fallbackRemoteName = resolvedTargets[0].remoteName;
let fallbackDefaultBranch = await getRepoDefaultBranch(octokit, fallbackRepo);
// The first closed/merged PR found, in target priority order. It is only
// returned once every target has been checked for an open PR, so an open
// upstream PR always wins over a merged fork PR for the same head.
let historicalMatch = null;
for (const target of resolvedTargets) {
const defaultBranch = await getRepoDefaultBranch(octokit, target.repo);
if (!fallbackRepo) {
@@ -618,18 +702,33 @@ export async function resolveGitHubPrStatus({ octokit, directory, branch, remote
continue;
}
const pr = await findFirstMatchingPr({
// History is only asked of the branch's own repo and its own name: the
// ranked-first target is the remote this branch actually pushes to.
// Searching the rest of the fork network for history would multiply
// serial GitHub calls for no additional user-visible information.
const isPrimaryAssociation = target === resolvedTargets[0] && candidateBranch === branchCandidates[0];
const { open, historical } = await findBranchPrCandidates({
octokit,
target,
branch: candidateBranch,
sourceCandidates,
force,
coverage,
includeHistory: isPrimaryAssociation,
});
if (pr) {
if (open) {
return {
repo: target.repo,
pr,
pr: open,
defaultBranch,
resolvedRemoteName: target.remoteName,
};
}
if (historical && !historicalMatch) {
historicalMatch = {
repo: target.repo,
pr: historical,
defaultBranch,
resolvedRemoteName: target.remoteName,
};
@@ -656,6 +755,10 @@ export async function resolveGitHubPrStatus({ octokit, directory, branch, remote
}
}
if (historicalMatch) {
return historicalMatch;
}
return {
repo: fallbackRepo,
pr: null,
+123 -48
View File
@@ -1,4 +1,4 @@
import { beforeEach, describe, expect, mock, test } from 'bun:test';
import { afterEach, beforeEach, describe, expect, mock, setSystemTime, test } from 'bun:test';
const listMock = mock(async () => ({ data: [] }));
@@ -15,7 +15,7 @@ mock.module('./rate-limit.js', () => ({
noteIfGitHubRateLimit: () => {},
}));
const { findFirstMatchingPr, invalidateRepoPullsCache } = await import('./pr-status.js');
const { findBranchPrCandidates, invalidateRepoPullsCache } = await import('./pr-status.js');
const openPr = {
number: 15,
@@ -28,7 +28,7 @@ const openPr = {
},
};
const closedPr = {
const mergedPr = {
number: 12,
state: 'closed',
merged_at: '2026-01-01T00:00:00Z',
@@ -40,66 +40,141 @@ const closedPr = {
},
};
describe('findFirstMatchingPr open-only branch status', () => {
const olderMergedPr = {
...mergedPr,
number: 7,
merged_at: '2025-11-01T00:00:00Z',
};
const call = (overrides = {}) => findBranchPrCandidates({
octokit: { rest: { pulls: { list: listMock } } },
target: { repo: { owner: 'acme', repo: 'app' }, remoteName: 'origin' },
branch: 'feature',
sourceCandidates: [{ repo: { owner: 'acme', repo: 'app' } }],
force: true,
includeHistory: true,
...overrides,
});
describe('findBranchPrCandidates', () => {
beforeEach(() => {
listMock.mockReset();
invalidateRepoPullsCache('acme', 'app');
});
test('returns a matching open PR', async () => {
listMock.mockImplementation(async ({ state }) => {
if (state === 'open') {
return { data: [openPr] };
}
return { data: [closedPr] };
});
const pr = await findFirstMatchingPr({
octokit: { rest: { pulls: { list: listMock } } },
target: { repo: { owner: 'acme', repo: 'app' }, remoteName: 'origin' },
branch: 'feature',
sourceCandidates: [{ repo: { owner: 'acme', repo: 'app' } }],
force: true,
});
expect(pr?.number).toBe(15);
expect(listMock.mock.calls.every((call) => call[0]?.state === 'open')).toBe(true);
afterEach(() => {
setSystemTime();
});
test('returns null when only a closed/merged PR exists for the head branch', async () => {
listMock.mockImplementation(async ({ state }) => {
if (state === 'open') {
return { data: [] };
}
return { data: [closedPr] };
});
test('an open PR wins and no history lookup is spent', async () => {
listMock.mockImplementation(async ({ state }) => (
state === 'open' ? { data: [openPr] } : { data: [mergedPr] }
));
const pr = await findFirstMatchingPr({
octokit: { rest: { pulls: { list: listMock } } },
target: { repo: { owner: 'acme', repo: 'app' }, remoteName: 'origin' },
branch: 'feature',
sourceCandidates: [{ repo: { owner: 'acme', repo: 'app' } }],
force: true,
});
const { open, historical } = await call();
expect(pr).toBeNull();
expect(listMock.mock.calls.every((call) => call[0]?.state === 'open')).toBe(true);
expect(listMock.mock.calls.some((call) => call[0]?.state === 'closed')).toBe(false);
expect(open?.number).toBe(15);
expect(historical).toBeNull();
expect(listMock.mock.calls.every((entry) => entry[0]?.state === 'open')).toBe(true);
});
test('does not query closed PRs when the open list is complete and empty', async () => {
test('an open PR still wins when the shared open list missed it', async () => {
// A repo with more than one page of open PRs: the shared list is incomplete,
// so the per-head query is the one that must find the open PR.
listMock.mockImplementation(async ({ head }) => (
head ? { data: [mergedPr, openPr] } : { data: new Array(100).fill(null).map((_, index) => ({ number: index, state: 'open', head: { ref: 'other' } })) }
));
const { open, historical } = await call();
expect(open?.number).toBe(15);
expect(historical).toBeNull();
});
test('returns the branch history when no open PR exists', async () => {
listMock.mockImplementation(async ({ head }) => (
head ? { data: [olderMergedPr, mergedPr] } : { data: [] }
));
const { open, historical } = await call();
expect(open).toBeNull();
// The newest past PR for the head is the relevant record.
expect(historical?.number).toBe(12);
});
test('returns no history for a branch that never had a PR', async () => {
listMock.mockImplementation(async () => ({ data: [] }));
const pr = await findFirstMatchingPr({
octokit: { rest: { pulls: { list: listMock } } },
target: { repo: { owner: 'acme', repo: 'app' }, remoteName: 'origin' },
branch: 'feature',
sourceCandidates: [{ repo: { owner: 'acme', repo: 'app' } }],
force: true,
});
const { open, historical } = await call();
expect(pr).toBeNull();
expect(open).toBeNull();
expect(historical).toBeNull();
expect(listMock.mock.calls.some((entry) => entry[0]?.state === 'all')).toBe(true);
});
test('spends no call on history for a secondary target', async () => {
listMock.mockImplementation(async ({ head }) => (
head ? { data: [mergedPr] } : { data: [] }
));
const { open, historical } = await call({ includeHistory: false });
expect(open).toBeNull();
expect(historical).toBeNull();
// The complete open list already answered the only question that matters
// for a secondary repo in the fork network.
expect(listMock.mock.calls).toHaveLength(1);
expect(listMock.mock.calls[0]?.[0]?.state).toBe('open');
});
test('reuses the cached history instead of re-querying every poll', async () => {
listMock.mockImplementation(async ({ head }) => (
head ? { data: [mergedPr] } : { data: [] }
));
await call();
const callsAfterFirst = listMock.mock.calls.length;
// A non-forced poll is answered entirely from the shared open list cache
// plus the remembered history — no extra GitHub call.
const { open, historical } = await call({ force: false });
expect(open).toBeNull();
expect(historical?.number).toBe(12);
expect(listMock.mock.calls.length).toBe(callsAfterFirst);
});
test('a found record outlives the shorter "no history" window', async () => {
const startedAt = Date.now();
listMock.mockImplementation(async ({ head }) => (
head ? { data: [mergedPr] } : { data: [] }
));
await call();
const callsAfterFirst = listMock.mock.calls.length;
// Past the "no history" expiry, but far short of the found-record one. The
// shared open list is re-fetched; the history answer is not re-queried.
setSystemTime(new Date(startedAt + 30 * 60 * 1000));
const { historical } = await call({ force: false });
expect(historical?.number).toBe(12);
expect(listMock.mock.calls.length).toBe(callsAfterFirst + 1);
expect(listMock.mock.calls.at(-1)?.[0]?.state).toBe('open');
});
test('re-queries a branch with no history once its shorter window passes', async () => {
const startedAt = Date.now();
listMock.mockImplementation(async () => ({ data: [] }));
await call();
const callsAfterFirst = listMock.mock.calls.length;
setSystemTime(new Date(startedAt + 30 * 60 * 1000));
await call({ force: false });
expect(listMock.mock.calls.some((entry) => entry[0]?.state === 'all')).toBe(true);
expect(listMock.mock.calls.length).toBeGreaterThan(callsAfterFirst + 1);
});
});
+36 -30
View File
@@ -574,10 +574,17 @@ export function registerGitHubRoutes(app) {
return res.json({ connected: true, repo: searchRepo, branch, pr: null, checks: null, canMerge: false });
}
const isMerged = Boolean(prData.merged || prData.merged_at);
const prState = isMerged ? 'merged' : (prData.state === 'closed' ? 'closed' : 'open');
// A closed/merged PR is a historical record for this branch: its checks
// are no longer actionable and it can never be merged from here, so skip
// the extra GitHub calls those two fields would cost.
const isHistorical = prState !== 'open';
// Checks summary: prefer check-runs (Actions), fallback to classic statuses.
let checks = null;
const sha = prData.head?.sha;
if (sha) {
if (sha && !isHistorical) {
try {
const runs = await octokit.rest.checks.listForRef({
owner: searchRepo.owner,
@@ -610,38 +617,37 @@ export function registerGitHubRoutes(app) {
// Permission check (best-effort)
let canMerge = false;
try {
const auth = getGitHubAuth();
// gh-CLI tokens have no persisted user record; resolve the login from
// the API once (memoized) so permissions still resolve for them.
let username = auth?.user?.login;
if (!username) {
if (!resolvedAuthLoginPromise) {
resolvedAuthLoginPromise = octokit.rest.users.getAuthenticated()
.then((resp) => resp?.data?.login || null)
.catch(() => {
resolvedAuthLoginPromise = null;
return null;
});
if (!isHistorical) {
try {
const auth = getGitHubAuth();
// gh-CLI tokens have no persisted user record; resolve the login from
// the API once (memoized) so permissions still resolve for them.
let username = auth?.user?.login;
if (!username) {
if (!resolvedAuthLoginPromise) {
resolvedAuthLoginPromise = octokit.rest.users.getAuthenticated()
.then((resp) => resp?.data?.login || null)
.catch(() => {
resolvedAuthLoginPromise = null;
return null;
});
}
username = await resolvedAuthLoginPromise;
}
username = await resolvedAuthLoginPromise;
if (username) {
const perm = await octokit.rest.repos.getCollaboratorPermissionLevel({
owner: searchRepo.owner,
repo: searchRepo.repo,
username,
});
const level = perm?.data?.permission;
canMerge = level === 'admin' || level === 'maintain' || level === 'write';
}
} catch {
canMerge = false;
}
if (username) {
const perm = await octokit.rest.repos.getCollaboratorPermissionLevel({
owner: searchRepo.owner,
repo: searchRepo.repo,
username,
});
const level = perm?.data?.permission;
canMerge = level === 'admin' || level === 'maintain' || level === 'write';
}
} catch {
canMerge = false;
}
const isMerged = Boolean(prData.merged || prData.merged_at);
const mergedState = isMerged ? 'merged' : (prData.state === 'closed' ? 'closed' : 'open');
return res.json({
connected: true,
repo: searchRepo,
@@ -651,7 +657,7 @@ export function registerGitHubRoutes(app) {
title: prData.title,
body: prData.body || '',
url: prData.html_url,
state: mergedState,
state: prState,
draft: Boolean(prData.draft),
base: prData.base?.ref,
head: prData.head?.ref,